| Message ID | 20200923084032.218619-1-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm/vc4: crtc: Rework a bit the CRTC state code | expand |
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. > > 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 >
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
Hi Dave, 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. > > > > 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. Good catch, I'll send another patch to fix it > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Does it apply to the second patch as well? Thanks! Maxime
On Fri, 25 Sep 2020 at 12:38, Maxime Ripard <maxime@cerno.tech> wrote: > > Hi Dave, > > 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. > > > > > > 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. > > Good catch, I'll send another patch to fix it > > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Does it apply to the second patch as well? No. I got another interrupt before I'd refreshed my memory over what the second patch was doing and responding to it. I'll do that now (I don't think it changed much from v1) Dave.
Hi, 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. > > > > 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> Applied 1 and 2, with the typo fixed (and the fixes tag suggested by Daniel) Maxime
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 = {