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

[Thomas Zimmermann]

Hi

Am 13.10.26 um 14:17 schrieb Jie Feng:
> On Tue, 13 Oct 2026 at 15:11, Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxxxx>
> > > ---
> > > 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.

Best regards
Thomas

> > > ...
> > > +static inline u32 drm_pixel_rgb_to_y_bt601(u32 r, u32 g, u32 b)
> > > +{
> > > +     const u32 luma = (16U << 16) + 16829 * r + 33039 * g + 6416 * b + 32768;
> > > +
> > > +     return luma >> 16;
> > > +}
> > > +
> > > +static inline u32 drm_pixel_rgb2_to_u_bt601(u32 r2, u32 g2, u32 b2)
> > > +{
> > > +     const u32 u = (128U << 17) - 9714 * r2 - 19071 * g2 + 28784 * b2 + 65536;
> > > +
> > > +     return u >> 17;
> > > +}
> > > +
> > > +static inline u32 drm_pixel_rgb2_to_v_bt601(u32 r2, u32 g2, u32 b2)
> > > +{
> > > +     const u32 v = (128U << 17) + 28784 * r2 - 24103 * g2 - 4681 * b2 + 65536;
> > > +
> > > +     return v >> 17;
> > > +}
> > > +
> > These helpers seem too fine-grained. IMHO it would be better to have a
> > helper like  drm_pixel_rgb_to_uyvy() that takes two RGB values and
> > returns the assembled 16-bit UYVY value.
> Makes sense.
>
> > Best regards
> > Thomas
> >
> > >    /*
> > >     * Conversion from ARGB8888
> > >     */
> > > diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> > > index 2b5c1aef80b0e..e50f96e07484c 100644
> > > --- a/include/drm/drm_format_helper.h
> > > +++ b/include/drm/drm_format_helper.h
> > > @@ -136,4 +136,8 @@ void drm_fb_xrgb8888_to_gray2(struct iosys_map *dst, const unsigned int *dst_pit
> > >                              const struct iosys_map *src, const struct drm_framebuffer *fb,
> > >                              const struct drm_rect *clip, struct drm_format_conv_state *state);
> > >
> > > +void drm_fb_xrgb8888_to_uyvy(struct iosys_map *dst, const unsigned int *dst_pitch,
> > > +                          const struct iosys_map *src, const struct drm_framebuffer *fb,
> > > +                          const struct drm_rect *clip, struct drm_format_conv_state *state);
> > > +
> > >    #endif /* __LINUX_DRM_FORMAT_HELPER_H */
> > --
> > --
> > 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)