Re: [PATCH v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer

[Abhinav Kumar]

On 3/19/2024 11:15 AM, Doug Anderson wrote:
> Hi,
>
> On Tue, Mar 19, 2024 at 10:27 AM Dmitry Baryshkov
> <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > On Tue, 19 Mar 2024 at 19:13, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> > > On 3/18/2024 5:55 PM, Dmitry Baryshkov wrote:
> > > > On Tue, 19 Mar 2024 at 02:19, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> > > > > +bjorn, johan as fyi for sc8280xp
> > > > >
> > > > > On 3/15/2024 2:36 PM, Douglas Anderson wrote:
> > > > > > Before the introduction of the wait_hpd_asserted() callback in commit
> > > > > > 841d742f094e ("drm/dp: Add wait_hpd_asserted() callback to struct
> > > > > > drm_dp_aux") the API between panel drivers and DP AUX bus drivers was
> > > > > > that it was up to the AUX bus driver to wait for HPD in the transfer()
> > > > > > function.
> > > > > >
> > > > > > Now wait_hpd_asserted() has been added. The two panel drivers that are
> > > > > > DP AUX endpoints use it. See commit 2327b13d6c47 ("drm/panel-edp: Take
> > > > > > advantage of wait_hpd_asserted() in struct drm_dp_aux") and commit
> > > > > > 3b5765df375c ("drm/panel: atna33xc20: Take advantage of
> > > > > > wait_hpd_asserted() in struct drm_dp_aux"). We've implemented
> > > > > > wait_hpd_asserted() in the MSM DP driver as of commit e2969ee30252
> > > > > > ("drm/msm/dp: move of_dp_aux_populate_bus() to eDP probe()"). There is
> > > > > > no longer any reason for long wait in the AUX transfer() function.
> > > > > > Remove it.
> > > > > >
> > > > > > NOTE: the wait_hpd_asserted() is listed as "optional". That means it's
> > > > > > optional for the DP AUX bus to implement. In the case of the MSM DP
> > > > > > driver we implement it so we can assume it will be called.
> > > > >
> > > > > How do we enforce that for any new edp panels to be used with MSM, the
> > > > > panels should atleast invoke wait_hpd_asserted()?
> > > > >
> > > > > I agree that since MSM implements it, even though its listed as
> > > > > optional, we can drop this additional wait. So nothing wrong with this
> > > > > patch for current users including sc8280xp, sc7280 and sc7180.
> > > > >
> > > > > But, does there need to be some documentation that the edp panels not
> > > > > using the panel-edp framework should still invoke wait_hpd_asserted()?
> > > > >
> > > > > Since its marked as optional, what happens if the edp panel driver,
> > > > > skips calling wait_hpd_asserted()?
> > > >
> > > > It is optional for the DP AUX implementations, not for the panel to be called.
> > >
> > > Yes, I understood that part, but is there anything from the panel side
> > > which mandates calling wait_hpd_asserted()?
> > >
> > > Is this documented somewhere for all edp panels to do:
> > >
> > > if (aux->wait_hpd_asserted)
> > >          aux->wait_hpd_asserted(aux, wait_us);
> >
> > That's obviously not true, e.g. if panel-edp.c handled HPD signal via
> > the GPIO pin.
> >
> > But the documentation explicitly says that the host will be powered up
> > automatically, but the caller must ensure that the panel is powered
> > on. `It's up to the caller of this code to make sure that the panel is
> > powered on if getting an error back is not OK.'
>
> It wouldn't hurt to send out a documentation patch that makes this
> more explicit. OK, I sent:
>
> https://lore.kernel.org/r/20240319111432.1.I521dad0693cc24fe4dd14cba0c7048d94f5b6b41@changeid
>
> -Doug

Thanks, with that in place, this is

Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>