Re: [PATCH] drm/vc4: dsi: Only register our component once a DSI device is attached

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jagan,

On Mon, Jun 21, 2021 at 07:41:07PM +0530, Jagan Teki wrote:
> On Mon, Jun 21, 2021 at 6:26 PM Laurent Pinchart wrote:
> > On Mon, Jun 21, 2021 at 12:49:14PM +0100, Dave Stevenson wrote:
> > > On Sun, 20 Jun 2021 at 23:49, Laurent Pinchart wrote:
> > > > On Sun, Jun 20, 2021 at 09:42:27PM +0300, Laurent Pinchart wrote:
> > > > > On Sun, Jun 20, 2021 at 03:29:03PM +0100, Dave Stevenson wrote:
> > > > > > On Sun, 20 Jun 2021 at 04:26, Laurent Pinchart wrote:
> > > > > > >
> > > > > > > Hi Maxime,
> > > > > > >
> > > > > > > I'm testing this, and I'm afraid it causes an issue with all the
> > > > > > > I2C-controlled bridges. I'm focussing on the newly merged ti-sn65dsi83
> > > > > > > driver at the moment, but other are affected the same way.
> > > > > > >
> > > > > > > With this patch, the DSI component is only added when the DSI device is
> > > > > > > attached to the host with mipi_dsi_attach(). In the ti-sn65dsi83 driver,
> > > > > > > this happens in the bridge attach callback, which is called when the
> > > > > > > bridge is attached by a call to drm_bridge_attach() in vc4_dsi_bind().
> > > > > > > This creates a circular dependency, and the DRM/KMS device is never
> > > > > > > created.
> > > > > > >
> > > > > > > How should this be solved ? Dave, I think you have shown an interest in
> > > > > > > the sn65dsi83 recently, any help would be appreciated. On a side note,
> > > > > > > I've tested the ti-sn65dsi83 driver on a v5.10 RPi kernel, without much
> > > > > > > success (on top of commit e1499baa0b0c I get a very weird frame rate -
> > > > > > > 147 fps of 99 fps instead of 60 fps - and nothing on the screen, and on
> > > > > > > top of the latest v5.10 RPi branch, I get lock-related warnings at every
> > > > > > > page flip), which is why I tried v5.12 and noticed this patch. Is it
> > > > > > > worth trying to bring up the display on the v5.10 RPi kernel in parallel
> > > > > > > to fixing the issue introduced in this patch, or is DSI known to be
> > > > > > > broken there ?
> > > > > >
> > > > > > I've been looking at SN65DSI83/4, but as I don't have any hardware
> > > > > > I've largely been suggesting things to try to those on the forums who
> > > > > > do [1].
> > > > > >
> > > > > > My branch at https://github.com/6by9/linux/tree/rpi-5.10.y-sn65dsi8x-marek
> > > > > > is the latest one I've worked on. It's rpi-5.10.y with Marek's driver
> > > > > > cherry-picked, and an overlay and simple-panel definition by others.
> > > > > > It also has a rework for vc4_dsi to use pm_runtime, instead of
> > > > > > breaking up the DSI bridge chain (which is flawed as it never calls
> > > > > > the bridge mode_set or mode_valid functions which sn65dsi83 relies
> > > > > > on).
> > > > > >
> > > > > > I ran it on Friday in the lab and encountered an issue with vc4_dsi
> > > > > > should vc4_dsi_encoder_mode_fixup wish for a divider of 7 (required
> > > > > > for this 800x1280 panel over 4 lanes) where it resulted in an invalid
> > > > > > mode configuration. That resulted in patch [2] which then gave me
> > > > > > sensible numbers.
> > > > > >
> > > > > > That branch with dtoverlay=vc4-kms-v3d and
> > > > > > dtoverlay=vc4-kms-dsi-ti-sn65dsi83 created all the expected devices,
> > > > > > and everything came up normally.
> > > > > > It was a busy day, but I think I even stuck a scope on the clock lanes
> > > > > > at that point and confirmed that they were at the link frequency
> > > > > > expected.
> > > > >
> > > > > Thanks, I'll test your branch and will report the results.
> > > >
> > > > I had to apply the following diff to work around a crash:
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > > > index 55b6c53207f5..647426aa793a 100644
> > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > > > @@ -525,6 +525,9 @@ static bool sn65dsi83_mode_fixup(struct drm_bridge *bridge,
> > > >
> > > >         /* The DSI format is always RGB888_1X24 */
> > > >         list_for_each_entry(connector, &ddev->mode_config.connector_list, head) {
> > > > +               if (!connector->display_info.bus_formats)
> > > > +                       continue;
> > > > +
> > > >                 switch (connector->display_info.bus_formats[0]) {
> > > >                 case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
> > > >                         ctx->lvds_format_24bpp = false;
> > > >
> > > > connector->display_info.bus_formats is NULL for the HDMI connectors, as
> > > > I have nothing connected to them, as well as for the writeback
> > > > connector.
> > >
> > > I'm now confused as to what I'm doing as my branch appears NOT to have
> > > Marek's latest version of the driver as it doesn't have
> > > sn65dsi83_mode_fixup.
> > > I need to have another look at what's going on - I think I've got
> > > branches confused when switching between machines :-( Remaking that
> > > branch now.
> > >
> > > I do see that Marek has sent another patch around
> > > sn65dsi83_mode_fixup, but it'll still dereference
> > > connector->display_info.bus_formats[0] on all connectors. Shouldn't it
> > > only be switching on the one connector that is connected to this
> > > bridge, not HDMI or writeback connectors? I'm not totally clear on
> > > which connectors are in that list.
> > > https://patchwork.freedesktop.org/patch/440175/
> >
> > The following series should fix the issue:
> >
> > [PATCH] drm/bridge: ti-sn65dsi83: Replace connector format patching with atomic_get_input_bus_fmts
> > [PATCH 0/5] ti-sn65dsi83: Finalize transition to atomic operations
> 
> Look like DSI on STM32MP1 seems broken even with these on top of
> drm-misc/drm-misc-next , anything broken on the tree?

No idea, I don't have a functional display on my RPi CM4 device yet, so
I can't tell :-)

-- 
Regards,

Laurent Pinchart