Re: [PATCH 2/7] drm/format-helper: Add drm_fb_xrgb8888_to_rgb332()

["Noralf Trønnes" <noralf@tronnes.org>]

Den 31.08.2021 14.23, skrev Daniel Vetter:
> On Mon, Aug 30, 2021 at 02:08:14PM +0200, Noralf Trønnes wrote:
>>
>>
>> Den 17.08.2021 15.56, skrev Daniel Vetter:
>>> On Tue, Aug 17, 2021 at 02:29:12PM +0200, Noralf Trønnes wrote:
>>>> Add XRGB8888 emulation support for devices that can only do RGB332.
>>>>
>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>> ---
>>>>  drivers/gpu/drm/drm_format_helper.c | 47 +++++++++++++++++++++++++++++
>>>>  include/drm/drm_format_helper.h     |  2 ++
>>>>  2 files changed, 49 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
>>>> index 5231104b1498..53b426da7467 100644
>>>> --- a/drivers/gpu/drm/drm_format_helper.c
>>>> +++ b/drivers/gpu/drm/drm_format_helper.c
>>>> @@ -135,6 +135,53 @@ void drm_fb_swab(void *dst, void *src, struct drm_framebuffer *fb,
>>>>  }
>>>>  EXPORT_SYMBOL(drm_fb_swab);
>>>>  
>>>> +static void drm_fb_xrgb8888_to_rgb332_line(u8 *dbuf, u32 *sbuf, unsigned int pixels)
>>>> +{
>>>> +	unsigned int x;
>>>> +
>>>> +	for (x = 0; x < pixels; x++)
>>>> +		dbuf[x] = ((sbuf[x] & 0x00e00000) >> 16) |
>>>
>>> I think for 2/3 bits correct rounding would be useful, not just masking.
>>> i.e. before you shift add 0x00100000 here, and similar below.
>>>
>>
>> Math isn't my strongest side and my brain failed to turn this into code.
> 
> Essentially add half of the lowest bit before you mask, so
> 
> ((sbuf[x] + 0x10) & 0xe0 )
> 

But what if the value is 0xff, it overflows:

((0xff + 0x10) & 0xe0 ) == 0x00

Should it be OR?

((0xff | 0x10) & 0xe0 ) == 0xe0

Noralf.

> I dropped the shift to make it clear what's going on. If you're mask is
> 0xc0, then you simply add 0x20 before masking.
> 
>>> Also I just realized we've totally ignored endianess on these, which is
>>> not great, because strictly speaking all the drm_fourcc codes should be
>>> little endian. But I'm also not sure that's worth fixing ...
>>>
>>
>> Is it as simple as using le32_to_cpu()?
> 
> I think so.
> 
> Plus on any format that has u16 output we'd need a cpu_to_le16 wrapped
> around the entire thing.
> -Daniel
> 
>> static void drm_fb_xrgb8888_to_rgb332_line(u8 *dbuf, __le32 *sbuf,
>> unsigned int pixels)
>> {
>> 	unsigned int x;
>> 	u32 pix;
>>
>> 	for (x = 0; x < pixels; x++) {
>> 		pix = le32_to_cpu(sbuf[x]);
>> 		dbuf[x] = ((pix & 0x00e00000) >> 16) |
>> 			  ((pix & 0x0000e000) >> 11) |
>> 			  ((pix & 0x000000c0) >> 6);
>> 	}
>> }
>>
>> Noralf.
>>
>>> Either way, lgtm:
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>
>>>> +			  ((sbuf[x] & 0x0000e000) >> 11) |
>>>> +			  ((sbuf[x] & 0x000000c0) >> 6);
>>>> +}
>>>> +
>>>> +/**
>>>> + * drm_fb_xrgb8888_to_rgb332 - Convert XRGB8888 to RGB332 clip buffer
>>>> + * @dst: RGB332 destination buffer
>>>> + * @src: XRGB8888 source buffer
>>>> + * @fb: DRM framebuffer
>>>> + * @clip: Clip rectangle area to copy
>>>> + *
>>>> + * Drivers can use this function for RGB332 devices that don't natively support XRGB8888.
>>>> + *
>>>> + * This function does not apply clipping on dst, i.e. the destination is a small buffer
>>>> + * containing the clip rect only.
>>>> + */
>>>> +void drm_fb_xrgb8888_to_rgb332(void *dst, void *src, struct drm_framebuffer *fb,
>>>> +			       struct drm_rect *clip)
>>>> +{
>>>> +	size_t width = drm_rect_width(clip);
>>>> +	size_t src_len = width * sizeof(u32);
>>>> +	unsigned int y;
>>>> +	void *sbuf;
>>>> +
>>>> +	/* Use a buffer to speed up access on buffers with uncached read mapping (i.e. WC) */
>>>> +	sbuf = kmalloc(src_len, GFP_KERNEL);
>>>> +	if (!sbuf)
>>>> +		return;
>>>> +
>>>> +	src += clip_offset(clip, fb->pitches[0], sizeof(u32));
>>>> +	for (y = 0; y < drm_rect_height(clip); y++) {
>>>> +		memcpy(sbuf, src, src_len);
>>>> +		drm_fb_xrgb8888_to_rgb332_line(dst, sbuf, width);
>>>> +		src += fb->pitches[0];
>>>> +		dst += width;
>>>> +	}
>>>> +
>>>> +	kfree(sbuf);
>>>> +}
>>>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb332);
>>>> +
>>>>  static void drm_fb_xrgb8888_to_rgb565_line(u16 *dbuf, u32 *sbuf,
>>>>  					   unsigned int pixels,
>>>>  					   bool swab)
>>>> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
>>>> index 4e0258a61311..d0809aff5cf8 100644
>>>> --- a/include/drm/drm_format_helper.h
>>>> +++ b/include/drm/drm_format_helper.h
>>>> @@ -16,6 +16,8 @@ void drm_fb_memcpy_dstclip(void __iomem *dst, unsigned int dst_pitch, void *vadd
>>>>  			   struct drm_rect *clip);
>>>>  void drm_fb_swab(void *dst, void *src, struct drm_framebuffer *fb,
>>>>  		 struct drm_rect *clip, bool cached);
>>>> +void drm_fb_xrgb8888_to_rgb332(void *dst, void *vaddr, struct drm_framebuffer *fb,
>>>> +			       struct drm_rect *clip);
>>>>  void drm_fb_xrgb8888_to_rgb565(void *dst, void *vaddr,
>>>>  			       struct drm_framebuffer *fb,
>>>>  			       struct drm_rect *clip, bool swab);
>>>> -- 
>>>> 2.32.0
>>>>
>>>
>