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

[Nick Hoath]

On 19/10/2015 10:48, Daniel Vetter wrote:
> On Fri, Oct 16, 2015 at 03:42:53PM +0100, Nick Hoath wrote:
>> 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 at 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.
>
> One of the plans for this is to also use it for to-be-unpinned
> framebuffers (4k buffers are huge ...). And in that case the unmap hook
> only, and on the vma is the design we want. And since it also seems to
> accomodate all the other users I do think it's the right choice.

I refer you to these words found on the mail list. The may be familiar:

As a rule of thumb for refactoring and share infastructure we use the 
following recipe in drm:
- first driver implements things as straightforward as possible
- 2nd user copypastes
- 3rd one has the duty to figure out whether some refactoring is in order
   or not.

The code as I have written it works best and simplest for my use case. 
If someone else wants to refactor it differently to shoe horn in their 
use case, that's up to them.


>
> Like I said, explicit setup and lazy, implicit cleanup is kinda how a lot
> of things in gem work.

The most dangerous phrase in the language is ‘we’ve always done it this 
way.’ - Grace Hopper

> -Daniel
>
>>>> 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
>>>
>>
>