Message ID | 20230919174813.26958-1-quic_abhinavk@quicinc.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:172:b0:3f2:4152:657d with SMTP id h50csp3609891vqi; Tue, 19 Sep 2023 11:55:54 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEXsmUO9Ea9K216vkq4FaSHF5H7vg0EDL1XzuQ8zLR9ZbkBVZ9QHQZAi5lO607jtBBVsNLb X-Received: by 2002:a05:6358:4411:b0:142:fba8:dcfc with SMTP id z17-20020a056358441100b00142fba8dcfcmr664105rwc.16.1695149754326; Tue, 19 Sep 2023 11:55:54 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695149754; cv=none; d=google.com; s=arc-20160816; b=CGzLRWoAMwEi7WhDln0nnPvxQcBD5Cmf3U+vkLmuR/s0yTyHrcN8AyoUE435fwNt3A wwX0EamobV4GKzGBGLAso5WCpdQxFuPh6/hwxGu0SW5dFST2A8gEyDDe9TNXTP3TOGBk nL45Zzf/0SokqlITBpwUzmTYSXHQ+1FNv0iZWQDGQt0nctynEkITY2CYSG+a3IEfTAOv 9zDE2oZqhy+sP/cMowXBD6eONXTFPr9D9AksvRoyyf0xmm64vroKPfkpjzqQ9LX3+oNt XVO6X49rSi0SE6TciRjH1PYjl2Fn1CNgtKLK9TDeiqyxpD5ar4ldlHyVyrZvlzg3OrGv uwWw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=0zyjwSc7Gp9FW1Ruyc85byvlIpWv6PcC5mSxYwi2VsA=; fh=REWC36CcYXQgaRG+usuoJNPaTAhBLfNTY0WOWEq5TGA=; b=uKRUropAnBFuKCHvGX4nfgZlKb+Sz2L4m4qCHlUMieFudOoFg4XH/RD30Uotk+qZSR /taVzOIZ4SkQ3nCZsfzdJDrI25/SpNgNEYdDuT6IFYqEscMITISJATnL+xlefOM4fDol Cyc+ogaSv+xaQ1pxrpKjNelnpSY4C66QPFEyDBaQVZt+yFWh5opSiwRZdoj5788zkCk1 F6IJ79YDtSMi6ANIclKTNuqVgxtvPRzEusFDn1B2kR6Hkl/5kqQdttTPHn3XxVw2/NiU 8QQKheQ8rhqNqVdvy3mLDpX/v0BkJUWX/glNUlTZEKdf03q36zDqZTFhnrpZb1nkIfSE uYqg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=CrgtiCoV; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: from agentk.vger.email (agentk.vger.email. [2620:137:e000::3:2]) by mx.google.com with ESMTPS id m11-20020a633f0b000000b0056f7592d736si9999987pga.34.2023.09.19.11.55.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 19 Sep 2023 11:55:54 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2; Authentication-Results: mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=CrgtiCoV; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id 9408381F8FEA; Tue, 19 Sep 2023 10:48:53 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232276AbjISRsn (ORCPT <rfc822;toshivichauhan@gmail.com> + 26 others); Tue, 19 Sep 2023 13:48:43 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42234 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232359AbjISRsm (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 19 Sep 2023 13:48:42 -0400 Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9CA4299 for <linux-kernel@vger.kernel.org>; Tue, 19 Sep 2023 10:48:36 -0700 (PDT) Received: from pps.filterd (m0279873.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 38JHJ8M3001928; Tue, 19 Sep 2023 17:48:24 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h=from : to : cc : subject : date : message-id : mime-version : content-transfer-encoding : content-type; s=qcppdkim1; bh=0zyjwSc7Gp9FW1Ruyc85byvlIpWv6PcC5mSxYwi2VsA=; b=CrgtiCoVRj358PBCuKy0cG3LdrFrKBznC3pAuKnsz0Y67j1dkd6NiCwKFRn3U3nHnW3t A0mBGQF0PwG67aqXjMSFU3kuy5PH5S19PficrkneMMx5q9js76jtwYOqkKLMtxygRxRH AJeMmpjtFiEF/YgwQr0Z4Dtjmke/j7BixHBT/UCPhgJlBSqYD94ciBMPbPgp7ZvrbMt9 WX8rD7GJkl4Cz8Qf0/nI+hMP6ISH45VZFZ3zLyUKn3noPbjYP23KcIHWG2EASfFF7OGF vQr5NtpdOSi3jimoX86Lim0Yq8jU3f3EXVC/1AU55RATO5O7OMR6WWpnPNCXzVeArIv/ AQ== Received: from nalasppmta01.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3t6trqjkhq-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 19 Sep 2023 17:48:24 +0000 Received: from nalasex01a.na.qualcomm.com (nalasex01a.na.qualcomm.com [10.47.209.196]) by NALASPPMTA01.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 38JHmNTO015473 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 19 Sep 2023 17:48:23 GMT Received: from abhinavk-linux.qualcomm.com (10.80.80.8) by nalasex01a.na.qualcomm.com (10.47.209.196) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.36; Tue, 19 Sep 2023 10:48:22 -0700 From: Abhinav Kumar <quic_abhinavk@quicinc.com> To: <dri-devel@lists.freedesktop.org>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch> CC: Abhinav Kumar <quic_abhinavk@quicinc.com>, <freedreno@lists.freedesktop.org>, <quic_jesszhan@quicinc.com>, <quic_parellan@quicinc.com>, <laurent.pinchart@ideasonboard.com>, <andersson@kernel.org>, <jani.nikula@linux.intel.com>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, <linux-kernel@vger.kernel.org> Subject: [PATCH] drm: remove drm_bridge_hpd_disable() from drm_bridge_connector_destroy() Date: Tue, 19 Sep 2023 10:48:12 -0700 Message-ID: <20230919174813.26958-1-quic_abhinavk@quicinc.com> X-Mailer: git-send-email 2.40.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nalasex01a.na.qualcomm.com (10.47.209.196) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-GUID: Tt0V_lMivdnpfIUFqMTVGbH9nQxcbIYY X-Proofpoint-ORIG-GUID: Tt0V_lMivdnpfIUFqMTVGbH9nQxcbIYY X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.267,Aquarius:18.0.980,Hydra:6.0.601,FMLib:17.11.176.26 definitions=2023-09-19_09,2023-09-19_01,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 suspectscore=0 impostorscore=0 clxscore=1011 lowpriorityscore=0 malwarescore=0 mlxlogscore=838 priorityscore=1501 phishscore=0 bulkscore=0 spamscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2308100000 definitions=main-2309190152 X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Tue, 19 Sep 2023 10:48:53 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1777493348388680311 X-GMAIL-MSGID: 1777493348388680311 |
Series |
drm: remove drm_bridge_hpd_disable() from drm_bridge_connector_destroy()
|
|
Commit Message
Abhinav Kumar
Sept. 19, 2023, 5:48 p.m. UTC
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(-)
Comments
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); >
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); >> >
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,
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);