Re: [PATCH 1/2] drm/amdgpu: use a dummy owner for sysfs triggered cleaner shaders v2

[SRINIVASAN SHANMUGAM <srinivasan.shanmugam@amd.com>]

On 4/3/2025 2:40 PM, Christian König wrote:
> Am 03.04.25 um 05:15 schrieb SRINIVASAN SHANMUGAM:
>> On 4/2/2025 7:32 PM, Christian König wrote:
>>> Otherwise triggering sysfs multiple times without other submissions in
>>> between only runs the shader once.
>>>
>>> v2: add some comment
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 14 +++++++++++---
>>>    1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> index f64675b2ab75..9a24be43e035 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> @@ -1439,9 +1439,11 @@ static int amdgpu_gfx_run_cleaner_shader_job(struct amdgpu_ring *ring)
>>>        struct amdgpu_device *adev = ring->adev;
>>>        struct drm_gpu_scheduler *sched = &ring->sched;
>>>        struct drm_sched_entity entity;
>>> +    static atomic_t counter;
>>>        struct dma_fence *f;
>>>        struct amdgpu_job *job;
>>>        struct amdgpu_ib *ib;
>>> +    void * owner;
>>>        int i, r;
>>>          /* Initialize the scheduler entity */
>>> @@ -1452,9 +1454,15 @@ static int amdgpu_gfx_run_cleaner_shader_job(struct amdgpu_ring *ring)
>>>            goto err;
>>>        }
>>>    -    r = amdgpu_job_alloc_with_ib(ring->adev, &entity, NULL,
>>> -                     64, 0,
>>> -                     &job);
>>> +    /*
>>> +     * Use some unique dummy value as the owner to make sure we execute
>>> +     * the cleaner shader on each submission. The value just need to change
>>> +     * for each submission and is otherwise meaningless.
>>> +     */
>>> +    owner = (unsigned long)atomic_inc_return(&counter),
>> Should this "atomic_inc_return()" needs to be casted to (void*), as it is return type is int?
> No, I had that in the first version of the patch but it's actually sufficient to cast it to a long (which has the same number of bits as void*).
Thus, the use of a static atomic counter allows the driver to pass a 
unique identifier (owner) to each job that triggers the cleaner shader. 
This approach is crucial because it avoids the potential problem of 
running the shader multiple times without other submissions in between 
(since each run requires this unique submission context).

With that casting fixed, this patch is:

Reviewed-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
>
> Regards,
> Christian.
>
>> Regards,
>> Srini
>>
>>> +
>>> +    r = amdgpu_job_alloc_with_ib(ring->adev, &entity, owner,
>>> +                     64, 0, &job);
>>>        if (r)
>>>            goto err;
>>>