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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote:
> Hi Maxime
> 
> On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@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.

Also your next atomic_check will probably look at this, most definitely in
the memcpy to take over the old state. So yeah KASAN should complain if
you get this wrong :-)

Probably want a Fixes: line for this too.
-Daniel

> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > Tested-by: Dave Stevenson <dave.stevenson@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@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
> >
> _______________________________________________
> 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel