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

[neil . armstrong]

Hi Maxime,

On 21/08/2023 10:17, Maxime Ripard wrote:
> Hi,
>
> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@xxxxxxxxxx wrote:
> > On 17/08/2023 20:35, Dmitry Baryshkov wrote:
> > > On 16/08/2023 10:51, neil.armstrong@xxxxxxxxxx wrote:
> > > > Sending HS commands will always work on any controller, it's all
> > > > about LP commands. The Samsung panels you listed only send HS
> > > > commands so they can use prepare_prev_first and work on any
> > > > controllers.
> > >
> > > I think there is some misunderstanding there, supported by the
> > > description of the flag.
> > >
> > > If I remember correctly, some hosts (sunxi) can not send DCS
> > > commands after enabling video stream and switching to HS mode, see
> > > [1]. Thus, as you know, most of the drivers have all DSI panel setup
> > > commands in drm_panel_funcs::prepare() /
> > > drm_bridge_funcs::pre_enable() callbacks, not paying attention
> > > whether these commands are to be sent in LP or in HS mode.
> > >
> > > Previously DSI source drivers could power on the DSI link either in
> > > mode_set() or in pre_enable() callbacks, with mode_set() being the
> > > hack to make panel/bridge drivers to be able to send commands from
> > > their prepare() / pre_enable() callbacks.
> > >
> > > With the prev_first flags being introduced, we have established that
> > > DSI link should be enabled in DSI host's pre_enable() callback and
> > > switched to HS mode (be it command or video) in the enable()
> > > callback.
> > >
> > > So far so good.
> >
> > It seems coherent, I would like first to have a state of all DSI host
> > drivers and make this would actually work first before adding the
> > prev_first flag to all the required panels.
>
> This is definitely what we should do in an ideal world, but at least for
> sunxi there's no easy way for it at the moment. There's no documentation
> for it and the driver provided doesn't allow this to happen.
>
> Note that I'm not trying to discourage you or something here, I'm simply
> pointing out that this will be something that we will have to take into
> account. And it's possible that other drivers are in a similar
> situation.
>
> > > Unfortunately this change is not fully backwards-compatible. This
> > > requires that all DSI panels sending commands from prepare() should
> > > have the prepare_prev_first flag. In some sense, all such patches
> > > might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first
> > > flag to drm_panel").
> >
> > This kind of migration should be done *before* any possible
> > regression, not the other way round.
> >
> > If all panels sending commands from prepare() should have the
> > prepare_prev_first flag, then it should be first, check for
> > regressions then continue.
> >
> > <snip>
> >
> > > > I understand, but this patch doesn't qualify as a fix for
> > > > 9e15123eca79 and is too late to be merged in drm-misc-next for
> > > > v6.6, and since 9e15123eca79 actually breaks some support it
> > > > should be reverted (+ deps) since we are late in the rc cycles.
> > >
> > > If we go this way, we can never reapply these patches. There will be
> > > no guarantee that all panel drivers are completely converted. We
> > > already have a story without an observable end -
> > > DRM_BRIDGE_ATTACH_NO_CONNECTOR.
> >
> > I don't understand this point, who would block re-applying the patches ?
> >
> > The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple
> > Linux version and went smoothly because we reverted regressing patches
> > and restarted when needed, I don't understand why we can't do this
> > here aswell.
> >
> > > I'd consider that the DSI driver is correct here and it is about the
> > > panel drivers that require fixes patches. If you care about the
> > > particular Fixes tag, I have provided one several lines above.
> >
> > Unfortunately it should be done in the other way round, prepare for
> > migration, then migrate,
> >
> > I mean if it's a required migration, then it should be done and I'll
> > support it from both bridge and panel PoV.
> >
> > So, first this patch has the wrong Fixes tag, and I would like a
> > better explanation on the commit message in any case. Then I would
> > like to have an ack from some drm-misc maintainers before applying it
> > because it fixes a patch that was sent via the msm tree thus per the
> > drm-misc rules I cannot apply it via the drm-misc-next-fixes tree.
>
> Sorry, it's not clear to me what you'd like our feedback on exactly?

So let me resume the situation:

- pre_enable_prev_first was introduced in [1]
- some panels made use of pre_enable_prev_first
- Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before
- patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms)
- this fix was sent late, and is now too late to be merged via drm-misc-next

I do not consider it's the right way to fix regression caused by [2]
I consider [2] should be reverted, panels migrated to pre_enable_prev_first when needed, tested and the [2] applied again

I have no objection about [2] and it should be done widely over the whole DSI controllers
and DSI Video panels.

I also object about the Fixes tag of this patch, which is wrong, and Dmitry considers [1]
should be used but it's even more wrong since [2] really caused the regression.

And if [2] was to correct one to use, it was pushed via the MSM tree so it couldn't be
applied via drm-misc-next-fixes, right ?

[1] 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order")
[2] 9e15123eca79 ("drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset")

Thanks,
Neil

> Maxime