Re: [Intel-gfx] [PATCH] drm/i915: Fix workarounds on Gen2-3

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

On 18/11/2022 17:14, Matt Roper wrote:
> On Fri, Nov 18, 2022 at 11:52:49AM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> In 3653727560d0 ("drm/i915: Simplify internal helper function signature")
>> I broke the old platforms by not noticing engine workaround init does not
>> initialize the list on old platforms. Fix it by always initializing which
>> already does the right thing by mostly not doing anything if there aren't
>> any workarounds on the list.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Fixes: 3653727560d0 ("drm/i915: Simplify internal helper function signature")
>> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> index 213160f29ec3..4d7a01b45e09 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> @@ -2991,7 +2991,7 @@ general_render_compute_wa_init(struct intel_engine_cs *engine, struct i915_wa_li
>>   static void
>>   engine_init_workarounds(struct intel_engine_cs *engine, struct i915_wa_list *wal)
>>   {
>> -	if (I915_SELFTEST_ONLY(GRAPHICS_VER(engine->i915) < 4))
>> +	if (GRAPHICS_VER(engine->i915) < 4)
>>   		return;
> 
> Do we even need this early return at all?  As far as I can see, letting
> this function run its course doesn't wind up having any effect or cause
> any problems (you still wind up with an empty list).

True, it looks to me like that as well, now that you are pointing it 
out. Btw originally I was most perplexed by the "selftests only" 
annotation, but did not find time to go digging through history to 
figure out why was that even needed.

I left the return as is for now and pushed it to fix the breakage. Will 
try to revisit this at some point. Thanks for the review!

Regards,

Tvrtko

> 
> Regardless,
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
>>   
>>   	engine_fake_wa_init(engine, wal);
>> @@ -3016,9 +3016,6 @@ void intel_engine_init_workarounds(struct intel_engine_cs *engine)
>>   {
>>   	struct i915_wa_list *wal = &engine->wa_list;
>>   
>> -	if (GRAPHICS_VER(engine->i915) < 4)
>> -		return;
>> -
>>   	wa_init_start(wal, engine->gt, "engine", engine->name);
>>   	engine_init_workarounds(engine, wal);
>>   	wa_init_finish(wal);
>> -- 
>> 2.34.1
>>
>