Re: [PATCH v2 4/7] drm/vc4: kms: Document the muxing corner cases

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 3680 bytes --]

Hi!

Thanks for your review

On Thu, Oct 29, 2020 at 09:51:04AM +0100, Daniel Vetter wrote:
> On Wed, Oct 28, 2020 at 01:41:01PM +0100, Maxime Ripard wrote:
> > We've had a number of muxing corner-cases with specific ways to reproduce
> > them, so let's document them to make sure they aren't lost and introduce
> > regressions later on.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/gpu/drm/vc4/vc4_kms.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> > index 4aa0577bd055..b0043abec16d 100644
> > --- a/drivers/gpu/drm/vc4/vc4_kms.c
> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> > @@ -612,6 +612,28 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = {
> >  };
> >  
> >  
> > +/*
> > + * The BCM2711 HVS has up to 7 output connected to the pixelvalves and
> > + * the TXP (and therefore all the CRTCs found on that platform).
> > + *
> > + * The naive (and our initial) implementation would just iterate over
> > + * all the active CRTCs, try to find a suitable FIFO, and then remove it
> > + * from the available FIFOs pool. However, there's a few corner cases
> > + * that need to be considered:
> > + *
> > + * - When running in a dual-display setup (so with two CRTCs involved),
> > + *   we can update the state of a single CRTC (for example by changing
> > + *   its mode using xrandr under X11) without affecting the other. In
> > + *   this case, the other CRTC wouldn't be in the state at all, so we
> > + *   need to consider all the running CRTCs in the DRM device to assign
> > + *   a FIFO, not just the one in the state.
> > + *
> > + * - Since we need the pixelvalve to be disabled and enabled back when
> > + *   the FIFO is changed, we should keep the FIFO assigned for as long
> > + *   as the CRTC is enabled, only considering it free again once that
> > + *   CRTC has been disabled. This can be tested by booting X11 on a
> > + *   single display, and changing the resolution down and then back up.
> 
> This is a bit much.

What do you find to be a bit much?

> With planes we have the same problem, and we're solving this with the
> drm_plane_state->commit tracker. If you have one of these per fifo
> then you can easily sync against the disabling crtc if the fifo
> becomes free.
> 
> Note to avoid locking headaches this would mean you'd need a per-fifo
> private state object. You can avoid this if you just track the
> ->disabling_commit per fifo, and sync against that when enabling it on a
> different fifo.
> 
> Note that it's somewhat tricky to do this correctly, since you need to
> copy that commit on each state duplication around, until it's either used
> in a new crtc (and hence tracked under that) or the commit has completed
> (but this is only an optimization, you could just keep them around forever
> for unused fifo that have been used in the past, it's a tiny struct with
> nothing hanging of it).

I'm not really following you here. The hardware that does the blending
(HVS) and the timing generation (pixelvalve) is mostly transparent to
DRM and plugged as a CRTC, with the pixelvalve being the device tied to
that CRTC, and the pixelvalve hooks calling into the HVS code when
needed.

The FIFO is in the HVS itself since it can only blend 3 different
scenes, and then you get to choose the output of that FIFO to send it to
the proper pixelvalve that matches the encoder you eventually want to
use.

So yeah, this FIFO is entirely internal to the CRTC as far as DRM is
concerned.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]