Re: [PATCH] drm/v3d: clean caches at the end of render jobs on request from user space

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1.1: Type: text/plain, Size: 3288 bytes --]

Iago Toral <itoral@igalia.com> writes:

> On Thu, 2019-09-12 at 10:25 -0700, Eric Anholt wrote:
>> Iago Toral Quiroga <itoral@igalia.com> 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@igalia.com>
>> > ---
>> >  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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel