diff mbox

[v2] drm: add a drm_atomic_helper_plane_check_update

Message ID 1436788242-19788-1-git-send-email-zhjwpku@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

John Hunter July 13, 2015, 11:50 a.m. UTC
From: Zhao Junwang <zhjwpku@gmail.com>

This is the equivalent helper to drm_plane_helper_check_update
for legacy drivers, but using atomic state to check things.

Motivated by the atomic conversion of the bochs driver.

v2: according to Daniel's comment
    -polish the kerneldoc comment to match the signatures
    -crtc->mode is legacy state, we need to look at crtc_state
     instead

    according to Maarten's comment
    -there is no need for can_update_disabled in the atomic world,
     so, we can delete that parameter

v3: according to Daniel's comment
    -this function call can't fail, add a WARN_ON
    -use drm_atomic_get_existing_crtc_state

    according to Maarten's comment
    -kill off the plane parameter and rename state to plane_state
    -do not handling NULL, i.e. no need to check plane_state in
     atomic.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
---
 drivers/gpu/drm/drm_atomic_helper.c |   56 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_plane_helper.c  |    6 ++++
 include/drm/drm_atomic_helper.h     |    5 ++++
 3 files changed, 67 insertions(+)

Comments

Maarten Lankhorst July 13, 2015, 11:59 a.m. UTC | #1
Op 13-07-15 om 13:50 schreef John Hunter:
> From: Zhao Junwang <zhjwpku@gmail.com>
>
> This is the equivalent helper to drm_plane_helper_check_update
> for legacy drivers, but using atomic state to check things.
>
> Motivated by the atomic conversion of the bochs driver.
>
> v2: according to Daniel's comment
>     -polish the kerneldoc comment to match the signatures
>     -crtc->mode is legacy state, we need to look at crtc_state
>      instead
>
>     according to Maarten's comment
>     -there is no need for can_update_disabled in the atomic world,
>      so, we can delete that parameter
>
> v3: according to Daniel's comment
>     -this function call can't fail, add a WARN_ON
>     -use drm_atomic_get_existing_crtc_state
>
>     according to Maarten's comment
>     -kill off the plane parameter and rename state to plane_state
>     -do not handling NULL, i.e. no need to check plane_state in
>      atomic.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Daniel Vetter July 13, 2015, 2:14 p.m. UTC | #2
On Mon, Jul 13, 2015 at 07:50:42PM +0800, John Hunter wrote:
> From: Zhao Junwang <zhjwpku@gmail.com>
> 
> This is the equivalent helper to drm_plane_helper_check_update
> for legacy drivers, but using atomic state to check things.
> 
> Motivated by the atomic conversion of the bochs driver.
> 
> v2: according to Daniel's comment
>     -polish the kerneldoc comment to match the signatures
>     -crtc->mode is legacy state, we need to look at crtc_state
>      instead
> 
>     according to Maarten's comment
>     -there is no need for can_update_disabled in the atomic world,
>      so, we can delete that parameter
> 
> v3: according to Daniel's comment
>     -this function call can't fail, add a WARN_ON
>     -use drm_atomic_get_existing_crtc_state
> 
>     according to Maarten's comment
>     -kill off the plane parameter and rename state to plane_state
>     -do not handling NULL, i.e. no need to check plane_state in
>      atomic.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c |   56 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_plane_helper.c  |    6 ++++
>  include/drm/drm_atomic_helper.h     |    5 ++++
>  3 files changed, 67 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 536ae4d..eaef689 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1336,6 +1336,62 @@ void drm_atomic_helper_swap_state(struct drm_device *dev,
>  EXPORT_SYMBOL(drm_atomic_helper_swap_state);
>  
>  /**
> + * drm_atomic_helper_plane_check_update
> + * @plane_state: drm plane state
> + * @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.
> + *
> + * This provides a helper to be used in a driver's plane atomic_check
> + * callback. It is the atomic equivalent of
> + * drm_plane_helper_check_update() and has the exact same semantics,
> + * except that it looks at the atomic CRTC state in the atomic update
> + * instead of legacy state directly embedded in struct &drm_crtc.
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_helper_plane_check_update(struct drm_plane_state *plane_state,
> +					 int min_scale,
> +					 int max_scale,
> +					 bool can_position,
> +					 bool *visible)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
> +							plane_state->crtc);

This function only returns NULL if the object is there, not an ERR_PTR.
Hence you only need to check for:

	if (WARN_ON(!crtc_state))
		return -EBUSY;

IS_ERR actually doesn't catch NULL pointers afaik, so this wouldn't catch
the one case we want it to catch. But not sure about that (IS_ERR is a bit
confusing to me).
-Daniel

> +	if (WARN_ON(IS_ERR(crtc_state)))
> +		return PTR_ERR(crtc_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,
> +	};
> +	const struct drm_rect clip = {
> +		.x2 = crtc_state->mode.hdisplay,
> +		.y2 = crtc_state->mode.vdisplay,
> +	};
> +
> +	return drm_plane_helper_check_update(plane_state->plane,
> +					     plane_state->crtc,
> +					     plane_state->fb,
> +					     &src, &dest, &clip,
> +					     min_scale, max_scale,
> +					     can_position, true, visible);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_plane_check_update);
> +
> +/**
>   * drm_atomic_helper_update_plane - Helper for primary plane update using atomic
>   * @plane: plane object to update
>   * @crtc: owning CRTC of owning plane
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index 10be2d2..e346fe2 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -125,6 +125,12 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
>   * still wish to call this function to avoid duplication of error checking
>   * code.
>   *
> + * Note: When converting over to atomic drivers you need to switch
> + * over to using drm_atomic_helper_plane_check_update() since only
> + * that correctly checks atomic state - this function here only looks
> + * at legacy state and hence will check against stale values in
> + * atomic updates.
> + *
>   * RETURNS:
>   * Zero if update appears valid, error code on failure
>   */
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index cc1fee8..eaa2544 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -64,6 +64,11 @@ void drm_atomic_helper_swap_state(struct drm_device *dev,
>  				  struct drm_atomic_state *state);
>  
>  /* implementations for legacy interfaces */
> +int drm_atomic_helper_plane_check_update(struct drm_plane_state *plane_state,
> +					 int min_scale,
> +					 int max_scale,
> +					 bool can_position,
> +					 bool *visible);
>  int drm_atomic_helper_update_plane(struct drm_plane *plane,
>  				   struct drm_crtc *crtc,
>  				   struct drm_framebuffer *fb,
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 536ae4d..eaef689 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1336,6 +1336,62 @@  void drm_atomic_helper_swap_state(struct drm_device *dev,
 EXPORT_SYMBOL(drm_atomic_helper_swap_state);
 
 /**
+ * drm_atomic_helper_plane_check_update
+ * @plane_state: drm plane state
+ * @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.
+ *
+ * This provides a helper to be used in a driver's plane atomic_check
+ * callback. It is the atomic equivalent of
+ * drm_plane_helper_check_update() and has the exact same semantics,
+ * except that it looks at the atomic CRTC state in the atomic update
+ * instead of legacy state directly embedded in struct &drm_crtc.
+ *
+ * RETURNS:
+ * Zero on success, error code on failure
+ */
+int drm_atomic_helper_plane_check_update(struct drm_plane_state *plane_state,
+					 int min_scale,
+					 int max_scale,
+					 bool can_position,
+					 bool *visible)
+{
+	struct drm_crtc_state *crtc_state;
+	crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
+							plane_state->crtc);
+	if (WARN_ON(IS_ERR(crtc_state)))
+		return PTR_ERR(crtc_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,
+	};
+	const struct drm_rect clip = {
+		.x2 = crtc_state->mode.hdisplay,
+		.y2 = crtc_state->mode.vdisplay,
+	};
+
+	return drm_plane_helper_check_update(plane_state->plane,
+					     plane_state->crtc,
+					     plane_state->fb,
+					     &src, &dest, &clip,
+					     min_scale, max_scale,
+					     can_position, true, visible);
+}
+EXPORT_SYMBOL(drm_atomic_helper_plane_check_update);
+
+/**
  * drm_atomic_helper_update_plane - Helper for primary plane update using atomic
  * @plane: plane object to update
  * @crtc: owning CRTC of owning plane
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index 10be2d2..e346fe2 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -125,6 +125,12 @@  static int get_connectors_for_crtc(struct drm_crtc *crtc,
  * still wish to call this function to avoid duplication of error checking
  * code.
  *
+ * Note: When converting over to atomic drivers you need to switch
+ * over to using drm_atomic_helper_plane_check_update() since only
+ * that correctly checks atomic state - this function here only looks
+ * at legacy state and hence will check against stale values in
+ * atomic updates.
+ *
  * RETURNS:
  * Zero if update appears valid, error code on failure
  */
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index cc1fee8..eaa2544 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -64,6 +64,11 @@  void drm_atomic_helper_swap_state(struct drm_device *dev,
 				  struct drm_atomic_state *state);
 
 /* implementations for legacy interfaces */
+int drm_atomic_helper_plane_check_update(struct drm_plane_state *plane_state,
+					 int min_scale,
+					 int max_scale,
+					 bool can_position,
+					 bool *visible);
 int drm_atomic_helper_update_plane(struct drm_plane *plane,
 				   struct drm_crtc *crtc,
 				   struct drm_framebuffer *fb,