[PATCH RESEND v9 0/6] MHDP8546 fixes related to DRM_BRIDGE_ATTACH_NO_CONNECTOR usecase

[PATCH RESEND v9 0/6] MHDP8546 fixes related to DRM_BRIDGE_ATTACH_NO_CONNECTOR usecase

[Harikrishna Shenoy]

With the DRM_BRIDGE_ATTACH_NO_CONNECTOR framework, the connector is 
no longer initialized in  bridge_attach() when the display controller 
sets the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag. 
This causes a null pointer dereference in cdns_mhdp_modeset_retry_fn() 
when trying to access &conn->dev->mode_config.mutex. 
Observed on a board where EDID read failed. 
(log: https://gist.github.com/Jayesh2000/233f87f9becdf1e66f1da6fd53f77429)

Patch 1 adds a connector_ptr which takes care of both 
DRM_BRIDGE_ATTACH_NO_CONNECTOR and !DRM_BRIDGE_ATTACH_NO_CONNECTOR 
case by setting the pointer in appropriate hooks and checking for pointer 
validity before accessing the connector.
Patch 2 adds mode validation hook to bridge fucntions.
Patch 3 fixes HDCP to work with both DRM_BRIDGE_ATTACH_NO_CONNECTOR 
and !DRM_BRIDGE_ATTACH_NO_CONNECTOR case by moving HDCP state handling 
into the bridge atomic check inline with the 
DRM_BRIDGE_ATTACH_NO_CONNECTOR model.
Patches 4,5 do necessary cleanup and alignment for using
connector pointer.

The rationale behind the sequence of commits is we can cleanly 
switch to drm_connector pointer after removal of connector helper 
code blocks, which are anyways not touch after 
DRM_BRIDGE_ATTACH_NO_CONNECTOR has been enabled in driver.

The last patch make smaller adjustment: lowering the log level for
noisy DPCD transfer errors.

v8 patch link:
<https://lore.kernel.org/all/20251014094527.3916421-1-h-shenoy@ti.com/>

Changelog v8-v9:
-Move the patch 6 in v8 related to HDCP to patch 3 and add fixes tag.
-Update to connector_ptr in HDCP code in patch 1.
-Rebased on next-20251114.

v7 patch link:
<https://lore.kernel.org/all/20250929083936.1575685-1-h-shenoy@ti.com/>

Changelog v7-v8:
-Move patches with firxes tag to top of series with appropriate changes
to them.
-Add R/B tag to patch 
https://lore.kernel.org/all/ae3snoap64r252sbqhsshsadxfmlqdfn6b4o5fgfcmxppglkqf@2lsstfsghzwb/

v6 patch link:
<https://lore.kernel.org/all/20250909090824.1655537-1-h-shenoy@ti.com/>

Changelog v6-v7:
-Update cover letter to explain the series.
-Add R/B tag in PATCH 1 and drop fixes tag as suggested.
-Drop fixes tag in PATCH 2.
-Update the commit messages for clear understanding of changes done in patches.

v5 patch link:
<https://lore.kernel.org/all/20250811075904.1613519-1-h-shenoy@ti.com/>

Changelog v5 -> v6:
-Update cover letter to clarify the series in better way.
-Add Reviewed-by tag to relevant patches.
 
v4 patch link: 
<https://lore.kernel.org/all/20250624054448.192801-1-j-choudhary@ti.com>

Changelog v4->v5:
- Handle HDCP state in bridge atomic check instead of connector 
atomic check
 
v3 patch link:
<https://lore.kernel.org/all/20250529142517.188786-1-j-choudhary@ti.com/>

Changelog v3->v4:
- Fix kernel test robot build warning:
  <https://lore.kernel.org/all/202505300201.2s6r12yc-lkp@intel.com/>

v2 patch link:
<https://lore.kernel.org/all/20250521073237.366463-1-j-choudhary@ti.com/>

Changelog v2->v3:
- Add mode_valid in drm_bridge_funcs to a separate patch
- Remove "if (mhdp->connector.dev)" conditions that were missed in v2
- Split out the move of drm_atomic_get_new_connector_for_encoder()
  to a separate patch
- Drop "R-by" considering the changes in v2[1/3]
- Add Fixes tag to first 4 patches:
  commit c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
  This added DBANC flag in tidss while attaching bridge to the encoder
- Drop RFC prefix

v1 patch link:
<https://lore.kernel.org/all/20250116111636.157641-1-j-choudhary@ti.com/>

Changelog v1->v2:
- Remove !DRM_BRIDGE_ATTACH_NO_CONNECTOR entirely
- Add mode_valid in drm_bridge_funcs[0]
- Fix NULL POINTER differently since we cannot access atomic_state
- Reduce log level in cdns_mhdp_transfer call

[0]: https://lore.kernel.org/all/20240530091757.433106-1-j-choudhary@ti.com/

Harikrishna Shenoy (1):
  drm/bridge: cadence: cdns-mhdp8546-core: Handle HDCP state in bridge
    atomic check

Jayesh Choudhary (5):
  drm/bridge: cadence: cdns-mhdp8546-core: Set the mhdp connector
    earlier in atomic_enable()
  drm/bridge: cadence: cdns-mhdp8546-core: Add mode_valid hook to
    drm_bridge_funcs
  drm/bridge: cadence: cdns-mhdp8546-core: Remove legacy support for
    connector initialisation in bridge
  drm/bridge: cadence: cdns-mhdp8546*: Change drm_connector from
    structure to pointer
  drm/bridge: cadence: cdns-mhdp8546-core: Reduce log level for DPCD
    read/write

 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 258 +++++-------------
 .../drm/bridge/cadence/cdns-mhdp8546-core.h   |   2 +-
 .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   |   8 +-
 3 files changed, 72 insertions(+), 196 deletions(-)

-- 
2.34.1

[PATCH RESEND v9 1/6] drm/bridge: cadence: cdns-mhdp8546-core: Set the mhdp connector earlier in atomic_enable()

[Harikrishna Shenoy]

From: Jayesh Choudhary <j-choudhary@ti.com>

In case if we get errors in cdns_mhdp_link_up() or cdns_mhdp_reg_read()
in atomic_enable, we will go to cdns_mhdp_modeset_retry_fn() and will hit
NULL pointer while trying to access the mutex. We need the connector to
be set before that. Unlike in legacy cases with flag
!DRM_BRIDGE_ATTACH_NO_CONNECTOR, we do not have connector initialised
in bridge_attach(), so add the mhdp->connector_ptr in device structure
to handle both cases with DRM_BRIDGE_ATTACH_NO_CONNECTOR and
!DRM_BRIDGE_ATTACH_NO_CONNECTOR, set it in atomic_enable() earlier to
avoid possible NULL pointer dereference in recovery paths like
modeset_retry_fn() with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag set.

Fixes: c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
---
 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 29 ++++++++++---------
 .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  1 +
 .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   | 26 ++++++++++++-----
 3 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 38726ae1bf150..f3076e9cdabbe 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -740,7 +740,7 @@ static void cdns_mhdp_fw_cb(const struct firmware *fw, void *context)
 	bridge_attached = mhdp->bridge_attached;
 	spin_unlock(&mhdp->start_lock);
 	if (bridge_attached) {
-		if (mhdp->connector.dev)
+		if (mhdp->connector_ptr && mhdp->connector_ptr->dev)
 			drm_kms_helper_hotplug_event(mhdp->bridge.dev);
 		else
 			drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
@@ -1636,6 +1636,7 @@ static int cdns_mhdp_connector_init(struct cdns_mhdp_device *mhdp)
 		return ret;
 	}
 
+	mhdp->connector_ptr = conn;
 	drm_connector_helper_add(conn, &cdns_mhdp_conn_helper_funcs);
 
 	ret = drm_display_info_set_bus_formats(&conn->display_info,
@@ -1915,17 +1916,25 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
 	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
 	struct cdns_mhdp_bridge_state *mhdp_state;
 	struct drm_crtc_state *crtc_state;
-	struct drm_connector *connector;
 	struct drm_connector_state *conn_state;
 	struct drm_bridge_state *new_state;
 	const struct drm_display_mode *mode;
 	u32 resp;
-	int ret;
+	int ret = 0;
 
 	dev_dbg(mhdp->dev, "bridge enable\n");
 
 	mutex_lock(&mhdp->link_mutex);
 
+	mhdp->connector_ptr = drm_atomic_get_new_connector_for_encoder(state,
+								       bridge->encoder);
+	if (WARN_ON(!mhdp->connector_ptr))
+		goto out;
+
+	conn_state = drm_atomic_get_new_connector_state(state, mhdp->connector_ptr);
+	if (WARN_ON(!conn_state))
+		goto out;
+
 	if (mhdp->plugged && !mhdp->link_up) {
 		ret = cdns_mhdp_link_up(mhdp);
 		if (ret < 0)
@@ -1945,15 +1954,6 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
 	cdns_mhdp_reg_write(mhdp, CDNS_DPTX_CAR,
 			    resp | CDNS_VIF_CLK_EN | CDNS_VIF_CLK_RSTN);
 
-	connector = drm_atomic_get_new_connector_for_encoder(state,
-							     bridge->encoder);
-	if (WARN_ON(!connector))
-		goto out;
-
-	conn_state = drm_atomic_get_new_connector_state(state, connector);
-	if (WARN_ON(!conn_state))
-		goto out;
-
 	if (mhdp->hdcp_supported &&
 	    mhdp->hw_state == MHDP_HW_READY &&
 	    conn_state->content_protection ==
@@ -2030,6 +2030,7 @@ static void cdns_mhdp_atomic_disable(struct drm_bridge *bridge,
 	if (mhdp->info && mhdp->info->ops && mhdp->info->ops->disable)
 		mhdp->info->ops->disable(mhdp);
 
+	mhdp->connector_ptr = NULL;
 	mutex_unlock(&mhdp->link_mutex);
 }
 
@@ -2296,7 +2297,7 @@ static void cdns_mhdp_modeset_retry_fn(struct work_struct *work)
 
 	mhdp = container_of(work, typeof(*mhdp), modeset_retry_work);
 
-	conn = &mhdp->connector;
+	conn = mhdp->connector_ptr;
 
 	/* Grab the locks before changing connector property */
 	mutex_lock(&conn->dev->mode_config.mutex);
@@ -2373,7 +2374,7 @@ static void cdns_mhdp_hpd_work(struct work_struct *work)
 	int ret;
 
 	ret = cdns_mhdp_update_link_status(mhdp);
-	if (mhdp->connector.dev) {
+	if (mhdp->connector_ptr && mhdp->connector_ptr->dev) {
 		if (ret < 0)
 			schedule_work(&mhdp->modeset_retry_work);
 		else
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
index bad2fc0c73066..a76775c768956 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
@@ -376,6 +376,7 @@ struct cdns_mhdp_device {
 	struct mutex link_mutex;
 
 	struct drm_connector connector;
+	struct drm_connector *connector_ptr;
 	struct drm_bridge bridge;
 
 	struct cdns_mhdp_link link;
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
index 42248f179b69d..5ac2fad2f0078 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
@@ -393,8 +393,10 @@ static int _cdns_mhdp_hdcp_disable(struct cdns_mhdp_device *mhdp)
 {
 	int ret;
 
-	dev_dbg(mhdp->dev, "[%s:%d] HDCP is being disabled...\n",
-		mhdp->connector.name, mhdp->connector.base.id);
+	if (mhdp->connector_ptr) {
+		dev_dbg(mhdp->dev, "[%s:%d] HDCP is being disabled...\n",
+			mhdp->connector_ptr->name, mhdp->connector_ptr->base.id);
+	}
 
 	ret = cdns_mhdp_hdcp_set_config(mhdp, 0, false);
 
@@ -443,9 +445,11 @@ static int cdns_mhdp_hdcp_check_link(struct cdns_mhdp_device *mhdp)
 	if (!ret && hdcp_port_status & HDCP_PORT_STS_AUTH)
 		goto out;
 
-	dev_err(mhdp->dev,
-		"[%s:%d] HDCP link failed, retrying authentication\n",
-		mhdp->connector.name, mhdp->connector.base.id);
+	if (mhdp->connector_ptr) {
+		dev_err(mhdp->dev,
+			"[%s:%d] HDCP link failed, retrying authentication\n",
+			mhdp->connector_ptr->name, mhdp->connector_ptr->base.id);
+	}
 
 	ret = _cdns_mhdp_hdcp_disable(mhdp);
 	if (ret) {
@@ -487,13 +491,19 @@ static void cdns_mhdp_hdcp_prop_work(struct work_struct *work)
 	struct cdns_mhdp_device *mhdp = container_of(hdcp,
 						     struct cdns_mhdp_device,
 						     hdcp);
-	struct drm_device *dev = mhdp->connector.dev;
+	struct drm_device *dev = NULL;
 	struct drm_connector_state *state;
 
+	if (mhdp->connector_ptr)
+		dev = mhdp->connector_ptr->dev;
+
+	if (!dev)
+		return;
+
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 	mutex_lock(&mhdp->hdcp.mutex);
-	if (mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
-		state = mhdp->connector.state;
+	if (mhdp->connector_ptr && mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
+		state = mhdp->connector_ptr->state;
 		state->content_protection = mhdp->hdcp.value;
 	}
 	mutex_unlock(&mhdp->hdcp.mutex);
-- 
2.34.1

[PATCH RESEND v9 2/6] drm/bridge: cadence: cdns-mhdp8546-core: Add mode_valid hook to drm_bridge_funcs

[Harikrishna Shenoy]

From: Jayesh Choudhary <j-choudhary@ti.com>

Add cdns_mhdp_bridge_mode_valid() to check if specific mode is valid for
this bridge or not. In the legacy usecase with
!DRM_BRIDGE_ATTACH_NO_CONNECTOR we were using the hook from
drm_connector_helper_funcs but with DRM_BRIDGE_ATTACH_NO_CONNECTOR
we need to have mode_valid() in drm_bridge_funcs.

Fixes: c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
---
 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index f3076e9cdabbe..7178a01e4d4d8 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -2162,6 +2162,25 @@ static const struct drm_edid *cdns_mhdp_bridge_edid_read(struct drm_bridge *brid
 	return cdns_mhdp_edid_read(mhdp, connector);
 }
 
+static enum drm_mode_status
+cdns_mhdp_bridge_mode_valid(struct drm_bridge *bridge,
+			    const struct drm_display_info *info,
+			    const struct drm_display_mode *mode)
+{
+	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
+
+	mutex_lock(&mhdp->link_mutex);
+
+	if (!cdns_mhdp_bandwidth_ok(mhdp, mode, mhdp->link.num_lanes,
+				    mhdp->link.rate)) {
+		mutex_unlock(&mhdp->link_mutex);
+		return MODE_CLOCK_HIGH;
+	}
+
+	mutex_unlock(&mhdp->link_mutex);
+	return MODE_OK;
+}
+
 static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
 	.atomic_enable = cdns_mhdp_atomic_enable,
 	.atomic_disable = cdns_mhdp_atomic_disable,
@@ -2176,6 +2195,7 @@ static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
 	.edid_read = cdns_mhdp_bridge_edid_read,
 	.hpd_enable = cdns_mhdp_bridge_hpd_enable,
 	.hpd_disable = cdns_mhdp_bridge_hpd_disable,
+	.mode_valid = cdns_mhdp_bridge_mode_valid,
 };
 
 static bool cdns_mhdp_detect_hpd(struct cdns_mhdp_device *mhdp, bool *hpd_pulse)
-- 
2.34.1

[PATCH RESEND v9 3/6] drm/bridge: cadence: cdns-mhdp8546-core: Handle HDCP state in bridge atomic check

[Harikrishna Shenoy]

Now that we have DRM_BRIDGE_ATTACH_NO_CONNECTOR framework, handle the
HDCP state change inbridge atomic check as well to enable correct
functioning for HDCP in both DRM_BRIDGE_ATTACH_NO_CONNECTOR and
!DRM_BRIDGE_ATTACH_NO_CONNECTOR case.

Fixes: 6a3608eae6d33 ("drm: bridge: cdns-mhdp8546: Enable HDCP")
Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
---
 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 7178a01e4d4d8..d944095da4722 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -2123,6 +2123,10 @@ static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
 {
 	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
 	const struct drm_display_mode *mode = &crtc_state->adjusted_mode;
+	struct drm_connector_state *old_state, *new_state;
+	struct drm_atomic_state *state = crtc_state->state;
+	struct drm_connector *conn = mhdp->connector_ptr;
+	u64 old_cp, new_cp;
 
 	mutex_lock(&mhdp->link_mutex);
 
@@ -2142,6 +2146,25 @@ static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
 	if (mhdp->info)
 		bridge_state->input_bus_cfg.flags = *mhdp->info->input_bus_flags;
 
+	if (conn && mhdp->hdcp_supported) {
+		old_state = drm_atomic_get_old_connector_state(state, conn);
+		new_state = drm_atomic_get_new_connector_state(state, conn);
+		old_cp = old_state->content_protection;
+		new_cp = new_state->content_protection;
+
+		if (old_state->hdcp_content_type != new_state->hdcp_content_type &&
+		    new_cp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
+			new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
+			crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
+			crtc_state->mode_changed = true;
+		}
+
+		if (!new_state->crtc) {
+			if (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)
+				new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
+		}
+	}
+
 	mutex_unlock(&mhdp->link_mutex);
 	return 0;
 }
-- 
2.34.1

[PATCH RESEND v9 4/6] drm/bridge: cadence: cdns-mhdp8546-core: Remove legacy support for connector initialisation in bridge

[Harikrishna Shenoy]

From: Jayesh Choudhary <j-choudhary@ti.com>

Now that we have DRM_BRIDGE_ATTACH_NO_CONNECTOR framework, remove the
connector initialisation code as that piece of code is not called
if DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is used.
Only TI K3 platforms consume this driver and tidss (their display
controller) has this flag set. So this legacy support can be dropped.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
---
 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 188 +-----------------
 1 file changed, 10 insertions(+), 178 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index d944095da4722..816d5d87b45fe 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -739,12 +739,8 @@ static void cdns_mhdp_fw_cb(const struct firmware *fw, void *context)
 	spin_lock(&mhdp->start_lock);
 	bridge_attached = mhdp->bridge_attached;
 	spin_unlock(&mhdp->start_lock);
-	if (bridge_attached) {
-		if (mhdp->connector_ptr && mhdp->connector_ptr->dev)
-			drm_kms_helper_hotplug_event(mhdp->bridge.dev);
-		else
-			drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
-	}
+	if (bridge_attached)
+		drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
 }
 
 static int cdns_mhdp_load_firmware(struct cdns_mhdp_device *mhdp)
@@ -1444,56 +1440,6 @@ static const struct drm_edid *cdns_mhdp_edid_read(struct cdns_mhdp_device *mhdp,
 	return drm_edid_read_custom(connector, cdns_mhdp_get_edid_block, mhdp);
 }
 
-static int cdns_mhdp_get_modes(struct drm_connector *connector)
-{
-	struct cdns_mhdp_device *mhdp = connector_to_mhdp(connector);
-	const struct drm_edid *drm_edid;
-	int num_modes;
-
-	if (!mhdp->plugged)
-		return 0;
-
-	drm_edid = cdns_mhdp_edid_read(mhdp, connector);
-
-	drm_edid_connector_update(connector, drm_edid);
-
-	if (!drm_edid) {
-		dev_err(mhdp->dev, "Failed to read EDID\n");
-		return 0;
-	}
-
-	num_modes = drm_edid_connector_add_modes(connector);
-	drm_edid_free(drm_edid);
-
-	/*
-	 * HACK: Warn about unsupported display formats until we deal
-	 *       with them correctly.
-	 */
-	if (connector->display_info.color_formats &&
-	    !(connector->display_info.color_formats &
-	      mhdp->display_fmt.color_format))
-		dev_warn(mhdp->dev,
-			 "%s: No supported color_format found (0x%08x)\n",
-			__func__, connector->display_info.color_formats);
-
-	if (connector->display_info.bpc &&
-	    connector->display_info.bpc < mhdp->display_fmt.bpc)
-		dev_warn(mhdp->dev, "%s: Display bpc only %d < %d\n",
-			 __func__, connector->display_info.bpc,
-			 mhdp->display_fmt.bpc);
-
-	return num_modes;
-}
-
-static int cdns_mhdp_connector_detect(struct drm_connector *conn,
-				      struct drm_modeset_acquire_ctx *ctx,
-				      bool force)
-{
-	struct cdns_mhdp_device *mhdp = connector_to_mhdp(conn);
-
-	return cdns_mhdp_detect(mhdp);
-}
-
 static u32 cdns_mhdp_get_bpp(struct cdns_mhdp_display_fmt *fmt)
 {
 	u32 bpp;
@@ -1547,115 +1493,6 @@ bool cdns_mhdp_bandwidth_ok(struct cdns_mhdp_device *mhdp,
 	return true;
 }
 
-static
-enum drm_mode_status cdns_mhdp_mode_valid(struct drm_connector *conn,
-					  const struct drm_display_mode *mode)
-{
-	struct cdns_mhdp_device *mhdp = connector_to_mhdp(conn);
-
-	mutex_lock(&mhdp->link_mutex);
-
-	if (!cdns_mhdp_bandwidth_ok(mhdp, mode, mhdp->link.num_lanes,
-				    mhdp->link.rate)) {
-		mutex_unlock(&mhdp->link_mutex);
-		return MODE_CLOCK_HIGH;
-	}
-
-	mutex_unlock(&mhdp->link_mutex);
-	return MODE_OK;
-}
-
-static int cdns_mhdp_connector_atomic_check(struct drm_connector *conn,
-					    struct drm_atomic_state *state)
-{
-	struct cdns_mhdp_device *mhdp = connector_to_mhdp(conn);
-	struct drm_connector_state *old_state, *new_state;
-	struct drm_crtc_state *crtc_state;
-	u64 old_cp, new_cp;
-
-	if (!mhdp->hdcp_supported)
-		return 0;
-
-	old_state = drm_atomic_get_old_connector_state(state, conn);
-	new_state = drm_atomic_get_new_connector_state(state, conn);
-	old_cp = old_state->content_protection;
-	new_cp = new_state->content_protection;
-
-	if (old_state->hdcp_content_type != new_state->hdcp_content_type &&
-	    new_cp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
-		new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
-		goto mode_changed;
-	}
-
-	if (!new_state->crtc) {
-		if (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)
-			new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
-		return 0;
-	}
-
-	if (old_cp == new_cp ||
-	    (old_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
-	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED))
-		return 0;
-
-mode_changed:
-	crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
-	crtc_state->mode_changed = true;
-
-	return 0;
-}
-
-static const struct drm_connector_helper_funcs cdns_mhdp_conn_helper_funcs = {
-	.detect_ctx = cdns_mhdp_connector_detect,
-	.get_modes = cdns_mhdp_get_modes,
-	.mode_valid = cdns_mhdp_mode_valid,
-	.atomic_check = cdns_mhdp_connector_atomic_check,
-};
-
-static const struct drm_connector_funcs cdns_mhdp_conn_funcs = {
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-	.reset = drm_atomic_helper_connector_reset,
-	.destroy = drm_connector_cleanup,
-};
-
-static int cdns_mhdp_connector_init(struct cdns_mhdp_device *mhdp)
-{
-	u32 bus_format = MEDIA_BUS_FMT_RGB121212_1X36;
-	struct drm_connector *conn = &mhdp->connector;
-	struct drm_bridge *bridge = &mhdp->bridge;
-	int ret;
-
-	conn->polled = DRM_CONNECTOR_POLL_HPD;
-
-	ret = drm_connector_init(bridge->dev, conn, &cdns_mhdp_conn_funcs,
-				 DRM_MODE_CONNECTOR_DisplayPort);
-	if (ret) {
-		dev_err(mhdp->dev, "Failed to initialize connector with drm\n");
-		return ret;
-	}
-
-	mhdp->connector_ptr = conn;
-	drm_connector_helper_add(conn, &cdns_mhdp_conn_helper_funcs);
-
-	ret = drm_display_info_set_bus_formats(&conn->display_info,
-					       &bus_format, 1);
-	if (ret)
-		return ret;
-
-	ret = drm_connector_attach_encoder(conn, bridge->encoder);
-	if (ret) {
-		dev_err(mhdp->dev, "Failed to attach connector to encoder\n");
-		return ret;
-	}
-
-	if (mhdp->hdcp_supported)
-		ret = drm_connector_attach_content_protection_property(conn, true);
-
-	return ret;
-}
-
 static int cdns_mhdp_attach(struct drm_bridge *bridge,
 			    struct drm_encoder *encoder,
 			    enum drm_bridge_attach_flags flags)
@@ -1672,9 +1509,11 @@ static int cdns_mhdp_attach(struct drm_bridge *bridge,
 		return ret;
 
 	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
-		ret = cdns_mhdp_connector_init(mhdp);
-		if (ret)
-			goto aux_unregister;
+		ret = -EINVAL;
+		dev_err(mhdp->dev,
+			"Connector initialisation not supported in bridge_attach %d\n",
+			ret);
+		goto aux_unregister;
 	}
 
 	spin_lock(&mhdp->start_lock);
@@ -2414,17 +2253,10 @@ static void cdns_mhdp_hpd_work(struct work_struct *work)
 	struct cdns_mhdp_device *mhdp = container_of(work,
 						     struct cdns_mhdp_device,
 						     hpd_work);
-	int ret;
 
-	ret = cdns_mhdp_update_link_status(mhdp);
-	if (mhdp->connector_ptr && mhdp->connector_ptr->dev) {
-		if (ret < 0)
-			schedule_work(&mhdp->modeset_retry_work);
-		else
-			drm_kms_helper_hotplug_event(mhdp->bridge.dev);
-	} else {
-		drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
-	}
+	cdns_mhdp_update_link_status(mhdp);
+
+	drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
 }
 
 static int cdns_mhdp_probe(struct platform_device *pdev)
-- 
2.34.1

[PATCH RESEND v9 5/6] cadence: cdns-mhdp8546*: Change drm_connector from structure to pointer

[Harikrishna Shenoy]

After enabling DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, mhdp->connector is not
initialised during bridge_attach(). The connector is however required in
few driver calls like cdns_mhdp_hdcp_enable() and
cdns_mhdp_modeset_retry_fn().Now that we have dropped the legacy code
which became redundant with introduction of DRM_BRIDGE_ATTACH_NO_CONNECTOR
usecase in driver,we can cleanly switch to drm_connector pointer
instead of structure.

Set it in bridge_enable() and clear it in bridge_disable(),
and make appropriate changes.

This allows us to dynamically set the reference in bridge_enable()
when the connector becomes available and clear it in bridge_disable().
This change is necessary to properly integrate with the
DRM_BRIDGE_ATTACH_NO_CONNECTOR flag set while maintaining all
connector-dependent functionality in the driver.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
---
 .../gpu/drm/bridge/cadence/cdns-mhdp8546-core.c  | 14 +++++++-------
 .../gpu/drm/bridge/cadence/cdns-mhdp8546-core.h  |  3 +--
 .../gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c  | 16 ++++++++--------
 3 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 816d5d87b45fe..002b4be3de674 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -1765,12 +1765,12 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
 
 	mutex_lock(&mhdp->link_mutex);
 
-	mhdp->connector_ptr = drm_atomic_get_new_connector_for_encoder(state,
-								       bridge->encoder);
-	if (WARN_ON(!mhdp->connector_ptr))
+	mhdp->connector = drm_atomic_get_new_connector_for_encoder(state,
+								   bridge->encoder);
+	if (WARN_ON(!mhdp->connector))
 		goto out;
 
-	conn_state = drm_atomic_get_new_connector_state(state, mhdp->connector_ptr);
+	conn_state = drm_atomic_get_new_connector_state(state, mhdp->connector);
 	if (WARN_ON(!conn_state))
 		goto out;
 
@@ -1869,7 +1869,7 @@ static void cdns_mhdp_atomic_disable(struct drm_bridge *bridge,
 	if (mhdp->info && mhdp->info->ops && mhdp->info->ops->disable)
 		mhdp->info->ops->disable(mhdp);
 
-	mhdp->connector_ptr = NULL;
+	mhdp->connector = NULL;
 	mutex_unlock(&mhdp->link_mutex);
 }
 
@@ -1964,7 +1964,7 @@ static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
 	const struct drm_display_mode *mode = &crtc_state->adjusted_mode;
 	struct drm_connector_state *old_state, *new_state;
 	struct drm_atomic_state *state = crtc_state->state;
-	struct drm_connector *conn = mhdp->connector_ptr;
+	struct drm_connector *conn = mhdp->connector;
 	u64 old_cp, new_cp;
 
 	mutex_lock(&mhdp->link_mutex);
@@ -2179,7 +2179,7 @@ static void cdns_mhdp_modeset_retry_fn(struct work_struct *work)
 
 	mhdp = container_of(work, typeof(*mhdp), modeset_retry_work);
 
-	conn = mhdp->connector_ptr;
+	conn = mhdp->connector;
 
 	/* Grab the locks before changing connector property */
 	mutex_lock(&conn->dev->mode_config.mutex);
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
index a76775c768956..b297db53ba283 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
@@ -375,8 +375,7 @@ struct cdns_mhdp_device {
 	 */
 	struct mutex link_mutex;
 
-	struct drm_connector connector;
-	struct drm_connector *connector_ptr;
+	struct drm_connector *connector;
 	struct drm_bridge bridge;
 
 	struct cdns_mhdp_link link;
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
index 5ac2fad2f0078..1d433ad3fe878 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
@@ -393,9 +393,9 @@ static int _cdns_mhdp_hdcp_disable(struct cdns_mhdp_device *mhdp)
 {
 	int ret;
 
-	if (mhdp->connector_ptr) {
+	if (mhdp->connector) {
 		dev_dbg(mhdp->dev, "[%s:%d] HDCP is being disabled...\n",
-			mhdp->connector_ptr->name, mhdp->connector_ptr->base.id);
+			mhdp->connector->name, mhdp->connector->base.id);
 	}
 
 	ret = cdns_mhdp_hdcp_set_config(mhdp, 0, false);
@@ -445,10 +445,10 @@ static int cdns_mhdp_hdcp_check_link(struct cdns_mhdp_device *mhdp)
 	if (!ret && hdcp_port_status & HDCP_PORT_STS_AUTH)
 		goto out;
 
-	if (mhdp->connector_ptr) {
+	if (mhdp->connector) {
 		dev_err(mhdp->dev,
 			"[%s:%d] HDCP link failed, retrying authentication\n",
-			mhdp->connector_ptr->name, mhdp->connector_ptr->base.id);
+			mhdp->connector->name, mhdp->connector->base.id);
 	}
 
 	ret = _cdns_mhdp_hdcp_disable(mhdp);
@@ -494,16 +494,16 @@ static void cdns_mhdp_hdcp_prop_work(struct work_struct *work)
 	struct drm_device *dev = NULL;
 	struct drm_connector_state *state;
 
-	if (mhdp->connector_ptr)
-		dev = mhdp->connector_ptr->dev;
+	if (mhdp->connector)
+		dev = mhdp->connector->dev;
 
 	if (!dev)
 		return;
 
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 	mutex_lock(&mhdp->hdcp.mutex);
-	if (mhdp->connector_ptr && mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
-		state = mhdp->connector_ptr->state;
+	if (mhdp->connector && mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
+		state = mhdp->connector->state;
 		state->content_protection = mhdp->hdcp.value;
 	}
 	mutex_unlock(&mhdp->hdcp.mutex);
-- 
2.34.1

[PATCH RESEND v9 6/6] drm/bridge: cadence: cdns-mhdp8546-core: Reduce log level for DPCD read/write

[Harikrishna Shenoy]

From: Jayesh Choudhary <j-choudhary@ti.com>

Reduce the log level for cdns_mhdp_dpcd_read and cdns_mhdp_dpcd_write
errors in cdns_mhdp_transfer function as in case of failure, there is
flooding of these prints along with other indicators like EDID failure
logs which are fairly intuitive in themselves rendering these error logs
useless.
Also, the caller functions for the cdns_mhdp_transfer in drm_dp_helper.c
(which calls it 32 times), has debug log level in case transfer fails.
So having a superseding log level in cdns_mhdp_transfer seems bad.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 002b4be3de674..120eb7ffe20c0 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -778,7 +778,7 @@ static ssize_t cdns_mhdp_transfer(struct drm_dp_aux *aux,
 			if (!ret)
 				continue;
 
-			dev_err(mhdp->dev,
+			dev_dbg(mhdp->dev,
 				"Failed to write DPCD addr %u\n",
 				msg->address + i);
 
@@ -788,7 +788,7 @@ static ssize_t cdns_mhdp_transfer(struct drm_dp_aux *aux,
 		ret = cdns_mhdp_dpcd_read(mhdp, msg->address,
 					  msg->buffer, msg->size);
 		if (ret) {
-			dev_err(mhdp->dev,
+			dev_dbg(mhdp->dev,
 				"Failed to read DPCD addr %u\n",
 				msg->address);
 
-- 
2.34.1

Re: [PATCH RESEND v9 5/6] cadence: cdns-mhdp8546*: Change drm_connector from structure to pointer

[Tomi Valkeinen]

Hi,

On 20/11/2025 14:14, Harikrishna Shenoy wrote:
> After enabling DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, mhdp->connector is not
> initialised during bridge_attach(). The connector is however required in
> few driver calls like cdns_mhdp_hdcp_enable() and
> cdns_mhdp_modeset_retry_fn().Now that we have dropped the legacy code
> which became redundant with introduction of DRM_BRIDGE_ATTACH_NO_CONNECTOR
> usecase in driver,we can cleanly switch to drm_connector pointer
> instead of structure.
> 
> Set it in bridge_enable() and clear it in bridge_disable(),
> and make appropriate changes.
> 
> This allows us to dynamically set the reference in bridge_enable()
> when the connector becomes available and clear it in bridge_disable().
> This change is necessary to properly integrate with the
> DRM_BRIDGE_ATTACH_NO_CONNECTOR flag set while maintaining all
> connector-dependent functionality in the driver.

Now the code looks fine, but you didn't update the description, which
now looks to be quite wrong for this.

 Tomi

> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
> ---
>  .../gpu/drm/bridge/cadence/cdns-mhdp8546-core.c  | 14 +++++++-------
>  .../gpu/drm/bridge/cadence/cdns-mhdp8546-core.h  |  3 +--
>  .../gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c  | 16 ++++++++--------
>  3 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index 816d5d87b45fe..002b4be3de674 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -1765,12 +1765,12 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
>  
>  	mutex_lock(&mhdp->link_mutex);
>  
> -	mhdp->connector_ptr = drm_atomic_get_new_connector_for_encoder(state,
> -								       bridge->encoder);
> -	if (WARN_ON(!mhdp->connector_ptr))
> +	mhdp->connector = drm_atomic_get_new_connector_for_encoder(state,
> +								   bridge->encoder);
> +	if (WARN_ON(!mhdp->connector))
>  		goto out;
>  
> -	conn_state = drm_atomic_get_new_connector_state(state, mhdp->connector_ptr);
> +	conn_state = drm_atomic_get_new_connector_state(state, mhdp->connector);
>  	if (WARN_ON(!conn_state))
>  		goto out;
>  
> @@ -1869,7 +1869,7 @@ static void cdns_mhdp_atomic_disable(struct drm_bridge *bridge,
>  	if (mhdp->info && mhdp->info->ops && mhdp->info->ops->disable)
>  		mhdp->info->ops->disable(mhdp);
>  
> -	mhdp->connector_ptr = NULL;
> +	mhdp->connector = NULL;
>  	mutex_unlock(&mhdp->link_mutex);
>  }
>  
> @@ -1964,7 +1964,7 @@ static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
>  	const struct drm_display_mode *mode = &crtc_state->adjusted_mode;
>  	struct drm_connector_state *old_state, *new_state;
>  	struct drm_atomic_state *state = crtc_state->state;
> -	struct drm_connector *conn = mhdp->connector_ptr;
> +	struct drm_connector *conn = mhdp->connector;
>  	u64 old_cp, new_cp;
>  
>  	mutex_lock(&mhdp->link_mutex);
> @@ -2179,7 +2179,7 @@ static void cdns_mhdp_modeset_retry_fn(struct work_struct *work)
>  
>  	mhdp = container_of(work, typeof(*mhdp), modeset_retry_work);
>  
> -	conn = mhdp->connector_ptr;
> +	conn = mhdp->connector;
>  
>  	/* Grab the locks before changing connector property */
>  	mutex_lock(&conn->dev->mode_config.mutex);
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> index a76775c768956..b297db53ba283 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> @@ -375,8 +375,7 @@ struct cdns_mhdp_device {
>  	 */
>  	struct mutex link_mutex;
>  
> -	struct drm_connector connector;
> -	struct drm_connector *connector_ptr;
> +	struct drm_connector *connector;
>  	struct drm_bridge bridge;
>  
>  	struct cdns_mhdp_link link;
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> index 5ac2fad2f0078..1d433ad3fe878 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> @@ -393,9 +393,9 @@ static int _cdns_mhdp_hdcp_disable(struct cdns_mhdp_device *mhdp)
>  {
>  	int ret;
>  
> -	if (mhdp->connector_ptr) {
> +	if (mhdp->connector) {
>  		dev_dbg(mhdp->dev, "[%s:%d] HDCP is being disabled...\n",
> -			mhdp->connector_ptr->name, mhdp->connector_ptr->base.id);
> +			mhdp->connector->name, mhdp->connector->base.id);
>  	}
>  
>  	ret = cdns_mhdp_hdcp_set_config(mhdp, 0, false);
> @@ -445,10 +445,10 @@ static int cdns_mhdp_hdcp_check_link(struct cdns_mhdp_device *mhdp)
>  	if (!ret && hdcp_port_status & HDCP_PORT_STS_AUTH)
>  		goto out;
>  
> -	if (mhdp->connector_ptr) {
> +	if (mhdp->connector) {
>  		dev_err(mhdp->dev,
>  			"[%s:%d] HDCP link failed, retrying authentication\n",
> -			mhdp->connector_ptr->name, mhdp->connector_ptr->base.id);
> +			mhdp->connector->name, mhdp->connector->base.id);
>  	}
>  
>  	ret = _cdns_mhdp_hdcp_disable(mhdp);
> @@ -494,16 +494,16 @@ static void cdns_mhdp_hdcp_prop_work(struct work_struct *work)
>  	struct drm_device *dev = NULL;
>  	struct drm_connector_state *state;
>  
> -	if (mhdp->connector_ptr)
> -		dev = mhdp->connector_ptr->dev;
> +	if (mhdp->connector)
> +		dev = mhdp->connector->dev;
>  
>  	if (!dev)
>  		return;
>  
>  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  	mutex_lock(&mhdp->hdcp.mutex);
> -	if (mhdp->connector_ptr && mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> -		state = mhdp->connector_ptr->state;
> +	if (mhdp->connector && mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> +		state = mhdp->connector->state;
>  		state->content_protection = mhdp->hdcp.value;
>  	}
>  	mutex_unlock(&mhdp->hdcp.mutex);

Re: [PATCH RESEND v9 0/6] MHDP8546 fixes related to DRM_BRIDGE_ATTACH_NO_CONNECTOR usecase

[Tomi Valkeinen]

Hi,

On 20/11/2025 14:14, Harikrishna Shenoy wrote:
> With the DRM_BRIDGE_ATTACH_NO_CONNECTOR framework, the connector is 
> no longer initialized in  bridge_attach() when the display controller 
> sets the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag. 
> This causes a null pointer dereference in cdns_mhdp_modeset_retry_fn() 
> when trying to access &conn->dev->mode_config.mutex. 
> Observed on a board where EDID read failed. 
> (log: https://gist.github.com/Jayesh2000/233f87f9becdf1e66f1da6fd53f77429)
> 
> Patch 1 adds a connector_ptr which takes care of both 
> DRM_BRIDGE_ATTACH_NO_CONNECTOR and !DRM_BRIDGE_ATTACH_NO_CONNECTOR 
> case by setting the pointer in appropriate hooks and checking for pointer 
> validity before accessing the connector.
> Patch 2 adds mode validation hook to bridge fucntions.
> Patch 3 fixes HDCP to work with both DRM_BRIDGE_ATTACH_NO_CONNECTOR 
> and !DRM_BRIDGE_ATTACH_NO_CONNECTOR case by moving HDCP state handling 
> into the bridge atomic check inline with the 
> DRM_BRIDGE_ATTACH_NO_CONNECTOR model.
> Patches 4,5 do necessary cleanup and alignment for using
> connector pointer.
> 
> The rationale behind the sequence of commits is we can cleanly 
> switch to drm_connector pointer after removal of connector helper 
> code blocks, which are anyways not touch after 
> DRM_BRIDGE_ATTACH_NO_CONNECTOR has been enabled in driver.
> 
> The last patch make smaller adjustment: lowering the log level for
> noisy DPCD transfer errors.
> 
> v8 patch link:
> <https://lore.kernel.org/all/20251014094527.3916421-1-h-shenoy@ti.com/>
> 
> Changelog v8-v9:
> -Move the patch 6 in v8 related to HDCP to patch 3 and add fixes tag.
> -Update to connector_ptr in HDCP code in patch 1.
> -Rebased on next-20251114.

Don't base on linux-next, except in some quite special circumstances.
Base on latest major version from Linus, or -rc from Linus, or
drm-misc-next. Usually drm-misc-next is a safe choice for DRM patches.

And if you make changes to a series, it's not a "resend" but a new version.

 Tomi

> 
> v7 patch link:
> <https://lore.kernel.org/all/20250929083936.1575685-1-h-shenoy@ti.com/>
> 
> Changelog v7-v8:
> -Move patches with firxes tag to top of series with appropriate changes
> to them.
> -Add R/B tag to patch 
> https://lore.kernel.org/all/ae3snoap64r252sbqhsshsadxfmlqdfn6b4o5fgfcmxppglkqf@2lsstfsghzwb/
> 
> v6 patch link:
> <https://lore.kernel.org/all/20250909090824.1655537-1-h-shenoy@ti.com/>
> 
> Changelog v6-v7:
> -Update cover letter to explain the series.
> -Add R/B tag in PATCH 1 and drop fixes tag as suggested.
> -Drop fixes tag in PATCH 2.
> -Update the commit messages for clear understanding of changes done in patches.
> 
> v5 patch link:
> <https://lore.kernel.org/all/20250811075904.1613519-1-h-shenoy@ti.com/>
> 
> Changelog v5 -> v6:
> -Update cover letter to clarify the series in better way.
> -Add Reviewed-by tag to relevant patches.
>  
> v4 patch link: 
> <https://lore.kernel.org/all/20250624054448.192801-1-j-choudhary@ti.com>
> 
> Changelog v4->v5:
> - Handle HDCP state in bridge atomic check instead of connector 
> atomic check
>  
> v3 patch link:
> <https://lore.kernel.org/all/20250529142517.188786-1-j-choudhary@ti.com/>
> 
> Changelog v3->v4:
> - Fix kernel test robot build warning:
>   <https://lore.kernel.org/all/202505300201.2s6r12yc-lkp@intel.com/>
> 
> v2 patch link:
> <https://lore.kernel.org/all/20250521073237.366463-1-j-choudhary@ti.com/>
> 
> Changelog v2->v3:
> - Add mode_valid in drm_bridge_funcs to a separate patch
> - Remove "if (mhdp->connector.dev)" conditions that were missed in v2
> - Split out the move of drm_atomic_get_new_connector_for_encoder()
>   to a separate patch
> - Drop "R-by" considering the changes in v2[1/3]
> - Add Fixes tag to first 4 patches:
>   commit c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
>   This added DBANC flag in tidss while attaching bridge to the encoder
> - Drop RFC prefix
> 
> v1 patch link:
> <https://lore.kernel.org/all/20250116111636.157641-1-j-choudhary@ti.com/>
> 
> Changelog v1->v2:
> - Remove !DRM_BRIDGE_ATTACH_NO_CONNECTOR entirely
> - Add mode_valid in drm_bridge_funcs[0]
> - Fix NULL POINTER differently since we cannot access atomic_state
> - Reduce log level in cdns_mhdp_transfer call
> 
> [0]: https://lore.kernel.org/all/20240530091757.433106-1-j-choudhary@ti.com/
> 
> Harikrishna Shenoy (1):
>   drm/bridge: cadence: cdns-mhdp8546-core: Handle HDCP state in bridge
>     atomic check
> 
> Jayesh Choudhary (5):
>   drm/bridge: cadence: cdns-mhdp8546-core: Set the mhdp connector
>     earlier in atomic_enable()
>   drm/bridge: cadence: cdns-mhdp8546-core: Add mode_valid hook to
>     drm_bridge_funcs
>   drm/bridge: cadence: cdns-mhdp8546-core: Remove legacy support for
>     connector initialisation in bridge
>   drm/bridge: cadence: cdns-mhdp8546*: Change drm_connector from
>     structure to pointer
>   drm/bridge: cadence: cdns-mhdp8546-core: Reduce log level for DPCD
>     read/write
> 
>  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 258 +++++-------------
>  .../drm/bridge/cadence/cdns-mhdp8546-core.h   |   2 +-
>  .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   |   8 +-
>  3 files changed, 72 insertions(+), 196 deletions(-)
> 

Re: [PATCH RESEND v9 5/6] cadence: cdns-mhdp8546*: Change drm_connector from structure to pointer

[Harikrishna shenoy]

On 21/11/25 17:32, Tomi Valkeinen wrote:
> Now the code looks fine, but you didn't update the description, which
> now looks to be quite wrong for this.

Hi Tomi,

mhdp->connector is still a structure before this commit and changed to 
pointer in this commit, we had introduced a extra connector pointer and 
variable for fix and kind of got renamed to 'connector' from 
'connector_ptr' and added checks in HDCP , will tweak the commit message 
describing these changes.

Thanks.

Re: [PATCH RESEND v9 1/6] drm/bridge: cadence: cdns-mhdp8546-core: Set the mhdp connector earlier in atomic_enable()

[Tomi Valkeinen]

Hi,

On 20/11/2025 14:14, Harikrishna Shenoy wrote:
> From: Jayesh Choudhary <j-choudhary@ti.com>
> 
> In case if we get errors in cdns_mhdp_link_up() or cdns_mhdp_reg_read()
> in atomic_enable, we will go to cdns_mhdp_modeset_retry_fn() and will hit
> NULL pointer while trying to access the mutex. We need the connector to
> be set before that. Unlike in legacy cases with flag
> !DRM_BRIDGE_ATTACH_NO_CONNECTOR, we do not have connector initialised
> in bridge_attach(), so add the mhdp->connector_ptr in device structure
> to handle both cases with DRM_BRIDGE_ATTACH_NO_CONNECTOR and
> !DRM_BRIDGE_ATTACH_NO_CONNECTOR, set it in atomic_enable() earlier to
> avoid possible NULL pointer dereference in recovery paths like
> modeset_retry_fn() with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag set.
> 
> Fixes: c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
> ---
>  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 29 ++++++++++---------
>  .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  1 +
>  .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   | 26 ++++++++++++-----
>  3 files changed, 34 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index 38726ae1bf150..f3076e9cdabbe 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -740,7 +740,7 @@ static void cdns_mhdp_fw_cb(const struct firmware *fw, void *context)
>  	bridge_attached = mhdp->bridge_attached;
>  	spin_unlock(&mhdp->start_lock);
>  	if (bridge_attached) {
> -		if (mhdp->connector.dev)
> +		if (mhdp->connector_ptr && mhdp->connector_ptr->dev)
>  			drm_kms_helper_hotplug_event(mhdp->bridge.dev);
>  		else
>  			drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
> @@ -1636,6 +1636,7 @@ static int cdns_mhdp_connector_init(struct cdns_mhdp_device *mhdp)
>  		return ret;
>  	}
>  
> +	mhdp->connector_ptr = conn;
>  	drm_connector_helper_add(conn, &cdns_mhdp_conn_helper_funcs);
>  
>  	ret = drm_display_info_set_bus_formats(&conn->display_info,
> @@ -1915,17 +1916,25 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
>  	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
>  	struct cdns_mhdp_bridge_state *mhdp_state;
>  	struct drm_crtc_state *crtc_state;
> -	struct drm_connector *connector;
>  	struct drm_connector_state *conn_state;
>  	struct drm_bridge_state *new_state;
>  	const struct drm_display_mode *mode;
>  	u32 resp;
> -	int ret;
> +	int ret = 0;
>  
>  	dev_dbg(mhdp->dev, "bridge enable\n");
>  
>  	mutex_lock(&mhdp->link_mutex);
>  
> +	mhdp->connector_ptr = drm_atomic_get_new_connector_for_encoder(state,
> +								       bridge->encoder);
> +	if (WARN_ON(!mhdp->connector_ptr))
> +		goto out;
> +
> +	conn_state = drm_atomic_get_new_connector_state(state, mhdp->connector_ptr);
> +	if (WARN_ON(!conn_state))
> +		goto out;
> +
>  	if (mhdp->plugged && !mhdp->link_up) {
>  		ret = cdns_mhdp_link_up(mhdp);
>  		if (ret < 0)
> @@ -1945,15 +1954,6 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
>  	cdns_mhdp_reg_write(mhdp, CDNS_DPTX_CAR,
>  			    resp | CDNS_VIF_CLK_EN | CDNS_VIF_CLK_RSTN);
>  
> -	connector = drm_atomic_get_new_connector_for_encoder(state,
> -							     bridge->encoder);
> -	if (WARN_ON(!connector))
> -		goto out;
> -
> -	conn_state = drm_atomic_get_new_connector_state(state, connector);
> -	if (WARN_ON(!conn_state))
> -		goto out;
> -
>  	if (mhdp->hdcp_supported &&
>  	    mhdp->hw_state == MHDP_HW_READY &&
>  	    conn_state->content_protection ==
> @@ -2030,6 +2030,7 @@ static void cdns_mhdp_atomic_disable(struct drm_bridge *bridge,
>  	if (mhdp->info && mhdp->info->ops && mhdp->info->ops->disable)
>  		mhdp->info->ops->disable(mhdp);
>  
> +	mhdp->connector_ptr = NULL;
>  	mutex_unlock(&mhdp->link_mutex);
>  }
>  
> @@ -2296,7 +2297,7 @@ static void cdns_mhdp_modeset_retry_fn(struct work_struct *work)
>  
>  	mhdp = container_of(work, typeof(*mhdp), modeset_retry_work);
>  
> -	conn = &mhdp->connector;
> +	conn = mhdp->connector_ptr;
>  
>  	/* Grab the locks before changing connector property */
>  	mutex_lock(&conn->dev->mode_config.mutex);
> @@ -2373,7 +2374,7 @@ static void cdns_mhdp_hpd_work(struct work_struct *work)
>  	int ret;
>  
>  	ret = cdns_mhdp_update_link_status(mhdp);
> -	if (mhdp->connector.dev) {
> +	if (mhdp->connector_ptr && mhdp->connector_ptr->dev) {
>  		if (ret < 0)
>  			schedule_work(&mhdp->modeset_retry_work);
>  		else
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> index bad2fc0c73066..a76775c768956 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> @@ -376,6 +376,7 @@ struct cdns_mhdp_device {
>  	struct mutex link_mutex;
>  
>  	struct drm_connector connector;
> +	struct drm_connector *connector_ptr;
>  	struct drm_bridge bridge;
>  
>  	struct cdns_mhdp_link link;
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> index 42248f179b69d..5ac2fad2f0078 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> @@ -393,8 +393,10 @@ static int _cdns_mhdp_hdcp_disable(struct cdns_mhdp_device *mhdp)
>  {
>  	int ret;
>  
> -	dev_dbg(mhdp->dev, "[%s:%d] HDCP is being disabled...\n",
> -		mhdp->connector.name, mhdp->connector.base.id);
> +	if (mhdp->connector_ptr) {
> +		dev_dbg(mhdp->dev, "[%s:%d] HDCP is being disabled...\n",
> +			mhdp->connector_ptr->name, mhdp->connector_ptr->base.id);
> +	}
>  
>  	ret = cdns_mhdp_hdcp_set_config(mhdp, 0, false);
>  
> @@ -443,9 +445,11 @@ static int cdns_mhdp_hdcp_check_link(struct cdns_mhdp_device *mhdp)
>  	if (!ret && hdcp_port_status & HDCP_PORT_STS_AUTH)
>  		goto out;
>  
> -	dev_err(mhdp->dev,
> -		"[%s:%d] HDCP link failed, retrying authentication\n",
> -		mhdp->connector.name, mhdp->connector.base.id);
> +	if (mhdp->connector_ptr) {
> +		dev_err(mhdp->dev,
> +			"[%s:%d] HDCP link failed, retrying authentication\n",
> +			mhdp->connector_ptr->name, mhdp->connector_ptr->base.id);
> +	}

This looks hackish... What's the point of printing the connector name
and id anyway? For MST? And if we don't have a connector, is there any
point in doing anything in cdns_mhdp_hdcp_check_link() or the other
functions that have similar prints? Or why would they even be called?
Are they ever called if we don't have a connector?

I think for now, it's simplest to just drop the use of connector_ptr
from the prints, instead of these odd if()s. Or assume that we do have
connector_ptr, if the driver is designed that way.

>  
>  	ret = _cdns_mhdp_hdcp_disable(mhdp);
>  	if (ret) {
> @@ -487,13 +491,19 @@ static void cdns_mhdp_hdcp_prop_work(struct work_struct *work)
>  	struct cdns_mhdp_device *mhdp = container_of(hdcp,
>  						     struct cdns_mhdp_device,
>  						     hdcp);
> -	struct drm_device *dev = mhdp->connector.dev;
> +	struct drm_device *dev = NULL;
>  	struct drm_connector_state *state;
>  
> +	if (mhdp->connector_ptr)
> +		dev = mhdp->connector_ptr->dev;
> +
> +	if (!dev)
> +		return;
> +
>  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  	mutex_lock(&mhdp->hdcp.mutex);
> -	if (mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> -		state = mhdp->connector.state;
> +	if (mhdp->connector_ptr && mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {

Earlier in the function you check if there's a connector, and if not,
dev is NULL, and there's return. So the code can never reach here if
connector_ptr is NULL.

> +		state = mhdp->connector_ptr->state;
>  		state->content_protection = mhdp->hdcp.value;

The only real thing this function does are the two lines above.

So I think you can just :

if (!mhdp->connector_ptr || !mhdp->connector_ptr->dev)
	return;

(or something similar) at the beginning of the function. Or should it be
inside the connection_mutex, this one is a scheduled work...

 Tomi


>  	}
>  	mutex_unlock(&mhdp->hdcp.mutex);

Re: [PATCH RESEND v9 2/6] drm/bridge: cadence: cdns-mhdp8546-core: Add mode_valid hook to drm_bridge_funcs

[Tomi Valkeinen]

Hi,

On 20/11/2025 14:14, Harikrishna Shenoy wrote:
> From: Jayesh Choudhary <j-choudhary@ti.com>
> 
> Add cdns_mhdp_bridge_mode_valid() to check if specific mode is valid for
> this bridge or not. In the legacy usecase with
> !DRM_BRIDGE_ATTACH_NO_CONNECTOR we were using the hook from
> drm_connector_helper_funcs but with DRM_BRIDGE_ATTACH_NO_CONNECTOR
> we need to have mode_valid() in drm_bridge_funcs.

This looks fine, but a fix should always explain what the issue is. What
is the behavior without this patch, if DRM_BRIDGE_ATTACH_NO_CONNECTOR is
set?

 Tomi

> Fixes: c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
> ---
>  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 20 +++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index f3076e9cdabbe..7178a01e4d4d8 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -2162,6 +2162,25 @@ static const struct drm_edid *cdns_mhdp_bridge_edid_read(struct drm_bridge *brid
>  	return cdns_mhdp_edid_read(mhdp, connector);
>  }
>  
> +static enum drm_mode_status
> +cdns_mhdp_bridge_mode_valid(struct drm_bridge *bridge,
> +			    const struct drm_display_info *info,
> +			    const struct drm_display_mode *mode)
> +{
> +	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
> +
> +	mutex_lock(&mhdp->link_mutex);
> +
> +	if (!cdns_mhdp_bandwidth_ok(mhdp, mode, mhdp->link.num_lanes,
> +				    mhdp->link.rate)) {
> +		mutex_unlock(&mhdp->link_mutex);
> +		return MODE_CLOCK_HIGH;
> +	}
> +
> +	mutex_unlock(&mhdp->link_mutex);
> +	return MODE_OK;
> +}
> +
>  static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
>  	.atomic_enable = cdns_mhdp_atomic_enable,
>  	.atomic_disable = cdns_mhdp_atomic_disable,
> @@ -2176,6 +2195,7 @@ static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
>  	.edid_read = cdns_mhdp_bridge_edid_read,
>  	.hpd_enable = cdns_mhdp_bridge_hpd_enable,
>  	.hpd_disable = cdns_mhdp_bridge_hpd_disable,
> +	.mode_valid = cdns_mhdp_bridge_mode_valid,
>  };
>  
>  static bool cdns_mhdp_detect_hpd(struct cdns_mhdp_device *mhdp, bool *hpd_pulse)

Re: [PATCH RESEND v9 3/6] drm/bridge: cadence: cdns-mhdp8546-core: Handle HDCP state in bridge atomic check

[Tomi Valkeinen]

Hi,

On 20/11/2025 14:14, Harikrishna Shenoy wrote:
> Now that we have DRM_BRIDGE_ATTACH_NO_CONNECTOR framework, handle the
> HDCP state change inbridge atomic check as well to enable correct

"in bridge's"

> functioning for HDCP in both DRM_BRIDGE_ATTACH_NO_CONNECTOR and
> !DRM_BRIDGE_ATTACH_NO_CONNECTOR case.

Same thing here. What is the issue? What behavior do you see without
this patch?

 Tomi

> Fixes: 6a3608eae6d33 ("drm: bridge: cdns-mhdp8546: Enable HDCP")
> Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
> ---
>  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index 7178a01e4d4d8..d944095da4722 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -2123,6 +2123,10 @@ static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
>  {
>  	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
>  	const struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> +	struct drm_connector_state *old_state, *new_state;
> +	struct drm_atomic_state *state = crtc_state->state;
> +	struct drm_connector *conn = mhdp->connector_ptr;
> +	u64 old_cp, new_cp;
>  
>  	mutex_lock(&mhdp->link_mutex);
>  
> @@ -2142,6 +2146,25 @@ static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
>  	if (mhdp->info)
>  		bridge_state->input_bus_cfg.flags = *mhdp->info->input_bus_flags;
>  
> +	if (conn && mhdp->hdcp_supported) {
> +		old_state = drm_atomic_get_old_connector_state(state, conn);
> +		new_state = drm_atomic_get_new_connector_state(state, conn);
> +		old_cp = old_state->content_protection;
> +		new_cp = new_state->content_protection;
> +
> +		if (old_state->hdcp_content_type != new_state->hdcp_content_type &&
> +		    new_cp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> +			new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> +			crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
> +			crtc_state->mode_changed = true;
> +		}
> +
> +		if (!new_state->crtc) {
> +			if (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)
> +				new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> +		}
> +	}
> +
>  	mutex_unlock(&mhdp->link_mutex);
>  	return 0;
>  }

Re: [PATCH RESEND v9 1/6] drm/bridge: cadence: cdns-mhdp8546-core: Set the mhdp connector earlier in atomic_enable()

[Tomi Valkeinen]

On 20/11/2025 14:14, Harikrishna Shenoy wrote:
> From: Jayesh Choudhary <j-choudhary@ti.com>
> 
> In case if we get errors in cdns_mhdp_link_up() or cdns_mhdp_reg_read()
> in atomic_enable, we will go to cdns_mhdp_modeset_retry_fn() and will hit
> NULL pointer while trying to access the mutex. We need the connector to
> be set before that. Unlike in legacy cases with flag
> !DRM_BRIDGE_ATTACH_NO_CONNECTOR, we do not have connector initialised
> in bridge_attach(), so add the mhdp->connector_ptr in device structure
> to handle both cases with DRM_BRIDGE_ATTACH_NO_CONNECTOR and
> !DRM_BRIDGE_ATTACH_NO_CONNECTOR, set it in atomic_enable() earlier to
> avoid possible NULL pointer dereference in recovery paths like
> modeset_retry_fn() with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag set.
> 
> Fixes: c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
> ---
>  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 29 ++++++++++---------
>  .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  1 +
>  .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   | 26 ++++++++++++-----
>  3 files changed, 34 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index 38726ae1bf150..f3076e9cdabbe 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -740,7 +740,7 @@ static void cdns_mhdp_fw_cb(const struct firmware *fw, void *context)
>  	bridge_attached = mhdp->bridge_attached;
>  	spin_unlock(&mhdp->start_lock);
>  	if (bridge_attached) {
> -		if (mhdp->connector.dev)
> +		if (mhdp->connector_ptr && mhdp->connector_ptr->dev)
>  			drm_kms_helper_hotplug_event(mhdp->bridge.dev);
>  		else
>  			drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));

This code looks odd, and there are a few other similar if-elses too.
What does this do?

Correct me if I'm wrong but:

With DRM_BRIDGE_ATTACH_NO_CONNECTOR, connector_ptr is set at
cdns_mhdp_atomic_enable(). If connector_ptr is set, I assume we also
always have dev there.

Without DRM_BRIDGE_ATTACH_NO_CONNECTOR, connector_ptr is set at
cdns_mhdp_connector_init(). If it is set, we have called
drm_connector_init(), and dev is valid.

So... If we have connector_ptr, connector_ptr->dev is always set? So no
need to check for the dev.

But overall I'm confused about the 'else' block. Why do we have these
code blocks that (in the current upstream code) check for
'mhdp->connector.dev' (I think that's checking if the
cdns_mhdp_connector_init() has been called), and then, e.g. here, call
either drm_kms_helper_hotplug_event() or drm_bridge_hpd_notify(). I
don't understand what is the idea here.

Also, in the above case, the code checks for 'bridge_attached'. It is
set in cdns_mhdp_attach(), after calling cdns_mhdp_connector_init().
So... If bridge_attached is true, we always have a connector if
DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set. And if
DRM_BRIDGE_ATTACH_NO_CONNECTOR is set, we never have a connector,
because it's set only in atomic enable.

So the 'if' is effectively checking for DRM_BRIDGE_ATTACH_NO_CONNECTOR
flag? It still doesn't explain why we need to check it here.

 Tomi

> @@ -1636,6 +1636,7 @@ static int cdns_mhdp_connector_init(struct cdns_mhdp_device *mhdp)
>  		return ret;
>  	}
>  
> +	mhdp->connector_ptr = conn;
>  	drm_connector_helper_add(conn, &cdns_mhdp_conn_helper_funcs);
>  
>  	ret = drm_display_info_set_bus_formats(&conn->display_info,
> @@ -1915,17 +1916,25 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
>  	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
>  	struct cdns_mhdp_bridge_state *mhdp_state;
>  	struct drm_crtc_state *crtc_state;
> -	struct drm_connector *connector;
>  	struct drm_connector_state *conn_state;
>  	struct drm_bridge_state *new_state;
>  	const struct drm_display_mode *mode;
>  	u32 resp;
> -	int ret;
> +	int ret = 0;
>  
>  	dev_dbg(mhdp->dev, "bridge enable\n");
>  
>  	mutex_lock(&mhdp->link_mutex);
>  
> +	mhdp->connector_ptr = drm_atomic_get_new_connector_for_encoder(state,
> +								       bridge->encoder);
> +	if (WARN_ON(!mhdp->connector_ptr))
> +		goto out;
> +
> +	conn_state = drm_atomic_get_new_connector_state(state, mhdp->connector_ptr);
> +	if (WARN_ON(!conn_state))
> +		goto out;
> +
>  	if (mhdp->plugged && !mhdp->link_up) {
>  		ret = cdns_mhdp_link_up(mhdp);
>  		if (ret < 0)
> @@ -1945,15 +1954,6 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
>  	cdns_mhdp_reg_write(mhdp, CDNS_DPTX_CAR,
>  			    resp | CDNS_VIF_CLK_EN | CDNS_VIF_CLK_RSTN);
>  
> -	connector = drm_atomic_get_new_connector_for_encoder(state,
> -							     bridge->encoder);
> -	if (WARN_ON(!connector))
> -		goto out;
> -
> -	conn_state = drm_atomic_get_new_connector_state(state, connector);
> -	if (WARN_ON(!conn_state))
> -		goto out;
> -
>  	if (mhdp->hdcp_supported &&
>  	    mhdp->hw_state == MHDP_HW_READY &&
>  	    conn_state->content_protection ==
> @@ -2030,6 +2030,7 @@ static void cdns_mhdp_atomic_disable(struct drm_bridge *bridge,
>  	if (mhdp->info && mhdp->info->ops && mhdp->info->ops->disable)
>  		mhdp->info->ops->disable(mhdp);
>  
> +	mhdp->connector_ptr = NULL;
>  	mutex_unlock(&mhdp->link_mutex);
>  }
>  
> @@ -2296,7 +2297,7 @@ static void cdns_mhdp_modeset_retry_fn(struct work_struct *work)
>  
>  	mhdp = container_of(work, typeof(*mhdp), modeset_retry_work);
>  
> -	conn = &mhdp->connector;
> +	conn = mhdp->connector_ptr;
>  
>  	/* Grab the locks before changing connector property */
>  	mutex_lock(&conn->dev->mode_config.mutex);
> @@ -2373,7 +2374,7 @@ static void cdns_mhdp_hpd_work(struct work_struct *work)
>  	int ret;
>  
>  	ret = cdns_mhdp_update_link_status(mhdp);
> -	if (mhdp->connector.dev) {
> +	if (mhdp->connector_ptr && mhdp->connector_ptr->dev) {
>  		if (ret < 0)
>  			schedule_work(&mhdp->modeset_retry_work);
>  		else
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> index bad2fc0c73066..a76775c768956 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> @@ -376,6 +376,7 @@ struct cdns_mhdp_device {
>  	struct mutex link_mutex;
>  
>  	struct drm_connector connector;
> +	struct drm_connector *connector_ptr;
>  	struct drm_bridge bridge;
>  
>  	struct cdns_mhdp_link link;
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> index 42248f179b69d..5ac2fad2f0078 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> @@ -393,8 +393,10 @@ static int _cdns_mhdp_hdcp_disable(struct cdns_mhdp_device *mhdp)
>  {
>  	int ret;
>  
> -	dev_dbg(mhdp->dev, "[%s:%d] HDCP is being disabled...\n",
> -		mhdp->connector.name, mhdp->connector.base.id);
> +	if (mhdp->connector_ptr) {
> +		dev_dbg(mhdp->dev, "[%s:%d] HDCP is being disabled...\n",
> +			mhdp->connector_ptr->name, mhdp->connector_ptr->base.id);
> +	}
>  
>  	ret = cdns_mhdp_hdcp_set_config(mhdp, 0, false);
>  
> @@ -443,9 +445,11 @@ static int cdns_mhdp_hdcp_check_link(struct cdns_mhdp_device *mhdp)
>  	if (!ret && hdcp_port_status & HDCP_PORT_STS_AUTH)
>  		goto out;
>  
> -	dev_err(mhdp->dev,
> -		"[%s:%d] HDCP link failed, retrying authentication\n",
> -		mhdp->connector.name, mhdp->connector.base.id);
> +	if (mhdp->connector_ptr) {
> +		dev_err(mhdp->dev,
> +			"[%s:%d] HDCP link failed, retrying authentication\n",
> +			mhdp->connector_ptr->name, mhdp->connector_ptr->base.id);
> +	}
>  
>  	ret = _cdns_mhdp_hdcp_disable(mhdp);
>  	if (ret) {
> @@ -487,13 +491,19 @@ static void cdns_mhdp_hdcp_prop_work(struct work_struct *work)
>  	struct cdns_mhdp_device *mhdp = container_of(hdcp,
>  						     struct cdns_mhdp_device,
>  						     hdcp);
> -	struct drm_device *dev = mhdp->connector.dev;
> +	struct drm_device *dev = NULL;
>  	struct drm_connector_state *state;
>  
> +	if (mhdp->connector_ptr)
> +		dev = mhdp->connector_ptr->dev;
> +
> +	if (!dev)
> +		return;
> +
>  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  	mutex_lock(&mhdp->hdcp.mutex);
> -	if (mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> -		state = mhdp->connector.state;
> +	if (mhdp->connector_ptr && mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> +		state = mhdp->connector_ptr->state;
>  		state->content_protection = mhdp->hdcp.value;
>  	}
>  	mutex_unlock(&mhdp->hdcp.mutex);

Re: [PATCH RESEND v9 4/6] drm/bridge: cadence: cdns-mhdp8546-core: Remove legacy support for connector initialisation in bridge

[Tomi Valkeinen]

Hi,

On 20/11/2025 14:14, Harikrishna Shenoy wrote:
> From: Jayesh Choudhary <j-choudhary@ti.com>
> 
> Now that we have DRM_BRIDGE_ATTACH_NO_CONNECTOR framework, remove the

I think you could rather say something like "now that this bridge
supports DRM_BRIDGE_ATTACH_NO_CONNECTOR, and tidss, the only user of
this bridge, always sets DRM_BRIDGE_ATTACH_NO_CONNECTOR, we can remove
the legacy code for the non-DRM_BRIDGE_ATTACH_NO_CONNECTOR case"

> connector initialisation code as that piece of code is not called
> if DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is used.
> Only TI K3 platforms consume this driver and tidss (their display
> controller) has this flag set. So this legacy support can be dropped.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
> ---
>  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 188 +-----------------
>  1 file changed, 10 insertions(+), 178 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index d944095da4722..816d5d87b45fe 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -739,12 +739,8 @@ static void cdns_mhdp_fw_cb(const struct firmware *fw, void *context)
>  	spin_lock(&mhdp->start_lock);
>  	bridge_attached = mhdp->bridge_attached;
>  	spin_unlock(&mhdp->start_lock);
> -	if (bridge_attached) {
> -		if (mhdp->connector_ptr && mhdp->connector_ptr->dev)
> -			drm_kms_helper_hotplug_event(mhdp->bridge.dev);
> -		else
> -			drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
> -	}
> +	if (bridge_attached)
> +		drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
>  }
>  
>  static int cdns_mhdp_load_firmware(struct cdns_mhdp_device *mhdp)
> @@ -1444,56 +1440,6 @@ static const struct drm_edid *cdns_mhdp_edid_read(struct cdns_mhdp_device *mhdp,
>  	return drm_edid_read_custom(connector, cdns_mhdp_get_edid_block, mhdp);
>  }
>  
> -static int cdns_mhdp_get_modes(struct drm_connector *connector)
> -{
> -	struct cdns_mhdp_device *mhdp = connector_to_mhdp(connector);
> -	const struct drm_edid *drm_edid;
> -	int num_modes;
> -
> -	if (!mhdp->plugged)
> -		return 0;
> -
> -	drm_edid = cdns_mhdp_edid_read(mhdp, connector);
> -
> -	drm_edid_connector_update(connector, drm_edid);
> -
> -	if (!drm_edid) {
> -		dev_err(mhdp->dev, "Failed to read EDID\n");
> -		return 0;
> -	}
> -
> -	num_modes = drm_edid_connector_add_modes(connector);
> -	drm_edid_free(drm_edid);
> -
> -	/*
> -	 * HACK: Warn about unsupported display formats until we deal
> -	 *       with them correctly.
> -	 */
> -	if (connector->display_info.color_formats &&
> -	    !(connector->display_info.color_formats &
> -	      mhdp->display_fmt.color_format))
> -		dev_warn(mhdp->dev,
> -			 "%s: No supported color_format found (0x%08x)\n",
> -			__func__, connector->display_info.color_formats);
> -
> -	if (connector->display_info.bpc &&
> -	    connector->display_info.bpc < mhdp->display_fmt.bpc)
> -		dev_warn(mhdp->dev, "%s: Display bpc only %d < %d\n",
> -			 __func__, connector->display_info.bpc,
> -			 mhdp->display_fmt.bpc);
> -
> -	return num_modes;
> -}
> -
> -static int cdns_mhdp_connector_detect(struct drm_connector *conn,
> -				      struct drm_modeset_acquire_ctx *ctx,
> -				      bool force)
> -{
> -	struct cdns_mhdp_device *mhdp = connector_to_mhdp(conn);
> -
> -	return cdns_mhdp_detect(mhdp);
> -}
> -
>  static u32 cdns_mhdp_get_bpp(struct cdns_mhdp_display_fmt *fmt)
>  {
>  	u32 bpp;
> @@ -1547,115 +1493,6 @@ bool cdns_mhdp_bandwidth_ok(struct cdns_mhdp_device *mhdp,
>  	return true;
>  }
>  
> -static
> -enum drm_mode_status cdns_mhdp_mode_valid(struct drm_connector *conn,
> -					  const struct drm_display_mode *mode)
> -{
> -	struct cdns_mhdp_device *mhdp = connector_to_mhdp(conn);
> -
> -	mutex_lock(&mhdp->link_mutex);
> -
> -	if (!cdns_mhdp_bandwidth_ok(mhdp, mode, mhdp->link.num_lanes,
> -				    mhdp->link.rate)) {
> -		mutex_unlock(&mhdp->link_mutex);
> -		return MODE_CLOCK_HIGH;
> -	}
> -
> -	mutex_unlock(&mhdp->link_mutex);
> -	return MODE_OK;
> -}
> -
> -static int cdns_mhdp_connector_atomic_check(struct drm_connector *conn,
> -					    struct drm_atomic_state *state)
> -{
> -	struct cdns_mhdp_device *mhdp = connector_to_mhdp(conn);
> -	struct drm_connector_state *old_state, *new_state;
> -	struct drm_crtc_state *crtc_state;
> -	u64 old_cp, new_cp;
> -
> -	if (!mhdp->hdcp_supported)
> -		return 0;
> -
> -	old_state = drm_atomic_get_old_connector_state(state, conn);
> -	new_state = drm_atomic_get_new_connector_state(state, conn);
> -	old_cp = old_state->content_protection;
> -	new_cp = new_state->content_protection;
> -
> -	if (old_state->hdcp_content_type != new_state->hdcp_content_type &&
> -	    new_cp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> -		new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> -		goto mode_changed;
> -	}
> -
> -	if (!new_state->crtc) {
> -		if (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)
> -			new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> -		return 0;
> -	}
> -
> -	if (old_cp == new_cp ||
> -	    (old_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
> -	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED))
> -		return 0;
> -
> -mode_changed:
> -	crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
> -	crtc_state->mode_changed = true;
> -
> -	return 0;
> -}
> -
> -static const struct drm_connector_helper_funcs cdns_mhdp_conn_helper_funcs = {
> -	.detect_ctx = cdns_mhdp_connector_detect,
> -	.get_modes = cdns_mhdp_get_modes,
> -	.mode_valid = cdns_mhdp_mode_valid,
> -	.atomic_check = cdns_mhdp_connector_atomic_check,
> -};
> -
> -static const struct drm_connector_funcs cdns_mhdp_conn_funcs = {
> -	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> -	.reset = drm_atomic_helper_connector_reset,
> -	.destroy = drm_connector_cleanup,
> -};
> -
> -static int cdns_mhdp_connector_init(struct cdns_mhdp_device *mhdp)
> -{
> -	u32 bus_format = MEDIA_BUS_FMT_RGB121212_1X36;
> -	struct drm_connector *conn = &mhdp->connector;
> -	struct drm_bridge *bridge = &mhdp->bridge;
> -	int ret;
> -
> -	conn->polled = DRM_CONNECTOR_POLL_HPD;
> -
> -	ret = drm_connector_init(bridge->dev, conn, &cdns_mhdp_conn_funcs,
> -				 DRM_MODE_CONNECTOR_DisplayPort);
> -	if (ret) {
> -		dev_err(mhdp->dev, "Failed to initialize connector with drm\n");
> -		return ret;
> -	}
> -
> -	mhdp->connector_ptr = conn;
> -	drm_connector_helper_add(conn, &cdns_mhdp_conn_helper_funcs);
> -
> -	ret = drm_display_info_set_bus_formats(&conn->display_info,
> -					       &bus_format, 1);
> -	if (ret)
> -		return ret;
> -
> -	ret = drm_connector_attach_encoder(conn, bridge->encoder);
> -	if (ret) {
> -		dev_err(mhdp->dev, "Failed to attach connector to encoder\n");
> -		return ret;
> -	}
> -
> -	if (mhdp->hdcp_supported)
> -		ret = drm_connector_attach_content_protection_property(conn, true);
> -
> -	return ret;
> -}
> -
>  static int cdns_mhdp_attach(struct drm_bridge *bridge,
>  			    struct drm_encoder *encoder,
>  			    enum drm_bridge_attach_flags flags)
> @@ -1672,9 +1509,11 @@ static int cdns_mhdp_attach(struct drm_bridge *bridge,
>  		return ret;
>  
>  	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> -		ret = cdns_mhdp_connector_init(mhdp);
> -		if (ret)
> -			goto aux_unregister;
> +		ret = -EINVAL;
> +		dev_err(mhdp->dev,
> +			"Connector initialisation not supported in bridge_attach %d\n",
> +			ret);
> +		goto aux_unregister;
>  	}
>  
>  	spin_lock(&mhdp->start_lock);
> @@ -2414,17 +2253,10 @@ static void cdns_mhdp_hpd_work(struct work_struct *work)
>  	struct cdns_mhdp_device *mhdp = container_of(work,
>  						     struct cdns_mhdp_device,
>  						     hpd_work);
> -	int ret;
>  
> -	ret = cdns_mhdp_update_link_status(mhdp);
> -	if (mhdp->connector_ptr && mhdp->connector_ptr->dev) {
> -		if (ret < 0)
> -			schedule_work(&mhdp->modeset_retry_work);
> -		else
> -			drm_kms_helper_hotplug_event(mhdp->bridge.dev);
> -	} else {
> -		drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
> -	}
> +	cdns_mhdp_update_link_status(mhdp);
> +
> +	drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));

This looks odd. The removed code looks odd too, so maybe the issue is in
patch 1 (I replied to that patch again). But assuming the old code is
right, the new code is still different than the old:

Previously cdns_mhdp_update_link_status()'s ret value is used, and
background work is started on error (when we have connector_ptr). Now
the ret value is ignored, no bg work.

Afaics, we can get the hpd_work call with or without connector_ptr, as
we can get HPD when the bridge is enabled or disabled. So in the removed
code both branches can be taken. But the new code only has one of the
branches. And I still don't understand what exactly it's doing.

 Tomi

Re: [PATCH RESEND v9 2/6] drm/bridge: cadence: cdns-mhdp8546-core: Add mode_valid hook to drm_bridge_funcs

[Harikrishna shenoy]

On 21/11/25 18:13, Tomi Valkeinen wrote:
> Hi,
> 
> On 20/11/2025 14:14, Harikrishna Shenoy wrote:
>> From: Jayesh Choudhary <j-choudhary@ti.com>
>>
>> Add cdns_mhdp_bridge_mode_valid() to check if specific mode is valid for
>> this bridge or not. In the legacy usecase with
>> !DRM_BRIDGE_ATTACH_NO_CONNECTOR we were using the hook from
>> drm_connector_helper_funcs but with DRM_BRIDGE_ATTACH_NO_CONNECTOR
>> we need to have mode_valid() in drm_bridge_funcs.
> 
> This looks fine, but a fix should always explain what the issue is. What
> is the behavior without this patch, if DRM_BRIDGE_ATTACH_NO_CONNECTOR is
> set?
> 
>   Tomi
Hi Tomi,

Without this patch some modes which should get rejected based on 
bandwidth requirement will not be rejected as mode check/validation will 
not hit 'cdns_mhdp_bandwidth_ok'as with DRM_BRIDGE_ATTACH_NO_CONNECTOR 
flag set we use hooks from drm_bridge_funcs.

will include this in commit message.

Regards,
Hari

> 
>> Fixes: c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
>> ---
>>   .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 20 +++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> index f3076e9cdabbe..7178a01e4d4d8 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> @@ -2162,6 +2162,25 @@ static const struct drm_edid *cdns_mhdp_bridge_edid_read(struct drm_bridge *brid
>>   	return cdns_mhdp_edid_read(mhdp, connector);
>>   }
>>   
>> +static enum drm_mode_status
>> +cdns_mhdp_bridge_mode_valid(struct drm_bridge *bridge,
>> +			    const struct drm_display_info *info,
>> +			    const struct drm_display_mode *mode)
>> +{
>> +	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
>> +
>> +	mutex_lock(&mhdp->link_mutex);
>> +
>> +	if (!cdns_mhdp_bandwidth_ok(mhdp, mode, mhdp->link.num_lanes,
>> +				    mhdp->link.rate)) {
>> +		mutex_unlock(&mhdp->link_mutex);
>> +		return MODE_CLOCK_HIGH;
>> +	}
>> +
>> +	mutex_unlock(&mhdp->link_mutex);
>> +	return MODE_OK;
>> +}
>> +
>>   static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
>>   	.atomic_enable = cdns_mhdp_atomic_enable,
>>   	.atomic_disable = cdns_mhdp_atomic_disable,
>> @@ -2176,6 +2195,7 @@ static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
>>   	.edid_read = cdns_mhdp_bridge_edid_read,
>>   	.hpd_enable = cdns_mhdp_bridge_hpd_enable,
>>   	.hpd_disable = cdns_mhdp_bridge_hpd_disable,
>> +	.mode_valid = cdns_mhdp_bridge_mode_valid,
>>   };
>>   
>>   static bool cdns_mhdp_detect_hpd(struct cdns_mhdp_device *mhdp, bool *hpd_pulse)
> 

Re: [PATCH RESEND v9 0/6] MHDP8546 fixes related to DRM_BRIDGE_ATTACH_NO_CONNECTOR usecase

[Harikrishna shenoy]

On 21/11/25 17:37, Tomi Valkeinen wrote:
> Hi,
> 
> On 20/11/2025 14:14, Harikrishna Shenoy wrote:
>> With the DRM_BRIDGE_ATTACH_NO_CONNECTOR framework, the connector is
>> no longer initialized in  bridge_attach() when the display controller
>> sets the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
>> This causes a null pointer dereference in cdns_mhdp_modeset_retry_fn()
>> when trying to access &conn->dev->mode_config.mutex.
>> Observed on a board where EDID read failed.
>> (log: https://gist.github.com/Jayesh2000/233f87f9becdf1e66f1da6fd53f77429)
>>
>> Patch 1 adds a connector_ptr which takes care of both
>> DRM_BRIDGE_ATTACH_NO_CONNECTOR and !DRM_BRIDGE_ATTACH_NO_CONNECTOR
>> case by setting the pointer in appropriate hooks and checking for pointer
>> validity before accessing the connector.
>> Patch 2 adds mode validation hook to bridge fucntions.
>> Patch 3 fixes HDCP to work with both DRM_BRIDGE_ATTACH_NO_CONNECTOR
>> and !DRM_BRIDGE_ATTACH_NO_CONNECTOR case by moving HDCP state handling
>> into the bridge atomic check inline with the
>> DRM_BRIDGE_ATTACH_NO_CONNECTOR model.
>> Patches 4,5 do necessary cleanup and alignment for using
>> connector pointer.
>>
>> The rationale behind the sequence of commits is we can cleanly
>> switch to drm_connector pointer after removal of connector helper
>> code blocks, which are anyways not touch after
>> DRM_BRIDGE_ATTACH_NO_CONNECTOR has been enabled in driver.
>>
>> The last patch make smaller adjustment: lowering the log level for
>> noisy DPCD transfer errors.
>>
>> v8 patch link:
>> <https://lore.kernel.org/all/20251014094527.3916421-1-h-shenoy@ti.com/>
>>
>> Changelog v8-v9:
>> -Move the patch 6 in v8 related to HDCP to patch 3 and add fixes tag.
>> -Update to connector_ptr in HDCP code in patch 1.
>> -Rebased on next-20251114.
> 
> Don't base on linux-next, except in some quite special circumstances.
> Base on latest major version from Linus, or -rc from Linus, or
> drm-misc-next. Usually drm-misc-next is a safe choice for DRM patches.
> 
> And if you make changes to a series, it's not a "resend" but a new version.
> 
>   Tomi
> 
Hi Tomi,

Thanks for pointing it out, will re-spin next version v10 and base it on 
drm-misc-next.

Regards.

>>
>> v7 patch link:
>> <https://lore.kernel.org/all/20250929083936.1575685-1-h-shenoy@ti.com/>
>>
>> Changelog v7-v8:
>> -Move patches with firxes tag to top of series with appropriate changes
>> to them.
>> -Add R/B tag to patch
>> https://lore.kernel.org/all/ae3snoap64r252sbqhsshsadxfmlqdfn6b4o5fgfcmxppglkqf@2lsstfsghzwb/
>>
>> v6 patch link:
>> <https://lore.kernel.org/all/20250909090824.1655537-1-h-shenoy@ti.com/>
>>
>> Changelog v6-v7:
>> -Update cover letter to explain the series.
>> -Add R/B tag in PATCH 1 and drop fixes tag as suggested.
>> -Drop fixes tag in PATCH 2.
>> -Update the commit messages for clear understanding of changes done in patches.
>>
>> v5 patch link:
>> <https://lore.kernel.org/all/20250811075904.1613519-1-h-shenoy@ti.com/>
>>
>> Changelog v5 -> v6:
>> -Update cover letter to clarify the series in better way.
>> -Add Reviewed-by tag to relevant patches.
>>   
>> v4 patch link:
>> <https://lore.kernel.org/all/20250624054448.192801-1-j-choudhary@ti.com>
>>
>> Changelog v4->v5:
>> - Handle HDCP state in bridge atomic check instead of connector
>> atomic check
>>   
>> v3 patch link:
>> <https://lore.kernel.org/all/20250529142517.188786-1-j-choudhary@ti.com/>
>>
>> Changelog v3->v4:
>> - Fix kernel test robot build warning:
>>    <https://lore.kernel.org/all/202505300201.2s6r12yc-lkp@intel.com/>
>>
>> v2 patch link:
>> <https://lore.kernel.org/all/20250521073237.366463-1-j-choudhary@ti.com/>
>>
>> Changelog v2->v3:
>> - Add mode_valid in drm_bridge_funcs to a separate patch
>> - Remove "if (mhdp->connector.dev)" conditions that were missed in v2
>> - Split out the move of drm_atomic_get_new_connector_for_encoder()
>>    to a separate patch
>> - Drop "R-by" considering the changes in v2[1/3]
>> - Add Fixes tag to first 4 patches:
>>    commit c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
>>    This added DBANC flag in tidss while attaching bridge to the encoder
>> - Drop RFC prefix
>>
>> v1 patch link:
>> <https://lore.kernel.org/all/20250116111636.157641-1-j-choudhary@ti.com/>
>>
>> Changelog v1->v2:
>> - Remove !DRM_BRIDGE_ATTACH_NO_CONNECTOR entirely
>> - Add mode_valid in drm_bridge_funcs[0]
>> - Fix NULL POINTER differently since we cannot access atomic_state
>> - Reduce log level in cdns_mhdp_transfer call
>>
>> [0]: https://lore.kernel.org/all/20240530091757.433106-1-j-choudhary@ti.com/
>>
>> Harikrishna Shenoy (1):
>>    drm/bridge: cadence: cdns-mhdp8546-core: Handle HDCP state in bridge
>>      atomic check
>>
>> Jayesh Choudhary (5):
>>    drm/bridge: cadence: cdns-mhdp8546-core: Set the mhdp connector
>>      earlier in atomic_enable()
>>    drm/bridge: cadence: cdns-mhdp8546-core: Add mode_valid hook to
>>      drm_bridge_funcs
>>    drm/bridge: cadence: cdns-mhdp8546-core: Remove legacy support for
>>      connector initialisation in bridge
>>    drm/bridge: cadence: cdns-mhdp8546*: Change drm_connector from
>>      structure to pointer
>>    drm/bridge: cadence: cdns-mhdp8546-core: Reduce log level for DPCD
>>      read/write
>>
>>   .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 258 +++++-------------
>>   .../drm/bridge/cadence/cdns-mhdp8546-core.h   |   2 +-
>>   .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   |   8 +-
>>   3 files changed, 72 insertions(+), 196 deletions(-)
>>
> 

Re: [PATCH RESEND v9 1/6] drm/bridge: cadence: cdns-mhdp8546-core: Set the mhdp connector earlier in atomic_enable()

[Harikrishna shenoy]

On 21/11/25 18:11, Tomi Valkeinen wrote:
> Hi,
> 
> On 20/11/2025 14:14, Harikrishna Shenoy wrote:
>> From: Jayesh Choudhary <j-choudhary@ti.com>
>>
>> In case if we get errors in cdns_mhdp_link_up() or cdns_mhdp_reg_read()
>> in atomic_enable, we will go to cdns_mhdp_modeset_retry_fn() and will hit
>> NULL pointer while trying to access the mutex. We need the connector to
>> be set before that. Unlike in legacy cases with flag
>> !DRM_BRIDGE_ATTACH_NO_CONNECTOR, we do not have connector initialised
>> in bridge_attach(), so add the mhdp->connector_ptr in device structure
>> to handle both cases with DRM_BRIDGE_ATTACH_NO_CONNECTOR and
>> !DRM_BRIDGE_ATTACH_NO_CONNECTOR, set it in atomic_enable() earlier to
>> avoid possible NULL pointer dereference in recovery paths like
>> modeset_retry_fn() with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag set.
>>
>> Fixes: c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
>> ---
>>   .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 29 ++++++++++---------
>>   .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  1 +
>>   .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   | 26 ++++++++++++-----
>>   3 files changed, 34 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> index 38726ae1bf150..f3076e9cdabbe 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> @@ -740,7 +740,7 @@ static void cdns_mhdp_fw_cb(const struct firmware *fw, void *context)
>>   	bridge_attached = mhdp->bridge_attached;
>>   	spin_unlock(&mhdp->start_lock);
>>   	if (bridge_attached) {
>> -		if (mhdp->connector.dev)
>> +		if (mhdp->connector_ptr && mhdp->connector_ptr->dev)
>>   			drm_kms_helper_hotplug_event(mhdp->bridge.dev);
>>   		else
>>   			drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
>> @@ -1636,6 +1636,7 @@ static int cdns_mhdp_connector_init(struct cdns_mhdp_device *mhdp)
>>   		return ret;
>>   	}
>>   
>> +	mhdp->connector_ptr = conn;
>>   	drm_connector_helper_add(conn, &cdns_mhdp_conn_helper_funcs);
>>   
>>   	ret = drm_display_info_set_bus_formats(&conn->display_info,
>> @@ -1915,17 +1916,25 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
>>   	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
>>   	struct cdns_mhdp_bridge_state *mhdp_state;
>>   	struct drm_crtc_state *crtc_state;
>> -	struct drm_connector *connector;
>>   	struct drm_connector_state *conn_state;
>>   	struct drm_bridge_state *new_state;
>>   	const struct drm_display_mode *mode;
>>   	u32 resp;
>> -	int ret;
>> +	int ret = 0;
>>   
>>   	dev_dbg(mhdp->dev, "bridge enable\n");
>>   
>>   	mutex_lock(&mhdp->link_mutex);
>>   
>> +	mhdp->connector_ptr = drm_atomic_get_new_connector_for_encoder(state,
>> +								       bridge->encoder);
>> +	if (WARN_ON(!mhdp->connector_ptr))
>> +		goto out;
>> +
>> +	conn_state = drm_atomic_get_new_connector_state(state, mhdp->connector_ptr);
>> +	if (WARN_ON(!conn_state))
>> +		goto out;
>> +
>>   	if (mhdp->plugged && !mhdp->link_up) {
>>   		ret = cdns_mhdp_link_up(mhdp);
>>   		if (ret < 0)
>> @@ -1945,15 +1954,6 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
>>   	cdns_mhdp_reg_write(mhdp, CDNS_DPTX_CAR,
>>   			    resp | CDNS_VIF_CLK_EN | CDNS_VIF_CLK_RSTN);
>>   
>> -	connector = drm_atomic_get_new_connector_for_encoder(state,
>> -							     bridge->encoder);
>> -	if (WARN_ON(!connector))
>> -		goto out;
>> -
>> -	conn_state = drm_atomic_get_new_connector_state(state, connector);
>> -	if (WARN_ON(!conn_state))
>> -		goto out;
>> -
>>   	if (mhdp->hdcp_supported &&
>>   	    mhdp->hw_state == MHDP_HW_READY &&
>>   	    conn_state->content_protection ==
>> @@ -2030,6 +2030,7 @@ static void cdns_mhdp_atomic_disable(struct drm_bridge *bridge,
>>   	if (mhdp->info && mhdp->info->ops && mhdp->info->ops->disable)
>>   		mhdp->info->ops->disable(mhdp);
>>   
>> +	mhdp->connector_ptr = NULL;
>>   	mutex_unlock(&mhdp->link_mutex);
>>   }
>>   
>> @@ -2296,7 +2297,7 @@ static void cdns_mhdp_modeset_retry_fn(struct work_struct *work)
>>   
>>   	mhdp = container_of(work, typeof(*mhdp), modeset_retry_work);
>>   
>> -	conn = &mhdp->connector;
>> +	conn = mhdp->connector_ptr;
>>   
>>   	/* Grab the locks before changing connector property */
>>   	mutex_lock(&conn->dev->mode_config.mutex);
>> @@ -2373,7 +2374,7 @@ static void cdns_mhdp_hpd_work(struct work_struct *work)
>>   	int ret;
>>   
>>   	ret = cdns_mhdp_update_link_status(mhdp);
>> -	if (mhdp->connector.dev) {
>> +	if (mhdp->connector_ptr && mhdp->connector_ptr->dev) {
>>   		if (ret < 0)
>>   			schedule_work(&mhdp->modeset_retry_work);
>>   		else
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>> index bad2fc0c73066..a76775c768956 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>> @@ -376,6 +376,7 @@ struct cdns_mhdp_device {
>>   	struct mutex link_mutex;
>>   
>>   	struct drm_connector connector;
>> +	struct drm_connector *connector_ptr;
>>   	struct drm_bridge bridge;
>>   
>>   	struct cdns_mhdp_link link;
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
>> index 42248f179b69d..5ac2fad2f0078 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
>> @@ -393,8 +393,10 @@ static int _cdns_mhdp_hdcp_disable(struct cdns_mhdp_device *mhdp)
>>   {
>>   	int ret;
>>   
>> -	dev_dbg(mhdp->dev, "[%s:%d] HDCP is being disabled...\n",
>> -		mhdp->connector.name, mhdp->connector.base.id);
>> +	if (mhdp->connector_ptr) {
>> +		dev_dbg(mhdp->dev, "[%s:%d] HDCP is being disabled...\n",
>> +			mhdp->connector_ptr->name, mhdp->connector_ptr->base.id);
>> +	}
>>   
>>   	ret = cdns_mhdp_hdcp_set_config(mhdp, 0, false);
>>   
>> @@ -443,9 +445,11 @@ static int cdns_mhdp_hdcp_check_link(struct cdns_mhdp_device *mhdp)
>>   	if (!ret && hdcp_port_status & HDCP_PORT_STS_AUTH)
>>   		goto out;
>>   
>> -	dev_err(mhdp->dev,
>> -		"[%s:%d] HDCP link failed, retrying authentication\n",
>> -		mhdp->connector.name, mhdp->connector.base.id);
>> +	if (mhdp->connector_ptr) {
>> +		dev_err(mhdp->dev,
>> +			"[%s:%d] HDCP link failed, retrying authentication\n",
>> +			mhdp->connector_ptr->name, mhdp->connector_ptr->base.id);
>> +	}
> 
> This looks hackish... What's the point of printing the connector name
> and id anyway? For MST? And if we don't have a connector, is there any
> point in doing anything in cdns_mhdp_hdcp_check_link() or the other
> functions that have similar prints? Or why would they even be called?
> Are they ever called if we don't have a connector?
> 
> I think for now, it's simplest to just drop the use of connector_ptr
> from the prints, instead of these odd if()s. Or assume that we do have
> connector_ptr, if the driver is designed that way.
> 
I think we should ignore hdcp until connector_ptr is available, as hdcp 
work is initialized in work queue in probe, and a delayed work is 
schduled every DRM_HDCP_CHECK_PERIOD_MS, and connector_ptr is enabled in 
atomic enable so connector_ptr needs to be checked at the start 
respective hdcp functions.
>>   
>>   	ret = _cdns_mhdp_hdcp_disable(mhdp);
>>   	if (ret) {
>> @@ -487,13 +491,19 @@ static void cdns_mhdp_hdcp_prop_work(struct work_struct *work)
>>   	struct cdns_mhdp_device *mhdp = container_of(hdcp,
>>   						     struct cdns_mhdp_device,
>>   						     hdcp);
>> -	struct drm_device *dev = mhdp->connector.dev;
>> +	struct drm_device *dev = NULL;
>>   	struct drm_connector_state *state;
>>   
>> +	if (mhdp->connector_ptr)
>> +		dev = mhdp->connector_ptr->dev;
>> +
>> +	if (!dev)
>> +		return;
>> +
>>   	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>   	mutex_lock(&mhdp->hdcp.mutex);
>> -	if (mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
>> -		state = mhdp->connector.state;
>> +	if (mhdp->connector_ptr && mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> 
> Earlier in the function you check if there's a connector, and if not,
> dev is NULL, and there's return. So the code can never reach here if
> connector_ptr is NULL.
Yes agreed, this is redundancy, will update this.
> 
>> +		state = mhdp->connector_ptr->state;
>>   		state->content_protection = mhdp->hdcp.value;
> 
> The only real thing this function does are the two lines above.
> 
> So I think you can just :
> 
> if (!mhdp->connector_ptr || !mhdp->connector_ptr->dev)
> 	return;
> 
> (or something similar) at the beginning of the function. Or should it be
> inside the connection_mutex, this one is a scheduled work...
> 
>   Tomi
>
Yes, will take this approach for handling connector_ptr in HDCP functions.

Thanks.

> 
>>   	}
>>   	mutex_unlock(&mhdp->hdcp.mutex);
> 

Re: [PATCH RESEND v9 1/6] drm/bridge: cadence: cdns-mhdp8546-core: Set the mhdp connector earlier in atomic_enable()

[Harikrishna shenoy]

On 21/11/25 18:42, Tomi Valkeinen wrote:
> 
> 
> On 20/11/2025 14:14, Harikrishna Shenoy wrote:
>> From: Jayesh Choudhary <j-choudhary@ti.com>
>>
>> In case if we get errors in cdns_mhdp_link_up() or cdns_mhdp_reg_read()
>> in atomic_enable, we will go to cdns_mhdp_modeset_retry_fn() and will hit
>> NULL pointer while trying to access the mutex. We need the connector to
>> be set before that. Unlike in legacy cases with flag
>> !DRM_BRIDGE_ATTACH_NO_CONNECTOR, we do not have connector initialised
>> in bridge_attach(), so add the mhdp->connector_ptr in device structure
>> to handle both cases with DRM_BRIDGE_ATTACH_NO_CONNECTOR and
>> !DRM_BRIDGE_ATTACH_NO_CONNECTOR, set it in atomic_enable() earlier to
>> avoid possible NULL pointer dereference in recovery paths like
>> modeset_retry_fn() with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag set.
>>
>> Fixes: c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
>> ---
>>   .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 29 ++++++++++---------
>>   .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  1 +
>>   .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   | 26 ++++++++++++-----
>>   3 files changed, 34 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> index 38726ae1bf150..f3076e9cdabbe 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> @@ -740,7 +740,7 @@ static void cdns_mhdp_fw_cb(const struct firmware *fw, void *context)
>>   	bridge_attached = mhdp->bridge_attached;
>>   	spin_unlock(&mhdp->start_lock);
>>   	if (bridge_attached) {
>> -		if (mhdp->connector.dev)
>> +		if (mhdp->connector_ptr && mhdp->connector_ptr->dev)
>>   			drm_kms_helper_hotplug_event(mhdp->bridge.dev);
>>   		else
>>   			drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
> 
Hi Tomi,

> This code looks odd, and there are a few other similar if-elses too.
> What does this do?
> 
> Correct me if I'm wrong but:
> 
> With DRM_BRIDGE_ATTACH_NO_CONNECTOR, connector_ptr is set at
> cdns_mhdp_atomic_enable(). If connector_ptr is set, I assume we also
> always have dev there.
> 
> Without DRM_BRIDGE_ATTACH_NO_CONNECTOR, connector_ptr is set at
> cdns_mhdp_connector_init(). If it is set, we have called
> drm_connector_init(), and dev is valid.
> 
> So... If we have connector_ptr, connector_ptr->dev is always set? So no
> need to check for the dev.
Yes this is correct, will remove the redundant checks here.
> 
> But overall I'm confused about the 'else' block. Why do we have these
> code blocks that (in the current upstream code) check for
> 'mhdp->connector.dev' (I think that's checking if the
> cdns_mhdp_connector_init() has been called), and then, e.g. here, call
> either drm_kms_helper_hotplug_event() or drm_bridge_hpd_notify(). I
> don't understand what is the idea here.
> 
For the else block, in code I can find some explanation in 
cdns_mhdp_irq_handler function, but not sure of reasoning behind the
'else' as even in 'if' condition check is for connector but inside the 
block drm_kms_helper_hotplug_event is called with bridge.dev.

> Also, in the above case, the code checks for 'bridge_attached'. It is
> set in cdns_mhdp_attach(), after calling cdns_mhdp_connector_init().
> So... If bridge_attached is true, we always have a connector if
> DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set. And if
> DRM_BRIDGE_ATTACH_NO_CONNECTOR is set, we never have a connector,
> because it's set only in atomic enable.
> 
> So the 'if' is effectively checking for DRM_BRIDGE_ATTACH_NO_CONNECTOR
> flag? It still doesn't explain why we need to check it here.
> 
>   Tomi
> 
>> @@ -1636,6 +1636,7 @@ static int cdns_mhdp_connector_init(struct cdns_mhdp_device *mhdp)
>>   		return ret;
>>   	}
>>   
>> +	mhdp->connector_ptr = conn;
>>   	drm_connector_helper_add(conn, &cdns_mhdp_conn_helper_funcs);
>>   
>>   	ret = drm_display_info_set_bus_formats(&conn->display_info,
>> @@ -1915,17 +1916,25 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
>>   	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
>>   	struct cdns_mhdp_bridge_state *mhdp_state;
>>   	struct drm_crtc_state *crtc_state;
>> -	struct drm_connector *connector;
>>   	struct drm_connector_state *conn_state;
>>   	struct drm_bridge_state *new_state;
>>   	const struct drm_display_mode *mode;
>>   	u32 resp;
>> -	int ret;
>> +	int ret = 0;
>>   
>>   	dev_dbg(mhdp->dev, "bridge enable\n");
>>   
>>   	mutex_lock(&mhdp->link_mutex);
>>   
>> +	mhdp->connector_ptr = drm_atomic_get_new_connector_for_encoder(state,
>> +								       bridge->encoder);
>> +	if (WARN_ON(!mhdp->connector_ptr))
>> +		goto out;
>> +
>> +	conn_state = drm_atomic_get_new_connector_state(state, mhdp->connector_ptr);
>> +	if (WARN_ON(!conn_state))
>> +		goto out;
>> +
>>   	if (mhdp->plugged && !mhdp->link_up) {
>>   		ret = cdns_mhdp_link_up(mhdp);
>>   		if (ret < 0)
>> @@ -1945,15 +1954,6 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
>>   	cdns_mhdp_reg_write(mhdp, CDNS_DPTX_CAR,
>>   			    resp | CDNS_VIF_CLK_EN | CDNS_VIF_CLK_RSTN);
>>   
>> -	connector = drm_atomic_get_new_connector_for_encoder(state,
>> -							     bridge->encoder);
>> -	if (WARN_ON(!connector))
>> -		goto out;
>> -
>> -	conn_state = drm_atomic_get_new_connector_state(state, connector);
>> -	if (WARN_ON(!conn_state))
>> -		goto out;
>> -
>>   	if (mhdp->hdcp_supported &&
>>   	    mhdp->hw_state == MHDP_HW_READY &&
>>   	    conn_state->content_protection ==
>> @@ -2030,6 +2030,7 @@ static void cdns_mhdp_atomic_disable(struct drm_bridge *bridge,
>>   	if (mhdp->info && mhdp->info->ops && mhdp->info->ops->disable)
>>   		mhdp->info->ops->disable(mhdp);
>>   
>> +	mhdp->connector_ptr = NULL;
>>   	mutex_unlock(&mhdp->link_mutex);
>>   }
>>   
>> @@ -2296,7 +2297,7 @@ static void cdns_mhdp_modeset_retry_fn(struct work_struct *work)
>>   
>>   	mhdp = container_of(work, typeof(*mhdp), modeset_retry_work);
>>   
>> -	conn = &mhdp->connector;
>> +	conn = mhdp->connector_ptr;
>>   
>>   	/* Grab the locks before changing connector property */
>>   	mutex_lock(&conn->dev->mode_config.mutex);
>> @@ -2373,7 +2374,7 @@ static void cdns_mhdp_hpd_work(struct work_struct *work)
>>   	int ret;
>>   
>>   	ret = cdns_mhdp_update_link_status(mhdp);
>> -	if (mhdp->connector.dev) {
>> +	if (mhdp->connector_ptr && mhdp->connector_ptr->dev) {
>>   		if (ret < 0)
>>   			schedule_work(&mhdp->modeset_retry_work);
>>   		else
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>> index bad2fc0c73066..a76775c768956 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>> @@ -376,6 +376,7 @@ struct cdns_mhdp_device {
>>   	struct mutex link_mutex;
>>   
>>   	struct drm_connector connector;
>> +	struct drm_connector *connector_ptr;
>>   	struct drm_bridge bridge;
>>   
>>   	struct cdns_mhdp_link link;
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
>> index 42248f179b69d..5ac2fad2f0078 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
>> @@ -393,8 +393,10 @@ static int _cdns_mhdp_hdcp_disable(struct cdns_mhdp_device *mhdp)
>>   {
>>   	int ret;
>>   
>> -	dev_dbg(mhdp->dev, "[%s:%d] HDCP is being disabled...\n",
>> -		mhdp->connector.name, mhdp->connector.base.id);
>> +	if (mhdp->connector_ptr) {
>> +		dev_dbg(mhdp->dev, "[%s:%d] HDCP is being disabled...\n",
>> +			mhdp->connector_ptr->name, mhdp->connector_ptr->base.id);
>> +	}
>>   
>>   	ret = cdns_mhdp_hdcp_set_config(mhdp, 0, false);
>>   
>> @@ -443,9 +445,11 @@ static int cdns_mhdp_hdcp_check_link(struct cdns_mhdp_device *mhdp)
>>   	if (!ret && hdcp_port_status & HDCP_PORT_STS_AUTH)
>>   		goto out;
>>   
>> -	dev_err(mhdp->dev,
>> -		"[%s:%d] HDCP link failed, retrying authentication\n",
>> -		mhdp->connector.name, mhdp->connector.base.id);
>> +	if (mhdp->connector_ptr) {
>> +		dev_err(mhdp->dev,
>> +			"[%s:%d] HDCP link failed, retrying authentication\n",
>> +			mhdp->connector_ptr->name, mhdp->connector_ptr->base.id);
>> +	}
>>   
>>   	ret = _cdns_mhdp_hdcp_disable(mhdp);
>>   	if (ret) {
>> @@ -487,13 +491,19 @@ static void cdns_mhdp_hdcp_prop_work(struct work_struct *work)
>>   	struct cdns_mhdp_device *mhdp = container_of(hdcp,
>>   						     struct cdns_mhdp_device,
>>   						     hdcp);
>> -	struct drm_device *dev = mhdp->connector.dev;
>> +	struct drm_device *dev = NULL;
>>   	struct drm_connector_state *state;
>>   
>> +	if (mhdp->connector_ptr)
>> +		dev = mhdp->connector_ptr->dev;
>> +
>> +	if (!dev)
>> +		return;
>> +
>>   	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>   	mutex_lock(&mhdp->hdcp.mutex);
>> -	if (mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
>> -		state = mhdp->connector.state;
>> +	if (mhdp->connector_ptr && mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
>> +		state = mhdp->connector_ptr->state;
>>   		state->content_protection = mhdp->hdcp.value;
>>   	}
>>   	mutex_unlock(&mhdp->hdcp.mutex);
> 

Re: [PATCH RESEND v9 3/6] drm/bridge: cadence: cdns-mhdp8546-core: Handle HDCP state in bridge atomic check

[Harikrishna shenoy]

On 21/11/25 18:16, Tomi Valkeinen wrote:
> Hi,
> 
> On 20/11/2025 14:14, Harikrishna Shenoy wrote:
>> Now that we have DRM_BRIDGE_ATTACH_NO_CONNECTOR framework, handle the
>> HDCP state change inbridge atomic check as well to enable correct
> 
> "in bridge's"
> 
>> functioning for HDCP in both DRM_BRIDGE_ATTACH_NO_CONNECTOR and
>> !DRM_BRIDGE_ATTACH_NO_CONNECTOR case.
> 
> Same thing here. What is the issue? What behavior do you see without
> this patch?
> 
>   Tomi
> 
Hi Tomi,

This patch update the HDCP state in bridge hooks, as connector hooks
are not used with DBANC, so essentially helps correct functioning of 
HDCP by updating its state.

Regards.
>> Fixes: 6a3608eae6d33 ("drm: bridge: cdns-mhdp8546: Enable HDCP")
>> Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
>> ---
>>   .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 23 +++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> index 7178a01e4d4d8..d944095da4722 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> @@ -2123,6 +2123,10 @@ static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
>>   {
>>   	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
>>   	const struct drm_display_mode *mode = &crtc_state->adjusted_mode;
>> +	struct drm_connector_state *old_state, *new_state;
>> +	struct drm_atomic_state *state = crtc_state->state;
>> +	struct drm_connector *conn = mhdp->connector_ptr;
>> +	u64 old_cp, new_cp;
>>   
>>   	mutex_lock(&mhdp->link_mutex);
>>   
>> @@ -2142,6 +2146,25 @@ static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
>>   	if (mhdp->info)
>>   		bridge_state->input_bus_cfg.flags = *mhdp->info->input_bus_flags;
>>   
>> +	if (conn && mhdp->hdcp_supported) {
>> +		old_state = drm_atomic_get_old_connector_state(state, conn);
>> +		new_state = drm_atomic_get_new_connector_state(state, conn);
>> +		old_cp = old_state->content_protection;
>> +		new_cp = new_state->content_protection;
>> +
>> +		if (old_state->hdcp_content_type != new_state->hdcp_content_type &&
>> +		    new_cp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
>> +			new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
>> +			crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
>> +			crtc_state->mode_changed = true;
>> +		}
>> +
>> +		if (!new_state->crtc) {
>> +			if (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)
>> +				new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
>> +		}
>> +	}
>> +
>>   	mutex_unlock(&mhdp->link_mutex);
>>   	return 0;
>>   }
>