[Intel-gfx] [PATCH 3/3] drm/atomic-helper: Don't call atomic_update_plane when it stays off

[Daniel Vetter]

On Thu, Apr 16, 2015 at 12:46:53AM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Friday 10 April 2015 16:22:39 Daniel Vetter wrote:
> > It's a silly thing to do and surprises driver writers. Most likely
> > this did already blow up for exynos.
> > 
> > It's also a silly thing to change plane state when it's off, but fbdev
> > is silly (it does an unconditional modeset over all planes). And
> > userspace can be evil. So I think we need this.
> > 
> > With this check in the helpers we can remove the one in i915 code for
> > the same conditions (becuase ->crtc iff ->fb).
> > 
> > Cc: Gustavo Padovan <gustavo at padovan.org>
> > Cc: dri-devel at lists.freedesktop.org
> > Cc: Inki Dae <inki.dae at samsung.com>
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> 
> Acked-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Tested-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> (with the ongoing omapdrm atomic update conversion work)
> 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c       | 3 ++-
> >  drivers/gpu/drm/i915/intel_atomic_plane.c | 4 ----
> >  2 files changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c index 41c38edade74..e1556143d811
> > 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1216,7 +1216,8 @@ void drm_atomic_helper_commit_planes(struct drm_device
> > *dev, if (drm_atomic_plane_disabling(plane, old_plane_state) &&
> >  		    funcs->atomic_disable)
> >  			funcs->atomic_disable(plane, old_plane_state);
> > -		else
> > +		else if (plane->state->crtc ||
> > +			 drm_atomic_plane_disabling(plane, old_plane_state))
> >  			funcs->atomic_update(plane, old_plane_state);
> 
> The test is so trivial that I wonder whether it makes sense to make 
> atomic_disable() optional. Wouldn't it be easier to either make 
> atomic_disable() mandatory, or to remove it completely ?

Yeah it's fairly simple. I think it's more to give drivers some guidance
and to more closely resemble the legacy plane entry points which also had
the split between update and disable. Imo doesn't really hurt to keep this
around.

Merged to topic/drm-misc, thanks for the feedback.
-Daniel

> 
> >  	}
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > b/drivers/gpu/drm/i915/intel_atomic_plane.c index
> > 976b89156570..cb383a0fc392 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -172,10 +172,6 @@ static void intel_plane_atomic_update(struct drm_plane
> > *plane, struct intel_plane_state *intel_state =
> >  		to_intel_plane_state(plane->state);
> > 
> > -	/* Don't disable an already disabled plane */
> > -	if (!plane->state->fb && !old_state->fb)
> > -		return;
> > -
> >  	intel_plane->commit_plane(plane, intel_state);
> >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch