Message-ID: <d19fe815-09cf-45f9-9f36-ff823216d17e@ideasonboard.com>
Date: Wed, 28 May 2025 15:32:44 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Jayesh Choudhary <j-choudhary@...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 28/05/2025 14:25, Jayesh Choudhary wrote:
> 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)

Okay, so moving the drm_atomic_get_new_connector_for_encoder() is a bug
fix, and should be a separate patch? But you can't do that until you
have changed the connector field to a pointer, and you can't do that
until you have removed the legacy code...

In theory that could be sorted out, but changing the connector field to
a pointer, while still keeping the legacy code, would be a bit laborious.

So, maybe keep the first patch, but split out the move of the
drm_atomic_get_new_connector_for_encoder() to a new third patch, so that
this one (2/3) would be only about changing the embedded connector field
to a pointer?

And a hint: If in the patch description you find yourself writing "Also,
...", it usually suggests that you might want to split the patch =).

 Tomi

Powered by blists - more mailing lists