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

[Aaron Plattner <aplattner@nvidia.com>]

On 01/16/2013 01:50 AM, Daniel Vetter wrote:
> On Tue, Jan 15, 2013 at 12:47:42PM -0800, 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>
>
> Two minor things, but since you're not touching drm/i915 I don't care that
> much ...
>
>> +static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
>> +             enum dma_data_direction dir)
>> +{
>> +     struct drm_gem_object *obj = attach->dmabuf->priv;
>> +     struct sg_table *sgt;
>> +
>> +     mutex_lock(&obj->dev->struct_mutex);
>> +
>> +     sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
>> +
>> +     if (!IS_ERR_OR_NULL(sgt))
>> +             dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);
>> +
>> +     mutex_unlock(&obj->dev->struct_mutex);
>
> The locking here looks superflous and not required to protect anything
> drm_gem_map_dma_buf does. If drivers need this (and ttm based ones
> shouldn't really), they can grab it in their ->gem_prime_get_sg_table
> callback. So I think a follow-up patch to ditch this would be good.

If you look at the later patches, this was just moved from the drivers. 
  I agree that evaluating whether or not it's actually necessary should 
be a separate follow-up.

>> +     return sgt;
>> +}
>> +
>> +static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
>> +             struct sg_table *sgt, enum dma_data_direction dir)
>> +{
>> +     dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
>> +     sg_free_table(sgt);
>> +     kfree(sgt);
>> +}
>> +
>> +static void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
>> +{
>> +     struct drm_gem_object *obj = dma_buf->priv;
>> +
>> +     if (obj->export_dma_buf == dma_buf) {
>> +             /* drop the reference on the export fd holds */
>> +             obj->export_dma_buf = NULL;
>> +             drm_gem_object_unreference_unlocked(obj);
>> +     }
>> +}
>> +
>> +static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)
>> +{
>> +     struct drm_gem_object *obj = dma_buf->priv;
>> +     struct drm_device *dev = obj->dev;
>> +
>> +     return dev->driver->gem_prime_vmap(obj);
>> +}
>> +
>> +static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
>> +{
>> +     struct drm_gem_object *obj = dma_buf->priv;
>> +     struct drm_device *dev = obj->dev;
>> +
>> +     dev->driver->gem_prime_vunmap(obj, vaddr);
>> +}
>
> Imo these two don't add value, simpler to add a typechecking
> drm_dmabuf_to_gem_obj static inline somewhere. But then you'd need to pass
> in the dmabuf fops struct from drivers and export the helpers. More
> flexible, but also a bit work to redo. Like I've said, I don't care too
> much ...

I considered that, but it seems like the wrong abstraction.  The 
advantage here is that drivers don't have to care even a little bit 
about the buffer sharing mechanism, which simplifies the drivers.

I.e., from patch #2,

diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c 
b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 8bf695c..24e0aab 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -24,8 +24,6 @@
   *
   */

-#include <linux/dma-buf.h>

Can I consider this a Reviewed-by?

-- 
Aaron