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

[Maarten Lankhorst]

Op 15-06-15 om 14:05 schreef Daniel Vetter:
> 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 at 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
Good point!

~Maarten