On Fri, May 2, 2014 at 4:42 PM, Thierry Reding thierry.reding@gmail.com wrote:
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 thierry.reding@gmail.com wrote:
From: Thierry Reding treding@nvidia.com
This library provides helpers for common functionality needed by test programs.
Signed-off-by: Thierry Reding treding@nvidia.com
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
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.
Right. Let's just leave it for later, then.
+int drm_tegra_gr2d_fill(struct drm_tegra_gr2d *gr2d, struct drm_framebuffer *fb,
unsigned int x, unsigned int y, unsigned int width,unsigned int height, uint32_t color)+{
struct drm_tegra_bo *fbo = fb->data;struct drm_tegra_pushbuf *pushbuf;struct drm_tegra_fence *fence;struct drm_tegra_job *job;int err;err = drm_tegra_job_new(&job, gr2d->channel);if (err < 0)return err;err = drm_tegra_pushbuf_new(&pushbuf, job);if (err < 0)return err;I think this helper would be generally more useful if it skipped the above two, and required the call-sites to do them instead.
err = drm_tegra_pushbuf_prepare(pushbuf, 32);if (err < 0)return err;*pushbuf->ptr++ = HOST1X_OPCODE_SETCL(0, HOST1X_CLASS_GR2D, 0);*pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x9, 0x9);*pushbuf->ptr++ = 0x0000003a;*pushbuf->ptr++ = 0x00000000;*pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x1e, 0x7);*pushbuf->ptr++ = 0x00000000;*pushbuf->ptr++ = (2 << 16) | (1 << 6) | (1 << 2);*pushbuf->ptr++ = 0x000000cc;*pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x2b, 0x9);/* relocate destination buffer */err = 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++ = fb->pitch;*pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x35, 1);*pushbuf->ptr++ = color;*pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x46, 1);*pushbuf->ptr++ = 0x00000000;*pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x38, 0x5);*pushbuf->ptr++ = height << 16 | width;*pushbuf->ptr++ = y << 16 | x;...and stop here.
That way we can use it for tests that perform multiple fills in one submit 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?
I wouldn't be against such a convenience-wrapper, no. I would personally leave that for the person who added more such tests, though. I guess that could be said about my suggestion as well ;-)
+#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
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.
OK. That's a bit of a shame though, as it means "everyone" (xf86-video-opentegra and mesa + grate tests) must carry these definitions. But it's not important, and can be fixed later if it turns out to be a nuisance.
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 = drmModeGetEncoder(screen->fd, connector->encoder_id);if (!encoder)return -ENODEV;crtc = drmModeGetCrtc(screen->fd, encoder->crtc_id);if (!crtc) {drmModeFreeEncoder(encoder);return -ENODEV;}screen->old_fb = crtc->buffer_id;fb = drmModeGetFB(screen->fd, crtc->buffer_id);if (!fb) {/* TODO: create new framebuffer */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.
Right. Perhaps it could happen if no monitor is connected? That's probably not important, though.