| Message ID | 20170508114902.18965-3-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 08-05-17 om 13:48 schreef Mahesh Kumar: > This patch adds few wrapper to perform fixed_point_16_16 operations > mul_u32_fixed_16_16_round_up : Multiplies u32 and fixed_16_16_t > variables & returns u32 result with > rounding-off. > mul_fixed_16_16 : Multiplies two fixed_16_16_t variable & returns > fixed_16_16 > div_round_up_fixed_16_16 : Perform division operation on fixed_16_16_t > variables & return u32 result with round-off > fixed_16_16_div_round_up_u64 : devide uint32_t variable by fixed_16_16 > variable and round_up the result to uint32_t. > > These wrappers will be used by later patches in the series. > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 43 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 5804734d30a3..5ff0091707c0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -152,6 +152,38 @@ static inline uint_fixed_16_16_t max_fixed_16_16(uint_fixed_16_16_t max1, > return max; > } > > +static inline uint32_t div_round_up_fixed_16_16(uint_fixed_16_16_t val, > + uint_fixed_16_16_t d) > +{ > + return DIV_ROUND_UP(val.val, d.val); > +} > + > +static inline uint32_t mul_u32_fixed_16_16_round_up(uint32_t val, > + uint_fixed_16_16_t mul) > +{ > + uint64_t intermediate_val; > + uint32_t result; > + > + intermediate_val = (uint64_t) val * mul.val; > + intermediate_val = DIV_ROUND_UP_ULL(intermediate_val, 1 << 16); > + WARN_ON(intermediate_val >> 32); > + result = clamp_t(uint32_t, intermediate_val, 0, ~0); > + return result; > +} > + > +static inline uint_fixed_16_16_t mul_fixed_16_16(uint_fixed_16_16_t val, > + uint_fixed_16_16_t mul) > +{ > + uint64_t intermediate_val; > + uint_fixed_16_16_t fp; > + > + intermediate_val = (uint64_t) val.val * mul.val; > + intermediate_val = intermediate_val >> 16; > + WARN_ON(intermediate_val >> 32); > + fp.val = clamp_t(uint32_t, intermediate_val, 0, ~0); > + return fp; > +} > + > static inline uint_fixed_16_16_t fixed_16_16_div(uint32_t val, uint32_t d) > { > uint_fixed_16_16_t fp, res; > @@ -174,6 +206,17 @@ static inline uint_fixed_16_16_t fixed_16_16_div_u64(uint32_t val, uint32_t d) > return res; > } > > +static inline uint32_t fixed_16_16_div_round_up_u64(uint32_t val, > + uint_fixed_16_16_t d) > +{ > + uint64_t interm_val; > + > + interm_val = (uint64_t)val << 16; > + interm_val = DIV_ROUND_UP_ULL(interm_val, d.val); > + WARN_ON(interm_val >> 32); > + return clamp_t(uint32_t, interm_val, 0, ~0); > +} > + > static inline uint_fixed_16_16_t mul_u32_fixed_16_16(uint32_t val, > uint_fixed_16_16_t mul) > { I wouldn't mind if those macros would be moved later to drm core, I don't think i915 is the only one to benefit from fixed 16.16 calculations. :) ~Maarten
On Mon, May 08, 2017 at 05:18:53PM +0530, Mahesh Kumar wrote: > This patch adds few wrapper to perform fixed_point_16_16 operations > mul_u32_fixed_16_16_round_up : Multiplies u32 and fixed_16_16_t > variables & returns u32 result with > rounding-off. > mul_fixed_16_16 : Multiplies two fixed_16_16_t variable & returns > fixed_16_16 > div_round_up_fixed_16_16 : Perform division operation on fixed_16_16_t > variables & return u32 result with round-off > fixed_16_16_div_round_up_u64 : devide uint32_t variable by fixed_16_16 > variable and round_up the result to uint32_t. Minor nitpick, but I'd say "rounding-up" rather than "rounding-off" in your descriptions here. > > These wrappers will be used by later patches in the series. The name on this last one (fixed_16_16_div_round_up_u64) doesn't seem to match what it does. You're using u64 internally, but the actual operands are a u32 and a fixed point value. Wouldn't it make more sense to call it div_round_up_u32_fixed_16_16 (that also keeps the dividend first and the divisor second, which seems more natural to me). Actually the naming of all the fixed point math functions is a bit confusing/inconsistent. Some of them are "op_type[_type][_round_up]," some of them are "op[_round_up]_type," some of them are "type_op[_round_up]," and some of them are "type_op[_round_up]_type." Also, none of them specify the return value type, but I guess that's not too much of a problem since the use of a fixed point structure will ensure the compiler warns if someone tries to use them assuming the wrong kind of result. Maybe we could standardize the names a bit to help avoid confusion? I'd suggest "op[_round_up]_type1_type2." If both types are the same, we'd still repeat the type for clarity, but maybe we could just use "fixed16" or "fix16" to keep the overall names from getting too long. And for division we'd always keep the dividend as the first type, divisor as second. E.g., mul_round_up_u32_fixed16() div_round_up_u32_fixed16() mul_fixed16_fixed16() div_round_up_fixed16_fixed16() And presumably the existing fixed point operations could be renamed in the same manner. Matt > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 43 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 5804734d30a3..5ff0091707c0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -152,6 +152,38 @@ static inline uint_fixed_16_16_t max_fixed_16_16(uint_fixed_16_16_t max1, > return max; > } > > +static inline uint32_t div_round_up_fixed_16_16(uint_fixed_16_16_t val, > + uint_fixed_16_16_t d) > +{ > + return DIV_ROUND_UP(val.val, d.val); > +} > + > +static inline uint32_t mul_u32_fixed_16_16_round_up(uint32_t val, > + uint_fixed_16_16_t mul) > +{ > + uint64_t intermediate_val; > + uint32_t result; > + > + intermediate_val = (uint64_t) val * mul.val; > + intermediate_val = DIV_ROUND_UP_ULL(intermediate_val, 1 << 16); > + WARN_ON(intermediate_val >> 32); > + result = clamp_t(uint32_t, intermediate_val, 0, ~0); > + return result; > +} > + > +static inline uint_fixed_16_16_t mul_fixed_16_16(uint_fixed_16_16_t val, > + uint_fixed_16_16_t mul) > +{ > + uint64_t intermediate_val; > + uint_fixed_16_16_t fp; > + > + intermediate_val = (uint64_t) val.val * mul.val; > + intermediate_val = intermediate_val >> 16; > + WARN_ON(intermediate_val >> 32); > + fp.val = clamp_t(uint32_t, intermediate_val, 0, ~0); > + return fp; > +} > + > static inline uint_fixed_16_16_t fixed_16_16_div(uint32_t val, uint32_t d) > { > uint_fixed_16_16_t fp, res; > @@ -174,6 +206,17 @@ static inline uint_fixed_16_16_t fixed_16_16_div_u64(uint32_t val, uint32_t d) > return res; > } > > +static inline uint32_t fixed_16_16_div_round_up_u64(uint32_t val, > + uint_fixed_16_16_t d) > +{ > + uint64_t interm_val; > + > + interm_val = (uint64_t)val << 16; > + interm_val = DIV_ROUND_UP_ULL(interm_val, d.val); > + WARN_ON(interm_val >> 32); > + return clamp_t(uint32_t, interm_val, 0, ~0); > +} > + > static inline uint_fixed_16_16_t mul_u32_fixed_16_16(uint32_t val, > uint_fixed_16_16_t mul) > { > -- > 2.11.0 >
Hi, On Friday 12 May 2017 05:52 AM, Matt Roper wrote: > On Mon, May 08, 2017 at 05:18:53PM +0530, Mahesh Kumar wrote: >> This patch adds few wrapper to perform fixed_point_16_16 operations >> mul_u32_fixed_16_16_round_up : Multiplies u32 and fixed_16_16_t >> variables & returns u32 result with >> rounding-off. >> mul_fixed_16_16 : Multiplies two fixed_16_16_t variable & returns >> fixed_16_16 >> div_round_up_fixed_16_16 : Perform division operation on fixed_16_16_t >> variables & return u32 result with round-off >> fixed_16_16_div_round_up_u64 : devide uint32_t variable by fixed_16_16 >> variable and round_up the result to uint32_t. > Minor nitpick, but I'd say "rounding-up" rather than "rounding-off" in > your descriptions here. Will update. > >> These wrappers will be used by later patches in the series. > The name on this last one (fixed_16_16_div_round_up_u64) doesn't seem to > match what it does. You're using u64 internally, but the actual > operands are a u32 and a fixed point value. Wouldn't it make more sense > to call it div_round_up_u32_fixed_16_16 (that also keeps the dividend > first and the divisor second, which seems more natural to me). > > Actually the naming of all the fixed point math functions is a bit > confusing/inconsistent. Some of them are "op_type[_type][_round_up]," > some of them are "op[_round_up]_type," some of them are > "type_op[_round_up]," and some of them are "type_op[_round_up]_type." > Also, none of them specify the return value type, but I guess that's not > too much of a problem since the use of a fixed point structure will > ensure the compiler warns if someone tries to use them assuming the > wrong kind of result. > > Maybe we could standardize the names a bit to help avoid confusion? I'd > suggest "op[_round_up]_type1_type2." If both types are the same, we'd > still repeat the type for clarity, but maybe we could just use "fixed16" > or "fix16" to keep the overall names from getting too long. And for > division we'd always keep the dividend as the first type, divisor as > second. > > E.g., > mul_round_up_u32_fixed16() > div_round_up_u32_fixed16() > mul_fixed16_fixed16() > div_round_up_fixed16_fixed16() will use name "mul_fixed16 / div_round_up_fixed16" (assuming same type for both the operand if only one type is there in function name), hope that is fine with you. > And presumably the existing fixed point operations could be renamed in > the same manner. thanks for suggestion/review, It sounds logical & more consistent, will fix it, In this patch will update only these 4 wrappers, will create a new patch at end of the series to address naming for other wrappers. thanks, -Mahesh > > Matt > >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 43 +++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 43 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 5804734d30a3..5ff0091707c0 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -152,6 +152,38 @@ static inline uint_fixed_16_16_t max_fixed_16_16(uint_fixed_16_16_t max1, >> return max; >> } >> >> +static inline uint32_t div_round_up_fixed_16_16(uint_fixed_16_16_t val, >> + uint_fixed_16_16_t d) >> +{ >> + return DIV_ROUND_UP(val.val, d.val); >> +} >> + >> +static inline uint32_t mul_u32_fixed_16_16_round_up(uint32_t val, >> + uint_fixed_16_16_t mul) >> +{ >> + uint64_t intermediate_val; >> + uint32_t result; >> + >> + intermediate_val = (uint64_t) val * mul.val; >> + intermediate_val = DIV_ROUND_UP_ULL(intermediate_val, 1 << 16); >> + WARN_ON(intermediate_val >> 32); >> + result = clamp_t(uint32_t, intermediate_val, 0, ~0); >> + return result; >> +} >> + >> +static inline uint_fixed_16_16_t mul_fixed_16_16(uint_fixed_16_16_t val, >> + uint_fixed_16_16_t mul) >> +{ >> + uint64_t intermediate_val; >> + uint_fixed_16_16_t fp; >> + >> + intermediate_val = (uint64_t) val.val * mul.val; >> + intermediate_val = intermediate_val >> 16; >> + WARN_ON(intermediate_val >> 32); >> + fp.val = clamp_t(uint32_t, intermediate_val, 0, ~0); >> + return fp; >> +} >> + >> static inline uint_fixed_16_16_t fixed_16_16_div(uint32_t val, uint32_t d) >> { >> uint_fixed_16_16_t fp, res; >> @@ -174,6 +206,17 @@ static inline uint_fixed_16_16_t fixed_16_16_div_u64(uint32_t val, uint32_t d) >> return res; >> } >> >> +static inline uint32_t fixed_16_16_div_round_up_u64(uint32_t val, >> + uint_fixed_16_16_t d) >> +{ >> + uint64_t interm_val; >> + >> + interm_val = (uint64_t)val << 16; >> + interm_val = DIV_ROUND_UP_ULL(interm_val, d.val); >> + WARN_ON(interm_val >> 32); >> + return clamp_t(uint32_t, interm_val, 0, ~0); >> +} >> + >> static inline uint_fixed_16_16_t mul_u32_fixed_16_16(uint32_t val, >> uint_fixed_16_16_t mul) >> { >> -- >> 2.11.0 >>
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5804734d30a3..5ff0091707c0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -152,6 +152,38 @@ static inline uint_fixed_16_16_t max_fixed_16_16(uint_fixed_16_16_t max1, return max; } +static inline uint32_t div_round_up_fixed_16_16(uint_fixed_16_16_t val, + uint_fixed_16_16_t d) +{ + return DIV_ROUND_UP(val.val, d.val); +} + +static inline uint32_t mul_u32_fixed_16_16_round_up(uint32_t val, + uint_fixed_16_16_t mul) +{ + uint64_t intermediate_val; + uint32_t result; + + intermediate_val = (uint64_t) val * mul.val; + intermediate_val = DIV_ROUND_UP_ULL(intermediate_val, 1 << 16); + WARN_ON(intermediate_val >> 32); + result = clamp_t(uint32_t, intermediate_val, 0, ~0); + return result; +} + +static inline uint_fixed_16_16_t mul_fixed_16_16(uint_fixed_16_16_t val, + uint_fixed_16_16_t mul) +{ + uint64_t intermediate_val; + uint_fixed_16_16_t fp; + + intermediate_val = (uint64_t) val.val * mul.val; + intermediate_val = intermediate_val >> 16; + WARN_ON(intermediate_val >> 32); + fp.val = clamp_t(uint32_t, intermediate_val, 0, ~0); + return fp; +} + static inline uint_fixed_16_16_t fixed_16_16_div(uint32_t val, uint32_t d) { uint_fixed_16_16_t fp, res; @@ -174,6 +206,17 @@ static inline uint_fixed_16_16_t fixed_16_16_div_u64(uint32_t val, uint32_t d) return res; } +static inline uint32_t fixed_16_16_div_round_up_u64(uint32_t val, + uint_fixed_16_16_t d) +{ + uint64_t interm_val; + + interm_val = (uint64_t)val << 16; + interm_val = DIV_ROUND_UP_ULL(interm_val, d.val); + WARN_ON(interm_val >> 32); + return clamp_t(uint32_t, interm_val, 0, ~0); +} + static inline uint_fixed_16_16_t mul_u32_fixed_16_16(uint32_t val, uint_fixed_16_16_t mul) {
This patch adds few wrapper to perform fixed_point_16_16 operations mul_u32_fixed_16_16_round_up : Multiplies u32 and fixed_16_16_t variables & returns u32 result with rounding-off. mul_fixed_16_16 : Multiplies two fixed_16_16_t variable & returns fixed_16_16 div_round_up_fixed_16_16 : Perform division operation on fixed_16_16_t variables & return u32 result with round-off fixed_16_16_div_round_up_u64 : devide uint32_t variable by fixed_16_16 variable and round_up the result to uint32_t. These wrappers will be used by later patches in the series. Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 43 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)