[Intel-gfx] [PATCH v2] drm/i915: Use correct reST syntax for WOPCM and GuC kernel-doc diagrams

[Yaodong Li]

On 03/14/2018 12:26 PM, Michal Wajdeczko wrote:
> On Wed, 14 Mar 2018 19:44:43 +0100, Jackie Li <yaodong.li at intel.com> 
> wrote:
>
>> GuC Address Space and WOPCM Layout diagrams won't be generated 
>> correctly by
>> sphinx build if not using proper reST syntax.
>>
>> This patch uses reST literal blocks to make sure GuC Address Space and
>> WOPCM Layout diagrams to be generated correctly, and it also corrects 
>> some
>> errors in the diagram description.
>>
>> v2:
>>  - Fixed errors in diagram description
>>
>> Signed-off-by: Jackie Li <yaodong.li at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_guc.c   | 52 
>> ++++++++++++++++++++------------------
>>  drivers/gpu/drm/i915/intel_wopcm.c | 44 
>> +++++++++++++++++---------------
>>  2 files changed, 50 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c 
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 3eb516e..6a4f36e 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -495,35 +495,37 @@ int intel_guc_resume(struct intel_guc *guc)
>>  /**
>>   * DOC: GuC Address Space
>>   *
>> - * The layout of GuC address space is shown as below:
>> + * The layout of GuC address space is shown below:
>>   *
>> - *    +==============> +====================+ <== GUC_GGTT_TOP
>> - *    ^                |                    |
>> - *    |                |                    |
>> - *    |                |        DRAM        |
>> - *    |                |       Memory       |
>> - *    |                |                    |
>> - *   GuC               |                    |
>> - * Address  +========> +====================+ <== WOPCM Top
>> - *  Space   ^          |   HW contexts RSVD |
>> - *    |     |          |        WOPCM       |
>> - *    |     |     +==> +--------------------+ <== GuC WOPCM Top
>> - *    |    GuC    ^    |                    |
>> - *    |    GGTT   |    |                    |
>> - *    |    Pin   GuC   |        GuC         |
>> - *    |    Bias WOPCM  |       WOPCM        |
>> - *    |     |    Size  |                    |
>> - *    |     |     |    |                    |
>> - *    v     v     v    |                    |
>> - *    +=====+=====+==> +====================+ <== GuC WOPCM Base
>> - *                     |   Non-GuC WOPCM    |
>> - *                     |   (HuC/Reserved)   |
>> - *                     +====================+ <== WOPCM Base
>> + * ::
>> + *
>> + *     +==============> +====================+ <== GUC_GGTT_TOP
>> + *     ^                |                    |
>> + *     |                |                    |
>> + *     |                |        DRAM        |
>> + *     |                |       Memory       |
>> + *     |                |                    |
>> + *    GuC               |                    |
>> + *  Address  +========> +====================+ <== WOPCM Top
>> + *   Space   ^          |   HW contexts RSVD |
>> + *     |     |          |        WOPCM       |
>> + *     |     |     +==> +--------------------+ <== GuC WOPCM Top
>> + *     |    GuC    ^    |                    |
>> + *     |    GGTT   |    |                    |
>> + *     |    Pin   GuC   |        GuC         |
>> + *     |    Bias WOPCM  |       WOPCM        |
>> + *     |     |    Size  |                    |
>> + *     |     |     |    |                    |
>> + *     v     v     v    |                    |
>> + *     +=====+=====+==> +====================+ <== GuC WOPCM Base
>> + *                      |   Non-GuC WOPCM    |
>> + *                      |   (HuC/Reserved)   |
>> + *                      +====================+ <== WOPCM Base
>>   *
>>   * The lower part [0, GuC ggtt_pin_bias) is mapped to WOPCM which 
>> consists of
>>   * GuC WOPCM and WOPCM reserved for other usage (e.g.RC6 context). The
>
> hmm, "lower part [...)" is little ambiguous here, as one may look for
> "upper part [...)", so maybe better to be explicit:
>
>     "The lower part of GuC Address Space ie. [0, ggtt_pin_bias)
>     is mapped to WOPCM, while upper part of GuC Address Space ie.
>     [ggtt_pin_bias, GUC_GGTT_TOP) is mapped to DRAM."
>
Agree. it's more clear in this way. but I think we should remove "ie." 
update it to
"The lower part of GuC Address Space [0, ggtt_pin_bias) is mapped to 
WOPCM"?
>> value of
>> - * the GuC ggtt_pin_bias is determined by the actually GuC WOPCM 
>> size which is
>> - * set in GUC_WOPCM_SIZE register.
>> + * the GuC ggtt_pin_bias is determined by the GuC WOPCM size which 
>> is set in
>> + * GUC_WOPCM_SIZE register.
>>   */
>
> hmm, I'm not sure that above statement is correct, compare your diagram
> and calculation:
>
>     guc->ggtt_pin_bias = i915->wopcm.size - i915->wopcm.guc.base;
>
> also I would not mention registers here, as we don't read them while
> calculating bias, so maybe something like this:
>
>     "The value of ggtt_pin_bias is determined by the WOPCM size and
>     actual GuC WOPCM base."
>
Thanks. Will update it.

Regards,
-Jackie