Re: [PATCH 3/8] drm/i915: Verify GT workaround state at runtime

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 30/11/2018 11:38, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-11-30 11:31:56)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Since we now have all the GT workarounds in a table, by adding a simple
>> shared helper function we can now verify that their values are still
>> applied after some interesting events in the lifetime of the driver.
>>
>> At this stage these are the driver initialization and engine reset.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c          |  4 +++
>>   drivers/gpu/drm/i915/i915_gem.c          |  3 ++
>>   drivers/gpu/drm/i915/intel_workarounds.c | 46 ++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_workarounds.h |  2 ++
>>   4 files changed, 55 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 2f3dc1cf83a6..14d019c9455b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -53,6 +53,7 @@
>>   #include "i915_vgpu.h"
>>   #include "intel_drv.h"
>>   #include "intel_uc.h"
>> +#include "intel_workarounds.h"
>>   
>>   static struct drm_driver driver;
>>   
>> @@ -2362,6 +2363,9 @@ int i915_reset_engine(struct intel_engine_cs *engine, const char *msg)
>>                  goto out;
>>          }
>>   
>> +       /* Catch GT workarounds affected by engine reset. */
>> +       intel_gt_workarounds_verify(engine->i915, engine->name);
> 
> I'd rather, quite strongly, not have this inside i915_reset_engine()
> itself. i915_reset_engine() is [designed to be] called from atomic
> context. Adding I915_READ() here is questionable. My preference is to
> have all this inside selftests/intel_workarounds where we can cross
> check the gt/all-engines after device reset and other engine reset.

I wasn't sure myself how much value checking it here adds, but regarding 
I915_READs, they are happening during engine->init_hw anyway. So I don't 
think a few more would harm. But yes, as said, since I was unsure myself 
I am happy to drop this hunk.

Regards,

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