[PATCH] drm: remove drm_bridge_hpd_disable() from drm_bridge_connector_destroy()

[PATCH] drm: remove drm_bridge_hpd_disable() from drm_bridge_connector_destroy()

[Abhinav Kumar]

drm_bridge_hpd_enable()/drm_bridge_hpd_disable() callbacks call into
the respective driver's hpd_enable()/hpd_disable() ops. These ops control
the HPD enable/disable logic which in some cases like MSM can be a
dedicate hardware block to control the HPD.

During probe_defer cases, a connector can be initialized and then later
destroyed till the probe is retried. During connector destroy in these
cases, the hpd_disable() callback gets called without a corresponding
hpd_enable() leading to an unbalanced state potentially causing even
a crash.

This can be avoided by the respective drivers maintaining their own
state logic to ensure that a hpd_disable() without a corresponding
hpd_enable() just returns without doing anything.

However, to have a generic fix it would be better to avoid the
hpd_disable() callback from the connector destroy path and let
the hpd_enable() / hpd_disable() balance be maintained by the
corresponding drm_bridge_connector_enable_hpd() /
drm_bridge_connector_disable_hpd() APIs which should get called by
drm_kms_helper_disable_hpd().

changes in v2:
	- minor change in commit text (Dmitry)

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_bridge_connector.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
index 1da93d5a1f61..c4dba39acfd8 100644
--- a/drivers/gpu/drm/drm_bridge_connector.c
+++ b/drivers/gpu/drm/drm_bridge_connector.c
@@ -187,12 +187,6 @@ static void drm_bridge_connector_destroy(struct drm_connector *connector)
 	struct drm_bridge_connector *bridge_connector =
 		to_drm_bridge_connector(connector);
 
-	if (bridge_connector->bridge_hpd) {
-		struct drm_bridge *hpd = bridge_connector->bridge_hpd;
-
-		drm_bridge_hpd_disable(hpd);
-	}
-
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 
-- 
2.40.1

Re: [PATCH] drm: remove drm_bridge_hpd_disable() from drm_bridge_connector_destroy()

[Laurent Pinchart]

Hi Abhinav,

Thank you for the patch.

On Tue, Sep 19, 2023 at 10:48:12AM -0700, Abhinav Kumar wrote:
> drm_bridge_hpd_enable()/drm_bridge_hpd_disable() callbacks call into
> the respective driver's hpd_enable()/hpd_disable() ops. These ops control
> the HPD enable/disable logic which in some cases like MSM can be a
> dedicate hardware block to control the HPD.
> 
> During probe_defer cases, a connector can be initialized and then later
> destroyed till the probe is retried. During connector destroy in these
> cases, the hpd_disable() callback gets called without a corresponding
> hpd_enable() leading to an unbalanced state potentially causing even
> a crash.
> 
> This can be avoided by the respective drivers maintaining their own
> state logic to ensure that a hpd_disable() without a corresponding
> hpd_enable() just returns without doing anything.
> 
> However, to have a generic fix it would be better to avoid the
> hpd_disable() callback from the connector destroy path and let
> the hpd_enable() / hpd_disable() balance be maintained by the
> corresponding drm_bridge_connector_enable_hpd() /
> drm_bridge_connector_disable_hpd() APIs which should get called by
> drm_kms_helper_disable_hpd().

The change makes sense to me, but I'm a bit worried this could introduce
a regression by leaving HPD enabled in some cases.

I agree that bridges shouldn't track the HPD state, it should be tracked
by the core and the .enable_hpd() and .disable_hpd() operations should
be balanced. Their documentation, however, doesn't clearly state this,
and the documentation of the callers of these operations is also fairly
unclear.

Could you perhaps try to improve the documentation ? With that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

for this patch.

> changes in v2:
> 	- minor change in commit text (Dmitry)
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/drm_bridge_connector.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> index 1da93d5a1f61..c4dba39acfd8 100644
> --- a/drivers/gpu/drm/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/drm_bridge_connector.c
> @@ -187,12 +187,6 @@ static void drm_bridge_connector_destroy(struct drm_connector *connector)
>  	struct drm_bridge_connector *bridge_connector =
>  		to_drm_bridge_connector(connector);
>  
> -	if (bridge_connector->bridge_hpd) {
> -		struct drm_bridge *hpd = bridge_connector->bridge_hpd;
> -
> -		drm_bridge_hpd_disable(hpd);
> -	}
> -
>  	drm_connector_unregister(connector);
>  	drm_connector_cleanup(connector);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: remove drm_bridge_hpd_disable() from drm_bridge_connector_destroy()

[Abhinav Kumar]

Hi Laurent

On 9/19/2023 11:12 AM, Laurent Pinchart wrote:
> Hi Abhinav,
> 
> Thank you for the patch.
> 
> On Tue, Sep 19, 2023 at 10:48:12AM -0700, Abhinav Kumar wrote:
>> drm_bridge_hpd_enable()/drm_bridge_hpd_disable() callbacks call into
>> the respective driver's hpd_enable()/hpd_disable() ops. These ops control
>> the HPD enable/disable logic which in some cases like MSM can be a
>> dedicate hardware block to control the HPD.
>>
>> During probe_defer cases, a connector can be initialized and then later
>> destroyed till the probe is retried. During connector destroy in these
>> cases, the hpd_disable() callback gets called without a corresponding
>> hpd_enable() leading to an unbalanced state potentially causing even
>> a crash.
>>
>> This can be avoided by the respective drivers maintaining their own
>> state logic to ensure that a hpd_disable() without a corresponding
>> hpd_enable() just returns without doing anything.
>>
>> However, to have a generic fix it would be better to avoid the
>> hpd_disable() callback from the connector destroy path and let
>> the hpd_enable() / hpd_disable() balance be maintained by the
>> corresponding drm_bridge_connector_enable_hpd() /
>> drm_bridge_connector_disable_hpd() APIs which should get called by
>> drm_kms_helper_disable_hpd().
> 
> The change makes sense to me, but I'm a bit worried this could introduce
> a regression by leaving HPD enabled in some cases.
> 
> I agree that bridges shouldn't track the HPD state, it should be tracked
> by the core and the .enable_hpd() and .disable_hpd() operations should
> be balanced. Their documentation, however, doesn't clearly state this,
> and the documentation of the callers of these operations is also fairly
> unclear.
> 
> Could you perhaps try to improve the documentation ? With that,
> 

Yes, sure, Let me upload another patch to improve the documentation of 
.enable_hpd(), .disable_hpd() and its callers.

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> for this patch.
> 

Thanks

Abhinav

>> changes in v2:
>> 	- minor change in commit text (Dmitry)
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/drm_bridge_connector.c | 6 ------
>>   1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
>> index 1da93d5a1f61..c4dba39acfd8 100644
>> --- a/drivers/gpu/drm/drm_bridge_connector.c
>> +++ b/drivers/gpu/drm/drm_bridge_connector.c
>> @@ -187,12 +187,6 @@ static void drm_bridge_connector_destroy(struct drm_connector *connector)
>>   	struct drm_bridge_connector *bridge_connector =
>>   		to_drm_bridge_connector(connector);
>>   
>> -	if (bridge_connector->bridge_hpd) {
>> -		struct drm_bridge *hpd = bridge_connector->bridge_hpd;
>> -
>> -		drm_bridge_hpd_disable(hpd);
>> -	}
>> -
>>   	drm_connector_unregister(connector);
>>   	drm_connector_cleanup(connector);
>>   
> 

Re: [PATCH] drm: remove drm_bridge_hpd_disable() from drm_bridge_connector_destroy()

[Dmitry Baryshkov]

On Tue, 19 Sep 2023 10:48:12 -0700, Abhinav Kumar wrote:
> drm_bridge_hpd_enable()/drm_bridge_hpd_disable() callbacks call into
> the respective driver's hpd_enable()/hpd_disable() ops. These ops control
> the HPD enable/disable logic which in some cases like MSM can be a
> dedicate hardware block to control the HPD.
> 
> During probe_defer cases, a connector can be initialized and then later
> destroyed till the probe is retried. During connector destroy in these
> cases, the hpd_disable() callback gets called without a corresponding
> hpd_enable() leading to an unbalanced state potentially causing even
> a crash.
> 
> [...]

Applied to drm-misc-next, thanks!

[1/1] drm: remove drm_bridge_hpd_disable() from drm_bridge_connector_destroy()
      commit: f730e7adfd69d7ac859d8fe4d67e980cbad1e445

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH] drm: remove drm_bridge_hpd_disable() from drm_bridge_connector_destroy()

[Abhinav Kumar]

drm_bridge_hpd_enable()/drm_bridge_hpd_disable() callbacks call into
the respective driver's hpd_enable()/hpd_disable() ops. These ops control
the HPD enable/disable logic which in some cases like MSM can be a
dedicate hardware block to control the HPD.

During probe_defer cases, a connector can be initialized and then later
destroyed till the probe is retried. During connector destroy in these
cases, the hpd_disable() callback gets called without a corresponding
hpd_enable() leading to an unbalanced state potentially causing even
a crash.

This can be avoided by the respective drivers maintaining their own
state logic to ensure that a hpd_disable() without a corresponding
hpd_enable() just returns without doing anything.

However, to have a generic fix it would be better to avoid the
hpd_disable() callback from the connector destroy path and let
the hpd_enable() / hpd_disable() balance be maintained by the
corresponding drm_bridge_connector_enable_hpd() /
drm_bridge_connector_disable_hpd() APIs.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/drm_bridge_connector.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
index 19ae4a177ac3..3d4e25c2f3d7 100644
--- a/drivers/gpu/drm/drm_bridge_connector.c
+++ b/drivers/gpu/drm/drm_bridge_connector.c
@@ -187,12 +187,6 @@ static void drm_bridge_connector_destroy(struct drm_connector *connector)
 	struct drm_bridge_connector *bridge_connector =
 		to_drm_bridge_connector(connector);
 
-	if (bridge_connector->bridge_hpd) {
-		struct drm_bridge *hpd = bridge_connector->bridge_hpd;
-
-		drm_bridge_hpd_disable(hpd);
-	}
-
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 
-- 
2.40.1

Re: [PATCH] drm: remove drm_bridge_hpd_disable() from drm_bridge_connector_destroy()

[Dmitry Baryshkov]

On 18/07/2023 01:34, Abhinav Kumar wrote:
> drm_bridge_hpd_enable()/drm_bridge_hpd_disable() callbacks call into
> the respective driver's hpd_enable()/hpd_disable() ops. These ops control
> the HPD enable/disable logic which in some cases like MSM can be a
> dedicate hardware block to control the HPD.
> 
> During probe_defer cases, a connector can be initialized and then later
> destroyed till the probe is retried. During connector destroy in these
> cases, the hpd_disable() callback gets called without a corresponding
> hpd_enable() leading to an unbalanced state potentially causing even
> a crash.
> 
> This can be avoided by the respective drivers maintaining their own
> state logic to ensure that a hpd_disable() without a corresponding
> hpd_enable() just returns without doing anything.
> 
> However, to have a generic fix it would be better to avoid the
> hpd_disable() callback from the connector destroy path and let
> the hpd_enable() / hpd_disable() balance be maintained by the
> corresponding drm_bridge_connector_enable_hpd() /
> drm_bridge_connector_disable_hpd() APIs.

Which should get called by the drm_kms_helper_disable_hpd().

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

> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/drm_bridge_connector.c | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> index 19ae4a177ac3..3d4e25c2f3d7 100644
> --- a/drivers/gpu/drm/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/drm_bridge_connector.c
> @@ -187,12 +187,6 @@ static void drm_bridge_connector_destroy(struct drm_connector *connector)
>   	struct drm_bridge_connector *bridge_connector =
>   		to_drm_bridge_connector(connector);
>   
> -	if (bridge_connector->bridge_hpd) {
> -		struct drm_bridge *hpd = bridge_connector->bridge_hpd;
> -
> -		drm_bridge_hpd_disable(hpd);
> -	}
> -
>   	drm_connector_unregister(connector);
>   	drm_connector_cleanup(connector);
>   

-- 
With best wishes
Dmitry