[PATCH 2/3] accel/ivpu: Improve stability of ivpu_submit_ioctl()

[Jeffrey Hugo]

On 1/22/2024 5:09 AM, Jacek Lawrynowicz wrote:
> - Wake up the device as late as possible

Can you amend with why this is a good idea?

> - Remove job reference counting in order to simplify the code
> - Don't put jobs that are not fully submitted on submitted_jobs_xa in
>    order to avoid potential races with reset/recovery
> 
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz at linux.intel.com>
> ---
>   drivers/accel/ivpu/ivpu_job.c | 139 +++++++++++++++-------------------
>   drivers/accel/ivpu/ivpu_job.h |   1 -
>   2 files changed, 62 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/accel/ivpu/ivpu_job.c b/drivers/accel/ivpu/ivpu_job.c
> index 4fed0c05e051..d9b47a04b35f 100644
> --- a/drivers/accel/ivpu/ivpu_job.c
> +++ b/drivers/accel/ivpu/ivpu_job.c
> @@ -125,7 +125,7 @@ void ivpu_cmdq_release_all_locked(struct ivpu_file_priv *file_priv)
>   /*
>    * Mark the doorbell as unregistered and reset job queue pointers.
>    * This function needs to be called when the VPU hardware is restarted
> - * and FW looses job queue state. The next time job queue is used it
> + * and FW loses job queue state. The next time job queue is used it
>    * will be registered again.
>    */
>   static void ivpu_cmdq_reset_locked(struct ivpu_file_priv *file_priv, u16 engine)
> @@ -239,60 +239,32 @@ static struct dma_fence *ivpu_fence_create(struct ivpu_device *vdev)
>   	return &fence->base;
>   }
>   
> -static void job_get(struct ivpu_job *job, struct ivpu_job **link)
> +static void ivpu_job_destroy(struct ivpu_job *job)
>   {
>   	struct ivpu_device *vdev = job->vdev;
> -
> -	kref_get(&job->ref);
> -	*link = job;
> -
> -	ivpu_dbg(vdev, KREF, "Job get: id %u refcount %u\n", job->job_id, kref_read(&job->ref));
> -}
> -
> -static void job_release(struct kref *ref)
> -{
> -	struct ivpu_job *job = container_of(ref, struct ivpu_job, ref);
> -	struct ivpu_device *vdev = job->vdev;
>   	u32 i;
>   
> +	ivpu_dbg(vdev, JOB, "Job destroyed: id %3u ctx %2d engine %d",
> +		 job->job_id, job->file_priv->ctx.id, job->engine_idx);
> +
>   	for (i = 0; i < job->bo_count; i++)
>   		if (job->bos[i])
>   			drm_gem_object_put(&job->bos[i]->base.base);
>   
>   	dma_fence_put(job->done_fence);
>   	ivpu_file_priv_put(&job->file_priv);
> -
> -	ivpu_dbg(vdev, KREF, "Job released: id %u\n", job->job_id);
>   	kfree(job);
> -
> -	/* Allow the VPU to get suspended, must be called after ivpu_file_priv_put() */
> -	ivpu_rpm_put(vdev);
> -}
> -
> -static void job_put(struct ivpu_job *job)
> -{
> -	struct ivpu_device *vdev = job->vdev;
> -
> -	ivpu_dbg(vdev, KREF, "Job put: id %u refcount %u\n", job->job_id, kref_read(&job->ref));
> -	kref_put(&job->ref, job_release);
>   }
>   
>   static struct ivpu_job *
> -ivpu_create_job(struct ivpu_file_priv *file_priv, u32 engine_idx, u32 bo_count)
> +ivpu_job_create(struct ivpu_file_priv *file_priv, u32 engine_idx, u32 bo_count)
>   {
>   	struct ivpu_device *vdev = file_priv->vdev;
>   	struct ivpu_job *job;
> -	int ret;
> -
> -	ret = ivpu_rpm_get(vdev);
> -	if (ret < 0)
> -		return NULL;
>   
>   	job = kzalloc(struct_size(job, bos, bo_count), GFP_KERNEL);
>   	if (!job)
> -		goto err_rpm_put;
> -
> -	kref_init(&job->ref);
> +		return NULL;
>   
>   	job->vdev = vdev;
>   	job->engine_idx = engine_idx;
> @@ -306,17 +278,14 @@ ivpu_create_job(struct ivpu_file_priv *file_priv, u32 engine_idx, u32 bo_count)
>   	job->file_priv = ivpu_file_priv_get(file_priv);
>   
>   	ivpu_dbg(vdev, JOB, "Job created: ctx %2d engine %d", file_priv->ctx.id, job->engine_idx);
> -
>   	return job;
>   
>   err_free_job:
>   	kfree(job);
> -err_rpm_put:
> -	ivpu_rpm_put(vdev);
>   	return NULL;
>   }
>   
> -static int ivpu_job_done(struct ivpu_device *vdev, u32 job_id, u32 job_status)
> +static int ivpu_job_signal_and_destroy(struct ivpu_device *vdev, u32 job_id, u32 job_status)
>   {
>   	struct ivpu_job *job;
>   
> @@ -333,9 +302,10 @@ static int ivpu_job_done(struct ivpu_device *vdev, u32 job_id, u32 job_status)
>   	ivpu_dbg(vdev, JOB, "Job complete:  id %3u ctx %2d engine %d status 0x%x\n",
>   		 job->job_id, job->file_priv->ctx.id, job->engine_idx, job_status);
>   
> +	ivpu_job_destroy(job);
>   	ivpu_stop_job_timeout_detection(vdev);
>   
> -	job_put(job);
> +	ivpu_rpm_put(vdev);

Since this put() corresponds to a get() that is not in this function, I 
suggest adding a comment that points to where the corresponding get() is.

Reviewed-by: Jeffrey Hugo <quic_jhugo at quicinc.com>