On Tue, Dec 08, 2015 at 01:44:10PM +0200, Jani Nikula wrote:
> On Mon, 07 Dec 2015, Wayne Boyer <wayne.boyer@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Signed-off-by: Wayne Boyer <wayne.boyer@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
