* [PATCH i-g-t] tests/kms_atomic: test that TEST_ONLY does not clobber state
@ 2017-02-17 13:39 Mika Kahola
2017-03-08 14:08 ` Maarten Lankhorst
0 siblings, 1 reply; 3+ messages in thread
From: Mika Kahola @ 2017-02-17 13:39 UTC (permalink / raw)
To: intel-gfx
We need to make sure that TEST_ONLY really only touches the free-standing
state objects and nothing else. Test approach here is the following:
- Create a config and submit it with TEST_ONLY.
- do dpms off/on cycle with the current config to reconfigure hw
- read back all legacy state to make sure none of that is clobbered
Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
tests/kms_atomic.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 79 insertions(+)
diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
index d6273f4..3531fa4 100644
--- a/tests/kms_atomic.c
+++ b/tests/kms_atomic.c
@@ -831,6 +831,25 @@ static uint32_t plane_get_igt_format(struct kms_atomic_plane_state *plane)
return ret;
}
+static void
+set_dpms(int fd, int mode)
+{
+ int i;
+ drmModeConnector *connector;
+ uint32_t id;
+ drmModeRes *resources = drmModeGetResources(fd);
+
+ for (i = 0; i < resources->count_connectors; i++) {
+ id = resources->connectors[i];
+
+ connector = drmModeGetConnectorCurrent(fd, id);
+
+ kmstest_set_connector_dpms(fd, connector, mode);
+
+ drmModeFreeConnector(connector);
+ }
+}
+
static void plane_overlay(struct kms_atomic_crtc_state *crtc,
struct kms_atomic_plane_state *plane_old)
{
@@ -930,6 +949,54 @@ static void plane_primary(struct kms_atomic_crtc_state *crtc,
drmModeAtomicFree(req);
}
+static void plane_primary_state_check(struct kms_atomic_crtc_state *crtc,
+ struct kms_atomic_plane_state *plane_old)
+{
+ struct drm_mode_modeinfo *mode = crtc->mode.data;
+ struct kms_atomic_plane_state plane = *plane_old;
+ uint32_t format = plane_get_igt_format(&plane);
+ drmModeAtomicReq *req = drmModeAtomicAlloc();
+ struct igt_fb fb;
+ int ret;
+
+ igt_require(format != 0);
+
+ plane.src_x = 0;
+ plane.src_y = 0;
+ plane.src_w = mode->hdisplay << 16;
+ plane.src_h = mode->vdisplay << 16;
+ plane.crtc_x = 0;
+ plane.crtc_y = 0;
+ plane.crtc_w = mode->hdisplay;
+ plane.crtc_h = mode->vdisplay;
+ plane.crtc_id = crtc->obj;
+ plane.fb_id = igt_create_pattern_fb(plane.state->desc->fd,
+ plane.crtc_w, plane.crtc_h,
+ format, I915_TILING_NONE, &fb);
+
+ drmModeAtomicSetCursor(req, 0);
+ crtc_populate_req(crtc, req);
+ plane_populate_req(&plane, req);
+ ret = drmModeAtomicCommit(crtc->state->desc->fd, req,
+ DRM_MODE_ATOMIC_TEST_ONLY, NULL);
+
+ igt_assert_eq(ret, 0);
+
+ /* go through dpms off/on cycle */
+ set_dpms(crtc->state->desc->fd, DRM_MODE_DPMS_OFF);
+ set_dpms(crtc->state->desc->fd, DRM_MODE_DPMS_ON);
+
+ /* check the state */
+ crtc_check_current_state(crtc, plane_old, CRTC_RELAX_MODE);
+ plane_check_current_state(plane_old, CRTC_RELAX_MODE);
+
+ /* Re-enable the plane through the legacy CRTC/primary-plane API, and
+ * verify through atomic. */
+ crtc_commit_legacy(crtc, plane_old, CRTC_RELAX_MODE);
+
+ drmModeAtomicFree(req);
+}
+
static void plane_cursor(struct kms_atomic_crtc_state *crtc,
struct kms_atomic_plane_state *plane_old)
{
@@ -1427,6 +1494,18 @@ igt_main
atomic_state_free(scratch);
}
+ igt_subtest("plane_primary_state_check") {
+ struct kms_atomic_state *scratch = atomic_state_dup(current);
+ struct kms_atomic_crtc_state *crtc = find_crtc(scratch, true);
+ struct kms_atomic_plane_state *plane =
+ find_plane(scratch, PLANE_TYPE_PRIMARY, crtc);
+
+ igt_require(crtc);
+ igt_require(plane);
+ plane_primary_state_check(crtc, plane);
+ atomic_state_free(scratch);
+ }
+
igt_subtest("plane_cursor_legacy") {
struct kms_atomic_state *scratch = atomic_state_dup(current);
struct kms_atomic_crtc_state *crtc = find_crtc(scratch, true);
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH i-g-t] tests/kms_atomic: test that TEST_ONLY does not clobber state
2017-02-17 13:39 [PATCH i-g-t] tests/kms_atomic: test that TEST_ONLY does not clobber state Mika Kahola
@ 2017-03-08 14:08 ` Maarten Lankhorst
2017-03-08 14:43 ` Mika Kahola
0 siblings, 1 reply; 3+ messages in thread
From: Maarten Lankhorst @ 2017-03-08 14:08 UTC (permalink / raw)
To: Mika Kahola, intel-gfx
Op 17-02-17 om 14:39 schreef Mika Kahola:
> We need to make sure that TEST_ONLY really only touches the free-standing
> state objects and nothing else. Test approach here is the following:
>
> - Create a config and submit it with TEST_ONLY.
> - do dpms off/on cycle with the current config to reconfigure hw
> - read back all legacy state to make sure none of that is clobbered
>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
> tests/kms_atomic.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 79 insertions(+)
>
> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
> index d6273f4..3531fa4 100644
> --- a/tests/kms_atomic.c
> +++ b/tests/kms_atomic.c
> @@ -831,6 +831,25 @@ static uint32_t plane_get_igt_format(struct kms_atomic_plane_state *plane)
> return ret;
> }
>
> +static void
> +set_dpms(int fd, int mode)
> +{
> + int i;
> + drmModeConnector *connector;
> + uint32_t id;
> + drmModeRes *resources = drmModeGetResources(fd);
> +
> + for (i = 0; i < resources->count_connectors; i++) {
> + id = resources->connectors[i];
> +
> + connector = drmModeGetConnectorCurrent(fd, id);
> +
> + kmstest_set_connector_dpms(fd, connector, mode);
> +
> + drmModeFreeConnector(connector);
> + }
> +}
> +
> static void plane_overlay(struct kms_atomic_crtc_state *crtc,
> struct kms_atomic_plane_state *plane_old)
> {
> @@ -930,6 +949,54 @@ static void plane_primary(struct kms_atomic_crtc_state *crtc,
> drmModeAtomicFree(req);
> }
>
> +static void plane_primary_state_check(struct kms_atomic_crtc_state *crtc,
> + struct kms_atomic_plane_state *plane_old)
> +{
> + struct drm_mode_modeinfo *mode = crtc->mode.data;
> + struct kms_atomic_plane_state plane = *plane_old;
> + uint32_t format = plane_get_igt_format(&plane);
> + drmModeAtomicReq *req = drmModeAtomicAlloc();
> + struct igt_fb fb;
> + int ret;
> +
> + igt_require(format != 0);
> +
> + plane.src_x = 0;
> + plane.src_y = 0;
> + plane.src_w = mode->hdisplay << 16;
> + plane.src_h = mode->vdisplay << 16;
> + plane.crtc_x = 0;
> + plane.crtc_y = 0;
> + plane.crtc_w = mode->hdisplay;
> + plane.crtc_h = mode->vdisplay;
> + plane.crtc_id = crtc->obj;
> + plane.fb_id = igt_create_pattern_fb(plane.state->desc->fd,
> + plane.crtc_w, plane.crtc_h,
> + format, I915_TILING_NONE, &fb);
> +
> + drmModeAtomicSetCursor(req, 0);
> + crtc_populate_req(crtc, req);
> + plane_populate_req(&plane, req);
> + ret = drmModeAtomicCommit(crtc->state->desc->fd, req,
> + DRM_MODE_ATOMIC_TEST_ONLY, NULL);
> +
> + igt_assert_eq(ret, 0);
> +
> + /* go through dpms off/on cycle */
> + set_dpms(crtc->state->desc->fd, DRM_MODE_DPMS_OFF);
> + set_dpms(crtc->state->desc->fd, DRM_MODE_DPMS_ON);
Is this required? If the state was changed, shouldn't we be able to tell through the properties?
> + /* check the state */
> + crtc_check_current_state(crtc, plane_old, CRTC_RELAX_MODE);
> + plane_check_current_state(plane_old, CRTC_RELAX_MODE);
Copy paste the relax states? Are the RELAXes required since you only set/unset the current mode?
> + /* Re-enable the plane through the legacy CRTC/primary-plane API, and
> + * verify through atomic. */
> + crtc_commit_legacy(crtc, plane_old, CRTC_RELAX_MODE);
> +
> + drmModeAtomicFree(req);
> +}
> +
> static void plane_cursor(struct kms_atomic_crtc_state *crtc,
> struct kms_atomic_plane_state *plane_old)
> {
> @@ -1427,6 +1494,18 @@ igt_main
> atomic_state_free(scratch);
> }
>
> + igt_subtest("plane_primary_state_check") {
> + struct kms_atomic_state *scratch = atomic_state_dup(current);
> + struct kms_atomic_crtc_state *crtc = find_crtc(scratch, true);
> + struct kms_atomic_plane_state *plane =
> + find_plane(scratch, PLANE_TYPE_PRIMARY, crtc);
> +
> + igt_require(crtc);
> + igt_require(plane);
> + plane_primary_state_check(crtc, plane);
> + atomic_state_free(scratch);
> + }
Test (and function) name doesn't match the description. It's hard to tell what this function is doing from the name. :)
> +
> igt_subtest("plane_cursor_legacy") {
> struct kms_atomic_state *scratch = atomic_state_dup(current);
> struct kms_atomic_crtc_state *crtc = find_crtc(scratch, true);
With some improvements, test looks sane. :)
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH i-g-t] tests/kms_atomic: test that TEST_ONLY does not clobber state
2017-03-08 14:08 ` Maarten Lankhorst
@ 2017-03-08 14:43 ` Mika Kahola
0 siblings, 0 replies; 3+ messages in thread
From: Mika Kahola @ 2017-03-08 14:43 UTC (permalink / raw)
To: Maarten Lankhorst, intel-gfx
On Wed, 2017-03-08 at 15:08 +0100, Maarten Lankhorst wrote:
> Op 17-02-17 om 14:39 schreef Mika Kahola:
> >
> > We need to make sure that TEST_ONLY really only touches the free-
> > standing
> > state objects and nothing else. Test approach here is the
> > following:
> >
> > - Create a config and submit it with TEST_ONLY.
> > - do dpms off/on cycle with the current config to reconfigure hw
> > - read back all legacy state to make sure none of that is clobbered
> >
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> > tests/kms_atomic.c | 79
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 79 insertions(+)
> >
> > diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
> > index d6273f4..3531fa4 100644
> > --- a/tests/kms_atomic.c
> > +++ b/tests/kms_atomic.c
> > @@ -831,6 +831,25 @@ static uint32_t plane_get_igt_format(struct
> > kms_atomic_plane_state *plane)
> > return ret;
> > }
> >
> > +static void
> > +set_dpms(int fd, int mode)
> > +{
> > + int i;
> > + drmModeConnector *connector;
> > + uint32_t id;
> > + drmModeRes *resources = drmModeGetResources(fd);
> > +
> > + for (i = 0; i < resources->count_connectors; i++) {
> > + id = resources->connectors[i];
> > +
> > + connector = drmModeGetConnectorCurrent(fd, id);
> > +
> > + kmstest_set_connector_dpms(fd, connector, mode);
> > +
> > + drmModeFreeConnector(connector);
> > + }
> > +}
> > +
> > static void plane_overlay(struct kms_atomic_crtc_state *crtc,
> > struct kms_atomic_plane_state
> > *plane_old)
> > {
> > @@ -930,6 +949,54 @@ static void plane_primary(struct
> > kms_atomic_crtc_state *crtc,
> > drmModeAtomicFree(req);
> > }
> >
> > +static void plane_primary_state_check(struct kms_atomic_crtc_state
> > *crtc,
> > + struct
> > kms_atomic_plane_state *plane_old)
> > +{
> > + struct drm_mode_modeinfo *mode = crtc->mode.data;
> > + struct kms_atomic_plane_state plane = *plane_old;
> > + uint32_t format = plane_get_igt_format(&plane);
> > + drmModeAtomicReq *req = drmModeAtomicAlloc();
> > + struct igt_fb fb;
> > + int ret;
> > +
> > + igt_require(format != 0);
> > +
> > + plane.src_x = 0;
> > + plane.src_y = 0;
> > + plane.src_w = mode->hdisplay << 16;
> > + plane.src_h = mode->vdisplay << 16;
> > + plane.crtc_x = 0;
> > + plane.crtc_y = 0;
> > + plane.crtc_w = mode->hdisplay;
> > + plane.crtc_h = mode->vdisplay;
> > + plane.crtc_id = crtc->obj;
> > + plane.fb_id = igt_create_pattern_fb(plane.state->desc->fd,
> > + plane.crtc_w,
> > plane.crtc_h,
> > + format,
> > I915_TILING_NONE, &fb);
> > +
> > + drmModeAtomicSetCursor(req, 0);
> > + crtc_populate_req(crtc, req);
> > + plane_populate_req(&plane, req);
> > + ret = drmModeAtomicCommit(crtc->state->desc->fd, req,
> > + DRM_MODE_ATOMIC_TEST_ONLY,
> > NULL);
> > +
> > + igt_assert_eq(ret, 0);
> > +
> > + /* go through dpms off/on cycle */
> > + set_dpms(crtc->state->desc->fd, DRM_MODE_DPMS_OFF);
> > + set_dpms(crtc->state->desc->fd, DRM_MODE_DPMS_ON);
> Is this required? If the state was changed, shouldn't we be able to
> tell through the properties?
I was trying to play safe here and that's why I chose to go through the
whole off/on cycle.
> >
> > + /* check the state */
> > + crtc_check_current_state(crtc, plane_old,
> > CRTC_RELAX_MODE);
> > + plane_check_current_state(plane_old, CRTC_RELAX_MODE);
> Copy paste the relax states? Are the RELAXes required since you only
> set/unset the current mode?
yep, this would probably work if changed to ATOMIC_RELAX_NONE.
> >
> > + /* Re-enable the plane through the legacy CRTC/primary-
> > plane API, and
> > + * verify through atomic. */
> > + crtc_commit_legacy(crtc, plane_old, CRTC_RELAX_MODE);
> > +
> > + drmModeAtomicFree(req);
> > +}
> > +
> > static void plane_cursor(struct kms_atomic_crtc_state *crtc,
> > struct kms_atomic_plane_state *plane_old)
> > {
> > @@ -1427,6 +1494,18 @@ igt_main
> > atomic_state_free(scratch);
> > }
> >
> > + igt_subtest("plane_primary_state_check") {
> > + struct kms_atomic_state *scratch =
> > atomic_state_dup(current);
> > + struct kms_atomic_crtc_state *crtc =
> > find_crtc(scratch, true);
> > + struct kms_atomic_plane_state *plane =
> > + find_plane(scratch, PLANE_TYPE_PRIMARY,
> > crtc);
> > +
> > + igt_require(crtc);
> > + igt_require(plane);
> > + plane_primary_state_check(crtc, plane);
> > + atomic_state_free(scratch);
> > + }
> Test (and function) name doesn't match the description. It's hard to
> tell what this function is doing from the name. :)
I'm bad at naming things. I'll try to figure out better name.
> >
> > +
> > igt_subtest("plane_cursor_legacy") {
> > struct kms_atomic_state *scratch =
> > atomic_state_dup(current);
> > struct kms_atomic_crtc_state *crtc =
> > find_crtc(scratch, true);
> With some improvements, test looks sane. :)
Thanks for the review!
Cheers,
Mika
>
> ~Maarten
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-03-08 14:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-17 13:39 [PATCH i-g-t] tests/kms_atomic: test that TEST_ONLY does not clobber state Mika Kahola
2017-03-08 14:08 ` Maarten Lankhorst
2017-03-08 14:43 ` Mika Kahola
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.