[RFC PATCH v2 0/3] CDNS-MHDP8546 minor cleanups

[RFC PATCH v2 0/3] CDNS-MHDP8546 minor cleanups

[Jayesh Choudhary]

Hello All,

These 3 patches does some fixup for the cdns-mhdp8546 bridge.
- First of all, it removes the legacy !DRM_BRIDGE_ATTACH_NO_CONNECTOR
  usecase.
- Then it fixes possible NULL POINTER in cdns_mhdp_modeset_retry_fn
  function call where the connector mutex is called. Since we cannot
  use the atomic_state in these worker threads, we cannot get to
  connector state in the worker thread. So we ensure that connector
  field is propagated before the first possible call for this worker
  thread by using pointer to the drm_connector.
- Then it reduces log level inside cdns_mhdp_transfer to avoid flooding
  of these logs.

v1 patch:
<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/

Jayesh Choudhary (3):
  drm/bridge: cadence: cdns-mhdp8546-core: Remove legacy support for
    connector initialisation in bridge
  drm/bridge: cadence: cdns-mhdp8546*: Change drm_connector from pointer
    to structure
  drm/bridge: cadence: cdns-mhdp8546-core: Reduce log level for DPCD
    read/write

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

-- 
2.34.1

[RFC PATCH v2 1/3] drm/bridge: cadence: cdns-mhdp8546-core: Remove legacy support for connector initialisation in bridge

[Jayesh Choudhary]

Now that we have DBANC 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.

Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
---
 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 186 +++---------------
 1 file changed, 25 insertions(+), 161 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index b431e7efd1f0..66bd916c2fe9 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -1444,56 +1444,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,114 +1497,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;
-	}
-
-	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)
@@ -1671,9 +1513,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);
@@ -2158,6 +2002,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,
@@ -2172,6 +2035,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

[RFC PATCH v2 2/3] drm/bridge: cadence: cdns-mhdp8546*: Change drm_connector from pointer to structure

[Jayesh Choudhary]

After adding DBANC framework, mhdp->connector is not initialised during
bridge calls. But the asyncronous work scheduled depends on the connector.
We cannot get to drm_atomic_state in these asyncronous calls running on
worker threads. So we need to store the data that we need in mhdp bridge
structure.
Like other bridge drivers, use drm_connector pointer instead of structure
and make appropriate changes to the conditionals and assignments related
to mhdp->connector.
Also, in the atomic enable call, move the connector  and connector state
calls above, so that we do have a connector before we can retry the
asyncronous work in case of any failure.

Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge")
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
---
 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 28 +++++++++----------
 .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  2 +-
 .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   |  8 +++---
 3 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 66bd916c2fe9..5388e62f230b 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)
 			drm_kms_helper_hotplug_event(mhdp->bridge.dev);
 		else
 			drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
@@ -1759,17 +1759,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 = 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);
+	if (WARN_ON(!conn_state))
+		goto out;
+
 	if (mhdp->plugged && !mhdp->link_up) {
 		ret = cdns_mhdp_link_up(mhdp);
 		if (ret < 0)
@@ -1789,15 +1797,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 ==
@@ -1857,6 +1856,7 @@ static void cdns_mhdp_atomic_disable(struct drm_bridge *bridge,
 		cdns_mhdp_hdcp_disable(mhdp);
 
 	mhdp->bridge_enabled = false;
+	mhdp->connector = NULL;
 	cdns_mhdp_reg_read(mhdp, CDNS_DP_FRAMER_GLOBAL_CONFIG, &resp);
 	resp &= ~CDNS_DP_FRAMER_EN;
 	resp |= CDNS_DP_NO_VIDEO_MODE;
@@ -2157,7 +2157,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;
 
 	/* Grab the locks before changing connector property */
 	mutex_lock(&conn->dev->mode_config.mutex);
@@ -2234,7 +2234,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) {
 		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 bad2fc0c7306..b297db53ba28 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
@@ -375,7 +375,7 @@ struct cdns_mhdp_device {
 	 */
 	struct mutex link_mutex;
 
-	struct drm_connector connector;
+	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 42248f179b69..59f18c3281ef 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
@@ -394,7 +394,7 @@ 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);
+		mhdp->connector->name, mhdp->connector->base.id);
 
 	ret = cdns_mhdp_hdcp_set_config(mhdp, 0, false);
 
@@ -445,7 +445,7 @@ static int cdns_mhdp_hdcp_check_link(struct cdns_mhdp_device *mhdp)
 
 	dev_err(mhdp->dev,
 		"[%s:%d] HDCP link failed, retrying authentication\n",
-		mhdp->connector.name, mhdp->connector.base.id);
+		mhdp->connector->name, mhdp->connector->base.id);
 
 	ret = _cdns_mhdp_hdcp_disable(mhdp);
 	if (ret) {
@@ -487,13 +487,13 @@ 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 = mhdp->connector->dev;
 	struct drm_connector_state *state;
 
 	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;
+		state = mhdp->connector->state;
 		state->content_protection = mhdp->hdcp.value;
 	}
 	mutex_unlock(&mhdp->hdcp.mutex);
-- 
2.34.1

[RFC PATCH v2 3/3] drm/bridge: cadence: cdns-mhdp8546-core: Reduce log level for DPCD read/write

[Jayesh Choudhary]

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.

Signed-off-by: Jayesh Choudhary <j-choudhary@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 5388e62f230b..71d35d0d3e74 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -782,7 +782,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);
 
@@ -792,7 +792,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:[RFC PATCH v2 1/3] drm/bridge: cadence: cdns-mhdp8546-core: Remove legacy support for connector initialisation in bridge

[Andy Yan]

Hello Javesh,

At 2025-05-21 15:32:35, "Jayesh Choudhary" <j-choudhary@ti.com> wrote:
>Now that we have DBANC 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.
>
>Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>---
> .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 186 +++---------------
> 1 file changed, 25 insertions(+), 161 deletions(-)
>
>diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>index b431e7efd1f0..66bd916c2fe9 100644
>--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>@@ -1444,56 +1444,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,114 +1497,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;
>-	}
>-
>-	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)
>@@ -1671,9 +1513,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);
>@@ -2158,6 +2002,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,
>@@ -2172,6 +2035,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,

Do we need the detect hook for bridge after cdns_mhdp_connector_detect removed ?



> };
> 
> static bool cdns_mhdp_detect_hpd(struct cdns_mhdp_device *mhdp, bool *hpd_pulse)
>-- 
>2.34.1

Re:Re:[RFC PATCH v2 1/3] drm/bridge: cadence: cdns-mhdp8546-core: Remove legacy support for connector initialisation in bridge

[Andy Yan]

Hello Jayesh,

At 2025-05-21 16:15:33, "Andy Yan" <andyshrk@163.com> wrote:
>
>
>Hello Javesh,
>
>At 2025-05-21 15:32:35, "Jayesh Choudhary" <j-choudhary@ti.com> wrote:
>>Now that we have DBANC 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.
>>
>>Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>---
>> .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 186 +++---------------
>> 1 file changed, 25 insertions(+), 161 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>index b431e7efd1f0..66bd916c2fe9 100644
>>--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>@@ -1444,56 +1444,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,114 +1497,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;
>>-	}
>>-
>>-	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)
>>@@ -1671,9 +1513,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);
>>@@ -2158,6 +2002,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,
>>@@ -2172,6 +2035,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,
>
>Do we need the detect hook for bridge after cdns_mhdp_connector_detect removed ?

Sorry, I didn't notice before. It's already here.


Reviewed-by: Andy Yan <andyshrk@163.com>

>
>
>
>> };
>> 
>> static bool cdns_mhdp_detect_hpd(struct cdns_mhdp_device *mhdp, bool *hpd_pulse)
>>-- 
>>2.34.1

Re: [RFC PATCH v2 1/3] drm/bridge: cadence: cdns-mhdp8546-core: Remove legacy support for connector initialisation in bridge

[Tomi Valkeinen]

Hi,

On 21/05/2025 10:32, Jayesh Choudhary wrote:
> Now that we have DBANC 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.
> 

Why is the series RFC? Does it not work? Is there something here you're
not comfortable with?

> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> ---
>  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 186 +++---------------
>  1 file changed, 25 insertions(+), 161 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index b431e7efd1f0..66bd916c2fe9 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -1444,56 +1444,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,114 +1497,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;
> -	}
> -
> -	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)
> @@ -1671,9 +1513,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);
> @@ -2158,6 +2002,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,
> @@ -2172,6 +2035,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)

Why do you need to add bridge mode_valid() when removing the legacy
non-DRM_BRIDGE_ATTACH_NO_CONNECTOR code?

 Tomi

Re: [RFC PATCH v2 2/3] drm/bridge: cadence: cdns-mhdp8546*: Change drm_connector from pointer to structure

[Tomi Valkeinen]

Hi,

On 21/05/2025 10:32, Jayesh Choudhary wrote:
> After adding DBANC framework, mhdp->connector is not initialised during
> bridge calls. But the asyncronous work scheduled depends on the connector.
> We cannot get to drm_atomic_state in these asyncronous calls running on
> worker threads. So we need to store the data that we need in mhdp bridge
> structure.
> Like other bridge drivers, use drm_connector pointer instead of structure
> and make appropriate changes to the conditionals and assignments related
> to mhdp->connector.
> Also, in the atomic enable call, move the connector  and connector state
> calls above, so that we do have a connector before we can retry the
> asyncronous work in case of any failure.
> 

I don't quite understand this patch. You change the mhdp->connector to a
pointer, which is set at bridge_enable and cleared at bridge_disable.
Then you change the "mhdp->connector.dev" checks to "mhdp->connector".

So, now in e.g. cdns_mhdp_fw_cb(), we check for mhdp->connector, which
is set at bridge_enable(). Can we ever have the bridge enabled before
the fb has been loaded? What is the check even supposed to do there?

Another in cdns_mhdp_hpd_work(), it checks for mhdp->connector. So...
HPD code behaves differently based on if the bridge has been enabled or
not? What is it supposed to do?

Isn't the whole "if (mhdp->connector.dev)" code for the legacy
non-DRM_BRIDGE_ATTACH_NO_CONNECTOR case?

 Tomi

> Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge")
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> ---
>  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 28 +++++++++----------
>  .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  2 +-
>  .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   |  8 +++---
>  3 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index 66bd916c2fe9..5388e62f230b 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)
>  			drm_kms_helper_hotplug_event(mhdp->bridge.dev);
>  		else
>  			drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
> @@ -1759,17 +1759,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 = 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);
> +	if (WARN_ON(!conn_state))
> +		goto out;
> +
>  	if (mhdp->plugged && !mhdp->link_up) {
>  		ret = cdns_mhdp_link_up(mhdp);
>  		if (ret < 0)
> @@ -1789,15 +1797,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 ==
> @@ -1857,6 +1856,7 @@ static void cdns_mhdp_atomic_disable(struct drm_bridge *bridge,
>  		cdns_mhdp_hdcp_disable(mhdp);
>  
>  	mhdp->bridge_enabled = false;
> +	mhdp->connector = NULL;
>  	cdns_mhdp_reg_read(mhdp, CDNS_DP_FRAMER_GLOBAL_CONFIG, &resp);
>  	resp &= ~CDNS_DP_FRAMER_EN;
>  	resp |= CDNS_DP_NO_VIDEO_MODE;
> @@ -2157,7 +2157,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;
>  
>  	/* Grab the locks before changing connector property */
>  	mutex_lock(&conn->dev->mode_config.mutex);
> @@ -2234,7 +2234,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) {
>  		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 bad2fc0c7306..b297db53ba28 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> @@ -375,7 +375,7 @@ struct cdns_mhdp_device {
>  	 */
>  	struct mutex link_mutex;
>  
> -	struct drm_connector connector;
> +	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 42248f179b69..59f18c3281ef 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> @@ -394,7 +394,7 @@ 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);
> +		mhdp->connector->name, mhdp->connector->base.id);
>  
>  	ret = cdns_mhdp_hdcp_set_config(mhdp, 0, false);
>  
> @@ -445,7 +445,7 @@ static int cdns_mhdp_hdcp_check_link(struct cdns_mhdp_device *mhdp)
>  
>  	dev_err(mhdp->dev,
>  		"[%s:%d] HDCP link failed, retrying authentication\n",
> -		mhdp->connector.name, mhdp->connector.base.id);
> +		mhdp->connector->name, mhdp->connector->base.id);
>  
>  	ret = _cdns_mhdp_hdcp_disable(mhdp);
>  	if (ret) {
> @@ -487,13 +487,13 @@ 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 = mhdp->connector->dev;
>  	struct drm_connector_state *state;
>  
>  	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;
> +		state = mhdp->connector->state;
>  		state->content_protection = mhdp->hdcp.value;
>  	}
>  	mutex_unlock(&mhdp->hdcp.mutex);

Re: [RFC PATCH v2 1/3] drm/bridge: cadence: cdns-mhdp8546-core: Remove legacy support for connector initialisation in bridge

[Jayesh Choudhary]

Hello Tomi,

On 27/05/25 13:08, Tomi Valkeinen wrote:
> Hi,
> 
> On 21/05/2025 10:32, Jayesh Choudhary wrote:
>> Now that we have DBANC 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.
>>
> 
> Why is the series RFC? Does it not work? Is there something here you're
> not comfortable with?

These changes work without any issue.

I was a little doubtful about the second patch so kept it as RFC.

> 
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> ---
>>   .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 186 +++---------------
>>   1 file changed, 25 insertions(+), 161 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> index b431e7efd1f0..66bd916c2fe9 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> @@ -1444,56 +1444,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,114 +1497,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;
>> -	}
>> -
>> -	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)
>> @@ -1671,9 +1513,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);
>> @@ -2158,6 +2002,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,
>> @@ -2172,6 +2035,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)
> 
> Why do you need to add bridge mode_valid() when removing the legacy
> non-DRM_BRIDGE_ATTACH_NO_CONNECTOR code?

Okay. Then I will add the bridge mode_valid as a separate patch.

Warm Regards,
Jayesh

> 
>   Tomi
> 

Re: [RFC PATCH v2 1/3] drm/bridge: cadence: cdns-mhdp8546-core: Remove legacy support for connector initialisation in bridge

[Tomi Valkeinen]

Hi,

On 27/05/2025 11:41, Jayesh Choudhary wrote:
> Hello Tomi,
> 
> On 27/05/25 13:08, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 21/05/2025 10:32, Jayesh Choudhary wrote:
>>> Now that we have DBANC 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.
>>>
>>
>> Why is the series RFC? Does it not work? Is there something here you're
>> not comfortable with?
> 
> These changes work without any issue.
> 
> I was a little doubtful about the second patch so kept it as RFC.

You should explain why the series/patch is RFC, unless it's somehow
obvious. In this series I didn't see any TODOs or open questions or
anything.

>>
>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>> ---
>>>   .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 186 +++---------------
>>>   1 file changed, 25 insertions(+), 161 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/
>>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>> index b431e7efd1f0..66bd916c2fe9 100644
>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>> @@ -1444,56 +1444,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,114 +1497,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;
>>> -    }
>>> -
>>> -    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)
>>> @@ -1671,9 +1513,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);
>>> @@ -2158,6 +2002,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,
>>> @@ -2172,6 +2035,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)
>>
>> Why do you need to add bridge mode_valid() when removing the legacy
>> non-DRM_BRIDGE_ATTACH_NO_CONNECTOR code?
> 
> Okay. Then I will add the bridge mode_valid as a separate patch.

Well, it was a question =). But indeed my thought was that it sounded
like material for a separate patch. Is it more of a fix for the
DRM_BRIDGE_ATTACH_NO_CONNECTOR case?

 Tomi

Re: [RFC PATCH v2 2/3] drm/bridge: cadence: cdns-mhdp8546*: Change drm_connector from pointer to structure

[Jayesh Choudhary]

Hello Tomi,

On 27/05/25 13:28, Tomi Valkeinen wrote:
> Hi,
> 
> On 21/05/2025 10:32, Jayesh Choudhary wrote:
>> After adding DBANC framework, mhdp->connector is not initialised during
>> bridge calls. But the asyncronous work scheduled depends on the connector.
>> We cannot get to drm_atomic_state in these asyncronous calls running on
>> worker threads. So we need to store the data that we need in mhdp bridge
>> structure.
>> Like other bridge drivers, use drm_connector pointer instead of structure
>> and make appropriate changes to the conditionals and assignments related
>> to mhdp->connector.
>> Also, in the atomic enable call, move the connector  and connector state
>> calls above, so that we do have a connector before we can retry the
>> asyncronous work in case of any failure.
>>
> 
> I don't quite understand this patch. You change the mhdp->connector to a
> pointer, which is set at bridge_enable and cleared at bridge_disable.
> Then you change the "mhdp->connector.dev" checks to "mhdp->connector".
> 
> So, now in e.g. cdns_mhdp_fw_cb(), we check for mhdp->connector, which
> is set at bridge_enable(). Can we ever have the bridge enabled before
> the fb has been loaded? What is the check even supposed to do there?
> 
> Another in cdns_mhdp_hpd_work(), it checks for mhdp->connector. So...
> HPD code behaves differently based on if the bridge has been enabled or
> not? What is it supposed to do?
> 
> Isn't the whole "if (mhdp->connector.dev)" code for the legacy
> non-DRM_BRIDGE_ATTACH_NO_CONNECTOR case?
> 
>   Tomi

I misinterpreted your comment in v1[0] regarding finding the connector
from the current state in cdns_mhdp_modeset_retry_fn() and I missed
this. I was more focused on finding a connector for that function.

For the current code, in all the conditionals involving mhdp->connector,
we are entering else statements as connector is not initialised.
So I will just drop if statements in cdns_mhdp_fw_cb() and
cdns_mhdp_hpd_work() (like you said, its legacy case) while still having
mhdp->connector as pointer as we need it for
cdns_mhdp_modeset_retry_fn() and in cdns-mhdp8546-hdcp driver.

That should be okay?

[0]: 
https://lore.kernel.org/all/e76f94b9-b138-46e7-bb18-b33dd98c9abb@ideasonboard.com/

Warm Regards,
Jayesh


> 
>> Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge")
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> ---
>>   .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 28 +++++++++----------
>>   .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  2 +-
>>   .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   |  8 +++---
>>   3 files changed, 19 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> index 66bd916c2fe9..5388e62f230b 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)
>>   			drm_kms_helper_hotplug_event(mhdp->bridge.dev);
>>   		else
>>   			drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
>> @@ -1759,17 +1759,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 = 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);
>> +	if (WARN_ON(!conn_state))
>> +		goto out;
>> +
>>   	if (mhdp->plugged && !mhdp->link_up) {
>>   		ret = cdns_mhdp_link_up(mhdp);
>>   		if (ret < 0)
>> @@ -1789,15 +1797,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 ==
>> @@ -1857,6 +1856,7 @@ static void cdns_mhdp_atomic_disable(struct drm_bridge *bridge,
>>   		cdns_mhdp_hdcp_disable(mhdp);
>>   
>>   	mhdp->bridge_enabled = false;
>> +	mhdp->connector = NULL;
>>   	cdns_mhdp_reg_read(mhdp, CDNS_DP_FRAMER_GLOBAL_CONFIG, &resp);
>>   	resp &= ~CDNS_DP_FRAMER_EN;
>>   	resp |= CDNS_DP_NO_VIDEO_MODE;
>> @@ -2157,7 +2157,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;
>>   
>>   	/* Grab the locks before changing connector property */
>>   	mutex_lock(&conn->dev->mode_config.mutex);
>> @@ -2234,7 +2234,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) {
>>   		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 bad2fc0c7306..b297db53ba28 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>> @@ -375,7 +375,7 @@ struct cdns_mhdp_device {
>>   	 */
>>   	struct mutex link_mutex;
>>   
>> -	struct drm_connector connector;
>> +	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 42248f179b69..59f18c3281ef 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
>> @@ -394,7 +394,7 @@ 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);
>> +		mhdp->connector->name, mhdp->connector->base.id);
>>   
>>   	ret = cdns_mhdp_hdcp_set_config(mhdp, 0, false);
>>   
>> @@ -445,7 +445,7 @@ static int cdns_mhdp_hdcp_check_link(struct cdns_mhdp_device *mhdp)
>>   
>>   	dev_err(mhdp->dev,
>>   		"[%s:%d] HDCP link failed, retrying authentication\n",
>> -		mhdp->connector.name, mhdp->connector.base.id);
>> +		mhdp->connector->name, mhdp->connector->base.id);
>>   
>>   	ret = _cdns_mhdp_hdcp_disable(mhdp);
>>   	if (ret) {
>> @@ -487,13 +487,13 @@ 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 = mhdp->connector->dev;
>>   	struct drm_connector_state *state;
>>   
>>   	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;
>> +		state = mhdp->connector->state;
>>   		state->content_protection = mhdp->hdcp.value;
>>   	}
>>   	mutex_unlock(&mhdp->hdcp.mutex);
> 

Re: [RFC PATCH v2 2/3] drm/bridge: cadence: cdns-mhdp8546*: Change drm_connector from pointer to structure

[Jayesh Choudhary]

On 27/05/25 14:59, Jayesh Choudhary wrote:
> Hello Tomi,
> 
> On 27/05/25 13:28, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 21/05/2025 10:32, Jayesh Choudhary wrote:
>>> After adding DBANC framework, mhdp->connector is not initialised during
>>> bridge calls. But the asyncronous work scheduled depends on the 
>>> connector.
>>> We cannot get to drm_atomic_state in these asyncronous calls running on
>>> worker threads. So we need to store the data that we need in mhdp bridge
>>> structure.
>>> Like other bridge drivers, use drm_connector pointer instead of 
>>> structure
>>> and make appropriate changes to the conditionals and assignments related
>>> to mhdp->connector.
>>> Also, in the atomic enable call, move the connector  and connector state
>>> calls above, so that we do have a connector before we can retry the
>>> asyncronous work in case of any failure.
>>>
>>
>> I don't quite understand this patch. You change the mhdp->connector to a
>> pointer, which is set at bridge_enable and cleared at bridge_disable.
>> Then you change the "mhdp->connector.dev" checks to "mhdp->connector".
>>
>> So, now in e.g. cdns_mhdp_fw_cb(), we check for mhdp->connector, which
>> is set at bridge_enable(). Can we ever have the bridge enabled before
>> the fb has been loaded? What is the check even supposed to do there?
>>
>> Another in cdns_mhdp_hpd_work(), it checks for mhdp->connector. So...
>> HPD code behaves differently based on if the bridge has been enabled or
>> not? What is it supposed to do?
>>
>> Isn't the whole "if (mhdp->connector.dev)" code for the legacy
>> non-DRM_BRIDGE_ATTACH_NO_CONNECTOR case?
>>
>>   Tomi
> 
> I misinterpreted your comment in v1[0] regarding finding the connector
> from the current state in cdns_mhdp_modeset_retry_fn() and I missed
> this. I was more focused on finding a connector for that function.
> 
> For the current code, in all the conditionals involving mhdp->connector,
> we are entering else statements as connector is not initialised.
> So I will just drop if statements in cdns_mhdp_fw_cb() and
> cdns_mhdp_hpd_work() (like you said, its legacy case) while still having
> mhdp->connector as pointer as we need it for
> cdns_mhdp_modeset_retry_fn() and in cdns-mhdp8546-hdcp driver.
> 
> That should be okay?
> 
> [0]: 
> https://lore.kernel.org/all/e76f94b9-b138-46e7-bb18-b33dd98c9abb@ideasonboard.com/
> 
> Warm Regards,
> Jayesh
> 
> 

Tomi,

One more thing here. Should this be squashed with the first patch as
this is sort of removing !(DRM_BRIDGE_ATTACH_NO_CONNECTOR) case and
associated changes?


>>
>>> Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546 
>>> DPI/DP bridge")
>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>> ---
>>>   .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 28 +++++++++----------
>>>   .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  2 +-
>>>   .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   |  8 +++---
>>>   3 files changed, 19 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c 
>>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>> index 66bd916c2fe9..5388e62f230b 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)
>>>               drm_kms_helper_hotplug_event(mhdp->bridge.dev);
>>>           else
>>>               drm_bridge_hpd_notify(&mhdp->bridge, 
>>> cdns_mhdp_detect(mhdp));
>>> @@ -1759,17 +1759,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 = 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);
>>> +    if (WARN_ON(!conn_state))
>>> +        goto out;
>>> +
>>>       if (mhdp->plugged && !mhdp->link_up) {
>>>           ret = cdns_mhdp_link_up(mhdp);
>>>           if (ret < 0)
>>> @@ -1789,15 +1797,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 ==
>>> @@ -1857,6 +1856,7 @@ static void cdns_mhdp_atomic_disable(struct 
>>> drm_bridge *bridge,
>>>           cdns_mhdp_hdcp_disable(mhdp);
>>>       mhdp->bridge_enabled = false;
>>> +    mhdp->connector = NULL;
>>>       cdns_mhdp_reg_read(mhdp, CDNS_DP_FRAMER_GLOBAL_CONFIG, &resp);
>>>       resp &= ~CDNS_DP_FRAMER_EN;
>>>       resp |= CDNS_DP_NO_VIDEO_MODE;
>>> @@ -2157,7 +2157,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;
>>>       /* Grab the locks before changing connector property */
>>>       mutex_lock(&conn->dev->mode_config.mutex);
>>> @@ -2234,7 +2234,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) {
>>>           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 bad2fc0c7306..b297db53ba28 100644
>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>>> @@ -375,7 +375,7 @@ struct cdns_mhdp_device {
>>>        */
>>>       struct mutex link_mutex;
>>> -    struct drm_connector connector;
>>> +    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 42248f179b69..59f18c3281ef 100644
>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
>>> @@ -394,7 +394,7 @@ 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);
>>> +        mhdp->connector->name, mhdp->connector->base.id);
>>>       ret = cdns_mhdp_hdcp_set_config(mhdp, 0, false);
>>> @@ -445,7 +445,7 @@ static int cdns_mhdp_hdcp_check_link(struct 
>>> cdns_mhdp_device *mhdp)
>>>       dev_err(mhdp->dev,
>>>           "[%s:%d] HDCP link failed, retrying authentication\n",
>>> -        mhdp->connector.name, mhdp->connector.base.id);
>>> +        mhdp->connector->name, mhdp->connector->base.id);
>>>       ret = _cdns_mhdp_hdcp_disable(mhdp);
>>>       if (ret) {
>>> @@ -487,13 +487,13 @@ 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 = mhdp->connector->dev;
>>>       struct drm_connector_state *state;
>>>       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;
>>> +        state = mhdp->connector->state;
>>>           state->content_protection = mhdp->hdcp.value;
>>>       }
>>>       mutex_unlock(&mhdp->hdcp.mutex);
>>

Re: [RFC PATCH v2 1/3] drm/bridge: cadence: cdns-mhdp8546-core: Remove legacy support for connector initialisation in bridge

[Jayesh Choudhary]

On 27/05/25 14:47, Tomi Valkeinen wrote:
> Hi,
> 
> On 27/05/2025 11:41, Jayesh Choudhary wrote:
>> Hello Tomi,
>>
>> On 27/05/25 13:08, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On 21/05/2025 10:32, Jayesh Choudhary wrote:
>>>> Now that we have DBANC 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.
>>>>
>>>
>>> Why is the series RFC? Does it not work? Is there something here you're
>>> not comfortable with?
>>
>> These changes work without any issue.
>>
>> I was a little doubtful about the second patch so kept it as RFC.
> 
> You should explain why the series/patch is RFC, unless it's somehow
> obvious. In this series I didn't see any TODOs or open questions or
> anything.
> 

Oh okay. Got it. Will drop RFC prefix in v3.

>>>
>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>> ---
>>>>    .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 186 +++---------------
>>>>    1 file changed, 25 insertions(+), 161 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/
>>>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>> index b431e7efd1f0..66bd916c2fe9 100644
>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>> @@ -1444,56 +1444,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,114 +1497,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;
>>>> -    }
>>>> -
>>>> -    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)
>>>> @@ -1671,9 +1513,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);
>>>> @@ -2158,6 +2002,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,
>>>> @@ -2172,6 +2035,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)
>>>
>>> Why do you need to add bridge mode_valid() when removing the legacy
>>> non-DRM_BRIDGE_ATTACH_NO_CONNECTOR code?
>>
>> Okay. Then I will add the bridge mode_valid as a separate patch.
> 
> Well, it was a question =). But indeed my thought was that it sounded
> like material for a separate patch. Is it more of a fix for the
> DRM_BRIDGE_ATTACH_NO_CONNECTOR case?
> 
>   Tomi
> 

Yes this is a fix for DBANC case.
Without mode_valid, I can see a lot of modes getting propagated which
are not even supported by display monitor.
Since mode_valid was added to connector_funcs in the initial commit, I
will use that commit for "Fixes:" tag

Warm Regards,
Jayesh

Re: [RFC PATCH v2 2/3] drm/bridge: cadence: cdns-mhdp8546*: Change drm_connector from pointer to structure

[Tomi Valkeinen]

Hi,

On 27/05/2025 13:39, Jayesh Choudhary wrote:
> 
> 
> On 27/05/25 14:59, Jayesh Choudhary wrote:
>> Hello Tomi,
>>
>> On 27/05/25 13:28, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On 21/05/2025 10:32, Jayesh Choudhary wrote:
>>>> After adding DBANC framework, mhdp->connector is not initialised during
>>>> bridge calls. But the asyncronous work scheduled depends on the
>>>> connector.
>>>> We cannot get to drm_atomic_state in these asyncronous calls running on
>>>> worker threads. So we need to store the data that we need in mhdp
>>>> bridge
>>>> structure.
>>>> Like other bridge drivers, use drm_connector pointer instead of
>>>> structure
>>>> and make appropriate changes to the conditionals and assignments
>>>> related
>>>> to mhdp->connector.
>>>> Also, in the atomic enable call, move the connector  and connector
>>>> state
>>>> calls above, so that we do have a connector before we can retry the
>>>> asyncronous work in case of any failure.
>>>>
>>>
>>> I don't quite understand this patch. You change the mhdp->connector to a
>>> pointer, which is set at bridge_enable and cleared at bridge_disable.
>>> Then you change the "mhdp->connector.dev" checks to "mhdp->connector".
>>>
>>> So, now in e.g. cdns_mhdp_fw_cb(), we check for mhdp->connector, which
>>> is set at bridge_enable(). Can we ever have the bridge enabled before
>>> the fb has been loaded? What is the check even supposed to do there?
>>>
>>> Another in cdns_mhdp_hpd_work(), it checks for mhdp->connector. So...
>>> HPD code behaves differently based on if the bridge has been enabled or
>>> not? What is it supposed to do?
>>>
>>> Isn't the whole "if (mhdp->connector.dev)" code for the legacy
>>> non-DRM_BRIDGE_ATTACH_NO_CONNECTOR case?
>>>
>>>   Tomi
>>
>> I misinterpreted your comment in v1[0] regarding finding the connector
>> from the current state in cdns_mhdp_modeset_retry_fn() and I missed
>> this. I was more focused on finding a connector for that function.
>>
>> For the current code, in all the conditionals involving mhdp->connector,
>> we are entering else statements as connector is not initialised.
>> So I will just drop if statements in cdns_mhdp_fw_cb() and
>> cdns_mhdp_hpd_work() (like you said, its legacy case) while still having
>> mhdp->connector as pointer as we need it for
>> cdns_mhdp_modeset_retry_fn() and in cdns-mhdp8546-hdcp driver.
>>
>> That should be okay?
>>
>> [0]: https://lore.kernel.org/all/e76f94b9-b138-46e7-bb18-
>> b33dd98c9abb@ideasonboard.com/
>>
>> Warm Regards,
>> Jayesh
>>
>>
> 
> Tomi,
> 
> One more thing here. Should this be squashed with the first patch as
> this is sort of removing !(DRM_BRIDGE_ATTACH_NO_CONNECTOR) case and
> associated changes?


All the legacy code should be removed in the previous patch, yes. But
it's not quite clear to me what's going on here. At least parts of this
patch seem to be... fixing some previous code? You move the
drm_atomic_get_new_connector_for_encoder() call to be earlier in the
bridge_enable. That doesn't sound like removing the legacy code. But
it's not quite clear to me why that's done (or why it wasn't needed
earlier. or was it?).

 Tomi

> 
>>>
>>>> Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546
>>>> DPI/DP bridge")
>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>> ---
>>>>   .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 28 ++++++++
>>>> +----------
>>>>   .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  2 +-
>>>>   .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   |  8 +++---
>>>>   3 files changed, 19 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/
>>>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>> index 66bd916c2fe9..5388e62f230b 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)
>>>>               drm_kms_helper_hotplug_event(mhdp->bridge.dev);
>>>>           else
>>>>               drm_bridge_hpd_notify(&mhdp->bridge,
>>>> cdns_mhdp_detect(mhdp));
>>>> @@ -1759,17 +1759,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 = 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);
>>>> +    if (WARN_ON(!conn_state))
>>>> +        goto out;
>>>> +
>>>>       if (mhdp->plugged && !mhdp->link_up) {
>>>>           ret = cdns_mhdp_link_up(mhdp);
>>>>           if (ret < 0)
>>>> @@ -1789,15 +1797,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 ==
>>>> @@ -1857,6 +1856,7 @@ static void cdns_mhdp_atomic_disable(struct
>>>> drm_bridge *bridge,
>>>>           cdns_mhdp_hdcp_disable(mhdp);
>>>>       mhdp->bridge_enabled = false;
>>>> +    mhdp->connector = NULL;
>>>>       cdns_mhdp_reg_read(mhdp, CDNS_DP_FRAMER_GLOBAL_CONFIG, &resp);
>>>>       resp &= ~CDNS_DP_FRAMER_EN;
>>>>       resp |= CDNS_DP_NO_VIDEO_MODE;
>>>> @@ -2157,7 +2157,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;
>>>>       /* Grab the locks before changing connector property */
>>>>       mutex_lock(&conn->dev->mode_config.mutex);
>>>> @@ -2234,7 +2234,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) {
>>>>           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 bad2fc0c7306..b297db53ba28 100644
>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>>>> @@ -375,7 +375,7 @@ struct cdns_mhdp_device {
>>>>        */
>>>>       struct mutex link_mutex;
>>>> -    struct drm_connector connector;
>>>> +    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 42248f179b69..59f18c3281ef 100644
>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
>>>> @@ -394,7 +394,7 @@ 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);
>>>> +        mhdp->connector->name, mhdp->connector->base.id);
>>>>       ret = cdns_mhdp_hdcp_set_config(mhdp, 0, false);
>>>> @@ -445,7 +445,7 @@ static int cdns_mhdp_hdcp_check_link(struct
>>>> cdns_mhdp_device *mhdp)
>>>>       dev_err(mhdp->dev,
>>>>           "[%s:%d] HDCP link failed, retrying authentication\n",
>>>> -        mhdp->connector.name, mhdp->connector.base.id);
>>>> +        mhdp->connector->name, mhdp->connector->base.id);
>>>>       ret = _cdns_mhdp_hdcp_disable(mhdp);
>>>>       if (ret) {
>>>> @@ -487,13 +487,13 @@ 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 = mhdp->connector->dev;
>>>>       struct drm_connector_state *state;
>>>>       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;
>>>> +        state = mhdp->connector->state;
>>>>           state->content_protection = mhdp->hdcp.value;
>>>>       }
>>>>       mutex_unlock(&mhdp->hdcp.mutex);
>>>

Re: [RFC PATCH v2 2/3] drm/bridge: cadence: cdns-mhdp8546*: Change drm_connector from pointer to structure

[Jayesh Choudhary]

Hi,

On 27/05/25 17:07, Tomi Valkeinen wrote:
> Hi,
> 
> On 27/05/2025 13:39, Jayesh Choudhary wrote:
>>
>>
>> On 27/05/25 14:59, Jayesh Choudhary wrote:
>>> Hello Tomi,
>>>
>>> On 27/05/25 13:28, Tomi Valkeinen wrote:
>>>> Hi,
>>>>
>>>> On 21/05/2025 10:32, Jayesh Choudhary wrote:
>>>>> After adding DBANC framework, mhdp->connector is not initialised during
>>>>> bridge calls. But the asyncronous work scheduled depends on the
>>>>> connector.
>>>>> We cannot get to drm_atomic_state in these asyncronous calls running on
>>>>> worker threads. So we need to store the data that we need in mhdp
>>>>> bridge
>>>>> structure.
>>>>> Like other bridge drivers, use drm_connector pointer instead of
>>>>> structure
>>>>> and make appropriate changes to the conditionals and assignments
>>>>> related
>>>>> to mhdp->connector.
>>>>> Also, in the atomic enable call, move the connector  and connector
>>>>> state
>>>>> calls above, so that we do have a connector before we can retry the
>>>>> asyncronous work in case of any failure.
>>>>>
>>>>
>>>> I don't quite understand this patch. You change the mhdp->connector to a
>>>> pointer, which is set at bridge_enable and cleared at bridge_disable.
>>>> Then you change the "mhdp->connector.dev" checks to "mhdp->connector".
>>>>
>>>> So, now in e.g. cdns_mhdp_fw_cb(), we check for mhdp->connector, which
>>>> is set at bridge_enable(). Can we ever have the bridge enabled before
>>>> the fb has been loaded? What is the check even supposed to do there?
>>>>
>>>> Another in cdns_mhdp_hpd_work(), it checks for mhdp->connector. So...
>>>> HPD code behaves differently based on if the bridge has been enabled or
>>>> not? What is it supposed to do?
>>>>
>>>> Isn't the whole "if (mhdp->connector.dev)" code for the legacy
>>>> non-DRM_BRIDGE_ATTACH_NO_CONNECTOR case?
>>>>
>>>>    Tomi
>>>
>>> I misinterpreted your comment in v1[0] regarding finding the connector
>>> from the current state in cdns_mhdp_modeset_retry_fn() and I missed
>>> this. I was more focused on finding a connector for that function.
>>>
>>> For the current code, in all the conditionals involving mhdp->connector,
>>> we are entering else statements as connector is not initialised.
>>> So I will just drop if statements in cdns_mhdp_fw_cb() and
>>> cdns_mhdp_hpd_work() (like you said, its legacy case) while still having
>>> mhdp->connector as pointer as we need it for
>>> cdns_mhdp_modeset_retry_fn() and in cdns-mhdp8546-hdcp driver.
>>>
>>> That should be okay?
>>>
>>> [0]: https://lore.kernel.org/all/e76f94b9-b138-46e7-bb18-
>>> b33dd98c9abb@ideasonboard.com/
>>>
>>> Warm Regards,
>>> Jayesh
>>>
>>>
>>
>> Tomi,
>>
>> One more thing here. Should this be squashed with the first patch as
>> this is sort of removing !(DRM_BRIDGE_ATTACH_NO_CONNECTOR) case and
>> associated changes?
> 
> 
> All the legacy code should be removed in the previous patch, yes. But
> it's not quite clear to me what's going on here. At least parts of this
> patch seem to be... fixing some previous code? You move the
> drm_atomic_get_new_connector_for_encoder() call to be earlier in the
> bridge_enable. That doesn't sound like removing the legacy code. But
> it's not quite clear to me why that's done (or why it wasn't needed
> earlier. or was it?).
> 
>   Tomi
> 

drm_atomic_get_new_connector_for_encoder() call is moved earlier
in bridge_enable to address the cases when we get error in
cdns_mhdp_link_up(mhdp) or cdns_mhdp_reg_read(mhdp, CDNS_DPTX_CAR,
&resp), and we goto 'out' to schedule modeset_retry_work. We need to
have drm_connector before that if we want to change the connector
link state here.

In legacy usecase we are not hitting this as attach already initialised
mhdp->connector before bridge_enable() that would be used by
cdns_mhdp_modeset_retry_fn() as required.

These errors usually don't hit during bridge_enable calls but in
one of my boards, I saw cdns_mhdp_link_up() giving error and after
that the null pointer dereference in cdns_mhdp_modeset_retry_fn()
while trying to access the mutex there (&conn->dev->mode_config.mutex)

-Jayesh



>>
>>>>
>>>>> Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546
>>>>> DPI/DP bridge")
>>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>>> ---
>>>>>    .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 28 ++++++++
>>>>> +----------
>>>>>    .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  2 +-
>>>>>    .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c   |  8 +++---
>>>>>    3 files changed, 19 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/
>>>>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>>> index 66bd916c2fe9..5388e62f230b 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)
>>>>>                drm_kms_helper_hotplug_event(mhdp->bridge.dev);
>>>>>            else
>>>>>                drm_bridge_hpd_notify(&mhdp->bridge,
>>>>> cdns_mhdp_detect(mhdp));
>>>>> @@ -1759,17 +1759,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 = 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);
>>>>> +    if (WARN_ON(!conn_state))
>>>>> +        goto out;
>>>>> +
>>>>>        if (mhdp->plugged && !mhdp->link_up) {
>>>>>            ret = cdns_mhdp_link_up(mhdp);
>>>>>            if (ret < 0)
>>>>> @@ -1789,15 +1797,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 ==
>>>>> @@ -1857,6 +1856,7 @@ static void cdns_mhdp_atomic_disable(struct
>>>>> drm_bridge *bridge,
>>>>>            cdns_mhdp_hdcp_disable(mhdp);
>>>>>        mhdp->bridge_enabled = false;
>>>>> +    mhdp->connector = NULL;
>>>>>        cdns_mhdp_reg_read(mhdp, CDNS_DP_FRAMER_GLOBAL_CONFIG, &resp);
>>>>>        resp &= ~CDNS_DP_FRAMER_EN;
>>>>>        resp |= CDNS_DP_NO_VIDEO_MODE;
>>>>> @@ -2157,7 +2157,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;
>>>>>        /* Grab the locks before changing connector property */
>>>>>        mutex_lock(&conn->dev->mode_config.mutex);
>>>>> @@ -2234,7 +2234,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) {
>>>>>            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 bad2fc0c7306..b297db53ba28 100644
>>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>>>>> @@ -375,7 +375,7 @@ struct cdns_mhdp_device {
>>>>>         */
>>>>>        struct mutex link_mutex;
>>>>> -    struct drm_connector connector;
>>>>> +    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 42248f179b69..59f18c3281ef 100644
>>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
>>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
>>>>> @@ -394,7 +394,7 @@ 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);
>>>>> +        mhdp->connector->name, mhdp->connector->base.id);
>>>>>        ret = cdns_mhdp_hdcp_set_config(mhdp, 0, false);
>>>>> @@ -445,7 +445,7 @@ static int cdns_mhdp_hdcp_check_link(struct
>>>>> cdns_mhdp_device *mhdp)
>>>>>        dev_err(mhdp->dev,
>>>>>            "[%s:%d] HDCP link failed, retrying authentication\n",
>>>>> -        mhdp->connector.name, mhdp->connector.base.id);
>>>>> +        mhdp->connector->name, mhdp->connector->base.id);
>>>>>        ret = _cdns_mhdp_hdcp_disable(mhdp);
>>>>>        if (ret) {
>>>>> @@ -487,13 +487,13 @@ 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 = mhdp->connector->dev;
>>>>>        struct drm_connector_state *state;
>>>>>        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;
>>>>> +        state = mhdp->connector->state;
>>>>>            state->content_protection = mhdp->hdcp.value;
>>>>>        }
>>>>>        mutex_unlock(&mhdp->hdcp.mutex);
>>>>
> 

Re: [RFC PATCH v2 2/3] drm/bridge: cadence: cdns-mhdp8546*: Change drm_connector from pointer to structure

[Tomi Valkeinen]

Hi,

On 28/05/2025 14:25, Jayesh Choudhary wrote:
> Hi,
> 
> On 27/05/25 17:07, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 27/05/2025 13:39, Jayesh Choudhary wrote:
>>>
>>>
>>> On 27/05/25 14:59, Jayesh Choudhary wrote:
>>>> Hello Tomi,
>>>>
>>>> On 27/05/25 13:28, Tomi Valkeinen wrote:
>>>>> Hi,
>>>>>
>>>>> On 21/05/2025 10:32, Jayesh Choudhary wrote:
>>>>>> After adding DBANC framework, mhdp->connector is not initialised
>>>>>> during
>>>>>> bridge calls. But the asyncronous work scheduled depends on the
>>>>>> connector.
>>>>>> We cannot get to drm_atomic_state in these asyncronous calls
>>>>>> running on
>>>>>> worker threads. So we need to store the data that we need in mhdp
>>>>>> bridge
>>>>>> structure.
>>>>>> Like other bridge drivers, use drm_connector pointer instead of
>>>>>> structure
>>>>>> and make appropriate changes to the conditionals and assignments
>>>>>> related
>>>>>> to mhdp->connector.
>>>>>> Also, in the atomic enable call, move the connector  and connector
>>>>>> state
>>>>>> calls above, so that we do have a connector before we can retry the
>>>>>> asyncronous work in case of any failure.
>>>>>>
>>>>>
>>>>> I don't quite understand this patch. You change the mhdp->connector
>>>>> to a
>>>>> pointer, which is set at bridge_enable and cleared at bridge_disable.
>>>>> Then you change the "mhdp->connector.dev" checks to "mhdp->connector".
>>>>>
>>>>> So, now in e.g. cdns_mhdp_fw_cb(), we check for mhdp->connector, which
>>>>> is set at bridge_enable(). Can we ever have the bridge enabled before
>>>>> the fb has been loaded? What is the check even supposed to do there?
>>>>>
>>>>> Another in cdns_mhdp_hpd_work(), it checks for mhdp->connector. So...
>>>>> HPD code behaves differently based on if the bridge has been
>>>>> enabled or
>>>>> not? What is it supposed to do?
>>>>>
>>>>> Isn't the whole "if (mhdp->connector.dev)" code for the legacy
>>>>> non-DRM_BRIDGE_ATTACH_NO_CONNECTOR case?
>>>>>
>>>>>    Tomi
>>>>
>>>> I misinterpreted your comment in v1[0] regarding finding the connector
>>>> from the current state in cdns_mhdp_modeset_retry_fn() and I missed
>>>> this. I was more focused on finding a connector for that function.
>>>>
>>>> For the current code, in all the conditionals involving mhdp-
>>>> >connector,
>>>> we are entering else statements as connector is not initialised.
>>>> So I will just drop if statements in cdns_mhdp_fw_cb() and
>>>> cdns_mhdp_hpd_work() (like you said, its legacy case) while still
>>>> having
>>>> mhdp->connector as pointer as we need it for
>>>> cdns_mhdp_modeset_retry_fn() and in cdns-mhdp8546-hdcp driver.
>>>>
>>>> That should be okay?
>>>>
>>>> [0]: https://lore.kernel.org/all/e76f94b9-b138-46e7-bb18-
>>>> b33dd98c9abb@ideasonboard.com/
>>>>
>>>> Warm Regards,
>>>> Jayesh
>>>>
>>>>
>>>
>>> Tomi,
>>>
>>> One more thing here. Should this be squashed with the first patch as
>>> this is sort of removing !(DRM_BRIDGE_ATTACH_NO_CONNECTOR) case and
>>> associated changes?
>>
>>
>> All the legacy code should be removed in the previous patch, yes. But
>> it's not quite clear to me what's going on here. At least parts of this
>> patch seem to be... fixing some previous code? You move the
>> drm_atomic_get_new_connector_for_encoder() call to be earlier in the
>> bridge_enable. That doesn't sound like removing the legacy code. But
>> it's not quite clear to me why that's done (or why it wasn't needed
>> earlier. or was it?).
>>
>>   Tomi
>>
> 
> drm_atomic_get_new_connector_for_encoder() call is moved earlier
> in bridge_enable to address the cases when we get error in
> cdns_mhdp_link_up(mhdp) or cdns_mhdp_reg_read(mhdp, CDNS_DPTX_CAR,
> &resp), and we goto 'out' to schedule modeset_retry_work. We need to
> have drm_connector before that if we want to change the connector
> link state here.
> 
> In legacy usecase we are not hitting this as attach already initialised
> mhdp->connector before bridge_enable() that would be used by
> cdns_mhdp_modeset_retry_fn() as required.
> 
> These errors usually don't hit during bridge_enable calls but in
> one of my boards, I saw cdns_mhdp_link_up() giving error and after
> that the null pointer dereference in cdns_mhdp_modeset_retry_fn()
> while trying to access the mutex there (&conn->dev->mode_config.mutex)

Okay, so moving the drm_atomic_get_new_connector_for_encoder() is a bug
fix, and should be a separate patch? But you can't do that until you
have changed the connector field to a pointer, and you can't do that
until you have removed the legacy code...

In theory that could be sorted out, but changing the connector field to
a pointer, while still keeping the legacy code, would be a bit laborious.

So, maybe keep the first patch, but split out the move of the
drm_atomic_get_new_connector_for_encoder() to a new third patch, so that
this one (2/3) would be only about changing the embedded connector field
to a pointer?

And a hint: If in the patch description you find yourself writing "Also,
...", it usually suggests that you might want to split the patch =).

 Tomi