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

[Goel, Akash]

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() ?

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.

diff --git a/drivers/gpu/drm/i915/i915_reg.h 
b/drivers/gpu/drm/i915/i915_reg.h
index 9397dde..c230039 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1647,7 +1647,7 @@ enum skl_disp_power_wells {
  #define   ARB_MODE_BWGTLB_DISABLE (1<<9)
  #define   ARB_MODE_SWIZZLE_BDW	(1<<1)
  #define RENDER_HWS_PGA_GEN7	_MMIO(0x04080)
-#define RING_FAULT_REG(engine)	_MMIO(0x4094 + 0x100*(engine)->id)
+#define RING_FAULT_REG(engine_id)	_MMIO(0x4094 + 0x100*(engine_id))
  #define   RING_FAULT_GTTSEL_MASK (1<<11)
  #define   RING_FAULT_SRCID(x)	(((x) >> 3) & 0xff)
  #define   RING_FAULT_FAULT_TYPE(x) (((x) >> 1) & 0x3)


diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 38d4586..c9d0d22 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2271,9 +2271,9 @@ void i915_check_and_clear_faults(struct 
drm_i915_private *dev_priv)
  	if (INTEL_INFO(dev_priv)->gen < 6)
  		return;

-	for_each_engine(engine, dev_priv, iter) {
+	for (id = 0; id < I915_NUM_ENGINES; id++) {
  		u32 fault_reg;
-		fault_reg = I915_READ(RING_FAULT_REG(engine));
+		fault_reg = I915_READ(RING_FAULT_REG(id));
  		if (fault_reg & RING_FAULT_VALID) {
  			DRM_DEBUG_DRIVER("Unexpected fault\n"
  					 "\tAddr: 0x%08lx\n"
@@ -2284,7 +2284,7 @@ void i915_check_and_clear_faults(struct 
drm_i915_private *dev_priv)
  					 fault_reg & RING_FAULT_GTTSEL_MASK ? "GGTT" : "PPGTT",
  					 RING_FAULT_SRCID(fault_reg),
  					 RING_FAULT_FAULT_TYPE(fault_reg));
-			I915_WRITE(RING_FAULT_REG(engine),
+			I915_WRITE(RING_FAULT_REG(id),
  				   fault_reg & ~RING_FAULT_VALID);
  		}
  	}
-	POSTING_READ(RING_FAULT_REG(&dev_priv->engine[RCS]));
+	POSTING_READ(RING_FAULT_REG(RCS));


Best regards
Akash
> -Chris
>