Abstracting buffer sharing mechanism from the drm drivers

Abstracting buffer sharing mechanism from the drm drivers

[Aaron Plattner]

At the suggestion of a few drm developers, I'm looking at abstracting the buffer
sharing mechanism away from the individual drm drivers and treating it as a
low-level interface that kernel subsystems use to communicate, rather than as
something drivers should be accessing directly.  This would also mean that they
wouldn't have to each implement their own set of dma_buf_ops, and the logic for
things like detecting that you're importing your own dma_buf would be written
once, in drm_prime.c and not in every driver's gem_prime_import function.

Of course, it's slightly difficult because each driver's implementation seems to
be subtly different.

 * i915 uses its own special locking function, i915_mutex_lock_interruptible.

 * nouveau and radeon pin the pages when the dma_buf is created, while i915 pins
   them at map time.

 * the vmap functions are different between i915 and radeon/nouveau, but it
   looks like all they use the dma_buf object for is to find the GEM object.

Does it make sense to try to abstract the dma_buf parts of this?  For
example, a hypothetical new drm_gem_map_dma_buf would call a hook that lets i915
do its i915_gem_wait_for_error thing, takes the lock, calls a new gem_get_pages
driver hook, does the dma_map_sg call, and handles the unlocking?  I'll come up
with a more detailed proposal or patches if this sounds like a good idea.

--
Aaron

Re: Abstracting buffer sharing mechanism from the drm drivers

[Jerome Glisse]

On Fri, Nov 16, 2012 at 12:05:40PM -0800, Aaron Plattner wrote:
> At the suggestion of a few drm developers, I'm looking at abstracting the buffer
> sharing mechanism away from the individual drm drivers and treating it as a
> low-level interface that kernel subsystems use to communicate, rather than as
> something drivers should be accessing directly.  This would also mean that they
> wouldn't have to each implement their own set of dma_buf_ops, and the logic for
> things like detecting that you're importing your own dma_buf would be written
> once, in drm_prime.c and not in every driver's gem_prime_import function.
> 
> Of course, it's slightly difficult because each driver's implementation seems to
> be subtly different.
> 
>  * i915 uses its own special locking function, i915_mutex_lock_interruptible.
> 
>  * nouveau and radeon pin the pages when the dma_buf is created, while i915 pins
>    them at map time.
> 
>  * the vmap functions are different between i915 and radeon/nouveau, but it
>    looks like all they use the dma_buf object for is to find the GEM object.
> 
> Does it make sense to try to abstract the dma_buf parts of this?  For
> example, a hypothetical new drm_gem_map_dma_buf would call a hook that lets i915
> do its i915_gem_wait_for_error thing, takes the lock, calls a new gem_get_pages
> driver hook, does the dma_map_sg call, and handles the unlocking?  I'll come up
> with a more detailed proposal or patches if this sounds like a good idea.
> 
> --
> Aaron

I don't think it's a good idea, i understand why people like sharing code, but we
many example that shows that at one point trying to come with a common
infrastructure just hurt you. There will always be little tweak and little things
that a specific driver of a specific gpu will want to do at one point. For instance
you can look at intel mode display code rework, it turn out that the crtc helper
is getting in there way and i have too similar code in wip for radeon.

When it comes to dma memory sharing i think the right level of helper are the one
that allow to retrive array of page, to pin/unpin those page, and possibly something
that can map those page into a vma at a given offset but wouldn't do any
synchronization. ie nothing that impose a common logic on how things happen and when
are the point of synchronization.

Yes it means there is some code duplication but i am always in the side that trying
to over factorize code hurt more than code duplication.

Cheers,
Jerome

Re: Abstracting buffer sharing mechanism from the drm drivers

[Aaron Plattner]

On 11/16/2012 01:14 PM, Jerome Glisse wrote:
> On Fri, Nov 16, 2012 at 12:05:40PM -0800, Aaron Plattner wrote:
>> At the suggestion of a few drm developers, I'm looking at abstracting the buffer
>> sharing mechanism away from the individual drm drivers and treating it as a
>> low-level interface that kernel subsystems use to communicate, rather than as
>> something drivers should be accessing directly.  This would also mean that they
>> wouldn't have to each implement their own set of dma_buf_ops, and the logic for
>> things like detecting that you're importing your own dma_buf would be written
>> once, in drm_prime.c and not in every driver's gem_prime_import function.
>>
>> Of course, it's slightly difficult because each driver's implementation seems to
>> be subtly different.
>>
>>   * i915 uses its own special locking function, i915_mutex_lock_interruptible.
>>
>>   * nouveau and radeon pin the pages when the dma_buf is created, while i915 pins
>>     them at map time.
>>
>>   * the vmap functions are different between i915 and radeon/nouveau, but it
>>     looks like all they use the dma_buf object for is to find the GEM object.
>>
>> Does it make sense to try to abstract the dma_buf parts of this?  For
>> example, a hypothetical new drm_gem_map_dma_buf would call a hook that lets i915
>> do its i915_gem_wait_for_error thing, takes the lock, calls a new gem_get_pages
>> driver hook, does the dma_map_sg call, and handles the unlocking?  I'll come up
>> with a more detailed proposal or patches if this sounds like a good idea.
>
> I don't think it's a good idea, i understand why people like sharing code, but we
> many example that shows that at one point trying to come with a common
> infrastructure just hurt you. There will always be little tweak and little things
> that a specific driver of a specific gpu will want to do at one point. For instance
> you can look at intel mode display code rework, it turn out that the crtc helper
> is getting in there way and i have too similar code in wip for radeon.

I think the patch "drm/prime: drop reference on imported dma-buf come from gem" 
[1] is a perfect example of why I think this code should be abstracted.  It 
applies an identical change to each of five identical chunks of code.  It's 
really easy to mess up merges of changes like this with later copying and 
pasting, e.g. the introduction of a new driver like tegradrm.

> When it comes to dma memory sharing i think the right level of helper are the one
> that allow to retrive array of page, to pin/unpin those page, and possibly something
> that can map those page into a vma at a given offset but wouldn't do any
> synchronization. ie nothing that impose a common logic on how things happen and when
> are the point of synchronization.
>
> Yes it means there is some code duplication but i am always in the side that trying
> to over factorize code hurt more than code duplication.

The main issue here is that drm drivers have to call dma_buf functions directly 
when dma_buf should be a drm_prime implementation detail.

-- Aaron

[1] http://lists.freedesktop.org/archives/dri-devel/2012-November/030417.html