Re: [PATCH 2/4] drm/i915: Improve dynamic management/eviction of lrc backing objects

[Nick Hoath <nicholas.hoath@intel.com>]

On 08/10/2015 14:35, Chris Wilson wrote:
> On Wed, Oct 07, 2015 at 06:05:46PM +0200, Daniel Vetter wrote:
>> On Tue, Oct 06, 2015 at 03:52:02PM +0100, Nick Hoath wrote:
>>> Shovel all context related objects through the active queue and obj
>>> management.
>>>
>>> - Added callback in vma_(un)bind to add CPU (un)mapping at same time
>>>    if desired
>>> - Inserted LRC hw context & ringbuf to vma active list
>>>
>>> Issue: VIZ-4277
>>> Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h         |  4 ++
>>>   drivers/gpu/drm/i915/i915_gem.c         |  3 ++
>>>   drivers/gpu/drm/i915/i915_gem_gtt.c     |  8 ++++
>>>   drivers/gpu/drm/i915/intel_lrc.c        | 28 +++++++++++--
>>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 71 ++++++++++++++++++---------------
>>>   drivers/gpu/drm/i915/intel_ringbuffer.h |  3 --
>>>   6 files changed, 79 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 3d217f9..d660ee3 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2169,6 +2169,10 @@ struct drm_i915_gem_object {
>>>   			struct work_struct *work;
>>>   		} userptr;
>>>   	};
>>> +
>>> +	/** Support for automatic CPU side mapping of object */
>>> +	int (*mmap)(struct drm_i915_gem_object *obj, bool unmap);
>>
>> I don't think we need a map hook, that can still be done (if not done so
I disagree - this keeps the interface symmetrical. Searching for the 
do/undo code paths and finding they are in difference places, called via 
different routes makes code harder to follow.
>> already) by the callers. Also it's better to rename this to vma_unbind
>> (and it should be at the vma level I think) since there's other potential
Nope - the obj is created first, at a point where the map/unamp function 
can be known. Moving the map/unmap to the vma would mean having a 
callback path to the object just to set up the callback path when the 
vma is created anonymously at some later point.
>> users. So explicit maping, lazy unmapping for the kmaps we need. That's
>> the same design we're using for binding objects into gpu address spaces.
>>
>> Also Chris Wilson has something similar, please align with him on the
>> precise design of this callback.
>
> We need the unbind hook because of the movement in the first patch (it
> is a separate issue, the code should work without it albeit having to
> remap the ring/context state more often). The changelog in this patch
> simply explains the i915_vma_move_to_active() additions. But to get the
> shrink accurate we do need the context unpin on retirement and to do the
> pin_count check in i915_vma_unbind() after waiting (rather than before,
> as we currently do). However, the eviction code will not inspect the
> active contexts objects yet (as it will continue to skip over the
> ggtt->pin_count on them). The way I allowed ctx objects to be evicted was
> to only keep the ctx->state pinned for the duration of the request
> construction.
>
> Note that I think it should be a vma->unbind hook not an object level
> one (it is i915_vma_unbind, without only a modicum of object level state
> being modified in that function).
> -Chris
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx