Messages in this thread | | | Date | Tue, 7 Dec 2021 11:27:10 +0100 | Subject | Re: [PATCH v2 2/3] drm/format-helper: Add drm_fb_xrgb8888_to_xrgb2101010_toio() | From | Thomas Zimmermann <> |
| |
Am 07.12.21 um 11:20 schrieb Thomas Zimmermann: > Hi > > Am 07.12.21 um 10:54 schrieb Hector Martin: >> Hi, thanks for the review! >> >> On 07/12/2021 18.40, Thomas Zimmermann wrote: >>> Hi >>> >>> Am 07.12.21 um 08:29 schrieb Hector Martin: >>>> Add XRGB8888 emulation support for devices that can only do >>>> XRGB2101010. >>>> >>>> This is chiefly useful for simpledrm on Apple devices where the >>>> bootloader-provided framebuffer is 10-bit. >>>> >>>> Signed-off-by: Hector Martin <marcan@marcan.st> >>>> --- >>>> drivers/gpu/drm/drm_format_helper.c | 62 >>>> +++++++++++++++++++++++++++++ >>>> include/drm/drm_format_helper.h | 3 ++ >>>> 2 files changed, 65 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_format_helper.c >>>> b/drivers/gpu/drm/drm_format_helper.c >>>> index dbe3e830096e..edd611d3ab6a 100644 >>>> --- a/drivers/gpu/drm/drm_format_helper.c >>>> +++ b/drivers/gpu/drm/drm_format_helper.c >>>> @@ -409,6 +409,59 @@ void drm_fb_xrgb8888_to_rgb888_toio(void >>>> __iomem *dst, unsigned int dst_pitch, >>>> } >>>> EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888_toio); >>>> +static void drm_fb_xrgb8888_to_xrgb2101010_line(u32 *dbuf, const >>>> u32 *sbuf, >>>> + unsigned int pixels) >>>> +{ >>>> + unsigned int x; >>>> + >>>> + for (x = 0; x < pixels; x++) { >>>> + *dbuf++ = ((sbuf[x] & 0x000000FF) << 2) | >>>> + ((sbuf[x] & 0x0000FF00) << 4) | >>>> + ((sbuf[x] & 0x00FF0000) << 6); >>> >>> This isn't quite right. The lowest two destination bits in each >>> component will always be zero. You have to do the shifting as above and >>> for each component the two highest source bits have to be OR'ed into the >>> two lowest destination bits. For example the source bits in a component >>> are numbered 7 to 0 >>> >>> | 7 6 5 4 3 2 1 0 | >>> >>> then the destination bits should be >>> >>> | 7 6 5 4 3 2 1 0 7 6 | >>> >> >> I think both approaches have pros and cons. Leaving the two LSBs >> always at 0 yields a fully linear transfer curve with no >> discontinuities, but means the maximum brightness is slightly less >> than full. Setting them fully maps the brightness range, but creates 4 >> double wide steps in the transfer curve (also it's potentially >> slightly slower CPU-wise). >> >> If you prefer the latter I'll do that for v2. > > We don't give guarantees for color output unless color spaces are > involved. But the lack of LSB bits can be more visible than larger steps > in the curve. With the current formats here, it's probably a non-issue. > But there can be conversions, such as RGB444 to RGB88, where these > missing LSBs make a visible difference. > > Therefore, please change the algorithm. It produces more consistent > results over a variety of format conversion. It's better to have the > same (default) algorithm for all of them.
FTR, I just tested this in a painting program. I can see a difference between ffffff and fcfcfc iff both are next to each other. f8f8f8 is obviously gray.
> > Best regards > Thomas > >> >
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev [unhandled content-type:application/pgp-signature]
| |