* [PATCH 3/5] drm/i915: Do not adjust scale when out of bounds, v2.
2018-04-30 13:46 [PATCH 0/5] drm: Fix rounding errors and use scaling in i915 Maarten Lankhorst
2018-04-30 13:46 ` [PATCH 1/5] drm/rect: Round above 1 << 16 upwards to correct scale calculation functions Maarten Lankhorst
2018-04-30 13:46 ` [PATCH 2/5] drm/rect: Handle rounding errors in drm_rect_clip_scaled, v2 Maarten Lankhorst
@ 2018-04-30 13:46 ` Maarten Lankhorst
2018-04-30 13:46 ` [PATCH 4/5] drm/selftests: Rename the Kconfig option to CONFIG_DRM_DEBUG_SELFTEST Maarten Lankhorst
2018-04-30 13:46 ` [PATCH 5/5] drm/selftests: Add drm helper selftest Maarten Lankhorst
4 siblings, 0 replies; 6+ messages in thread
From: Maarten Lankhorst @ 2018-04-30 13:46 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Vidya Srinivas
With the previous patch drm_atomic_helper_check_plane_state correctly
calculates clipping and the xf86-video-intel ddx is fixed to fall back
to GPU correctly when SetPlane fails, we can remove the hack where
we try to pan/zoom when out of min/max scaling range. This was already
poor behavior where the screen didn't show what was requested, and now
instead we reject it outright. This simplifies check_sprite_plane a lot.
Changes since v1:
- Set crtc_h to the height correctly.
- Reject < 3x3 rectangles instead of making them invisible for <gen9.
For gen9+ skl_update_scaler_plane will reject them.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_sprite.c | 144 +++++++---------------------
1 file changed, 35 insertions(+), 109 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index aa1dfaa692b9..970015dcc6f1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -936,22 +936,12 @@ intel_check_sprite_plane(struct intel_plane *plane,
struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
struct drm_framebuffer *fb = state->base.fb;
- int crtc_x, crtc_y;
- unsigned int crtc_w, crtc_h;
- uint32_t src_x, src_y, src_w, src_h;
- struct drm_rect *src = &state->base.src;
- struct drm_rect *dst = &state->base.dst;
- struct drm_rect clip = {};
int max_stride = INTEL_GEN(dev_priv) >= 9 ? 32768 : 16384;
- int hscale, vscale;
int max_scale, min_scale;
bool can_scale;
int ret;
uint32_t pixel_format = 0;
- *src = drm_plane_state_src(&state->base);
- *dst = drm_plane_state_dest(&state->base);
-
if (!fb) {
state->base.visible = false;
return 0;
@@ -990,64 +980,19 @@ intel_check_sprite_plane(struct intel_plane *plane,
min_scale = plane->can_scale ? 1 : (1 << 16);
}
- /*
- * FIXME the following code does a bunch of fuzzy adjustments to the
- * coordinates and sizes. We probably need some way to decide whether
- * more strict checking should be done instead.
- */
- drm_rect_rotate(src, fb->width << 16, fb->height << 16,
- state->base.rotation);
-
- hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale);
- BUG_ON(hscale < 0);
-
- vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
- BUG_ON(vscale < 0);
-
- if (crtc_state->base.enable)
- drm_mode_get_hv_timing(&crtc_state->base.mode,
- &clip.x2, &clip.y2);
-
- state->base.visible = drm_rect_clip_scaled(src, dst, &clip, hscale, vscale);
-
- crtc_x = dst->x1;
- crtc_y = dst->y1;
- crtc_w = drm_rect_width(dst);
- crtc_h = drm_rect_height(dst);
+ ret = drm_atomic_helper_check_plane_state(&state->base,
+ &crtc_state->base,
+ min_scale, max_scale,
+ true, true);
+ if (ret)
+ return ret;
if (state->base.visible) {
- /* check again in case clipping clamped the results */
- hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
- if (hscale < 0) {
- DRM_DEBUG_KMS("Horizontal scaling factor out of limits\n");
- drm_rect_debug_print("src: ", src, true);
- drm_rect_debug_print("dst: ", dst, false);
-
- return hscale;
- }
-
- vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
- if (vscale < 0) {
- DRM_DEBUG_KMS("Vertical scaling factor out of limits\n");
- drm_rect_debug_print("src: ", src, true);
- drm_rect_debug_print("dst: ", dst, false);
-
- return vscale;
- }
-
- /* Make the source viewport size an exact multiple of the scaling factors. */
- drm_rect_adjust_size(src,
- drm_rect_width(dst) * hscale - drm_rect_width(src),
- drm_rect_height(dst) * vscale - drm_rect_height(src));
-
- drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
- state->base.rotation);
-
- /* sanity check to make sure the src viewport wasn't enlarged */
- WARN_ON(src->x1 < (int) state->base.src_x ||
- src->y1 < (int) state->base.src_y ||
- src->x2 > (int) state->base.src_x + state->base.src_w ||
- src->y2 > (int) state->base.src_y + state->base.src_h);
+ struct drm_rect *src = &state->base.src;
+ struct drm_rect *dst = &state->base.dst;
+ unsigned int crtc_w = drm_rect_width(dst);
+ unsigned int crtc_h = drm_rect_height(dst);
+ uint32_t src_x, src_y, src_w, src_h;
/*
* Hardware doesn't handle subpixel coordinates.
@@ -1060,58 +1005,39 @@ intel_check_sprite_plane(struct intel_plane *plane,
src_y = src->y1 >> 16;
src_h = drm_rect_height(src) >> 16;
- if (intel_format_is_yuv(fb->format->format)) {
- src_x &= ~1;
- src_w &= ~1;
-
- /*
- * Must keep src and dst the
- * same if we can't scale.
- */
- if (!can_scale)
- crtc_w &= ~1;
+ src->x1 = src_x << 16;
+ src->x2 = (src_x + src_w) << 16;
+ src->y1 = src_y << 16;
+ src->y2 = (src_y + src_h) << 16;
- if (crtc_w == 0)
- state->base.visible = false;
+ if (intel_format_is_yuv(fb->format->format) &&
+ (src_x % 2 || src_w % 2)) {
+ DRM_DEBUG_KMS("src x/w (%u, %u) must be a multiple of 2 for YUV planes\n",
+ src_x, src_w);
+ return -EINVAL;
}
- }
- /* Check size restrictions when scaling */
- if (state->base.visible && (src_w != crtc_w || src_h != crtc_h)) {
- unsigned int width_bytes;
- int cpp = fb->format->cpp[0];
+ /* Check size restrictions when scaling */
+ if (src_w != crtc_w || src_h != crtc_h) {
+ unsigned int width_bytes;
+ int cpp = fb->format->cpp[0];
- WARN_ON(!can_scale);
+ WARN_ON(!can_scale);
- /* FIXME interlacing min height is 6 */
+ width_bytes = ((src_x * cpp) & 63) + src_w * cpp;
- if (crtc_w < 3 || crtc_h < 3)
- state->base.visible = false;
-
- if (src_w < 3 || src_h < 3)
- state->base.visible = false;
-
- width_bytes = ((src_x * cpp) & 63) + src_w * cpp;
-
- if (INTEL_GEN(dev_priv) < 9 && (src_w > 2048 || src_h > 2048 ||
- width_bytes > 4096 || fb->pitches[0] > 4096)) {
- DRM_DEBUG_KMS("Source dimensions exceed hardware limits\n");
- return -EINVAL;
+ /* FIXME interlacing min height is 6 */
+ if (INTEL_GEN(dev_priv) < 9 && (
+ src_w < 3 || src_h < 3 ||
+ src_w > 2048 || src_h > 2048 ||
+ crtc_w < 3 || crtc_h < 3 ||
+ width_bytes > 4096 || fb->pitches[0] > 4096)) {
+ DRM_DEBUG_KMS("Source dimensions exceed hardware limits\n");
+ return -EINVAL;
+ }
}
}
- if (state->base.visible) {
- src->x1 = src_x << 16;
- src->x2 = (src_x + src_w) << 16;
- src->y1 = src_y << 16;
- src->y2 = (src_y + src_h) << 16;
- }
-
- dst->x1 = crtc_x;
- dst->x2 = crtc_x + crtc_w;
- dst->y1 = crtc_y;
- dst->y2 = crtc_y + crtc_h;
-
if (INTEL_GEN(dev_priv) >= 9) {
ret = skl_check_plane_surface(crtc_state, state);
if (ret)
--
2.17.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 5/5] drm/selftests: Add drm helper selftest
2018-04-30 13:46 [PATCH 0/5] drm: Fix rounding errors and use scaling in i915 Maarten Lankhorst
` (3 preceding siblings ...)
2018-04-30 13:46 ` [PATCH 4/5] drm/selftests: Rename the Kconfig option to CONFIG_DRM_DEBUG_SELFTEST Maarten Lankhorst
@ 2018-04-30 13:46 ` Maarten Lankhorst
4 siblings, 0 replies; 6+ messages in thread
From: Maarten Lankhorst @ 2018-04-30 13:46 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Vidya Srinivas
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/Kconfig | 1 +
drivers/gpu/drm/selftests/Makefile | 2 +-
.../gpu/drm/selftests/drm_helper_selftests.h | 9 +
drivers/gpu/drm/selftests/test-drm-helper.c | 247 ++++++++++++++++++
4 files changed, 258 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/selftests/drm_helper_selftests.h
create mode 100644 drivers/gpu/drm/selftests/test-drm-helper.c
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index d684855b95c2..28d059007ac2 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -55,6 +55,7 @@ config DRM_DEBUG_SELFTEST
depends on DEBUG_KERNEL
select PRIME_NUMBERS
select DRM_LIB_RANDOM
+ select DRM_KMS_HELPER
default n
help
This option provides kernel modules that can be used to run
diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile
index f7dd66e859a9..9fc349fa18e9 100644
--- a/drivers/gpu/drm/selftests/Makefile
+++ b/drivers/gpu/drm/selftests/Makefile
@@ -1 +1 @@
-obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o
+obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm-helper.o
diff --git a/drivers/gpu/drm/selftests/drm_helper_selftests.h b/drivers/gpu/drm/selftests/drm_helper_selftests.h
new file mode 100644
index 000000000000..9771290ed228
--- /dev/null
+++ b/drivers/gpu/drm/selftests/drm_helper_selftests.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* List each unit test as selftest(name, function)
+ *
+ * The name is used as both an enum and expanded as igt__name to create
+ * a module parameter. It must be unique and legal for a C identifier.
+ *
+ * Tests are executed in order by igt/drm_selftests_helper
+ */
+selftest(check_plane_state, igt_check_plane_state)
diff --git a/drivers/gpu/drm/selftests/test-drm-helper.c b/drivers/gpu/drm/selftests/test-drm-helper.c
new file mode 100644
index 000000000000..a015712b43e8
--- /dev/null
+++ b/drivers/gpu/drm/selftests/test-drm-helper.c
@@ -0,0 +1,247 @@
+/*
+ * Test cases for the drm_kms_helper functions
+ */
+
+#define pr_fmt(fmt) "drm_kms_helper: " fmt
+
+#include <linux/module.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_modes.h>
+
+#define TESTS "drm_helper_selftests.h"
+#include "drm_selftest.h"
+
+#define FAIL(test, msg, ...) \
+ do { \
+ if (test) { \
+ pr_err("%s/%u: " msg, __FUNCTION__, __LINE__, ##__VA_ARGS__); \
+ return -EINVAL; \
+ } \
+ } while (0)
+
+#define FAIL_ON(x) FAIL((x), "%s", "FAIL_ON(" __stringify(x) ")\n")
+
+static void set_src(struct drm_plane_state *plane_state,
+ unsigned src_x, unsigned src_y,
+ unsigned src_w, unsigned src_h)
+{
+ plane_state->src_x = src_x;
+ plane_state->src_y = src_y;
+ plane_state->src_w = src_w;
+ plane_state->src_h = src_h;
+}
+
+static bool check_src_eq(struct drm_plane_state *plane_state,
+ unsigned src_x, unsigned src_y,
+ unsigned src_w, unsigned src_h)
+{
+ if (plane_state->src.x1 < 0) {
+ pr_err("src x coordinate %x should never be below 0.\n", plane_state->src.x1);
+ drm_rect_debug_print("src: ", &plane_state->src, true);
+ return false;
+ }
+ if (plane_state->src.y1 < 0) {
+ pr_err("src y coordinate %x should never be below 0.\n", plane_state->src.y1);
+ drm_rect_debug_print("src: ", &plane_state->src, true);
+ return false;
+ }
+
+ if (plane_state->src.x1 != src_x ||
+ plane_state->src.y1 != src_y ||
+ drm_rect_width(&plane_state->src) != src_w ||
+ drm_rect_height(&plane_state->src) != src_h) {
+ drm_rect_debug_print("src: ", &plane_state->src, true);
+ return false;
+ }
+
+ return true;
+}
+
+static void set_crtc(struct drm_plane_state *plane_state,
+ int crtc_x, int crtc_y,
+ unsigned crtc_w, unsigned crtc_h)
+{
+ plane_state->crtc_x = crtc_x;
+ plane_state->crtc_y = crtc_y;
+ plane_state->crtc_w = crtc_w;
+ plane_state->crtc_h = crtc_h;
+}
+
+static bool check_crtc_eq(struct drm_plane_state *plane_state,
+ int crtc_x, int crtc_y,
+ unsigned crtc_w, unsigned crtc_h)
+{
+ if (plane_state->dst.x1 != crtc_x ||
+ plane_state->dst.y1 != crtc_y ||
+ drm_rect_width(&plane_state->dst) != crtc_w ||
+ drm_rect_height(&plane_state->dst) != crtc_h) {
+ drm_rect_debug_print("dst: ", &plane_state->dst, false);
+
+ return false;
+ }
+
+ return true;
+}
+
+static int igt_check_plane_state(void *ignored)
+{
+ int ret;
+
+ const struct drm_crtc_state crtc_state = {
+ .crtc = ZERO_SIZE_PTR,
+ .enable = true,
+ .active = true,
+ .mode = {
+ DRM_MODE("1024x768", 0, 65000, 1024, 1048,
+ 1184, 1344, 0, 768, 771, 777, 806, 0,
+ DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
+ },
+ };
+ struct drm_framebuffer fb = {
+ .width = 2048,
+ .height = 2048
+ };
+ struct drm_plane_state plane_state = {
+ .crtc = ZERO_SIZE_PTR,
+ .fb = &fb,
+ .rotation = DRM_MODE_ROTATE_0
+ };
+
+ /* Simple clipping, no scaling. */
+ set_src(&plane_state, 0, 0, fb.width << 16, fb.height << 16);
+ set_crtc(&plane_state, 0, 0, fb.width, fb.height);
+ ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+ DRM_PLANE_HELPER_NO_SCALING,
+ DRM_PLANE_HELPER_NO_SCALING,
+ false, false);
+ FAIL(ret < 0, "Simple clipping check should pass\n");
+ FAIL_ON(!plane_state.visible);
+ FAIL_ON(!check_src_eq(&plane_state, 0, 0, 1024 << 16, 768 << 16));
+ FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+
+ /* Rotated clipping + reflection, no scaling. */
+ plane_state.rotation = DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X;
+ ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+ DRM_PLANE_HELPER_NO_SCALING,
+ DRM_PLANE_HELPER_NO_SCALING,
+ false, false);
+ FAIL(ret < 0, "Rotated clipping check should pass\n");
+ FAIL_ON(!plane_state.visible);
+ FAIL_ON(!check_src_eq(&plane_state, 0, 0, 768 << 16, 1024 << 16));
+ FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+ plane_state.rotation = DRM_MODE_ROTATE_0;
+
+ /* Check whether positioning works correctly. */
+ set_src(&plane_state, 0, 0, 1023 << 16, 767 << 16);
+ set_crtc(&plane_state, 0, 0, 1023, 767);
+ ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+ DRM_PLANE_HELPER_NO_SCALING,
+ DRM_PLANE_HELPER_NO_SCALING,
+ false, false);
+ FAIL(!ret, "Should not be able to position on the crtc with can_position=false\n");
+
+ ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+ DRM_PLANE_HELPER_NO_SCALING,
+ DRM_PLANE_HELPER_NO_SCALING,
+ true, false);
+ FAIL(ret < 0, "Simple positioning should work\n");
+ FAIL_ON(!plane_state.visible);
+ FAIL_ON(!check_src_eq(&plane_state, 0, 0, 1023 << 16, 767 << 16));
+ FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1023, 767));
+
+ /* Simple scaling tests. */
+ set_src(&plane_state, 0, 0, 512 << 16, 384 << 16);
+ set_crtc(&plane_state, 0, 0, 1024, 768);
+ ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+ 0x8001,
+ DRM_PLANE_HELPER_NO_SCALING,
+ false, false);
+ FAIL(!ret, "Upscaling out of range should fail.\n");
+ ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+ 0x8000,
+ DRM_PLANE_HELPER_NO_SCALING,
+ false, false);
+ FAIL(ret < 0, "Upscaling exactly 2x should work\n");
+ FAIL_ON(!plane_state.visible);
+ FAIL_ON(!check_src_eq(&plane_state, 0, 0, 512 << 16, 384 << 16));
+ FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+
+ set_src(&plane_state, 0, 0, 2048 << 16, 1536 << 16);
+ ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+ DRM_PLANE_HELPER_NO_SCALING,
+ 0x1ffff, false, false);
+ FAIL(!ret, "Downscaling out of range should fail.\n");
+ ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+ DRM_PLANE_HELPER_NO_SCALING,
+ 0x20000, false, false);
+ FAIL(ret < 0, "Should succeed with exact scaling limit\n");
+ FAIL_ON(!plane_state.visible);
+ FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2048 << 16, 1536 << 16));
+ FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+
+ /* Testing rounding errors. */
+ set_src(&plane_state, 0, 0, 0x40001, 0x40001);
+ set_crtc(&plane_state, 1022, 766, 4, 4);
+ ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+ DRM_PLANE_HELPER_NO_SCALING,
+ 0x10001,
+ true, false);
+ FAIL(ret < 0, "Should succeed by clipping to exact multiple");
+ FAIL_ON(!plane_state.visible);
+ FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2 << 16, 2 << 16));
+ FAIL_ON(!check_crtc_eq(&plane_state, 1022, 766, 2, 2));
+
+ set_src(&plane_state, 0x20001, 0x20001, 0x4040001, 0x3040001);
+ set_crtc(&plane_state, -2, -2, 1028, 772);
+ ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+ DRM_PLANE_HELPER_NO_SCALING,
+ 0x10001,
+ false, false);
+ FAIL(ret < 0, "Should succeed by clipping to exact multiple");
+ FAIL_ON(!plane_state.visible);
+ FAIL_ON(!check_src_eq(&plane_state, 0x40002, 0x40002, 1024 << 16, 768 << 16));
+ FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+
+ set_src(&plane_state, 0, 0, 0x3ffff, 0x3ffff);
+ set_crtc(&plane_state, 1022, 766, 4, 4);
+ ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+ 0xffff,
+ DRM_PLANE_HELPER_NO_SCALING,
+ true, false);
+ FAIL(ret < 0, "Should succeed by clipping to exact multiple");
+ FAIL_ON(!plane_state.visible);
+ /* Should not be rounded to 0x20001, which would be upscaling. */
+ FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2 << 16, 2 << 16));
+ FAIL_ON(!check_crtc_eq(&plane_state, 1022, 766, 2, 2));
+
+ set_src(&plane_state, 0x1ffff, 0x1ffff, 0x403ffff, 0x303ffff);
+ set_crtc(&plane_state, -2, -2, 1028, 772);
+ ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+ 0xffff,
+ DRM_PLANE_HELPER_NO_SCALING,
+ false, false);
+ FAIL(ret < 0, "Should succeed by clipping to exact multiple");
+ FAIL_ON(!plane_state.visible);
+ FAIL_ON(!check_src_eq(&plane_state, 0x3fffe, 0x3fffe, 1024 << 16, 768 << 16));
+ FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+
+ return 0;
+}
+
+#include "drm_selftest.c"
+
+static int __init test_drm_helper_init(void)
+{
+ int err;
+
+ err = run_selftests(selftests, ARRAY_SIZE(selftests), NULL);
+
+ return err > 0 ? 0 : err;
+}
+
+module_init(test_drm_helper_init);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
--
2.17.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread