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

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

On 05/02/16 22:56, Rodrigo Vivi wrote:
> On Fri, Feb 5, 2016 at 11:31 AM, Dave Gordon <david.s.gordon@intel.com> wrote:
>> From: Chris Wilson <chris@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@gmail.com>
>> Rewritten-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Further-tweaked-by: Dave Gordon <david.s.gordon@intel.com>
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Ben Widawsky <benjamin.widawsky@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.

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