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

[Daniel Vetter <daniel@ffwll.ch>]

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. 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).
-Daniel

> + */
>  static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>  				      struct drm_atomic_state *state)
>  {
> -- 
> git-series 0.9.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch