On Thu, Oct 29, 2020 at 11:47:28AM +0100, Maxime Ripard wrote:
> 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@xxxxxxxxxx>
> > > ---
> > > 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.
So why do you dynamically assign it in a global state object? It sounded
like you assign these things dynamically, and that there's a problem with
sync when you move it from one crtc to the other. And that kind of problem
is solved by tracking the last crtc using a given resource that can be
used by different crtc with a drm_crtc_commit *last_user pointer.
Otherwise I think if you follow 2 crtc commits, one that disables a CRTC
user and releases a FIFO, and the next crtc (a different one) that uses it
right away, both nonblocking, then the 2nd crtc might start using your
shared resources before the first one actually stopped using it.
The other thing is also if you need multiple of these shared resources on
a CRTC, and dynamically reassigning them, then forcing them to be assigned
until the crtc is completely off is a bit too much synchronization. E.g.
we don't have that rule for drm planes. Now I have no idea whether your
CRTC:FIFO is 1:1 or 1:n, so maybe you only have the sync issue and not the
over-sync issue :-)
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
