Re: [PATCH v3 1/3] drm: add prime helpers

[Aaron Plattner <aplattner@nvidia.com>]

On 06/18/2013 04:37 PM, Laurent Pinchart wrote:
> Hi Aaron,
>
> On Tuesday 18 June 2013 16:28:15 Aaron Plattner wrote:
>> On 06/18/2013 04:08 PM, Laurent Pinchart wrote:
>>> Hi Aaron,
>>>
>>> A bit late, but here's a small question.
>>>
>>> On Tuesday 15 January 2013 12:47:42 Aaron Plattner wrote:
>>>> Instead of reimplementing all of the dma_buf functionality in every
>>>> driver, create helpers drm_prime_import and drm_prime_export that
>>>> implement them in terms of new, lower-level hook functions:
>>>> gem_prime_pin: callback when a buffer is created, used to pin buffers
>>>> into
>>>> GTT gem_prime_get_sg_table: convert a drm_gem_object to an sg_table for
>>>> export gem_prime_import_sg_table: convert an sg_table into a
>>>> drm_gem_object
>>>> gem_prime_vmap, gem_prime_vunmap: map and unmap an object
>>>>
>>>> These hooks are optional; drivers can opt in by using
>>>> drm_gem_prime_import and drm_gem_prime_export as the .gem_prime_import
>>>> and .gem_prime_export fields of struct drm_driver.
>>>>
>>>> v2:
>>>> - Drop .begin_cpu_access.  None of the drivers this code replaces
>>>> implemented it.  Having it here was a leftover from when I was trying to
>>>> include i915 in this rework.
>>>> - Use mutex_lock instead of mutex_lock_interruptible, as these three
>>>> drivers did.  This patch series shouldn't change that behavior.
>>>> - Rename helpers to gem_prime_get_sg_table and gem_prime_import_sg_table.
>>>>
>>>>     Rename struct sg_table* variables to 'sgt' for clarity.
>>>>
>>>> - Update drm.tmpl for these new hooks.
>>>>
>>>> v3:
>>>> - Pass the vaddr down to the driver.  This lets drivers that just call
>>>> vunmap on the pointer avoid having to store the pointer in their GEM
>>>> private structures. - Move documentation into a /** DOC */ comment in
>>>> drm_prime.c and include it in drm.tmpl with a !P line.  I tried to use !F
>>>> lines to include documentation of the individual functions from drmP.h,
>>>> but
>>>> the docproc / kernel-doc scripts barf on that file, so hopefully this is
>>>> good enough for now.
>>>> - apply refcount fix from commit be8a42ae60addd8b6092535c11b42d099d6470ec
>>>>
>>>>     ("drm/prime: drop reference on imported dma-buf come from gem")
>>>>
>>>> Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Cc: David Airlie <airlied@linux.ie>
>>>> ---
>>>>
>>>>    Documentation/DocBook/drm.tmpl |   4 +
>>>>    drivers/gpu/drm/drm_prime.c    | 186
>>>>    +++++++++++++++++++++++++++++++++++++-
>>>>    include/drm/drmP.h             |  12 +++
>>>>    3 files changed, 201 insertions(+), 1 deletion(-)
>>>
>>> [snip]
>>>
>>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>>>> index 7f12573..366910d 100644
>>>> --- a/drivers/gpu/drm/drm_prime.c
>>>> +++ b/drivers/gpu/drm/drm_prime.c
>>>
>>> [snip]
>>>
>>>> +/**
>>>> + * DOC: PRIME Helpers
>>>> + *
>>>> + * Drivers can implement @gem_prime_export and @gem_prime_import in
>>>> terms
>>>> of + * simpler APIs by using the helper functions @drm_gem_prime_export
>>>> and
>>>> + * @drm_gem_prime_import.  These functions implement dma-buf support in
>>>> terms of + * five lower-level driver callbacks:
>>>> + *
>>>> + * Export callbacks:
>>>> + *
>>>> + *  - @gem_prime_pin (optional): prepare a GEM object for exporting
>>>> + *
>>>> + *  - @gem_prime_get_sg_table: provide a scatter/gather table of pinned
>>>> pages + *
>>>> + *  - @gem_prime_vmap: vmap a buffer exported by your driver
>>>> + *
>>>> + *  - @gem_prime_vunmap: vunmap a buffer exported by your driver
>>>> + *
>>>> + * Import callback:
>>>> + *
>>>> + *  - @gem_prime_import_sg_table (import): produce a GEM object from
>>>> another + *    driver's scatter/gather table
>>>> + */
>>>> +
>>>> +struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
>>>> +				     struct drm_gem_object *obj, int flags)
>>>> +{
>>>> +	if (dev->driver->gem_prime_pin) {
>>>> +		int ret = dev->driver->gem_prime_pin(obj);
>>>> +		if (ret)
>>>> +			return ERR_PTR(ret);
>>>> +	}
>>>> +	return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size,
>>>> +			      0600);
>>>
>>> Why do you use 0600 instead of the flags passed by the caller ?
>>
>> Because I copied & pasted it from i915_gem_prime_export prior to commit
>> 5b42427fc38ecb9056c4e64deaff36d6d6ba1b67 and didn't notice until you
>> pointed it out just now.
>
> That's a pretty valid reason I guess :-) Should I submit a patch or are you
> already working on one ?

:)  Sorry!

I'm not working on this right now, but I can put it on my queue if you 
like.  You'll probably be able to fix and test it sooner than I will, 
though.

>>>> +}
>>>> +EXPORT_SYMBOL(drm_gem_prime_export);

-- 
Aaron