[PATCH v4] drm/i915: Allocate intel_engine_cs structure only for the enabled engines

[Chris Wilson]

On Thu, Jul 28, 2016 at 08:37:13AM +0530, Goel, Akash wrote:
> 
> 
> On 7/27/2016 11:18 PM, Chris Wilson wrote:
> >On Wed, Jul 27, 2016 at 11:10:01PM +0530, Goel, Akash wrote:
> >>
> >>
> >>On 7/27/2016 10:54 PM, Chris Wilson wrote:
> >>>On Wed, Jul 27, 2016 at 10:45:45PM +0530, akash.goel at intel.com wrote:
> >>>>void i915_check_and_clear_faults(struct drm_i915_private *dev_priv)
> >>>>{
> >>>>	struct intel_engine_cs *engine;
> >>>>+	u32 iter;
> >>>>
> >>>>	if (INTEL_INFO(dev_priv)->gen < 6)
> >>>>		return;
> >>>>
> >>>>-	for_each_engine(engine, dev_priv) {
> >>>>+	for_each_engine(engine, dev_priv, iter) {
> >>>>		u32 fault_reg;
> >>>>		fault_reg = I915_READ(RING_FAULT_REG(engine));
> >>>>		if (fault_reg & RING_FAULT_VALID) {
> >>>>@@ -2285,7 +2288,10 @@ void i915_check_and_clear_faults(struct drm_i915_private *dev_priv)
> >>>>				   fault_reg & ~RING_FAULT_VALID);
> >>>>		}
> >>>>	}
> >>>>-	POSTING_READ(RING_FAULT_REG(&dev_priv->engine[RCS]));
> >>>>+
> >>>>+	/* Engine specific init may not have been done till this point. */
> >>>>+	if (dev_priv->engine[RCS])
> >>>>+		POSTING_READ(RING_FAULT_REG(dev_priv->engine[RCS]));
> >>>>}
> >>>
> >>>That deserves to be fixed first. If i915_check_and_clear_faults() is
> >>>being run before engine setup, it is reading bogus addresses. This needs
> >>>to be moved to intel_engines_init().
> >>
> >>Yes i915_check_and_clear_faults() gets called before engine setup,
> >>
> >>i915_driver_load
> >>	i915_driver_init_mmio
> >>		intel_uncore_init
> >>			i915_check_and_clear_faults
> >>
> >>	i915_load_modeset_init
> >>		i915_gem_init
> >>			intel_engines_init
> >>
> >>
> >>POSTING_READ(RING_FAULT_REG(&dev_priv->engine[RCS]));
> >>#define RING_FAULT_REG(engine)	_MMIO(0x4094 + 0x100*(engine)->id)
> >>
> >>Currently the address may not be bogus as
> >>(&dev_priv->engine[RCS])->id would come as zero.
> >
> >(Ok, I didn't check I just assumed it followed the RING_x() logic of
> >using engine->mmio_base.)
> >
> >But we still want to iterate over all registers for each engine. We
> >can't do that if we don't have the array of engines. We do it very
> >early, so it really should operate directly without requiring
> >for_each_engine(). Hmm.
> 
> Currently there won't be any iteration happening.
> 
> Should the call to i915_check_and_clear_faults() be moved from
> intel_uncore_init() to the end of intel_engines_init() ?

I sent a patch to do this, ...
 
> OR
> 
> Can the RING_FAULT_REG macro be re-defined, so that we can avoid
> using the for_each_engine() in this case, as per the below snippet.

because we may then iterate over undefined registers (presuming the bank
may not be wired up for say VECS2 on IVB).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre