Re: [Intel-gfx] [PATCH 2/6] drm/atomic: Unconditionally call prepare_fb.

From: Daniel Vetter <daniel@ffwll.ch>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 2/6] drm/atomic: Unconditionally call prepare_fb.
Date: Fri, 9 Dec 2016 09:25:49 +0100	[thread overview]
Message-ID: <20161209082549.jdzl2kinu3j7zacv@phenom.ffwll.local> (raw)
In-Reply-To: <1995226.mylCh0k4y5@avalon>

On Fri, Dec 09, 2016 at 12:42:19AM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Thursday 08 Dec 2016 16:41:04 Daniel Vetter wrote:
> > On Thu, Dec 08, 2016 at 02:45:25PM +0100, Maarten Lankhorst wrote:
> > > Atomic drivers may set properties like rotation on the same fb, which
> > > may require a call to prepare_fb even when framebuffer stays identical.
> > > 
> > > Instead of handling all the special cases in the core, let the driver
> > > decide when prepare_fb and cleanup_fb are noops.
> > > 
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > 
> > I think this makes sense, but would be really good to get a pile of acks
> > from driver maintainers on this one. Rob, Eric, Laurent, others?
> 
> This is all very nice, but it will introduce at least a performance 
> regression, and possibly worse, until drivers get updated. There are 7 drivers 
> implementing the .prepare_fb() callback (plus a bunch of drivers that probably 
> should use drm_fb_cma_prepare_fb() but don't at the moment). I can't ack this 
> patch before they get fixed.

Maarten's commit message is insufficient, since this is defacto a revert
of

commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
Author: Keith Packard <keithp@keithp.com>
Date:   Sat Jun 4 01:16:22 2016 -0700

    drm: Don't prepare or cleanup unchanging frame buffers [v3]

because that breaks stuff. We're simply going back to where we've been a
few months ago. Since this is a regression fix, back to original
behaviour, can you ack (assuming Maarten updates the commit message to
reflect the nature of the commit here)?
-Daniel

> 
> > > ---
> > > 
> > >  drivers/gpu/drm/drm_atomic_helper.c | 9 ---------
> > >  1 file changed, 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > b/drivers/gpu/drm/drm_atomic_helper.c index 23767df72615..d19563651e07
> > > 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -1664,9 +1664,6 @@ int drm_atomic_helper_prepare_planes(struct
> > > drm_device *dev,> 
> > >  		funcs = plane->helper_private;
> > > 
> > > -		if (!drm_atomic_helper_framebuffer_changed(dev, state,
> > > plane_state->crtc)) -			continue;
> > > -
> > > 
> > >  		if (funcs->prepare_fb) {
> > >  		
> > >  			ret = funcs->prepare_fb(plane, plane_state);
> > >  			if (ret)
> > > 
> > > @@ -1683,9 +1680,6 @@ int drm_atomic_helper_prepare_planes(struct
> > > drm_device *dev,> 
> > >  		if (j >= i)
> > >  		
> > >  			continue;
> > > 
> > > -		if (!drm_atomic_helper_framebuffer_changed(dev, state,
> > > plane_state->crtc)) -			continue;
> > > -
> > > 
> > >  		funcs = plane->helper_private;
> > >  		
> > >  		if (funcs->cleanup_fb)
> > > 
> > > @@ -1952,9 +1946,6 @@ void drm_atomic_helper_cleanup_planes(struct
> > > drm_device *dev,> 
> > >  	for_each_plane_in_state(old_state, plane, plane_state, i) {
> > >  	
> > >  		const struct drm_plane_helper_funcs *funcs;
> > > 
> > > -		if (!drm_atomic_helper_framebuffer_changed(dev, old_state,
> > > plane_state->crtc)) -			continue;
> > > -
> > > 
> > >  		funcs = plane->helper_private;
> > >  		
> > >  		if (funcs->cleanup_fb)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel