Re: [PATCH v5 2/5] drm/i915/guc: Move the pin bias value from GuC to GGTT

["Michal Wajdeczko" <michal.wajdeczko@xxxxxxxxx>]

On Wed, 25 Jul 2018 12:56:53 +0200, Jakub Bartmiński  
<jakub.bartminski@xxxxxxxxx> wrote:

> Removing the pin bias from GuC allows us to not check for GuC every time
> we pin a context, which fixes the assertion error on unresolved GuC
> platform default in mock contexts selftest.
>
> It also seems that we were using uninitialized WOPCM variables when
> setting the GuC pin bias. The pin bias has to be set after the WOPCM,
> but before the call to i915_gem_contexts_init where the first contexts
> are pinned.
>
> v2:
> This also makes it so that there's no need to set GuC variables from
> within the WOPCM init function or to move the WOPCM init, while keeping
> the correct initialization order. Also for mock tests the pin bias is
> left at 0 and we make sure that the pin bias with GuC will not be
> smaller than without GuC.
>
> v3:
> Avoid unused i915 in intel_guc_ggtt_offset if debug is disabled.
>
> v4:
> Squash with WOPCM init reordering.
> Moved the i915_ggtt_pin_bias helper to this patch, and made some
> functions use it instead of directly dereferencing i915->ggtt.
>
> v5:
> Since we now don't use wopcm.guc.base for the pin bias there's no need to
> validate it. It also has already been verified in WOPCM init.
>
> Fixes: f7dc0157e4b5 ("drm/i915/uc: Fetch GuC/HuC firmwares from guc/huc  
> specific init")
> Testcase: igt/drv_selftest/mock_contexts #GuC
> Signed-off-by: Jakub Bartmiński <jakub.bartminski@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 22 ++++++------------
>  drivers/gpu/drm/i915/i915_gem_gtt.c     | 18 ++++++++++----
>  drivers/gpu/drm/i915/i915_gem_gtt.h     |  2 ++
>  drivers/gpu/drm/i915/i915_vma.h         |  5 ++++
>  drivers/gpu/drm/i915/intel_guc.c        | 31 +++++--------------------
>  drivers/gpu/drm/i915/intel_guc.h        | 11 ++++-----
>  drivers/gpu/drm/i915/intel_huc.c        |  2 +-
>  drivers/gpu/drm/i915/intel_uc_fw.c      |  2 +-
>  drivers/gpu/drm/i915/intel_wopcm.h      | 16 +++++++++++++
>  9 files changed, 55 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c  
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index b10770cfccd2..ae27caad1766 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -265,7 +265,7 @@ static u32 default_desc_template(const struct  
> drm_i915_private *i915,
>  }
> static struct i915_gem_context *
> -__create_hw_context(struct drm_i915_private *dev_priv,
> +__create_hw_context(struct drm_i915_private *i915,

please avoid s/dev_priv/i915 as it unnecessarily makes this patch
harder to review (actual changes vs renames ratio is unbalanced)

>  		    struct drm_i915_file_private *file_priv)
>  {
>  	struct i915_gem_context *ctx;
> @@ -276,15 +276,15 @@ __create_hw_context(struct drm_i915_private  
> *dev_priv,
>  	if (ctx == NULL)
>  		return ERR_PTR(-ENOMEM);
> -	ret = assign_hw_id(dev_priv, &ctx->hw_id);
> +	ret = assign_hw_id(i915, &ctx->hw_id);
>  	if (ret) {
>  		kfree(ctx);
>  		return ERR_PTR(ret);
>  	}
> 	kref_init(&ctx->ref);
> -	list_add_tail(&ctx->link, &dev_priv->contexts.list);
> -	ctx->i915 = dev_priv;
> +	list_add_tail(&ctx->link, &i915->contexts.list);
> +	ctx->i915 = i915;
>  	ctx->sched.priority = I915_PRIORITY_NORMAL;
> 	for (n = 0; n < ARRAY_SIZE(ctx->__engine); n++) {
> @@ -322,22 +322,14 @@ __create_hw_context(struct drm_i915_private  
> *dev_priv,
>  	/* NB: Mark all slices as needing a remap so that when the context  
> first
>  	 * loads it will restore whatever remap state already exists. If there
>  	 * is no remap info, it will be a NOP. */
> -	ctx->remap_slice = ALL_L3_SLICES(dev_priv);
> +	ctx->remap_slice = ALL_L3_SLICES(i915);
> 	i915_gem_context_set_bannable(ctx);
>  	ctx->ring_size = 4 * PAGE_SIZE;
>  	ctx->desc_template =
> -		default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt);
> +		default_desc_template(i915, i915->mm.aliasing_ppgtt);
> -	/*
> -	 * GuC requires the ring to be placed in Non-WOPCM memory. If GuC is  
> not
> -	 * present or not in use we still need a small bias as ring wraparound
> -	 * at offset 0 sometimes hangs. No idea why.
> -	 */
> -	if (USES_GUC(dev_priv))
> -		ctx->ggtt_offset_bias = dev_priv->guc.ggtt_pin_bias;
> -	else
> -		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
> +	ctx->ggtt_offset_bias = i915->ggtt.pin_bias;
> 	return ctx;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c  
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index d0acef299b9c..1f5d0334f0f5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2901,7 +2901,7 @@ void i915_gem_fini_aliasing_ppgtt(struct  
> drm_i915_private *i915)
>  	ggtt->vm.vma_ops.unbind_vma = ggtt_unbind_vma;
>  }
> -int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
> +int i915_gem_init_ggtt(struct drm_i915_private *i915)
>  {
>  	/* Let GEM Manage all of the aperture.
>  	 *
> @@ -2912,12 +2912,20 @@ int i915_gem_init_ggtt(struct drm_i915_private  
> *dev_priv)
>  	 * aperture.  One page should be enough to keep any prefetching inside
>  	 * of the aperture.
>  	 */
> -	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> +	struct i915_ggtt *ggtt = &i915->ggtt;
>  	unsigned long hole_start, hole_end;
>  	struct drm_mm_node *entry;
>  	int ret;
> -	ret = intel_vgt_balloon(dev_priv);
> +	/*
> +	 * GuC requires the ring to be placed in Non-WOPCM memory. If GuC is  
> not
> +	 * present or not in use we still need a small bias as ring wraparound
> +	 * at offset 0 sometimes hangs. No idea why.
> +	 */
> +	ggtt->pin_bias = max_t(u32, I915_GTT_PAGE_SIZE,
> +			       intel_wopcm_guc_pin_bias(&i915->wopcm));
> +
> +	ret = intel_vgt_balloon(i915);
>  	if (ret)
>  		return ret;
> @@ -2940,8 +2948,8 @@ int i915_gem_init_ggtt(struct drm_i915_private  
> *dev_priv)
>  	/* And finally clear the reserved guard page */
>  	ggtt->vm.clear_range(&ggtt->vm, ggtt->vm.total - PAGE_SIZE, PAGE_SIZE);
> -	if (USES_PPGTT(dev_priv) && !USES_FULL_PPGTT(dev_priv)) {
> -		ret = i915_gem_init_aliasing_ppgtt(dev_priv);
> +	if (USES_PPGTT(i915) && !USES_FULL_PPGTT(i915)) {
> +		ret = i915_gem_init_aliasing_ppgtt(i915);
>  		if (ret)
>  			goto err;
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h  
> b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 14e62651010b..71e8cf24c800 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -396,6 +396,8 @@ struct i915_ggtt {
> 	int mtrr;
> +	u32 pin_bias;
> +
>  	struct drm_mm_node error_capture;
>  };
> diff --git a/drivers/gpu/drm/i915/i915_vma.h  
> b/drivers/gpu/drm/i915/i915_vma.h
> index f06d66377107..abf6144e3296 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -207,6 +207,11 @@ static inline u32 i915_ggtt_offset(const struct  
> i915_vma *vma)
>  	return lower_32_bits(vma->node.start);
>  }
> +static inline u32 i915_ggtt_pin_bias(struct i915_vma *vma)
> +{
> +	return i915_vm_to_ggtt(vma->vm)->pin_bias;
> +}
> +
>  static inline struct i915_vma *i915_vma_get(struct i915_vma *vma)
>  {
>  	i915_gem_object_get(vma->obj);
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index aa28368f8ba7..74043a13e9bc 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -27,8 +27,6 @@
>  #include "intel_guc_submission.h"
>  #include "i915_drv.h"
> -static void guc_init_ggtt_pin_bias(struct intel_guc *guc);
> -
>  static void gen8_guc_raise_irq(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> @@ -142,8 +140,6 @@ int intel_guc_init_misc(struct intel_guc *guc)
>  	struct drm_i915_private *i915 = guc_to_i915(guc);
>  	int ret;
> -	guc_init_ggtt_pin_bias(guc);
> -
>  	ret = guc_init_wq(guc);
>  	if (ret)
>  		return ret;
> @@ -603,22 +599,6 @@ int intel_guc_resume(struct intel_guc *guc)
>   * 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 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);
> -
> -	guc->ggtt_pin_bias = i915->wopcm.guc.size;
> -}
> -
>  /**
>   * intel_guc_allocate_vma() - Allocate a GGTT VMA for GuC usage
>   * @guc:	the guc
> @@ -634,21 +614,22 @@ static void guc_init_ggtt_pin_bias(struct  
> intel_guc *guc)
>   */
>  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
>  {
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	struct drm_i915_private *i915 = guc_to_i915(guc);
>  	struct drm_i915_gem_object *obj;
>  	struct i915_vma *vma;
> +	u64 flags;
>  	int ret;
> -	obj = i915_gem_object_create(dev_priv, size);
> +	obj = i915_gem_object_create(i915, size);
>  	if (IS_ERR(obj))
>  		return ERR_CAST(obj);
> -	vma = i915_vma_instance(obj, &dev_priv->ggtt.vm, NULL);
> +	vma = i915_vma_instance(obj, &i915->ggtt.vm, NULL);
>  	if (IS_ERR(vma))
>  		goto err;
> -	ret = i915_vma_pin(vma, 0, PAGE_SIZE,
> -			   PIN_GLOBAL | PIN_OFFSET_BIAS | guc->ggtt_pin_bias);
> +	flags = PIN_GLOBAL | PIN_OFFSET_BIAS | i915_ggtt_pin_bias(vma);
> +	ret = i915_vma_pin(vma, 0, PAGE_SIZE, flags);
>  	if (ret) {
>  		vma = ERR_PTR(ret);
>  		goto err;
> diff --git a/drivers/gpu/drm/i915/intel_guc.h  
> b/drivers/gpu/drm/i915/intel_guc.h
> index 4121928a495e..751f31c3c6c4 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -49,9 +49,6 @@ struct intel_guc {
>  	struct intel_guc_log log;
>  	struct intel_guc_ct ct;
> -	/* Offset where Non-WOPCM memory starts. */
> -	u32 ggtt_pin_bias;
> -
>  	/* Log snapshot if GuC errors during load */
>  	struct drm_i915_gem_object *load_err_log;
> @@ -130,10 +127,10 @@ static inline void  
> intel_guc_to_host_event_handler(struct intel_guc *guc)
>   * @vma: i915 graphics virtual memory area.
>   *
>   * GuC does not allow any gfx GGTT address that falls into range
> - * [0, GuC ggtt_pin_bias), which is reserved for Boot ROM, SRAM and  
> WOPCM.
> - * Currently, in order to exclude [0, GuC ggtt_pin_bias) address space  
> from
> + * [0, ggtt.pin_bias), which is reserved for Boot ROM, SRAM and WOPCM.
> + * Currently, in order to exclude [0, ggtt.pin_bias) address space from
>   * GGTT, all gfx objects used by GuC are allocated with  
> intel_guc_allocate_vma()
> - * and pinned with PIN_OFFSET_BIAS along with the value of GuC  
> ggtt_pin_bias.
> + * and pinned with PIN_OFFSET_BIAS along with the value of  
> ggtt.pin_bias.
>   *
>   * Return: GGTT offset of the @vma.
>   */
> @@ -142,7 +139,7 @@ static inline u32 intel_guc_ggtt_offset(struct  
> intel_guc *guc,
>  {
>  	u32 offset = i915_ggtt_offset(vma);
> -	GEM_BUG_ON(offset < guc->ggtt_pin_bias);
> +	GEM_BUG_ON(offset < i915_ggtt_pin_bias(vma));
>  	GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
> 	return offset;
> diff --git a/drivers/gpu/drm/i915/intel_huc.c  
> b/drivers/gpu/drm/i915/intel_huc.c
> index ffcad5fad6a7..37ef540dd280 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -63,7 +63,7 @@ int intel_huc_auth(struct intel_huc *huc)
>  		return -ENOEXEC;
> 	vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
> -				       PIN_OFFSET_BIAS | guc->ggtt_pin_bias);
> +				       PIN_OFFSET_BIAS | i915->ggtt.pin_bias);
>  	if (IS_ERR(vma)) {
>  		ret = PTR_ERR(vma);
>  		DRM_ERROR("HuC: Failed to pin huc fw object %d\n", ret);
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c  
> b/drivers/gpu/drm/i915/intel_uc_fw.c
> index 6e8e0b546743..fd496416087c 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
> @@ -222,7 +222,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>  		goto fail;
>  	}
> -	ggtt_pin_bias = to_i915(uc_fw->obj->base.dev)->guc.ggtt_pin_bias;
> +	ggtt_pin_bias = to_i915(uc_fw->obj->base.dev)->ggtt.pin_bias;
>  	vma = i915_gem_object_ggtt_pin(uc_fw->obj, NULL, 0, 0,
>  				       PIN_OFFSET_BIAS | ggtt_pin_bias);
>  	if (IS_ERR(vma)) {
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.h  
> b/drivers/gpu/drm/i915/intel_wopcm.h
> index 6298910a384c..0130165a9cdd 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.h
> +++ b/drivers/gpu/drm/i915/intel_wopcm.h
> @@ -7,6 +7,8 @@
>  #ifndef _INTEL_WOPCM_H_
>  #define _INTEL_WOPCM_H_
> +#include "i915_gem.h"

system headers shall be included first, so move new one down

> +#include "i915_utils.h"

do we really need this header here ?

>  #include <linux/types.h>
> /**
> @@ -24,6 +26,20 @@ struct intel_wopcm {
>  	} guc;
>  };
> +/**
> + * intel_wopcm_guc_pin_bias() - Get the GuC pin bias value.
> + * @wopcm: pointer to intel_wopcm.
> + *
> + * Returns:
> + * 0 if GuC is not present or not in use.
> + * Otherwise, the GuC pin bias value based on the GuC WOPCM size.
> + */
> +static inline u32 intel_wopcm_guc_pin_bias(struct intel_wopcm *wopcm)

hmm, I'm not sure that we should put GuC address space limitations/details
inside WOPCM code (which we wanted to be focused on partitioning only).

as this is used/needed only once in i915_gem_init_ggtt(), btw already with
some comment about wopcm, maybe you can inline this simply code there ?

or to be fully object oriented export this function from guc.c:

void intel_guc_get_ggtt_pin_bias(struct intel_guc *guc)
{
	return guc_to_i915(guc)->wopcm.guc.size;
}

> +{
> +	GEM_BUG_ON(!wopcm->size);
> +	return wopcm->guc.size;
> +}
> +
>  void intel_wopcm_init_early(struct intel_wopcm *wopcm);
>  int intel_wopcm_init(struct intel_wopcm *wopcm);
>  int intel_wopcm_init_hw(struct intel_wopcm *wopcm);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx