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

Commit Message

Ville Syrjala July 26, 2016, 5:34 p.m. UTC

From: Ville Syrjälä <[e-mail]>

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 <[e-mail]>
Signed-off-by: Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/drm_plane_helper.c | 139 ++++++++++++++++++++++++++++---------
 include/drm/drm_plane_helper.h     |   5 ++
 2 files changed, 112 insertions(+), 32 deletions(-)

Comments

Sean Paul Aug. 1, 2016, 3:10 p.m. UTC | #1

On Tue, Jul 26, 2016 at 1:34 PM,  <[e-mail]> wrote:
> From: Ville Syrjälä <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Ville Syrjälä <[e-mail]>

Reviewed-by: Sean Paul <[e-mail]>

> ---
>  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
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Kurtz Aug. 8, 2016, 7:29 a.m. UTC | #2

Hi Ville,

Two fixes inline...

On Wed, Jul 27, 2016 at 1:34 AM, <[e-mail]> wrote:
>
> From: Ville Syrjälä <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Ville Syrjälä <[e-mail]>

[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

Ville Syrjala Aug. 8, 2016, 7:44 a.m. UTC | #3

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, <[e-mail]> wrote:
> >
> > From: Ville Syrjälä <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> 
> [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.

Patch

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,