Re: [PATCH 2/4] drm/imagination: Don't timeout job if its fence has been signaled

[Matt Coster]

Hi Brajesh,

On 12/05/2026 07:47, Brajesh Gupta wrote:
> Check for job's fence in timedout handler and report NO HANG if its

Nit: s/its/it/

> is signaled

This could use more explanation: _why_ is returning NO_HANG in this
instance the correct behaviour? See the kernel docs section "Describe
your changes"[1] for inspiration.

> 
> Signed-off-by: Brajesh Gupta <brajesh.gupta@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/imagination/pvr_queue.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_queue.c b/drivers/gpu/drm/imagination/pvr_queue.c
> index d10a13173f0f..073478919b22 100644
> --- a/drivers/gpu/drm/imagination/pvr_queue.c
> +++ b/drivers/gpu/drm/imagination/pvr_queue.c
> @@ -851,7 +851,8 @@ static void pvr_queue_start(struct pvr_queue *queue)
>   * the scheduler, and re-assign parent fences in the middle.
>   *
>   * Return:
> - *  * DRM_GPU_SCHED_STAT_RESET.
> + *  * Fence signaled : DRM_GPU_SCHED_STAT_NO_HANG
> + *  * otherwise      : DRM_GPU_SCHED_STAT_RESET

The formatting here is off (and won't render nicely in the docs). I
believe it should be something like:

   * Return:
   *  * %DRM_GPU_SCHED_STAT_NO_HANG if the job fence has already been
   *    signaled, or
   *  * %DRM_gPU_SCHED_STAT_RESET otherwise.

>   */
>  static enum drm_gpu_sched_stat
>  pvr_queue_timedout_job(struct drm_sched_job *s_job)
> @@ -862,6 +863,10 @@ pvr_queue_timedout_job(struct drm_sched_job *s_job)
>         struct pvr_job *job;
>         u32 job_count = 0;
> 
> +       /* If fence is already signaled then return no hang */

I don't think this comment really adds anything, the function name and
return value are fairly descriptive already.

Cheers,
Matt

[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

> +       if (dma_fence_is_signaled(s_job->s_fence->parent))
> +               return DRM_GPU_SCHED_STAT_NO_HANG;
> +
>         dev_err(sched->dev, "Job timeout\n");
> 
>         /* Before we stop the scheduler, make sure the queue is out of any list, so
> 
> --
> 2.43.0
> 


-- 
Matt Coster
E: matt.coster@xxxxxxxxxx

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature