[PATCH] drm/vc4: hvs: Pull the state of all the CRTCs prior to PV muxing

[Dave Stevenson]

Hi Maxime

On Thu, 17 Sep 2020 at 13:16, Maxime Ripard <maxime at cerno.tech> wrote:
>
> The vc4 display engine has a first controller called the HVS that will
> perform the composition of the planes. That HVS has 3 FIFOs and can
> therefore compose planes for up to three outputs. The timings part is
> generated through a component called the Pixel Valve, and the BCM2711 has 6
> of them.
>
> Thus, the HVS has some bits to control which FIFO gets output to which
> Pixel Valve. The current code supports that muxing by looking at all the
> CRTCs in a new DRM atomic state in atomic_check, and given the set of
> contraints that we have, assigns FIFOs to CRTCs or reject the mode

s/contraints/constraints

> entirely. The actual muxing will occur during atomic_commit.
>
> However, that doesn't work if only a fraction of the CRTCs' state is
> updated in that state, since it will ignore the CRTCs that are kept running
> unmodified, and will thus unassign its associated FIFO, and later disable
> it.

Yup, that would surely mess things up :)

> In order to make the code work as expected, let's pull the CRTC state of
> all the enabled CRTC in our atomic_check so that we can operate on all the
> running CRTCs, no matter whether they are affected by the new state or not.
>
> Cc: Hoegeun Kwon <hoegeun.kwon at samsung.com>
> Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> Signed-off-by: Maxime Ripard <maxime at cerno.tech>

Tested-by: Dave Stevenson <dave.stevenson at raspberrypi.com>
Reviewed-by: Dave Stevenson <dave.stevenson at raspberrypi.com>

Thanks
  Dave

> ---
>  drivers/gpu/drm/vc4/vc4_kms.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 16e233e1406e..af3ee3dcdab6 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -620,6 +620,23 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>         struct drm_crtc *crtc;
>         int i, ret;
>
> +       /*
> +        * Since the HVS FIFOs are shared across all the pixelvalves and
> +        * the TXP (and thus all the CRTCs), we need to pull the current
> +        * state of all the enabled CRTCs so that an update to a single
> +        * CRTC still keeps the previous FIFOs enabled and assigned to
> +        * the same CRTCs, instead of evaluating only the CRTC being
> +        * modified.
> +        */
> +       list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +               if (!crtc->state->enable)
> +                       continue;
> +
> +               crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +               if (IS_ERR(crtc_state))
> +                       return PTR_ERR(crtc_state);
> +       }
> +
>         for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>                 struct vc4_crtc_state *vc4_crtc_state =
>                         to_vc4_crtc_state(crtc_state);
> --
> 2.26.2
>