[Intel-gfx] [PATCH v5 2/5] drm/i915/guc: Move the pin bias value from GuC to GGTT
Michal Wajdeczko
michal.wajdeczko at intel.com
Wed Jul 25 17:45:45 UTC 2018
On Wed, 25 Jul 2018 12:56:53 +0200, Jakub Bartmiński
<jakub.bartminski at intel.com> 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 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/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);
More information about the Intel-gfx
mailing list