| Message ID | 20201214174912.174065-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6,1/5] drm: Add function to convert rect in 16.16 fixed format to regular format | expand |
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
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
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.
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. >
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).
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
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(+)