Re: [PATCH v2 2/3] drm/format-helper: Add drm_fb_xrgb8888_to_xrgb2101010_toio()

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3117 bytes --]

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.

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]