Hi Neil
On Mon, 28 Aug 2023 at 09:49, <neil.armstr...@linaro.org> wrote:
>
> Hi Jessica,
>
> On 25/08/2023 20:37, Jessica Zhang wrote:
> >
> >
> > On 8/21/2023 3:01 AM, neil.armstr...@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.armstr...@linaro.org wrote:
> >>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote:
> >>>>> On 16/08/2023 10:51, neil.armstr...@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
> >
> > Hi Neil and Maxime,
> >
> > I agree with Neil that 9e15123eca79 was the commit that introduced the
> > issue (since it changed the MSM DSI host behavior).
> >
> > However, I'm not too keen on simply reverting that patch because
> >
> > 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it
> > actually makes more sense to power on DSI host in pre_enable than in
> > modeset (since modeset is meant for setting the bridge mode), and
>
> I never objected that, it's the right path to go.
>
> >
> > 2) I think it would be good practice to keep specific bridge chip checks
> > out of the DSI host driver.
>
> We discussed about a plan with Maxime and Dmitry about that, and it would
> require adding
> a proper atomic panel API to handle a "negociation" with the host controller.
>
> >
> >
> > That being said, what do you think about setting the default value of
> > prepare_prev_first to true (possibly in panel_bridge_attach)?
>
> As Dmitry pointed, all panels sending LP commands in pre_enable() should have
> prepare_prev_first to true.
Any panel wishing the clock and data lanes to be in a defined LP-11
state before pre_enable() is called need to set prepare_prev_first to
true. This is not a universal requirement of all DSI peripherals for
which commands are sent from pre_enable - a number will happily power
up at LP-00.
It is true that no harm will occur on those devices that do support
non-LP-11 power up if the host is in LP-11, so a blanket setting of
the flag for any panel driver sending DSI commands in pre_enable
should be safe.
It is documented [1] that transfer can be called at any time,
regardless of the state of the host. The MSM driver isn't supporting
that, hence issues.
[2] further clarifies that it is expected to power up the host
controller, send the message, and power down again.
[1]
https://github.com/torvalds/linux/blob/master/include/drm/drm_mipi_dsi.h#L84-L87
[2]
https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_bridge.c#L185-L188
> >
> > It seems to me that most panel drivers send DCS commands during pre_enable,
> > so maybe it would make more sense to power on DSI host before panel
> > enable() by default. Any panel that needs DSI host to be powered on later
> > could then explicitly set the flag to false in their respective drivers.
>
> A proper migration should be done, yes, but not as a fix on top of v6.5.
I looked at this when adding prepare_prev_first, but as the DSI
control path is separate from the bridge chain there's no obvious way
to automatically set a bridge flag from the mipi_dsi registration.
Dave
> Neil
>
> >
> > Thanks,
> >
> > Jessica Zhang
> >
> >
> >>
> >> 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
> >>
>
