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@xxxxxx/ > [3]: https://elixir.bootlin.com/linux/latest/source/include/linux/math.h#L86 -- With Best Regards, Andy Shevchenko
