| Message ID | 1452848784-7970-1-git-send-email-marius.c.vlad@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 15-01-16 om 10:06 schreef Marius Vlad: > 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@intel.com> > --- > 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); Set crtc_id and fb_id to 0 when disabling plane. > + 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); One drmModeAtomicCommit per igt_display_commit2 is enough. :) Patch is looking good otherwise, would be nice if we could get rid of the duplication with kms_atomic.c That will make it a lot easier to add more tests in the future. ~Maarten
Hi, On 20 January 2016 at 14:52, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > Op 15-01-16 om 10:06 schreef Marius Vlad: >> + /* populate plane req */ >> + igt_atomic_populate_plane_req(req, plane, IGT_PLANE_CRTC_ID, crtc_id); > Set crtc_id and fb_id to 0 when disabling plane. >> + 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); > One drmModeAtomicCommit per igt_display_commit2 is enough. :) > > Patch is looking good otherwise, would be nice if we could get rid of the duplication with kms_atomic.c > That will make it a lot easier to add more tests in the future. I agree, it'd be really nice indeed. Some of kms_atomic can't sensibly use it, since it's pushing tricky edge cases, but certainly a good chunk of the boilerplate to populate requests, get properties, etc, could be merged with this. Cheers, Daniel
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@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com> Cc: intel-gfx@lists.freedesktop.org; Palleti, Avinash Reddy <avinash.reddy.palleti@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@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@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
Hi, On Fri, Jan 22, 2016 at 07:43:14AM +0200, Palleti, Avinash Reddy wrote: > Thanks Matt for pointing me to this. > > Vlad, It's Marius :-). > 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. Hmm, I wanted to submit a new version addressing the issues raised by Maarten. Seeing that you spent some time into this I'll drop my version then. I'll wait for your patches and help with testing if you want. Thanks for the feedback! > > Thanks, > Avinash > > -----Original Message----- > From: Roper, Matthew D > Sent: Friday, January 22, 2016 12:28 AM > To: Vlad, Marius C <marius.c.vlad@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Palleti, Avinash Reddy <avinash.reddy.palleti@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@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@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
On Thu, Jan 21, 2016 at 09:43:14PM -0800, Palleti, Avinash Reddy wrote: > 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. Atomic (as an interface) allows you to submit a single propertyset that updates multiple CRTC's. Our Intel platforms don't have locked vblanks, so you don't have a guarantee that the changes will be visible at exactly the same time across the displays, but you do still get a guarantee that the commit as a whole completely succeeds or completely fails without leaving you in some kind of halfway limbo state. "Nuclear pageflip" is a subset of that greater atomic modeset functionality where you're only submitting changes that affect a single CRTC. This is interesting because it matches the behavior of most userspace compositors; a lot of compositors tend to handle each CRTC separately and submit new update transactions tied to the specific CRTC's vblanks. Some of my initial work had two new IGT commit styles, NUCLEAR and ATOMIC, because we had nuclear pageflip support in i915 well before we finished implementing full atomic modeset. These days (thanks to lots of work by Maarten and others) our kernel code supports both; the only thing we're really lacking from an interface perspective is support for non-blocking commits, and that's another issue altogether. It's probably fine to just have a single new commit style in IGT now, but I'd suggest calling it 'ATOMIC' rather than 'NUCLEAR' and make sure that it allows properties for multple CRTC's to all be committed together. If a specific IGT test wants to exercise the subset of functionality sometimes referred to as "nuclear pageflip" then that test can easily just make sure that it only updates the properties of a single CRTC before submitting the commit. Matt > > Thanks, > Avinash > > -----Original Message----- > From: Roper, Matthew D > Sent: Friday, January 22, 2016 12:28 AM > To: Vlad, Marius C <marius.c.vlad@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Palleti, Avinash Reddy <avinash.reddy.palleti@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@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@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
On Fri, Jan 22, 2016 at 08:00:10AM -0800, Matt Roper wrote: > On Thu, Jan 21, 2016 at 09:43:14PM -0800, Palleti, Avinash Reddy wrote: > > 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. > > Atomic (as an interface) allows you to submit a single propertyset that > updates multiple CRTC's. Our Intel platforms don't have locked vblanks, > so you don't have a guarantee that the changes will be visible at > exactly the same time across the displays, but you do still get a > guarantee that the commit as a whole completely succeeds or completely > fails without leaving you in some kind of halfway limbo state. > > "Nuclear pageflip" is a subset of that greater atomic modeset > functionality where you're only submitting changes that affect a single > CRTC. This is interesting because it matches the behavior of most > userspace compositors; a lot of compositors tend to handle each CRTC > separately and submit new update transactions tied to the specific > CRTC's vblanks. > > Some of my initial work had two new IGT commit styles, NUCLEAR and > ATOMIC, because we had nuclear pageflip support in i915 well before we > finished implementing full atomic modeset. These days (thanks to lots > of work by Maarten and others) our kernel code supports both; the only > thing we're really lacking from an interface perspective is support for > non-blocking commits, and that's another issue altogether. It's > probably fine to just have a single new commit style in IGT now, but I'd > suggest calling it 'ATOMIC' rather than 'NUCLEAR' and make sure that it > allows properties for multple CRTC's to all be committed together. If a > specific IGT test wants to exercise the subset of functionality > sometimes referred to as "nuclear pageflip" then that test can easily > just make sure that it only updates the properties of a single CRTC > before submitting the commit. Yeah, COMMIT_ATOMIC should also set the ALLOW_MODESET flag imo. That would reflect popular usage at least. btw another consideration is how to expose TEST_ONLY. I think best option is probably to have a new igt_commit_check (which again needs to take commit flags becaue of the nuclear vs. atomic thing) for this. -Daniel
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);
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@intel.com> --- lib/igt_kms.c | 190 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++- lib/igt_kms.h | 33 +++++++++- 2 files changed, 221 insertions(+), 2 deletions(-)