[AMD Official Use Only - AMD Internal Distribution Only]
Thank you very much Christian.
-----Original Message-----
From: Koenig, Christian <Christian.Koenig@xxxxxxx>
Sent: Tuesday, November 19, 2024 11:00 AM
To: Nirujogi, Pratap <Pratap.Nirujogi@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Limonciello, Mario <Mario.Limonciello@xxxxxxx>; Chan, Benjamin (Koon Pan) <Benjamin.Chan@xxxxxxx>; Li, King <King.Li@xxxxxxx>; Du, Bin <Bin.Du@xxxxxxx>
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.koenig@xxxxxxx>
Regards,
Christian.
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@xxxxxxx>
> Sent: Tuesday, November 19, 2024 10:30 AM
> To: Nirujogi, Pratap <Pratap.Nirujogi@xxxxxxx>;
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Limonciello, Mario
> <Mario.Limonciello@xxxxxxx>; Chan, Benjamin (Koon Pan)
> <Benjamin.Chan@xxxxxxx>; Li, King <King.Li@xxxxxxx>; Du, Bin
> <Bin.Du@xxxxxxx>
> 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.nirujogi@xxxxxxx>
>> ---
>> 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);
