Re: [PATCH v4 1/6] drm/format-helper: Add drm_fb_xrgb8888_to_gray8_line()

[Pekka Paalanen]

On Fri, 11 Feb 2022 19:27:12 +0200
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:

> On Fri, Feb 11, 2022 at 06:25:17PM +0200, Jani Nikula wrote:
> > On Fri, 11 Feb 2022, Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:  
> > > On Fri, Feb 11, 2022 at 02:05:56PM +0200, Jani Nikula wrote:  
> > >> On Fri, 11 Feb 2022, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:  
> > >> > Am 11.02.22 um 12:12 schrieb Andy Shevchenko:  
> > >> >> On Fri, Feb 11, 2022 at 11:40:13AM +0100, Javier Martinez Canillas wrote:  
> > >> >>> On 2/11/22 11:28, Andy Shevchenko wrote:  
> > >> >>>> On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas wrote:  
> > >
> > > ...
> > >  
> > >> >>>>> +static void drm_fb_xrgb8888_to_gray8_line(u8 *dst, const u32 *src, unsigned int pixels)
> > >> >>>>> +{
> > >> >>>>> +	unsigned int x;
> > >> >>>>> +
> > >> >>>>> +	for (x = 0; x < pixels; x++) {
> > >> >>>>> +		u8 r = (*src & 0x00ff0000) >> 16;
> > >> >>>>> +		u8 g = (*src & 0x0000ff00) >> 8;
> > >> >>>>> +		u8 b =  *src & 0x000000ff;
> > >> >>>>> +
> > >> >>>>> +		/* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
> > >> >>>>> +		*dst++ = (3 * r + 6 * g + b) / 10;
> > >> >>>>> +		src++;
> > >> >>>>> +	}  
> > >> >>>>
> > >> >>>> Can be done as
> > >> >>>>
> > >> >>>> 	while (pixels--) {
> > >> >>>> 		...
> > >> >>>> 	}
> > >> >>>>
> > >> >>>> or
> > >> >>>>
> > >> >>>> 	do {
> > >> >>>> 		...
> > >> >>>> 	} while (--pixels);
> > >> >>>>  
> > >> >>>
> > >> >>> I don't see why a while loop would be an improvement here TBH.  
> > >> >> 
> > >> >> Less letters to parse when reading the code.  
> > >> >
> > >> > It's a simple refactoring of code that has worked well so far. Let's 
> > >> > leave it as-is for now.  
> > >> 
> > >> IMO *always* prefer a for loop over while or do-while.
> > >> 
> > >> The for (i = 0; i < N; i++) is such a strong paradigm in C. You
> > >> instantly know how many times you're going to loop, at a glance. Not so
> > >> with with the alternatives, which should be used sparingly.  
> > >
> > > while () {}  _is_ a paradigm, for-loop is syntax sugar on top of it.  
> > 
> > And while() is just syntax sugar for goto. :p
> > 
> > The for loop written as for (i = 0; i < N; i++) is hands down the most
> > obvious counting loop pattern there is in C.
> >   
> > >> And yes, the do-while suggested above is buggy, and you actually need to
> > >> stop and think to see why.  
> > >
> > > It depends if pixels can be 0 or not and if it's not, then does it contain last
> > > or number.
> > >
> > > The do {} while (--pixels); might be buggy iff pixels may be 0.  
> > 
> > Yeah. And how long does it take to figure that out?  
> 
> Okay, I made a mistake to drop the explanation. So, I (mistakenly) assumed
> that people know this difference between post-decrement and pre-decrement
> (note, while-loop here is not what is problematic).

That was not the question.

The question was, how long does it take to figure out if pixels can or
cannot be zero?

Code is styled for humans other than the author, not for compilers.

Having to stop to think about the difference between post- and
pre-decrement to figure out when the while-loop runs does take me a few
more brain cycles to understand, even though I know the rules very well.

I would call that brain cycle optimization, and leave the CPU cycle
optimization for the compiler in these cases.


Thanks,
pq

Attachment: pgpV5qDqECMmM.pgp
Description: OpenPGP digital signature