[PATCH 0/9] drm: Store clipped coordinates in drm_plane_state

[PATCH 0/9] drm: Store clipped coordinates in drm_plane_state

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Moving the clipped plane coordinates into drm_plane_state has been
discussed a few times, but as no patches seems to have materialized,
I decoded to do it myself. I also added a new helper function
like drm_plane_helper_check_update() that takes a plane state instead.

I converted i915, rockchip, and mediatek over to the new stuff. rockchip
already looked pretty solid, mediatek had some bugs in there that I
hopefully fixed. The rest of the non-x86 drivers seem to entirely lack
any plane clipping code, so I decided that I don't care enough to
write it from scratch. I also converted drm_simple_kms_helper, but
there are no drivers using it so far.

I've only actually tested i915, the rest are just compile tested.

Entire series available here:
git://github.com/vsyrjala/linux.git plane_state_rects

Ville Syrjälä (9):
  drm: Warn about negative sizes when calculating scale factor
  drm: Store clipped src/dst coordinatee in drm_plane_state
  drm/plane-helper: Add drm_plane_helper_check_state()
  drm/i915: Use drm_plane_state.{src,dst,visible}
  drm/i915: Use drm_plane_helper_check_state()
  drm/rockchip: Use drm_plane_state.{src,dst}
  drm/rockchip: Use drm_plane_helper_check_state()
  drm/mediatek: Use drm_plane_helper_check_state()
  drm/simple_kms_helper: Use drm_plane_helper_check_state()

 drivers/gpu/drm/drm_plane_helper.c          | 136 +++++++++++++++++++++------
 drivers/gpu/drm/drm_rect.c                  |   2 +-
 drivers/gpu/drm/drm_simple_kms_helper.c     |  27 ++----
 drivers/gpu/drm/i915/intel_atomic_plane.c   |  18 +---
 drivers/gpu/drm/i915/intel_display.c        | 140 ++++++++++++++--------------
 drivers/gpu/drm/i915/intel_drv.h            |   3 -
 drivers/gpu/drm/i915/intel_fbc.c            |  12 +--
 drivers/gpu/drm/i915/intel_pm.c             |  60 ++++++------
 drivers/gpu/drm/i915/intel_sprite.c         |  94 ++++++++++---------
 drivers/gpu/drm/mediatek/mtk_drm_plane.c    |  72 ++++----------
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  31 ++----
 include/drm/drm_crtc.h                      |  13 +++
 include/drm/drm_plane_helper.h              |   5 +
 13 files changed, 315 insertions(+), 298 deletions(-)

-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/9] drm: Warn about negative sizes when calculating scale factor

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Passing negative width/hight to scale factor calculations is not
legal. Let's WARN if that happens.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_rect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
index a8e2c8603945..512199b60728 100644
--- a/drivers/gpu/drm/drm_rect.c
+++ b/drivers/gpu/drm/drm_rect.c
@@ -100,7 +100,7 @@ static int drm_calc_scale(int src, int dst)
 {
 	int scale = 0;
 
-	if (src < 0 || dst < 0)
+	if (WARN_ON(src < 0 || dst < 0))
 		return -EINVAL;
 
 	if (dst == 0)
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/9] drm: Store clipped src/dst coordinatee in drm_plane_state

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pretty much all driver will have need for the clipped plane
coordinates, so let's stuff then into drm_plane_state.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 include/drm/drm_crtc.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 3edeaf88ebc0..00a1868940b8 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -35,6 +35,7 @@
 #include <uapi/drm/drm_mode.h>
 #include <uapi/drm/drm_fourcc.h>
 #include <drm/drm_modeset_lock.h>
+#include <drm/drm_rect.h>
 
 struct drm_device;
 struct drm_mode_set;
@@ -1409,6 +1410,9 @@ struct drm_connector {
  * @src_w: width of visible portion of plane (in 16.16)
  * @src_h: height of visible portion of plane (in 16.16)
  * @rotation: rotation of the plane
+ * @src: clipped source coordinates of the plane (in 16.16)
+ * @dst: clipped destination coordinates of the plane
+ * @visible: visibility of the plane
  * @state: backpointer to global drm_atomic_state
  */
 struct drm_plane_state {
@@ -1429,6 +1433,15 @@ struct drm_plane_state {
 	/* Plane rotation */
 	unsigned int rotation;
 
+	/* Clipped coordinates */
+	struct drm_rect src, dst;
+
+	/*
+	 * Is the plane actually visible? Can be false even
+	 * if fb!=NULL and crtc!=NULL, due to clipping.
+	 */
+	bool visible;
+
 	struct drm_atomic_state *state;
 };
 
-- 
2.7.4

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

[PATCH 3/9] drm/plane-helper: Add drm_plane_helper_check_state()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a version of drm_plane_helper_check_update() which takes a plane
state instead of having the caller pass in everything.

And to reduce code duplication, let's reimplement
drm_plane_helper_check_update() in terms of the new function, by
having a tempororary plane state on the stack.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane_helper.c | 136 ++++++++++++++++++++++++++++---------
 include/drm/drm_plane_helper.h     |   5 ++
 2 files changed, 110 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index 16c4a7bd7465..1124eb827671 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -108,14 +108,9 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
 }
 
 /**
- * drm_plane_helper_check_update() - Check plane update for validity
- * @plane: plane object to update
- * @crtc: owning CRTC of owning plane
- * @fb: framebuffer to flip onto plane
- * @src: source coordinates in 16.16 fixed point
- * @dest: integer destination coordinates
+ * drm_plane_helper_check_state() - Check plane state for validity
+ * @state: plane state to check
  * @clip: integer clipping coordinates
- * @rotation: plane rotation
  * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
  * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
  * @can_position: is it legal to position the plane such that it
@@ -123,8 +118,6 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
  *                only be false for primary planes.
  * @can_update_disabled: can the plane be updated while the crtc
  *                       is disabled?
- * @visible: output parameter indicating whether plane is still visible after
- *           clipping
  *
  * Checks that a desired plane update is valid.  Drivers that provide
  * their own plane handling rather than helper-provided implementations may
@@ -134,29 +127,38 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
  * RETURNS:
  * Zero if update appears valid, error code on failure
  */
-int drm_plane_helper_check_update(struct drm_plane *plane,
-				  struct drm_crtc *crtc,
-				  struct drm_framebuffer *fb,
-				  struct drm_rect *src,
-				  struct drm_rect *dest,
-				  const struct drm_rect *clip,
-				  unsigned int rotation,
-				  int min_scale,
-				  int max_scale,
-				  bool can_position,
-				  bool can_update_disabled,
-				  bool *visible)
+int drm_plane_helper_check_state(struct drm_plane_state *state,
+				 const struct drm_rect *clip,
+				 int min_scale,
+				 int max_scale,
+				 bool can_position,
+				 bool can_update_disabled)
 {
+	struct drm_crtc *crtc = state->crtc;
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_rect *src = &state->src;
+	struct drm_rect *dst = &state->dst;
+	unsigned int rotation = state->rotation;
 	int hscale, vscale;
 
+	src->x1 = state->src_x;
+	src->y1 = state->src_y;
+	src->x2 = state->src_x + state->src_w;
+	src->y2 = state->src_y + state->src_h;
+
+	dst->x1 = state->crtc_x;
+	dst->y1 = state->crtc_y;
+	dst->x2 = state->crtc_x + state->crtc_w;
+	dst->y2 = state->crtc_y + state->crtc_h;
+
 	if (!fb) {
-		*visible = false;
+		state->visible = false;
 		return 0;
 	}
 
 	/* crtc should only be NULL when disabling (i.e., !fb) */
 	if (WARN_ON(!crtc)) {
-		*visible = false;
+		state->visible = false;
 		return 0;
 	}
 
@@ -168,20 +170,20 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
 	drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
 
 	/* Check scaling */
-	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
-	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
+	hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
+	vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
 	if (hscale < 0 || vscale < 0) {
 		DRM_DEBUG_KMS("Invalid scaling of plane\n");
-		drm_rect_debug_print("src: ", src, true);
-		drm_rect_debug_print("dst: ", dest, false);
+		drm_rect_debug_print("src: ", &state->src, true);
+		drm_rect_debug_print("dst: ", &state->dst, false);
 		return -ERANGE;
 	}
 
-	*visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale);
+	state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
 
 	drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
 
-	if (!*visible)
+	if (!state->visible)
 		/*
 		 * Plane isn't visible; some drivers can handle this
 		 * so we just return success here.  Drivers that can't
@@ -191,15 +193,87 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
 		 */
 		return 0;
 
-	if (!can_position && !drm_rect_equals(dest, clip)) {
+	if (!can_position && !drm_rect_equals(dst, clip)) {
 		DRM_DEBUG_KMS("Plane must cover entire CRTC\n");
-		drm_rect_debug_print("dst: ", dest, false);
+		drm_rect_debug_print("dst: ", dst, false);
 		drm_rect_debug_print("clip: ", clip, false);
 		return -EINVAL;
 	}
 
 	return 0;
 }
+EXPORT_SYMBOL(drm_plane_helper_check_state);
+
+/**
+ * drm_plane_helper_check_update() - Check plane update for validity
+ * @plane: plane object to update
+ * @crtc: owning CRTC of owning plane
+ * @fb: framebuffer to flip onto plane
+ * @src: source coordinates in 16.16 fixed point
+ * @dest: integer destination coordinates
+ * @clip: integer clipping coordinates
+ * @rotation: plane rotation
+ * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
+ * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
+ * @can_position: is it legal to position the plane such that it
+ *                doesn't cover the entire crtc?  This will generally
+ *                only be false for primary planes.
+ * @can_update_disabled: can the plane be updated while the crtc
+ *                       is disabled?
+ * @visible: output parameter indicating whether plane is still visible after
+ *           clipping
+ *
+ * Checks that a desired plane update is valid.  Drivers that provide
+ * their own plane handling rather than helper-provided implementations may
+ * still wish to call this function to avoid duplication of error checking
+ * code.
+ *
+ * RETURNS:
+ * Zero if update appears valid, error code on failure
+ */
+int drm_plane_helper_check_update(struct drm_plane *plane,
+				  struct drm_crtc *crtc,
+				  struct drm_framebuffer *fb,
+				  struct drm_rect *src,
+				  struct drm_rect *dst,
+				  const struct drm_rect *clip,
+				  unsigned int rotation,
+				  int min_scale,
+				  int max_scale,
+				  bool can_position,
+				  bool can_update_disabled,
+				  bool *visible)
+{
+	struct drm_plane_state state = {
+		.plane = plane,
+		.crtc = crtc,
+		.fb = fb,
+		.src_x = src->x1,
+		.src_y = src->x2,
+		.src_w = drm_rect_width(src),
+		.src_h = drm_rect_height(src),
+		.crtc_x = dst->x1,
+		.crtc_y = dst->x2,
+		.crtc_w = drm_rect_width(dst),
+		.crtc_h = drm_rect_height(dst),
+		.rotation = rotation,
+		.visible = *visible,
+	};
+	int ret;
+
+	ret = drm_plane_helper_check_state(&state, clip,
+					   min_scale, max_scale,
+					   can_position,
+					   can_update_disabled);
+	if (ret)
+		return ret;
+
+	*src = state.src;
+	*dst = state.dst;
+	*visible = state.visible;
+
+	return 0;
+}
 EXPORT_SYMBOL(drm_plane_helper_check_update);
 
 /**
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index 0e0c3573cce0..fbc8ecb3e5e8 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -40,6 +40,11 @@
 int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		  const struct drm_crtc_funcs *funcs);
 
+int drm_plane_helper_check_state(struct drm_plane_state *state,
+				 const struct drm_rect *clip,
+				 int min_scale, int max_scale,
+				 bool can_position,
+				 bool can_update_disabled);
 int drm_plane_helper_check_update(struct drm_plane *plane,
 				  struct drm_crtc *crtc,
 				  struct drm_framebuffer *fb,
-- 
2.7.4

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

[PATCH 4/9] drm/i915: Use drm_plane_state.{src,dst,visible}

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Replace the private drm_rects/flags in intel_plane_state
with the ones now living in drm_plane_state.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c |  20 ++---
 drivers/gpu/drm/i915/intel_display.c      | 132 +++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h          |   3 -
 drivers/gpu/drm/i915/intel_fbc.c          |  12 +--
 drivers/gpu/drm/i915/intel_pm.c           |  60 +++++++-------
 drivers/gpu/drm/i915/intel_sprite.c       |  84 +++++++++----------
 6 files changed, 156 insertions(+), 155 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 7de7721f65bc..14d40261db21 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -139,14 +139,14 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	 * we want to keep another copy internal to our driver that we can
 	 * clip/modify ourselves.
 	 */
-	intel_state->src.x1 = state->src_x;
-	intel_state->src.y1 = state->src_y;
-	intel_state->src.x2 = state->src_x + state->src_w;
-	intel_state->src.y2 = state->src_y + state->src_h;
-	intel_state->dst.x1 = state->crtc_x;
-	intel_state->dst.y1 = state->crtc_y;
-	intel_state->dst.x2 = state->crtc_x + state->crtc_w;
-	intel_state->dst.y2 = state->crtc_y + state->crtc_h;
+	intel_state->base.src.x1 = state->src_x;
+	intel_state->base.src.y1 = state->src_y;
+	intel_state->base.src.x2 = state->src_x + state->src_w;
+	intel_state->base.src.y2 = state->src_y + state->src_h;
+	intel_state->base.dst.x1 = state->crtc_x;
+	intel_state->base.dst.y1 = state->crtc_y;
+	intel_state->base.dst.x2 = state->crtc_x + state->crtc_w;
+	intel_state->base.dst.y2 = state->crtc_y + state->crtc_h;
 
 	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
 	intel_state->clip.x1 = 0;
@@ -180,7 +180,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 		}
 	}
 
-	intel_state->visible = false;
+	intel_state->base.visible = false;
 	ret = intel_plane->check_plane(plane, crtc_state, intel_state);
 	if (ret)
 		return ret;
@@ -196,7 +196,7 @@ static void intel_plane_atomic_update(struct drm_plane *plane,
 		to_intel_plane_state(plane->state);
 	struct drm_crtc *crtc = plane->state->crtc ?: old_state->crtc;
 
-	if (intel_state->visible)
+	if (intel_state->base.visible)
 		intel_plane->update_plane(plane,
 					  to_intel_crtc_state(crtc->state),
 					  intel_state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 78beb7e9d384..4f67b7c19b75 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2565,7 +2565,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 	 * simplest solution is to just disable the primary plane now and
 	 * pretend the BIOS never had it enabled.
 	 */
-	to_intel_plane_state(plane_state)->visible = false;
+	to_intel_plane_state(plane_state)->base.visible = false;
 	crtc_state->plane_mask &= ~(1 << drm_plane_index(primary));
 	intel_pre_disable_primary_noatomic(&intel_crtc->base);
 	intel_plane->disable_plane(primary, &intel_crtc->base);
@@ -2583,14 +2583,14 @@ valid_fb:
 	plane_state->crtc_w = fb->width;
 	plane_state->crtc_h = fb->height;
 
-	intel_state->src.x1 = plane_state->src_x;
-	intel_state->src.y1 = plane_state->src_y;
-	intel_state->src.x2 = plane_state->src_x + plane_state->src_w;
-	intel_state->src.y2 = plane_state->src_y + plane_state->src_h;
-	intel_state->dst.x1 = plane_state->crtc_x;
-	intel_state->dst.y1 = plane_state->crtc_y;
-	intel_state->dst.x2 = plane_state->crtc_x + plane_state->crtc_w;
-	intel_state->dst.y2 = plane_state->crtc_y + plane_state->crtc_h;
+	intel_state->base.src.x1 = plane_state->src_x;
+	intel_state->base.src.y1 = plane_state->src_y;
+	intel_state->base.src.x2 = plane_state->src_x + plane_state->src_w;
+	intel_state->base.src.y2 = plane_state->src_y + plane_state->src_h;
+	intel_state->base.dst.x1 = plane_state->crtc_x;
+	intel_state->base.dst.y1 = plane_state->crtc_y;
+	intel_state->base.dst.x2 = plane_state->crtc_x + plane_state->crtc_w;
+	intel_state->base.dst.y2 = plane_state->crtc_y + plane_state->crtc_h;
 
 	obj = intel_fb_obj(fb);
 	if (obj->tiling_mode != I915_TILING_NONE)
@@ -2618,8 +2618,8 @@ static void i9xx_update_primary_plane(struct drm_plane *primary,
 	i915_reg_t reg = DSPCNTR(plane);
 	unsigned int rotation = plane_state->base.rotation;
 	int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
-	int x = plane_state->src.x1 >> 16;
-	int y = plane_state->src.y1 >> 16;
+	int x = plane_state->base.src.x1 >> 16;
+	int y = plane_state->base.src.y1 >> 16;
 
 	dspcntr = DISPPLANE_GAMMA_ENABLE;
 
@@ -2748,8 +2748,8 @@ static void ironlake_update_primary_plane(struct drm_plane *primary,
 	i915_reg_t reg = DSPCNTR(plane);
 	unsigned int rotation = plane_state->base.rotation;
 	int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
-	int x = plane_state->src.x1 >> 16;
-	int y = plane_state->src.y1 >> 16;
+	int x = plane_state->base.src.x1 >> 16;
+	int y = plane_state->base.src.y1 >> 16;
 
 	dspcntr = DISPPLANE_GAMMA_ENABLE;
 	dspcntr |= DISPLAY_PLANE_ENABLE;
@@ -2987,14 +2987,14 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	int x_offset, y_offset;
 	u32 surf_addr;
 	int scaler_id = plane_state->scaler_id;
-	int src_x = plane_state->src.x1 >> 16;
-	int src_y = plane_state->src.y1 >> 16;
-	int src_w = drm_rect_width(&plane_state->src) >> 16;
-	int src_h = drm_rect_height(&plane_state->src) >> 16;
-	int dst_x = plane_state->dst.x1;
-	int dst_y = plane_state->dst.y1;
-	int dst_w = drm_rect_width(&plane_state->dst);
-	int dst_h = drm_rect_height(&plane_state->dst);
+	int src_x = plane_state->base.src.x1 >> 16;
+	int src_y = plane_state->base.src.y1 >> 16;
+	int src_w = drm_rect_width(&plane_state->base.src) >> 16;
+	int src_h = drm_rect_height(&plane_state->base.src) >> 16;
+	int dst_x = plane_state->base.dst.x1;
+	int dst_y = plane_state->base.dst.y1;
+	int dst_w = drm_rect_width(&plane_state->base.dst);
+	int dst_h = drm_rect_height(&plane_state->base.dst);
 
 	plane_ctl = PLANE_CTL_ENABLE |
 		    PLANE_CTL_PIPE_GAMMA_ENABLE |
@@ -3009,7 +3009,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 					       fb->pixel_format);
 	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj, 0);
 
-	WARN_ON(drm_rect_width(&plane_state->src) == 0);
+	WARN_ON(drm_rect_width(&plane_state->base.src) == 0);
 
 	if (intel_rotation_90_or_270(rotation)) {
 		int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
@@ -3098,7 +3098,7 @@ static void intel_update_primary_planes(struct drm_device *dev)
 		drm_modeset_lock_crtc(crtc, &plane->base);
 		plane_state = to_intel_plane_state(plane->base.state);
 
-		if (plane_state->visible)
+		if (plane_state->base.visible)
 			plane->update_plane(&plane->base,
 					    to_intel_crtc_state(crtc->state),
 					    plane_state);
@@ -4273,7 +4273,7 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
 	struct drm_framebuffer *fb = plane_state->base.fb;
 	int ret;
 
-	bool force_detach = !fb || !plane_state->visible;
+	bool force_detach = !fb || !plane_state->base.visible;
 
 	DRM_DEBUG_KMS("Updating scaler for [PLANE:%d:%s] scaler_user index %u.%u\n",
 		      intel_plane->base.base.id, intel_plane->base.name,
@@ -4283,10 +4283,10 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
 				drm_plane_index(&intel_plane->base),
 				&plane_state->scaler_id,
 				plane_state->base.rotation,
-				drm_rect_width(&plane_state->src) >> 16,
-				drm_rect_height(&plane_state->src) >> 16,
-				drm_rect_width(&plane_state->dst),
-				drm_rect_height(&plane_state->dst));
+				drm_rect_width(&plane_state->base.src) >> 16,
+				drm_rect_height(&plane_state->base.src) >> 16,
+				drm_rect_width(&plane_state->base.dst),
+				drm_rect_height(&plane_state->base.dst));
 
 	if (ret || plane_state->scaler_id < 0)
 		return ret;
@@ -4584,9 +4584,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 
 		intel_fbc_post_update(crtc);
 
-		if (primary_state->visible &&
+		if (primary_state->base.visible &&
 		    (needs_modeset(&pipe_config->base) ||
-		     !old_primary_state->visible))
+		     !old_primary_state->base.visible))
 			intel_post_enable_primary(&crtc->base);
 	}
 }
@@ -4612,8 +4612,8 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 
 		intel_fbc_pre_update(crtc, pipe_config, primary_state);
 
-		if (old_primary_state->visible &&
-		    (modeset || !primary_state->visible))
+		if (old_primary_state->base.visible &&
+		    (modeset || !primary_state->base.visible))
 			intel_pre_disable_primary(&crtc->base);
 	}
 
@@ -6298,13 +6298,13 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
 	if (!intel_crtc->active)
 		return;
 
-	if (to_intel_plane_state(crtc->primary->state)->visible) {
+	if (to_intel_plane_state(crtc->primary->state)->base.visible) {
 		WARN_ON(intel_crtc->flip_work);
 
 		intel_pre_disable_primary_noatomic(crtc);
 
 		intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc->primary));
-		to_intel_plane_state(crtc->primary->state)->visible = false;
+		to_intel_plane_state(crtc->primary->state)->base.visible = false;
 	}
 
 	dev_priv->display.crtc_disable(crtc);
@@ -10178,7 +10178,7 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	uint32_t cntl = 0, size = 0;
 
-	if (plane_state && plane_state->visible) {
+	if (plane_state && plane_state->base.visible) {
 		unsigned int width = plane_state->base.crtc_w;
 		unsigned int height = plane_state->base.crtc_h;
 		unsigned int stride = roundup_pow_of_two(width) * 4;
@@ -10242,7 +10242,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
 	int pipe = intel_crtc->pipe;
 	uint32_t cntl = 0;
 
-	if (plane_state && plane_state->visible) {
+	if (plane_state && plane_state->base.visible) {
 		cntl = MCURSOR_GAMMA_ENABLE;
 		switch (plane_state->base.crtc_w) {
 			case 64:
@@ -11824,7 +11824,7 @@ static bool intel_wm_need_update(struct drm_plane *plane,
 	struct intel_plane_state *cur = to_intel_plane_state(plane->state);
 
 	/* Update watermarks on tiling or size changes. */
-	if (new->visible != cur->visible)
+	if (new->base.visible != cur->base.visible)
 		return true;
 
 	if (!cur->base.fb || !new->base.fb)
@@ -11832,10 +11832,10 @@ static bool intel_wm_need_update(struct drm_plane *plane,
 
 	if (cur->base.fb->modifier[0] != new->base.fb->modifier[0] ||
 	    cur->base.rotation != new->base.rotation ||
-	    drm_rect_width(&new->src) != drm_rect_width(&cur->src) ||
-	    drm_rect_height(&new->src) != drm_rect_height(&cur->src) ||
-	    drm_rect_width(&new->dst) != drm_rect_width(&cur->dst) ||
-	    drm_rect_height(&new->dst) != drm_rect_height(&cur->dst))
+	    drm_rect_width(&new->base.src) != drm_rect_width(&cur->base.src) ||
+	    drm_rect_height(&new->base.src) != drm_rect_height(&cur->base.src) ||
+	    drm_rect_width(&new->base.dst) != drm_rect_width(&cur->base.dst) ||
+	    drm_rect_height(&new->base.dst) != drm_rect_height(&cur->base.dst))
 		return true;
 
 	return false;
@@ -11843,10 +11843,10 @@ static bool intel_wm_need_update(struct drm_plane *plane,
 
 static bool needs_scaling(struct intel_plane_state *state)
 {
-	int src_w = drm_rect_width(&state->src) >> 16;
-	int src_h = drm_rect_height(&state->src) >> 16;
-	int dst_w = drm_rect_width(&state->dst);
-	int dst_h = drm_rect_height(&state->dst);
+	int src_w = drm_rect_width(&state->base.src) >> 16;
+	int src_h = drm_rect_height(&state->base.src) >> 16;
+	int dst_w = drm_rect_width(&state->base.dst);
+	int dst_h = drm_rect_height(&state->base.dst);
 
 	return (src_w != dst_w || src_h != dst_h);
 }
@@ -11877,8 +11877,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 			return ret;
 	}
 
-	was_visible = old_plane_state->visible;
-	visible = to_intel_plane_state(plane_state)->visible;
+	was_visible = old_plane_state->base.visible;
+	visible = to_intel_plane_state(plane_state)->base.visible;
 
 	if (!was_crtc_enabled && WARN_ON(was_visible))
 		was_visible = false;
@@ -11894,7 +11894,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	 * only combine the results from all planes in the current place?
 	 */
 	if (!is_crtc_enabled)
-		to_intel_plane_state(plane_state)->visible = visible = false;
+		to_intel_plane_state(plane_state)->base.visible = visible = false;
 
 	if (!was_visible && !visible)
 		return 0;
@@ -12299,12 +12299,13 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 			      drm_get_format_name(fb->pixel_format));
 		DRM_DEBUG_KMS("\tscaler:%d src %dx%d+%d+%d dst %dx%d+%d+%d\n",
 			      state->scaler_id,
-			      state->src.x1 >> 16, state->src.y1 >> 16,
-			      drm_rect_width(&state->src) >> 16,
-			      drm_rect_height(&state->src) >> 16,
-			      state->dst.x1, state->dst.y1,
-			      drm_rect_width(&state->dst),
-			      drm_rect_height(&state->dst));
+			      state->base.src.x1 >> 16,
+			      state->base.src.y1 >> 16,
+			      drm_rect_width(&state->base.src) >> 16,
+			      drm_rect_height(&state->base.src) >> 16,
+			      state->base.dst.x1, state->base.dst.y1,
+			      drm_rect_width(&state->base.dst),
+			      drm_rect_height(&state->base.dst));
 	}
 }
 
@@ -14139,12 +14140,14 @@ intel_check_primary_plane(struct drm_plane *plane,
 		can_position = true;
 	}
 
-	return drm_plane_helper_check_update(plane, crtc, fb, &state->src,
-					     &state->dst, &state->clip,
+	return drm_plane_helper_check_update(plane, crtc, fb,
+					     &state->base.src,
+					     &state->base.dst,
+					     &state->base.clip,
 					     state->base.rotation,
 					     min_scale, max_scale,
 					     can_position, true,
-					     &state->visible);
+					     &state->base.visible);
 }
 
 static void intel_begin_crtc_commit(struct drm_crtc *crtc,
@@ -14331,12 +14334,13 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	unsigned stride;
 	int ret;
 
-	ret = drm_plane_helper_check_update(plane, crtc, fb, &state->src,
-					    &state->dst, &state->clip,
+	ret = drm_plane_helper_check_update(plane, crtc, fb, &state->base.src,
+					    &state->base.dst,
+					    &state->base.clip,
 					    state->base.rotation,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    DRM_PLANE_HELPER_NO_SCALING,
-					    true, true, &state->visible);
+					    true, true, &state->base.visible);
 	if (ret)
 		return ret;
 
@@ -14373,7 +14377,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	 * Refuse the put the cursor into that compromised position.
 	 */
 	if (IS_CHERRYVIEW(plane->dev) && pipe == PIPE_C &&
-	    state->visible && state->base.crtc_x < 0) {
+	    state->base.visible && state->base.crtc_x < 0) {
 		DRM_DEBUG_KMS("CHV cursor C not allowed to straddle the left screen edge\n");
 		return -EINVAL;
 	}
@@ -15822,7 +15826,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		 * Temporarily change the plane mapping and disable everything
 		 * ...  */
 		plane = crtc->plane;
-		to_intel_plane_state(crtc->base.primary->state)->visible = true;
+		to_intel_plane_state(crtc->base.primary->state)->base.visible = true;
 		crtc->plane = !plane;
 		intel_crtc_disable_noatomic(&crtc->base);
 		crtc->plane = plane;
@@ -15949,10 +15953,10 @@ static void readout_plane_state(struct intel_crtc *crtc)
 	struct intel_plane_state *plane_state =
 		to_intel_plane_state(primary->state);
 
-	plane_state->visible = crtc->active &&
+	plane_state->base.visible = crtc->active &&
 		primary_get_hw_state(to_intel_plane(primary));
 
-	if (plane_state->visible)
+	if (plane_state->base.visible)
 		crtc->base.state->plane_mask |= 1 << drm_plane_index(primary);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e74d851868c5..2f533aa0c3f3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -338,10 +338,7 @@ struct intel_atomic_state {
 
 struct intel_plane_state {
 	struct drm_plane_state base;
-	struct drm_rect src;
-	struct drm_rect dst;
 	struct drm_rect clip;
-	bool visible;
 
 	/*
 	 * scaler_id
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 781e2f5f7cd8..23d494b18459 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -494,7 +494,7 @@ static bool multiple_pipes_ok(struct intel_crtc *crtc,
 	if (!no_fbc_on_multiple_pipes(dev_priv))
 		return true;
 
-	if (plane_state->visible)
+	if (plane_state->base.visible)
 		fbc->visible_pipes_mask |= (1 << pipe);
 	else
 		fbc->visible_pipes_mask &= ~(1 << pipe);
@@ -725,9 +725,9 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
 			ilk_pipe_pixel_rate(crtc_state);
 
 	cache->plane.rotation = plane_state->base.rotation;
-	cache->plane.src_w = drm_rect_width(&plane_state->src) >> 16;
-	cache->plane.src_h = drm_rect_height(&plane_state->src) >> 16;
-	cache->plane.visible = plane_state->visible;
+	cache->plane.src_w = drm_rect_width(&plane_state->base.src) >> 16;
+	cache->plane.src_h = drm_rect_height(&plane_state->base.src) >> 16;
+	cache->plane.visible = plane_state->base.visible;
 
 	if (!cache->plane.visible)
 		return;
@@ -1050,7 +1050,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
 		struct intel_plane_state *intel_plane_state =
 			to_intel_plane_state(plane_state);
 
-		if (!intel_plane_state->visible)
+		if (!intel_plane_state->base.visible)
 			continue;
 
 		for_each_crtc_in_state(state, crtc, crtc_state, j) {
@@ -1214,7 +1214,7 @@ void intel_fbc_init_pipe_state(struct drm_i915_private *dev_priv)
 
 	for_each_intel_crtc(&dev_priv->drm, crtc)
 		if (intel_crtc_active(&crtc->base) &&
-		    to_intel_plane_state(crtc->base.primary->state)->visible)
+		    to_intel_plane_state(crtc->base.primary->state)->base.visible)
 			dev_priv->fbc.visible_pipes_mask |= (1 << crtc->pipe);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 64d628c915a3..8584e8f78b2e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -960,7 +960,7 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
 	if (dev_priv->wm.pri_latency[level] == 0)
 		return USHRT_MAX;
 
-	if (!state->visible)
+	if (!state->base.visible)
 		return 0;
 
 	cpp = drm_format_plane_cpp(state->base.fb->pixel_format, 0);
@@ -1002,7 +1002,7 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
 		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
 			continue;
 
-		if (state->visible) {
+		if (state->base.visible) {
 			wm_state->num_active_planes++;
 			total_rate += drm_format_plane_cpp(state->base.fb->pixel_format, 0);
 		}
@@ -1018,7 +1018,7 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
 			continue;
 		}
 
-		if (!state->visible) {
+		if (!state->base.visible) {
 			plane->wm.fifo_size = 0;
 			continue;
 		}
@@ -1118,7 +1118,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 		struct intel_plane_state *state =
 			to_intel_plane_state(plane->base.state);
 
-		if (!state->visible)
+		if (!state->base.visible)
 			continue;
 
 		/* normal watermarks */
@@ -1767,7 +1767,7 @@ static uint32_t ilk_compute_pri_wm(const struct intel_crtc_state *cstate,
 		drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0;
 	uint32_t method1, method2;
 
-	if (!cstate->base.active || !pstate->visible)
+	if (!cstate->base.active || !pstate->base.visible)
 		return 0;
 
 	method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), cpp, mem_value);
@@ -1777,7 +1777,7 @@ static uint32_t ilk_compute_pri_wm(const struct intel_crtc_state *cstate,
 
 	method2 = ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
 				 cstate->base.adjusted_mode.crtc_htotal,
-				 drm_rect_width(&pstate->dst),
+				 drm_rect_width(&pstate->base.dst),
 				 cpp, mem_value);
 
 	return min(method1, method2);
@@ -1795,13 +1795,13 @@ static uint32_t ilk_compute_spr_wm(const struct intel_crtc_state *cstate,
 		drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0;
 	uint32_t method1, method2;
 
-	if (!cstate->base.active || !pstate->visible)
+	if (!cstate->base.active || !pstate->base.visible)
 		return 0;
 
 	method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), cpp, mem_value);
 	method2 = ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
 				 cstate->base.adjusted_mode.crtc_htotal,
-				 drm_rect_width(&pstate->dst),
+				 drm_rect_width(&pstate->base.dst),
 				 cpp, mem_value);
 	return min(method1, method2);
 }
@@ -1820,7 +1820,7 @@ static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate,
 	 * this is necessary to avoid flickering.
 	 */
 	int cpp = 4;
-	int width = pstate->visible ? pstate->base.crtc_w : 64;
+	int width = pstate->base.visible ? pstate->base.crtc_w : 64;
 
 	if (!cstate->base.active)
 		return 0;
@@ -1838,10 +1838,10 @@ static uint32_t ilk_compute_fbc_wm(const struct intel_crtc_state *cstate,
 	int cpp = pstate->base.fb ?
 		drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0;
 
-	if (!cstate->base.active || !pstate->visible)
+	if (!cstate->base.active || !pstate->base.visible)
 		return 0;
 
-	return ilk_wm_fbc(pri_val, drm_rect_width(&pstate->dst), cpp);
+	return ilk_wm_fbc(pri_val, drm_rect_width(&pstate->base.dst), cpp);
 }
 
 static unsigned int ilk_display_fifo_size(const struct drm_device *dev)
@@ -2358,10 +2358,10 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
 
 	pipe_wm->pipe_enabled = cstate->base.active;
 	if (sprstate) {
-		pipe_wm->sprites_enabled = sprstate->visible;
-		pipe_wm->sprites_scaled = sprstate->visible &&
-			(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
-			 drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
+		pipe_wm->sprites_enabled = sprstate->base.visible;
+		pipe_wm->sprites_scaled = sprstate->base.visible &&
+			(drm_rect_width(&sprstate->base.dst) != drm_rect_width(&sprstate->base.src) >> 16 ||
+			 drm_rect_height(&sprstate->base.dst) != drm_rect_height(&sprstate->base.src) >> 16);
 	}
 
 	usable_level = max_level;
@@ -2996,14 +2996,14 @@ skl_plane_downscale_amount(const struct intel_plane_state *pstate)
 	uint32_t downscale_h, downscale_w;
 	uint32_t src_w, src_h, dst_w, dst_h;
 
-	if (WARN_ON(!pstate->visible))
+	if (WARN_ON(!pstate->base.visible))
 		return DRM_PLANE_HELPER_NO_SCALING;
 
 	/* n.b., src is 16.16 fixed point, dst is whole integer */
-	src_w = drm_rect_width(&pstate->src);
-	src_h = drm_rect_height(&pstate->src);
-	dst_w = drm_rect_width(&pstate->dst);
-	dst_h = drm_rect_height(&pstate->dst);
+	src_w = drm_rect_width(&pstate->base.src);
+	src_h = drm_rect_height(&pstate->base.src);
+	dst_w = drm_rect_width(&pstate->base.dst);
+	dst_h = drm_rect_height(&pstate->base.dst);
 	if (intel_rotation_90_or_270(pstate->base.rotation))
 		swap(dst_w, dst_h);
 
@@ -3025,15 +3025,15 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 	uint32_t width = 0, height = 0;
 	unsigned format = fb ? fb->pixel_format : DRM_FORMAT_XRGB8888;
 
-	if (!intel_pstate->visible)
+	if (!intel_pstate->base.visible)
 		return 0;
 	if (pstate->plane->type == DRM_PLANE_TYPE_CURSOR)
 		return 0;
 	if (y && format != DRM_FORMAT_NV12)
 		return 0;
 
-	width = drm_rect_width(&intel_pstate->src) >> 16;
-	height = drm_rect_height(&intel_pstate->src) >> 16;
+	width = drm_rect_width(&intel_pstate->base.src) >> 16;
+	height = drm_rect_height(&intel_pstate->base.src) >> 16;
 
 	if (intel_rotation_90_or_270(pstate->rotation))
 		swap(width, height);
@@ -3134,8 +3134,8 @@ skl_ddb_min_alloc(const struct drm_plane_state *pstate,
 	    fb->modifier[0] != I915_FORMAT_MOD_Yf_TILED)
 		return 8;
 
-	src_w = drm_rect_width(&intel_pstate->src) >> 16;
-	src_h = drm_rect_height(&intel_pstate->src) >> 16;
+	src_w = drm_rect_width(&intel_pstate->base.src) >> 16;
+	src_h = drm_rect_height(&intel_pstate->base.src) >> 16;
 
 	if (intel_rotation_90_or_270(pstate->rotation))
 		swap(src_w, src_h);
@@ -3226,7 +3226,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		if (intel_plane->pipe != pipe)
 			continue;
 
-		if (!to_intel_plane_state(pstate)->visible) {
+		if (!to_intel_plane_state(pstate)->base.visible) {
 			minimum[id] = 0;
 			y_minimum[id] = 0;
 			continue;
@@ -3363,7 +3363,7 @@ static uint32_t skl_adjusted_plane_pixel_rate(const struct intel_crtc_state *cst
 	uint64_t pixel_rate;
 
 	/* Shouldn't reach here on disabled planes... */
-	if (WARN_ON(!pstate->visible))
+	if (WARN_ON(!pstate->base.visible))
 		return 0;
 
 	/*
@@ -3399,13 +3399,13 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	uint32_t width = 0, height = 0;
 	uint32_t plane_pixel_rate;
 
-	if (latency == 0 || !cstate->base.active || !intel_pstate->visible) {
+	if (latency == 0 || !cstate->base.active || !intel_pstate->base.visible) {
 		*enabled = false;
 		return 0;
 	}
 
-	width = drm_rect_width(&intel_pstate->src) >> 16;
-	height = drm_rect_height(&intel_pstate->src) >> 16;
+	width = drm_rect_width(&intel_pstate->base.src) >> 16;
+	height = drm_rect_height(&intel_pstate->base.src) >> 16;
 
 	if (intel_rotation_90_or_270(pstate->rotation))
 		swap(width, height);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0de935ad01c2..10f10915c0bc 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -211,14 +211,14 @@ skl_update_plane(struct drm_plane *drm_plane,
 	u32 tile_height, plane_offset, plane_size;
 	unsigned int rotation = plane_state->base.rotation;
 	int x_offset, y_offset;
-	int crtc_x = plane_state->dst.x1;
-	int crtc_y = plane_state->dst.y1;
-	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
-	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
-	uint32_t x = plane_state->src.x1 >> 16;
-	uint32_t y = plane_state->src.y1 >> 16;
-	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
-	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
+	int crtc_x = plane_state->base.dst.x1;
+	int crtc_y = plane_state->base.dst.y1;
+	uint32_t crtc_w = drm_rect_width(&plane_state->base.dst);
+	uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
+	uint32_t x = plane_state->base.src.x1 >> 16;
+	uint32_t y = plane_state->base.src.y1 >> 16;
+	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
+	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
 
 	plane_ctl = PLANE_CTL_ENABLE |
 		PLANE_CTL_PIPE_GAMMA_ENABLE |
@@ -370,14 +370,14 @@ vlv_update_plane(struct drm_plane *dplane,
 	unsigned int rotation = dplane->state->rotation;
 	int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
 	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
-	int crtc_x = plane_state->dst.x1;
-	int crtc_y = plane_state->dst.y1;
-	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
-	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
-	uint32_t x = plane_state->src.x1 >> 16;
-	uint32_t y = plane_state->src.y1 >> 16;
-	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
-	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
+	int crtc_x = plane_state->base.dst.x1;
+	int crtc_y = plane_state->base.dst.y1;
+	uint32_t crtc_w = drm_rect_width(&plane_state->base.dst);
+	uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
+	uint32_t x = plane_state->base.src.x1 >> 16;
+	uint32_t y = plane_state->base.src.y1 >> 16;
+	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
+	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
 
 	sprctl = SP_ENABLE;
 
@@ -512,14 +512,14 @@ ivb_update_plane(struct drm_plane *plane,
 	unsigned int rotation = plane_state->base.rotation;
 	int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
 	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
-	int crtc_x = plane_state->dst.x1;
-	int crtc_y = plane_state->dst.y1;
-	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
-	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
-	uint32_t x = plane_state->src.x1 >> 16;
-	uint32_t y = plane_state->src.y1 >> 16;
-	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
-	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
+	int crtc_x = plane_state->base.dst.x1;
+	int crtc_y = plane_state->base.dst.y1;
+	uint32_t crtc_w = drm_rect_width(&plane_state->base.dst);
+	uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
+	uint32_t x = plane_state->base.src.x1 >> 16;
+	uint32_t y = plane_state->base.src.y1 >> 16;
+	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
+	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
 
 	sprctl = SPRITE_ENABLE;
 
@@ -653,14 +653,14 @@ ilk_update_plane(struct drm_plane *plane,
 	unsigned int rotation = plane_state->base.rotation;
 	int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
 	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
-	int crtc_x = plane_state->dst.x1;
-	int crtc_y = plane_state->dst.y1;
-	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
-	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
-	uint32_t x = plane_state->src.x1 >> 16;
-	uint32_t y = plane_state->src.y1 >> 16;
-	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
-	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
+	int crtc_x = plane_state->base.dst.x1;
+	int crtc_y = plane_state->base.dst.y1;
+	uint32_t crtc_w = drm_rect_width(&plane_state->base.dst);
+	uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
+	uint32_t x = plane_state->base.src.x1 >> 16;
+	uint32_t y = plane_state->base.src.y1 >> 16;
+	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
+	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
 
 	dvscntr = DVS_ENABLE;
 
@@ -778,15 +778,15 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	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->src;
-	struct drm_rect *dst = &state->dst;
+	struct drm_rect *src = &state->base.src;
+	struct drm_rect *dst = &state->base.dst;
 	const struct drm_rect *clip = &state->clip;
 	int hscale, vscale;
 	int max_scale, min_scale;
 	bool can_scale;
 
 	if (!fb) {
-		state->visible = false;
+		state->base.visible = false;
 		return 0;
 	}
 
@@ -834,14 +834,14 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
 	BUG_ON(vscale < 0);
 
-	state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
+	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);
 
-	if (state->visible) {
+	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) {
@@ -898,12 +898,12 @@ intel_check_sprite_plane(struct drm_plane *plane,
 				crtc_w &= ~1;
 
 			if (crtc_w == 0)
-				state->visible = false;
+				state->base.visible = false;
 		}
 	}
 
 	/* Check size restrictions when scaling */
-	if (state->visible && (src_w != crtc_w || src_h != crtc_h)) {
+	if (state->base.visible && (src_w != crtc_w || src_h != crtc_h)) {
 		unsigned int width_bytes;
 		int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
 
@@ -912,10 +912,10 @@ intel_check_sprite_plane(struct drm_plane *plane,
 		/* FIXME interlacing min height is 6 */
 
 		if (crtc_w < 3 || crtc_h < 3)
-			state->visible = false;
+			state->base.visible = false;
 
 		if (src_w < 3 || src_h < 3)
-			state->visible = false;
+			state->base.visible = false;
 
 		width_bytes = ((src_x * cpp) & 63) + src_w * cpp;
 
@@ -926,7 +926,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 		}
 	}
 
-	if (state->visible) {
+	if (state->base.visible) {
 		src->x1 = src_x << 16;
 		src->x2 = (src_x + src_w) << 16;
 		src->y1 = src_y << 16;
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/9] drm/i915: Use drm_plane_helper_check_state()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Replace the use of drm_plane_helper_check_update() with
drm_plane_helper_check_state() since we have a plane state.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 14 --------------
 drivers/gpu/drm/i915/intel_display.c      | 26 +++++++++-----------------
 drivers/gpu/drm/i915/intel_sprite.c       | 10 ++++++++++
 3 files changed, 19 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 14d40261db21..2d831dd43d44 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -134,20 +134,6 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 
 	crtc_state = to_intel_crtc_state(drm_crtc_state);
 
-	/*
-	 * The original src/dest coordinates are stored in state->base, but
-	 * we want to keep another copy internal to our driver that we can
-	 * clip/modify ourselves.
-	 */
-	intel_state->base.src.x1 = state->src_x;
-	intel_state->base.src.y1 = state->src_y;
-	intel_state->base.src.x2 = state->src_x + state->src_w;
-	intel_state->base.src.y2 = state->src_y + state->src_h;
-	intel_state->base.dst.x1 = state->crtc_x;
-	intel_state->base.dst.y1 = state->crtc_y;
-	intel_state->base.dst.x2 = state->crtc_x + state->crtc_w;
-	intel_state->base.dst.y2 = state->crtc_y + state->crtc_h;
-
 	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
 	intel_state->clip.x1 = 0;
 	intel_state->clip.y1 = 0;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4f67b7c19b75..eae8a72bad5a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14126,7 +14126,6 @@ intel_check_primary_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->base.crtc;
-	struct drm_framebuffer *fb = state->base.fb;
 	int min_scale = DRM_PLANE_HELPER_NO_SCALING;
 	int max_scale = DRM_PLANE_HELPER_NO_SCALING;
 	bool can_position = false;
@@ -14140,14 +14139,10 @@ intel_check_primary_plane(struct drm_plane *plane,
 		can_position = true;
 	}
 
-	return drm_plane_helper_check_update(plane, crtc, fb,
-					     &state->base.src,
-					     &state->base.dst,
-					     &state->base.clip,
-					     state->base.rotation,
-					     min_scale, max_scale,
-					     can_position, true,
-					     &state->base.visible);
+	return drm_plane_helper_check_state(&state->base,
+					    &state->clip,
+					    min_scale, max_scale,
+					    can_position, true);
 }
 
 static void intel_begin_crtc_commit(struct drm_crtc *crtc,
@@ -14327,20 +14322,17 @@ intel_check_cursor_plane(struct drm_plane *plane,
 			 struct intel_crtc_state *crtc_state,
 			 struct intel_plane_state *state)
 {
-	struct drm_crtc *crtc = crtc_state->base.crtc;
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	enum pipe pipe = to_intel_plane(plane)->pipe;
 	unsigned stride;
 	int ret;
 
-	ret = drm_plane_helper_check_update(plane, crtc, fb, &state->base.src,
-					    &state->base.dst,
-					    &state->base.clip,
-					    state->base.rotation,
-					    DRM_PLANE_HELPER_NO_SCALING,
-					    DRM_PLANE_HELPER_NO_SCALING,
-					    true, true, &state->base.visible);
+	ret = drm_plane_helper_check_state(&state->base,
+					   &state->clip,
+					   DRM_PLANE_HELPER_NO_SCALING,
+					   DRM_PLANE_HELPER_NO_SCALING,
+					   true, true);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 10f10915c0bc..b38956624f04 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -785,6 +785,16 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	int max_scale, min_scale;
 	bool can_scale;
 
+	src->x1 = state->base.src_x;
+	src->y1 = state->base.src_y;
+	src->x2 = state->base.src_x + state->base.src_w;
+	src->y2 = state->base.src_y + state->base.src_h;
+
+	dst->x1 = state->base.crtc_x;
+	dst->y1 = state->base.crtc_y;
+	dst->x2 = state->base.crtc_x + state->base.crtc_w;
+	dst->y2 = state->base.crtc_y + state->base.crtc_h;
+
 	if (!fb) {
 		state->base.visible = false;
 		return 0;
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/9] drm/rockchip: Use drm_plane_state.{src,dst}

[ville.syrjala-VuQAYsv1563Yd54FQh9/CA]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Replace the private drm_rects in vop_plane_state with
the ones now living in drm_plane_state.

Cc: Yao <mark.yao@rock-chips.com>
Cc: linux-rockchip@lists.infradead.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 91305eb7d312..c566c740ab49 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -87,8 +87,6 @@
 struct vop_plane_state {
 	struct drm_plane_state base;
 	int format;
-	struct drm_rect src;
-	struct drm_rect dest;
 	dma_addr_t yrgb_mst;
 	bool enable;
 };
@@ -595,8 +593,8 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
 	const struct vop_win_data *win = vop_win->data;
 	bool visible;
 	int ret;
-	struct drm_rect *dest = &vop_plane_state->dest;
-	struct drm_rect *src = &vop_plane_state->src;
+	struct drm_rect *dest = &state->dst;
+	struct drm_rect *src = &state->src;
 	struct drm_rect clip;
 	int min_scale = win->phy->scl ? FRAC_16_16(1, 8) :
 					DRM_PLANE_HELPER_NO_SCALING;
@@ -694,8 +692,8 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
 	unsigned int actual_w, actual_h;
 	unsigned int dsp_stx, dsp_sty;
 	uint32_t act_info, dsp_info, dsp_st;
-	struct drm_rect *src = &vop_plane_state->src;
-	struct drm_rect *dest = &vop_plane_state->dest;
+	struct drm_rect *src = &state->src;
+	struct drm_rect *dest = &state->dst;
 	struct drm_gem_object *obj, *uv_obj;
 	struct rockchip_gem_object *rk_obj, *rk_uv_obj;
 	unsigned long offset;
-- 
2.7.4


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH 7/9] drm/rockchip: Use drm_plane_helper_check_state()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Replace the use of drm_plane_helper_check_update() with
drm_plane_helper_check_state() since we have a plane state.

Rockchip looks to handling plane clipping rather well already
(unlikje most arm drm drivers) so there are no function changes
here.

Cc: Yao <mark.yao@rock-chips.com>
Cc: linux-rockchip@lists.infradead.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index c566c740ab49..31744fe99b38 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -591,10 +591,7 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
 	struct vop_win *vop_win = to_vop_win(plane);
 	struct vop_plane_state *vop_plane_state = to_vop_plane_state(state);
 	const struct vop_win_data *win = vop_win->data;
-	bool visible;
 	int ret;
-	struct drm_rect *dest = &state->dst;
-	struct drm_rect *src = &state->src;
 	struct drm_rect clip;
 	int min_scale = win->phy->scl ? FRAC_16_16(1, 8) :
 					DRM_PLANE_HELPER_NO_SCALING;
@@ -608,30 +605,18 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
 	if (WARN_ON(!crtc_state))
 		return -EINVAL;
 
-	src->x1 = state->src_x;
-	src->y1 = state->src_y;
-	src->x2 = state->src_x + state->src_w;
-	src->y2 = state->src_y + state->src_h;
-	dest->x1 = state->crtc_x;
-	dest->y1 = state->crtc_y;
-	dest->x2 = state->crtc_x + state->crtc_w;
-	dest->y2 = state->crtc_y + state->crtc_h;
-
 	clip.x1 = 0;
 	clip.y1 = 0;
 	clip.x2 = crtc_state->adjusted_mode.hdisplay;
 	clip.y2 = crtc_state->adjusted_mode.vdisplay;
 
-	ret = drm_plane_helper_check_update(plane, crtc, state->fb,
-					    src, dest, &clip,
-					    state->rotation,
-					    min_scale,
-					    max_scale,
-					    true, true, &visible);
+	ret = drm_plane_helper_check_state(state, &clip,
+					   min_scale, max_scale,
+					   true, true);
 	if (ret)
 		return ret;
 
-	if (!visible)
+	if (!state->visible)
 		goto out_disable;
 
 	vop_plane_state->format = vop_convert_format(fb->pixel_format);
@@ -642,7 +627,7 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
 	 * Src.x1 can be odd when do clip, but yuv plane start point
 	 * need align with 2 pixel.
 	 */
-	if (is_yuv_support(fb->pixel_format) && ((src->x1 >> 16) % 2))
+	if (is_yuv_support(fb->pixel_format) && ((state->src.x1 >> 16) % 2))
 		return -EINVAL;
 
 	vop_plane_state->enable = true;
-- 
2.7.4

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

[PATCH 8/9] drm/mediatek: Use drm_plane_helper_check_state()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Replace the use of drm_plane_helper_check_update() with
drm_plane_helper_check_state() since we have a plane state.

This also eliminates the double clipping the driver was doing
in both check and commit phases). And it should fix src coordinate
addr adjustement. Previously the driver was expecting negative dst
coordinates after clipping, which is not going happen, so any clipping
induced addr adjustment simply didn't happen. Neither did the driver
respect any user configured src coordinates, so panning and such would
have been totally broken. It should be all good now.

Cc: CK Hu <ck.hu@mediatek.com>
Cc: linux-mediatek@lists.infradead.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 72 +++++++++-----------------------
 1 file changed, 20 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index 3995765a90dc..5f2516fca079 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -30,15 +30,20 @@ static const u32 formats[] = {
 	DRM_FORMAT_RGB565,
 };
 
-static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable,
-			     dma_addr_t addr, struct drm_rect *dest)
+static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane,
+			     dma_addr_t addr)
 {
 	struct drm_plane *plane = &mtk_plane->base;
 	struct mtk_plane_state *state = to_mtk_plane_state(plane->state);
 	unsigned int pitch, format;
-	int x, y;
+	bool enable;
 
-	if (WARN_ON(!plane->state || (enable && !plane->state->fb)))
+	if (WARN_ON(!plane->state))
+		return;
+
+	enable = state->base.visible;
+
+	if (WARN_ON(enable && !plane->state->fb))
 		return;
 
 	if (plane->state->fb) {
@@ -49,27 +54,17 @@ static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable,
 		format = DRM_FORMAT_RGBA8888;
 	}
 
-	x = plane->state->crtc_x;
-	y = plane->state->crtc_y;
-
-	if (x < 0) {
-		addr -= x * 4;
-		x = 0;
-	}
-
-	if (y < 0) {
-		addr -= y * pitch;
-		y = 0;
-	}
+	addr += (state->base.src.x1 >> 16) * 4;
+	addr += (state->base.src.y1 >> 16) * pitch;
 
 	state->pending.enable = enable;
 	state->pending.pitch = pitch;
 	state->pending.format = format;
 	state->pending.addr = addr;
-	state->pending.x = x;
-	state->pending.y = y;
-	state->pending.width = dest->x2 - dest->x1;
-	state->pending.height = dest->y2 - dest->y1;
+	state->pending.x = state->base.dst.x1;
+	state->pending.y = state->base.dst.y1;
+	state->pending.width = drm_rect_width(&state->base.dst);
+	state->pending.height = drm_rect_height(&state->base.dst);
 	wmb(); /* Make sure the above parameters are set before update */
 	state->pending.dirty = true;
 }
@@ -134,20 +129,6 @@ static int mtk_plane_atomic_check(struct drm_plane *plane,
 {
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_crtc_state *crtc_state;
-	bool visible;
-	struct drm_rect dest = {
-		.x1 = state->crtc_x,
-		.y1 = state->crtc_y,
-		.x2 = state->crtc_x + state->crtc_w,
-		.y2 = state->crtc_y + state->crtc_h,
-	};
-	struct drm_rect src = {
-		/* 16.16 fixed point */
-		.x1 = state->src_x,
-		.y1 = state->src_y,
-		.x2 = state->src_x + state->src_w,
-		.y2 = state->src_y + state->src_h,
-	};
 	struct drm_rect clip = { 0, };
 
 	if (!fb)
@@ -168,12 +149,10 @@ static int mtk_plane_atomic_check(struct drm_plane *plane,
 	clip.x2 = crtc_state->mode.hdisplay;
 	clip.y2 = crtc_state->mode.vdisplay;
 
-	return drm_plane_helper_check_update(plane, state->crtc, fb,
-					     &src, &dest, &clip,
-					     state->rotation,
-					     DRM_PLANE_HELPER_NO_SCALING,
-					     DRM_PLANE_HELPER_NO_SCALING,
-					     true, true, &visible);
+	return drm_plane_helper_check_state(state, &clip,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    true, true);
 }
 
 static void mtk_plane_atomic_update(struct drm_plane *plane,
@@ -184,24 +163,13 @@ static void mtk_plane_atomic_update(struct drm_plane *plane,
 	struct drm_gem_object *gem;
 	struct mtk_drm_gem_obj *mtk_gem;
 	struct mtk_drm_plane *mtk_plane = to_mtk_plane(plane);
-	struct drm_rect dest = {
-		.x1 = state->base.crtc_x,
-		.y1 = state->base.crtc_y,
-		.x2 = state->base.crtc_x + state->base.crtc_w,
-		.y2 = state->base.crtc_y + state->base.crtc_h,
-	};
-	struct drm_rect clip = { 0, };
 
 	if (!crtc)
 		return;
 
-	clip.x2 = state->base.crtc->state->mode.hdisplay;
-	clip.y2 = state->base.crtc->state->mode.vdisplay;
-	drm_rect_intersect(&dest, &clip);
-
 	gem = mtk_fb_get_gem_obj(state->base.fb);
 	mtk_gem = to_mtk_gem_obj(gem);
-	mtk_plane_enable(mtk_plane, true, mtk_gem->dma_addr, &dest);
+	mtk_plane_enable(mtk_plane, mtk_gem->dma_addr);
 }
 
 static void mtk_plane_atomic_disable(struct drm_plane *plane,
-- 
2.7.4

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

[PATCH 9/9] drm/simple_kms_helper: Use drm_plane_helper_check_state()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Replace the use of drm_plane_helper_check_update() with
drm_plane_helper_check_state() since we have a plane state.

I don't see any actual users of drm_simple_kms_helper yet, so
no actual plane clipping bugs to fix.

Cc: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_simple_kms_helper.c | 27 ++++++---------------------
 1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index 0db36d27e90b..0a02efe978ee 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -73,22 +73,9 @@ static const struct drm_crtc_funcs drm_simple_kms_crtc_funcs = {
 static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
 					struct drm_plane_state *plane_state)
 {
-	struct drm_rect src = {
-		.x1 = plane_state->src_x,
-		.y1 = plane_state->src_y,
-		.x2 = plane_state->src_x + plane_state->src_w,
-		.y2 = plane_state->src_y + plane_state->src_h,
-	};
-	struct drm_rect dest = {
-		.x1 = plane_state->crtc_x,
-		.y1 = plane_state->crtc_y,
-		.x2 = plane_state->crtc_x + plane_state->crtc_w,
-		.y2 = plane_state->crtc_y + plane_state->crtc_h,
-	};
 	struct drm_rect clip = { 0 };
 	struct drm_simple_display_pipe *pipe;
 	struct drm_crtc_state *crtc_state;
-	bool visible;
 	int ret;
 
 	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
@@ -102,17 +89,15 @@ static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
 
 	clip.x2 = crtc_state->adjusted_mode.hdisplay;
 	clip.y2 = crtc_state->adjusted_mode.vdisplay;
-	ret = drm_plane_helper_check_update(plane, &pipe->crtc,
-					    plane_state->fb,
-					    &src, &dest, &clip,
-					    plane_state->rotation,
-					    DRM_PLANE_HELPER_NO_SCALING,
-					    DRM_PLANE_HELPER_NO_SCALING,
-					    false, true, &visible);
+
+	ret = drm_plane_helper_check_state(plane_state, &clip,
+					   DRM_PLANE_HELPER_NO_SCALING,
+					   DRM_PLANE_HELPER_NO_SCALING,
+					   false, true);
 	if (ret)
 		return ret;
 
-	if (!visible)
+	if (!plane_state->visible)
 		return -EINVAL;
 
 	if (!pipe->funcs || !pipe->funcs->check)
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/9] drm/i915: Use drm_plane_state.{src,dst,visible}

[Chris Wilson]

On Tue, Jul 26, 2016 at 07:06:59PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Replace the private drm_rects/flags in intel_plane_state
> with the ones now living in drm_plane_state.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Didn't spot any mistakes creeping in, and assuming the patch is
complete:
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/9] drm: Warn about negative sizes when calculating scale factor

[Chris Wilson]

On Tue, Jul 26, 2016 at 07:06:56PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Passing negative width/hight to scale factor calculations is not
> legal. Let's WARN if that happens.

Does this get called with user controllable inputs? A quick grep leads
me to drm_primary_helper_update() which suggests no. Did I miss a
potential user controllable WARN->panic?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Ro.CI.BAT: success for drm: Store clipped coordinates in drm_plane_state

[Patchwork]

== Series Details ==

Series: drm: Store clipped coordinates in drm_plane_state
URL   : https://patchwork.freedesktop.org/series/10279/
State : success

== Summary ==

Series 10279v1 drm: Store clipped coordinates in drm_plane_state
http://patchwork.freedesktop.org/api/1.0/series/10279/revisions/1/mbox

Test kms_cursor_legacy:
        Subgroup basic-cursor-vs-flip-legacy:
                fail       -> PASS       (ro-ilk1-i5-650)
        Subgroup basic-flip-vs-cursor-legacy:
                fail       -> PASS       (ro-hsw-i7-4770r)
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-flip-vs-cursor-varying-size:
                fail       -> PASS       (ro-bdw-i5-5250u)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (ro-byt-n2820)

fi-hsw-i7-4770k  total:235  pass:215  dwarn:0   dfail:0   fail:0   skip:20 
fi-kbl-qkkr      total:235  pass:180  dwarn:29  dfail:0   fail:0   skip:26 
fi-skl-i5-6260u  total:235  pass:223  dwarn:0   dfail:0   fail:0   skip:12 
fi-skl-i7-6700k  total:235  pass:209  dwarn:0   dfail:0   fail:0   skip:26 
fi-snb-i7-2600   total:235  pass:195  dwarn:0   dfail:0   fail:0   skip:40 
ro-bdw-i5-5250u  total:239  pass:218  dwarn:4   dfail:0   fail:1   skip:16 
ro-bdw-i7-5600u  total:239  pass:206  dwarn:0   dfail:0   fail:1   skip:32 
ro-bsw-n3050     total:239  pass:194  dwarn:0   dfail:0   fail:3   skip:42 
ro-byt-n2820     total:239  pass:196  dwarn:0   dfail:0   fail:3   skip:40 
ro-hsw-i3-4010u  total:239  pass:213  dwarn:0   dfail:0   fail:0   skip:26 
ro-hsw-i7-4770r  total:239  pass:213  dwarn:0   dfail:0   fail:0   skip:26 
ro-ilk-i7-620lm  total:239  pass:172  dwarn:1   dfail:0   fail:1   skip:65 
ro-ilk1-i5-650   total:234  pass:172  dwarn:0   dfail:0   fail:2   skip:60 
ro-ivb-i7-3770   total:239  pass:204  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:239  pass:208  dwarn:0   dfail:0   fail:0   skip:31 
ro-skl3-i5-6260u total:239  pass:224  dwarn:0   dfail:0   fail:1   skip:14 
ro-snb-i7-2620M  total:239  pass:197  dwarn:0   dfail:0   fail:1   skip:41 
ro-bdw-i7-5557U failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1611/

819b812 drm-intel-nightly: 2016y-07m-26d-14h-26m-25s UTC integration manifest
a48f990 drm/simple_kms_helper: Use drm_plane_helper_check_state()
06794fa drm/mediatek: Use drm_plane_helper_check_state()
8130a90 drm/rockchip: Use drm_plane_helper_check_state()
56900eb drm/rockchip: Use drm_plane_state.{src, dst}
13d220e drm/i915: Use drm_plane_helper_check_state()
3a8e68b drm/i915: Use drm_plane_state.{src, dst, visible}
2ff62fe drm/plane-helper: Add drm_plane_helper_check_state()
40c802b drm: Store clipped src/dst coordinatee in drm_plane_state
e772d51 drm: Warn about negative sizes when calculating scale factor

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

Re: [PATCH 3/9] drm/plane-helper: Add drm_plane_helper_check_state()

[Chris Wilson]

On Tue, Jul 26, 2016 at 07:06:58PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add a version of drm_plane_helper_check_update() which takes a plane
> state instead of having the caller pass in everything.
> 
> And to reduce code duplication, let's reimplement
> drm_plane_helper_check_update() in terms of the new function, by
> having a tempororary plane state on the stack.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_plane_helper.c | 136 ++++++++++++++++++++++++++++---------
>  include/drm/drm_plane_helper.h     |   5 ++
>  2 files changed, 110 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index 16c4a7bd7465..1124eb827671 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -108,14 +108,9 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
>  }
>  
>  /**
> - * drm_plane_helper_check_update() - Check plane update for validity
> - * @plane: plane object to update
> - * @crtc: owning CRTC of owning plane
> - * @fb: framebuffer to flip onto plane
> - * @src: source coordinates in 16.16 fixed point
> - * @dest: integer destination coordinates
> + * drm_plane_helper_check_state() - Check plane state for validity
> + * @state: plane state to check
>   * @clip: integer clipping coordinates
> - * @rotation: plane rotation
>   * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
>   * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
>   * @can_position: is it legal to position the plane such that it
> @@ -123,8 +118,6 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
>   *                only be false for primary planes.
>   * @can_update_disabled: can the plane be updated while the crtc
>   *                       is disabled?
> - * @visible: output parameter indicating whether plane is still visible after
> - *           clipping
>   *
>   * Checks that a desired plane update is valid.  Drivers that provide
>   * their own plane handling rather than helper-provided implementations may
> @@ -134,29 +127,38 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
>   * RETURNS:
>   * Zero if update appears valid, error code on failure
>   */
> -int drm_plane_helper_check_update(struct drm_plane *plane,
> -				  struct drm_crtc *crtc,
> -				  struct drm_framebuffer *fb,
> -				  struct drm_rect *src,
> -				  struct drm_rect *dest,
> -				  const struct drm_rect *clip,
> -				  unsigned int rotation,
> -				  int min_scale,
> -				  int max_scale,
> -				  bool can_position,
> -				  bool can_update_disabled,
> -				  bool *visible)
> +int drm_plane_helper_check_state(struct drm_plane_state *state,
> +				 const struct drm_rect *clip,
> +				 int min_scale,
> +				 int max_scale,
> +				 bool can_position,
> +				 bool can_update_disabled)
>  {
> +	struct drm_crtc *crtc = state->crtc;
> +	struct drm_framebuffer *fb = state->fb;
> +	struct drm_rect *src = &state->src;
> +	struct drm_rect *dst = &state->dst;
> +	unsigned int rotation = state->rotation;
>  	int hscale, vscale;
>  
> +	src->x1 = state->src_x;
> +	src->y1 = state->src_y;
> +	src->x2 = state->src_x + state->src_w;
> +	src->y2 = state->src_y + state->src_h;
> +
> +	dst->x1 = state->crtc_x;
> +	dst->y1 = state->crtc_y;
> +	dst->x2 = state->crtc_x + state->crtc_w;
> +	dst->y2 = state->crtc_y + state->crtc_h;

It's a little surprising to me that the check writes fields into the
drm_plane_state. Care to at least mention the side-effects in the kernel
doc?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/9] drm: Warn about negative sizes when calculating scale factor

[Ville Syrjälä]

On Tue, Jul 26, 2016 at 05:24:42PM +0100, Chris Wilson wrote:
> On Tue, Jul 26, 2016 at 07:06:56PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Passing negative width/hight to scale factor calculations is not
> > legal. Let's WARN if that happens.
> 
> Does this get called with user controllable inputs?

User controllable to a degree. width/height can only ever be positive
though.

> A quick grep leads
> me to drm_primary_helper_update() which suggests no. Did I miss a
> potential user controllable WARN->panic?

I just landed in the BUG_ON in intel_sprite.c on account of a typo I
made in the user src/crtc coordinate -> drm_rect conversion. Should
probably replace the BUG_ON() with WARN_ON() in i915 as well...

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

Re: [PATCH 3/9] drm/plane-helper: Add drm_plane_helper_check_state()

[Ville Syrjälä]

On Tue, Jul 26, 2016 at 05:33:29PM +0100, Chris Wilson wrote:
> On Tue, Jul 26, 2016 at 07:06:58PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add a version of drm_plane_helper_check_update() which takes a plane
> > state instead of having the caller pass in everything.
> > 
> > And to reduce code duplication, let's reimplement
> > drm_plane_helper_check_update() in terms of the new function, by
> > having a tempororary plane state on the stack.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_plane_helper.c | 136 ++++++++++++++++++++++++++++---------
> >  include/drm/drm_plane_helper.h     |   5 ++
> >  2 files changed, 110 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> > index 16c4a7bd7465..1124eb827671 100644
> > --- a/drivers/gpu/drm/drm_plane_helper.c
> > +++ b/drivers/gpu/drm/drm_plane_helper.c
> > @@ -108,14 +108,9 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
> >  }
> >  
> >  /**
> > - * drm_plane_helper_check_update() - Check plane update for validity
> > - * @plane: plane object to update
> > - * @crtc: owning CRTC of owning plane
> > - * @fb: framebuffer to flip onto plane
> > - * @src: source coordinates in 16.16 fixed point
> > - * @dest: integer destination coordinates
> > + * drm_plane_helper_check_state() - Check plane state for validity
> > + * @state: plane state to check
> >   * @clip: integer clipping coordinates
> > - * @rotation: plane rotation
> >   * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
> >   * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
> >   * @can_position: is it legal to position the plane such that it
> > @@ -123,8 +118,6 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
> >   *                only be false for primary planes.
> >   * @can_update_disabled: can the plane be updated while the crtc
> >   *                       is disabled?
> > - * @visible: output parameter indicating whether plane is still visible after
> > - *           clipping
> >   *
> >   * Checks that a desired plane update is valid.  Drivers that provide
> >   * their own plane handling rather than helper-provided implementations may
> > @@ -134,29 +127,38 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
> >   * RETURNS:
> >   * Zero if update appears valid, error code on failure
> >   */
> > -int drm_plane_helper_check_update(struct drm_plane *plane,
> > -				  struct drm_crtc *crtc,
> > -				  struct drm_framebuffer *fb,
> > -				  struct drm_rect *src,
> > -				  struct drm_rect *dest,
> > -				  const struct drm_rect *clip,
> > -				  unsigned int rotation,
> > -				  int min_scale,
> > -				  int max_scale,
> > -				  bool can_position,
> > -				  bool can_update_disabled,
> > -				  bool *visible)
> > +int drm_plane_helper_check_state(struct drm_plane_state *state,
> > +				 const struct drm_rect *clip,
> > +				 int min_scale,
> > +				 int max_scale,
> > +				 bool can_position,
> > +				 bool can_update_disabled)
> >  {
> > +	struct drm_crtc *crtc = state->crtc;
> > +	struct drm_framebuffer *fb = state->fb;
> > +	struct drm_rect *src = &state->src;
> > +	struct drm_rect *dst = &state->dst;
> > +	unsigned int rotation = state->rotation;
> >  	int hscale, vscale;
> >  
> > +	src->x1 = state->src_x;
> > +	src->y1 = state->src_y;
> > +	src->x2 = state->src_x + state->src_w;
> > +	src->y2 = state->src_y + state->src_h;
> > +
> > +	dst->x1 = state->crtc_x;
> > +	dst->y1 = state->crtc_y;
> > +	dst->x2 = state->crtc_x + state->crtc_w;
> > +	dst->y2 = state->crtc_y + state->crtc_h;
> 
> It's a little surprising to me that the check writes fields into the
> drm_plane_state. Care to at least mention the side-effects in the kernel
> doc?

That's a lot of what the atomic "check" functions do. A bit of a bad
naming convention that. Not sure who came up with it. Should probably
rename them all to "compute" or something.

But I can at leat expand the docs here a bit in the meantime.

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

[PATCH v2 3/9] drm/plane-helper: Add drm_plane_helper_check_state()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a version of drm_plane_helper_check_update() which takes a plane
state instead of having the caller pass in everything.

And to reduce code duplication, let's reimplement
drm_plane_helper_check_update() in terms of the new function, by
having a tempororary plane state on the stack.

v2: Add a note that the functions modifies the state (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane_helper.c | 139 ++++++++++++++++++++++++++++---------
 include/drm/drm_plane_helper.h     |   5 ++
 2 files changed, 112 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index 16c4a7bd7465..ca02997b1c36 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -108,14 +108,9 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
 }
 
 /**
- * drm_plane_helper_check_update() - Check plane update for validity
- * @plane: plane object to update
- * @crtc: owning CRTC of owning plane
- * @fb: framebuffer to flip onto plane
- * @src: source coordinates in 16.16 fixed point
- * @dest: integer destination coordinates
+ * drm_plane_helper_check_state() - Check plane state for validity
+ * @state: plane state to check
  * @clip: integer clipping coordinates
- * @rotation: plane rotation
  * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
  * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
  * @can_position: is it legal to position the plane such that it
@@ -123,10 +118,9 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
  *                only be false for primary planes.
  * @can_update_disabled: can the plane be updated while the crtc
  *                       is disabled?
- * @visible: output parameter indicating whether plane is still visible after
- *           clipping
  *
- * Checks that a desired plane update is valid.  Drivers that provide
+ * Checks that a desired plane update is valid, and updates various
+ * bits of derived state (clipped coordinates etc.). Drivers that provide
  * their own plane handling rather than helper-provided implementations may
  * still wish to call this function to avoid duplication of error checking
  * code.
@@ -134,29 +128,38 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
  * RETURNS:
  * Zero if update appears valid, error code on failure
  */
-int drm_plane_helper_check_update(struct drm_plane *plane,
-				  struct drm_crtc *crtc,
-				  struct drm_framebuffer *fb,
-				  struct drm_rect *src,
-				  struct drm_rect *dest,
-				  const struct drm_rect *clip,
-				  unsigned int rotation,
-				  int min_scale,
-				  int max_scale,
-				  bool can_position,
-				  bool can_update_disabled,
-				  bool *visible)
+int drm_plane_helper_check_state(struct drm_plane_state *state,
+				 const struct drm_rect *clip,
+				 int min_scale,
+				 int max_scale,
+				 bool can_position,
+				 bool can_update_disabled)
 {
+	struct drm_crtc *crtc = state->crtc;
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_rect *src = &state->src;
+	struct drm_rect *dst = &state->dst;
+	unsigned int rotation = state->rotation;
 	int hscale, vscale;
 
+	src->x1 = state->src_x;
+	src->y1 = state->src_y;
+	src->x2 = state->src_x + state->src_w;
+	src->y2 = state->src_y + state->src_h;
+
+	dst->x1 = state->crtc_x;
+	dst->y1 = state->crtc_y;
+	dst->x2 = state->crtc_x + state->crtc_w;
+	dst->y2 = state->crtc_y + state->crtc_h;
+
 	if (!fb) {
-		*visible = false;
+		state->visible = false;
 		return 0;
 	}
 
 	/* crtc should only be NULL when disabling (i.e., !fb) */
 	if (WARN_ON(!crtc)) {
-		*visible = false;
+		state->visible = false;
 		return 0;
 	}
 
@@ -168,20 +171,20 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
 	drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
 
 	/* Check scaling */
-	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
-	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
+	hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
+	vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
 	if (hscale < 0 || vscale < 0) {
 		DRM_DEBUG_KMS("Invalid scaling of plane\n");
-		drm_rect_debug_print("src: ", src, true);
-		drm_rect_debug_print("dst: ", dest, false);
+		drm_rect_debug_print("src: ", &state->src, true);
+		drm_rect_debug_print("dst: ", &state->dst, false);
 		return -ERANGE;
 	}
 
-	*visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale);
+	state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
 
 	drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
 
-	if (!*visible)
+	if (!state->visible)
 		/*
 		 * Plane isn't visible; some drivers can handle this
 		 * so we just return success here.  Drivers that can't
@@ -191,15 +194,87 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
 		 */
 		return 0;
 
-	if (!can_position && !drm_rect_equals(dest, clip)) {
+	if (!can_position && !drm_rect_equals(dst, clip)) {
 		DRM_DEBUG_KMS("Plane must cover entire CRTC\n");
-		drm_rect_debug_print("dst: ", dest, false);
+		drm_rect_debug_print("dst: ", dst, false);
 		drm_rect_debug_print("clip: ", clip, false);
 		return -EINVAL;
 	}
 
 	return 0;
 }
+EXPORT_SYMBOL(drm_plane_helper_check_state);
+
+/**
+ * drm_plane_helper_check_update() - Check plane update for validity
+ * @plane: plane object to update
+ * @crtc: owning CRTC of owning plane
+ * @fb: framebuffer to flip onto plane
+ * @src: source coordinates in 16.16 fixed point
+ * @dest: integer destination coordinates
+ * @clip: integer clipping coordinates
+ * @rotation: plane rotation
+ * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
+ * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
+ * @can_position: is it legal to position the plane such that it
+ *                doesn't cover the entire crtc?  This will generally
+ *                only be false for primary planes.
+ * @can_update_disabled: can the plane be updated while the crtc
+ *                       is disabled?
+ * @visible: output parameter indicating whether plane is still visible after
+ *           clipping
+ *
+ * Checks that a desired plane update is valid.  Drivers that provide
+ * their own plane handling rather than helper-provided implementations may
+ * still wish to call this function to avoid duplication of error checking
+ * code.
+ *
+ * RETURNS:
+ * Zero if update appears valid, error code on failure
+ */
+int drm_plane_helper_check_update(struct drm_plane *plane,
+				  struct drm_crtc *crtc,
+				  struct drm_framebuffer *fb,
+				  struct drm_rect *src,
+				  struct drm_rect *dst,
+				  const struct drm_rect *clip,
+				  unsigned int rotation,
+				  int min_scale,
+				  int max_scale,
+				  bool can_position,
+				  bool can_update_disabled,
+				  bool *visible)
+{
+	struct drm_plane_state state = {
+		.plane = plane,
+		.crtc = crtc,
+		.fb = fb,
+		.src_x = src->x1,
+		.src_y = src->x2,
+		.src_w = drm_rect_width(src),
+		.src_h = drm_rect_height(src),
+		.crtc_x = dst->x1,
+		.crtc_y = dst->x2,
+		.crtc_w = drm_rect_width(dst),
+		.crtc_h = drm_rect_height(dst),
+		.rotation = rotation,
+		.visible = *visible,
+	};
+	int ret;
+
+	ret = drm_plane_helper_check_state(&state, clip,
+					   min_scale, max_scale,
+					   can_position,
+					   can_update_disabled);
+	if (ret)
+		return ret;
+
+	*src = state.src;
+	*dst = state.dst;
+	*visible = state.visible;
+
+	return 0;
+}
 EXPORT_SYMBOL(drm_plane_helper_check_update);
 
 /**
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index 0e0c3573cce0..fbc8ecb3e5e8 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -40,6 +40,11 @@
 int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		  const struct drm_crtc_funcs *funcs);
 
+int drm_plane_helper_check_state(struct drm_plane_state *state,
+				 const struct drm_rect *clip,
+				 int min_scale, int max_scale,
+				 bool can_position,
+				 bool can_update_disabled);
 int drm_plane_helper_check_update(struct drm_plane *plane,
 				  struct drm_crtc *crtc,
 				  struct drm_framebuffer *fb,
-- 
2.7.4

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

Re: [PATCH 6/9] drm/rockchip: Use drm_plane_state.{src,dst}

[Mark yao]

On 2016年07月27日 00:07, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Replace the private drm_rects in vop_plane_state with
> the ones now living in drm_plane_state.
>
> Cc: Yao <mark.yao@rock-chips.com>
> Cc: linux-rockchip@lists.infradead.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Good cleanup,

Acked-by: Mark Yao <mark.yao@rock-chips.com>

> ---
>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 91305eb7d312..c566c740ab49 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -87,8 +87,6 @@
>   struct vop_plane_state {
>   	struct drm_plane_state base;
>   	int format;
> -	struct drm_rect src;
> -	struct drm_rect dest;
>   	dma_addr_t yrgb_mst;
>   	bool enable;
>   };
> @@ -595,8 +593,8 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
>   	const struct vop_win_data *win = vop_win->data;
>   	bool visible;
>   	int ret;
> -	struct drm_rect *dest = &vop_plane_state->dest;
> -	struct drm_rect *src = &vop_plane_state->src;
> +	struct drm_rect *dest = &state->dst;
> +	struct drm_rect *src = &state->src;
>   	struct drm_rect clip;
>   	int min_scale = win->phy->scl ? FRAC_16_16(1, 8) :
>   					DRM_PLANE_HELPER_NO_SCALING;
> @@ -694,8 +692,8 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
>   	unsigned int actual_w, actual_h;
>   	unsigned int dsp_stx, dsp_sty;
>   	uint32_t act_info, dsp_info, dsp_st;
> -	struct drm_rect *src = &vop_plane_state->src;
> -	struct drm_rect *dest = &vop_plane_state->dest;
> +	struct drm_rect *src = &state->src;
> +	struct drm_rect *dest = &state->dst;
>   	struct drm_gem_object *obj, *uv_obj;
>   	struct rockchip_gem_object *rk_obj, *rk_uv_obj;
>   	unsigned long offset;


-- 
Ｍark Yao


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/9] drm/rockchip: Use drm_plane_helper_check_state()

[Mark yao]

On 2016年07月27日 00:07, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä<ville.syrjala@linux.intel.com>
>
> Replace the use of drm_plane_helper_check_update() with
> drm_plane_helper_check_state() since we have a plane state.
>
> Rockchip looks to handling plane clipping rather well already
> (unlikje most arm drm drivers) so there are no function changes
> here.
>
> Cc: Yao<mark.yao@rock-chips.com>
> Cc:linux-rockchip@lists.infradead.org
> Signed-off-by: Ville Syrjälä<ville.syrjala@linux.intel.com>
Acked-by: Mark Yao <mark.yao@rock-chips.com>

Thanks.

-- 
Ｍark Yao


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Ro.CI.BAT: failure for drm: Store clipped coordinates in drm_plane_state (rev2)

[Patchwork]

== Series Details ==

Series: drm: Store clipped coordinates in drm_plane_state (rev2)
URL   : https://patchwork.freedesktop.org/series/10279/
State : failure

== Summary ==

Series 10279v2 drm: Store clipped coordinates in drm_plane_state
http://patchwork.freedesktop.org/api/1.0/series/10279/revisions/2/mbox

Test gem_exec_suspend:
        Subgroup basic-s3:
                incomplete -> PASS       (fi-hsw-i7-4770k)
Test kms_cursor_legacy:
        Subgroup basic-cursor-vs-flip-legacy:
                fail       -> PASS       (ro-ilk1-i5-650)
        Subgroup basic-flip-vs-cursor-legacy:
                fail       -> PASS       (ro-hsw-i7-4770r)
        Subgroup basic-flip-vs-cursor-varying-size:
                pass       -> FAIL       (ro-snb-i7-2620M)

fi-hsw-i7-4770k  total:239  pass:216  dwarn:0   dfail:0   fail:1   skip:22 
fi-kbl-qkkr      total:239  pass:181  dwarn:29  dfail:0   fail:2   skip:27 
fi-skl-i5-6260u  total:239  pass:225  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-i7-2600   total:239  pass:197  dwarn:0   dfail:0   fail:0   skip:42 
ro-bdw-i5-5250u  total:239  pass:218  dwarn:4   dfail:0   fail:1   skip:16 
ro-bdw-i7-5600u  total:239  pass:206  dwarn:0   dfail:0   fail:1   skip:32 
ro-bsw-n3050     total:239  pass:193  dwarn:0   dfail:0   fail:4   skip:42 
ro-byt-n2820     total:239  pass:195  dwarn:0   dfail:0   fail:4   skip:40 
ro-hsw-i7-4770r  total:239  pass:213  dwarn:0   dfail:0   fail:0   skip:26 
ro-ilk-i7-620lm  total:239  pass:172  dwarn:1   dfail:0   fail:1   skip:65 
ro-ilk1-i5-650   total:234  pass:172  dwarn:0   dfail:0   fail:2   skip:60 
ro-ivb-i7-3770   total:239  pass:204  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:239  pass:208  dwarn:0   dfail:0   fail:0   skip:31 
ro-snb-i7-2620M  total:239  pass:196  dwarn:0   dfail:0   fail:2   skip:41 
fi-skl-i7-6700k failed to connect after reboot
ro-bdw-i7-5557U failed to connect after reboot
ro-hsw-i3-4010u failed to connect after reboot
ro-skl3-i5-6260u failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1615/

a53dfb0 drm-intel-nightly: 2016y-07m-26d-17h-51m-26s UTC integration manifest
143977c drm/simple_kms_helper: Use drm_plane_helper_check_state()
4f35d9b drm/mediatek: Use drm_plane_helper_check_state()
0a83f5b drm/rockchip: Use drm_plane_helper_check_state()
8d85168 drm/rockchip: Use drm_plane_state.{src, dst}
ae1f07e drm/i915: Use drm_plane_helper_check_state()
70e028d drm/i915: Use drm_plane_state.{src, dst, visible}
dcb3c1a drm/plane-helper: Add drm_plane_helper_check_state()
ae06eca drm: Store clipped src/dst coordinatee in drm_plane_state
6ab56c0 drm: Warn about negative sizes when calculating scale factor

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

Re: [Intel-gfx] [PATCH 3/9] drm/plane-helper: Add drm_plane_helper_check_state()

[Daniel Vetter]

On Tue, Jul 26, 2016 at 07:42:16PM +0300, Ville Syrjälä wrote:
> On Tue, Jul 26, 2016 at 05:33:29PM +0100, Chris Wilson wrote:
> > On Tue, Jul 26, 2016 at 07:06:58PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Add a version of drm_plane_helper_check_update() which takes a plane
> > > state instead of having the caller pass in everything.
> > > 
> > > And to reduce code duplication, let's reimplement
> > > drm_plane_helper_check_update() in terms of the new function, by
> > > having a tempororary plane state on the stack.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_plane_helper.c | 136 ++++++++++++++++++++++++++++---------
> > >  include/drm/drm_plane_helper.h     |   5 ++
> > >  2 files changed, 110 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> > > index 16c4a7bd7465..1124eb827671 100644
> > > --- a/drivers/gpu/drm/drm_plane_helper.c
> > > +++ b/drivers/gpu/drm/drm_plane_helper.c
> > > @@ -108,14 +108,9 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
> > >  }
> > >  
> > >  /**
> > > - * drm_plane_helper_check_update() - Check plane update for validity
> > > - * @plane: plane object to update
> > > - * @crtc: owning CRTC of owning plane
> > > - * @fb: framebuffer to flip onto plane
> > > - * @src: source coordinates in 16.16 fixed point
> > > - * @dest: integer destination coordinates
> > > + * drm_plane_helper_check_state() - Check plane state for validity
> > > + * @state: plane state to check
> > >   * @clip: integer clipping coordinates
> > > - * @rotation: plane rotation
> > >   * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
> > >   * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
> > >   * @can_position: is it legal to position the plane such that it
> > > @@ -123,8 +118,6 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
> > >   *                only be false for primary planes.
> > >   * @can_update_disabled: can the plane be updated while the crtc
> > >   *                       is disabled?
> > > - * @visible: output parameter indicating whether plane is still visible after
> > > - *           clipping
> > >   *
> > >   * Checks that a desired plane update is valid.  Drivers that provide
> > >   * their own plane handling rather than helper-provided implementations may
> > > @@ -134,29 +127,38 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
> > >   * RETURNS:
> > >   * Zero if update appears valid, error code on failure
> > >   */
> > > -int drm_plane_helper_check_update(struct drm_plane *plane,
> > > -				  struct drm_crtc *crtc,
> > > -				  struct drm_framebuffer *fb,
> > > -				  struct drm_rect *src,
> > > -				  struct drm_rect *dest,
> > > -				  const struct drm_rect *clip,
> > > -				  unsigned int rotation,
> > > -				  int min_scale,
> > > -				  int max_scale,
> > > -				  bool can_position,
> > > -				  bool can_update_disabled,
> > > -				  bool *visible)
> > > +int drm_plane_helper_check_state(struct drm_plane_state *state,
> > > +				 const struct drm_rect *clip,
> > > +				 int min_scale,
> > > +				 int max_scale,
> > > +				 bool can_position,
> > > +				 bool can_update_disabled)
> > >  {
> > > +	struct drm_crtc *crtc = state->crtc;
> > > +	struct drm_framebuffer *fb = state->fb;
> > > +	struct drm_rect *src = &state->src;
> > > +	struct drm_rect *dst = &state->dst;
> > > +	unsigned int rotation = state->rotation;
> > >  	int hscale, vscale;
> > >  
> > > +	src->x1 = state->src_x;
> > > +	src->y1 = state->src_y;
> > > +	src->x2 = state->src_x + state->src_w;
> > > +	src->y2 = state->src_y + state->src_h;
> > > +
> > > +	dst->x1 = state->crtc_x;
> > > +	dst->y1 = state->crtc_y;
> > > +	dst->x2 = state->crtc_x + state->crtc_w;
> > > +	dst->y2 = state->crtc_y + state->crtc_h;
> > 
> > It's a little surprising to me that the check writes fields into the
> > drm_plane_state. Care to at least mention the side-effects in the kernel
> > doc?
> 
> That's a lot of what the atomic "check" functions do. A bit of a bad
> naming convention that. Not sure who came up with it. Should probably
> rename them all to "compute" or something.
> 
> But I can at leat expand the docs here a bit in the meantime.

While you expand docs, pls link the atomic to the legacy version with a
cross reference in each. Also I wonder whether it would make sense to put
this into the atomic namespace and into drm_atomic_helper.c and call it
something like drm_atomic_helper_check_plane_state. For greater visibility
and chances of it being used.

Finally I think it'd be good to put a reference to this into the
plane_helper_funcs->atomic_check kerneldoc.
-Daniel
-- 
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

Re: [Intel-gfx] [PATCH 1/9] drm: Warn about negative sizes when calculating scale factor

[Sean Paul]

On Tue, Jul 26, 2016 at 12:39 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Tue, Jul 26, 2016 at 05:24:42PM +0100, Chris Wilson wrote:
>> On Tue, Jul 26, 2016 at 07:06:56PM +0300, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > Passing negative width/hight to scale factor calculations is not

nit: s/hight/height/

>> > legal. Let's WARN if that happens.
>>
>> Does this get called with user controllable inputs?
>
> User controllable to a degree. width/height can only ever be positive
> though.
>

I think the only risk is getting UINT_MAX from userspace, since
drm_rect stores ints. However, it looks like check_src_coords() and
the check in __setplane_internal() should ensure those values are
pruned out.

Reviewed-by: Sean Paul <seanpaul@chromium.org>

>> A quick grep leads
>> me to drm_primary_helper_update() which suggests no. Did I miss a
>> potential user controllable WARN->panic?
>
> I just landed in the BUG_ON in intel_sprite.c on account of a typo I
> made in the user src/crtc coordinate -> drm_rect conversion. Should
> probably replace the BUG_ON() with WARN_ON() in i915 as well...
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/9] drm: Store clipped src/dst coordinatee in drm_plane_state

[Sean Paul]

On Tue, Jul 26, 2016 at 12:06 PM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Pretty much all driver will have need for the clipped plane
> coordinates, so let's stuff then into drm_plane_state.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  include/drm/drm_crtc.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3edeaf88ebc0..00a1868940b8 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -35,6 +35,7 @@
>  #include <uapi/drm/drm_mode.h>
>  #include <uapi/drm/drm_fourcc.h>
>  #include <drm/drm_modeset_lock.h>
> +#include <drm/drm_rect.h>
>
>  struct drm_device;
>  struct drm_mode_set;
> @@ -1409,6 +1410,9 @@ struct drm_connector {
>   * @src_w: width of visible portion of plane (in 16.16)
>   * @src_h: height of visible portion of plane (in 16.16)
>   * @rotation: rotation of the plane
> + * @src: clipped source coordinates of the plane (in 16.16)
> + * @dst: clipped destination coordinates of the plane
> + * @visible: visibility of the plane
>   * @state: backpointer to global drm_atomic_state
>   */
>  struct drm_plane_state {
> @@ -1429,6 +1433,15 @@ struct drm_plane_state {
>         /* Plane rotation */
>         unsigned int rotation;
>
> +       /* Clipped coordinates */
> +       struct drm_rect src, dst;
> +
> +       /*
> +        * Is the plane actually visible? Can be false even
> +        * if fb!=NULL and crtc!=NULL, due to clipping.
> +        */
> +       bool visible;
> +
>         struct drm_atomic_state *state;
>  };
>
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 3/9] drm/plane-helper: Add drm_plane_helper_check_state()

[Sean Paul]

On Tue, Jul 26, 2016 at 1:34 PM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Add a version of drm_plane_helper_check_update() which takes a plane
> state instead of having the caller pass in everything.
>
> And to reduce code duplication, let's reimplement
> drm_plane_helper_check_update() in terms of the new function, by
> having a tempororary plane state on the stack.
>
> v2: Add a note that the functions modifies the state (Chris)
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/drm_plane_helper.c | 139 ++++++++++++++++++++++++++++---------
>  include/drm/drm_plane_helper.h     |   5 ++
>  2 files changed, 112 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index 16c4a7bd7465..ca02997b1c36 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -108,14 +108,9 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
>  }
>
>  /**
> - * drm_plane_helper_check_update() - Check plane update for validity
> - * @plane: plane object to update
> - * @crtc: owning CRTC of owning plane
> - * @fb: framebuffer to flip onto plane
> - * @src: source coordinates in 16.16 fixed point
> - * @dest: integer destination coordinates
> + * drm_plane_helper_check_state() - Check plane state for validity
> + * @state: plane state to check
>   * @clip: integer clipping coordinates
> - * @rotation: plane rotation
>   * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
>   * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
>   * @can_position: is it legal to position the plane such that it
> @@ -123,10 +118,9 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
>   *                only be false for primary planes.
>   * @can_update_disabled: can the plane be updated while the crtc
>   *                       is disabled?
> - * @visible: output parameter indicating whether plane is still visible after
> - *           clipping
>   *
> - * Checks that a desired plane update is valid.  Drivers that provide
> + * Checks that a desired plane update is valid, and updates various
> + * bits of derived state (clipped coordinates etc.). Drivers that provide
>   * their own plane handling rather than helper-provided implementations may
>   * still wish to call this function to avoid duplication of error checking
>   * code.
> @@ -134,29 +128,38 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
>   * RETURNS:
>   * Zero if update appears valid, error code on failure
>   */
> -int drm_plane_helper_check_update(struct drm_plane *plane,
> -                                 struct drm_crtc *crtc,
> -                                 struct drm_framebuffer *fb,
> -                                 struct drm_rect *src,
> -                                 struct drm_rect *dest,
> -                                 const struct drm_rect *clip,
> -                                 unsigned int rotation,
> -                                 int min_scale,
> -                                 int max_scale,
> -                                 bool can_position,
> -                                 bool can_update_disabled,
> -                                 bool *visible)
> +int drm_plane_helper_check_state(struct drm_plane_state *state,
> +                                const struct drm_rect *clip,
> +                                int min_scale,
> +                                int max_scale,
> +                                bool can_position,
> +                                bool can_update_disabled)
>  {
> +       struct drm_crtc *crtc = state->crtc;
> +       struct drm_framebuffer *fb = state->fb;
> +       struct drm_rect *src = &state->src;
> +       struct drm_rect *dst = &state->dst;
> +       unsigned int rotation = state->rotation;
>         int hscale, vscale;
>
> +       src->x1 = state->src_x;
> +       src->y1 = state->src_y;
> +       src->x2 = state->src_x + state->src_w;
> +       src->y2 = state->src_y + state->src_h;
> +
> +       dst->x1 = state->crtc_x;
> +       dst->y1 = state->crtc_y;
> +       dst->x2 = state->crtc_x + state->crtc_w;
> +       dst->y2 = state->crtc_y + state->crtc_h;
> +
>         if (!fb) {
> -               *visible = false;
> +               state->visible = false;
>                 return 0;
>         }
>
>         /* crtc should only be NULL when disabling (i.e., !fb) */
>         if (WARN_ON(!crtc)) {
> -               *visible = false;
> +               state->visible = false;
>                 return 0;
>         }
>
> @@ -168,20 +171,20 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
>         drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
>
>         /* Check scaling */
> -       hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> -       vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> +       hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
> +       vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
>         if (hscale < 0 || vscale < 0) {
>                 DRM_DEBUG_KMS("Invalid scaling of plane\n");
> -               drm_rect_debug_print("src: ", src, true);
> -               drm_rect_debug_print("dst: ", dest, false);
> +               drm_rect_debug_print("src: ", &state->src, true);
> +               drm_rect_debug_print("dst: ", &state->dst, false);
>                 return -ERANGE;
>         }
>
> -       *visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale);
> +       state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
>
>         drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
>
> -       if (!*visible)
> +       if (!state->visible)
>                 /*
>                  * Plane isn't visible; some drivers can handle this
>                  * so we just return success here.  Drivers that can't
> @@ -191,15 +194,87 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
>                  */
>                 return 0;
>
> -       if (!can_position && !drm_rect_equals(dest, clip)) {
> +       if (!can_position && !drm_rect_equals(dst, clip)) {
>                 DRM_DEBUG_KMS("Plane must cover entire CRTC\n");
> -               drm_rect_debug_print("dst: ", dest, false);
> +               drm_rect_debug_print("dst: ", dst, false);
>                 drm_rect_debug_print("clip: ", clip, false);
>                 return -EINVAL;
>         }
>
>         return 0;
>  }
> +EXPORT_SYMBOL(drm_plane_helper_check_state);
> +
> +/**
> + * drm_plane_helper_check_update() - Check plane update for validity
> + * @plane: plane object to update
> + * @crtc: owning CRTC of owning plane
> + * @fb: framebuffer to flip onto plane
> + * @src: source coordinates in 16.16 fixed point
> + * @dest: integer destination coordinates
> + * @clip: integer clipping coordinates
> + * @rotation: plane rotation
> + * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
> + * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
> + * @can_position: is it legal to position the plane such that it
> + *                doesn't cover the entire crtc?  This will generally
> + *                only be false for primary planes.
> + * @can_update_disabled: can the plane be updated while the crtc
> + *                       is disabled?
> + * @visible: output parameter indicating whether plane is still visible after
> + *           clipping
> + *
> + * Checks that a desired plane update is valid.  Drivers that provide
> + * their own plane handling rather than helper-provided implementations may
> + * still wish to call this function to avoid duplication of error checking
> + * code.
> + *
> + * RETURNS:
> + * Zero if update appears valid, error code on failure
> + */
> +int drm_plane_helper_check_update(struct drm_plane *plane,
> +                                 struct drm_crtc *crtc,
> +                                 struct drm_framebuffer *fb,
> +                                 struct drm_rect *src,
> +                                 struct drm_rect *dst,
> +                                 const struct drm_rect *clip,
> +                                 unsigned int rotation,
> +                                 int min_scale,
> +                                 int max_scale,
> +                                 bool can_position,
> +                                 bool can_update_disabled,
> +                                 bool *visible)
> +{
> +       struct drm_plane_state state = {
> +               .plane = plane,
> +               .crtc = crtc,
> +               .fb = fb,
> +               .src_x = src->x1,
> +               .src_y = src->x2,
> +               .src_w = drm_rect_width(src),
> +               .src_h = drm_rect_height(src),
> +               .crtc_x = dst->x1,
> +               .crtc_y = dst->x2,
> +               .crtc_w = drm_rect_width(dst),
> +               .crtc_h = drm_rect_height(dst),
> +               .rotation = rotation,
> +               .visible = *visible,
> +       };
> +       int ret;
> +
> +       ret = drm_plane_helper_check_state(&state, clip,
> +                                          min_scale, max_scale,
> +                                          can_position,
> +                                          can_update_disabled);
> +       if (ret)
> +               return ret;
> +
> +       *src = state.src;
> +       *dst = state.dst;
> +       *visible = state.visible;
> +
> +       return 0;
> +}
>  EXPORT_SYMBOL(drm_plane_helper_check_update);
>
>  /**
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index 0e0c3573cce0..fbc8ecb3e5e8 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -40,6 +40,11 @@
>  int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>                   const struct drm_crtc_funcs *funcs);
>
> +int drm_plane_helper_check_state(struct drm_plane_state *state,
> +                                const struct drm_rect *clip,
> +                                int min_scale, int max_scale,
> +                                bool can_position,
> +                                bool can_update_disabled);
>  int drm_plane_helper_check_update(struct drm_plane *plane,
>                                   struct drm_crtc *crtc,
>                                   struct drm_framebuffer *fb,
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/9] drm/i915: Use drm_plane_state.{src, dst, visible}

[Sean Paul]

On Tue, Jul 26, 2016 at 12:06 PM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Replace the private drm_rects/flags in intel_plane_state
> with the ones now living in drm_plane_state.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c |  20 ++---
>  drivers/gpu/drm/i915/intel_display.c      | 132 +++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_drv.h          |   3 -
>  drivers/gpu/drm/i915/intel_fbc.c          |  12 +--
>  drivers/gpu/drm/i915/intel_pm.c           |  60 +++++++-------
>  drivers/gpu/drm/i915/intel_sprite.c       |  84 +++++++++----------
>  6 files changed, 156 insertions(+), 155 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 7de7721f65bc..14d40261db21 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -139,14 +139,14 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>          * we want to keep another copy internal to our driver that we can
>          * clip/modify ourselves.
>          */
> -       intel_state->src.x1 = state->src_x;
> -       intel_state->src.y1 = state->src_y;
> -       intel_state->src.x2 = state->src_x + state->src_w;
> -       intel_state->src.y2 = state->src_y + state->src_h;
> -       intel_state->dst.x1 = state->crtc_x;
> -       intel_state->dst.y1 = state->crtc_y;
> -       intel_state->dst.x2 = state->crtc_x + state->crtc_w;
> -       intel_state->dst.y2 = state->crtc_y + state->crtc_h;
> +       intel_state->base.src.x1 = state->src_x;
> +       intel_state->base.src.y1 = state->src_y;
> +       intel_state->base.src.x2 = state->src_x + state->src_w;
> +       intel_state->base.src.y2 = state->src_y + state->src_h;
> +       intel_state->base.dst.x1 = state->crtc_x;
> +       intel_state->base.dst.y1 = state->crtc_y;
> +       intel_state->base.dst.x2 = state->crtc_x + state->crtc_w;
> +       intel_state->base.dst.y2 = state->crtc_y + state->crtc_h;
>
>         /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
>         intel_state->clip.x1 = 0;
> @@ -180,7 +180,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>                 }
>         }
>
> -       intel_state->visible = false;
> +       intel_state->base.visible = false;
>         ret = intel_plane->check_plane(plane, crtc_state, intel_state);
>         if (ret)
>                 return ret;
> @@ -196,7 +196,7 @@ static void intel_plane_atomic_update(struct drm_plane *plane,
>                 to_intel_plane_state(plane->state);
>         struct drm_crtc *crtc = plane->state->crtc ?: old_state->crtc;
>
> -       if (intel_state->visible)
> +       if (intel_state->base.visible)
>                 intel_plane->update_plane(plane,
>                                           to_intel_crtc_state(crtc->state),
>                                           intel_state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 78beb7e9d384..4f67b7c19b75 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2565,7 +2565,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>          * simplest solution is to just disable the primary plane now and
>          * pretend the BIOS never had it enabled.
>          */
> -       to_intel_plane_state(plane_state)->visible = false;
> +       to_intel_plane_state(plane_state)->base.visible = false;
>         crtc_state->plane_mask &= ~(1 << drm_plane_index(primary));
>         intel_pre_disable_primary_noatomic(&intel_crtc->base);
>         intel_plane->disable_plane(primary, &intel_crtc->base);
> @@ -2583,14 +2583,14 @@ valid_fb:
>         plane_state->crtc_w = fb->width;
>         plane_state->crtc_h = fb->height;
>
> -       intel_state->src.x1 = plane_state->src_x;
> -       intel_state->src.y1 = plane_state->src_y;
> -       intel_state->src.x2 = plane_state->src_x + plane_state->src_w;
> -       intel_state->src.y2 = plane_state->src_y + plane_state->src_h;
> -       intel_state->dst.x1 = plane_state->crtc_x;
> -       intel_state->dst.y1 = plane_state->crtc_y;
> -       intel_state->dst.x2 = plane_state->crtc_x + plane_state->crtc_w;
> -       intel_state->dst.y2 = plane_state->crtc_y + plane_state->crtc_h;
> +       intel_state->base.src.x1 = plane_state->src_x;
> +       intel_state->base.src.y1 = plane_state->src_y;
> +       intel_state->base.src.x2 = plane_state->src_x + plane_state->src_w;
> +       intel_state->base.src.y2 = plane_state->src_y + plane_state->src_h;
> +       intel_state->base.dst.x1 = plane_state->crtc_x;
> +       intel_state->base.dst.y1 = plane_state->crtc_y;
> +       intel_state->base.dst.x2 = plane_state->crtc_x + plane_state->crtc_w;
> +       intel_state->base.dst.y2 = plane_state->crtc_y + plane_state->crtc_h;
>
>         obj = intel_fb_obj(fb);
>         if (obj->tiling_mode != I915_TILING_NONE)
> @@ -2618,8 +2618,8 @@ static void i9xx_update_primary_plane(struct drm_plane *primary,
>         i915_reg_t reg = DSPCNTR(plane);
>         unsigned int rotation = plane_state->base.rotation;
>         int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> -       int x = plane_state->src.x1 >> 16;
> -       int y = plane_state->src.y1 >> 16;
> +       int x = plane_state->base.src.x1 >> 16;
> +       int y = plane_state->base.src.y1 >> 16;
>
>         dspcntr = DISPPLANE_GAMMA_ENABLE;
>
> @@ -2748,8 +2748,8 @@ static void ironlake_update_primary_plane(struct drm_plane *primary,
>         i915_reg_t reg = DSPCNTR(plane);
>         unsigned int rotation = plane_state->base.rotation;
>         int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> -       int x = plane_state->src.x1 >> 16;
> -       int y = plane_state->src.y1 >> 16;
> +       int x = plane_state->base.src.x1 >> 16;
> +       int y = plane_state->base.src.y1 >> 16;
>
>         dspcntr = DISPPLANE_GAMMA_ENABLE;
>         dspcntr |= DISPLAY_PLANE_ENABLE;
> @@ -2987,14 +2987,14 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>         int x_offset, y_offset;
>         u32 surf_addr;
>         int scaler_id = plane_state->scaler_id;
> -       int src_x = plane_state->src.x1 >> 16;
> -       int src_y = plane_state->src.y1 >> 16;
> -       int src_w = drm_rect_width(&plane_state->src) >> 16;
> -       int src_h = drm_rect_height(&plane_state->src) >> 16;
> -       int dst_x = plane_state->dst.x1;
> -       int dst_y = plane_state->dst.y1;
> -       int dst_w = drm_rect_width(&plane_state->dst);
> -       int dst_h = drm_rect_height(&plane_state->dst);
> +       int src_x = plane_state->base.src.x1 >> 16;
> +       int src_y = plane_state->base.src.y1 >> 16;
> +       int src_w = drm_rect_width(&plane_state->base.src) >> 16;
> +       int src_h = drm_rect_height(&plane_state->base.src) >> 16;
> +       int dst_x = plane_state->base.dst.x1;
> +       int dst_y = plane_state->base.dst.y1;
> +       int dst_w = drm_rect_width(&plane_state->base.dst);
> +       int dst_h = drm_rect_height(&plane_state->base.dst);
>
>         plane_ctl = PLANE_CTL_ENABLE |
>                     PLANE_CTL_PIPE_GAMMA_ENABLE |
> @@ -3009,7 +3009,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>                                                fb->pixel_format);
>         surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj, 0);
>
> -       WARN_ON(drm_rect_width(&plane_state->src) == 0);
> +       WARN_ON(drm_rect_width(&plane_state->base.src) == 0);
>
>         if (intel_rotation_90_or_270(rotation)) {
>                 int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> @@ -3098,7 +3098,7 @@ static void intel_update_primary_planes(struct drm_device *dev)
>                 drm_modeset_lock_crtc(crtc, &plane->base);
>                 plane_state = to_intel_plane_state(plane->base.state);
>
> -               if (plane_state->visible)
> +               if (plane_state->base.visible)
>                         plane->update_plane(&plane->base,
>                                             to_intel_crtc_state(crtc->state),
>                                             plane_state);
> @@ -4273,7 +4273,7 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
>         struct drm_framebuffer *fb = plane_state->base.fb;
>         int ret;
>
> -       bool force_detach = !fb || !plane_state->visible;
> +       bool force_detach = !fb || !plane_state->base.visible;
>
>         DRM_DEBUG_KMS("Updating scaler for [PLANE:%d:%s] scaler_user index %u.%u\n",
>                       intel_plane->base.base.id, intel_plane->base.name,
> @@ -4283,10 +4283,10 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
>                                 drm_plane_index(&intel_plane->base),
>                                 &plane_state->scaler_id,
>                                 plane_state->base.rotation,
> -                               drm_rect_width(&plane_state->src) >> 16,
> -                               drm_rect_height(&plane_state->src) >> 16,
> -                               drm_rect_width(&plane_state->dst),
> -                               drm_rect_height(&plane_state->dst));
> +                               drm_rect_width(&plane_state->base.src) >> 16,
> +                               drm_rect_height(&plane_state->base.src) >> 16,
> +                               drm_rect_width(&plane_state->base.dst),
> +                               drm_rect_height(&plane_state->base.dst));
>
>         if (ret || plane_state->scaler_id < 0)
>                 return ret;
> @@ -4584,9 +4584,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>
>                 intel_fbc_post_update(crtc);
>
> -               if (primary_state->visible &&
> +               if (primary_state->base.visible &&
>                     (needs_modeset(&pipe_config->base) ||
> -                    !old_primary_state->visible))
> +                    !old_primary_state->base.visible))
>                         intel_post_enable_primary(&crtc->base);
>         }
>  }
> @@ -4612,8 +4612,8 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
>
>                 intel_fbc_pre_update(crtc, pipe_config, primary_state);
>
> -               if (old_primary_state->visible &&
> -                   (modeset || !primary_state->visible))
> +               if (old_primary_state->base.visible &&
> +                   (modeset || !primary_state->base.visible))
>                         intel_pre_disable_primary(&crtc->base);
>         }
>
> @@ -6298,13 +6298,13 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
>         if (!intel_crtc->active)
>                 return;
>
> -       if (to_intel_plane_state(crtc->primary->state)->visible) {
> +       if (to_intel_plane_state(crtc->primary->state)->base.visible) {
>                 WARN_ON(intel_crtc->flip_work);
>
>                 intel_pre_disable_primary_noatomic(crtc);
>
>                 intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc->primary));
> -               to_intel_plane_state(crtc->primary->state)->visible = false;
> +               to_intel_plane_state(crtc->primary->state)->base.visible = false;
>         }
>
>         dev_priv->display.crtc_disable(crtc);
> @@ -10178,7 +10178,7 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base,
>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>         uint32_t cntl = 0, size = 0;
>
> -       if (plane_state && plane_state->visible) {
> +       if (plane_state && plane_state->base.visible) {
>                 unsigned int width = plane_state->base.crtc_w;
>                 unsigned int height = plane_state->base.crtc_h;
>                 unsigned int stride = roundup_pow_of_two(width) * 4;
> @@ -10242,7 +10242,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
>         int pipe = intel_crtc->pipe;
>         uint32_t cntl = 0;
>
> -       if (plane_state && plane_state->visible) {
> +       if (plane_state && plane_state->base.visible) {
>                 cntl = MCURSOR_GAMMA_ENABLE;
>                 switch (plane_state->base.crtc_w) {
>                         case 64:
> @@ -11824,7 +11824,7 @@ static bool intel_wm_need_update(struct drm_plane *plane,
>         struct intel_plane_state *cur = to_intel_plane_state(plane->state);
>
>         /* Update watermarks on tiling or size changes. */
> -       if (new->visible != cur->visible)
> +       if (new->base.visible != cur->base.visible)
>                 return true;
>
>         if (!cur->base.fb || !new->base.fb)
> @@ -11832,10 +11832,10 @@ static bool intel_wm_need_update(struct drm_plane *plane,
>
>         if (cur->base.fb->modifier[0] != new->base.fb->modifier[0] ||
>             cur->base.rotation != new->base.rotation ||
> -           drm_rect_width(&new->src) != drm_rect_width(&cur->src) ||
> -           drm_rect_height(&new->src) != drm_rect_height(&cur->src) ||
> -           drm_rect_width(&new->dst) != drm_rect_width(&cur->dst) ||
> -           drm_rect_height(&new->dst) != drm_rect_height(&cur->dst))
> +           drm_rect_width(&new->base.src) != drm_rect_width(&cur->base.src) ||
> +           drm_rect_height(&new->base.src) != drm_rect_height(&cur->base.src) ||
> +           drm_rect_width(&new->base.dst) != drm_rect_width(&cur->base.dst) ||
> +           drm_rect_height(&new->base.dst) != drm_rect_height(&cur->base.dst))
>                 return true;
>
>         return false;
> @@ -11843,10 +11843,10 @@ static bool intel_wm_need_update(struct drm_plane *plane,
>
>  static bool needs_scaling(struct intel_plane_state *state)
>  {
> -       int src_w = drm_rect_width(&state->src) >> 16;
> -       int src_h = drm_rect_height(&state->src) >> 16;
> -       int dst_w = drm_rect_width(&state->dst);
> -       int dst_h = drm_rect_height(&state->dst);
> +       int src_w = drm_rect_width(&state->base.src) >> 16;
> +       int src_h = drm_rect_height(&state->base.src) >> 16;
> +       int dst_w = drm_rect_width(&state->base.dst);
> +       int dst_h = drm_rect_height(&state->base.dst);
>
>         return (src_w != dst_w || src_h != dst_h);
>  }
> @@ -11877,8 +11877,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>                         return ret;
>         }
>
> -       was_visible = old_plane_state->visible;
> -       visible = to_intel_plane_state(plane_state)->visible;
> +       was_visible = old_plane_state->base.visible;
> +       visible = to_intel_plane_state(plane_state)->base.visible;
>
>         if (!was_crtc_enabled && WARN_ON(was_visible))
>                 was_visible = false;
> @@ -11894,7 +11894,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>          * only combine the results from all planes in the current place?
>          */
>         if (!is_crtc_enabled)
> -               to_intel_plane_state(plane_state)->visible = visible = false;
> +               to_intel_plane_state(plane_state)->base.visible = visible = false;
>
>         if (!was_visible && !visible)
>                 return 0;
> @@ -12299,12 +12299,13 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>                               drm_get_format_name(fb->pixel_format));
>                 DRM_DEBUG_KMS("\tscaler:%d src %dx%d+%d+%d dst %dx%d+%d+%d\n",
>                               state->scaler_id,
> -                             state->src.x1 >> 16, state->src.y1 >> 16,
> -                             drm_rect_width(&state->src) >> 16,
> -                             drm_rect_height(&state->src) >> 16,
> -                             state->dst.x1, state->dst.y1,
> -                             drm_rect_width(&state->dst),
> -                             drm_rect_height(&state->dst));
> +                             state->base.src.x1 >> 16,
> +                             state->base.src.y1 >> 16,
> +                             drm_rect_width(&state->base.src) >> 16,
> +                             drm_rect_height(&state->base.src) >> 16,
> +                             state->base.dst.x1, state->base.dst.y1,
> +                             drm_rect_width(&state->base.dst),
> +                             drm_rect_height(&state->base.dst));
>         }
>  }
>
> @@ -14139,12 +14140,14 @@ intel_check_primary_plane(struct drm_plane *plane,
>                 can_position = true;
>         }
>
> -       return drm_plane_helper_check_update(plane, crtc, fb, &state->src,
> -                                            &state->dst, &state->clip,
> +       return drm_plane_helper_check_update(plane, crtc, fb,
> +                                            &state->base.src,
> +                                            &state->base.dst,
> +                                            &state->base.clip,
>                                              state->base.rotation,
>                                              min_scale, max_scale,
>                                              can_position, true,
> -                                            &state->visible);
> +                                            &state->base.visible);
>  }
>
>  static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> @@ -14331,12 +14334,13 @@ intel_check_cursor_plane(struct drm_plane *plane,
>         unsigned stride;
>         int ret;
>
> -       ret = drm_plane_helper_check_update(plane, crtc, fb, &state->src,
> -                                           &state->dst, &state->clip,
> +       ret = drm_plane_helper_check_update(plane, crtc, fb, &state->base.src,
> +                                           &state->base.dst,
> +                                           &state->base.clip,
>                                             state->base.rotation,
>                                             DRM_PLANE_HELPER_NO_SCALING,
>                                             DRM_PLANE_HELPER_NO_SCALING,
> -                                           true, true, &state->visible);
> +                                           true, true, &state->base.visible);
>         if (ret)
>                 return ret;
>
> @@ -14373,7 +14377,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>          * Refuse the put the cursor into that compromised position.
>          */
>         if (IS_CHERRYVIEW(plane->dev) && pipe == PIPE_C &&
> -           state->visible && state->base.crtc_x < 0) {
> +           state->base.visible && state->base.crtc_x < 0) {
>                 DRM_DEBUG_KMS("CHV cursor C not allowed to straddle the left screen edge\n");
>                 return -EINVAL;
>         }
> @@ -15822,7 +15826,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>                  * Temporarily change the plane mapping and disable everything
>                  * ...  */
>                 plane = crtc->plane;
> -               to_intel_plane_state(crtc->base.primary->state)->visible = true;
> +               to_intel_plane_state(crtc->base.primary->state)->base.visible = true;
>                 crtc->plane = !plane;
>                 intel_crtc_disable_noatomic(&crtc->base);
>                 crtc->plane = plane;
> @@ -15949,10 +15953,10 @@ static void readout_plane_state(struct intel_crtc *crtc)
>         struct intel_plane_state *plane_state =
>                 to_intel_plane_state(primary->state);
>
> -       plane_state->visible = crtc->active &&
> +       plane_state->base.visible = crtc->active &&
>                 primary_get_hw_state(to_intel_plane(primary));
>
> -       if (plane_state->visible)
> +       if (plane_state->base.visible)
>                 crtc->base.state->plane_mask |= 1 << drm_plane_index(primary);
>  }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e74d851868c5..2f533aa0c3f3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -338,10 +338,7 @@ struct intel_atomic_state {
>
>  struct intel_plane_state {
>         struct drm_plane_state base;
> -       struct drm_rect src;
> -       struct drm_rect dst;
>         struct drm_rect clip;
> -       bool visible;
>
>         /*
>          * scaler_id
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 781e2f5f7cd8..23d494b18459 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -494,7 +494,7 @@ static bool multiple_pipes_ok(struct intel_crtc *crtc,
>         if (!no_fbc_on_multiple_pipes(dev_priv))
>                 return true;
>
> -       if (plane_state->visible)
> +       if (plane_state->base.visible)
>                 fbc->visible_pipes_mask |= (1 << pipe);
>         else
>                 fbc->visible_pipes_mask &= ~(1 << pipe);
> @@ -725,9 +725,9 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
>                         ilk_pipe_pixel_rate(crtc_state);
>
>         cache->plane.rotation = plane_state->base.rotation;
> -       cache->plane.src_w = drm_rect_width(&plane_state->src) >> 16;
> -       cache->plane.src_h = drm_rect_height(&plane_state->src) >> 16;
> -       cache->plane.visible = plane_state->visible;
> +       cache->plane.src_w = drm_rect_width(&plane_state->base.src) >> 16;
> +       cache->plane.src_h = drm_rect_height(&plane_state->base.src) >> 16;
> +       cache->plane.visible = plane_state->base.visible;
>
>         if (!cache->plane.visible)
>                 return;
> @@ -1050,7 +1050,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
>                 struct intel_plane_state *intel_plane_state =
>                         to_intel_plane_state(plane_state);
>
> -               if (!intel_plane_state->visible)
> +               if (!intel_plane_state->base.visible)
>                         continue;
>
>                 for_each_crtc_in_state(state, crtc, crtc_state, j) {
> @@ -1214,7 +1214,7 @@ void intel_fbc_init_pipe_state(struct drm_i915_private *dev_priv)
>
>         for_each_intel_crtc(&dev_priv->drm, crtc)
>                 if (intel_crtc_active(&crtc->base) &&
> -                   to_intel_plane_state(crtc->base.primary->state)->visible)
> +                   to_intel_plane_state(crtc->base.primary->state)->base.visible)
>                         dev_priv->fbc.visible_pipes_mask |= (1 << crtc->pipe);
>  }
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 64d628c915a3..8584e8f78b2e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -960,7 +960,7 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
>         if (dev_priv->wm.pri_latency[level] == 0)
>                 return USHRT_MAX;
>
> -       if (!state->visible)
> +       if (!state->base.visible)
>                 return 0;
>
>         cpp = drm_format_plane_cpp(state->base.fb->pixel_format, 0);
> @@ -1002,7 +1002,7 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
>                 if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
>                         continue;
>
> -               if (state->visible) {
> +               if (state->base.visible) {
>                         wm_state->num_active_planes++;
>                         total_rate += drm_format_plane_cpp(state->base.fb->pixel_format, 0);
>                 }
> @@ -1018,7 +1018,7 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
>                         continue;
>                 }
>
> -               if (!state->visible) {
> +               if (!state->base.visible) {
>                         plane->wm.fifo_size = 0;
>                         continue;
>                 }
> @@ -1118,7 +1118,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>                 struct intel_plane_state *state =
>                         to_intel_plane_state(plane->base.state);
>
> -               if (!state->visible)
> +               if (!state->base.visible)
>                         continue;
>
>                 /* normal watermarks */
> @@ -1767,7 +1767,7 @@ static uint32_t ilk_compute_pri_wm(const struct intel_crtc_state *cstate,
>                 drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0;
>         uint32_t method1, method2;
>
> -       if (!cstate->base.active || !pstate->visible)
> +       if (!cstate->base.active || !pstate->base.visible)
>                 return 0;
>
>         method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), cpp, mem_value);
> @@ -1777,7 +1777,7 @@ static uint32_t ilk_compute_pri_wm(const struct intel_crtc_state *cstate,
>
>         method2 = ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
>                                  cstate->base.adjusted_mode.crtc_htotal,
> -                                drm_rect_width(&pstate->dst),
> +                                drm_rect_width(&pstate->base.dst),
>                                  cpp, mem_value);
>
>         return min(method1, method2);
> @@ -1795,13 +1795,13 @@ static uint32_t ilk_compute_spr_wm(const struct intel_crtc_state *cstate,
>                 drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0;
>         uint32_t method1, method2;
>
> -       if (!cstate->base.active || !pstate->visible)
> +       if (!cstate->base.active || !pstate->base.visible)
>                 return 0;
>
>         method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), cpp, mem_value);
>         method2 = ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
>                                  cstate->base.adjusted_mode.crtc_htotal,
> -                                drm_rect_width(&pstate->dst),
> +                                drm_rect_width(&pstate->base.dst),
>                                  cpp, mem_value);
>         return min(method1, method2);
>  }
> @@ -1820,7 +1820,7 @@ static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate,
>          * this is necessary to avoid flickering.
>          */
>         int cpp = 4;
> -       int width = pstate->visible ? pstate->base.crtc_w : 64;
> +       int width = pstate->base.visible ? pstate->base.crtc_w : 64;
>
>         if (!cstate->base.active)
>                 return 0;
> @@ -1838,10 +1838,10 @@ static uint32_t ilk_compute_fbc_wm(const struct intel_crtc_state *cstate,
>         int cpp = pstate->base.fb ?
>                 drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0;
>
> -       if (!cstate->base.active || !pstate->visible)
> +       if (!cstate->base.active || !pstate->base.visible)
>                 return 0;
>
> -       return ilk_wm_fbc(pri_val, drm_rect_width(&pstate->dst), cpp);
> +       return ilk_wm_fbc(pri_val, drm_rect_width(&pstate->base.dst), cpp);
>  }
>
>  static unsigned int ilk_display_fifo_size(const struct drm_device *dev)
> @@ -2358,10 +2358,10 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>
>         pipe_wm->pipe_enabled = cstate->base.active;
>         if (sprstate) {
> -               pipe_wm->sprites_enabled = sprstate->visible;
> -               pipe_wm->sprites_scaled = sprstate->visible &&
> -                       (drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
> -                        drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
> +               pipe_wm->sprites_enabled = sprstate->base.visible;
> +               pipe_wm->sprites_scaled = sprstate->base.visible &&
> +                       (drm_rect_width(&sprstate->base.dst) != drm_rect_width(&sprstate->base.src) >> 16 ||
> +                        drm_rect_height(&sprstate->base.dst) != drm_rect_height(&sprstate->base.src) >> 16);
>         }
>
>         usable_level = max_level;
> @@ -2996,14 +2996,14 @@ skl_plane_downscale_amount(const struct intel_plane_state *pstate)
>         uint32_t downscale_h, downscale_w;
>         uint32_t src_w, src_h, dst_w, dst_h;
>
> -       if (WARN_ON(!pstate->visible))
> +       if (WARN_ON(!pstate->base.visible))
>                 return DRM_PLANE_HELPER_NO_SCALING;
>
>         /* n.b., src is 16.16 fixed point, dst is whole integer */
> -       src_w = drm_rect_width(&pstate->src);
> -       src_h = drm_rect_height(&pstate->src);
> -       dst_w = drm_rect_width(&pstate->dst);
> -       dst_h = drm_rect_height(&pstate->dst);
> +       src_w = drm_rect_width(&pstate->base.src);
> +       src_h = drm_rect_height(&pstate->base.src);
> +       dst_w = drm_rect_width(&pstate->base.dst);
> +       dst_h = drm_rect_height(&pstate->base.dst);
>         if (intel_rotation_90_or_270(pstate->base.rotation))
>                 swap(dst_w, dst_h);
>
> @@ -3025,15 +3025,15 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
>         uint32_t width = 0, height = 0;
>         unsigned format = fb ? fb->pixel_format : DRM_FORMAT_XRGB8888;
>
> -       if (!intel_pstate->visible)
> +       if (!intel_pstate->base.visible)
>                 return 0;
>         if (pstate->plane->type == DRM_PLANE_TYPE_CURSOR)
>                 return 0;
>         if (y && format != DRM_FORMAT_NV12)
>                 return 0;
>
> -       width = drm_rect_width(&intel_pstate->src) >> 16;
> -       height = drm_rect_height(&intel_pstate->src) >> 16;
> +       width = drm_rect_width(&intel_pstate->base.src) >> 16;
> +       height = drm_rect_height(&intel_pstate->base.src) >> 16;
>
>         if (intel_rotation_90_or_270(pstate->rotation))
>                 swap(width, height);
> @@ -3134,8 +3134,8 @@ skl_ddb_min_alloc(const struct drm_plane_state *pstate,
>             fb->modifier[0] != I915_FORMAT_MOD_Yf_TILED)
>                 return 8;
>
> -       src_w = drm_rect_width(&intel_pstate->src) >> 16;
> -       src_h = drm_rect_height(&intel_pstate->src) >> 16;
> +       src_w = drm_rect_width(&intel_pstate->base.src) >> 16;
> +       src_h = drm_rect_height(&intel_pstate->base.src) >> 16;
>
>         if (intel_rotation_90_or_270(pstate->rotation))
>                 swap(src_w, src_h);
> @@ -3226,7 +3226,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>                 if (intel_plane->pipe != pipe)
>                         continue;
>
> -               if (!to_intel_plane_state(pstate)->visible) {
> +               if (!to_intel_plane_state(pstate)->base.visible) {
>                         minimum[id] = 0;
>                         y_minimum[id] = 0;
>                         continue;
> @@ -3363,7 +3363,7 @@ static uint32_t skl_adjusted_plane_pixel_rate(const struct intel_crtc_state *cst
>         uint64_t pixel_rate;
>
>         /* Shouldn't reach here on disabled planes... */
> -       if (WARN_ON(!pstate->visible))
> +       if (WARN_ON(!pstate->base.visible))
>                 return 0;
>
>         /*
> @@ -3399,13 +3399,13 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>         uint32_t width = 0, height = 0;
>         uint32_t plane_pixel_rate;
>
> -       if (latency == 0 || !cstate->base.active || !intel_pstate->visible) {
> +       if (latency == 0 || !cstate->base.active || !intel_pstate->base.visible) {
>                 *enabled = false;
>                 return 0;
>         }
>
> -       width = drm_rect_width(&intel_pstate->src) >> 16;
> -       height = drm_rect_height(&intel_pstate->src) >> 16;
> +       width = drm_rect_width(&intel_pstate->base.src) >> 16;
> +       height = drm_rect_height(&intel_pstate->base.src) >> 16;
>
>         if (intel_rotation_90_or_270(pstate->rotation))
>                 swap(width, height);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0de935ad01c2..10f10915c0bc 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -211,14 +211,14 @@ skl_update_plane(struct drm_plane *drm_plane,
>         u32 tile_height, plane_offset, plane_size;
>         unsigned int rotation = plane_state->base.rotation;
>         int x_offset, y_offset;
> -       int crtc_x = plane_state->dst.x1;
> -       int crtc_y = plane_state->dst.y1;
> -       uint32_t crtc_w = drm_rect_width(&plane_state->dst);
> -       uint32_t crtc_h = drm_rect_height(&plane_state->dst);
> -       uint32_t x = plane_state->src.x1 >> 16;
> -       uint32_t y = plane_state->src.y1 >> 16;
> -       uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
> -       uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
> +       int crtc_x = plane_state->base.dst.x1;
> +       int crtc_y = plane_state->base.dst.y1;
> +       uint32_t crtc_w = drm_rect_width(&plane_state->base.dst);
> +       uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
> +       uint32_t x = plane_state->base.src.x1 >> 16;
> +       uint32_t y = plane_state->base.src.y1 >> 16;
> +       uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> +       uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
>
>         plane_ctl = PLANE_CTL_ENABLE |
>                 PLANE_CTL_PIPE_GAMMA_ENABLE |
> @@ -370,14 +370,14 @@ vlv_update_plane(struct drm_plane *dplane,
>         unsigned int rotation = dplane->state->rotation;
>         int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
>         const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> -       int crtc_x = plane_state->dst.x1;
> -       int crtc_y = plane_state->dst.y1;
> -       uint32_t crtc_w = drm_rect_width(&plane_state->dst);
> -       uint32_t crtc_h = drm_rect_height(&plane_state->dst);
> -       uint32_t x = plane_state->src.x1 >> 16;
> -       uint32_t y = plane_state->src.y1 >> 16;
> -       uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
> -       uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
> +       int crtc_x = plane_state->base.dst.x1;
> +       int crtc_y = plane_state->base.dst.y1;
> +       uint32_t crtc_w = drm_rect_width(&plane_state->base.dst);
> +       uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
> +       uint32_t x = plane_state->base.src.x1 >> 16;
> +       uint32_t y = plane_state->base.src.y1 >> 16;
> +       uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> +       uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
>
>         sprctl = SP_ENABLE;
>
> @@ -512,14 +512,14 @@ ivb_update_plane(struct drm_plane *plane,
>         unsigned int rotation = plane_state->base.rotation;
>         int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
>         const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> -       int crtc_x = plane_state->dst.x1;
> -       int crtc_y = plane_state->dst.y1;
> -       uint32_t crtc_w = drm_rect_width(&plane_state->dst);
> -       uint32_t crtc_h = drm_rect_height(&plane_state->dst);
> -       uint32_t x = plane_state->src.x1 >> 16;
> -       uint32_t y = plane_state->src.y1 >> 16;
> -       uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
> -       uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
> +       int crtc_x = plane_state->base.dst.x1;
> +       int crtc_y = plane_state->base.dst.y1;
> +       uint32_t crtc_w = drm_rect_width(&plane_state->base.dst);
> +       uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
> +       uint32_t x = plane_state->base.src.x1 >> 16;
> +       uint32_t y = plane_state->base.src.y1 >> 16;
> +       uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> +       uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
>
>         sprctl = SPRITE_ENABLE;
>
> @@ -653,14 +653,14 @@ ilk_update_plane(struct drm_plane *plane,
>         unsigned int rotation = plane_state->base.rotation;
>         int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
>         const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> -       int crtc_x = plane_state->dst.x1;
> -       int crtc_y = plane_state->dst.y1;
> -       uint32_t crtc_w = drm_rect_width(&plane_state->dst);
> -       uint32_t crtc_h = drm_rect_height(&plane_state->dst);
> -       uint32_t x = plane_state->src.x1 >> 16;
> -       uint32_t y = plane_state->src.y1 >> 16;
> -       uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
> -       uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
> +       int crtc_x = plane_state->base.dst.x1;
> +       int crtc_y = plane_state->base.dst.y1;
> +       uint32_t crtc_w = drm_rect_width(&plane_state->base.dst);
> +       uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
> +       uint32_t x = plane_state->base.src.x1 >> 16;
> +       uint32_t y = plane_state->base.src.y1 >> 16;
> +       uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> +       uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
>
>         dvscntr = DVS_ENABLE;
>
> @@ -778,15 +778,15 @@ intel_check_sprite_plane(struct drm_plane *plane,
>         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->src;
> -       struct drm_rect *dst = &state->dst;
> +       struct drm_rect *src = &state->base.src;
> +       struct drm_rect *dst = &state->base.dst;
>         const struct drm_rect *clip = &state->clip;
>         int hscale, vscale;
>         int max_scale, min_scale;
>         bool can_scale;
>
>         if (!fb) {
> -               state->visible = false;
> +               state->base.visible = false;
>                 return 0;
>         }
>
> @@ -834,14 +834,14 @@ intel_check_sprite_plane(struct drm_plane *plane,
>         vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
>         BUG_ON(vscale < 0);
>
> -       state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
> +       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);
>
> -       if (state->visible) {
> +       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) {
> @@ -898,12 +898,12 @@ intel_check_sprite_plane(struct drm_plane *plane,
>                                 crtc_w &= ~1;
>
>                         if (crtc_w == 0)
> -                               state->visible = false;
> +                               state->base.visible = false;
>                 }
>         }
>
>         /* Check size restrictions when scaling */
> -       if (state->visible && (src_w != crtc_w || src_h != crtc_h)) {
> +       if (state->base.visible && (src_w != crtc_w || src_h != crtc_h)) {
>                 unsigned int width_bytes;
>                 int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
>
> @@ -912,10 +912,10 @@ intel_check_sprite_plane(struct drm_plane *plane,
>                 /* FIXME interlacing min height is 6 */
>
>                 if (crtc_w < 3 || crtc_h < 3)
> -                       state->visible = false;
> +                       state->base.visible = false;
>
>                 if (src_w < 3 || src_h < 3)
> -                       state->visible = false;
> +                       state->base.visible = false;
>
>                 width_bytes = ((src_x * cpp) & 63) + src_w * cpp;
>
> @@ -926,7 +926,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>                 }
>         }
>
> -       if (state->visible) {
> +       if (state->base.visible) {
>                 src->x1 = src_x << 16;
>                 src->x2 = (src_x + src_w) << 16;
>                 src->y1 = src_y << 16;
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/9] drm/i915: Use drm_plane_helper_check_state()

[Sean Paul]

On Tue, Jul 26, 2016 at 12:07 PM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Replace the use of drm_plane_helper_check_update() with
> drm_plane_helper_check_state() since we have a plane state.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


Reviewed-by: Sean Paul <seanpaul@chromium.org>


> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 14 --------------
>  drivers/gpu/drm/i915/intel_display.c      | 26 +++++++++-----------------
>  drivers/gpu/drm/i915/intel_sprite.c       | 10 ++++++++++
>  3 files changed, 19 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 14d40261db21..2d831dd43d44 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -134,20 +134,6 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>
>         crtc_state = to_intel_crtc_state(drm_crtc_state);
>
> -       /*
> -        * The original src/dest coordinates are stored in state->base, but
> -        * we want to keep another copy internal to our driver that we can
> -        * clip/modify ourselves.
> -        */
> -       intel_state->base.src.x1 = state->src_x;
> -       intel_state->base.src.y1 = state->src_y;
> -       intel_state->base.src.x2 = state->src_x + state->src_w;
> -       intel_state->base.src.y2 = state->src_y + state->src_h;
> -       intel_state->base.dst.x1 = state->crtc_x;
> -       intel_state->base.dst.y1 = state->crtc_y;
> -       intel_state->base.dst.x2 = state->crtc_x + state->crtc_w;
> -       intel_state->base.dst.y2 = state->crtc_y + state->crtc_h;
> -
>         /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
>         intel_state->clip.x1 = 0;
>         intel_state->clip.y1 = 0;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4f67b7c19b75..eae8a72bad5a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14126,7 +14126,6 @@ intel_check_primary_plane(struct drm_plane *plane,
>                           struct intel_plane_state *state)
>  {
>         struct drm_crtc *crtc = state->base.crtc;
> -       struct drm_framebuffer *fb = state->base.fb;
>         int min_scale = DRM_PLANE_HELPER_NO_SCALING;
>         int max_scale = DRM_PLANE_HELPER_NO_SCALING;
>         bool can_position = false;
> @@ -14140,14 +14139,10 @@ intel_check_primary_plane(struct drm_plane *plane,
>                 can_position = true;
>         }
>
> -       return drm_plane_helper_check_update(plane, crtc, fb,
> -                                            &state->base.src,
> -                                            &state->base.dst,
> -                                            &state->base.clip,
> -                                            state->base.rotation,
> -                                            min_scale, max_scale,
> -                                            can_position, true,
> -                                            &state->base.visible);
> +       return drm_plane_helper_check_state(&state->base,
> +                                           &state->clip,
> +                                           min_scale, max_scale,
> +                                           can_position, true);
>  }
>
>  static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> @@ -14327,20 +14322,17 @@ intel_check_cursor_plane(struct drm_plane *plane,
>                          struct intel_crtc_state *crtc_state,
>                          struct intel_plane_state *state)
>  {
> -       struct drm_crtc *crtc = crtc_state->base.crtc;
>         struct drm_framebuffer *fb = state->base.fb;
>         struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>         enum pipe pipe = to_intel_plane(plane)->pipe;
>         unsigned stride;
>         int ret;
>
> -       ret = drm_plane_helper_check_update(plane, crtc, fb, &state->base.src,
> -                                           &state->base.dst,
> -                                           &state->base.clip,
> -                                           state->base.rotation,
> -                                           DRM_PLANE_HELPER_NO_SCALING,
> -                                           DRM_PLANE_HELPER_NO_SCALING,
> -                                           true, true, &state->base.visible);
> +       ret = drm_plane_helper_check_state(&state->base,
> +                                          &state->clip,
> +                                          DRM_PLANE_HELPER_NO_SCALING,
> +                                          DRM_PLANE_HELPER_NO_SCALING,
> +                                          true, true);
>         if (ret)
>                 return ret;
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 10f10915c0bc..b38956624f04 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -785,6 +785,16 @@ intel_check_sprite_plane(struct drm_plane *plane,
>         int max_scale, min_scale;
>         bool can_scale;
>
> +       src->x1 = state->base.src_x;
> +       src->y1 = state->base.src_y;
> +       src->x2 = state->base.src_x + state->base.src_w;
> +       src->y2 = state->base.src_y + state->base.src_h;
> +
> +       dst->x1 = state->base.crtc_x;
> +       dst->y1 = state->base.crtc_y;
> +       dst->x2 = state->base.crtc_x + state->base.crtc_w;
> +       dst->y2 = state->base.crtc_y + state->base.crtc_h;
> +
>         if (!fb) {
>                 state->base.visible = false;
>                 return 0;
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 6/9] drm/rockchip: Use drm_plane_state.{src, dst}

[Sean Paul]

On Tue, Jul 26, 2016 at 12:07 PM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Replace the private drm_rects in vop_plane_state with
> the ones now living in drm_plane_state.
>
> Cc: Yao <mark.yao@rock-chips.com>
> Cc: linux-rockchip@lists.infradead.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 91305eb7d312..c566c740ab49 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -87,8 +87,6 @@
>  struct vop_plane_state {
>         struct drm_plane_state base;
>         int format;
> -       struct drm_rect src;
> -       struct drm_rect dest;
>         dma_addr_t yrgb_mst;
>         bool enable;
>  };
> @@ -595,8 +593,8 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
>         const struct vop_win_data *win = vop_win->data;
>         bool visible;
>         int ret;
> -       struct drm_rect *dest = &vop_plane_state->dest;
> -       struct drm_rect *src = &vop_plane_state->src;
> +       struct drm_rect *dest = &state->dst;
> +       struct drm_rect *src = &state->src;
>         struct drm_rect clip;
>         int min_scale = win->phy->scl ? FRAC_16_16(1, 8) :
>                                         DRM_PLANE_HELPER_NO_SCALING;
> @@ -694,8 +692,8 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
>         unsigned int actual_w, actual_h;
>         unsigned int dsp_stx, dsp_sty;
>         uint32_t act_info, dsp_info, dsp_st;
> -       struct drm_rect *src = &vop_plane_state->src;
> -       struct drm_rect *dest = &vop_plane_state->dest;
> +       struct drm_rect *src = &state->src;
> +       struct drm_rect *dest = &state->dst;
>         struct drm_gem_object *obj, *uv_obj;
>         struct rockchip_gem_object *rk_obj, *rk_uv_obj;
>         unsigned long offset;
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 7/9] drm/rockchip: Use drm_plane_helper_check_state()

[Sean Paul]

On Tue, Jul 26, 2016 at 12:07 PM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Replace the use of drm_plane_helper_check_update() with
> drm_plane_helper_check_state() since we have a plane state.
>
> Rockchip looks to handling plane clipping rather well already
> (unlikje most arm drm drivers) so there are no function changes
> here.
>
> Cc: Yao <mark.yao@rock-chips.com>
> Cc: linux-rockchip@lists.infradead.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 25 +++++--------------------
>  1 file changed, 5 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index c566c740ab49..31744fe99b38 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -591,10 +591,7 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
>         struct vop_win *vop_win = to_vop_win(plane);
>         struct vop_plane_state *vop_plane_state = to_vop_plane_state(state);
>         const struct vop_win_data *win = vop_win->data;
> -       bool visible;
>         int ret;
> -       struct drm_rect *dest = &state->dst;
> -       struct drm_rect *src = &state->src;
>         struct drm_rect clip;
>         int min_scale = win->phy->scl ? FRAC_16_16(1, 8) :
>                                         DRM_PLANE_HELPER_NO_SCALING;
> @@ -608,30 +605,18 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
>         if (WARN_ON(!crtc_state))
>                 return -EINVAL;
>
> -       src->x1 = state->src_x;
> -       src->y1 = state->src_y;
> -       src->x2 = state->src_x + state->src_w;
> -       src->y2 = state->src_y + state->src_h;
> -       dest->x1 = state->crtc_x;
> -       dest->y1 = state->crtc_y;
> -       dest->x2 = state->crtc_x + state->crtc_w;
> -       dest->y2 = state->crtc_y + state->crtc_h;
> -
>         clip.x1 = 0;
>         clip.y1 = 0;
>         clip.x2 = crtc_state->adjusted_mode.hdisplay;
>         clip.y2 = crtc_state->adjusted_mode.vdisplay;
>
> -       ret = drm_plane_helper_check_update(plane, crtc, state->fb,
> -                                           src, dest, &clip,
> -                                           state->rotation,
> -                                           min_scale,
> -                                           max_scale,
> -                                           true, true, &visible);
> +       ret = drm_plane_helper_check_state(state, &clip,
> +                                          min_scale, max_scale,
> +                                          true, true);
>         if (ret)
>                 return ret;
>
> -       if (!visible)
> +       if (!state->visible)
>                 goto out_disable;
>
>         vop_plane_state->format = vop_convert_format(fb->pixel_format);
> @@ -642,7 +627,7 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
>          * Src.x1 can be odd when do clip, but yuv plane start point
>          * need align with 2 pixel.
>          */
> -       if (is_yuv_support(fb->pixel_format) && ((src->x1 >> 16) % 2))
> +       if (is_yuv_support(fb->pixel_format) && ((state->src.x1 >> 16) % 2))
>                 return -EINVAL;
>
>         vop_plane_state->enable = true;
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/9] drm/mediatek: Use drm_plane_helper_check_state()

[Sean Paul]

On Tue, Jul 26, 2016 at 12:07 PM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Replace the use of drm_plane_helper_check_update() with
> drm_plane_helper_check_state() since we have a plane state.
>
> This also eliminates the double clipping the driver was doing
> in both check and commit phases). And it should fix src coordinate
> addr adjustement. Previously the driver was expecting negative dst
> coordinates after clipping, which is not going happen, so any clipping
> induced addr adjustment simply didn't happen. Neither did the driver
> respect any user configured src coordinates, so panning and such would
> have been totally broken. It should be all good now.
>
> Cc: CK Hu <ck.hu@mediatek.com>
> Cc: linux-mediatek@lists.infradead.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 72 +++++++++-----------------------
>  1 file changed, 20 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index 3995765a90dc..5f2516fca079 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -30,15 +30,20 @@ static const u32 formats[] = {
>         DRM_FORMAT_RGB565,
>  };
>
> -static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable,
> -                            dma_addr_t addr, struct drm_rect *dest)
> +static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane,
> +                            dma_addr_t addr)
>  {
>         struct drm_plane *plane = &mtk_plane->base;
>         struct mtk_plane_state *state = to_mtk_plane_state(plane->state);
>         unsigned int pitch, format;
> -       int x, y;
> +       bool enable;
>
> -       if (WARN_ON(!plane->state || (enable && !plane->state->fb)))
> +       if (WARN_ON(!plane->state))
> +               return;
> +
> +       enable = state->base.visible;
> +
> +       if (WARN_ON(enable && !plane->state->fb))
>                 return;
>
>         if (plane->state->fb) {
> @@ -49,27 +54,17 @@ static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable,
>                 format = DRM_FORMAT_RGBA8888;
>         }
>
> -       x = plane->state->crtc_x;
> -       y = plane->state->crtc_y;
> -
> -       if (x < 0) {
> -               addr -= x * 4;
> -               x = 0;
> -       }
> -
> -       if (y < 0) {
> -               addr -= y * pitch;
> -               y = 0;
> -       }
> +       addr += (state->base.src.x1 >> 16) * 4;
> +       addr += (state->base.src.y1 >> 16) * pitch;
>
>         state->pending.enable = enable;
>         state->pending.pitch = pitch;
>         state->pending.format = format;
>         state->pending.addr = addr;
> -       state->pending.x = x;
> -       state->pending.y = y;
> -       state->pending.width = dest->x2 - dest->x1;
> -       state->pending.height = dest->y2 - dest->y1;
> +       state->pending.x = state->base.dst.x1;
> +       state->pending.y = state->base.dst.y1;
> +       state->pending.width = drm_rect_width(&state->base.dst);
> +       state->pending.height = drm_rect_height(&state->base.dst);
>         wmb(); /* Make sure the above parameters are set before update */
>         state->pending.dirty = true;
>  }
> @@ -134,20 +129,6 @@ static int mtk_plane_atomic_check(struct drm_plane *plane,
>  {
>         struct drm_framebuffer *fb = state->fb;
>         struct drm_crtc_state *crtc_state;
> -       bool visible;
> -       struct drm_rect dest = {
> -               .x1 = state->crtc_x,
> -               .y1 = state->crtc_y,
> -               .x2 = state->crtc_x + state->crtc_w,
> -               .y2 = state->crtc_y + state->crtc_h,
> -       };
> -       struct drm_rect src = {
> -               /* 16.16 fixed point */
> -               .x1 = state->src_x,
> -               .y1 = state->src_y,
> -               .x2 = state->src_x + state->src_w,
> -               .y2 = state->src_y + state->src_h,
> -       };
>         struct drm_rect clip = { 0, };
>
>         if (!fb)
> @@ -168,12 +149,10 @@ static int mtk_plane_atomic_check(struct drm_plane *plane,
>         clip.x2 = crtc_state->mode.hdisplay;
>         clip.y2 = crtc_state->mode.vdisplay;
>
> -       return drm_plane_helper_check_update(plane, state->crtc, fb,
> -                                            &src, &dest, &clip,
> -                                            state->rotation,
> -                                            DRM_PLANE_HELPER_NO_SCALING,
> -                                            DRM_PLANE_HELPER_NO_SCALING,
> -                                            true, true, &visible);
> +       return drm_plane_helper_check_state(state, &clip,
> +                                           DRM_PLANE_HELPER_NO_SCALING,
> +                                           DRM_PLANE_HELPER_NO_SCALING,
> +                                           true, true);
>  }
>
>  static void mtk_plane_atomic_update(struct drm_plane *plane,
> @@ -184,24 +163,13 @@ static void mtk_plane_atomic_update(struct drm_plane *plane,
>         struct drm_gem_object *gem;
>         struct mtk_drm_gem_obj *mtk_gem;
>         struct mtk_drm_plane *mtk_plane = to_mtk_plane(plane);
> -       struct drm_rect dest = {
> -               .x1 = state->base.crtc_x,
> -               .y1 = state->base.crtc_y,
> -               .x2 = state->base.crtc_x + state->base.crtc_w,
> -               .y2 = state->base.crtc_y + state->base.crtc_h,
> -       };
> -       struct drm_rect clip = { 0, };
>
>         if (!crtc)
>                 return;
>
> -       clip.x2 = state->base.crtc->state->mode.hdisplay;
> -       clip.y2 = state->base.crtc->state->mode.vdisplay;
> -       drm_rect_intersect(&dest, &clip);
> -
>         gem = mtk_fb_get_gem_obj(state->base.fb);
>         mtk_gem = to_mtk_gem_obj(gem);
> -       mtk_plane_enable(mtk_plane, true, mtk_gem->dma_addr, &dest);
> +       mtk_plane_enable(mtk_plane, mtk_gem->dma_addr);
>  }
>
>  static void mtk_plane_atomic_disable(struct drm_plane *plane,
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 9/9] drm/simple_kms_helper: Use drm_plane_helper_check_state()

[Sean Paul]

On Tue, Jul 26, 2016 at 12:07 PM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Replace the use of drm_plane_helper_check_update() with
> drm_plane_helper_check_state() since we have a plane state.
>
> I don't see any actual users of drm_simple_kms_helper yet, so
> no actual plane clipping bugs to fix.
>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/drm_simple_kms_helper.c | 27 ++++++---------------------
>  1 file changed, 6 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 0db36d27e90b..0a02efe978ee 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -73,22 +73,9 @@ static const struct drm_crtc_funcs drm_simple_kms_crtc_funcs = {
>  static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
>                                         struct drm_plane_state *plane_state)
>  {
> -       struct drm_rect src = {
> -               .x1 = plane_state->src_x,
> -               .y1 = plane_state->src_y,
> -               .x2 = plane_state->src_x + plane_state->src_w,
> -               .y2 = plane_state->src_y + plane_state->src_h,
> -       };
> -       struct drm_rect dest = {
> -               .x1 = plane_state->crtc_x,
> -               .y1 = plane_state->crtc_y,
> -               .x2 = plane_state->crtc_x + plane_state->crtc_w,
> -               .y2 = plane_state->crtc_y + plane_state->crtc_h,
> -       };
>         struct drm_rect clip = { 0 };
>         struct drm_simple_display_pipe *pipe;
>         struct drm_crtc_state *crtc_state;
> -       bool visible;
>         int ret;
>
>         pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> @@ -102,17 +89,15 @@ static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
>
>         clip.x2 = crtc_state->adjusted_mode.hdisplay;
>         clip.y2 = crtc_state->adjusted_mode.vdisplay;
> -       ret = drm_plane_helper_check_update(plane, &pipe->crtc,
> -                                           plane_state->fb,
> -                                           &src, &dest, &clip,
> -                                           plane_state->rotation,
> -                                           DRM_PLANE_HELPER_NO_SCALING,
> -                                           DRM_PLANE_HELPER_NO_SCALING,
> -                                           false, true, &visible);
> +
> +       ret = drm_plane_helper_check_state(plane_state, &clip,
> +                                          DRM_PLANE_HELPER_NO_SCALING,
> +                                          DRM_PLANE_HELPER_NO_SCALING,
> +                                          false, true);
>         if (ret)
>                 return ret;
>
> -       if (!visible)
> +       if (!plane_state->visible)
>                 return -EINVAL;
>
>         if (!pipe->funcs || !pipe->funcs->check)
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/9] drm: Store clipped coordinates in drm_plane_state

[Sean Paul]

On Tue, Jul 26, 2016 at 12:06 PM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Moving the clipped plane coordinates into drm_plane_state has been
> discussed a few times, but as no patches seems to have materialized,
> I decoded to do it myself. I also added a new helper function
> like drm_plane_helper_check_update() that takes a plane state instead.
>
> I converted i915, rockchip, and mediatek over to the new stuff. rockchip
> already looked pretty solid, mediatek had some bugs in there that I
> hopefully fixed. The rest of the non-x86 drivers seem to entirely lack
> any plane clipping code, so I decided that I don't care enough to
> write it from scratch. I also converted drm_simple_kms_helper, but
> there are no drivers using it so far.
>
> I've only actually tested i915, the rest are just compile tested.
>
> Entire series available here:
> git://github.com/vsyrjala/linux.git plane_state_rects
>
> Ville Syrjälä (9):
>   drm: Warn about negative sizes when calculating scale factor
>   drm: Store clipped src/dst coordinatee in drm_plane_state
>   drm/plane-helper: Add drm_plane_helper_check_state()
>   drm/i915: Use drm_plane_state.{src,dst,visible}
>   drm/i915: Use drm_plane_helper_check_state()
>   drm/rockchip: Use drm_plane_state.{src,dst}
>   drm/rockchip: Use drm_plane_helper_check_state()
>   drm/mediatek: Use drm_plane_helper_check_state()
>   drm/simple_kms_helper: Use drm_plane_helper_check_state()


Looks good to me, all patches have been reviewed.

It seems like the only consumer of drm_plane_helper_check_update()
left is armada. Are you planning on converting it as well? Then we can
nuke the function.

Sean

>
>  drivers/gpu/drm/drm_plane_helper.c          | 136 +++++++++++++++++++++------
>  drivers/gpu/drm/drm_rect.c                  |   2 +-
>  drivers/gpu/drm/drm_simple_kms_helper.c     |  27 ++----
>  drivers/gpu/drm/i915/intel_atomic_plane.c   |  18 +---
>  drivers/gpu/drm/i915/intel_display.c        | 140 ++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_drv.h            |   3 -
>  drivers/gpu/drm/i915/intel_fbc.c            |  12 +--
>  drivers/gpu/drm/i915/intel_pm.c             |  60 ++++++------
>  drivers/gpu/drm/i915/intel_sprite.c         |  94 ++++++++++---------
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c    |  72 ++++----------
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  31 ++----
>  include/drm/drm_crtc.h                      |  13 +++
>  include/drm/drm_plane_helper.h              |   5 +
>  13 files changed, 315 insertions(+), 298 deletions(-)
>
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/9] drm: Store clipped coordinates in drm_plane_state

[Ville Syrjälä]

On Mon, Aug 01, 2016 at 11:12:05AM -0400, Sean Paul wrote:
> On Tue, Jul 26, 2016 at 12:06 PM,  <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Moving the clipped plane coordinates into drm_plane_state has been
> > discussed a few times, but as no patches seems to have materialized,
> > I decoded to do it myself. I also added a new helper function
> > like drm_plane_helper_check_update() that takes a plane state instead.
> >
> > I converted i915, rockchip, and mediatek over to the new stuff. rockchip
> > already looked pretty solid, mediatek had some bugs in there that I
> > hopefully fixed. The rest of the non-x86 drivers seem to entirely lack
> > any plane clipping code, so I decided that I don't care enough to
> > write it from scratch. I also converted drm_simple_kms_helper, but
> > there are no drivers using it so far.
> >
> > I've only actually tested i915, the rest are just compile tested.
> >
> > Entire series available here:
> > git://github.com/vsyrjala/linux.git plane_state_rects
> >
> > Ville Syrjälä (9):
> >   drm: Warn about negative sizes when calculating scale factor
> >   drm: Store clipped src/dst coordinatee in drm_plane_state
> >   drm/plane-helper: Add drm_plane_helper_check_state()
> >   drm/i915: Use drm_plane_state.{src,dst,visible}
> >   drm/i915: Use drm_plane_helper_check_state()
> >   drm/rockchip: Use drm_plane_state.{src,dst}
> >   drm/rockchip: Use drm_plane_helper_check_state()
> >   drm/mediatek: Use drm_plane_helper_check_state()
> >   drm/simple_kms_helper: Use drm_plane_helper_check_state()
> 
> 
> Looks good to me, all patches have been reviewed.
> 
> It seems like the only consumer of drm_plane_helper_check_update()
> left is armada. Are you planning on converting it as well? Then we can
> nuke the function.

IIRC that driver isn't atomic enough for the new function.

> 
> Sean
> 
> >
> >  drivers/gpu/drm/drm_plane_helper.c          | 136 +++++++++++++++++++++------
> >  drivers/gpu/drm/drm_rect.c                  |   2 +-
> >  drivers/gpu/drm/drm_simple_kms_helper.c     |  27 ++----
> >  drivers/gpu/drm/i915/intel_atomic_plane.c   |  18 +---
> >  drivers/gpu/drm/i915/intel_display.c        | 140 ++++++++++++++--------------
> >  drivers/gpu/drm/i915/intel_drv.h            |   3 -
> >  drivers/gpu/drm/i915/intel_fbc.c            |  12 +--
> >  drivers/gpu/drm/i915/intel_pm.c             |  60 ++++++------
> >  drivers/gpu/drm/i915/intel_sprite.c         |  94 ++++++++++---------
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.c    |  72 ++++----------
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  31 ++----
> >  include/drm/drm_crtc.h                      |  13 +++
> >  include/drm/drm_plane_helper.h              |   5 +
> >  13 files changed, 315 insertions(+), 298 deletions(-)
> >
> > --
> > 2.7.4
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/9] drm/mediatek: Use drm_plane_helper_check_state()

[Bibby Hsieh]

On Tue, 2016-07-26 at 19:07 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Replace the use of drm_plane_helper_check_update() with
> drm_plane_helper_check_state() since we have a plane state.
> 
> This also eliminates the double clipping the driver was doing
> in both check and commit phases). And it should fix src coordinate
> addr adjustement. Previously the driver was expecting negative dst
> coordinates after clipping, which is not going happen, so any clipping
> induced addr adjustment simply didn't happen. Neither did the driver
> respect any user configured src coordinates, so panning and such would
> have been totally broken. It should be all good now.
> 
> Cc: CK Hu <ck.hu@mediatek.com>
> Cc: linux-mediatek@lists.infradead.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

This patch looks fine to me, thanks for your patch.

Reviewed-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
Tested-by: Bibby Hsieh <bibby.hsieh@mediatek.com>

> ---
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 72 +++++++++-----------------------
>  1 file changed, 20 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index 3995765a90dc..5f2516fca079 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -30,15 +30,20 @@ static const u32 formats[] = {
>  	DRM_FORMAT_RGB565,
>  };
>  
> -static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable,
> -			     dma_addr_t addr, struct drm_rect *dest)
> +static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane,
> +			     dma_addr_t addr)
>  {
>  	struct drm_plane *plane = &mtk_plane->base;
>  	struct mtk_plane_state *state = to_mtk_plane_state(plane->state);
>  	unsigned int pitch, format;
> -	int x, y;
> +	bool enable;
>  
> -	if (WARN_ON(!plane->state || (enable && !plane->state->fb)))
> +	if (WARN_ON(!plane->state))
> +		return;
> +
> +	enable = state->base.visible;
> +
> +	if (WARN_ON(enable && !plane->state->fb))
>  		return;
>  
>  	if (plane->state->fb) {
> @@ -49,27 +54,17 @@ static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable,
>  		format = DRM_FORMAT_RGBA8888;
>  	}
>  
> -	x = plane->state->crtc_x;
> -	y = plane->state->crtc_y;
> -
> -	if (x < 0) {
> -		addr -= x * 4;
> -		x = 0;
> -	}
> -
> -	if (y < 0) {
> -		addr -= y * pitch;
> -		y = 0;
> -	}
> +	addr += (state->base.src.x1 >> 16) * 4;
> +	addr += (state->base.src.y1 >> 16) * pitch;
>  
>  	state->pending.enable = enable;
>  	state->pending.pitch = pitch;
>  	state->pending.format = format;
>  	state->pending.addr = addr;
> -	state->pending.x = x;
> -	state->pending.y = y;
> -	state->pending.width = dest->x2 - dest->x1;
> -	state->pending.height = dest->y2 - dest->y1;
> +	state->pending.x = state->base.dst.x1;
> +	state->pending.y = state->base.dst.y1;
> +	state->pending.width = drm_rect_width(&state->base.dst);
> +	state->pending.height = drm_rect_height(&state->base.dst);
>  	wmb(); /* Make sure the above parameters are set before update */
>  	state->pending.dirty = true;
>  }
> @@ -134,20 +129,6 @@ static int mtk_plane_atomic_check(struct drm_plane *plane,
>  {
>  	struct drm_framebuffer *fb = state->fb;
>  	struct drm_crtc_state *crtc_state;
> -	bool visible;
> -	struct drm_rect dest = {
> -		.x1 = state->crtc_x,
> -		.y1 = state->crtc_y,
> -		.x2 = state->crtc_x + state->crtc_w,
> -		.y2 = state->crtc_y + state->crtc_h,
> -	};
> -	struct drm_rect src = {
> -		/* 16.16 fixed point */
> -		.x1 = state->src_x,
> -		.y1 = state->src_y,
> -		.x2 = state->src_x + state->src_w,
> -		.y2 = state->src_y + state->src_h,
> -	};
>  	struct drm_rect clip = { 0, };
>  
>  	if (!fb)
> @@ -168,12 +149,10 @@ static int mtk_plane_atomic_check(struct drm_plane *plane,
>  	clip.x2 = crtc_state->mode.hdisplay;
>  	clip.y2 = crtc_state->mode.vdisplay;
>  
> -	return drm_plane_helper_check_update(plane, state->crtc, fb,
> -					     &src, &dest, &clip,
> -					     state->rotation,
> -					     DRM_PLANE_HELPER_NO_SCALING,
> -					     DRM_PLANE_HELPER_NO_SCALING,
> -					     true, true, &visible);
> +	return drm_plane_helper_check_state(state, &clip,
> +					    DRM_PLANE_HELPER_NO_SCALING,
> +					    DRM_PLANE_HELPER_NO_SCALING,
> +					    true, true);
>  }
>  
>  static void mtk_plane_atomic_update(struct drm_plane *plane,
> @@ -184,24 +163,13 @@ static void mtk_plane_atomic_update(struct drm_plane *plane,
>  	struct drm_gem_object *gem;
>  	struct mtk_drm_gem_obj *mtk_gem;
>  	struct mtk_drm_plane *mtk_plane = to_mtk_plane(plane);
> -	struct drm_rect dest = {
> -		.x1 = state->base.crtc_x,
> -		.y1 = state->base.crtc_y,
> -		.x2 = state->base.crtc_x + state->base.crtc_w,
> -		.y2 = state->base.crtc_y + state->base.crtc_h,
> -	};
> -	struct drm_rect clip = { 0, };
>  
>  	if (!crtc)
>  		return;
>  
> -	clip.x2 = state->base.crtc->state->mode.hdisplay;
> -	clip.y2 = state->base.crtc->state->mode.vdisplay;
> -	drm_rect_intersect(&dest, &clip);
> -
>  	gem = mtk_fb_get_gem_obj(state->base.fb);
>  	mtk_gem = to_mtk_gem_obj(gem);
> -	mtk_plane_enable(mtk_plane, true, mtk_gem->dma_addr, &dest);
> +	mtk_plane_enable(mtk_plane, mtk_gem->dma_addr);
>  }
>  
>  static void mtk_plane_atomic_disable(struct drm_plane *plane,

-- 
Bibby

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/9] drm/mediatek: Use drm_plane_helper_check_state()

[CK Hu]

On Tue, 2016-07-26 at 19:07 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Replace the use of drm_plane_helper_check_update() with
> drm_plane_helper_check_state() since we have a plane state.
> 
> This also eliminates the double clipping the driver was doing
> in both check and commit phases). And it should fix src coordinate
> addr adjustement. Previously the driver was expecting negative dst
> coordinates after clipping, which is not going happen, so any clipping
> induced addr adjustment simply didn't happen. Neither did the driver
> respect any user configured src coordinates, so panning and such would
> have been totally broken. It should be all good now.
> 
> Cc: CK Hu <ck.hu@mediatek.com>
> Cc: linux-mediatek@lists.infradead.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---

Acked-by: CK Hu <ck.hu@mediatek.com>

>  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 72 +++++++++-----------------------
>  1 file changed, 20 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index 3995765a90dc..5f2516fca079 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -30,15 +30,20 @@ static const u32 formats[] = {
>  	DRM_FORMAT_RGB565,
>  };
>  
> -static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable,
> -			     dma_addr_t addr, struct drm_rect *dest)
> +static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane,
> +			     dma_addr_t addr)
>  {
>  	struct drm_plane *plane = &mtk_plane->base;
>  	struct mtk_plane_state *state = to_mtk_plane_state(plane->state);
>  	unsigned int pitch, format;
> -	int x, y;
> +	bool enable;
>  
> -	if (WARN_ON(!plane->state || (enable && !plane->state->fb)))
> +	if (WARN_ON(!plane->state))
> +		return;
> +
> +	enable = state->base.visible;
> +
> +	if (WARN_ON(enable && !plane->state->fb))
>  		return;
>  
>  	if (plane->state->fb) {
> @@ -49,27 +54,17 @@ static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable,
>  		format = DRM_FORMAT_RGBA8888;
>  	}
>  
> -	x = plane->state->crtc_x;
> -	y = plane->state->crtc_y;
> -
> -	if (x < 0) {
> -		addr -= x * 4;
> -		x = 0;
> -	}
> -
> -	if (y < 0) {
> -		addr -= y * pitch;
> -		y = 0;
> -	}
> +	addr += (state->base.src.x1 >> 16) * 4;
> +	addr += (state->base.src.y1 >> 16) * pitch;
>  
>  	state->pending.enable = enable;
>  	state->pending.pitch = pitch;
>  	state->pending.format = format;
>  	state->pending.addr = addr;
> -	state->pending.x = x;
> -	state->pending.y = y;
> -	state->pending.width = dest->x2 - dest->x1;
> -	state->pending.height = dest->y2 - dest->y1;
> +	state->pending.x = state->base.dst.x1;
> +	state->pending.y = state->base.dst.y1;
> +	state->pending.width = drm_rect_width(&state->base.dst);
> +	state->pending.height = drm_rect_height(&state->base.dst);
>  	wmb(); /* Make sure the above parameters are set before update */
>  	state->pending.dirty = true;
>  }
> @@ -134,20 +129,6 @@ static int mtk_plane_atomic_check(struct drm_plane *plane,
>  {
>  	struct drm_framebuffer *fb = state->fb;
>  	struct drm_crtc_state *crtc_state;
> -	bool visible;
> -	struct drm_rect dest = {
> -		.x1 = state->crtc_x,
> -		.y1 = state->crtc_y,
> -		.x2 = state->crtc_x + state->crtc_w,
> -		.y2 = state->crtc_y + state->crtc_h,
> -	};
> -	struct drm_rect src = {
> -		/* 16.16 fixed point */
> -		.x1 = state->src_x,
> -		.y1 = state->src_y,
> -		.x2 = state->src_x + state->src_w,
> -		.y2 = state->src_y + state->src_h,
> -	};
>  	struct drm_rect clip = { 0, };
>  
>  	if (!fb)
> @@ -168,12 +149,10 @@ static int mtk_plane_atomic_check(struct drm_plane *plane,
>  	clip.x2 = crtc_state->mode.hdisplay;
>  	clip.y2 = crtc_state->mode.vdisplay;
>  
> -	return drm_plane_helper_check_update(plane, state->crtc, fb,
> -					     &src, &dest, &clip,
> -					     state->rotation,
> -					     DRM_PLANE_HELPER_NO_SCALING,
> -					     DRM_PLANE_HELPER_NO_SCALING,
> -					     true, true, &visible);
> +	return drm_plane_helper_check_state(state, &clip,
> +					    DRM_PLANE_HELPER_NO_SCALING,
> +					    DRM_PLANE_HELPER_NO_SCALING,
> +					    true, true);
>  }
>  
>  static void mtk_plane_atomic_update(struct drm_plane *plane,
> @@ -184,24 +163,13 @@ static void mtk_plane_atomic_update(struct drm_plane *plane,
>  	struct drm_gem_object *gem;
>  	struct mtk_drm_gem_obj *mtk_gem;
>  	struct mtk_drm_plane *mtk_plane = to_mtk_plane(plane);
> -	struct drm_rect dest = {
> -		.x1 = state->base.crtc_x,
> -		.y1 = state->base.crtc_y,
> -		.x2 = state->base.crtc_x + state->base.crtc_w,
> -		.y2 = state->base.crtc_y + state->base.crtc_h,
> -	};
> -	struct drm_rect clip = { 0, };
>  
>  	if (!crtc)
>  		return;
>  
> -	clip.x2 = state->base.crtc->state->mode.hdisplay;
> -	clip.y2 = state->base.crtc->state->mode.vdisplay;
> -	drm_rect_intersect(&dest, &clip);
> -
>  	gem = mtk_fb_get_gem_obj(state->base.fb);
>  	mtk_gem = to_mtk_gem_obj(gem);
> -	mtk_plane_enable(mtk_plane, true, mtk_gem->dma_addr, &dest);
> +	mtk_plane_enable(mtk_plane, mtk_gem->dma_addr);
>  }
>  
>  static void mtk_plane_atomic_disable(struct drm_plane *plane,


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/9] drm: Store clipped coordinates in drm_plane_state

[Daniel Vetter]

On Mon, Aug 01, 2016 at 06:19:02PM +0300, Ville Syrjälä wrote:
> On Mon, Aug 01, 2016 at 11:12:05AM -0400, Sean Paul wrote:
> > On Tue, Jul 26, 2016 at 12:06 PM,  <ville.syrjala@linux.intel.com> wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Moving the clipped plane coordinates into drm_plane_state has been
> > > discussed a few times, but as no patches seems to have materialized,
> > > I decoded to do it myself. I also added a new helper function
> > > like drm_plane_helper_check_update() that takes a plane state instead.
> > >
> > > I converted i915, rockchip, and mediatek over to the new stuff. rockchip
> > > already looked pretty solid, mediatek had some bugs in there that I
> > > hopefully fixed. The rest of the non-x86 drivers seem to entirely lack
> > > any plane clipping code, so I decided that I don't care enough to
> > > write it from scratch. I also converted drm_simple_kms_helper, but
> > > there are no drivers using it so far.
> > >
> > > I've only actually tested i915, the rest are just compile tested.
> > >
> > > Entire series available here:
> > > git://github.com/vsyrjala/linux.git plane_state_rects
> > >
> > > Ville Syrjälä (9):
> > >   drm: Warn about negative sizes when calculating scale factor
> > >   drm: Store clipped src/dst coordinatee in drm_plane_state
> > >   drm/plane-helper: Add drm_plane_helper_check_state()
> > >   drm/i915: Use drm_plane_state.{src,dst,visible}
> > >   drm/i915: Use drm_plane_helper_check_state()
> > >   drm/rockchip: Use drm_plane_state.{src,dst}
> > >   drm/rockchip: Use drm_plane_helper_check_state()
> > >   drm/mediatek: Use drm_plane_helper_check_state()
> > >   drm/simple_kms_helper: Use drm_plane_helper_check_state()
> > 
> > 
> > Looks good to me, all patches have been reviewed.
> > 
> > It seems like the only consumer of drm_plane_helper_check_update()
> > left is armada. Are you planning on converting it as well? Then we can
> > nuke the function.
> 
> IIRC that driver isn't atomic enough for the new function.

Yeah, I think no one has yet started an effort to make armada atomic :(
-Daniel

> 
> > 
> > Sean
> > 
> > >
> > >  drivers/gpu/drm/drm_plane_helper.c          | 136 +++++++++++++++++++++------
> > >  drivers/gpu/drm/drm_rect.c                  |   2 +-
> > >  drivers/gpu/drm/drm_simple_kms_helper.c     |  27 ++----
> > >  drivers/gpu/drm/i915/intel_atomic_plane.c   |  18 +---
> > >  drivers/gpu/drm/i915/intel_display.c        | 140 ++++++++++++++--------------
> > >  drivers/gpu/drm/i915/intel_drv.h            |   3 -
> > >  drivers/gpu/drm/i915/intel_fbc.c            |  12 +--
> > >  drivers/gpu/drm/i915/intel_pm.c             |  60 ++++++------
> > >  drivers/gpu/drm/i915/intel_sprite.c         |  94 ++++++++++---------
> > >  drivers/gpu/drm/mediatek/mtk_drm_plane.c    |  72 ++++----------
> > >  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  31 ++----
> > >  include/drm/drm_crtc.h                      |  13 +++
> > >  include/drm/drm_plane_helper.h              |   5 +
> > >  13 files changed, 315 insertions(+), 298 deletions(-)
> > >
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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

Re: [PATCH v2 3/9] drm/plane-helper: Add drm_plane_helper_check_state()

[Daniel Kurtz]

Hi Ville,

Two fixes inline...

On Wed, Jul 27, 2016 at 1:34 AM, <ville.syrjala@linux.intel.com> wrote:
>
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Add a version of drm_plane_helper_check_update() which takes a plane
> state instead of having the caller pass in everything.
>
> And to reduce code duplication, let's reimplement
> drm_plane_helper_check_update() in terms of the new function, by
> having a tempororary plane state on the stack.
>
> v2: Add a note that the functions modifies the state (Chris)
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

[snip...]

> +int drm_plane_helper_check_update(struct drm_plane *plane,
> +                                 struct drm_crtc *crtc,
> +                                 struct drm_framebuffer *fb,
> +                                 struct drm_rect *src,
> +                                 struct drm_rect *dst,
> +                                 const struct drm_rect *clip,
> +                                 unsigned int rotation,
> +                                 int min_scale,
> +                                 int max_scale,
> +                                 bool can_position,
> +                                 bool can_update_disabled,
> +                                 bool *visible)
> +{
> +       struct drm_plane_state state = {
> +               .plane = plane,
> +               .crtc = crtc,
> +               .fb = fb,
> +               .src_x = src->x1,
> +               .src_y = src->x2,

This should be:
src->y1

> +               .src_w = drm_rect_width(src),
> +               .src_h = drm_rect_height(src),
> +               .crtc_x = dst->x1,
> +               .crtc_y = dst->x2,

And this should be:
dst->y1

Thanks,
-Dan
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 3/9] drm/plane-helper: Add drm_plane_helper_check_state()

[Ville Syrjälä]

On Mon, Aug 08, 2016 at 03:29:24PM +0800, Daniel Kurtz wrote:
> Hi Ville,
> 
> Two fixes inline...
> 
> On Wed, Jul 27, 2016 at 1:34 AM, <ville.syrjala@linux.intel.com> wrote:
> >
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Add a version of drm_plane_helper_check_update() which takes a plane
> > state instead of having the caller pass in everything.
> >
> > And to reduce code duplication, let's reimplement
> > drm_plane_helper_check_update() in terms of the new function, by
> > having a tempororary plane state on the stack.
> >
> > v2: Add a note that the functions modifies the state (Chris)
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> [snip...]
> 
> > +int drm_plane_helper_check_update(struct drm_plane *plane,
> > +                                 struct drm_crtc *crtc,
> > +                                 struct drm_framebuffer *fb,
> > +                                 struct drm_rect *src,
> > +                                 struct drm_rect *dst,
> > +                                 const struct drm_rect *clip,
> > +                                 unsigned int rotation,
> > +                                 int min_scale,
> > +                                 int max_scale,
> > +                                 bool can_position,
> > +                                 bool can_update_disabled,
> > +                                 bool *visible)
> > +{
> > +       struct drm_plane_state state = {
> > +               .plane = plane,
> > +               .crtc = crtc,
> > +               .fb = fb,
> > +               .src_x = src->x1,
> > +               .src_y = src->x2,
> 
> This should be:
> src->y1
> 
> > +               .src_w = drm_rect_width(src),
> > +               .src_h = drm_rect_height(src),
> > +               .crtc_x = dst->x1,
> > +               .crtc_y = dst->x2,
> 
> And this should be:
> dst->y1

Whoops. Copypaste fail, or something. Thanks for catching those.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 3/9] drm/plane-helper: Add drm_plane_helper_check_state()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a version of drm_plane_helper_check_update() which takes a plane
state instead of having the caller pass in everything.

And to reduce code duplication, let's reimplement
drm_plane_helper_check_update() in terms of the new function, by
having a tempororary plane state on the stack.

v2: Add a note that the functions modifies the state (Chris)
v3: Fix drm_plane_helper_check_update() y coordinates (Daniel Kurtz)

Cc: Daniel Kurtz <djkurtz@chromium.org>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Sean Paul <seanpaul@chromium.org> (v2)
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane_helper.c | 139 ++++++++++++++++++++++++++++---------
 include/drm/drm_plane_helper.h     |   5 ++
 2 files changed, 112 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index 16c4a7bd7465..0251aeec2bf3 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -108,14 +108,9 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
 }
 
 /**
- * drm_plane_helper_check_update() - Check plane update for validity
- * @plane: plane object to update
- * @crtc: owning CRTC of owning plane
- * @fb: framebuffer to flip onto plane
- * @src: source coordinates in 16.16 fixed point
- * @dest: integer destination coordinates
+ * drm_plane_helper_check_state() - Check plane state for validity
+ * @state: plane state to check
  * @clip: integer clipping coordinates
- * @rotation: plane rotation
  * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
  * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
  * @can_position: is it legal to position the plane such that it
@@ -123,10 +118,9 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
  *                only be false for primary planes.
  * @can_update_disabled: can the plane be updated while the crtc
  *                       is disabled?
- * @visible: output parameter indicating whether plane is still visible after
- *           clipping
  *
- * Checks that a desired plane update is valid.  Drivers that provide
+ * Checks that a desired plane update is valid, and updates various
+ * bits of derived state (clipped coordinates etc.). Drivers that provide
  * their own plane handling rather than helper-provided implementations may
  * still wish to call this function to avoid duplication of error checking
  * code.
@@ -134,29 +128,38 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
  * RETURNS:
  * Zero if update appears valid, error code on failure
  */
-int drm_plane_helper_check_update(struct drm_plane *plane,
-				  struct drm_crtc *crtc,
-				  struct drm_framebuffer *fb,
-				  struct drm_rect *src,
-				  struct drm_rect *dest,
-				  const struct drm_rect *clip,
-				  unsigned int rotation,
-				  int min_scale,
-				  int max_scale,
-				  bool can_position,
-				  bool can_update_disabled,
-				  bool *visible)
+int drm_plane_helper_check_state(struct drm_plane_state *state,
+				 const struct drm_rect *clip,
+				 int min_scale,
+				 int max_scale,
+				 bool can_position,
+				 bool can_update_disabled)
 {
+	struct drm_crtc *crtc = state->crtc;
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_rect *src = &state->src;
+	struct drm_rect *dst = &state->dst;
+	unsigned int rotation = state->rotation;
 	int hscale, vscale;
 
+	src->x1 = state->src_x;
+	src->y1 = state->src_y;
+	src->x2 = state->src_x + state->src_w;
+	src->y2 = state->src_y + state->src_h;
+
+	dst->x1 = state->crtc_x;
+	dst->y1 = state->crtc_y;
+	dst->x2 = state->crtc_x + state->crtc_w;
+	dst->y2 = state->crtc_y + state->crtc_h;
+
 	if (!fb) {
-		*visible = false;
+		state->visible = false;
 		return 0;
 	}
 
 	/* crtc should only be NULL when disabling (i.e., !fb) */
 	if (WARN_ON(!crtc)) {
-		*visible = false;
+		state->visible = false;
 		return 0;
 	}
 
@@ -168,20 +171,20 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
 	drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
 
 	/* Check scaling */
-	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
-	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
+	hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
+	vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
 	if (hscale < 0 || vscale < 0) {
 		DRM_DEBUG_KMS("Invalid scaling of plane\n");
-		drm_rect_debug_print("src: ", src, true);
-		drm_rect_debug_print("dst: ", dest, false);
+		drm_rect_debug_print("src: ", &state->src, true);
+		drm_rect_debug_print("dst: ", &state->dst, false);
 		return -ERANGE;
 	}
 
-	*visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale);
+	state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
 
 	drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
 
-	if (!*visible)
+	if (!state->visible)
 		/*
 		 * Plane isn't visible; some drivers can handle this
 		 * so we just return success here.  Drivers that can't
@@ -191,15 +194,87 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
 		 */
 		return 0;
 
-	if (!can_position && !drm_rect_equals(dest, clip)) {
+	if (!can_position && !drm_rect_equals(dst, clip)) {
 		DRM_DEBUG_KMS("Plane must cover entire CRTC\n");
-		drm_rect_debug_print("dst: ", dest, false);
+		drm_rect_debug_print("dst: ", dst, false);
 		drm_rect_debug_print("clip: ", clip, false);
 		return -EINVAL;
 	}
 
 	return 0;
 }
+EXPORT_SYMBOL(drm_plane_helper_check_state);
+
+/**
+ * drm_plane_helper_check_update() - Check plane update for validity
+ * @plane: plane object to update
+ * @crtc: owning CRTC of owning plane
+ * @fb: framebuffer to flip onto plane
+ * @src: source coordinates in 16.16 fixed point
+ * @dest: integer destination coordinates
+ * @clip: integer clipping coordinates
+ * @rotation: plane rotation
+ * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
+ * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
+ * @can_position: is it legal to position the plane such that it
+ *                doesn't cover the entire crtc?  This will generally
+ *                only be false for primary planes.
+ * @can_update_disabled: can the plane be updated while the crtc
+ *                       is disabled?
+ * @visible: output parameter indicating whether plane is still visible after
+ *           clipping
+ *
+ * Checks that a desired plane update is valid.  Drivers that provide
+ * their own plane handling rather than helper-provided implementations may
+ * still wish to call this function to avoid duplication of error checking
+ * code.
+ *
+ * RETURNS:
+ * Zero if update appears valid, error code on failure
+ */
+int drm_plane_helper_check_update(struct drm_plane *plane,
+				  struct drm_crtc *crtc,
+				  struct drm_framebuffer *fb,
+				  struct drm_rect *src,
+				  struct drm_rect *dst,
+				  const struct drm_rect *clip,
+				  unsigned int rotation,
+				  int min_scale,
+				  int max_scale,
+				  bool can_position,
+				  bool can_update_disabled,
+				  bool *visible)
+{
+	struct drm_plane_state state = {
+		.plane = plane,
+		.crtc = crtc,
+		.fb = fb,
+		.src_x = src->x1,
+		.src_y = src->y1,
+		.src_w = drm_rect_width(src),
+		.src_h = drm_rect_height(src),
+		.crtc_x = dst->x1,
+		.crtc_y = dst->y1,
+		.crtc_w = drm_rect_width(dst),
+		.crtc_h = drm_rect_height(dst),
+		.rotation = rotation,
+		.visible = *visible,
+	};
+	int ret;
+
+	ret = drm_plane_helper_check_state(&state, clip,
+					   min_scale, max_scale,
+					   can_position,
+					   can_update_disabled);
+	if (ret)
+		return ret;
+
+	*src = state.src;
+	*dst = state.dst;
+	*visible = state.visible;
+
+	return 0;
+}
 EXPORT_SYMBOL(drm_plane_helper_check_update);
 
 /**
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index 0e0c3573cce0..fbc8ecb3e5e8 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -40,6 +40,11 @@
 int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		  const struct drm_crtc_funcs *funcs);
 
+int drm_plane_helper_check_state(struct drm_plane_state *state,
+				 const struct drm_rect *clip,
+				 int min_scale, int max_scale,
+				 bool can_position,
+				 bool can_update_disabled);
 int drm_plane_helper_check_update(struct drm_plane *plane,
 				  struct drm_crtc *crtc,
 				  struct drm_framebuffer *fb,
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Ro.CI.BAT: failure for drm: Store clipped coordinates in drm_plane_state (rev3)

[Patchwork]

== Series Details ==

Series: drm: Store clipped coordinates in drm_plane_state (rev3)
URL   : https://patchwork.freedesktop.org/series/10279/
State : failure

== Summary ==

Applying: drm: Warn about negative sizes when calculating scale factor
Applying: drm: Store clipped src/dst coordinatee in drm_plane_state
Using index info to reconstruct a base tree...
M	include/drm/drm_crtc.h
Falling back to patching base and 3-way merge...
Auto-merging include/drm/drm_crtc.h
CONFLICT (content): Merge conflict in include/drm/drm_crtc.h
error: Failed to merge in the changes.
Patch failed at 0002 drm: Store clipped src/dst coordinatee in drm_plane_state
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

✗ Fi.CI.BAT: failure for drm: Store clipped coordinates in drm_plane_state (rev3)

[Patchwork]

== Series Details ==

Series: drm: Store clipped coordinates in drm_plane_state (rev3)
URL   : https://patchwork.freedesktop.org/series/10279/
State : failure

== Summary ==

  CC      drivers/gpio/gpiolib-acpi.o
  CC      drivers/input/mouse/alps.o
  CC      drivers/input/mouse/focaltech.o
  CC      drivers/input/mouse/byd.o
  CC      drivers/firmware/efi/libstub/gop.o
  CC      drivers/input/mouse/elantech.o
  CC      drivers/input/mouse/logips2pp.o
  CC      drivers/input/mouse/lifebook.o
  CC [M]  drivers/i2c/busses/i2c-designware-core.o
  CC      drivers/input/mouse/trackpoint.o
  CC [M]  drivers/i2c/busses/i2c-designware-platdrv.o
  CC      drivers/input/mouse/cypress_ps2.o
  CC [M]  drivers/i2c/busses/i2c-designware-pcidrv.o
  CC [M]  drivers/input/mouse/synaptics_usb.o
  LD      drivers/hid/usbhid/usbhid.o
  LD      drivers/hid/usbhid/built-in.o
  LD      drivers/hid/built-in.o
  AR      drivers/firmware/efi/libstub/lib.a
  LD      drivers/firmware/efi/built-in.o
  LD      drivers/firmware/built-in.o
  LD      drivers/gpio/built-in.o
  LD [M]  drivers/i2c/busses/i2c-designware-platform.o
  LD [M]  drivers/i2c/busses/i2c-designware-pci.o
  LD      drivers/i2c/built-in.o
  LD      drivers/input/mouse/psmouse.o
  LD      drivers/input/mouse/built-in.o
  LD      drivers/input/built-in.o
  LD      drivers/iommu/built-in.o
Makefile:975: recipe for target 'drivers' failed
make: *** [drivers] Error 2

Full logs at /archive/deploy/logs/CI_Patchwork_build_2250

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