Re: [PATCH 1/2] drm/i915: Clean up skl_plane_has_planar()

From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
To: "ville.syrjala@linux.intel.com" <ville.syrjala@linux.intel.com>,
	"Vivi, Rodrigo" <rodrigo.vivi@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/2] drm/i915: Clean up skl_plane_has_planar()
Date: Mon, 27 Aug 2018 17:53:43 +0000	[thread overview]
Message-ID: <f335d818de3ae397cefe5949276e4e2088c6bc7f.camel@intel.com> (raw)
In-Reply-To: <20180827115618.GP5565@intel.com>


On Mon, 2018-08-27 at 14:56 +0300, Ville Syrjälä wrote:
> On Fri, Aug 24, 2018 at 01:38:55PM -0700, Dhinakaran Pandiyan wrote:
> > skl_plane_has_planar is hard to read, simplify the logic by
> > checking for
> > support in the order of platform, pipe and plane.
> 
> I had a slightly different version of this somewhere. But this one
> might
> be even better.
> 
> > 
> > No change in functionality intended.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++---------------
> > ---
> >  1 file changed, 9 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 30fdfd1a3037..7e18bd8b21b8 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13622,24 +13622,15 @@ static bool skl_plane_has_fbc(struct
> > drm_i915_private *dev_priv,
> >  bool skl_plane_has_planar(struct drm_i915_private *dev_priv,
> >  			  enum pipe pipe, enum plane_id plane_id)
> >  {
> > -	if (plane_id == PLANE_PRIMARY) {
> > -		if (IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv))
> > -			return false;
> > -		else if ((INTEL_GEN(dev_priv) == 9 && pipe ==
> > PIPE_C) &&
> > -			 !IS_GEMINILAKE(dev_priv))
> > -			return false;
> > -	} else if (plane_id >= PLANE_SPRITE0) {
> > -		if (plane_id == PLANE_CURSOR)
> > -			return false;
> > -		if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv)
> > == 10) {
> > -			if (plane_id != PLANE_SPRITE0)
> > -				return false;
> > -		} else {
> > -			if (plane_id != PLANE_SPRITE0 || pipe ==
> > PIPE_C ||
> > -			    IS_SKYLAKE(dev_priv) ||
> > IS_BROXTON(dev_priv))
> > -				return false;
> > -		}
> > -	}
> > +	if (IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv))
> > +		return false;
> > +
> > +	if (INTEL_GEN(dev_priv) == 9 && !IS_GEMINILAKE(dev_priv)
> > && pipe == PIPE_C)
> > +		return false;
> > +
> > +	if (plane_id == PLANE_CURSOR || plane_id != PLANE_SPRITE0)
> > +		return false;
> 
> The cursor check is rather redundant here. IIRC I put it at the very
> start of the function to make it obvious that cursor never supports
> this. But we could just as well drop the check entirely.
> 
> This also disables NV12 for the primary plane which isn't correct.
That's a good catch, thanks.
Switch the condition to
	if (plane_id != PLANE_PRIMARY && plane_id != PLANE_SPRITE0)
		
return false;
?

> 
> > +
> >  	return true;
> >  }
> >  
> > -- 
> > 2.17.1
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx