diff mbox series

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

Message ID 20241202-hpd_display_off-v1-1-8d0551847753@quicinc.com (mailing list archive)
State New, archived
Headers show
Series drm/msm/dp: ST_DISPLAY_OFF hpd cleanup | expand

Commit Message

Abhinav Kumar Dec. 3, 2024, 12:39 a.m. UTC 
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.

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@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 10 ----------
 1 file changed, 10 deletions(-)

Comments

Dmitry Baryshkov Dec. 3, 2024, 1:50 p.m. UTC | #1 
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?

> 
> 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@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
>
Abhinav Kumar Dec. 4, 2024, 2:36 a.m. UTC | #2 
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.

>>
>> 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@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
>>
>
Dmitry Baryshkov Dec. 4, 2024, 3:03 a.m. UTC | #3 
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@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@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
> > > 
> >
diff mbox series

Patch

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 */