<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr"> Please send the drm prime patch to dri-devel if you didn't already. Alex </div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Samuel Li <samuel.li@amd.com> Sent: Wednesday, December 13, 2017 2:17:49 PM To: Koenig, Christian; amd-gfx@lists.freedesktop.org Subject: Re: FW: [PATCH v2 2/2] drm/amdgpu: Move to gtt before cpu accesses dma buf. <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">For the record. On 2017-12-13 01:26 PM, Christian König wrote: > Actually we try to avoid that drivers define their own dma_buf_ops in DRM. > > That's why you have all those callbacks in drm_driver which just mirror the dma_buf interface but unpack the GEM object from the dma-buf object. > > There are quite a number of exceptions, but those drivers then implement everything on their own because the DRM marshaling doesn't make sense for them. > > Christian. > > Am 13.12.2017 um 19:01 schrieb Samuel Li: >> That is an approach. The cost is to add a new call back, which is not necessary though, since driver can always actually define their own dma_buf_ops. >> The intention here is to allow a driver reuse drm_gem_prime_dmabuf_ops{}. If you would like to go this far, maybe a more straight forward way is to export those ops, e.g. drm_gem_map_attach, so that a driver can use them in its own definitions. >> >> Sam >> >> >> >> On 2017-12-13 05:23 AM, Christian König wrote: >>> Something like the attached patch. Not even compile tested. >>> >>> Christian. >>> >>> Am 12.12.2017 um 20:13 schrieb Samuel Li: >>>> Not sure if I understand your comments correctly. Currently amdgpu prime reuses drm_gem_prime_dmabuf_ops{}, and it is defined as static which is reasonable. I do not see an easier way to introduce amdgpu_gem_begin_cpu_access(). >>>> >>>> Sam >>>> >>>> On 2017-12-12 01:30 PM, Christian König wrote: >>>>>> + while (amdgpu_dmabuf_ops.begin_cpu_access != amdgpu_gem_begin_cpu_access) >>>>> I would rather just add the four liner code to drm to forward the begin_cpu_access callback into a drm_driver callback instead of all this. >>>>> >>>>> But apart from that it looks good to me. >>>>> >>>>> Christian. >>>>> >>>>> Am 12.12.2017 um 19:14 schrieb Li, Samuel: >>>>>> A gentle ping on this one, Christian, can you take a look at this? >>>>>> >>>>>> Sam >>>>>> >>>>>> -----Original Message----- >>>>>> From: Li, Samuel >>>>>> Sent: Friday, December 08, 2017 5:22 PM >>>>>> To: amd-gfx@lists.freedesktop.org >>>>>> Cc: Li, Samuel <Samuel.Li@amd.com> >>>>>> Subject: [PATCH v2 2/2] drm/amdgpu: Move to gtt before cpu accesses dma buf. >>>>>> >>>>>> To improve cpu read performance. This is implemented for APUs currently. >>>>>> >>>>>> v2: Adapt to change <a href="https://lists.freedesktop.org/archives/amd-gfx/2017-October/015174.html"> https://lists.freedesktop.org/archives/amd-gfx/2017-October/015174.html</a> >>>>>> >>>>>> Change-Id: I7a583e23a9ee706e0edd2a46f4e4186a609368e3 >>>>>> --- >>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 ++ >>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- >>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 58 +++++++++++++++++++++++++++++++ >>>>>> 3 files changed, 61 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>>>> index f8657c3..193db70 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>>>> @@ -417,6 +417,8 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev, struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev, >>>>>> struct drm_gem_object *gobj, >>>>>> int flags); >>>>>> +struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev, >>>>>> + struct dma_buf *dma_buf); >>>>>> int amdgpu_gem_prime_pin(struct drm_gem_object *obj); void amdgpu_gem_prime_unpin(struct drm_gem_object *obj); struct reservation_object *amdgpu_gem_prime_res_obj(struct drm_gem_object *); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >>>>>> index 31383e0..df30b08 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >>>>>> @@ -868,7 +868,7 @@ static struct drm_driver kms_driver = { >>>>>> .prime_handle_to_fd = drm_gem_prime_handle_to_fd, >>>>>> .prime_fd_to_handle = drm_gem_prime_fd_to_handle, >>>>>> .gem_prime_export = amdgpu_gem_prime_export, >>>>>> - .gem_prime_import = drm_gem_prime_import, >>>>>> + .gem_prime_import = amdgpu_gem_prime_import, >>>>>> .gem_prime_pin = amdgpu_gem_prime_pin, >>>>>> .gem_prime_unpin = amdgpu_gem_prime_unpin, >>>>>> .gem_prime_res_obj = amdgpu_gem_prime_res_obj, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c >>>>>> index ae9c106..de6f599 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c >>>>>> @@ -26,6 +26,7 @@ >>>>>> #include <drm/drmP.h> >>>>>> #include "amdgpu.h" >>>>>> +#include "amdgpu_display.h" >>>>>> #include <drm/amdgpu_drm.h> >>>>>> #include <linux/dma-buf.h> >>>>>> @@ -164,6 +165,33 @@ struct reservation_object *amdgpu_gem_prime_res_obj(struct drm_gem_object *obj) >>>>>> return bo->tbo.resv; >>>>>> } >>>>>> +static int amdgpu_gem_begin_cpu_access(struct dma_buf *dma_buf, enum >>>>>> +dma_data_direction direction) { >>>>>> + struct amdgpu_bo *bo = gem_to_amdgpu_bo(dma_buf->priv); >>>>>> + struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); >>>>>> + struct ttm_operation_ctx ctx = { true, false }; >>>>>> + u32 domain = amdgpu_framebuffer_domains(adev); >>>>>> + long ret = 0; >>>>>> + bool reads = (direction == DMA_BIDIRECTIONAL || direction == >>>>>> +DMA_FROM_DEVICE); >>>>>> + >>>>>> + if (!reads || !(domain | AMDGPU_GEM_DOMAIN_GTT) || bo->pin_count) >>>>>> + return 0; >>>>>> + >>>>>> + /* move to gtt */ >>>>>> + ret = amdgpu_bo_reserve(bo, false); >>>>>> + if (unlikely(ret != 0)) >>>>>> + return ret; >>>>>> + >>>>>> + amdgpu_ttm_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT); >>>>>> + ret = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); >>>>>> + >>>>>> + amdgpu_bo_unreserve(bo); >>>>>> + return ret; >>>>>> +} >>>>>> + >>>>>> +static struct dma_buf_ops amdgpu_dmabuf_ops; static atomic_t aops_lock; >>>>>> + >>>>>> struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev, >>>>>> struct drm_gem_object *gobj, >>>>>> int flags) >>>>>> @@ -178,5 +206,35 @@ struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev, >>>>>> buf = drm_gem_prime_export(dev, gobj, flags); >>>>>> if (!IS_ERR(buf)) >>>>>> buf->file->f_mapping = dev->anon_inode->i_mapping; >>>>>> + >>>>>> + while (amdgpu_dmabuf_ops.begin_cpu_access != amdgpu_gem_begin_cpu_access) >>>>>> + { >>>>>> + if (!atomic_cmpxchg(&aops_lock, 0, 1)) { >>>>>> + amdgpu_dmabuf_ops = *(buf->ops); >>>>>> + amdgpu_dmabuf_ops.begin_cpu_access = amdgpu_gem_begin_cpu_access; >>>>>> + } >>>>>> + } >>>>>> + buf->ops = &amdgpu_dmabuf_ops; >>>>>> + >>>>>> return buf; >>>>>> } >>>>>> + >>>>>> +struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev, >>>>>> + struct dma_buf *dma_buf) >>>>>> +{ >>>>>> + struct drm_gem_object *obj; >>>>>> + >>>>>> + if (dma_buf->ops == &amdgpu_dmabuf_ops) { >>>>>> + obj = dma_buf->priv; >>>>>> + if (obj->dev == dev) { >>>>>> + /* >>>>>> + * Importing dmabuf exported from out own gem increases >>>>>> + * refcount on gem itself instead of f_count of dmabuf. >>>>>> + */ >>>>>> + drm_gem_object_get(obj); >>>>>> + return obj; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + return drm_gem_prime_import(dev, dma_buf); } >>>>>> -- >>>>>> 2.7.4 >>>>>> > </div> </div> </body> </html>