diff mbox

[2/2] drm/i915: generate address mode bit from PPGTT instance

Message ID 1461683872-13868-2-git-send-email-matthew.auld@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Matthew Auld April 26, 2016, 3:17 p.m. UTC 
From: "Wang, Zhi A" <zhi.a.wang@intel.com>

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 <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Chris Wilson April 26, 2016, 3:56 p.m. UTC | #1 
On Tue, Apr 26, 2016 at 04:17:52PM +0100, Matthew Auld wrote:
> From: "Wang, Zhi A" <zhi.a.wang@intel.com>
> 
> 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 <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> ---
>  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
Wang, Zhi A April 26, 2016, 6:51 p.m. UTC | #2 
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:chris@chris-wilson.co.uk] 
Sent: Tuesday, April 26, 2016 8:56 AM
To: Auld, Matthew <matthew.auld@intel.com>
Cc: intel-gfx@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>; Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
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" <zhi.a.wang@intel.com>
> 
> 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 <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> ---
>  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
diff mbox

Patch

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;
 	desc |= lrca;					   /* bits 12-31 */
 	desc |= (lrca >> PAGE_SHIFT) << GEN8_CTX_ID_SHIFT; /* bits 32-51 */