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

[Daniel Vetter]

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). 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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch