[RFC PATCH v2 2/3] drm/bridge: cadence: cdns-mhdp8546*: Change drm_connector from pointer to structure
Jayesh Choudhary
j-choudhary at ti.com
Tue May 27 10:39:18 UTC 2025
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 at 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);
>>
More information about the dri-devel
mailing list