Re: [PATCH v2 14/27] drm/i915: clean up atomic plane check functions

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jun 11, 2015 at 06:23:09AM +0200, Maarten Lankhorst wrote:
> Op 11-06-15 om 03:35 schreef Matt Roper:
> > On Thu, Jun 04, 2015 at 02:47:44PM +0200, Maarten Lankhorst wrote:
> >> By passing crtc_state to the check_plane functions a lot of duplicated
> >> code can be removed. And now that the transitional helpers are gone the
> >> crtc_state can be reliably obtained.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_atomic_plane.c |  4 ++-
> >>  drivers/gpu/drm/i915/intel_display.c      | 48 ++++++++++---------------------
> >>  drivers/gpu/drm/i915/intel_drv.h          |  1 +
> >>  drivers/gpu/drm/i915/intel_sprite.c       | 13 +++------
> >>  4 files changed, 23 insertions(+), 43 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> index aa2128369a0a..4d8cacbca777 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> @@ -119,6 +119,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >>  	crtc = crtc ? crtc : plane->crtc;
> >>  	intel_crtc = to_intel_crtc(crtc);
> >>  
> >> +	intel_state->visible = false;
> >> +
> > What do we need this change for?  Primary and cursor check functions
> > immediately overwrite state->visible, so setting this here has no
> > effect.  The sprite case where fb==NULL is the only case where this
> > would matter, but moving the assignment from the sprite check function
> > to here doesn't seem like it gains us anything.
> >
> I was using it to clear intel_state->visible even if no crtc is set. In that case state->visible
> should already be false, but a bit of paranoia never hunts. :-)

Resetting of state should be done in the duplicate functions. This makes
sure we never ever forget to compute it correctly ;-) Sprinkling clearing
code all over (and especially over the compute code) is imo fragile and
should be avoided if possible.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx