[PATCH 1/2] drm/sched: Set error to s_fence if HW job submission failed.

[PATCH 1/2] drm/sched: Set error to s_fence if HW job submission failed.

[Andrey Grodzovsky]

Problem:
When run_job fails and HW fence returned is NULL we still signal
the s_fence to avoid hangs but the user has no way of knowing if
the actual HW job was ran and finished.

Fix:
Allow .run_job implementations to return ERR_PTR in the fence pointer
returned and then set this error for s_fence->finished fence so whoever
wait on this fence can inspect the signaled fence for an error.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 9a0ee74..f39b97e 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -479,6 +479,7 @@ void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)
 	struct drm_sched_job *s_job, *tmp;
 	uint64_t guilty_context;
 	bool found_guilty = false;
+	struct dma_fence *fence;
 
 	list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) {
 		struct drm_sched_fence *s_fence = s_job->s_fence;
@@ -492,7 +493,16 @@ void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)
 			dma_fence_set_error(&s_fence->finished, -ECANCELED);
 
 		dma_fence_put(s_job->s_fence->parent);
-		s_job->s_fence->parent = sched->ops->run_job(s_job);
+		fence = sched->ops->run_job(s_job);
+
+		if (IS_ERR_OR_NULL(fence)) {
+			s_job->s_fence->parent = NULL;
+			dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
+		} else {
+			s_job->s_fence->parent = fence;
+		}
+
+
 	}
 }
 EXPORT_SYMBOL(drm_sched_resubmit_jobs);
@@ -720,7 +730,7 @@ static int drm_sched_main(void *param)
 		fence = sched->ops->run_job(sched_job);
 		drm_sched_fence_scheduled(s_fence);
 
-		if (fence) {
+		if (!IS_ERR_OR_NULL(fence)) {
 			s_fence->parent = dma_fence_get(fence);
 			r = dma_fence_add_callback(fence, &sched_job->cb,
 						   drm_sched_process_job);
@@ -730,8 +740,11 @@ static int drm_sched_main(void *param)
 				DRM_ERROR("fence add callback failed (%d)\n",
 					  r);
 			dma_fence_put(fence);
-		} else
+		} else {
+
+			dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
 			drm_sched_process_job(NULL, &sched_job->cb);
+		}
 
 		wake_up(&sched->job_scheduled);
 	}
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/sched: Set error to s_fence if HW job submission failed.

[Andrey Grodzovsky]

Problem:
When run_job fails and HW fence returned is NULL we still signal
the s_fence to avoid hangs but the user has no way of knowing if
the actual HW job was ran and finished.

Fix:
Allow .run_job implementations to return ERR_PTR in the fence pointer
returned and then set this error for s_fence->finished fence so whoever
wait on this fence can inspect the signaled fence for an error.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 9a0ee74..f39b97e 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -479,6 +479,7 @@ void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)
 	struct drm_sched_job *s_job, *tmp;
 	uint64_t guilty_context;
 	bool found_guilty = false;
+	struct dma_fence *fence;
 
 	list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) {
 		struct drm_sched_fence *s_fence = s_job->s_fence;
@@ -492,7 +493,16 @@ void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)
 			dma_fence_set_error(&s_fence->finished, -ECANCELED);
 
 		dma_fence_put(s_job->s_fence->parent);
-		s_job->s_fence->parent = sched->ops->run_job(s_job);
+		fence = sched->ops->run_job(s_job);
+
+		if (IS_ERR_OR_NULL(fence)) {
+			s_job->s_fence->parent = NULL;
+			dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
+		} else {
+			s_job->s_fence->parent = fence;
+		}
+
+
 	}
 }
 EXPORT_SYMBOL(drm_sched_resubmit_jobs);
@@ -720,7 +730,7 @@ static int drm_sched_main(void *param)
 		fence = sched->ops->run_job(sched_job);
 		drm_sched_fence_scheduled(s_fence);
 
-		if (fence) {
+		if (!IS_ERR_OR_NULL(fence)) {
 			s_fence->parent = dma_fence_get(fence);
 			r = dma_fence_add_callback(fence, &sched_job->cb,
 						   drm_sched_process_job);
@@ -730,8 +740,11 @@ static int drm_sched_main(void *param)
 				DRM_ERROR("fence add callback failed (%d)\n",
 					  r);
 			dma_fence_put(fence);
-		} else
+		} else {
+
+			dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
 			drm_sched_process_job(NULL, &sched_job->cb);
+		}
 
 		wake_up(&sched->job_scheduled);
 	}
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/sched: Set error to s_fence if HW job submission failed.

[Andrey Grodzovsky]

Problem:
When run_job fails and HW fence returned is NULL we still signal
the s_fence to avoid hangs but the user has no way of knowing if
the actual HW job was ran and finished.

Fix:
Allow .run_job implementations to return ERR_PTR in the fence pointer
returned and then set this error for s_fence->finished fence so whoever
wait on this fence can inspect the signaled fence for an error.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 9a0ee74..f39b97e 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -479,6 +479,7 @@ void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)
 	struct drm_sched_job *s_job, *tmp;
 	uint64_t guilty_context;
 	bool found_guilty = false;
+	struct dma_fence *fence;
 
 	list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) {
 		struct drm_sched_fence *s_fence = s_job->s_fence;
@@ -492,7 +493,16 @@ void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)
 			dma_fence_set_error(&s_fence->finished, -ECANCELED);
 
 		dma_fence_put(s_job->s_fence->parent);
-		s_job->s_fence->parent = sched->ops->run_job(s_job);
+		fence = sched->ops->run_job(s_job);
+
+		if (IS_ERR_OR_NULL(fence)) {
+			s_job->s_fence->parent = NULL;
+			dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
+		} else {
+			s_job->s_fence->parent = fence;
+		}
+
+
 	}
 }
 EXPORT_SYMBOL(drm_sched_resubmit_jobs);
@@ -720,7 +730,7 @@ static int drm_sched_main(void *param)
 		fence = sched->ops->run_job(sched_job);
 		drm_sched_fence_scheduled(s_fence);
 
-		if (fence) {
+		if (!IS_ERR_OR_NULL(fence)) {
 			s_fence->parent = dma_fence_get(fence);
 			r = dma_fence_add_callback(fence, &sched_job->cb,
 						   drm_sched_process_job);
@@ -730,8 +740,11 @@ static int drm_sched_main(void *param)
 				DRM_ERROR("fence add callback failed (%d)\n",
 					  r);
 			dma_fence_put(fence);
-		} else
+		} else {
+
+			dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
 			drm_sched_process_job(NULL, &sched_job->cb);
+		}
 
 		wake_up(&sched->job_scheduled);
 	}
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amdgpu: If amdgpu_ib_schedule fails return back the error.

[Andrey Grodzovsky]

Use ERR_PTR to return back the error happened during amdgpu_ib_schedule.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index e1bad99..9769bb5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -243,6 +243,8 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
 	job->fence = dma_fence_get(fence);
 
 	amdgpu_job_free_resources(job);
+
+	fence = r ? ERR_PTR(r) : fence;
 	return fence;
 }
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/amdgpu: If amdgpu_ib_schedule fails return back the error.

[Andrey Grodzovsky]

Use ERR_PTR to return back the error happened during amdgpu_ib_schedule.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index e1bad99..9769bb5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -243,6 +243,8 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
 	job->fence = dma_fence_get(fence);
 
 	amdgpu_job_free_resources(job);
+
+	fence = r ? ERR_PTR(r) : fence;
 	return fence;
 }
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/amdgpu: If amdgpu_ib_schedule fails return back the error.

[Andrey Grodzovsky]

Use ERR_PTR to return back the error happened during amdgpu_ib_schedule.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index e1bad99..9769bb5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -243,6 +243,8 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
 	job->fence = dma_fence_get(fence);
 
 	amdgpu_job_free_resources(job);
+
+	fence = r ? ERR_PTR(r) : fence;
 	return fence;
 }
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/sched: Set error to s_fence if HW job submission failed.

[Christian König]

Am 24.10.19 um 21:57 schrieb Andrey Grodzovsky:
> Problem:
> When run_job fails and HW fence returned is NULL we still signal
> the s_fence to avoid hangs but the user has no way of knowing if
> the actual HW job was ran and finished.
>
> Fix:
> Allow .run_job implementations to return ERR_PTR in the fence pointer
> returned and then set this error for s_fence->finished fence so whoever
> wait on this fence can inspect the signaled fence for an error.
>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>   drivers/gpu/drm/scheduler/sched_main.c | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 9a0ee74..f39b97e 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -479,6 +479,7 @@ void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)
>   	struct drm_sched_job *s_job, *tmp;
>   	uint64_t guilty_context;
>   	bool found_guilty = false;
> +	struct dma_fence *fence;
>   
>   	list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) {
>   		struct drm_sched_fence *s_fence = s_job->s_fence;
> @@ -492,7 +493,16 @@ void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)
>   			dma_fence_set_error(&s_fence->finished, -ECANCELED);
>   
>   		dma_fence_put(s_job->s_fence->parent);
> -		s_job->s_fence->parent = sched->ops->run_job(s_job);
> +		fence = sched->ops->run_job(s_job);
> +
> +		if (IS_ERR_OR_NULL(fence)) {
> +			s_job->s_fence->parent = NULL;
> +			dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
> +		} else {
> +			s_job->s_fence->parent = fence;
> +		}
> +
> +

Maybe time for a drm_sched_run_job() function which does that handling? 
And why don't we need to install the callback here?

Apart from that looks good to me,
Christian.

>   	}
>   }
>   EXPORT_SYMBOL(drm_sched_resubmit_jobs);
> @@ -720,7 +730,7 @@ static int drm_sched_main(void *param)
>   		fence = sched->ops->run_job(sched_job);
>   		drm_sched_fence_scheduled(s_fence);
>   
> -		if (fence) {
> +		if (!IS_ERR_OR_NULL(fence)) {
>   			s_fence->parent = dma_fence_get(fence);
>   			r = dma_fence_add_callback(fence, &sched_job->cb,
>   						   drm_sched_process_job);
> @@ -730,8 +740,11 @@ static int drm_sched_main(void *param)
>   				DRM_ERROR("fence add callback failed (%d)\n",
>   					  r);
>   			dma_fence_put(fence);
> -		} else
> +		} else {
> +
> +			dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>   			drm_sched_process_job(NULL, &sched_job->cb);
> +		}
>   
>   		wake_up(&sched->job_scheduled);
>   	}

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/sched: Set error to s_fence if HW job submission failed.

[Christian König]

Am 24.10.19 um 21:57 schrieb Andrey Grodzovsky:
> Problem:
> When run_job fails and HW fence returned is NULL we still signal
> the s_fence to avoid hangs but the user has no way of knowing if
> the actual HW job was ran and finished.
>
> Fix:
> Allow .run_job implementations to return ERR_PTR in the fence pointer
> returned and then set this error for s_fence->finished fence so whoever
> wait on this fence can inspect the signaled fence for an error.
>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>   drivers/gpu/drm/scheduler/sched_main.c | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 9a0ee74..f39b97e 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -479,6 +479,7 @@ void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)
>   	struct drm_sched_job *s_job, *tmp;
>   	uint64_t guilty_context;
>   	bool found_guilty = false;
> +	struct dma_fence *fence;
>   
>   	list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) {
>   		struct drm_sched_fence *s_fence = s_job->s_fence;
> @@ -492,7 +493,16 @@ void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)
>   			dma_fence_set_error(&s_fence->finished, -ECANCELED);
>   
>   		dma_fence_put(s_job->s_fence->parent);
> -		s_job->s_fence->parent = sched->ops->run_job(s_job);
> +		fence = sched->ops->run_job(s_job);
> +
> +		if (IS_ERR_OR_NULL(fence)) {
> +			s_job->s_fence->parent = NULL;
> +			dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
> +		} else {
> +			s_job->s_fence->parent = fence;
> +		}
> +
> +

Maybe time for a drm_sched_run_job() function which does that handling? 
And why don't we need to install the callback here?

Apart from that looks good to me,
Christian.

>   	}
>   }
>   EXPORT_SYMBOL(drm_sched_resubmit_jobs);
> @@ -720,7 +730,7 @@ static int drm_sched_main(void *param)
>   		fence = sched->ops->run_job(sched_job);
>   		drm_sched_fence_scheduled(s_fence);
>   
> -		if (fence) {
> +		if (!IS_ERR_OR_NULL(fence)) {
>   			s_fence->parent = dma_fence_get(fence);
>   			r = dma_fence_add_callback(fence, &sched_job->cb,
>   						   drm_sched_process_job);
> @@ -730,8 +740,11 @@ static int drm_sched_main(void *param)
>   				DRM_ERROR("fence add callback failed (%d)\n",
>   					  r);
>   			dma_fence_put(fence);
> -		} else
> +		} else {
> +
> +			dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>   			drm_sched_process_job(NULL, &sched_job->cb);
> +		}
>   
>   		wake_up(&sched->job_scheduled);
>   	}

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/sched: Set error to s_fence if HW job submission failed.

[Christian König]

Am 24.10.19 um 21:57 schrieb Andrey Grodzovsky:
> Problem:
> When run_job fails and HW fence returned is NULL we still signal
> the s_fence to avoid hangs but the user has no way of knowing if
> the actual HW job was ran and finished.
>
> Fix:
> Allow .run_job implementations to return ERR_PTR in the fence pointer
> returned and then set this error for s_fence->finished fence so whoever
> wait on this fence can inspect the signaled fence for an error.
>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>   drivers/gpu/drm/scheduler/sched_main.c | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 9a0ee74..f39b97e 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -479,6 +479,7 @@ void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)
>   	struct drm_sched_job *s_job, *tmp;
>   	uint64_t guilty_context;
>   	bool found_guilty = false;
> +	struct dma_fence *fence;
>   
>   	list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) {
>   		struct drm_sched_fence *s_fence = s_job->s_fence;
> @@ -492,7 +493,16 @@ void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)
>   			dma_fence_set_error(&s_fence->finished, -ECANCELED);
>   
>   		dma_fence_put(s_job->s_fence->parent);
> -		s_job->s_fence->parent = sched->ops->run_job(s_job);
> +		fence = sched->ops->run_job(s_job);
> +
> +		if (IS_ERR_OR_NULL(fence)) {
> +			s_job->s_fence->parent = NULL;
> +			dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
> +		} else {
> +			s_job->s_fence->parent = fence;
> +		}
> +
> +

Maybe time for a drm_sched_run_job() function which does that handling? 
And why don't we need to install the callback here?

Apart from that looks good to me,
Christian.

>   	}
>   }
>   EXPORT_SYMBOL(drm_sched_resubmit_jobs);
> @@ -720,7 +730,7 @@ static int drm_sched_main(void *param)
>   		fence = sched->ops->run_job(sched_job);
>   		drm_sched_fence_scheduled(s_fence);
>   
> -		if (fence) {
> +		if (!IS_ERR_OR_NULL(fence)) {
>   			s_fence->parent = dma_fence_get(fence);
>   			r = dma_fence_add_callback(fence, &sched_job->cb,
>   						   drm_sched_process_job);
> @@ -730,8 +740,11 @@ static int drm_sched_main(void *param)
>   				DRM_ERROR("fence add callback failed (%d)\n",
>   					  r);
>   			dma_fence_put(fence);
> -		} else
> +		} else {
> +
> +			dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>   			drm_sched_process_job(NULL, &sched_job->cb);
> +		}
>   
>   		wake_up(&sched->job_scheduled);
>   	}

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/sched: Set error to s_fence if HW job submission failed.

[Grodzovsky, Andrey]

On 10/25/19 4:44 AM, Christian König wrote:
> Am 24.10.19 um 21:57 schrieb Andrey Grodzovsky:
>> Problem:
>> When run_job fails and HW fence returned is NULL we still signal
>> the s_fence to avoid hangs but the user has no way of knowing if
>> the actual HW job was ran and finished.
>>
>> Fix:
>> Allow .run_job implementations to return ERR_PTR in the fence pointer
>> returned and then set this error for s_fence->finished fence so whoever
>> wait on this fence can inspect the signaled fence for an error.
>>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> ---
>>   drivers/gpu/drm/scheduler/sched_main.c | 19 ++++++++++++++++---
>>   1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index 9a0ee74..f39b97e 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -479,6 +479,7 @@ void drm_sched_resubmit_jobs(struct 
>> drm_gpu_scheduler *sched)
>>       struct drm_sched_job *s_job, *tmp;
>>       uint64_t guilty_context;
>>       bool found_guilty = false;
>> +    struct dma_fence *fence;
>>         list_for_each_entry_safe(s_job, tmp, 
>> &sched->ring_mirror_list, node) {
>>           struct drm_sched_fence *s_fence = s_job->s_fence;
>> @@ -492,7 +493,16 @@ void drm_sched_resubmit_jobs(struct 
>> drm_gpu_scheduler *sched)
>>               dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>             dma_fence_put(s_job->s_fence->parent);
>> -        s_job->s_fence->parent = sched->ops->run_job(s_job);
>> +        fence = sched->ops->run_job(s_job);
>> +
>> +        if (IS_ERR_OR_NULL(fence)) {
>> +            s_job->s_fence->parent = NULL;
>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>> +        } else {
>> +            s_job->s_fence->parent = fence;
>> +        }
>> +
>> +
>
> Maybe time for a drm_sched_run_job() function which does that 
> handling? And why don't we need to install the callback here?


What code do you want to put in drm_sched_run_job ?

We reinstall the callback later in drm_sched_start, 
drm_sched_resubmit_jobs is conditional on whether the guilty fence did 
signal by this time or not and so the split of the logic into 
drm_sched_start and drm_sched_resubmit_jobs.

Andrey


>
> Apart from that looks good to me,
> Christian.
>
>>       }
>>   }
>>   EXPORT_SYMBOL(drm_sched_resubmit_jobs);
>> @@ -720,7 +730,7 @@ static int drm_sched_main(void *param)
>>           fence = sched->ops->run_job(sched_job);
>>           drm_sched_fence_scheduled(s_fence);
>>   -        if (fence) {
>> +        if (!IS_ERR_OR_NULL(fence)) {
>>               s_fence->parent = dma_fence_get(fence);
>>               r = dma_fence_add_callback(fence, &sched_job->cb,
>>                              drm_sched_process_job);
>> @@ -730,8 +740,11 @@ static int drm_sched_main(void *param)
>>                   DRM_ERROR("fence add callback failed (%d)\n",
>>                         r);
>>               dma_fence_put(fence);
>> -        } else
>> +        } else {
>> +
>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>               drm_sched_process_job(NULL, &sched_job->cb);
>> +        }
>>             wake_up(&sched->job_scheduled);
>>       }
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/sched: Set error to s_fence if HW job submission failed.

[Grodzovsky, Andrey]

On 10/25/19 4:44 AM, Christian König wrote:
> Am 24.10.19 um 21:57 schrieb Andrey Grodzovsky:
>> Problem:
>> When run_job fails and HW fence returned is NULL we still signal
>> the s_fence to avoid hangs but the user has no way of knowing if
>> the actual HW job was ran and finished.
>>
>> Fix:
>> Allow .run_job implementations to return ERR_PTR in the fence pointer
>> returned and then set this error for s_fence->finished fence so whoever
>> wait on this fence can inspect the signaled fence for an error.
>>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> ---
>>   drivers/gpu/drm/scheduler/sched_main.c | 19 ++++++++++++++++---
>>   1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index 9a0ee74..f39b97e 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -479,6 +479,7 @@ void drm_sched_resubmit_jobs(struct 
>> drm_gpu_scheduler *sched)
>>       struct drm_sched_job *s_job, *tmp;
>>       uint64_t guilty_context;
>>       bool found_guilty = false;
>> +    struct dma_fence *fence;
>>         list_for_each_entry_safe(s_job, tmp, 
>> &sched->ring_mirror_list, node) {
>>           struct drm_sched_fence *s_fence = s_job->s_fence;
>> @@ -492,7 +493,16 @@ void drm_sched_resubmit_jobs(struct 
>> drm_gpu_scheduler *sched)
>>               dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>             dma_fence_put(s_job->s_fence->parent);
>> -        s_job->s_fence->parent = sched->ops->run_job(s_job);
>> +        fence = sched->ops->run_job(s_job);
>> +
>> +        if (IS_ERR_OR_NULL(fence)) {
>> +            s_job->s_fence->parent = NULL;
>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>> +        } else {
>> +            s_job->s_fence->parent = fence;
>> +        }
>> +
>> +
>
> Maybe time for a drm_sched_run_job() function which does that 
> handling? And why don't we need to install the callback here?


What code do you want to put in drm_sched_run_job ?

We reinstall the callback later in drm_sched_start, 
drm_sched_resubmit_jobs is conditional on whether the guilty fence did 
signal by this time or not and so the split of the logic into 
drm_sched_start and drm_sched_resubmit_jobs.

Andrey


>
> Apart from that looks good to me,
> Christian.
>
>>       }
>>   }
>>   EXPORT_SYMBOL(drm_sched_resubmit_jobs);
>> @@ -720,7 +730,7 @@ static int drm_sched_main(void *param)
>>           fence = sched->ops->run_job(sched_job);
>>           drm_sched_fence_scheduled(s_fence);
>>   -        if (fence) {
>> +        if (!IS_ERR_OR_NULL(fence)) {
>>               s_fence->parent = dma_fence_get(fence);
>>               r = dma_fence_add_callback(fence, &sched_job->cb,
>>                              drm_sched_process_job);
>> @@ -730,8 +740,11 @@ static int drm_sched_main(void *param)
>>                   DRM_ERROR("fence add callback failed (%d)\n",
>>                         r);
>>               dma_fence_put(fence);
>> -        } else
>> +        } else {
>> +
>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>               drm_sched_process_job(NULL, &sched_job->cb);
>> +        }
>>             wake_up(&sched->job_scheduled);
>>       }
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/sched: Set error to s_fence if HW job submission failed.

[Koenig, Christian]

Am 25.10.19 um 16:57 schrieb Grodzovsky, Andrey:
> On 10/25/19 4:44 AM, Christian König wrote:
>> Am 24.10.19 um 21:57 schrieb Andrey Grodzovsky:
>>> Problem:
>>> When run_job fails and HW fence returned is NULL we still signal
>>> the s_fence to avoid hangs but the user has no way of knowing if
>>> the actual HW job was ran and finished.
>>>
>>> Fix:
>>> Allow .run_job implementations to return ERR_PTR in the fence pointer
>>> returned and then set this error for s_fence->finished fence so whoever
>>> wait on this fence can inspect the signaled fence for an error.
>>>
>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>> ---
>>>    drivers/gpu/drm/scheduler/sched_main.c | 19 ++++++++++++++++---
>>>    1 file changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index 9a0ee74..f39b97e 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -479,6 +479,7 @@ void drm_sched_resubmit_jobs(struct
>>> drm_gpu_scheduler *sched)
>>>        struct drm_sched_job *s_job, *tmp;
>>>        uint64_t guilty_context;
>>>        bool found_guilty = false;
>>> +    struct dma_fence *fence;
>>>          list_for_each_entry_safe(s_job, tmp,
>>> &sched->ring_mirror_list, node) {
>>>            struct drm_sched_fence *s_fence = s_job->s_fence;
>>> @@ -492,7 +493,16 @@ void drm_sched_resubmit_jobs(struct
>>> drm_gpu_scheduler *sched)
>>>                dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>>              dma_fence_put(s_job->s_fence->parent);
>>> -        s_job->s_fence->parent = sched->ops->run_job(s_job);
>>> +        fence = sched->ops->run_job(s_job);
>>> +
>>> +        if (IS_ERR_OR_NULL(fence)) {
>>> +            s_job->s_fence->parent = NULL;
>>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>> +        } else {
>>> +            s_job->s_fence->parent = fence;
>>> +        }
>>> +
>>> +
>> Maybe time for a drm_sched_run_job() function which does that
>> handling? And why don't we need to install the callback here?
>
> What code do you want to put in drm_sched_run_job ?
>
> We reinstall the callback later in drm_sched_start,
> drm_sched_resubmit_jobs is conditional on whether the guilty fence did
> signal by this time or not and so the split of the logic into
> drm_sched_start and drm_sched_resubmit_jobs.

Ah, yes of course. In this case the patch is Reviewed-by: Christian 
König <christian.koenig@amd.com>.

Regards,
Christian.

>
> Andrey
>
>
>> Apart from that looks good to me,
>> Christian.
>>
>>>        }
>>>    }
>>>    EXPORT_SYMBOL(drm_sched_resubmit_jobs);
>>> @@ -720,7 +730,7 @@ static int drm_sched_main(void *param)
>>>            fence = sched->ops->run_job(sched_job);
>>>            drm_sched_fence_scheduled(s_fence);
>>>    -        if (fence) {
>>> +        if (!IS_ERR_OR_NULL(fence)) {
>>>                s_fence->parent = dma_fence_get(fence);
>>>                r = dma_fence_add_callback(fence, &sched_job->cb,
>>>                               drm_sched_process_job);
>>> @@ -730,8 +740,11 @@ static int drm_sched_main(void *param)
>>>                    DRM_ERROR("fence add callback failed (%d)\n",
>>>                          r);
>>>                dma_fence_put(fence);
>>> -        } else
>>> +        } else {
>>> +
>>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>>                drm_sched_process_job(NULL, &sched_job->cb);
>>> +        }
>>>              wake_up(&sched->job_scheduled);
>>>        }

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/sched: Set error to s_fence if HW job submission failed.

[Koenig, Christian]

Am 25.10.19 um 16:57 schrieb Grodzovsky, Andrey:
> On 10/25/19 4:44 AM, Christian König wrote:
>> Am 24.10.19 um 21:57 schrieb Andrey Grodzovsky:
>>> Problem:
>>> When run_job fails and HW fence returned is NULL we still signal
>>> the s_fence to avoid hangs but the user has no way of knowing if
>>> the actual HW job was ran and finished.
>>>
>>> Fix:
>>> Allow .run_job implementations to return ERR_PTR in the fence pointer
>>> returned and then set this error for s_fence->finished fence so whoever
>>> wait on this fence can inspect the signaled fence for an error.
>>>
>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>> ---
>>>    drivers/gpu/drm/scheduler/sched_main.c | 19 ++++++++++++++++---
>>>    1 file changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index 9a0ee74..f39b97e 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -479,6 +479,7 @@ void drm_sched_resubmit_jobs(struct
>>> drm_gpu_scheduler *sched)
>>>        struct drm_sched_job *s_job, *tmp;
>>>        uint64_t guilty_context;
>>>        bool found_guilty = false;
>>> +    struct dma_fence *fence;
>>>          list_for_each_entry_safe(s_job, tmp,
>>> &sched->ring_mirror_list, node) {
>>>            struct drm_sched_fence *s_fence = s_job->s_fence;
>>> @@ -492,7 +493,16 @@ void drm_sched_resubmit_jobs(struct
>>> drm_gpu_scheduler *sched)
>>>                dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>>              dma_fence_put(s_job->s_fence->parent);
>>> -        s_job->s_fence->parent = sched->ops->run_job(s_job);
>>> +        fence = sched->ops->run_job(s_job);
>>> +
>>> +        if (IS_ERR_OR_NULL(fence)) {
>>> +            s_job->s_fence->parent = NULL;
>>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>> +        } else {
>>> +            s_job->s_fence->parent = fence;
>>> +        }
>>> +
>>> +
>> Maybe time for a drm_sched_run_job() function which does that
>> handling? And why don't we need to install the callback here?
>
> What code do you want to put in drm_sched_run_job ?
>
> We reinstall the callback later in drm_sched_start,
> drm_sched_resubmit_jobs is conditional on whether the guilty fence did
> signal by this time or not and so the split of the logic into
> drm_sched_start and drm_sched_resubmit_jobs.

Ah, yes of course. In this case the patch is Reviewed-by: Christian 
König <christian.koenig@amd.com>.

Regards,
Christian.

>
> Andrey
>
>
>> Apart from that looks good to me,
>> Christian.
>>
>>>        }
>>>    }
>>>    EXPORT_SYMBOL(drm_sched_resubmit_jobs);
>>> @@ -720,7 +730,7 @@ static int drm_sched_main(void *param)
>>>            fence = sched->ops->run_job(sched_job);
>>>            drm_sched_fence_scheduled(s_fence);
>>>    -        if (fence) {
>>> +        if (!IS_ERR_OR_NULL(fence)) {
>>>                s_fence->parent = dma_fence_get(fence);
>>>                r = dma_fence_add_callback(fence, &sched_job->cb,
>>>                               drm_sched_process_job);
>>> @@ -730,8 +740,11 @@ static int drm_sched_main(void *param)
>>>                    DRM_ERROR("fence add callback failed (%d)\n",
>>>                          r);
>>>                dma_fence_put(fence);
>>> -        } else
>>> +        } else {
>>> +
>>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>>                drm_sched_process_job(NULL, &sched_job->cb);
>>> +        }
>>>              wake_up(&sched->job_scheduled);
>>>        }

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/sched: Set error to s_fence if HW job submission failed.

[Koenig, Christian]

Am 25.10.19 um 16:57 schrieb Grodzovsky, Andrey:
> On 10/25/19 4:44 AM, Christian König wrote:
>> Am 24.10.19 um 21:57 schrieb Andrey Grodzovsky:
>>> Problem:
>>> When run_job fails and HW fence returned is NULL we still signal
>>> the s_fence to avoid hangs but the user has no way of knowing if
>>> the actual HW job was ran and finished.
>>>
>>> Fix:
>>> Allow .run_job implementations to return ERR_PTR in the fence pointer
>>> returned and then set this error for s_fence->finished fence so whoever
>>> wait on this fence can inspect the signaled fence for an error.
>>>
>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>> ---
>>>    drivers/gpu/drm/scheduler/sched_main.c | 19 ++++++++++++++++---
>>>    1 file changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index 9a0ee74..f39b97e 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -479,6 +479,7 @@ void drm_sched_resubmit_jobs(struct
>>> drm_gpu_scheduler *sched)
>>>        struct drm_sched_job *s_job, *tmp;
>>>        uint64_t guilty_context;
>>>        bool found_guilty = false;
>>> +    struct dma_fence *fence;
>>>          list_for_each_entry_safe(s_job, tmp,
>>> &sched->ring_mirror_list, node) {
>>>            struct drm_sched_fence *s_fence = s_job->s_fence;
>>> @@ -492,7 +493,16 @@ void drm_sched_resubmit_jobs(struct
>>> drm_gpu_scheduler *sched)
>>>                dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>>              dma_fence_put(s_job->s_fence->parent);
>>> -        s_job->s_fence->parent = sched->ops->run_job(s_job);
>>> +        fence = sched->ops->run_job(s_job);
>>> +
>>> +        if (IS_ERR_OR_NULL(fence)) {
>>> +            s_job->s_fence->parent = NULL;
>>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>> +        } else {
>>> +            s_job->s_fence->parent = fence;
>>> +        }
>>> +
>>> +
>> Maybe time for a drm_sched_run_job() function which does that
>> handling? And why don't we need to install the callback here?
>
> What code do you want to put in drm_sched_run_job ?
>
> We reinstall the callback later in drm_sched_start,
> drm_sched_resubmit_jobs is conditional on whether the guilty fence did
> signal by this time or not and so the split of the logic into
> drm_sched_start and drm_sched_resubmit_jobs.

Ah, yes of course. In this case the patch is Reviewed-by: Christian 
König <christian.koenig@amd.com>.

Regards,
Christian.

>
> Andrey
>
>
>> Apart from that looks good to me,
>> Christian.
>>
>>>        }
>>>    }
>>>    EXPORT_SYMBOL(drm_sched_resubmit_jobs);
>>> @@ -720,7 +730,7 @@ static int drm_sched_main(void *param)
>>>            fence = sched->ops->run_job(sched_job);
>>>            drm_sched_fence_scheduled(s_fence);
>>>    -        if (fence) {
>>> +        if (!IS_ERR_OR_NULL(fence)) {
>>>                s_fence->parent = dma_fence_get(fence);
>>>                r = dma_fence_add_callback(fence, &sched_job->cb,
>>>                               drm_sched_process_job);
>>> @@ -730,8 +740,11 @@ static int drm_sched_main(void *param)
>>>                    DRM_ERROR("fence add callback failed (%d)\n",
>>>                          r);
>>>                dma_fence_put(fence);
>>> -        } else
>>> +        } else {
>>> +
>>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>>                drm_sched_process_job(NULL, &sched_job->cb);
>>> +        }
>>>              wake_up(&sched->job_scheduled);
>>>        }

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/sched: Set error to s_fence if HW job submission failed.

[Grodzovsky, Andrey]

On 10/25/19 11:55 AM, Koenig, Christian wrote:
> Am 25.10.19 um 16:57 schrieb Grodzovsky, Andrey:
>> On 10/25/19 4:44 AM, Christian König wrote:
>>> Am 24.10.19 um 21:57 schrieb Andrey Grodzovsky:
>>>> Problem:
>>>> When run_job fails and HW fence returned is NULL we still signal
>>>> the s_fence to avoid hangs but the user has no way of knowing if
>>>> the actual HW job was ran and finished.
>>>>
>>>> Fix:
>>>> Allow .run_job implementations to return ERR_PTR in the fence pointer
>>>> returned and then set this error for s_fence->finished fence so whoever
>>>> wait on this fence can inspect the signaled fence for an error.
>>>>
>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>> ---
>>>>     drivers/gpu/drm/scheduler/sched_main.c | 19 ++++++++++++++++---
>>>>     1 file changed, 16 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>> index 9a0ee74..f39b97e 100644
>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>> @@ -479,6 +479,7 @@ void drm_sched_resubmit_jobs(struct
>>>> drm_gpu_scheduler *sched)
>>>>         struct drm_sched_job *s_job, *tmp;
>>>>         uint64_t guilty_context;
>>>>         bool found_guilty = false;
>>>> +    struct dma_fence *fence;
>>>>           list_for_each_entry_safe(s_job, tmp,
>>>> &sched->ring_mirror_list, node) {
>>>>             struct drm_sched_fence *s_fence = s_job->s_fence;
>>>> @@ -492,7 +493,16 @@ void drm_sched_resubmit_jobs(struct
>>>> drm_gpu_scheduler *sched)
>>>>                 dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>>>               dma_fence_put(s_job->s_fence->parent);
>>>> -        s_job->s_fence->parent = sched->ops->run_job(s_job);
>>>> +        fence = sched->ops->run_job(s_job);
>>>> +
>>>> +        if (IS_ERR_OR_NULL(fence)) {
>>>> +            s_job->s_fence->parent = NULL;
>>>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>>> +        } else {
>>>> +            s_job->s_fence->parent = fence;
>>>> +        }
>>>> +
>>>> +
>>> Maybe time for a drm_sched_run_job() function which does that
>>> handling? And why don't we need to install the callback here?
>> What code do you want to put in drm_sched_run_job ?
>>
>> We reinstall the callback later in drm_sched_start,
>> drm_sched_resubmit_jobs is conditional on whether the guilty fence did
>> signal by this time or not and so the split of the logic into
>> drm_sched_start and drm_sched_resubmit_jobs.
> Ah, yes of course. In this case the patch is Reviewed-by: Christian
> König <christian.koenig@amd.com>.
>
> Regards,
> Christian.


Thanks, there is also 2/2 patch for amdgpu, please take a look.

Andrey


>
>> Andrey
>>
>>
>>> Apart from that looks good to me,
>>> Christian.
>>>
>>>>         }
>>>>     }
>>>>     EXPORT_SYMBOL(drm_sched_resubmit_jobs);
>>>> @@ -720,7 +730,7 @@ static int drm_sched_main(void *param)
>>>>             fence = sched->ops->run_job(sched_job);
>>>>             drm_sched_fence_scheduled(s_fence);
>>>>     -        if (fence) {
>>>> +        if (!IS_ERR_OR_NULL(fence)) {
>>>>                 s_fence->parent = dma_fence_get(fence);
>>>>                 r = dma_fence_add_callback(fence, &sched_job->cb,
>>>>                                drm_sched_process_job);
>>>> @@ -730,8 +740,11 @@ static int drm_sched_main(void *param)
>>>>                     DRM_ERROR("fence add callback failed (%d)\n",
>>>>                           r);
>>>>                 dma_fence_put(fence);
>>>> -        } else
>>>> +        } else {
>>>> +
>>>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>>>                 drm_sched_process_job(NULL, &sched_job->cb);
>>>> +        }
>>>>               wake_up(&sched->job_scheduled);
>>>>         }
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/sched: Set error to s_fence if HW job submission failed.

[Grodzovsky, Andrey]

On 10/25/19 11:55 AM, Koenig, Christian wrote:
> Am 25.10.19 um 16:57 schrieb Grodzovsky, Andrey:
>> On 10/25/19 4:44 AM, Christian König wrote:
>>> Am 24.10.19 um 21:57 schrieb Andrey Grodzovsky:
>>>> Problem:
>>>> When run_job fails and HW fence returned is NULL we still signal
>>>> the s_fence to avoid hangs but the user has no way of knowing if
>>>> the actual HW job was ran and finished.
>>>>
>>>> Fix:
>>>> Allow .run_job implementations to return ERR_PTR in the fence pointer
>>>> returned and then set this error for s_fence->finished fence so whoever
>>>> wait on this fence can inspect the signaled fence for an error.
>>>>
>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>> ---
>>>>     drivers/gpu/drm/scheduler/sched_main.c | 19 ++++++++++++++++---
>>>>     1 file changed, 16 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>> index 9a0ee74..f39b97e 100644
>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>> @@ -479,6 +479,7 @@ void drm_sched_resubmit_jobs(struct
>>>> drm_gpu_scheduler *sched)
>>>>         struct drm_sched_job *s_job, *tmp;
>>>>         uint64_t guilty_context;
>>>>         bool found_guilty = false;
>>>> +    struct dma_fence *fence;
>>>>           list_for_each_entry_safe(s_job, tmp,
>>>> &sched->ring_mirror_list, node) {
>>>>             struct drm_sched_fence *s_fence = s_job->s_fence;
>>>> @@ -492,7 +493,16 @@ void drm_sched_resubmit_jobs(struct
>>>> drm_gpu_scheduler *sched)
>>>>                 dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>>>               dma_fence_put(s_job->s_fence->parent);
>>>> -        s_job->s_fence->parent = sched->ops->run_job(s_job);
>>>> +        fence = sched->ops->run_job(s_job);
>>>> +
>>>> +        if (IS_ERR_OR_NULL(fence)) {
>>>> +            s_job->s_fence->parent = NULL;
>>>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>>> +        } else {
>>>> +            s_job->s_fence->parent = fence;
>>>> +        }
>>>> +
>>>> +
>>> Maybe time for a drm_sched_run_job() function which does that
>>> handling? And why don't we need to install the callback here?
>> What code do you want to put in drm_sched_run_job ?
>>
>> We reinstall the callback later in drm_sched_start,
>> drm_sched_resubmit_jobs is conditional on whether the guilty fence did
>> signal by this time or not and so the split of the logic into
>> drm_sched_start and drm_sched_resubmit_jobs.
> Ah, yes of course. In this case the patch is Reviewed-by: Christian
> König <christian.koenig@amd.com>.
>
> Regards,
> Christian.


Thanks, there is also 2/2 patch for amdgpu, please take a look.

Andrey


>
>> Andrey
>>
>>
>>> Apart from that looks good to me,
>>> Christian.
>>>
>>>>         }
>>>>     }
>>>>     EXPORT_SYMBOL(drm_sched_resubmit_jobs);
>>>> @@ -720,7 +730,7 @@ static int drm_sched_main(void *param)
>>>>             fence = sched->ops->run_job(sched_job);
>>>>             drm_sched_fence_scheduled(s_fence);
>>>>     -        if (fence) {
>>>> +        if (!IS_ERR_OR_NULL(fence)) {
>>>>                 s_fence->parent = dma_fence_get(fence);
>>>>                 r = dma_fence_add_callback(fence, &sched_job->cb,
>>>>                                drm_sched_process_job);
>>>> @@ -730,8 +740,11 @@ static int drm_sched_main(void *param)
>>>>                     DRM_ERROR("fence add callback failed (%d)\n",
>>>>                           r);
>>>>                 dma_fence_put(fence);
>>>> -        } else
>>>> +        } else {
>>>> +
>>>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>>>                 drm_sched_process_job(NULL, &sched_job->cb);
>>>> +        }
>>>>               wake_up(&sched->job_scheduled);
>>>>         }
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/sched: Set error to s_fence if HW job submission failed.

[Koenig, Christian]

Am 25.10.19 um 17:56 schrieb Grodzovsky, Andrey:
> On 10/25/19 11:55 AM, Koenig, Christian wrote:
>> Am 25.10.19 um 16:57 schrieb Grodzovsky, Andrey:
>>> On 10/25/19 4:44 AM, Christian König wrote:
>>>> Am 24.10.19 um 21:57 schrieb Andrey Grodzovsky:
>>>>> Problem:
>>>>> When run_job fails and HW fence returned is NULL we still signal
>>>>> the s_fence to avoid hangs but the user has no way of knowing if
>>>>> the actual HW job was ran and finished.
>>>>>
>>>>> Fix:
>>>>> Allow .run_job implementations to return ERR_PTR in the fence pointer
>>>>> returned and then set this error for s_fence->finished fence so whoever
>>>>> wait on this fence can inspect the signaled fence for an error.
>>>>>
>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>> ---
>>>>>      drivers/gpu/drm/scheduler/sched_main.c | 19 ++++++++++++++++---
>>>>>      1 file changed, 16 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> index 9a0ee74..f39b97e 100644
>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> @@ -479,6 +479,7 @@ void drm_sched_resubmit_jobs(struct
>>>>> drm_gpu_scheduler *sched)
>>>>>          struct drm_sched_job *s_job, *tmp;
>>>>>          uint64_t guilty_context;
>>>>>          bool found_guilty = false;
>>>>> +    struct dma_fence *fence;
>>>>>            list_for_each_entry_safe(s_job, tmp,
>>>>> &sched->ring_mirror_list, node) {
>>>>>              struct drm_sched_fence *s_fence = s_job->s_fence;
>>>>> @@ -492,7 +493,16 @@ void drm_sched_resubmit_jobs(struct
>>>>> drm_gpu_scheduler *sched)
>>>>>                  dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>>>>                dma_fence_put(s_job->s_fence->parent);
>>>>> -        s_job->s_fence->parent = sched->ops->run_job(s_job);
>>>>> +        fence = sched->ops->run_job(s_job);
>>>>> +
>>>>> +        if (IS_ERR_OR_NULL(fence)) {
>>>>> +            s_job->s_fence->parent = NULL;
>>>>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>>>> +        } else {
>>>>> +            s_job->s_fence->parent = fence;
>>>>> +        }
>>>>> +
>>>>> +
>>>> Maybe time for a drm_sched_run_job() function which does that
>>>> handling? And why don't we need to install the callback here?
>>> What code do you want to put in drm_sched_run_job ?
>>>
>>> We reinstall the callback later in drm_sched_start,
>>> drm_sched_resubmit_jobs is conditional on whether the guilty fence did
>>> signal by this time or not and so the split of the logic into
>>> drm_sched_start and drm_sched_resubmit_jobs.
>> Ah, yes of course. In this case the patch is Reviewed-by: Christian
>> König <christian.koenig@amd.com>.
>>
>> Regards,
>> Christian.
>
> Thanks, there is also 2/2 patch for amdgpu, please take a look.

Seen that, feel free to add my rb as well.

Christian.

>
> Andrey
>
>
>>> Andrey
>>>
>>>
>>>> Apart from that looks good to me,
>>>> Christian.
>>>>
>>>>>          }
>>>>>      }
>>>>>      EXPORT_SYMBOL(drm_sched_resubmit_jobs);
>>>>> @@ -720,7 +730,7 @@ static int drm_sched_main(void *param)
>>>>>              fence = sched->ops->run_job(sched_job);
>>>>>              drm_sched_fence_scheduled(s_fence);
>>>>>      -        if (fence) {
>>>>> +        if (!IS_ERR_OR_NULL(fence)) {
>>>>>                  s_fence->parent = dma_fence_get(fence);
>>>>>                  r = dma_fence_add_callback(fence, &sched_job->cb,
>>>>>                                 drm_sched_process_job);
>>>>> @@ -730,8 +740,11 @@ static int drm_sched_main(void *param)
>>>>>                      DRM_ERROR("fence add callback failed (%d)\n",
>>>>>                            r);
>>>>>                  dma_fence_put(fence);
>>>>> -        } else
>>>>> +        } else {
>>>>> +
>>>>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>>>>                  drm_sched_process_job(NULL, &sched_job->cb);
>>>>> +        }
>>>>>                wake_up(&sched->job_scheduled);
>>>>>          }

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/sched: Set error to s_fence if HW job submission failed.

[Koenig, Christian]

Am 25.10.19 um 17:56 schrieb Grodzovsky, Andrey:
> On 10/25/19 11:55 AM, Koenig, Christian wrote:
>> Am 25.10.19 um 16:57 schrieb Grodzovsky, Andrey:
>>> On 10/25/19 4:44 AM, Christian König wrote:
>>>> Am 24.10.19 um 21:57 schrieb Andrey Grodzovsky:
>>>>> Problem:
>>>>> When run_job fails and HW fence returned is NULL we still signal
>>>>> the s_fence to avoid hangs but the user has no way of knowing if
>>>>> the actual HW job was ran and finished.
>>>>>
>>>>> Fix:
>>>>> Allow .run_job implementations to return ERR_PTR in the fence pointer
>>>>> returned and then set this error for s_fence->finished fence so whoever
>>>>> wait on this fence can inspect the signaled fence for an error.
>>>>>
>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>> ---
>>>>>      drivers/gpu/drm/scheduler/sched_main.c | 19 ++++++++++++++++---
>>>>>      1 file changed, 16 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> index 9a0ee74..f39b97e 100644
>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> @@ -479,6 +479,7 @@ void drm_sched_resubmit_jobs(struct
>>>>> drm_gpu_scheduler *sched)
>>>>>          struct drm_sched_job *s_job, *tmp;
>>>>>          uint64_t guilty_context;
>>>>>          bool found_guilty = false;
>>>>> +    struct dma_fence *fence;
>>>>>            list_for_each_entry_safe(s_job, tmp,
>>>>> &sched->ring_mirror_list, node) {
>>>>>              struct drm_sched_fence *s_fence = s_job->s_fence;
>>>>> @@ -492,7 +493,16 @@ void drm_sched_resubmit_jobs(struct
>>>>> drm_gpu_scheduler *sched)
>>>>>                  dma_fence_set_error(&s_fence->finished, -ECANCELED);
>>>>>                dma_fence_put(s_job->s_fence->parent);
>>>>> -        s_job->s_fence->parent = sched->ops->run_job(s_job);
>>>>> +        fence = sched->ops->run_job(s_job);
>>>>> +
>>>>> +        if (IS_ERR_OR_NULL(fence)) {
>>>>> +            s_job->s_fence->parent = NULL;
>>>>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>>>> +        } else {
>>>>> +            s_job->s_fence->parent = fence;
>>>>> +        }
>>>>> +
>>>>> +
>>>> Maybe time for a drm_sched_run_job() function which does that
>>>> handling? And why don't we need to install the callback here?
>>> What code do you want to put in drm_sched_run_job ?
>>>
>>> We reinstall the callback later in drm_sched_start,
>>> drm_sched_resubmit_jobs is conditional on whether the guilty fence did
>>> signal by this time or not and so the split of the logic into
>>> drm_sched_start and drm_sched_resubmit_jobs.
>> Ah, yes of course. In this case the patch is Reviewed-by: Christian
>> König <christian.koenig@amd.com>.
>>
>> Regards,
>> Christian.
>
> Thanks, there is also 2/2 patch for amdgpu, please take a look.

Seen that, feel free to add my rb as well.

Christian.

>
> Andrey
>
>
>>> Andrey
>>>
>>>
>>>> Apart from that looks good to me,
>>>> Christian.
>>>>
>>>>>          }
>>>>>      }
>>>>>      EXPORT_SYMBOL(drm_sched_resubmit_jobs);
>>>>> @@ -720,7 +730,7 @@ static int drm_sched_main(void *param)
>>>>>              fence = sched->ops->run_job(sched_job);
>>>>>              drm_sched_fence_scheduled(s_fence);
>>>>>      -        if (fence) {
>>>>> +        if (!IS_ERR_OR_NULL(fence)) {
>>>>>                  s_fence->parent = dma_fence_get(fence);
>>>>>                  r = dma_fence_add_callback(fence, &sched_job->cb,
>>>>>                                 drm_sched_process_job);
>>>>> @@ -730,8 +740,11 @@ static int drm_sched_main(void *param)
>>>>>                      DRM_ERROR("fence add callback failed (%d)\n",
>>>>>                            r);
>>>>>                  dma_fence_put(fence);
>>>>> -        } else
>>>>> +        } else {
>>>>> +
>>>>> +            dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>>>>                  drm_sched_process_job(NULL, &sched_job->cb);
>>>>> +        }
>>>>>                wake_up(&sched->job_scheduled);
>>>>>          }

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx