Iago Toral <itoral@xxxxxxxxxx> writes:
> On Thu, 2019-09-12 at 10:25 -0700, Eric Anholt wrote:
>> Iago Toral Quiroga <itoral@xxxxxxxxxx> writes:
>>
>> > Extends the user space ioctl for CL submissions so it can include a
>> > request
>> > to flush the cache once the CL execution has completed. Fixes
>> > memory
>> > write violation messages reported by the kernel in workloads
>> > involving
>> > shader memory writes (SSBOs, shader images, scratch, etc) which
>> > sometimes
>> > also lead to GPU resets during Piglit and CTS workloads.
>>
>> Some context for any other reviewers: This patch is the interface
>> change
>> necessary to expose GLES 3.1 on V3D. It turns out the HW packets for
>> flushing the caches were broken in multiple ways.
>>
>> > Signed-off-by: Iago Toral Quiroga <itoral@xxxxxxxxxx>
>> > ---
>> > drivers/gpu/drm/v3d/v3d_gem.c | 51 +++++++++++++++++++++++++++++
>> > ------
>> > include/uapi/drm/v3d_drm.h | 7 ++---
>> > 2 files changed, 47 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/v3d/v3d_gem.c
>> > b/drivers/gpu/drm/v3d/v3d_gem.c
>> > index 5d80507b539b..530fe9d9d5bd 100644
>> > --- a/drivers/gpu/drm/v3d/v3d_gem.c
>> > +++ b/drivers/gpu/drm/v3d/v3d_gem.c
>> > @@ -530,13 +530,16 @@ v3d_submit_cl_ioctl(struct drm_device *dev,
>> > void *data,
>> > struct drm_v3d_submit_cl *args = data;
>> > struct v3d_bin_job *bin = NULL;
>> > struct v3d_render_job *render;
>> > + struct v3d_job *clean_job = NULL;
>> > + struct v3d_job *last_job;
>> > struct ww_acquire_ctx acquire_ctx;
>> > int ret = 0;
>> >
>> > trace_v3d_submit_cl_ioctl(&v3d->drm, args->rcl_start, args-
>> > >rcl_end);
>> >
>> > - if (args->pad != 0) {
>> > - DRM_INFO("pad must be zero: %d\n", args->pad);
>> > + if (args->flags != 0 &&
>> > + args->flags != DRM_V3D_SUBMIT_CL_FLUSH_CACHE_FLAG) {
>> > + DRM_INFO("invalid flags: %d\n", args->flags);
>> > return -EINVAL;
>> > }
>> >
>> > @@ -575,12 +578,28 @@ v3d_submit_cl_ioctl(struct drm_device *dev,
>> > void *data,
>> > bin->render = render;
>> > }
>> >
>> > - ret = v3d_lookup_bos(dev, file_priv, &render->base,
>> > + if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE_FLAG) {
>> > + clean_job = kcalloc(1, sizeof(*clean_job), GFP_KERNEL);
>> > + if (!clean_job) {
>> > + ret = -ENOMEM;
>> > + goto fail;
>> > + }
>> > +
>> > + ret = v3d_job_init(v3d, file_priv, clean_job,
>> > v3d_job_free, 0);
>> > + if (ret)
>> > + goto fail;
>>
>> Only issue I see: If v3d_job_init() fails, we need to not
>> v3d_job_put()
>> it. I'm fine with either kfree() it and NULL the ptr before jumping
>> to
>> fail, or open code the bin/render puts.
>
> It seems we also call v3d_job_put() for the bin job when v3d_job_init()
> fails, which also returns immediately in that case instead of jumping
> to fail to v3d_job_put the render job, so I guess we need the same
> treatment there. Shall I fix that in this patch too or would you rather
> see a different patch sent separately for that?
I think you might be looking at the put of the (already-inited) render
job when initing bin job fails?
Looks like we do leak bin in that case, though. Happy to see that as a
fixup patch.
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
