| Message ID | 20220211091927.2988283-2-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm: Add driver for Solomon SSD130x OLED displays | expand |
Am 11.02.22 um 10:19 schrieb Javier Martinez Canillas: > Pull the per-line conversion logic into a separate helper function. > > This will allow to do line-by-line conversion in other helpers that > convert to a gray8 format. > > Suggested-by: Thomas Zimmermann <tzimmermann@suse.de> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > > (no changes since v3) > > Changes in v3: > - Add a drm_fb_xrgb8888_to_gray8_line() helper function (Thomas Zimmermann) > > drivers/gpu/drm/drm_format_helper.c | 31 ++++++++++++++++++----------- > 1 file changed, 19 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c > index 0f28dd2bdd72..b981712623d3 100644 > --- a/drivers/gpu/drm/drm_format_helper.c > +++ b/drivers/gpu/drm/drm_format_helper.c > @@ -464,6 +464,21 @@ void drm_fb_xrgb8888_to_xrgb2101010_toio(void __iomem *dst, > } > EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010_toio); > > +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++; > + } > +} > + > /** > * drm_fb_xrgb8888_to_gray8 - Convert XRGB8888 to grayscale > * @dst: 8-bit grayscale destination buffer > @@ -484,8 +499,9 @@ EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010_toio); > void drm_fb_xrgb8888_to_gray8(void *dst, unsigned int dst_pitch, const void *vaddr, > const struct drm_framebuffer *fb, const struct drm_rect *clip) > { > - unsigned int len = (clip->x2 - clip->x1) * sizeof(u32); > - unsigned int x, y; > + unsigned int linepixels = clip->x2 - clip->x1; > + unsigned int len = linepixels * sizeof(u32); > + unsigned int y; > void *buf; > u8 *dst8; > u32 *src32; > @@ -508,16 +524,7 @@ void drm_fb_xrgb8888_to_gray8(void *dst, unsigned int dst_pitch, const void *vad > for (y = clip->y1; y < clip->y2; y++) { > dst8 = dst; > src32 = memcpy(buf, vaddr, len); > - for (x = clip->x1; x < clip->x2; x++) { > - u8 r = (*src32 & 0x00ff0000) >> 16; > - u8 g = (*src32 & 0x0000ff00) >> 8; > - u8 b = *src32 & 0x000000ff; > - > - /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */ > - *dst8++ = (3 * r + 6 * g + b) / 10; > - src32++; > - } > - > + drm_fb_xrgb8888_to_gray8_line(dst8, src32, linepixels); > vaddr += fb->pitches[0]; > dst += dst_pitch; > }
On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas wrote: > Pull the per-line conversion logic into a separate helper function. > > This will allow to do line-by-line conversion in other helpers that > convert to a gray8 format. ... > +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); > +}
Hello Andy, On 2/11/22 11:28, Andy Shevchenko wrote: > On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas wrote: >> Pull the per-line conversion logic into a separate helper function. >> >> This will allow to do line-by-line conversion in other helpers that >> convert to a gray8 format. > > ... > >> +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. In any case, I just pulled the line conversion logic as a separate function with minimal code changes since doing that should be in a separate patch. Feel free to post a patch if you want to change that while loop. Best regards,
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. > In any case, I just pulled the line conversion logic as a separate > function with minimal code changes since doing that should be in a > separate patch. > Feel free to post a patch if you want to change that while loop. Perhaps some day :-)
Hi 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. Best regards Thomas > >> In any case, I just pulled the line conversion logic as a separate >> function with minimal code changes since doing that should be in a >> separate patch. > > >> Feel free to post a patch if you want to change that while loop. > > Perhaps some day :-) >
On Fri, 11 Feb 2022, Thomas Zimmermann <tzimmermann@suse.de> wrote: > Hi > > 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. And yes, the do-while suggested above is buggy, and you actually need to stop and think to see why. BR, Jani. > > Best regards > Thomas > >> >>> In any case, I just pulled the line conversion logic as a separate >>> function with minimal code changes since doing that should be in a >>> separate patch. >> >> >>> Feel free to post a patch if you want to change that while loop. >> >> Perhaps some day :-) >>
Hello Jani, On 2/11/22 13:05, Jani Nikula wrote: [snip] >>>> 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. > > And yes, the do-while suggested above is buggy, and you actually need to > stop and think to see why. > Absolutely agree. These format conversion helpers are not trivial to read and understand (at least for me). In my opinion the code should be written in a way that ease readability and make as robust and less error prone as possible. > > BR, > Jani. Best regards,
Hi Jani, On Fri, Feb 11, 2022 at 1:06 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Fri, 11 Feb 2022, Thomas Zimmermann <tzimmermann@suse.de> 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. (guess what ;-) I tend to disagree. > 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. In this case it's fairly obvious, and you get rid of the extra variable x. Less code, less variables, what can go wrong? ;-) > And yes, the do-while suggested above is buggy, and you actually need to > stop and think to see why. Yes, that one is wrong. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Fri, Feb 11, 2022 at 02:05:56PM +0200, Jani Nikula wrote: > On Fri, 11 Feb 2022, Thomas Zimmermann <tzimmermann@suse.de> 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 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.
On Fri, 11 Feb 2022, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Fri, Feb 11, 2022 at 02:05:56PM +0200, Jani Nikula wrote: >> On Fri, 11 Feb 2022, Thomas Zimmermann <tzimmermann@suse.de> 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? BR, Jani.
On Fri, Feb 11, 2022 at 06:25:17PM +0200, Jani Nikula wrote: > On Fri, 11 Feb 2022, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Feb 11, 2022 at 02:05:56PM +0200, Jani Nikula wrote: > >> On Fri, 11 Feb 2022, Thomas Zimmermann <tzimmermann@suse.de> 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).
Hi Am 11.02.22 um 16:41 schrieb Andy Shevchenko: [...] >> 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. Naw, that's not true. An idiomatic for loop, such as for (i = ...; i < N; ++i), is such a strong pattern that it's way better than the corresponding while loop. Best regards Thomas > >> 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. >
On Fri, 11 Feb 2022 19:27:12 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Fri, Feb 11, 2022 at 06:25:17PM +0200, Jani Nikula wrote: > > On Fri, 11 Feb 2022, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Fri, Feb 11, 2022 at 02:05:56PM +0200, Jani Nikula wrote: > > >> On Fri, 11 Feb 2022, Thomas Zimmermann <tzimmermann@suse.de> 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
On Mon, Feb 14, 2022 at 11:17:11AM +0200, Pekka Paalanen wrote: > On Fri, 11 Feb 2022 19:27:12 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Feb 11, 2022 at 06:25:17PM +0200, Jani Nikula wrote: > > > On Fri, 11 Feb 2022, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > On Fri, Feb 11, 2022 at 02:05:56PM +0200, Jani Nikula wrote: > > > >> On Fri, 11 Feb 2022, Thomas Zimmermann <tzimmermann@suse.de> 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? To me these patterns, while() {} and do {} while(), while being shorter, also give a hint. So if one is familiar with C, the do {} while (--foo) _gives a hint_ while being shorter. It requires _less_ brain power to get this. But I assume my brain is unique and not working as million of others. > 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.
On Mon, Feb 14, 2022 at 10:03:53AM +0100, Thomas Zimmermann wrote: > Am 11.02.22 um 16:41 schrieb Andy Shevchenko: ... > > > 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. > > Naw, that's not true. In the section 3.5 "Loops - While and For" in "The C Programming Language" 2nd by K&R, the authors said: The for statement ... is equivalent to ... while..." They said that for is equivalent to while, and not otherwise. Also, syntax sugar by definition declares something that can be written as a single line of code, which usually is done using more (not always). > An idiomatic for loop, such as for (i = ...; i < N; > ++i), is such a strong pattern that it's way better than the corresponding > while loop. > > > 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.
On Monday, February 14th, 2022 at 11:38, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > 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. > > > > Naw, that's not true. > > In the section 3.5 "Loops - While and For" in "The C Programming > Language" 2nd by K&R, the authors said: > > The for statement ... is equivalent to ... while..." > > They said that for is equivalent to while, and not otherwise. > > Also, syntax sugar by definition declares something that can be written as > a single line of code, which usually is done using more (not always). arr[i] is syntaxic sugar for *(arr + i), yet we keep writing the former, because it's way more readable. The same goes for the for vs. while loops. It may be obvious for you because you're a C guru, but to me it just obfuscates the code. Too many C projects end up becoming completely unreadable because of patterns like these. Idiomatic C code isn't written by doing pointless micro-optimizations.
Hi Andy, On Mon, Feb 14, 2022 at 11:39 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, Feb 14, 2022 at 10:03:53AM +0100, Thomas Zimmermann wrote: > > Am 11.02.22 um 16:41 schrieb Andy Shevchenko: > > > > 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. > > > > Naw, that's not true. > > In the section 3.5 "Loops - While and For" in "The C Programming > Language" 2nd by K&R, the authors said: > > The for statement ... is equivalent to ... while..." > > They said that for is equivalent to while, and not otherwise. When I learned C, people told me to prefer while() over for() when possible, as several compilers are better at optimizing while()-loops than for()-loops. During the last 3 decades, optimizers got better, and all the bad old compilers went the way of the dodo (see also [1])... But even for a human, it's still less symbols to decode (and verify all the details about =/</>/<=/>=/++/--/...) for while (n--) { ... } than for for (i = 0; i < n; i++) { ... } [1] https://lwn.net/Articles/871283/ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Am 14.02.22 um 11:38 schrieb Andy Shevchenko: > On Mon, Feb 14, 2022 at 10:03:53AM +0100, Thomas Zimmermann wrote: >> Am 11.02.22 um 16:41 schrieb Andy Shevchenko: > > ... > >>>> 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. >> >> Naw, that's not true. > > In the section 3.5 "Loops - While and For" in "The C Programming > Language" 2nd by K&R, the authors said: Year of publication: 1988 . It's not the most up-to-date reference for C programming. > > The for statement ... is equivalent to ... while..." > > They said that for is equivalent to while, and not otherwise. Even leaving readability aside, it's not equivalent. You can declare variables as part of the for statement. (I know it's not the kernel's style.) Also, 'continue' statements are not well-suited in for loops, because it's non-obvious if the loop's update statement is being executed. (It isn't.) > > Also, syntax sugar by definition declares something that can be written as > a single line of code, which usually is done using more (not always). The discussion has entered the phase of hair splitting. Good. Best regards Thomas > >> An idiomatic for loop, such as for (i = ...; i < N; >> ++i), is such a strong pattern that it's way better than the corresponding >> while loop. > >>>> 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. >
On Mon, Feb 14, 2022 at 01:12:48PM +0100, Thomas Zimmermann wrote: > Hi > > Am 14.02.22 um 11:38 schrieb Andy Shevchenko: > > On Mon, Feb 14, 2022 at 10:03:53AM +0100, Thomas Zimmermann wrote: > >> Am 11.02.22 um 16:41 schrieb Andy Shevchenko: > > > > ... > > > >>>> 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. > >> > >> Naw, that's not true. > > > > In the section 3.5 "Loops - While and For" in "The C Programming > > Language" 2nd by K&R, the authors said: > > Year of publication: 1988 . It's not the most up-to-date reference for C > programming. > > > > > The for statement ... is equivalent to ... while..." > > > > They said that for is equivalent to while, and not otherwise. > > Even leaving readability aside, it's not equivalent. You can declare > variables as part of the for statement. (I know it's not the kernel's > style.) Also, 'continue' statements are not well-suited in for loops, > because it's non-obvious if the loop's update statement is being > executed. (It isn't.) It is. 'continue' is just shorthand for 'goto end_of_loop_body'.
Hi Am 14.02.22 um 13:47 schrieb Ville Syrjälä: > On Mon, Feb 14, 2022 at 01:12:48PM +0100, Thomas Zimmermann wrote: >> Hi >> >> Am 14.02.22 um 11:38 schrieb Andy Shevchenko: >>> On Mon, Feb 14, 2022 at 10:03:53AM +0100, Thomas Zimmermann wrote: >>>> Am 11.02.22 um 16:41 schrieb Andy Shevchenko: >>> >>> ... >>> >>>>>> 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. >>>> >>>> Naw, that's not true. >>> >>> In the section 3.5 "Loops - While and For" in "The C Programming >>> Language" 2nd by K&R, the authors said: >> >> Year of publication: 1988 . It's not the most up-to-date reference for C >> programming. >> >>> >>> The for statement ... is equivalent to ... while..." >>> >>> They said that for is equivalent to while, and not otherwise. >> >> Even leaving readability aside, it's not equivalent. You can declare >> variables as part of the for statement. (I know it's not the kernel's >> style.) Also, 'continue' statements are not well-suited in for loops, >> because it's non-obvious if the loop's update statement is being >> executed. (It isn't.) > > It is. > > 'continue' is just shorthand for 'goto end_of_loop_body'. Well, indeed. lol Fun fact: I actually had to look this up and still got it wrong. Let me just count it under proving-my-point: continue in a for statement is a bad idea and for isn't equivalent to while. Best regards Thomas >
On Mon, Feb 14, 2022 at 01:54:59PM +0100, Thomas Zimmermann wrote: > Hi > > Am 14.02.22 um 13:47 schrieb Ville Syrjälä: > > On Mon, Feb 14, 2022 at 01:12:48PM +0100, Thomas Zimmermann wrote: > >> Hi > >> > >> Am 14.02.22 um 11:38 schrieb Andy Shevchenko: > >>> On Mon, Feb 14, 2022 at 10:03:53AM +0100, Thomas Zimmermann wrote: > >>>> Am 11.02.22 um 16:41 schrieb Andy Shevchenko: > >>> > >>> ... > >>> > >>>>>> 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. > >>>> > >>>> Naw, that's not true. > >>> > >>> In the section 3.5 "Loops - While and For" in "The C Programming > >>> Language" 2nd by K&R, the authors said: > >> > >> Year of publication: 1988 . It's not the most up-to-date reference for C > >> programming. > >> > >>> > >>> The for statement ... is equivalent to ... while..." > >>> > >>> They said that for is equivalent to while, and not otherwise. > >> > >> Even leaving readability aside, it's not equivalent. You can declare > >> variables as part of the for statement. (I know it's not the kernel's > >> style.) Also, 'continue' statements are not well-suited in for loops, > >> because it's non-obvious if the loop's update statement is being > >> executed. (It isn't.) > > > > It is. > > > > 'continue' is just shorthand for 'goto end_of_loop_body'. > > Well, indeed. lol > > Fun fact: I actually had to look this up and still got it wrong. Let me > just count it under proving-my-point: continue in a for statement is a > bad idea and for isn't equivalent to while. Nah. We use 'continue' a *lot* in for loops in kms/atomic code. I'd be surprised if you can find many loops without a 'continue'. Looking at the loc stats I was a bit surprised to see more 'break' but then I realized switch() is bloating up those numbers quite a bit.
On Mon, Feb 14, 2022 at 01:12:48PM +0100, Thomas Zimmermann wrote: > Am 14.02.22 um 11:38 schrieb Andy Shevchenko: > > On Mon, Feb 14, 2022 at 10:03:53AM +0100, Thomas Zimmermann wrote: > > > Am 11.02.22 um 16:41 schrieb Andy Shevchenko: ... > > > > > 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. > > > > > > Naw, that's not true. > > > > In the section 3.5 "Loops - While and For" in "The C Programming > > Language" 2nd by K&R, the authors said: > > Year of publication: 1988 . It's not the most up-to-date reference for C > programming. Yet this makes your above remark invalid, i.e. `for` _is_ syntax sugar despite what you think it's idiomatic _nowadays_. > > The for statement ... is equivalent to ... while..." > > > > They said that for is equivalent to while, and not otherwise. > > Even leaving readability aside, it's not equivalent. You can declare > variables as part of the for statement. (I know it's not the kernel's > style.) Also, 'continue' statements are not well-suited in for loops, > because it's non-obvious if the loop's update statement is being executed. > (It isn't.) It's also written in the book :-) > > Also, syntax sugar by definition declares something that can be written as > > a single line of code, which usually is done using more (not always). > > The discussion has entered the phase of hair splitting. Good. I don't know why we are adding an oil into the flames...
diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c index 0f28dd2bdd72..b981712623d3 100644 --- a/drivers/gpu/drm/drm_format_helper.c +++ b/drivers/gpu/drm/drm_format_helper.c @@ -464,6 +464,21 @@ void drm_fb_xrgb8888_to_xrgb2101010_toio(void __iomem *dst, } EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010_toio); +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++; + } +} + /** * drm_fb_xrgb8888_to_gray8 - Convert XRGB8888 to grayscale * @dst: 8-bit grayscale destination buffer @@ -484,8 +499,9 @@ EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010_toio); void drm_fb_xrgb8888_to_gray8(void *dst, unsigned int dst_pitch, const void *vaddr, const struct drm_framebuffer *fb, const struct drm_rect *clip) { - unsigned int len = (clip->x2 - clip->x1) * sizeof(u32); - unsigned int x, y; + unsigned int linepixels = clip->x2 - clip->x1; + unsigned int len = linepixels * sizeof(u32); + unsigned int y; void *buf; u8 *dst8; u32 *src32; @@ -508,16 +524,7 @@ void drm_fb_xrgb8888_to_gray8(void *dst, unsigned int dst_pitch, const void *vad for (y = clip->y1; y < clip->y2; y++) { dst8 = dst; src32 = memcpy(buf, vaddr, len); - for (x = clip->x1; x < clip->x2; x++) { - u8 r = (*src32 & 0x00ff0000) >> 16; - u8 g = (*src32 & 0x0000ff00) >> 8; - u8 b = *src32 & 0x000000ff; - - /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */ - *dst8++ = (3 * r + 6 * g + b) / 10; - src32++; - } - + drm_fb_xrgb8888_to_gray8_line(dst8, src32, linepixels); vaddr += fb->pitches[0]; dst += dst_pitch; }
Pull the per-line conversion logic into a separate helper function. This will allow to do line-by-line conversion in other helpers that convert to a gray8 format. Suggested-by: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- (no changes since v3) Changes in v3: - Add a drm_fb_xrgb8888_to_gray8_line() helper function (Thomas Zimmermann) drivers/gpu/drm/drm_format_helper.c | 31 ++++++++++++++++++----------- 1 file changed, 19 insertions(+), 12 deletions(-)