On 30/03/2023 22:35, Alex Deucher wrote:
On Thu, Mar 30, 2023 at 11:21 AM Shashank Sharma
<shashank.sharma@xxxxxxx> wrote:
On 30/03/2023 16:55, Alex Deucher wrote:
On Thu, Mar 30, 2023 at 10:34 AM Shashank Sharma
<shashank.sharma@xxxxxxx> wrote:
On 30/03/2023 16:15, Luben Tuikov wrote:
On 2023-03-30 10:04, Shashank Sharma wrote:
On 30/03/2023 15:42, Luben Tuikov wrote:
On 2023-03-29 11:47, Shashank Sharma wrote:
From: Shashank Sharma <contactshashanksharma@xxxxxxxxx>
This patch adds helper functions to create and free doorbell
pages for kernel objects.
Cc: Alex Deucher <alexander.deucher@xxxxxxx>
Cc: Christian Koenig <christian.koenig@xxxxxxx>
Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h | 41 ++++++++++++++++
.../gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c | 49 +++++++++++++++++++
2 files changed, 90 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
index f9c3b77bf65d..6581b78fe438 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
@@ -27,6 +27,24 @@
/*
* GPU doorbell structures, functions & helpers
*/
+
+/* Structure to hold doorbell pages from PCI doorbell BAR */
+struct amdgpu_doorbell_obj {
In the comment you say "Structure to hold ...";
it is a C structure, but then in the name of a function we see "obj".
(Object is something which is defined like in memory, i.e. it exists, not
something which is only declared.)
This is just a declaration of a structure, not an object per se.
I'd call it "struct amdgpu_doorbell_struct" or just "struct amdgpu_doorbell".
It is similar to struct amdgpu buffer object (struct amdgpu_bo), and
many more existing structure.
The amdpgu_bo is very different than a structure describing a doorbell.
The doorbell description isn't really "an object". I understand
the enthusiasm, but it is really not "an object". It's just a doorbell
description. :-)
amdgpu_bo is page of ttm_memory with additional information,
amdgpu_doorbell_obj is a page of ttm_doorbells with additional information
(it is not just one doorbell description)
I don't see a problem here.
I find the new API confusing. I would expect to see
amdgpu_bo_create_kernel(...DOORBELL...), amdgpu_bo_reserve(),
amdgpu_bo_kmap(), etc. That makes it consistent with the other
resource pools that we manage in ttm.
I am assuming here you are talking about why do we need
amdgpu_doorbell_page_create()/free() API here.
The wrappers here allow us to do additional book keeping work.
For example:
- We need to validate kernel doorbell writes, which means we need the
range of kernel doorbells.
- There are 3 kernel doorbell pages, for KGD, KFD and MES. These are non
consecutive pages.
- While we do doorbell_write in kernel, we need to check if this index
is correct, which means:
kgd_doobrell_min < index < kgd_doorbell_max
kfd_doobrell_min < index < kgd_doorbell_max
mes_doobrell_min < index < kgd_doorbell_max
- which means we need start/end limits set at every object.
- we have to either do this work at each place where we want to call
amdgpu_bo_create(DOORBELL)
which means KFD, KGD and MES all places (which will look irrelevant
in the context)
or we can do this in one place, which is the doorbell wrapper API.
Please see patch 10 for this range check.
I don't think we need the range checks for anything other than the
KGD. The MES stuff should just use the same allocation as KGD. KFD
has their own mapping in kfd_doorbell.c and they don't use the
WDOORBELL[64] macros.
Alex
Noted, I will remove the book keeping and the wrapper.
- Shashank
- Shashank
now kfd is setting start/end limits by calling
amdgpu_get_doorbell_index() function.
Alex
- Shashank
Regards,
Luben
- Shashank
Then in the definition, you can call it an object/objects, if you'd like,
like "struct amdgpu_doorbell *doorb_object[];" then you can say
"db = doorb_object[i]";
Regards,
Luben
+ struct amdgpu_bo *bo;
+ uint64_t gpu_addr;
+ uint32_t *cpu_addr;
+ uint32_t size;
+
+ /* First index in this object */
+ uint32_t start;
+
+ /* Last index in this object */
+ uint32_t end;
+
+ /* bitmap for dynamic doorbell allocation from this object */
+ unsigned long *doorbell_bitmap;
+};
+
struct amdgpu_doorbell {
/* doorbell mmio */
resource_size_t base;
@@ -328,6 +346,29 @@ int amdgpu_device_doorbell_init(struct amdgpu_device *adev);
*/
void amdgpu_device_doorbell_fini(struct amdgpu_device *adev);
+/**
+ * amdgpu_doorbell_free_page - Free a doorbell page
+ *
+ * @adev: amdgpu_device pointer
+ *
+ * @db_age: previously allocated doobell page details
+ *
+ */
+void amdgpu_doorbell_free_page(struct amdgpu_device *adev,
+ struct amdgpu_doorbell_obj *db_obj);
+
+/**
+ * amdgpu_doorbell_alloc_page - create a page from doorbell pool
+ *
+ * @adev: amdgpu_device pointer
+ *
+ * @db_age: doobell page structure to fill details with
+ *
+ * returns 0 on success, else error number
+ */
+int amdgpu_doorbell_alloc_page(struct amdgpu_device *adev,
+ struct amdgpu_doorbell_obj *db_obj);
+
#define RDOORBELL32(index) amdgpu_mm_rdoorbell(adev, (index))
#define WDOORBELL32(index, v) amdgpu_mm_wdoorbell(adev, (index), (v))
#define RDOORBELL64(index) amdgpu_mm_rdoorbell64(adev, (index))
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c
index 1aea92363fd3..8be15b82b545 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c
@@ -111,6 +111,55 @@ void amdgpu_mm_wdoorbell64(struct amdgpu_device *adev, u32 index, u64 v)
}
}
+/**
+ * amdgpu_doorbell_free_page - Free a doorbell page
+ *
+ * @adev: amdgpu_device pointer
+ *
+ * @db_age: previously allocated doobell page details
+ *
+ */
+void amdgpu_doorbell_free_page(struct amdgpu_device *adev,
+ struct amdgpu_doorbell_obj *db_obj)
+{
+ amdgpu_bo_free_kernel(&db_obj->bo,
+ &db_obj->gpu_addr,
+ (void **)&db_obj->cpu_addr);
+
+}
+
+/**
+ * amdgpu_doorbell_alloc_page - create a page from doorbell pool
+ *
+ * @adev: amdgpu_device pointer
+ *
+ * @db_age: doobell page structure to fill details with
+ *
+ * returns 0 on success, else error number
+ */
+int amdgpu_doorbell_alloc_page(struct amdgpu_device *adev,
+ struct amdgpu_doorbell_obj *db_obj)
+{
+ int r;
+
+ db_obj->size = ALIGN(db_obj->size, PAGE_SIZE);
+
+ r = amdgpu_bo_create_kernel(adev,
+ db_obj->size,
+ PAGE_SIZE,
+ AMDGPU_GEM_DOMAIN_DOORBELL,
+ &db_obj->bo,
+ &db_obj->gpu_addr,
+ (void **)&db_obj->cpu_addr);
+
+ if (r) {
+ DRM_ERROR("Failed to create doorbell BO, err=%d\n", r);
+ return r;
+ }
+
+ return 0;
+}
+
/*
* GPU doorbell aperture helpers function.
*/
