[PATCH 1/2] drm/sched: Add device pointer to drm_gpu_scheduler

[PATCH 1/2] drm/sched: Add device pointer to drm_gpu_scheduler

[Jiawei Gu]

Add device pointer so scheduler's printing can use
DRM_DEV_ERROR() instead, which makes life easier under multiple GPU

v2: remove drm_sched_init() interface change to avoid compatibility
issue

Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 6 +++---
 include/drm/gpu_scheduler.h            | 1 +
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 3e0bbc797eaa..83bc3fc2ad53 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -489,7 +489,7 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
 			if (r == -ENOENT)
 				drm_sched_job_done(s_job);
 			else if (r)
-				DRM_ERROR("fence add callback failed (%d)\n",
+				DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
 					  r);
 		} else
 			drm_sched_job_done(s_job);
@@ -815,7 +815,7 @@ static int drm_sched_main(void *param)
 			if (r == -ENOENT)
 				drm_sched_job_done(sched_job);
 			else if (r)
-				DRM_ERROR("fence add callback failed (%d)\n",
+				DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
 					  r);
 			dma_fence_put(fence);
 		} else {
@@ -872,7 +872,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 	if (IS_ERR(sched->thread)) {
 		ret = PTR_ERR(sched->thread);
 		sched->thread = NULL;
-		DRM_ERROR("Failed to create scheduler for %s.\n", name);
+		DRM_DEV_ERROR(sched->dev, "Failed to create scheduler for %s.\n", name);
 		return ret;
 	}
 
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index d18af49fd009..2d034c9a9299 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -302,6 +302,7 @@ struct drm_gpu_scheduler {
 	atomic_t                        _score;
 	bool				ready;
 	bool				free_guilty;
+	struct device			*dev;
 };
 
 int drm_sched_init(struct drm_gpu_scheduler *sched,
-- 
2.17.1

[PATCH 2/2] drm/amdgpu: fill scheduler with device ptr

[Jiawei Gu]

Now scheduler contains device ptr. Add it so scheduler printing can be
more reader-friendly under multiple GPU scenario.

Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 4787cb3acaed..da53983c93f9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -506,6 +506,7 @@ int amdgpu_fence_driver_init_ring(struct amdgpu_ring *ring,
 		break;
 	}
 
+	ring->sched.dev = adev->dev;
 	r = drm_sched_init(&ring->sched, &amdgpu_sched_ops,
 			   num_hw_submission, amdgpu_job_hang_limit,
 			   timeout, sched_score, ring->name);
-- 
2.17.1

Re: [PATCH 2/2] drm/amdgpu: fill scheduler with device ptr

[Christian König]

Am 16.02.22 um 08:22 schrieb Jiawei Gu:
> Now scheduler contains device ptr. Add it so scheduler printing can be
> more reader-friendly under multiple GPU scenario.
>
> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> index 4787cb3acaed..da53983c93f9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> @@ -506,6 +506,7 @@ int amdgpu_fence_driver_init_ring(struct amdgpu_ring *ring,
>   		break;
>   	}
>   
> +	ring->sched.dev = adev->dev;

That should probably be a parameter to drm_sched_init() instead and I'm 
not sure what happens in the print when this is NULL.

So make sure to update all other drivers which want to use 
drm_sched_init() as well.

Regards,
Christian.

>   	r = drm_sched_init(&ring->sched, &amdgpu_sched_ops,
>   			   num_hw_submission, amdgpu_job_hang_limit,
>   			   timeout, sched_score, ring->name);

RE: [PATCH 2/2] drm/amdgpu: fill scheduler with device ptr

[Gu, JiaWei (Will)]

[AMD Official Use Only]

Hi Christian,

My same concern is that an additional parameter may affects other drivers which want to use public drm_sched_init(), and I want to reduce the scope of affection.
Will it avoid potential compatibility issues if we keep the interface unchanged and let driver fill device pointer by itself?

And DRM_DEV_ERROR() print is fine with NULL device pointer, there's a NULL pointer check inside of it already.

Best regards,
Jiawei

-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com> 
Sent: Wednesday, February 16, 2022 7:17 PM
To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>; amd-gfx@lists.freedesktop.org; Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>; Liu, Monk <Monk.Liu@amd.com>; Deng, Emily <Emily.Deng@amd.com>; Chen, Horace <Horace.Chen@amd.com>
Subject: Re: [PATCH 2/2] drm/amdgpu: fill scheduler with device ptr



Am 16.02.22 um 08:22 schrieb Jiawei Gu:
> Now scheduler contains device ptr. Add it so scheduler printing can be 
> more reader-friendly under multiple GPU scenario.
>
> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> index 4787cb3acaed..da53983c93f9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> @@ -506,6 +506,7 @@ int amdgpu_fence_driver_init_ring(struct amdgpu_ring *ring,
>   		break;
>   	}
>   
> +	ring->sched.dev = adev->dev;

That should probably be a parameter to drm_sched_init() instead and I'm not sure what happens in the print when this is NULL.

So make sure to update all other drivers which want to use
drm_sched_init() as well.

Regards,
Christian.

>   	r = drm_sched_init(&ring->sched, &amdgpu_sched_ops,
>   			   num_hw_submission, amdgpu_job_hang_limit,
>   			   timeout, sched_score, ring->name);

Re: [PATCH 2/2] drm/amdgpu: fill scheduler with device ptr

[Christian König]

Am 17.02.22 um 03:46 schrieb Gu, JiaWei (Will):
> [AMD Official Use Only]
>
> Hi Christian,
>
> My same concern is that an additional parameter may affects other drivers which want to use public drm_sched_init(), and I want to reduce the scope of affection.
> Will it avoid potential compatibility issues if we keep the interface unchanged and let driver fill device pointer by itself?

No, that will just cause confusion. We keep all drivers in a single 
Linux kernel exactly to avoid stuff like that.

Regards,
Christian.

>
> And DRM_DEV_ERROR() print is fine with NULL device pointer, there's a NULL pointer check inside of it already.
>
> Best regards,
> Jiawei
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Wednesday, February 16, 2022 7:17 PM
> To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>; amd-gfx@lists.freedesktop.org; Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>; Liu, Monk <Monk.Liu@amd.com>; Deng, Emily <Emily.Deng@amd.com>; Chen, Horace <Horace.Chen@amd.com>
> Subject: Re: [PATCH 2/2] drm/amdgpu: fill scheduler with device ptr
>
>
>
> Am 16.02.22 um 08:22 schrieb Jiawei Gu:
>> Now scheduler contains device ptr. Add it so scheduler printing can be
>> more reader-friendly under multiple GPU scenario.
>>
>> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 1 +
>>    1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>> index 4787cb3acaed..da53983c93f9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>> @@ -506,6 +506,7 @@ int amdgpu_fence_driver_init_ring(struct amdgpu_ring *ring,
>>    		break;
>>    	}
>>    
>> +	ring->sched.dev = adev->dev;
> That should probably be a parameter to drm_sched_init() instead and I'm not sure what happens in the print when this is NULL.
>
> So make sure to update all other drivers which want to use
> drm_sched_init() as well.
>
> Regards,
> Christian.
>
>>    	r = drm_sched_init(&ring->sched, &amdgpu_sched_ops,
>>    			   num_hw_submission, amdgpu_job_hang_limit,
>>    			   timeout, sched_score, ring->name);