[PATCH] drm: drm_rect and clipping for intel sprite planes

[PATCH] drm: drm_rect and clipping for intel sprite planes

[ville.syrjala]

I'm re-sending this whole plane clipping series, as it has been a quite
while since the last time I posted it.

Laurent went through patch 1 and I made the requested changes, and I also
fixed a minor issue in patch 3 that I spotted myself.

The other patches are unchanged and looking for volunteers to review them...

[PATCH v5 1/4] drm: Add struct drm_rect and assorted utility functions

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

struct drm_rect represents a simple rectangle. The utility
functions are there to help driver writers.

v2: Moved the region stuff into its own file, made the smaller funcs
    static inline, used 64bit maths in the scaled clipping function to
    avoid overflows (instead it will saturate to INT_MIN or INT_MAX).
v3: Renamed drm_region to drm_rect, drm_region_clip to
    drm_rect_intersect, and drm_region_subsample to drm_rect_downscale.
v4: Renamed some function parameters, improve kernel-doc comments a bit,
    and actually generate documentation for drm_rect.[ch].
v5: s/RETUTRNS/RETURNS/

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 Documentation/DocBook/drm.tmpl |   2 +
 drivers/gpu/drm/Makefile       |   3 +-
 drivers/gpu/drm/drm_rect.c     |  96 ++++++++++++++++++++++++++++++
 include/drm/drm_rect.h         | 132 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 232 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/drm_rect.c
 create mode 100644 include/drm/drm_rect.h

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index f9df3b8..7c7af25 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -1653,6 +1653,8 @@ void intel_crt_init(struct drm_device *dev)
     <sect2>
       <title>KMS API Functions</title>
 !Edrivers/gpu/drm/drm_crtc.c
+!Edrivers/gpu/drm/drm_rect.c
+!Finclude/drm/drm_rect.h
     </sect2>
   </sect1>
 
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 6a42115..3374aac 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -12,7 +12,8 @@ drm-y       :=	drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
 		drm_platform.o drm_sysfs.o drm_hashtab.o drm_mm.o \
 		drm_crtc.o drm_modes.o drm_edid.o \
 		drm_info.o drm_debugfs.o drm_encoder_slave.o \
-		drm_trace_points.o drm_global.o drm_prime.o
+		drm_trace_points.o drm_global.o drm_prime.o \
+		drm_rect.o
 
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
new file mode 100644
index 0000000..22091ec
--- /dev/null
+++ b/drivers/gpu/drm/drm_rect.c
@@ -0,0 +1,96 @@
+/*
+ * Copyright (C) 2011-2013 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <drm/drm_rect.h>
+
+/**
+ * drm_rect_intersect - intersect two rectangles
+ * @r1: first rectangle
+ * @r2: second rectangle
+ *
+ * Calculate the intersection of rectangles @r1 and @r2.
+ * @r1 will be overwritten with the intersection.
+ *
+ * RETURNS:
+ * %true if rectangle @r1 is still visible after the operation,
+ * %false otherwise.
+ */
+bool drm_rect_intersect(struct drm_rect *r1, const struct drm_rect *r2)
+{
+	r1->x1 = max(r1->x1, r2->x1);
+	r1->y1 = max(r1->y1, r2->y1);
+	r1->x2 = min(r1->x2, r2->x2);
+	r1->y2 = min(r1->y2, r2->y2);
+
+	return drm_rect_visible(r1);
+}
+EXPORT_SYMBOL(drm_rect_intersect);
+
+/**
+ * drm_rect_clip_scaled - perform a scaled clip operation
+ * @src: source window rectangle
+ * @dst: destination window rectangle
+ * @clip: clip rectangle
+ * @hscale: horizontal scaling factor
+ * @vscale: vertical scaling factor
+ *
+ * Clip rectangle @dst by rectangle @clip. Clip rectangle @src by the
+ * same amounts multiplied by @hscale and @vscale.
+ *
+ * RETURNS:
+ * %true if rectangle @dst is still visible after being clipped,
+ * %false otherwise
+ */
+bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
+			  const struct drm_rect *clip,
+			  int hscale, int vscale)
+{
+	int diff;
+
+	diff = clip->x1 - dst->x1;
+	if (diff > 0) {
+		int64_t tmp = src->x1 + (int64_t) diff * hscale;
+		src->x1 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
+	}
+	diff = clip->y1 - dst->y1;
+	if (diff > 0) {
+		int64_t tmp = src->y1 + (int64_t) diff * vscale;
+		src->y1 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
+	}
+	diff = dst->x2 - clip->x2;
+	if (diff > 0) {
+		int64_t tmp = src->x2 - (int64_t) diff * hscale;
+		src->x2 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
+	}
+	diff = dst->y2 - clip->y2;
+	if (diff > 0) {
+		int64_t tmp = src->y2 - (int64_t) diff * vscale;
+		src->y2 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
+	}
+
+	return drm_rect_intersect(dst, clip);
+}
+EXPORT_SYMBOL(drm_rect_clip_scaled);
diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
new file mode 100644
index 0000000..2b7278c
--- /dev/null
+++ b/include/drm/drm_rect.h
@@ -0,0 +1,132 @@
+/*
+ * Copyright (C) 2011-2013 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef DRM_RECT_H
+#define DRM_RECT_H
+
+/**
+ * drm_rect - two dimensional rectangle
+ * @x1: horizontal starting coordinate (inclusive)
+ * @x2: horizontal ending coordinate (exclusive)
+ * @y1: vertical starting coordinate (inclusive)
+ * @y2: vertical ending coordinate (exclusive)
+ */
+struct drm_rect {
+	int x1, y1, x2, y2;
+};
+
+/**
+ * drm_rect_adjust_size - adjust the size of the rectangle
+ * @r: rectangle to be adjusted
+ * @dw: horizontal adjustment
+ * @dh: vertical adjustment
+ *
+ * Change the size of rectangle @r by @dw in the horizontal direction,
+ * and by @dh in the vertical direction, while keeping the center
+ * of @r stationary.
+ *
+ * Positive @dw and @dh increase the size, negative values decrease it.
+ */
+static inline void drm_rect_adjust_size(struct drm_rect *r, int dw, int dh)
+{
+	r->x1 -= dw >> 1;
+	r->y1 -= dh >> 1;
+	r->x2 += (dw + 1) >> 1;
+	r->y2 += (dh + 1) >> 1;
+}
+
+/**
+ * drm_rect_translate - translate the rectangle
+ * @r: rectangle to be tranlated
+ * @dx: horizontal translation
+ * @dy: vertical translation
+ *
+ * Move rectangle @r by @dx in the horizontal direction,
+ * and by @dy in the vertical direction.
+ */
+static inline void drm_rect_translate(struct drm_rect *r, int dx, int dy)
+{
+	r->x1 += dx;
+	r->y1 += dy;
+	r->x2 += dx;
+	r->y2 += dy;
+}
+
+/**
+ * drm_rect_downscale - downscale a rectangle
+ * @r: rectangle to be downscaled
+ * @horz: horizontal downscale factor
+ * @vert: vertical downscale factor
+ *
+ * Divide the coordinates of rectangle @r by @horz and @vert.
+ */
+static inline void drm_rect_downscale(struct drm_rect *r, int horz, int vert)
+{
+	r->x1 /= horz;
+	r->y1 /= vert;
+	r->x2 /= horz;
+	r->y2 /= vert;
+}
+
+/**
+ * drm_rect_width - determine the rectangle width
+ * @r: rectangle whose width is returned
+ *
+ * RETURNS:
+ * The width of the rectangle.
+ */
+static inline int drm_rect_width(const struct drm_rect *r)
+{
+	return r->x2 - r->x1;
+}
+
+/**
+ * drm_rect_height - determine the rectangle height
+ * @r: rectangle whose height is returned
+ *
+ * RETURNS:
+ * The height of the rectangle.
+ */
+static inline int drm_rect_height(const struct drm_rect *r)
+{
+	return r->y2 - r->y1;
+}
+
+/**
+ * drm_rect_visible - determine if the the rectangle is visible
+ * @r: rectangle whose visibility is returned
+ *
+ * RETURNS:
+ * %true if the rectangle is visible, %false otherwise.
+ */
+static inline bool drm_rect_visible(const struct drm_rect *r)
+{
+	return drm_rect_width(r) > 0 && drm_rect_height(r) > 0;
+}
+
+bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip);
+bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
+			  const struct drm_rect *clip,
+			  int hscale, int vscale);
+
+#endif
-- 
1.8.1.5

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

[PATCH v3 2/4] drm: Add drm_rect_calc_{hscale, vscale}() utility functions

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

These functions calculcate the scaling factor based on the source and
destination rectangles.

There are two version of the functions, the strict ones that will
return an error if the min/max scaling factor is exceeded, and the
relaxed versions that will adjust the src/dst rectangles in order to
keep the scaling factor withing the limits.

v2: Return error instead of adjusting regions, refactor common parts
    into one function, and split into strict and relaxed versions.
v3: Renamed drm_region to drm_rect, add "_rect_" to the function
    names.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_rect.c | 177 +++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_rect.h     |  12 +++
 2 files changed, 189 insertions(+)

diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
index 22091ec..5dd9411 100644
--- a/drivers/gpu/drm/drm_rect.c
+++ b/drivers/gpu/drm/drm_rect.c
@@ -94,3 +94,180 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
 	return drm_rect_intersect(dst, clip);
 }
 EXPORT_SYMBOL(drm_rect_clip_scaled);
+
+static int drm_calc_scale(int src, int dst)
+{
+	int scale = 0;
+
+	if (src < 0 || dst < 0)
+		return -EINVAL;
+
+	if (dst == 0)
+		return 0;
+
+	scale = src / dst;
+
+	return scale;
+}
+
+/**
+ * drm_rect_calc_hscale - calculate the horizontal scaling factor
+ * @src: source window rectangle
+ * @dst: destination window rectangle
+ * @min_hscale: minimum allowed horizontal scaling factor
+ * @max_hscale: maximum allowed horizontal scaling factor
+ *
+ * Calculate the horizontal scaling factor as
+ * (@src width) / (@dst width).
+ *
+ * RETURNS:
+ * The horizontal scaling factor, or errno of out of limits.
+ */
+int drm_rect_calc_hscale(const struct drm_rect *src,
+			 const struct drm_rect *dst,
+			 int min_hscale, int max_hscale)
+{
+	int src_w = drm_rect_width(src);
+	int dst_w = drm_rect_width(dst);
+	int hscale = drm_calc_scale(src_w, dst_w);
+
+	if (hscale < 0 || dst_w == 0)
+		return hscale;
+
+	if (hscale < min_hscale || hscale > max_hscale)
+		return -ERANGE;
+
+	return hscale;
+}
+EXPORT_SYMBOL(drm_rect_calc_hscale);
+
+/**
+ * drm_rect_calc_vscale - calculate the vertical scaling factor
+ * @src: source window rectangle
+ * @dst: destination window rectangle
+ * @min_vscale: minimum allowed vertical scaling factor
+ * @max_vscale: maximum allowed vertical scaling factor
+ *
+ * Calculate the vertical scaling factor as
+ * (@src height) / (@dst height).
+ *
+ * RETURNS:
+ * The vertical scaling factor, or errno of out of limits.
+ */
+int drm_rect_calc_vscale(const struct drm_rect *src,
+			 const struct drm_rect *dst,
+			 int min_vscale, int max_vscale)
+{
+	int src_h = drm_rect_height(src);
+	int dst_h = drm_rect_height(dst);
+	int vscale = drm_calc_scale(src_h, dst_h);
+
+	if (vscale < 0 || dst_h == 0)
+		return vscale;
+
+	if (vscale < min_vscale || vscale > max_vscale)
+		return -ERANGE;
+
+	return vscale;
+}
+EXPORT_SYMBOL(drm_rect_calc_vscale);
+
+/**
+ * drm_calc_hscale_relaxed - calculate the horizontal scaling factor
+ * @src: source window rectangle
+ * @dst: destination window rectangle
+ * @min_hscale: minimum allowed horizontal scaling factor
+ * @max_hscale: maximum allowed horizontal scaling factor
+ *
+ * Calculate the horizontal scaling factor as
+ * (@src width) / (@dst width).
+ *
+ * If the calculated scaling factor is below @min_vscale,
+ * decrease the height of rectangle @dst to compensate.
+ *
+ * If the calculcated scaling factor is above @max_vscale,
+ * decrease the height of rectangle @src to compensate.
+ *
+ * RETURNS:
+ * The horizontal scaling factor.
+ */
+int drm_rect_calc_hscale_relaxed(struct drm_rect *src,
+				 struct drm_rect *dst,
+				 int min_hscale, int max_hscale)
+{
+	int src_w = drm_rect_width(src);
+	int dst_w = drm_rect_width(dst);
+	int hscale = drm_calc_scale(src_w, dst_w);
+
+	if (hscale < 0 || dst_w == 0)
+		return hscale;
+
+	if (hscale < min_hscale) {
+		int max_dst_w = src_w / min_hscale;
+
+		drm_rect_adjust_size(dst, max_dst_w - dst_w, 0);
+
+		return min_hscale;
+	}
+
+	if (hscale > max_hscale) {
+		int max_src_w = dst_w * max_hscale;
+
+		drm_rect_adjust_size(src, max_src_w - src_w, 0);
+
+		return max_hscale;
+	}
+
+	return hscale;
+}
+EXPORT_SYMBOL(drm_rect_calc_hscale_relaxed);
+
+/**
+ * drm_rect_calc_vscale_relaxed - calculate the vertical scaling factor
+ * @src: source window rectangle
+ * @dst: destination window rectangle
+ * @min_vscale: minimum allowed vertical scaling factor
+ * @max_vscale: maximum allowed vertical scaling factor
+ *
+ * Calculate the vertical scaling factor as
+ * (@src height) / (@dst height).
+ *
+ * If the calculated scaling factor is below @min_vscale,
+ * decrease the height of rectangle @dst to compensate.
+ *
+ * If the calculcated scaling factor is above @max_vscale,
+ * decrease the height of rectangle @src to compensate.
+ *
+ * RETURNS:
+ * The vertical scaling factor.
+ */
+int drm_rect_calc_vscale_relaxed(struct drm_rect *src,
+				 struct drm_rect *dst,
+				 int min_vscale, int max_vscale)
+{
+	int src_h = drm_rect_height(src);
+	int dst_h = drm_rect_height(dst);
+	int vscale = drm_calc_scale(src_h, dst_h);
+
+	if (vscale < 0 || dst_h == 0)
+		return vscale;
+
+	if (vscale < min_vscale) {
+		int max_dst_h = src_h / min_vscale;
+
+		drm_rect_adjust_size(dst, 0, max_dst_h - dst_h);
+
+		return min_vscale;
+	}
+
+	if (vscale > max_vscale) {
+		int max_src_h = dst_h * max_vscale;
+
+		drm_rect_adjust_size(src, 0, max_src_h - src_h);
+
+		return max_vscale;
+	}
+
+	return vscale;
+}
+EXPORT_SYMBOL(drm_rect_calc_vscale_relaxed);
diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
index 2b7278c..de24f16 100644
--- a/include/drm/drm_rect.h
+++ b/include/drm/drm_rect.h
@@ -128,5 +128,17 @@ bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip);
 bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
 			  const struct drm_rect *clip,
 			  int hscale, int vscale);
+int drm_rect_calc_hscale(const struct drm_rect *src,
+			 const struct drm_rect *dst,
+			 int min_hscale, int max_hscale);
+int drm_rect_calc_vscale(const struct drm_rect *src,
+			 const struct drm_rect *dst,
+			 int min_vscale, int max_vscale);
+int drm_rect_calc_hscale_relaxed(struct drm_rect *src,
+				 struct drm_rect *dst,
+				 int min_hscale, int max_hscale);
+int drm_rect_calc_vscale_relaxed(struct drm_rect *src,
+				 struct drm_rect *dst,
+				 int min_vscale, int max_vscale);
 
 #endif
-- 
1.8.1.5

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

Re: [PATCH v3 2/4] drm: Add drm_rect_calc_{hscale, vscale}() utility functions

[Chris Wilson]

On Tue, Apr 16, 2013 at 01:47:20PM +0300, ville.syrjala@linux.intel.com wrote:
> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> index 2b7278c..de24f16 100644
> --- a/include/drm/drm_rect.h
> +++ b/include/drm/drm_rect.h
> @@ -128,5 +128,17 @@ bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip);
>  bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
>  			  const struct drm_rect *clip,
>  			  int hscale, int vscale);
> +int drm_rect_calc_hscale(const struct drm_rect *src,
> +			 const struct drm_rect *dst,
> +			 int min_hscale, int max_hscale);
> +int drm_rect_calc_vscale(const struct drm_rect *src,
> +			 const struct drm_rect *dst,
> +			 int min_vscale, int max_vscale);
> +int drm_rect_calc_hscale_relaxed(struct drm_rect *src,
> +				 struct drm_rect *dst,
> +				 int min_hscale, int max_hscale);
> +int drm_rect_calc_vscale_relaxed(struct drm_rect *src,
> +				 struct drm_rect *dst,
> +				 int min_vscale, int max_vscale);

These struct drm_rect *src should be const so it is clear that dst is
being manipulated.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3 2/4] drm: Add drm_rect_calc_{hscale, vscale}() utility functions

[Ville Syrjälä]

On Tue, Apr 16, 2013 at 02:42:34PM +0100, Chris Wilson wrote:
> On Tue, Apr 16, 2013 at 01:47:20PM +0300, ville.syrjala@linux.intel.com wrote:
> > diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> > index 2b7278c..de24f16 100644
> > --- a/include/drm/drm_rect.h
> > +++ b/include/drm/drm_rect.h
> > @@ -128,5 +128,17 @@ bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip);
> >  bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
> >  			  const struct drm_rect *clip,
> >  			  int hscale, int vscale);
> > +int drm_rect_calc_hscale(const struct drm_rect *src,
> > +			 const struct drm_rect *dst,
> > +			 int min_hscale, int max_hscale);
> > +int drm_rect_calc_vscale(const struct drm_rect *src,
> > +			 const struct drm_rect *dst,
> > +			 int min_vscale, int max_vscale);
> > +int drm_rect_calc_hscale_relaxed(struct drm_rect *src,
> > +				 struct drm_rect *dst,
> > +				 int min_hscale, int max_hscale);
> > +int drm_rect_calc_vscale_relaxed(struct drm_rect *src,
> > +				 struct drm_rect *dst,
> > +				 int min_vscale, int max_vscale);
> 
> These struct drm_rect *src should be const so it is clear that dst is
> being manipulated.

Actually they can manipulate either src or dst, depending on whether
we're trying upscale or downscale too much. The idea being that we
can only decrease the size of either rect, never increase.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 2/4] drm: Add drm_rect_calc_{hscale, vscale}() utility functions

[Chris Wilson]

On Tue, Apr 16, 2013 at 05:14:14PM +0300, Ville Syrjälä wrote:
> On Tue, Apr 16, 2013 at 02:42:34PM +0100, Chris Wilson wrote:
> > On Tue, Apr 16, 2013 at 01:47:20PM +0300, ville.syrjala@linux.intel.com wrote:
> > > diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> > > index 2b7278c..de24f16 100644
> > > --- a/include/drm/drm_rect.h
> > > +++ b/include/drm/drm_rect.h
> > > @@ -128,5 +128,17 @@ bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip);
> > >  bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
> > >  			  const struct drm_rect *clip,
> > >  			  int hscale, int vscale);
> > > +int drm_rect_calc_hscale(const struct drm_rect *src,
> > > +			 const struct drm_rect *dst,
> > > +			 int min_hscale, int max_hscale);
> > > +int drm_rect_calc_vscale(const struct drm_rect *src,
> > > +			 const struct drm_rect *dst,
> > > +			 int min_vscale, int max_vscale);
> > > +int drm_rect_calc_hscale_relaxed(struct drm_rect *src,
> > > +				 struct drm_rect *dst,
> > > +				 int min_hscale, int max_hscale);
> > > +int drm_rect_calc_vscale_relaxed(struct drm_rect *src,
> > > +				 struct drm_rect *dst,
> > > +				 int min_vscale, int max_vscale);
> > 
> > These struct drm_rect *src should be const so it is clear that dst is
> > being manipulated.
> 
> Actually they can manipulate either src or dst, depending on whether
> we're trying upscale or downscale too much. The idea being that we
> can only decrease the size of either rect, never increase.

Hmm, ofc you are right.  I guess I'm just not that comfortable with the
concept of relaxed scaling. Dare I ask you to split patch 4 so that you
can convince me with a solid changelog?

s/calculcated/calculated.
-Chris
-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3 2/4] drm: Add drm_rect_calc_{hscale, vscale}() utility functions

[Ville Syrjälä]

On Tue, Apr 16, 2013 at 03:49:58PM +0100, Chris Wilson wrote:
> On Tue, Apr 16, 2013 at 05:14:14PM +0300, Ville Syrjälä wrote:
> > On Tue, Apr 16, 2013 at 02:42:34PM +0100, Chris Wilson wrote:
> > > On Tue, Apr 16, 2013 at 01:47:20PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> > > > index 2b7278c..de24f16 100644
> > > > --- a/include/drm/drm_rect.h
> > > > +++ b/include/drm/drm_rect.h
> > > > @@ -128,5 +128,17 @@ bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip);
> > > >  bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
> > > >  			  const struct drm_rect *clip,
> > > >  			  int hscale, int vscale);
> > > > +int drm_rect_calc_hscale(const struct drm_rect *src,
> > > > +			 const struct drm_rect *dst,
> > > > +			 int min_hscale, int max_hscale);
> > > > +int drm_rect_calc_vscale(const struct drm_rect *src,
> > > > +			 const struct drm_rect *dst,
> > > > +			 int min_vscale, int max_vscale);
> > > > +int drm_rect_calc_hscale_relaxed(struct drm_rect *src,
> > > > +				 struct drm_rect *dst,
> > > > +				 int min_hscale, int max_hscale);
> > > > +int drm_rect_calc_vscale_relaxed(struct drm_rect *src,
> > > > +				 struct drm_rect *dst,
> > > > +				 int min_vscale, int max_vscale);
> > > 
> > > These struct drm_rect *src should be const so it is clear that dst is
> > > being manipulated.
> > 
> > Actually they can manipulate either src or dst, depending on whether
> > we're trying upscale or downscale too much. The idea being that we
> > can only decrease the size of either rect, never increase.
> 
> Hmm, ofc you are right.  I guess I'm just not that comfortable with the
> concept of relaxed scaling.

Yeah it's a bit of an open question how we should do things. The
hardware constraints can be very complicated to describe, so I
don't see any simple/sane way to report them to userspace. Which
means writing simple hardware agnostic userspace code is pretty
much impossible unless the kernel is very relaxed about what it
accepts.

OTOH if you want to be sure that the final picture will look
exactly as specified, then you'd probably want an error
instead.

But I haven't really made up my mind on how we should handle those two
cases. Some kind of control knob might be nice, but I've not yet figured
out where the knob should live, and what kind of granularity it should
have.

And then we also have the problem that our errno based error reporting
isn't really adequate for figuring out why something failed. Usually
you just have to enable drm debugs, try again, and read through the log.

> Dare I ask you to split patch 4 so that you
> can convince me with a solid changelog?

So you'd like me to implement strict checks first, and then relax them
in a follow up patch?

> 
> s/calculcated/calculated.

Fixed. Thanks.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 2/4] drm: Add drm_rect_calc_{hscale, vscale}() utility functions

[Chris Wilson]

On Tue, Apr 16, 2013 at 06:16:10PM +0300, Ville Syrjälä wrote:
> On Tue, Apr 16, 2013 at 03:49:58PM +0100, Chris Wilson wrote:
[snip]
> > Dare I ask you to split patch 4 so that you
> > can convince me with a solid changelog?
> 
> So you'd like me to implement strict checks first, and then relax them
> in a follow up patch?

Yes, please. The strict checking should give us an interface with the
least surprises. Then we can discuss how to relax those checks in
combination with atomic modesetting & flipping to make sure the
interface remains consistent and unsurprising.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 1/3] drm: Add drm_rect_equals()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

drm_rect_equals() tells whether two drm_rects are equal.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 include/drm/drm_rect.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
index fe767b7..64fa265 100644
--- a/include/drm/drm_rect.h
+++ b/include/drm/drm_rect.h
@@ -124,6 +124,21 @@ static inline bool drm_rect_visible(const struct drm_rect *r)
 	return drm_rect_width(r) > 0 && drm_rect_height(r) > 0;
 }
 
+/**
+ * drm_rect_equals - determine if two rectangles are equal
+ * @r1: first rectangle
+ * @r2: second rectangle
+ *
+ * RETURNS:
+ * %true if the rectangles are equal, %false otherwise.
+ */
+static inline bool drm_rect_equals(const struct drm_rect *r1,
+				   const struct drm_rect *r2)
+{
+	return r1->x1 == r2->x1 && r1->x2 == r2->x2 &&
+		r1->y1 == r2->y1 && r1->y2 == r2->y2;
+}
+
 bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip);
 bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
 			  const struct drm_rect *clip,
-- 
1.8.1.5

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

[PATCH v4 2/3] drm/i915: Implement proper clipping for video sprites

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Properly clip the source when the destination gets clipped
by the pipe dimensions.

Sadly the video sprite hardware is rather limited so it can't do proper
sub-pixel postitioning. Resort to truncating the source coordinates to
(macro)pixel boundary.

The scaling checks are done using the strict drm_region functions.
Which means that an error is returned when the min/max scaling
ratios are exceeded.

Also do some additional checking against various hardware limits.

v2: Truncate src coords instead of rounding to avoid increasing src
    viewport size, and adapt to changes in drm_calc_{h,v}scale().
v3: Adapt to drm_region->drm_rect rename. Fix misaligned crtc_w for
    packed YUV formats when scaling isn't supported.
v4: Use stricter scaling checks, use drm_rect_equals()

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 202 ++++++++++++++++++++++++++----------
 1 file changed, 150 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index c7d25c5..93a6657 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -32,6 +32,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_rect.h>
 #include "intel_drv.h"
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
@@ -583,6 +584,20 @@ ilk_get_colorkey(struct drm_plane *plane, struct drm_intel_sprite_colorkey *key)
 		key->flags = I915_SET_COLORKEY_NONE;
 }
 
+static bool
+format_is_yuv(uint32_t format)
+{
+	switch (format) {
+	case DRM_FORMAT_YUYV:
+	case DRM_FORMAT_UYVY:
+	case DRM_FORMAT_VYUY:
+	case DRM_FORMAT_YVYU:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static int
 intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
@@ -600,9 +615,27 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
 								      pipe);
 	int ret = 0;
-	int x = src_x >> 16, y = src_y >> 16;
-	int primary_w = crtc->mode.hdisplay, primary_h = crtc->mode.vdisplay;
 	bool disable_primary = false;
+	bool visible;
+	int hscale, vscale;
+	int max_scale, min_scale;
+	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	struct drm_rect src = {
+		.x1 = src_x,
+		.x2 = src_x + src_w,
+		.y1 = src_y,
+		.y2 = src_y + src_h,
+	};
+	struct drm_rect dst = {
+		.x1 = crtc_x,
+		.x2 = crtc_x + crtc_w,
+		.y1 = crtc_y,
+		.y2 = crtc_y + crtc_h,
+	};
+	const struct drm_rect clip = {
+		.x2 = crtc->mode.hdisplay,
+		.y2 = crtc->mode.vdisplay,
+	};
 
 	intel_fb = to_intel_framebuffer(fb);
 	obj = intel_fb->obj;
@@ -618,19 +651,23 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	intel_plane->src_w = src_w;
 	intel_plane->src_h = src_h;
 
-	src_w = src_w >> 16;
-	src_h = src_h >> 16;
-
 	/* Pipe must be running... */
-	if (!(I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE))
-		return -EINVAL;
-
-	if (crtc_x >= primary_w || crtc_y >= primary_h)
+	if (!(I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE)) {
+		DRM_DEBUG_KMS("Pipe disabled\n");
 		return -EINVAL;
+	}
 
 	/* Don't modify another pipe's plane */
-	if (intel_plane->pipe != intel_crtc->pipe)
+	if (intel_plane->pipe != intel_crtc->pipe) {
+		DRM_DEBUG_KMS("Wrong plane <-> crtc mapping\n");
 		return -EINVAL;
+	}
+
+	/* FIXME check all gen limits */
+	if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > 16384) {
+		DRM_DEBUG_KMS("Unsuitable framebuffer for plane\n");
+		return -EINVAL;
+	}
 
 	/* Sprite planes can be linear or x-tiled surfaces */
 	switch (obj->tiling_mode) {
@@ -638,55 +675,113 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		case I915_TILING_X:
 			break;
 		default:
+			DRM_DEBUG_KMS("Unsupported tiling mode\n");
 			return -EINVAL;
 	}
 
-	/*
-	 * Clamp the width & height into the visible area.  Note we don't
-	 * try to scale the source if part of the visible region is offscreen.
-	 * The caller must handle that by adjusting source offset and size.
-	 */
-	if ((crtc_x < 0) && ((crtc_x + crtc_w) > 0)) {
-		crtc_w += crtc_x;
-		crtc_x = 0;
+	max_scale = intel_plane->max_downscale << 16;
+	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
+
+	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, true);
+		drm_rect_debug_print(&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, true);
+		drm_rect_debug_print(&dst, false);
+
+		return vscale;
+	}
+
+	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 (visible) {
+		/* 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));
+
+		/*
+		 * 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.
+		 *
+		 * FIXME Should we be really strict and reject the
+		 * config if it results in non (macro)pixel aligned
+		 * coords?
+		 */
+		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 (format_is_yuv(fb->pixel_format)) {
+			src_x &= ~1;
+			src_w &= ~1;
+
+			/*
+			 * Must keep src and dst the
+			 * same if we can't scale.
+			 */
+			if (!intel_plane->can_scale)
+				crtc_w &= ~1;
+
+			if (crtc_w == 0)
+				visible = false;
+		}
 	}
-	if ((crtc_x + crtc_w) <= 0) /* Nothing to display */
-		goto out;
-	if ((crtc_x + crtc_w) > primary_w)
-		crtc_w = primary_w - crtc_x;
-
-	if ((crtc_y < 0) && ((crtc_y + crtc_h) > 0)) {
-		crtc_h += crtc_y;
-		crtc_y = 0;
+
+	/* Check size restrictions when scaling */
+	if (visible && (src_w != crtc_w || src_h != crtc_h)) {
+		unsigned int width_bytes;
+
+		BUG_ON(!intel_plane->can_scale);
+
+		/* FIXME interlacing min height is 6 */
+
+		if (crtc_w < 3 || crtc_h < 3) {
+			DRM_DEBUG_KMS("Destination dimensions too small\n");
+			return -EINVAL;
+		}
+
+		if (src_w < 3 || src_h < 3) {
+			DRM_DEBUG_KMS("Source dimensions too small\n");
+			return -EINVAL;
+		}
+
+		width_bytes = ((src_x * pixel_size) & 63) + src_w * pixel_size;
+
+		if (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 ((crtc_y + crtc_h) <= 0) /* Nothing to display */
-		goto out;
-	if (crtc_y + crtc_h > primary_h)
-		crtc_h = primary_h - crtc_y;
-
-	if (!crtc_w || !crtc_h) /* Again, nothing to display */
-		goto out;
-
-	/*
-	 * We may not have a scaler, eg. HSW does not have it any more
-	 */
-	if (!intel_plane->can_scale && (crtc_w != src_w || crtc_h != src_h))
-		return -EINVAL;
-
-	/*
-	 * We can take a larger source and scale it down, but
-	 * only so much...  16x is the max on SNB.
-	 */
-	if (((src_w * src_h) / (crtc_w * crtc_h)) > intel_plane->max_downscale)
-		return -EINVAL;
+
+	dst.x1 = crtc_x;
+	dst.x2 = crtc_x + crtc_w;
+	dst.y1 = crtc_y;
+	dst.y2 = crtc_y + crtc_h;
 
 	/*
 	 * If the sprite is completely covering the primary plane,
 	 * we can disable the primary and save power.
 	 */
-	if ((crtc_x == 0) && (crtc_y == 0) &&
-	    (crtc_w == primary_w) && (crtc_h == primary_h))
-		disable_primary = true;
+	disable_primary = drm_rect_equals(&dst, &clip);
+	BUG_ON(disable_primary && !visible);
 
 	mutex_lock(&dev->struct_mutex);
 
@@ -708,8 +803,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (!disable_primary)
 		intel_enable_primary(crtc);
 
-	intel_plane->update_plane(plane, fb, obj, crtc_x, crtc_y,
-				  crtc_w, crtc_h, x, y, src_w, src_h);
+	if (visible)
+		intel_plane->update_plane(plane, fb, obj,
+					  crtc_x, crtc_y, crtc_w, crtc_h,
+					  src_x, src_y, src_w, src_h);
+	else
+		intel_plane->disable_plane(plane);
 
 	if (disable_primary)
 		intel_disable_primary(crtc);
@@ -732,7 +831,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 out_unlock:
 	mutex_unlock(&dev->struct_mutex);
-out:
 	return ret;
 }
 
-- 
1.8.1.5

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

[PATCH 3/3] drm/i915: Relax the sprite scaling limits checks

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reduce the size of the the src/dst viewport to keep the scalign ratios
in check.

Also treat sprites below the minimum size as invisble.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 60 ++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 93a6657..c3a5688 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -679,26 +679,19 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 			return -EINVAL;
 	}
 
+	/*
+	 * 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.
+	 */
 	max_scale = intel_plane->max_downscale << 16;
 	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
 
-	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, true);
-		drm_rect_debug_print(&dst, false);
+	hscale = drm_rect_calc_hscale_relaxed(&src, &dst, min_scale, max_scale);
+	BUG_ON(hscale < 0);
 
-		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, true);
-		drm_rect_debug_print(&dst, false);
-
-		return vscale;
-	}
+	vscale = drm_rect_calc_vscale_relaxed(&src, &dst, min_scale, max_scale);
+	BUG_ON(vscale < 0);
 
 	visible = drm_rect_clip_scaled(&src, &dst, &clip, hscale, vscale);
 
@@ -708,6 +701,25 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	crtc_h = drm_rect_height(&dst);
 
 	if (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, true);
+			drm_rect_debug_print(&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, true);
+			drm_rect_debug_print(&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),
@@ -718,10 +730,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		 * 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.
-		 *
-		 * FIXME Should we be really strict and reject the
-		 * config if it results in non (macro)pixel aligned
-		 * coords?
 		 */
 		src_x = src.x1 >> 16;
 		src_w = drm_rect_width(&src) >> 16;
@@ -752,15 +760,11 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 		/* FIXME interlacing min height is 6 */
 
-		if (crtc_w < 3 || crtc_h < 3) {
-			DRM_DEBUG_KMS("Destination dimensions too small\n");
-			return -EINVAL;
-		}
+		if (crtc_w < 3 || crtc_h < 3)
+			visible = false;
 
-		if (src_w < 3 || src_h < 3) {
-			DRM_DEBUG_KMS("Source dimensions too small\n");
-			return -EINVAL;
-		}
+		if (src_w < 3 || src_h < 3)
+			visible = false;
 
 		width_bytes = ((src_x * pixel_size) & 63) + src_w * pixel_size;
 
-- 
1.8.1.5

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

[PATCH v3 3/4] drm: Add drm_rect_debug_print()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a debug function to print the rectangle in a human readable format.

v2: Renamed drm_region to drm_rect, the function from drm_region_debug
    to drm_rect_debug_print(), and use %+d instead of +%d in the format.
v3: Use %d format for width/height in the non fixed point case as well

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_rect.c | 22 ++++++++++++++++++++++
 include/drm/drm_rect.h     |  1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
index 5dd9411..33d5930 100644
--- a/drivers/gpu/drm/drm_rect.c
+++ b/drivers/gpu/drm/drm_rect.c
@@ -24,6 +24,7 @@
 #include <linux/errno.h>
 #include <linux/export.h>
 #include <linux/kernel.h>
+#include <drm/drmP.h>
 #include <drm/drm_rect.h>
 
 /**
@@ -271,3 +272,24 @@ int drm_rect_calc_vscale_relaxed(struct drm_rect *src,
 	return vscale;
 }
 EXPORT_SYMBOL(drm_rect_calc_vscale_relaxed);
+
+/**
+ * drm_rect_debug_print - print the rectangle information
+ * @r: rectangle to print
+ * @fixed_point: rectangle is in 16.16 fixed point format
+ */
+void drm_rect_debug_print(const struct drm_rect *r, bool fixed_point)
+{
+	int w = drm_rect_width(r);
+	int h = drm_rect_height(r);
+
+	if (fixed_point)
+		DRM_DEBUG_KMS("%d.%06ux%d.%06u%+d.%06u%+d.%06u\n",
+			      w >> 16, ((w & 0xffff) * 15625) >> 10,
+			      h >> 16, ((h & 0xffff) * 15625) >> 10,
+			      r->x1 >> 16, ((r->x1 & 0xffff) * 15625) >> 10,
+			      r->y1 >> 16, ((r->y1 & 0xffff) * 15625) >> 10);
+	else
+		DRM_DEBUG_KMS("%dx%d%+d%+d\n", w, h, r->x1, r->y1);
+}
+EXPORT_SYMBOL(drm_rect_debug_print);
diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
index de24f16..fe767b7 100644
--- a/include/drm/drm_rect.h
+++ b/include/drm/drm_rect.h
@@ -140,5 +140,6 @@ int drm_rect_calc_hscale_relaxed(struct drm_rect *src,
 int drm_rect_calc_vscale_relaxed(struct drm_rect *src,
 				 struct drm_rect *dst,
 				 int min_vscale, int max_vscale);
+void drm_rect_debug_print(const struct drm_rect *r, bool fixed_point);
 
 #endif
-- 
1.8.1.5

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

[PATCH v3 4/4] drm/i915: Implement proper clipping for video sprites

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Properly clip the source when the destination gets clipped
by the pipe dimensions.

Sadly the video sprite hardware is rather limited so it can't do proper
sub-pixel postitioning. Resort to truncating the source coordinates to
(macro)pixel boundary.

The scaling checks are done using the relaxed drm_region functions.
That means that the src/dst regions are reduced in size in order to keep
the scaling factor within the limits.

Also do some additional checking against various hardware limits.

v2: Truncate src coords instead of rounding to avoid increasing src
    viewport size, and adapt to changes in drm_calc_{h,v}scale().
v3: Adapt to drm_region->drm_rect rename. Fix misaligned crtc_w for
    packed YUV formats when scaling isn't supported.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 191 +++++++++++++++++++++++++++---------
 1 file changed, 145 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index c7d25c5..72b2ce4 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -32,6 +32,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_rect.h>
 #include "intel_drv.h"
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
@@ -583,6 +584,20 @@ ilk_get_colorkey(struct drm_plane *plane, struct drm_intel_sprite_colorkey *key)
 		key->flags = I915_SET_COLORKEY_NONE;
 }
 
+static bool
+format_is_yuv(uint32_t format)
+{
+	switch (format) {
+	case DRM_FORMAT_YUYV:
+	case DRM_FORMAT_UYVY:
+	case DRM_FORMAT_VYUY:
+	case DRM_FORMAT_YVYU:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static int
 intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
@@ -600,9 +615,28 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
 								      pipe);
 	int ret = 0;
-	int x = src_x >> 16, y = src_y >> 16;
 	int primary_w = crtc->mode.hdisplay, primary_h = crtc->mode.vdisplay;
 	bool disable_primary = false;
+	bool visible;
+	int hscale, vscale;
+	int max_scale, min_scale;
+	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	struct drm_rect src = {
+		.x1 = src_x,
+		.x2 = src_x + src_w,
+		.y1 = src_y,
+		.y2 = src_y + src_h,
+	};
+	struct drm_rect dst = {
+		.x1 = crtc_x,
+		.x2 = crtc_x + crtc_w,
+		.y1 = crtc_y,
+		.y2 = crtc_y + crtc_h,
+	};
+	const struct drm_rect clip = {
+		.x2 = crtc->mode.hdisplay,
+		.y2 = crtc->mode.vdisplay,
+	};
 
 	intel_fb = to_intel_framebuffer(fb);
 	obj = intel_fb->obj;
@@ -618,19 +652,23 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	intel_plane->src_w = src_w;
 	intel_plane->src_h = src_h;
 
-	src_w = src_w >> 16;
-	src_h = src_h >> 16;
-
 	/* Pipe must be running... */
-	if (!(I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE))
-		return -EINVAL;
-
-	if (crtc_x >= primary_w || crtc_y >= primary_h)
+	if (!(I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE)) {
+		DRM_DEBUG_KMS("Pipe disabled\n");
 		return -EINVAL;
+	}
 
 	/* Don't modify another pipe's plane */
-	if (intel_plane->pipe != intel_crtc->pipe)
+	if (intel_plane->pipe != intel_crtc->pipe) {
+		DRM_DEBUG_KMS("Wrong plane <-> crtc mapping\n");
 		return -EINVAL;
+	}
+
+	/* FIXME check all gen limits */
+	if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > 16384) {
+		DRM_DEBUG_KMS("Unsuitable framebuffer for plane\n");
+		return -EINVAL;
+	}
 
 	/* Sprite planes can be linear or x-tiled surfaces */
 	switch (obj->tiling_mode) {
@@ -638,53 +676,111 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		case I915_TILING_X:
 			break;
 		default:
+			DRM_DEBUG_KMS("Unsupported tiling mode\n");
 			return -EINVAL;
 	}
 
 	/*
-	 * Clamp the width & height into the visible area.  Note we don't
-	 * try to scale the source if part of the visible region is offscreen.
-	 * The caller must handle that by adjusting source offset and size.
+	 * 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.
 	 */
-	if ((crtc_x < 0) && ((crtc_x + crtc_w) > 0)) {
-		crtc_w += crtc_x;
-		crtc_x = 0;
+	max_scale = intel_plane->max_downscale << 16;
+	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
+
+	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);
+
+	visible = drm_rect_clip_scaled(&src, &dst, &clip, hscale, vscale);
+
+	if (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, true);
+			drm_rect_debug_print(&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, true);
+			drm_rect_debug_print(&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));
+
+		crtc_x = dst.x1;
+		crtc_y = dst.y1;
+		crtc_w = drm_rect_width(&dst);
+		crtc_h = drm_rect_height(&dst);
+
+		/*
+		 * 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 (format_is_yuv(fb->pixel_format)) {
+			src_x &= ~1;
+			src_w &= ~1;
+
+			/*
+			 * Must keep src and dst the
+			 * same if we can't scale.
+			 */
+			if (!intel_plane->can_scale)
+				crtc_w &= ~1;
+		}
 	}
-	if ((crtc_x + crtc_w) <= 0) /* Nothing to display */
-		goto out;
-	if ((crtc_x + crtc_w) > primary_w)
-		crtc_w = primary_w - crtc_x;
-
-	if ((crtc_y < 0) && ((crtc_y + crtc_h) > 0)) {
-		crtc_h += crtc_y;
-		crtc_y = 0;
+
+	/* Account for minimum size when scaling */
+	if (visible && (src_w != crtc_w || src_h != crtc_h)) {
+		BUG_ON(!intel_plane->can_scale);
+
+		/* FIXME interlacing min height is 6 */
+		/* FIXME return an error instead? */
+
+		if (crtc_w < 3 || crtc_h < 3)
+			visible = false;
+
+		if (src_w < 3 || src_h < 3)
+			visible = false;
 	}
-	if ((crtc_y + crtc_h) <= 0) /* Nothing to display */
-		goto out;
-	if (crtc_y + crtc_h > primary_h)
-		crtc_h = primary_h - crtc_y;
 
-	if (!crtc_w || !crtc_h) /* Again, nothing to display */
-		goto out;
+	/* Check size restrictions when scaling */
+	if (visible && (src_w != crtc_w || src_h != crtc_h)) {
+		unsigned int width_bytes = ((src_x * pixel_size) & 63) + src_w * pixel_size;
 
-	/*
-	 * We may not have a scaler, eg. HSW does not have it any more
-	 */
-	if (!intel_plane->can_scale && (crtc_w != src_w || crtc_h != src_h))
-		return -EINVAL;
+		BUG_ON(!intel_plane->can_scale);
 
-	/*
-	 * We can take a larger source and scale it down, but
-	 * only so much...  16x is the max on SNB.
-	 */
-	if (((src_w * src_h) / (crtc_w * crtc_h)) > intel_plane->max_downscale)
-		return -EINVAL;
+		if (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 the sprite is completely covering the primary plane,
 	 * we can disable the primary and save power.
 	 */
-	if ((crtc_x == 0) && (crtc_y == 0) &&
+	if (visible &&
+	    (crtc_x == 0) && (crtc_y == 0) &&
 	    (crtc_w == primary_w) && (crtc_h == primary_h))
 		disable_primary = true;
 
@@ -708,11 +804,15 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (!disable_primary)
 		intel_enable_primary(crtc);
 
-	intel_plane->update_plane(plane, fb, obj, crtc_x, crtc_y,
-				  crtc_w, crtc_h, x, y, src_w, src_h);
+	if (visible) {
+		intel_plane->update_plane(plane, fb, obj,
+					  crtc_x, crtc_y, crtc_w, crtc_h,
+					  src_x, src_y, src_w, src_h);
 
-	if (disable_primary)
-		intel_disable_primary(crtc);
+		if (disable_primary)
+			intel_disable_primary(crtc);
+	} else
+		intel_plane->disable_plane(plane);
 
 	/* Unpin old obj after new one is active to avoid ugliness */
 	if (old_obj) {
@@ -732,7 +832,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 out_unlock:
 	mutex_unlock(&dev->struct_mutex);
-out:
 	return ret;
 }
 
-- 
1.8.1.5

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

Re: [PATCH v3 4/4] drm/i915: Implement proper clipping for video sprites

[Chris Wilson]

On Tue, Apr 16, 2013 at 01:47:22PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Properly clip the source when the destination gets clipped
> by the pipe dimensions.
> 
> Sadly the video sprite hardware is rather limited so it can't do proper
> sub-pixel postitioning. Resort to truncating the source coordinates to
> (macro)pixel boundary.
> 
> The scaling checks are done using the relaxed drm_region functions.
> That means that the src/dst regions are reduced in size in order to keep
> the scaling factor within the limits.
> 
> Also do some additional checking against various hardware limits.
> 
> v2: Truncate src coords instead of rounding to avoid increasing src
>     viewport size, and adapt to changes in drm_calc_{h,v}scale().
> v3: Adapt to drm_region->drm_rect rename. Fix misaligned crtc_w for
>     packed YUV formats when scaling isn't supported.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Skipping to the end, use of drm_rect looks good.

The one ugly that stood out is:

>  	/*
>  	 * If the sprite is completely covering the primary plane,
>  	 * we can disable the primary and save power.
>  	 */
> -	if ((crtc_x == 0) && (crtc_y == 0) &&
> +	if (visible &&
> +	    (crtc_x == 0) && (crtc_y == 0) &&
>  	    (crtc_w == primary_w) && (crtc_h == primary_h))
>  		disable_primary = true;

which would be
  disable_primary = drm_rect_equals(&dst, &clip);
  BUG_ON(disable_primary && !visible);
with a little bit of massaging of crtc/dst.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3 4/4] drm/i915: Implement proper clipping for video sprites

[Ville Syrjälä]

On Tue, Apr 16, 2013 at 02:37:24PM +0100, Chris Wilson wrote:
> On Tue, Apr 16, 2013 at 01:47:22PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Properly clip the source when the destination gets clipped
> > by the pipe dimensions.
> > 
> > Sadly the video sprite hardware is rather limited so it can't do proper
> > sub-pixel postitioning. Resort to truncating the source coordinates to
> > (macro)pixel boundary.
> > 
> > The scaling checks are done using the relaxed drm_region functions.
> > That means that the src/dst regions are reduced in size in order to keep
> > the scaling factor within the limits.
> > 
> > Also do some additional checking against various hardware limits.
> > 
> > v2: Truncate src coords instead of rounding to avoid increasing src
> >     viewport size, and adapt to changes in drm_calc_{h,v}scale().
> > v3: Adapt to drm_region->drm_rect rename. Fix misaligned crtc_w for
> >     packed YUV formats when scaling isn't supported.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Skipping to the end, use of drm_rect looks good.
> 
> The one ugly that stood out is:
> 
> >  	/*
> >  	 * If the sprite is completely covering the primary plane,
> >  	 * we can disable the primary and save power.
> >  	 */
> > -	if ((crtc_x == 0) && (crtc_y == 0) &&
> > +	if (visible &&
> > +	    (crtc_x == 0) && (crtc_y == 0) &&
> >  	    (crtc_w == primary_w) && (crtc_h == primary_h))
> >  		disable_primary = true;
> 
> which would be
>   disable_primary = drm_rect_equals(&dst, &clip);
>   BUG_ON(disable_primary && !visible);
> with a little bit of massaging of crtc/dst.

Yeah, looks nicer. I'll add drm_rect_equals() to our repertoire.

-- 
Ville Syrjälä
Intel OTC

[PATCH v3 1/4] drm: Add struct drm_rect and assorted utility functions

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

struct drm_rect represents a simple rectangle. The utility
functions are there to help driver writers.

v2: Moved the region stuff into its own file, made the smaller funcs
    static inline, used 64bit maths in the scaled clipping function to
    avoid overflows (instead it will saturate to INT_MIN or INT_MAX).
v3: Renamed drm_region to drm_rect, drm_region_clip to
    drm_rect_intersect, and drm_region_subsample to drm_rect_downscale.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/Makefile   |   3 +-
 drivers/gpu/drm/drm_rect.c |  96 +++++++++++++++++++++++++++++++++
 include/drm/drm_rect.h     | 132 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 230 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/drm_rect.c
 create mode 100644 include/drm/drm_rect.h

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 0d59b24..8f94018 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -12,7 +12,8 @@ drm-y       :=	drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
 		drm_platform.o drm_sysfs.o drm_hashtab.o drm_mm.o \
 		drm_crtc.o drm_modes.o drm_edid.o \
 		drm_info.o drm_debugfs.o drm_encoder_slave.o \
-		drm_trace_points.o drm_global.o drm_prime.o
+		drm_trace_points.o drm_global.o drm_prime.o \
+		drm_rect.o
 
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
new file mode 100644
index 0000000..1ad4f5e
--- /dev/null
+++ b/drivers/gpu/drm/drm_rect.c
@@ -0,0 +1,96 @@
+/*
+ * Copyright (C) 2011-2013 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <drm/drm_rect.h>
+
+/**
+ * drm_rect_intersect - intersect two rectangles
+ * @r1: first rectangle
+ * @r2: second rectangle
+ *
+ * Calculate the intersection of rectangles @r1 and @r2.
+ * @r1 will be overwritten with the intersection.
+ *
+ * RETURNS:
+ * @true if rectangle @r1 is still visible after the operation,
+ * @false otherwise.
+ */
+bool drm_rect_intersect(struct drm_rect *r1, const struct drm_rect *r2)
+{
+	r1->x1 = max(r1->x1, r2->x1);
+	r1->y1 = max(r1->y1, r2->y1);
+	r1->x2 = min(r1->x2, r2->x2);
+	r1->y2 = min(r1->y2, r2->y2);
+
+	return drm_rect_visible(r1);
+}
+EXPORT_SYMBOL(drm_rect_intersect);
+
+/**
+ * drm_rect_clip_scaled - perform a scaled clip operation
+ * @src: source window rectangle
+ * @dst: destination window rectangle
+ * @clip: clip rectangle
+ * @hscale: horizontal scaling factor
+ * @vscale: vertical scaling factor
+ *
+ * Clip rectangle @dst by rectangle @clip. Clip rectangle @src by the
+ * same amounts multiplied by @hscale and @vscale.
+ *
+ * RETUTRNS:
+ * @true if rectangle @dst is still visible after being clipped,
+ * @false otherwise
+ */
+bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
+			  const struct drm_rect *clip,
+			  int hscale, int vscale)
+{
+	int diff;
+
+	diff = clip->x1 - dst->x1;
+	if (diff > 0) {
+		int64_t tmp = src->x1 + (int64_t) diff * hscale;
+		src->x1 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
+	}
+	diff = clip->y1 - dst->y1;
+	if (diff > 0) {
+		int64_t tmp = src->y1 + (int64_t) diff * vscale;
+		src->y1 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
+	}
+	diff = dst->x2 - clip->x2;
+	if (diff > 0) {
+		int64_t tmp = src->x2 - (int64_t) diff * hscale;
+		src->x2 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
+	}
+	diff = dst->y2 - clip->y2;
+	if (diff > 0) {
+		int64_t tmp = src->y2 - (int64_t) diff * vscale;
+		src->y2 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
+	}
+
+	return drm_rect_intersect(dst, clip);
+}
+EXPORT_SYMBOL(drm_rect_clip_scaled);
diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
new file mode 100644
index 0000000..40b09a4
--- /dev/null
+++ b/include/drm/drm_rect.h
@@ -0,0 +1,132 @@
+/*
+ * Copyright (C) 2011-2013 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef DRM_RECT_H
+#define DRM_RECT_H
+
+/**
+ * drm_rect - two dimensional rect
+ * @x1: horizontal starting coordinate (inclusive)
+ * @x2: horizontal ending coordinate (exclusive)
+ * @y1: vertical starting coordinate (inclusive)
+ * @y2: vertical ending coordinate (exclusive)
+ */
+struct drm_rect {
+	int x1, y1, x2, y2;
+};
+
+/**
+ * drm_rect_adjust_size - adjust the size of the rect
+ * @r: rect to be adjusted
+ * @x: horizontal adjustment
+ * @y: vertical adjustment
+ *
+ * Change the size of rect @r by @x in the horizontal direction,
+ * and by @y in the vertical direction, while keeping the center
+ * of @r stationary.
+ *
+ * Positive @x and @y increase the size, negative values decrease it.
+ */
+static inline void drm_rect_adjust_size(struct drm_rect *r, int x, int y)
+{
+	r->x1 -= x >> 1;
+	r->y1 -= y >> 1;
+	r->x2 += (x + 1) >> 1;
+	r->y2 += (y + 1) >> 1;
+}
+
+/**
+ * drm_rect_translate - translate the rect
+ * @r: rect to be tranlated
+ * @x: horizontal translation
+ * @y: vertical translation
+ *
+ * Move rect @r by @x in the horizontal direction,
+ * and by @y in the vertical direction.
+ */
+static inline void drm_rect_translate(struct drm_rect *r, int x, int y)
+{
+	r->x1 += x;
+	r->y1 += y;
+	r->x2 += x;
+	r->y2 += y;
+}
+
+/**
+ * drm_rect_downscale - downscale a rect
+ * @r: rect to be downscaled
+ * @horz: horizontal downscale factor
+ * @vert: vertical downscale factor
+ *
+ * Divide the coordinates of rect @r by @horz and @vert.
+ */
+static inline void drm_rect_downscale(struct drm_rect *r, int horz, int vert)
+{
+	r->x1 /= horz;
+	r->y1 /= vert;
+	r->x2 /= horz;
+	r->y2 /= vert;
+}
+
+/**
+ * drm_rect_width - determine the rect width
+ * @r: rect whose width is returned
+ *
+ * RETURNS:
+ * The width of the rect.
+ */
+static inline int drm_rect_width(const struct drm_rect *r)
+{
+	return r->x2 - r->x1;
+}
+
+/**
+ * drm_rect_height - determine the rect height
+ * @r: rect whose height is returned
+ *
+ * RETURNS:
+ * The height of the rect.
+ */
+static inline int drm_rect_height(const struct drm_rect *r)
+{
+	return r->y2 - r->y1;
+}
+
+/**
+ * drm_rect_visible - determine if the the rect is visible
+ * @r: rect whose visibility is returned
+ *
+ * RETURNS:
+ * @true if the rect is visible, @false otherwise.
+ */
+static inline bool drm_rect_visible(const struct drm_rect *r)
+{
+	return drm_rect_width(r) > 0 && drm_rect_height(r) > 0;
+}
+
+bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip);
+bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
+			  const struct drm_rect *clip,
+			  int hscale, int vscale);
+
+#endif
-- 
1.8.1.5

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

[PATCH v3 4/4] drm/i915: Implement proper clipping for video sprites

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Properly clip the source when the destination gets clipped
by the pipe dimensions.

Sadly the video sprite hardware is rather limited so it can't do proper
sub-pixel postitioning. Resort to truncating the source coordinates to
(macro)pixel boundary.

The scaling checks are done using the relaxed drm_region functions.
That means that the src/dst regions are reduced in size in order to keep
the scaling factor within the limits.

Also do some additional checking against various hardware limits.

v2: Truncate src coords instead of rounding to avoid increasing src
    viewport size, and adapt to changes in drm_calc_{h,v}scale().
v3: Adapt to drm_region->drm_rect rename. Fix misaligned crtc_w for
    packed YUV formats when scaling isn't supported.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 191 +++++++++++++++++++++++++++---------
 1 file changed, 145 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 414d325..62e09d1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -32,6 +32,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_rect.h>
 #include "intel_drv.h"
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
@@ -415,6 +416,20 @@ ilk_get_colorkey(struct drm_plane *plane, struct drm_intel_sprite_colorkey *key)
 		key->flags = I915_SET_COLORKEY_NONE;
 }
 
+static bool
+format_is_yuv(uint32_t format)
+{
+	switch (format) {
+	case DRM_FORMAT_YUYV:
+	case DRM_FORMAT_UYVY:
+	case DRM_FORMAT_VYUY:
+	case DRM_FORMAT_YVYU:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static int
 intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
@@ -432,9 +447,28 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
 								      pipe);
 	int ret = 0;
-	int x = src_x >> 16, y = src_y >> 16;
 	int primary_w = crtc->mode.hdisplay, primary_h = crtc->mode.vdisplay;
 	bool disable_primary = false;
+	bool visible;
+	int hscale, vscale;
+	int max_scale, min_scale;
+	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	struct drm_rect src = {
+		.x1 = src_x,
+		.x2 = src_x + src_w,
+		.y1 = src_y,
+		.y2 = src_y + src_h,
+	};
+	struct drm_rect dst = {
+		.x1 = crtc_x,
+		.x2 = crtc_x + crtc_w,
+		.y1 = crtc_y,
+		.y2 = crtc_y + crtc_h,
+	};
+	const struct drm_rect clip = {
+		.x2 = crtc->mode.hdisplay,
+		.y2 = crtc->mode.vdisplay,
+	};
 
 	intel_fb = to_intel_framebuffer(fb);
 	obj = intel_fb->obj;
@@ -450,19 +484,23 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	intel_plane->src_w = src_w;
 	intel_plane->src_h = src_h;
 
-	src_w = src_w >> 16;
-	src_h = src_h >> 16;
-
 	/* Pipe must be running... */
-	if (!(I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE))
-		return -EINVAL;
-
-	if (crtc_x >= primary_w || crtc_y >= primary_h)
+	if (!(I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE)) {
+		DRM_DEBUG_KMS("Pipe disabled\n");
 		return -EINVAL;
+	}
 
 	/* Don't modify another pipe's plane */
-	if (intel_plane->pipe != intel_crtc->pipe)
+	if (intel_plane->pipe != intel_crtc->pipe) {
+		DRM_DEBUG_KMS("Wrong plane <-> crtc mapping\n");
 		return -EINVAL;
+	}
+
+	/* FIXME check all gen limits */
+	if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > 16384) {
+		DRM_DEBUG_KMS("Unsuitable framebuffer for plane\n");
+		return -EINVAL;
+	}
 
 	/* Sprite planes can be linear or x-tiled surfaces */
 	switch (obj->tiling_mode) {
@@ -470,53 +508,111 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		case I915_TILING_X:
 			break;
 		default:
+			DRM_DEBUG_KMS("Unsupported tiling mode\n");
 			return -EINVAL;
 	}
 
 	/*
-	 * Clamp the width & height into the visible area.  Note we don't
-	 * try to scale the source if part of the visible region is offscreen.
-	 * The caller must handle that by adjusting source offset and size.
+	 * 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.
 	 */
-	if ((crtc_x < 0) && ((crtc_x + crtc_w) > 0)) {
-		crtc_w += crtc_x;
-		crtc_x = 0;
+	max_scale = intel_plane->max_downscale << 16;
+	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
+
+	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);
+
+	visible = drm_rect_clip_scaled(&src, &dst, &clip, hscale, vscale);
+
+	if (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, true);
+			drm_rect_debug_print(&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, true);
+			drm_rect_debug_print(&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));
+
+		crtc_x = dst.x1;
+		crtc_y = dst.y1;
+		crtc_w = drm_rect_width(&dst);
+		crtc_h = drm_rect_height(&dst);
+
+		/*
+		 * 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 (format_is_yuv(fb->pixel_format)) {
+			src_x &= ~1;
+			src_w &= ~1;
+
+			/*
+			 * Must keep src and dst the
+			 * same if we can't scale.
+			 */
+			if (!intel_plane->can_scale)
+				crtc_w &= ~1;
+		}
 	}
-	if ((crtc_x + crtc_w) <= 0) /* Nothing to display */
-		goto out;
-	if ((crtc_x + crtc_w) > primary_w)
-		crtc_w = primary_w - crtc_x;
-
-	if ((crtc_y < 0) && ((crtc_y + crtc_h) > 0)) {
-		crtc_h += crtc_y;
-		crtc_y = 0;
+
+	/* Account for minimum size when scaling */
+	if (visible && (src_w != crtc_w || src_h != crtc_h)) {
+		BUG_ON(!intel_plane->can_scale);
+
+		/* FIXME interlacing min height is 6 */
+		/* FIXME return an error instead? */
+
+		if (crtc_w < 3 || crtc_h < 3)
+			visible = false;
+
+		if (src_w < 3 || src_h < 3)
+			visible = false;
 	}
-	if ((crtc_y + crtc_h) <= 0) /* Nothing to display */
-		goto out;
-	if (crtc_y + crtc_h > primary_h)
-		crtc_h = primary_h - crtc_y;
 
-	if (!crtc_w || !crtc_h) /* Again, nothing to display */
-		goto out;
+	/* Check size restrictions when scaling */
+	if (visible && (src_w != crtc_w || src_h != crtc_h)) {
+		unsigned int width_bytes = ((src_x * pixel_size) & 63) + src_w * pixel_size;
 
-	/*
-	 * We may not have a scaler, eg. HSW does not have it any more
-	 */
-	if (!intel_plane->can_scale && (crtc_w != src_w || crtc_h != src_h))
-		return -EINVAL;
+		BUG_ON(!intel_plane->can_scale);
 
-	/*
-	 * We can take a larger source and scale it down, but
-	 * only so much...  16x is the max on SNB.
-	 */
-	if (((src_w * src_h) / (crtc_w * crtc_h)) > intel_plane->max_downscale)
-		return -EINVAL;
+		if (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 the sprite is completely covering the primary plane,
 	 * we can disable the primary and save power.
 	 */
-	if ((crtc_x == 0) && (crtc_y == 0) &&
+	if (visible &&
+	    (crtc_x == 0) && (crtc_y == 0) &&
 	    (crtc_w == primary_w) && (crtc_h == primary_h))
 		disable_primary = true;
 
@@ -540,11 +636,15 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (!disable_primary)
 		intel_enable_primary(crtc);
 
-	intel_plane->update_plane(plane, fb, obj, crtc_x, crtc_y,
-				  crtc_w, crtc_h, x, y, src_w, src_h);
+	if (visible) {
+		intel_plane->update_plane(plane, fb, obj,
+					  crtc_x, crtc_y, crtc_w, crtc_h,
+					  src_x, src_y, src_w, src_h);
 
-	if (disable_primary)
-		intel_disable_primary(crtc);
+		if (disable_primary)
+			intel_disable_primary(crtc);
+	} else
+		intel_plane->disable_plane(plane);
 
 	/* Unpin old obj after new one is active to avoid ugliness */
 	if (old_obj) {
@@ -564,7 +664,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 out_unlock:
 	mutex_unlock(&dev->struct_mutex);
-out:
 	return ret;
 }
 
-- 
1.8.1.5

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