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

[Chris Wilson]

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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre