linux-kernel - Re: [PATCH v4 2/6] drm/format-helper: Add drm_fb_xrgb8888_to_mono

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YgaG98O12Fg7WmQJ@smile.fi.intel.com>
Date:   Fri, 11 Feb 2022 17:55:35 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Javier Martinez Canillas <javierm@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-fbdev@...r.kernel.org,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Maxime Ripard <maxime@...no.tech>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        dri-devel@...ts.freedesktop.org,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Sam Ravnborg <sam@...nborg.org>,
        Noralf Trønnes <noralf@...nnes.org>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>
Subject: Re: [PATCH v4 2/6] drm/format-helper: Add
 drm_fb_xrgb8888_to_mono_reversed()

On Fri, Feb 11, 2022 at 12:50:04PM +0100, Javier Martinez Canillas wrote:
> On 2/11/22 12:10, Andy Shevchenko wrote:

...

> >> +	for (xb = 0; xb < pixels; xb++) {
> >> +		unsigned int start = 0, end = 8;
> >> +		u8 byte = 0x00;
> > 
> >> +		if (xb == 0 && start_offset)
> >> +			start = start_offset;
> > 
> > This is invariant to the loop, can be moved out.
> > 
> >> +		if (xb == pixels - 1 && end_len)
> >> +			end = end_len;
> > 
> > Ditto. However it may require to factor out the following loop to a helper.
> 
> Not sure I'm following, it's not invariant since it depends on the
> loop iterator value. It only applies to the first and last pixels.

It's. You simply does it at the last iteration which may be perfectly done
outside of the main (aligned) loop.

...

> >> +		dst_pitch = DIV_ROUND_UP(linepixels, 8);
> > 
> > round_up() ?
> 
> But it's not a round up operation but a div and round up.

Indeed.

...

> >> +	WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n");
> > 
> > 
> > I would move this to the if conditional, i.e.
> > 
> > 	if (dst_pitch)
> > 		WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n");
> > 	else
> > 		dst_pitch = round_up(linepixels, 8);
> 
> No, because we always need to div and round up. The warning is just printed to
> let know that the dst pitch is not a multiple of 8 as it should be. So callers
> could be fixed.

Okay, you expect that linepixels to be multiple of 64? Otherwise I didn't get
what's going on with this warning.

...

> >> +	start_offset = clip->x1 % 8;
> >> +	end_len = clip->x2 % 8;
> > 
> > ALIGN() ?
> 
> But we don't want to align here but to know what's the start and end if is
> not aligned since that would mean converting to mono in the middle of a byte.

Indeed. Somehow I missed that it's a complimentary to ALIGN().

-- 
With Best Regards,
Andy Shevchenko