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

[neil.armstrong@linaro.org]

Hi Maxime,

On 21/08/2023 10:17, Maxime Ripard wrote:
> Hi,
> 
> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote:
>> On 17/08/2023 20:35, Dmitry Baryshkov wrote:
>>> On 16/08/2023 10:51, neil.armstrong@linaro.org 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