Message-ID: <9e1cf069-43ba-4f49-8d2d-1cc9c16bd65e@ti.com>
Date: Sat, 20 Jul 2024 17:06:38 +0530
From: Devarsh Thakkar <devarsht@...com>
To: Dave Martin <Dave.Martin@....com>
CC: <mchehab@...nel.org>, <hverkuil-cisco@...all.nl>,
        <linux-media@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <sebastian.fricke@...labora.com>, <andriy.shevchenko@...ux.intel.com>,
        <jani.nikula@...el.com>, <jirislaby@...nel.org>, <corbet@....net>,
        <broonie@...nel.org>, <rdunlap@...radead.org>,
        <linux-doc@...r.kernel.org>, <laurent.pinchart@...asonboard.com>,
        <praneeth@...com>, <nm@...com>, <vigneshr@...com>, <a-bhatia1@...com>,
        <j-luthra@...com>, <b-brnich@...com>, <detheridge@...com>,
        <p-mantena@...com>, <vijayp@...com>, <andi.shyti@...ux.intel.com>,
        <nicolas@...fresne.ca>, <davidgow@...gle.com>, <dlatypov@...gle.com>
Subject: Re: [PATCH 1/6] math.h: Add macros for rounding to closest value

Hi Dave,

Thanks for the review.

On 09/07/24 22:48, Dave Martin wrote:
> On Mon, Jul 08, 2024 at 09:29:38PM +0530, Devarsh Thakkar wrote:
>> Add below rounding related macros:
>>
>> round_closest_up(x, y) : Rounds x to closest multiple of y where y is a
>> power of 2, with a preference to round up in case two nearest values are
>> possible.
>>
>> round_closest_down(x, y) : Rounds x to closest multiple of y where y is a
>> power of 2, with a preference to round down in case two nearest values are
>> possible.
>>
>> roundclosest(x, y) : Rounds x to closest multiple of y, this macro should
>> generally be used only when y is not multiple of 2 as otherwise
>> round_closest* macros should be used which are much faster.
>>
>> Examples:
>>  * round_closest_up(17, 4) = 16
>>  * round_closest_up(15, 4) = 16
>>  * round_closest_up(14, 4) = 16
>>  * round_closest_down(17, 4) = 16
>>  * round_closest_down(15, 4) = 16
>>  * round_closest_down(14, 4) = 12
>>  * roundclosest(21, 5) = 20
>>  * roundclosest(19, 5) = 20
>>  * roundclosest(17, 5) = 15
>>
>> Signed-off-by: Devarsh Thakkar <devarsht@...com>
>> Acked-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
>> ---
>> NOTE: This patch is inspired from the Mentor Graphics IPU driver [1]
>> which uses similar macro locally and which is updated in further patch
>> in the series to use this generic macro instead along with other drivers
>> having similar requirements.
>>
>> Link: https://elixir.bootlin.com/linux/v6.8.9/source/drivers/gpu/ipu-v3/ipu-image-convert.c#L480 [1]
>> ---
>>  include/linux/math.h | 63 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 63 insertions(+)
>>
>> diff --git a/include/linux/math.h b/include/linux/math.h
>> index dd4152711de7..79e3dfda77fc 100644
>> --- a/include/linux/math.h
>> +++ b/include/linux/math.h
>> @@ -34,6 +34,52 @@
>>   */
>>  #define round_down(x, y) ((x) & ~__round_mask(x, y))
>>  
>> +/**
>> + * round_closest_up - round closest to be multiple of specified value (which is
>> + *                    power of 2) with preference to rounding up
>> + * @x: the value to round
>> + * @y: multiple to round closest to (must be a power of 2)
>> + *
>> + * Rounds @x to closest multiple of @y (which must be a power of 2).
>> + * The value can be either rounded up or rounded down depending upon rounded
>> + * value's closeness to the specified value. If there are two closest possible
>> + * values, i.e. the difference between the specified value and it's rounded up
>> + * and rounded down values is same then preference is given to rounded up
>> + * value.
>> + *
>> + * To perform arbitrary rounding to closest value (not multiple of 2), use
>> + * roundclosest().
>> + *
>> + * Examples:
>> + * * round_closest_up(17, 4) = 16
>> + * * round_closest_up(15, 4) = 16
>> + * * round_closest_up(14, 4) = 16
>> + */
>> +#define round_closest_up(x, y) round_down((x) + (y) / 2, (y))
>> +
>> +/**
>> + * round_closest_down - round closest to be multiple of specified value (which
>> + *			is power of 2) with preference to rounding down
>> + * @x: the value to round
>> + * @y: multiple to round closest to (must be a power of 2)
>> + *
>> + * Rounds @x to closest multiple of @y (which must be a power of 2).
>> + * The value can be either rounded up or rounded down depending upon rounded
>> + * value's closeness to the specified value. If there are two closest possible
>> + * values, i.e. the difference between the specified value and it's rounded up
>> + * and rounded down values is same then preference is given to rounded up
>> + * value.
>> + *
>> + * To perform arbitrary rounding to closest value (not multiple of 2), use
>> + * roundclosest().
>> + *
>> + * Examples:
>> + * * round_closest_down(17, 4) = 16
>> + * * round_closest_down(15, 4) = 16
>> + * * round_closest_down(14, 4) = 12
>> + */
>> +#define round_closest_down(x, y) round_up((x) - (y) / 2, (y))
>> +
> 
> Naming aside, is there an actual use case for having both roundclosest()
> and round_closest_up() today?
> 

Both the macros are different, roundclosest is for arbitrary rounding
(not multiple of 2) where round_closest_up/down are optimized for
rounding to values which are powers of 2. So where there is a surety
that rounding value would be power of 2, round_closest* macros are
recommended. Regarding the use-cases, there are drivers already using
such type of macros locally [1] and new drivers such as [2] required it,
so we aligned to have generic macros for all rounding to nearest value
scenarios (this patch was earlier part of another series with 7
revisions, see the discussions here [2]).


> (i.e., is there any potential caller that would actually care about the
> rounding direction for borderline cases?)

I think a transparent scheme is better where caller should be aware of
rounding direction for borderline cases too so that it gets predictable
values w.r.t what it requested for rather than leaving it ambiguous. For
e.g. in this patchset [3], it suited more to use round_closest_down
instead of round_closest_up keeping in mind hw constraints and use-case
requirements, but same might not be true for other drivers. Same was
aligned in earlier patch submissions too [2].


> 
>>  #define DIV_ROUND_UP __KERNEL_DIV_ROUND_UP
>>  
>>  #define DIV_ROUND_DOWN_ULL(ll, d) \
>> @@ -77,6 +123,23 @@
>>  }							\
>>  )
>>  
>> +/**
>> + * roundclosest - round to nearest multiple
>> + * @x: the value to round
>> + * @y: multiple to round nearest to
>> + *
>> + * Rounds @x to nearest multiple of @y.
>> + * The rounded value can be greater than or less than @x depending
>> + * upon it's nearness to @x. If @y will always be a power of 2, consider
>> + * using the faster round_closest_up() or round_closest_down().
>> + *
>> + * Examples:
>> + * * roundclosest(21, 5) = 20
>> + * * roundclosest(19, 5) = 20
>> + * * roundclosest(17, 5) = 15
>> + */
>> +#define roundclosest(x, y) rounddown((x) + (y) / 2, (y))
> 
> Won't this go wrong if (x) + (y) / 2 overflows?  This may happen even in
> some cases where the correctly rounded value would be in range.
> 

Yes I think it is possible, it actually depends upon the datatype of x.
But anyways, I could make it as below which would yield the same result
as arguments are non-multiple of 2:

#define roundclosest(x, y) roundup((x) - (y) / 2, (y))


> The existing rounddown() already leaves something to be desired IIUC: if
> given a negative dividend, it looks like it actually rounds up, at least
> on some arches.  But maybe people don't use it that way very often.
> Perhaps I'm missing something.

I am not sure about above.

[1]:
https://elixir.bootlin.com/linux/v6.10/source/drivers/gpu/ipu-v3/ipu-image-convert.c#L480
https://elixir.bootlin.com/linux/v6.10/source/drivers/staging/media/ipu3/ipu3-css-params.c#L443
https://lore.kernel.org/all/ZlTt-YWzyRyhmT9n@smile.fi.intel.com/

[2]:
https://lore.kernel.org/all/7e3ad816-6a2a-4e02-9b41-03a8562812ad@ti.com/#r
https://lore.kernel.org/all/ZkISG6p1tn9Do-xY@smile.fi.intel.com/#r
https://lore.kernel.org/all/ZlTt-YWzyRyhmT9n@smile.fi.intel.com/
https://lore.kernel.org/all/ZmHDWeuezCEgj20m@smile.fi.intel.com/
https://lore.kernel.org/all/ZloMFfGKLry6EWNL@smile.fi.intel.com/

[3]:
https://lore.kernel.org/all/20240708155943.2314427-7-devarsht@ti.com/

Regards
Devarsh

Powered by blists - more mailing lists