Re: [PATCH 02/11] drm/i915: Add more wrapper for fixed_point_16_16 operations

[Matt Roper <matthew.d.roper@intel.com>]

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
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx