[Intel-gfx] [PATCH] drm/i915/guc: Don't make assumptions while getting the lrca offset

[Michel Thierry]

On 7/11/2017 5:54 PM, Chris Wilson wrote:
> Quoting Michel Thierry (2017-07-12 01:14:46)
>> On 7/11/2017 5:07 PM, Chris Wilson wrote:
>>> Quoting Chris Wilson (2017-07-12 01:00:02)
>>>> Also feel free to try and fixup the other confusion in intel_lrc.c.
>>>> Perhaps something like:
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>>> index b0738d2..f498aa6 100644
>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>>> @@ -2031,8 +2031,9 @@ 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;
>>>> +       /* Add an extra page at the start for sharing data with the GuC */
>>>> +       BUILD_BUG_ON(LRC_PPHWSP_PN != 1);
>>>> +       context_size += PAGE_SIZE;
>>>>    
>>>>           ctx_obj = i915_gem_object_create(ctx->i915, context_size);
>>>>           if (IS_ERR(ctx_obj)) {
>>>
>>> Or better
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>> index b0738d2..0b7fb38 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -2031,8 +2031,8 @@ 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;
>>> +       /* Add an extra page at the start for sharing data with the GuC */
>>> +       context_size += LRC_GUCSHR_PAGES * PAGE_SIZE;
> 
> One more thought about this, and with a little coaxing LRC_PPHWSP_PN may
> be the most appropriate constant to use here.
> 
> /* 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;
> 
>>>           ctx_obj = i915_gem_object_create(ctx->i915, context_size);
>>>           if (IS_ERR(ctx_obj)) {
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
>>> index 52b3a1f..848ca21 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.h
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.h
>>> @@ -72,7 +72,8 @@ int logical_xcs_ring_init(struct intel_engine_cs *engine);
>>>    
>>>    /* One extra page is added before LRC for GuC as shared data */
>>>    #define LRC_GUCSHR_PN  (0)
>>> -#define LRC_PPHWSP_PN  (LRC_GUCSHR_PN + 1)
>>> +#define LRC_GUCSHR_PAGES 1
>>> +#define LRC_PPHWSP_PN  (LRC_GUCSHR_PN + LRC_GUCSHR_PAGES)
>>>    #define LRC_STATE_PN   (LRC_PPHWSP_PN + 1)
> 
> but
> 
> /* We allocate a header at the start of the context image for our own
>   * use, therefore the actual location of the logical state is offset
>   * from the start of the VMA. The layout is
>   *
>   * | [guc]          | [hwsp] [logical state] |
>   * |<- our header ->|<- context image      ->|
>   *
>   */
> 
> /* One extra page is used for sharing with the GuC */
> #define LRC_GUCSHR_PN  (0)
> #define LRC_GUCSHR_PAGES 1
> 
> /* At the start of the context image is its per-process HWS page */
> #define LRC_PPHWSP_PN  (LRC_GUCSHR_PN + LRC_GUCSHR_PAGES)
> 
> /* Finally we have the logical state for the context */
> #define LRC_STATE_PN   (LRC_PPHWSP_PN + 1)
> 
> /* Currently we include the PPHWSP in __intel_engine_context_size() so
>   * the size of the header is synonymous with the start of the PPHWSP.
>   */
> #define LRC_HEADER_PAGES LRC_PPHWSP_PN
> 

It cannot be clearer now.

The only extra thing I was thinking about was also defining the 'size' 
(length?) of the guc and pphwsp sections, in case those "+ 1" were not 
clear:


  #define LRC_GUCSHR_PN  (0)
-#define LRC_PPHWSP_PN  (LRC_GUCSHR_PN + 1)
-#define LRC_STATE_PN   (LRC_PPHWSP_PN + 1)
+#define LRC_GUCSHR_SZ  (1)
+#define LRC_PPHWSP_PN  (LRC_GUCSHR_PN + LRC_GUCSHR_SZ)
+#define LRC_PPHWSP_SZ  (1)
+#define LRC_STATE_PN   (LRC_PPHWSP_PN + LRC_PPHWSP_SZ)

>>>    
>>>    struct drm_i915_private;
>>>
>>> Nowhere do we use LRC_GUCSHR_PN. Having it just gives us the mirage of
>>> flexibility...
>>
>> LRC_GUCSHR_PN is not used because people assumed it is always page 0 of
>> the kernel ctx :S
>>
>> One more thing to fix, use LRC_GUCSHR_PN in all the places where this
>> _shared_ GuC page is used (guc_ggtt_offset(ctx->engine[RCS].state))
> 
> That may prove very handy.
> -Chris
>