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

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

[Kuogee Hsieh]

eDP implementation does not reuried to support hpd signal. Therefore
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.

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);
-
 	mutex_lock(&dp_display->event_mutex);
 
 	state = dp_display->hpd_state;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Stephen Boyd]

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?

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

[Kuogee Hsieh]

On 6/23/2022 5:09 PM, Stephen Boyd wrote:
> 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.

no, this patch will not fix edp (primary display) corruption issue.

This patch is pure clean up edp.

I will submit fixes edp corruption issue at other patch.

>> 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?