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

[Maxime Ripard]

On Mon, Aug 21, 2023 at 12:01:19PM +0200, neil.armstrong@xxxxxxxxxx wrote:
> 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.

Ok.

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

AFAICT, 2 is in 6.5 now, so it would be drm-misc-fixes. But yeah, it
would make more sense to take it through the MSM tree.

Maxime

Attachment: signature.asc
Description: PGP signature