[Intel-gfx] [PATCH i-g-t] lib/igt_kms: Add COMIT_ATOMIC to igt_display_commit2()
Palleti, Avinash Reddy
avinash.reddy.palleti at intel.com
Thu Jan 21 21:43:14 PST 2016
Thanks Matt for pointing me to this.
Vlad,
As Matt mentioned, we are also working on this to get atomic support in i-g-t. Last week we finalized the design with Matt. I am putting the design we discussed here for reference,
- New commit style will be added as "COMMIT_NUCLEAR"
- If the commit style is nuclear we will use libdrm interface to build the input structure by taking each property(drmModeAtomicAddProperty)
- Above libdrm interface will create the list of properties and add each property to list whenever called
- Once all properties are added into list, call drmModeAtomicCommit() to do final commit.
- There will be configuration variable or environment variable which specifies commit style (e.g., IGT_COMMIT_STYLE) that will allow to override the commit style of existing IGT tests.
Matt,
As far as I know both nuclear and atomic are same, and they mean commit per CRTC. Though userspace do commit once at top level for multiple CRTC together, Kernel will internally commit once per CRTC. So no need of two commit styles to be exposed in IGT.
Thanks,
Avinash
-----Original Message-----
From: Roper, Matthew D
Sent: Friday, January 22, 2016 12:28 AM
To: Vlad, Marius C <marius.c.vlad at intel.com>
Cc: intel-gfx at lists.freedesktop.org; Palleti, Avinash Reddy <avinash.reddy.palleti at intel.com>
Subject: Re: [Intel-gfx] [PATCH i-g-t] lib/igt_kms: Add COMIT_ATOMIC to igt_display_commit2()
On Fri, Jan 15, 2016 at 11:06:24AM +0200, Marius Vlad wrote:
> So far, we had only COMMIT_UNIVERSAL and COMMIT_LEGACY, using
> drmModeSetPlane()/drmSetCrtc(). This patch adds COMMIT_ATOMIC to
> igt_display_commit2() that makes use of drmModeAtomicCommit().
>
> Signed-off-by: Marius Vlad <marius.c.vlad at intel.com>
Hmm, this looks pretty similar to the IGT infrastructure I added for nuclear/atomic about a year ago:
https://github.com/mattrope/intel-gpu-tools/commits/nuclear_pageflip
Specifically the commit "lib/kms: Add nuclear pageflip commit style"
At the time atomic was still too new and the various pieces like libdrm weren't readily available so we didn't actually merge it. Then I got too busy and never updated/merged it once the API finalized.
One of the VPG validation teams was in the process of resurrecting my old IGT work and updating it to match the small changes that snuck in as the API finalized; it may make more sense for them to leverage your work here instead since it's already up-to-date. +Cc Avinash.
Also, as noted by Maarten, you're calling drmModeAtomicCommit() at the wrong layer and submitting every single plane update as its own transaction (and missing the CRTC-level updates?). You either want to call commit once per-top level update (for true atomic) or once per CRTC update (for the "nuclear pageflip" subset). I don't think I had it in my github repo, but I had some patches that made these two separate commit styles, COMMIT_ATOMIC and COMMIT_NUCLEAR that would allow either of these styles to be used.
Matt
> ---
> lib/igt_kms.c | 190
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> lib/igt_kms.h | 33 +++++++++-
> 2 files changed, 221 insertions(+), 2 deletions(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 497118a..61f7a39
> 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1306,6 +1306,191 @@ static uint32_t igt_plane_get_fb_gem_handle(igt_plane_t *plane)
> igt_assert(r == 0); \
> }
>
> +static const char *igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
> + "SRC_X",
> + "SRC_Y",
> + "SRC_W",
> + "SRC_H",
> + "CRTC_X",
> + "CRTC_Y",
> + "CRTC_W",
> + "CRTC_H",
> + "FB_ID",
> + "CRTC_ID",
> + "type"
> +};
> +
> +/*
> + * Retrieve all the properies specified in props_name and store them
> +into
> + * plane->atomic_props_plane.
> + */
> +static void
> +igt_atomic_fill_plane_props(igt_display_t *display, igt_plane_t *plane,
> + int type, int num_props, const char **prop_names) {
> + drmModeObjectPropertiesPtr props;
> + int i, j, fd;
> +
> + fd = display->drm_fd;
> +
> + props = drmModeObjectGetProperties(fd, plane->drm_plane->plane_id, type);
> + igt_assert(props);
> +
> + for (i = 0; i < props->count_props; i++) {
> + drmModePropertyPtr prop =
> + drmModeGetProperty(fd, props->props[i]);
> +
> + for (j = 0; j < num_props; j++) {
> + if (strcmp(prop->name, prop_names[j]) != 0)
> + continue;
> + plane->atomic_props_plane[j] = props->props[i];
> + break;
> + }
> +
> + drmModeFreeProperty(prop);
> + }
> +
> + drmModeFreeObjectProperties(props);
> +}
> +
> +/*
> + * Commit position and fb changes to a DRM plane via the
> +AtomicCommit()
> + * ioctl; if the DRM call to program the plane fails, we'll either
> +fail
> + * immediately (for tests that expect the commit to succeed) or
> +return the
> + * failure code (for tests that expect a specific error code).
> + */
> +static int
> +igt_atomic_plane_commit(igt_plane_t *plane, igt_output_t *output,
> + bool fail_on_error)
> +{
> + igt_display_t *display = output->display;
> +
> + uint32_t fb_id, crtc_id;
> + int ret;
> + uint32_t src_x;
> + uint32_t src_y;
> + uint32_t src_w;
> + uint32_t src_h;
> + int32_t crtc_x;
> + int32_t crtc_y;
> + uint32_t crtc_w;
> + uint32_t crtc_h;
> + drmModeAtomicReq *req;
> +
> + igt_assert(plane->drm_plane);
> +
> + do_or_die(drmSetClientCap(display->drm_fd, DRM_CLIENT_CAP_ATOMIC,
> +1));
> +
> + /* it's an error to try an unsupported feature */
> + igt_assert(igt_plane_supports_rotation(plane) ||
> + !plane->rotation_changed);
> +
> + fb_id = igt_plane_get_fb_id(plane);
> + crtc_id = output->config.crtc->crtc_id;
> +
> + if ((plane->fb_changed || plane->size_changed) && fb_id == 0) {
> +
> + LOG(display,
> + "%s: drmModeAtomicCommit pipe %s, plane %d, disabling\n",
> + igt_output_name(output),
> + kmstest_pipe_name(output->config.pipe),
> + plane->index);
> +
> + req = drmModeAtomicAlloc();
> + igt_atomic_fill_plane_props(display, plane,
> + DRM_MODE_OBJECT_PLANE,
> + IGT_NUM_PLANE_PROPS,
> + igt_plane_prop_names);
> +
> + drmModeAtomicSetCursor(req, 0);
> +
> + /* populate plane req */
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_ID,
> +crtc_id);
> +
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_X, IGT_FIXED(0, 0));
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_Y, IGT_FIXED(0, 0));
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_W, IGT_FIXED(0, 0));
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_H,
> +IGT_FIXED(0, 0));
> +
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_X, 0);
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_Y, 0);
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_W, 0);
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_H, 0);
> +
> + ret = drmModeAtomicCommit(display->drm_fd, req, 0, NULL);
> + drmModeAtomicFree(req);
> +
> + CHECK_RETURN(ret, fail_on_error);
> +
> + } else if (plane->fb_changed || plane->position_changed ||
> + plane->size_changed) {
> +
> + src_x = IGT_FIXED(plane->fb->src_x,0); /* src_x */
> + src_y = IGT_FIXED(plane->fb->src_y,0); /* src_y */
> + src_w = IGT_FIXED(plane->fb->src_w,0); /* src_w */
> + src_h = IGT_FIXED(plane->fb->src_h,0); /* src_h */
> + crtc_x = plane->crtc_x;
> + crtc_y = plane->crtc_y;
> + crtc_w = plane->crtc_w;
> + crtc_h = plane->crtc_h;
> +
> + LOG(display,
> + "%s: drmModeAtomicCommit %s.%d, fb %u, src = (%d, %d) "
> + "%ux%u dst = (%u, %u) %ux%u\n",
> + igt_output_name(output),
> + kmstest_pipe_name(output->config.pipe),
> + plane->index,
> + fb_id,
> + src_x >> 16, src_y >> 16, src_w >> 16, src_h >> 16,
> + crtc_x, crtc_y, crtc_w, crtc_h);
> +
> +
> + req = drmModeAtomicAlloc();
> + igt_atomic_fill_plane_props(display, plane,
> + DRM_MODE_OBJECT_PLANE,
> + IGT_NUM_PLANE_PROPS,
> + igt_plane_prop_names);
> +
> + drmModeAtomicSetCursor(req, 0);
> +
> + /* populate plane req */
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_ID, crtc_id);
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_FB_ID, fb_id);
> +
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_X, src_x);
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_Y, src_y);
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_W, src_w);
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_SRC_H, src_h);
> +
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_X, crtc_x);
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_Y, crtc_y);
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_W, crtc_w);
> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_H,
> +crtc_h);
> +
> + ret = drmModeAtomicCommit(display->drm_fd, req, 0, NULL);
> + drmModeAtomicFree(req);
> +
> + CHECK_RETURN(ret, fail_on_error);
> + }
> +
> + plane->fb_changed = false;
> + plane->position_changed = false;
> + plane->size_changed = false;
> +
> +
> + if (plane->rotation_changed) {
> + ret = igt_plane_set_property(plane, plane->rotation_property,
> + plane->rotation);
> +
> + plane->rotation_changed = false;
> + CHECK_RETURN(ret, fail_on_error);
> + }
> +
> + return 0;
> +}
> +
> +
> +
> /*
> * Commit position and fb changes to a DRM plane via the SetPlane ioctl; if the
> * DRM call to program the plane fails, we'll either fail immediately
> (for @@ -1558,7 +1743,10 @@ static int igt_plane_commit(igt_plane_t *plane,
> return igt_primary_plane_commit_legacy(plane, output,
> fail_on_error);
> } else {
> - return igt_drm_plane_commit(plane, output, fail_on_error);
> + if (s == COMMIT_ATOMIC)
> + return igt_atomic_plane_commit(plane, output, fail_on_error);
> + else
> + return igt_drm_plane_commit(plane, output, fail_on_error);
> }
> }
>
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h index 94f315f..81d8dd7
> 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -156,9 +156,27 @@ void kmstest_unset_all_crtcs(int drm_fd,
> drmModeResPtr resources); enum igt_commit_style {
> COMMIT_LEGACY = 0,
> COMMIT_UNIVERSAL,
> - /* We'll add atomic here eventually. */
> + COMMIT_ATOMIC,
> };
>
> +enum igt_atomic_plane_properties {
> + IGT_PLANE_SRC_X = 0,
> + IGT_PLANE_SRC_Y,
> + IGT_PLANE_SRC_W,
> + IGT_PLANE_SRC_H,
> +
> + IGT_PLANE_CRTC_X,
> + IGT_PLANE_CRTC_Y,
> + IGT_PLANE_CRTC_W,
> + IGT_PLANE_CRTC_H,
> +
> + IGT_PLANE_FB_ID,
> + IGT_PLANE_CRTC_ID,
> + IGT_PLANE_TYPE,
> + IGT_NUM_PLANE_PROPS
> +};
> +
> +
> typedef struct igt_display igt_display_t; typedef struct igt_pipe
> igt_pipe_t;
> typedef uint32_t igt_fixed_t; /* 16.16 fixed point */
> @@ -200,6 +218,7 @@ typedef struct {
> /* panning offset within the fb */
> unsigned int pan_x, pan_y;
> igt_rotation_t rotation;
> + uint32_t atomic_props_plane[IGT_NUM_PLANE_PROPS];
> } igt_plane_t;
>
> struct igt_pipe {
> @@ -281,6 +300,18 @@ void igt_wait_for_vblank(int drm_fd, enum pipe
> pipe);
>
> #define IGT_FIXED(i,f) ((i) << 16 | (f))
>
> +/**
> + * igt_atomic_populate_plane_req:
> + * @req: A pointer to drmModeAtomicReq
> + * @plane: A pointer igt_plane_t
> + * @prop: one of igt_atomic_plane_properties
> + * @value: the value to add
> + */
> +#define igt_atomic_populate_plane_req(req, plane, prop, value) \
> + igt_assert_lt(0, drmModeAtomicAddProperty(req, plane->drm_plane->plane_id,\
> + plane->atomic_props_plane[prop], value))
> +
> +
> void igt_enable_connectors(void);
> void igt_reset_connectors(void);
>
> --
> 2.6.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
More information about the Intel-gfx
mailing list