diff mbox

[12/43] drm/i915/bdw: Don't write PDP in the legacy way when using LRCs

Message ID 1407413860-5926-1-git-send-email-thomas.daniel@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Thomas Daniel Aug. 7, 2014, 12:17 p.m. UTC
From: Oscar Mateo <oscar.mateo@intel.com>

This is mostly for correctness so that we know we are running the LR
context correctly (this is, the PDPs are contained inside the context
object).

v2: Move the check to inside the enable PPGTT function. The switch
happens in two places: the legacy context switch (that we won't hit
when Execlists are enabled) and the PPGTT enable, which unfortunately
we need. This would look much nicer if the ppgtt->enable was part of
the ring init, where it logically belongs.

v3: Move the check to the start of the enable PPGTT function.  None
of the legacy PPGTT enabling is required when using LRCs as the
PPGTT is enabled in the context descriptor and the PDPs are written
in the LRC.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
Signed-off-by: Thomas Daniel <thomas.daniel@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c |    5 +++++
 1 file changed, 5 insertions(+)

Comments

Lespiau, Damien Aug. 8, 2014, 3:59 p.m. UTC | #1
On Thu, Aug 07, 2014 at 01:17:40PM +0100, Thomas Daniel wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> This is mostly for correctness so that we know we are running the LR
> context correctly (this is, the PDPs are contained inside the context
> object).
> 
> v2: Move the check to inside the enable PPGTT function. The switch
> happens in two places: the legacy context switch (that we won't hit
> when Execlists are enabled) and the PPGTT enable, which unfortunately
> we need. This would look much nicer if the ppgtt->enable was part of
> the ring init, where it logically belongs.
> 
> v3: Move the check to the start of the enable PPGTT function.  None
> of the legacy PPGTT enabling is required when using LRCs as the
> PPGTT is enabled in the context descriptor and the PDPs are written
> in the LRC.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> Signed-off-by: Thomas Daniel <thomas.daniel@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
Daniel Vetter Aug. 11, 2014, 2:32 p.m. UTC | #2
On Thu, Aug 07, 2014 at 01:17:40PM +0100, Thomas Daniel wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> This is mostly for correctness so that we know we are running the LR
> context correctly (this is, the PDPs are contained inside the context
> object).
> 
> v2: Move the check to inside the enable PPGTT function. The switch
> happens in two places: the legacy context switch (that we won't hit
> when Execlists are enabled) and the PPGTT enable, which unfortunately
> we need. This would look much nicer if the ppgtt->enable was part of
> the ring init, where it logically belongs.
> 
> v3: Move the check to the start of the enable PPGTT function.  None
> of the legacy PPGTT enabling is required when using LRCs as the
> PPGTT is enabled in the context descriptor and the PDPs are written
> in the LRC.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> Signed-off-by: Thomas Daniel <thomas.daniel@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 5188936..cfbf272 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -843,6 +843,11 @@ static int gen8_ppgtt_enable(struct i915_hw_ppgtt *ppgtt)
>  	struct intel_engine_cs *ring;
>  	int j, ret;
>  
> +	/* In the case of Execlists, we don't want to write the PDPs

This comment is misleading since you now also disable the ppgtt enable bit
setting. Not that the code disabled in v2 of this patch is for aliasing
ppgtt mode only anyway, so irrelevant for execlist mode. Aside: A few
patches from myself will clear this up a bit better.

As-is this just looks confusing, so I'll punt for now.
-Daniel

> +	 * in the legacy way (they live inside the context now) */
> +	if (i915.enable_execlists)
> +		return 0;
> +
>  	for_each_ring(ring, dev_priv, j) {
>  		I915_WRITE(RING_MODE_GEN7(ring),
>  			   _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 5188936..cfbf272 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -843,6 +843,11 @@  static int gen8_ppgtt_enable(struct i915_hw_ppgtt *ppgtt)
 	struct intel_engine_cs *ring;
 	int j, ret;
 
+	/* In the case of Execlists, we don't want to write the PDPs
+	 * in the legacy way (they live inside the context now) */
+	if (i915.enable_execlists)
+		return 0;
+
 	for_each_ring(ring, dev_priv, j) {
 		I915_WRITE(RING_MODE_GEN7(ring),
 			   _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));