On Tue, 29 Aug 2023 at 20:22, Abhinav Kumar <quic_abhin...@quicinc.com> wrote:
>
>
>
> On 8/29/2023 9:43 AM, Dmitry Baryshkov wrote:
> > On Tue, 29 Aug 2023 at 19:37, Abhinav Kumar <quic_abhin...@quicinc.com>
> > wrote:
> >>
> >>
> >>
> >> On 8/29/2023 2:26 AM, Dmitry Baryshkov wrote:
> >>> On Tue, 29 Aug 2023 at 12:22, <neil.armstr...@linaro.org> wrote:
> >>>>
> >>>> On 28/08/2023 19:07, Abhinav Kumar wrote:
> >>>>> Hi Neil
> >>>>>
> >>>>> Sorry I didnt respond earlier on this thread.
> >>>>>
> >>>>> On 8/28/2023 1:49 AM, 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.
> >>>>>>
> >>>>>
> >>>>> Ack.
> >>>>>
> >>>>>>>
> >>>>>>> 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.
> >>>>>>
> >>>>>
> >>>>> May I know what type of negotiation is needed here?
> >>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> 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.
> >>>>>>
> >>>>>
> >>>>> I wanted to respond to this earlier but didnt get a chance.
> >>>>>
> >>>>> From the documentation of this flag, this has nothing to do whether
> >>>>> panels are sending the LP commands (commands sent in LP mode) OR HS
> >>>>> commands (commands sent in HS mode).
> >>>>>
> >>>>> This is more about sending the commands whether the lanes are in LP11
> >>>>> state before sending the ON commands.
> >>>>>
> >>>>> 195 * The previous controller should be prepared first, before the
> >>>>> prepare
> >>>>> 196 * for the panel is called. This is largely required for DSI
> >>>>> panels
> >>>>> 197 * where the DSI host controller should be initialised to LP-11
> >>>>> before
> >>>>> 198 * the panel is powered up.
> >>>>> 199 */
> >>>>> 200 bool prepare_prev_first;
> >>>>>
> >>>>> These are conceptually different and thats what I explained Dmitry in
> >>>>> our call.
> >>>>>
> >>>>> Sending ON commands in LP11 state is a requirement I have seen with
> >>>>> many panels and its actually the right expectation as well to send the
> >>>>> commands when the lanes are in a well-defined LP11 state.
> >>>>>
> >>>>> From the panels which I have seen, the opposite is never true (OR i
> >>>>> have never seen it this way).
> >>>>>
> >>>>> The parade chip was the only exception and that issue was never
> >>>>> root-caused leading us to have bridge specific handling in MSM driver.
> >>>>>
> >>>>> In other words, it would be very unlikely that a panel should be broken
> >>>>> or shouldn't work when the ON commands are sent when the lanes are in
> >>>>> LP11 state.
> >>>>>
> >>>>> So I agree with Jessica, that we should set the default value of this
> >>>>> flag to true in the framework so that only the bridges/panels which
> >>>>> need this to be false do that explicitly. From the examples I pointed
> >>>>> out including MTK, even those vendors are powering on their DSI in
> >>>>> pre_enable() which means none of these panels will work there too.
> >>>>>
> >>>>>>>
> >>>>>>> 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 am fine to drop this fix in favor of making the prepare_prev_first as
> >>>>> default true but we need an agreement first. From what I can see,
> >>>>> parade chip will be the only one which will need this to be set to
> >>>>> false and we can make that change.
> >>>>>
> >>>>> Let me know if this works as a migration plan.
> >>>>
> >>>> Yep agreed, let's do this
> >>>>
> >>>> The panel's prepare_prev_first should be reversed to something like
> >>>> not_prepare_prev_first to make it an exception.
> >>>
> >>> This will break all non-DSI panels, which might depend on the current
> >>> bridge calling order.
> >>>
> >>> I started looking at the explicit DSI power up sequencing, but it will
> >>> take a few more days to mature.
> >>>
> >>
> >> May I know why this would break all non-DSI panels?
> >
> > Existing panel drivers might be depending on the init order. Do we
> > know for sure that none of DPI panels will be broken if there is a
> > video stream ongoing during the reset procedure?
> > Or the panel-edp, which I'm pretty sure will require not_prepare_prev_first.
> >
>
> Can you please explain why would video stream be ON in pre_enable()?
>
> Even though we call msm_dsi_host_enable() in the DSI's pre_enable(), the
> timing engine is not enabled until the encoder's enable and the first
> commit after that so video stream wont be sent till then.
You are describing the MSM DSI case. I was pointing to the fact that
parent's pre_enable if called too early might conflict with the next
bridge driver in the _generic_ case. E.g. eDP or DPI. Even if is not a
full-featured video stream, this state might confuse the panel. So we
can not blindly switch the order of pre_enable callbacks for the
bridge-panel pair.
>
> drm_atomic_bridge_chain_pre_enable() is called before the encoder's enable.
>
> drm_atomic_bridge_chain_enable() is the one which is called after the
> encoder's enable().
>
> >>
> >> Like I said, we dont know the full details of the parade issue but I do
> >> not see any reason why powering on a bridge chip with the DSI lanes
> >> being in proper LP11 state should cause an issue. Its a well defined and
> >> documented state in DSI spec.
> >>
> >> On the contrary, trying to turn on a bridge chip before powering on a
> >> controller could have more issues as we do not know what state the lanes
> >> are in when the MIPI devices (panel or bridge) are powered up.
> >>
> >> This sets the expectation and handshake straight.
> >
> >
--
With best wishes
Dmitry
