From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH] drm/v3d: clean caches at the end of render jobs on request from user space Date: Fri, 13 Sep 2019 13:35:44 -0700 Message-ID: <87ftl0szmn.fsf@anholt.net> References: <20190912083516.13797-1-itoral@igalia.com> <87d0g5l94v.fsf@anholt.net> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1082447523==" Return-path: Received: from anholt.net (anholt.net [50.246.234.109]) by gabe.freedesktop.org (Postfix) with ESMTP id 409966F457 for ; Fri, 13 Sep 2019 20:36:05 +0000 (UTC) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Iago Toral , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1082447523== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Iago Toral writes: > On Thu, 2019-09-12 at 10:25 -0700, Eric Anholt wrote: >> Iago Toral Quiroga writes: >>=20 >> > 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. >>=20 >> 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. >>=20 >> > Signed-off-by: Iago Toral Quiroga >> > --- >> > drivers/gpu/drm/v3d/v3d_gem.c | 51 +++++++++++++++++++++++++++++ >> > ------ >> > include/uapi/drm/v3d_drm.h | 7 ++--- >> > 2 files changed, 47 insertions(+), 11 deletions(-) >> >=20 >> > 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 =3D data; >> > struct v3d_bin_job *bin =3D NULL; >> > struct v3d_render_job *render; >> > + struct v3d_job *clean_job =3D NULL; >> > + struct v3d_job *last_job; >> > struct ww_acquire_ctx acquire_ctx; >> > int ret =3D 0; >> >=20=20 >> > trace_v3d_submit_cl_ioctl(&v3d->drm, args->rcl_start, args- >> > >rcl_end); >> >=20=20 >> > - if (args->pad !=3D 0) { >> > - DRM_INFO("pad must be zero: %d\n", args->pad); >> > + if (args->flags !=3D 0 && >> > + args->flags !=3D DRM_V3D_SUBMIT_CL_FLUSH_CACHE_FLAG) { >> > + DRM_INFO("invalid flags: %d\n", args->flags); >> > return -EINVAL; >> > } >> >=20=20 >> > @@ -575,12 +578,28 @@ v3d_submit_cl_ioctl(struct drm_device *dev, >> > void *data, >> > bin->render =3D render; >> > } >> >=20=20 >> > - ret =3D v3d_lookup_bos(dev, file_priv, &render->base, >> > + if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE_FLAG) { >> > + clean_job =3D kcalloc(1, sizeof(*clean_job), GFP_KERNEL); >> > + if (!clean_job) { >> > + ret =3D -ENOMEM; >> > + goto fail; >> > + } >> > + >> > + ret =3D v3d_job_init(v3d, file_priv, clean_job, >> > v3d_job_free, 0); >> > + if (ret) >> > + goto fail; >>=20 >> 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. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAl17/aAACgkQtdYpNtH8 nugxTxAAsVGZRpBgSPRz+egUMEwm5F4LUap5O5KboIfLJZ1KffcE3LC4EmxXkWlW +BBIdcf5CqTy82K6S2BY+PIiirTm5jWdiRWt9LbcAS7IUWiuAl88n4JMCqRS5Bou dDbDUsARpaSNjXZPGrgjVmk4OSln7q2TJhDrtXkvD4iD6xCdoOEHmiLrWgCo1fBM a+rQ1tAAwP91KvcKrx81CoBHIfDb2NWW44GqCEW8sGoQvEue0DOlYIBv12f4xkbx Fps70eulZMLlOV7uBEvOlyWd+bYpdFPw6E/y/e2QM4lHwAVmSpcdMxN0n/S2AC7G 2WkP4O9L2vfBOVKL9r3IrEf/FoeCqz7K543TOOkwxOGAA1ffdTylE2F3MKVi42+t J86dvWvbEPEPfxzwLh4Skr3OlfKndtFt+Bo/4kRIiPJDQ207dLvy0++pxSZ2Uj8L PFAlDrQ1UMOfymyBdBiBVA7H3zqsCSxb9np/v3okAjB5GAQblqpUNwBbsASMEQWm aG9i2/U4e/C3QnZS9MVW/rob8fLO83SIYzY28N3Pia/yq3ZOx9cD0m1qgARIkje5 B7z7vwNMSC4/Vt7zb5ZT6WnDAL6BSWYK5Xk4EZnmG6QxPHnp3758g2IAVpblau5+ HR6FJgxzR98bpe4nChtZNGD5eL9XSBuex6wGufGsApHkRK544Hc= =b1ce -----END PGP SIGNATURE----- --=-=-=-- --===============1082447523== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVs --===============1082447523==--