diff mbox

drm/i915 : clip yuv primary planes to hw constraints

Message ID 20180525180023.204274-1-frkoenig@google.com (mailing list archive)
State New, archived
Headers show

Commit Message

Fritz Koenig May 25, 2018, 6 p.m. UTC
YUV planes need to be multiples of 2 to scan out. This was
handled correctly for planes other than the primary in the
intel_check_sprite_plane, where the code fixes up the plane
and makes it compliant. Move this code into a location that
allows the primary check to access it as well.

Signed-off-by: Fritz Koenig <frkoenig@google.com>
---

Hi,

I think this is a better implementation where instead of rejecting
yuv planes that are not correctly aligned, they are fixed up.  This
is done by reusing the sprite check code that was already doing that.

 drivers/gpu/drm/i915/intel_display.c | 170 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_sprite.c  | 154 +-----------------------
 3 files changed, 175 insertions(+), 151 deletions(-)

Comments

Ville Syrjälä May 25, 2018, 6:12 p.m. UTC | #1
On Fri, May 25, 2018 at 11:00:23AM -0700, Fritz Koenig wrote:
> YUV planes need to be multiples of 2 to scan out. This was
> handled correctly for planes other than the primary in the
> intel_check_sprite_plane, where the code fixes up the plane
> and makes it compliant. Move this code into a location that
> allows the primary check to access it as well.
> 
> Signed-off-by: Fritz Koenig <frkoenig@google.com>
> ---
> 
> Hi,
> 
> I think this is a better implementation where instead of rejecting
> yuv planes that are not correctly aligned, they are fixed up.  This
> is done by reusing the sprite check code that was already doing that.
> 
>  drivers/gpu/drm/i915/intel_display.c | 170 +++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>  drivers/gpu/drm/i915/intel_sprite.c  | 154 +-----------------------
>  3 files changed, 175 insertions(+), 151 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 56004ffbd8bb..1328fb90367f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12854,6 +12854,170 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state
>  	return max_scale;
>  }
>  
> +int
> +intel_clip_src_rect(struct intel_plane *plane,
> +		    struct intel_crtc_state *crtc_state,
> +		    struct intel_plane_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_framebuffer *fb = state->base.fb;
> +	int crtc_x, crtc_y;
> +	unsigned int crtc_w, crtc_h;
> +	uint32_t src_x, src_y, src_w, src_h;
> +	struct drm_rect *src = &state->base.src;
> +	struct drm_rect *dst = &state->base.dst;
> +	struct drm_rect clip = {};
> +	int hscale, vscale;
> +	int max_scale, min_scale;
> +	bool can_scale;
> +
> +	*src = drm_plane_state_src(&state->base);
> +	*dst = drm_plane_state_dest(&state->base);
> +
> +	/* setup can_scale, min_scale, max_scale */
> +	if (INTEL_GEN(dev_priv) >= 9) {
> +		/* use scaler when colorkey is not required */
> +		if (!state->ckey.flags) {
> +			can_scale = 1;
> +			min_scale = 1;
> +			max_scale = skl_max_scale(crtc, crtc_state);
> +		} else {
> +			can_scale = 0;
> +			min_scale = DRM_PLANE_HELPER_NO_SCALING;
> +			max_scale = DRM_PLANE_HELPER_NO_SCALING;
> +		}
> +	} else {
> +		can_scale = plane->can_scale;
> +		max_scale = plane->max_downscale << 16;
> +		min_scale = plane->can_scale ? 1 : (1 << 16);
> +	}
> +
> +	/*
> +	 * FIXME the following code does a bunch of fuzzy adjustments to the
> +	 * coordinates and sizes. We probably need some way to decide whether
> +	 * more strict checking should be done instead.
> +	 */
> +	drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> +			state->base.rotation);
> +
> +	hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale);
> +	BUG_ON(hscale < 0);
> +
> +	vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
> +	BUG_ON(vscale < 0);

Your baseline is already outdated.
Fritz Koenig May 25, 2018, 8:48 p.m. UTC | #2
On Fri, May 25, 2018 at 11:12 AM Ville Syrjälä <
ville.syrjala@linux.intel.com> wrote:

> On Fri, May 25, 2018 at 11:00:23AM -0700, Fritz Koenig wrote:
> > YUV planes need to be multiples of 2 to scan out. This was
> > handled correctly for planes other than the primary in the
> > intel_check_sprite_plane, where the code fixes up the plane
> > and makes it compliant. Move this code into a location that
> > allows the primary check to access it as well.
> >
> > Signed-off-by: Fritz Koenig <frkoenig@google.com>
> > ---
> >
> > Hi,
> >
> > I think this is a better implementation where instead of rejecting
> > yuv planes that are not correctly aligned, they are fixed up.  This
> > is done by reusing the sprite check code that was already doing that.
> >
> >  drivers/gpu/drm/i915/intel_display.c | 170 +++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |   2 +
> >  drivers/gpu/drm/i915/intel_sprite.c  | 154 +-----------------------
> >  3 files changed, 175 insertions(+), 151 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
> > index 56004ffbd8bb..1328fb90367f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12854,6 +12854,170 @@ skl_max_scale(struct intel_crtc *intel_crtc,
struct intel_crtc_state *crtc_state
> >       return max_scale;
> >  }
> >
> > +int
> > +intel_clip_src_rect(struct intel_plane *plane,
> > +                 struct intel_crtc_state *crtc_state,
> > +                 struct intel_plane_state *state)
> > +{
> > +     struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +     struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > +     struct drm_framebuffer *fb = state->base.fb;
> > +     int crtc_x, crtc_y;
> > +     unsigned int crtc_w, crtc_h;
> > +     uint32_t src_x, src_y, src_w, src_h;
> > +     struct drm_rect *src = &state->base.src;
> > +     struct drm_rect *dst = &state->base.dst;
> > +     struct drm_rect clip = {};
> > +     int hscale, vscale;
> > +     int max_scale, min_scale;
> > +     bool can_scale;
> > +
> > +     *src = drm_plane_state_src(&state->base);
> > +     *dst = drm_plane_state_dest(&state->base);
> > +
> > +     /* setup can_scale, min_scale, max_scale */
> > +     if (INTEL_GEN(dev_priv) >= 9) {
> > +             /* use scaler when colorkey is not required */
> > +             if (!state->ckey.flags) {
> > +                     can_scale = 1;
> > +                     min_scale = 1;
> > +                     max_scale = skl_max_scale(crtc, crtc_state);
> > +             } else {
> > +                     can_scale = 0;
> > +                     min_scale = DRM_PLANE_HELPER_NO_SCALING;
> > +                     max_scale = DRM_PLANE_HELPER_NO_SCALING;
> > +             }
> > +     } else {
> > +             can_scale = plane->can_scale;
> > +             max_scale = plane->max_downscale << 16;
> > +             min_scale = plane->can_scale ? 1 : (1 << 16);
> > +     }
> > +
> > +     /*
> > +      * FIXME the following code does a bunch of fuzzy adjustments to
the
> > +      * coordinates and sizes. We probably need some way to decide
whether
> > +      * more strict checking should be done instead.
> > +      */
> > +     drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> > +                     state->base.rotation);
> > +
> > +     hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale,
max_scale);
> > +     BUG_ON(hscale < 0);
> > +
> > +     vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale,
max_scale);
> > +     BUG_ON(vscale < 0);

> Your baseline is already outdated.


Sorry, I was on the incorrect branch.

I tried to retarget at drm-intel-fixes, but this still appears to be
incorrect as it is still failing to patch.  Should I be targeting
drm-intel-next instead?

> --
> Ville Syrjälä
> Intel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 56004ffbd8bb..1328fb90367f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12854,6 +12854,170 @@  skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state
 	return max_scale;
 }
 
+int
+intel_clip_src_rect(struct intel_plane *plane,
+		    struct intel_crtc_state *crtc_state,
+		    struct intel_plane_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_framebuffer *fb = state->base.fb;
+	int crtc_x, crtc_y;
+	unsigned int crtc_w, crtc_h;
+	uint32_t src_x, src_y, src_w, src_h;
+	struct drm_rect *src = &state->base.src;
+	struct drm_rect *dst = &state->base.dst;
+	struct drm_rect clip = {};
+	int hscale, vscale;
+	int max_scale, min_scale;
+	bool can_scale;
+
+	*src = drm_plane_state_src(&state->base);
+	*dst = drm_plane_state_dest(&state->base);
+
+	/* setup can_scale, min_scale, max_scale */
+	if (INTEL_GEN(dev_priv) >= 9) {
+		/* use scaler when colorkey is not required */
+		if (!state->ckey.flags) {
+			can_scale = 1;
+			min_scale = 1;
+			max_scale = skl_max_scale(crtc, crtc_state);
+		} else {
+			can_scale = 0;
+			min_scale = DRM_PLANE_HELPER_NO_SCALING;
+			max_scale = DRM_PLANE_HELPER_NO_SCALING;
+		}
+	} else {
+		can_scale = plane->can_scale;
+		max_scale = plane->max_downscale << 16;
+		min_scale = plane->can_scale ? 1 : (1 << 16);
+	}
+
+	/*
+	 * FIXME the following code does a bunch of fuzzy adjustments to the
+	 * coordinates and sizes. We probably need some way to decide whether
+	 * more strict checking should be done instead.
+	 */
+	drm_rect_rotate(src, fb->width << 16, fb->height << 16,
+			state->base.rotation);
+
+	hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale);
+	BUG_ON(hscale < 0);
+
+	vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
+	BUG_ON(vscale < 0);
+
+	if (crtc_state->base.enable)
+		drm_mode_get_hv_timing(&crtc_state->base.mode,
+				       &clip.x2, &clip.y2);
+
+	state->base.visible = drm_rect_clip_scaled(src, dst, &clip, hscale, vscale);
+
+	crtc_x = dst->x1;
+	crtc_y = dst->y1;
+	crtc_w = drm_rect_width(dst);
+	crtc_h = drm_rect_height(dst);
+
+	if (state->base.visible) {
+		/* check again in case clipping clamped the results */
+		hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
+		if (hscale < 0) {
+			DRM_DEBUG_KMS("Horizontal scaling factor out of limits\n");
+			drm_rect_debug_print("src: ", src, true);
+			drm_rect_debug_print("dst: ", dst, false);
+
+			return hscale;
+		}
+
+		vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
+		if (vscale < 0) {
+			DRM_DEBUG_KMS("Vertical scaling factor out of limits\n");
+			drm_rect_debug_print("src: ", src, true);
+			drm_rect_debug_print("dst: ", dst, false);
+
+			return vscale;
+		}
+
+		/* Make the source viewport size an exact multiple of the scaling factors. */
+		drm_rect_adjust_size(src,
+				     drm_rect_width(dst) * hscale - drm_rect_width(src),
+				     drm_rect_height(dst) * vscale - drm_rect_height(src));
+
+		drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
+				    state->base.rotation);
+
+		/* sanity check to make sure the src viewport wasn't enlarged */
+		WARN_ON(src->x1 < (int) state->base.src_x ||
+			src->y1 < (int) state->base.src_y ||
+			src->x2 > (int) state->base.src_x + state->base.src_w ||
+			src->y2 > (int) state->base.src_y + state->base.src_h);
+
+		/*
+		 * Hardware doesn't handle subpixel coordinates.
+		 * Adjust to (macro)pixel boundary, but be careful not to
+		 * increase the source viewport size, because that could
+		 * push the downscaling factor out of bounds.
+		 */
+		src_x = src->x1 >> 16;
+		src_w = drm_rect_width(src) >> 16;
+		src_y = src->y1 >> 16;
+		src_h = drm_rect_height(src) >> 16;
+
+		if (intel_format_is_yuv(fb->format->format)) {
+			src_x &= ~1;
+			src_w &= ~1;
+
+			/*
+			 * Must keep src and dst the
+			 * same if we can't scale.
+			 */
+			if (!can_scale)
+				crtc_w &= ~1;
+
+			if (crtc_w == 0)
+				state->base.visible = false;
+		}
+	}
+
+	/* Check size restrictions when scaling */
+	if (state->base.visible && (src_w != crtc_w || src_h != crtc_h)) {
+		unsigned int width_bytes;
+		int cpp = fb->format->cpp[0];
+
+		WARN_ON(!can_scale);
+
+		/* FIXME interlacing min height is 6 */
+
+		if (crtc_w < 3 || crtc_h < 3)
+			state->base.visible = false;
+
+		if (src_w < 3 || src_h < 3)
+			state->base.visible = false;
+
+		width_bytes = ((src_x * cpp) & 63) + src_w * cpp;
+
+		if (INTEL_GEN(dev_priv) < 9 && (src_w > 2048 || src_h > 2048 ||
+		    width_bytes > 4096 || fb->pitches[0] > 4096)) {
+			DRM_DEBUG_KMS("Source dimensions exceed hardware limits\n");
+			return -EINVAL;
+		}
+	}
+
+	if (state->base.visible) {
+		src->x1 = src_x << 16;
+		src->x2 = (src_x + src_w) << 16;
+		src->y1 = src_y << 16;
+		src->y2 = (src_y + src_h) << 16;
+	}
+
+	dst->x1 = crtc_x;
+	dst->x2 = crtc_x + crtc_w;
+	dst->y1 = crtc_y;
+	dst->y2 = crtc_y + crtc_h;
+
+	return 0;
+}
+
 static int
 intel_check_primary_plane(struct intel_plane *plane,
 			  struct intel_crtc_state *crtc_state,
@@ -12885,6 +13049,12 @@  intel_check_primary_plane(struct intel_plane *plane,
 	if (!state->base.fb)
 		return 0;
 
+	if (intel_format_is_yuv(state->base.fb->format->format)) {
+		ret = intel_clip_src_rect(plane, crtc_state, state);
+		if (ret)
+			return ret;
+	}
+
 	if (INTEL_GEN(dev_priv) >= 9) {
 		ret = skl_check_plane_surface(crtc_state, state);
 		if (ret)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a80fbad9be0f..43847927a927 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1591,6 +1591,8 @@  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 
 int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
 int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
+int intel_clip_src_rect(struct intel_plane *plane,
+			struct intel_crtc_state *crtc_state, struct intel_plane_state *state);
 
 static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
 {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index dbdcf85032df..892d3247ed69 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -935,21 +935,9 @@  intel_check_sprite_plane(struct intel_plane *plane,
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_framebuffer *fb = state->base.fb;
-	int crtc_x, crtc_y;
-	unsigned int crtc_w, crtc_h;
-	uint32_t src_x, src_y, src_w, src_h;
-	struct drm_rect *src = &state->base.src;
-	struct drm_rect *dst = &state->base.dst;
-	struct drm_rect clip = {};
 	int max_stride = INTEL_GEN(dev_priv) >= 9 ? 32768 : 16384;
-	int hscale, vscale;
-	int max_scale, min_scale;
-	bool can_scale;
 	int ret;
 
-	*src = drm_plane_state_src(&state->base);
-	*dst = drm_plane_state_dest(&state->base);
-
 	if (!fb) {
 		state->base.visible = false;
 		return 0;
@@ -967,145 +955,9 @@  intel_check_sprite_plane(struct intel_plane *plane,
 		return -EINVAL;
 	}
 
-	/* setup can_scale, min_scale, max_scale */
-	if (INTEL_GEN(dev_priv) >= 9) {
-		/* use scaler when colorkey is not required */
-		if (!state->ckey.flags) {
-			can_scale = 1;
-			min_scale = 1;
-			max_scale = skl_max_scale(crtc, crtc_state);
-		} else {
-			can_scale = 0;
-			min_scale = DRM_PLANE_HELPER_NO_SCALING;
-			max_scale = DRM_PLANE_HELPER_NO_SCALING;
-		}
-	} else {
-		can_scale = plane->can_scale;
-		max_scale = plane->max_downscale << 16;
-		min_scale = plane->can_scale ? 1 : (1 << 16);
-	}
-
-	/*
-	 * FIXME the following code does a bunch of fuzzy adjustments to the
-	 * coordinates and sizes. We probably need some way to decide whether
-	 * more strict checking should be done instead.
-	 */
-	drm_rect_rotate(src, fb->width << 16, fb->height << 16,
-			state->base.rotation);
-
-	hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale);
-	BUG_ON(hscale < 0);
-
-	vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
-	BUG_ON(vscale < 0);
-
-	if (crtc_state->base.enable)
-		drm_mode_get_hv_timing(&crtc_state->base.mode,
-				       &clip.x2, &clip.y2);
-
-	state->base.visible = drm_rect_clip_scaled(src, dst, &clip, hscale, vscale);
-
-	crtc_x = dst->x1;
-	crtc_y = dst->y1;
-	crtc_w = drm_rect_width(dst);
-	crtc_h = drm_rect_height(dst);
-
-	if (state->base.visible) {
-		/* check again in case clipping clamped the results */
-		hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
-		if (hscale < 0) {
-			DRM_DEBUG_KMS("Horizontal scaling factor out of limits\n");
-			drm_rect_debug_print("src: ", src, true);
-			drm_rect_debug_print("dst: ", dst, false);
-
-			return hscale;
-		}
-
-		vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
-		if (vscale < 0) {
-			DRM_DEBUG_KMS("Vertical scaling factor out of limits\n");
-			drm_rect_debug_print("src: ", src, true);
-			drm_rect_debug_print("dst: ", dst, false);
-
-			return vscale;
-		}
-
-		/* Make the source viewport size an exact multiple of the scaling factors. */
-		drm_rect_adjust_size(src,
-				     drm_rect_width(dst) * hscale - drm_rect_width(src),
-				     drm_rect_height(dst) * vscale - drm_rect_height(src));
-
-		drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
-				    state->base.rotation);
-
-		/* sanity check to make sure the src viewport wasn't enlarged */
-		WARN_ON(src->x1 < (int) state->base.src_x ||
-			src->y1 < (int) state->base.src_y ||
-			src->x2 > (int) state->base.src_x + state->base.src_w ||
-			src->y2 > (int) state->base.src_y + state->base.src_h);
-
-		/*
-		 * Hardware doesn't handle subpixel coordinates.
-		 * Adjust to (macro)pixel boundary, but be careful not to
-		 * increase the source viewport size, because that could
-		 * push the downscaling factor out of bounds.
-		 */
-		src_x = src->x1 >> 16;
-		src_w = drm_rect_width(src) >> 16;
-		src_y = src->y1 >> 16;
-		src_h = drm_rect_height(src) >> 16;
-
-		if (intel_format_is_yuv(fb->format->format)) {
-			src_x &= ~1;
-			src_w &= ~1;
-
-			/*
-			 * Must keep src and dst the
-			 * same if we can't scale.
-			 */
-			if (!can_scale)
-				crtc_w &= ~1;
-
-			if (crtc_w == 0)
-				state->base.visible = false;
-		}
-	}
-
-	/* Check size restrictions when scaling */
-	if (state->base.visible && (src_w != crtc_w || src_h != crtc_h)) {
-		unsigned int width_bytes;
-		int cpp = fb->format->cpp[0];
-
-		WARN_ON(!can_scale);
-
-		/* FIXME interlacing min height is 6 */
-
-		if (crtc_w < 3 || crtc_h < 3)
-			state->base.visible = false;
-
-		if (src_w < 3 || src_h < 3)
-			state->base.visible = false;
-
-		width_bytes = ((src_x * cpp) & 63) + src_w * cpp;
-
-		if (INTEL_GEN(dev_priv) < 9 && (src_w > 2048 || src_h > 2048 ||
-		    width_bytes > 4096 || fb->pitches[0] > 4096)) {
-			DRM_DEBUG_KMS("Source dimensions exceed hardware limits\n");
-			return -EINVAL;
-		}
-	}
-
-	if (state->base.visible) {
-		src->x1 = src_x << 16;
-		src->x2 = (src_x + src_w) << 16;
-		src->y1 = src_y << 16;
-		src->y2 = (src_y + src_h) << 16;
-	}
-
-	dst->x1 = crtc_x;
-	dst->x2 = crtc_x + crtc_w;
-	dst->y1 = crtc_y;
-	dst->y2 = crtc_y + crtc_h;
+	ret = intel_clip_src_rect(plane, crtc_state, state);
+	if (ret)
+		return ret;
 
 	if (INTEL_GEN(dev_priv) >= 9) {
 		ret = skl_check_plane_surface(crtc_state, state);