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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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
-- 
Regards,

Laurent Pinchart