From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Date: Fri, 14 Aug 2015 07:30:15 +0000 Subject: Re: [PATCH] drm: rcar-du: Fix plane state free in plane reset handler Message-Id: <20150814073015.GG17734@phenom.ffwll.local> List-Id: References: <1438820580-14301-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <20150807125321.GE25792@ulmo> <2325494.jkNV16dCnQ@avalon> <1768630.dTmQoR56Ge@avalon> In-Reply-To: <1768630.dTmQoR56Ge@avalon> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Laurent Pinchart Cc: Thierry Reding , Laurent Pinchart , dri-devel@lists.freedesktop.org, linux-sh@vger.kernel.org, Daniel Vetter On Fri, Aug 14, 2015 at 12:19:03AM +0300, Laurent Pinchart wrote: > On Friday 07 August 2015 17:30:08 Laurent Pinchart wrote: > > On Friday 07 August 2015 14:53:22 Thierry Reding wrote: > > > On Thu, Aug 06, 2015 at 03:23:00AM +0300, 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 rcar_du_plane_atomic_destroy_state() function to destroy the plane > > > > state instead of duplicating the code. > > > > > > > > Signed-off-by: Laurent Pinchart > > > > > > > > --- > > > > > > > > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 45 ++++++++++++-------------- > > > > 1 file changed, 22 insertions(+), 23 deletions(-) > > > > > > > > Should the DRM core free the atomic state before calling the reset > > > > operation ? That would simplify drivers. > > > > > > The core can't do that because drivers might have subclassed the state. > > > > But the core can call the .atomic_destroy_state() operation, can't it ? > > Thierry, Daniel, any comment on this ? Doesn't really help you since the kzalloc is still in the helper. Btw this is all helper code, core won't do here anything at all ;-) But we could still do it simply as a refactoring and to show drivers how to implement their _reset with less copypasting. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm: rcar-du: Fix plane state free in plane reset handler Date: Fri, 14 Aug 2015 09:30:15 +0200 Message-ID: <20150814073015.GG17734@phenom.ffwll.local> References: <1438820580-14301-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <20150807125321.GE25792@ulmo> <2325494.jkNV16dCnQ@avalon> <1768630.dTmQoR56Ge@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1768630.dTmQoR56Ge@avalon> Sender: linux-sh-owner@vger.kernel.org To: Laurent Pinchart Cc: Thierry Reding , Laurent Pinchart , dri-devel@lists.freedesktop.org, linux-sh@vger.kernel.org, Daniel Vetter List-Id: dri-devel@lists.freedesktop.org On Fri, Aug 14, 2015 at 12:19:03AM +0300, Laurent Pinchart wrote: > On Friday 07 August 2015 17:30:08 Laurent Pinchart wrote: > > On Friday 07 August 2015 14:53:22 Thierry Reding wrote: > > > On Thu, Aug 06, 2015 at 03:23:00AM +0300, 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 rcar_du_plane_atomic_destroy_state() function to destroy the plane > > > > state instead of duplicating the code. > > > > > > > > Signed-off-by: Laurent Pinchart > > > > > > > > --- > > > > > > > > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 45 ++++++++++++-------------- > > > > 1 file changed, 22 insertions(+), 23 deletions(-) > > > > > > > > Should the DRM core free the atomic state before calling the reset > > > > operation ? That would simplify drivers. > > > > > > The core can't do that because drivers might have subclassed the state. > > > > But the core can call the .atomic_destroy_state() operation, can't it ? > > Thierry, Daniel, any comment on this ? Doesn't really help you since the kzalloc is still in the helper. Btw this is all helper code, core won't do here anything at all ;-) But we could still do it simply as a refactoring and to show drivers how to implement their _reset with less copypasting. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch