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
