<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<p><br>
</p>
<div class="moz-cite-prefix">On 8/3/2023 2:12 PM, Christian König
wrote:<br>
</div>
<blockquote type="cite" cite="mid:af0efe19-e78c-2a8c-96c3-ddec10a62cf4@amd.com">
Am 03.08.23 um 07:23 schrieb SHANMUGAM, SRINIVASAN:<br>
<blockquote type="cite" cite="mid:f8c546b3-669b-41dc-bc35-c7fef96ad4e4@amd.com">
<p><br>
</p>
<div class="moz-cite-prefix">On 7/24/2023 10:43 PM, Alex Deucher
wrote:<br>
</div>
<blockquote type="cite" cite="mid:CADnq5_NcEr3=bDoSo5_YYYuNAMps655fcOuUDaAPmw3_QDwQNQ@mail.gmail.com">
<pre class="moz-quote-pre" wrap="">On Mon, Jul 24, 2023 at 11:54 AM Srinivasan Shanmugam
<a class="moz-txt-link-rfc2396E" href="mailto:srinivasan.shanmugam@amd.com" moz-do-not-send="true"><srinivasan.shanmugam@amd.com></a> wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Fixes the following from checkpatch.pl:
WARNING: Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst
+ volatile uint32_t *wb;
WARNING: Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst
+ volatile uint32_t *ptr;
'wb' field from 'amdgpu_wb' struct & 'ptr' field from
'amdgpu_mem_scratch', is not used to access h/w directly, neither they
are shared variables, so volatile is not necessary
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">How did you come to that determination? Both are GPU accessible
memory allocations. The writeback (wb) allocation happens to be in
GTT so it's system memory, but the the mem_scratch allocation can be
in device memory.
Alex
</pre>
</blockquote>
<p>Hi Alex,</p>
<p>Thanks for your feedbacks!<br>
</p>
<p>Commit message is misleading, I presumed that this volatile
modifiers are used for monitoring HW status registers due to
external events & for some shared variables - <span><span class="ui-provider fy b c d e f g h i j k l m n o p q r s t
u v w x y z ab ac ae af ag ah ai aj ak" dir="ltr"> may be
volatile might be needed for *wb pointer variable - as
they may be used for caches in between (on surface level
info), can we split this patch into two, I felt volatile
for *ptr is unnecessary as it is type casted with void
type [(void **)&adev->mem_scratch.ptr); in
amdgpu_device.c]- Any advises onto this please?</span></span></p>
</blockquote>
<br>
Instead of declaring pointers we should use
READ_ONCE()/WRITE_ONCE() when accessing those values to make sure
that the compiler doesn't do any nasty things.<br>
<br>
Regards,<br>
Christian.<br>
<br>
</blockquote>
<p>Thanks a lot Christian! <br>
</p>
<p>So both the variables needs to be changed to plain variables -
(ie., "u32 wb" & "u32 ptr" without any volatile keyword or
pointer variable) & then protect this variables with "READ_ONCE()/WRITE_ONCE()",
For ex: I have proposed -
<a class="moz-txt-link-freetext" href="https://patchwork.freedesktop.org/patch/551273/">https://patchwork.freedesktop.org/patch/551273/</a> am I correct
please? but may I know please is that volatile keyword, is that
doing the same job as "READ_ONCE()/WRITE_ONCE()", where compiler
optimizations is disabled? so that we can leave it as it is. &
ignore the checkpatch warning onto this please?<br>
</p>
<p>-Srini<br>
</p>
<blockquote type="cite" cite="mid:af0efe19-e78c-2a8c-96c3-ddec10a62cf4@amd.com">
<blockquote type="cite" cite="mid:f8c546b3-669b-41dc-bc35-c7fef96ad4e4@amd.com">
<p><span><span class="ui-provider fy b c d e f g h i j k l m n o p
q r s t u v w x y z ab ac ae af ag ah ai aj ak" dir="ltr">Best
regards,</span></span></p>
<p><span><span class="ui-provider fy b c d e f g h i j k l m n o p
q r s t u v w x y z ab ac ae af ag ah ai aj ak" dir="ltr">Srini<br>
</span></span></p>
<blockquote type="cite" cite="mid:CADnq5_NcEr3=bDoSo5_YYYuNAMps655fcOuUDaAPmw3_QDwQNQ@mail.gmail.com">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Cc: Christian König <a class="moz-txt-link-rfc2396E" href="mailto:christian.koenig@amd.com" moz-do-not-send="true"><christian.koenig@amd.com></a>
Cc: Alex Deucher <a class="moz-txt-link-rfc2396E" href="mailto:alexander.deucher@amd.com" moz-do-not-send="true"><alexander.deucher@amd.com></a>
Signed-off-by: Srinivasan Shanmugam <a class="moz-txt-link-rfc2396E" href="mailto:srinivasan.shanmugam@amd.com" moz-do-not-send="true"><srinivasan.shanmugam@amd.com></a>
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index a046160b6a0e..06f79a84ff4b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -502,7 +502,7 @@ int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv);
struct amdgpu_wb {
struct amdgpu_bo *wb_obj;
- volatile uint32_t *wb;
+ u32 *wb;
uint64_t gpu_addr;
u32 num_wb; /* Number of wb slots actually reserved for amdgpu. */
unsigned long used[DIV_ROUND_UP(AMDGPU_MAX_WB, BITS_PER_LONG)];
@@ -621,7 +621,7 @@ int amdgpu_cs_wait_fences_ioctl(struct drm_device *dev, void *data,
/* VRAM scratch page for HDP bug, default vram page */
struct amdgpu_mem_scratch {
struct amdgpu_bo *robj;
- volatile uint32_t *ptr;
+ u32 *ptr;
u64 gpu_addr;
};
--
2.25.1
</pre>
</blockquote>
</blockquote>
</blockquote>
<br>
</blockquote>
</body>
</html>