Message-ID: <870d036a-0fa0-43e0-8b72-428096173526@ti.com>
Date: Wed, 28 May 2025 16:55:57 +0530
From: Jayesh Choudhary <j-choudhary@...com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
CC: <jonas@...boo.se>, <jernej.skrabec@...il.com>,
        <maarten.lankhorst@...ux.intel.com>, <mripard@...nel.org>,
        <tzimmermann@...e.de>, <airlied@...il.com>, <simona@...ll.ch>,
        <lumag@...nel.org>, <jani.nikula@...el.com>, <andy.yan@...k-chips.com>,
        <mordan@...ras.ru>, <linux@...blig.org>, <viro@...iv.linux.org.uk>,
        <yamonkar@...ence.com>, <sjakhade@...ence.com>,
        <quentin.schulz@...e-electrons.com>, <jsarha@...com>,
        <linux-kernel@...r.kernel.org>, <devarsht@...com>,
        <dianders@...omium.org>, <andrzej.hajda@...el.com>,
        <neil.armstrong@...aro.org>, <rfoss@...nel.org>,
        <Laurent.pinchart@...asonboard.com>, <dri-devel@...ts.freedesktop.org>,
        <alexander.stein@...tq-group.com>
Subject: Re: [RFC PATCH v2 2/3] drm/bridge: cadence: cdns-mhdp8546*: Change
 drm_connector from pointer to structure

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@...asonboard.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@...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);
>>>>
> 

Powered by blists - more mailing lists