[Intel-gfx] [PATCH v4 1/5] drm/i915/guc: Avoid wasting memory on incorrect GuC pin bias

[Michal Wajdeczko]

On Fri, 20 Jul 2018 15:33:51 +0200, Jakub Bartmiński  
<jakub.bartminski at intel.com> wrote:

> It would appear that the calculated GuC pin bias was larger than it
> should be, as the GuC address space does NOT contain the "HW contexts  
> RSVD"
> part of the WOPCM. Thus, the GuC pin bias is simply the GuC WOPCM size.
>
> Signed-off-by: Jakub Bartmiński <jakub.bartminski at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc.c | 50 ++++++++++++++------------------
>  1 file changed, 22 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index e12bd259df17..17753952933e 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -582,50 +582,44 @@ int intel_guc_resume(struct intel_guc *guc)
>   *
>   * ::
>   *
> - *     +==============> +====================+ <== 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    +====> +====================+ <== GuC WOPCM Top
> + *   Space     ^      |                    |
> + *     |       |      |                    |
> + *     |      GuC     |        GuC         |
> + *     |     WOPCM    |       WOPCM        |
> + *     |      Size    |                    |
> + *     |       |      |                    |
> + *     v       v      |                    |
> + *     +=======+====> +====================+ <== GuC WOPCM Base

as things are now simpler, can you clarify this diagram little more,
like this:

     + ----------- +====================+ <= FFFF FFFF
     ^             |      Reserved      |
     |             +====================+ <= GUC_GGTT_TOP
     |             |                    |
    GuC            |       DRAM         |
  Address          |                    |
   Space      + -- +====================+ <= GuC's ggtt_pin_bias
     |        ^    |                    |
     |        |    |                    |
     |       GuC   |                    |
     |      WOPCM  |       WOPCM        |
     |       Size  |                    |
     |        |    |                    |
     v        v    |                    |
     + ------ + -- +====================+ <= 0000 0000


>   *
>   * The lower part of GuC Address Space [0, ggtt_pin_bias) is mapped to  
> WOPCM
>   * while upper part of GuC Address Space [ggtt_pin_bias, GUC_GGTT_TOP)  
> is mapped
> - * to DRAM. The value of the GuC ggtt_pin_bias is determined by WOPCM  
> size and
> - * actual GuC WOPCM size.
> + * to DRAM. The value of the GuC ggtt_pin_bias is the GuC WOPCM size.
>   */
> /**
>   * guc_init_ggtt_pin_bias() - Initialize the GuC ggtt_pin_bias value.
>   * @guc: intel_guc structure.
>   *
> - * This function will calculate and initialize the ggtt_pin_bias value  
> based on
> - * overall WOPCM size and GuC WOPCM size.
> + * This function will calculate and initialize the ggtt_pin_bias value
> + * based on the GuC WOPCM size.
>   */
>  static void guc_init_ggtt_pin_bias(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *i915 = guc_to_i915(guc);
> 	GEM_BUG_ON(!i915->wopcm.size);

hmm, maybe we should only care about i915->wopcm.guc.size ?

> -	GEM_BUG_ON(i915->wopcm.size < i915->wopcm.guc.base);
> +	GEM_BUG_ON(range_overflows(i915->wopcm.guc.base, i915->wopcm.guc.size,
> +				   i915->wopcm.size));

why do you want to validate base/size here? you don't use guc.base anymore
and, btw, it should be already calculated/verified in intel_wopcm.c

> -	guc->ggtt_pin_bias = i915->wopcm.size - i915->wopcm.guc.base;
> +	guc->ggtt_pin_bias = i915->wopcm.guc.size;
>  }
> /**

maybe we should also add

Bspec: 1180

as patch seems to be aligned with it

with all above,

Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>