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

From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
To: "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: Fri, 24 Aug 2018 23:38:26 +0000	[thread overview]
Message-ID: <3e6d05c85749230197f59e7fd7c5bf93e91feaf4.camel@intel.com> (raw)
In-Reply-To: <20180824231609.GH6432@intel.com>

On Fri, 2018-08-24 at 16:16 -0700, Rodrigo Vivi wrote:
> On Fri, Aug 24, 2018 at 10:27:09PM +0000, Pandiyan, Dhinakaran wrote:
> > 
> > 
> > On Fri, 2018-08-24 at 15:15 -0700, Rodrigo Vivi 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.
> > > > 
> > > > No change in functionality intended.
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.c
> > > > om>
> > > > ---
> > > >  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;
> > 
> > ^ Here it does, or am I not seeing the paranthesis correctly.
> 
> dam... it is so bad that I got confused when drawing a
> table here...
> 
> nip: The name of the function seems wrong since "skl_" always
> return false....
Looks like the first versions of the patch implementing nv12 did
include SKL, not clear what happened later.

> but I don't have a better suggestion ... maybe just "intel_"
> and can be in a follow up patch...
> 
> so,
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Thanks!

-DK

> 
> > 
> > > > -		}
> > > > -	}
> > > > +	if (IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv))
> > > > +		return false;
> > > 
> > > The current code is indeed ugly, but I'm afraid it doesn't always
> > > return
> > > false for these platforms.
> > > 
> > > for instance plane_id = PLANE_SPRITE0
> > > 
> > > > +
> > > > +	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;
> > > > +
> > > >  	return true;
> > > >  }
> > > >  
> > > > -- 
> > > > 2.17.1
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx