| Message ID | 1519256134-653-1-git-send-email-oscar.mateo@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Looks good to me. Few cosmetic changes suggested below. With those addressed: Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> 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" > > 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@intel.com> > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > --- > 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. > +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. > __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); > + 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);
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@intel.com> > > 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@intel.com> >> Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> >> --- >> 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); >
Hi Oscar,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20180222]
[cannot apply to v4.16-rc2]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Oscar-Mateo/drm-i915-icl-Check-for-fused-off-VDBOX-and-VEBOX-instances/20180223-095336
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-x004-201807 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All error/warnings (new ones prefixed by >>):
drivers/gpu/drm/i915/intel_device_info.c: In function 'intel_device_info_init_mmio':
>> drivers/gpu/drm/i915/intel_device_info.c:619:18: error: 'I915_MAX_VCS' undeclared (first use in this function); did you mean 'I915_MAP_WC'?
for (i = 0; i < I915_MAX_VCS; i++) {
^~~~~~~~~~~~
I915_MAP_WC
drivers/gpu/drm/i915/intel_device_info.c:619:18: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/gpu/drm/i915/intel_device_info.c:631:18: error: 'I915_MAX_VECS' undeclared (first use in this function); did you mean 'I915_MAX_VCS'?
for (i = 0; i < I915_MAX_VECS; i++) {
^~~~~~~~~~~~~
I915_MAX_VCS
In file included from include/linux/kernel.h:11:0,
from include/asm-generic/bug.h:18,
from arch/x86/include/asm/bug.h:82,
from include/linux/bug.h:5,
from include/linux/seq_file.h:7,
from include/drm/drm_print.h:31,
from drivers/gpu/drm/i915/intel_device_info.c:25:
>> drivers/gpu/drm/i915/intel_device_info.c:632:29: error: implicit declaration of function '_VECS'; did you mean '_VCS'? [-Werror=implicit-function-declaration]
if (!HAS_ENGINE(dev_priv, _VECS(i)))
^
include/linux/bitops.h:7:28: note: in definition of macro 'BIT'
#define BIT(nr) (1UL << (nr))
^~
drivers/gpu/drm/i915/i915_drv.h:2752:35: note: in expansion of macro 'ENGINE_MASK'
(!!((dev_priv)->info.ring_mask & ENGINE_MASK(id)))
^~~~~~~~~~~
>> drivers/gpu/drm/i915/intel_device_info.c:632:8: note: in expansion of macro 'HAS_ENGINE'
if (!HAS_ENGINE(dev_priv, _VECS(i)))
^~~~~~~~~~
cc1: some warnings being treated as errors
--
drivers/gpu/drm/i915/intel_uncore.c: In function 'intel_uncore_prune':
>> drivers/gpu/drm/i915/intel_uncore.c:1468:19: error: 'I915_MAX_VCS' undeclared (first use in this function); did you mean 'I915_MAP_WC'?
for (i = 0; i < I915_MAX_VCS; i++) {
^~~~~~~~~~~~
I915_MAP_WC
drivers/gpu/drm/i915/intel_uncore.c:1468:19: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/gpu/drm/i915/intel_uncore.c:1469:16: error: 'FW_DOMAIN_ID_MEDIA_VDBOX0' undeclared (first use in this function); did you mean 'FW_DOMAIN_ID_MEDIA'?
domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i;
^~~~~~~~~~~~~~~~~~~~~~~~~
FW_DOMAIN_ID_MEDIA
>> drivers/gpu/drm/i915/intel_uncore.c:1478:19: error: 'I915_MAX_VECS' undeclared (first use in this function); did you mean 'I915_MAX_VCS'?
for (i = 0; i < I915_MAX_VECS; i++) {
^~~~~~~~~~~~~
I915_MAX_VCS
>> drivers/gpu/drm/i915/intel_uncore.c:1479:16: error: 'FW_DOMAIN_ID_MEDIA_VEBOX0' undeclared (first use in this function); did you mean 'FW_DOMAIN_ID_MEDIA_VDBOX0'?
domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i;
^~~~~~~~~~~~~~~~~~~~~~~~~
FW_DOMAIN_ID_MEDIA_VDBOX0
In file included from include/linux/kernel.h:11:0,
from include/asm-generic/bug.h:18,
from arch/x86/include/asm/bug.h:82,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/gfp.h:5,
from include/linux/slab.h:15,
from include/linux/io-mapping.h:22,
from drivers/gpu/drm/i915/i915_drv.h:36,
from drivers/gpu/drm/i915/intel_uncore.c:24:
>> drivers/gpu/drm/i915/intel_uncore.c:1481:29: error: implicit declaration of function '_VECS'; did you mean '_VCS'? [-Werror=implicit-function-declaration]
if (HAS_ENGINE(dev_priv, _VECS(i)))
^
include/linux/bitops.h:7:28: note: in definition of macro 'BIT'
#define BIT(nr) (1UL << (nr))
^~
drivers/gpu/drm/i915/i915_drv.h:2752:35: note: in expansion of macro 'ENGINE_MASK'
(!!((dev_priv)->info.ring_mask & ENGINE_MASK(id)))
^~~~~~~~~~~
drivers/gpu/drm/i915/intel_uncore.c:1481:8: note: in expansion of macro 'HAS_ENGINE'
if (HAS_ENGINE(dev_priv, _VECS(i)))
^~~~~~~~~~
cc1: some warnings being treated as errors
vim +619 drivers/gpu/drm/i915/intel_device_info.c
595
596 /*
597 * Determine which engines are fused off in our particular hardware. Since the
598 * fuse register is in the blitter powerwell, we need forcewake to be ready at
599 * this point (but later we need to prune the forcewake domains for engines that
600 * are indeed fused off).
601 */
602 void intel_device_info_init_mmio(struct drm_i915_private *dev_priv)
603 {
604 struct intel_device_info *info = mkwrite_device_info(dev_priv);
605 u8 vdbox_disable, vebox_disable;
606 u32 media_fuse;
607 int i;
608
609 if (INTEL_GEN(dev_priv) < 11)
610 return;
611
612 media_fuse = I915_READ(GEN11_GT_VEBOX_VDBOX_DISABLE);
613
614 vdbox_disable = media_fuse & GEN11_GT_VDBOX_DISABLE_MASK;
615 vebox_disable = (media_fuse & GEN11_GT_VEBOX_DISABLE_MASK) >>
616 GEN11_GT_VEBOX_DISABLE_SHIFT;
617
618 DRM_DEBUG_DRIVER("vdbox disable: %04x\n", vdbox_disable);
> 619 for (i = 0; i < I915_MAX_VCS; i++) {
620 if (!HAS_ENGINE(dev_priv, _VCS(i)))
621 continue;
622
623 if (!(BIT(i) & vdbox_disable))
624 continue;
625
626 info->ring_mask &= ~ENGINE_MASK(_VCS(i));
627 DRM_DEBUG_DRIVER("vcs%u fused off\n", i);
628 }
629
630 DRM_DEBUG_DRIVER("vebox disable: %04x\n", vebox_disable);
> 631 for (i = 0; i < I915_MAX_VECS; i++) {
> 632 if (!HAS_ENGINE(dev_priv, _VECS(i)))
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Oscar,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20180222]
[cannot apply to v4.16-rc2]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Oscar-Mateo/drm-i915-icl-Check-for-fused-off-VDBOX-and-VEBOX-instances/20180223-095336
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-a1-201807 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/intel_device_info.c: In function 'intel_device_info_init_mmio':
>> drivers/gpu/drm/i915/intel_device_info.c:619:18: error: 'I915_MAX_VCS' undeclared (first use in this function)
for (i = 0; i < I915_MAX_VCS; i++) {
^
drivers/gpu/drm/i915/intel_device_info.c:619:18: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/gpu/drm/i915/intel_device_info.c:631:18: error: 'I915_MAX_VECS' undeclared (first use in this function)
for (i = 0; i < I915_MAX_VECS; i++) {
^
>> drivers/gpu/drm/i915/intel_device_info.c:632:3: error: implicit declaration of function '_VECS' [-Werror=implicit-function-declaration]
if (!HAS_ENGINE(dev_priv, _VECS(i)))
^
cc1: some warnings being treated as errors
--
drivers/gpu/drm/i915/intel_uncore.c: In function 'intel_uncore_prune':
drivers/gpu/drm/i915/intel_uncore.c:1468:19: error: 'I915_MAX_VCS' undeclared (first use in this function)
for (i = 0; i < I915_MAX_VCS; i++) {
^
drivers/gpu/drm/i915/intel_uncore.c:1468:19: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/gpu/drm/i915/intel_uncore.c:1469:16: error: 'FW_DOMAIN_ID_MEDIA_VDBOX0' undeclared (first use in this function)
domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i;
^
drivers/gpu/drm/i915/intel_uncore.c:1478:19: error: 'I915_MAX_VECS' undeclared (first use in this function)
for (i = 0; i < I915_MAX_VECS; i++) {
^
>> drivers/gpu/drm/i915/intel_uncore.c:1479:16: error: 'FW_DOMAIN_ID_MEDIA_VEBOX0' undeclared (first use in this function)
domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i;
^
drivers/gpu/drm/i915/intel_uncore.c:1481:4: error: implicit declaration of function '_VECS' [-Werror=implicit-function-declaration]
if (HAS_ENGINE(dev_priv, _VECS(i)))
^
cc1: some warnings being treated as errors
vim +/I915_MAX_VCS +619 drivers/gpu/drm/i915/intel_device_info.c
595
596 /*
597 * Determine which engines are fused off in our particular hardware. Since the
598 * fuse register is in the blitter powerwell, we need forcewake to be ready at
599 * this point (but later we need to prune the forcewake domains for engines that
600 * are indeed fused off).
601 */
602 void intel_device_info_init_mmio(struct drm_i915_private *dev_priv)
603 {
604 struct intel_device_info *info = mkwrite_device_info(dev_priv);
605 u8 vdbox_disable, vebox_disable;
606 u32 media_fuse;
607 int i;
608
609 if (INTEL_GEN(dev_priv) < 11)
610 return;
611
612 media_fuse = I915_READ(GEN11_GT_VEBOX_VDBOX_DISABLE);
613
614 vdbox_disable = media_fuse & GEN11_GT_VDBOX_DISABLE_MASK;
615 vebox_disable = (media_fuse & GEN11_GT_VEBOX_DISABLE_MASK) >>
616 GEN11_GT_VEBOX_DISABLE_SHIFT;
617
618 DRM_DEBUG_DRIVER("vdbox disable: %04x\n", vdbox_disable);
> 619 for (i = 0; i < I915_MAX_VCS; i++) {
620 if (!HAS_ENGINE(dev_priv, _VCS(i)))
621 continue;
622
623 if (!(BIT(i) & vdbox_disable))
624 continue;
625
626 info->ring_mask &= ~ENGINE_MASK(_VCS(i));
627 DRM_DEBUG_DRIVER("vcs%u fused off\n", i);
628 }
629
630 DRM_DEBUG_DRIVER("vebox disable: %04x\n", vebox_disable);
> 631 for (i = 0; i < I915_MAX_VECS; i++) {
> 632 if (!HAS_ENGINE(dev_priv, _VECS(i)))
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 2/23/2018 4:35 AM, Oscar Mateo wrote: > > <snip> >>> + * 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). > Right. Makes sense. for loop iterating over engines based on ring_mask can help us rely on only HAS_ENGINE condition and then we can have complete pruning in single for loop. what do you think? >>> + 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); >> >
On 2/25/2018 9:22 PM, Sagar Arun Kamble wrote: > > > On 2/23/2018 4:35 AM, Oscar Mateo wrote: >> >> > <snip> >>>> + * 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). >> > Right. Makes sense. > for loop iterating over engines based on ring_mask can help us rely on > only HAS_ENGINE condition and then we can have complete pruning in > single for loop. > what do you think? Hmmm.. I'm not sure I follow: intel_device_info_init_mmio modifies ring_mask, so if I loop over engines based on ring_mask I am going to miss those I want to prune, right? >>>> + 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); >>> >> >
On 2/27/2018 4:34 AM, Oscar Mateo wrote: > > > On 2/25/2018 9:22 PM, Sagar Arun Kamble wrote: >> >> >> On 2/23/2018 4:35 AM, Oscar Mateo wrote: >>> >>> >> <snip> >>>>> + * 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). >>> >> Right. Makes sense. >> for loop iterating over engines based on ring_mask can help us rely >> on only HAS_ENGINE condition and then we can have complete pruning in >> single for loop. >> what do you think? > > Hmmm.. I'm not sure I follow: intel_device_info_init_mmio modifies > ring_mask, so if I loop over engines based on ring_mask I am going to > miss those I want to prune, right? > Oops ... you are right ... My suggestion about skipping fw_domains check will not work currently. In future may be if we create default ring_mask and runtime ring_mask it can be reworked. Other suggestion to use single for loop (for_each_engine()) can be done I think. It will make it generic for all engine types. Below is what I am thinking of as part of intel_uncore_prune: for (i = 0; i < ARRAY_SIZE(intel_engines); i++) { if (HAS_ENGINE(dev_priv, i)) continue; if (fw_domains & BIT(i)) fw_domain_fini(dev_priv, i); } >>>>> + 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); >>>> >>> >> >
On 2/26/2018 9:49 PM, Sagar Arun Kamble wrote: > > > On 2/27/2018 4:34 AM, Oscar Mateo wrote: >> >> >> On 2/25/2018 9:22 PM, Sagar Arun Kamble wrote: >>> >>> >>> On 2/23/2018 4:35 AM, Oscar Mateo wrote: >>>> >>>> >>> <snip> >>>>>> + * 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). >>>> >>> Right. Makes sense. >>> for loop iterating over engines based on ring_mask can help us rely >>> on only HAS_ENGINE condition and then we can have complete pruning >>> in single for loop. >>> what do you think? >> >> Hmmm.. I'm not sure I follow: intel_device_info_init_mmio modifies >> ring_mask, so if I loop over engines based on ring_mask I am going to >> miss those I want to prune, right? >> > Oops ... you are right ... > My suggestion about skipping fw_domains check will not work currently. > In future may be if we create default ring_mask and runtime ring_mask > it can be reworked. > > Other suggestion to use single for loop (for_each_engine()) can be > done I think. > It will make it generic for all engine types. Below is what I am > thinking of as part of intel_uncore_prune: > > for (i = 0; i < ARRAY_SIZE(intel_engines); i++) { > if (HAS_ENGINE(dev_priv, i)) > continue; > if (fw_domains & BIT(i)) > fw_domain_fini(dev_priv, i); > } This won't work either, because the index for engines and forcewake domains is different. If you think it is important to make the prune function more generic, I can translate between the two (but it will be for naught if, as you mentioned, we are working to create separate default ring_mask and runtime ring_mask in the future). >>>>>> + 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); >>>>> >>>> >>> >> >
On 2/28/2018 11:29 PM, Oscar Mateo wrote: > > > On 2/26/2018 9:49 PM, Sagar Arun Kamble wrote: >> >> >> On 2/27/2018 4:34 AM, Oscar Mateo wrote: >>> >>> >>> On 2/25/2018 9:22 PM, Sagar Arun Kamble wrote: >>>> >>>> >>>> On 2/23/2018 4:35 AM, Oscar Mateo wrote: >>>>> >>>>> >>>> <snip> >>>>>>> + * 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). >>>>> >>>> Right. Makes sense. >>>> for loop iterating over engines based on ring_mask can help us rely >>>> on only HAS_ENGINE condition and then we can have complete pruning >>>> in single for loop. >>>> what do you think? >>> >>> Hmmm.. I'm not sure I follow: intel_device_info_init_mmio modifies >>> ring_mask, so if I loop over engines based on ring_mask I am going >>> to miss those I want to prune, right? >>> >> Oops ... you are right ... >> My suggestion about skipping fw_domains check will not work >> currently. In future may be if we create default ring_mask and >> runtime ring_mask it can be reworked. >> >> Other suggestion to use single for loop (for_each_engine()) can be >> done I think. >> It will make it generic for all engine types. Below is what I am >> thinking of as part of intel_uncore_prune: >> >> for (i = 0; i < ARRAY_SIZE(intel_engines); i++) { >> if (HAS_ENGINE(dev_priv, i)) >> continue; >> if (fw_domains & BIT(i)) >> fw_domain_fini(dev_priv, i); >> } > > This won't work either, because the index for engines and forcewake > domains is different. If you think it is important to make the prune > function more generic, I can translate between the two (but it will be > for naught if, as you mentioned, we are working to create separate > default ring_mask and runtime ring_mask in the future). > Yes. Translation will help (I thought of FW_D_ID_MEDIA to be reused for FW_D_ID_MEDIA_VDB0). I think this patch can go in current shape and will pursue other changes as follow up based on inputs. I feel making it generic will allow pruning to scale across engine types (if that is needed in future). I am not sure if we want to pursue the default/runtime ring_mask change (another use case of this that i think is if user wants to know default config vs fused) Tvrtko, Chris - what do you think? >>>>>>> + 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); >>>>>> >>>>> >>>> >>> >> >
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); +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 + */ __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_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);
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. We workaround this problem by pruning the forcewake domains after reading the fuse information. 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@intel.com> Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> --- 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(+)