Re: [PATCH 12/17] drm/rcar-du: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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().

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel