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

[Dave Stevenson]

Hi Dmitry

On Fri, 18 Aug 2023 at 11:27, Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> On 18/08/2023 11:25, neil.armstrong@xxxxxxxxxx wrote:
> > 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 ?
>
> Consider us reverting 9e15123eca79 now and then reapplying it next
> cycle. Then another panel / bridge that was not converted to use
> pre_enable_prev_first pops up. And suddently we have to revert them again.
>
> > 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.
>
> With DRM_BRIDGE_ATTACH_NO_CONNECTOR both host and peripheral drivers
> were involved. This way they share knowledge about the migration state.
>
> With prev_first we do not have such shared knowledge. Host assumes that
> it can work according to the documentation: turn DSI link to LP-11 in
> pre_enable(), switch to HS in enable(). It can not check whether the
> next bridge did not set pre_enable_prev_first because of it not being
> required (like for the Parade bridge) or because next bridge is not
> converted yet (and thus DSI host should power up the link in
> atomic_mode_set).
>
> Granted that there is no way for the DSI host driver to attune itself to
> the DSI peripheral driver requirements, I can only consider
> corresponding (requiring prev_first) panel drivers broken since
> 5ea6b1702781 ("drm/panel: Add prepare_prev_first flag to drm_panel") and
> all bridge drivers with this issue broken since 4fb912e5e190
> ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order").

Can I point out that even prior to 5ea6b1702781 the docs stated [1]

"Also note that those callbacks can be called no matter the state the
host is in. Drivers that need the underlying device to be powered to
perform these operations will first need to make sure it’s been
properly enabled."

added in bacbab58f09dc. So your DSI host driver isn't working in the
documented manner prior to 5ea6b1702781, therefore 5ea6b1702781
doesn't cause a regression in itself, and there was no direct
requirement for 5ea6b1702781 to add the flag to all panels.

Looking at JDI LT070ME05000 [2], the backlight is controlled via DCS
commands, therefore transfer can be called at any time to change or
read the backlight intensity, not just between pre_enable and
post_disable.


5ea6b1702781 and 4fb912e5e190 were largely trying to address the
requirements of devices such as TI's SN65DSI83 that require the DSI
data lanes to be in LP-11 before the bridge is brought out of reset
otherwise they malfunction. Being able to add better definition over
when the DSI host needs to be powered up/down is a bonus, but it
doesn't trump the requirement for transfer to be callable at any time.

  Dave

[1] https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#c.mipi_dsi_host_ops
[2] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c


> >
> >>
> >> 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>
> >
>
> --
> With best wishes
> Dmitry
>