From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v3 1/4] drm: Add struct drm_rect and assorted utility functions Date: Thu, 4 Apr 2013 22:52:37 +0300 Message-ID: <20130404195237.GA4469@intel.com> References: <1364399185-23694-1-git-send-email-ville.syrjala@linux.intel.com> <3423680.vJorC32jN4@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <3423680.vJorC32jN4@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Laurent Pinchart Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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=E4l=E4 > > = > > 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=E4l=E4 > > --- > > 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 > > +#include > > +#include > > +#include > > + > > +/** > > + * 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 =3D max(r1->x1, r2->x1); > > + r1->y1 =3D max(r1->y1, r2->y1); > > + r1->x2 =3D min(r1->x2, r2->x2); > > + r1->y2 =3D 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 =3D clip->x1 - dst->x1; > > + if (diff > 0) { > > + int64_t tmp =3D src->x1 + (int64_t) diff * hscale; > > + src->x1 =3D clamp_t(int64_t, tmp, INT_MIN, INT_MAX); > > + } > > + diff =3D clip->y1 - dst->y1; > > + if (diff > 0) { > > + int64_t tmp =3D src->y1 + (int64_t) diff * vscale; > > + src->y1 =3D clamp_t(int64_t, tmp, INT_MIN, INT_MAX); > > + } > > + diff =3D dst->x2 - clip->x2; > > + if (diff > 0) { > > + int64_t tmp =3D src->x2 - (int64_t) diff * hscale; > > + src->x2 =3D clamp_t(int64_t, tmp, INT_MIN, INT_MAX); > > + } > > + diff =3D dst->y2 - clip->y2; > > + if (diff > 0) { > > + int64_t tmp =3D src->y2 - (int64_t) diff * vscale; > > + src->y2 =3D 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=3D0.0x0000 x2=3D10.0x0000, rather than x1=3D0.0x0000 x2=3D9.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 -=3D x >> 1; > > + r->y1 -=3D y >> 1; > > + r->x2 +=3D (x + 1) >> 1; > > + r->y2 +=3D (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 +=3D x; > > + r->y1 +=3D y; > > + r->x2 +=3D x; > > + r->y2 +=3D 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 /=3D horz; > > + r->y1 /=3D vert; > > + r->x2 /=3D horz; > > + r->y2 /=3D 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 *cli= p); > > +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=E4l=E4 Intel OTC