<html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> Am 03.08.23 um 07:23 schrieb SHANMUGAM, SRINIVASAN: <blockquote type="cite" cite="mid:f8c546b3-669b-41dc-bc35-c7fef96ad4e4@amd.com"> <div class="moz-cite-prefix">On 7/24/2023 10:43 PM, Alex Deucher wrote: </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> Hi Alex, Thanks for your feedbacks! 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 - 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? </blockquote> 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. Regards, Christian. <blockquote type="cite" cite="mid:f8c546b3-669b-41dc-bc35-c7fef96ad4e4@amd.com"> Best regards, Srini <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> </body> </html>