On 09/17, Iago Toral wrote:

Reviewed-by: Iago Toral Quiroga itoral@igalia.com

With that said, I don't like how we are doing error handling here, I think we want to simplify this and try to make it so we centralize error handling in one place instead of having multiple error exits paths, each one trying to do the right thing at that point. This is error prone, as this patch is showing.

Here is a proposal to make this better:

Make job memory allocation part of v3d_job_init. v3d_job init already handles error conditions nicely. If we do this, we no longer need to handle allocation errors in ioctls one by one and for any job we only have two scenarios: v3d_job_init was successul or it failed (in which case we know it already cleaned up after itself and we should have a NULL job as a result). If v3d_job_init failed, then we *always* jump to the fail tag and there we call v3d_job_cleanup for all jobs that can be created in that ioctl. If a job is NULL then v3d_job_cleanup returns early, otherwise, we know it is a fully initialized job, so it does drm_sched_job_cleanup + v3d_job_put.

I think that should make error handling in these functions a lot easier.

Hi,

yes, sounds good. I can include this refactoring in the v2 of the multisync patchset, since there is a prep work there (first patch).

Thanks for reviewing both,

Melissa

Iago

On Thu, 2021-09-16 at 22:27 +0100, Melissa Wen wrote:

...

In a cl submission, when bin job initialization fails, sched job resources were already allocated for the render job. At this point, drm_sched_job_init(render) was done in v3d_job_init but the render job is aborted before drm_sched_job_arm (in v3d_job_push) happens; therefore, not only v3d_job_put but also drm_sched_job_cleanup should be called (by v3d_job_cleanup). A similar issue is addressed for csd and tfu submissions.

The issue was noticed from a review by Iago Toral in a patch that touches the same part of the code.

Fixes: 916044fac8623 ("drm/v3d: Move drm_sched_job_init to v3d_job_init") Signed-off-by: Melissa Wen mwen@igalia.com

---

drivers/gpu/drm/v3d/v3d_gem.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index 1953706bdaeb..ead0be8d48a7 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -567,14 +567,14 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, if (args->bcl_start != args->bcl_end) { bin = kcalloc(1, sizeof(*bin), GFP_KERNEL); if (!bin) {

• 	v3d_job_put(&render->base);
  

• 	v3d_job_cleanup(&render->base);
  
  

...

	return -ENOMEM;

}

ret = v3d_job_init(v3d, file_priv, &bin->base,
		   v3d_job_free, args->in_sync_bcl,

V3D_BIN); if (ret) {

• 	v3d_job_put(&render->base);
  

• 	v3d_job_cleanup(&render->base);
  kfree(bin);
  return ret;
  

  }

@@ -716,7 +716,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, job->base.bo = kcalloc(ARRAY_SIZE(args->bo_handles), sizeof(*job->base.bo), GFP_KERNEL); if (!job->base.bo) {

• v3d_job_put(&job->base);
  

• v3d_job_cleanup(&job->base);
  

  return -ENOMEM; }

@@ -810,14 +810,13 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,

clean_job = kcalloc(1, sizeof(*clean_job), GFP_KERNEL); if (!clean_job) {

• v3d_job_put(&job->base);
  

• kfree(job);
  

• v3d_job_cleanup(&job->base);
  

  return -ENOMEM; }

  ret = v3d_job_init(v3d, file_priv, clean_job, v3d_job_free, 0,

V3D_CACHE_CLEAN); if (ret) {

• v3d_job_put(&job->base);
  

• v3d_job_cleanup(&job->base);
  

  kfree(clean_job); return ret; }