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

[Jani Nikula <jani.nikula@linux.intel.com>]

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?

>  		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?

>  			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