| Message ID | 1656027256-6552-1-git-send-email-quic_khsieh@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dp: no dp_hpd_unplug_handle() required for eDP | expand |
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?
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?
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;
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(-)