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

[Michael H. Nguyen]

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.

Thank you much,
-Mike

> -Chris
>