From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH v2 libdrm 5/7] tegra: Add helper library for tests Date: Fri, 2 May 2014 16:42:39 +0200 Message-ID: <20140502144237.GE21515@ulmo> References: <1397043630-13972-1-git-send-email-thierry.reding@gmail.com> <1397043630-13972-6-git-send-email-thierry.reding@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0924943250==" Return-path: Received: from mail-ee0-f42.google.com (mail-ee0-f42.google.com [74.125.83.42]) by gabe.freedesktop.org (Postfix) with ESMTP id 1F5196E3C8 for ; Fri, 2 May 2014 07:44:24 -0700 (PDT) Received: by mail-ee0-f42.google.com with SMTP id d17so3241839eek.29 for ; Fri, 02 May 2014 07:44:24 -0700 (PDT) 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: Erik Faye-Lund Cc: "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org --===============0924943250== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Ns7jmDPpOpCD+GE/" Content-Disposition: inline --Ns7jmDPpOpCD+GE/ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Apr 10, 2014 at 07:33:33PM +0200, Erik Faye-Lund wrote: > On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding = wrote: > > From: Thierry Reding > > > > This library provides helpers for common functionality needed by test > > programs. > > > > Signed-off-by: Thierry Reding > > --- > > Changes in v2: > > - fix a couple of memory leaks and get rid of some unneeded code > > > > tests/tegra/Makefile.am | 10 +- > > tests/tegra/drm-test-tegra.c | 137 ++++++++++++++++++++++++ > > tests/tegra/drm-test-tegra.h | 55 ++++++++++ > > tests/tegra/drm-test.c | 248 +++++++++++++++++++++++++++++++++++= ++++++++ > > tests/tegra/drm-test.h | 72 +++++++++++++ > > 5 files changed, 521 insertions(+), 1 deletion(-) > > create mode 100644 tests/tegra/drm-test-tegra.c > > create mode 100644 tests/tegra/drm-test-tegra.h > > create mode 100644 tests/tegra/drm-test.c > > create mode 100644 tests/tegra/drm-test.h >=20 > This isn't really important at this point, but it looks to me like > tests/tegra/drm-test.[ch] isn't really tegra-specific. If so, perhaps > some other tests can benefit from it? Doing so is of course something > whoever writes those tests should do, though. Leaving them in the > tegra-subdir is probably fine. Daniel Vetter and I have been "discussing" this for a while now. There are a bunch of tests in the intel-gpu-tools repository that aren't Intel specific either and the idea is to eventually collect all those test cases in a common location so that they can be reused by other drivers too, but so far nobody's had time to do that yet. > > +int drm_tegra_gr2d_fill(struct drm_tegra_gr2d *gr2d, struct drm_frameb= uffer *fb, > > + unsigned int x, unsigned int y, unsigned int wi= dth, > > + unsigned int height, uint32_t color) > > +{ > > + struct drm_tegra_bo *fbo =3D fb->data; > > + struct drm_tegra_pushbuf *pushbuf; > > + struct drm_tegra_fence *fence; > > + struct drm_tegra_job *job; > > + int err; > > + > > + err =3D drm_tegra_job_new(&job, gr2d->channel); > > + if (err < 0) > > + return err; > > + > > + err =3D drm_tegra_pushbuf_new(&pushbuf, job); > > + if (err < 0) > > + return err; > > + >=20 > I think this helper would be generally more useful if it skipped the > above two, and required the call-sites to do them instead. >=20 > > + err =3D drm_tegra_pushbuf_prepare(pushbuf, 32); > > + if (err < 0) > > + return err; > > + > > + *pushbuf->ptr++ =3D HOST1X_OPCODE_SETCL(0, HOST1X_CLASS_GR2D, 0= ); > > + > > + *pushbuf->ptr++ =3D HOST1X_OPCODE_MASK(0x9, 0x9); > > + *pushbuf->ptr++ =3D 0x0000003a; > > + *pushbuf->ptr++ =3D 0x00000000; > > + > > + *pushbuf->ptr++ =3D HOST1X_OPCODE_MASK(0x1e, 0x7); > > + *pushbuf->ptr++ =3D 0x00000000; > > + *pushbuf->ptr++ =3D (2 << 16) | (1 << 6) | (1 << 2); > > + *pushbuf->ptr++ =3D 0x000000cc; > > + > > + *pushbuf->ptr++ =3D HOST1X_OPCODE_MASK(0x2b, 0x9); > > + > > + /* relocate destination buffer */ > > + err =3D drm_tegra_pushbuf_relocate(pushbuf, fbo, 0, 0); > > + if (err < 0) { > > + fprintf(stderr, "failed to relocate buffer object: %d\n= ", err); > > + return err; > > + } > > + > > + *pushbuf->ptr++ =3D fb->pitch; > > + > > + *pushbuf->ptr++ =3D HOST1X_OPCODE_NONINCR(0x35, 1); > > + *pushbuf->ptr++ =3D color; > > + > > + *pushbuf->ptr++ =3D HOST1X_OPCODE_NONINCR(0x46, 1); > > + *pushbuf->ptr++ =3D 0x00000000; > > + > > + *pushbuf->ptr++ =3D HOST1X_OPCODE_MASK(0x38, 0x5); > > + *pushbuf->ptr++ =3D height << 16 | width; > > + *pushbuf->ptr++ =3D y << 16 | x; > > + >=20 > ...and stop here. >=20 > That way we can use it for tests that perform multiple fills in one submi= t etc. How about we make drm_tegra_gr2d_fill() take a drm_tegra_pushbuf object and push the commands as you suggest and then add a wrapper, say drm_tegra_gr2d_fill_simple() for convenience when a single fill per submit is good enough? > > +#define HOST1X_OPCODE_SETCL(offset, classid, mask) \ > > + ((0x0 << 28) | (((offset) & 0xfff) << 16) | (((classid) & 0x3ff) <= < 6) | ((mask) & 0x3f)) > > +#define HOST1X_OPCODE_INCR(offset, count) \ > > + ((0x1 << 28) | (((offset) & 0xfff) << 16) | ((count) & 0xffff)) > > +#define HOST1X_OPCODE_NONINCR(offset, count) \ > > + ((0x2 << 28) | (((offset) & 0xfff) << 16) | ((count) & 0xffff)) > > +#define HOST1X_OPCODE_MASK(offset, mask) \ > > + ((0x3 << 28) | (((offset) & 0xfff) << 16) | ((mask) & 0xffff)) > > +#define HOST1X_OPCODE_IMM(offset, data) \ > > + ((0x4 << 28) | (((offset) & 0xfff) << 16) | ((data) & 0xffff)) > > +#define HOST1X_OPCODE_EXTEND(subop, value) \ > > + ((0xe << 28) | (((subop) & 0xf) << 24) | ((value) & 0xffffff)) > > + > > +#define HOST1X_CLASS_GR2D 0x51 >=20 > Hmm, shouldn't these be available from somewhere else already? No > point in repeating the same macros over and over again, no? I don't think we have these anywhere else. It seems to be custom to have numerous redefinitions of this kind of macro throughout various drivers. I suspect that the reason is that these can vary depending on chipset revision and keeping a global list would require a revision prefix to be prepended to all of them. Even if we want them somewhere else, I wouldn't know where they'd be best kept to be honest. > > diff --git a/tests/tegra/drm-test.c b/tests/tegra/drm-test.c [...] > > +static int drm_screen_probe_connector(struct drm_screen *screen, > > + drmModeConnectorPtr connector) > > +{ > > + drmModeEncoderPtr encoder; > > + drmModeCrtcPtr crtc; > > + drmModeFBPtr fb; > > + > > + encoder =3D drmModeGetEncoder(screen->fd, connector->encoder_id= ); > > + if (!encoder) > > + return -ENODEV; > > + > > + crtc =3D drmModeGetCrtc(screen->fd, encoder->crtc_id); > > + if (!crtc) { > > + drmModeFreeEncoder(encoder); > > + return -ENODEV; > > + } > > + > > + screen->old_fb =3D crtc->buffer_id; > > + > > + fb =3D drmModeGetFB(screen->fd, crtc->buffer_id); > > + if (!fb) { > > + /* TODO: create new framebuffer */ >=20 > What's the implications of not doing what this TODO says? It currently just means that we don't want to deal with this situation, which I think shouldn't be happening in the first place anyway. So the TODO is there mostly as a reminder that this could happen and that there *might* be something more useful than returning an error that could be done. Thierry --Ns7jmDPpOpCD+GE/ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJTY67dAAoJEN0jrNd/PrOhjDEQAIms7vt8zyPQ/UgpjQCoAqHd VBrHX7YRoQlKR07gOaBmX2IOgObmesCJ2ZUyBNuQv91eBKEjq5+Ynqt4/rFzc4Hs Wp0Sy8GEQxBOO0FVkQIcFUtJlDncVceQsgzw5osDU2JQiXUf0pabMEsphLgVV+Mw CGNnYMnBxkCqdHTBVzLv/l/Kw63QgoYwWhkxk6aNN7TFLxaUP/m6tkA9ttQoaVaW DWm+LFnanhGtWUGoh0coEe5NcztfaBBjvTu09Enap/KjkkRh8nipV5GaeBqxMdt7 IJ2OypjVOQP0q84oSOtka9t/S9fEXSS8CKVH03WwY8Qv54N8GhmSEJo87mUDgoQb nocEsjjYzPF5F9hYQF8tUpjFcoi1GzGCxdsEVusaBm87Phf3QqT+6MYxuQjgsvi1 k00KH3tdL8axMFICB3QCpN3htloDGLXnuIeOheG6O8j/7oi5yESR2FFmN1Jn24WA 6KGsZ2U/85JwRsZAwzMcgthN2CPtVPhFcPumWtTMC6MAvMu+R6MqZDKYWgXK137g x0VJ/EJieL+G3xjAxKOQjv+QpXgBf02paVskns0j7EVKQLz+b48FH9YonhxCuoei 5Y0AYLJ8XJXnacoorVwPuKL/iP6acSg1JWkCtDpEhZ15PR4A0ZmI/CeiRAP5WwyN i0fHoKExmOEFeTXgW9vL =Z04u -----END PGP SIGNATURE----- --Ns7jmDPpOpCD+GE/-- --===============0924943250== 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 --===============0924943250==--