Re: [PATCH v9] drm/i915/icl: Check for fused-off VDBOX and VEBOX instances

[Oscar Mateo <oscar.mateo@xxxxxxxxx>]

On 2/21/2018 10:17 PM, Sagar Arun Kamble wrote:
> Looks good to me. Few cosmetic changes suggested below. With those 
> addressed:
> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
>
> On 2/22/2018 5:05 AM, Oscar Mateo wrote:
> > In Gen11, the Video Decode engines (aka VDBOX, aka VCS, aka BSD) and the
> > Video Enhancement engines (aka VEBOX, aka VECS) could be fused off. 
> > Also,
> > each VDBOX and VEBOX has its own power well, which only exist if the 
> > related
> > engine exists in the HW.
> >
> > Unfortunately, we have a Catch-22 situation going on: we need the 
> > blitter
> > forcewake to read the register with the fuse info, but we cannot 
> > initialize
> > the forcewake domains without knowin about the engines present in the 
> > HW.
> *knowing
> > We workaround this problem by pruning the forcewake domains after 
> > reading
> > the fuse information.
> This line can be re-framed like:
> "We workaround this problem by allowing initialization of all 
> forcewake domains and then pruning the fused off forcewake domains
> based on fuse info which can be read acquiring blitter forcewake"

Can do.

> > Bspec: 20680
> >
> > v2: We were shifting incorrectly for vebox disable (Vinay)
> >
> > v3: Assert mmio is ready and warn if we have attempted to initialize
> >      forcewake for fused-off engines (Paulo)
> >
> > v4:
> >    - Use INTEL_GEN in new code (Tvrtko)
> >    - Shorter local variable (Tvrtko, Michal)
> >    - Keep "if (!...) continue" style (Tvrtko)
> >    - No unnecessary BUG_ON (Tvrtko)
> >    - WARN_ON and cleanup if wrong mask (Tvrtko, Michal)
> >    - Use I915_READ_FW (Michal)
> >    - Use I915_MAX_VCS/VECS macros (Michal)
> >
> > v5: Rebased by Rodrigo fixing conflicts on top of:
> >      commit 33def1ff7b0 ("drm/i915: Simplify intel_engines_init")
> >
> > v6: Fix v5. Remove info->num_rings. (by Oscar)
> >
> > v7: Rebase (Rodrigo).
> >
> > v8:
> >    - 
> > s/intel_device_info_fused_off_engines/intel_device_info_init_mmio 
> > (Chris)
> >    - Make vdbox_disable & vebox_disable local variables (Chris)
> >
> > v9:
> >    - Move function declaration to intel_device_info.h (Michal)
> >    - Missing indent in bit fields definitions (Michal)
> >    - When RC6 is enabled by BIOS, the fuse register cannot be read until
> >      the blitter powerwell is awake. Shuffle where the fuse is read, 
> > prune
> >      the forcewake domains after the fact and change the commit message
> >      accordingly (Vinay, Sagar, Chris).
> >
> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > Cc: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Signed-off-by: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.c          |  4 +++
> >   drivers/gpu/drm/i915/i915_reg.h          |  5 +++
> >   drivers/gpu/drm/i915/intel_device_info.c | 47 
> > +++++++++++++++++++++++++++
> >   drivers/gpu/drm/i915/intel_device_info.h |  1 +
> >   drivers/gpu/drm/i915/intel_uncore.c      | 55 
> > ++++++++++++++++++++++++++++++++
> >   drivers/gpu/drm/i915/intel_uncore.h      |  1 +
> >   6 files changed, 113 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c 
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index d09f8e6..2269b56 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1031,6 +1031,10 @@ static int i915_driver_init_mmio(struct 
> > drm_i915_private *dev_priv)
> >         intel_uncore_init(dev_priv);
> >   +    intel_device_info_init_mmio(dev_priv);
> > +
> > +    intel_uncore_prune(dev_priv);
> > +
> >       intel_uc_init_mmio(dev_priv);
> >         ret = intel_engines_init_mmio(dev_priv);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 784d79c..e6a0d84 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -2854,6 +2854,11 @@ enum i915_power_well_id {
> >   #define GEN10_EU_DISABLE3        _MMIO(0x9140)
> >   #define   GEN10_EU_DIS_SS_MASK        0xff
> >   +#define GEN11_GT_VEBOX_VDBOX_DISABLE    _MMIO(0x9140)
> > +#define   GEN11_GT_VDBOX_DISABLE_MASK    0xff
> > +#define   GEN11_GT_VEBOX_DISABLE_SHIFT    16
> > +#define   GEN11_GT_VEBOX_DISABLE_MASK    (0xff << 
> > GEN11_GT_VEBOX_DISABLE_SHIFT)
> > +
> >   #define GEN6_BSD_SLEEP_PSMI_CONTROL    _MMIO(0x12050)
> >   #define   GEN6_BSD_SLEEP_MSG_DISABLE    (1 << 0)
> >   #define   GEN6_BSD_SLEEP_FLUSH_DISABLE    (1 << 2)
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
> > b/drivers/gpu/drm/i915/intel_device_info.c
> > index 9352f34..70ea654 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > @@ -595,3 +595,50 @@ void intel_driver_caps_print(const struct 
> > intel_driver_caps *caps,
> >   {
> >       drm_printf(p, "scheduler: %x\n", caps->scheduler);
> >   }
> > +
> > +/*
> > + * Determine which engines are fused off in our particular hardware. 
> > Since the
> > + * fuse register is in the blitter powerwell, we need forcewake to 
> > be ready at
> > + * this point (but later we need to prune the forcewake domains for 
> > engines that
> > + * are indeed fused off).
> > + */
> > +void intel_device_info_init_mmio(struct drm_i915_private *dev_priv)
> > +{
> > +    struct intel_device_info *info = mkwrite_device_info(dev_priv);
> > +    u8 vdbox_disable, vebox_disable;
> > +    u32 media_fuse;
> > +    int i;
> > +
> > +    if (INTEL_GEN(dev_priv) < 11)
> > +        return;
> > +
> > +    media_fuse = I915_READ(GEN11_GT_VEBOX_VDBOX_DISABLE);
> > +
> > +    vdbox_disable = media_fuse & GEN11_GT_VDBOX_DISABLE_MASK;
> > +    vebox_disable = (media_fuse & GEN11_GT_VEBOX_DISABLE_MASK) >>
> > +            GEN11_GT_VEBOX_DISABLE_SHIFT;
> > +
> > +    DRM_DEBUG_DRIVER("vdbox disable: %04x\n", vdbox_disable);
> > +    for (i = 0; i < I915_MAX_VCS; i++) {
> > +        if (!HAS_ENGINE(dev_priv, _VCS(i)))
> > +            continue;
> > +
> > +        if (!(BIT(i) & vdbox_disable))
> > +            continue;
> > +
> > +        info->ring_mask &= ~ENGINE_MASK(_VCS(i));
> > +        DRM_DEBUG_DRIVER("vcs%u fused off\n", i);
> > +    }
> > +
> > +    DRM_DEBUG_DRIVER("vebox disable: %04x\n", vebox_disable);
> > +    for (i = 0; i < I915_MAX_VECS; i++) {
> > +        if (!HAS_ENGINE(dev_priv, _VECS(i)))
> > +            continue;
> > +
> > +        if (!(BIT(i) & vebox_disable))
> > +            continue;
> > +
> > +        info->ring_mask &= ~ENGINE_MASK(_VECS(i));
> > +        DRM_DEBUG_DRIVER("vecs%u fused off\n", i);
> > +    }
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.h 
> > b/drivers/gpu/drm/i915/intel_device_info.h
> > index 4c6f83b..2233a2f 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > @@ -187,6 +187,7 @@ void intel_device_info_dump_flags(const struct 
> > intel_device_info *info,
> >                     struct drm_printer *p);
> >   void intel_device_info_dump_runtime(const struct intel_device_info 
> > *info,
> >                       struct drm_printer *p);
> May be we need to add extra line to differentiate from 
> "device_info_*_runtime_*" function declarations.

Can do.

> > +void intel_device_info_init_mmio(struct drm_i915_private *dev_priv);
> >     void intel_driver_caps_print(const struct intel_driver_caps *caps,
> >                    struct drm_printer *p);
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
> > b/drivers/gpu/drm/i915/intel_uncore.c
> > index 5ae9a62..5de0d26 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -56,6 +56,10 @@
> >   fw_domain_reset(struct drm_i915_private *i915,
> >           const struct intel_uncore_forcewake_domain *d)
> >   {
> > +    /*
> > +     * We don't really know if the powerwell for the forcewake 
> > domain we are
> > +     * trying to reset here does exist at this point, so no waiting 
> > for acks
> > +     */
> We should also add that this applies to ICL.

Can do.

> >       __raw_i915_write32(i915, d->reg_set, i915->uncore.fw_reset);
> >   }
> >   @@ -1251,6 +1255,23 @@ static void fw_domain_init(struct 
> > drm_i915_private *dev_priv,
> >       fw_domain_reset(dev_priv, d);
> >   }
> >   +static void fw_domain_fini(struct drm_i915_private *dev_priv,
> > +               enum forcewake_domain_id domain_id)
> > +{
> > +    struct intel_uncore_forcewake_domain *d;
> > +
> > +    if (WARN_ON(domain_id >= FW_DOMAIN_ID_COUNT))
> > +        return;
> > +
> > +    d = &dev_priv->uncore.fw_domain[domain_id];
> > +
> > +    WARN_ON(d->wake_count);
> > +    WARN_ON(hrtimer_cancel(&d->timer));
> > +    memset(d, 0, sizeof(*d));
> > +
> > +    dev_priv->uncore.fw_domains &= ~BIT(domain_id);
> > +}
> > +
> >   static void intel_uncore_fw_domains_init(struct drm_i915_private 
> > *dev_priv)
> >   {
> >       if (INTEL_GEN(dev_priv) <= 5 || intel_vgpu_active(dev_priv))
> > @@ -1432,6 +1453,40 @@ void intel_uncore_init(struct drm_i915_private 
> > *dev_priv)
> >           &dev_priv->uncore.pmic_bus_access_nb);
> >   }
> >   +/*
> > + * We might have detected that some engines are fused off after we 
> > initialized
> > + * the forcewake domains. Prune them, to make sure they only 
> > reference existing
> > + * engines.
> > + */
> > +void intel_uncore_prune(struct drm_i915_private *dev_priv)
> > +{
> > +    if (INTEL_GEN(dev_priv) >= 11) {
> > +        enum forcewake_domains fw_domains = 
> > dev_priv->uncore.fw_domains;
> > +        enum forcewake_domain_id domain_id;
> > +        int i;
> > +
> > +        for (i = 0; i < I915_MAX_VCS; i++) {
> > +            domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i;
> > +
> > +            if (HAS_ENGINE(dev_priv, _VCS(i)))
> > +                continue;
> > +
> > +            if (fw_domains & BIT(domain_id))
> fw_domains check seems redundant as it is initialized based on 
> HAS_ENGINE.
> we can just have
> if (!HAS_ENGINE(dev_priv, _VCS(i)))
>     fw_domain_fini(dev_priv, domain_id);

I don't want to call fw_domain_fini on something we never initialized in 
the first place (e.g. VCS1/3 and VECS1/2/3 on an ICL-LP).

> > +                fw_domain_fini(dev_priv, domain_id);
> > +        }
> > +
> > +        for (i = 0; i < I915_MAX_VECS; i++) {
> > +            domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i;
> > +
> > +            if (HAS_ENGINE(dev_priv, _VECS(i)))
> > +                continue;
> > +
> > +            if (fw_domains & BIT(domain_id))
> > +                fw_domain_fini(dev_priv, domain_id);
> > +        }
> > +    }
> > +}
> > +
> >   void intel_uncore_fini(struct drm_i915_private *dev_priv)
> >   {
> >       /* Paranoia: make sure we have disabled everything before we 
> > exit. */
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.h 
> > b/drivers/gpu/drm/i915/intel_uncore.h
> > index 53ef77d..28feabf 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.h
> > +++ b/drivers/gpu/drm/i915/intel_uncore.h
> > @@ -129,6 +129,7 @@ struct intel_uncore {
> >     void intel_uncore_sanitize(struct drm_i915_private *dev_priv);
> >   void intel_uncore_init(struct drm_i915_private *dev_priv);
> > +void intel_uncore_prune(struct drm_i915_private *dev_priv);
> >   bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv);
> >   bool intel_uncore_arm_unclaimed_mmio_detection(struct 
> > drm_i915_private *dev_priv);
> >   void intel_uncore_fini(struct drm_i915_private *dev_priv);

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