Re: [PATCH v3 31/37] drm/bridge: Provide pointers to the connector and crtc in bridge state

[Simona Vetter]

On Wed, Feb 19, 2025 at 04:56:11PM +0100, Maxime Ripard wrote:
> On Wed, Feb 19, 2025 at 02:35:25PM +0100, Simona Vetter wrote:
> > On Tue, Feb 18, 2025 at 11:23:00AM +0100, Maxime Ripard wrote:
> > > Hi,
> > > 
> > > Thanks for your answer
> > > 
> > > On Mon, Feb 17, 2025 at 05:41:24PM +0100, Simona Vetter wrote:
> > > > On Thu, Feb 13, 2025 at 03:43:50PM +0100, Maxime Ripard wrote:
> > > > > Now that connectors are no longer necessarily created by the bridges
> > > > > drivers themselves but might be created by drm_bridge_connector, it's
> > > > > pretty hard for bridge drivers to retrieve pointers to the connector and
> > > > > CRTC they are attached to.
> > > > > 
> > > > > Indeed, the only way to retrieve the CRTC is to follow the drm_bridge
> > > > > encoder field, and then the drm_encoder crtc field, both of them being
> > > > > deprecated.
> > > > 
> > > > Eh, this isn't quite how this works. So unless bridges have become very
> > > > dynamic and gained flexible routing the bridge(->bridge->...)->encoder
> > > > chain is static. And the crtc for an encoder you find by walking the
> > > > connector states in a drm_atomic_state, finding the right one that points
> > > > at your encoder, and then return the ->crtc pointer from that connector
> > > > state.
> > > > 
> > > > It's a bit bonkers, but I think it's better to compute this than adding
> > > > more pointers that potentially diverge. Unless there's a grand plan here,
> > > > but then I think we want some safety checks that all the pointers never
> > > > get out of sync here.
> > > 
> > > That work stemed from this series
> > > https://lore.kernel.org/all/20250210132620.42263-1-herve.codina@xxxxxxxxxxx/
> > > 
> > > and in particular:
> > > https://lore.kernel.org/all/20250210132620.42263-5-herve.codina@xxxxxxxxxxx/
> > > 
> > > Bridges, outside of the modesetting code path, don't have a way to
> > > access the drm_atomic_state since drm_bridge_state->state is typically
> > > cleared after swap_state. So accessing the connectors and CRTCs don't
> > > work anymore.
> > > 
> > > In this particular case, we needed to access those from the bridge
> > > interrupt handler.
> > 
> > Uh for interrupt handler you can't use anything stored in state objects
> > anyway. So I'm even more confused.
> 
> Why not? As long as we're in a threaded handler, and take the proper
> locks, what's wrong with it, and how is it fundamentally different than,
> idk, cec or audio hooks?

Well, _threaded_ interrupt handler is a quite important distinction. Then
it's indeed fine from a locking pov. I'm still not sure it's actually what
you want, because obj->state is the logical state. It does not reflect the
hw state, and grabbing a lock won't change that. You can do nonblocking
atomic modeset changes after all. So you might still run the risk that
your irq handler is looking at state which does not reflect the actual hw
configuration. And if it's then messing around with it, you're racing
against a concurrent atomic commit work.

This is why all the vblank state is protected by separate spinlocks, and
copied over to that separate state at the appropriate time within the
atomic commit sequence. So if you need something like this for bridges,
you probably need still need the spinlock (or a mutex, for threaded
interrupt handlers) and separate state.

Meaning: I'm still rather skeptical of what you're doing here ...
-Sima
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch