Re: [PATCH 01/15] drm: omapdrm: Fix plane state free in plane reset handler

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

Hi Tomi,

On Wednesday 09 December 2015 14:43:19 Tomi Valkeinen wrote:
> On 05/12/15 00:27, Laurent Pinchart wrote:
> > The plane reset handler frees the plane state and allocates a new
> > default state, but when doing so attempt to free the plane state using
> > the base plane state pointer instead of casting it to the
> > driver-specific state object that has been allocated. Fix it by using
> > the omap_plane_atomic_destroy_state() function to destroy the plane
> > state instead of duplicating the code.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_plane.c | 53 ++++++++++++++++---------------
> >  1 file changed, 26 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
> > b/drivers/gpu/drm/omapdrm/omap_plane.c index 3054bda72688..11d406b160e1
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> > @@ -188,33 +188,6 @@ static const struct drm_plane_helper_funcs
> > omap_plane_helper_funcs = {
> >  	.atomic_disable = omap_plane_atomic_disable,
> >  };
> > 
> > -static void omap_plane_reset(struct drm_plane *plane)
> > -{
> > -	struct omap_plane *omap_plane = to_omap_plane(plane);
> > -	struct omap_plane_state *omap_state;
> > -
> > -	if (plane->state && plane->state->fb)
> > -		drm_framebuffer_unreference(plane->state->fb);
> > -
> > -	kfree(plane->state);
> > -	plane->state = NULL;
> > -
> > -	omap_state = kzalloc(sizeof(*omap_state), GFP_KERNEL);
> > -	if (omap_state == NULL)
> > -		return;
> > -
> > -	/*
> > -	 * Set defaults depending on whether we are a primary or overlay
> > -	 * plane.
> > -	 */
> > -	omap_state->zorder = plane->type == DRM_PLANE_TYPE_PRIMARY
> > -			   ? 0 : omap_plane->id;
> > -	omap_state->base.rotation = BIT(DRM_ROTATE_0);
> > -
> > -	plane->state = &omap_state->base;
> > -	plane->state->plane = plane;
> > -}
> > -
> >  static void omap_plane_destroy(struct drm_plane *plane)
> >  {
> >  	struct omap_plane *omap_plane = to_omap_plane(plane);
> > @@ -270,6 +243,32 @@ static void omap_plane_atomic_destroy_state(struct
> > drm_plane *plane,
> >  	kfree(to_omap_plane_state(state));
> >  }
> > 
> > +static void omap_plane_reset(struct drm_plane *plane)
> > +{
> > +	struct omap_plane *omap_plane = to_omap_plane(plane);
> > +	struct omap_plane_state *omap_state;
> > +
> > +	if (plane->state) {
> > +		omap_plane_atomic_destroy_state(plane, plane->state);
> > +		plane->state = NULL;
> > +	}
> > +
> > +	omap_state = kzalloc(sizeof(*omap_state), GFP_KERNEL);
> > +	if (omap_state == NULL)
> > +		return;
> > +
> > +	/*
> > +	 * Set defaults depending on whether we are a primary or overlay
> > +	 * plane.
> > +	 */
> > +	omap_state->zorder = plane->type == DRM_PLANE_TYPE_PRIMARY
> > +			   ? 0 : omap_plane->id;
> > +	omap_state->base.rotation = BIT(DRM_ROTATE_0);
> > +
> > +	plane->state = &omap_state->base;
> > +	plane->state->plane = plane;
> > +}
> > +
> >  static int omap_plane_atomic_set_property(struct drm_plane *plane,
> >  					  struct drm_plane_state *state,
> >  					  struct drm_property *property,
> 
> This one moves the function, so it's pretty hard to see what actually
> was changed.

It's unfortunately needed if we want to avoid forward declarations. I could 
split the patch in two, but given the size of the function and the extent of 
the change I thought it wouldn't be worth it.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel