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-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);
