Re: [PATCH v5] drm/i915/execlists: Move WA_TAIL_DWORDS to callee

[Dave Gordon <david.s.gordon@intel.com>]

On 25/02/16 10:05, Chris Wilson wrote:
> On Wed, Feb 24, 2016 at 10:02:58AM +0000, Dave Gordon wrote:
>> @@ -907,7 +942,8 @@ int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request)
>>   	 * adding any commands to it then there might not actually be
>>   	 * sufficient room for the submission commands.
>>   	 */
>> -	intel_ring_reserved_space_reserve(request->ringbuf, MIN_SPACE_FOR_ADD_REQUEST);
>> +	intel_ring_reserved_space_reserve(request->ringbuf,
>> +		MIN_SPACE_FOR_ADD_REQUEST + WA_TAIL_DWORDS(request));
>
> No, no and thrice no. MIN_SPACE_FOR_ADD_REQUEST already has to and does
> take this into account. We either make it variable and universally compute
> it per-engine/per-gen or keep using the fixed constant that is large enough
> for everybody. This code should remain common to all paths until the
> duplication is removed.
> -Chris

As I said on the last iteration:

I didn't put it there because we *needed* the extra space -- as you say, 
the constant is already large enough -- but rather so that people 
changing either of those two values would be more likely to think about 
whether there were any unexpected interactions.

The sum of two constants is still just a constant. We *could* just 
update the comment about how MIN_SPACE_FOR_ADD_REQUEST was arrived at, 
noting that includes enough space for any possible workaround on any 
platform, without even changing the value. But that's more likely to get 
ignored if anyone ever finds a reason to increase the size of the 
padding (for example, if a context can be resubmitted more than once due 
to preemption, do we need to apply the workaround of adding 2 DWords 
EACH time?)

When we come to *use* the padding (in intel_logical_ring_submit()), 
there's a comment noting that the ring_begin() "is safe because we 
reserved the space earlier". So mentioning the padding at the point of 
allocation helps tie these two together and makes it clearer that the 
padding being consumed here is the same padding reserved earlier.

But the whole point of Rodrigo's original patch:
   drm/i915: Make wa_tail_dwords flexible for future platforms.
was to make this NOT (necessarily) a constant -- in which case we MUST 
add it to the reserved amount at the point of allocation, as above.
The commit message noted:
   we don't have a clean way to implement or remove WaIdleLiteRestore
   for different platforms.
   This patch aims to let it more flexible. So we just emit the NOOPs
   equivalent of what was initialized.
   Also let's just include the platforms we know that needs this Wa,
   i.e gen8 and gen9 platforms.

Personally, I'm not convinced that it really *needs* to be dynamic; but 
the implementation above /allows/ it to be so if it's ever necessary -- 
thus satisfying Rodrigo's intent -- while adding no overhead as long as 
the actual value remains a constant.

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