[Intel-gfx] [PATCH v9] drm/i915/icl: Check for fused-off VDBOX and VEBOX instances
Oscar Mateo
oscar.mateo at intel.com
Thu Feb 22 23:05:47 UTC 2018
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 at 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 at intel.com>
>> Cc: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Signed-off-by: Oscar Mateo <oscar.mateo at 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);
>
More information about the Intel-gfx
mailing list