Re: [PATCH] drm/msm/dp: no dp_hpd_unplug_handle() required for eDP

From: Stephen Boyd <swboyd@chromium.org>
To: Kuogee Hsieh <quic_khsieh@quicinc.com>,
	agross@kernel.org, airlied@linux.ie, bjorn.andersson@linaro.org,
	daniel@ffwll.ch, dianders@chromium.org,
	dmitry.baryshkov@linaro.org, robdclark@gmail.com,
	sean@poorly.run, vkoul@kernel.org
Cc: quic_abhinavk@quicinc.com, quic_aravindh@quicinc.com,
	quic_sbillaka@quicinc.com, freedreno@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/msm/dp: no dp_hpd_unplug_handle() required for eDP
Date: Thu, 23 Jun 2022 17:09:47 -0700	[thread overview]
Message-ID: <CAE-0n534jvnjX5TShZw7CB9Cu9F7yowhwUosNkJE8t_R4xHYOw@mail.gmail.com> (raw)
In-Reply-To: <1656027256-6552-1-git-send-email-quic_khsieh@quicinc.com>

Quoting Kuogee Hsieh (2022-06-23 16:34:16)
> eDP implementation does not reuried to support hpd signal. Therefore

s/reuried/require/

> it only has either ST_DISPLAY_OFF or ST_CONNECTED state during normal
> operation. This patch remove unnecessary dp_hpd_unplug_handle() for
> eDP but still keep dp_hpd_plug_handle() to support eDP to either
> booting up or resume from ST_DISCONNECTED state.

I take it that making this change also fixes a glitch seen on the eDP
panel when a second modeset happens? Can you add that detail to the
commit text? The way it reads makes it sound like this is purely a
cleanup patch, but then there's a Fixes tag so it must be a bug fix or
worthy optimization, neither of which is described.

>
> Fixes: 391c96ff0555 ("drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP")
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index da5c03a..ef9794e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1666,7 +1666,7 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
>                 return;
>         }
>
> -       if (dp->is_edp)
> +       if (dp->is_edp && dp_display->hpd_state == ST_DISCONNECTED)
>                 dp_hpd_plug_handle(dp_display, 0);
>
>         mutex_lock(&dp_display->event_mutex);
> @@ -1737,9 +1737,6 @@ void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
>
>         dp_display = container_of(dp, struct dp_display_private, dp_display);
>
> -       if (dp->is_edp)
> -               dp_hpd_unplug_handle(dp_display, 0);

dp_hpd_unplug_handle() has a !edp check, and from what I can tell after
this patch that condition will always trigger? But then I wonder why we
aren't masking the irqs for hpd when the eDP display is disabled.
Shouldn't we at least be doing that so that we don't get spurious hpd
irqs when the eDP display is off or on the path to suspend where I
suspect the power may be removed from the panel?