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

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx