[PATCH 0/2] accel/ivpu: Fixes for linux-6.3-rc6

[PATCH 0/2] accel/ivpu: Fixes for linux-6.3-rc6

[Stanislaw Gruszka]

Jacek Lawrynowicz (1):
  accel/ivpu: Fix S3 system suspend when not idle

Karol Wachowski (1):
  accel/ivpu: Add dma fence to command buffers only

 drivers/accel/ivpu/ivpu_job.c | 18 +++++++-----------
 drivers/accel/ivpu/ivpu_pm.c  | 26 +++++++++++---------------
 2 files changed, 18 insertions(+), 26 deletions(-)

-- 
2.25.1

[PATCH 1/2] accel/ivpu: Add dma fence to command buffers only

[Stanislaw Gruszka]

From: Karol Wachowski <karol.wachowski@linux.intel.com>

Currently job->done_fence is added to every BO handle within a job. If job
handle (command buffer) is shared between multiple submits, KMD will add
the fence in each of them. Then bo_wait_ioctl() executed on command buffer
will exit only when all jobs containing that handle are done.

This creates deadlock scenario for user mode driver in case when job handle
is added as dependency of another job, because bo_wait_ioctl() of first job
will wait until second job finishes, and second job can not finish before
first one.

Having fences added only to job buffer handle allows user space to execute
bo_wait_ioctl() on the job even if it's handle is submitted with other job.

Fixes: cd7272215c44 ("accel/ivpu: Add command buffer submission logic")
Signed-off-by: Karol Wachowski <karol.wachowski@linux.intel.com>
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 drivers/accel/ivpu/ivpu_job.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/accel/ivpu/ivpu_job.c b/drivers/accel/ivpu/ivpu_job.c
index 910301fae435..3c6f1e16cf2f 100644
--- a/drivers/accel/ivpu/ivpu_job.c
+++ b/drivers/accel/ivpu/ivpu_job.c
@@ -461,26 +461,22 @@ ivpu_job_prepare_bos_for_submit(struct drm_file *file, struct ivpu_job *job, u32
 
 	job->cmd_buf_vpu_addr = bo->vpu_addr + commands_offset;
 
-	ret = drm_gem_lock_reservations((struct drm_gem_object **)job->bos, buf_count,
-					&acquire_ctx);
+	ret = drm_gem_lock_reservations((struct drm_gem_object **)job->bos, 1, &acquire_ctx);
 	if (ret) {
 		ivpu_warn(vdev, "Failed to lock reservations: %d\n", ret);
 		return ret;
 	}
 
-	for (i = 0; i < buf_count; i++) {
-		ret = dma_resv_reserve_fences(job->bos[i]->base.resv, 1);
-		if (ret) {
-			ivpu_warn(vdev, "Failed to reserve fences: %d\n", ret);
-			goto unlock_reservations;
-		}
+	ret = dma_resv_reserve_fences(bo->base.resv, 1);
+	if (ret) {
+		ivpu_warn(vdev, "Failed to reserve fences: %d\n", ret);
+		goto unlock_reservations;
 	}
 
-	for (i = 0; i < buf_count; i++)
-		dma_resv_add_fence(job->bos[i]->base.resv, job->done_fence, DMA_RESV_USAGE_WRITE);
+	dma_resv_add_fence(bo->base.resv, job->done_fence, DMA_RESV_USAGE_WRITE);
 
 unlock_reservations:
-	drm_gem_unlock_reservations((struct drm_gem_object **)job->bos, buf_count, &acquire_ctx);
+	drm_gem_unlock_reservations((struct drm_gem_object **)job->bos, 1, &acquire_ctx);
 
 	wmb(); /* Flush write combining buffers */
 
-- 
2.25.1

Re: [PATCH 1/2] accel/ivpu: Add dma fence to command buffers only

[Jeffrey Hugo]

On 3/31/2023 5:36 AM, Stanislaw Gruszka wrote:
> From: Karol Wachowski <karol.wachowski@linux.intel.com>
> 
> Currently job->done_fence is added to every BO handle within a job. If job
> handle (command buffer) is shared between multiple submits, KMD will add
> the fence in each of them. Then bo_wait_ioctl() executed on command buffer
> will exit only when all jobs containing that handle are done.
> 
> This creates deadlock scenario for user mode driver in case when job handle
> is added as dependency of another job, because bo_wait_ioctl() of first job
> will wait until second job finishes, and second job can not finish before
> first one.
> 
> Having fences added only to job buffer handle allows user space to execute
> bo_wait_ioctl() on the job even if it's handle is submitted with other job.
> 
> Fixes: cd7272215c44 ("accel/ivpu: Add command buffer submission logic")
> Signed-off-by: Karol Wachowski <karol.wachowski@linux.intel.com>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

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

Re: [PATCH 1/2] accel/ivpu: Add dma fence to command buffers only

[Daniel Vetter]

On Fri, Mar 31, 2023 at 01:36:02PM +0200, Stanislaw Gruszka wrote:
> From: Karol Wachowski <karol.wachowski@linux.intel.com>
> 
> Currently job->done_fence is added to every BO handle within a job. If job
> handle (command buffer) is shared between multiple submits, KMD will add
> the fence in each of them. Then bo_wait_ioctl() executed on command buffer
> will exit only when all jobs containing that handle are done.
> 
> This creates deadlock scenario for user mode driver in case when job handle
> is added as dependency of another job, because bo_wait_ioctl() of first job
> will wait until second job finishes, and second job can not finish before
> first one.
> 
> Having fences added only to job buffer handle allows user space to execute
> bo_wait_ioctl() on the job even if it's handle is submitted with other job.
> 
> Fixes: cd7272215c44 ("accel/ivpu: Add command buffer submission logic")
> Signed-off-by: Karol Wachowski <karol.wachowski@linux.intel.com>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

Uh this is confused on a lot of levels ...

Frist having a new driver which uses the dma_resv/bo implicit fencing for
umd synchronization is not great at all. The modern way of doing is:
- in/out dependencies are handling with drm_syncobj
- userspace waits on the drm_syncobj, not with a driver-private wait ioctl
  on a specific bo

The other issue is that the below starts to fall over once you do dynamic
memory management, for that case you _always_ have to install a fence.

Now fear not, there's a solution here:
- you always install a fence (i.e. revert this patch), but by default is a
  DMA_RESV_USAGE_BOOKKEEP fence. See the kerneldoc for enum dma_resv_usage
  for what that means.
- only for the special job bo you set the DMA_RESV_USAGE_READ flag. You
  want read because really that's what the gpu is doing for the job bo.
- the bo_wait ioctl then waits for write access internally

Should result in the same uapi as your patch here, but without abusing
dma_resv in a way that'll go boom.

Note that userspace can get at the dma_resv READ/WRITE fences through
ioctls on a dma-buf, so which one you pick here is uabi relevant.
bo-as-job-fence is USAGE_READ.

Please take care of this in -next.

Cheers, Daniel

> ---
>  drivers/accel/ivpu/ivpu_job.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/accel/ivpu/ivpu_job.c b/drivers/accel/ivpu/ivpu_job.c
> index 910301fae435..3c6f1e16cf2f 100644
> --- a/drivers/accel/ivpu/ivpu_job.c
> +++ b/drivers/accel/ivpu/ivpu_job.c
> @@ -461,26 +461,22 @@ ivpu_job_prepare_bos_for_submit(struct drm_file *file, struct ivpu_job *job, u32
>  
>  	job->cmd_buf_vpu_addr = bo->vpu_addr + commands_offset;
>  
> -	ret = drm_gem_lock_reservations((struct drm_gem_object **)job->bos, buf_count,
> -					&acquire_ctx);
> +	ret = drm_gem_lock_reservations((struct drm_gem_object **)job->bos, 1, &acquire_ctx);
>  	if (ret) {
>  		ivpu_warn(vdev, "Failed to lock reservations: %d\n", ret);
>  		return ret;
>  	}
>  
> -	for (i = 0; i < buf_count; i++) {
> -		ret = dma_resv_reserve_fences(job->bos[i]->base.resv, 1);
> -		if (ret) {
> -			ivpu_warn(vdev, "Failed to reserve fences: %d\n", ret);
> -			goto unlock_reservations;
> -		}
> +	ret = dma_resv_reserve_fences(bo->base.resv, 1);
> +	if (ret) {
> +		ivpu_warn(vdev, "Failed to reserve fences: %d\n", ret);
> +		goto unlock_reservations;
>  	}
>  
> -	for (i = 0; i < buf_count; i++)
> -		dma_resv_add_fence(job->bos[i]->base.resv, job->done_fence, DMA_RESV_USAGE_WRITE);
> +	dma_resv_add_fence(bo->base.resv, job->done_fence, DMA_RESV_USAGE_WRITE);
>  
>  unlock_reservations:
> -	drm_gem_unlock_reservations((struct drm_gem_object **)job->bos, buf_count, &acquire_ctx);
> +	drm_gem_unlock_reservations((struct drm_gem_object **)job->bos, 1, &acquire_ctx);
>  
>  	wmb(); /* Flush write combining buffers */
>  
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2] accel/ivpu: Add dma fence to command buffers only

[Stanislaw Gruszka]

On Thu, Apr 06, 2023 at 06:57:34PM +0200, Daniel Vetter wrote:
> On Fri, Mar 31, 2023 at 01:36:02PM +0200, Stanislaw Gruszka wrote:
> > From: Karol Wachowski <karol.wachowski@linux.intel.com>
> > 
> > Currently job->done_fence is added to every BO handle within a job. If job
> > handle (command buffer) is shared between multiple submits, KMD will add
> > the fence in each of them. Then bo_wait_ioctl() executed on command buffer
> > will exit only when all jobs containing that handle are done.
> > 
> > This creates deadlock scenario for user mode driver in case when job handle
> > is added as dependency of another job, because bo_wait_ioctl() of first job
> > will wait until second job finishes, and second job can not finish before
> > first one.
> > 
> > Having fences added only to job buffer handle allows user space to execute
> > bo_wait_ioctl() on the job even if it's handle is submitted with other job.
> > 
> > Fixes: cd7272215c44 ("accel/ivpu: Add command buffer submission logic")
> > Signed-off-by: Karol Wachowski <karol.wachowski@linux.intel.com>
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> 
> Uh this is confused on a lot of levels ...
> 
> Frist having a new driver which uses the dma_resv/bo implicit fencing for
> umd synchronization is not great at all. The modern way of doing is:
> - in/out dependencies are handling with drm_syncobj
> - userspace waits on the drm_syncobj, not with a driver-private wait ioctl
>   on a specific bo

We have synobj on our TODO list, will work on it.

> The other issue is that the below starts to fall over once you do dynamic
> memory management, for that case you _always_ have to install a fence.
> 
> Now fear not, there's a solution here:
> - you always install a fence (i.e. revert this patch), but by default is a
>   DMA_RESV_USAGE_BOOKKEEP fence. See the kerneldoc for enum dma_resv_usage
>   for what that means.
> - only for the special job bo you set the DMA_RESV_USAGE_READ flag. You
>   want read because really that's what the gpu is doing for the job bo.
> - the bo_wait ioctl then waits for write access internally

In our case VPU can write to command buffer (there is special context
save area), so I think keeping USAGE_WRITE is appropriate.

> Should result in the same uapi as your patch here, but without abusing
> dma_resv in a way that'll go boom.
> 
> Note that userspace can get at the dma_resv READ/WRITE fences through
> ioctls on a dma-buf, so which one you pick here is uabi relevant.
> bo-as-job-fence is USAGE_READ.
> 
> Please take care of this in -next.

Sure.

Regards
Stanislaw

[PATCH 2/2] accel/ivpu: Fix S3 system suspend when not idle

[Stanislaw Gruszka]

From: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>

Wait for VPU to be idle in ivpu_pm_suspend_cb() before powering off
the device, so jobs are not lost and TDRs are not triggered after
resume.

Fixes: 852be13f3bd3 ("accel/ivpu: Add PM support")
Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 drivers/accel/ivpu/ivpu_pm.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/accel/ivpu/ivpu_pm.c b/drivers/accel/ivpu/ivpu_pm.c
index da0bbc46a024..aa4d56dc52b3 100644
--- a/drivers/accel/ivpu/ivpu_pm.c
+++ b/drivers/accel/ivpu/ivpu_pm.c
@@ -140,32 +140,28 @@ int ivpu_pm_suspend_cb(struct device *dev)
 {
 	struct drm_device *drm = dev_get_drvdata(dev);
 	struct ivpu_device *vdev = to_ivpu_device(drm);
-	int ret;
+	unsigned long timeout;
 
 	ivpu_dbg(vdev, PM, "Suspend..\n");
 
-	ret = ivpu_suspend(vdev);
-	if (ret && vdev->pm->suspend_reschedule_counter) {
-		ivpu_dbg(vdev, PM, "Failed to enter idle, rescheduling suspend, retries left %d\n",
-			 vdev->pm->suspend_reschedule_counter);
-		pm_schedule_suspend(dev, vdev->timeout.reschedule_suspend);
-		vdev->pm->suspend_reschedule_counter--;
-		return -EBUSY;
-	} else if (!vdev->pm->suspend_reschedule_counter) {
-		ivpu_warn(vdev, "Failed to enter idle, force suspend\n");
-		ivpu_pm_prepare_cold_boot(vdev);
-	} else {
-		ivpu_pm_prepare_warm_boot(vdev);
+	timeout = jiffies + msecs_to_jiffies(vdev->timeout.tdr);
+	while (!ivpu_hw_is_idle(vdev)) {
+		cond_resched();
+		if (time_after_eq(jiffies, timeout)) {
+			ivpu_err(vdev, "Failed to enter idle on system suspend\n");
+			return -EBUSY;
+		}
 	}
 
-	vdev->pm->suspend_reschedule_counter = PM_RESCHEDULE_LIMIT;
+	ivpu_suspend(vdev);
+	ivpu_pm_prepare_warm_boot(vdev);
 
 	pci_save_state(to_pci_dev(dev));
 	pci_set_power_state(to_pci_dev(dev), PCI_D3hot);
 
 	ivpu_dbg(vdev, PM, "Suspend done.\n");
 
-	return ret;
+	return 0;
 }
 
 int ivpu_pm_resume_cb(struct device *dev)
-- 
2.25.1

Re: [PATCH 2/2] accel/ivpu: Fix S3 system suspend when not idle

[Jeffrey Hugo]

On 3/31/2023 5:36 AM, Stanislaw Gruszka wrote:
> From: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
> 
> Wait for VPU to be idle in ivpu_pm_suspend_cb() before powering off
> the device, so jobs are not lost and TDRs are not triggered after
> resume.
> 
> Fixes: 852be13f3bd3 ("accel/ivpu: Add PM support")
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

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