Re: [PATCH] drm/bridge: panel: Set orientation on panel_bridge connector

[Neil Armstrong]

On 03/02/2023 01:45, Doug Anderson wrote:
> Hi,
>
> On Mon, Jan 23, 2023 at 8:05 AM Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx> 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