[RFC PATCH v2 2/3] drm/bridge: cadence: cdns-mhdp8546*: Change drm_connector from pointer to structure
Tomi Valkeinen
tomi.valkeinen at ideasonboard.com
Tue May 27 11:37:20 UTC 2025
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 at 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 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