[PATCH 1/4] drm: Check CRTC compatibility in setplane

[PATCH 1/4] drm: Check CRTC compatibility in setplane

[Matt Roper]

The DRM core setplane code should check that the plane is usable on the
specified CRTC before calling into the driver.

Prior to this patch, a plane's possible_crtcs field was purely
informational for userspace and was never actually verified at the
kernel level (aside from the primary plane helper).

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c         | 7 +++++++
 drivers/gpu/drm/drm_plane_helper.c | 6 ------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 461d19b..b6d6c04 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2140,6 +2140,13 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	}
 	crtc = obj_to_crtc(obj);
 
+	/* Check whether this plane is usable on this CRTC */
+	if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
+		DRM_DEBUG_KMS("Invalid crtc for plane\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
 	fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
 	if (!fb) {
 		DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index d966afa..b601233 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -137,12 +137,6 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
-	/* Primary planes are locked to their owning CRTC */
-	if (plane->possible_crtcs != drm_crtc_mask(crtc)) {
-		DRM_DEBUG_KMS("Cannot change primary plane CRTC\n");
-		return -EINVAL;
-	}
-
 	/* Disallow scaling */
 	src_w >>= 16;
 	src_h >>= 16;
-- 
1.8.5.1

[PATCH 2/4] drm/plane-helper: Add drm_primary_helper_check_update() (v2)

[Matt Roper]

Pull the parameter checking from drm_primary_helper_update() out into
its own function; drivers that provide their own setplane()
implementations rather than using the helper may still want to share
this parameter checking logic.

A few of the checks here were also updated based on suggestions by
Ville Syrjälä.

v2:
  - Pass src/dest/clip rects and min/max scaling down to helper to avoid
    duplication of effort between helper and drivers (suggested by
    Ville).
  - Allow caller to specify whether the primary plane should be
    updatable while the crtc is disabled.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_plane_helper.c | 123 ++++++++++++++++++++++++++++---------
 include/drm/drm_plane_helper.h     |  24 +++++++-
 2 files changed, 116 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index b601233..11e8b82 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -26,6 +26,7 @@
 #include <linux/list.h>
 #include <drm/drmP.h>
 #include <drm/drm_rect.h>
+#include <drm/drm_plane_helper.h>
 
 #define SUBPIXEL_MASK 0xffff
 
@@ -66,6 +67,77 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
 }
 
 /**
+ * drm_primary_helper_check_update() - Check primary 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
+ * @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 primary plane such that it
+ *                doesn't cover the entire crtc?
+ * @can_update_disabled: can the primary plane be updated while the crtc
+ *                       is disabled?
+ * @visible: output parameter indicating whether plane is still visible after
+ *           clipping
+ *
+ * Checks that a desired primary plane update is valid.  Drivers that provide
+ * their own primary plane handling 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_primary_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,
+				    int min_scale,
+				    int max_scale,
+				    bool can_position,
+				    bool can_update_disabled,
+				    bool *visible)
+{
+	int hscale, vscale;
+
+	if (!crtc->enabled && !can_update_disabled) {
+		DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n");
+		return -EINVAL;
+	}
+
+	/* Check scaling */
+	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
+	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
+	if (hscale < 0 || vscale < 0) {
+		DRM_DEBUG_KMS("Invalid scaling of primary plane\n");
+		return -ERANGE;
+	}
+
+	*visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale);
+	if (!visible)
+		/*
+		 * Primary plane isn't visible; some drivers can handle this
+		 * so we just return success here.  Drivers that can't
+		 * (including those that use the primary plane helper's
+		 * update function) will return an error from their
+		 * update_plane handler.
+		 */
+		return 0;
+
+	if (!can_position && !drm_rect_equals(dest, clip)) {
+		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_primary_helper_check_update);
+
+/**
  * drm_primary_helper_update() - Helper for primary plane update
  * @plane: plane object to update
  * @crtc: owning CRTC of owning plane
@@ -113,51 +185,42 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		.x = src_x >> 16,
 		.y = src_y >> 16,
 	};
+	struct drm_rect src = {
+		.x1 = src_x,
+		.y1 = src_y,
+		.x2 = src_x + src_w,
+		.y2 = src_y + src_h,
+	};
 	struct drm_rect dest = {
 		.x1 = crtc_x,
 		.y1 = crtc_y,
 		.x2 = crtc_x + crtc_w,
 		.y2 = crtc_y + crtc_h,
 	};
-	struct drm_rect clip = {
+	const struct drm_rect clip = {
 		.x2 = crtc->mode.hdisplay,
 		.y2 = crtc->mode.vdisplay,
 	};
 	struct drm_connector **connector_list;
 	int num_connectors, ret;
+	bool visible;
 
-	if (!crtc->enabled) {
-		DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n");
-		return -EINVAL;
-	}
-
-	/* Disallow subpixel positioning */
-	if ((src_x | src_y | src_w | src_h) & SUBPIXEL_MASK) {
-		DRM_DEBUG_KMS("Primary plane does not support subpixel positioning\n");
-		return -EINVAL;
-	}
-
-	/* Disallow scaling */
-	src_w >>= 16;
-	src_h >>= 16;
-	if (crtc_w != src_w || crtc_h != src_h) {
-		DRM_DEBUG_KMS("Can't scale primary plane\n");
-		return -EINVAL;
-	}
-
-	/* Make sure primary plane covers entire CRTC */
-	drm_rect_intersect(&dest, &clip);
-	if (dest.x1 != 0 || dest.y1 != 0 ||
-	    dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) {
-		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
-		return -EINVAL;
-	}
-
-	/* Framebuffer must be big enough to cover entire plane */
-	ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb);
+	ret = drm_primary_helper_check_update(plane, crtc, fb,
+					      &src, &dest, &clip,
+					      DRM_PLANE_HELPER_NO_SCALING,
+					      DRM_PLANE_HELPER_NO_SCALING,
+					      false, false, &visible);
 	if (ret)
 		return ret;
 
+	if (!visible)
+		/*
+		 * Primary plane isn't visible.  Note that unless a driver
+		 * provides their own disable function, this will just
+		 * wind up returning -EINVAL to userspace.
+		 */
+		return plane->funcs->disable_plane(plane);
+
 	/* Find current connectors for CRTC */
 	num_connectors = get_connectors_for_crtc(crtc, NULL, 0);
 	BUG_ON(num_connectors == 0);
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index 09824be..05e1357 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -24,6 +24,17 @@
 #ifndef DRM_PLANE_HELPER_H
 #define DRM_PLANE_HELPER_H
 
+#include <drm/drm_rect.h>
+
+/*
+ * Drivers that don't allow primary plane scaling may pass this macro in place
+ * of the min/max scale parameters of the update checker function.
+ *
+ * Due to src being in 16.16 fixed point and dest being in integer pixels,
+ * 1<<16 represents no scaling.
+ */
+#define DRM_PLANE_HELPER_NO_SCALING (1<<16)
+
 /**
  * DOC: plane helpers
  *
@@ -31,6 +42,17 @@
  * planes.
  */
 
+extern int drm_primary_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,
+					   int min_scale,
+					   int max_scale,
+					   bool can_position,
+					   bool can_update_disabled,
+					   bool *visible);
 extern int drm_primary_helper_update(struct drm_plane *plane,
 				     struct drm_crtc *crtc,
 				     struct drm_framebuffer *fb,
@@ -42,7 +64,7 @@ extern int drm_primary_helper_disable(struct drm_plane *plane);
 extern void drm_primary_helper_destroy(struct drm_plane *plane);
 extern const struct drm_plane_funcs drm_primary_helper_funcs;
 extern struct drm_plane *drm_primary_helper_create_plane(struct drm_device *dev,
-							 uint32_t *formats,
+							 const uint32_t *formats,
 							 int num_formats);
 
 
-- 
1.8.5.1

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

Re: [PATCH 2/4] drm/plane-helper: Add drm_primary_helper_check_update() (v2)

[Lee, Chon Ming]

On 04/30 10:07, Matt Roper wrote:
> Pull the parameter checking from drm_primary_helper_update() out into
> its own function; drivers that provide their own setplane()
> implementations rather than using the helper may still want to share
> this parameter checking logic.
> 
> A few of the checks here were also updated based on suggestions by
> Ville Syrjälä.
> 
> v2:
>   - Pass src/dest/clip rects and min/max scaling down to helper to avoid
>     duplication of effort between helper and drivers (suggested by
>     Ville).
>   - Allow caller to specify whether the primary plane should be
>     updatable while the crtc is disabled.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_plane_helper.c | 123 ++++++++++++++++++++++++++++---------
>  include/drm/drm_plane_helper.h     |  24 +++++++-
>  2 files changed, 116 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index b601233..11e8b82 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -26,6 +26,7 @@
>  #include <linux/list.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_rect.h>
> +#include <drm/drm_plane_helper.h>
>  
>  #define SUBPIXEL_MASK 0xffff
>  
> @@ -66,6 +67,77 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
>  }
>  
>  /**
> + * drm_primary_helper_check_update() - Check primary 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
> + * @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 primary plane such that it
> + *                doesn't cover the entire crtc?
> + * @can_update_disabled: can the primary plane be updated while the crtc
> + *                       is disabled?
> + * @visible: output parameter indicating whether plane is still visible after
> + *           clipping
> + *
> + * Checks that a desired primary plane update is valid.  Drivers that provide
> + * their own primary plane handling 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_primary_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,
> +				    int min_scale,
> +				    int max_scale,
> +				    bool can_position,
> +				    bool can_update_disabled,
> +				    bool *visible)
> +{
> +	int hscale, vscale;
> +
> +	if (!crtc->enabled && !can_update_disabled) {
> +		DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Check scaling */
> +	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> +	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> +	if (hscale < 0 || vscale < 0) {
> +		DRM_DEBUG_KMS("Invalid scaling of primary plane\n");
> +		return -ERANGE;
> +	}
> +
> +	*visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale);
> +	if (!visible)
> +		/*
> +		 * Primary plane isn't visible; some drivers can handle this
> +		 * so we just return success here.  Drivers that can't
> +		 * (including those that use the primary plane helper's
> +		 * update function) will return an error from their
> +		 * update_plane handler.
> +		 */
> +		return 0;
> +
> +	if (!can_position && !drm_rect_equals(dest, clip)) {
> +		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
> +		return -EINVAL;
> +	}

Cherryview display allow the primary plane to be position at any location
similiar to sprite plane for certain port. So, this shouldn't need to check here.  

And the width/height doesn't need to cover the whole screen.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_primary_helper_check_update);
> +
> +/**
>   * drm_primary_helper_update() - Helper for primary plane update
>   * @plane: plane object to update
>   * @crtc: owning CRTC of owning plane
> @@ -113,51 +185,42 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
>  		.x = src_x >> 16,
>  		.y = src_y >> 16,
>  	};
> +	struct drm_rect src = {
> +		.x1 = src_x,
> +		.y1 = src_y,
> +		.x2 = src_x + src_w,
> +		.y2 = src_y + src_h,
> +	};
>  	struct drm_rect dest = {
>  		.x1 = crtc_x,
>  		.y1 = crtc_y,
>  		.x2 = crtc_x + crtc_w,
>  		.y2 = crtc_y + crtc_h,
>  	};
> -	struct drm_rect clip = {
> +	const struct drm_rect clip = {
>  		.x2 = crtc->mode.hdisplay,
>  		.y2 = crtc->mode.vdisplay,
>  	};
>  	struct drm_connector **connector_list;
>  	int num_connectors, ret;
> +	bool visible;
>  
> -	if (!crtc->enabled) {
> -		DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n");
> -		return -EINVAL;
> -	}
> -
> -	/* Disallow subpixel positioning */
> -	if ((src_x | src_y | src_w | src_h) & SUBPIXEL_MASK) {
> -		DRM_DEBUG_KMS("Primary plane does not support subpixel positioning\n");
> -		return -EINVAL;
> -	}
> -
> -	/* Disallow scaling */
> -	src_w >>= 16;
> -	src_h >>= 16;
> -	if (crtc_w != src_w || crtc_h != src_h) {
> -		DRM_DEBUG_KMS("Can't scale primary plane\n");
> -		return -EINVAL;
> -	}
> -
> -	/* Make sure primary plane covers entire CRTC */
> -	drm_rect_intersect(&dest, &clip);
> -	if (dest.x1 != 0 || dest.y1 != 0 ||
> -	    dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) {
> -		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
> -		return -EINVAL;
> -	}
> -
> -	/* Framebuffer must be big enough to cover entire plane */
> -	ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb);
> +	ret = drm_primary_helper_check_update(plane, crtc, fb,
> +					      &src, &dest, &clip,
> +					      DRM_PLANE_HELPER_NO_SCALING,
> +					      DRM_PLANE_HELPER_NO_SCALING,
> +					      false, false, &visible);
>  	if (ret)
>  		return ret;
>  
> +	if (!visible)
> +		/*
> +		 * Primary plane isn't visible.  Note that unless a driver
> +		 * provides their own disable function, this will just
> +		 * wind up returning -EINVAL to userspace.
> +		 */
> +		return plane->funcs->disable_plane(plane);
> +
>  	/* Find current connectors for CRTC */
>  	num_connectors = get_connectors_for_crtc(crtc, NULL, 0);
>  	BUG_ON(num_connectors == 0);
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index 09824be..05e1357 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -24,6 +24,17 @@
>  #ifndef DRM_PLANE_HELPER_H
>  #define DRM_PLANE_HELPER_H
>  
> +#include <drm/drm_rect.h>
> +
> +/*
> + * Drivers that don't allow primary plane scaling may pass this macro in place
> + * of the min/max scale parameters of the update checker function.
> + *
> + * Due to src being in 16.16 fixed point and dest being in integer pixels,
> + * 1<<16 represents no scaling.
> + */
> +#define DRM_PLANE_HELPER_NO_SCALING (1<<16)
> +
>  /**
>   * DOC: plane helpers
>   *
> @@ -31,6 +42,17 @@
>   * planes.
>   */
>  
> +extern int drm_primary_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,
> +					   int min_scale,
> +					   int max_scale,
> +					   bool can_position,
> +					   bool can_update_disabled,
> +					   bool *visible);
>  extern int drm_primary_helper_update(struct drm_plane *plane,
>  				     struct drm_crtc *crtc,
>  				     struct drm_framebuffer *fb,
> @@ -42,7 +64,7 @@ extern int drm_primary_helper_disable(struct drm_plane *plane);
>  extern void drm_primary_helper_destroy(struct drm_plane *plane);
>  extern const struct drm_plane_funcs drm_primary_helper_funcs;
>  extern struct drm_plane *drm_primary_helper_create_plane(struct drm_device *dev,
> -							 uint32_t *formats,
> +							 const uint32_t *formats,
>  							 int num_formats);
>  
>  
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/plane-helper: Add drm_primary_helper_check_update() (v2)

[Rob Clark]

On Thu, May 15, 2014 at 10:51 PM, Lee, Chon Ming
<chon.ming.lee@intel.com> wrote:
> On 04/30 10:07, Matt Roper wrote:
>> Pull the parameter checking from drm_primary_helper_update() out into
>> its own function; drivers that provide their own setplane()
>> implementations rather than using the helper may still want to share
>> this parameter checking logic.
>>
>> A few of the checks here were also updated based on suggestions by
>> Ville Syrjälä.
>>
>> v2:
>>   - Pass src/dest/clip rects and min/max scaling down to helper to avoid
>>     duplication of effort between helper and drivers (suggested by
>>     Ville).
>>   - Allow caller to specify whether the primary plane should be
>>     updatable while the crtc is disabled.
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>  drivers/gpu/drm/drm_plane_helper.c | 123 ++++++++++++++++++++++++++++---------
>>  include/drm/drm_plane_helper.h     |  24 +++++++-
>>  2 files changed, 116 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
>> index b601233..11e8b82 100644
>> --- a/drivers/gpu/drm/drm_plane_helper.c
>> +++ b/drivers/gpu/drm/drm_plane_helper.c
>> @@ -26,6 +26,7 @@
>>  #include <linux/list.h>
>>  #include <drm/drmP.h>
>>  #include <drm/drm_rect.h>
>> +#include <drm/drm_plane_helper.h>
>>
>>  #define SUBPIXEL_MASK 0xffff
>>
>> @@ -66,6 +67,77 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
>>  }
>>
>>  /**
>> + * drm_primary_helper_check_update() - Check primary 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
>> + * @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 primary plane such that it
>> + *                doesn't cover the entire crtc?
>> + * @can_update_disabled: can the primary plane be updated while the crtc
>> + *                       is disabled?
>> + * @visible: output parameter indicating whether plane is still visible after
>> + *           clipping
>> + *
>> + * Checks that a desired primary plane update is valid.  Drivers that provide
>> + * their own primary plane handling 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_primary_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,
>> +                                 int min_scale,
>> +                                 int max_scale,
>> +                                 bool can_position,
>> +                                 bool can_update_disabled,
>> +                                 bool *visible)
>> +{
>> +     int hscale, vscale;
>> +
>> +     if (!crtc->enabled && !can_update_disabled) {
>> +             DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* Check scaling */
>> +     hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
>> +     vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
>> +     if (hscale < 0 || vscale < 0) {
>> +             DRM_DEBUG_KMS("Invalid scaling of primary plane\n");
>> +             return -ERANGE;
>> +     }
>> +
>> +     *visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale);
>> +     if (!visible)
>> +             /*
>> +              * Primary plane isn't visible; some drivers can handle this
>> +              * so we just return success here.  Drivers that can't
>> +              * (including those that use the primary plane helper's
>> +              * update function) will return an error from their
>> +              * update_plane handler.
>> +              */
>> +             return 0;
>> +
>> +     if (!can_position && !drm_rect_equals(dest, clip)) {
>> +             DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
>> +             return -EINVAL;
>> +     }
>
> Cherryview display allow the primary plane to be position at any location
> similiar to sprite plane for certain port. So, this shouldn't need to check here.
>
> And the width/height doesn't need to cover the whole screen.

In that case, wouldn't it make sense (at least when you want to expose
those features) to *not* use primary plane helpers for that hw?

IMHO, the primary plane helpers should be for "traditional" crtcs
which do not have these features..

BR,
-R

>
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL(drm_primary_helper_check_update);
>> +
>> +/**
>>   * drm_primary_helper_update() - Helper for primary plane update
>>   * @plane: plane object to update
>>   * @crtc: owning CRTC of owning plane
>> @@ -113,51 +185,42 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
>>               .x = src_x >> 16,
>>               .y = src_y >> 16,
>>       };
>> +     struct drm_rect src = {
>> +             .x1 = src_x,
>> +             .y1 = src_y,
>> +             .x2 = src_x + src_w,
>> +             .y2 = src_y + src_h,
>> +     };
>>       struct drm_rect dest = {
>>               .x1 = crtc_x,
>>               .y1 = crtc_y,
>>               .x2 = crtc_x + crtc_w,
>>               .y2 = crtc_y + crtc_h,
>>       };
>> -     struct drm_rect clip = {
>> +     const struct drm_rect clip = {
>>               .x2 = crtc->mode.hdisplay,
>>               .y2 = crtc->mode.vdisplay,
>>       };
>>       struct drm_connector **connector_list;
>>       int num_connectors, ret;
>> +     bool visible;
>>
>> -     if (!crtc->enabled) {
>> -             DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n");
>> -             return -EINVAL;
>> -     }
>> -
>> -     /* Disallow subpixel positioning */
>> -     if ((src_x | src_y | src_w | src_h) & SUBPIXEL_MASK) {
>> -             DRM_DEBUG_KMS("Primary plane does not support subpixel positioning\n");
>> -             return -EINVAL;
>> -     }
>> -
>> -     /* Disallow scaling */
>> -     src_w >>= 16;
>> -     src_h >>= 16;
>> -     if (crtc_w != src_w || crtc_h != src_h) {
>> -             DRM_DEBUG_KMS("Can't scale primary plane\n");
>> -             return -EINVAL;
>> -     }
>> -
>> -     /* Make sure primary plane covers entire CRTC */
>> -     drm_rect_intersect(&dest, &clip);
>> -     if (dest.x1 != 0 || dest.y1 != 0 ||
>> -         dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) {
>> -             DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
>> -             return -EINVAL;
>> -     }
>> -
>> -     /* Framebuffer must be big enough to cover entire plane */
>> -     ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb);
>> +     ret = drm_primary_helper_check_update(plane, crtc, fb,
>> +                                           &src, &dest, &clip,
>> +                                           DRM_PLANE_HELPER_NO_SCALING,
>> +                                           DRM_PLANE_HELPER_NO_SCALING,
>> +                                           false, false, &visible);
>>       if (ret)
>>               return ret;
>>
>> +     if (!visible)
>> +             /*
>> +              * Primary plane isn't visible.  Note that unless a driver
>> +              * provides their own disable function, this will just
>> +              * wind up returning -EINVAL to userspace.
>> +              */
>> +             return plane->funcs->disable_plane(plane);
>> +
>>       /* Find current connectors for CRTC */
>>       num_connectors = get_connectors_for_crtc(crtc, NULL, 0);
>>       BUG_ON(num_connectors == 0);
>> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
>> index 09824be..05e1357 100644
>> --- a/include/drm/drm_plane_helper.h
>> +++ b/include/drm/drm_plane_helper.h
>> @@ -24,6 +24,17 @@
>>  #ifndef DRM_PLANE_HELPER_H
>>  #define DRM_PLANE_HELPER_H
>>
>> +#include <drm/drm_rect.h>
>> +
>> +/*
>> + * Drivers that don't allow primary plane scaling may pass this macro in place
>> + * of the min/max scale parameters of the update checker function.
>> + *
>> + * Due to src being in 16.16 fixed point and dest being in integer pixels,
>> + * 1<<16 represents no scaling.
>> + */
>> +#define DRM_PLANE_HELPER_NO_SCALING (1<<16)
>> +
>>  /**
>>   * DOC: plane helpers
>>   *
>> @@ -31,6 +42,17 @@
>>   * planes.
>>   */
>>
>> +extern int drm_primary_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,
>> +                                        int min_scale,
>> +                                        int max_scale,
>> +                                        bool can_position,
>> +                                        bool can_update_disabled,
>> +                                        bool *visible);
>>  extern int drm_primary_helper_update(struct drm_plane *plane,
>>                                    struct drm_crtc *crtc,
>>                                    struct drm_framebuffer *fb,
>> @@ -42,7 +64,7 @@ extern int drm_primary_helper_disable(struct drm_plane *plane);
>>  extern void drm_primary_helper_destroy(struct drm_plane *plane);
>>  extern const struct drm_plane_funcs drm_primary_helper_funcs;
>>  extern struct drm_plane *drm_primary_helper_create_plane(struct drm_device *dev,
>> -                                                      uint32_t *formats,
>> +                                                      const uint32_t *formats,
>>                                                        int num_formats);
>>
>>
>> --
>> 1.8.5.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/plane-helper: Add drm_primary_helper_check_update() (v2)

[Lee, Chon Ming]

> -----Original Message-----
> From: Rob Clark [mailto:robdclark@gmail.com]
> Sent: Friday, May 16, 2014 11:05 AM
> To: Lee, Chon Ming
> Cc: Roper, Matthew D; Intel Graphics Development; dri-
> devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/4] drm/plane-helper: Add
> drm_primary_helper_check_update() (v2)
> 
> On Thu, May 15, 2014 at 10:51 PM, Lee, Chon Ming
> <chon.ming.lee@intel.com> wrote:
> > On 04/30 10:07, Matt Roper wrote:
> >> Pull the parameter checking from drm_primary_helper_update() out into
> >> its own function; drivers that provide their own setplane()
> >> implementations rather than using the helper may still want to share
> >> this parameter checking logic.
> >>
> >> A few of the checks here were also updated based on suggestions by
> >> Ville Syrjälä.
> >>
> >> v2:
> >>   - Pass src/dest/clip rects and min/max scaling down to helper to avoid
> >>     duplication of effort between helper and drivers (suggested by
> >>     Ville).
> >>   - Allow caller to specify whether the primary plane should be
> >>     updatable while the crtc is disabled.
> >>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >> ---
> >>  drivers/gpu/drm/drm_plane_helper.c | 123
> ++++++++++++++++++++++++++++---------
> >>  include/drm/drm_plane_helper.h     |  24 +++++++-
> >>  2 files changed, 116 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_plane_helper.c
> >> b/drivers/gpu/drm/drm_plane_helper.c
> >> index b601233..11e8b82 100644
> >> --- a/drivers/gpu/drm/drm_plane_helper.c
> >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> >> @@ -26,6 +26,7 @@
> >>  #include <linux/list.h>
> >>  #include <drm/drmP.h>
> >>  #include <drm/drm_rect.h>
> >> +#include <drm/drm_plane_helper.h>
> >>
> >>  #define SUBPIXEL_MASK 0xffff
> >>
> >> @@ -66,6 +67,77 @@ static int get_connectors_for_crtc(struct drm_crtc
> >> *crtc,  }
> >>
> >>  /**
> >> + * drm_primary_helper_check_update() - Check primary 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
> >> + * @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 primary plane such that it
> >> + *                doesn't cover the entire crtc?
> >> + * @can_update_disabled: can the primary plane be updated while the
> crtc
> >> + *                       is disabled?
> >> + * @visible: output parameter indicating whether plane is still visible
> after
> >> + *           clipping
> >> + *
> >> + * Checks that a desired primary plane update is valid.  Drivers
> >> +that provide
> >> + * their own primary plane handling 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_primary_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,
> >> +                                 int min_scale,
> >> +                                 int max_scale,
> >> +                                 bool can_position,
> >> +                                 bool can_update_disabled,
> >> +                                 bool *visible) {
> >> +     int hscale, vscale;
> >> +
> >> +     if (!crtc->enabled && !can_update_disabled) {
> >> +             DRM_DEBUG_KMS("Cannot update primary plane of a disabled
> CRTC.\n");
> >> +             return -EINVAL;
> >> +     }
> >> +
> >> +     /* Check scaling */
> >> +     hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> >> +     vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> >> +     if (hscale < 0 || vscale < 0) {
> >> +             DRM_DEBUG_KMS("Invalid scaling of primary plane\n");
> >> +             return -ERANGE;
> >> +     }
> >> +
> >> +     *visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale);
> >> +     if (!visible)
> >> +             /*
> >> +              * Primary plane isn't visible; some drivers can handle this
> >> +              * so we just return success here.  Drivers that can't
> >> +              * (including those that use the primary plane helper's
> >> +              * update function) will return an error from their
> >> +              * update_plane handler.
> >> +              */
> >> +             return 0;
> >> +
> >> +     if (!can_position && !drm_rect_equals(dest, clip)) {
> >> +             DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
> >> +             return -EINVAL;
> >> +     }
> >
> > Cherryview display allow the primary plane to be position at any
> > location similiar to sprite plane for certain port. So, this shouldn't need to
> check here.
> >
> > And the width/height doesn't need to cover the whole screen.
> 
> In that case, wouldn't it make sense (at least when you want to expose those
> features) to *not* use primary plane helpers for that hw?
> 
> IMHO, the primary plane helpers should be for "traditional" crtcs which do
> not have these features..
> 
I don't have the usage model for that windowing the primary plane now.   Let say a primary plane at the beginning is using in traditional way, but if it want to switch to window mode,  it might not make sense to create another plane to handle this right?

Regards,
Chon Ming

> BR,
> -R
> 
> >
> >> +
> >> +     return 0;
> >> +}
> >> +EXPORT_SYMBOL(drm_primary_helper_check_update);
> >> +
> >> +/**
> >>   * drm_primary_helper_update() - Helper for primary plane update
> >>   * @plane: plane object to update
> >>   * @crtc: owning CRTC of owning plane @@ -113,51 +185,42 @@ int
> >> drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc
> *crtc,
> >>               .x = src_x >> 16,
> >>               .y = src_y >> 16,
> >>       };
> >> +     struct drm_rect src = {
> >> +             .x1 = src_x,
> >> +             .y1 = src_y,
> >> +             .x2 = src_x + src_w,
> >> +             .y2 = src_y + src_h,
> >> +     };
> >>       struct drm_rect dest = {
> >>               .x1 = crtc_x,
> >>               .y1 = crtc_y,
> >>               .x2 = crtc_x + crtc_w,
> >>               .y2 = crtc_y + crtc_h,
> >>       };
> >> -     struct drm_rect clip = {
> >> +     const struct drm_rect clip = {
> >>               .x2 = crtc->mode.hdisplay,
> >>               .y2 = crtc->mode.vdisplay,
> >>       };
> >>       struct drm_connector **connector_list;
> >>       int num_connectors, ret;
> >> +     bool visible;
> >>
> >> -     if (!crtc->enabled) {
> >> -             DRM_DEBUG_KMS("Cannot update primary plane of a disabled
> CRTC.\n");
> >> -             return -EINVAL;
> >> -     }
> >> -
> >> -     /* Disallow subpixel positioning */
> >> -     if ((src_x | src_y | src_w | src_h) & SUBPIXEL_MASK) {
> >> -             DRM_DEBUG_KMS("Primary plane does not support subpixel
> positioning\n");
> >> -             return -EINVAL;
> >> -     }
> >> -
> >> -     /* Disallow scaling */
> >> -     src_w >>= 16;
> >> -     src_h >>= 16;
> >> -     if (crtc_w != src_w || crtc_h != src_h) {
> >> -             DRM_DEBUG_KMS("Can't scale primary plane\n");
> >> -             return -EINVAL;
> >> -     }
> >> -
> >> -     /* Make sure primary plane covers entire CRTC */
> >> -     drm_rect_intersect(&dest, &clip);
> >> -     if (dest.x1 != 0 || dest.y1 != 0 ||
> >> -         dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) {
> >> -             DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
> >> -             return -EINVAL;
> >> -     }
> >> -
> >> -     /* Framebuffer must be big enough to cover entire plane */
> >> -     ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb);
> >> +     ret = drm_primary_helper_check_update(plane, crtc, fb,
> >> +                                           &src, &dest, &clip,
> >> +                                           DRM_PLANE_HELPER_NO_SCALING,
> >> +                                           DRM_PLANE_HELPER_NO_SCALING,
> >> +                                           false, false, &visible);
> >>       if (ret)
> >>               return ret;
> >>
> >> +     if (!visible)
> >> +             /*
> >> +              * Primary plane isn't visible.  Note that unless a driver
> >> +              * provides their own disable function, this will just
> >> +              * wind up returning -EINVAL to userspace.
> >> +              */
> >> +             return plane->funcs->disable_plane(plane);
> >> +
> >>       /* Find current connectors for CRTC */
> >>       num_connectors = get_connectors_for_crtc(crtc, NULL, 0);
> >>       BUG_ON(num_connectors == 0);
> >> diff --git a/include/drm/drm_plane_helper.h
> >> b/include/drm/drm_plane_helper.h index 09824be..05e1357 100644
> >> --- a/include/drm/drm_plane_helper.h
> >> +++ b/include/drm/drm_plane_helper.h
> >> @@ -24,6 +24,17 @@
> >>  #ifndef DRM_PLANE_HELPER_H
> >>  #define DRM_PLANE_HELPER_H
> >>
> >> +#include <drm/drm_rect.h>
> >> +
> >> +/*
> >> + * Drivers that don't allow primary plane scaling may pass this
> >> +macro in place
> >> + * of the min/max scale parameters of the update checker function.
> >> + *
> >> + * Due to src being in 16.16 fixed point and dest being in integer
> >> +pixels,
> >> + * 1<<16 represents no scaling.
> >> + */
> >> +#define DRM_PLANE_HELPER_NO_SCALING (1<<16)
> >> +
> >>  /**
> >>   * DOC: plane helpers
> >>   *
> >> @@ -31,6 +42,17 @@
> >>   * planes.
> >>   */
> >>
> >> +extern int drm_primary_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,
> >> +                                        int min_scale,
> >> +                                        int max_scale,
> >> +                                        bool can_position,
> >> +                                        bool can_update_disabled,
> >> +                                        bool *visible);
> >>  extern int drm_primary_helper_update(struct drm_plane *plane,
> >>                                    struct drm_crtc *crtc,
> >>                                    struct drm_framebuffer *fb, @@
> >> -42,7 +64,7 @@ extern int drm_primary_helper_disable(struct drm_plane
> >> *plane);  extern void drm_primary_helper_destroy(struct drm_plane
> >> *plane);  extern const struct drm_plane_funcs
> >> drm_primary_helper_funcs;  extern struct drm_plane
> *drm_primary_helper_create_plane(struct drm_device *dev,
> >> -                                                      uint32_t *formats,
> >> +                                                      const uint32_t
> >> + *formats,
> >>                                                        int
> >> num_formats);
> >>
> >>
> >> --
> >> 1.8.5.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/plane-helper: Add drm_primary_helper_check_update() (v2)

[Daniel Vetter]

On Fri, May 16, 2014 at 7:25 AM, Lee, Chon Ming <chon.ming.lee@intel.com> wrote:
>> > Cherryview display allow the primary plane to be position at any
>> > location similiar to sprite plane for certain port. So, this shouldn't need to
>> check here.
>> >
>> > And the width/height doesn't need to cover the whole screen.
>>
>> In that case, wouldn't it make sense (at least when you want to expose those
>> features) to *not* use primary plane helpers for that hw?
>>
>> IMHO, the primary plane helpers should be for "traditional" crtcs which do
>> not have these features..
>>
> I don't have the usage model for that windowing the primary plane now.   Let say a primary plane at the beginning is using in traditional way, but if it want to switch to window mode,  it might not make sense to create another plane to handle this right?

I think what Rob means is that for now we should just use the primary
plane helpers everywhere for easier conversion. Then later on we can
enable the primary plane position feature on chv. So doing things
step-by-step.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/4] drm/plane-helper: Add drm_primary_helper_check_update() (v2)

[Matt Roper]

On Fri, May 16, 2014 at 10:51:44AM +0800, Lee, Chon Ming wrote:
...
> > +int drm_primary_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,
> > +				    int min_scale,
> > +				    int max_scale,
> > +				    bool can_position,
> > +				    bool can_update_disabled,
> > +				    bool *visible)
> > +{
> > +	int hscale, vscale;
> > +
> > +	if (!crtc->enabled && !can_update_disabled) {
> > +		DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Check scaling */
> > +	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> > +	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> > +	if (hscale < 0 || vscale < 0) {
> > +		DRM_DEBUG_KMS("Invalid scaling of primary plane\n");
> > +		return -ERANGE;
> > +	}
> > +
> > +	*visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale);
> > +	if (!visible)
> > +		/*
> > +		 * Primary plane isn't visible; some drivers can handle this
> > +		 * so we just return success here.  Drivers that can't
> > +		 * (including those that use the primary plane helper's
> > +		 * update function) will return an error from their
> > +		 * update_plane handler.
> > +		 */
> > +		return 0;
> > +
> > +	if (!can_position && !drm_rect_equals(dest, clip)) {
> > +		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
> > +		return -EINVAL;
> > +	}
> 
> Cherryview display allow the primary plane to be position at any location
> similiar to sprite plane for certain port. So, this shouldn't need to check here.  
> 
> And the width/height doesn't need to cover the whole screen.

Right, but this is a general helper function in the DRM core that is
meant to be usable on all hardware and on all vendors' drivers
(including the simple primary planes that are automatically created by
helper functions for drivers that don't provide their own primary plane
support).  The goal here is to centralize the common parameter checking
in one place so that all drivers don't have to duplicate the same set of
checks, but give a little bit of flexibility so that drivers for more
feature-rich hardware can relax some of the restrictions that their
hardware can actually handle (such as Cherryview being able to do
primary plane windowing as you pointed out).

It's true that the i915-specific implementation could be further
extended to pass true for the 'can_position' parameter when running on
Cherrytrail and then program the hardware accordingly, but that's really
an extra feature beyond what I'm adding here; we'd want to add that as a
follow-on patch later and come up with a whole extra set of tests to
exercise it.  I'd rather focus on getting this general i915 support here
merged first, then go back and start enabling new hardware
functionalities like that on the newer platforms that can handle it.

I'll add Cherryview primary plane windowing to my TODO list for future
work if nobody beats me to it (I think some of the guys in VPG may
already be looking into this).


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

[PATCH 2/4] drm/plane-helper: Add drm_plane_helper_check_update() (v3)

[Matt Roper]

Pull the parameter checking from drm_primary_helper_update() out into
its own function; drivers that provide their own setplane()
implementations rather than using the helper may still want to share
this parameter checking logic.

A few of the checks here were also updated based on suggestions by
Ville Syrjälä.

v3:
 - s/primary_helper/plane_helper/ --- this checking logic may be useful
   for other types of planes as well.
 - Fix visibility check (need to dereference visibility pointer)
v2:
 - Pass src/dest/clip rects and min/max scaling down to helper to avoid
   duplication of effort between helper and drivers (suggested by
   Ville).
 - Allow caller to specify whether the primary plane should be
   updatable while the crtc is disabled.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_plane_helper.c | 124 ++++++++++++++++++++++++++++---------
 include/drm/drm_plane_helper.h     |  22 +++++++
 2 files changed, 116 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index a255bea..83c8f0b 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -67,6 +67,79 @@ 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
+ * @clip: integer clipping coordinates
+ * @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 *dest,
+				    const struct drm_rect *clip,
+				    int min_scale,
+				    int max_scale,
+				    bool can_position,
+				    bool can_update_disabled,
+				    bool *visible)
+{
+	int hscale, vscale;
+
+	if (!crtc->enabled && !can_update_disabled) {
+		DRM_DEBUG_KMS("Cannot update plane of a disabled CRTC.\n");
+		return -EINVAL;
+	}
+
+	/* Check scaling */
+	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
+	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
+	if (hscale < 0 || vscale < 0) {
+		DRM_DEBUG_KMS("Invalid scaling of plane\n");
+		return -ERANGE;
+	}
+
+	*visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale);
+	if (!*visible)
+		/*
+		 * Plane isn't visible; some drivers can handle this
+		 * so we just return success here.  Drivers that can't
+		 * (including those that use the primary plane helper's
+		 * update function) will return an error from their
+		 * update_plane handler.
+		 */
+		return 0;
+
+	if (!can_position && !drm_rect_equals(dest, clip)) {
+		DRM_DEBUG_KMS("Plane must cover entire CRTC\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_plane_helper_check_update);
+
+/**
  * drm_primary_helper_update() - Helper for primary plane update
  * @plane: plane object to update
  * @crtc: owning CRTC of owning plane
@@ -114,51 +187,42 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		.x = src_x >> 16,
 		.y = src_y >> 16,
 	};
+	struct drm_rect src = {
+		.x1 = src_x,
+		.y1 = src_y,
+		.x2 = src_x + src_w,
+		.y2 = src_y + src_h,
+	};
 	struct drm_rect dest = {
 		.x1 = crtc_x,
 		.y1 = crtc_y,
 		.x2 = crtc_x + crtc_w,
 		.y2 = crtc_y + crtc_h,
 	};
-	struct drm_rect clip = {
+	const struct drm_rect clip = {
 		.x2 = crtc->mode.hdisplay,
 		.y2 = crtc->mode.vdisplay,
 	};
 	struct drm_connector **connector_list;
 	int num_connectors, ret;
+	bool visible;
 
-	if (!crtc->enabled) {
-		DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n");
-		return -EINVAL;
-	}
-
-	/* Disallow subpixel positioning */
-	if ((src_x | src_y | src_w | src_h) & SUBPIXEL_MASK) {
-		DRM_DEBUG_KMS("Primary plane does not support subpixel positioning\n");
-		return -EINVAL;
-	}
-
-	/* Disallow scaling */
-	src_w >>= 16;
-	src_h >>= 16;
-	if (crtc_w != src_w || crtc_h != src_h) {
-		DRM_DEBUG_KMS("Can't scale primary plane\n");
-		return -EINVAL;
-	}
-
-	/* Make sure primary plane covers entire CRTC */
-	drm_rect_intersect(&dest, &clip);
-	if (dest.x1 != 0 || dest.y1 != 0 ||
-	    dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) {
-		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
-		return -EINVAL;
-	}
-
-	/* Framebuffer must be big enough to cover entire plane */
-	ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb);
+	ret = drm_plane_helper_check_update(plane, crtc, fb,
+					    &src, &dest, &clip,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    false, false, &visible);
 	if (ret)
 		return ret;
 
+	if (!visible)
+		/*
+		 * Primary plane isn't visible.  Note that unless a driver
+		 * provides their own disable function, this will just
+		 * wind up returning -EINVAL to userspace.
+		 */
+		return plane->funcs->disable_plane(plane);
+
 	/* Find current connectors for CRTC */
 	num_connectors = get_connectors_for_crtc(crtc, NULL, 0);
 	BUG_ON(num_connectors == 0);
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index c5e7ab9..52e6870 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -24,6 +24,17 @@
 #ifndef DRM_PLANE_HELPER_H
 #define DRM_PLANE_HELPER_H
 
+#include <drm/drm_rect.h>
+
+/*
+ * Drivers that don't allow primary plane scaling may pass this macro in place
+ * of the min/max scale parameters of the update checker function.
+ *
+ * Due to src being in 16.16 fixed point and dest being in integer pixels,
+ * 1<<16 represents no scaling.
+ */
+#define DRM_PLANE_HELPER_NO_SCALING (1<<16)
+
 /**
  * DOC: plane helpers
  *
@@ -31,6 +42,17 @@
  * planes.
  */
 
+extern 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,
+					 int min_scale,
+					 int max_scale,
+					 bool can_position,
+					 bool can_update_disabled,
+					 bool *visible);
 extern int drm_primary_helper_update(struct drm_plane *plane,
 				     struct drm_crtc *crtc,
 				     struct drm_framebuffer *fb,
-- 
1.8.5.1

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