From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v3 1/4] drm: Add struct drm_rect and assorted utility functions Date: Fri, 05 Apr 2013 01:51:24 +0200 Message-ID: <8128127.OV61LSCs9m@avalon> References: <1364399185-23694-1-git-send-email-ville.syrjala@linux.intel.com> <3423680.vJorC32jN4@avalon> <20130404195237.GA4469@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20130404195237.GA4469@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Ville =?ISO-8859-1?Q?Syrj=E4l=E4?= Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org Hi Ville, On Thursday 04 April 2013 22:52:37 Ville Syrj=E4l=E4 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=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_downscal= e. > > > = > > > 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. I'm fine with whichever you think will be better. I just wanted to raise th= is = 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 =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.0x000= 0, > 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 explic= it > > 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, i= nt > > > 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 > > > *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