| Message ID | 20190424131355.62817-1-tomeu.vizoso@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panfrost: Add sanity checks to submit IOCTL | expand |
On Wed, Apr 24, 2019 at 03:13:53PM +0200, Tomeu Vizoso wrote: > So userspace can get feedback on any error conditions, instead of going > ahead and things breaking later. > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> Bonus: some igts to exercise these corner cases. It helps a lot with uapi review and testing. -Daniel > --- > drivers/gpu/drm/panfrost/panfrost_drv.c | 35 +++++++++++++++++-------- > 1 file changed, 24 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c > index c06af78ab833..0f2863cb8077 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > @@ -172,13 +172,27 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data, > { > struct panfrost_device *pfdev = dev->dev_private; > struct drm_panfrost_submit *args = data; > - struct drm_syncobj *sync_out; > + struct drm_syncobj *sync_out = NULL; > struct panfrost_job *job; > int ret = 0; > > + if (!args->jc) > + return -EINVAL; > + > + if (args->requirements && args->requirements != PANFROST_JD_REQ_FS) > + return -EINVAL; > + > + if (args->out_sync > 0) { > + sync_out = drm_syncobj_find(file, args->out_sync); > + if (!sync_out) > + return -ENODEV; > + } > + > job = kzalloc(sizeof(*job), GFP_KERNEL); > - if (!job) > - return -ENOMEM; > + if (!job) { > + ret = -ENOMEM; > + goto fail_out_sync; > + } > > kref_init(&job->refcount); > > @@ -190,25 +204,24 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data, > > ret = panfrost_copy_in_sync(dev, file, args, job); > if (ret) > - goto fail; > + goto fail_job; > > ret = panfrost_lookup_bos(dev, file, args, job); > if (ret) > - goto fail; > + goto fail_job; > > ret = panfrost_job_push(job); > if (ret) > - goto fail; > + goto fail_job; > > /* Update the return sync object for the job */ > - sync_out = drm_syncobj_find(file, args->out_sync); > - if (sync_out) { > + if (sync_out) > drm_syncobj_replace_fence(sync_out, job->render_done_fence); > - drm_syncobj_put(sync_out); > - } > > -fail: > +fail_job: > panfrost_job_put(job); > +fail_out_sync: > + drm_syncobj_put(sync_out); > > return ret; > } > -- > 2.20.1 >
On Wed, 24 Apr 2019 at 15:20, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Apr 24, 2019 at 03:13:53PM +0200, Tomeu Vizoso wrote: > > So userspace can get feedback on any error conditions, instead of going > > ahead and things breaking later. > > > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> > > Bonus: some igts to exercise these corner cases. It helps a lot with uapi > review and testing. I'm rebasing that igt branch as we speak :) Tomeu > -Daniel > > > --- > > drivers/gpu/drm/panfrost/panfrost_drv.c | 35 +++++++++++++++++-------- > > 1 file changed, 24 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c > > index c06af78ab833..0f2863cb8077 100644 > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > > @@ -172,13 +172,27 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data, > > { > > struct panfrost_device *pfdev = dev->dev_private; > > struct drm_panfrost_submit *args = data; > > - struct drm_syncobj *sync_out; > > + struct drm_syncobj *sync_out = NULL; > > struct panfrost_job *job; > > int ret = 0; > > > > + if (!args->jc) > > + return -EINVAL; > > + > > + if (args->requirements && args->requirements != PANFROST_JD_REQ_FS) > > + return -EINVAL; > > + > > + if (args->out_sync > 0) { > > + sync_out = drm_syncobj_find(file, args->out_sync); > > + if (!sync_out) > > + return -ENODEV; > > + } > > + > > job = kzalloc(sizeof(*job), GFP_KERNEL); > > - if (!job) > > - return -ENOMEM; > > + if (!job) { > > + ret = -ENOMEM; > > + goto fail_out_sync; > > + } > > > > kref_init(&job->refcount); > > > > @@ -190,25 +204,24 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data, > > > > ret = panfrost_copy_in_sync(dev, file, args, job); > > if (ret) > > - goto fail; > > + goto fail_job; > > > > ret = panfrost_lookup_bos(dev, file, args, job); > > if (ret) > > - goto fail; > > + goto fail_job; > > > > ret = panfrost_job_push(job); > > if (ret) > > - goto fail; > > + goto fail_job; > > > > /* Update the return sync object for the job */ > > - sync_out = drm_syncobj_find(file, args->out_sync); > > - if (sync_out) { > > + if (sync_out) > > drm_syncobj_replace_fence(sync_out, job->render_done_fence); > > - drm_syncobj_put(sync_out); > > - } > > > > -fail: > > +fail_job: > > panfrost_job_put(job); > > +fail_out_sync: > > + drm_syncobj_put(sync_out); > > > > return ret; > > } > > -- > > 2.20.1 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index c06af78ab833..0f2863cb8077 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -172,13 +172,27 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data, { struct panfrost_device *pfdev = dev->dev_private; struct drm_panfrost_submit *args = data; - struct drm_syncobj *sync_out; + struct drm_syncobj *sync_out = NULL; struct panfrost_job *job; int ret = 0; + if (!args->jc) + return -EINVAL; + + if (args->requirements && args->requirements != PANFROST_JD_REQ_FS) + return -EINVAL; + + if (args->out_sync > 0) { + sync_out = drm_syncobj_find(file, args->out_sync); + if (!sync_out) + return -ENODEV; + } + job = kzalloc(sizeof(*job), GFP_KERNEL); - if (!job) - return -ENOMEM; + if (!job) { + ret = -ENOMEM; + goto fail_out_sync; + } kref_init(&job->refcount); @@ -190,25 +204,24 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data, ret = panfrost_copy_in_sync(dev, file, args, job); if (ret) - goto fail; + goto fail_job; ret = panfrost_lookup_bos(dev, file, args, job); if (ret) - goto fail; + goto fail_job; ret = panfrost_job_push(job); if (ret) - goto fail; + goto fail_job; /* Update the return sync object for the job */ - sync_out = drm_syncobj_find(file, args->out_sync); - if (sync_out) { + if (sync_out) drm_syncobj_replace_fence(sync_out, job->render_done_fence); - drm_syncobj_put(sync_out); - } -fail: +fail_job: panfrost_job_put(job); +fail_out_sync: + drm_syncobj_put(sync_out); return ret; }
So userspace can get feedback on any error conditions, instead of going ahead and things breaking later. Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> --- drivers/gpu/drm/panfrost/panfrost_drv.c | 35 +++++++++++++++++-------- 1 file changed, 24 insertions(+), 11 deletions(-)