Re: [PATCH 2/9] drm/i915: add HAS_FORCEWAKE flag to uncore

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>,
	Michal Wajdeczko <michal.wajdeczko@intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Subject: Re: [PATCH 2/9] drm/i915: add HAS_FORCEWAKE flag to uncore
Date: Tue, 26 Mar 2019 21:48:51 +0000	[thread overview]
Message-ID: <155363692641.7042.14235605903868115478@skylake-alporthouse-com> (raw)
In-Reply-To: <op.zy9e4mjuxaggs7@mwajdecz-mobl1.ger.corp.intel.com>

Quoting Michal Wajdeczko (2019-03-26 12:47:00)
> On Mon, 25 Mar 2019 22:49:33 +0100, Daniele Ceraolo Spurio  
> <daniele.ceraolospurio@intel.com> wrote:
> 
> > We have several cases where we don't have forcewake (older gens, GVT and
> > planned display-only uncore), so, instead of checking every time against
> > the various condition, save the info in a flag and use that.
> >
> > Note that this patch also change the behavior for gen5 with vpgu
> > enabled, but this is not an issue since we don't support vgpu on gen5.
> >
> > v2: split out from previous path, fix check for missing case (Paulo)
> >
> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_uncore.c | 31 +++++++++++++++++++----------
> >  drivers/gpu/drm/i915/intel_uncore.h |  3 +++
> >  2 files changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c  
> > b/drivers/gpu/drm/i915/intel_uncore.c
> > index 3f74889c4212..0259a61a745f 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -1391,7 +1391,7 @@ static void intel_uncore_fw_domains_init(struct  
> > intel_uncore *uncore)
> >  {
> >       struct drm_i915_private *i915 = uncore_to_i915(uncore);
> > -     if (INTEL_GEN(i915) <= 5 || intel_vgpu_active(i915))
> > +     if (!(uncore->flags & UNCORE_HAS_FORCEWAKE))
> >               return;
> >       if (INTEL_GEN(i915) >= 11) {
> > @@ -1590,6 +1590,9 @@ int intel_uncore_init(struct intel_uncore *uncore)
> >       i915_check_vgpu(i915);
> > +     if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915))
> > +             uncore->flags |= UNCORE_HAS_FORCEWAKE;
> > +
> >       intel_uncore_edram_detect(i915);
> >       intel_uncore_fw_domains_init(uncore);
> >       __intel_uncore_early_sanitize(uncore, 0);
> > @@ -1598,12 +1601,14 @@ int intel_uncore_init(struct intel_uncore  
> > *uncore)
> >       uncore->pmic_bus_access_nb.notifier_call =
> >               i915_pmic_bus_access_notifier;
> > -     if (IS_GEN_RANGE(i915, 2, 4) || intel_vgpu_active(i915)) {
> > -             ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen2);
> > -             ASSIGN_READ_MMIO_VFUNCS(uncore, gen2);
> > -     } else if (IS_GEN(i915, 5)) {
> > -             ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen5);
> > -             ASSIGN_READ_MMIO_VFUNCS(uncore, gen5);
> > +     if (!(uncore->flags & UNCORE_HAS_FORCEWAKE)) {
> > +             if (IS_GEN(i915, 5)) {
> > +                     ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen5);
> > +                     ASSIGN_READ_MMIO_VFUNCS(uncore, gen5);
> > +             } else {
> > +                     ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen2);
> > +                     ASSIGN_READ_MMIO_VFUNCS(uncore, gen2);
> > +             }
> >       } else if (IS_GEN_RANGE(i915, 6, 7)) {
> >               ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6);
> > @@ -1912,7 +1917,10 @@ intel_uncore_forcewake_for_read(struct  
> > drm_i915_private *dev_priv,
> >       } else if (INTEL_GEN(dev_priv) >= 6) {
> >               fw_domains = __gen6_reg_read_fw_domains(uncore, offset);
> >       } else {
> > -             WARN_ON(!IS_GEN_RANGE(dev_priv, 2, 5));
> > +             /* on devices with FW we expect to hit one of the above cases */
> > +             if (unlikely(uncore->flags & UNCORE_HAS_FORCEWAKE))
> > +                     MISSING_CASE(INTEL_GEN(dev_priv));
> > +
> >               fw_domains = 0;
> >       }
> > @@ -1938,7 +1946,10 @@ intel_uncore_forcewake_for_write(struct  
> > drm_i915_private *dev_priv,
> >       } else if (IS_GEN_RANGE(dev_priv, 6, 7)) {
> >               fw_domains = FORCEWAKE_RENDER;
> >       } else {
> > -             WARN_ON(!IS_GEN_RANGE(dev_priv, 2, 5));
> > +             /* on devices with FW we expect to hit one of the above cases */
> > +             if (unlikely(uncore->flags & UNCORE_HAS_FORCEWAKE))
> > +                     MISSING_CASE(INTEL_GEN(dev_priv));
> > +
> >               fw_domains = 0;
> >       }
> > @@ -1969,7 +1980,7 @@ intel_uncore_forcewake_for_reg(struct  
> > drm_i915_private *dev_priv,
> >       WARN_ON(!op);
> > -     if (intel_vgpu_active(dev_priv))
> > +     if (!(dev_priv->uncore.flags & UNCORE_HAS_FORCEWAKE))
> 
> nit: as this condition is used in few places, maybe we can add nice helper:
> 
> static inline bool intel_uncore_has_forcewake(struct intel_uncore *uncore)
> {
>         return uncore->flags & UNCORE_HAS_FORCEWAKE;
> }

Did a quick pass to apply the nit and quieten checkpatch and pushed.

Thanks,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx