Re: [PATCH 0/3] embed drm_gem_object into radeon_bo

From: Thomas Hellstrom <thomas@shipmail.org>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 0/3] embed drm_gem_object into radeon_bo
Date: Mon, 15 Nov 2010 21:48:06 +0100	[thread overview]
Message-ID: <4CE19C86.4010509@shipmail.org> (raw)
In-Reply-To: <20101115184555.GA3484@viiv.ffwll.ch>

On 11/15/2010 07:45 PM, Daniel Vetter wrote:
> Hi Thomas,
>
> Thanks for your comments about ttm and vmwgfx. Some of my own ideas about
> where this might all be heading below.
>
> On Mon, Nov 15, 2010 at 08:25:16AM +0100, Thomas Hellstrom wrote:
>    
>> Hi, Daniel,
>>
>> My main concerns previously for embedding GEM objects as user-space
>> references for TTM has been twofold and implementation specific.
>>
>> 1) The locking has been using global mutexes where local spin- or
>> RCU locks have been more appropriate. It looks like this has finally
>> been / is finally going to be addressed.
>>      
> gem object lookup / insert has always (iirc) been protected by a spinlock.
> drm/i915 is just a bit inefficient with lookups, hence this spinlock
> showing up in profiling.
>
>    
>> 2) The gem object is too specialized for general purpose use:
>> a) The shmem member has no natural use with TTM except possibly as a
>> persistent swap storage, but in an ideal world, TTM would talk to
>> the swap cache directly so in the longer term there is no need for
>> the shmem object at all.
>>      
> Chris Wilson is working on a gem_vm for i915 that employs a address_space
> per bo and directly manages swap entries and has its own page allocator
> (actualy cflushed page cache). I haven't yet looked into this closely
> (especially the ttm part), but this might (at least in parts) be shareable
> between i915 and ttm.  At least it gets rid of the shmfs inode in struct
> drm_gem_object!
>
>    
>> b) Implementations may want to use other user-space visible objects
>> than buffer objects:
>> For example fence objects or CPU synchronization objects. The common
>> traits of / operations on these are user-space visibility,
>> inter-process visibility, refcounting and destruction when the
>> relevant file is closed.
>>
>> Hence a class that provides only the user-space handles, naming
>> (flink), refcounting and registering with a file handle would be the
>> best choice of a "base" class. Traditional Gem objects could derive
>> from those and provide any extra *generic* members needed for buffer
>> objects.
>>
>> This doesn't really affect your work though. Just some comments on
>> why vmwgfx don't use GEM objects by default and how they could be
>> made optimal for TTM-aware drivers.
>>      
> Yeah, I've noticed that vmwgfx is the (sometimes) sole user of many of the
> more fancy stuff in ttm. And I also don't like the duplication between gem
> and ttm. My plan (i.e. wishful thinking) is to have a common set of helper
> functions that can be shared between i915, radeon and nouveau and any
> other driver with a gem-like interface (i.e. buffer-objects + execbuffer,
> gpu<->cpu sync abstracted away by the kernel). Leaving the cracy stuff for
> drivers that need it (vmwgfx). Nothing concrete though (besides a new
> testing rig to start hacking on nouveau), I'll intend to simply plow
> along, hunting for common patterns.
>    

Sounds good. I actually think the only thing which is currently 
vmwgfx-specific are the ttm_objects and the ttm_locks. TTM objects could 
really be replaced by gem objects with the bo-specific part stripped, as 
discussed above. And then there is the ttm lock to allow concurrent 
execbufs running.

/Thomas

>    
>> Thanks,
>> /Thomas
>>      
> Yours, Daniel
>    