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

[Ville Syrjälä]

On Fri, Apr 05, 2013 at 01:51:24AM +0200, Laurent Pinchart wrote:
> 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.

I changed it to '%' since that's apparently meant for constants.

> > 
> > > > + */
> > > > +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.

I left this as is for now. We can split it later if there's need for it.

<snip>
> > > > +/**
> > > > + * 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.

I decided to leave the function as is. Using unsigned int for the
arguments would promote the numerator of the division to unsigned int
as well, and then the result would be wrong. So we'd either require
explicit casts to int, or we'd need to go with something like unsigned
short instead.

-- 
Ville Syrjälä
Intel OTC