Re: [PATCH 1/5] drm/i915: Separate cherryview from valleyview

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Dec 08, 2015 at 01:44:10PM +0200, Jani Nikula wrote:
> On Mon, 07 Dec 2015, Wayne Boyer <wayne.boyer@intel.com> wrote:
> > The cherryview device shares many characteristics with the valleyview
> > device.  When support was added to the driver for cherryview, the
> > corresponding device info structure included .is_valleyview = 1.
> > This is not correct and leads to some confusion.
> >
> > This patch changes .is_valleyview to .is_cherryview in the cherryview
> > device info structure and simplifies the IS_CHERRYVIEW macro.
> > Then where appropriate, instances of IS_VALLEYVIEW are replaced with
> > IS_VALLEYVIEW || IS_CHERRYVIEW or equivalent.
> >
> > v2: Use IS_VALLEYVIEW || IS_CHERRYVIEW instead of defining a new macro.
> >     Also add followup patches to fix issues discovered during the first
> >     review. (Ville)
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Wayne Boyer <wayne.boyer@intel.com>
> 
> I eyeballed through the changes here, comments inline. I didn't check
> which places you missed, if any.
> 
> BR,
> Jani.
> 
> 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index a8721fc..a9ae642 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1142,8 +1142,34 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> >  			   MEMSTAT_VID_SHIFT);
> >  		seq_printf(m, "Current P-state: %d\n",
> >  			   (rgvstat & MEMSTAT_PSTATE_MASK) >> MEMSTAT_PSTATE_SHIFT);
> > -	} else if (IS_GEN6(dev) || (IS_GEN7(dev) && !IS_VALLEYVIEW(dev)) ||
> > -		   IS_BROADWELL(dev) || IS_GEN9(dev)) {
> > +	} else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
> > +		u32 freq_sts;
> > +
> > +		mutex_lock(&dev_priv->rps.hw_lock);
> > +		freq_sts = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> > +		seq_printf(m, "PUNIT_REG_GPU_FREQ_STS: 0x%08x\n", freq_sts);
> > +		seq_printf(m, "DDR freq: %d MHz\n", dev_priv->mem_freq);
> > +
> > +		seq_printf(m, "actual GPU freq: %d MHz\n",
> > +			   intel_gpu_freq(dev_priv, (freq_sts >> 8) & 0xff));
> > +
> > +		seq_printf(m, "current GPU freq: %d MHz\n",
> > +			   intel_gpu_freq(dev_priv, dev_priv->rps.cur_freq));
> > +
> > +		seq_printf(m, "max GPU freq: %d MHz\n",
> > +			   intel_gpu_freq(dev_priv, dev_priv->rps.max_freq));
> > +
> > +		seq_printf(m, "min GPU freq: %d MHz\n",
> > +			   intel_gpu_freq(dev_priv, dev_priv->rps.min_freq));
> > +
> > +		seq_printf(m, "idle GPU freq: %d MHz\n",
> > +			   intel_gpu_freq(dev_priv, dev_priv->rps.idle_freq));
> > +
> > +		seq_printf(m,
> > +			   "efficient (RPe) frequency: %d MHz\n",
> > +			   intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq));
> > +		mutex_unlock(&dev_priv->rps.hw_lock);
> > +	} else if (IS_GEN6(dev) || IS_BROADWELL(dev) || IS_GEN9(dev)) {
> 
> Where did IS_GEN7() go?

With VLV/CHV out of the way This can be 'gen >= 6 && gen < 10'. Maybe we
could even drop the upper bound and just assume it'll keep working?

> 
> >  		u32 rp_state_limits;
> >  		u32 gt_perf_status;
> >  		u32 rp_state_cap;
> > @@ -1284,33 +1310,6 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> >  		seq_printf(m,
> >  			   "efficient (RPe) frequency: %d MHz\n",
> >  			   intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq));
> > -	} else if (IS_VALLEYVIEW(dev)) {
> > -		u32 freq_sts;
> > -
> > -		mutex_lock(&dev_priv->rps.hw_lock);
> > -		freq_sts = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> > -		seq_printf(m, "PUNIT_REG_GPU_FREQ_STS: 0x%08x\n", freq_sts);
> > -		seq_printf(m, "DDR freq: %d MHz\n", dev_priv->mem_freq);
> > -
> > -		seq_printf(m, "actual GPU freq: %d MHz\n",
> > -			   intel_gpu_freq(dev_priv, (freq_sts >> 8) & 0xff));
> > -
> > -		seq_printf(m, "current GPU freq: %d MHz\n",
> > -			   intel_gpu_freq(dev_priv, dev_priv->rps.cur_freq));
> > -
> > -		seq_printf(m, "max GPU freq: %d MHz\n",
> > -			   intel_gpu_freq(dev_priv, dev_priv->rps.max_freq));
> > -
> > -		seq_printf(m, "min GPU freq: %d MHz\n",
> > -			   intel_gpu_freq(dev_priv, dev_priv->rps.min_freq));
> > -
> > -		seq_printf(m, "idle GPU freq: %d MHz\n",
> > -			   intel_gpu_freq(dev_priv, dev_priv->rps.idle_freq));
> > -
> > -		seq_printf(m,
> > -			   "efficient (RPe) frequency: %d MHz\n",
> > -			   intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq));
> > -		mutex_unlock(&dev_priv->rps.hw_lock);
> >  	} else {
> >  		seq_puts(m, "no P-state info available\n");
> >  	}
> 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index a81c766..b156b08e 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> 
> > @@ -836,7 +836,7 @@ static void intel_device_info_runtime_init(struct drm_device *dev)
> >  
> >  static void intel_init_dpio(struct drm_i915_private *dev_priv)
> >  {
> > -	if (!IS_VALLEYVIEW(dev_priv))
> > +	if (!(IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)))
> 
> Hmm, I think I'd probably end up writing this as:
> 
> 	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
> 
> Matter of style and phase of the moon I guess. But it's nicer
> particularly when it's combined with other !platforms.
> 
> >  		return;
> >  
> >  	/*
> 
> 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 43761c5..4b1161d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -190,7 +190,7 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size)
> >  	 * would make the object snooped which might have a
> >  	 * negative performance impact.
> >  	 */
> > -	if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev)) {
> > +	if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
> 
> Ahah, you seem to be ambivalent about !(vlv || chv) vs. !vlv && !chv
> yourself. ;)
> 
> Also makes me think you didn't use sed or coccinelle for the change.
> 
> >  		ret = i915_gem_object_set_cache_level(obj, I915_CACHE_L3_LLC);
> >  		/* Failure shouldn't ever happen this early */
> >  		if (WARN_ON(ret)) {
> 
> 
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > index 070470f..e384d24 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -357,8 +357,8 @@ parse_general_features(struct drm_i915_private *dev_priv,
> >  	if (general) {
> >  		dev_priv->vbt.int_tv_support = general->int_tv_support;
> >  		/* int_crt_support can't be trusted on earlier platforms */
> > -		if (bdb->version >= 155 &&
> > -		    (HAS_DDI(dev_priv) || IS_VALLEYVIEW(dev_priv)))
> > +		if (bdb->version >= 155 && (HAS_DDI(dev_priv) ||
> > +		    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)))
> 
> Hmm, is that the same as gen >= 7 && !ivb?

This can be left alone. CHV doesn't do CRT anyway.

> 
> >  			dev_priv->vbt.int_crt_support = general->int_crt_support;
> >  		dev_priv->vbt.lvds_use_ssc = general->enable_ssc;
> >  		dev_priv->vbt.lvds_ssc_freq =
> 
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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