bridge: cadence: cdns-mhdp8546*: Change drm_connector from pointer to structure

Jayesh Choudhary Tue, 27 May 2025 02:31:02 -0700

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-b33dd98c9...@ideasonboard.com/


Warm Regards,
Jayesh

Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546 DPI/DP 
bridge")
Signed-off-by: Jayesh Choudhary <j-choudh...@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

Reply via email to