[PATCH 2/2] drm/vc4: crtc: Keep the previously assigned HVS FIFO

[Dave Stevenson]

Hi Maxime

Thanks for the patch - it makes mode switching reliable for me! :-)

On Fri, 18 Sep 2020 at 15:59, Maxime Ripard <maxime at cerno.tech> wrote:
>
> The HVS FIFOs are currently assigned each time we have an atomic_check
> for all the enabled CRTCs.
>
> However, if we are running multiple outputs in parallel and we happen to
> disable the first (by index) CRTC, we end up changing the assigned FIFO
> of the second CRTC without disabling and reenabling the pixelvalve which
> ends up in a stall and eventually a VBLANK timeout.
>
> In order to fix this, we can create a special value for our assigned
> channel to mark it as disabled, and if our CRTC already had an assigned
> channel in its previous state, we keep on using it.
>
> 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>

Review comments below though.

> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++---
>  drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
>  drivers/gpu/drm/vc4/vc4_kms.c  | 21 +++++++++++++++------
>  3 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index a393f93390a2..be754120faa8 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -852,11 +852,18 @@ void vc4_crtc_destroy_state(struct drm_crtc *crtc,
>
>  void vc4_crtc_reset(struct drm_crtc *crtc)
>  {
> +       struct vc4_crtc_state *vc4_crtc_state;
> +
>         if (crtc->state)
>                 vc4_crtc_destroy_state(crtc, crtc->state);
> -       crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL);
> -       if (crtc->state)
> -               __drm_atomic_helper_crtc_reset(crtc, crtc->state);
> +
> +       vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL);
> +       if (!vc4_crtc_state)
> +               return;

This error path has me worried, but I may have missed it.

If crtc->state was set then it's been freed via
vc4_crtc_destroy_state. However I don't see anything that has reset
crtc->state to NULL.
If the new alloc fails then I believe we exit with crtc->state still
set to the old freed pointer.

The old code directly set crtc->state, so it got reset to NULL by the failure.

> +
> +       vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
> +       crtc->state = &vc4_crtc_state->base;
> +       __drm_atomic_helper_crtc_reset(crtc, crtc->state);
>  }
>
>  static const struct drm_crtc_funcs vc4_crtc_funcs = {
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 8c8d96b6289f..2b13f2126f13 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -531,6 +531,7 @@ struct vc4_crtc_state {
>                 unsigned int bottom;
>         } margins;
>  };
> +#define VC4_HVS_CHANNEL_DISABLED ((unsigned int) -1)

Checkpatch whinge on that
CHECK: No space is necessary after a cast
#55: FILE: drivers/gpu/drm/vc4/vc4_drv.h:539:
+#define VC4_HVS_CHANNEL_DISABLED ((unsigned int) -1)

CHECK: Please use a blank line after function/struct/union/enum declarations
#55: FILE: drivers/gpu/drm/vc4/vc4_drv.h:539:
 };
+#define VC4_HVS_CHANNEL_DISABLED ((unsigned int) -1)

>  static inline struct vc4_crtc_state *
>  to_vc4_crtc_state(struct drm_crtc_state *crtc_state)
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 01fa60844695..f452dad50c22 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -616,7 +616,7 @@ static int
>  vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>  {
>         unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0);
> -       struct drm_crtc_state *crtc_state;
> +       struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>         struct drm_crtc *crtc;
>         int i, ret;
>
> @@ -629,6 +629,7 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>          * modified.
>          */
>         list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +               struct drm_crtc_state *crtc_state;

Blank line between variables and code, or not in this subsystem?
Checkpatch hasn't complained to me here.

>                 if (!crtc->state->enable)
>                         continue;
>
> @@ -637,15 +638,23 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *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);
> +       for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +               struct vc4_crtc_state *new_vc4_crtc_state =
> +                       to_vc4_crtc_state(new_crtc_state);
>                 struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
>                 unsigned int matching_channels;
>
> -               if (!crtc_state->enable)
> +               if (old_crtc_state->enable && !new_crtc_state->enable)
> +                       new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
> +
> +               if (!new_crtc_state->enable)
>                         continue;
>
> +               if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED) {
> +                       unassigned_channels &= ~BIT(new_vc4_crtc_state->assigned_channel);
> +                       continue;
> +               }
> +
>                 /*
>                  * The problem we have to solve here is that we have
>                  * up to 7 encoders, connected to up to 6 CRTCs.
> @@ -674,7 +683,7 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>                 if (matching_channels) {
>                         unsigned int channel = ffs(matching_channels) - 1;
>
> -                       vc4_crtc_state->assigned_channel = channel;
> +                       new_vc4_crtc_state->assigned_channel = channel;
>                         unassigned_channels &= ~BIT(channel);
>                 } else {
>                         return -EINVAL;
> --
> 2.26.2
>