On 30/03/2023 16:59, Luben Tuikov wrote:
On 2023-03-30 10:53, Shashank Sharma wrote:
On 30/03/2023 16:49, Christian König wrote:
Am 30.03.23 um 16:40 schrieb Shashank Sharma:
On 30/03/2023 16:24, Luben Tuikov wrote:
On 2023-03-29 11:47, Shashank Sharma wrote:
From: Shashank Sharma <contactshashanksharma@xxxxxxxxx>
This patch:
- creates a doorbell page for graphics driver usages.
- removes the adev->doorbell.ptr variable, replaces it with
kernel-doorbell-bo's cpu address.
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 | 16 ++++++-
.../gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c | 44
+++++++++++++++----
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 7 +++
3 files changed, 57 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
index 6581b78fe438..10a9bb10e974 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
@@ -49,10 +49,13 @@ struct amdgpu_doorbell {
/* doorbell mmio */
resource_size_t base;
resource_size_t size;
- u32 __iomem *ptr;
+ u32 __iomem *ptr;
/* Number of doorbells reserved for amdgpu kernel driver */
u32 num_kernel_doorbells;
+
+ /* For kernel doorbell pages */
+ struct amdgpu_doorbell_obj kernel_doorbells;
};
Here's an example where it could be confusing what the difference is
between "struct amdgpu_doorbell" and "struct amdgpu_doobell_obj".
As the comment to the struct doorbell_obj declarations says
in patch 7,
+/* Structure to hold doorbell pages from PCI doorbell BAR */
+struct amdgpu_doorbell_obj {
What is the confusion ? This is the object which is holding doorbell
page. There could be 2 type of
doorbell pages, kernel and process, this is a kernel one.
Perhaps we should call it "struct amdgpu_doorbell_bo", since
it does contain amdgpu_bo's, which are doorbell's bos.
This is not a buffer object (memory), this is doorbell object, so
calling it bo would be wrong.
I think what Luben means is that in object orient programming this
here would be the class. The object is then the actual instantiation
of that.
Why should we even bother about OOPs terminology in kernel C code ? I
think we are spending too much time in something not worth.
Because you're using "object" incorrectly. Especially for people with
vast programming experience, this creates confusion. Please don't use
"obj" in the name of a structure. Perhaps use "bo" or "page" or something
which it really _is_. But don't mix OOP terminology in non-OOP code. We
have people who program both sides of the isle and this creates confusion.
The correct usage of object is valid only in context of OOP, not in C
code. As I have mentioned,
object or obj has been used in the existing AMDGPU code at multiple
places, and it did not create
confusion so far, so it should be good now as well. I certainly don't
find it a reason to change the
name of the structure.
- Shashank
Let's use structure names which really describe what something is. This would
help very much new people reading the code in the future, to form mental
concepts and better understand the code.
Regards,
Luben
But I have some real doubts that this is the right approach in the
first place.
I would like to discuss and understand more on this technical aspect.
Can you please have a look at the whole series and check how we have
handled the existing doorbell clients (KFD, MES), and if you feel the
same, we should talk more on this ?
- Shashank
Regards,
Christian.
- Shashank
Regards,
Luben
/* Reserved doorbells for amdgpu (including multimedia).
@@ -369,6 +372,17 @@ void amdgpu_doorbell_free_page(struct
amdgpu_device *adev,
int amdgpu_doorbell_alloc_page(struct amdgpu_device *adev,
struct amdgpu_doorbell_obj *db_obj);
+/**
+ * amdgpu_doorbell_create_kernel_doorbells - Create kernel
doorbells for graphics
+ *
+ * @adev: amdgpu_device pointer
+ *
+ * Creates doorbells for graphics driver
+ *
+ * returns 0 on success, error otherwise.
+ */
+int amdgpu_doorbell_create_kernel_doorbells(struct amdgpu_device
*adev);
+
#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 8be15b82b545..b46fe8b1378d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c
@@ -160,6 +160,38 @@ int amdgpu_doorbell_alloc_page(struct
amdgpu_device *adev,
return 0;
}
+/**
+ * amdgpu_doorbell_create_kernel_doorbells - Create kernel
doorbells for graphics
+ *
+ * @adev: amdgpu_device pointer
+ *
+ * Creates doorbells for graphics driver
+ *
+ * returns 0 on success, error otherwise.
+ */
+int amdgpu_doorbell_create_kernel_doorbells(struct amdgpu_device
*adev)
+{
+ int r;
+ struct amdgpu_doorbell_obj *kernel_doorbells =
&adev->doorbell.kernel_doorbells;
+
+ kernel_doorbells->doorbell_bitmap =
bitmap_zalloc(adev->doorbell.num_kernel_doorbells,
+ GFP_KERNEL);
+ if (!kernel_doorbells->doorbell_bitmap) {
+ DRM_ERROR("Failed to create kernel doorbell bitmap\n");
+ return -ENOMEM;
+ }
+
+ kernel_doorbells->size = adev->doorbell.num_kernel_doorbells *
sizeof(u32);
+ r = amdgpu_doorbell_alloc_page(adev, kernel_doorbells);
+ if (r) {
+ bitmap_free(kernel_doorbells->doorbell_bitmap);
+ DRM_ERROR("Failed to allocate kernel doorbells, err=%d\n",
r);
+ return r;
+ }
+
+ return 0;
+}
+
/*
* GPU doorbell aperture helpers function.
*/
@@ -179,7 +211,6 @@ int amdgpu_device_doorbell_init(struct
amdgpu_device *adev)
adev->doorbell.base = 0;
adev->doorbell.size = 0;
adev->doorbell.num_kernel_doorbells = 0;
- adev->doorbell.ptr = NULL;
return 0;
}
@@ -208,12 +239,7 @@ int amdgpu_device_doorbell_init(struct
amdgpu_device *adev)
if (adev->asic_type >= CHIP_VEGA10)
adev->doorbell.num_kernel_doorbells += 0x400;
- adev->doorbell.ptr = ioremap(adev->doorbell.base,
- adev->doorbell.num_kernel_doorbells *
- sizeof(u32));
- if (adev->doorbell.ptr == NULL)
- return -ENOMEM;
-
+ adev->doorbell.ptr = ioremap(adev->doorbell.base,
adev->doorbell.size);
return 0;
}
@@ -226,6 +252,6 @@ int amdgpu_device_doorbell_init(struct
amdgpu_device *adev)
*/
void amdgpu_device_doorbell_fini(struct amdgpu_device *adev)
{
- iounmap(adev->doorbell.ptr);
- adev->doorbell.ptr = NULL;
+ bitmap_free(adev->doorbell.kernel_doorbells.doorbell_bitmap);
+ amdgpu_doorbell_free_page(adev,
&adev->doorbell.kernel_doorbells);
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 203d77a20507..75c6852845c4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1866,6 +1866,13 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
return r;
}
+ /* Create a boorbell page for kernel usages */
+ r = amdgpu_doorbell_create_kernel_doorbells(adev);
+ if (r) {
+ DRM_ERROR("Failed to initialize kernel doorbells. \n");
+ return r;
+ }
+
/* Initialize preemptible memory pool */
r = amdgpu_preempt_mgr_init(adev);
if (r) {
