Messages in this thread | | | Date | Tue, 19 Sep 2023 15:53:16 -0700 | Subject | Re: [PATCH] drm: remove drm_bridge_hpd_disable() from drm_bridge_connector_destroy() | From | 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); >> >
| |