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- >>>> b33dd98c9...@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 >> > > 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
