[PATCH v6 1/5] drm: Add function to convert rect in 16.16 fixed format to regular format

[PATCH v6 1/5] drm: Add function to convert rect in 16.16 fixed format to regular format

[José Roberto de Souza]

Much more clear to read one function call than four lines doing this
conversion.

Cc: dri-devel@lists.freedesktop.org
Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/drm_rect.c | 15 +++++++++++++++
 include/drm/drm_rect.h     |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
index 0460e874896e..24345704b353 100644
--- a/drivers/gpu/drm/drm_rect.c
+++ b/drivers/gpu/drm/drm_rect.c
@@ -373,3 +373,18 @@ void drm_rect_rotate_inv(struct drm_rect *r,
 	}
 }
 EXPORT_SYMBOL(drm_rect_rotate_inv);
+
+/**
+ * drm_rect_convert_16_16_to_regular - Convert a rect in 16.16 fixed point form
+ * to regular form.
+ * @in: rect in 16.16 fixed point form
+ * @out: rect to be stored the converted value
+ */
+void drm_rect_convert_16_16_to_regular(struct drm_rect *in, struct drm_rect *out)
+{
+	out->x1 = in->x1 >> 16;
+	out->y1 = in->y1 >> 16;
+	out->x2 = in->x2 >> 16;
+	out->y2 = in->y2 >> 16;
+}
+EXPORT_SYMBOL(drm_rect_convert_16_16_to_regular);
diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
index e7f4d24cdd00..2ef8180416cd 100644
--- a/include/drm/drm_rect.h
+++ b/include/drm/drm_rect.h
@@ -223,5 +223,7 @@ void drm_rect_rotate(struct drm_rect *r,
 void drm_rect_rotate_inv(struct drm_rect *r,
 			 int width, int height,
 			 unsigned int rotation);
+void drm_rect_convert_16_16_to_regular(struct drm_rect *in,
+				       struct drm_rect *out);
 
 #endif
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 1/5] drm: Add function to convert rect in 16.16 fixed format to regular format

[Mun, Gwan-gyeong]

On Mon, 2020-12-14 at 09:49 -0800, José Roberto de Souza wrote:
> Much more clear to read one function call than four lines doing this
> conversion.
> 
> Cc: dri-devel@lists.freedesktop.org
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/drm_rect.c | 15 +++++++++++++++
>  include/drm/drm_rect.h     |  2 ++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> index 0460e874896e..24345704b353 100644
> --- a/drivers/gpu/drm/drm_rect.c
> +++ b/drivers/gpu/drm/drm_rect.c
> @@ -373,3 +373,18 @@ void drm_rect_rotate_inv(struct drm_rect *r,
>  	}
>  }
>  EXPORT_SYMBOL(drm_rect_rotate_inv);
> +
> +/**
> + * drm_rect_convert_16_16_to_regular - Convert a rect in 16.16 fixed
> point form
> + * to regular form.
> + * @in: rect in 16.16 fixed point form
> + * @out: rect to be stored the converted value
> + */
> +void drm_rect_convert_16_16_to_regular(struct drm_rect *in, struct
> drm_rect *out)
> +{
> +	out->x1 = in->x1 >> 16;
> +	out->y1 = in->y1 >> 16;
> +	out->x2 = in->x2 >> 16;
> +	out->y2 = in->y2 >> 16;
> +}
> +EXPORT_SYMBOL(drm_rect_convert_16_16_to_regular);
> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> index e7f4d24cdd00..2ef8180416cd 100644
> --- a/include/drm/drm_rect.h
> +++ b/include/drm/drm_rect.h
> @@ -223,5 +223,7 @@ void drm_rect_rotate(struct drm_rect *r,
>  void drm_rect_rotate_inv(struct drm_rect *r,
>  			 int width, int height,
>  			 unsigned int rotation);
> +void drm_rect_convert_16_16_to_regular(struct drm_rect *in,
> +				       struct drm_rect *out);
> 
Hi,
if it's purpose is just converting 16.16 fp to integer, how about you
have function prototype like this?
extern inline struct drm_rect
drm_rect_convert_16_16_fp_to_integer(struct drm_rect in)

And if there are no use case on drm core or other drivers except i915
display yet,
before adding this function to drm core, how about you add this
function code to i915 first?

Br,
G.G.
>  #endif
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 1/5] drm: Add function to convert rect in 16.16 fixed format to regular format

[Souza, Jose]

On Tue, 2020-12-15 at 12:52 +0000, Mun, Gwan-gyeong wrote:
> On Mon, 2020-12-14 at 09:49 -0800, José Roberto de Souza wrote:
> > Much more clear to read one function call than four lines doing this
> > conversion.
> > 
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/drm_rect.c | 15 +++++++++++++++
> >  include/drm/drm_rect.h     |  2 ++
> >  2 files changed, 17 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> > index 0460e874896e..24345704b353 100644
> > --- a/drivers/gpu/drm/drm_rect.c
> > +++ b/drivers/gpu/drm/drm_rect.c
> > @@ -373,3 +373,18 @@ void drm_rect_rotate_inv(struct drm_rect *r,
> >  	}
> >  }
> >  EXPORT_SYMBOL(drm_rect_rotate_inv);
> > +
> > +/**
> > + * drm_rect_convert_16_16_to_regular - Convert a rect in 16.16 fixed
> > point form
> > + * to regular form.
> > + * @in: rect in 16.16 fixed point form
> > + * @out: rect to be stored the converted value
> > + */
> > +void drm_rect_convert_16_16_to_regular(struct drm_rect *in, struct
> > drm_rect *out)
> > +{
> > +	out->x1 = in->x1 >> 16;
> > +	out->y1 = in->y1 >> 16;
> > +	out->x2 = in->x2 >> 16;
> > +	out->y2 = in->y2 >> 16;
> > +}
> > +EXPORT_SYMBOL(drm_rect_convert_16_16_to_regular);
> > diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> > index e7f4d24cdd00..2ef8180416cd 100644
> > --- a/include/drm/drm_rect.h
> > +++ b/include/drm/drm_rect.h
> > @@ -223,5 +223,7 @@ void drm_rect_rotate(struct drm_rect *r,
> >  void drm_rect_rotate_inv(struct drm_rect *r,
> >  			 int width, int height,
> >  			 unsigned int rotation);
> > +void drm_rect_convert_16_16_to_regular(struct drm_rect *in,
> > +				       struct drm_rect *out);
> > 
> Hi,
> if it's purpose is just converting 16.16 fp to integer, how about you
> have function prototype like this?
> extern inline struct drm_rect
> drm_rect_convert_16_16_fp_to_integer(struct drm_rect in)

I prefer have a function call as this can be reused in several places, so the binaries size can decrease a bit.
Also pointers are better, compiler can decide to not inline the function above and it would need to allocate in stack 2 drm_rects for every call.

> 
> And if there are no use case on drm core or other drivers except i915
> display yet,
> before adding this function to drm core, how about you add this
> function code to i915 first?

There is plenty of users in other drivers, just not doing in this series.

> 
> Br,
> G.G.
> >  #endif

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 1/5] drm: Add function to convert rect in 16.16 fixed format to regular format

[Ville Syrjälä]

On Mon, Dec 14, 2020 at 09:49:08AM -0800, José Roberto de Souza wrote:
> Much more clear to read one function call than four lines doing this
> conversion.
> 
> Cc: dri-devel@lists.freedesktop.org
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/drm_rect.c | 15 +++++++++++++++
>  include/drm/drm_rect.h     |  2 ++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> index 0460e874896e..24345704b353 100644
> --- a/drivers/gpu/drm/drm_rect.c
> +++ b/drivers/gpu/drm/drm_rect.c
> @@ -373,3 +373,18 @@ void drm_rect_rotate_inv(struct drm_rect *r,
>  	}
>  }
>  EXPORT_SYMBOL(drm_rect_rotate_inv);
> +
> +/**
> + * drm_rect_convert_16_16_to_regular - Convert a rect in 16.16 fixed point form
> + * to regular form.
> + * @in: rect in 16.16 fixed point form
> + * @out: rect to be stored the converted value
> + */
> +void drm_rect_convert_16_16_to_regular(struct drm_rect *in, struct drm_rect *out)
> +{
> +	out->x1 = in->x1 >> 16;
> +	out->y1 = in->y1 >> 16;
> +	out->x2 = in->x2 >> 16;
> +	out->y2 = in->y2 >> 16;
> +}

That's not the same as what we do in most places. We truncate
the width/height, not x2/y2. Doing it on x2/y2 may increase
the width/height.

So I suggest something more like:

static inline void drm_rect_fp_to_int(struct drm_rect *r)
{
	drm_rect_init(r, r->x1 >> 16, r->y1 >> 16,
		      drm_rect_width(r) >> 16,
		      drm_rect_height(r) >> 16);
}

to match the current way of doing things.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 1/5] drm: Add function to convert rect in 16.16 fixed format to regular format

[Souza, Jose]

On Tue, 2020-12-15 at 16:44 +0200, Ville Syrjälä wrote:
> On Mon, Dec 14, 2020 at 09:49:08AM -0800, José Roberto de Souza wrote:
> > Much more clear to read one function call than four lines doing this
> > conversion.
> > 
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/drm_rect.c | 15 +++++++++++++++
> >  include/drm/drm_rect.h     |  2 ++
> >  2 files changed, 17 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> > index 0460e874896e..24345704b353 100644
> > --- a/drivers/gpu/drm/drm_rect.c
> > +++ b/drivers/gpu/drm/drm_rect.c
> > @@ -373,3 +373,18 @@ void drm_rect_rotate_inv(struct drm_rect *r,
> >  	}
> >  }
> >  EXPORT_SYMBOL(drm_rect_rotate_inv);
> > +
> > +/**
> > + * drm_rect_convert_16_16_to_regular - Convert a rect in 16.16 fixed point form
> > + * to regular form.
> > + * @in: rect in 16.16 fixed point form
> > + * @out: rect to be stored the converted value
> > + */
> > +void drm_rect_convert_16_16_to_regular(struct drm_rect *in, struct drm_rect *out)
> > +{
> > +	out->x1 = in->x1 >> 16;
> > +	out->y1 = in->y1 >> 16;
> > +	out->x2 = in->x2 >> 16;
> > +	out->y2 = in->y2 >> 16;
> > +}
> 
> That's not the same as what we do in most places. We truncate
> the width/height, not x2/y2. Doing it on x2/y2 may increase
> the width/height.
> 
> So I suggest something more like:
> 
> static inline void drm_rect_fp_to_int(struct drm_rect *r)
> {
> 	drm_rect_init(r, r->x1 >> 16, r->y1 >> 16,
> 		      drm_rect_width(r) >> 16,
> 		      drm_rect_height(r) >> 16);
> }
> 
> to match the current way of doing things.

Okay, but most use cases takes drm_plane_state.src and converts and sets it in another rect, so will modify it to have two parameters.


> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 1/5] drm: Add function to convert rect in 16.16 fixed format to regular format

[Ville Syrjälä]

On Tue, Dec 15, 2020 at 03:43:00PM +0000, Souza, Jose wrote:
> On Tue, 2020-12-15 at 16:44 +0200, Ville Syrjälä wrote:
> > On Mon, Dec 14, 2020 at 09:49:08AM -0800, José Roberto de Souza wrote:
> > > Much more clear to read one function call than four lines doing this
> > > conversion.
> > > 
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_rect.c | 15 +++++++++++++++
> > >  include/drm/drm_rect.h     |  2 ++
> > >  2 files changed, 17 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> > > index 0460e874896e..24345704b353 100644
> > > --- a/drivers/gpu/drm/drm_rect.c
> > > +++ b/drivers/gpu/drm/drm_rect.c
> > > @@ -373,3 +373,18 @@ void drm_rect_rotate_inv(struct drm_rect *r,
> > >  	}
> > >  }
> > >  EXPORT_SYMBOL(drm_rect_rotate_inv);
> > > +
> > > +/**
> > > + * drm_rect_convert_16_16_to_regular - Convert a rect in 16.16 fixed point form
> > > + * to regular form.
> > > + * @in: rect in 16.16 fixed point form
> > > + * @out: rect to be stored the converted value
> > > + */
> > > +void drm_rect_convert_16_16_to_regular(struct drm_rect *in, struct drm_rect *out)
> > > +{
> > > +	out->x1 = in->x1 >> 16;
> > > +	out->y1 = in->y1 >> 16;
> > > +	out->x2 = in->x2 >> 16;
> > > +	out->y2 = in->y2 >> 16;
> > > +}
> > 
> > That's not the same as what we do in most places. We truncate
> > the width/height, not x2/y2. Doing it on x2/y2 may increase
> > the width/height.
> > 
> > So I suggest something more like:
> > 
> > static inline void drm_rect_fp_to_int(struct drm_rect *r)
> > {
> > 	drm_rect_init(r, r->x1 >> 16, r->y1 >> 16,
> > 		      drm_rect_width(r) >> 16,
> > 		      drm_rect_height(r) >> 16);
> > }
> > 
> > to match the current way of doing things.
> 
> Okay, but most use cases takes drm_plane_state.src and converts and sets it in another rect, so will modify it to have two parameters.

Would seem a bit more generic by having the caller make the copy
if needed. But I guess not big deal either way.

At least make it follow the correct argument order as laid out
by memcpy() ;) (+const for the input argument ofc).

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel