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

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

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);

-- 
Thanks,
Sagar

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