| Message ID | 1487338779-11583-1-git-send-email-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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);
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(+)