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

[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 at linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala at 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 at 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
-- 
Regards,

Laurent Pinchart