[PATCH 0/4] Intel primary plane support (v6)

[PATCH 0/4] Intel primary plane support (v6)

[Matt Roper]

This iteration of the patch series fixes up some conflicts between primary
plane disabling and other KMS operations (i-g-t tests will be sent shortly).
It also refactors the plane update parameter checking into a new DRM core
primary plane helper function.  Unlike the previous iteration of the patch, the
helper now takes src, dest, and clip rects provided by the driver, which
eliminates a lot of unnecessary duplication of work between the helper and the
driver.

The previous iteration of this patch series is at
   http://lists.freedesktop.org/archives/intel-gfx/2014-April/044069.html


Matt Roper (4):
  drm: Check CRTC compatibility in setplane
  drm/plane-helper: Add drm_primary_helper_check_update() (v2)
  drm/i915: don't force full modeset if primary plane is disabled
  drm/i915: Intel-specific primary plane handling (v6)

 drivers/gpu/drm/drm_crtc.c           |   7 ++
 drivers/gpu/drm/drm_plane_helper.c   | 129 +++++++++++++++------
 drivers/gpu/drm/i915/intel_display.c | 216 ++++++++++++++++++++++++++++++++++-
 include/drm/drm_plane_helper.h       |  24 +++-
 4 files changed, 334 insertions(+), 42 deletions(-)

-- 
1.8.5.1

[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

[PATCH 3/4] drm/i915: don't force full modeset if primary plane is disabled

[Matt Roper]

In a future patch, we'll allow the primary plane to be disabled by
userspace via the universal plane API.  If a modeset is requested while
the primary plane is disabled, crtc->primary->fb will be NULL which
generally triggers a full modeset (except in fastboot situations).  If
we detect that the crtc is active, but there's no primary plane fb,
we should still allow a simple plane update rather than a full modeset
if the mode isn't actually changing (after re-enabling the primary plane
of course).

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e1e1239..04bd821 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10136,12 +10136,17 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
 	if (is_crtc_connector_off(set)) {
 		config->mode_changed = true;
 	} else if (set->crtc->primary->fb != set->fb) {
-		/* If we have no fb then treat it as a full mode set */
+		/*
+		 * If we have no fb, we can only flip as long as the crtc is
+		 * active, otherwise we need a full mode set.  The crtc may
+		 * be active if we've only disabled the primary plane, or
+		 * in fastboot situations.
+		 */
 		if (set->crtc->primary->fb == NULL) {
 			struct intel_crtc *intel_crtc =
 				to_intel_crtc(set->crtc);
 
-			if (intel_crtc->active && i915.fastboot) {
+			if (intel_crtc->active) {
 				DRM_DEBUG_KMS("crtc has no fb, will flip\n");
 				config->fb_changed = true;
 			} else {
@@ -10380,8 +10385,21 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 		ret = intel_set_mode(set->crtc, set->mode,
 				     set->x, set->y, set->fb);
 	} else if (config->fb_changed) {
+		struct drm_i915_private *dev_priv = dev->dev_private;
+		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
+
 		intel_crtc_wait_for_pending_flips(set->crtc);
 
+		/*
+		 * We need to make sure the primary plane is re-enabled if it
+		 * has previously been turned off.
+		 */
+		if (!intel_crtc->primary_enabled) {
+			BUG_ON(!intel_crtc->active);
+			intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
+						      intel_crtc->pipe);
+		}
+
 		ret = intel_pipe_set_base(set->crtc,
 					  set->x, set->y, set->fb);
 		/*
-- 
1.8.5.1

Re: [PATCH 3/4] drm/i915: don't force full modeset if primary plane is disabled

[Ville Syrjälä]

On Wed, Apr 30, 2014 at 10:07:02AM -0700, Matt Roper wrote:
> In a future patch, we'll allow the primary plane to be disabled by
> userspace via the universal plane API.  If a modeset is requested while
> the primary plane is disabled, crtc->primary->fb will be NULL which
> generally triggers a full modeset (except in fastboot situations).  If
> we detect that the crtc is active, but there's no primary plane fb,
> we should still allow a simple plane update rather than a full modeset
> if the mode isn't actually changing (after re-enabling the primary plane
> of course).
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e1e1239..04bd821 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10136,12 +10136,17 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
>  	if (is_crtc_connector_off(set)) {
>  		config->mode_changed = true;
>  	} else if (set->crtc->primary->fb != set->fb) {
> -		/* If we have no fb then treat it as a full mode set */
> +		/*
> +		 * If we have no fb, we can only flip as long as the crtc is
> +		 * active, otherwise we need a full mode set.  The crtc may
> +		 * be active if we've only disabled the primary plane, or
> +		 * in fastboot situations.
> +		 */
>  		if (set->crtc->primary->fb == NULL) {
>  			struct intel_crtc *intel_crtc =
>  				to_intel_crtc(set->crtc);
>  
> -			if (intel_crtc->active && i915.fastboot) {
> +			if (intel_crtc->active) {
>  				DRM_DEBUG_KMS("crtc has no fb, will flip\n");
>  				config->fb_changed = true;
>  			} else {
> @@ -10380,8 +10385,21 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  		ret = intel_set_mode(set->crtc, set->mode,
>  				     set->x, set->y, set->fb);
>  	} else if (config->fb_changed) {
> +		struct drm_i915_private *dev_priv = dev->dev_private;
> +		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
> +
>  		intel_crtc_wait_for_pending_flips(set->crtc);
>  
> +		/*
> +		 * We need to make sure the primary plane is re-enabled if it
> +		 * has previously been turned off.
> +		 */
> +		if (!intel_crtc->primary_enabled) {
> +			BUG_ON(!intel_crtc->active);

BUG seems a bit harsh.

> +			intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> +						      intel_crtc->pipe);
> +		}
> +
>  		ret = intel_pipe_set_base(set->crtc,
>  					  set->x, set->y, set->fb);

Enable the plane after set_base to avoid lighting up the plane if
set_base fails, and it'll also make the flip+enable atomic and so
we avoid flashing the old fb at the user.

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

-- 
Ville Syrjälä
Intel OTC

[PATCH 3/4] drm/i915: don't force full modeset if primary plane is disabled (v2)

[Matt Roper]

In a future patch, we'll allow the primary plane to be disabled by
userspace via the universal plane API.  If a modeset is requested while
the primary plane is disabled, crtc->primary->fb will be NULL which
generally triggers a full modeset (except in fastboot situations).  If
we detect that the crtc is active, but there's no primary plane fb,
we should still allow a simple plane update rather than a full modeset
if the mode isn't actually changing (after re-enabling the primary plane
of course).

v2:
 - Enable plane after set_base to avoid enabling the plane if set_base
   fails, and to make flip+enable atomic (suggested by Ville)
 - Drop BUG to WARN if we somehow enter the 'fb_changed' modeset case
   with the crtc disabled (suggested by Ville)

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0f8f9bc..3aedc64 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10417,12 +10417,17 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
 	if (is_crtc_connector_off(set)) {
 		config->mode_changed = true;
 	} else if (set->crtc->primary->fb != set->fb) {
-		/* If we have no fb then treat it as a full mode set */
+		/*
+		 * If we have no fb, we can only flip as long as the crtc is
+		 * active, otherwise we need a full mode set.  The crtc may
+		 * be active if we've only disabled the primary plane, or
+		 * in fastboot situations.
+		 */
 		if (set->crtc->primary->fb == NULL) {
 			struct intel_crtc *intel_crtc =
 				to_intel_crtc(set->crtc);
 
-			if (intel_crtc->active && i915.fastboot) {
+			if (intel_crtc->active) {
 				DRM_DEBUG_KMS("crtc has no fb, will flip\n");
 				config->fb_changed = true;
 			} else {
@@ -10660,10 +10665,24 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 		ret = intel_set_mode(set->crtc, set->mode,
 				     set->x, set->y, set->fb);
 	} else if (config->fb_changed) {
+		struct drm_i915_private *dev_priv = dev->dev_private;
+		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
+
 		intel_crtc_wait_for_pending_flips(set->crtc);
 
 		ret = intel_pipe_set_base(set->crtc,
 					  set->x, set->y, set->fb);
+
+		/*
+		 * We need to make sure the primary plane is re-enabled if it
+		 * has previously been turned off.
+		 */
+		if (!intel_crtc->primary_enabled && ret == 0) {
+			WARN_ON(!intel_crtc->active);
+			intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
+						      intel_crtc->pipe);
+		}
+
 		/*
 		 * In the fastboot case this may be our only check of the
 		 * state after boot.  It would be better to only do it on
-- 
1.8.5.1

[PATCH 4/4] drm/i915: Intel-specific primary plane handling (v6)

[Matt Roper]

Intel hardware allows the primary plane to be disabled independently of
the CRTC.  Provide custom primary plane handling to allow this.

v6:
 - Pass rectangles to primary helper check function and get plane
   visibility back.
 - Wait for pending pageflips on primary plane update/disable.
 - Allow primary plane to be updated while the crtc is disabled (changes
   will take effect when the crtc is re-enabled if modeset passes -1
   for the fb id).
 - Drop WARN() if we try to disable the primary plane when it's
   already been disabled.  This will happen if the crtc gets disabled
   after the primary plane has already been disabled independently.
v5:
 - Use new drm_primary_helper_check_update() helper function to check
   setplane parameter validity.
 - Swap primary plane's pipe for pre-gen4 FBC (caught by Ville Syrjälä)
 - Cleanup primary plane properly on crtc init failure
v4:
 - Don't add a primary_plane field to intel_crtc; that was left over
   from a much earlier iteration of this patch series, but is no longer
   needed/used now that the DRM core primary plane support has been
   merged.
v3:
 - Provide gen-specific primary plane format lists (suggested by Daniel
   Vetter).
 - If the primary plane is already enabled, go ahead and just call the
   primary plane helper to do the update (suggested by Daniel Vetter).
 - Don't try to disable the primary plane on destruction; the DRM layer
   should have already taken care of this for us.
v2:
 - Unpin fb properly on primary plane disable
 - Provide an Intel-specific set of primary plane formats
 - Additional sanity checks on setplane (in line with the checks
   currently being done by the DRM core primary plane helper)

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 194 ++++++++++++++++++++++++++++++++++-
 1 file changed, 191 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 04bd821..33ed52d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -39,8 +39,35 @@
 #include "i915_trace.h"
 #include <drm/drm_dp_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_rect.h>
 #include <linux/dma_remapping.h>
 
+/* Primary plane formats supported by all gen */
+#define COMMON_PRIMARY_FORMATS \
+	DRM_FORMAT_C8, \
+	DRM_FORMAT_RGB565, \
+	DRM_FORMAT_XRGB8888, \
+	DRM_FORMAT_ARGB8888
+
+/* Primary plane formats for gen <= 3 */
+static const uint32_t intel_primary_formats_gen3[] = {
+	COMMON_PRIMARY_FORMATS,
+	DRM_FORMAT_XRGB1555,
+	DRM_FORMAT_ARGB1555,
+};
+
+/* Primary plane formats for gen >= 4 */
+static const uint32_t intel_primary_formats_gen4[] = {
+	COMMON_PRIMARY_FORMATS, \
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_XRGB2101010,
+	DRM_FORMAT_ARGB2101010,
+	DRM_FORMAT_XBGR2101010,
+	DRM_FORMAT_ABGR2101010,
+};
+
 static void intel_increase_pllclock(struct drm_crtc *crtc);
 static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
 
@@ -1909,7 +1936,8 @@ static void intel_disable_primary_hw_plane(struct drm_i915_private *dev_priv,
 	int reg;
 	u32 val;
 
-	WARN(!intel_crtc->primary_enabled, "Primary plane already disabled\n");
+	if (!intel_crtc->primary_enabled)
+		return;
 
 	intel_crtc->primary_enabled = false;
 
@@ -10551,17 +10579,177 @@ static void intel_shared_dpll_init(struct drm_device *dev)
 	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
 }
 
+static int
+intel_primary_plane_disable(struct drm_plane *plane)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc;
+
+	if (!plane->fb)
+		return 0;
+
+	BUG_ON(!plane->crtc);
+
+	intel_crtc = to_intel_crtc(plane->crtc);
+
+	/*
+	 * Even though we checked plane->fb above, it's still possible that
+	 * the primary plane has been implicitly disabled because we detected
+	 * that it was 100% covered by a sprite plane.  Or, the CRTC may be
+	 * off and we've set a fb, but haven't actually turned on the CRTC yet.
+	 * In either case, we need to unpin the FB and let the fb pointer get
+	 * updated, but otherwise we don't need to touch the hardware.
+	 */
+	if (!intel_crtc->primary_enabled)
+		goto disable_unpin;
+
+	intel_crtc_wait_for_pending_flips(plane->crtc);
+	intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
+				       intel_plane->pipe);
+
+disable_unpin:
+	/*
+	 * N.B. The DRM setplane code will update the plane->fb pointer after
+	 * we finish here.
+	 */
+	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+
+	return 0;
+}
+
+static int
+intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
+			     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+			     unsigned int crtc_w, unsigned int crtc_h,
+			     uint32_t src_x, uint32_t src_y,
+			     uint32_t src_w, uint32_t src_h)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_rect dest = {
+		/* integer pixels */
+		.x1 = crtc_x,
+		.y1 = crtc_y,
+		.x2 = crtc_x + crtc_w,
+		.y2 = crtc_y + crtc_h,
+	};
+	struct drm_rect src = {
+		/* 16.16 fixed point */
+		.x1 = src_x,
+		.y1 = src_y,
+		.x2 = src_x + src_w,
+		.y2 = src_y + src_h,
+	};
+	const struct drm_rect clip = {
+		/* integer pixels */
+		.x2 = intel_crtc->config.pipe_src_w,
+		.y2 = intel_crtc->config.pipe_src_h,
+	};
+	bool visible;
+	int ret;
+
+	ret = drm_primary_helper_check_update(plane, crtc, fb,
+					      &src, &dest, &clip,
+					      DRM_PLANE_HELPER_NO_SCALING,
+					      DRM_PLANE_HELPER_NO_SCALING,
+					      false, true, &visible);
+	if (ret)
+		return ret;
+
+	if (!visible)
+		return intel_primary_plane_disable(plane);
+
+	/*
+	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
+	 * updating the fb pointer, and returning without touching the
+	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
+	 * turn on the display with all planes setup as desired.
+	 */
+	if (!crtc->enabled)
+		/* Pin and return without programming hardware */
+		return intel_pin_and_fence_fb_obj(dev,
+						  to_intel_framebuffer(fb)->obj,
+						  NULL);
+
+	intel_crtc_wait_for_pending_flips(crtc);
+	ret = intel_pipe_set_base(crtc, src.x1, src.y1, fb);
+	if (ret)
+		return ret;
+
+	if (!intel_crtc->primary_enabled)
+		intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
+					      intel_crtc->pipe);
+
+	return 0;
+}
+
+static void intel_primary_plane_destroy(struct drm_plane *plane)
+{
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	drm_plane_cleanup(plane);
+	kfree(intel_plane);
+}
+
+static const struct drm_plane_funcs intel_primary_plane_funcs = {
+	.update_plane = intel_primary_plane_setplane,
+	.disable_plane = intel_primary_plane_disable,
+	.destroy = intel_primary_plane_destroy,
+};
+
+static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
+						    int pipe)
+{
+	struct intel_plane *primary;
+	const uint32_t *intel_primary_formats;
+	int num_formats;
+
+	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
+	if (primary == NULL)
+		return NULL;
+
+	primary->can_scale = false;
+	primary->max_downscale = 1;
+	primary->pipe = pipe;
+	primary->plane = pipe;
+	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
+		primary->plane = !pipe;
+
+	if (INTEL_INFO(dev)->gen <= 3) {
+		intel_primary_formats = intel_primary_formats_gen3;
+		num_formats = ARRAY_SIZE(intel_primary_formats_gen3);
+	} else {
+		intel_primary_formats = intel_primary_formats_gen4;
+		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
+	}
+
+	drm_plane_init(dev, &primary->base, 0,
+		       &intel_primary_plane_funcs, intel_primary_formats,
+		       num_formats, DRM_PLANE_TYPE_PRIMARY);
+	return &primary->base;
+}
+
 static void intel_crtc_init(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc;
-	int i;
+	struct drm_plane *primary;
+	int i, ret;
 
 	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
 	if (intel_crtc == NULL)
 		return;
 
-	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
+	primary = intel_primary_plane_create(dev, pipe);
+	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
+					NULL, &intel_crtc_funcs);
+	if (ret) {
+		drm_plane_cleanup(primary);
+		kfree(intel_crtc);
+		return;
+	}
 
 	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
 	for (i = 0; i < 256; i++) {
-- 
1.8.5.1

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

Re: [PATCH 4/4] drm/i915: Intel-specific primary plane handling (v6)

[Ville Syrjälä]

On Wed, Apr 30, 2014 at 10:07:03AM -0700, Matt Roper wrote:
> Intel hardware allows the primary plane to be disabled independently of
> the CRTC.  Provide custom primary plane handling to allow this.
> 
> v6:
>  - Pass rectangles to primary helper check function and get plane
>    visibility back.
>  - Wait for pending pageflips on primary plane update/disable.
>  - Allow primary plane to be updated while the crtc is disabled (changes
>    will take effect when the crtc is re-enabled if modeset passes -1
>    for the fb id).
>  - Drop WARN() if we try to disable the primary plane when it's
>    already been disabled.  This will happen if the crtc gets disabled
>    after the primary plane has already been disabled independently.
> v5:
>  - Use new drm_primary_helper_check_update() helper function to check
>    setplane parameter validity.
>  - Swap primary plane's pipe for pre-gen4 FBC (caught by Ville Syrjälä)
>  - Cleanup primary plane properly on crtc init failure
> v4:
>  - Don't add a primary_plane field to intel_crtc; that was left over
>    from a much earlier iteration of this patch series, but is no longer
>    needed/used now that the DRM core primary plane support has been
>    merged.
> v3:
>  - Provide gen-specific primary plane format lists (suggested by Daniel
>    Vetter).
>  - If the primary plane is already enabled, go ahead and just call the
>    primary plane helper to do the update (suggested by Daniel Vetter).
>  - Don't try to disable the primary plane on destruction; the DRM layer
>    should have already taken care of this for us.
> v2:
>  - Unpin fb properly on primary plane disable
>  - Provide an Intel-specific set of primary plane formats
>  - Additional sanity checks on setplane (in line with the checks
>    currently being done by the DRM core primary plane helper)
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 194 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 191 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 04bd821..33ed52d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -39,8 +39,35 @@
>  #include "i915_trace.h"
>  #include <drm/drm_dp_helper.h>
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_plane_helper.h>
> +#include <drm/drm_rect.h>
>  #include <linux/dma_remapping.h>
>  
> +/* Primary plane formats supported by all gen */
> +#define COMMON_PRIMARY_FORMATS \
> +	DRM_FORMAT_C8, \
> +	DRM_FORMAT_RGB565, \
> +	DRM_FORMAT_XRGB8888, \
> +	DRM_FORMAT_ARGB8888
> +
> +/* Primary plane formats for gen <= 3 */
> +static const uint32_t intel_primary_formats_gen3[] = {

nit: I'd call it _gen2 since we usually name things based on the oldest
thing that supports it.

> +	COMMON_PRIMARY_FORMATS,
> +	DRM_FORMAT_XRGB1555,
> +	DRM_FORMAT_ARGB1555,
> +};
> +
> +/* Primary plane formats for gen >= 4 */
> +static const uint32_t intel_primary_formats_gen4[] = {
> +	COMMON_PRIMARY_FORMATS, \
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_XRGB2101010,
> +	DRM_FORMAT_ARGB2101010,
> +	DRM_FORMAT_XBGR2101010,
> +	DRM_FORMAT_ABGR2101010,
> +};
> +
>  static void intel_increase_pllclock(struct drm_crtc *crtc);
>  static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
>  
> @@ -1909,7 +1936,8 @@ static void intel_disable_primary_hw_plane(struct drm_i915_private *dev_priv,
>  	int reg;
>  	u32 val;
>  
> -	WARN(!intel_crtc->primary_enabled, "Primary plane already disabled\n");
> +	if (!intel_crtc->primary_enabled)
> +		return;
>  
>  	intel_crtc->primary_enabled = false;
>  
> @@ -10551,17 +10579,177 @@ static void intel_shared_dpll_init(struct drm_device *dev)
>  	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
>  }
>  
> +static int
> +intel_primary_plane_disable(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc;
> +
> +	if (!plane->fb)
> +		return 0;
> +
> +	BUG_ON(!plane->crtc);
> +
> +	intel_crtc = to_intel_crtc(plane->crtc);
> +
> +	/*
> +	 * Even though we checked plane->fb above, it's still possible that
> +	 * the primary plane has been implicitly disabled because we detected
> +	 * that it was 100% covered by a sprite plane.  Or, the CRTC may be
> +	 * off and we've set a fb, but haven't actually turned on the CRTC yet.
> +	 * In either case, we need to unpin the FB and let the fb pointer get
> +	 * updated, but otherwise we don't need to touch the hardware.
> +	 */
> +	if (!intel_crtc->primary_enabled)
> +		goto disable_unpin;
> +
> +	intel_crtc_wait_for_pending_flips(plane->crtc);
> +	intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
> +				       intel_plane->pipe);
> +
> +disable_unpin:
> +	/*
> +	 * N.B. The DRM setplane code will update the plane->fb pointer after
> +	 * we finish here.
> +	 */
> +	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
> +
> +	return 0;
> +}
> +
> +static int
> +intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
> +			     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> +			     unsigned int crtc_w, unsigned int crtc_h,
> +			     uint32_t src_x, uint32_t src_y,
> +			     uint32_t src_w, uint32_t src_h)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_rect dest = {
> +		/* integer pixels */
> +		.x1 = crtc_x,
> +		.y1 = crtc_y,
> +		.x2 = crtc_x + crtc_w,
> +		.y2 = crtc_y + crtc_h,
> +	};
> +	struct drm_rect src = {
> +		/* 16.16 fixed point */
> +		.x1 = src_x,
> +		.y1 = src_y,
> +		.x2 = src_x + src_w,
> +		.y2 = src_y + src_h,
> +	};
> +	const struct drm_rect clip = {
> +		/* integer pixels */
> +		.x2 = intel_crtc->config.pipe_src_w,
> +		.y2 = intel_crtc->config.pipe_src_h,

These might contain garbage if the pipe isn't enabled. The sprite code
initializes the clip size to 0 in that case, which neatly results in
visible==false always.

> +	};
> +	bool visible;
> +	int ret;
> +
> +	ret = drm_primary_helper_check_update(plane, crtc, fb,
> +					      &src, &dest, &clip,
> +					      DRM_PLANE_HELPER_NO_SCALING,
> +					      DRM_PLANE_HELPER_NO_SCALING,
> +					      false, true, &visible);
> +	if (ret)
> +		return ret;
> +
> +	if (!visible)
> +		return intel_primary_plane_disable(plane);

Here we unpin the old fb...

> +
> +	/*
> +	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
> +	 * updating the fb pointer, and returning without touching the
> +	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
> +	 * turn on the display with all planes setup as desired.
> +	 */
> +	if (!crtc->enabled)
> +		/* Pin and return without programming hardware */
> +		return intel_pin_and_fence_fb_obj(dev,
> +						  to_intel_framebuffer(fb)->obj,
> +						  NULL);

...but here we just pin the new fb and leave the old also pinned?

Something's a bit fishy here. Also a non-enabled crtc but visible primary
plane doesn't seem to make sense. We also need to remember that set_base
will always unpin the old_fb. So if something can result in set_base
being called w/ old_fb!=NULL when it's not pinned, we'll be in deep
doodoo.

> +
> +	intel_crtc_wait_for_pending_flips(crtc);
> +	ret = intel_pipe_set_base(crtc, src.x1, src.y1, fb);
> +	if (ret)
> +		return ret;
> +
> +	if (!intel_crtc->primary_enabled)
> +		intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> +					      intel_crtc->pipe);
> +
> +	return 0;
> +}
> +
> +static void intel_primary_plane_destroy(struct drm_plane *plane)
> +{
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	drm_plane_cleanup(plane);
> +	kfree(intel_plane);
> +}
> +
> +static const struct drm_plane_funcs intel_primary_plane_funcs = {
> +	.update_plane = intel_primary_plane_setplane,
> +	.disable_plane = intel_primary_plane_disable,
> +	.destroy = intel_primary_plane_destroy,
> +};
> +
> +static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> +						    int pipe)
> +{
> +	struct intel_plane *primary;
> +	const uint32_t *intel_primary_formats;
> +	int num_formats;
> +
> +	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> +	if (primary == NULL)
> +		return NULL;
> +
> +	primary->can_scale = false;
> +	primary->max_downscale = 1;
> +	primary->pipe = pipe;
> +	primary->plane = pipe;
> +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> +		primary->plane = !pipe;
> +
> +	if (INTEL_INFO(dev)->gen <= 3) {
> +		intel_primary_formats = intel_primary_formats_gen3;
> +		num_formats = ARRAY_SIZE(intel_primary_formats_gen3);
> +	} else {
> +		intel_primary_formats = intel_primary_formats_gen4;
> +		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
> +	}
> +
> +	drm_plane_init(dev, &primary->base, 0,
> +		       &intel_primary_plane_funcs, intel_primary_formats,
> +		       num_formats, DRM_PLANE_TYPE_PRIMARY);
> +	return &primary->base;
> +}
> +
>  static void intel_crtc_init(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc;
> -	int i;
> +	struct drm_plane *primary;
> +	int i, ret;
>  
>  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
>  	if (intel_crtc == NULL)
>  		return;
>  
> -	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
> +	primary = intel_primary_plane_create(dev, pipe);
> +	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> +					NULL, &intel_crtc_funcs);
> +	if (ret) {
> +		drm_plane_cleanup(primary);
> +		kfree(intel_crtc);
> +		return;
> +	}
>  
>  	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
>  	for (i = 0; i < 256; i++) {
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 4/4] drm/i915: Intel-specific primary plane handling (v6)

[Matt Roper]

On Thu, May 15, 2014 at 06:52:28PM +0300, Ville Syrjälä wrote:
...
> 
> > +	};
> > +	bool visible;
> > +	int ret;
> > +
> > +	ret = drm_primary_helper_check_update(plane, crtc, fb,
> > +					      &src, &dest, &clip,
> > +					      DRM_PLANE_HELPER_NO_SCALING,
> > +					      DRM_PLANE_HELPER_NO_SCALING,
> > +					      false, true, &visible);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!visible)
> > +		return intel_primary_plane_disable(plane);
> 
> Here we unpin the old fb...
> 
> > +
> > +	/*
> > +	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
> > +	 * updating the fb pointer, and returning without touching the
> > +	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
> > +	 * turn on the display with all planes setup as desired.
> > +	 */
> > +	if (!crtc->enabled)
> > +		/* Pin and return without programming hardware */
> > +		return intel_pin_and_fence_fb_obj(dev,
> > +						  to_intel_framebuffer(fb)->obj,
> > +						  NULL);
> 
> ...but here we just pin the new fb and leave the old also pinned?
> 
> Something's a bit fishy here. Also a non-enabled crtc but visible primary
> plane doesn't seem to make sense. We also need to remember that set_base
> will always unpin the old_fb. So if something can result in set_base
> being called w/ old_fb!=NULL when it's not pinned, we'll be in deep
> doodoo.

Right, I guess we need to unpin the old fb, if any, here as well in case
they perform several setplane() calls while the crtc is disabled.

Eventually the crtc will get reenabled by a drmModeSetCrtc call.  If we
do setcrtc(fb = -1), then it should keep whatever fb we've already
pinned via the setplane.  If they provide a new fb, then the pinning
we're doing here will get unpinned by the set_base that gets called.

I don't see a way that you can hit set_base with an unpinned
old_fb!=NULL (since the disable plane case farther up also clears
primary->fb).


Matt

> 
> > +
> > +	intel_crtc_wait_for_pending_flips(crtc);
> > +	ret = intel_pipe_set_base(crtc, src.x1, src.y1, fb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!intel_crtc->primary_enabled)
> > +		intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> > +					      intel_crtc->pipe);
> > +
> > +	return 0;
> > +}
> > +
> > +static void intel_primary_plane_destroy(struct drm_plane *plane)
> > +{
> > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > +	drm_plane_cleanup(plane);
> > +	kfree(intel_plane);
> > +}
> > +
> > +static const struct drm_plane_funcs intel_primary_plane_funcs = {
> > +	.update_plane = intel_primary_plane_setplane,
> > +	.disable_plane = intel_primary_plane_disable,
> > +	.destroy = intel_primary_plane_destroy,
> > +};
> > +
> > +static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> > +						    int pipe)
> > +{
> > +	struct intel_plane *primary;
> > +	const uint32_t *intel_primary_formats;
> > +	int num_formats;
> > +
> > +	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> > +	if (primary == NULL)
> > +		return NULL;
> > +
> > +	primary->can_scale = false;
> > +	primary->max_downscale = 1;
> > +	primary->pipe = pipe;
> > +	primary->plane = pipe;
> > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > +		primary->plane = !pipe;
> > +
> > +	if (INTEL_INFO(dev)->gen <= 3) {
> > +		intel_primary_formats = intel_primary_formats_gen3;
> > +		num_formats = ARRAY_SIZE(intel_primary_formats_gen3);
> > +	} else {
> > +		intel_primary_formats = intel_primary_formats_gen4;
> > +		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
> > +	}
> > +
> > +	drm_plane_init(dev, &primary->base, 0,
> > +		       &intel_primary_plane_funcs, intel_primary_formats,
> > +		       num_formats, DRM_PLANE_TYPE_PRIMARY);
> > +	return &primary->base;
> > +}
> > +
> >  static void intel_crtc_init(struct drm_device *dev, int pipe)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc;
> > -	int i;
> > +	struct drm_plane *primary;
> > +	int i, ret;
> >  
> >  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
> >  	if (intel_crtc == NULL)
> >  		return;
> >  
> > -	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
> > +	primary = intel_primary_plane_create(dev, pipe);
> > +	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> > +					NULL, &intel_crtc_funcs);
> > +	if (ret) {
> > +		drm_plane_cleanup(primary);
> > +		kfree(intel_crtc);
> > +		return;
> > +	}
> >  
> >  	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
> >  	for (i = 0; i < 256; i++) {
> > -- 
> > 1.8.5.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

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

Re: [PATCH 4/4] drm/i915: Intel-specific primary plane handling (v6)

[Ville Syrjälä]

On Thu, May 15, 2014 at 09:37:55AM -0700, Matt Roper wrote:
> On Thu, May 15, 2014 at 06:52:28PM +0300, Ville Syrjälä wrote:
> ...
> > 
> > > +	};
> > > +	bool visible;
> > > +	int ret;
> > > +
> > > +	ret = drm_primary_helper_check_update(plane, crtc, fb,
> > > +					      &src, &dest, &clip,
> > > +					      DRM_PLANE_HELPER_NO_SCALING,
> > > +					      DRM_PLANE_HELPER_NO_SCALING,
> > > +					      false, true, &visible);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (!visible)
> > > +		return intel_primary_plane_disable(plane);
> > 
> > Here we unpin the old fb...
> > 
> > > +
> > > +	/*
> > > +	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
> > > +	 * updating the fb pointer, and returning without touching the
> > > +	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
> > > +	 * turn on the display with all planes setup as desired.
> > > +	 */
> > > +	if (!crtc->enabled)
> > > +		/* Pin and return without programming hardware */
> > > +		return intel_pin_and_fence_fb_obj(dev,
> > > +						  to_intel_framebuffer(fb)->obj,
> > > +						  NULL);
> > 
> > ...but here we just pin the new fb and leave the old also pinned?
> > 
> > Something's a bit fishy here. Also a non-enabled crtc but visible primary
> > plane doesn't seem to make sense. We also need to remember that set_base
> > will always unpin the old_fb. So if something can result in set_base
> > being called w/ old_fb!=NULL when it's not pinned, we'll be in deep
> > doodoo.
> 
> Right, I guess we need to unpin the old fb, if any, here as well in case
> they perform several setplane() calls while the crtc is disabled.
> 
> Eventually the crtc will get reenabled by a drmModeSetCrtc call.  If we
> do setcrtc(fb = -1), then it should keep whatever fb we've already
> pinned via the setplane.  If they provide a new fb, then the pinning
> we're doing here will get unpinned by the set_base that gets called.
> 
> I don't see a way that you can hit set_base with an unpinned
> old_fb!=NULL (since the disable plane case farther up also clears
> primary->fb).

But it doesn't.


> 
> 
> Matt
> 
> > 
> > > +
> > > +	intel_crtc_wait_for_pending_flips(crtc);
> > > +	ret = intel_pipe_set_base(crtc, src.x1, src.y1, fb);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (!intel_crtc->primary_enabled)
> > > +		intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> > > +					      intel_crtc->pipe);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void intel_primary_plane_destroy(struct drm_plane *plane)
> > > +{
> > > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > +	drm_plane_cleanup(plane);
> > > +	kfree(intel_plane);
> > > +}
> > > +
> > > +static const struct drm_plane_funcs intel_primary_plane_funcs = {
> > > +	.update_plane = intel_primary_plane_setplane,
> > > +	.disable_plane = intel_primary_plane_disable,
> > > +	.destroy = intel_primary_plane_destroy,
> > > +};
> > > +
> > > +static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> > > +						    int pipe)
> > > +{
> > > +	struct intel_plane *primary;
> > > +	const uint32_t *intel_primary_formats;
> > > +	int num_formats;
> > > +
> > > +	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> > > +	if (primary == NULL)
> > > +		return NULL;
> > > +
> > > +	primary->can_scale = false;
> > > +	primary->max_downscale = 1;
> > > +	primary->pipe = pipe;
> > > +	primary->plane = pipe;
> > > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > > +		primary->plane = !pipe;
> > > +
> > > +	if (INTEL_INFO(dev)->gen <= 3) {
> > > +		intel_primary_formats = intel_primary_formats_gen3;
> > > +		num_formats = ARRAY_SIZE(intel_primary_formats_gen3);
> > > +	} else {
> > > +		intel_primary_formats = intel_primary_formats_gen4;
> > > +		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
> > > +	}
> > > +
> > > +	drm_plane_init(dev, &primary->base, 0,
> > > +		       &intel_primary_plane_funcs, intel_primary_formats,
> > > +		       num_formats, DRM_PLANE_TYPE_PRIMARY);
> > > +	return &primary->base;
> > > +}
> > > +
> > >  static void intel_crtc_init(struct drm_device *dev, int pipe)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	struct intel_crtc *intel_crtc;
> > > -	int i;
> > > +	struct drm_plane *primary;
> > > +	int i, ret;
> > >  
> > >  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
> > >  	if (intel_crtc == NULL)
> > >  		return;
> > >  
> > > -	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
> > > +	primary = intel_primary_plane_create(dev, pipe);
> > > +	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> > > +					NULL, &intel_crtc_funcs);
> > > +	if (ret) {
> > > +		drm_plane_cleanup(primary);
> > > +		kfree(intel_crtc);
> > > +		return;
> > > +	}
> > >  
> > >  	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
> > >  	for (i = 0; i < 256; i++) {
> > > -- 
> > > 1.8.5.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 4/4] drm/i915: Intel-specific primary plane handling (v6)

[Matt Roper]

On Thu, May 15, 2014 at 08:00:48PM +0300, Ville Syrjälä wrote:
> On Thu, May 15, 2014 at 09:37:55AM -0700, Matt Roper wrote:
> > On Thu, May 15, 2014 at 06:52:28PM +0300, Ville Syrjälä wrote:
> > ...
> > > 
> > > > +	};
> > > > +	bool visible;
> > > > +	int ret;
> > > > +
> > > > +	ret = drm_primary_helper_check_update(plane, crtc, fb,
> > > > +					      &src, &dest, &clip,
> > > > +					      DRM_PLANE_HELPER_NO_SCALING,
> > > > +					      DRM_PLANE_HELPER_NO_SCALING,
> > > > +					      false, true, &visible);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	if (!visible)
> > > > +		return intel_primary_plane_disable(plane);
> > > 
> > > Here we unpin the old fb...
> > > 
> > > > +
> > > > +	/*
> > > > +	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
> > > > +	 * updating the fb pointer, and returning without touching the
> > > > +	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
> > > > +	 * turn on the display with all planes setup as desired.
> > > > +	 */
> > > > +	if (!crtc->enabled)
> > > > +		/* Pin and return without programming hardware */
> > > > +		return intel_pin_and_fence_fb_obj(dev,
> > > > +						  to_intel_framebuffer(fb)->obj,
> > > > +						  NULL);
> > > 
> > > ...but here we just pin the new fb and leave the old also pinned?
> > > 
> > > Something's a bit fishy here. Also a non-enabled crtc but visible primary
> > > plane doesn't seem to make sense. We also need to remember that set_base
> > > will always unpin the old_fb. So if something can result in set_base
> > > being called w/ old_fb!=NULL when it's not pinned, we'll be in deep
> > > doodoo.
> > 
> > Right, I guess we need to unpin the old fb, if any, here as well in case
> > they perform several setplane() calls while the crtc is disabled.
> > 
> > Eventually the crtc will get reenabled by a drmModeSetCrtc call.  If we
> > do setcrtc(fb = -1), then it should keep whatever fb we've already
> > pinned via the setplane.  If they provide a new fb, then the pinning
> > we're doing here will get unpinned by the set_base that gets called.
> > 
> > I don't see a way that you can hit set_base with an unpinned
> > old_fb!=NULL (since the disable plane case farther up also clears
> > primary->fb).
> 
> But it doesn't.
> 

Ah, you're right.  I was conflating explicit disables (fb=0) with
implicit disables (clipped to invisible).  I think the v7 I just sent
should handle this properly...for the implicit disable case we leave the
fb pinned and pointed to by primary->fb.  So when we switch to another
fb (or explicitly disable with fb=0), we should unpin it properly.


Matt

> 
> > 
> > 
> > Matt
> > 
> > > 
> > > > +
> > > > +	intel_crtc_wait_for_pending_flips(crtc);
> > > > +	ret = intel_pipe_set_base(crtc, src.x1, src.y1, fb);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	if (!intel_crtc->primary_enabled)
> > > > +		intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> > > > +					      intel_crtc->pipe);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void intel_primary_plane_destroy(struct drm_plane *plane)
> > > > +{
> > > > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > > +	drm_plane_cleanup(plane);
> > > > +	kfree(intel_plane);
> > > > +}
> > > > +
> > > > +static const struct drm_plane_funcs intel_primary_plane_funcs = {
> > > > +	.update_plane = intel_primary_plane_setplane,
> > > > +	.disable_plane = intel_primary_plane_disable,
> > > > +	.destroy = intel_primary_plane_destroy,
> > > > +};
> > > > +
> > > > +static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> > > > +						    int pipe)
> > > > +{
> > > > +	struct intel_plane *primary;
> > > > +	const uint32_t *intel_primary_formats;
> > > > +	int num_formats;
> > > > +
> > > > +	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> > > > +	if (primary == NULL)
> > > > +		return NULL;
> > > > +
> > > > +	primary->can_scale = false;
> > > > +	primary->max_downscale = 1;
> > > > +	primary->pipe = pipe;
> > > > +	primary->plane = pipe;
> > > > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > > > +		primary->plane = !pipe;
> > > > +
> > > > +	if (INTEL_INFO(dev)->gen <= 3) {
> > > > +		intel_primary_formats = intel_primary_formats_gen3;
> > > > +		num_formats = ARRAY_SIZE(intel_primary_formats_gen3);
> > > > +	} else {
> > > > +		intel_primary_formats = intel_primary_formats_gen4;
> > > > +		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
> > > > +	}
> > > > +
> > > > +	drm_plane_init(dev, &primary->base, 0,
> > > > +		       &intel_primary_plane_funcs, intel_primary_formats,
> > > > +		       num_formats, DRM_PLANE_TYPE_PRIMARY);
> > > > +	return &primary->base;
> > > > +}
> > > > +
> > > >  static void intel_crtc_init(struct drm_device *dev, int pipe)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > >  	struct intel_crtc *intel_crtc;
> > > > -	int i;
> > > > +	struct drm_plane *primary;
> > > > +	int i, ret;
> > > >  
> > > >  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
> > > >  	if (intel_crtc == NULL)
> > > >  		return;
> > > >  
> > > > -	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
> > > > +	primary = intel_primary_plane_create(dev, pipe);
> > > > +	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> > > > +					NULL, &intel_crtc_funcs);
> > > > +	if (ret) {
> > > > +		drm_plane_cleanup(primary);
> > > > +		kfree(intel_crtc);
> > > > +		return;
> > > > +	}
> > > >  
> > > >  	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
> > > >  	for (i = 0; i < 256; i++) {
> > > > -- 
> > > > 1.8.5.1
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> 
> -- 
> Ville Syrjälä
> Intel OTC

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

Re: [PATCH 4/4] drm/i915: Intel-specific primary plane handling (v6)

[Daniel Vetter]

On Thu, May 15, 2014 at 12:35:17PM -0700, Matt Roper wrote:
> On Thu, May 15, 2014 at 08:00:48PM +0300, Ville Syrjälä wrote:
> > On Thu, May 15, 2014 at 09:37:55AM -0700, Matt Roper wrote:
> > > On Thu, May 15, 2014 at 06:52:28PM +0300, Ville Syrjälä wrote:
> > > ...
> > > > 
> > > > > +	};
> > > > > +	bool visible;
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = drm_primary_helper_check_update(plane, crtc, fb,
> > > > > +					      &src, &dest, &clip,
> > > > > +					      DRM_PLANE_HELPER_NO_SCALING,
> > > > > +					      DRM_PLANE_HELPER_NO_SCALING,
> > > > > +					      false, true, &visible);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	if (!visible)
> > > > > +		return intel_primary_plane_disable(plane);
> > > > 
> > > > Here we unpin the old fb...
> > > > 
> > > > > +
> > > > > +	/*
> > > > > +	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
> > > > > +	 * updating the fb pointer, and returning without touching the
> > > > > +	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
> > > > > +	 * turn on the display with all planes setup as desired.
> > > > > +	 */
> > > > > +	if (!crtc->enabled)
> > > > > +		/* Pin and return without programming hardware */
> > > > > +		return intel_pin_and_fence_fb_obj(dev,
> > > > > +						  to_intel_framebuffer(fb)->obj,
> > > > > +						  NULL);
> > > > 
> > > > ...but here we just pin the new fb and leave the old also pinned?
> > > > 
> > > > Something's a bit fishy here. Also a non-enabled crtc but visible primary
> > > > plane doesn't seem to make sense. We also need to remember that set_base
> > > > will always unpin the old_fb. So if something can result in set_base
> > > > being called w/ old_fb!=NULL when it's not pinned, we'll be in deep
> > > > doodoo.
> > > 
> > > Right, I guess we need to unpin the old fb, if any, here as well in case
> > > they perform several setplane() calls while the crtc is disabled.
> > > 
> > > Eventually the crtc will get reenabled by a drmModeSetCrtc call.  If we
> > > do setcrtc(fb = -1), then it should keep whatever fb we've already
> > > pinned via the setplane.  If they provide a new fb, then the pinning
> > > we're doing here will get unpinned by the set_base that gets called.
> > > 
> > > I don't see a way that you can hit set_base with an unpinned
> > > old_fb!=NULL (since the disable plane case farther up also clears
> > > primary->fb).
> > 
> > But it doesn't.
> > 
> 
> Ah, you're right.  I was conflating explicit disables (fb=0) with
> implicit disables (clipped to invisible).  I think the v7 I just sent
> should handle this properly...for the implicit disable case we leave the
> fb pinned and pointed to by primary->fb.  So when we switch to another
> fb (or explicitly disable with fb=0), we should unpin it properly.

Do we have proper coverage for this fun in our primary plane helper tests?
This is the kind of complexity that freaks me out ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: Intel-specific primary plane handling (v6)

[Matt Roper]

On Thu, May 15, 2014 at 10:49:52PM +0200, Daniel Vetter wrote:
> On Thu, May 15, 2014 at 12:35:17PM -0700, Matt Roper wrote:
> > On Thu, May 15, 2014 at 08:00:48PM +0300, Ville Syrjälä wrote:
> > > On Thu, May 15, 2014 at 09:37:55AM -0700, Matt Roper wrote:
> > > > On Thu, May 15, 2014 at 06:52:28PM +0300, Ville Syrjälä wrote:
> > > > ...
> > > > > 
> > > > > > +	};
> > > > > > +	bool visible;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	ret = drm_primary_helper_check_update(plane, crtc, fb,
> > > > > > +					      &src, &dest, &clip,
> > > > > > +					      DRM_PLANE_HELPER_NO_SCALING,
> > > > > > +					      DRM_PLANE_HELPER_NO_SCALING,
> > > > > > +					      false, true, &visible);
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	if (!visible)
> > > > > > +		return intel_primary_plane_disable(plane);
> > > > > 
> > > > > Here we unpin the old fb...
> > > > > 
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
> > > > > > +	 * updating the fb pointer, and returning without touching the
> > > > > > +	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
> > > > > > +	 * turn on the display with all planes setup as desired.
> > > > > > +	 */
> > > > > > +	if (!crtc->enabled)
> > > > > > +		/* Pin and return without programming hardware */
> > > > > > +		return intel_pin_and_fence_fb_obj(dev,
> > > > > > +						  to_intel_framebuffer(fb)->obj,
> > > > > > +						  NULL);
> > > > > 
> > > > > ...but here we just pin the new fb and leave the old also pinned?
> > > > > 
> > > > > Something's a bit fishy here. Also a non-enabled crtc but visible primary
> > > > > plane doesn't seem to make sense. We also need to remember that set_base
> > > > > will always unpin the old_fb. So if something can result in set_base
> > > > > being called w/ old_fb!=NULL when it's not pinned, we'll be in deep
> > > > > doodoo.
> > > > 
> > > > Right, I guess we need to unpin the old fb, if any, here as well in case
> > > > they perform several setplane() calls while the crtc is disabled.
> > > > 
> > > > Eventually the crtc will get reenabled by a drmModeSetCrtc call.  If we
> > > > do setcrtc(fb = -1), then it should keep whatever fb we've already
> > > > pinned via the setplane.  If they provide a new fb, then the pinning
> > > > we're doing here will get unpinned by the set_base that gets called.
> > > > 
> > > > I don't see a way that you can hit set_base with an unpinned
> > > > old_fb!=NULL (since the disable plane case farther up also clears
> > > > primary->fb).
> > > 
> > > But it doesn't.
> > > 
> > 
> > Ah, you're right.  I was conflating explicit disables (fb=0) with
> > implicit disables (clipped to invisible).  I think the v7 I just sent
> > should handle this properly...for the implicit disable case we leave the
> > fb pinned and pointed to by primary->fb.  So when we switch to another
> > fb (or explicitly disable with fb=0), we should unpin it properly.
> 
> Do we have proper coverage for this fun in our primary plane helper tests?
> This is the kind of complexity that freaks me out ;-)
> -Daniel

Was 'helper' in your question above a typo?  The i-g-t tests I've
written have been intended for use with this patch (i.e., i915-specific
primary plane support), so I don't really have any tests that only test
the lesser, helper-provided functionality (and drivers using the helpers
shouldn't run into the things Ville is pointing out here because they
can't disable the primary plane independent of the crtc).

But assuming you meant the general i-g-t tests, yeah, I also posted a
slightly updated version so that it now tries to set multiple fb's while
the crtc is off.  Since crtc=off causes the primary plane to be fully
clipped and implicitly disabled, it should exercise these cases and
catch the pin/unpin mistakes that Ville's review caught.


Matt

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

Re: [PATCH 4/4] drm/i915: Intel-specific primary plane handling (v6)

[Daniel Vetter]

On Thu, May 15, 2014 at 01:59:39PM -0700, Matt Roper wrote:
> On Thu, May 15, 2014 at 10:49:52PM +0200, Daniel Vetter wrote:
> > On Thu, May 15, 2014 at 12:35:17PM -0700, Matt Roper wrote:
> > > On Thu, May 15, 2014 at 08:00:48PM +0300, Ville Syrjälä wrote:
> > > > On Thu, May 15, 2014 at 09:37:55AM -0700, Matt Roper wrote:
> > > > > On Thu, May 15, 2014 at 06:52:28PM +0300, Ville Syrjälä wrote:
> > > > > ...
> > > > > > 
> > > > > > > +	};
> > > > > > > +	bool visible;
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	ret = drm_primary_helper_check_update(plane, crtc, fb,
> > > > > > > +					      &src, &dest, &clip,
> > > > > > > +					      DRM_PLANE_HELPER_NO_SCALING,
> > > > > > > +					      DRM_PLANE_HELPER_NO_SCALING,
> > > > > > > +					      false, true, &visible);
> > > > > > > +	if (ret)
> > > > > > > +		return ret;
> > > > > > > +
> > > > > > > +	if (!visible)
> > > > > > > +		return intel_primary_plane_disable(plane);
> > > > > > 
> > > > > > Here we unpin the old fb...
> > > > > > 
> > > > > > > +
> > > > > > > +	/*
> > > > > > > +	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
> > > > > > > +	 * updating the fb pointer, and returning without touching the
> > > > > > > +	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
> > > > > > > +	 * turn on the display with all planes setup as desired.
> > > > > > > +	 */
> > > > > > > +	if (!crtc->enabled)
> > > > > > > +		/* Pin and return without programming hardware */
> > > > > > > +		return intel_pin_and_fence_fb_obj(dev,
> > > > > > > +						  to_intel_framebuffer(fb)->obj,
> > > > > > > +						  NULL);
> > > > > > 
> > > > > > ...but here we just pin the new fb and leave the old also pinned?
> > > > > > 
> > > > > > Something's a bit fishy here. Also a non-enabled crtc but visible primary
> > > > > > plane doesn't seem to make sense. We also need to remember that set_base
> > > > > > will always unpin the old_fb. So if something can result in set_base
> > > > > > being called w/ old_fb!=NULL when it's not pinned, we'll be in deep
> > > > > > doodoo.
> > > > > 
> > > > > Right, I guess we need to unpin the old fb, if any, here as well in case
> > > > > they perform several setplane() calls while the crtc is disabled.
> > > > > 
> > > > > Eventually the crtc will get reenabled by a drmModeSetCrtc call.  If we
> > > > > do setcrtc(fb = -1), then it should keep whatever fb we've already
> > > > > pinned via the setplane.  If they provide a new fb, then the pinning
> > > > > we're doing here will get unpinned by the set_base that gets called.
> > > > > 
> > > > > I don't see a way that you can hit set_base with an unpinned
> > > > > old_fb!=NULL (since the disable plane case farther up also clears
> > > > > primary->fb).
> > > > 
> > > > But it doesn't.
> > > > 
> > > 
> > > Ah, you're right.  I was conflating explicit disables (fb=0) with
> > > implicit disables (clipped to invisible).  I think the v7 I just sent
> > > should handle this properly...for the implicit disable case we leave the
> > > fb pinned and pointed to by primary->fb.  So when we switch to another
> > > fb (or explicitly disable with fb=0), we should unpin it properly.
> > 
> > Do we have proper coverage for this fun in our primary plane helper tests?
> > This is the kind of complexity that freaks me out ;-)
> > -Daniel
> 
> Was 'helper' in your question above a typo?  The i-g-t tests I've
> written have been intended for use with this patch (i.e., i915-specific
> primary plane support), so I don't really have any tests that only test
> the lesser, helper-provided functionality (and drivers using the helpers
> shouldn't run into the things Ville is pointing out here because they
> can't disable the primary plane independent of the crtc).
> 
> But assuming you meant the general i-g-t tests, yeah, I also posted a
> slightly updated version so that it now tries to set multiple fb's while
> the crtc is off.  Since crtc=off causes the primary plane to be fully
> clipped and implicitly disabled, it should exercise these cases and
> catch the pin/unpin mistakes that Ville's review caught.

Yeah, s/helper// ;-) For the tests, do you also have some that use the
implicit fb (i.e. fb = -1) with setcrtc? Just kinda for completeness.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: Intel-specific primary plane handling (v6)

[Matt Roper]

On Thu, May 15, 2014 at 11:20:39PM +0200, Daniel Vetter wrote:
> > > > Ah, you're right.  I was conflating explicit disables (fb=0) with
> > > > implicit disables (clipped to invisible).  I think the v7 I just sent
> > > > should handle this properly...for the implicit disable case we leave the
> > > > fb pinned and pointed to by primary->fb.  So when we switch to another
> > > > fb (or explicitly disable with fb=0), we should unpin it properly.
> > > 
> > > Do we have proper coverage for this fun in our primary plane helper tests?
> > > This is the kind of complexity that freaks me out ;-)
> > > -Daniel
> > 
> > Was 'helper' in your question above a typo?  The i-g-t tests I've
> > written have been intended for use with this patch (i.e., i915-specific
> > primary plane support), so I don't really have any tests that only test
> > the lesser, helper-provided functionality (and drivers using the helpers
> > shouldn't run into the things Ville is pointing out here because they
> > can't disable the primary plane independent of the crtc).
> > 
> > But assuming you meant the general i-g-t tests, yeah, I also posted a
> > slightly updated version so that it now tries to set multiple fb's while
> > the crtc is off.  Since crtc=off causes the primary plane to be fully
> > clipped and implicitly disabled, it should exercise these cases and
> > catch the pin/unpin mistakes that Ville's review caught.
> 
> Yeah, s/helper// ;-) For the tests, do you also have some that use the
> implicit fb (i.e. fb = -1) with setcrtc? Just kinda for completeness.
> -Daniel

Yeah...turn off the crtc, set a few different fb's via setplane, then
turn the crtc back on with drmModeSetCrtc(fb = -1) and test that it pops
up with the right framebuffer set.


Matt

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

[PATCH 4/4] drm/i915: Intel-specific primary plane handling (v7)

[Matt Roper]

Intel hardware allows the primary plane to be disabled independently of
the CRTC.  Provide custom primary plane handling to allow this.

v7:
 - Clip primary plane to invisible when crtc is disabled since
   intel_crtc->config.pipe_src_{w,h} may be garbage otherwise.
 - Unpin old fb before pinning new one in the "just pin and
   return" case that is used when the crtc is disabled.
 - Don't treat implicit disabling of the primary plane (caused by
   clipping) the same way as explicit disabling (caused by fb=0).
   For implicit disables, we should leave the fb set and pinned,
   whereas for explicit disables we need to unpin the fb before
   primary->fb is cleared.
v6:
 - Pass rectangles to primary helper check function and get plane
   visibility back.
 - Wait for pending pageflips on primary plane update/disable.
 - Allow primary plane to be updated while the crtc is disabled (changes
   will take effect when the crtc is re-enabled if modeset passes -1
   for the fb id).
 - Drop WARN() if we try to disable the primary plane when it's
   already been disabled.  This will happen if the crtc gets disabled
   after the primary plane has already been disabled independently.
v5:
 - Use new drm_primary_helper_check_update() helper function to check
   setplane parameter validity.
 - Swap primary plane's pipe for pre-gen4 FBC (caught by Ville Syrjälä)
 - Cleanup primary plane properly on crtc init failure
v4:
 - Don't add a primary_plane field to intel_crtc; that was left over
   from a much earlier iteration of this patch series, but is no longer
   needed/used now that the DRM core primary plane support has been
   merged.
v3:
 - Provide gen-specific primary plane format lists (suggested by Daniel
   Vetter).
 - If the primary plane is already enabled, go ahead and just call the
   primary plane helper to do the update (suggested by Daniel Vetter).
 - Don't try to disable the primary plane on destruction; the DRM layer
   should have already taken care of this for us.
v2:
 - Unpin fb properly on primary plane disable
 - Provide an Intel-specific set of primary plane formats
 - Additional sanity checks on setplane (in line with the checks
   currently being done by the DRM core primary plane helper)

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 212 ++++++++++++++++++++++++++++++++++-
 1 file changed, 209 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3aedc64..e9f196e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -39,10 +39,37 @@
 #include "i915_trace.h"
 #include <drm/drm_dp_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_rect.h>
 #include <linux/dma_remapping.h>
 
+/* Primary plane formats supported by all gen */
+#define COMMON_PRIMARY_FORMATS \
+	DRM_FORMAT_C8, \
+	DRM_FORMAT_RGB565, \
+	DRM_FORMAT_XRGB8888, \
+	DRM_FORMAT_ARGB8888
+
+/* Primary plane formats for gen <= 3 */
+static const uint32_t intel_primary_formats_gen2[] = {
+	COMMON_PRIMARY_FORMATS,
+	DRM_FORMAT_XRGB1555,
+	DRM_FORMAT_ARGB1555,
+};
+
+/* Primary plane formats for gen >= 4 */
+static const uint32_t intel_primary_formats_gen4[] = {
+	COMMON_PRIMARY_FORMATS, \
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_XRGB2101010,
+	DRM_FORMAT_ARGB2101010,
+	DRM_FORMAT_XBGR2101010,
+	DRM_FORMAT_ABGR2101010,
+};
+
 #define DIV_ROUND_CLOSEST_ULL(ll, d)	\
-	({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
+({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
 
 static void intel_increase_pllclock(struct drm_crtc *crtc);
 static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
@@ -10832,17 +10859,196 @@ static void intel_shared_dpll_init(struct drm_device *dev)
 	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
 }
 
+static int
+intel_primary_plane_disable(struct drm_plane *plane)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc;
+
+	if (!plane->fb)
+		return 0;
+
+	BUG_ON(!plane->crtc);
+
+	intel_crtc = to_intel_crtc(plane->crtc);
+
+	/*
+	 * Even though we checked plane->fb above, it's still possible that
+	 * the primary plane has been implicitly disabled because the crtc
+	 * coordinates given weren't visible, or because we detected
+	 * that it was 100% covered by a sprite plane.  Or, the CRTC may be
+	 * off and we've set a fb, but haven't actually turned on the CRTC yet.
+	 * In either case, we need to unpin the FB and let the fb pointer get
+	 * updated, but otherwise we don't need to touch the hardware.
+	 */
+	if (!intel_crtc->primary_enabled)
+		goto disable_unpin;
+
+	intel_crtc_wait_for_pending_flips(plane->crtc);
+	intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
+				       intel_plane->pipe);
+
+disable_unpin:
+	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+	plane->fb = NULL;
+
+	return 0;
+}
+
+static int
+intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
+			     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+			     unsigned int crtc_w, unsigned int crtc_h,
+			     uint32_t src_x, uint32_t src_y,
+			     uint32_t src_w, uint32_t src_h)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_rect dest = {
+		/* integer pixels */
+		.x1 = crtc_x,
+		.y1 = crtc_y,
+		.x2 = crtc_x + crtc_w,
+		.y2 = crtc_y + crtc_h,
+	};
+	struct drm_rect src = {
+		/* 16.16 fixed point */
+		.x1 = src_x,
+		.y1 = src_y,
+		.x2 = src_x + src_w,
+		.y2 = src_y + src_h,
+	};
+	const struct drm_rect clip = {
+		/* integer pixels */
+		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
+		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
+	};
+	bool visible;
+	int ret;
+
+	ret = drm_primary_helper_check_update(plane, crtc, fb,
+					      &src, &dest, &clip,
+					      DRM_PLANE_HELPER_NO_SCALING,
+					      DRM_PLANE_HELPER_NO_SCALING,
+					      false, true, &visible);
+
+	if (ret)
+		return ret;
+
+	/*
+	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
+	 * updating the fb pointer, and returning without touching the
+	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
+	 * turn on the display with all planes setup as desired.
+	 */
+	if (!crtc->enabled) {
+		/*
+		 * If we already called setplane while the crtc was disabled,
+		 * we may have an fb pinned; unpin it.
+		 */
+		if (plane->fb)
+			intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+
+		/* Pin and return without programming hardware */
+		return intel_pin_and_fence_fb_obj(dev,
+						  to_intel_framebuffer(fb)->obj,
+						  NULL);
+	}
+
+	intel_crtc_wait_for_pending_flips(crtc);
+
+	/*
+	 * If clipping results in a non-visible primary plane, just disable the
+	 * plane hardware.  Note that this is a bit different than if userspace
+	 * explicitly disables the plane by passing fb=0 because plane->fb
+	 * remains set and pinned until we switch to a different fb.
+	 */
+	if (!visible && intel_crtc->primary_enabled) {
+		intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
+					       intel_plane->pipe);
+		return 0;
+	}
+
+	ret = intel_pipe_set_base(crtc, src.x1, src.y1, fb);
+	if (ret)
+		return ret;
+
+	if (!intel_crtc->primary_enabled)
+		intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
+					      intel_crtc->pipe);
+
+	return 0;
+}
+
+static void intel_primary_plane_destroy(struct drm_plane *plane)
+{
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	drm_plane_cleanup(plane);
+	kfree(intel_plane);
+}
+
+static const struct drm_plane_funcs intel_primary_plane_funcs = {
+	.update_plane = intel_primary_plane_setplane,
+	.disable_plane = intel_primary_plane_disable,
+	.destroy = intel_primary_plane_destroy,
+};
+
+static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
+						    int pipe)
+{
+	struct intel_plane *primary;
+	const uint32_t *intel_primary_formats;
+	int num_formats;
+
+	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
+	if (primary == NULL)
+		return NULL;
+
+	primary->can_scale = false;
+	primary->max_downscale = 1;
+	primary->pipe = pipe;
+	primary->plane = pipe;
+	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
+		primary->plane = !pipe;
+
+	if (INTEL_INFO(dev)->gen <= 3) {
+		intel_primary_formats = intel_primary_formats_gen2;
+		num_formats = ARRAY_SIZE(intel_primary_formats_gen2);
+	} else {
+		intel_primary_formats = intel_primary_formats_gen4;
+		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
+	}
+
+	drm_universal_plane_init(dev, &primary->base, 0,
+				 &intel_primary_plane_funcs,
+				 intel_primary_formats, num_formats,
+				 DRM_PLANE_TYPE_PRIMARY);
+	return &primary->base;
+}
+
 static void intel_crtc_init(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc;
-	int i;
+	struct drm_plane *primary;
+	int i, ret;
 
 	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
 	if (intel_crtc == NULL)
 		return;
 
-	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
+	primary = intel_primary_plane_create(dev, pipe);
+	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
+					NULL, &intel_crtc_funcs);
+	if (ret) {
+		drm_plane_cleanup(primary);
+		kfree(intel_crtc);
+		return;
+	}
 
 	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
 	for (i = 0; i < 256; i++) {
-- 
1.8.5.1

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

Re: [PATCH 4/4] drm/i915: Intel-specific primary plane handling (v7)

[Lee, Chon Ming]

On 05/15 12:21, Matt Roper wrote:
> Intel hardware allows the primary plane to be disabled independently of
> the CRTC.  Provide custom primary plane handling to allow this.
> 
> v7:
>  - Clip primary plane to invisible when crtc is disabled since
>    intel_crtc->config.pipe_src_{w,h} may be garbage otherwise.
>  - Unpin old fb before pinning new one in the "just pin and
>    return" case that is used when the crtc is disabled.
>  - Don't treat implicit disabling of the primary plane (caused by
>    clipping) the same way as explicit disabling (caused by fb=0).
>    For implicit disables, we should leave the fb set and pinned,
>    whereas for explicit disables we need to unpin the fb before
>    primary->fb is cleared.
> v6:
>  - Pass rectangles to primary helper check function and get plane
>    visibility back.
>  - Wait for pending pageflips on primary plane update/disable.
>  - Allow primary plane to be updated while the crtc is disabled (changes
>    will take effect when the crtc is re-enabled if modeset passes -1
>    for the fb id).
>  - Drop WARN() if we try to disable the primary plane when it's
>    already been disabled.  This will happen if the crtc gets disabled
>    after the primary plane has already been disabled independently.
> v5:
>  - Use new drm_primary_helper_check_update() helper function to check
>    setplane parameter validity.
>  - Swap primary plane's pipe for pre-gen4 FBC (caught by Ville Syrjälä)
>  - Cleanup primary plane properly on crtc init failure
> v4:
>  - Don't add a primary_plane field to intel_crtc; that was left over
>    from a much earlier iteration of this patch series, but is no longer
>    needed/used now that the DRM core primary plane support has been
>    merged.
> v3:
>  - Provide gen-specific primary plane format lists (suggested by Daniel
>    Vetter).
>  - If the primary plane is already enabled, go ahead and just call the
>    primary plane helper to do the update (suggested by Daniel Vetter).
>  - Don't try to disable the primary plane on destruction; the DRM layer
>    should have already taken care of this for us.
> v2:
>  - Unpin fb properly on primary plane disable
>  - Provide an Intel-specific set of primary plane formats
>  - Additional sanity checks on setplane (in line with the checks
>    currently being done by the DRM core primary plane helper)
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 212 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 209 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3aedc64..e9f196e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -39,10 +39,37 @@
>  #include "i915_trace.h"
>  #include <drm/drm_dp_helper.h>
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_plane_helper.h>
> +#include <drm/drm_rect.h>
>  #include <linux/dma_remapping.h>
>  
> +/* Primary plane formats supported by all gen */
> +#define COMMON_PRIMARY_FORMATS \
> +	DRM_FORMAT_C8, \
> +	DRM_FORMAT_RGB565, \
> +	DRM_FORMAT_XRGB8888, \
> +	DRM_FORMAT_ARGB8888
> +
> +/* Primary plane formats for gen <= 3 */
> +static const uint32_t intel_primary_formats_gen2[] = {
> +	COMMON_PRIMARY_FORMATS,
> +	DRM_FORMAT_XRGB1555,
> +	DRM_FORMAT_ARGB1555,
> +};
> +
> +/* Primary plane formats for gen >= 4 */
> +static const uint32_t intel_primary_formats_gen4[] = {
> +	COMMON_PRIMARY_FORMATS, \
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_XRGB2101010,
> +	DRM_FORMAT_ARGB2101010,
> +	DRM_FORMAT_XBGR2101010,
> +	DRM_FORMAT_ABGR2101010,
> +};
> +
>  #define DIV_ROUND_CLOSEST_ULL(ll, d)	\
> -	({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
> +({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
>  
>  static void intel_increase_pllclock(struct drm_crtc *crtc);
>  static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
> @@ -10832,17 +10859,196 @@ static void intel_shared_dpll_init(struct drm_device *dev)
>  	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
>  }
>  
> +static int
> +intel_primary_plane_disable(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc;
> +
> +	if (!plane->fb)
> +		return 0;
> +
> +	BUG_ON(!plane->crtc);
> +
> +	intel_crtc = to_intel_crtc(plane->crtc);
> +
> +	/*
> +	 * Even though we checked plane->fb above, it's still possible that
> +	 * the primary plane has been implicitly disabled because the crtc
> +	 * coordinates given weren't visible, or because we detected
> +	 * that it was 100% covered by a sprite plane.  Or, the CRTC may be
> +	 * off and we've set a fb, but haven't actually turned on the CRTC yet.
> +	 * In either case, we need to unpin the FB and let the fb pointer get
> +	 * updated, but otherwise we don't need to touch the hardware.
> +	 */
> +	if (!intel_crtc->primary_enabled)
> +		goto disable_unpin;
> +
> +	intel_crtc_wait_for_pending_flips(plane->crtc);
> +	intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
> +				       intel_plane->pipe);
> +
> +disable_unpin:
> +	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
> +	plane->fb = NULL;
> +
> +	return 0;
> +}
> +
> +static int
> +intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
> +			     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> +			     unsigned int crtc_w, unsigned int crtc_h,
> +			     uint32_t src_x, uint32_t src_y,
> +			     uint32_t src_w, uint32_t src_h)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct drm_rect dest = {
> +		/* integer pixels */
> +		.x1 = crtc_x,
> +		.y1 = crtc_y,
> +		.x2 = crtc_x + crtc_w,
> +		.y2 = crtc_y + crtc_h,
> +	};
> +	struct drm_rect src = {
> +		/* 16.16 fixed point */
> +		.x1 = src_x,
> +		.y1 = src_y,
> +		.x2 = src_x + src_w,
> +		.y2 = src_y + src_h,
> +	};
> +	const struct drm_rect clip = {
> +		/* integer pixels */
> +		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
> +		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
> +	};
> +	bool visible;
> +	int ret;
> +
> +	ret = drm_primary_helper_check_update(plane, crtc, fb,
> +					      &src, &dest, &clip,
> +					      DRM_PLANE_HELPER_NO_SCALING,
> +					      DRM_PLANE_HELPER_NO_SCALING,
> +					      false, true, &visible);
> +
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
> +	 * updating the fb pointer, and returning without touching the
> +	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
> +	 * turn on the display with all planes setup as desired.
> +	 */
> +	if (!crtc->enabled) {
> +		/*
> +		 * If we already called setplane while the crtc was disabled,
> +		 * we may have an fb pinned; unpin it.
> +		 */
> +		if (plane->fb)
> +			intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
> +
> +		/* Pin and return without programming hardware */
> +		return intel_pin_and_fence_fb_obj(dev,
> +						  to_intel_framebuffer(fb)->obj,
> +						  NULL);
> +	}
> +
> +	intel_crtc_wait_for_pending_flips(crtc);

You may move the intel_crtc_wait_for_pending_flips after below.  If the primary
plane is always cover by another plane, it doesn't need to wait for previously
flip to complete.
> +
> +	/*
> +	 * If clipping results in a non-visible primary plane, just disable the
> +	 * plane hardware.  Note that this is a bit different than if userspace
> +	 * explicitly disables the plane by passing fb=0 because plane->fb
> +	 * remains set and pinned until we switch to a different fb.
> +	 */
> +	if (!visible && intel_crtc->primary_enabled) {
> +		intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
> +					       intel_plane->pipe);
> +		return 0;

If the primary plane is not visible, there is no pin here. In drm_mode_setplane,
plane->fb = fb after this. 

The next call to disable the plane will do unpin the plane->fb which has never
pinned. 

Regards,
Chon Ming

> +	}
> +
> +	ret = intel_pipe_set_base(crtc, src.x1, src.y1, fb);
> +	if (ret)
> +		return ret;
> +
> +	if (!intel_crtc->primary_enabled)
> +		intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> +					      intel_crtc->pipe);
> +
> +	return 0;
> +}
> +
> +static void intel_primary_plane_destroy(struct drm_plane *plane)
> +{
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	drm_plane_cleanup(plane);
> +	kfree(intel_plane);
> +}
> +
> +static const struct drm_plane_funcs intel_primary_plane_funcs = {
> +	.update_plane = intel_primary_plane_setplane,
> +	.disable_plane = intel_primary_plane_disable,
> +	.destroy = intel_primary_plane_destroy,
> +};
> +
> +static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> +						    int pipe)
> +{
> +	struct intel_plane *primary;
> +	const uint32_t *intel_primary_formats;
> +	int num_formats;
> +
> +	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> +	if (primary == NULL)
> +		return NULL;
> +
> +	primary->can_scale = false;
> +	primary->max_downscale = 1;
> +	primary->pipe = pipe;
> +	primary->plane = pipe;
> +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> +		primary->plane = !pipe;
> +
> +	if (INTEL_INFO(dev)->gen <= 3) {
> +		intel_primary_formats = intel_primary_formats_gen2;
> +		num_formats = ARRAY_SIZE(intel_primary_formats_gen2);
> +	} else {
> +		intel_primary_formats = intel_primary_formats_gen4;
> +		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
> +	}
> +
> +	drm_universal_plane_init(dev, &primary->base, 0,
> +				 &intel_primary_plane_funcs,
> +				 intel_primary_formats, num_formats,
> +				 DRM_PLANE_TYPE_PRIMARY);
> +	return &primary->base;
> +}
> +
>  static void intel_crtc_init(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc;
> -	int i;
> +	struct drm_plane *primary;
> +	int i, ret;
>  
>  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
>  	if (intel_crtc == NULL)
>  		return;
>  
> -	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
> +	primary = intel_primary_plane_create(dev, pipe);
> +	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> +					NULL, &intel_crtc_funcs);
> +	if (ret) {
> +		drm_plane_cleanup(primary);
> +		kfree(intel_crtc);
> +		return;
> +	}
>  
>  	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
>  	for (i = 0; i < 256; i++) {
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Intel-specific primary plane handling (v7)

[Matt Roper]

On Sun, May 18, 2014 at 11:53:13PM +0800, Lee, Chon Ming wrote:
> On 05/15 12:21, Matt Roper wrote:
> > Intel hardware allows the primary plane to be disabled independently of
> > the CRTC.  Provide custom primary plane handling to allow this.
> > 
> > v7:
> >  - Clip primary plane to invisible when crtc is disabled since
> >    intel_crtc->config.pipe_src_{w,h} may be garbage otherwise.
> >  - Unpin old fb before pinning new one in the "just pin and
> >    return" case that is used when the crtc is disabled.
> >  - Don't treat implicit disabling of the primary plane (caused by
> >    clipping) the same way as explicit disabling (caused by fb=0).
> >    For implicit disables, we should leave the fb set and pinned,
> >    whereas for explicit disables we need to unpin the fb before
> >    primary->fb is cleared.
> > v6:
> >  - Pass rectangles to primary helper check function and get plane
> >    visibility back.
> >  - Wait for pending pageflips on primary plane update/disable.
> >  - Allow primary plane to be updated while the crtc is disabled (changes
> >    will take effect when the crtc is re-enabled if modeset passes -1
> >    for the fb id).
> >  - Drop WARN() if we try to disable the primary plane when it's
> >    already been disabled.  This will happen if the crtc gets disabled
> >    after the primary plane has already been disabled independently.
> > v5:
> >  - Use new drm_primary_helper_check_update() helper function to check
> >    setplane parameter validity.
> >  - Swap primary plane's pipe for pre-gen4 FBC (caught by Ville Syrjälä)
> >  - Cleanup primary plane properly on crtc init failure
> > v4:
> >  - Don't add a primary_plane field to intel_crtc; that was left over
> >    from a much earlier iteration of this patch series, but is no longer
> >    needed/used now that the DRM core primary plane support has been
> >    merged.
> > v3:
> >  - Provide gen-specific primary plane format lists (suggested by Daniel
> >    Vetter).
> >  - If the primary plane is already enabled, go ahead and just call the
> >    primary plane helper to do the update (suggested by Daniel Vetter).
> >  - Don't try to disable the primary plane on destruction; the DRM layer
> >    should have already taken care of this for us.
> > v2:
> >  - Unpin fb properly on primary plane disable
> >  - Provide an Intel-specific set of primary plane formats
> >  - Additional sanity checks on setplane (in line with the checks
> >    currently being done by the DRM core primary plane helper)
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 212 ++++++++++++++++++++++++++++++++++-
> >  1 file changed, 209 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3aedc64..e9f196e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -39,10 +39,37 @@
> >  #include "i915_trace.h"
> >  #include <drm/drm_dp_helper.h>
> >  #include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_plane_helper.h>
> > +#include <drm/drm_rect.h>
> >  #include <linux/dma_remapping.h>
> >  
> > +/* Primary plane formats supported by all gen */
> > +#define COMMON_PRIMARY_FORMATS \
> > +	DRM_FORMAT_C8, \
> > +	DRM_FORMAT_RGB565, \
> > +	DRM_FORMAT_XRGB8888, \
> > +	DRM_FORMAT_ARGB8888
> > +
> > +/* Primary plane formats for gen <= 3 */
> > +static const uint32_t intel_primary_formats_gen2[] = {
> > +	COMMON_PRIMARY_FORMATS,
> > +	DRM_FORMAT_XRGB1555,
> > +	DRM_FORMAT_ARGB1555,
> > +};
> > +
> > +/* Primary plane formats for gen >= 4 */
> > +static const uint32_t intel_primary_formats_gen4[] = {
> > +	COMMON_PRIMARY_FORMATS, \
> > +	DRM_FORMAT_XBGR8888,
> > +	DRM_FORMAT_ABGR8888,
> > +	DRM_FORMAT_XRGB2101010,
> > +	DRM_FORMAT_ARGB2101010,
> > +	DRM_FORMAT_XBGR2101010,
> > +	DRM_FORMAT_ABGR2101010,
> > +};
> > +
> >  #define DIV_ROUND_CLOSEST_ULL(ll, d)	\
> > -	({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
> > +({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
> >  
> >  static void intel_increase_pllclock(struct drm_crtc *crtc);
> >  static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
> > @@ -10832,17 +10859,196 @@ static void intel_shared_dpll_init(struct drm_device *dev)
> >  	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
> >  }
> >  
> > +static int
> > +intel_primary_plane_disable(struct drm_plane *plane)
> > +{
> > +	struct drm_device *dev = plane->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > +	struct intel_crtc *intel_crtc;
> > +
> > +	if (!plane->fb)
> > +		return 0;
> > +
> > +	BUG_ON(!plane->crtc);
> > +
> > +	intel_crtc = to_intel_crtc(plane->crtc);
> > +
> > +	/*
> > +	 * Even though we checked plane->fb above, it's still possible that
> > +	 * the primary plane has been implicitly disabled because the crtc
> > +	 * coordinates given weren't visible, or because we detected
> > +	 * that it was 100% covered by a sprite plane.  Or, the CRTC may be
> > +	 * off and we've set a fb, but haven't actually turned on the CRTC yet.
> > +	 * In either case, we need to unpin the FB and let the fb pointer get
> > +	 * updated, but otherwise we don't need to touch the hardware.
> > +	 */
> > +	if (!intel_crtc->primary_enabled)
> > +		goto disable_unpin;
> > +
> > +	intel_crtc_wait_for_pending_flips(plane->crtc);
> > +	intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
> > +				       intel_plane->pipe);
> > +
> > +disable_unpin:
> > +	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
> > +	plane->fb = NULL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
> > +			     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> > +			     unsigned int crtc_w, unsigned int crtc_h,
> > +			     uint32_t src_x, uint32_t src_y,
> > +			     uint32_t src_w, uint32_t src_h)
> > +{
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > +	struct drm_rect dest = {
> > +		/* integer pixels */
> > +		.x1 = crtc_x,
> > +		.y1 = crtc_y,
> > +		.x2 = crtc_x + crtc_w,
> > +		.y2 = crtc_y + crtc_h,
> > +	};
> > +	struct drm_rect src = {
> > +		/* 16.16 fixed point */
> > +		.x1 = src_x,
> > +		.y1 = src_y,
> > +		.x2 = src_x + src_w,
> > +		.y2 = src_y + src_h,
> > +	};
> > +	const struct drm_rect clip = {
> > +		/* integer pixels */
> > +		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
> > +		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
> > +	};
> > +	bool visible;
> > +	int ret;
> > +
> > +	ret = drm_primary_helper_check_update(plane, crtc, fb,
> > +					      &src, &dest, &clip,
> > +					      DRM_PLANE_HELPER_NO_SCALING,
> > +					      DRM_PLANE_HELPER_NO_SCALING,
> > +					      false, true, &visible);
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
> > +	 * updating the fb pointer, and returning without touching the
> > +	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
> > +	 * turn on the display with all planes setup as desired.
> > +	 */
> > +	if (!crtc->enabled) {
> > +		/*
> > +		 * If we already called setplane while the crtc was disabled,
> > +		 * we may have an fb pinned; unpin it.
> > +		 */
> > +		if (plane->fb)
> > +			intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
> > +
> > +		/* Pin and return without programming hardware */
> > +		return intel_pin_and_fence_fb_obj(dev,
> > +						  to_intel_framebuffer(fb)->obj,
> > +						  NULL);
> > +	}
> > +
> > +	intel_crtc_wait_for_pending_flips(crtc);
> 
> You may move the intel_crtc_wait_for_pending_flips after below.  If the primary
> plane is always cover by another plane, it doesn't need to wait for previously
> flip to complete.

The primary plane may be completely clipped (and thus invisible) for the
next frame that we're trying to program here, but it's possible that it
was visible for the last (and still currently displayed) frame.  If
there's a pageflip pending in that case, I don't think we can disable
the primary plane out from under it until it completes.

> > +
> > +	/*
> > +	 * If clipping results in a non-visible primary plane, just disable the
> > +	 * plane hardware.  Note that this is a bit different than if userspace
> > +	 * explicitly disables the plane by passing fb=0 because plane->fb
> > +	 * remains set and pinned until we switch to a different fb.
> > +	 */
> > +	if (!visible && intel_crtc->primary_enabled) {
> > +		intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
> > +					       intel_plane->pipe);
> > +		return 0;
> 
> If the primary plane is not visible, there is no pin here. In drm_mode_setplane,
> plane->fb = fb after this. 
> 
> The next call to disable the plane will do unpin the plane->fb which has never
> pinned. 

Yeah, you're right about this one; I'll send an update that fixes this.

Looks like I'm missing an i-g-t testcase that does an explicit disable
following an implicit disable.  That should cover this path.


Matt

> 
> Regards,
> Chon Ming
> 
> > +	}
> > +
> > +	ret = intel_pipe_set_base(crtc, src.x1, src.y1, fb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!intel_crtc->primary_enabled)
> > +		intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> > +					      intel_crtc->pipe);
> > +
> > +	return 0;
> > +}
> > +
> > +static void intel_primary_plane_destroy(struct drm_plane *plane)
> > +{
> > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > +	drm_plane_cleanup(plane);
> > +	kfree(intel_plane);
> > +}
> > +
> > +static const struct drm_plane_funcs intel_primary_plane_funcs = {
> > +	.update_plane = intel_primary_plane_setplane,
> > +	.disable_plane = intel_primary_plane_disable,
> > +	.destroy = intel_primary_plane_destroy,
> > +};
> > +
> > +static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> > +						    int pipe)
> > +{
> > +	struct intel_plane *primary;
> > +	const uint32_t *intel_primary_formats;
> > +	int num_formats;
> > +
> > +	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> > +	if (primary == NULL)
> > +		return NULL;
> > +
> > +	primary->can_scale = false;
> > +	primary->max_downscale = 1;
> > +	primary->pipe = pipe;
> > +	primary->plane = pipe;
> > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > +		primary->plane = !pipe;
> > +
> > +	if (INTEL_INFO(dev)->gen <= 3) {
> > +		intel_primary_formats = intel_primary_formats_gen2;
> > +		num_formats = ARRAY_SIZE(intel_primary_formats_gen2);
> > +	} else {
> > +		intel_primary_formats = intel_primary_formats_gen4;
> > +		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
> > +	}
> > +
> > +	drm_universal_plane_init(dev, &primary->base, 0,
> > +				 &intel_primary_plane_funcs,
> > +				 intel_primary_formats, num_formats,
> > +				 DRM_PLANE_TYPE_PRIMARY);
> > +	return &primary->base;
> > +}
> > +
> >  static void intel_crtc_init(struct drm_device *dev, int pipe)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc;
> > -	int i;
> > +	struct drm_plane *primary;
> > +	int i, ret;
> >  
> >  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
> >  	if (intel_crtc == NULL)
> >  		return;
> >  
> > -	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
> > +	primary = intel_primary_plane_create(dev, pipe);
> > +	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> > +					NULL, &intel_crtc_funcs);
> > +	if (ret) {
> > +		drm_plane_cleanup(primary);
> > +		kfree(intel_crtc);
> > +		return;
> > +	}
> >  
> >  	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
> >  	for (i = 0; i < 256; i++) {
> > -- 
> > 1.8.5.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 4/4] drm/i915: Intel-specific primary plane handling (v8)

[Matt Roper]

Intel hardware allows the primary plane to be disabled independently of
the CRTC.  Provide custom primary plane handling to allow this.

v8:
 - Pin/unpin properly when clipping causes the primary plane to be
   disabled when it has previously been enabled.
 - s/drm_primary_helper_check_update/drm_plane_helper_check_update/
v7:
 - Clip primary plane to invisible when crtc is disabled since
   intel_crtc->config.pipe_src_{w,h} may be garbage otherwise.
 - Unpin old fb before pinning new one in the "just pin and
   return" case that is used when the crtc is disabled.
 - Don't treat implicit disabling of the primary plane (caused by
   clipping) the same way as explicit disabling (caused by fb=0).
   For implicit disables, we should leave the fb set and pinned,
   whereas for explicit disables we need to unpin the fb before
   primary->fb is cleared.
v6:
 - Pass rectangles to primary helper check function and get plane
   visibility back.
 - Wait for pending pageflips on primary plane update/disable.
 - Allow primary plane to be updated while the crtc is disabled (changes
   will take effect when the crtc is re-enabled if modeset passes -1
   for the fb id).
 - Drop WARN() if we try to disable the primary plane when it's
   already been disabled.  This will happen if the crtc gets disabled
   after the primary plane has already been disabled independently.
v5:
 - Use new drm_primary_helper_check_update() helper function to check
   setplane parameter validity.
 - Swap primary plane's pipe for pre-gen4 FBC (caught by Ville Syrjälä)
 - Cleanup primary plane properly on crtc init failure
v4:
 - Don't add a primary_plane field to intel_crtc; that was left over
   from a much earlier iteration of this patch series, but is no longer
   needed/used now that the DRM core primary plane support has been
   merged.
v3:
 - Provide gen-specific primary plane format lists (suggested by Daniel
   Vetter).
 - If the primary plane is already enabled, go ahead and just call the
   primary plane helper to do the update (suggested by Daniel Vetter).
 - Don't try to disable the primary plane on destruction; the DRM layer
   should have already taken care of this for us.
v2:
 - Unpin fb properly on primary plane disable
 - Provide an Intel-specific set of primary plane formats
 - Additional sanity checks on setplane (in line with the checks
   currently being done by the DRM core primary plane helper)

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 232 ++++++++++++++++++++++++++++++++++-
 1 file changed, 229 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ce71717..809e03b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -39,10 +39,37 @@
 #include "i915_trace.h"
 #include <drm/drm_dp_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_rect.h>
 #include <linux/dma_remapping.h>
 
+/* Primary plane formats supported by all gen */
+#define COMMON_PRIMARY_FORMATS \
+	DRM_FORMAT_C8, \
+	DRM_FORMAT_RGB565, \
+	DRM_FORMAT_XRGB8888, \
+	DRM_FORMAT_ARGB8888
+
+/* Primary plane formats for gen <= 3 */
+static const uint32_t intel_primary_formats_gen2[] = {
+	COMMON_PRIMARY_FORMATS,
+	DRM_FORMAT_XRGB1555,
+	DRM_FORMAT_ARGB1555,
+};
+
+/* Primary plane formats for gen >= 4 */
+static const uint32_t intel_primary_formats_gen4[] = {
+	COMMON_PRIMARY_FORMATS, \
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_XRGB2101010,
+	DRM_FORMAT_ARGB2101010,
+	DRM_FORMAT_XBGR2101010,
+	DRM_FORMAT_ABGR2101010,
+};
+
 #define DIV_ROUND_CLOSEST_ULL(ll, d)	\
-	({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
+({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
 
 static void intel_increase_pllclock(struct drm_crtc *crtc);
 static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
@@ -10824,17 +10851,216 @@ static void intel_shared_dpll_init(struct drm_device *dev)
 	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
 }
 
+static int
+intel_primary_plane_disable(struct drm_plane *plane)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc;
+
+	if (!plane->fb)
+		return 0;
+
+	BUG_ON(!plane->crtc);
+
+	intel_crtc = to_intel_crtc(plane->crtc);
+
+	/*
+	 * Even though we checked plane->fb above, it's still possible that
+	 * the primary plane has been implicitly disabled because the crtc
+	 * coordinates given weren't visible, or because we detected
+	 * that it was 100% covered by a sprite plane.  Or, the CRTC may be
+	 * off and we've set a fb, but haven't actually turned on the CRTC yet.
+	 * In either case, we need to unpin the FB and let the fb pointer get
+	 * updated, but otherwise we don't need to touch the hardware.
+	 */
+	if (!intel_crtc->primary_enabled)
+		goto disable_unpin;
+
+	intel_crtc_wait_for_pending_flips(plane->crtc);
+	intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
+				       intel_plane->pipe);
+
+disable_unpin:
+	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+	plane->fb = NULL;
+
+	return 0;
+}
+
+static int
+intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
+			     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+			     unsigned int crtc_w, unsigned int crtc_h,
+			     uint32_t src_x, uint32_t src_y,
+			     uint32_t src_w, uint32_t src_h)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_rect dest = {
+		/* integer pixels */
+		.x1 = crtc_x,
+		.y1 = crtc_y,
+		.x2 = crtc_x + crtc_w,
+		.y2 = crtc_y + crtc_h,
+	};
+	struct drm_rect src = {
+		/* 16.16 fixed point */
+		.x1 = src_x,
+		.y1 = src_y,
+		.x2 = src_x + src_w,
+		.y2 = src_y + src_h,
+	};
+	const struct drm_rect clip = {
+		/* integer pixels */
+		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
+		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
+	};
+	bool visible;
+	int ret;
+
+	ret = drm_plane_helper_check_update(plane, crtc, fb,
+					    &src, &dest, &clip,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    false, true, &visible);
+
+	if (ret)
+		return ret;
+
+	/*
+	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
+	 * updating the fb pointer, and returning without touching the
+	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
+	 * turn on the display with all planes setup as desired.
+	 */
+	if (!crtc->enabled) {
+		/*
+		 * If we already called setplane while the crtc was disabled,
+		 * we may have an fb pinned; unpin it.
+		 */
+		if (plane->fb)
+			intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+
+		/* Pin and return without programming hardware */
+		return intel_pin_and_fence_fb_obj(dev,
+						  to_intel_framebuffer(fb)->obj,
+						  NULL);
+	}
+
+	intel_crtc_wait_for_pending_flips(crtc);
+
+	/*
+	 * If clipping results in a non-visible primary plane, we'll disable
+	 * the primary plane.  Note that this is a bit different than what
+	 * happens if userspace explicitly disables the plane by passing fb=0
+	 * because plane->fb still gets set and pinned.
+	 */
+	if (!visible) {
+		/*
+		 * Try to pin the new fb first so that we can bail out if we
+		 * fail.
+		 */
+		if (plane->fb != fb) {
+			ret = intel_pin_and_fence_fb_obj(dev,
+							 to_intel_framebuffer(fb)->obj,
+							 NULL);
+			if (ret)
+				return ret;
+		}
+
+		if (intel_crtc->primary_enabled)
+			intel_disable_primary_hw_plane(dev_priv,
+						       intel_plane->plane,
+						       intel_plane->pipe);
+
+
+		if (plane->fb != fb)
+			if (plane->fb)
+				intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+
+		return 0;
+	}
+
+	ret = intel_pipe_set_base(crtc, src.x1, src.y1, fb);
+	if (ret)
+		return ret;
+
+	if (!intel_crtc->primary_enabled)
+		intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
+					      intel_crtc->pipe);
+
+	return 0;
+}
+
+static void intel_primary_plane_destroy(struct drm_plane *plane)
+{
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	drm_plane_cleanup(plane);
+	kfree(intel_plane);
+}
+
+static const struct drm_plane_funcs intel_primary_plane_funcs = {
+	.update_plane = intel_primary_plane_setplane,
+	.disable_plane = intel_primary_plane_disable,
+	.destroy = intel_primary_plane_destroy,
+};
+
+static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
+						    int pipe)
+{
+	struct intel_plane *primary;
+	const uint32_t *intel_primary_formats;
+	int num_formats;
+
+	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
+	if (primary == NULL)
+		return NULL;
+
+	primary->can_scale = false;
+	primary->max_downscale = 1;
+	primary->pipe = pipe;
+	primary->plane = pipe;
+	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
+		primary->plane = !pipe;
+
+	if (INTEL_INFO(dev)->gen <= 3) {
+		intel_primary_formats = intel_primary_formats_gen2;
+		num_formats = ARRAY_SIZE(intel_primary_formats_gen2);
+	} else {
+		intel_primary_formats = intel_primary_formats_gen4;
+		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
+	}
+
+	drm_universal_plane_init(dev, &primary->base, 0,
+				 &intel_primary_plane_funcs,
+				 intel_primary_formats, num_formats,
+				 DRM_PLANE_TYPE_PRIMARY);
+	return &primary->base;
+}
+
 static void intel_crtc_init(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc;
-	int i;
+	struct drm_plane *primary;
+	int i, ret;
 
 	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
 	if (intel_crtc == NULL)
 		return;
 
-	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
+	primary = intel_primary_plane_create(dev, pipe);
+	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
+					NULL, &intel_crtc_funcs);
+	if (ret) {
+		drm_plane_cleanup(primary);
+		kfree(intel_crtc);
+		return;
+	}
 
 	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
 	for (i = 0; i < 256; i++) {
-- 
1.8.5.1

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

Re: [PATCH 4/4] drm/i915: Intel-specific primary plane handling (v8)

[Lee, Chon Ming]

On 05/19 14:48, Matt Roper wrote:
> Intel hardware allows the primary plane to be disabled independently of
> the CRTC.  Provide custom primary plane handling to allow this.
> 
> v8:
>  - Pin/unpin properly when clipping causes the primary plane to be
>    disabled when it has previously been enabled.
>  - s/drm_primary_helper_check_update/drm_plane_helper_check_update/
> v7:
>  - Clip primary plane to invisible when crtc is disabled since
>    intel_crtc->config.pipe_src_{w,h} may be garbage otherwise.
>  - Unpin old fb before pinning new one in the "just pin and
>    return" case that is used when the crtc is disabled.
>  - Don't treat implicit disabling of the primary plane (caused by
>    clipping) the same way as explicit disabling (caused by fb=0).
>    For implicit disables, we should leave the fb set and pinned,
>    whereas for explicit disables we need to unpin the fb before
>    primary->fb is cleared.
> v6:
>  - Pass rectangles to primary helper check function and get plane
>    visibility back.
>  - Wait for pending pageflips on primary plane update/disable.
>  - Allow primary plane to be updated while the crtc is disabled (changes
>    will take effect when the crtc is re-enabled if modeset passes -1
>    for the fb id).
>  - Drop WARN() if we try to disable the primary plane when it's
>    already been disabled.  This will happen if the crtc gets disabled
>    after the primary plane has already been disabled independently.
> v5:
>  - Use new drm_primary_helper_check_update() helper function to check
>    setplane parameter validity.
>  - Swap primary plane's pipe for pre-gen4 FBC (caught by Ville Syrjälä)
>  - Cleanup primary plane properly on crtc init failure
> v4:
>  - Don't add a primary_plane field to intel_crtc; that was left over
>    from a much earlier iteration of this patch series, but is no longer
>    needed/used now that the DRM core primary plane support has been
>    merged.
> v3:
>  - Provide gen-specific primary plane format lists (suggested by Daniel
>    Vetter).
>  - If the primary plane is already enabled, go ahead and just call the
>    primary plane helper to do the update (suggested by Daniel Vetter).
>  - Don't try to disable the primary plane on destruction; the DRM layer
>    should have already taken care of this for us.
> v2:
>  - Unpin fb properly on primary plane disable
>  - Provide an Intel-specific set of primary plane formats
>  - Additional sanity checks on setplane (in line with the checks
>    currently being done by the DRM core primary plane helper)
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Look go to me.

Reviewed-by: Chon Ming Lee <chon.ming.lee@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 232 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 229 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ce71717..809e03b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -39,10 +39,37 @@
>  #include "i915_trace.h"
>  #include <drm/drm_dp_helper.h>
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_plane_helper.h>
> +#include <drm/drm_rect.h>
>  #include <linux/dma_remapping.h>
>  
> +/* Primary plane formats supported by all gen */
> +#define COMMON_PRIMARY_FORMATS \
> +	DRM_FORMAT_C8, \
> +	DRM_FORMAT_RGB565, \
> +	DRM_FORMAT_XRGB8888, \
> +	DRM_FORMAT_ARGB8888
> +
> +/* Primary plane formats for gen <= 3 */
> +static const uint32_t intel_primary_formats_gen2[] = {
> +	COMMON_PRIMARY_FORMATS,
> +	DRM_FORMAT_XRGB1555,
> +	DRM_FORMAT_ARGB1555,
> +};
> +
> +/* Primary plane formats for gen >= 4 */
> +static const uint32_t intel_primary_formats_gen4[] = {
> +	COMMON_PRIMARY_FORMATS, \
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_XRGB2101010,
> +	DRM_FORMAT_ARGB2101010,
> +	DRM_FORMAT_XBGR2101010,
> +	DRM_FORMAT_ABGR2101010,
> +};
> +
>  #define DIV_ROUND_CLOSEST_ULL(ll, d)	\
> -	({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
> +({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
>  
>  static void intel_increase_pllclock(struct drm_crtc *crtc);
>  static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
> @@ -10824,17 +10851,216 @@ static void intel_shared_dpll_init(struct drm_device *dev)
>  	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
>  }
>  
> +static int
> +intel_primary_plane_disable(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc;
> +
> +	if (!plane->fb)
> +		return 0;
> +
> +	BUG_ON(!plane->crtc);
> +
> +	intel_crtc = to_intel_crtc(plane->crtc);
> +
> +	/*
> +	 * Even though we checked plane->fb above, it's still possible that
> +	 * the primary plane has been implicitly disabled because the crtc
> +	 * coordinates given weren't visible, or because we detected
> +	 * that it was 100% covered by a sprite plane.  Or, the CRTC may be
> +	 * off and we've set a fb, but haven't actually turned on the CRTC yet.
> +	 * In either case, we need to unpin the FB and let the fb pointer get
> +	 * updated, but otherwise we don't need to touch the hardware.
> +	 */
> +	if (!intel_crtc->primary_enabled)
> +		goto disable_unpin;
> +
> +	intel_crtc_wait_for_pending_flips(plane->crtc);
> +	intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
> +				       intel_plane->pipe);
> +
> +disable_unpin:
> +	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
> +	plane->fb = NULL;
> +
> +	return 0;
> +}
> +
> +static int
> +intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
> +			     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> +			     unsigned int crtc_w, unsigned int crtc_h,
> +			     uint32_t src_x, uint32_t src_y,
> +			     uint32_t src_w, uint32_t src_h)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct drm_rect dest = {
> +		/* integer pixels */
> +		.x1 = crtc_x,
> +		.y1 = crtc_y,
> +		.x2 = crtc_x + crtc_w,
> +		.y2 = crtc_y + crtc_h,
> +	};
> +	struct drm_rect src = {
> +		/* 16.16 fixed point */
> +		.x1 = src_x,
> +		.y1 = src_y,
> +		.x2 = src_x + src_w,
> +		.y2 = src_y + src_h,
> +	};
> +	const struct drm_rect clip = {
> +		/* integer pixels */
> +		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
> +		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
> +	};
> +	bool visible;
> +	int ret;
> +
> +	ret = drm_plane_helper_check_update(plane, crtc, fb,
> +					    &src, &dest, &clip,
> +					    DRM_PLANE_HELPER_NO_SCALING,
> +					    DRM_PLANE_HELPER_NO_SCALING,
> +					    false, true, &visible);
> +
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
> +	 * updating the fb pointer, and returning without touching the
> +	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
> +	 * turn on the display with all planes setup as desired.
> +	 */
> +	if (!crtc->enabled) {
> +		/*
> +		 * If we already called setplane while the crtc was disabled,
> +		 * we may have an fb pinned; unpin it.
> +		 */
> +		if (plane->fb)
> +			intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
> +
> +		/* Pin and return without programming hardware */
> +		return intel_pin_and_fence_fb_obj(dev,
> +						  to_intel_framebuffer(fb)->obj,
> +						  NULL);
> +	}
> +
> +	intel_crtc_wait_for_pending_flips(crtc);
> +
> +	/*
> +	 * If clipping results in a non-visible primary plane, we'll disable
> +	 * the primary plane.  Note that this is a bit different than what
> +	 * happens if userspace explicitly disables the plane by passing fb=0
> +	 * because plane->fb still gets set and pinned.
> +	 */
> +	if (!visible) {
> +		/*
> +		 * Try to pin the new fb first so that we can bail out if we
> +		 * fail.
> +		 */
> +		if (plane->fb != fb) {
> +			ret = intel_pin_and_fence_fb_obj(dev,
> +							 to_intel_framebuffer(fb)->obj,
> +							 NULL);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		if (intel_crtc->primary_enabled)
> +			intel_disable_primary_hw_plane(dev_priv,
> +						       intel_plane->plane,
> +						       intel_plane->pipe);
> +
> +
> +		if (plane->fb != fb)
> +			if (plane->fb)
> +				intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
> +
> +		return 0;
> +	}
> +
> +	ret = intel_pipe_set_base(crtc, src.x1, src.y1, fb);
> +	if (ret)
> +		return ret;
> +
> +	if (!intel_crtc->primary_enabled)
> +		intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> +					      intel_crtc->pipe);
> +
> +	return 0;
> +}
> +
> +static void intel_primary_plane_destroy(struct drm_plane *plane)
> +{
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	drm_plane_cleanup(plane);
> +	kfree(intel_plane);
> +}
> +
> +static const struct drm_plane_funcs intel_primary_plane_funcs = {
> +	.update_plane = intel_primary_plane_setplane,
> +	.disable_plane = intel_primary_plane_disable,
> +	.destroy = intel_primary_plane_destroy,
> +};
> +
> +static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> +						    int pipe)
> +{
> +	struct intel_plane *primary;
> +	const uint32_t *intel_primary_formats;
> +	int num_formats;
> +
> +	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> +	if (primary == NULL)
> +		return NULL;
> +
> +	primary->can_scale = false;
> +	primary->max_downscale = 1;
> +	primary->pipe = pipe;
> +	primary->plane = pipe;
> +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> +		primary->plane = !pipe;
> +
> +	if (INTEL_INFO(dev)->gen <= 3) {
> +		intel_primary_formats = intel_primary_formats_gen2;
> +		num_formats = ARRAY_SIZE(intel_primary_formats_gen2);
> +	} else {
> +		intel_primary_formats = intel_primary_formats_gen4;
> +		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
> +	}
> +
> +	drm_universal_plane_init(dev, &primary->base, 0,
> +				 &intel_primary_plane_funcs,
> +				 intel_primary_formats, num_formats,
> +				 DRM_PLANE_TYPE_PRIMARY);
> +	return &primary->base;
> +}
> +
>  static void intel_crtc_init(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc;
> -	int i;
> +	struct drm_plane *primary;
> +	int i, ret;
>  
>  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
>  	if (intel_crtc == NULL)
>  		return;
>  
> -	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
> +	primary = intel_primary_plane_create(dev, pipe);
> +	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> +					NULL, &intel_crtc_funcs);
> +	if (ret) {
> +		drm_plane_cleanup(primary);
> +		kfree(intel_crtc);
> +		return;
> +	}
>  
>  	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
>  	for (i = 0; i < 256; i++) {
> -- 
> 1.8.5.1
>