diff mbox

[04/13] drm/i915: Fail engine initialization if LRCA is incorrectly aligned

Message ID 1452252592-24803-5-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin Jan. 8, 2016, 11:29 a.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

LRCA can change only when it goes from unpinned to pinned so it
makes sense to check its alignment at that point rather than at
every batch buffer submission.

Furthermore, if we check it at pin time we can actually
gracefuly fail the engine initialization rather than just
spamming the logs at runtime with WARNs.

v2: Return ENODEV for bad alignment. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Daniel Vetter Jan. 11, 2016, 8:31 a.m. UTC | #1 
On Fri, Jan 08, 2016 at 11:29:43AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> LRCA can change only when it goes from unpinned to pinned so it
> makes sense to check its alignment at that point rather than at
> every batch buffer submission.
> 
> Furthermore, if we check it at pin time we can actually
> gracefuly fail the engine initialization rather than just
> spamming the logs at runtime with WARNs.
> 
> v2: Return ENODEV for bad alignment. (Chris Wilson)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 84977a6e6f3f..ff146a15d395 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -302,8 +302,6 @@ uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
>  	uint64_t lrca = i915_gem_obj_ggtt_offset(ctx_obj) +
>  			LRC_PPHWSP_PN * PAGE_SIZE;
>  
> -	WARN_ON(lrca & 0xFFFFFFFF00000FFFULL);
> -
>  	desc |= lrca;
>  	desc |= (u64)intel_execlists_ctx_id(ctx_obj) << GEN8_CTX_ID_SHIFT;
>  
> @@ -1030,6 +1028,7 @@ static int intel_lr_context_do_pin(struct intel_engine_cs *ring,
>  {
>  	struct drm_device *dev = ring->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u64 lrca;
>  	int ret = 0;
>  
>  	WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
> @@ -1038,6 +1037,12 @@ static int intel_lr_context_do_pin(struct intel_engine_cs *ring,
>  	if (ret)
>  		return ret;
>  
> +	lrca = i915_gem_obj_ggtt_offset(ctx_obj) + LRC_PPHWSP_PN * PAGE_SIZE;
> +	if (WARN_ON(lrca & 0xFFFFFFFF00000FFFULL)) {

Essentially this checks that it's page-aligned (which is a fundamental
assumption of how we place objects we depend upon everywhere) and that it
fits within the 4G hw limit of the global gtt (again we assume our code is
correct that way). tbh I'd just drop entirely, it's a useless check.
-Daniel

> +		ret = -ENODEV;
> +		goto unpin_ctx_obj;
> +	}
> +
>  	ret = intel_pin_and_map_ringbuffer_obj(ring->dev, ringbuf);
>  	if (ret)
>  		goto unpin_ctx_obj;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Dave Gordon Jan. 11, 2016, 4:02 p.m. UTC | #2 
On 11/01/16 08:31, Daniel Vetter wrote:
> On Fri, Jan 08, 2016 at 11:29:43AM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> LRCA can change only when it goes from unpinned to pinned so it
>> makes sense to check its alignment at that point rather than at
>> every batch buffer submission.
>>
>> Furthermore, if we check it at pin time we can actually
>> gracefuly fail the engine initialization rather than just
>> spamming the logs at runtime with WARNs.
>>
>> v2: Return ENODEV for bad alignment. (Chris Wilson)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_lrc.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 84977a6e6f3f..ff146a15d395 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -302,8 +302,6 @@ uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
>>   	uint64_t lrca = i915_gem_obj_ggtt_offset(ctx_obj) +
>>   			LRC_PPHWSP_PN * PAGE_SIZE;
>>
>> -	WARN_ON(lrca & 0xFFFFFFFF00000FFFULL);
>> -
>>   	desc |= lrca;
>>   	desc |= (u64)intel_execlists_ctx_id(ctx_obj) << GEN8_CTX_ID_SHIFT;
>>
>> @@ -1030,6 +1028,7 @@ static int intel_lr_context_do_pin(struct intel_engine_cs *ring,
>>   {
>>   	struct drm_device *dev = ring->dev;
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	u64 lrca;
>>   	int ret = 0;
>>
>>   	WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
>> @@ -1038,6 +1037,12 @@ static int intel_lr_context_do_pin(struct intel_engine_cs *ring,
>>   	if (ret)
>>   		return ret;
>>
>> +	lrca = i915_gem_obj_ggtt_offset(ctx_obj) + LRC_PPHWSP_PN * PAGE_SIZE;
>> +	if (WARN_ON(lrca & 0xFFFFFFFF00000FFFULL)) {
>
> Essentially this checks that it's page-aligned (which is a fundamental
> assumption of how we place objects we depend upon everywhere) and that it
> fits within the 4G hw limit of the global gtt (again we assume our code is
> correct that way). tbh I'd just drop entirely, it's a useless check.
> -Daniel

IIRC the original version of this WARN (in intel_lr_context_descriptor()
above) was added with the GuC submission code, because the context 
descriptor as used in execlist code is a 64-bit value, but the GuC 
requires that all the unique stuff fits in those 20 unmasked bits of a 
32-bit value, with the low 12 bits being used for flags. So we wanted to 
check that we never got a context ID that couldn't be pruned down to 
just those 20 bits without losing information. It's never been seen to 
happen since GuC development finished, so we can reasonably lose the 
check now.

Unless anyone wants to be prepared for the expansion of the GTT beyond 4Gb!

.Dave.
Chris Wilson Jan. 11, 2016, 11:31 p.m. UTC | #3 
On Mon, Jan 11, 2016 at 04:02:09PM +0000, Dave Gordon wrote:
> IIRC the original version of this WARN (in intel_lr_context_descriptor()
> above) was added with the GuC submission code, because the context
> descriptor as used in execlist code is a 64-bit value, but the GuC
> requires that all the unique stuff fits in those 20 unmasked bits of
> a 32-bit value, with the low 12 bits being used for flags. So we
> wanted to check that we never got a context ID that couldn't be
> pruned down to just those 20 bits without losing information. It's
> never been seen to happen since GuC development finished, so we can
> reasonably lose the check now.

I am missing something here as the GuC doesn't use the high 32bits of
the context descriptor, i.e. it never touches the lrca portion?
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 84977a6e6f3f..ff146a15d395 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -302,8 +302,6 @@  uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
 	uint64_t lrca = i915_gem_obj_ggtt_offset(ctx_obj) +
 			LRC_PPHWSP_PN * PAGE_SIZE;
 
-	WARN_ON(lrca & 0xFFFFFFFF00000FFFULL);
-
 	desc |= lrca;
 	desc |= (u64)intel_execlists_ctx_id(ctx_obj) << GEN8_CTX_ID_SHIFT;
 
@@ -1030,6 +1028,7 @@  static int intel_lr_context_do_pin(struct intel_engine_cs *ring,
 {
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	u64 lrca;
 	int ret = 0;
 
 	WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
@@ -1038,6 +1037,12 @@  static int intel_lr_context_do_pin(struct intel_engine_cs *ring,
 	if (ret)
 		return ret;
 
+	lrca = i915_gem_obj_ggtt_offset(ctx_obj) + LRC_PPHWSP_PN * PAGE_SIZE;
+	if (WARN_ON(lrca & 0xFFFFFFFF00000FFFULL)) {
+		ret = -ENODEV;
+		goto unpin_ctx_obj;
+	}
+
 	ret = intel_pin_and_map_ringbuffer_obj(ring->dev, ringbuf);
 	if (ret)
 		goto unpin_ctx_obj;