[PATCH 3/4] drm/amdgpu: add independent DMA-buf export v3

Christian König ckoenig.leichtzumerken at gmail.com
Tue Jul 3 11:35:25 UTC 2018


Am 28.06.2018 um 11:58 schrieb Zhang, Jerry (Junwei):
> On 06/22/2018 10:11 PM, Christian König wrote:
>> The caching of SGT's done by the DRM code is actually quite harmful and
>> should probably removed altogether in the long term.
>>
>> Start by providing a separate DMA-buf export implementation in 
>> amdgpu. This is
>> also a prerequisite of unpinned DMA-buf handling.
>>
>> v2: fix unintended recursion, remove debugging leftovers
>> v3: split out from unpinned DMA-buf work
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h       |  1 -
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  1 -
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 94 
>> +++++++++++++------------------
>>   3 files changed, 39 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index d8e0cc08f9db..5e71af8dd3a7 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -373,7 +373,6 @@ int amdgpu_gem_object_open(struct drm_gem_object 
>> *obj,
>>   void amdgpu_gem_object_close(struct drm_gem_object *obj,
>>                   struct drm_file *file_priv);
>>   unsigned long amdgpu_gem_timeout(uint64_t timeout_ns);
>> -struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object 
>> *obj);
>>   struct drm_gem_object *
>>   amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
>>                    struct dma_buf_attachment *attach,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index a549483032b0..cdf0be85d361 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -919,7 +919,6 @@ static struct drm_driver kms_driver = {
>>       .gem_prime_export = amdgpu_gem_prime_export,
>>       .gem_prime_import = amdgpu_gem_prime_import,
>>       .gem_prime_res_obj = amdgpu_gem_prime_res_obj,
>> -    .gem_prime_get_sg_table = amdgpu_gem_prime_get_sg_table,
>
> I may miss some patches or important info.
>
> Even applied the series of patch, I still could find below code in 
> drm_gem_map_dma_buf().
> In this case, it will cause NULL pointer access. Please help me out of 
> here.
> {{{
> sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
> }}}

Take a look at the change a bit further down.

>
> Jerry
>
>>       .gem_prime_import_sg_table = amdgpu_gem_prime_import_sg_table,
>>       .gem_prime_vmap = amdgpu_gem_prime_vmap,
>>       .gem_prime_vunmap = amdgpu_gem_prime_vunmap,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>> index b2286bc41aec..038a8c8488b7 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>> @@ -40,22 +40,6 @@
>>
>>   static const struct dma_buf_ops amdgpu_dmabuf_ops;
>>
>> -/**
>> - * amdgpu_gem_prime_get_sg_table - &drm_driver.gem_prime_get_sg_table
>> - * implementation
>> - * @obj: GEM buffer object
>> - *
>> - * Returns:
>> - * A scatter/gather table for the pinned pages of the buffer 
>> object's memory.
>> - */
>> -struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object 
>> *obj)
>> -{
>> -    struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>> -    int npages = bo->tbo.num_pages;
>> -
>> -    return drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages);
>> -}
>> -
>>   /**
>>    * amdgpu_gem_prime_vmap - &dma_buf_ops.vmap implementation
>>    * @obj: GEM buffer object
>> @@ -189,35 +173,29 @@ amdgpu_gem_prime_import_sg_table(struct 
>> drm_device *dev,
>>   }
>>
>>   /**
>> - * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
>> - * @dma_buf: shared DMA buffer
>> - * @target_dev: target device
>> + * amdgpu_gem_map_dma_buf - &dma_buf_ops.map_dma_buf implementation
>>    * @attach: DMA-buf attachment
>> + * @dir: DMA direction
>>    *
>>    * Makes sure that the shared DMA buffer can be accessed by the 
>> target device.
>>    * For now, simply pins it to the GTT domain, where it should be 
>> accessible by
>>    * all DMA devices.
>>    *
>>    * Returns:
>> - * 0 on success or negative error code.
>> + * sg_table filled with the DMA addresses to use or ERR_PRT with 
>> negative error
>> + * code.
>>    */
>> -static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
>> -                 struct dma_buf_attachment *attach)
>> +static struct sg_table *
>> +amdgpu_gem_map_dma_buf(struct dma_buf_attachment *attach,
>> +               enum dma_data_direction dir)
>>   {
>> +    struct dma_buf *dma_buf = attach->dmabuf;
>>       struct drm_gem_object *obj = dma_buf->priv;
>>       struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>>       struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>> +    struct sg_table *sgt;
>>       long r;
>>
>> -    r = drm_gem_map_attach(dma_buf, attach);
>> -    if (r)
>> -        return r;
>> -
>> -    r = amdgpu_bo_reserve(bo, false);
>> -    if (unlikely(r != 0))
>> -        goto error_detach;
>> -
>> -
>>       if (attach->dev->driver != adev->dev->driver) {
>>           /*
>>            * Wait for all shared fences to complete before we switch 
>> to future
>> @@ -228,54 +206,62 @@ static int amdgpu_gem_map_attach(struct dma_buf 
>> *dma_buf,
>>                               MAX_SCHEDULE_TIMEOUT);
>>           if (unlikely(r < 0)) {
>>               DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
>> -            goto error_unreserve;
>> +            return ERR_PTR(r);
>>           }
>>       }
>>
>>       /* pin buffer into GTT */
>>       r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT, NULL);
>>       if (r)
>> -        goto error_unreserve;
>> +        return ERR_PTR(r);
>> +
>> +    sgt = drm_prime_pages_to_sg(bo->tbo.ttm->pages, bo->tbo.num_pages);
>> +    if (IS_ERR(sgt))
>> +        return sgt;
>> +
>> +    if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
>> +                  DMA_ATTR_SKIP_CPU_SYNC))
>> +        goto error_free;
>>
>>       if (attach->dev->driver != adev->dev->driver)
>>           bo->prime_shared_count++;
>>
>> -error_unreserve:
>> -    amdgpu_bo_unreserve(bo);
>> +    return sgt;
>>
>> -error_detach:
>> -    if (r)
>> -        drm_gem_map_detach(dma_buf, attach);
>> -    return r;
>> +error_free:
>> +    sg_free_table(sgt);
>> +    kfree(sgt);
>> +    return ERR_PTR(-ENOMEM);
>>   }
>>
>>   /**
>> - * amdgpu_gem_map_detach - &dma_buf_ops.detach implementation
>> - * @dma_buf: shared DMA buffer
>> + * amdgpu_gem_unmap_dma_buf - &dma_buf_ops.unmap_dma_buf implementation
>>    * @attach: DMA-buf attachment
>> + * @sgt: sg_table to unmap
>> + * @dir: DMA direction
>>    *
>>    * This is called when a shared DMA buffer no longer needs to be 
>> accessible by
>>    * the other device. For now, simply unpins the buffer from GTT.
>>    */
>> -static void amdgpu_gem_map_detach(struct dma_buf *dma_buf,
>> -                  struct dma_buf_attachment *attach)
>> +static void amdgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
>> +                     struct sg_table *sgt,
>> +                     enum dma_data_direction dir)
>>   {
>> +    struct dma_buf *dma_buf = attach->dmabuf;
>>       struct drm_gem_object *obj = dma_buf->priv;
>>       struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>>       struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>> -    int ret = 0;
>> -
>> -    ret = amdgpu_bo_reserve(bo, true);
>> -    if (unlikely(ret != 0))
>> -        goto error;
>>
>>       amdgpu_bo_unpin(bo);
>> +
>>       if (attach->dev->driver != adev->dev->driver && 
>> bo->prime_shared_count)
>>           bo->prime_shared_count--;
>> -    amdgpu_bo_unreserve(bo);
>>
>> -error:
>> -    drm_gem_map_detach(dma_buf, attach);
>> +    if (sgt) {
>> +        dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
>> +        sg_free_table(sgt);
>> +        kfree(sgt);
>> +    }
>>   }
>>
>>   /**
>> @@ -333,10 +319,8 @@ static int amdgpu_gem_begin_cpu_access(struct 
>> dma_buf *dma_buf,
>>   }
>>
>>   static const struct dma_buf_ops amdgpu_dmabuf_ops = {
>> -    .attach = amdgpu_gem_map_attach,
>> -    .detach = amdgpu_gem_map_detach,
>> -    .map_dma_buf = drm_gem_map_dma_buf,
>> -    .unmap_dma_buf = drm_gem_unmap_dma_buf,

Here we are stopping to use drm_gem_map_dma_buf(), so we don't need 
amdgpu_gem_prime_get_sg_table() any more either.

Christian.

>> +    .map_dma_buf = amdgpu_gem_map_dma_buf,
>> +    .unmap_dma_buf = amdgpu_gem_unmap_dma_buf,
>>       .release = drm_gem_dmabuf_release,
>>       .begin_cpu_access = amdgpu_gem_begin_cpu_access,
>>       .map = drm_gem_dmabuf_kmap,
>>



More information about the dri-devel mailing list