Re: [PATCH v4 1/2] drm/format-helper: Add conversion from XRGB8888 to BGR888

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 24.02.25 um 15:29 schrieb andriy.shevchenko@linux.intel.com:
> On Mon, Feb 24, 2025 at 01:38:32PM +0000, Aditya Garg wrote:
>> From: Kerem Karabay <kekrby@gmail.com>
>>
>> Add XRGB8888 emulation helper for devices that only support BGR888.
> ...
>
>> +static void drm_fb_xrgb8888_to_bgr888_line(void *dbuf, const void *sbuf, unsigned int pixels)
> Okay the xrgb8888 is the actual pixel format independently on
> the CPU endianess.
>
>> +{
>> +	u8 *dbuf8 = dbuf;
>> +	const __le32 *sbuf32 = sbuf;
> But here we assume that sbuf is __le32.
> And I think we may benefit from the __be32 there.

No, please. XRGB is the logical order. The raw physical byte order for 
DRM formats is always* little endian, hence reversed from the logical 
one. sbuf points to raw memory and is therefore __le32. DRM-format byte 
order is impossible to understand, I know. But that code is correct.

Best regards
Thomas

*) White lie: there's a DRM format flag signalling physical big 
endianess. That isn't the case here. So nothing here should ever 
indicate big endianess.


>
>> +	unsigned int x;
>> +	u32 pix;
>> +
>> +	for (x = 0; x < pixels; x++) {
>> +		pix = le32_to_cpu(sbuf32[x]);
>> +		/* write red-green-blue to output in little endianness */
>> +		*dbuf8++ = (pix & 0x00ff0000) >> 16;
>> +		*dbuf8++ = (pix & 0x0000ff00) >> 8;
>> +		*dbuf8++ = (pix & 0x000000ff) >> 0;
> 		pix = be32_to_cpu(sbuf[4 * x]) >> 8;
> 		put_unaligned_le24(pix, &dbuf[3 * x]);
>
>> +	}
> Or, after all, this __le32 magic might be not needed at all. Wouldn't the below
> be the equivalent
>
> static void drm_fb_xrgb8888_to_bgr888_line(void *dbuf, const void *sbuf, unsigned int pixels)
> {
> 	unsigned int x;
> 	u32 pix;
>
> 	for (x = 0; x < pixels; x++) {
> 		/* Read red-green-blue from input in big endianess and... */
> 		pix = get_unaligned_be24(sbuf + x * 4 + 1);
> 		/* ...write it to output in little endianness. */
> 		put_unaligned_le24(pix, dbuf + x * 3);
> 	}
> }
>
> The comments can even be dropped as the code quite clear about what's going on.
>
>> +}
> But it's up to you. I don't know which solution gives better code generation
> either.
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)