[Intel-gfx] [PATCH v3] drm/i915/execlists: Move WA_TAIL_DWORDS to callee
Dave Gordon
david.s.gordon at intel.com
Mon Feb 8 11:08:12 UTC 2016
On 05/02/16 22:56, Rodrigo Vivi wrote:
> On Fri, Feb 5, 2016 at 11:31 AM, Dave Gordon <david.s.gordon at intel.com> wrote:
>> From: Chris Wilson <chris at chris-wilson.co.uk>
>>
>> Currently emit-request starts writing to the ring and reserves space
>> for a workaround to be emitted later whilst submitting the request. It
>> is easier to read if the caller only allocates sufficient space for
>> its access (then the reader can quickly verify that the ring begin
>> allocates the exact space for the number of dwords emitted) and closes
>> the access to the ring. During submit, if we need to add the workaround,
>> we can reacquire ring access, in the assurance that we reserved space
>> for ourselves when beginning the request.
>>
>> v3:
>> Reinstated #define for WA_TAIL_DWORDS so we could accommodate GPUs
>> that required different amounts of padding, generalised NOOP fill
>> [Rodrigo Vivi], added W/A space to reserved amount and updated
>> code comments [Dave Gordon],
>>
>> Originally-by: Rodrigo Vivi <rodrigo.vivi at gmail.com>
>> Rewritten-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Further-tweaked-by: Dave Gordon <david.s.gordon at intel.com>
>>
>> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi at gmail.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Dave Gordon <david.s.gordon at intel.com>
>> Cc: Ben Widawsky <benjamin.widawsky at intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_lrc.c | 80 ++++++++++++++++++++++++----------------
>> 1 file changed, 49 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 3a03646..8b278f1 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -225,6 +225,13 @@ enum {
>> #define GEN8_CTX_ID_SHIFT 32
>> #define CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT 0x17
>>
>> +/*
>> + * Reserve space for 2 NOOPs at the end of each request,
>> + * to be used as a workaround for not being allowed to
>> + * do lite restore with HEAD==TAIL (WaIdleLiteRestore).
>> + */
>> +#define WA_TAIL_DWORDS 2
>
> This patch really organize things better, but I still don't like the
> WA_TAIL_DWORDS hardcoded here instead in a place where I can switch
> for different platofms.
1. it's a *workaround* for a hardware limitation. Let's hope it's not
even necessary on future chips!
2. it's a really small overhead, so why not just define it as the max
over all supported platforms?
3. it's a macro, that means it's not *hardcoded*; if there's ever a
platform where we actually need this fix and it's not the same number,
we can just redefine this as a function call. Would you prefer it to be
a function-type macro:
#define WA_TAIL_DWORDS(dev) (2) /* doesn't currently depend on dev */
so that it could more easily be converted to a platform-specific value
in future?
.Dave.
More information about the Intel-gfx
mailing list