Re: [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.lawrynow...@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_jh...@quicinc.com>