[Intel-gfx] [PATCH 1/2] drm/i915/lrc: Clarify the format of the context image

[Michel Thierry]

On 7/12/2017 12:45 PM, Chris Wilson wrote:
> Quoting Michel Thierry (2017-07-12 20:30:31)
>> Not only the context image consist of two parts (the PPHWSP, and the
>> logical context state), but we also allocate a header at the start of
>> for sharing data with GuC. Thus every lrc looks like this:
>>
>>    | [guc]          | [hwsp] [logical state] |
>>    |<- our header ->|<- context image      ->|
>>
>> So far, we have oversimplified whenever we use each of these parts of the
>> context, just because the GuC header happens to be in page 0, and the
>> (PP)HWSP is in page 1. But this had led to using the same define for more
>> than one meaning (as a page index in the lrc and as 1 page).
>>
>> This patch adds defines for the GuC shared page, the PPHWSP page and the
>> start of the logical state. It also updated the places where the old
>> define was being used. Since we are not changing the size (or format) of
>> the context, there are no functional changes.
>>
>> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Signed-off-by: Michel Thierry <michel.thierry at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Oscar Mateo <oscar.mateo at intel.com>
>> Cc: intel-gvt-dev at lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/i915/gvt/scheduler.c       |  4 ++--
>>   drivers/gpu/drm/i915/i915_guc_submission.c |  4 ++--
>>   drivers/gpu/drm/i915/intel_lrc.c           | 11 +++++++----
>>   drivers/gpu/drm/i915/intel_lrc.h           | 25 ++++++++++++++++++++++---
>>   4 files changed, 33 insertions(+), 11 deletions(-)
>>
...
>> @@ -1695,7 +1695,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine)
>>   static int
>>   lrc_setup_hws(struct intel_engine_cs *engine, struct i915_vma *vma)
>>   {
>> -       const int hws_offset = LRC_PPHWSP_PN * PAGE_SIZE;
>> +       const int hws_offset = LRC_HEADER_PAGES * PAGE_SIZE;
> 
> This should be PPHWSP_PN since we explicitly want the pointer tot he
> pphwsp.
>Oh right, this was the only place where it was really correct.

I was about to resend it with this change, but then realised it just 
means the lrc_setup_hws chunk of the patch disappears... let me know if 
I should still resend it.

Thanks,

>>          void *hws;
>>   
>>          /* The HWSP is part of the default context object in LRC mode. */
>> @@ -2015,8 +2015,11 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
>>   
>>          context_size = round_up(engine->context_size, I915_GTT_PAGE_SIZE);
>>   
>> -       /* One extra page as the sharing data between driver and GuC */
>> -       context_size += PAGE_SIZE * LRC_PPHWSP_PN;
>> +       /*
>> +        * Before the actual start of the context image, we insert a few pages
>> +        * for our own use and for sharing with the GuC.
>> +        */
>> +       context_size += LRC_HEADER_PAGES * PAGE_SIZE;
> 
> Good riddance, that LRC_PPHWSP_PN has left me quite confused for some
> time.
> 
> With the change to lrc_setup_hws,
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> -Chris
>