Hi Matthew:
Thanks for spending efforts on rebase! :) I'm sorry that we will drop this
2 patches in the next round patch review.
In the previous discussion, there were two options:
Option A. Unify GVT-g PPGTT shadow with i915 PPGTT routines, which needs these
2 patches, then LRC submission routines could generate context descriptor
according to the addressing space mode in i915_hw_ppgtt. (Daniel)
The reason this option is dropped because:
a. Different requirement. In i915, The caller of PPGTT routines mostly is
VMA-oriented. It doesn't care about how the page table created. Usually i915
will create the upper level of page table according to the wanted VMA. And
GVT-g shadow is PTE-oriented, mostly it will populate the page table according
to how guest modifies its PPGTT.
b. Different abstraction. GVT-g shadow is based on an unique-leveled page table
manipulation design. E.g. both PML4/PDP/PDE population could use same
functions, different with i915, which uses different functions to handle
different PML4/PDP/PDE population. It hard to merge them together without big
changes. We definitely want to prevent heavy modifications of i915, as that
might cause regressions.
Option B. Modify the related functions in intel_lrc.c to update the PDP root
pointers in each submission.
And the pros is we don't need to modify i915 PPGTT now. Definitely cons here is
we need to modify LRC related routines. Chris suggested we could use
ppgtt->pd_dirty_rings before, and the ppgtt->drity_pd_rings is highly combined
with 32-bit page table routines. I think we might needs some modifications
The ideal option we are currently thinking is:
a. Issue LRIs to update PDPs in shadow ring buffer so that we don't need to
modify PDP loading routines in intel_lrc.c
b. Add addressing mode bit in intel_context and make it configurable.
Definitely i915 host context will configure this bit according to
i915.enable_ppgtt. And GVT-g could configure it by guest requirements.
Thanks,
Zhi.
-----Original Message-----
From: Chris Wilson [mailto:[email protected]]
Sent: Tuesday, April 26, 2016 8:56 AM
To: Auld, Matthew <[email protected]>
Cc: [email protected]; Wang, Zhi A <[email protected]>; Joonas
Lahtinen <[email protected]>
Subject: Re: [PATCH 2/2] drm/i915: generate address mode bit from PPGTT instance
On Tue, Apr 26, 2016 at 04:17:52PM +0100, Matthew Auld wrote:
> From: "Wang, Zhi A" <[email protected]>
>
> After the per-PPGTT address mode gets support, the LRC submission
> should generate the address mode bit from PPGTT instance, instead of
> the hard-coded system configuration.
>
> v2:
> (Matthew Auld)
> - rebase on latest -nightly
>
> Cc: Chris Wilson <[email protected]>
> Cc: Joonas Lahtinen <[email protected]>
> Signed-off-by: Matthew Auld <[email protected]>
> Signed-off-by: Zhi Wang <[email protected]>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> b/drivers/gpu/drm/i915/intel_lrc.c
> index 13cb1b3..17bd811 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -214,7 +214,7 @@ enum {
> LEGACY_64B_CONTEXT
> };
> #define GEN8_CTX_ADDRESSING_MODE_SHIFT 3 -#define
> GEN8_CTX_ADDRESSING_MODE(dev) (USES_FULL_48BIT_PPGTT(dev) ?\
> +#define GEN8_CTX_ADDRESSING_MODE(ppgtt) (IS_48BIT_PPGTT(ppgtt) ? \
> LEGACY_64B_CONTEXT :\
> LEGACY_32B_CONTEXT)
> enum {
> @@ -276,8 +276,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) <<
> - GEN8_CTX_ADDRESSING_MODE_SHIFT;
> if (IS_GEN8(dev))
> engine->ctx_desc_template |= GEN8_CTX_L3LLC_COHERENT;
> engine->ctx_desc_template |= GEN8_CTX_PRIVILEGE; @@ -319,7 +317,9 @@
> intel_lr_context_descriptor_update(struct intel_context *ctx,
> lrca = ctx->engine[engine->id].lrc_vma->node.start +
> LRC_PPHWSP_PN * PAGE_SIZE;
>
> - desc = engine->ctx_desc_template; /* bits
> 0-11 */
> + desc = engine->ctx_desc_template; /* bits 0-11 */
> + desc |= GEN8_CTX_ADDRESSING_MODE(ctx->ppgtt) << /* bits 3-4 */
> + GEN8_CTX_ADDRESSING_MODE_SHIFT;
Would it not be simpler for us to use the GEN8_CTX_ADDRESSING_MODE() as our
enum, then we would just do desc |= ctx->ppgtt->addressing_mode <<
GEN8_CTX_ADDRESSING_MODE_SHIFT;
And then we would have an enum already defined!
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
