lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2C14A755-81B2-44CD-A75B-627A137251A1@linaro.org>
Date:   Tue, 06 Dec 2022 00:11:22 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Bjorn Andersson <quic_bjorande@...cinc.com>
CC:     Rob Clark <robdclark@...il.com>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Sean Paul <sean@...rly.run>, David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Kalyan Thota <quic_kalyant@...cinc.com>,
        Jessica Zhang <quic_jesszhan@...cinc.com>,
        Kuogee Hsieh <quic_khsieh@...cinc.com>,
        Johan Hovold <johan+linaro@...nel.org>,
        Sankeerth Billakanti <quic_sbillaka@...cinc.com>,
        linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        freedreno@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 10/13] drm/msm/dp: Rely on hpd_enable/disable callbacks



On 5 December 2022 20:44:30 GMT+03:00, Bjorn Andersson <quic_bjorande@...cinc.com> wrote:
>From: Bjorn Andersson <bjorn.andersson@...aro.org>
>
>The DisplayPort controller's internal HPD interrupt handling is used for
>cases where the HPD signal is connected to a GPIO which is pinmuxed into
>the DisplayPort controller. In other configurations the HPD notification
>might be delivered by the DRM framework from an associated bridge.
>
>This difference is not appropriately represented by the "is_edp"
>boolean, but is properly represented by the frameworks invocation of the
>hpd_enable() and hpd_disable() callbacks. Switch the current condition
>to rely on these callbacks instead.
>
>This ensures appropriate handling of the three cases; no bridge
>connected, a bridge without DRM_BRIDGE_OP_HPD and a bridge with
>DRM_BRIDGE_OP_HPD.
>
>Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.org>
>Signed-off-by: Bjorn Andersson <quic_bjorande@...cinc.com>
>---
>
>Worth mentioning, I did look into moving the HPD enablement/disablement
>completely into these new callbacks, but that affect the entire power
>management model of the driver, so I think it's worth to tackle that in
>subsequent changes. It seems also reasonable to expect that we by such
>modifications could leave the block unclocked until the external HPD
>notification arrives...

I see... I still suppose this is the way to go in the long term.

For now:

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

>
>Changes since v3:
>- Introduced reliance on hpd_enable/disable callbacks instead of next_bridge
>
> drivers/gpu/drm/msm/dp/dp_display.c | 35 ++++++++++++++++++++---------
> drivers/gpu/drm/msm/dp/dp_display.h |  1 +
> drivers/gpu/drm/msm/dp/dp_drm.c     |  2 ++
> drivers/gpu/drm/msm/dp/dp_drm.h     |  2 ++
> 4 files changed, 30 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>index bb92c33beff8..3e464c33ff11 100644
>--- a/drivers/gpu/drm/msm/dp/dp_display.c
>+++ b/drivers/gpu/drm/msm/dp/dp_display.c
>@@ -610,7 +610,7 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
> 	}
> 
> 	/* enable HDP irq_hpd/replug interrupt */
>-	if (!dp->dp_display.is_edp)
>+	if (dp->dp_display.internal_hpd)
> 		dp_catalog_hpd_config_intr(dp->catalog,
> 					   DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK,
> 					   true);
>@@ -653,7 +653,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
> 			dp->dp_display.connector_type, state);
> 
> 	/* disable irq_hpd/replug interrupts */
>-	if (!dp->dp_display.is_edp)
>+	if (dp->dp_display.internal_hpd)
> 		dp_catalog_hpd_config_intr(dp->catalog,
> 					   DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK,
> 					   false);
>@@ -682,7 +682,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
> 	}
> 
> 	/* disable HPD plug interrupts */
>-	if (!dp->dp_display.is_edp)
>+	if (dp->dp_display.internal_hpd)
> 		dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, false);
> 
> 	/*
>@@ -701,7 +701,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
> 	dp_display_handle_plugged_change(&dp->dp_display, false);
> 
> 	/* enable HDP plug interrupt to prepare for next plugin */
>-	if (!dp->dp_display.is_edp)
>+	if (dp->dp_display.internal_hpd)
> 		dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
> 
> 	drm_dbg_dp(dp->drm_dev, "After, type=%d hpd_state=%d\n",
>@@ -1086,8 +1086,8 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
> 	dp_display_host_init(dp);
> 	dp_catalog_ctrl_hpd_config(dp->catalog);
> 
>-	/* Enable plug and unplug interrupts only for external DisplayPort */
>-	if (!dp->dp_display.is_edp)
>+	/* Enable plug and unplug interrupts only if requested */
>+	if (dp->dp_display.internal_hpd)
> 		dp_catalog_hpd_config_intr(dp->catalog,
> 				DP_DP_HPD_PLUG_INT_MASK |
> 				DP_DP_HPD_UNPLUG_INT_MASK,
>@@ -1379,8 +1379,7 @@ static int dp_pm_resume(struct device *dev)
> 
> 	dp_catalog_ctrl_hpd_config(dp->catalog);
> 
>-
>-	if (!dp->dp_display.is_edp)
>+	if (dp->dp_display.internal_hpd)
> 		dp_catalog_hpd_config_intr(dp->catalog,
> 				DP_DP_HPD_PLUG_INT_MASK |
> 				DP_DP_HPD_UNPLUG_INT_MASK,
>@@ -1778,6 +1777,22 @@ void dp_bridge_mode_set(struct drm_bridge *drm_bridge,
> 		!!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC);
> }
> 
>+void dp_bridge_hpd_enable(struct drm_bridge *bridge)
>+{
>+	struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
>+	struct msm_dp *dp_display = dp_bridge->dp_display;
>+
>+	dp_display->internal_hpd = true;
>+}
>+
>+void dp_bridge_hpd_disable(struct drm_bridge *bridge)
>+{
>+	struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
>+	struct msm_dp *dp_display = dp_bridge->dp_display;
>+
>+	dp_display->internal_hpd = false;
>+}
>+
> void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> 			  enum drm_connector_status status)
> {
>@@ -1785,8 +1800,8 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> 	struct msm_dp *dp_display = dp_bridge->dp_display;
> 	struct dp_display_private *dp = container_of(dp_display, struct dp_display_private, dp_display);
> 
>-	/* Without next_bridge interrupts are handled by the DP core directly */
>-	if (!dp_display->next_bridge)
>+	/* Plug events are generated by the dp_display_irq_handler() */
>+	if (dp_display->internal_hpd)
> 		return;
> 
> 	if (!dp->core_initialized) {
>diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
>index dcedf021f7fe..371337d0fae2 100644
>--- a/drivers/gpu/drm/msm/dp/dp_display.h
>+++ b/drivers/gpu/drm/msm/dp/dp_display.h
>@@ -21,6 +21,7 @@ struct msm_dp {
> 	bool power_on;
> 	unsigned int connector_type;
> 	bool is_edp;
>+	bool internal_hpd;
> 
> 	hdmi_codec_plugged_cb plugged_cb;
> 
>diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
>index 3898366ebd5e..275370f21115 100644
>--- a/drivers/gpu/drm/msm/dp/dp_drm.c
>+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
>@@ -102,6 +102,8 @@ static const struct drm_bridge_funcs dp_bridge_ops = {
> 	.get_modes    = dp_bridge_get_modes,
> 	.detect       = dp_bridge_detect,
> 	.atomic_check = dp_bridge_atomic_check,
>+	.hpd_enable   = dp_bridge_hpd_enable,
>+	.hpd_disable  = dp_bridge_hpd_disable,
> 	.hpd_notify   = dp_bridge_hpd_notify,
> };
> 
>diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h
>index 79e6b2cf2d25..250f7c66201f 100644
>--- a/drivers/gpu/drm/msm/dp/dp_drm.h
>+++ b/drivers/gpu/drm/msm/dp/dp_drm.h
>@@ -32,6 +32,8 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
> void dp_bridge_mode_set(struct drm_bridge *drm_bridge,
> 			const struct drm_display_mode *mode,
> 			const struct drm_display_mode *adjusted_mode);
>+void dp_bridge_hpd_enable(struct drm_bridge *bridge);
>+void dp_bridge_hpd_disable(struct drm_bridge *bridge);
> void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> 			  enum drm_connector_status status);
> 

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ