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

[Christian König]

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.

> 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/20230803/bb1b2a56/attachment-0001.htm>