[Intel-gfx] [PATCH] drm/i915: Infrastructure for supporting different GGTT views per object
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Mon Dec 1 03:32:42 PST 2014
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 at 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 at intel.com>
>> Cc: Daniel Vetter <daniel.vetter at 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
More information about the Intel-gfx
mailing list