Re: [PATCH] drm/panel: Add prepare_prev_first flag to Visionox VTDR6130

[Abhinav Kumar]

Hi Neil

On 8/14/2023 1:01 AM, neil.armstrong@xxxxxxxxxx wrote:
> Hi Abhinav,
>
> On 10/08/2023 18:26, Abhinav Kumar wrote:
> > Hi Neil
> >
> > On 8/3/2023 10:19 AM, Jessica Zhang wrote:
> > > On 7/31/2023 6:00 AM, Neil Armstrong wrote:
> > > > Hi,
> > > >
> > > > On 26/07/2023 00:56, Jessica Zhang wrote:
> > > > > Due to a recent introduction of the pre_enable_prev_first bridge 
> > > > > flag [1],
> > > > > the panel driver will be probed before the DSI is enabled, causing the
> > > > > DCS commands to fail to send.
> > > > >
> > > > > Ensure that DSI is enabled before panel probe by setting the
> > > > > prepare_prev_first flag for the panel.
> > > >
> > > > Well this is specific to MSM DSI driver, it's not related at all to 
> > > > the panel.
> >
> > I dont fully agree this is a MSM DSI driver specific thing.
> >
> > If the panel can send its commands in its enable() callback, then this 
> > flag need not be set.
> >
> > When a panel sends its DCS commands in its pre_enable() callback, any 
> > DSI controller will need to be ON before that otherwise DCS commands 
> > cannot be sent.
> >
> > With this in mind, may I know why is this a MSM change and not a panel 
> > change?
> >
> > As per my discussion with Dmitry during the last sync up, we were 
> > aligned on this expectation.
>
> As of today, only the MSM DSI driver expects panels to have 
> prepare_prev_first because it's the first
> one calling pre_enable() before the DSI controller to be on, all other 
> DSI drivers I know
> still enables the DSI controller in mode_set() and thus can send 
> commands in pre_enable() which
> is a loose way to map the pre-video state for DSI panels...

It looks like there are multiple panels already setting this flag so 
this panel will not be the first unless they were added to make those 
work with MSM (which seems unlikely)

panel-samsung-s6d7aa0.c:        ctx->panel.prepare_prev_first = true;
panel-samsung-s6e3ha2.c:        ctx->panel.prepare_prev_first = true;
panel-samsung-s6e63j0x03.c:     ctx->panel.prepare_prev_first = true;
panel-samsung-s6e8aa0.c:        ctx->panel.prepare_prev_first = true;

This is where I would like to understand a bit that if the panel sends 
out the ON commands in enable() instead of pre_enable() then, this flag 
will not be needed. So its also depends on the panel side and thats why
the bridge feeds of the panel's input in devm_drm_panel_bridge_add_typed()

bridge->pre_enable_prev_first = panel->prepare_prev_first;

> A panel driver should not depend on features of a DSI controller, which 
> is the case here
> with this patch. Today's expectation is to send DSI commands in 
> pre_enable() then when enabled
> expect to be in video mode when enable() is called.

We are not depending on any feature as such. Any DSI controller , not 
just MSM's would need to be ON for DCS commands to be sent out in the 
panel's pre_enable() callback.

Its not true that MSM is the only driver powering on the DSI controller 
in pre_enable(). Even MTK seems to be doing that

mtk_dsi_bridge_atomic_pre_enable

So I assume any panel which sends out commands in pre_enable() will not 
work with MTK as well.

> The main reason is because some DSI controllers cannot send LP commands 
> after switching
> to video mode (allwinner for example), so we must take this into account.
>
> For v6.6, I don't see other solutions than reverting 9e15123eca79 
> (reverting won't regress anything,
> because now it regresses also other panels on MSM platforms) and try to 
> find a proper solution for v6.7...

No, I would prefer not to revert that. It will bring back special 
handling for the parade chip into MSM driver, something which I would 
prefer not to go back to. Powering on the DSI in modeset() was done only 
for the parade chip.

> Neil
>
> > Thanks
> >
> > Abhinav
> >
> > > Hi Neil,
> > >
> > > I think there might be some confusion caused by the commit message -- 
> > > instead of "enabled before panel probe", it should be "enabled before 
> > > panel pre_enable()" as the panel on commands are sent during 
> > > prepare(), which is matched to bridge pre_enable().
> > >
> > > IIRC the general rule is that the panel driver should set the 
> > > prepare_prev_first flag if the on commands are sent during 
> > > pre_enable(), so I'll keep the code change but correct the commit 
> > > message if that's ok with you.
> > >
> > > Thanks,
> >
> > > Jessica Zhang
> > >
> > > > Neil
> > > >
> > > > > [1] commit 4fb912e5e190 ("drm/bridge: Introduce 
> > > > > pre_enable_prev_first to alter bridge init order")
>
> It's not the right commit that cause regression here, it's :
>
> 9e15123eca79 drm/msm/dsi: Stop unconditionally powering up DSI hosts at 
> modeset
>
> > > > > Fixes: 2349183d32d8 ("drm/panel: add visionox vtdr6130 DSI panel 
> > > > > driver")
> > > > > Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
> > > > > ---
> > > > >   drivers/gpu/drm/panel/panel-visionox-vtdr6130.c | 1 +
> > > > >   1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c 
> > > > > b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> > > > > index bb0dfd86ea67..e1363e128e7e 100644
> > > > > --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> > > > > +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> > > > > @@ -296,6 +296,7 @@ static int visionox_vtdr6130_probe(struct 
> > > > > mipi_dsi_device *dsi)
> > > > >       dsi->format = MIPI_DSI_FMT_RGB888;
> > > > >       dsi->mode_flags = MIPI_DSI_MODE_VIDEO | 
> > > > > MIPI_DSI_MODE_NO_EOT_PACKET |
> > > > >                 MIPI_DSI_CLOCK_NON_CONTINUOUS;
> > > > > +    ctx->panel.prepare_prev_first = true;
> > > > >       drm_panel_init(&ctx->panel, dev, &visionox_vtdr6130_panel_funcs,
> > > > >                  DRM_MODE_CONNECTOR_DSI);
> > > > >
> > > > > ---
> > > > > base-commit: 28a5c036b05fc5c935cc72d76abd3589825ea9cd
> > > > > change-id: 20230717-visionox-vtdr-prev-first-e00ae02eec9f
> > > > >
> > > > > Best regards,