Re: [PATCH v2 2/6] drm/atomic: Unconditionally call prepare_fb.

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

Hi Maarten,

Thank you for the patch.

On Monday 19 Dec 2016 12:08:16 Maarten Lankhorst wrote:
> Op 13-12-16 om 18:10 schreef Daniel Vetter:
> > On Tue, Dec 13, 2016 at 03:13:54PM +0100, Maarten Lankhorst wrote:
> >> Op 09-12-16 om 09:25 schreef Daniel Vetter:
> >>> On Fri, Dec 09, 2016 at 12:42:19AM +0200, Laurent Pinchart wrote:
> >>>> 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)?
> >> 
> >> Waiting on a reply, but what about this commit message for this patch?
> >> ---
> >> 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.
> >> 
> >> This is 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]
> >> 
> >> The original commit mentions that this prevents waiting in i915 on all
> >> previous rendering during cursor updates, but there are better ways to
> >> fix this.
> >> 
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > 
> > Yeah sounds good to me. Since we don't want to backport all the i915
> > cursor patches no cc: stable on this. Also, this is only an issue for
> > drivers which both have a cursor plane, and implement that cursor using
> > universal planes (i.e. settting drm_crtc->cursor). Afaik the only two are
> > vc4 and i915, and after this series both will have appropriate hacks (for
> > now) to keep existing userspace happy.
> > -Daniel
> 
> Same patch, reworded!
> ---8<---
> 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.
> 
> This is 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]
> 
> The original commit mentions that this prevents waiting in i915 on all
> previous rendering during cursor updates, but there are better ways to
> fix this.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx