Re: [PATCH v2 libdrm 5/7] tegra: Add helper library for tests

[Thierry Reding <thierry.reding@gmail.com>]

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

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.

> > +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?

> > +#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.

> > 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.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

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

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