[Intel-gfx] [PATCH v5 0/7] Command parser batch buffer copy

[Daniel Vetter]

On Wed, Dec 03, 2014 at 09:31:37AM +0000, Chris Wilson wrote:
> On Wed, Dec 03, 2014 at 10:17:32AM +0100, Daniel Vetter wrote:
> > On Tue, Dec 02, 2014 at 01:57:32PM -0800, Michael H. Nguyen wrote:
> > > 
> > > 
> > > On 12/02/2014 03:13 AM, Chris Wilson wrote:
> > > > On Mon, Dec 01, 2014 at 02:39:51PM -0800, Michael H. Nguyen wrote:
> > > >> Re: madvise on creation
> > > >>
> > > >> Were you referring to this?
> > > >>
> > > >> from http://lists.freedesktop.org/archives/intel-gfx/2014-November/055060.htm
> > > >>
> > > >> 	obj = i915_gem_obj_alloc();
> > > >> 	i915_gem_object_get_pages(obj);
> > > >> 	obj->madv = I915_MADV_DONTNEED;
> > > >>
> > > >> If so, I don't understand . _get is returning obj and it'll be
> > > >> needed so would expect to set 'obj->madv = I915_MADV_WILLNEED' which
> > > >> is the case now.
> > > > 
> > > > madv is only evaluated at get_pages(). Once you have the pages, you keep
> > > > them until the shrinker purges them. Hence you only need to call
> > > > get_pages() once and set obj->madv = DONTNEED afterwards, and then you
> > > > only need to check whether the obj is purged before your next reuse (you
> > > > do not need to touch madv ever again). Whilst the object is active it is
> > > > a low priority target for the shrinker. That greatly simplifies the pool
> > > > code.
> > > 
> > > I have a feeling this may make the driver less readable imo and could
> > > also require a re-write of the series. The current code may call
> > > get_pages() more than once and occurs outside of the batch_pool
> > > management fncs. Would have to re-write things to stop that.
> > > 
> > > i915_parse_cmds()
> > >   copy_batch()
> > >     i915_gem_obj_prepare_shmem_read()
> > >       i915_gem_object_get_pages()
> > > 
> > > After removing the fancy retry loop suggested by Daniel and moving to a
> > > single cache list, the implementation looks very simple imo. And,
> > > setting 'obj->madv = I915_MADV_WILLNEED;' looks right. Readability wise,
> > > you don't have to investigate further to understand the justification
> > > for that statement. Here is an RFC snippet...
> > > 
> > > i915_gem_batch_pool_get(struct i915_gem_batch_pool *pool,    
> > >                         size_t size)
> > > {  
> > >         struct drm_i915_gem_object *obj = NULL;
> > >         struct drm_i915_gem_object *tmp, *next;
> > >    
> > >         WARN_ON(!mutex_is_locked(&pool->dev->struct_mutex));
> > >    
> > >         list_for_each_entry_safe(tmp, next,
> > >                         &pool->cache_list, batch_pool_list) {
> > >    
> > >                 if (tmp->active)
> > >                         continue;
> > >           
> > >                 if (tmp->madv == __I915_MADV_PURGED) {                      
> > >                         list_del(&tmp->batch_pool_list);                    
> > >                         drm_gem_object_unreference(&tmp->base);             
> > >                         continue;
> > >                 }
> > >    
> > >                 if (tmp->base.size >= size &&
> > >                     tmp->base.size <= (2 * size)) {
> > >                         obj = tmp;
> > >                         break;
> > >                 }
> > >         }
> > > 
> > >         if (!obj) {
> > >                 obj = i915_gem_alloc_object(pool->dev, size);
> > >                 if (!obj)
> > >                         return ERR_PTR(-ENOMEM);
> > > 
> > >                 list_add_tail(&obj->batch_pool_list, &pool->cache_list);
> > >         }
> > > 
> > >         obj->madv = I915_MADV_WILLNEED;
> > > 
> > >         return obj;
> > > }
> > > 
> > > Given the spirit of your feedback was to simplify pool_get(), does this
> > > RFC do it for you? If not, I kindly request we have a sync up meeting to
> > > discuss your 'obj->madv = DONTNEED' suggestion.
> > 
> > Yeah I think this looks good. And setting DONTNEED should imo be done in
> > the _put function, which we do right away in execbuf (after having pushed
> > the shadow batch to the active list to make sure it doesn't disappear too
> > quickly).
> 
> It can't disappear as we should have the pages pinned.
> 
> > Maybe I'm misunderstanding what Chris means, but setting
> > DONTNEED right away in _get is too early and needs a lot more care in
> > users of the batch pool like you point out. It's also not how libdrm does
> > use the madv settings.
> 
> No, it is not. You need exactly the same care either way. The shrinker
> will steal WILLNEED pages if it has to... Only pinning prevents the
> shrinker from reaping the shadow_obj. And use pin_pages not ggtt_pin for
> that purpose!

We certainly need pin_pages after pool_get. But if we set PURGEABLE right
away the shrinker might throw away the backing storage before we get
around to pin it. And I don't see any benefit of doing things that way,
especially since the libdrm cache also only sets PURGEABLE after we've
filled the batch and everything (but before the batch is retired ofc).

So the sequence I see is
- pool_get (mad = WILLNEED)
- pin_pages
- uplaod scanned buffer
- unpin_pages
- throw onto active list
- pool_put (madv = PURGEABLE)
- return frome execbuf ioctl

Moving the madv = PURGEABLE up makes the pool internal resizing logic
needlessly leaky and I don't see any benefit. We need to call pool_put
anyway before returning from the ioctl. So any other thread _never_
observes the madv = WILLNEED state, it's only the shrinker which might
cause havoc.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch