* [PATCH] drm/amdgpu: Use READ_ONCE() when reading the values in 'sdma_v4_4_2_ring_get_rptr'
@ 2023-08-04 5:46 Srinivasan Shanmugam
2023-08-18 11:26 ` Ma, Le
2023-08-21 11:23 ` Christian König
0 siblings, 2 replies; 5+ messages in thread
From: Srinivasan Shanmugam @ 2023-08-04 5:46 UTC (permalink / raw)
To: Christian König, Alex Deucher, Guchun Chen, Pan Xinhui
Cc: Le Ma, Srinivasan Shanmugam, amd-gfx, Hawking Zhang
Instead of declaring pointers use READ_ONCE(), when accessing those
values to make sure that the compiler doesn't voilate any cache
coherences
Cc: Guchun Chen <guchun.chen@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
Cc: Le Ma <le.ma@amd.com>
Cc: Hawking Zhang <Hawking.Zhang@amd.com>
Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
---
drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
index f413898dda37..267c1b7b8dcd 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
@@ -154,13 +154,13 @@ static int sdma_v4_4_2_init_microcode(struct amdgpu_device *adev)
*/
static uint64_t sdma_v4_4_2_ring_get_rptr(struct amdgpu_ring *ring)
{
- u64 *rptr;
+ u64 rptr;
/* XXX check if swapping is necessary on BE */
- rptr = ((u64 *)&ring->adev->wb.wb[ring->rptr_offs]);
+ rptr = READ_ONCE(*((u64 *)&ring->adev->wb.wb[ring->rptr_offs]));
- DRM_DEBUG("rptr before shift == 0x%016llx\n", *rptr);
- return ((*rptr) >> 2);
+ DRM_DEBUG("rptr before shift == 0x%016llx\n", rptr);
+ return rptr >> 2;
}
/**
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [PATCH] drm/amdgpu: Use READ_ONCE() when reading the values in 'sdma_v4_4_2_ring_get_rptr'
2023-08-04 5:46 [PATCH] drm/amdgpu: Use READ_ONCE() when reading the values in 'sdma_v4_4_2_ring_get_rptr' Srinivasan Shanmugam
@ 2023-08-18 11:26 ` Ma, Le
2023-08-21 11:23 ` Christian König
1 sibling, 0 replies; 5+ messages in thread
From: Ma, Le @ 2023-08-18 11:26 UTC (permalink / raw)
To: SHANMUGAM, SRINIVASAN, Koenig, Christian, Deucher, Alexander,
Chen, Guchun, Pan, Xinhui
Cc: amd-gfx, Zhang, Hawking
[AMD Official Use Only - General]
Reviewed-by: Le Ma <le.ma@amd.com>
> -----Original Message-----
> From: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@amd.com>
> Sent: Friday, August 4, 2023 1:47 PM
> To: Koenig, Christian <Christian.Koenig@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>;
> Pan, Xinhui <Xinhui.Pan@amd.com>
> Cc: amd-gfx@lists.freedesktop.org; SHANMUGAM, SRINIVASAN
> <SRINIVASAN.SHANMUGAM@amd.com>; Ma, Le <Le.Ma@amd.com>; Zhang,
> Hawking <Hawking.Zhang@amd.com>
> Subject: [PATCH] drm/amdgpu: Use READ_ONCE() when reading the values in
> 'sdma_v4_4_2_ring_get_rptr'
>
> Instead of declaring pointers use READ_ONCE(), when accessing those values to
> make sure that the compiler doesn't voilate any cache coherences
>
> Cc: Guchun Chen <guchun.chen@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
> Cc: Le Ma <le.ma@amd.com>
> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> index f413898dda37..267c1b7b8dcd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> @@ -154,13 +154,13 @@ static int sdma_v4_4_2_init_microcode(struct
> amdgpu_device *adev)
> */
> static uint64_t sdma_v4_4_2_ring_get_rptr(struct amdgpu_ring *ring) {
> - u64 *rptr;
> + u64 rptr;
>
> /* XXX check if swapping is necessary on BE */
> - rptr = ((u64 *)&ring->adev->wb.wb[ring->rptr_offs]);
> + rptr = READ_ONCE(*((u64 *)&ring->adev->wb.wb[ring->rptr_offs]));
>
> - DRM_DEBUG("rptr before shift == 0x%016llx\n", *rptr);
> - return ((*rptr) >> 2);
> + DRM_DEBUG("rptr before shift == 0x%016llx\n", rptr);
> + return rptr >> 2;
> }
>
> /**
> --
> 2.25.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amdgpu: Use READ_ONCE() when reading the values in 'sdma_v4_4_2_ring_get_rptr'
2023-08-04 5:46 [PATCH] drm/amdgpu: Use READ_ONCE() when reading the values in 'sdma_v4_4_2_ring_get_rptr' Srinivasan Shanmugam
2023-08-18 11:26 ` Ma, Le
@ 2023-08-21 11:23 ` Christian König
2023-08-21 20:49 ` Felix Kuehling
1 sibling, 1 reply; 5+ messages in thread
From: Christian König @ 2023-08-21 11:23 UTC (permalink / raw)
To: Srinivasan Shanmugam, Christian König, Alex Deucher,
Guchun Chen, Pan Xinhui
Cc: Le Ma, amd-gfx, Hawking Zhang
Am 04.08.23 um 07:46 schrieb Srinivasan Shanmugam:
> Instead of declaring pointers use READ_ONCE(), when accessing those
> values to make sure that the compiler doesn't voilate any cache
> coherences
That commit message is a bit confusing and not 100% technically correct.
The compiler is not causing any cache coherency issues, but potentially
re-ordering things or reading the value multiple times.
Just write something like "Use READ_ONCE() instead of declaring the
pointer volatile.". The background explanation would exceed the
information suitable for a commit message anyway.
Apart from that looks good to me,
Christian.
>
> Cc: Guchun Chen <guchun.chen@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
> Cc: Le Ma <le.ma@amd.com>
> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> index f413898dda37..267c1b7b8dcd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> @@ -154,13 +154,13 @@ static int sdma_v4_4_2_init_microcode(struct amdgpu_device *adev)
> */
> static uint64_t sdma_v4_4_2_ring_get_rptr(struct amdgpu_ring *ring)
> {
> - u64 *rptr;
> + u64 rptr;
>
> /* XXX check if swapping is necessary on BE */
> - rptr = ((u64 *)&ring->adev->wb.wb[ring->rptr_offs]);
> + rptr = READ_ONCE(*((u64 *)&ring->adev->wb.wb[ring->rptr_offs]));
>
> - DRM_DEBUG("rptr before shift == 0x%016llx\n", *rptr);
> - return ((*rptr) >> 2);
> + DRM_DEBUG("rptr before shift == 0x%016llx\n", rptr);
> + return rptr >> 2;
> }
>
> /**
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amdgpu: Use READ_ONCE() when reading the values in 'sdma_v4_4_2_ring_get_rptr'
2023-08-21 11:23 ` Christian König
@ 2023-08-21 20:49 ` Felix Kuehling
2023-08-22 9:53 ` Christian König
0 siblings, 1 reply; 5+ messages in thread
From: Felix Kuehling @ 2023-08-21 20:49 UTC (permalink / raw)
To: Christian König, Srinivasan Shanmugam, Alex Deucher,
Guchun Chen, Pan Xinhui
Cc: Le Ma, amd-gfx, Hawking Zhang
Would it make sense to include a link to a better explanation of the
underlying issue? E.g. https://lwn.net/Articles/624126/?
Regards,
Felix
On 2023-08-21 07:23, Christian König wrote:
> Am 04.08.23 um 07:46 schrieb Srinivasan Shanmugam:
>> Instead of declaring pointers use READ_ONCE(), when accessing those
>> values to make sure that the compiler doesn't voilate any cache
>> coherences
>
> That commit message is a bit confusing and not 100% technically correct.
>
> The compiler is not causing any cache coherency issues, but
> potentially re-ordering things or reading the value multiple times.
>
> Just write something like "Use READ_ONCE() instead of declaring the
> pointer volatile.". The background explanation would exceed the
> information suitable for a commit message anyway.
>
> Apart from that looks good to me,
> Christian.
>
>>
>> Cc: Guchun Chen <guchun.chen@amd.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
>> Cc: Le Ma <le.ma@amd.com>
>> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
>> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>> b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>> index f413898dda37..267c1b7b8dcd 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>> @@ -154,13 +154,13 @@ static int sdma_v4_4_2_init_microcode(struct
>> amdgpu_device *adev)
>> */
>> static uint64_t sdma_v4_4_2_ring_get_rptr(struct amdgpu_ring *ring)
>> {
>> - u64 *rptr;
>> + u64 rptr;
>> /* XXX check if swapping is necessary on BE */
>> - rptr = ((u64 *)&ring->adev->wb.wb[ring->rptr_offs]);
>> + rptr = READ_ONCE(*((u64 *)&ring->adev->wb.wb[ring->rptr_offs]));
>> - DRM_DEBUG("rptr before shift == 0x%016llx\n", *rptr);
>> - return ((*rptr) >> 2);
>> + DRM_DEBUG("rptr before shift == 0x%016llx\n", rptr);
>> + return rptr >> 2;
>> }
>> /**
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amdgpu: Use READ_ONCE() when reading the values in 'sdma_v4_4_2_ring_get_rptr'
2023-08-21 20:49 ` Felix Kuehling
@ 2023-08-22 9:53 ` Christian König
0 siblings, 0 replies; 5+ messages in thread
From: Christian König @ 2023-08-22 9:53 UTC (permalink / raw)
To: Felix Kuehling, Srinivasan Shanmugam, Alex Deucher, Guchun Chen,
Pan Xinhui
Cc: Le Ma, amd-gfx, Hawking Zhang
Yes, that's a very good idea as well. Going to keep that link around if
anybody asks why we do this.
Christian.
Am 21.08.23 um 22:49 schrieb Felix Kuehling:
> Would it make sense to include a link to a better explanation of the
> underlying issue? E.g. https://lwn.net/Articles/624126/?
>
> Regards,
> Felix
>
>
> On 2023-08-21 07:23, Christian König wrote:
>> Am 04.08.23 um 07:46 schrieb Srinivasan Shanmugam:
>>> Instead of declaring pointers use READ_ONCE(), when accessing those
>>> values to make sure that the compiler doesn't voilate any cache
>>> coherences
>>
>> That commit message is a bit confusing and not 100% technically correct.
>>
>> The compiler is not causing any cache coherency issues, but
>> potentially re-ordering things or reading the value multiple times.
>>
>> Just write something like "Use READ_ONCE() instead of declaring the
>> pointer volatile.". The background explanation would exceed the
>> information suitable for a commit message anyway.
>>
>> Apart from that looks good to me,
>> Christian.
>>
>>>
>>> Cc: Guchun Chen <guchun.chen@amd.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
>>> Cc: Le Ma <le.ma@amd.com>
>>> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
>>> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>>> b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>>> index f413898dda37..267c1b7b8dcd 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>>> @@ -154,13 +154,13 @@ static int sdma_v4_4_2_init_microcode(struct
>>> amdgpu_device *adev)
>>> */
>>> static uint64_t sdma_v4_4_2_ring_get_rptr(struct amdgpu_ring *ring)
>>> {
>>> - u64 *rptr;
>>> + u64 rptr;
>>> /* XXX check if swapping is necessary on BE */
>>> - rptr = ((u64 *)&ring->adev->wb.wb[ring->rptr_offs]);
>>> + rptr = READ_ONCE(*((u64 *)&ring->adev->wb.wb[ring->rptr_offs]));
>>> - DRM_DEBUG("rptr before shift == 0x%016llx\n", *rptr);
>>> - return ((*rptr) >> 2);
>>> + DRM_DEBUG("rptr before shift == 0x%016llx\n", rptr);
>>> + return rptr >> 2;
>>> }
>>> /**
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-08-22 9:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-04 5:46 [PATCH] drm/amdgpu: Use READ_ONCE() when reading the values in 'sdma_v4_4_2_ring_get_rptr' Srinivasan Shanmugam
2023-08-18 11:26 ` Ma, Le
2023-08-21 11:23 ` Christian König
2023-08-21 20:49 ` Felix Kuehling
2023-08-22 9:53 ` Christian König
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.