Re: [PATCH] drm/format-helper: Add XRGB8888 to UYVY conversion

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 14.10.26 um 18:19 schrieb Jie Feng:
> On Wed, 14 Oct 2026 at 17:03, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>>> ...
>>>>>>> +             v = drm_pixel_rgb2_to_v_bt601(r, g, b);
>>>>>>> +             u = drm_pixel_rgb2_to_u_bt601(r, g, b);
>>>>>>> +
>>>>>>> +             transfer_buffer[dst_offset++] = u;
>>>>>>> +             transfer_buffer[dst_offset++] = y1;
>>>>>>> +             transfer_buffer[dst_offset++] = v;
>>>>>>> +             transfer_buffer[dst_offset++] = y2;
>>>>>> Rather that writing bytes individually, use put_unaligned_le32() or a
>>>>>> similar helper.  Byte order within a pixel is little endian, although
>>>>>> I'm not sure what this means for UYVY.
>>>>> This is meant to be sent over a usb transfer, so byte order needs to be
>>>>> the same on both little and big endian systems.
>>>> DRM formats always refer to little endian. As it is implemented now,
>>>> it's not clear what the endianess of your output is. Please duplicate
>>>> the other helpers' logic. They all pack the output with appropriate
>>>> helpers. If the USB device requires BE, you have to write a separate
>>>> conversion helper for that.
>>>>
>>>> BTW, you have an OOB access if the line width is not a multiple of 2.
>>> How should I handle this? Rounding it within the helper doesn't seem
>>> to make sense and the function return is a void, so it can't return -EINVAL.
>> It's the callers responsibility to pass in enough pixels. You'll have to
>> take care of it in the driver by rounding the damage area accordingly.
>> It's not optimal but it's not a big deal either.
>>
>> In the line-conversion helper, simply round down to the nearest multiple
>> of 2.
> Yes, that is already done in my driver.
>
>>> Other helpers ignore the -ENOMEM error when drm_format_conv_state_reserve
>>> fails, so should it follow this convention?
>> Do you have a specific example?
> The call chain goes like this:
> drm_fb_xrgb8888_to_rgb888
> drm_fb_xfrm
> __drm_fb_xfrm:
>          stmp = drm_format_conv_state_reserve(state, sbuf_len, GFP_KERNEL);
>          if (!stmp)
>              return -ENOMEM;
>
> Since drm_fb_xrgb8888_to_rgb888 is a void, it calls drm_fb_xfrm without
> checking for the return value. Similarly for other drm_fb_xrgb8888_to*
> functions. For the uyvy helper it could just return without
> doing any work if it is not a multiple of 2, like in the -ENOMEM case.

Ok, that's what you mean.  We run these conversion functions in 
atomic_update, when it's too late to do anything with the return value. 
So we drop the return value.

You should rather not return an error if the width is not a multiple of 
2. Even then, there's some data available for output to the user.

Errors and problems need to be detected by atomic_check, prepare_fb or a 
similar callback that runs before atomic_update.

Best regards
Thomas


>
>>> Alternatively, drm_fb_xfrm can be exported, and let the driver define
>>> the rgb->yuv coefficients? But I think it is fine since _to_gray8 also
>>> cites BT.601.
>> The current constants seem fine. With the 2-pixel alignment, there
>> shouldn't be any other cases required.
>>
>> Best regards
>> Thomas
>>
>>
>>
>> --
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
>> GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)
>>
>>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)