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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC