[AMD Official Use Only - AMD Internal Distribution Only]
Thank you very much Christian.
-----Original Message-----
From: Koenig, Christian <christian.koe...@amd.com>
Sent: Tuesday, November 19, 2024 11:00 AM
To: Nirujogi, Pratap <pratap.niruj...@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <alexander.deuc...@amd.com>; Limonciello, Mario
<mario.limoncie...@amd.com>; Chan, Benjamin (Koon Pan) <benjamin.c...@amd.com>;
Li, King <king...@amd.com>; Du, Bin <bin...@amd.com>
Subject: Re: [PATCH 1/1] drm/amd/amdgpu: Add support for isp buffers
Am 19.11.24 um 16:51 schrieb Nirujogi, Pratap:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Thanks Christian, please see inline comments for this question.
>
> RE: Where exactly is this DMA-buf coming from? E.g. can you guarantee that
> this will ever be an amdgpu buffer object or could that also be something
> else?
>>>> The DMA-buf handle passed to amdgpu in this case is coming from ISP driver
>>>> for the buffer allocated in the system memory. I also would like to
>>>> clarify that the dma-buf handle used here for import and obtain amdgpu BO
>>>> is actually for the buffer allocated and owned by the ISP driver.
In that case feel free to add my Reviewed-by: Christian König
<christian.koe...@amd.com>
Regards,
Christian.
>
> -----Original Message-----
> From: Koenig, Christian <christian.koe...@amd.com>
> Sent: Tuesday, November 19, 2024 10:30 AM
> To: Nirujogi, Pratap <pratap.niruj...@amd.com>;
> amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <alexander.deuc...@amd.com>; Limonciello, Mario
> <mario.limoncie...@amd.com>; Chan, Benjamin (Koon Pan)
> <benjamin.c...@amd.com>; Li, King <king...@amd.com>; Du, Bin
> <bin...@amd.com>
> Subject: Re: [PATCH 1/1] drm/amd/amdgpu: Add support for isp buffers
>
> Am 15.07.24 um 16:42 schrieb Pratap Nirujogi:
>> Add support to create user BOs with MC address for isp using the
>> dma-buf handle exported for the buffers allocated from system memory in isp
>> driver.
>>
>> Export amdgpu_bo_create_kernel() and amdgpu_bo_free_kernel() as well
>> for isp to allocate GTT internal buffers required for fw to run.
>>
>> Signed-off-by: Pratap Nirujogi <pratap.niruj...@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 103 +++++++++++++++++++++
>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 4 +
>> 2 files changed, 107 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 6faeb9e4a572..517c9567a332 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -39,6 +39,7 @@
>> #include "amdgpu.h"
>> #include "amdgpu_trace.h"
>> #include "amdgpu_amdkfd.h"
>> +#include "amdgpu_dma_buf.h"
>>
>> /**
>> * DOC: amdgpu_object
>> @@ -334,6 +335,9 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
>> *
>> * Allocates and pins a BO for kernel internal use.
>> *
>> + * This function is exported to allow the V4L2 isp device
>> + * external to drm device to create and access the kernel BO.
>> + *
>> * Note: For bo_ptr new BO is only created if bo_ptr points to NULL.
>> *
>> * Returns:
>> @@ -357,6 +361,77 @@ int amdgpu_bo_create_kernel(struct amdgpu_device
>> *adev,
>>
>> return 0;
>> }
>> +EXPORT_SYMBOL(amdgpu_bo_create_kernel);
>> +
>> +/**
>> + * amdgpu_bo_create_isp_user - create user BO for isp
>> + *
>> + * @adev: amdgpu device object
>> + * @dma_buf: DMABUF handle for isp buffer
>> + * @domain: where to place it
>> + * @bo: used to initialize BOs in structures
>> + * @gpu_addr: GPU addr of the pinned BO
>> + *
>> + * Imports isp DMABUF to allocate and pin a user BO for isp internal
>> +use. It does
>> + * GART alloc to generate gpu_addr for BO to make it accessible
>> +through the
>> + * GART aperture for ISP HW.
> Where exactly is this DMA-buf coming from? E.g. can you guarantee that this
> will ever be an amdgpu buffer object or could that also be something else?
>
> Apart from that the patch looks good to me.
>
> Regards,
> Christian.
>
>> + *
>> + * This function is exported to allow the V4L2 isp device external
>> +to drm device
>> + * to create and access the isp user BO.
>> + *
>> + * Returns:
>> + * 0 on success, negative error code otherwise.
>> + */
>> +int amdgpu_bo_create_isp_user(struct amdgpu_device *adev,
>> + struct dma_buf *dbuf, u32 domain, struct amdgpu_bo
>> **bo,
>> + u64 *gpu_addr)
>> +
>> +{
>> + struct drm_gem_object *gem_obj;
>> + int r;
>> +
>> + gem_obj = amdgpu_gem_prime_import(&adev->ddev, dbuf);
>> + *bo = gem_to_amdgpu_bo(gem_obj);
>> + if (!(*bo)) {
>> + dev_err(adev->dev, "failed to get valid isp user bo\n");
>> + return -EINVAL;
>> + }
>> +
>> + r = amdgpu_bo_reserve(*bo, false);
>> + if (r) {
>> + dev_err(adev->dev, "(%d) failed to reserve isp user bo\n", r);
>> + return r;
>> + }
>> +
>> + r = amdgpu_bo_pin(*bo, domain);
>> + if (r) {
>> + dev_err(adev->dev, "(%d) isp user bo pin failed\n", r);
>> + goto error_unreserve;
>> + }
>> +
>> + r = amdgpu_ttm_alloc_gart(&(*bo)->tbo);
>> + if (r) {
>> + dev_err(adev->dev, "%p bind failed\n", *bo);
>> + goto error_unpin;
>> + }
>> +
>> + if (!WARN_ON(!gpu_addr))
>> + *gpu_addr = amdgpu_bo_gpu_offset(*bo);
>> +
>> + amdgpu_bo_unreserve(*bo);
>> +
>> + return 0;
>> +
>> +error_unpin:
>> + amdgpu_bo_unpin(*bo);
>> +error_unreserve:
>> + amdgpu_bo_unreserve(*bo);
>> + amdgpu_bo_unref(bo);
>> +
>> + return r;
>> +}
>> +EXPORT_SYMBOL(amdgpu_bo_create_isp_user);
>> +
>>
>> /**
>> * amdgpu_bo_create_kernel_at - create BO for kernel use at
>> specific location @@ -433,6 +508,9 @@ int amdgpu_bo_create_kernel_at(struct
>> amdgpu_device *adev,
>> * @cpu_addr: pointer to where the BO's CPU memory space address was
>> stored
>> *
>> * unmaps and unpin a BO for kernel internal use.
>> + *
>> + * This function is exported to allow the V4L2 isp device
>> + * external to drm device to free the kernel BO.
>> */
>> void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
>> void **cpu_addr) @@ -457,6 +535,31 @@ void
>> amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
>> if (cpu_addr)
>> *cpu_addr = NULL;
>> }
>> +EXPORT_SYMBOL(amdgpu_bo_free_kernel);
>> +
>> +
>> +/**
>> + * amdgpu_bo_free_isp_user - free BO for isp use
>> + *
>> + * @bo: amdgpu isp user BO to free
>> + *
>> + * unpin and unref BO for isp internal use.
>> + *
>> + * This function is exported to allow the V4L2 isp device
>> + * external to drm device to free the isp user BO.
>> + */
>> +void amdgpu_bo_free_isp_user(struct amdgpu_bo *bo) {
>> + if (bo == NULL)
>> + return;
>> +
>> + if (amdgpu_bo_reserve(bo, true) == 0) {
>> + amdgpu_bo_unpin(bo);
>> + amdgpu_bo_unreserve(bo);
>> + }
>> + amdgpu_bo_unref(&bo);
>> +}
>> +EXPORT_SYMBOL(amdgpu_bo_free_isp_user);
>>
>> /* Validate bo size is bit bigger than the request domain */
>> static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> index bc42ccbde659..17aa99b8311d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> @@ -299,6 +299,9 @@ int amdgpu_bo_create_kernel(struct amdgpu_device *adev,
>> unsigned long size, int align,
>> u32 domain, struct amdgpu_bo **bo_ptr,
>> u64 *gpu_addr, void **cpu_addr);
>> +int amdgpu_bo_create_isp_user(struct amdgpu_device *adev,
>> + struct dma_buf *dbuf, u32 domain, struct amdgpu_bo
>> **bo,
>> + u64 *gpu_addr);
>> int amdgpu_bo_create_kernel_at(struct amdgpu_device *adev,
>> uint64_t offset, uint64_t size,
>> struct amdgpu_bo **bo_ptr, void
>> **cpu_addr); @@ -310,6
>> +313,7 @@ int amdgpu_bo_create_vm(struct amdgpu_device *adev,
>> struct amdgpu_bo_vm **ubo_ptr);
>> void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
>> void **cpu_addr);
>> +void amdgpu_bo_free_isp_user(struct amdgpu_bo *bo);
>> int amdgpu_bo_kmap(struct amdgpu_bo *bo, void **ptr);
>> void *amdgpu_bo_kptr(struct amdgpu_bo *bo);
>> void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
