Re: [PATCH v6 6/9] drm/i915: Make addressing mode bits in context descriptor configurable

Joonas Lahtinen
2016-06-03T09:26:03Z

On to, 2016-06-02 at 12:36 -0400, Zhi Wang wrote:
> Currently the addressing mode bit in context descriptor is statically
> generated from the configuration of system-wide PPGTT usage model.
>
> GVT-g will load the PPGTT shadow page table by itself and probably one
> guest is using a different addressing mode with i915 host. The addressing
> mode bits of a LRC context should be configurable under this case.
>
> v6:
>
> - Directly save the addressing mode bits inside i915_gem_context. (Chris)
> - Move the LRC context addressing mode bits into intel_lrc.h. (Chris)
>
> v5:
>
> - Change USES_FULL_48BIT(dev) to USES_FULL_48BIT(dev_priv) (Tvrtko)
>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  1 +
>  drivers/gpu/drm/i915/i915_gem_context.c |  2 ++
>  drivers/gpu/drm/i915/intel_lrc.c        | 13 +------------
>  drivers/gpu/drm/i915/intel_lrc.h        | 11 +++++++++++
>  4 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a4af035..f6cb60a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -879,6 +879,7 @@ struct i915_gem_context {
>   bool initialised;
>   } engine[I915_NUM_ENGINES];
>   u32 lrc_ring_buffer_size;
> + u32 lrc_addressing_mode_bits;
>
>   struct list_head link;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 1663981..d9d7779 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -296,6 +296,8 @@ __create_hw_context(struct drm_device *dev,
>
>   ctx->hang_stats.ban_period_seconds = DRM_I915_CTX_BAN_PERIOD;
>   ctx->lrc_ring_buffer_size = 4 * PAGE_SIZE;
> + ctx->lrc_addressing_mode_bits = GEN8_CTX_ADDRESSING_MODE(dev_priv) <<
> + GEN8_CTX_ADDRESSING_MODE_SHIFT;
>
>   return ctx;
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 72a0cca..ffb436c 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -208,16 +208,6 @@
>  } while (0)
>
>  enum {
> - ADVANCED_CONTEXT = 0,
> - LEGACY_32B_CONTEXT,
> - ADVANCED_AD_CONTEXT,
> - LEGACY_64B_CONTEXT
> -};
> -#define GEN8_CTX_ADDRESSING_MODE_SHIFT 3
> -#define GEN8_CTX_ADDRESSING_MODE(dev)  (USES_FULL_48BIT_PPGTT(dev) ?\
> - LEGACY_64B_CONTEXT :\
> - LEGACY_32B_CONTEXT)
> -enum {
>   FAULT_AND_HANG = 0,
>   FAULT_AND_HALT, /* Debug only */
>   FAULT_AND_STREAM,
> @@ -281,8 +271,6 @@ logical_ring_init_platform_invariants(struct intel_engine_cs *engine)
>   (engine->id == VCS || engine->id == VCS2);
>
>   engine->ctx_desc_template = GEN8_CTX_VALID;
> - engine->ctx_desc_template |= GEN8_CTX_ADDRESSING_MODE(dev_priv) <<
> -    GEN8_CTX_ADDRESSING_MODE_SHIFT;
>   if (IS_GEN8(dev_priv))
>   engine->ctx_desc_template |= GEN8_CTX_L3LLC_COHERENT;
>   engine->ctx_desc_template |= GEN8_CTX_PRIVILEGE;
> @@ -326,6 +314,7 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx,
>   BUILD_BUG_ON(MAX_CONTEXT_HW_ID > (1<
>
>   desc = engine->ctx_desc_template; /* bits  0-11 */
> + desc |= ctx->lrc_addressing_mode_bits; /* bits  3-4  */
>   desc |= ce->lrc_vma->node.start + LRC_PPHWSP_PN * PAGE_SIZE;
>   /* bits 12-31 */
>   desc |= (u64)ctx->hw_id << GEN8_CTX_ID_SHIFT; /* bits 32-52 */
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index a8db42a..e133c33 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -28,6 +28,17 @@
>
>  #define GEN8_LR_CONTEXT_ALIGN 4096
>
> +enum {
> + ADVANCED_CONTEXT = 0,
> + LEGACY_32B_CONTEXT,
> + ADVANCED_AD_CONTEXT,
> + LEGACY_64B_CONTEXT
> +};

I think these should be prefixed somehow?

> +#define GEN8_CTX_ADDRESSING_MODE_SHIFT 3
> +#define GEN8_CTX_ADDRESSING_MODE(dev_priv) (USES_FULL_48BIT_PPGTT(dev_priv) ?\
> + LEGACY_64B_CONTEXT : \
> + LEGACY_32B_CONTEXT)
> +

I'm also unsure if there is a story behind having these regs here
instead of i915_reg.h? I would not add more at least.

Regards, Joonas

>  /* Execlists regs */
>  #define RING_ELSP(ring) _MMIO((ring)->mmio_base + 0x230)
>  #define RING_EXECLIST_STATUS_LO(ring) _MMIO((ring)->mmio_base + 0x234)
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation