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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 13.10.26 um 15:07 schrieb Jie Feng:
> On Tue, 13 Oct 2026 at 20:32, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 13.10.26 um 14:17 schrieb Jie Feng:
>>> On Tue, 13 Oct 2026 at 15:11, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>> Hi
>>>>
>>>> Am 12.10.26 um 19:08 schrieb Ho Jie Feng:
>>>>> Add a helper for devices that use packed UYVY but accept XRGB8888
>>>>> framebuffers.
>>>> Is there a description of UYVY available somewhere?
>>> This is for a reverse engineered usb display adapter, which accepts
>>> input in bytes of U, Y, V, Y in that order. I'm not sure what
>>> counts as a description, apart from it matching DRM_FORMAT_UYVY?
>> I see. The format exists already in DRM.
>>
>>> This patch is meant to move this out of the driver:
>>> https://github.com/rhgndf/ms912x/blob/main/ms912x_transfer.c#L153
>>>
>>>>> Signed-off-by: Ho Jie Feng <hjf3108@gmail.com>
>>>>> ---
>>>>> This is used for a driver, ms912x that I will be submitting. It only
>>>>> supports uyvy framebuffers.
>>>> Please resubmit as part of the driver series. We're not going to merge
>>>> it without callers anyway.
>>>> ...
>>>> For naming, please follow the conventions used by the other helpers in
>>>> this file.
>>> Will send the driver in with this patch and its fixes, thanks for the review.
>>>
>>>> ...
>>>>> +             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.

>
> 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?

>
> 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)