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

[Thomas Zimmermann]

Hi

Am 14.10.26 um 18:19 schrieb Jie Feng:
> On Wed, 14 Oct 2026 at 17:03, Thomas Zimmermann <tzimmermann@xxxxxxx> 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)