Re: [PATCH 3/8] drm/i915: Unconditionally clear plane visibility, v2.

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/8] drm/i915: Unconditionally clear plane visibility, v2.
Date: Fri, 21 Sep 2018 18:26:53 +0300	[thread overview]
Message-ID: <20180921152653.GC5565@intel.com> (raw)
In-Reply-To: <20180920102711.4184-4-maarten.lankhorst@linux.intel.com>

On Thu, Sep 20, 2018 at 12:27:06PM +0200, Maarten Lankhorst wrote:
> We need to assume the plane has been visible before, even if no CRTC
> is assigned to the plane. This is because nv12 will enable a a extra
> plane and make it visible by marking it in crtc_state->active_planes
> for intel_update_planes_on_crtc().
> 
> Additionally, clear visible flag in intel_plane_atomic_check, in case
> we ever hit a bug with visibility. Our code implicitly assumes that
> plane_state->visible is only true when crtc and fb are set,
> so we will either null deref in intel_fbc_choose_crtc() or
> do something bad during the actual commit which cares even more.
> 
> Changes since v1:
> - Unconditionally clear crtc_state->active_planes as well.
> - Reword commit message, since this is now a preparation patch for
>   NV12 Y / UV plane linking.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index aabebe0d2e9b..f70e9cb9cf02 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -117,10 +117,13 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	int ret;
>  
> +	crtc_state->active_planes &= ~BIT(intel_plane->id);

nv12_planes too?

> +	intel_state->base.visible = false;
> +
> +	/* If this is a cursor plane, no further checks are needed. */
>  	if (!intel_state->base.crtc && !old_plane_state->base.crtc)
>  		return 0;
>  
> -	intel_state->base.visible = false;
>  	ret = intel_plane->check_plane(crtc_state, intel_state);
>  	if (ret)
>  		return ret;
> @@ -128,8 +131,6 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>  	/* FIXME pre-g4x don't work like this */
>  	if (state->visible)
>  		crtc_state->active_planes |= BIT(intel_plane->id);
> -	else
> -		crtc_state->active_planes &= ~BIT(intel_plane->id);
>  
>  	if (state->visible && state->fb->format->format == DRM_FORMAT_NV12)
>  		crtc_state->nv12_planes |= BIT(intel_plane->id);
> @@ -152,6 +153,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  	const struct drm_crtc_state *old_crtc_state;
>  	struct drm_crtc_state *new_crtc_state;
>  
> +	new_plane_state->visible = false;
>  	if (!crtc)
>  		return 0;
>  
> -- 
> 2.18.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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