[PATCH] drm: Check fb against plane size rather than CRTC mode for pageflip

[PATCH] drm: Check fb against plane size rather than CRTC mode for pageflip

[Matt Roper]

The legacy pageflip ioctl calls drm_crtc_check_viewport() to determine
whether the framebuffer being flipped is big enough to fill the display
it is being flipped to.  However some drivers support "windowing" of
their primary planes (i.e., a primary plane that does not cover the
entire CRTC dimensions); in such situations we can wind up rejecting
valid pageflips of buffers that are smaller than the display mode, but
still large enough to fill the entire primary plane.

What we really want to be comparing against for pageflips is the size of
the primary plane, which can be found in crtc->primary->state for atomic
drivers (and drivers in the process of transitioning to atomic).  There
are no non-atomic drivers that support primary plane windowing at the
moment, so we'll continue to use the current behavior of looking at the
CRTC mode size on drivers that don't have a crtc->primary->state.  We'll
also continue to use the existing logic for SetCrtc, which is the other
callsite for drm_crtc_check_viewport(), since legacy modesets reprogram
the primary plane and remove windowing.

Note that the existing code was checking a crtc->invert_dimensions field
to determine whether the width/height of the mode needed to be swapped.
A bonus of checking plane size is that the source width/height we get
already take rotation into account so that check is no longer necessary
when using the plane size.

Testcase: igt/universal-plane-gen9-features-pipe-#
Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 87 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 71 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e600a5f..35cd4dc 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2534,9 +2534,76 @@ void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
 }
 EXPORT_SYMBOL(drm_crtc_get_hv_timing);
 
+static int check_viewport(int hdisplay,
+			  int vdisplay,
+			  int x,
+			  int y,
+			  bool invert_dimensions,
+			  const struct drm_framebuffer *fb)
+{
+	if (invert_dimensions)
+		swap(hdisplay, vdisplay);
+
+	if (hdisplay > fb->width ||
+	    vdisplay > fb->height ||
+	    x > fb->width - hdisplay ||
+	    y > fb->height - vdisplay) {
+		DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n",
+			      fb->width, fb->height, hdisplay, vdisplay, x, y,
+			      invert_dimensions ? " (inverted)" : "");
+		return -ENOSPC;
+	}
+
+	return 0;
+}
+
+/**
+ * drm_plane_check_viewport - Checks that a framebuffer is big enough for the
+ *     plane's viewport
+ * @plane: Plane that framebuffer will be displayed on
+ * @x: x panning
+ * @y: y panning
+ * @fb: framebuffer to check size of
+ *
+ * Atomic drivers (or transitioning drivers that support proper plane state)
+ * may call this function on any plane.  Non-atomic drivers may only call this
+ * for the primary plane while the CRTC is active (we'll assume that the
+ * primary plane covers the entire CRTC in that case).
+ */
+int drm_plane_check_viewport(const struct drm_plane *plane,
+			     int x,
+			     int y,
+			     const struct drm_framebuffer *fb)
+
+{
+	struct drm_crtc *crtc = plane->crtc;
+	int hdisplay, vdisplay;
+
+	if (WARN_ON(plane->state == NULL &&
+		    plane->type != DRM_PLANE_TYPE_PRIMARY))
+		return -EINVAL;
+
+	/*
+	 * Non-atomic drivers may not have valid plane state to look at.  But
+	 * those drivers also don't support windowing of the primary plane, so
+	 * we can fall back to looking at the mode of the owning CRTC.
+	 */
+	if (plane->state) {
+		hdisplay = plane->state->src_w >> 16;
+		vdisplay = plane->state->src_h >> 16;
+	} else if (WARN_ON(!crtc)) {
+		hdisplay = vdisplay = 0;
+	} else {
+		drm_crtc_get_hv_timing(&crtc->mode, &hdisplay, &vdisplay);
+	}
+
+	return check_viewport(hdisplay, vdisplay, x, y, false, fb);
+}
+EXPORT_SYMBOL(drm_plane_check_viewport);
+
 /**
  * drm_crtc_check_viewport - Checks that a framebuffer is big enough for the
- *     CRTC viewport
+ *     CRTC viewport when running in the specified mode
  * @crtc: CRTC that framebuffer will be displayed on
  * @x: x panning
  * @y: y panning
@@ -2553,20 +2620,8 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
 
 	drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
 
-	if (crtc->invert_dimensions)
-		swap(hdisplay, vdisplay);
-
-	if (hdisplay > fb->width ||
-	    vdisplay > fb->height ||
-	    x > fb->width - hdisplay ||
-	    y > fb->height - vdisplay) {
-		DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n",
-			      fb->width, fb->height, hdisplay, vdisplay, x, y,
-			      crtc->invert_dimensions ? " (inverted)" : "");
-		return -ENOSPC;
-	}
-
-	return 0;
+	return check_viewport(hdisplay, vdisplay, x, y,
+			      crtc->invert_dimensions, fb);
 }
 EXPORT_SYMBOL(drm_crtc_check_viewport);
 
@@ -5181,7 +5236,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		goto out;
 	}
 
-	ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb);
+	ret = drm_plane_check_viewport(crtc->primary, crtc->x, crtc->y, fb);
 	if (ret)
 		goto out;
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t] kms_universal_plane: Add gen9-specific test

[Matt Roper]

Gen9 adds some new capabilities not present on previous platforms
(primary plane windowing, 90/270 rotation, etc.).  Add a new subtest to
check how these new features interact with the use of the universal
plane API.

For now we just check whether pageflips work as expected in a windowed
setting.  We may want to add some rotation testing in future patches.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 tests/kms_universal_plane.c | 107 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/tests/kms_universal_plane.c b/tests/kms_universal_plane.c
index b233166..b06b51e 100644
--- a/tests/kms_universal_plane.c
+++ b/tests/kms_universal_plane.c
@@ -54,6 +54,13 @@ typedef struct {
 	struct igt_fb red_fb, blue_fb;
 } pageflip_test_t;
 
+typedef struct {
+	data_t *data;
+	int x, y;
+	int w, h;
+	struct igt_fb biggreen_fb, smallred_fb, smallblue_fb;
+} gen9_test_t;
+
 static void
 functional_test_init(functional_test_t *test, igt_output_t *output, enum pipe pipe)
 {
@@ -637,6 +644,101 @@ cursor_leak_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
 }
 
 static void
+gen9_test_init(gen9_test_t *test, igt_output_t *output, enum pipe pipe)
+{
+	data_t *data = test->data;
+	drmModeModeInfo *mode;
+
+	igt_output_set_pipe(output, pipe);
+
+	mode = igt_output_get_mode(output);
+	test->w = mode->hdisplay / 2;
+	test->h = mode->vdisplay / 2;
+	test->x = mode->hdisplay / 4;
+	test->y = mode->vdisplay / 4;
+
+	/* Initial framebuffer of full CRTC size */
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+			    DRM_FORMAT_XRGB8888,
+			    LOCAL_DRM_FORMAT_MOD_NONE,
+			    0.0, 1.0, 0.0,
+			    &test->biggreen_fb);
+
+	/* Framebuffers that only cover a quarter of the CRTC size */
+	igt_create_color_fb(data->drm_fd, test->w, test->h,
+			    DRM_FORMAT_XRGB8888,
+			    LOCAL_DRM_FORMAT_MOD_NONE,
+			    1.0, 0.0, 0.0,
+			    &test->smallred_fb);
+	igt_create_color_fb(data->drm_fd, test->w, test->h,
+			    DRM_FORMAT_XRGB8888,
+			    LOCAL_DRM_FORMAT_MOD_NONE,
+			    0.0, 0.0, 1.0,
+			    &test->smallblue_fb);
+}
+
+static void
+gen9_test_fini(gen9_test_t *test, igt_output_t *output)
+{
+	igt_remove_fb(test->data->drm_fd, &test->biggreen_fb);
+	igt_remove_fb(test->data->drm_fd, &test->smallred_fb);
+	igt_remove_fb(test->data->drm_fd, &test->smallblue_fb);
+
+	igt_output_set_pipe(output, PIPE_ANY);
+	igt_display_commit2(&test->data->display, COMMIT_LEGACY);
+}
+
+/*
+ * Test features specific to gen9+ platforms (i.e., primary plane
+ * windowing)
+ */
+static void
+gen9_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+	gen9_test_t test = { .data = data };
+	igt_plane_t *primary;
+
+	int ret = 0;
+
+	igt_skip_on(data->gen < 9);
+	igt_skip_on(pipe >= data->display.n_pipes);
+
+	igt_output_set_pipe(output, pipe);
+
+	gen9_test_init(&test, output, pipe);
+
+	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
+
+	/* Start with a full-screen primary plane */
+	igt_plane_set_fb(primary, &test.biggreen_fb);
+	igt_display_commit2(&data->display, COMMIT_LEGACY);
+
+	/* Set primary to windowed size/position */
+	igt_plane_set_fb(primary, &test.smallblue_fb);
+	igt_plane_set_position(primary, test.x, test.y);
+	igt_plane_set_size(primary, test.w, test.h);
+	igt_display_commit2(&data->display, COMMIT_UNIVERSAL);
+
+	/*
+	 * SetPlane update to another framebuffer of the same size
+	 * should succeed
+	 */
+	igt_plane_set_fb(primary, &test.smallred_fb);
+	igt_plane_set_position(primary, test.x, test.y);
+	igt_plane_set_size(primary, test.w, test.h);
+	igt_display_commit2(&data->display, COMMIT_UNIVERSAL);
+
+	/* PageFlip should also succeed */
+	ret = drmModePageFlip(data->drm_fd, output->config.crtc->crtc_id,
+			      test.smallblue_fb.fb_id, 0, NULL);
+	igt_assert_eq(ret, 0);
+
+	igt_plane_set_fb(primary, NULL);
+	igt_plane_set_position(primary, 0, 0);
+	gen9_test_fini(&test, output);
+}
+
+static void
 run_tests_for_pipe(data_t *data, enum pipe pipe)
 {
 	igt_output_t *output;
@@ -660,6 +762,11 @@ run_tests_for_pipe(data_t *data, enum pipe pipe)
 		      kmstest_pipe_name(pipe))
 		for_each_connected_output(&data->display, output)
 			cursor_leak_test_pipe(data, pipe, output);
+
+	igt_subtest_f("universal-plane-gen9-features-pipe-%s",
+		      kmstest_pipe_name(pipe))
+		for_each_connected_output(&data->display, output)
+			gen9_test_pipe(data, pipe, output);
 }
 
 static data_t data;
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Check fb against plane size rather than CRTC mode for pageflip

[Ville Syrjälä]

On Tue, Oct 06, 2015 at 05:02:47PM -0700, Matt Roper wrote:
> The legacy pageflip ioctl calls drm_crtc_check_viewport() to determine
> whether the framebuffer being flipped is big enough to fill the display
> it is being flipped to.  However some drivers support "windowing" of
> their primary planes (i.e., a primary plane that does not cover the
> entire CRTC dimensions); in such situations we can wind up rejecting
> valid pageflips of buffers that are smaller than the display mode, but
> still large enough to fill the entire primary plane.
> 
> What we really want to be comparing against for pageflips is the size of
> the primary plane, which can be found in crtc->primary->state for atomic
> drivers (and drivers in the process of transitioning to atomic).  There
> are no non-atomic drivers that support primary plane windowing at the
> moment, so we'll continue to use the current behavior of looking at the
> CRTC mode size on drivers that don't have a crtc->primary->state.  We'll
> also continue to use the existing logic for SetCrtc, which is the other
> callsite for drm_crtc_check_viewport(), since legacy modesets reprogram
> the primary plane and remove windowing.
> 
> Note that the existing code was checking a crtc->invert_dimensions field
> to determine whether the width/height of the mode needed to be swapped.
> A bonus of checking plane size is that the source width/height we get
> already take rotation into account so that check is no longer necessary
> when using the plane size.
> 
> Testcase: igt/universal-plane-gen9-features-pipe-#
> Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 87 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 71 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e600a5f..35cd4dc 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2534,9 +2534,76 @@ void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
>  }
>  EXPORT_SYMBOL(drm_crtc_get_hv_timing);
>  
> +static int check_viewport(int hdisplay,
> +			  int vdisplay,
> +			  int x,
> +			  int y,
> +			  bool invert_dimensions,
> +			  const struct drm_framebuffer *fb)
> +{
> +	if (invert_dimensions)
> +		swap(hdisplay, vdisplay);
> +
> +	if (hdisplay > fb->width ||
> +	    vdisplay > fb->height ||
> +	    x > fb->width - hdisplay ||
> +	    y > fb->height - vdisplay) {
> +		DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n",
> +			      fb->width, fb->height, hdisplay, vdisplay, x, y,
> +			      invert_dimensions ? " (inverted)" : "");
> +		return -ENOSPC;
> +	}
> +
> +	return 0;
> +}

I think I'd rather pull the limit check code from the setplane path to a
separate function, and call that with the src coordinates derived from
either the plane state or the mode depending on the use case. That way
the invert_dimensions crap can stay only in the codepath that actually
needs it.

> +
> +/**
> + * drm_plane_check_viewport - Checks that a framebuffer is big enough for the
> + *     plane's viewport
> + * @plane: Plane that framebuffer will be displayed on
> + * @x: x panning
> + * @y: y panning
> + * @fb: framebuffer to check size of
> + *
> + * Atomic drivers (or transitioning drivers that support proper plane state)
> + * may call this function on any plane.  Non-atomic drivers may only call this
> + * for the primary plane while the CRTC is active (we'll assume that the
> + * primary plane covers the entire CRTC in that case).
> + */
> +int drm_plane_check_viewport(const struct drm_plane *plane,
> +			     int x,
> +			     int y,
> +			     const struct drm_framebuffer *fb)
> +
> +{
> +	struct drm_crtc *crtc = plane->crtc;
> +	int hdisplay, vdisplay;
> +
> +	if (WARN_ON(plane->state == NULL &&
> +		    plane->type != DRM_PLANE_TYPE_PRIMARY))
> +		return -EINVAL;
> +
> +	/*
> +	 * Non-atomic drivers may not have valid plane state to look at.  But
> +	 * those drivers also don't support windowing of the primary plane, so
> +	 * we can fall back to looking at the mode of the owning CRTC.
> +	 */
> +	if (plane->state) {
> +		hdisplay = plane->state->src_w >> 16;
> +		vdisplay = plane->state->src_h >> 16;

You shouldn't truncate these.

> +	} else if (WARN_ON(!crtc)) {
> +		hdisplay = vdisplay = 0;
> +	} else {
> +		drm_crtc_get_hv_timing(&crtc->mode, &hdisplay, &vdisplay);
> +	}
> +
> +	return check_viewport(hdisplay, vdisplay, x, y, false, fb);
> +}
> +EXPORT_SYMBOL(drm_plane_check_viewport);
> +
>  /**
>   * drm_crtc_check_viewport - Checks that a framebuffer is big enough for the
> - *     CRTC viewport
> + *     CRTC viewport when running in the specified mode
>   * @crtc: CRTC that framebuffer will be displayed on
>   * @x: x panning
>   * @y: y panning
> @@ -2553,20 +2620,8 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
>  
>  	drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
>  
> -	if (crtc->invert_dimensions)
> -		swap(hdisplay, vdisplay);
> -
> -	if (hdisplay > fb->width ||
> -	    vdisplay > fb->height ||
> -	    x > fb->width - hdisplay ||
> -	    y > fb->height - vdisplay) {
> -		DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n",
> -			      fb->width, fb->height, hdisplay, vdisplay, x, y,
> -			      crtc->invert_dimensions ? " (inverted)" : "");
> -		return -ENOSPC;
> -	}
> -
> -	return 0;
> +	return check_viewport(hdisplay, vdisplay, x, y,
> +			      crtc->invert_dimensions, fb);
>  }
>  EXPORT_SYMBOL(drm_crtc_check_viewport);
>  
> @@ -5181,7 +5236,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  		goto out;
>  	}
>  
> -	ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb);
> +	ret = drm_plane_check_viewport(crtc->primary, crtc->x, crtc->y, fb);
>  	if (ret)
>  		goto out;
>  
> -- 
> 2.1.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx