lkml.org 
[lkml]   [2023]   [Aug]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] drm/panel: Add prepare_prev_first flag to Visionox VTDR6130
    On Mon, Aug 21, 2023 at 12:01:19PM +0200, neil.armstrong@linaro.org 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@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.

    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
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2023-08-21 13:37    [W:4.530 / U:1.424 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site