| Message ID | 20240509183952.4064331-1-devarsht@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7,1/8] media: dt-bindings: Add Imagination E5010 JPEG Encoder | expand |
On Fri, May 10, 2024 at 12:09:52AM +0530, Devarsh Thakkar wrote: > Add macros to round to nearest specified power of 2. This is not what they are doing. For the above we already have macros defined. > Two macros are added : (Yes, after I wrapped to comment this line looks better on its own, so whatever will be the first sentence, this line should be separated from.) > round_closest_up and round_closest_down which round up to nearest multiple round_closest_up() and round_closest_down() > of 2 with a preference to round up or round down respectively if there are > two possible nearest values to the given number. You should reformulate, because AFAICS there is the crucial difference from these and existing round_*_pow_of_two(). > This patch is inspired from the Mentor Graphics IPU driver [1] which uses > similar macro locally and which can be updated to use this generic macro > instead along with other drivers having similar requirements. > > [1]: > https://elixir.bootlin.com/linux/v6.8.9/source/drivers/gpu/ipu-v3/ipu-image-convert.c#L480 Instead of this, just add a patch to convert that driver to use this new macro. Besides, this paragraph should go to the comment/changelog area below. > Signed-off-by: Devarsh Thakkar <devarsht@ti.com> > --- > V1->V6 (No change, patch introduced in V7) > ---
On Fri, 10 May 2024, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Fri, May 10, 2024 at 12:09:52AM +0530, Devarsh Thakkar wrote: >> Add macros to round to nearest specified power of 2. > > This is not what they are doing. For the above we already have macros defined. > >> Two macros are added : > > (Yes, after I wrapped to comment this line looks better on its own, > so whatever will be the first sentence, this line should be separated > from.) > >> round_closest_up and round_closest_down which round up to nearest multiple > > round_closest_up() and round_closest_down() > > >> of 2 with a preference to round up or round down respectively if there are >> two possible nearest values to the given number. > > You should reformulate, because AFAICS there is the crucial difference > from these and existing round_*_pow_of_two(). Moreover, I think the naming of round_up() and round_down() should have reflected the fact that they operate on powers of 2. It's unfortunate that the difference to roundup() and rounddown() is just the underscore! That's just a trap. So let's perhaps not repeat the same with round_closest_up() and round_closest_down()? BR, Jani. > >> This patch is inspired from the Mentor Graphics IPU driver [1] which uses >> similar macro locally and which can be updated to use this generic macro >> instead along with other drivers having similar requirements. >> >> [1]: >> https://elixir.bootlin.com/linux/v6.8.9/source/drivers/gpu/ipu-v3/ipu-image-convert.c#L480 > > Instead of this, just add a patch to convert that driver to use this new macro. > Besides, this paragraph should go to the comment/changelog area below. > >> Signed-off-by: Devarsh Thakkar <devarsht@ti.com> >> --- >> V1->V6 (No change, patch introduced in V7) >> ---
On Fri, May 10, 2024 at 06:15:34PM +0300, Jani Nikula wrote: > On Fri, 10 May 2024, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, May 10, 2024 at 12:09:52AM +0530, Devarsh Thakkar wrote: > >> Add macros to round to nearest specified power of 2. > > > > This is not what they are doing. For the above we already have macros defined. > > > >> Two macros are added : > > > > (Yes, after I wrapped to comment this line looks better on its own, > > so whatever will be the first sentence, this line should be separated > > from.) > > > >> round_closest_up and round_closest_down which round up to nearest multiple > > > > round_closest_up() and round_closest_down() > > > > > >> of 2 with a preference to round up or round down respectively if there are > >> two possible nearest values to the given number. > > > > You should reformulate, because AFAICS there is the crucial difference > > from these and existing round_*_pow_of_two(). > > Moreover, I think the naming of round_up() and round_down() should have > reflected the fact that they operate on powers of 2. It's unfortunate > that the difference to roundup() and rounddown() is just the underscore! > That's just a trap. FYI: https://stackoverflow.com/questions/58139219/difference-of-align-and-round-up-macro-in-the-linux-kernel > So let's perhaps not repeat the same with round_closest_up() and > round_closest_down()?
Hi Andy, Thanks for the quick review. On 10/05/24 20:31, Andy Shevchenko wrote: > On Fri, May 10, 2024 at 12:09:52AM +0530, Devarsh Thakkar wrote: >> Add macros to round to nearest specified power of 2. > > This is not what they are doing. For the above we already have macros defined. > Sorry I did not understand this comment, if you are talking about existing macros round_up & round_down they either round "up" and round "down" to specified power of 2 as specified here [1]. whereas the macros introduced in this patch round to "nearest" specified power of 2. >> Two macros are added : > > (Yes, after I wrapped to comment this line looks better on its own, > so whatever will be the first sentence, this line should be separated > from.) > Agreed. >> round_closest_up and round_closest_down which round up to nearest multiple > > round_closest_up() and round_closest_down() > > >> of 2 with a preference to round up or round down respectively if there are >> two possible nearest values to the given number. > > You should reformulate, because AFAICS there is the crucial difference > from these and existing round_*_pow_of_two(). > In math.h, we already have round_up/round_down macros which rounded up/down to next specified power of 2. Then we had the DIV_ROUND_CLOSEST which used the suffix _CLOSEST to imply the meaning that divided value will be rounded to nearest/closest int value either by rounding up or rounding down. So inspired from naming convention of this macros given developers are already familiar with them, I used round_closest for specifying the rounded value to nearest/closest value which can be achieved either by rounding up/down. And I also wanted user to have finer control for the scenarios where there are two possible nearest values : For e.g round(160, 64) can be either 192 and 128 and both are 32 away from 160. And that's the reason I went ahead with two macros instead i.e round_closest_up, round_closest_down so that developers can choose exactly what they want. Regards Devarsh
Hi Jani, Thanks for the quick review. On 10/05/24 20:45, Jani Nikula wrote: [...] > Moreover, I think the naming of round_up() and round_down() should have > reflected the fact that they operate on powers of 2. It's unfortunate > that the difference to roundup() and rounddown() is just the underscore! > That's just a trap. > > So let's perhaps not repeat the same with round_closest_up() and > round_closest_down()? > Yes the naming is inspired by existing macros i.e. round_up, round_down (which round up/down to next pow of 2) and DIV_ROUND_CLOSEST (which round the divided value to closest value) and there are already a lot of users for these API's : linux-next git:(heads/next-20240509) ✗ grep -nr round_up drivers | wc 730 4261 74775 linux-next git:(heads/next-20240509) ✗ grep -nr round_down drivers | wc 226 1293 22194 linux-next git:(heads/next-20240509) ✗ grep -nr DIV_ROUND_CLOSEST drivers | wc 1207 7461 111822 so I thought to align with existing naming convention assuming developers are already familiar with this. But if a wider consensus is to go with a newer naming convention then I am open to it, although a challenge there would be to keep it short. For e.g. this one is already 3 words, if we go with more explicit "round_closest_up_pow_2" it looks quite long in my opinion :) . Regards Devarsh
I think roundup(x, 1 << n) is more readable.
On Sun, May 12, 2024 at 07:46:58AM +0300, Alexey Dobriyan wrote: > I think > > roundup(x, 1 << n) Since it's about power-of-two, round_up() is better. > is more readable.
On Sat, May 11, 2024 at 11:11:14PM +0530, Devarsh Thakkar wrote: > On 10/05/24 20:45, Jani Nikula wrote: [...] > > Moreover, I think the naming of round_up() and round_down() should have > > reflected the fact that they operate on powers of 2. It's unfortunate > > that the difference to roundup() and rounddown() is just the underscore! > > That's just a trap. > > > > So let's perhaps not repeat the same with round_closest_up() and > > round_closest_down()? > > Yes the naming is inspired by existing macros i.e. round_up, round_down > (which round up/down to next pow of 2) and DIV_ROUND_CLOSEST (which > round the divided value to closest value) and there are already a lot of > users for these API's : > > linux-next git:(heads/next-20240509) ✗ grep -nr round_up drivers | wc > 730 4261 74775 > > linux-next git:(heads/next-20240509) ✗ grep -nr round_down drivers | wc > 226 1293 22194 > > linux-next git:(heads/next-20240509) ✗ grep -nr DIV_ROUND_CLOSEST > drivers | wc > 1207 7461 111822 Side note, discover `git grep ...`: it's much much faster on Git index, than classic one on a working copy. > so I thought to align with existing naming convention assuming > developers are already familiar with this. > > But if a wider consensus is to go with a newer naming convention then I > am open to it, although a challenge there would be to keep it short. For > e.g. this one is already 3 words, if we go with more explicit > "round_closest_up_pow_2" it looks quite long in my opinion :) . You need properly name the macros. Again, round_up() / roundup() and roundup_pow_of_two() are three _different_ macros, and it's not clear why you can't use one of them in your case. The patch that changes those to a new one are doubtful to begin with. I.e. need a careful review on the arithmetics side of the change including HW capabilities of handling "closest" results.
Hi Andy, On 13/05/24 14:29, Andy Shevchenko wrote: > On Sat, May 11, 2024 at 11:11:14PM +0530, Devarsh Thakkar wrote: >> On 10/05/24 20:45, Jani Nikula wrote: > > [...] > >>> Moreover, I think the naming of round_up() and round_down() should have >>> reflected the fact that they operate on powers of 2. It's unfortunate >>> that the difference to roundup() and rounddown() is just the underscore! >>> That's just a trap. >>> >>> So let's perhaps not repeat the same with round_closest_up() and >>> round_closest_down()? >> >> Yes the naming is inspired by existing macros i.e. round_up, round_down >> (which round up/down to next pow of 2) and DIV_ROUND_CLOSEST (which >> round the divided value to closest value) and there are already a lot of >> users for these API's : >> >> linux-next git:(heads/next-20240509) ✗ grep -nr round_up drivers | wc >> 730 4261 74775 >> >> linux-next git:(heads/next-20240509) ✗ grep -nr round_down drivers | wc >> 226 1293 22194 >> >> linux-next git:(heads/next-20240509) ✗ grep -nr DIV_ROUND_CLOSEST >> drivers | wc >> 1207 7461 111822 > > Side note, discover `git grep ...`: it's much much faster on Git index, > than classic one on a working copy. > >> so I thought to align with existing naming convention assuming >> developers are already familiar with this. >> >> But if a wider consensus is to go with a newer naming convention then I >> am open to it, although a challenge there would be to keep it short. For >> e.g. this one is already 3 words, if we go with more explicit >> "round_closest_up_pow_2" it looks quite long in my opinion :) . > > You need properly name the macros. Again, round_up() / roundup() and > roundup_pow_of_two() are three _different_ macros, and it's not clear > why you can't use one of them in your case. > I can't use any of these because these macros either round up or round down, whereas I want to round to closest value for the argument specified by the user, be it achieved either by rounding up or rounding down depending upon whichever makes the answer closer to the user-specified argument. To make it clear, I have already included the examples in the macro description [2], copying it here, maybe I can put the same examples in the commit message too to avoid confusions : 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 Coming back to naming, this is as per existing convention used for naming round_up, round_down (notice the `_` being used for macros working with pow of 2) and DIV_ROUND_CLOSEST (notice the work `closest` used to specify the answer to be nearest to specified value). Naming is a bit subjective, but I personally don't think it is a good idea to go away with the existing naming convention or go with longer names. > The patch that changes those to a new one are doubtful to begin with. > I.e. need a careful review on the arithmetics side of the change > including HW capabilities of handling "closest" results. > This is already tested from my side, in-fact I have posted some of the results in cover-letter with these macros [1] : Regarding hardware capabilities, it uses existing round_up, round_down macros underneath which are optimized to handle pow of 2 after modifying the user provided argument using addition/subtraction and division, so I don't think it should generally a problem with the hardware. And I see other macros DIV_ROUND_CLOSEST [3] already using similar operations i.e. addition/subtraction and division so don't think it should be a problem to keep similar other macros in the same file. [1]: https://gist.github.com/devarsht/de6f5142f678bb1a5338abfd9f814abd#file-v7_jpeg_encoder_crop_validation-L204 [2]: https://lore.kernel.org/all/20240509183952.4064331-1-devarsht@ti.com/ [3]: https://elixir.bootlin.com/linux/latest/source/include/linux/math.h#L86 Regards Devarsh
On Mon, May 13, 2024 at 04:55:58PM +0530, Devarsh Thakkar wrote: > On 13/05/24 14:29, Andy Shevchenko wrote: > > On Sat, May 11, 2024 at 11:11:14PM +0530, Devarsh Thakkar wrote: > >> On 10/05/24 20:45, Jani Nikula wrote: [...] > >>> Moreover, I think the naming of round_up() and round_down() should have > >>> reflected the fact that they operate on powers of 2. It's unfortunate > >>> that the difference to roundup() and rounddown() is just the underscore! > >>> That's just a trap. > >>> > >>> So let's perhaps not repeat the same with round_closest_up() and > >>> round_closest_down()? > >> > >> Yes the naming is inspired by existing macros i.e. round_up, round_down > >> (which round up/down to next pow of 2) and DIV_ROUND_CLOSEST (which > >> round the divided value to closest value) and there are already a lot of > >> users for these API's : > >> > >> linux-next git:(heads/next-20240509) ✗ grep -nr round_up drivers | wc > >> 730 4261 74775 > >> > >> linux-next git:(heads/next-20240509) ✗ grep -nr round_down drivers | wc > >> 226 1293 22194 > >> > >> linux-next git:(heads/next-20240509) ✗ grep -nr DIV_ROUND_CLOSEST > >> drivers | wc > >> 1207 7461 111822 > > > > Side note, discover `git grep ...`: it's much much faster on Git index, > > than classic one on a working copy. > > > >> so I thought to align with existing naming convention assuming > >> developers are already familiar with this. > >> > >> But if a wider consensus is to go with a newer naming convention then I > >> am open to it, although a challenge there would be to keep it short. For > >> e.g. this one is already 3 words, if we go with more explicit > >> "round_closest_up_pow_2" it looks quite long in my opinion :) . > > > > You need properly name the macros. Again, round_up() / roundup() and > > roundup_pow_of_two() are three _different_ macros, and it's not clear > > why you can't use one of them in your case. > > I can't use any of these because these macros either round up or round down, > whereas I want to round to closest value for the argument specified by the > user, be it achieved either by rounding up or rounding down depending upon > whichever makes the answer closer to the user-specified argument. > > To make it clear, I have already included the examples in the macro > description [2], copying it here, maybe I can put the same examples in the > commit message too to avoid confusions : > > 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 > > Coming back to naming, this is as per existing convention used for naming > round_up, round_down (notice the `_` being used for macros working with pow of > 2) and DIV_ROUND_CLOSEST (notice the work `closest` used to specify the answer > to be nearest to specified value). Naming is a bit subjective, but I > personally don't think it is a good idea to go away with the existing naming > convention or go with longer names. > > > The patch that changes those to a new one are doubtful to begin with. > > I.e. need a careful review on the arithmetics side of the change > > including HW capabilities of handling "closest" results. > > This is already tested from my side, in-fact I have posted some of the results > in cover-letter with these macros [1] : > > Regarding hardware capabilities, it uses existing round_up, round_down macros > underneath which are optimized to handle pow of 2 after modifying the user > provided argument using addition/subtraction and division, so I don't think it > should generally a problem with the hardware. > And I see other macros DIV_ROUND_CLOSEST [3] already using similar operations > i.e. addition/subtraction and division so don't think it should be a problem > to keep similar other macros in the same file. Okay, thank you for elaborating. So, what I would expect in the new version of the series is that: - align naming (with the existing round*() macros) - add examples into commit message of the math.h patch - add test cases (you need to create lib/math_kunit.c for that) - elaborate in the commit message of the IPU3 change what you posted above (some kind of a summary) > [1]: > https://gist.github.com/devarsht/de6f5142f678bb1a5338abfd9f814abd#file-v7_jpeg_encoder_crop_validation-L204 > [2]: https://lore.kernel.org/all/20240509183952.4064331-1-devarsht@ti.com/ > [3]: https://elixir.bootlin.com/linux/latest/source/include/linux/math.h#L86
Hi Andy, On 13/05/24 17:55, Andy Shevchenko wrote: > On Mon, May 13, 2024 at 04:55:58PM +0530, Devarsh Thakkar wrote: >> On 13/05/24 14:29, Andy Shevchenko wrote: >>> On Sat, May 11, 2024 at 11:11:14PM +0530, Devarsh Thakkar wrote: >>>> On 10/05/24 20:45, Jani Nikula wrote: > > [...] > - align naming (with the existing round*() macros) I think round_closest_up/round_closest_down align already and inspired by the existing naming convention used for round*() and DIV_ROUND_CLOSEST() macros in math.h as explained below (copied from my previous reply [1]) "Coming back to naming, this is as per existing convention used for naming round_up, round_down (notice the `_` being used for macros working with pow of 2) and DIV_ROUND_CLOSEST (notice the work `closest` used to specify the answer to be nearest to specified value)" But do let me know if you have any other suggestions for naming? > - add examples into commit message of the math.h patch Agreed > - add test cases (you need to create lib/math_kunit.c for that) Agreed. > - elaborate in the commit message of the IPU3 change what you posted above > (some kind of a summary) Agreed. [1]: https://lore.kernel.org/all/ZkIG0-01pz632l4R@smile.fi.intel.com/ Regards Devarsh
On Mon, May 13, 2024 at 06:34:19PM +0530, Devarsh Thakkar wrote: > On 13/05/24 17:55, Andy Shevchenko wrote: > > On Mon, May 13, 2024 at 04:55:58PM +0530, Devarsh Thakkar wrote: > >> On 13/05/24 14:29, Andy Shevchenko wrote: > >>> On Sat, May 11, 2024 at 11:11:14PM +0530, Devarsh Thakkar wrote: > >>>> On 10/05/24 20:45, Jani Nikula wrote: [...] > > - align naming (with the existing round*() macros) > > I think round_closest_up/round_closest_down align already and inspired by the > existing naming convention used for round*() and DIV_ROUND_CLOSEST() macros in > math.h as explained below (copied from my previous reply [1]) > > "Coming back to naming, this is as per existing convention used for naming > round_up, round_down (notice the `_` being used for macros working with pow of > 2) and DIV_ROUND_CLOSEST (notice the work `closest` used to specify the answer > to be nearest to specified value)" > > But do let me know if you have any other suggestions for naming? Just make sure that semantically the naming is aligned, that's it. If you think it's already done that way, fine!
diff --git a/include/linux/math.h b/include/linux/math.h index dd4152711de7..82ee3265c910 100644 --- a/include/linux/math.h +++ b/include/linux/math.h @@ -34,6 +34,42 @@ */ #define round_down(x, y) ((x) & ~__round_mask(x, y)) +/** + * round_closest_up - round to nearest specified power of 2 with preference + * to rounding up + * @x: the value to round + * @y: multiple to round to (must be a power of 2) + * + * Rounds @x to nearest multiple of @y (which must be a power of 2). + * The rounded value can be greater than or less than @x depending + * upon it's nearness to @x. If there are two nearest possible values, + * then preference will be given to the greater value. + * + * 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 to nearest specified power of 2 with preference + * to rounding down + * @x: the value to round + * @y: multiple to round down to (must be a power of 2) + * + * Rounds @x to nearest multiple of @y (which must be a power of 2). + * The rounded value can be greater than or less than @x depending + * upon it's nearness to @x. If there are two nearest possible values, + * then preference will be given to the lesser value. + * + * 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)) + #define DIV_ROUND_UP __KERNEL_DIV_ROUND_UP #define DIV_ROUND_DOWN_ULL(ll, d) \
Add macros to round to nearest specified power of 2. Two macros are added : round_closest_up and round_closest_down which round up to nearest multiple of 2 with a preference to round up or round down respectively if there are two possible nearest values to the given number. This patch is inspired from the Mentor Graphics IPU driver [1] which uses similar macro locally and which can be updated to use this generic macro instead along with other drivers having similar requirements. [1]: https://elixir.bootlin.com/linux/v6.8.9/source/drivers/gpu/ipu-v3/ipu-image-convert.c#L480 Signed-off-by: Devarsh Thakkar <devarsht@ti.com> --- V1->V6 (No change, patch introduced in V7) --- include/linux/math.h | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)