[PATCH] drm/amdgpu: Remove volatile from 'wb' & from 'ptr' in amdgpu.h

[SHANMUGAM, SRINIVASAN]

On 8/3/2023 2:12 PM, Christian König wrote:
> Am 03.08.23 um 07:23 schrieb SHANMUGAM, SRINIVASAN:
>>
>>
>> On 7/24/2023 10:43 PM, Alex Deucher wrote:
>>> On Mon, Jul 24, 2023 at 11:54 AM Srinivasan Shanmugam
>>> <srinivasan.shanmugam at amd.com>  wrote:
>>>> 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
>>> 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
>>
>> 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?
>>
>
> 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.
>
Thanks a lot Christian!

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 - https://patchwork.freedesktop.org/patch/551273/ 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?

-Srini

>> Best regards,
>>
>> Srini
>>
>>>> Cc: Christian König<christian.koenig at amd.com>
>>>> Cc: Alex Deucher<alexander.deucher at amd.com>
>>>> Signed-off-by: Srinivasan Shanmugam<srinivasan.shanmugam at amd.com>
>>>> ---
>>>>   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
>>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20230804/f8eb1a52/attachment.htm>