From: Daniel Vetter <daniel@ffwll.ch> To: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH 5/7] drm/selftests: Add some selftests for drm_atomic_set_mode_for_crtc() Date: Tue, 19 Nov 2019 11:39:53 +0100 [thread overview] Message-ID: <20191119103953.GC30416@phenom.ffwll.local> (raw) In-Reply-To: <20191115194204.22244-6-ville.syrjala@linux.intel.com> On Fri, Nov 15, 2019 at 09:42:02PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Test the basics of drm_atomic_set_mode_for_crtc(), and in particular > verify that the function doesn't take the shortcut incorrectly. > > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/selftests/Makefile | 3 +- > .../gpu/drm/selftests/drm_modeset_selftests.h | 2 + > .../gpu/drm/selftests/test-drm_atomic_uapi.c | 110 ++++++++++++++++++ > .../drm/selftests/test-drm_modeset_common.h | 2 + > 4 files changed, 116 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/selftests/test-drm_atomic_uapi.c > > diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile > index d2137342b371..a5adc25bd345 100644 > --- a/drivers/gpu/drm/selftests/Makefile > +++ b/drivers/gpu/drm/selftests/Makefile > @@ -1,6 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0-only > test-drm_modeset-y := test-drm_modeset_common.o test-drm_plane_helper.o \ > test-drm_format.o test-drm_framebuffer.o \ > - test-drm_damage_helper.o test-drm_dp_mst_helper.o > + test-drm_damage_helper.o test-drm_dp_mst_helper.o \ > + test-drm_atomic_uapi.o > > obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o test-drm_cmdline_parser.o > diff --git a/drivers/gpu/drm/selftests/drm_modeset_selftests.h b/drivers/gpu/drm/selftests/drm_modeset_selftests.h > index 1898de0b4a4d..2cc4e2f43286 100644 > --- a/drivers/gpu/drm/selftests/drm_modeset_selftests.h > +++ b/drivers/gpu/drm/selftests/drm_modeset_selftests.h > @@ -6,6 +6,8 @@ > * > * Tests are executed in order by igt/drm_selftests_helper > */ > +selftest(atomic_set_mode_for_crtc, igt_atomic_set_mode_for_crtc) > +selftest(atomic_set_zeroed_mode_for_crtc, igt_atomic_set_zeroed_mode_for_crtc) > selftest(check_plane_state, igt_check_plane_state) > selftest(check_drm_format_block_width, igt_check_drm_format_block_width) > selftest(check_drm_format_block_height, igt_check_drm_format_block_height) > diff --git a/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c b/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c > new file mode 100644 > index 000000000000..0f9a4d0d2541 > --- /dev/null > +++ b/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c > @@ -0,0 +1,110 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Test cases for the drm_atomic_uapi functions > + */ > + > +#define pr_fmt(fmt) "drm_atomic_uapi: " fmt > + > +#include <drm/drm_atomic_uapi.h> > +#include <drm/drm_drv.h> > +#include <drm/drm_crtc.h> > +#include <drm/drm_modes.h> > + > +#include "test-drm_modeset_common.h" > + > +static const struct drm_display_mode zeroed_mode; > + > +static struct drm_driver mock_driver; > +static struct drm_device mock_device; > +static struct drm_crtc mock_crtc; > + > +static void init(void) > +{ > + memset(&mock_driver, 0, sizeof(mock_driver)); > + memset(&mock_device, 0, sizeof(mock_device)); > + memset(&mock_crtc, 0, sizeof(mock_crtc)); > + > + mock_device.driver = &mock_driver; > + mock_crtc.dev = &mock_device; > + > + drm_mode_config_init(&mock_device); > +} Hm some todof for a real mock_driver would be nice. I think the static stuff is a bit too icky, I think we should go a proper mock_drm_device a la mock_gem_device in i915 selftests. and mock_drm_crtc. But meh ... > + > +static void cleanup(void) > +{ > + drm_mode_config_cleanup(&mock_device); > +} > + > +int igt_atomic_set_mode_for_crtc(void *ignored) > +{ > + static const struct drm_display_mode mode1 = { > + DRM_MODE("640x480", DRM_MODE_TYPE_DRIVER, 25175, > + 640, 656, 752, 800, 0, 480, 490, 492, 525, 0, > + DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC) > + }; > + static const struct drm_display_mode mode2 = { > + DRM_MODE("1024x768", DRM_MODE_TYPE_DRIVER, 65000, > + 1024, 1048, 1184, 1344, 0, 768, 771, 777, 806, 0, > + DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC) > + }; > + struct drm_crtc_state crtc_state = { > + .crtc = &mock_crtc, > + }; > + int ret; > + > + init(); > + > + /* > + * Make sure drm_atomic_set_mode_for_crtc() > + * updates the crtc state correctly. > + */ > + ret = drm_atomic_set_mode_for_crtc(&crtc_state, &mode1); > + FAIL(ret < 0, "Unable to set mode on crtc\n"); > + FAIL_ON(!crtc_state.enable); > + FAIL_ON(memcmp(&crtc_state.mode, &mode1, sizeof(mode1))); > + > + ret = drm_atomic_set_mode_for_crtc(&crtc_state, &mode2); > + FAIL(ret < 0, "Unable to set mode on crtc\n"); > + FAIL_ON(!crtc_state.enable); > + FAIL_ON(memcmp(&crtc_state.mode, &mode2, sizeof(mode2))); > + > + ret = drm_atomic_set_mode_for_crtc(&crtc_state, NULL); > + FAIL(ret < 0, "Unable to unset mode on crtc\n"); > + FAIL_ON(crtc_state.enable); > + FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode))); > + > + cleanup(); > + > + return 0; > +} > + > +int igt_atomic_set_zeroed_mode_for_crtc(void *ignored) > +{ > + struct drm_crtc_state crtc_state = { > + .crtc = &mock_crtc, > + }; > + int ret; > + > + init(); > + > + /* > + * Make sure drm_atomic_set_mode_for_crtc() doesn't > + * take the shortcut and fail to update crtc_state.enable > + * when passed a zeroed mode (which is technically not > + * valid, but as drm_atomic_set_mode_for_crtc() does no > + * mode validation it should still work). > + */ > + ret = drm_atomic_set_mode_for_crtc(&crtc_state, &zeroed_mode); > + FAIL(ret < 0, "Unable to set mode on crtc\n"); > + FAIL_ON(!crtc_state.enable); > + FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode))); > + > + ret = drm_atomic_set_mode_for_crtc(&crtc_state, NULL); > + FAIL(ret < 0, "Unable to unset mode on crtc\n"); > + FAIL_ON(crtc_state.enable); > + FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode))); Not really seeing the point of why we have to be careful here, but I guess silly stuff can happen, and better to not be accidentally inconsistent. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + > + cleanup(); > + > + return 0; > +} > diff --git a/drivers/gpu/drm/selftests/test-drm_modeset_common.h b/drivers/gpu/drm/selftests/test-drm_modeset_common.h > index 0fcb8bbc6a1b..a1248abf86f9 100644 > --- a/drivers/gpu/drm/selftests/test-drm_modeset_common.h > +++ b/drivers/gpu/drm/selftests/test-drm_modeset_common.h > @@ -13,6 +13,8 @@ > > #define FAIL_ON(x) FAIL((x), "%s", "FAIL_ON(" __stringify(x) ")\n") > > +int igt_atomic_set_mode_for_crtc(void *ignored); > +int igt_atomic_set_zeroed_mode_for_crtc(void *ignored); > int igt_check_plane_state(void *ignored); > int igt_check_drm_format_block_width(void *ignored); > int igt_check_drm_format_block_height(void *ignored); > -- > 2.23.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 5/7] drm/selftests: Add some selftests for drm_atomic_set_mode_for_crtc() Date: Tue, 19 Nov 2019 11:39:53 +0100 [thread overview] Message-ID: <20191119103953.GC30416@phenom.ffwll.local> (raw) Message-ID: <20191119103953.Y7mWjIbSkJ0CetArKOsypivuKt3oZ0SEqamlnHtDyng@z> (raw) In-Reply-To: <20191115194204.22244-6-ville.syrjala@linux.intel.com> On Fri, Nov 15, 2019 at 09:42:02PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Test the basics of drm_atomic_set_mode_for_crtc(), and in particular > verify that the function doesn't take the shortcut incorrectly. > > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/selftests/Makefile | 3 +- > .../gpu/drm/selftests/drm_modeset_selftests.h | 2 + > .../gpu/drm/selftests/test-drm_atomic_uapi.c | 110 ++++++++++++++++++ > .../drm/selftests/test-drm_modeset_common.h | 2 + > 4 files changed, 116 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/selftests/test-drm_atomic_uapi.c > > diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile > index d2137342b371..a5adc25bd345 100644 > --- a/drivers/gpu/drm/selftests/Makefile > +++ b/drivers/gpu/drm/selftests/Makefile > @@ -1,6 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0-only > test-drm_modeset-y := test-drm_modeset_common.o test-drm_plane_helper.o \ > test-drm_format.o test-drm_framebuffer.o \ > - test-drm_damage_helper.o test-drm_dp_mst_helper.o > + test-drm_damage_helper.o test-drm_dp_mst_helper.o \ > + test-drm_atomic_uapi.o > > obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o test-drm_cmdline_parser.o > diff --git a/drivers/gpu/drm/selftests/drm_modeset_selftests.h b/drivers/gpu/drm/selftests/drm_modeset_selftests.h > index 1898de0b4a4d..2cc4e2f43286 100644 > --- a/drivers/gpu/drm/selftests/drm_modeset_selftests.h > +++ b/drivers/gpu/drm/selftests/drm_modeset_selftests.h > @@ -6,6 +6,8 @@ > * > * Tests are executed in order by igt/drm_selftests_helper > */ > +selftest(atomic_set_mode_for_crtc, igt_atomic_set_mode_for_crtc) > +selftest(atomic_set_zeroed_mode_for_crtc, igt_atomic_set_zeroed_mode_for_crtc) > selftest(check_plane_state, igt_check_plane_state) > selftest(check_drm_format_block_width, igt_check_drm_format_block_width) > selftest(check_drm_format_block_height, igt_check_drm_format_block_height) > diff --git a/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c b/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c > new file mode 100644 > index 000000000000..0f9a4d0d2541 > --- /dev/null > +++ b/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c > @@ -0,0 +1,110 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Test cases for the drm_atomic_uapi functions > + */ > + > +#define pr_fmt(fmt) "drm_atomic_uapi: " fmt > + > +#include <drm/drm_atomic_uapi.h> > +#include <drm/drm_drv.h> > +#include <drm/drm_crtc.h> > +#include <drm/drm_modes.h> > + > +#include "test-drm_modeset_common.h" > + > +static const struct drm_display_mode zeroed_mode; > + > +static struct drm_driver mock_driver; > +static struct drm_device mock_device; > +static struct drm_crtc mock_crtc; > + > +static void init(void) > +{ > + memset(&mock_driver, 0, sizeof(mock_driver)); > + memset(&mock_device, 0, sizeof(mock_device)); > + memset(&mock_crtc, 0, sizeof(mock_crtc)); > + > + mock_device.driver = &mock_driver; > + mock_crtc.dev = &mock_device; > + > + drm_mode_config_init(&mock_device); > +} Hm some todof for a real mock_driver would be nice. I think the static stuff is a bit too icky, I think we should go a proper mock_drm_device a la mock_gem_device in i915 selftests. and mock_drm_crtc. But meh ... > + > +static void cleanup(void) > +{ > + drm_mode_config_cleanup(&mock_device); > +} > + > +int igt_atomic_set_mode_for_crtc(void *ignored) > +{ > + static const struct drm_display_mode mode1 = { > + DRM_MODE("640x480", DRM_MODE_TYPE_DRIVER, 25175, > + 640, 656, 752, 800, 0, 480, 490, 492, 525, 0, > + DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC) > + }; > + static const struct drm_display_mode mode2 = { > + DRM_MODE("1024x768", DRM_MODE_TYPE_DRIVER, 65000, > + 1024, 1048, 1184, 1344, 0, 768, 771, 777, 806, 0, > + DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC) > + }; > + struct drm_crtc_state crtc_state = { > + .crtc = &mock_crtc, > + }; > + int ret; > + > + init(); > + > + /* > + * Make sure drm_atomic_set_mode_for_crtc() > + * updates the crtc state correctly. > + */ > + ret = drm_atomic_set_mode_for_crtc(&crtc_state, &mode1); > + FAIL(ret < 0, "Unable to set mode on crtc\n"); > + FAIL_ON(!crtc_state.enable); > + FAIL_ON(memcmp(&crtc_state.mode, &mode1, sizeof(mode1))); > + > + ret = drm_atomic_set_mode_for_crtc(&crtc_state, &mode2); > + FAIL(ret < 0, "Unable to set mode on crtc\n"); > + FAIL_ON(!crtc_state.enable); > + FAIL_ON(memcmp(&crtc_state.mode, &mode2, sizeof(mode2))); > + > + ret = drm_atomic_set_mode_for_crtc(&crtc_state, NULL); > + FAIL(ret < 0, "Unable to unset mode on crtc\n"); > + FAIL_ON(crtc_state.enable); > + FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode))); > + > + cleanup(); > + > + return 0; > +} > + > +int igt_atomic_set_zeroed_mode_for_crtc(void *ignored) > +{ > + struct drm_crtc_state crtc_state = { > + .crtc = &mock_crtc, > + }; > + int ret; > + > + init(); > + > + /* > + * Make sure drm_atomic_set_mode_for_crtc() doesn't > + * take the shortcut and fail to update crtc_state.enable > + * when passed a zeroed mode (which is technically not > + * valid, but as drm_atomic_set_mode_for_crtc() does no > + * mode validation it should still work). > + */ > + ret = drm_atomic_set_mode_for_crtc(&crtc_state, &zeroed_mode); > + FAIL(ret < 0, "Unable to set mode on crtc\n"); > + FAIL_ON(!crtc_state.enable); > + FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode))); > + > + ret = drm_atomic_set_mode_for_crtc(&crtc_state, NULL); > + FAIL(ret < 0, "Unable to unset mode on crtc\n"); > + FAIL_ON(crtc_state.enable); > + FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode))); Not really seeing the point of why we have to be careful here, but I guess silly stuff can happen, and better to not be accidentally inconsistent. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + > + cleanup(); > + > + return 0; > +} > diff --git a/drivers/gpu/drm/selftests/test-drm_modeset_common.h b/drivers/gpu/drm/selftests/test-drm_modeset_common.h > index 0fcb8bbc6a1b..a1248abf86f9 100644 > --- a/drivers/gpu/drm/selftests/test-drm_modeset_common.h > +++ b/drivers/gpu/drm/selftests/test-drm_modeset_common.h > @@ -13,6 +13,8 @@ > > #define FAIL_ON(x) FAIL((x), "%s", "FAIL_ON(" __stringify(x) ")\n") > > +int igt_atomic_set_mode_for_crtc(void *ignored); > +int igt_atomic_set_zeroed_mode_for_crtc(void *ignored); > int igt_check_plane_state(void *ignored); > int igt_check_drm_format_block_width(void *ignored); > int igt_check_drm_format_block_height(void *ignored); > -- > 2.23.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-11-19 10:39 UTC|newest] Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-15 19:41 [PATCH 0/7] drm: Random pile of core stuff Ville Syrjala 2019-11-15 19:41 ` [Intel-gfx] " Ville Syrjala 2019-11-15 19:41 ` Ville Syrjala 2019-11-15 19:41 ` [PATCH 1/7] drm: Move page_flip fb lookup earlier Ville Syrjala 2019-11-15 19:41 ` [Intel-gfx] " Ville Syrjala 2019-11-15 19:41 ` Ville Syrjala 2019-11-15 19:41 ` [PATCH 2/7] drm: Allocate the page flip event earlier Ville Syrjala 2019-11-15 19:41 ` [Intel-gfx] " Ville Syrjala 2019-11-15 19:41 ` Ville Syrjala 2019-11-15 19:42 ` [PATCH 3/7] drm: Extract page_flip_{internal, atomic}() Ville Syrjala 2019-11-15 19:42 ` [Intel-gfx] " Ville Syrjala 2019-11-15 19:42 ` [PATCH 3/7] drm: Extract page_flip_{internal,atomic}() Ville Syrjala 2019-11-19 10:14 ` [PATCH 3/7] drm: Extract page_flip_{internal, atomic}() Daniel Vetter 2019-11-19 10:14 ` [Intel-gfx] " Daniel Vetter 2019-11-19 10:14 ` Daniel Vetter 2019-11-22 18:35 ` Ville Syrjälä 2019-11-22 18:35 ` [Intel-gfx] " Ville Syrjälä 2019-11-22 18:35 ` Ville Syrjälä 2019-11-25 9:02 ` Daniel Vetter 2019-11-25 9:02 ` [Intel-gfx] " Daniel Vetter 2019-11-25 9:02 ` Daniel Vetter 2019-11-25 15:05 ` Ville Syrjälä 2019-11-25 15:05 ` [Intel-gfx] " Ville Syrjälä 2019-11-25 15:05 ` Ville Syrjälä 2019-11-25 17:24 ` Daniel Vetter 2019-11-25 17:24 ` [Intel-gfx] " Daniel Vetter 2019-11-25 17:24 ` Daniel Vetter 2019-11-15 19:42 ` [PATCH 4/7] drm: Simplify the setplane old_fb handling further Ville Syrjala 2019-11-15 19:42 ` [Intel-gfx] " Ville Syrjala 2019-11-15 19:42 ` Ville Syrjala 2019-11-15 19:42 ` [PATCH 5/7] drm/selftests: Add some selftests for drm_atomic_set_mode_for_crtc() Ville Syrjala 2019-11-15 19:42 ` [Intel-gfx] " Ville Syrjala 2019-11-15 19:42 ` Ville Syrjala 2019-11-19 10:39 ` Daniel Vetter [this message] 2019-11-19 10:39 ` [Intel-gfx] " Daniel Vetter 2019-11-15 19:42 ` [PATCH 6/7] drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc() Ville Syrjala 2019-11-15 19:42 ` [Intel-gfx] " Ville Syrjala 2019-11-19 10:20 ` Daniel Vetter 2019-11-19 10:20 ` [Intel-gfx] " Daniel Vetter 2019-11-19 10:20 ` Daniel Vetter 2019-11-15 19:42 ` [PATCH 7/7] drm/atomic: Reduce setplane locking Ville Syrjala 2019-11-15 19:42 ` [Intel-gfx] " Ville Syrjala 2019-11-19 10:47 ` Daniel Vetter 2019-11-19 10:47 ` [Intel-gfx] " Daniel Vetter 2019-11-19 10:47 ` Daniel Vetter 2019-11-15 22:59 ` ✗ Fi.CI.CHECKPATCH: warning for drm: Random pile of core stuff Patchwork 2019-11-15 22:59 ` [Intel-gfx] " Patchwork 2019-11-15 23:20 ` ✓ Fi.CI.BAT: success " Patchwork 2019-11-15 23:20 ` [Intel-gfx] " Patchwork 2019-11-17 12:20 ` ✗ Fi.CI.IGT: failure " Patchwork 2019-11-17 12:20 ` [Intel-gfx] " Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191119103953.GC30416@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=ville.syrjala@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.