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

[neil . armstrong]

Hi Dmitry,

On 17/08/2023 20:35, Dmitry Baryshkov wrote:
> On 16/08/2023 10:51, neil.armstrong@xxxxxxxxxx wrote:
> > Hi Abhinav,
> >
> > On 14/08/2023 20:02, Abhinav Kumar wrote:

<snip>

> > 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.

> 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.

Neil

<snip>