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,  wrote:
> >
> > From: Ville Syrjälä 
> >
> > 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 
> > Signed-off-by: Ville Syrjälä 
> 
> [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
___
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()

[Daniel Kurtz]

Hi Ville,

Two fixes inline...

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

[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()

[Sean Paul]

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

Reviewed-by: Sean Paul 

> ---
>  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 = >src;
> +   struct drm_rect *dst = >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 = 

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

[ville . syrjala]

From: Ville Syrjälä 

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 
Signed-off-by: Ville Syrjälä 
---
 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 = >src;
+   struct drm_rect *dst = >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