diff mbox series

[v3,5/7] drm/msm/hdmi: stop using drm_bridge_connector_en/disable_hpd()

Message ID 20221102180705.459294-6-dmitry.baryshkov@linaro.org (mailing list archive)
State Not Applicable
Headers show
Series drm/bridge_connector: perform HPD enablement automatically | expand

Commit Message

Dmitry Baryshkov Nov. 2, 2022, 6:07 p.m. UTC
The functionality of drm_bridge_connector_enable_hpd() and
drm_bridge_connector_disable_hpd() is provided automatically by the
drm_kms_poll helpers. Stop calling these functions manually.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Johan Hovold March 8, 2023, 10:21 a.m. UTC | #1
On Wed, Nov 02, 2022 at 09:07:03PM +0300, Dmitry Baryshkov wrote:
> The functionality of drm_bridge_connector_enable_hpd() and
> drm_bridge_connector_disable_hpd() is provided automatically by the
> drm_kms_poll helpers. Stop calling these functions manually.

I stumbled over this one when investigating a hotplug-related crash in
the MSM DRM driver which this series prevents by moving hotplug
notification enable to drm_kms_helper_poll_enable().

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/msm/hdmi/hdmi.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 93fe61b86967..a540c45d4fd3 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -348,8 +348,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>  		goto fail;
>  	}
>  
> -	drm_bridge_connector_enable_hpd(hdmi->connector);
> -
>  	ret = msm_hdmi_hpd_enable(hdmi->bridge);
>  	if (ret < 0) {
>  		DRM_DEV_ERROR(&hdmi->pdev->dev, "failed to enable HPD: %d\n", ret);

It looks like you are now enabling hotplug events before the DRM driver
is ready to receive them (i.e. msm_hdmi_hpd_enable() is called before
drm_bridge_connector_enable_hpd()).

Could this not lead to missed events or is the state being setup
correctly somewhere else?

Shouldn't the call to msm_hdmi_hpd_enable() be moved to when HPD is
enabled either way (e.g. by being converted to a hpd_enable callback)?

Johan
Dmitry Baryshkov March 8, 2023, 12:35 p.m. UTC | #2
On Wed, 8 Mar 2023 at 12:20, Johan Hovold <johan@kernel.org> wrote:
>
> On Wed, Nov 02, 2022 at 09:07:03PM +0300, Dmitry Baryshkov wrote:
> > The functionality of drm_bridge_connector_enable_hpd() and
> > drm_bridge_connector_disable_hpd() is provided automatically by the
> > drm_kms_poll helpers. Stop calling these functions manually.
>
> I stumbled over this one when investigating a hotplug-related crash in
> the MSM DRM driver which this series prevents by moving hotplug
> notification enable to drm_kms_helper_poll_enable().
>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/msm/hdmi/hdmi.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> > index 93fe61b86967..a540c45d4fd3 100644
> > --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> > @@ -348,8 +348,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
> >               goto fail;
> >       }
> >
> > -     drm_bridge_connector_enable_hpd(hdmi->connector);
> > -
> >       ret = msm_hdmi_hpd_enable(hdmi->bridge);
> >       if (ret < 0) {
> >               DRM_DEV_ERROR(&hdmi->pdev->dev, "failed to enable HPD: %d\n", ret);
>
> It looks like you are now enabling hotplug events before the DRM driver
> is ready to receive them (i.e. msm_hdmi_hpd_enable() is called before
> drm_bridge_connector_enable_hpd()).
>
> Could this not lead to missed events or is the state being setup
> correctly somewhere else?
>
> Shouldn't the call to msm_hdmi_hpd_enable() be moved to when HPD is
> enabled either way (e.g. by being converted to a hpd_enable callback)?

Eventually I'll get to it (hopefully during this weekend). There is
one item which needs to be investigated, see [1]. I have to check if
this is applicable to earlier generations, which also means
resurrecting the msm8974 HDMI PHY patchset posted ages ago. I think
the initial status is determined correctly using the .detect(). At
least I saw no issues with this patchset. However, thanks for the
pointer.

[1] https://git.codelinaro.org/linaro/qcomlt/kernel/-/commit/6ae2c308555f470ba63f90b7171519a242f96a67
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 93fe61b86967..a540c45d4fd3 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -348,8 +348,6 @@  int msm_hdmi_modeset_init(struct hdmi *hdmi,
 		goto fail;
 	}
 
-	drm_bridge_connector_enable_hpd(hdmi->connector);
-
 	ret = msm_hdmi_hpd_enable(hdmi->bridge);
 	if (ret < 0) {
 		DRM_DEV_ERROR(&hdmi->pdev->dev, "failed to enable HPD: %d\n", ret);