Hi Tuukka, Andy, others,
On Wed, Oct 11, 2017 at 05:27:27PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 11, 2017 at 5:01 PM, Tuukka Toivonen
> <tuukka.toivonen@xxxxxxxxx> wrote:
> > On Wed, 2017-10-11 at 16:31 +0300, Andy Shevchenko wrote:
> >> On Wed, Oct 11, 2017 at 10:29 AM, sakari.ailus@xxxxxxxxxxxxxxx
> >> <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> >> > On Wed, Oct 11, 2017 at 04:14:37AM +0000, Zhi, Yong wrote:
>
> >> > > > > +static unsigned int ipu3_css_scaler_get_exp(unsigned int
> >> > > > > counter,
> >> > > > > + unsigned int
> >> > > > > divider) {
> >> > > > > + unsigned int i = 0;
> >> > > > > +
> >> > > > > + while (counter <= divider / 2) {
> >> > > > > + divider /= 2;
> >> > > > > + i++;
> >> > > > > + }
> >> > > > > +
> >> > > > > + return i;
>
> >> Roughly like
> >>
> >> if (!counter || divider < counter)
> >> return 0;
> >> return order_base_2(divider) - order_base_2(counter);
> >
> > The original loop is typical ran just couple of times, so I think
> > that fls or division are probably slower than the original loop.
> > Furthermore, these "optimizations" are also harder to read, so in
> > my opinion there's no advantage in using them.
>
> Honestly I'm opposing that.
> It took me about minute to be clear what is going on on that loop
> while fls() / ffs() / ilog2() like stuff can be read fast.
>
> Like
>
> int shift = order_base_2(divider) - order_base_2(counter);
>
> return shift > 0 ? shift : 0;
>
> And frankly I don't care about under the hoods of order_base_2(). I
> care about this certain piece of code to be simpler.
>
> One may put a comment line:
>
> # Get log2 of how divider bigger than counter
>
> And thinking more while writing this message the use of order_base_2()
> actually explains what's going on here.
I guess this isn't really worth spending much time on; either way, please
at least fix the current implementation so it won't end up in an infinite
loop. This happens now if counter is zero.
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx
