[Intel-gfx] [PATCH] drm/i915: Simplify has_sagv

Tue Oct 23 00:06:11 UTC 2018

On Mon, Oct 22, 2018 at 04:48:50PM -0700, Paulo Zanoni wrote:
> Em Seg, 2018-10-22 às 09:57 -0700, Rodrigo Vivi escreveu:
> > Let's add a platform has_sagv instead of having a full
> > function that handle platform by platform.
> > 
> > The specially case for SKL for not controlled sagv
> > is already taken care inside intel_enable_sagv, so there's
> > no need to duplicate the check here.
> > 
> > v2: Go one step further and remove skl special case. (Jani)
> > 
> > v3: Separate runtime status handle from has_sagv flag. (Jani)
> 
> I know this has probably been discussed in the past and I missed it
> (since I know this was done a few times for other things), but how/why
> is this approach better than the current one? Can you please write it
> in the commit message?

my bad... after stripping this out of my RFC series this got
indeed out of context.

I'm trying to get rid of many platform codename checks spread
around the code.

I believe we should do most of the decisions based on display_gen
or has_feature. And use the platform codename as the last resource
only when gen or feature cannot describe things better.

> Quickly checking the implementation of intel_has_sagv() seems much
> easier than swimming through the nested macros of i915_pci.c. And the 
> special-caseness of the NOT_CONTROLLED case being restricted to gen9 in
> the current (non-patched) code also at first appears to be better than
> this proposal. But I'm totally willing to read your arguments and
> reevaluate my decisions based on them, so please present them.

When you are looking only to a single feature this might be true.
When you are looking to the platform itself consolidating the features
definitions in a single place is easier to check legacy features and
add new platforms.

Specially I'd like to add a new platform gen line gen++ without
need to use any codename. Or without mixing things up.
INTEL_GEN >= 11 || IS_CANNONLAKE :/

So adding gen_n+1 should be as trivial
as gen_n + 1 legacy...

> 
> > 
> > Cc: Jani Nikula <jani.nikula at intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h          |  1 +
> >  drivers/gpu/drm/i915/i915_pci.c          |  1 +
> >  drivers/gpu/drm/i915/intel_device_info.h |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c          | 29 ++++++++------------
> > ----
> >  4 files changed, 13 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 3017ef037fed..9b98ceb2d029 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2650,6 +2650,7 @@ intel_info(const struct drm_i915_private
> > *dev_priv)
> >  #define HAS_DDI(dev_priv)		 ((dev_priv)->info.has_ddi)
> >  #define HAS_FPGA_DBG_UNCLAIMED(dev_priv) ((dev_priv)-
> > >info.has_fpga_dbg)
> >  #define HAS_PSR(dev_priv)		 ((dev_priv)->info.has_psr)
> > +#define HAS_SAGV(dev_priv)		 ((dev_priv)-
> > >info.has_sagv)
> >  
> >  #define HAS_RC6(dev_priv)		 ((dev_priv)->info.has_rc6)
> >  #define HAS_RC6p(dev_priv)		 ((dev_priv)-
> > >info.has_rc6p)
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c
> > b/drivers/gpu/drm/i915/i915_pci.c
> > index 44e745921ac1..0b09155eab62 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -465,6 +465,7 @@ static const struct intel_device_info
> > intel_cherryview_info = {
> >  	.has_csr = 1, \
> >  	.has_guc = 1, \
> >  	.has_ipc = 1, \
> > +	.has_sagv = 1, \
> >  	.ddb_size = 896
> >  
> >  #define SKL_PLATFORM \
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.h
> > b/drivers/gpu/drm/i915/intel_device_info.h
> > index af7002640cdf..e77c8b62783f 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > @@ -117,6 +117,7 @@ enum intel_ppgtt {
> >  	func(hws_needs_physical); \
> >  	func(overlay_needs_physical); \
> >  	func(supports_tv); \
> > +	func(has_sagv); \
> >  	func(has_ipc);
> >  
> >  #define GEN_MAX_SLICES		(6) /* CNL upper bound */
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 67a4d0735291..7e38ed8421c7 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3609,20 +3609,6 @@ static bool skl_needs_memory_bw_wa(struct
> > intel_atomic_state *state)
> >  	return false;
> >  }
> >  
> > -static bool
> > -intel_has_sagv(struct drm_i915_private *dev_priv)
> > -{
> > -	if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
> > -	    IS_CANNONLAKE(dev_priv) || IS_ICELAKE(dev_priv))
> > -		return true;
> > -
> > -	if (IS_SKYLAKE(dev_priv) &&
> > -	    dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED)
> > -		return true;
> > -
> > -	return false;
> > -}
> > -
> >  /*
> >   * SAGV dynamically adjusts the system agent voltage and clock
> > frequencies
> >   * depending on power and performance requirements. The display
> > engine access
> > @@ -3639,10 +3625,11 @@ intel_enable_sagv(struct drm_i915_private
> > *dev_priv)
> >  {
> >  	int ret;
> >  
> > -	if (!intel_has_sagv(dev_priv))
> > +	if (!HAS_SAGV(dev_priv))
> >  		return 0;
> >  
> > -	if (dev_priv->sagv_status == I915_SAGV_ENABLED)
> > +	if (dev_priv->sagv_status == I915_SAGV_ENABLED ||
> > +	    dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED)
> >  		return 0;
> >  
> >  	DRM_DEBUG_KMS("Enabling the SAGV\n");
> > @@ -3676,10 +3663,11 @@ intel_disable_sagv(struct drm_i915_private
> > *dev_priv)
> >  {
> >  	int ret;
> >  
> > -	if (!intel_has_sagv(dev_priv))
> > +	if (!HAS_SAGV(dev_priv))
> >  		return 0;
> >  
> > -	if (dev_priv->sagv_status == I915_SAGV_DISABLED)
> > +	if (dev_priv->sagv_status == I915_SAGV_DISABLED ||
> > +	    dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED)
> >  		return 0;
> >  
> >  	DRM_DEBUG_KMS("Disabling the SAGV\n");
> > @@ -3721,7 +3709,10 @@ bool intel_can_enable_sagv(struct
> > drm_atomic_state *state)
> >  	int level, latency;
> >  	int sagv_block_time_us;
> >  
> > -	if (!intel_has_sagv(dev_priv))
> > +	if (!HAS_SAGV(dev_priv))
> > +		return false;
> > +
> > +	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED)
> >  		return false;
> >  
> >  	if (IS_GEN9(dev_priv))