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

Commit Message

Ville Syrjala April 16, 2013, 10:47 a.m. UTC

From: Ville Syrjälä <[e-mail]>

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ä <[e-mail]>
---
 drivers/gpu/drm/drm_rect.c | 177 +++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_rect.h     |  12 +++
 2 files changed, 189 insertions(+)

Comments

Chris Wilson April 16, 2013, 1:42 p.m. UTC | #1

On Tue, Apr 16, 2013 at 01:47:20PM +0300, [e-mail] 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

Ville Syrjala April 16, 2013, 2:14 p.m. UTC | #2

On Tue, Apr 16, 2013 at 02:42:34PM +0100, Chris Wilson wrote:
> On Tue, Apr 16, 2013 at 01:47:20PM +0300, [e-mail] 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.

Chris Wilson April 16, 2013, 2:49 p.m. UTC | #3

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, [e-mail] 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

Ville Syrjala April 16, 2013, 3:16 p.m. UTC | #4

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, [e-mail] 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.

Chris Wilson April 16, 2013, 3:27 p.m. UTC | #5

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

Patch

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