Hi Ville,
On Thursday 04 April 2013 22:52:37 Ville Syrjälä wrote:
On Thu, Apr 04, 2013 at 06:38:15PM +0200, Laurent Pinchart wrote:
On Wednesday 27 March 2013 17:46:22 ville.syrjala@linux.intel.com wrote:
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
[snip]
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 @@
[snip]
+#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.
Isn't @ used for function parameters only ?
Not sure. It's been a while since I wrote these, and I guess I thought that the @ was there just for higlighting purposes. Looks like the documentation for the documentation system isn't that great :) so I guess I'll need to try building the docs and see what happens.
- */
+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);
Do callers always need that information, or should they instead call drm_rect_visible() explicitly when they need it ?
I suppose someone might call it w/o checking the visibility right away. In my current use case I do use the return value, so it saves one line of code :) But I don't mind changing it if you think that would be better w/o the implicit drm_rect_visible() call.
I'm fine with whichever you think will be better. I just wanted to raise this point to make sure it has been thought about.
+} +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 @@
[snip]
+/**
- 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)
What's the rationale for making x2 and y2 exclusive ?
I think exclusive makes things easier.
You don't have to add/subtract 1 when going between x1/x2 and x/w representations. Based on some experience, it's surprisingly easy to accidentally forget the 1 or add/subtract too many times when doing this kind of conversions with inclusive end coordinates.
And especially with fixed point coordinates it's probably clearer that say a rectangle with width 10 has coordinates x1=0.0x0000 x2=10.0x0000, rather than x1=0.0x0000 x2=9.0xffff. IMHO the latter doesn't really convey the fact that the edge is exactly at an integer coordinate.
- */
+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
What about renaming x and y to dx and dy ? It would make it more explicit that the adjustements are incremental and not absolute values.
Good point. Or actually maybe they should be dw and dh.
That's even better, yes.
- 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
dx and dy here too ?
Sure.
- 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)
Shouldn't horz and vert be unsigned ?
Maybe. I'm actually not using this function currently (mainly because our current hardware doesn't support planar formats) so I could just remove the whole thing until it's needed.
+{
- 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