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

[PATCH 1/3] 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 b72736d..65c4a00 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/3] drm/plane-helper: Add drm_primary_helper_check_update()

[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ä.

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 | 148 +++++++++++++++++++++++++++++--------
 include/drm/drm_plane_helper.h     |   9 +++
 2 files changed, 128 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index 65c4a00..9bbbdf2 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -66,6 +66,102 @@ 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
+ * @crtc_x: x offset of primary plane on crtc
+ * @crtc_y: y offset of primary plane on crtc
+ * @crtc_w: width of primary plane rectangle on crtc
+ * @crtc_h: height of primary plane rectangle on crtc
+ * @src_x: x offset of @fb for panning
+ * @src_y: y offset of @fb for panning
+ * @src_w: width of source rectangle in @fb
+ * @src_h: height of source rectangle in @fb
+ * @can_scale: is primary plane scaling legal?
+ * @can_position: is it legal to position the primary plane such that it
+ *                doesn't cover the entire crtc?
+ *
+ * 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,
+				    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,
+				    bool can_scale,
+				    bool can_position)
+{
+	struct drm_rect dest = {
+		.x1 = crtc_x,
+		.y1 = crtc_y,
+		.x2 = crtc_x + crtc_w,
+		.y2 = crtc_y + crtc_h,
+	};
+	struct drm_rect src = {
+		.x1 = src_x >> 16,
+		.y1 = src_y >> 16,
+		.x2 = (src_x + src_w) >> 16,
+		.y2 = (src_y + src_h) >> 16,
+	};
+	const struct drm_rect clip = {
+		.x2 = crtc->mode.hdisplay,
+		.y2 = crtc->mode.vdisplay,
+	};
+	int hscale, vscale;
+	int visible;
+
+	if (!crtc->enabled) {
+		DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n");
+		return -EINVAL;
+	}
+
+	/* setplane API takes shifted source rectangle values; unshift them */
+	src_x >>= 16;
+	src_y >>= 16;
+	src_w >>= 16;
+	src_h >>= 16;
+
+	/* check scaling */
+	if (!can_scale && (crtc_w != src_w || crtc_h != src_h)) {
+		DRM_DEBUG_KMS("Can't scale primary plane\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Drivers that can scale should perform their own min/max scale
+	 * factor checks.
+	 */
+	hscale = drm_rect_calc_hscale(&src, &dest, 0, INT_MAX);
+	vscale = drm_rect_calc_vscale(&src, &dest, 0, INT_MAX);
+	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,6 +209,12 @@ 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 >> 16,
+		.y1 = src_y >> 16,
+		.x2 = (src_x + src_w) >> 16,
+		.y2 = (src_y + src_h) >> 16,
+	};
 	struct drm_rect dest = {
 		.x1 = crtc_x,
 		.y1 = crtc_y,
@@ -124,40 +226,28 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		.y2 = crtc->mode.vdisplay,
 	};
 	struct drm_connector **connector_list;
+	int visible;
+	int hscale, vscale;
 	int num_connectors, ret;
 
-	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,
+					      crtc_x, crtc_y, crtc_w, crtc_h,
+					      src_x, src_y, src_w, src_h,
+					      false, false);
 	if (ret)
 		return ret;
 
+	hscale = drm_rect_calc_hscale(&src, &dest, 0, INT_MAX);
+	vscale = drm_rect_calc_vscale(&src, &dest, 0, INT_MAX);
+	visible = drm_rect_clip_scaled(&src, &dest, &clip, hscale, vscale);
+	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..373201a 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -31,6 +31,15 @@
  * planes.
  */
 
+extern int drm_primary_helper_check_update(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,
+					   bool can_scale,
+					   bool can_position);
 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

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

[Ville Syrjälä]

On Wed, Apr 23, 2014 at 10:04:00AM -0700, 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ä.
> 
> 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 | 148 +++++++++++++++++++++++++++++--------
>  include/drm/drm_plane_helper.h     |   9 +++
>  2 files changed, 128 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index 65c4a00..9bbbdf2 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -66,6 +66,102 @@ 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
> + * @crtc_x: x offset of primary plane on crtc
> + * @crtc_y: y offset of primary plane on crtc
> + * @crtc_w: width of primary plane rectangle on crtc
> + * @crtc_h: height of primary plane rectangle on crtc
> + * @src_x: x offset of @fb for panning
> + * @src_y: y offset of @fb for panning
> + * @src_w: width of source rectangle in @fb
> + * @src_h: height of source rectangle in @fb
> + * @can_scale: is primary plane scaling legal?
> + * @can_position: is it legal to position the primary plane such that it
> + *                doesn't cover the entire crtc?
> + *
> + * 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,
> +				    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,
> +				    bool can_scale,
> +				    bool can_position)
> +{
> +	struct drm_rect dest = {
> +		.x1 = crtc_x,
> +		.y1 = crtc_y,
> +		.x2 = crtc_x + crtc_w,
> +		.y2 = crtc_y + crtc_h,
> +	};
> +	struct drm_rect src = {
> +		.x1 = src_x >> 16,
> +		.y1 = src_y >> 16,
> +		.x2 = (src_x + src_w) >> 16,
> +		.y2 = (src_y + src_h) >> 16,

I think you want '(x>>16) + (y>>16)' instead. Otherwise you may end up
increasing the source width/height.

> +	};
> +	const struct drm_rect clip = {
> +		.x2 = crtc->mode.hdisplay,
> +		.y2 = crtc->mode.vdisplay,
> +	};
> +	int hscale, vscale;
> +	int visible;
> +
> +	if (!crtc->enabled) {
> +		DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n");
> +		return -EINVAL;
> +	}

We allow this for sprites, so I'd allow it for everything. I'd be fine
with leaving this restriction in drm_primary_helper_update() simply
because I have no interst in auditing every other driver.

Although I think we still overwrite the primary plane configure during
setcrtc. We should really change that so that the user can pre-configure
all the planes and then watch them pop into view during a modeset as
previously configured.

> +
> +	/* setplane API takes shifted source rectangle values; unshift them */
> +	src_x >>= 16;
> +	src_y >>= 16;
> +	src_w >>= 16;
> +	src_h >>= 16;
> +
> +	/* check scaling */
> +	if (!can_scale && (crtc_w != src_w || crtc_h != src_h)) {
> +		DRM_DEBUG_KMS("Can't scale primary plane\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Drivers that can scale should perform their own min/max scale
> +	 * factor checks.
> +	 */
> +	hscale = drm_rect_calc_hscale(&src, &dest, 0, INT_MAX);
> +	vscale = drm_rect_calc_vscale(&src, &dest, 0, INT_MAX);
> +	visible = drm_rect_clip_scaled(&src, &dest, &clip, hscale, vscale);

w/o sub-pixel coordinates the scaling factors will be truncated
to integers, which makes the clipping rather bogus if the plane can
actually scale. I think I'd just make this code assume that scaling
isn't supported, and if the driver supports scaling it can just
implent the appropriate code itself. We can try to refactor some of
the scaling aware code from intel_sprite later if warranted.

But I'm starting to question the usefulness of this function. We
anyway have to reclip in i915 code due to stereo/doublewide/etc,
and actually you also reclip in the plane helper since the
clipped coordinates aren't passed back to the caller. In order to
avoid the extra work, I'd just have the caller pass in all the
drm_rects. That would actually make this function useful even for
i915 since you could then pass the correct clip rect rather than
assume that it comes from crtc->mode.

> +	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,6 +209,12 @@ 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 >> 16,
> +		.y1 = src_y >> 16,
> +		.x2 = (src_x + src_w) >> 16,
> +		.y2 = (src_y + src_h) >> 16,
> +	};
>  	struct drm_rect dest = {
>  		.x1 = crtc_x,
>  		.y1 = crtc_y,
> @@ -124,40 +226,28 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
>  		.y2 = crtc->mode.vdisplay,
>  	};
>  	struct drm_connector **connector_list;
> +	int visible;
> +	int hscale, vscale;
>  	int num_connectors, ret;
>  
> -	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,
> +					      crtc_x, crtc_y, crtc_w, crtc_h,
> +					      src_x, src_y, src_w, src_h,
> +					      false, false);
>  	if (ret)
>  		return ret;
>  
> +	hscale = drm_rect_calc_hscale(&src, &dest, 0, INT_MAX);
> +	vscale = drm_rect_calc_vscale(&src, &dest, 0, INT_MAX);
> +	visible = drm_rect_clip_scaled(&src, &dest, &clip, hscale, vscale);
> +	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..373201a 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -31,6 +31,15 @@
>   * planes.
>   */
>  
> +extern int drm_primary_helper_check_update(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,
> +					   bool can_scale,
> +					   bool can_position);
>  extern int drm_primary_helper_update(struct drm_plane *plane,
>  				     struct drm_crtc *crtc,
>  				     struct drm_framebuffer *fb,
> -- 
> 1.8.5.1

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 1/3] drm: Check CRTC compatibility in setplane

[Daniel Vetter]

On Wed, Apr 23, 2014 at 10:03:59AM -0700, Matt Roper wrote:
> 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>

Do you have a nasty igt somewhere which tries to use a plane on the wrong
crtc? Especially useful since our i915 code and hw relies on this.
-Daniel

> ---
>  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 b72736d..65c4a00 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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Matt Roper]

On Wed, Apr 23, 2014 at 08:03:50PM +0200, Daniel Vetter wrote:
> On Wed, Apr 23, 2014 at 10:03:59AM -0700, Matt Roper wrote:
> > 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>
> 
> Do you have a nasty igt somewhere which tries to use a plane on the wrong
> crtc? Especially useful since our i915 code and hw relies on this.
> -Daniel

Not yet; I'll add/modify a test to include that.  I'm still working on
some other i-g-t test updates for the primary plane stuff based on your
previous feedback.

Speaking of i-g-t testing, what is the expected behavior if someone
issues a pageflip ioctl while the primary plane is disabled?  I'd expect
it to return an error, but the -EBUSY currently returned by the DRM core
seems a bit confusing/misleading in my opinion.  The comments indicate
that the existing assumption is that crtc->primary->fb == NULL indicates 
a hotplug event that userspace hasn't noticed yet, although now we
clearly have other ways to hit that error, so I'm not sure EBUSY is
really the right response.


Matt

> 
> > ---
> >  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 b72736d..65c4a00 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
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

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

[Daniel Vetter]

On Wed, Apr 23, 2014 at 11:26:04AM -0700, Matt Roper wrote:
> On Wed, Apr 23, 2014 at 08:03:50PM +0200, Daniel Vetter wrote:
> > On Wed, Apr 23, 2014 at 10:03:59AM -0700, Matt Roper wrote:
> > > 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>
> > 
> > Do you have a nasty igt somewhere which tries to use a plane on the wrong
> > crtc? Especially useful since our i915 code and hw relies on this.
> > -Daniel
> 
> Not yet; I'll add/modify a test to include that.  I'm still working on
> some other i-g-t test updates for the primary plane stuff based on your
> previous feedback.
> 
> Speaking of i-g-t testing, what is the expected behavior if someone
> issues a pageflip ioctl while the primary plane is disabled?  I'd expect
> it to return an error, but the -EBUSY currently returned by the DRM core
> seems a bit confusing/misleading in my opinion.  The comments indicate
> that the existing assumption is that crtc->primary->fb == NULL indicates 
> a hotplug event that userspace hasn't noticed yet, although now we
> clearly have other ways to hit that error, so I'm not sure EBUSY is
> really the right response.

That comment is outdated since nowadays the kernel doesn't randomly kill a
crtc if its outputs gets unplugged. I think a simple -EINVAL should be
good. We'd need to update kms_flip with that new value though.

A quick grep through the intel ddx shows that we don't really depend on
this either way. -EBUSY for a disabled primary plane (whether the crtc is
on or not doesn't matter imo) really makes no sense.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch