Re: [PATCH] drm/i915: Infrastructure for supporting different GGTT views per object

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 11/28/2014 05:31 PM, Daniel Vetter wrote:
> On Thu, Nov 27, 2014 at 02:52:44PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Things like reliable GGTT mappings and mirrored 2d-on-3d display will need
>> to map objects into the same address space multiple times.
>>
>> Added a GGTT view concept and linked it with the VMA to distinguish between
>> multiple instances per address space.
>>
>> New objects and GEM functions which do not take this new view as a parameter
>> assume the default of zero (I915_GGTT_VIEW_NORMAL) which preserves the
>> previous behaviour.
>>
>> This now means that objects can have multiple VMA entries so the code which
>> assumed there will only be one also had to be modified.
>>
>> Alternative GGTT views are supposed to borrow DMA addresses from obj->pages
>> which is DMA mapped on first VMA instantiation and unmapped on the last one
>> going away.
>>
>> v2:
>>      * Removed per view special casing in i915_gem_ggtt_prepare /
>>        finish_object in favour of creating and destroying DMA mappings
>>        on first VMA instantiation and last VMA destruction. (Daniel Vetter)
>>      * Simplified i915_vma_unbind which does not need to count the GGTT views.
>>        (Daniel Vetter)
>>      * Also moved obj->map_and_fenceable reset under the same check.
>>      * Checkpatch cleanups.
>>
>> Issue: VIZ-4544
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> lgtm overall. Please find someone for detailed review for knowledge
> sharing (so different geo/team).

With the current state of who is doing what and where it made most sense 
for Michel to do this.

> A few comemnts and questions below still. Also could you please do a
> follow-up patch which adds a DOC: section with a short exlanation of gtt
> views and pulls it into the i915 docbook template? We need to start
> somewhere with gem docs ...

Sure, I'll find some previous patches from this area to see how roughly 
it should look like.

> Cheers, Daniel
>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c        |  5 +-
>>   drivers/gpu/drm/i915/i915_drv.h            | 46 +++++++++++++++++--
>>   drivers/gpu/drm/i915/i915_gem.c            | 73 ++++++++++++++++--------------
>>   drivers/gpu/drm/i915/i915_gem_context.c    |  4 +-
>>   drivers/gpu/drm/i915/i915_gem_execbuffer.c |  4 +-
>>   drivers/gpu/drm/i915/i915_gem_gtt.c        | 61 +++++++++++++++++++------
>>   drivers/gpu/drm/i915/i915_gem_gtt.h        | 22 ++++++++-
>>   drivers/gpu/drm/i915/i915_gpu_error.c      |  8 +---
>>   8 files changed, 159 insertions(+), 64 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 319da61..1a9569f 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -154,8 +154,9 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>>   			seq_puts(m, " (pp");
>>   		else
>>   			seq_puts(m, " (g");
>> -		seq_printf(m, "gtt offset: %08lx, size: %08lx)",
>> -			   vma->node.start, vma->node.size);
>> +		seq_printf(m, "gtt offset: %08lx, size: %08lx, type: %u)",
>> +			   vma->node.start, vma->node.size,
>> +			   vma->ggtt_view.type);
>>   	}
>>   	if (obj->stolen)
>>   		seq_printf(m, " (stolen: %08lx)", obj->stolen->start);
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index c4f2cb6..6250a2c 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2501,10 +2501,23 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
>>   #define PIN_GLOBAL 0x4
>>   #define PIN_OFFSET_BIAS 0x8
>>   #define PIN_OFFSET_MASK (~4095)
>> +int __must_check i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
>> +					  struct i915_address_space *vm,
>> +					  uint32_t alignment,
>> +					  uint64_t flags,
>> +					  const struct i915_ggtt_view *view);
>> +static inline
>>   int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
>>   				     struct i915_address_space *vm,
>>   				     uint32_t alignment,
>> -				     uint64_t flags);
>> +				     uint64_t flags)
>> +{
>> +	return i915_gem_object_pin_view(obj, vm, alignment, flags,
>> +						&i915_ggtt_view_normal);
>> +}
>> +
>> +void i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>> +		   u32 flags);
>>   int __must_check i915_vma_unbind(struct i915_vma *vma);
>>   int i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
>>   void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv);
>> @@ -2656,18 +2669,43 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
>>
>>   void i915_gem_restore_fences(struct drm_device *dev);
>>
>> +unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
>> +				       struct i915_address_space *vm,
>> +				       enum i915_ggtt_view_type view);
>> +static inline
>>   unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
>> -				  struct i915_address_space *vm);
>> +				  struct i915_address_space *vm)
>> +{
>> +	return i915_gem_obj_offset_view(o, vm, I915_GGTT_VIEW_NORMAL);
>> +}
>>   bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o);
>>   bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
>>   			struct i915_address_space *vm);
>>   unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
>>   				struct i915_address_space *vm);
>> +struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
>> +					  struct i915_address_space *vm,
>> +					  const struct i915_ggtt_view *view);
>> +static inline
>>   struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
>> -				     struct i915_address_space *vm);
>> +				     struct i915_address_space *vm)
>> +{
>> +	return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal);
>> +}
>> +
>> +struct i915_vma *
>> +i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
>> +				       struct i915_address_space *vm,
>> +				       const struct i915_ggtt_view *view);
>> +
>> +static inline
>>   struct i915_vma *
>>   i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
>> -				  struct i915_address_space *vm);
>> +				  struct i915_address_space *vm)
>> +{
>> +	return i915_gem_obj_lookup_or_create_vma_view(obj, vm,
>> +						&i915_ggtt_view_normal);
>> +}
>>
>>   struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
>>   static inline bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) {
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 86cf428..6213c07 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2090,8 +2090,7 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
>>   			/* For the unbound phase, this should be a no-op! */
>>   			list_for_each_entry_safe(vma, v,
>>   						 &obj->vma_list, vma_link)
>> -				if (i915_vma_unbind(vma))
>> -					break;
>> +				i915_vma_unbind(vma);
>
> Why drop the early break if a vma_unbind fails? Looks like a superflous
> hunk to me.

I wasn't sure about this. (Does it makes sense to try and unbind other 
VMAs if one couldn't be unbound?)

In fact, looking at it now, I am not sure about the unbind flow 
(i915_vma_unbind). Won't i915_gem_object_retire move all VMAs to 
inactive list on first VMA unbind? Retire only on last VMA going away?

>> @@ -5430,9 +5434,12 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
>>   {
>>   	struct i915_vma *vma;
>>
>> -	vma = list_first_entry(&obj->vma_list, typeof(*vma), vma_link);
>> -	if (vma->vm != i915_obj_to_ggtt(obj))
>> -		return NULL;
>> +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>> +		if (vma->vm != i915_obj_to_ggtt(obj))
>> +			continue;
>> +		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
>> +			return vma;
>> +	}
>
> We fairly put the ggtt vma into the head of the list. Imo better to keep
> the head slot reserved for the ggtt normal view, might be some random code
> relying upon this.

Ok.

>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
>> index 89a2f3d..77f1bdc 100644
>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>> @@ -717,10 +717,8 @@ static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
>>   			break;
>>
>>   		list_for_each_entry(vma, &obj->vma_list, vma_link)
>> -			if (vma->vm == vm && vma->pin_count > 0) {
>> +			if (vma->vm == vm && vma->pin_count > 0)
>>   				capture_bo(err++, vma);
>> -				break;
>
> Not fully sure about this one, but can't hurt I guess.

Not sure if it is useful at the moment or at all?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx