[PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code

[Dave Stevenson]

Hi Maxime

On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime at cerno.tech> wrote:
>
> The current CRTC state reset hook in vc4 allocates a vc4_crtc_state
> structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state
> embeds drm_crtc_state as its first member, and therefore can be safely
> casted.

s/casted/cast

> However, this is pretty fragile especially since there's no check for this
> in place, and we're going to need to access vc4_crtc_state member at reset
> so this looks like a good occasion to make it more robust.
>
> Signed-off-by: Maxime Ripard <maxime at cerno.tech>
> Tested-by: Dave Stevenson <dave.stevenson at raspberrypi.com>

Based on the issue I perceived with the previous patch, I'm happy. I
haven't thought about how the framework handles losing the state, but
that's not the driver's problem.

There is still an implicit assumption that drm_crtc_state is the first
member from the implementation of to_vc4_crtc_state in vc4_drv.h. To
make it even more robust that could be a container_of instead. I
haven't checked for any other places that make the assumption though.

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

> ---
>
> Changes from v1:
>   - new patch
> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index a393f93390a2..7ef20adedee5 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) {
> +               crtc->state = NULL;
> +               return;
> +       }
> +
> +       __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base);
>  }
>
>  static const struct drm_crtc_funcs vc4_crtc_funcs = {
> --
> 2.26.2
>