| Message ID | 20190301125627.7285-13-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | None | expand |
Hi Maarten, Thank you for the patch. On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote: > Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of > writing its own version. Instead of open coding destroy_state(), call > it directly for freeing the old state. I don't think the second sentence applies to this patch. > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > Cc: linux-renesas-soc@vger.kernel.org > --- > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > index 4cdea14d552f..7766551e67fc 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc) > > static void rcar_du_crtc_reset(struct drm_crtc *crtc) > { > - struct rcar_du_crtc_state *state; > + struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL); > > - if (crtc->state) { > + if (crtc->state) > rcar_du_crtc_atomic_destroy_state(crtc, crtc->state); > - crtc->state = NULL; > - } > > - state = kzalloc(sizeof(*state), GFP_KERNEL); > + __drm_atomic_helper_crtc_reset(crtc, &state->state); state may be NULL here if the above kzalloc() failed. Let's keep the original order of the function, and simply call __drm_atomic_helper_crtc_reset() after the NULL check below. > if (state == NULL) > return; > > state->crc.source = VSP1_DU_CRC_NONE; > state->crc.index = 0; > - > - crtc->state = &state->state; > - crtc->state->crtc = crtc; > } > > static int rcar_du_crtc_enable_vblank(struct drm_crtc *crtc)
Op 01-03-2019 om 14:13 schreef Laurent Pinchart: > Hi Maarten, > > Thank you for the patch. > > On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote: >> Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of >> writing its own version. Instead of open coding destroy_state(), call >> it directly for freeing the old state. > I don't think the second sentence applies to this patch. > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >> Cc: linux-renesas-soc@vger.kernel.org >> --- >> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++-------- >> 1 file changed, 3 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c >> index 4cdea14d552f..7766551e67fc 100644 >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c >> @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc) >> >> static void rcar_du_crtc_reset(struct drm_crtc *crtc) >> { >> - struct rcar_du_crtc_state *state; >> + struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL); >> >> - if (crtc->state) { >> + if (crtc->state) >> rcar_du_crtc_atomic_destroy_state(crtc, crtc->state); >> - crtc->state = NULL; >> - } >> >> - state = kzalloc(sizeof(*state), GFP_KERNEL); >> + __drm_atomic_helper_crtc_reset(crtc, &state->state); > state may be NULL here if the above kzalloc() failed. Let's keep the > original order of the function, and simply call > __drm_atomic_helper_crtc_reset() after the NULL check below. There were 10 different ways crtc was implemented, I felt it was good to settle on one. We don't handle during reset at all, would need to start propagating this first before we should handle errors, imho. Looking more closely, it's the same way that errors in rcar_du_plane_reset() are handled. :) Cheers, ~Maarten
Hi Marteen, On Fri, Mar 01, 2019 at 03:08:20PM +0100, Maarten Lankhorst wrote: > Op 01-03-2019 om 14:13 schreef Laurent Pinchart: > > On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote: > >> Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of > >> writing its own version. Instead of open coding destroy_state(), call > >> it directly for freeing the old state. > > I don't think the second sentence applies to this patch. > > > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > >> Cc: linux-renesas-soc@vger.kernel.org > >> --- > >> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++-------- > >> 1 file changed, 3 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > >> index 4cdea14d552f..7766551e67fc 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > >> @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc) > >> > >> static void rcar_du_crtc_reset(struct drm_crtc *crtc) > >> { > >> - struct rcar_du_crtc_state *state; > >> + struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL); > >> > >> - if (crtc->state) { > >> + if (crtc->state) > >> rcar_du_crtc_atomic_destroy_state(crtc, crtc->state); > >> - crtc->state = NULL; > >> - } > >> > >> - state = kzalloc(sizeof(*state), GFP_KERNEL); > >> + __drm_atomic_helper_crtc_reset(crtc, &state->state); > > > > state may be NULL here if the above kzalloc() failed. Let's keep the > > original order of the function, and simply call > > __drm_atomic_helper_crtc_reset() after the NULL check below. > > There were 10 different ways crtc was implemented, I felt it was good to settle on one. > > We don't handle during reset at all, would need to start propagating this first before we should handle errors, imho. That's not the point. As state can be NULL, you could end up dereferencing a NULL pointer. The fact that the base state is the first field in the rcar_du_crtc_state structure is just luck, and shouldn't be relied on. > Looking more closely, it's the same way that errors in > rcar_du_plane_reset() are handled. :) It's not, the return value of kzalloc() is checked explicitly in rcar_du_plane_reset() before calling __drm_atomic_helper_plane_reset(). Please copy the code flow of rcar_du_plane_reset() to implement rcar_du_crtc_reset().
Op 01-03-2019 om 15:36 schreef Laurent Pinchart: > Hi Marteen, > > On Fri, Mar 01, 2019 at 03:08:20PM +0100, Maarten Lankhorst wrote: >> Op 01-03-2019 om 14:13 schreef Laurent Pinchart: >>> On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote: >>>> Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of >>>> writing its own version. Instead of open coding destroy_state(), call >>>> it directly for freeing the old state. >>> I don't think the second sentence applies to this patch. >>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >>>> Cc: linux-renesas-soc@vger.kernel.org >>>> --- >>>> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++-------- >>>> 1 file changed, 3 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c >>>> index 4cdea14d552f..7766551e67fc 100644 >>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c >>>> @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc) >>>> >>>> static void rcar_du_crtc_reset(struct drm_crtc *crtc) >>>> { >>>> - struct rcar_du_crtc_state *state; >>>> + struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL); >>>> >>>> - if (crtc->state) { >>>> + if (crtc->state) >>>> rcar_du_crtc_atomic_destroy_state(crtc, crtc->state); >>>> - crtc->state = NULL; >>>> - } >>>> >>>> - state = kzalloc(sizeof(*state), GFP_KERNEL); >>>> + __drm_atomic_helper_crtc_reset(crtc, &state->state); >>> state may be NULL here if the above kzalloc() failed. Let's keep the >>> original order of the function, and simply call >>> __drm_atomic_helper_crtc_reset() after the NULL check below. >> There were 10 different ways crtc was implemented, I felt it was good to settle on one. >> >> We don't handle during reset at all, would need to start propagating this first before we should handle errors, imho. > That's not the point. As state can be NULL, you could end up > dereferencing a NULL pointer. The fact that the base state is the first > field in the rcar_du_crtc_state structure is just luck, and shouldn't be > relied on. Would it be ok if I changed it to state ? &state->state : NULL and let the compiler deal with it? Will probably fix up all other patches as well before committing. >> Looking more closely, it's the same way that errors in >> rcar_du_plane_reset() are handled. :) > It's not, the return value of kzalloc() is checked explicitly in > rcar_du_plane_reset() before calling __drm_atomic_helper_plane_reset(). > Please copy the code flow of rcar_du_plane_reset() to implement > rcar_du_crtc_reset(). >
Hi Marteen, On Fri, Mar 01, 2019 at 03:47:02PM +0100, Maarten Lankhorst wrote: > Op 01-03-2019 om 15:36 schreef Laurent Pinchart: > > On Fri, Mar 01, 2019 at 03:08:20PM +0100, Maarten Lankhorst wrote: > >> Op 01-03-2019 om 14:13 schreef Laurent Pinchart: > >>> On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote: > >>>> Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of > >>>> writing its own version. Instead of open coding destroy_state(), call > >>>> it directly for freeing the old state. > >>> I don't think the second sentence applies to this patch. > >>> > >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > >>>> Cc: linux-renesas-soc@vger.kernel.org > >>>> --- > >>>> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++-------- > >>>> 1 file changed, 3 insertions(+), 8 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > >>>> index 4cdea14d552f..7766551e67fc 100644 > >>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > >>>> @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc) > >>>> > >>>> static void rcar_du_crtc_reset(struct drm_crtc *crtc) > >>>> { > >>>> - struct rcar_du_crtc_state *state; > >>>> + struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL); > >>>> > >>>> - if (crtc->state) { > >>>> + if (crtc->state) > >>>> rcar_du_crtc_atomic_destroy_state(crtc, crtc->state); > >>>> - crtc->state = NULL; > >>>> - } > >>>> > >>>> - state = kzalloc(sizeof(*state), GFP_KERNEL); > >>>> + __drm_atomic_helper_crtc_reset(crtc, &state->state); > >>> state may be NULL here if the above kzalloc() failed. Let's keep the > >>> original order of the function, and simply call > >>> __drm_atomic_helper_crtc_reset() after the NULL check below. > >> There were 10 different ways crtc was implemented, I felt it was good to settle on one. > >> > >> We don't handle during reset at all, would need to start propagating this first before we should handle errors, imho. > > That's not the point. As state can be NULL, you could end up > > dereferencing a NULL pointer. The fact that the base state is the first > > field in the rcar_du_crtc_state structure is just luck, and shouldn't be > > relied on. > > Would it be ok if I changed it to state ? &state->state : NULL and let > the compiler deal with it? What's wrong with a proper implementation ? static void rcar_du_crtc_reset(struct drm_crtc *crtc) { struct rcar_du_crtc_state *state; if (crtc->state) { rcar_du_crtc_atomic_destroy_state(crtc, crtc->state); crtc->state = NULL; } state = kzalloc(sizeof(*state), GFP_KERNEL); if (state == NULL) return; __drm_atomic_helper_crtc_reset(crtc, &state->state); state->crc.source = VSP1_DU_CRC_NONE; state->crc.index = 0; } > Will probably fix up all other patches as well before committing. You won't commit this one before I ack it, right ? :-) > >> Looking more closely, it's the same way that errors in > >> rcar_du_plane_reset() are handled. :) > > It's not, the return value of kzalloc() is checked explicitly in > > rcar_du_plane_reset() before calling __drm_atomic_helper_plane_reset(). > > Please copy the code flow of rcar_du_plane_reset() to implement > > rcar_du_crtc_reset().
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 4cdea14d552f..7766551e67fc 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc) static void rcar_du_crtc_reset(struct drm_crtc *crtc) { - struct rcar_du_crtc_state *state; + struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL); - if (crtc->state) { + if (crtc->state) rcar_du_crtc_atomic_destroy_state(crtc, crtc->state); - crtc->state = NULL; - } - state = kzalloc(sizeof(*state), GFP_KERNEL); + __drm_atomic_helper_crtc_reset(crtc, &state->state); if (state == NULL) return; state->crc.source = VSP1_DU_CRC_NONE; state->crc.index = 0; - - crtc->state = &state->state; - crtc->state->crtc = crtc; } static int rcar_du_crtc_enable_vblank(struct drm_crtc *crtc)
Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of writing its own version. Instead of open coding destroy_state(), call it directly for freeing the old state. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> Cc: linux-renesas-soc@vger.kernel.org --- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)