Re: [PATCH v10 3/7] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Yaodong Li <yaodong.li@intel.com>]

On 02/14/2018 09:38 AM, Michal Wajdeczko wrote:
> On Wed, 14 Feb 2018 03:26:12 +0100, Yaodong Li <yaodong.li@intel.com> 
> wrote:
>
>>
>> On 02/13/2018 07:56 AM, Michal Wajdeczko wrote:
>>>
>>> Also, comparing again your drawing with the spec I think it
>>> should look like this:
>>>
>>>    +-------- +====================+ <== WOPCM top
>>>   /|\        |     HW RESERVED    |
>>>    |         +------------------- +
>>>    |         |                    |
>>>    |     +-- +====================+ <== GuC WOPCM top
>>>    |    /|\  |                    |
>>>    |     |   |                    |
>>>    |    GuC  |                    |
>>>    |   WOPCM |    GuC firmware    |
>>>    |    size |                    |
>>>  WOPCM   |   +--------------------+
>>>   size  \|/  |    GuC reserved    |
>>>    |     +-- +====================+ <== GuC WOPCM base (offset from 
>>> WOPCM base)
>>>    |         |   WOPCM RESERVED   |
>>>    |         +--------------------+
>>>   \|/        |    HuC firmware    |
>>>    +-------- +====================+ <== WOPCM base
>>>
>>>
>>>> +
>>>> +/* GUC WOPCM offset needs to be 16KB aligned. */
>>>> +#define GUC_WOPCM_OFFSET_ALIGNMENT    (16 << 10)
>>>> +/* 16KB reserved at the beginning of GuC WOPCM. */
>>>> +#define GUC_WOPCM_RESERVED        (16 << 10)
>>>> +/* 8KB from GUC_WOPCM_RESERVED is reserved for GuC stack. */
>>>> +#define GUC_WOPCM_STACK_RESERVED    (8 << 10)
>>>> +/* 24KB at the end of GuC WOPCM is reserved for RC6 CTX on BXT. */
>>>> +#define BXT_GUC_WOPCM_RC6_CTX_RESERVED    (24 << 10)
>>>
>>> Hmm, I'm still not convinced that we correctly attach it to "GuC 
>>> WOPCM".
>>>
>>> In the result, maybe we should stop focusing on "intel_guc_wopcm" 
>>> and define
>>> everything as "intel_wopcm" as it was earlier suggested by Joonas.
>>>
>>> Then we can define our struct to match diagram above as
>>>
>>> struct intel_wopcm {
>>>     u32 size;
>>>     struct {
>>>         u32 base;
>>>         u32 size;
>>>     } guc;
>>> };
>>>
>>> u32 intel_wopcm_guc_top(struct intel_wopcm *wopcm)
>>> {
>>>     return wopcm->guc.base + wopcm->guc.size;
>>> }
>> More comments about the *top*:-)
>>
>> The *top* we used in driver code to verify where the non-wopcm guc 
>> memory
>> allocation starts is the offset value from guc.base to WOPCM_TOP. so 
>> we don't need to
>> add the guc.base to it.
>>
>>  From GuC point of view the *top* (boundary between wopcm and 
>> non-wopcm memory)
>> is like:
>> +------------------+ <=== GuC memory end
>>
>>     Non WOPCM
>>
>> +------------------+ <=== *top* (This is actually in top of the whole 
>> WOPCM)
>>       CTX Rsvd
>> +------------------+ <=== *__top*  (wopcm->guc.size)
>>        WOPCM
>> +------------------+ <=== guc base (wopcm->guc.base)
>>
>> actually, we had a discussion whether we should set the guc wopcm and 
>> non-wopcm
>> boundary to *__top* but the suggestion we got is to stick to the spec 
>> description which
>> says to allocate "non-wopcm memory starts from WOPCM TOP"(even if it 
>> seems
>> we could use *__top* as the boundary:-)). and it's because of this 
>> reason we called the
>> wopcm from guc_base to *top* as GuC WOPCM since we count the wopcm 
>> between
>> *__top* to *top* as a part of GuC address space while trying to 
>> allocate non-wopcm
>> for GuC (even though GuC won't access CTX Rsvd at allO:-)).
>
> Ok, so we should program GUC_WOPCM_SIZE to wopcm->guc.size and then
> use (wopcm->size - wopcm->guc.base) as offset to i915_vma_pin, right?
>
All values of size and top are relevant to guc.base (offsets from 
guc.base), and currently
we are using guc.size + hole between guc.size and ctx reserved (if any) 
+ ctx reserved size
as the offset to i915_vma_pin as it's the actual WOPCM_TOP.:-)

Regards,
-Jackie

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