Re: Potential BUG: [PATCH 2/8] drm/amdgpu: add a generic fb accessing helper function(v3)
Hi Luben & Iago,
no the code is completely correct and intentional like this.
See the code path which takes the lock in amdgpu_mm_wreg() is a
workaround when the rmmio_size doesn't allow access to the full register
BAR.
In this case the MM_INDEX/MM_DATA registers are used as side path and
because of this we need the lock.
But this case can't happen when you use the MM_INDEX/MM_DATA pair
directly as parameters for the function.
Regards,
Christian.
Am 20.11.19 um 01:23 schrieb Luben Tuikov:
Hi Iago,
Thank you for finding and reporting this potential double lock.
Yes indeed, I see it--it can indeed happen.
Now, since the primitives used--macros using "amdgpu_mm_(r|w)reg\(.*\)"--in
"amdgpu_device_vram_access()" do use their own register-access spinlocks,
it maybe wise to remove the spinlock take/release in
"amdgpu_device_vram_access()".
We'll look into it and possibly submit another patch.
Thanks again.
Regards,
Luben
On 2019-11-16 11:21 a.m., Iago Abal wrote:
Hi,
With the help of a static bug finder (EBA - https://github.com/IagoAbal/eba) I
have found a potential double lock in Linux Next tag next-20191115, file
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c.
This bug seems to be introduced by commit
e35e2b117f4 ("drm/amdgpu: add a generic fb accessing helper function(v3)").
The steps to reproduce it would be:
1. Start in function `amdgpu_device_vram_access`.
2. Enter for-loop `for (last += pos; pos <= last; pos += 4)`.
3. First lock: `spin_lock_irqsave(&adev->mmio_idx_lock, flags)`.
4. Call to `WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x8000)`.
5. Note `#define WREG32_NO_KIQ(reg, v) amdgpu_mm_wreg(adev, (reg), (v),
AMDGPU_REGS_NO_KIQ)`.
6. Continue in function `amdgpu_mm_wreg`.
7. Take else-branch in the third if-statement.
8. Double lock: `spin_lock_irqsave(&adev->mmio_idx_lock, flags)`.
I think the control flow could reach that second lock, but you may know better.
Hope it helps!
-- iago
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: Potential BUG: [PATCH 2/8] drm/amdgpu: add a generic fb accessing helper function(v3)
Hi Iago,
Thank you for finding and reporting this potential double lock.
Yes indeed, I see it--it can indeed happen.
Now, since the primitives used--macros using "amdgpu_mm_(r|w)reg\(.*\)"--in
"amdgpu_device_vram_access()" do use their own register-access spinlocks,
it maybe wise to remove the spinlock take/release in
"amdgpu_device_vram_access()".
We'll look into it and possibly submit another patch.
Thanks again.
Regards,
Luben
On 2019-11-16 11:21 a.m., Iago Abal wrote:
> Hi,
>
> With the help of a static bug finder (EBA - https://github.com/IagoAbal/eba)
> I have found a potential double lock in Linux Next tag next-20191115, file
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c.
>
> This bug seems to be introduced by commit
> e35e2b117f4 ("drm/amdgpu: add a generic fb accessing helper function(v3)").
>
> The steps to reproduce it would be:
>
> 1. Start in function `amdgpu_device_vram_access`.
> 2. Enter for-loop `for (last += pos; pos <= last; pos += 4)`.
> 3. First lock: `spin_lock_irqsave(&adev->mmio_idx_lock, flags)`.
> 4. Call to `WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x8000)`.
> 5. Note `#define WREG32_NO_KIQ(reg, v) amdgpu_mm_wreg(adev, (reg), (v),
> AMDGPU_REGS_NO_KIQ)`.
> 6. Continue in function `amdgpu_mm_wreg`.
> 7. Take else-branch in the third if-statement.
> 8. Double lock: `spin_lock_irqsave(&adev->mmio_idx_lock, flags)`.
>
> I think the control flow could reach that second lock, but you may know
> better.
>
> Hope it helps!
>
> -- iago
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Potential BUG: [PATCH 2/8] drm/amdgpu: add a generic fb accessing helper function(v3)
Hi,
With the help of a static bug finder (EBA - https://github.com/IagoAbal/eba)
I have found a potential double lock in Linux Next tag next-20191115, file
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c.
This bug seems to be introduced by commit
e35e2b117f4 ("drm/amdgpu: add a generic fb accessing helper function(v3)").
The steps to reproduce it would be:
1. Start in function `amdgpu_device_vram_access`.
2. Enter for-loop `for (last += pos; pos <= last; pos += 4)`.
3. First lock: `spin_lock_irqsave(&adev->mmio_idx_lock, flags)`.
4. Call to `WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x8000)`.
5. Note `#define WREG32_NO_KIQ(reg, v) amdgpu_mm_wreg(adev, (reg), (v),
AMDGPU_REGS_NO_KIQ)`.
6. Continue in function `amdgpu_mm_wreg`.
7. Take else-branch in the third if-statement.
8. Double lock: `spin_lock_irqsave(&adev->mmio_idx_lock, flags)`.
I think the control flow could reach that second lock, but you may know
better.
Hope it helps!
-- iago
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 2/8] drm/amdgpu: add a generic fb accessing helper function(v3)
From: "Tianci.Yin"
add a generic helper function for accessing framebuffer via MMIO
Change-Id: I4baa0aa53c93a94c2eff98c6211a61f369239982
Reviewed-by: Alex Deucher
Signed-off-by: Tianci.Yin
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c| 30 +++
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 12 +---
3 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 7fa8e438f679..64a43b65a197 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -992,6 +992,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
void amdgpu_device_fini(struct amdgpu_device *adev);
int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
+void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
+ uint32_t *buf, size_t size, bool write);
uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
uint32_t acc_flags);
void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 598158e95ec1..13cc3aa52b8f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -154,6 +154,36 @@ bool amdgpu_device_is_px(struct drm_device *dev)
return false;
}
+/**
+ * VRAM access helper functions.
+ *
+ * amdgpu_device_vram_access - read/write a buffer in vram
+ *
+ * @adev: amdgpu_device pointer
+ * @pos: offset of the buffer in vram
+ * @buf: virtual address of the buffer in system memory
+ * @size: read/write size, sizeof(@buf) must > @size
+ * @write: true - write to vram, otherwise - read from vram
+ */
+void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
+ uint32_t *buf, size_t size, bool write)
+{
+ uint64_t last;
+ unsigned long flags;
+
+ last = size - 4;
+ for (last += pos; pos <= last; pos += 4) {
+ spin_lock_irqsave(&adev->mmio_idx_lock, flags);
+ WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x8000);
+ WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31);
+ if (write)
+ WREG32_NO_KIQ(mmMM_DATA, *buf++);
+ else
+ *buf++ = RREG32_NO_KIQ(mmMM_DATA);
+ spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
+ }
+}
+
/*
* MMIO register access helper functions.
*/
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index ddd8364102a2..f95092741c38 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -134,20 +134,10 @@ static int hw_id_map[MAX_HWIP] = {
static int amdgpu_discovery_read_binary(struct amdgpu_device *adev, uint8_t
*binary)
{
- uint32_t *p = (uint32_t *)binary;
uint64_t vram_size = (uint64_t)RREG32(mmRCC_CONFIG_MEMSIZE) << 20;
uint64_t pos = vram_size - DISCOVERY_TMR_SIZE;
- unsigned long flags;
-
- while (pos < vram_size) {
- spin_lock_irqsave(&adev->mmio_idx_lock, flags);
- WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x8000);
- WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31);
- *p++ = RREG32_NO_KIQ(mmMM_DATA);
- spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
- pos += 4;
- }
+ amdgpu_device_vram_access(adev, pos, (uint32_t *)binary,
DISCOVERY_TMR_SIZE, false);
return 0;
}
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 2/8] drm/amdgpu: add a generic fb accessing helper function
On 2019-10-10 11:50 p.m., Tianci Yin wrote:
> From: "Tianci.Yin"
>
> add a generic helper function for accessing framebuffer via MMIO
>
> Change-Id: I4baa0aa53c93a94c2eff98c6211a61f369239982
> Reviewed-by: Alex Deucher
> Signed-off-by: Tianci.Yin
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 ++
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c| 34 +++
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 13 +--
> 3 files changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 51ccf175cda0..1102e6bae5d5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -991,6 +991,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> void amdgpu_device_fini(struct amdgpu_device *adev);
> int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
>
> +int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> + uint32_t *buf, size_t size, bool write);
> uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
> uint32_t acc_flags);
> void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 598158e95ec1..fb21ec1f8a61 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -154,6 +154,40 @@ bool amdgpu_device_is_px(struct drm_device *dev)
> return false;
> }
>
> +/**
> + * VRAM access helper functions.
> + *
> + * amdgpu_device_vram_access - read/write a buffer in vram
> + *
> + * @adev: amdgpu_device pointer
> + * @pos: offset of the buffer in vram
> + * @buf: virtual address of the buffer in system memory
> + * @size: read/write size, sizeof(@buf) must > @size
> + * @write: true - write to vram, otherwise - read from vram
> + *
> + * Returns 0 on success or an -error on failure.
Really? Where?
This function seems to return 0.
Traditionally read/write functions
return the number of bytes read/written or error.
You do neither. Just define it void.
> + */
> +int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> + uint32_t *buf, size_t size, bool write)
> +{
> + uint64_t end = pos + size;
NAK! to preinitializing automatic variables.
> + unsigned long flags;
> +
> + while (pos < end) {
> + spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> + WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x8000);
> + WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31);
> + if (write)
> + WREG32_NO_KIQ(mmMM_DATA, *buf++);
> + else
> + *buf++ = RREG32_NO_KIQ(mmMM_DATA);
> + spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> + pos += 4;
> + }
NAK! to this this:
while (pos < end) { <---+
|--> Part of the loop definition
pos += 4; <---+
}
Instead of breaking up the loop definition like this,
use a for-loop, and DO NOT preinitialize the "end" variable,
and also protect from overflow, all like this:
last = size - 1;
for (last += pos; pos <= last; pos += 4) {
}
I mentioned the why of this in my previous review of the same
topic patchset over the same while loop fiasco.
Regards,
Luben
> +
> + return 0;
> +}
> +
> /*
> * MMIO register access helper functions.
> */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index db2dab3a6dff..324c2d605f54 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -134,21 +134,10 @@ static int hw_id_map[MAX_HWIP] = {
>
> static int amdgpu_discovery_read_binary(struct amdgpu_device *adev, uint8_t
> *binary)
> {
> - uint32_t *p = (uint32_t *)binary;
> uint64_t vram_size = (uint64_t)RREG32(mmRCC_CONFIG_MEMSIZE) << 20;
> uint64_t pos = vram_size - BINARY_MAX_SIZE;
> - unsigned long flags;
> -
> - while (pos < vram_size) {
> - spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> - WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x8000);
> - WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31);
> - *p++ = RREG32_NO_KIQ(mmMM_DATA);
> - spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> - pos += 4;
> - }
>
> - return 0;
> + return amdgpu_device_vram_access(adev, pos, (uint32_t *)binary,
> BINARY_MAX_SIZE, false);
> }
>
> static uint16_t amdgpu_discovery_calculate_checksum(uint8_t *data, uint32_t
> size)
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 2/8] drm/amdgpu: add a generic fb accessing helper function
From: "Tianci.Yin"
add a generic helper function for accessing framebuffer via MMIO
Change-Id: I4baa0aa53c93a94c2eff98c6211a61f369239982
Reviewed-by: Alex Deucher
Signed-off-by: Tianci.Yin
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c| 34 +++
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 13 +--
3 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 51ccf175cda0..1102e6bae5d5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -991,6 +991,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
void amdgpu_device_fini(struct amdgpu_device *adev);
int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
+int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
+ uint32_t *buf, size_t size, bool write);
uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
uint32_t acc_flags);
void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 598158e95ec1..fb21ec1f8a61 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -154,6 +154,40 @@ bool amdgpu_device_is_px(struct drm_device *dev)
return false;
}
+/**
+ * VRAM access helper functions.
+ *
+ * amdgpu_device_vram_access - read/write a buffer in vram
+ *
+ * @adev: amdgpu_device pointer
+ * @pos: offset of the buffer in vram
+ * @buf: virtual address of the buffer in system memory
+ * @size: read/write size, sizeof(@buf) must > @size
+ * @write: true - write to vram, otherwise - read from vram
+ *
+ * Returns 0 on success or an -error on failure.
+ */
+int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
+ uint32_t *buf, size_t size, bool write)
+{
+ uint64_t end = pos + size;
+ unsigned long flags;
+
+ while (pos < end) {
+ spin_lock_irqsave(&adev->mmio_idx_lock, flags);
+ WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x8000);
+ WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31);
+ if (write)
+ WREG32_NO_KIQ(mmMM_DATA, *buf++);
+ else
+ *buf++ = RREG32_NO_KIQ(mmMM_DATA);
+ spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
+ pos += 4;
+ }
+
+ return 0;
+}
+
/*
* MMIO register access helper functions.
*/
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index db2dab3a6dff..324c2d605f54 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -134,21 +134,10 @@ static int hw_id_map[MAX_HWIP] = {
static int amdgpu_discovery_read_binary(struct amdgpu_device *adev, uint8_t
*binary)
{
- uint32_t *p = (uint32_t *)binary;
uint64_t vram_size = (uint64_t)RREG32(mmRCC_CONFIG_MEMSIZE) << 20;
uint64_t pos = vram_size - BINARY_MAX_SIZE;
- unsigned long flags;
-
- while (pos < vram_size) {
- spin_lock_irqsave(&adev->mmio_idx_lock, flags);
- WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x8000);
- WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31);
- *p++ = RREG32_NO_KIQ(mmMM_DATA);
- spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
- pos += 4;
- }
- return 0;
+ return amdgpu_device_vram_access(adev, pos, (uint32_t *)binary,
BINARY_MAX_SIZE, false);
}
static uint16_t amdgpu_discovery_calculate_checksum(uint8_t *data, uint32_t
size)
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 2/8] drm/amdgpu: add a generic fb accessing helper function
Ok,
Thanks for your reviewing!
Rico
From: Christian K?nig
Sent: Wednesday, October 9, 2019 16:25
To: Alex Deucher ; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander ; Yin, Tianci (Rico)
Subject: Re: [PATCH 2/8] drm/amdgpu: add a generic fb accessing helper function
Am 08.10.19 um 21:29 schrieb Alex Deucher:
> From: "Tianci.Yin"
>
> add a generic helper function for accessing framebuffer via MMIO
>
> Change-Id: I4baa0aa53c93a94c2eff98c6211a61f369239982
> Reviewed-by: Alex Deucher
> Signed-off-by: Tianci.Yin
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c| 42 +++
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 13 +-
> 3 files changed, 45 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 17ccb9015141..0d60c2e6c592 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -985,6 +985,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> void amdgpu_device_fini(struct amdgpu_device *adev);
> int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
>
> +int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> +uint32_t *buf, size_t size, bool write);
> uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
>uint32_t acc_flags);
> void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index f25275abf408..53ce227f759c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -153,6 +153,48 @@ bool amdgpu_device_is_px(struct drm_device *dev)
>return false;
> }
>
> +/**
> + * VRAM access helper functions.
> + *
> + * amdgpu_device_vram_access - read/write a buffer in vram
> + *
> + * @adev: amdgpu_device pointer
> + * @pos: offset of the buffer in vram
> + * @buf: virtual address of the buffer in system memory
> + * @size: read/write size, sizeof(@buf) must > @size
> + * @write: true - write to vram, otherwise - read from vram
> + *
> + * Returns 0 on success or an -error on failure.
> + */
> +int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> +uint32_t *buf, size_t size, bool write)
Indentation seems to be incorrect here.
> +{
> + uint64_t end = pos + size;
> + unsigned long flags;
> +
> + if (IS_ERR_OR_NULL(buf) ||
> + (adev->gmc.mc_vram_size > 0 &&
> + end > adev->gmc.mc_vram_size)) {
> + DRM_ERROR("parameter error! pos:%llx, buf:%llx, size:%zx.\n",
> + pos, (u64)buf, size);
> + return -EINVAL;
> + }
Please drop that, this is a purely internal functions and parameter
checking like this doesn't really make sense.
Regards,
Christian.
> +
> + while (pos < end) {
> + spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> + WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x8000);
> + WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31);
> + if (write)
> + WREG32_NO_KIQ(mmMM_DATA, *buf++);
> + else
> + *buf++ = RREG32_NO_KIQ(mmMM_DATA);
> + spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> + pos += 4;
> + }
> +
> + return 0;
> +}
> +
> /*
>* MMIO register access helper functions.
>*/
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index db2dab3a6dff..324c2d605f54 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -134,21 +134,10 @@ static int hw_id_map[MAX_HWIP] = {
>
> static int amdgpu_discovery_read_binary(struct amdgpu_device *adev, uint8_t
> *binary)
> {
> - uint32_t *p = (uint32_t *)binary;
>uint64_t vram_size = (uint64_t)RREG32(mmRCC_CONFIG_MEMSIZE) << 20;
>uint64_t pos = vram_size - BINARY_MAX_SIZE;
> - unsigned long flags;
> -
> - while (pos < vram_size) {
> - spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> - WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x8000);
> - WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31);
> - *p++ = RREG32_NO_KIQ(mmMM_DATA);
> - spin_unlock_irqrestore(&adev->mmio_idx_lock, flags
Re: [PATCH 2/8] drm/amdgpu: add a generic fb accessing helper function
Am 08.10.19 um 21:29 schrieb Alex Deucher:
From: "Tianci.Yin"
add a generic helper function for accessing framebuffer via MMIO
Change-Id: I4baa0aa53c93a94c2eff98c6211a61f369239982
Reviewed-by: Alex Deucher
Signed-off-by: Tianci.Yin
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 +
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c| 42 +++
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 13 +-
3 files changed, 45 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 17ccb9015141..0d60c2e6c592 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -985,6 +985,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
void amdgpu_device_fini(struct amdgpu_device *adev);
int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
+int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
+ uint32_t *buf, size_t size, bool write);
uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
uint32_t acc_flags);
void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index f25275abf408..53ce227f759c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -153,6 +153,48 @@ bool amdgpu_device_is_px(struct drm_device *dev)
return false;
}
+/**
+ * VRAM access helper functions.
+ *
+ * amdgpu_device_vram_access - read/write a buffer in vram
+ *
+ * @adev: amdgpu_device pointer
+ * @pos: offset of the buffer in vram
+ * @buf: virtual address of the buffer in system memory
+ * @size: read/write size, sizeof(@buf) must > @size
+ * @write: true - write to vram, otherwise - read from vram
+ *
+ * Returns 0 on success or an -error on failure.
+ */
+int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
+ uint32_t *buf, size_t size, bool write)
Indentation seems to be incorrect here.
+{
+ uint64_t end = pos + size;
+ unsigned long flags;
+
+ if (IS_ERR_OR_NULL(buf) ||
+ (adev->gmc.mc_vram_size > 0 &&
+end > adev->gmc.mc_vram_size)) {
+ DRM_ERROR("parameter error! pos:%llx, buf:%llx, size:%zx.\n",
+ pos, (u64)buf, size);
+ return -EINVAL;
+ }
Please drop that, this is a purely internal functions and parameter
checking like this doesn't really make sense.
Regards,
Christian.
+
+ while (pos < end) {
+ spin_lock_irqsave(&adev->mmio_idx_lock, flags);
+ WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x8000);
+ WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31);
+ if (write)
+ WREG32_NO_KIQ(mmMM_DATA, *buf++);
+ else
+ *buf++ = RREG32_NO_KIQ(mmMM_DATA);
+ spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
+ pos += 4;
+ }
+
+ return 0;
+}
+
/*
* MMIO register access helper functions.
*/
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index db2dab3a6dff..324c2d605f54 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -134,21 +134,10 @@ static int hw_id_map[MAX_HWIP] = {
static int amdgpu_discovery_read_binary(struct amdgpu_device *adev, uint8_t *binary)
{
- uint32_t *p = (uint32_t *)binary;
uint64_t vram_size = (uint64_t)RREG32(mmRCC_CONFIG_MEMSIZE) << 20;
uint64_t pos = vram_size - BINARY_MAX_SIZE;
- unsigned long flags;
-
- while (pos < vram_size) {
- spin_lock_irqsave(&adev->mmio_idx_lock, flags);
- WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x8000);
- WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31);
- *p++ = RREG32_NO_KIQ(mmMM_DATA);
- spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
- pos += 4;
- }
- return 0;
+ return amdgpu_device_vram_access(adev, pos, (uint32_t *)binary,
BINARY_MAX_SIZE, false);
}
static uint16_t amdgpu_discovery_calculate_checksum(uint8_t *data, uint32_t size)
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 2/8] drm/amdgpu: add a generic fb accessing helper function
From: "Tianci.Yin"
add a generic helper function for accessing framebuffer via MMIO
Change-Id: I4baa0aa53c93a94c2eff98c6211a61f369239982
Reviewed-by: Alex Deucher
Signed-off-by: Tianci.Yin
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 +
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c| 42 +++
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 13 +-
3 files changed, 45 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 17ccb9015141..0d60c2e6c592 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -985,6 +985,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
void amdgpu_device_fini(struct amdgpu_device *adev);
int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
+int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
+ uint32_t *buf, size_t size, bool write);
uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
uint32_t acc_flags);
void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index f25275abf408..53ce227f759c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -153,6 +153,48 @@ bool amdgpu_device_is_px(struct drm_device *dev)
return false;
}
+/**
+ * VRAM access helper functions.
+ *
+ * amdgpu_device_vram_access - read/write a buffer in vram
+ *
+ * @adev: amdgpu_device pointer
+ * @pos: offset of the buffer in vram
+ * @buf: virtual address of the buffer in system memory
+ * @size: read/write size, sizeof(@buf) must > @size
+ * @write: true - write to vram, otherwise - read from vram
+ *
+ * Returns 0 on success or an -error on failure.
+ */
+int amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
+ uint32_t *buf, size_t size, bool write)
+{
+ uint64_t end = pos + size;
+ unsigned long flags;
+
+ if (IS_ERR_OR_NULL(buf) ||
+ (adev->gmc.mc_vram_size > 0 &&
+end > adev->gmc.mc_vram_size)) {
+ DRM_ERROR("parameter error! pos:%llx, buf:%llx, size:%zx.\n",
+ pos, (u64)buf, size);
+ return -EINVAL;
+ }
+
+ while (pos < end) {
+ spin_lock_irqsave(&adev->mmio_idx_lock, flags);
+ WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x8000);
+ WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31);
+ if (write)
+ WREG32_NO_KIQ(mmMM_DATA, *buf++);
+ else
+ *buf++ = RREG32_NO_KIQ(mmMM_DATA);
+ spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
+ pos += 4;
+ }
+
+ return 0;
+}
+
/*
* MMIO register access helper functions.
*/
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index db2dab3a6dff..324c2d605f54 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -134,21 +134,10 @@ static int hw_id_map[MAX_HWIP] = {
static int amdgpu_discovery_read_binary(struct amdgpu_device *adev, uint8_t
*binary)
{
- uint32_t *p = (uint32_t *)binary;
uint64_t vram_size = (uint64_t)RREG32(mmRCC_CONFIG_MEMSIZE) << 20;
uint64_t pos = vram_size - BINARY_MAX_SIZE;
- unsigned long flags;
-
- while (pos < vram_size) {
- spin_lock_irqsave(&adev->mmio_idx_lock, flags);
- WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x8000);
- WREG32_NO_KIQ(mmMM_INDEX_HI, pos >> 31);
- *p++ = RREG32_NO_KIQ(mmMM_DATA);
- spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
- pos += 4;
- }
- return 0;
+ return amdgpu_device_vram_access(adev, pos, (uint32_t *)binary,
BINARY_MAX_SIZE, false);
}
static uint16_t amdgpu_discovery_calculate_checksum(uint8_t *data, uint32_t
size)
--
2.20.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
