| Message ID | 20230120114313.2087015-1-john@metanate.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: panel: Set orientation on panel_bridge connector | expand |
Hi, On Fri, Jan 20, 2023 at 3:43 AM John Keeping <john@metanate.com> wrote: > > Commit 15b9ca1641f0 ("drm: Config orientation property if panel provides > it") added a helper to set the panel panel orientation early but only > connected this for drm_bridge_connector, which constructs a panel bridge > with DRM_BRIDGE_ATTACH_NO_CONNECTOR and creates the connector itself. > > When the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is not specified and the > panel_bridge creates its own connector the orientation is not set unless > the panel does it in .get_modes which is too late and leads to a warning > splat from __drm_mode_object_add() because the device is already > registered. > > Call the necessary function to set add the orientation property when the > connector is created so that it is available before the device is > registered. I have no huge objection to your patch and it looks OK to me. That being said, my understanding is that: 1. DRM_BRIDGE_ATTACH_NO_CONNECTOR is "the future" and not using the flag is "deprecated". 2. In general, if something about the deprecated method used to work and a patch broke it then we should fix it until we can finish fully deprecating. However, we should avoid adding new features to the old deprecated method and instead encourage people to move to "the future". 3. I don't think any of the orientation patches broke the deprecated path. Before those patches, nothing used to configure the orientation property properly. After those patches, "the future" method (AKA DRM_BRIDGE_ATTACH_NO_CONNECTOR) did configure the orientation property properly. ...so by those arguments I would say that we shouldn't land your patch and that instead you should work to get your drivers moving to DRM_BRIDGE_ATTACH_NO_CONNECTOR. Now, all that being said, your patch adds one line of code and really doesn't seem like a big deal. I'd personally be OK with landing it, but I'd prefer to hear an opinion from someone more senior in the DRM world before doing so. I'm still fairly low on the totem pole. ;-) > Fixes: 15b9ca1641f0 ("drm: Config orientation property if panel provides it") Maybe remove the "Fixes" tag here. That patch didn't break you, right? It just didn't happen to _also_ fix you.
Hi John/Douglas. On Fri, Jan 20, 2023 at 01:44:38PM -0800, Doug Anderson wrote: > Hi, > > On Fri, Jan 20, 2023 at 3:43 AM John Keeping <john@metanate.com> wrote: > > > > Commit 15b9ca1641f0 ("drm: Config orientation property if panel provides > > it") added a helper to set the panel panel orientation early but only > > connected this for drm_bridge_connector, which constructs a panel bridge > > with DRM_BRIDGE_ATTACH_NO_CONNECTOR and creates the connector itself. > > > > When the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is not specified and the > > panel_bridge creates its own connector the orientation is not set unless > > the panel does it in .get_modes which is too late and leads to a warning > > splat from __drm_mode_object_add() because the device is already > > registered. > > > > Call the necessary function to set add the orientation property when the > > connector is created so that it is available before the device is > > registered. > > I have no huge objection to your patch and it looks OK to me. That > being said, my understanding is that: > > 1. DRM_BRIDGE_ATTACH_NO_CONNECTOR is "the future" and not using the > flag is "deprecated". Correct. Could we take a look at how much is required to move the relevant driver to use DRM_BRIDGE_ATTACH_NO_CONNECTOR? If this is too much work now we may land this simple patch, but the preference is to move all drivers to the new bridge handling and thus asking display drivers to create the connector. What display driver are we dealing with here? Sam
Hi Sam & Doug, On Sat, Jan 21, 2023 at 09:57:18AM +0100, Sam Ravnborg wrote: > On Fri, Jan 20, 2023 at 01:44:38PM -0800, Doug Anderson wrote: > > On Fri, Jan 20, 2023 at 3:43 AM John Keeping <john@metanate.com> wrote: > > > > > > Commit 15b9ca1641f0 ("drm: Config orientation property if panel provides > > > it") added a helper to set the panel panel orientation early but only > > > connected this for drm_bridge_connector, which constructs a panel bridge > > > with DRM_BRIDGE_ATTACH_NO_CONNECTOR and creates the connector itself. > > > > > > When the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is not specified and the > > > panel_bridge creates its own connector the orientation is not set unless > > > the panel does it in .get_modes which is too late and leads to a warning > > > splat from __drm_mode_object_add() because the device is already > > > registered. > > > > > > Call the necessary function to set add the orientation property when the > > > connector is created so that it is available before the device is > > > registered. > > > > I have no huge objection to your patch and it looks OK to me. That > > being said, my understanding is that: > > > > 1. DRM_BRIDGE_ATTACH_NO_CONNECTOR is "the future" and not using the > > flag is "deprecated". > Correct. > Could we take a look at how much is required to move the relevant driver > to use DRM_BRIDGE_ATTACH_NO_CONNECTOR? > > If this is too much work now we may land this simple patch, but the > preference is to move all drivers to the new bridge handling and thus > asking display drivers to create the connector. > > What display driver are we dealing with here? This is dw-mipi-dsi-rockchip which uses the component path in dw-mipi-dsi (and, in fact, is the only driver using that mode of dw-mipi-dsi). I'm not familiar enough with DRM to say whether it's easy to convert to DRM_BRIDGE_ATTACH_NO_CONNECTOR - should dw-mipi-dsi-rockchip be moving to use dw-mipi-dsi as a bridge driver or should dw_mipi_dsi_bind() have a drm_bridge_attach_flags argument? But I'm happy to test patches if it looks easy to convert to you :-) John
Hello, On Sat, Jan 21, 2023 at 05:58:11PM +0000, John Keeping wrote: > On Sat, Jan 21, 2023 at 09:57:18AM +0100, Sam Ravnborg wrote: > > On Fri, Jan 20, 2023 at 01:44:38PM -0800, Doug Anderson wrote: > > > On Fri, Jan 20, 2023 at 3:43 AM John Keeping wrote: > > > > > > > > Commit 15b9ca1641f0 ("drm: Config orientation property if panel provides > > > > it") added a helper to set the panel panel orientation early but only > > > > connected this for drm_bridge_connector, which constructs a panel bridge > > > > with DRM_BRIDGE_ATTACH_NO_CONNECTOR and creates the connector itself. > > > > > > > > When the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is not specified and the > > > > panel_bridge creates its own connector the orientation is not set unless > > > > the panel does it in .get_modes which is too late and leads to a warning > > > > splat from __drm_mode_object_add() because the device is already > > > > registered. > > > > > > > > Call the necessary function to set add the orientation property when the > > > > connector is created so that it is available before the device is > > > > registered. > > > > > > I have no huge objection to your patch and it looks OK to me. That > > > being said, my understanding is that: > > > > > > 1. DRM_BRIDGE_ATTACH_NO_CONNECTOR is "the future" and not using the > > > flag is "deprecated". > > > > Correct. > > Could we take a look at how much is required to move the relevant driver > > to use DRM_BRIDGE_ATTACH_NO_CONNECTOR? > > > > If this is too much work now we may land this simple patch, but the > > preference is to move all drivers to the new bridge handling and thus > > asking display drivers to create the connector. I fully agree with Doug and Sam here. Let's see if we can keep the yak shaving minimal :-) > > What display driver are we dealing with here? > > This is dw-mipi-dsi-rockchip which uses the component path in > dw-mipi-dsi (and, in fact, is the only driver using that mode of > dw-mipi-dsi). > > I'm not familiar enough with DRM to say whether it's easy to convert to > DRM_BRIDGE_ATTACH_NO_CONNECTOR - should dw-mipi-dsi-rockchip be moving > to use dw-mipi-dsi as a bridge driver or should dw_mipi_dsi_bind() have > a drm_bridge_attach_flags argument? But I'm happy to test patches if it > looks easy to convert to you :-) I'd go for the former (use dw_mipi_dsi_probe() and acquire the DSI bridge with of_drm_find_bridge() instead of using the component framework) if possible, but I don't know how intrusive that would be.
Hi Laurent, On Sun, Jan 22, 2023 at 05:01:27PM +0200, Laurent Pinchart wrote: > On Sat, Jan 21, 2023 at 05:58:11PM +0000, John Keeping wrote: > > On Sat, Jan 21, 2023 at 09:57:18AM +0100, Sam Ravnborg wrote: > > > On Fri, Jan 20, 2023 at 01:44:38PM -0800, Doug Anderson wrote: > > > > On Fri, Jan 20, 2023 at 3:43 AM John Keeping wrote: > > > > > > > > > > Commit 15b9ca1641f0 ("drm: Config orientation property if panel provides > > > > > it") added a helper to set the panel panel orientation early but only > > > > > connected this for drm_bridge_connector, which constructs a panel bridge > > > > > with DRM_BRIDGE_ATTACH_NO_CONNECTOR and creates the connector itself. > > > > > > > > > > When the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is not specified and the > > > > > panel_bridge creates its own connector the orientation is not set unless > > > > > the panel does it in .get_modes which is too late and leads to a warning > > > > > splat from __drm_mode_object_add() because the device is already > > > > > registered. > > > > > > > > > > Call the necessary function to set add the orientation property when the > > > > > connector is created so that it is available before the device is > > > > > registered. > > > > > > > > I have no huge objection to your patch and it looks OK to me. That > > > > being said, my understanding is that: > > > > > > > > 1. DRM_BRIDGE_ATTACH_NO_CONNECTOR is "the future" and not using the > > > > flag is "deprecated". > > > > > > Correct. > > > Could we take a look at how much is required to move the relevant driver > > > to use DRM_BRIDGE_ATTACH_NO_CONNECTOR? > > > > > > If this is too much work now we may land this simple patch, but the > > > preference is to move all drivers to the new bridge handling and thus > > > asking display drivers to create the connector. > > I fully agree with Doug and Sam here. Let's see if we can keep the yak > shaving minimal :-) > > > > What display driver are we dealing with here? > > > > This is dw-mipi-dsi-rockchip which uses the component path in > > dw-mipi-dsi (and, in fact, is the only driver using that mode of > > dw-mipi-dsi). > > > > I'm not familiar enough with DRM to say whether it's easy to convert to > > DRM_BRIDGE_ATTACH_NO_CONNECTOR - should dw-mipi-dsi-rockchip be moving > > to use dw-mipi-dsi as a bridge driver or should dw_mipi_dsi_bind() have > > a drm_bridge_attach_flags argument? But I'm happy to test patches if it > > looks easy to convert to you :-) > > I'd go for the former (use dw_mipi_dsi_probe() and acquire the DSI > bridge with of_drm_find_bridge() instead of using the component > framework) if possible, but I don't know how intrusive that would be. I'm a bit confused about what's required since dw-mipi-dsi-rockchip already uses dw_mipi_dsi_probe(), but I think moving away from the component framework would be significant work as that's how the MIPI subdriver fits in to the overall Rockchip display driver. Any changes / modernisation to the Rockchip MIPI driver look like it will take more time than I have available to spend on this, so I'd really like to see this patch land as it's a simple fix to an existing working code path. John
Hi John, On Mon, Jan 23, 2023 at 12:16:45PM +0000, John Keeping wrote: > On Sun, Jan 22, 2023 at 05:01:27PM +0200, Laurent Pinchart wrote: > > On Sat, Jan 21, 2023 at 05:58:11PM +0000, John Keeping wrote: > > > On Sat, Jan 21, 2023 at 09:57:18AM +0100, Sam Ravnborg wrote: > > > > On Fri, Jan 20, 2023 at 01:44:38PM -0800, Doug Anderson wrote: > > > > > On Fri, Jan 20, 2023 at 3:43 AM John Keeping wrote: > > > > > > > > > > > > Commit 15b9ca1641f0 ("drm: Config orientation property if panel provides > > > > > > it") added a helper to set the panel panel orientation early but only > > > > > > connected this for drm_bridge_connector, which constructs a panel bridge > > > > > > with DRM_BRIDGE_ATTACH_NO_CONNECTOR and creates the connector itself. > > > > > > > > > > > > When the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is not specified and the > > > > > > panel_bridge creates its own connector the orientation is not set unless > > > > > > the panel does it in .get_modes which is too late and leads to a warning > > > > > > splat from __drm_mode_object_add() because the device is already > > > > > > registered. > > > > > > > > > > > > Call the necessary function to set add the orientation property when the > > > > > > connector is created so that it is available before the device is > > > > > > registered. > > > > > > > > > > I have no huge objection to your patch and it looks OK to me. That > > > > > being said, my understanding is that: > > > > > > > > > > 1. DRM_BRIDGE_ATTACH_NO_CONNECTOR is "the future" and not using the > > > > > flag is "deprecated". > > > > > > > > Correct. > > > > Could we take a look at how much is required to move the relevant driver > > > > to use DRM_BRIDGE_ATTACH_NO_CONNECTOR? > > > > > > > > If this is too much work now we may land this simple patch, but the > > > > preference is to move all drivers to the new bridge handling and thus > > > > asking display drivers to create the connector. > > > > I fully agree with Doug and Sam here. Let's see if we can keep the yak > > shaving minimal :-) > > > > > > What display driver are we dealing with here? > > > > > > This is dw-mipi-dsi-rockchip which uses the component path in > > > dw-mipi-dsi (and, in fact, is the only driver using that mode of > > > dw-mipi-dsi). > > > > > > I'm not familiar enough with DRM to say whether it's easy to convert to > > > DRM_BRIDGE_ATTACH_NO_CONNECTOR - should dw-mipi-dsi-rockchip be moving > > > to use dw-mipi-dsi as a bridge driver or should dw_mipi_dsi_bind() have > > > a drm_bridge_attach_flags argument? But I'm happy to test patches if it > > > looks easy to convert to you :-) > > > > I'd go for the former (use dw_mipi_dsi_probe() and acquire the DSI > > bridge with of_drm_find_bridge() instead of using the component > > framework) if possible, but I don't know how intrusive that would be. > > I'm a bit confused about what's required since dw-mipi-dsi-rockchip > already uses dw_mipi_dsi_probe(), Indeed, my bad. > but I think moving away from the > component framework would be significant work as that's how the MIPI > subdriver fits in to the overall Rockchip display driver. It will be some work, yes. It however doesn't mean that the whole Rockchip display driver needs to move away from the component framework, it can be limited to the DSI encoder. It's not immediately clear to me why the DSI encoder uses the component framework in the first place, and if it would be difficult to move away from it. > Any changes / modernisation to the Rockchip MIPI driver look like it > will take more time than I have available to spend on this, so I'd > really like to see this patch land as it's a simple fix to an existing > working code path. So who volunteers for fixing it properly ? :-) I'll let Doug and Sam decide regarding mering this patch.
Hi, On Mon, Jan 23, 2023 at 8:05 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi John, > > On Mon, Jan 23, 2023 at 12:16:45PM +0000, John Keeping wrote: > > On Sun, Jan 22, 2023 at 05:01:27PM +0200, Laurent Pinchart wrote: > > > On Sat, Jan 21, 2023 at 05:58:11PM +0000, John Keeping wrote: > > > > On Sat, Jan 21, 2023 at 09:57:18AM +0100, Sam Ravnborg wrote: > > > > > On Fri, Jan 20, 2023 at 01:44:38PM -0800, Doug Anderson wrote: > > > > > > On Fri, Jan 20, 2023 at 3:43 AM John Keeping wrote: > > > > > > > > > > > > > > Commit 15b9ca1641f0 ("drm: Config orientation property if panel provides > > > > > > > it") added a helper to set the panel panel orientation early but only > > > > > > > connected this for drm_bridge_connector, which constructs a panel bridge > > > > > > > with DRM_BRIDGE_ATTACH_NO_CONNECTOR and creates the connector itself. > > > > > > > > > > > > > > When the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is not specified and the > > > > > > > panel_bridge creates its own connector the orientation is not set unless > > > > > > > the panel does it in .get_modes which is too late and leads to a warning > > > > > > > splat from __drm_mode_object_add() because the device is already > > > > > > > registered. > > > > > > > > > > > > > > Call the necessary function to set add the orientation property when the > > > > > > > connector is created so that it is available before the device is > > > > > > > registered. > > > > > > > > > > > > I have no huge objection to your patch and it looks OK to me. That > > > > > > being said, my understanding is that: > > > > > > > > > > > > 1. DRM_BRIDGE_ATTACH_NO_CONNECTOR is "the future" and not using the > > > > > > flag is "deprecated". > > > > > > > > > > Correct. > > > > > Could we take a look at how much is required to move the relevant driver > > > > > to use DRM_BRIDGE_ATTACH_NO_CONNECTOR? > > > > > > > > > > If this is too much work now we may land this simple patch, but the > > > > > preference is to move all drivers to the new bridge handling and thus > > > > > asking display drivers to create the connector. > > > > > > I fully agree with Doug and Sam here. Let's see if we can keep the yak > > > shaving minimal :-) > > > > > > > > What display driver are we dealing with here? > > > > > > > > This is dw-mipi-dsi-rockchip which uses the component path in > > > > dw-mipi-dsi (and, in fact, is the only driver using that mode of > > > > dw-mipi-dsi). > > > > > > > > I'm not familiar enough with DRM to say whether it's easy to convert to > > > > DRM_BRIDGE_ATTACH_NO_CONNECTOR - should dw-mipi-dsi-rockchip be moving > > > > to use dw-mipi-dsi as a bridge driver or should dw_mipi_dsi_bind() have > > > > a drm_bridge_attach_flags argument? But I'm happy to test patches if it > > > > looks easy to convert to you :-) > > > > > > I'd go for the former (use dw_mipi_dsi_probe() and acquire the DSI > > > bridge with of_drm_find_bridge() instead of using the component > > > framework) if possible, but I don't know how intrusive that would be. > > > > I'm a bit confused about what's required since dw-mipi-dsi-rockchip > > already uses dw_mipi_dsi_probe(), > > Indeed, my bad. > > > but I think moving away from the > > component framework would be significant work as that's how the MIPI > > subdriver fits in to the overall Rockchip display driver. > > It will be some work, yes. It however doesn't mean that the whole > Rockchip display driver needs to move away from the component framework, > it can be limited to the DSI encoder. It's not immediately clear to me > why the DSI encoder uses the component framework in the first place, and > if it would be difficult to move away from it. > > > Any changes / modernisation to the Rockchip MIPI driver look like it > > will take more time than I have available to spend on this, so I'd > > really like to see this patch land as it's a simple fix to an existing > > working code path. > > So who volunteers for fixing it properly ? :-) > > I'll let Doug and Sam decide regarding mering this patch. This thread seems to have gone silent. I'm inclined to merge this patch (removing the "Fixes" tag) since it's a one-line fix. While we want to encourage people to move to "the future", it seems like it would be better to wait until someone is trying to land something more intrusive than a 1-line fix before truly forcing the issue. I'll plan to merge the patch to drm-misc-next early next week assuming there are no objections. -Doug
On 03/02/2023 01:45, Doug Anderson wrote: > Hi, > > On Mon, Jan 23, 2023 at 8:05 AM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: >> >> Hi John, >> >> On Mon, Jan 23, 2023 at 12:16:45PM +0000, John Keeping wrote: >>> On Sun, Jan 22, 2023 at 05:01:27PM +0200, Laurent Pinchart wrote: >>>> On Sat, Jan 21, 2023 at 05:58:11PM +0000, John Keeping wrote: >>>>> On Sat, Jan 21, 2023 at 09:57:18AM +0100, Sam Ravnborg wrote: >>>>>> On Fri, Jan 20, 2023 at 01:44:38PM -0800, Doug Anderson wrote: >>>>>>> On Fri, Jan 20, 2023 at 3:43 AM John Keeping wrote: >>>>>>>> >>>>>>>> Commit 15b9ca1641f0 ("drm: Config orientation property if panel provides >>>>>>>> it") added a helper to set the panel panel orientation early but only >>>>>>>> connected this for drm_bridge_connector, which constructs a panel bridge >>>>>>>> with DRM_BRIDGE_ATTACH_NO_CONNECTOR and creates the connector itself. >>>>>>>> >>>>>>>> When the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is not specified and the >>>>>>>> panel_bridge creates its own connector the orientation is not set unless >>>>>>>> the panel does it in .get_modes which is too late and leads to a warning >>>>>>>> splat from __drm_mode_object_add() because the device is already >>>>>>>> registered. >>>>>>>> >>>>>>>> Call the necessary function to set add the orientation property when the >>>>>>>> connector is created so that it is available before the device is >>>>>>>> registered. >>>>>>> >>>>>>> I have no huge objection to your patch and it looks OK to me. That >>>>>>> being said, my understanding is that: >>>>>>> >>>>>>> 1. DRM_BRIDGE_ATTACH_NO_CONNECTOR is "the future" and not using the >>>>>>> flag is "deprecated". >>>>>> >>>>>> Correct. >>>>>> Could we take a look at how much is required to move the relevant driver >>>>>> to use DRM_BRIDGE_ATTACH_NO_CONNECTOR? >>>>>> >>>>>> If this is too much work now we may land this simple patch, but the >>>>>> preference is to move all drivers to the new bridge handling and thus >>>>>> asking display drivers to create the connector. >>>> >>>> I fully agree with Doug and Sam here. Let's see if we can keep the yak >>>> shaving minimal :-) >>>> >>>>>> What display driver are we dealing with here? >>>>> >>>>> This is dw-mipi-dsi-rockchip which uses the component path in >>>>> dw-mipi-dsi (and, in fact, is the only driver using that mode of >>>>> dw-mipi-dsi). >>>>> >>>>> I'm not familiar enough with DRM to say whether it's easy to convert to >>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR - should dw-mipi-dsi-rockchip be moving >>>>> to use dw-mipi-dsi as a bridge driver or should dw_mipi_dsi_bind() have >>>>> a drm_bridge_attach_flags argument? But I'm happy to test patches if it >>>>> looks easy to convert to you :-) >>>> >>>> I'd go for the former (use dw_mipi_dsi_probe() and acquire the DSI >>>> bridge with of_drm_find_bridge() instead of using the component >>>> framework) if possible, but I don't know how intrusive that would be. >>> >>> I'm a bit confused about what's required since dw-mipi-dsi-rockchip >>> already uses dw_mipi_dsi_probe(), >> >> Indeed, my bad. >> >>> but I think moving away from the >>> component framework would be significant work as that's how the MIPI >>> subdriver fits in to the overall Rockchip display driver. >> >> It will be some work, yes. It however doesn't mean that the whole >> Rockchip display driver needs to move away from the component framework, >> it can be limited to the DSI encoder. It's not immediately clear to me >> why the DSI encoder uses the component framework in the first place, and >> if it would be difficult to move away from it. >> >>> Any changes / modernisation to the Rockchip MIPI driver look like it >>> will take more time than I have available to spend on this, so I'd >>> really like to see this patch land as it's a simple fix to an existing >>> working code path. >> >> So who volunteers for fixing it properly ? :-) >> >> I'll let Doug and Sam decide regarding mering this patch. > > This thread seems to have gone silent. > > I'm inclined to merge this patch (removing the "Fixes" tag) since it's > a one-line fix. While we want to encourage people to move to "the > future", it seems like it would be better to wait until someone is > trying to land something more intrusive than a 1-line fix before truly > forcing the issue. > > I'll plan to merge the patch to drm-misc-next early next week assuming > there are no objections. I'm ok for that, Neil > > -Doug
Hi, On Thu, Feb 2, 2023 at 4:45 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Mon, Jan 23, 2023 at 8:05 AM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi John, > > > > On Mon, Jan 23, 2023 at 12:16:45PM +0000, John Keeping wrote: > > > On Sun, Jan 22, 2023 at 05:01:27PM +0200, Laurent Pinchart wrote: > > > > On Sat, Jan 21, 2023 at 05:58:11PM +0000, John Keeping wrote: > > > > > On Sat, Jan 21, 2023 at 09:57:18AM +0100, Sam Ravnborg wrote: > > > > > > On Fri, Jan 20, 2023 at 01:44:38PM -0800, Doug Anderson wrote: > > > > > > > On Fri, Jan 20, 2023 at 3:43 AM John Keeping wrote: > > > > > > > > > > > > > > > > Commit 15b9ca1641f0 ("drm: Config orientation property if panel provides > > > > > > > > it") added a helper to set the panel panel orientation early but only > > > > > > > > connected this for drm_bridge_connector, which constructs a panel bridge > > > > > > > > with DRM_BRIDGE_ATTACH_NO_CONNECTOR and creates the connector itself. > > > > > > > > > > > > > > > > When the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is not specified and the > > > > > > > > panel_bridge creates its own connector the orientation is not set unless > > > > > > > > the panel does it in .get_modes which is too late and leads to a warning > > > > > > > > splat from __drm_mode_object_add() because the device is already > > > > > > > > registered. > > > > > > > > > > > > > > > > Call the necessary function to set add the orientation property when the > > > > > > > > connector is created so that it is available before the device is > > > > > > > > registered. > > > > > > > > > > > > > > I have no huge objection to your patch and it looks OK to me. That > > > > > > > being said, my understanding is that: > > > > > > > > > > > > > > 1. DRM_BRIDGE_ATTACH_NO_CONNECTOR is "the future" and not using the > > > > > > > flag is "deprecated". > > > > > > > > > > > > Correct. > > > > > > Could we take a look at how much is required to move the relevant driver > > > > > > to use DRM_BRIDGE_ATTACH_NO_CONNECTOR? > > > > > > > > > > > > If this is too much work now we may land this simple patch, but the > > > > > > preference is to move all drivers to the new bridge handling and thus > > > > > > asking display drivers to create the connector. > > > > > > > > I fully agree with Doug and Sam here. Let's see if we can keep the yak > > > > shaving minimal :-) > > > > > > > > > > What display driver are we dealing with here? > > > > > > > > > > This is dw-mipi-dsi-rockchip which uses the component path in > > > > > dw-mipi-dsi (and, in fact, is the only driver using that mode of > > > > > dw-mipi-dsi). > > > > > > > > > > I'm not familiar enough with DRM to say whether it's easy to convert to > > > > > DRM_BRIDGE_ATTACH_NO_CONNECTOR - should dw-mipi-dsi-rockchip be moving > > > > > to use dw-mipi-dsi as a bridge driver or should dw_mipi_dsi_bind() have > > > > > a drm_bridge_attach_flags argument? But I'm happy to test patches if it > > > > > looks easy to convert to you :-) > > > > > > > > I'd go for the former (use dw_mipi_dsi_probe() and acquire the DSI > > > > bridge with of_drm_find_bridge() instead of using the component > > > > framework) if possible, but I don't know how intrusive that would be. > > > > > > I'm a bit confused about what's required since dw-mipi-dsi-rockchip > > > already uses dw_mipi_dsi_probe(), > > > > Indeed, my bad. > > > > > but I think moving away from the > > > component framework would be significant work as that's how the MIPI > > > subdriver fits in to the overall Rockchip display driver. > > > > It will be some work, yes. It however doesn't mean that the whole > > Rockchip display driver needs to move away from the component framework, > > it can be limited to the DSI encoder. It's not immediately clear to me > > why the DSI encoder uses the component framework in the first place, and > > if it would be difficult to move away from it. > > > > > Any changes / modernisation to the Rockchip MIPI driver look like it > > > will take more time than I have available to spend on this, so I'd > > > really like to see this patch land as it's a simple fix to an existing > > > working code path. > > > > So who volunteers for fixing it properly ? :-) > > > > I'll let Doug and Sam decide regarding mering this patch. > > This thread seems to have gone silent. > > I'm inclined to merge this patch (removing the "Fixes" tag) since it's > a one-line fix. While we want to encourage people to move to "the > future", it seems like it would be better to wait until someone is > trying to land something more intrusive than a 1-line fix before truly > forcing the issue. > > I'll plan to merge the patch to drm-misc-next early next week assuming > there are no objections. Pushed to drm-misc-next after removing the "Fixes" tag and also fixing this warning: > -:7: WARNING:REPEATED_WORD: Possible repeated word: 'panel' > #7: > it") added a helper to set the panel panel orientation early but only e3ea1806e4ad drm/bridge: panel: Set orientation on panel_bridge connector -Doug
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index e8aae3cdc73d..d4b112911a99 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -81,6 +81,8 @@ static int panel_bridge_attach(struct drm_bridge *bridge, return ret; } + drm_panel_bridge_set_orientation(connector, bridge); + drm_connector_attach_encoder(&panel_bridge->connector, bridge->encoder);
Commit 15b9ca1641f0 ("drm: Config orientation property if panel provides it") added a helper to set the panel panel orientation early but only connected this for drm_bridge_connector, which constructs a panel bridge with DRM_BRIDGE_ATTACH_NO_CONNECTOR and creates the connector itself. When the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is not specified and the panel_bridge creates its own connector the orientation is not set unless the panel does it in .get_modes which is too late and leads to a warning splat from __drm_mode_object_add() because the device is already registered. Call the necessary function to set add the orientation property when the connector is created so that it is available before the device is registered. Fixes: 15b9ca1641f0 ("drm: Config orientation property if panel provides it") Signed-off-by: John Keeping <john@metanate.com> --- drivers/gpu/drm/bridge/panel.c | 2 ++ 1 file changed, 2 insertions(+)