[PATCH 1/4] drm/msm/dp: remove redundant checks related to ST_DISPLAY_OFF in plug/irq_ipd handlers

[Dmitry Baryshkov]

On Tue, Dec 03, 2024 at 06:36:45PM -0800, Abhinav Kumar wrote:
> 
> 
> On 12/3/2024 5:50 AM, Dmitry Baryshkov wrote:
> > On Mon, Dec 02, 2024 at 04:39:00PM -0800, Abhinav Kumar wrote:
> > > In commit 8ede2ecc3e5ee ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets"),
> > > checks were introduced to avoid handling any plug or irq hpd events in
> > > ST_DISPLAY_OFF state.
> > > 
> > > Even if we do get hpd events, after the bridge was disabled,
> > > it should get handled. Moreover, its unclear under what circumstances
> > > these events will fire because ST_DISPLAY_OFF means that the link was
> > > still connected but only the bridge was disabled. If the link was untouched,
> > > then interrupts shouldn't fire.
> > 
> > What about the link being untouched, but the monitor being toggled
> > somehow, which might generate HPD / attention events?
> > 
> 
> To confirm my understanding of this, I tested this case again with and
> without a dongle on sc7180.
> 
> Without a dongle - When the monitor is powered off and powered on, without
> physically disturbing the cable, it still generates a hotplug disconnect
> event when powered off and hotplug connect event when its powered on again.
> 
> It gets handled the same way as a user would hotplug it using the cable.
> 
> With a dongle - When the monitor is powered off , nothing happens and it
> stays in connected state. When the monitor is powered back on, it generates
> a hotplug disable followed by hotplug enable event. This behavior is
> consistent with or without this series with this dongle.
> 
> So from the DP driver point of view, for both these cases, its just another
> hotlplug disconnect/connect.
> 
> Both these cases still work fine with these changes.

Thanks for the confirmation!


Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>


> 
> > > 
> > > Even in the case of the DP compliance equipment, it should be raising these
> > > interrupts during the start of the test which is usually accompanied with either
> > > a HPD pulse or a IRQ HPD but after the bridge is disabled it should be fine
> > > to handle these anyway. In the absence of a better reason to keep these checks,
> > > drop these and if any other issues do arise, it should be handled in a different
> > > way.
> > > 
> > > Signed-off-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
> > > ---
> > >   drivers/gpu/drm/msm/dp/dp_display.c | 10 ----------
> > >   1 file changed, 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index aba925aab7ad..992184cc17e4 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -562,11 +562,6 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp, u32 data)
> > >   	drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
> > >   			dp->msm_dp_display.connector_type, state);
> > > -	if (state == ST_DISPLAY_OFF) {
> > > -		mutex_unlock(&dp->event_mutex);
> > > -		return 0;
> > > -	}
> > > -
> > >   	if (state == ST_MAINLINK_READY || state == ST_CONNECTED) {
> > >   		mutex_unlock(&dp->event_mutex);
> > >   		return 0;
> > > @@ -689,11 +684,6 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp, u32 data)
> > >   	drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
> > >   			dp->msm_dp_display.connector_type, state);
> > > -	if (state == ST_DISPLAY_OFF) {
> > > -		mutex_unlock(&dp->event_mutex);
> > > -		return 0;
> > > -	}
> > > -
> > >   	if (state == ST_MAINLINK_READY || state == ST_DISCONNECT_PENDING) {
> > >   		/* wait until ST_CONNECTED */
> > >   		msm_dp_add_event(dp, EV_IRQ_HPD_INT, 0, 1); /* delay = 1 */
> > > 
> > > -- 
> > > 2.34.1
> > > 
> > 

-- 
With best wishes
Dmitry