From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [RFC libdrm 4/6] tegra: Add channel, job, pushbuf and fence APIs Date: Wed, 19 Feb 2014 22:11:59 +0100 Message-ID: <20140219211158.GA15830@ulmo.nvidia.com> References: <1392825893-7380-1-git-send-email-thierry.reding@gmail.com> <1392825893-7380-5-git-send-email-thierry.reding@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0431314122==" Return-path: Received: from mail-bk0-f42.google.com (mail-bk0-f42.google.com [209.85.214.42]) by gabe.freedesktop.org (Postfix) with ESMTP id 4644FFAF8B for ; Wed, 19 Feb 2014 13:12:03 -0800 (PST) Received: by mail-bk0-f42.google.com with SMTP id 6so381873bkj.29 for ; Wed, 19 Feb 2014 13:12:02 -0800 (PST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: Erik Faye-Lund Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0431314122== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="IS0zKkzwUGydFO0o" Content-Disposition: inline --IS0zKkzwUGydFO0o Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Feb 19, 2014 at 08:57:29PM +0100, Erik Faye-Lund wrote: > On Wed, Feb 19, 2014 at 5:04 PM, Thierry Reding wrote: [...] > > +drm_public > > +int drm_tegra_fence_wait_timeout(struct drm_tegra_fence *fence, > > + unsigned long timeout) > > +{ > > + struct drm_tegra_syncpt_wait args; > > + int err; > > + > > + memset(&args, 0, sizeof(args)); >=20 > Nit: how about >=20 > struct drm_tegra_syncpt_wait args =3D { 0 }; >=20 > instead? I've compiled both variants and they seem to be generating exactly the same code. Oddly enough, neither of them seems to be explicitly clearing any part of the stack and indeed leaving out memset() and =3D { 0, } does generate the same code again. It looks like the compiler is being pretty clever about optimizing this part. The reason I prefer memset() is that it's somewhat more explicit. So if you don't have any strong objections I'd like to stick with it. > > + args.id =3D fence->syncpt; > > + args.thresh =3D fence->value; > > + args.timeout =3D timeout; > > + > > + while (true) { > > + err =3D ioctl(fence->drm->fd, DRM_IOCTL_TEGRA_SYNCPT_WA= IT, &args); > > + if (err < 0) { > > + if (errno =3D=3D EINTR) > > + continue; > > + > > + drmMsg("DRM_IOCTL_TEGRA_SYNCPT_WAIT: %d\n", -er= rno); >=20 > What's the reason for printing the errno negated? And could we do > '...%s\n" strerror(errno));' instead? Yeah, strerror(errno) would be preferable. On second thought maybe I should drop that message altogether since we return -errno anyway and therefore have access to it in the caller who can then decide to print a message or not. > > +int drm_tegra_job_add_reloc(struct drm_tegra_job *job, > > + const struct drm_tegra_reloc *reloc) > > +{ > > + struct drm_tegra_reloc *relocs; > > + size_t size; > > + > > + size =3D (job->num_relocs + 1) * sizeof(*reloc); > > + > > + relocs =3D realloc(job->relocs, size); >=20 > Nit: there's no point in not assigning those while declaring them, no? >=20 > size_t size =3D (job->num_relocs + 1) * sizeof(*reloc); > struct drm_tegra_reloc *relocs; =3D realloc(job->relocs, size); In my opinion that's a lot of clutter and very hard to read. So it's really just a matter of preferred coding style. > > +drm_public > > +int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbufp, > > + struct drm_tegra_job *job, > > + struct drm_tegra_bo *bo, > > + unsigned long offset) > > +{ > > + struct drm_tegra_pushbuf_private *pushbuf; > > + void *ptr; > > + int err; > > + > > + pushbuf =3D calloc(1, sizeof(*pushbuf)); > > + if (!pushbuf) > > + return -ENOMEM; > > + > > + pushbuf->bo =3D drm_tegra_bo_get(bo); > > + DRMINITLISTHEAD(&pushbuf->list); > > + pushbuf->job =3D job; > > + > > + err =3D drm_tegra_bo_map(bo, &ptr); > > + if (err < 0) { > > + drm_tegra_bo_put(bo); > > + free(pushbuf); > > + return err; > > + } > > + > > + pushbuf->start =3D pushbuf->base.ptr =3D ptr + offset; > > + pushbuf->offset =3D offset; > > + > > + DRMLISTADD(&pushbuf->list, &job->pushbufs); > > + job->num_pushbufs++; > > + > > + *pushbufp =3D &pushbuf->base; > > + > > + return 0; > > +} >=20 > It feels quite wasteful to me to have to allocate a new pushbuf in > order to be able to use a new BO. I'd much rather see the pushbuf > being a persisting object that's the interface to the command-stream > (that produces jobs). >=20 > I was thinking something like: >=20 > int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbufp, struct > drm_tegra_job *job) > int drm_tegra_pushbuf_room(struct drm_tegra_pushbuf *pushbuf, int num_wor= ds); >=20 > Where room guarantees that there's space for those words in the > pushbuf. A simple implementation could just allocate a bo of that > size, but a slightly more sophisticated one can allocate larger ones > and reuse them. Even more sophisticated ones could keep old cmdbufs > around and reuse them once the hardware is done reading them, do > exponential grow-factors etc. Okay, so you suggest that the backing buffer objects are handled entirely by the push buffer implementation? Yeah, I think that makes a lot of sense actually. > I've implemented the "slightly more sophisticated" approach here: >=20 > https://github.com/grate-driver/libdrm/commit/f90ea2f57ca4d8c81768402900c= 663ce526bac11 >=20 > In my implementation, I've changed the job-structure to build the list > of cmdbufs directly rather than keeping a list of the pushbufs. Sure, > that means another allocation every time we need a new cmdbuf, but > hopefully we should be able to produce much less of them this way. Okay, I'll try to integrate your implementation. It looks somewhat complex but still manageable. The important part at this point is to get the API right. That way we can still implement whatever complex scheme we want underneath. > > +int drm_tegra_pushbuf_relocate(struct drm_tegra_pushbuf *pushbuf, > > + struct drm_tegra_bo *target, > > + unsigned long offset, > > + unsigned long shift) > > +{ > > + struct drm_tegra_pushbuf_private *priv =3D pushbuf_priv(pushbuf= ); > > + struct drm_tegra_reloc reloc; > > + int err; > > + > > + memset(&reloc, 0, sizeof(reloc)); > > + reloc.cmdbuf.handle =3D priv->bo->handle; > > + reloc.cmdbuf.offset =3D drm_tegra_pushbuf_get_offset(pushbuf); > > + reloc.target.handle =3D target->handle; > > + reloc.target.offset =3D offset; > > + reloc.shift =3D shift; > > + > > + err =3D drm_tegra_job_add_reloc(priv->job, &reloc); > > + if (err < 0) > > + return err; > > + > > + return 0; > > +} >=20 > Whenever we insert a reloc, we also insert a DEADBEEF in the command > stream. Why not formalize this into this function? That's a good idea. Thierry --IS0zKkzwUGydFO0o Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJTBR4eAAoJEN0jrNd/PrOhv5kP/j0gQXo749mmV3O1vx7/TC7j c242tWp+avEfkX0DXzmm2j4z+SgZae+JhPLIUDoBmwMKFrCX8rLg4vV068uSy9NH DJSIRw49N7VLL9ni4wV6b8eckKjGQIdATu99SwqL9ROCjwgU7jWF1+or22hWtdyo VQgRRJHrVvYK7PRYnibTLR1d3lvEEtQYn6R3oHTeGe60v2fCN7zB6KdXi2NoUeuK zItKtKXfbqlhelTDSjpINWEy4Y0iPHcyRNL5l5xGrdY2tCyIe8cw7oSEgMCeoMNY obbW/9cb9LGVL1EwRZ4wnkNmXiIuw/33BadV87jAY7zCY5kvox8mLm9R5839wQEp 7iWwGsd2quNXJv1Efizwnv6SFFtGJ4GjJl3GstqgSx1yJV0O5urjg3r81O5aLZA4 z+112Yx19/GZKrBr66Zx0xQ629lxwfeD4qYZSBtEN8o6+vx5goZwp7dWx3CUretB qCFgz7VgQxIuu9zdDB2cXI5nPCANJzuJYHEpJx4OAGoLFrt9w/rr6Jc/dCKTDvMq bOISebV2GiiAzxd/IltPc+f+WTRPE5NDxETcN/vaVwBKbKL9VlbCDtEAggdZxHNK bfZ8+P+yk7u3sVo1SR9cdwI0+xrOVZ5+6QPJdFBKEKIaQ/2ZJmFlErCKBEH5EwE+ 17o3IFpACn7TfI5oW/OV =JpJt -----END PGP SIGNATURE----- --IS0zKkzwUGydFO0o-- --===============0431314122== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============0431314122==--