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

From: Paulo Zanoni <paulo.r.zanoni@intel.com>
To: Lionel Landwerlin <lionel.g.landwerlin@intel.com>,
	Mika Kuoppala <mika.kuoppala@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH 1/8] drm/i915/icl: Check for fused-off VDBOX and VEBOX instances
Date: Tue, 27 Mar 2018 15:42:27 -0700	[thread overview]
Message-ID: <1522190547.2643.0.camel@intel.com> (raw)
In-Reply-To: <771b0d51-c765-4e39-d604-691bb41d6820@intel.com>

Em Sex, 2018-03-23 às 16:28 +0000, Lionel Landwerlin escreveu:
> Hi Mika,
> 
> Even after this series, we're still missing support for reading the 
> timestamp frequency (read_timestamp_frequency in
> intel_device_info.c).
> I'm pretty sure someone wrote a patch for it. Do you any idea?
> 
> If not, I can send something.

Yes, we have them. I'll see if I missed them while upstreaming and
resend in that case.

> 
> Thanks,
> 
> -
> Lionel
> 
> On 16/03/18 12:14, Mika Kuoppala wrote:
> > From: Oscar Mateo <oscar.mateo@intel.com>
> > 
> > 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 allowing the initialization of all
> > forcewake
> > domains and then pruning the fused off ones, as per 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).
> > 
> > v10:
> >    - Improved commit message (Sagar)
> >    - New line in header file (Sagar)
> >    - Specify the message in fw_domain_reset applies to ICL+ (Sagar)
> > 
> > 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 |  2 ++
> >   drivers/gpu/drm/i915/intel_uncore.c      | 56
> > ++++++++++++++++++++++++++++++++
> >   drivers/gpu/drm/i915/intel_uncore.h      |  1 +
> >   6 files changed, 115 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 3df5193487f3..83df8e21cec0 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1033,6 +1033,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 cf7c837d6a09..982e72e73e99 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -2545,6 +2545,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 3dd350f7b8e6..4babfc6ee45b 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > @@ -780,3 +780,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 0835752c8b22..0cbb92223013 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > @@ -247,6 +247,8 @@ void intel_device_info_dump_runtime(const
> > struct intel_device_info *info,
> >   void intel_device_info_dump_topology(const struct sseu_dev_info
> > *sseu,
> >   				     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 4df7c2ef8576..4c616d074a97 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -62,6 +62,11 @@ static inline void
> >   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 (engines
> > could be fused
> > +	 * off in ICL+), so no waiting for acks
> > +	 */
> >   	__raw_i915_write32(i915, d->reg_set, i915-
> > >uncore.fw_reset);
> >   }
> >   
> > @@ -1353,6 +1358,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))
> > @@ -1565,6 +1587,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 dfdf444e4bcc..47478d609630 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.h
> > +++ b/drivers/gpu/drm/i915/intel_uncore.h
> > @@ -140,6 +140,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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx