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 <[email protected]>
---
drivers/gpu/drm/bridge/panel.c | 2 ++
1 file changed, 2 insertions(+)
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);
--
2.39.1
Hi,
On Fri, Jan 20, 2023 at 3:43 AM John Keeping <[email protected]> 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 <[email protected]> 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 <[email protected]> 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.
--
Regards,
Laurent Pinchart
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.
--
Regards,
Laurent Pinchart
Hi,
On Mon, Jan 23, 2023 at 8:05 AM Laurent Pinchart
<[email protected]> 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
> <[email protected]> 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 <[email protected]> wrote:
>
> Hi,
>
> On Mon, Jan 23, 2023 at 8:05 AM Laurent Pinchart
> <[email protected]> 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