Re: [PATCH 2/5] drm/i915: Do not access stolen memory directly by the CPU, even for error capture

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Mika Kuoppala <mika.kuoppala@linux.intel.com> writes:

> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
>> For stolen pages, since it is verboten to access them directly on many
>> architectures, we have to read them through the GTT aperture. If they
>> are not accessible through the aperture, then we have to abort.
>>
>> This was complicated by
>>
>> commit 8b6124a633d8095b0c8364f585edff9c59568a96
>> Author: Chris Wilson <chris@chris-wilson.co.uk>
>> Date:   Thu Jan 30 14:38:16 2014 +0000
>>
>>     drm/i915: Don't access snooped pages through the GTT (even for error capture)
>>
>> and the desire to use stolen memory for ringbuffers, contexts and
>> batches in the future.
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>  drivers/gpu/drm/i915/i915_gpu_error.c | 50 ++++++++++++++++++++++-------------
>>  1 file changed, 31 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
>> index 35e70d5..6d280c07 100644
>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>> @@ -561,10 +561,11 @@ static struct drm_i915_error_object *
>>  i915_error_object_create_sized(struct drm_i915_private *dev_priv,
>>  			       struct drm_i915_gem_object *src,
>>  			       struct i915_address_space *vm,
>> -			       const int num_pages)
>> +			       int num_pages)
>>  {
>>  	struct drm_i915_error_object *dst;
>> -	int i;
>> +	bool use_ggtt;
>> +	int i = 0;
>>  	u32 reloc_offset;
>>  
>>  	if (src == NULL || src->pages == NULL)
>> @@ -574,8 +575,32 @@ i915_error_object_create_sized(struct drm_i915_private *dev_priv,
>>  	if (dst == NULL)
>>  		return NULL;
>>  
>> -	reloc_offset = dst->gtt_offset = i915_gem_obj_offset(src, vm);
>> -	for (i = 0; i < num_pages; i++) {
>> +	dst->gtt_offset = i915_gem_obj_offset(src, vm);
>> +
>> +	reloc_offset = dst->gtt_offset;
>> +	use_ggtt = (src->cache_level == I915_CACHE_NONE &&
>                      
> Take this cache level check out so that we end up doing the
> snoopable check for non stolen objects too?

This _is_ the snoopable check for non stolen objects.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>


> -Mika
>
>> +		    i915_is_ggtt(vm) &&
>> +		    src->has_global_gtt_mapping &&
>> +		    reloc_offset + num_pages * PAGE_SIZE <= dev_priv->gtt.mappable_end);
>> +
>> +	/* Cannot access stolen address directly, try to use the aperture */
>> +	if (src->stolen) {
>> +		use_ggtt = true;
>> +
>> +		if (!src->has_global_gtt_mapping)
>> +			goto unwind;
>> +
>> +		reloc_offset = i915_gem_obj_ggtt_offset(src);
>> +		if (reloc_offset + num_pages * PAGE_SIZE > dev_priv->gtt.mappable_end)
>> +			goto unwind;
>> +	}
>> +
>> +	/* Cannot access snooped pages through the aperture */
>> +	if (use_ggtt && src->cache_level != I915_CACHE_NONE && !HAS_LLC(dev_priv->dev))
>> +		goto unwind;
>> +
>> +	dst->page_count = num_pages;
>> +	while (num_pages--) {
>>  		unsigned long flags;
>>  		void *d;
>>  
>> @@ -584,10 +609,7 @@ i915_error_object_create_sized(struct drm_i915_private *dev_priv,
>>  			goto unwind;
>>  
>>  		local_irq_save(flags);
>> -		if (src->cache_level == I915_CACHE_NONE &&
>> -		    reloc_offset < dev_priv->gtt.mappable_end &&
>> -		    src->has_global_gtt_mapping &&
>> -		    i915_is_ggtt(vm)) {
>> +		if (use_ggtt) {
>>  			void __iomem *s;
>>  
>>  			/* Simply ignore tiling or any overlapping fence.
>> @@ -599,14 +621,6 @@ i915_error_object_create_sized(struct drm_i915_private *dev_priv,
>>  						     reloc_offset);
>>  			memcpy_fromio(d, s, PAGE_SIZE);
>>  			io_mapping_unmap_atomic(s);
>> -		} else if (src->stolen) {
>> -			unsigned long offset;
>> -
>> -			offset = dev_priv->mm.stolen_base;
>> -			offset += src->stolen->start;
>> -			offset += i << PAGE_SHIFT;
>> -
>> -			memcpy_fromio(d, (void __iomem *) offset, PAGE_SIZE);
>>  		} else {
>>  			struct page *page;
>>  			void *s;
>> @@ -623,11 +637,9 @@ i915_error_object_create_sized(struct drm_i915_private *dev_priv,
>>  		}
>>  		local_irq_restore(flags);
>>  
>> -		dst->pages[i] = d;
>> -
>> +		dst->pages[i++] = d;
>>  		reloc_offset += PAGE_SIZE;
>>  	}
>> -	dst->page_count = num_pages;
>>  
>>  	return dst;
>>  
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx