| Message ID | 1364399185-23694-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ville, Thanks for the patch. 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 ? > + */ > +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 ? > +} > +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 ? > + */ > +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. > + * 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 ? > + * > + * 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 ? > +{ > + 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
On Thu, Apr 04, 2013 at 06:38:15PM +0200, Laurent Pinchart wrote: > Hi Ville, > > Thanks for the patch. > > 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. > > > +} > > +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. > > > + * 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 > -- > Regards, > > Laurent Pinchart
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
On Fri, Apr 05, 2013 at 01:51:24AM +0200, Laurent Pinchart wrote: > 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. I changed it to '%' since that's apparently meant for constants. > > > > > > + */ > > > > +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. I left this as is for now. We can split it later if there's need for it. <snip> > > > > +/** > > > > + * 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. I decided to leave the function as is. Using unsigned int for the arguments would promote the numerator of the division to unsigned int as well, and then the result would be wrong. So we'd either require explicit casts to int, or we'd need to go with something like unsigned short instead.
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