[PATCH v2 0/4] drm: Add MacroSilicon MS912x/MS913x USB display support

[PATCH v2 0/4] drm: Add MacroSilicon MS912x/MS913x USB display support

[Ho Jie Feng]

From: Jie Feng <hjf3108@gmail.com>

This series adds a DRM/KMS driver for MacroSilicon MS912x/MS913x
USB display adapters. These chips are commonly found in low-cost
USB to HDMI/VGA adapters.

Chip support:
- MS9120 to MS9125 (USB 2)
- MS9132 and MS9133 (USB 3)

The four patches add:
  1. XRGB8888-to-UYVY conversion helper.
  2. Register access and connector support.
  3. Atomic modesetting and USB framebuffer transfers.
  4. Kconfig, Makefile and MAINTAINERS integration.

The custom-timing support is based on MacroSilicon's GPLv2 Linux
driver.

Tested on USB to composite, VGA and HDMI adapters, covering both
USB 2 and USB 3 devices.

Changelog v2:
- Reject custom timings with odd active widths for packed UYVY output.
- Mark only one timing as preferred
- Keep rects for UYVY conversion even width

Ho Jie Feng (4):
  drm/format-helper: Add XRGB8888 to UYVY conversion
  drm/ms91xx: Add register access and connector support
  drm/ms91xx: Add atomic modesetting and USB framebuffer transfers
  drm/ms91xx: Add Kconfig and Makefile integration

 MAINTAINERS                               |   7 +
 drivers/gpu/drm/Kconfig                   |   1 +
 drivers/gpu/drm/Makefile                  |   1 +
 drivers/gpu/drm/drm_format_helper.c       |  50 ++
 drivers/gpu/drm/drm_format_internal.h     |  19 +
 drivers/gpu/drm/ms91xx/Kconfig            |  13 +
 drivers/gpu/drm/ms91xx/Makefile           |   9 +
 drivers/gpu/drm/ms91xx/ms91xx.h           | 221 +++++++++
 drivers/gpu/drm/ms91xx/ms91xx_connector.c | 267 +++++++++++
 drivers/gpu/drm/ms91xx/ms91xx_drv.c       | 528 ++++++++++++++++++++++
 drivers/gpu/drm/ms91xx/ms91xx_registers.c | 393 ++++++++++++++++
 drivers/gpu/drm/ms91xx/ms91xx_transfer.c  | 222 +++++++++
 include/drm/drm_format_helper.h           |   4 +
 13 files changed, 1735 insertions(+)
 create mode 100644 drivers/gpu/drm/ms91xx/Kconfig
 create mode 100644 drivers/gpu/drm/ms91xx/Makefile
 create mode 100644 drivers/gpu/drm/ms91xx/ms91xx.h
 create mode 100644 drivers/gpu/drm/ms91xx/ms91xx_connector.c
 create mode 100644 drivers/gpu/drm/ms91xx/ms91xx_drv.c
 create mode 100644 drivers/gpu/drm/ms91xx/ms91xx_registers.c
 create mode 100644 drivers/gpu/drm/ms91xx/ms91xx_transfer.c


base-commit: 476d658928af91f5ccb455957be60763d8b00fd3
-- 
2.55.0

[PATCH v2 1/4] drm/format-helper: Add XRGB8888 to UYVY conversion

[Ho Jie Feng]

Add a helper for devices that use packed UYVY but accept XRGB8888
framebuffers.

Signed-off-by: Ho Jie Feng <hjf3108@gmail.com>
---
 drivers/gpu/drm/drm_format_helper.c   | 50 +++++++++++++++++++++++++++
 drivers/gpu/drm/drm_format_internal.h | 19 ++++++++++
 include/drm/drm_format_helper.h       |  4 +++
 3 files changed, 73 insertions(+)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index 6cddf05c4..7e5cbb605 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -1379,3 +1379,53 @@ void drm_fb_xrgb8888_to_gray2(struct iosys_map *dst, const unsigned int *dst_pit
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_gray2);
 
+static void drm_fb_xrgb8888_to_uyvy_line(void *dbuf, const void *sbuf, unsigned int pixels)
+{
+	__le32 *dbuf32 = dbuf;
+	const __le32 *sbuf32 = sbuf;
+	const __le32 *send32 = sbuf32 + pixels;
+
+	while (sbuf32 < send32) {
+		u32 pix1 = le32_to_cpup(sbuf32);
+		u32 pix2 = le32_to_cpup(sbuf32 + 1);
+		u32 val32 = drm_pixel_xrgb8888_to_uyvy_bt601(pix1, pix2);
+
+		*dbuf32++ = cpu_to_le32(val32);
+		sbuf32 += 2;
+	}
+}
+
+/**
+ * drm_fb_xrgb8888_to_uyvy - Convert XRGB8888 to UYVY clip buffer
+ * @dst: Array of UYVY destination buffers
+ * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
+ *             within @dst; can be NULL if scanlines are stored next to each other.
+ * @src: Array of XRGB8888 source buffers
+ * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy; x1 and x2 must be even
+ * @state: Transform and conversion state
+ *
+ * This function copies parts of a framebuffer to display memory and converts the
+ * color format during the process. The parameters @dst, @dst_pitch and @src refer
+ * to arrays. Each array must have at least as many entries as there are planes in
+ * @fb's format. Each entry stores the value for the format's respective color plane
+ * at the same index.
+ *
+ * This function does not apply clipping on @dst (i.e. the destination is at the
+ * top-left corner).
+ *
+ * Drivers can use this function for UYVY devices that don't support XRGB8888
+ * natively. ITU BT.601 is being used for the RGB -> YUV conversion.
+ */
+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)
+{
+	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
+		2,
+	};
+
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
+		    drm_fb_xrgb8888_to_uyvy_line);
+}
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_uyvy);
diff --git a/drivers/gpu/drm/drm_format_internal.h b/drivers/gpu/drm/drm_format_internal.h
index ce29dd05b..41875656e 100644
--- a/drivers/gpu/drm/drm_format_internal.h
+++ b/drivers/gpu/drm/drm_format_internal.h
@@ -159,6 +159,25 @@ static inline u32 drm_pixel_xrgb8888_to_abgr2101010(u32 pix)
 	       drm_pixel_xrgb8888_to_xbgr2101010(pix);
 }
 
+static inline u32 drm_pixel_xrgb8888_to_uyvy_bt601(u32 pix1, u32 pix2)
+{
+	u32 r1 = (pix1 & 0x00ff0000) >> 16;
+	u32 g1 = (pix1 & 0x0000ff00) >> 8;
+	u32 b1 =  pix1 & 0x000000ff;
+	u32 r2 = (pix2 & 0x00ff0000) >> 16;
+	u32 g2 = (pix2 & 0x0000ff00) >> 8;
+	u32 b2 =  pix2 & 0x000000ff;
+	u32 r = r1 + r2;
+	u32 g = g1 + g2;
+	u32 b = b1 + b2;
+	u32 y1 = ((16U << 16) + 16829 * r1 + 33039 * g1 + 6416 * b1 + 32768) >> 16;
+	u32 y2 = ((16U << 16) + 16829 * r2 + 33039 * g2 + 6416 * b2 + 32768) >> 16;
+	u32 u = ((128U << 17) - 9714 * r - 19071 * g + 28784 * b + 65536) >> 17;
+	u32 v = ((128U << 17) + 28784 * r - 24103 * g - 4681 * b + 65536) >> 17;
+
+	return u | (y1 << 8) | (v << 16) | (y2 << 24);
+}
+
 /*
  * Conversion from ARGB8888
  */
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index 2b5c1aef8..e50f96e07 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 */
-- 
2.55.0

[PATCH v2 2/4] drm/ms91xx: Add register access and connector support

[Ho Jie Feng]

Add HID register and flash access, decode custom device timings, and enumerate connector modes for MacroSilicon MS912x/MS913x adapters.

Signed-off-by: Ho Jie Feng <hjf3108@gmail.com>
---
 drivers/gpu/drm/ms91xx/ms91xx.h           | 221 ++++++++++++
 drivers/gpu/drm/ms91xx/ms91xx_connector.c | 267 +++++++++++++++
 drivers/gpu/drm/ms91xx/ms91xx_registers.c | 393 ++++++++++++++++++++++
 3 files changed, 881 insertions(+)
 create mode 100644 drivers/gpu/drm/ms91xx/ms91xx.h
 create mode 100644 drivers/gpu/drm/ms91xx/ms91xx_connector.c
 create mode 100644 drivers/gpu/drm/ms91xx/ms91xx_registers.c

diff --git a/drivers/gpu/drm/ms91xx/ms91xx.h b/drivers/gpu/drm/ms91xx/ms91xx.h
new file mode 100644
index 000000000..6e8c4d64a
--- /dev/null
+++ b/drivers/gpu/drm/ms91xx/ms91xx.h
@@ -0,0 +1,221 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2022-2026 Ho Jie Feng <hjf3108@gmail.com>
+ * Copyright (C) 2023 MacroSilicon Technology Co., Ltd.
+ */
+
+#ifndef MS91XX_H
+#define MS91XX_H
+
+#include <linux/bits.h>
+#include <linux/compiler_attributes.h>
+#include <linux/completion.h>
+#include <linux/container_of.h>
+#include <linux/mutex.h>
+#include <linux/scatterlist.h>
+#include <linux/timer_types.h>
+#include <linux/types.h>
+#include <linux/usb.h>
+#include <linux/workqueue.h>
+
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_device.h>
+#include <drm/drm_encoder.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_plane.h>
+#include <drm/drm_rect.h>
+
+struct drm_format_conv_state;
+struct drm_framebuffer;
+struct iosys_map;
+
+#define DRIVER_NAME "ms91xx"
+#define DRIVER_DESC "MacroSilicon USB to VGA/HDMI"
+
+#define DRIVER_MAJOR 1
+#define DRIVER_MINOR 0
+
+#define MS91XX_BULK_OUT_ENDPOINT	4
+
+#define MS91XX_REQ_TYPE_WRITE_6_BYTES	0xa6
+#define MS91XX_REQ_TYPE_READ_BYTE	0xb5
+#define MS91XX_REQ_TYPE_READ_FLASH	0xf5
+
+#define MS91XX_REG_VIDEO_PORT		0x0031
+#define MS91XX_REG_DISPLAY_STATUS	0x0032
+#define MS91XX_REG_EDID_BASE		0xc000
+
+#define MS91XX_REG_MODE_SEQUENCE_0	0x0030
+#define MS91XX_REG_MODE_SEQUENCE_1	0x0033
+#define MS91XX_REG_MODE_SEQUENCE_2	0xc620
+
+#define MS91XX_CMD_RESOLUTION		0x01
+#define MS91XX_CMD_MODE			0x02
+#define MS91XX_CMD_UNKNOWN1		0x03
+#define MS91XX_CMD_UNKNOWN2		0x04
+#define MS91XX_CMD_OUTPUT_ENABLE	0x05
+#define MS91XX_CMD_POWER		0x07
+
+#define MS913X_REG_CHIP_ID			0xff00
+#define MS912X_REG_CHIP_ID			0xf000
+#define MS913X_CHIP_ID_SIGNATURE_MSB		0x13
+#define MS912X_CHIP_ID_SIGNATURE_MSB		0x16
+#define MS91XX_CHIP_ID_SIGNATURE_LSB		0x0a
+#define MS913X_CUSTOM_TIMING_BASE		0xfc50
+#define MS912X_CUSTOM_TIMING_BASE		0x1c00
+#define MS91XX_CUSTOM_TIMING_OFFSET		0x10
+#define MS91XX_CUSTOM_TIMING_STRIDE		0x20
+
+#define MS91XX_TIMING_PROGRESSIVE		BIT(0)
+#define MS91XX_TIMING_POSITIVE_HSYNC		BIT(1)
+#define MS91XX_TIMING_POSITIVE_VSYNC		BIT(2)
+
+enum ms91xx_video_port {
+	MS91XX_VIDEO_PORT_CVBS = 0,
+	MS91XX_VIDEO_PORT_SVIDEO = 1,
+	MS91XX_VIDEO_PORT_VGA = 2,
+	MS91XX_VIDEO_PORT_YPBPR = 3,
+	MS91XX_VIDEO_PORT_CVBS_SVIDEO = 4,
+	MS91XX_VIDEO_PORT_HDMI = 5,
+	MS91XX_VIDEO_PORT_DIGITAL = 6,
+	MS91XX_VIDEO_PORT_UNKNOWN = 0xff,
+};
+
+struct ms91xx_usb_request {
+	void *transfer_buffer;
+	struct ms91xx_device *ms91xx;
+	size_t transfer_len;
+	struct drm_rect transfer_rect;
+	struct sg_table transfer_sgt;
+	struct usb_sg_request sgr;
+	struct work_struct work;
+	struct timer_list timer;
+	struct completion done;
+};
+
+struct ms91xx_mode {
+	int width;
+	int height;
+	int hz;
+	int mode;
+};
+
+struct ms91xx_custom_mode {
+	struct ms91xx_mode mode;
+	struct drm_display_mode display_mode;
+};
+
+#define MS91XX_PIXFMT_RGB888		0x11
+#define MS91XX_PIXFMT_UYVY		0x22
+#define MS91XX_BYTE_SELECT_UYVY		0x00
+
+#define MS91XX_MODE(w, h, z, m)                                               \
+	{                                                                      \
+		.width = w, .height = h, .hz = z, .mode = m                    \
+	}
+
+struct ms91xx_device {
+	struct drm_device drm;
+	struct usb_interface *intf;
+	unsigned int bulk_pipe;
+	enum ms91xx_video_port port_type;
+	struct workqueue_struct *workqueue;
+	/* Serializes HID control request/response sequences */
+	struct mutex ctrl_lock;
+
+	struct drm_connector connector;
+	struct drm_encoder encoder;
+	struct drm_crtc crtc;
+	struct drm_plane plane;
+
+	struct ms91xx_custom_mode custom_modes[2];
+	unsigned int num_custom_modes;
+
+	struct drm_rect update_rect;
+
+	/* Double buffer to allow memcpy and transfer
+	 * to happen in parallel
+	 */
+	int current_request;
+	struct ms91xx_usb_request requests[2];
+};
+
+struct ms91xx_request {
+	u8 type;
+	__be16 addr;
+	u8 data[5];
+} __packed;
+
+struct ms91xx_write_request {
+	u8 type;
+	u8 addr;
+	u8 data[6];
+} __packed;
+
+struct ms91xx_resolution_request {
+	__be16 width;
+	__be16 height;
+	u8 pixel_format;
+	u8 byte_select;
+} __packed;
+
+struct ms91xx_mode_request {
+	u8 mode;
+	u8 pixel_format;
+	__be16 width;
+	__be16 height;
+} __packed;
+
+struct ms91xx_frame_update_header {
+	__be16 marker;
+	u8 position[3]; /* x:y, packed 12-bit big-endian values */
+	u8 dimensions[3]; /* width:height, packed 12-bit big-endian values */
+} __packed;
+
+struct ms91xx_flash_read_request {
+	u8 type;
+	u8 addr[3];
+	u8 reserved[4];
+} __packed;
+
+struct ms91xx_custom_timing_record {
+	u8 vic;
+	u8 polarity;
+	__le16 htotal;
+	__le16 vtotal;
+	__le16 hactive;
+	__le16 vactive;
+	__le16 pixclk;
+	__le16 vfreq;
+	__le16 hoffset;
+	__le16 voffset;
+	__le16 hsyncwidth;
+	__le16 vsyncwidth;
+} __packed;
+
+#define MS91XX_FRAME_OVERHEAD 16
+#define MS91XX_MAX_WIDTH 1920
+#define MS91XX_MAX_HEIGHT 1200
+#define MS91XX_MAX_TRANSFER_LEN \
+	(MS91XX_MAX_WIDTH * MS91XX_MAX_HEIGHT * 2 + MS91XX_FRAME_OVERHEAD)
+
+#define to_ms91xx(x) container_of(x, struct ms91xx_device, drm)
+
+int ms91xx_read_byte(struct ms91xx_device *ms91xx, u16 address);
+int ms91xx_read_custom_timing(struct ms91xx_device *ms91xx);
+int ms91xx_connector_init(struct ms91xx_device *ms91xx);
+int ms91xx_set_resolution(struct ms91xx_device *ms91xx,
+			  const struct ms91xx_mode *mode);
+
+int ms91xx_power_on(struct ms91xx_device *ms91xx);
+int ms91xx_power_off(struct ms91xx_device *ms91xx);
+
+int ms91xx_fb_send_rect(struct drm_framebuffer *fb, const struct iosys_map *map,
+			struct drm_format_conv_state *fmtcnv_state,
+			struct drm_rect *rect);
+
+void ms91xx_free_request(struct ms91xx_usb_request *request);
+int ms91xx_init_request(struct ms91xx_device *ms91xx,
+			struct ms91xx_usb_request *request, size_t len);
+#endif
diff --git a/drivers/gpu/drm/ms91xx/ms91xx_connector.c b/drivers/gpu/drm/ms91xx/ms91xx_connector.c
new file mode 100644
index 000000000..08371d121
--- /dev/null
+++ b/drivers/gpu/drm/ms91xx/ms91xx_connector.c
@@ -0,0 +1,267 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022-2026 Ho Jie Feng <hjf3108@gmail.com>
+ */
+
+#include <linux/array_size.h>
+#include <linux/types.h>
+
+#include <drm/drm_atomic_state_helper.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_device.h>
+#include <drm/drm_edid.h>
+#include <drm/drm_encoder.h>
+#include <drm/drm_mode_config.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_print.h>
+
+#include "ms91xx.h"
+
+static int ms91xx_get_connector_type(struct ms91xx_device *ms91xx)
+{
+	int port_type;
+
+	port_type = ms91xx_read_byte(ms91xx, MS91XX_REG_VIDEO_PORT);
+	if (port_type < 0) {
+		drm_warn(&ms91xx->drm,
+			 "failed to read video port type: %d\n", port_type);
+		ms91xx->port_type = MS91XX_VIDEO_PORT_UNKNOWN;
+		return DRM_MODE_CONNECTOR_Unknown;
+	}
+
+	ms91xx->port_type = port_type;
+
+	switch (ms91xx->port_type) {
+	case MS91XX_VIDEO_PORT_CVBS:
+		return DRM_MODE_CONNECTOR_Composite;
+	case MS91XX_VIDEO_PORT_VGA:
+		return DRM_MODE_CONNECTOR_VGA;
+	case MS91XX_VIDEO_PORT_YPBPR:
+		return DRM_MODE_CONNECTOR_Component;
+	/*
+	 * Combo VGA+HDMI device seems to report this as connector type
+	 * Since there doesn't seem to be any S-Video devices out there,
+	 * set this to HDMI instead.
+	 */
+	case MS91XX_VIDEO_PORT_SVIDEO:
+	case MS91XX_VIDEO_PORT_HDMI:
+		return DRM_MODE_CONNECTOR_HDMIA;
+	case MS91XX_VIDEO_PORT_DIGITAL:
+		return DRM_MODE_CONNECTOR_DPI;
+	case MS91XX_VIDEO_PORT_CVBS_SVIDEO:
+		return DRM_MODE_CONNECTOR_9PinDIN;
+	default:
+		drm_warn(&ms91xx->drm,
+			 "unknown video port type: %d\n", port_type);
+		ms91xx->port_type = MS91XX_VIDEO_PORT_UNKNOWN;
+		return DRM_MODE_CONNECTOR_Unknown;
+	}
+}
+
+static int
+ms91xx_get_encoder_type(enum ms91xx_video_port port_type)
+{
+	switch (port_type) {
+	case MS91XX_VIDEO_PORT_VGA:
+		return DRM_MODE_ENCODER_DAC;
+	case MS91XX_VIDEO_PORT_CVBS:
+	case MS91XX_VIDEO_PORT_YPBPR:
+	case MS91XX_VIDEO_PORT_CVBS_SVIDEO:
+		return DRM_MODE_ENCODER_TVDAC;
+	/* See above comment */
+	case MS91XX_VIDEO_PORT_SVIDEO:
+	case MS91XX_VIDEO_PORT_HDMI:
+		return DRM_MODE_ENCODER_TMDS;
+	case MS91XX_VIDEO_PORT_DIGITAL:
+		return DRM_MODE_ENCODER_DPI;
+	case MS91XX_VIDEO_PORT_UNKNOWN:
+	default:
+		return DRM_MODE_ENCODER_NONE;
+	}
+}
+
+static int ms91xx_read_edid(void *data, u8 *buf, unsigned int block, size_t len)
+{
+	struct ms91xx_device *ms91xx = data;
+	int offset = block * EDID_LENGTH;
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < len; i++) {
+		u16 address = MS91XX_REG_EDID_BASE + offset + i;
+
+		ret = ms91xx_read_byte(ms91xx, address);
+		if (ret < 0)
+			return ret;
+		buf[i] = ret;
+	}
+	return 0;
+}
+
+static int ms91xx_add_cea_modes(struct drm_connector *connector,
+				const u8 *vics, size_t num_vics)
+{
+	struct drm_display_mode *mode;
+	unsigned int i;
+	int count = 0;
+
+	drm_edid_connector_update(connector, NULL);
+
+	for (i = 0; i < num_vics; i++) {
+		mode = drm_display_mode_from_cea_vic(connector->dev, vics[i]);
+		if (!mode)
+			continue;
+
+		if (!count)
+			mode->type |= DRM_MODE_TYPE_PREFERRED;
+		drm_mode_probed_add(connector, mode);
+		count++;
+	}
+
+	return count;
+}
+
+static int ms91xx_add_cvbs_svideo_modes(struct drm_connector *connector)
+{
+	static const u8 vics[] = {
+		2,  /* 720x480p60 */
+		17, /* 720x576p50 */
+	};
+
+	return ms91xx_add_cea_modes(connector, vics, ARRAY_SIZE(vics));
+}
+
+static int ms91xx_add_ypbpr_modes(struct drm_connector *connector)
+{
+	static const u8 vics[] = {
+		4,  /* 1280x720p60 */
+		16, /* 1920x1080p60 */
+		2,  /* 720x480p60 */
+		17, /* 720x576p50 */
+	};
+
+	return ms91xx_add_cea_modes(connector, vics, ARRAY_SIZE(vics));
+}
+
+static int
+ms91xx_add_default_hdmi_vga_modes(struct drm_connector *connector)
+{
+	struct drm_mode_config *mode_config = &connector->dev->mode_config;
+	int count;
+
+	drm_edid_connector_update(connector, NULL);
+	count = drm_add_modes_noedid(connector, mode_config->max_width,
+				     mode_config->max_height);
+	if (count)
+		drm_set_preferred_mode(connector, 1024, 768);
+
+	return count;
+}
+
+static int ms91xx_add_custom_mode(struct drm_connector *connector)
+{
+	struct ms91xx_device *ms91xx = to_ms91xx(connector->dev);
+	struct drm_display_mode *mode;
+	unsigned int i;
+	int count = 0;
+
+	for (i = 0; i < ms91xx->num_custom_modes; i++) {
+		mode = drm_mode_duplicate(
+			connector->dev,
+			&ms91xx->custom_modes[i].display_mode);
+		if (!mode)
+			continue;
+
+		drm_mode_probed_add(connector, mode);
+		count++;
+	}
+
+	return count;
+}
+
+static int ms91xx_connector_get_modes(struct drm_connector *connector)
+{
+	struct ms91xx_device *ms91xx = to_ms91xx(connector->dev);
+	const struct drm_edid *edid;
+	int ret = ms91xx_add_custom_mode(connector);
+
+	if (ms91xx->port_type == MS91XX_VIDEO_PORT_CVBS ||
+	    ms91xx->port_type == MS91XX_VIDEO_PORT_CVBS_SVIDEO ||
+	    ms91xx->port_type == MS91XX_VIDEO_PORT_UNKNOWN)
+		return ret + ms91xx_add_cvbs_svideo_modes(connector);
+
+	if (ms91xx->port_type == MS91XX_VIDEO_PORT_YPBPR)
+		return ret + ms91xx_add_ypbpr_modes(connector);
+
+	edid = drm_edid_read_custom(connector, ms91xx_read_edid, ms91xx);
+	if (!edid)
+		return ret + ms91xx_add_default_hdmi_vga_modes(connector);
+
+	if (drm_edid_connector_update(connector, edid) >= 0)
+		ret += drm_edid_connector_add_modes(connector);
+	drm_edid_free(edid);
+	return ret;
+}
+
+static enum drm_connector_status ms91xx_detect(struct drm_connector *connector,
+					       bool force)
+{
+	struct ms91xx_device *ms91xx = to_ms91xx(connector->dev);
+	int status = ms91xx_read_byte(ms91xx, MS91XX_REG_DISPLAY_STATUS);
+
+	if (status < 0)
+		return connector_status_unknown;
+
+	return status == 1 ? connector_status_connected :
+			     connector_status_disconnected;
+}
+
+static const struct drm_encoder_funcs ms91xx_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
+static const struct drm_connector_helper_funcs ms91xx_connector_helper_funcs = {
+	.get_modes = ms91xx_connector_get_modes,
+};
+
+static const struct drm_connector_funcs ms91xx_connector_funcs = {
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.detect = ms91xx_detect,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+int ms91xx_connector_init(struct ms91xx_device *ms91xx)
+{
+	int ret;
+	int connector_type;
+	int encoder_type;
+
+	connector_type = ms91xx_get_connector_type(ms91xx);
+	encoder_type = ms91xx_get_encoder_type(ms91xx->port_type);
+
+	ret = drm_encoder_init(&ms91xx->drm, &ms91xx->encoder,
+			       &ms91xx_encoder_funcs, encoder_type, NULL);
+	if (ret)
+		return ret;
+	ms91xx->encoder.possible_crtcs = drm_crtc_mask(&ms91xx->crtc);
+
+	drm_connector_helper_add(&ms91xx->connector,
+				 &ms91xx_connector_helper_funcs);
+	ret = drm_connector_init(&ms91xx->drm, &ms91xx->connector,
+				 &ms91xx_connector_funcs, connector_type);
+	if (ret)
+		return ret;
+
+	ms91xx->connector.polled =
+		DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
+	ms91xx->connector.interlace_allowed = true;
+
+	return drm_connector_attach_encoder(&ms91xx->connector,
+					    &ms91xx->encoder);
+}
diff --git a/drivers/gpu/drm/ms91xx/ms91xx_registers.c b/drivers/gpu/drm/ms91xx/ms91xx_registers.c
new file mode 100644
index 000000000..2a45ebc86
--- /dev/null
+++ b/drivers/gpu/drm/ms91xx/ms91xx_registers.c
@@ -0,0 +1,393 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022-2026 Ho Jie Feng <hjf3108@gmail.com>
+ * Copyright (C) 2023 MacroSilicon Technology Co., Ltd.
+ */
+
+#include <asm/byteorder.h>
+#include <linux/errno.h>
+#include <linux/gfp.h>
+#include <linux/hid.h>
+#include <linux/math.h>
+#include <linux/minmax.h>
+#include <linux/mutex.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/usb.h>
+
+#include <drm/drm_drv.h>
+#include <drm/drm_modes.h>
+
+#include "ms91xx.h"
+
+int ms91xx_read_byte(struct ms91xx_device *ms91xx, u16 address)
+{
+	struct ms91xx_request request;
+	struct usb_device *usb_dev;
+	int idx, ret;
+
+	if (!drm_dev_enter(&ms91xx->drm, &idx))
+		return -ENODEV;
+
+	mutex_lock(&ms91xx->ctrl_lock);
+
+	usb_dev = interface_to_usbdev(ms91xx->intf);
+	memset(&request, 0, sizeof(request));
+	request.type = MS91XX_REQ_TYPE_READ_BYTE;
+	request.addr = cpu_to_be16(address);
+
+	ret = usb_control_msg_send(usb_dev, 0, HID_REQ_SET_REPORT,
+				   USB_DIR_OUT | USB_TYPE_CLASS |
+					   USB_RECIP_INTERFACE,
+				   0x0300, 0, &request, sizeof(request),
+				   USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
+	if (ret)
+		goto ctrl_unlock;
+
+	ret = usb_control_msg_recv(usb_dev, 0, HID_REQ_GET_REPORT,
+				   USB_DIR_IN | USB_TYPE_CLASS |
+					   USB_RECIP_INTERFACE,
+				   0x0300, 0, &request, sizeof(request),
+				   USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
+	if (!ret)
+		ret = request.data[0];
+
+ctrl_unlock:
+	mutex_unlock(&ms91xx->ctrl_lock);
+	drm_dev_exit(idx);
+	return ret;
+}
+
+static int ms91xx_read_flash(struct ms91xx_device *ms91xx, u32 address,
+			     void *data, size_t len)
+{
+	struct ms91xx_flash_read_request request;
+	struct usb_device *usb_dev;
+	size_t offset = 0;
+	u8 *dst = data;
+	int idx, ret = 0;
+
+	if (address > 0xffffff || len > 0x1000000 - address)
+		return -ERANGE;
+
+	if (!drm_dev_enter(&ms91xx->drm, &idx))
+		return -ENODEV;
+
+	mutex_lock(&ms91xx->ctrl_lock);
+	usb_dev = interface_to_usbdev(ms91xx->intf);
+
+	while (offset < len) {
+		size_t chunk = min_t(size_t, len - offset, sizeof(request));
+		u32 current_address = address + offset;
+
+		memset(&request, 0, sizeof(request));
+		request.type = MS91XX_REQ_TYPE_READ_FLASH;
+		request.addr[0] = current_address >> 16;
+		request.addr[1] = current_address >> 8;
+		request.addr[2] = current_address;
+
+		ret = usb_control_msg_send(usb_dev, 0, HID_REQ_SET_REPORT,
+					   USB_DIR_OUT | USB_TYPE_CLASS |
+						   USB_RECIP_INTERFACE,
+					   0x0300, 0, &request,
+					   sizeof(request),
+					   USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
+		if (ret)
+			goto ctrl_unlock;
+
+		ret = usb_control_msg_recv(usb_dev, 0, HID_REQ_GET_REPORT,
+					   USB_DIR_IN | USB_TYPE_CLASS |
+						   USB_RECIP_INTERFACE,
+					   0x0300, 0, &request,
+					   sizeof(request),
+					   USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
+		if (ret)
+			goto ctrl_unlock;
+
+		memcpy(dst + offset, &request, chunk);
+		offset += chunk;
+	}
+
+ctrl_unlock:
+	mutex_unlock(&ms91xx->ctrl_lock);
+	drm_dev_exit(idx);
+	return ret;
+}
+
+static int ms91xx_custom_timing_base(struct ms91xx_device *ms91xx)
+{
+	u8 signature[3];
+	int ret;
+
+	/* Read chip signature */
+	ret = ms91xx_read_byte(ms91xx, MS913X_REG_CHIP_ID);
+	if (ret < 0)
+		return ret;
+	signature[0] = ret;
+
+	ret = ms91xx_read_byte(ms91xx, MS913X_REG_CHIP_ID + 1);
+	if (ret < 0)
+		return ret;
+	signature[1] = ret;
+
+	ret = ms91xx_read_byte(ms91xx, MS913X_REG_CHIP_ID + 2);
+	if (ret < 0)
+		return ret;
+	signature[2] = ret;
+
+	if (signature[1] == MS913X_CHIP_ID_SIGNATURE_MSB &&
+	    signature[2] == MS91XX_CHIP_ID_SIGNATURE_LSB)
+		return MS913X_CUSTOM_TIMING_BASE;
+
+	ret = ms91xx_read_byte(ms91xx, MS912X_REG_CHIP_ID);
+	if (ret < 0)
+		return ret;
+	signature[0] = ret;
+
+	ret = ms91xx_read_byte(ms91xx, MS912X_REG_CHIP_ID + 1);
+	if (ret < 0)
+		return ret;
+	signature[1] = ret;
+
+	ret = ms91xx_read_byte(ms91xx, MS912X_REG_CHIP_ID + 2);
+	if (ret < 0)
+		return ret;
+	signature[2] = ret;
+
+	if (signature[1] == MS912X_CHIP_ID_SIGNATURE_MSB &&
+	    signature[2] == MS91XX_CHIP_ID_SIGNATURE_LSB)
+		return MS912X_CUSTOM_TIMING_BASE;
+
+	return -ENODEV;
+}
+
+static int
+ms91xx_decode_custom_timing(struct ms91xx_custom_mode *custom_mode,
+			    const struct ms91xx_custom_timing_record *record)
+{
+	struct drm_display_mode *display_mode = &custom_mode->display_mode;
+	u16 hsyncwidth = le16_to_cpu(record->hsyncwidth);
+	u16 vsyncwidth = le16_to_cpu(record->vsyncwidth);
+	u16 hoffset = le16_to_cpu(record->hoffset);
+	u16 voffset = le16_to_cpu(record->voffset);
+	u16 hactive = le16_to_cpu(record->hactive);
+	u16 vactive = le16_to_cpu(record->vactive);
+	u16 htotal = le16_to_cpu(record->htotal);
+	u16 vtotal = le16_to_cpu(record->vtotal);
+	u16 pixclk = le16_to_cpu(record->pixclk);
+	u16 vfreq = le16_to_cpu(record->vfreq);
+	unsigned int hblank, vblank;
+	int refresh;
+
+	if (!hactive || hactive > MS91XX_MAX_WIDTH || (hactive & 1) ||
+	    !vactive || vactive > MS91XX_MAX_HEIGHT ||
+	    htotal <= hactive || vtotal <= vactive || !pixclk || !vfreq)
+		return -EINVAL;
+
+	hblank = htotal - hactive;
+	vblank = vtotal - vactive;
+	if (!hsyncwidth || hsyncwidth > hoffset || hoffset > hblank ||
+	    !vsyncwidth || vsyncwidth > voffset || voffset > vblank)
+		return -EINVAL;
+
+	memset(display_mode, 0, sizeof(*display_mode));
+	display_mode->clock = pixclk * 10;
+	display_mode->hdisplay = hactive;
+	display_mode->hsync_start = htotal - hoffset;
+	display_mode->hsync_end = display_mode->hsync_start + hsyncwidth;
+	display_mode->htotal = htotal;
+	display_mode->vdisplay = vactive;
+	display_mode->vsync_start = vtotal - voffset;
+	display_mode->vsync_end = display_mode->vsync_start + vsyncwidth;
+	display_mode->vtotal = vtotal;
+	display_mode->type = DRM_MODE_TYPE_DRIVER;
+	display_mode->flags =
+		record->polarity & MS91XX_TIMING_POSITIVE_HSYNC ?
+			DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC;
+	display_mode->flags |=
+		record->polarity & MS91XX_TIMING_POSITIVE_VSYNC ?
+			DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC;
+	if (!(record->polarity & MS91XX_TIMING_PROGRESSIVE))
+		display_mode->flags |= DRM_MODE_FLAG_INTERLACE;
+
+	refresh = DIV_ROUND_CLOSEST(vfreq, 100);
+	if (!refresh || drm_mode_vrefresh(display_mode) != refresh)
+		return -EINVAL;
+
+	drm_mode_set_name(display_mode);
+	custom_mode->mode.width = hactive;
+	custom_mode->mode.height = vactive;
+	custom_mode->mode.hz = refresh;
+	custom_mode->mode.mode = record->vic;
+
+	return 0;
+}
+
+int ms91xx_read_custom_timing(struct ms91xx_device *ms91xx)
+{
+	struct ms91xx_custom_timing_record record;
+	unsigned int num_records, i;
+	int error = 0;
+	u8 marker[7];
+	int base, ret;
+
+	ms91xx->num_custom_modes = 0;
+
+	base = ms91xx_custom_timing_base(ms91xx);
+	if (base < 0)
+		return base;
+
+	ret = ms91xx_read_flash(ms91xx, base, marker, sizeof(marker));
+	if (ret)
+		return ret;
+
+	if (!memcmp(marker, "modify1", sizeof(marker)))
+		num_records = 1;
+	else if (!memcmp(marker, "modify2", sizeof(marker)))
+		num_records = 2;
+	else
+		return 0;
+
+	for (i = 0; i < num_records; i++) {
+		ret = ms91xx_read_flash(ms91xx,
+					base + MS91XX_CUSTOM_TIMING_OFFSET +
+						i * MS91XX_CUSTOM_TIMING_STRIDE,
+					&record, sizeof(record));
+		if (ret) {
+			error = ret;
+			continue;
+		}
+
+		ret = ms91xx_decode_custom_timing(
+			&ms91xx->custom_modes[ms91xx->num_custom_modes],
+			&record);
+		if (ret) {
+			error = ret;
+			continue;
+		}
+
+		ms91xx->num_custom_modes++;
+	}
+
+	if (ms91xx->num_custom_modes)
+		ms91xx->custom_modes[0].display_mode.type |= DRM_MODE_TYPE_PREFERRED;
+
+	return ms91xx->num_custom_modes ? ms91xx->num_custom_modes : error;
+}
+
+static int ms91xx_write_6_bytes(struct ms91xx_device *ms91xx,
+				u8 address, const void *data)
+{
+	struct ms91xx_write_request request;
+	struct usb_device *usb_dev;
+	int idx, ret;
+
+	if (!drm_dev_enter(&ms91xx->drm, &idx))
+		return -ENODEV;
+
+	mutex_lock(&ms91xx->ctrl_lock);
+
+	usb_dev = interface_to_usbdev(ms91xx->intf);
+	request.type = MS91XX_REQ_TYPE_WRITE_6_BYTES;
+	request.addr = address;
+	memcpy(request.data, data, sizeof(request.data));
+
+	ret = usb_control_msg_send(usb_dev, 0, HID_REQ_SET_REPORT,
+				   USB_DIR_OUT | USB_TYPE_CLASS |
+					   USB_RECIP_INTERFACE,
+				   0x0300, 0, &request, sizeof(request),
+				   USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
+	mutex_unlock(&ms91xx->ctrl_lock);
+	drm_dev_exit(idx);
+
+	return ret;
+}
+
+int ms91xx_power_on(struct ms91xx_device *ms91xx)
+{
+	u8 data[6];
+
+	memset(data, 0, sizeof(data));
+	data[0] = 0x01;
+	data[1] = 0x02;
+
+	return ms91xx_write_6_bytes(ms91xx, MS91XX_CMD_POWER, data);
+}
+
+int ms91xx_power_off(struct ms91xx_device *ms91xx)
+{
+	u8 data[6];
+
+	memset(data, 0, sizeof(data));
+
+	return ms91xx_write_6_bytes(ms91xx, MS91XX_CMD_POWER, data);
+}
+
+int ms91xx_set_resolution(struct ms91xx_device *ms91xx,
+			  const struct ms91xx_mode *mode)
+{
+	int ret;
+	u8 data[6];
+	struct ms91xx_resolution_request resolution_request;
+	struct ms91xx_mode_request mode_request;
+
+	int width = mode->width;
+	int height = mode->height;
+	int mode_num = mode->mode;
+
+	/* Sequence obtained from Windows USB captures */
+	memset(data, 0, sizeof(data));
+	data[0] = 0;
+	ret = ms91xx_write_6_bytes(ms91xx, MS91XX_CMD_UNKNOWN2, data);
+	if (ret < 0)
+		return ret;
+
+	ret = ms91xx_read_byte(ms91xx, MS91XX_REG_MODE_SEQUENCE_0);
+	if (ret < 0)
+		return ret;
+	ret = ms91xx_read_byte(ms91xx, MS91XX_REG_MODE_SEQUENCE_1);
+	if (ret < 0)
+		return ret;
+	ret = ms91xx_read_byte(ms91xx, MS91XX_REG_MODE_SEQUENCE_2);
+	if (ret < 0)
+		return ret;
+
+	memset(data, 0, sizeof(data));
+	data[0] = 0x03;
+	ret = ms91xx_write_6_bytes(ms91xx, MS91XX_CMD_UNKNOWN1, data);
+	if (ret < 0)
+		return ret;
+
+	/* Write resolution */
+	resolution_request.width = cpu_to_be16(width);
+	resolution_request.height = cpu_to_be16(height);
+	resolution_request.pixel_format = MS91XX_PIXFMT_UYVY;
+	resolution_request.byte_select = MS91XX_BYTE_SELECT_UYVY;
+	ret = ms91xx_write_6_bytes(ms91xx, MS91XX_CMD_RESOLUTION,
+				   &resolution_request);
+	if (ret < 0)
+		return ret;
+
+	/* Write mode */
+	mode_request.mode = mode_num;
+	mode_request.pixel_format = 0x01;
+	mode_request.width = cpu_to_be16(width);
+	mode_request.height = cpu_to_be16(height);
+	ret = ms91xx_write_6_bytes(ms91xx, MS91XX_CMD_MODE, &mode_request);
+	if (ret < 0)
+		return ret;
+
+	memset(data, 0, sizeof(data));
+	data[0] = 1;
+	ret = ms91xx_write_6_bytes(ms91xx, MS91XX_CMD_UNKNOWN2, data);
+	if (ret < 0)
+		return ret;
+
+	memset(data, 0, sizeof(data));
+	data[0] = 1;
+	ret = ms91xx_write_6_bytes(ms91xx, MS91XX_CMD_OUTPUT_ENABLE, data);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
-- 
2.55.0

Re: [PATCH v2 2/4] drm/ms91xx: Add register access and connector support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] drm/ms91xx: unbounded EDID block offset and u16 wrap in ms91xx_read_edid()
- [Medium] drm/ms91xx: multiple preferred modes set in ms91xx_connector_get_modes()
- [Medium] drm/ms91xx: unplug -ENODEV treated as unknown status in ms91xx_detect()
--

commit 32cd5aee0f634799386d1af3debfec7f0d6318ac
Author: Ho Jie Feng <hjf3108@gmail.com>

drm/ms91xx: Add register access and connector support

This commit adds HID register and flash access helpers, custom timing
record decoding, and DRM connector mode enumeration and detection for
MacroSilicon MS912x and MS913x USB display adapters.

> diff --git a/drivers/gpu/drm/ms91xx/ms91xx_connector.c b/drivers/gpu/drm/ms91xx/ms91xx_connector.c
> new file mode 100644
> index 0000000000000..08371d121091f
> --- /dev/null
> +++ b/drivers/gpu/drm/ms91xx/ms91xx_connector.c
> @@ -0,0 +1,267 @@

[ ... ]

> +static int ms91xx_read_edid(void *data, u8 *buf, unsigned int block, size_t len)
> +{
> +	struct ms91xx_device *ms91xx = data;
> +	int offset = block * EDID_LENGTH;
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < len; i++) {
> +		u16 address = MS91XX_REG_EDID_BASE + offset + i;
> +
> +		ret = ms91xx_read_byte(ms91xx, address);
> +		if (ret < 0)
> +			return ret;
> +		buf[i] = ret;
> +	}
> +	return 0;
> +}

[Severity: Medium]
Can ms91xx_read_edid() in drivers/gpu/drm/ms91xx/ms91xx_connector.c read
past the EDID register window and wrap address around u16 when a sink
reports EDID extension blocks?

When ms91xx_connector_get_modes() calls drm_edid_read_custom(),
_drm_do_get_edid() can request up to 255 extension blocks. Because
MS91XX_REG_EDID_BASE is 0xc000 in drivers/gpu/drm/ms91xx/ms91xx.h, higher
block indices read across unrelated registers such as
MS91XX_REG_MODE_SEQUENCE_2 (0xc620), MS912X_REG_CHIP_ID (0xf000),
MS913X_CUSTOM_TIMING_BASE (0xfc50), and MS913X_REG_CHIP_ID (0xff00).

Once block reaches 128 (offset >= 0x4000), MS91XX_REG_EDID_BASE + offset + i
overflows u16 and wraps into 0x0000..0x3fff, reading low control and status
registers such as MS91XX_REG_MODE_SEQUENCE_0 (0x0030) through
MS91XX_REG_MODE_SEQUENCE_1 (0x0033). Should ms91xx_read_edid() bound-check
block and offset + len against the hardware EDID buffer size before reading?

> +
> +static int ms91xx_add_cea_modes(struct drm_connector *connector,
> +				const u8 *vics, size_t num_vics)
> +{
> +	struct drm_display_mode *mode;
> +	unsigned int i;
> +	int count = 0;
> +
> +	drm_edid_connector_update(connector, NULL);
> +
> +	for (i = 0; i < num_vics; i++) {
> +		mode = drm_display_mode_from_cea_vic(connector->dev, vics[i]);
> +		if (!mode)
> +			continue;
> +
> +		if (!count)
> +			mode->type |= DRM_MODE_TYPE_PREFERRED;
> +		drm_mode_probed_add(connector, mode);
> +		count++;
> +	}
> +
> +	return count;
> +}

[ ... ]

> +static int
> +ms91xx_add_default_hdmi_vga_modes(struct drm_connector *connector)
> +{
> +	struct drm_mode_config *mode_config = &connector->dev->mode_config;
> +	int count;
> +
> +	drm_edid_connector_update(connector, NULL);
> +	count = drm_add_modes_noedid(connector, mode_config->max_width,
> +				     mode_config->max_height);
> +	if (count)
> +		drm_set_preferred_mode(connector, 1024, 768);
> +
> +	return count;
> +}
> +
> +static int ms91xx_add_custom_mode(struct drm_connector *connector)
> +{
> +	struct ms91xx_device *ms91xx = to_ms91xx(connector->dev);
> +	struct drm_display_mode *mode;
> +	unsigned int i;
> +	int count = 0;
> +
> +	for (i = 0; i < ms91xx->num_custom_modes; i++) {
> +		mode = drm_mode_duplicate(
> +			connector->dev,
> +			&ms91xx->custom_modes[i].display_mode);
> +		if (!mode)
> +			continue;
> +
> +		drm_mode_probed_add(connector, mode);
> +		count++;
> +	}
> +
> +	return count;
> +}
> +
> +static int ms91xx_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct ms91xx_device *ms91xx = to_ms91xx(connector->dev);
> +	const struct drm_edid *edid;
> +	int ret = ms91xx_add_custom_mode(connector);
> +
> +	if (ms91xx->port_type == MS91XX_VIDEO_PORT_CVBS ||
> +	    ms91xx->port_type == MS91XX_VIDEO_PORT_CVBS_SVIDEO ||
> +	    ms91xx->port_type == MS91XX_VIDEO_PORT_UNKNOWN)
> +		return ret + ms91xx_add_cvbs_svideo_modes(connector);
> +
> +	if (ms91xx->port_type == MS91XX_VIDEO_PORT_YPBPR)
> +		return ret + ms91xx_add_ypbpr_modes(connector);
> +
> +	edid = drm_edid_read_custom(connector, ms91xx_read_edid, ms91xx);
> +	if (!edid)
> +		return ret + ms91xx_add_default_hdmi_vga_modes(connector);
> +
> +	if (drm_edid_connector_update(connector, edid) >= 0)
> +		ret += drm_edid_connector_add_modes(connector);
> +	drm_edid_free(edid);
> +	return ret;
> +}

[Severity: Medium]
When custom timings are present in flash, does ms91xx_connector_get_modes()
in drivers/gpu/drm/ms91xx/ms91xx_connector.c end up marking multiple modes
on the connector with DRM_MODE_TYPE_PREFERRED?

In drivers/gpu/drm/ms91xx/ms91xx_registers.c, ms91xx_read_custom_timing()
marks the first custom mode as preferred:

    if (ms91xx->num_custom_modes)
            ms91xx->custom_modes[0].display_mode.type |= DRM_MODE_TYPE_PREFERRED;

During connector probing, ms91xx_connector_get_modes() calls
ms91xx_add_custom_mode(), which duplicates custom_modes[0].display_mode onto
connector->probed_modes with DRM_MODE_TYPE_PREFERRED set. It then proceeds to
call ms91xx_add_cvbs_svideo_modes(), ms91xx_add_ypbpr_modes(),
ms91xx_add_default_hdmi_vga_modes(), or drm_edid_connector_add_modes(), each
of which also sets DRM_MODE_TYPE_PREFERRED on another mode.

When drm_helper_probe_single_connector_modes() calls drm_mode_sort(), ties
between modes with DRM_MODE_TYPE_PREFERRED are broken by hdisplay * vdisplay,
causing any fallback, CEA, or EDID preferred mode with a larger resolution
to sort ahead of custom_modes[0]. Should only one mode be marked preferred?

> +
> +static enum drm_connector_status ms91xx_detect(struct drm_connector *connector,
> +					       bool force)
> +{
> +	struct ms91xx_device *ms91xx = to_ms91xx(connector->dev);
> +	int status = ms91xx_read_byte(ms91xx, MS91XX_REG_DISPLAY_STATUS);
> +
> +	if (status < 0)
> +		return connector_status_unknown;
> +
> +	return status == 1 ? connector_status_connected :
> +			     connector_status_disconnected;
> +}

[Severity: Medium]
When the USB device is unplugged, ms91xx_read_byte() in
drivers/gpu/drm/ms91xx/ms91xx_registers.c returns -ENODEV (either when
drm_dev_enter() fails or when usb_control_msg_send() fails on disconnect).

In drivers/gpu/drm/ms91xx/ms91xx_connector.c, ms91xx_detect() maps all
status < 0 values to connector_status_unknown instead of
connector_status_disconnected. In drm_helper_probe_single_connector_modes(),
connector_status_unknown continues to call ms91xx_connector_get_modes(),
which then fails drm_edid_read_custom() and populates fallback modes via
ms91xx_add_default_hdmi_vga_modes() (or CEA/custom modes) on the unplugged
device rather than pruning modes.

Should ms91xx_detect() return connector_status_disconnected when status is
-ENODEV (or when !drm_dev_enter())?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104151652.59135-1-hjf3108@gmail.com?part=2

[PATCH v2 3/4] drm/ms91xx: Add atomic modesetting and USB framebuffer transfers

[Ho Jie Feng]

Add the atomic display pipeline and double-buffered USB transfers for MacroSilicon adapters. Use the DRM UYVY conversion helper for framebuffer uploads.

Signed-off-by: Ho Jie Feng <hjf3108@gmail.com>
---
 drivers/gpu/drm/ms91xx/ms91xx_drv.c      | 528 +++++++++++++++++++++++
 drivers/gpu/drm/ms91xx/ms91xx_transfer.c | 222 ++++++++++
 2 files changed, 750 insertions(+)
 create mode 100644 drivers/gpu/drm/ms91xx/ms91xx_drv.c
 create mode 100644 drivers/gpu/drm/ms91xx/ms91xx_transfer.c

diff --git a/drivers/gpu/drm/ms91xx/ms91xx_drv.c b/drivers/gpu/drm/ms91xx/ms91xx_drv.c
new file mode 100644
index 000000000..7b8d1f1e3
--- /dev/null
+++ b/drivers/gpu/drm/ms91xx/ms91xx_drv.c
@@ -0,0 +1,528 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022-2026 Ho Jie Feng <hjf3108@gmail.com>
+ */
+
+#include <linux/array_size.h>
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/limits.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm.h>
+#include <linux/types.h>
+#include <linux/usb.h>
+#include <linux/workqueue.h>
+
+#include <drm/clients/drm_client_setup.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_atomic_state_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fbdev_shmem.h>
+#include <drm/drm_file.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_ioctl.h>
+#include <drm/drm_managed.h>
+#include <drm/drm_mode_config.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_modeset_helper.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_print.h>
+#include <drm/drm_rect.h>
+
+#include "ms91xx.h"
+
+static int ms91xx_usb_suspend(struct usb_interface *interface,
+			      pm_message_t message)
+{
+	struct ms91xx_device *ms91xx = usb_get_intfdata(interface);
+
+	return drm_mode_config_helper_suspend(&ms91xx->drm);
+}
+
+static int ms91xx_usb_resume(struct usb_interface *interface)
+{
+	struct ms91xx_device *ms91xx = usb_get_intfdata(interface);
+
+	return drm_mode_config_helper_resume(&ms91xx->drm);
+}
+
+DEFINE_DRM_GEM_FOPS(ms91xx_driver_fops);
+
+static const struct drm_driver ms91xx_drm_driver = {
+	.driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
+
+	/* GEM hooks */
+	.fops = &ms91xx_driver_fops,
+	DRM_GEM_SHMEM_DRIVER_OPS,
+	DRM_FBDEV_SHMEM_DRIVER_OPS,
+
+	.name = DRIVER_NAME,
+	.desc = DRIVER_DESC,
+	.major = DRIVER_MAJOR,
+	.minor = DRIVER_MINOR,
+};
+
+static const struct drm_mode_config_funcs ms91xx_mode_config_funcs = {
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+static const struct drm_mode_config_helper_funcs
+ms91xx_mode_config_helper_funcs = {
+	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+};
+
+static const struct ms91xx_mode ms91xx_mode_list[] = {
+	/* Found in captures of the Windows driver and dumped from device */
+	MS91XX_MODE(720,   480, 60, 0x02), /* 60.00 Hz */
+	MS91XX_MODE(720,   576, 50, 0x11), /* 50.00 Hz */
+	MS91XX_MODE(1280,  720, 50, 0x13), /* 50.00 Hz */
+	MS91XX_MODE(1920, 1080, 50, 0x1f), /* 50.00 Hz */
+	MS91XX_MODE(1920, 1080, 30, 0x22), /* 30.00 Hz */
+	MS91XX_MODE(640,   480, 60, 0x40), /* 59.94 Hz */
+	MS91XX_MODE(800,   600, 60, 0x42), /* 60.32 Hz */
+	MS91XX_MODE(800,   600, 75, 0x44), /* 75.00 Hz */
+	MS91XX_MODE(1024,  768, 60, 0x47), /* 60.00 Hz */
+	MS91XX_MODE(1024,  768, 75, 0x49), /* 75.03 Hz */
+	MS91XX_MODE(1152,  864, 60, 0x4c), /* 60.00 Hz */
+	MS91XX_MODE(1280,  600, 60, 0x4e), /* 60.00 Hz */
+	MS91XX_MODE(1280,  720, 60, 0x4f), /* 60.00 Hz */
+	MS91XX_MODE(1280,  768, 60, 0x54), /* 59.87 Hz */
+	MS91XX_MODE(1280,  768, 75, 0x56), /* 74.89 Hz */
+	MS91XX_MODE(1280,  800, 60, 0x57), /* 59.81 Hz */
+	MS91XX_MODE(1280,  960, 60, 0x5b), /* 60.00 Hz */
+	MS91XX_MODE(1280, 1024, 60, 0x60), /* 60.02 Hz */
+	MS91XX_MODE(1280, 1024, 75, 0x61), /* 75.02 Hz */
+	MS91XX_MODE(1360,  768, 60, 0x64), /* 60.02 Hz */
+	MS91XX_MODE(1366,  768, 60, 0x66), /* 59.79 Hz */
+	MS91XX_MODE(1400, 1050, 60, 0x67), /* 59.98 Hz */
+	MS91XX_MODE(1440,  900, 60, 0x6b), /* 59.89 Hz */
+	MS91XX_MODE(1600, 1200, 60, 0x73), /* 60.00 Hz */
+	MS91XX_MODE(1680, 1050, 60, 0x78), /* 59.95 Hz */
+	MS91XX_MODE(1920, 1080, 60, 0x81), /* 60.00 Hz */
+};
+
+static const struct ms91xx_mode *
+ms91xx_get_mode(struct ms91xx_device *ms91xx,
+		const struct drm_display_mode *mode)
+{
+	unsigned int i;
+	int width = mode->hdisplay;
+	int height = mode->vdisplay;
+	int hz = drm_mode_vrefresh(mode);
+
+	for (i = 0; i < ms91xx->num_custom_modes; i++) {
+		const struct ms91xx_mode *custom_mode =
+			&ms91xx->custom_modes[i].mode;
+
+		if ((ms91xx->custom_modes[i].display_mode.flags ^ mode->flags) &
+		    DRM_MODE_FLAG_INTERLACE)
+			continue;
+
+		if (custom_mode->width == width &&
+		    custom_mode->height == height &&
+		    custom_mode->hz == hz)
+			return custom_mode;
+	}
+
+	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+		return NULL;
+
+	for (i = 0; i < ARRAY_SIZE(ms91xx_mode_list); i++) {
+		if (ms91xx_mode_list[i].width == width &&
+		    ms91xx_mode_list[i].height == height &&
+		    ms91xx_mode_list[i].hz == hz) {
+			return &ms91xx_mode_list[i];
+		}
+	}
+	return NULL;
+}
+
+static void ms91xx_crtc_atomic_enable(struct drm_crtc *crtc,
+				      struct drm_atomic_commit *state)
+{
+	struct drm_crtc_state *crtc_state =
+		drm_atomic_get_new_crtc_state(state, crtc);
+	struct drm_device *dev = crtc->dev;
+	struct ms91xx_device *ms91xx = to_ms91xx(dev);
+	const struct ms91xx_mode *ms_mode;
+	int ret;
+
+	ret = ms91xx_power_on(ms91xx);
+	if (ret) {
+		drm_err(dev, "failed to power on display: %d\n", ret);
+		return;
+	}
+
+	ms_mode = ms91xx_get_mode(ms91xx, &crtc_state->mode);
+	if (!ms_mode) {
+		drm_err(dev, "unsupported mode passed to CRTC enable\n");
+		return;
+	}
+
+	ret = ms91xx_set_resolution(ms91xx, ms_mode);
+	if (ret)
+		drm_err(dev, "failed to set display mode: %d\n", ret);
+}
+
+static void ms91xx_cancel_transfer_work(struct ms91xx_device *ms91xx)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(ms91xx->requests); i++) {
+		struct ms91xx_usb_request *request = &ms91xx->requests[i];
+
+		if (cancel_work_sync(&request->work))
+			complete(&request->done);
+	}
+}
+
+static void ms91xx_crtc_atomic_disable(struct drm_crtc *crtc,
+				       struct drm_atomic_commit *state)
+{
+	struct drm_device *dev = crtc->dev;
+	struct ms91xx_device *ms91xx = to_ms91xx(dev);
+	int ret;
+
+	ms91xx_cancel_transfer_work(ms91xx);
+	ret = ms91xx_power_off(ms91xx);
+	if (ret && ret != -ENODEV)
+		drm_err(dev, "failed to power off display: %d\n", ret);
+}
+
+static enum drm_mode_status
+ms91xx_crtc_mode_valid(struct drm_crtc *crtc,
+		       const struct drm_display_mode *mode)
+{
+	struct ms91xx_device *ms91xx = to_ms91xx(crtc->dev);
+	const struct ms91xx_mode *ms_mode = ms91xx_get_mode(ms91xx, mode);
+
+	if (!ms_mode)
+		return MODE_BAD;
+
+	return MODE_OK;
+}
+
+static int ms91xx_plane_atomic_check(struct drm_plane *plane,
+				     struct drm_atomic_commit *state)
+{
+	struct drm_plane_state *old_plane_state;
+	struct drm_plane_state *new_plane_state;
+	struct drm_crtc_state *crtc_state = NULL;
+
+	old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+	new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+	if (new_plane_state->crtc)
+		crtc_state = drm_atomic_get_new_crtc_state(state,
+							   new_plane_state->crtc);
+
+	/*
+	 * A mode set does change how the device interprets the framebuffer
+	 * so ask for a full plane update for the first frame after one.
+	 */
+	new_plane_state->ignore_damage_clips =
+		old_plane_state->fb != new_plane_state->fb ||
+		(crtc_state && drm_atomic_crtc_needs_modeset(crtc_state));
+
+	return drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
+						   DRM_PLANE_NO_SCALING,
+						   DRM_PLANE_NO_SCALING,
+						   false, false);
+}
+
+static void ms91xx_clear_rect(struct drm_rect *rect)
+{
+	rect->x1 = INT_MAX;
+	rect->y1 = INT_MAX;
+	rect->x2 = 0;
+	rect->y2 = 0;
+}
+
+static void ms91xx_merge_rects(struct drm_rect *dest, struct drm_rect *r1,
+			       struct drm_rect *r2)
+{
+	dest->x1 = min(r1->x1, r2->x1);
+	dest->y1 = min(r1->y1, r2->y1);
+	dest->x2 = max(r1->x2, r2->x2);
+	dest->y2 = max(r1->y2, r2->y2);
+}
+
+static void ms91xx_plane_atomic_update(struct drm_plane *plane,
+				       struct drm_atomic_commit *state)
+{
+	struct drm_plane_state *old_plane_state;
+	struct drm_plane_state *new_plane_state;
+	struct drm_crtc_state *new_crtc_state;
+	struct drm_shadow_plane_state *shadow_plane_state;
+	struct ms91xx_device *ms91xx = to_ms91xx(plane->dev);
+	struct ms91xx_usb_request *current_request, *previous_request;
+	struct drm_rect current_rect, rect;
+
+	old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+	new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+
+	if (!new_plane_state->fb)
+		return;
+	new_crtc_state = drm_atomic_get_new_crtc_state(state,
+						       new_plane_state->crtc);
+	shadow_plane_state = to_drm_shadow_plane_state(new_plane_state);
+
+	/* Damage from the previous mode may not fit the new framebuffer. */
+	if (drm_atomic_crtc_needs_modeset(new_crtc_state))
+		ms91xx_clear_rect(&ms91xx->update_rect);
+
+	if (drm_atomic_helper_damage_merged(old_plane_state, new_plane_state,
+					    &current_rect)) {
+		current_request = &ms91xx->requests[ms91xx->current_request];
+		previous_request = &ms91xx->requests[!ms91xx->current_request];
+
+		/* Latency reduction: requeue with the latest frame data. */
+		if (cancel_work(&previous_request->work)) {
+			complete(&previous_request->done);
+			ms91xx_merge_rects(&current_rect, &current_rect,
+					   &previous_request->transfer_rect);
+			current_request = previous_request;
+			ms91xx->current_request = !ms91xx->current_request;
+		}
+
+		current_request->transfer_rect = current_rect;
+
+		/*
+		 * The device double buffers, so we need to send the update
+		 * rects of the last two frames.
+		 */
+		ms91xx_merge_rects(&rect, &current_rect, &ms91xx->update_rect);
+		if (ms91xx_fb_send_rect(new_plane_state->fb,
+					&shadow_plane_state->data[0],
+					&shadow_plane_state->fmtcnv_state,
+					&rect)) {
+			/* In case of error, merge the rects to update later */
+			ms91xx_merge_rects(&ms91xx->update_rect,
+					   &ms91xx->update_rect, &rect);
+		} else {
+			ms91xx->update_rect = current_rect;
+		}
+	}
+}
+
+static const struct drm_crtc_helper_funcs ms91xx_crtc_helper_funcs = {
+	.mode_valid = ms91xx_crtc_mode_valid,
+	.atomic_check = drm_crtc_helper_atomic_check,
+	.atomic_enable = ms91xx_crtc_atomic_enable,
+	.atomic_disable = ms91xx_crtc_atomic_disable,
+};
+
+static const struct drm_crtc_funcs ms91xx_crtc_funcs = {
+	.atomic_create_state = drm_atomic_helper_crtc_create_state,
+	.destroy = drm_crtc_cleanup,
+	.set_config = drm_atomic_helper_set_config,
+	.page_flip = drm_atomic_helper_page_flip,
+	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+static const struct drm_plane_helper_funcs ms91xx_plane_helper_funcs = {
+	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+	.atomic_check = ms91xx_plane_atomic_check,
+	.atomic_update = ms91xx_plane_atomic_update,
+};
+
+static const struct drm_plane_funcs ms91xx_plane_funcs = {
+	.update_plane = drm_atomic_helper_update_plane,
+	.disable_plane = drm_atomic_helper_disable_plane,
+	.destroy = drm_plane_cleanup,
+	DRM_GEM_SHADOW_PLANE_FUNCS,
+};
+
+static const u32 ms91xx_plane_formats[] = {
+	DRM_FORMAT_XRGB8888,
+};
+
+static const u8 ms91xx_bulk_out_endpoints[] = {
+	MS91XX_BULK_OUT_ENDPOINT, 0
+};
+
+static int ms91xx_usb_probe(struct usb_interface *interface,
+			    const struct usb_device_id *id)
+{
+	int ret;
+	unsigned int i;
+	struct ms91xx_device *ms91xx;
+	struct drm_device *dev;
+	struct device *dma_dev;
+	struct usb_device *usbdev;
+
+	ms91xx = devm_drm_dev_alloc(&interface->dev, &ms91xx_drm_driver,
+				    struct ms91xx_device, drm);
+	if (IS_ERR(ms91xx))
+		return PTR_ERR(ms91xx);
+
+	ms91xx->intf = interface;
+	ret = devm_mutex_init(&interface->dev, &ms91xx->ctrl_lock);
+	if (ret)
+		return ret;
+
+	if (!usb_check_bulk_endpoints(interface, ms91xx_bulk_out_endpoints))
+		return -ENXIO;
+
+	ret = ms91xx_read_custom_timing(ms91xx);
+	if (ret < 0) {
+		drm_warn(&ms91xx->drm,
+			 "failed to read custom timing: %d\n", ret);
+	}
+	for (i = 0; i < ms91xx->num_custom_modes; i++) {
+		const struct ms91xx_mode *mode = &ms91xx->custom_modes[i].mode;
+
+		drm_info(&ms91xx->drm,
+			 "custom mode %dx%d@%d uses mode 0x%02x\n",
+			 mode->width, mode->height, mode->hz, mode->mode);
+	}
+
+	usbdev = interface_to_usbdev(interface);
+	ms91xx->bulk_pipe =
+		usb_sndbulkpipe(usbdev, MS91XX_BULK_OUT_ENDPOINT);
+	dev = &ms91xx->drm;
+
+	dma_dev = usb_intf_get_dma_device(interface);
+	if (dma_dev) {
+		drm_dev_set_dma_dev(dev, dma_dev);
+		put_device(dma_dev);
+	} else {
+		drm_warn(dev,
+			 "buffer sharing not supported\n"); /* not an error */
+	}
+	ret = drmm_mode_config_init(dev);
+	if (ret)
+		return ret;
+
+	dev->mode_config.min_width = 0;
+	dev->mode_config.max_width = MS91XX_MAX_WIDTH;
+	dev->mode_config.min_height = 0;
+	dev->mode_config.max_height = MS91XX_MAX_HEIGHT;
+	dev->mode_config.funcs = &ms91xx_mode_config_funcs;
+	dev->mode_config.helper_private = &ms91xx_mode_config_helper_funcs;
+
+	/* This stops weird behavior in the device */
+	ret = ms91xx_set_resolution(ms91xx, &ms91xx_mode_list[0]);
+	if (ret)
+		return ret;
+
+	ms91xx->workqueue =
+		drmm_alloc_ordered_workqueue(dev, DRIVER_NAME, 0);
+	if (IS_ERR(ms91xx->workqueue))
+		return PTR_ERR(ms91xx->workqueue);
+
+	ret = ms91xx_init_request(ms91xx, &ms91xx->requests[0],
+				  MS91XX_MAX_TRANSFER_LEN);
+	if (ret)
+		return ret;
+
+	ret = ms91xx_init_request(ms91xx, &ms91xx->requests[1],
+				  MS91XX_MAX_TRANSFER_LEN);
+	if (ret)
+		goto err_free_request_0;
+
+	ret = drm_universal_plane_init(dev, &ms91xx->plane, 0,
+				       &ms91xx_plane_funcs,
+				       ms91xx_plane_formats,
+				       ARRAY_SIZE(ms91xx_plane_formats), NULL,
+				       DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret)
+		goto err_free_request_1;
+
+	drm_plane_helper_add(&ms91xx->plane, &ms91xx_plane_helper_funcs);
+	drm_plane_enable_fb_damage_clips(&ms91xx->plane);
+	ms91xx_clear_rect(&ms91xx->update_rect);
+
+	ret = drm_crtc_init_with_planes(dev, &ms91xx->crtc, &ms91xx->plane,
+					NULL, &ms91xx_crtc_funcs, NULL);
+	if (ret)
+		goto err_free_request_1;
+
+	drm_crtc_helper_add(&ms91xx->crtc, &ms91xx_crtc_helper_funcs);
+
+	ret = ms91xx_connector_init(ms91xx);
+	if (ret)
+		goto err_free_request_1;
+
+	drm_mode_config_reset(dev);
+
+	usb_set_intfdata(interface, ms91xx);
+
+	drmm_kms_helper_poll_init(dev);
+
+	ret = drm_dev_register(dev, 0);
+	if (ret)
+		goto err_free_request_1;
+
+	drm_client_setup(dev, NULL);
+
+	return 0;
+
+err_free_request_1:
+	ms91xx_free_request(&ms91xx->requests[1]);
+err_free_request_0:
+	ms91xx_free_request(&ms91xx->requests[0]);
+	return ret;
+}
+
+static void ms91xx_usb_disconnect(struct usb_interface *interface)
+{
+	struct ms91xx_device *ms91xx = usb_get_intfdata(interface);
+	struct drm_device *dev = &ms91xx->drm;
+
+	drm_dev_unplug(dev);
+	drm_atomic_helper_shutdown(dev);
+	ms91xx_cancel_transfer_work(ms91xx);
+	ms91xx_free_request(&ms91xx->requests[0]);
+	ms91xx_free_request(&ms91xx->requests[1]);
+}
+
+static void ms91xx_usb_shutdown(struct usb_interface *interface)
+{
+	struct ms91xx_device *ms91xx = usb_get_intfdata(interface);
+
+	drm_atomic_helper_shutdown(&ms91xx->drm);
+}
+
+static const struct usb_device_id id_table[] = {
+	/* USB 2 */
+	{ USB_DEVICE_AND_INTERFACE_INFO(0x534d, 0x6021, 0xff, 0x00, 0x00) },
+	/* USB 2 Sometimes this PID will pop up */
+	{ USB_DEVICE_AND_INTERFACE_INFO(0x534d, 0x0821, 0xff, 0x00, 0x00) },
+	/* USB 3 */
+	{ USB_DEVICE_AND_INTERFACE_INFO(0x345f, 0x9132, 0xff, 0x00, 0x00) },
+	{ USB_DEVICE_AND_INTERFACE_INFO(0x345f, 0x9133, 0xff, 0x00, 0x00) },
+	{ USB_DEVICE_AND_INTERFACE_INFO(0x345f, 0x9135, 0xff, 0x00, 0x00) },
+	{ }
+};
+MODULE_DEVICE_TABLE(usb, id_table);
+
+static struct usb_driver ms91xx_driver = {
+	.name = DRIVER_NAME,
+	.probe = ms91xx_usb_probe,
+	.disconnect = ms91xx_usb_disconnect,
+	.suspend = ms91xx_usb_suspend,
+	.resume = ms91xx_usb_resume,
+	.reset_resume = ms91xx_usb_resume,
+	.shutdown = ms91xx_usb_shutdown,
+	.id_table = id_table,
+};
+module_usb_driver(ms91xx_driver);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/ms91xx/ms91xx_transfer.c b/drivers/gpu/drm/ms91xx/ms91xx_transfer.c
new file mode 100644
index 000000000..0e26d5d7a
--- /dev/null
+++ b/drivers/gpu/drm/ms91xx/ms91xx_transfer.c
@@ -0,0 +1,222 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022-2026 Ho Jie Feng <hjf3108@gmail.com>
+ */
+
+#include <asm/byteorder.h>
+#include <linux/align.h>
+#include <linux/completion.h>
+#include <linux/container_of.h>
+#include <linux/dma-direction.h>
+#include <linux/errno.h>
+#include <linux/gfp.h>
+#include <linux/highmem.h>
+#include <linux/iosys-map.h>
+#include <linux/jiffies.h>
+#include <linux/math.h>
+#include <linux/minmax.h>
+#include <linux/mm.h>
+#include <linux/scatterlist.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/timer.h>
+#include <linux/types.h>
+#include <linux/unaligned.h>
+#include <linux/usb.h>
+#include <linux/vmalloc.h>
+#include <linux/workqueue.h>
+
+#include <drm/drm_drv.h>
+#include <drm/drm_format_helper.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_print.h>
+#include <drm/drm_rect.h>
+
+#include "ms91xx.h"
+
+static void ms91xx_request_timeout(struct timer_list *t)
+{
+	struct ms91xx_usb_request *request = timer_container_of(request, t, timer);
+
+	usb_sg_cancel(&request->sgr);
+}
+
+static void ms91xx_request_work(struct work_struct *work)
+{
+	struct ms91xx_usb_request *request =
+		container_of(work, struct ms91xx_usb_request, work);
+	struct ms91xx_device *ms91xx = request->ms91xx;
+	struct usb_device *usbdev = interface_to_usbdev(ms91xx->intf);
+	struct usb_sg_request *sgr = &request->sgr;
+	struct sg_table *transfer_sgt = &request->transfer_sgt;
+	int idx, ret;
+
+	if (!drm_dev_enter(&ms91xx->drm, &idx)) {
+		ret = -ENODEV;
+		goto complete;
+	}
+
+	ret = usb_sg_init(sgr, usbdev, ms91xx->bulk_pipe, 0, transfer_sgt->sgl,
+			  transfer_sgt->nents, request->transfer_len,
+			  GFP_KERNEL);
+	if (ret)
+		goto dev_exit;
+
+	mod_timer(&request->timer, jiffies + msecs_to_jiffies(5000));
+	usb_sg_wait(sgr);
+
+	if (!timer_delete_sync(&request->timer))
+		ret = -ETIMEDOUT;
+	else if (sgr->status < 0)
+		ret = sgr->status;
+	else if (sgr->bytes != request->transfer_len)
+		ret = -EIO;
+
+dev_exit:
+	drm_dev_exit(idx);
+complete:
+	if (ret < 0 && ret != -ENODEV)
+		drm_err_ratelimited(&ms91xx->drm,
+				    "failed to send framebuffer: %d\n", ret);
+	complete(&request->done);
+}
+
+void ms91xx_free_request(struct ms91xx_usb_request *request)
+{
+	if (!request->transfer_buffer)
+		return;
+
+	timer_shutdown_sync(&request->timer);
+	sg_free_table(&request->transfer_sgt);
+	vfree(request->transfer_buffer);
+	request->transfer_buffer = NULL;
+}
+
+int ms91xx_init_request(struct ms91xx_device *ms91xx,
+			struct ms91xx_usb_request *request, size_t len)
+{
+	int ret;
+	unsigned int i, num_pages;
+	void *data;
+	struct page **pages;
+	void *ptr;
+
+	data = vmalloc_32(len);
+	if (!data)
+		return -ENOMEM;
+
+	num_pages = DIV_ROUND_UP(len, PAGE_SIZE);
+	pages = kmalloc_array(num_pages, sizeof(struct page *), GFP_KERNEL);
+	if (!pages) {
+		ret = -ENOMEM;
+		goto err_vfree;
+	}
+
+	for (i = 0, ptr = data; i < num_pages; i++, ptr += PAGE_SIZE)
+		pages[i] = vmalloc_to_page(ptr);
+	ret = sg_alloc_table_from_pages(&request->transfer_sgt, pages,
+					num_pages, 0, len, GFP_KERNEL);
+	kfree(pages);
+	if (ret)
+		goto err_vfree;
+
+	request->transfer_buffer = data;
+	request->ms91xx = ms91xx;
+
+	init_completion(&request->done);
+	complete(&request->done);
+	timer_setup(&request->timer, ms91xx_request_timeout, 0);
+	INIT_WORK(&request->work, ms91xx_request_work);
+	return 0;
+
+err_vfree:
+	vfree(data);
+	return ret;
+}
+
+static const u8 ms91xx_end_of_buffer[] = { 0xff, 0xc0, 0x00, 0x00,
+				    0x00, 0x00, 0x00, 0x00 };
+
+static void ms91xx_fb_xrgb8888_to_yuv422(void *dst,
+					 const struct iosys_map *src,
+					 struct drm_framebuffer *fb,
+					 const struct drm_rect *rect,
+					 struct drm_format_conv_state *fmtcnv_state)
+{
+	struct ms91xx_frame_update_header *header = dst;
+	struct iosys_map dst_map;
+	u32 position, dimensions;
+	int x, y1, width;
+
+	y1 = rect->y1;
+	x = rect->x1;
+	width = drm_rect_width(rect);
+
+	header->marker = cpu_to_be16(0xff00);
+	position = ((x & 0xfff) << 12) | (y1 & 0xfff);
+	dimensions = ((width & 0xfff) << 12) |
+		     (drm_rect_height(rect) & 0xfff);
+	put_unaligned_be24(position, header->position);
+	put_unaligned_be24(dimensions, header->dimensions);
+	dst += sizeof(*header);
+
+	iosys_map_set_vaddr(&dst_map, dst);
+	drm_fb_xrgb8888_to_uyvy(&dst_map, NULL, src, fb, rect, fmtcnv_state);
+	dst += width * drm_rect_height(rect) * 2;
+
+	memcpy(dst, ms91xx_end_of_buffer, sizeof(ms91xx_end_of_buffer));
+}
+
+int ms91xx_fb_send_rect(struct drm_framebuffer *fb, const struct iosys_map *map,
+			struct drm_format_conv_state *fmtcnv_state,
+			struct drm_rect *rect)
+{
+	int ret = 0, idx;
+	struct ms91xx_device *ms91xx = to_ms91xx(fb->dev);
+	struct drm_device *drm = &ms91xx->drm;
+	struct ms91xx_usb_request *current_request;
+	int x, width;
+
+	/* UYVY stores pixels in pairs. Expand damage to a complete pair. */
+	x = rect->x1;
+	width = ALIGN(min_t(int, rect->x2, fb->width) - x, 2);
+	if (x + width > fb->width)
+		width -= 2;
+	rect->x2 = x + width;
+	rect->y2 = min_t(unsigned int, rect->y2, fb->height);
+	current_request = &ms91xx->requests[ms91xx->current_request];
+
+	if (!drm_dev_enter(drm, &idx))
+		return -ENODEV;
+
+	/* Transfer buffer still in use, drop this frame. */
+	if (!wait_for_completion_timeout(&current_request->done,
+					 msecs_to_jiffies(10))) {
+		ret = -ETIMEDOUT;
+		goto dev_exit;
+	}
+
+	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+	if (ret < 0)
+		goto request_complete;
+
+	ms91xx_fb_xrgb8888_to_yuv422(current_request->transfer_buffer,
+				     map, fb, rect, fmtcnv_state);
+	current_request->transfer_len =
+		width * 2 * drm_rect_height(rect) + MS91XX_FRAME_OVERHEAD;
+	flush_kernel_vmap_range(current_request->transfer_buffer,
+				current_request->transfer_len);
+
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+
+	queue_work(ms91xx->workqueue, &current_request->work);
+	ms91xx->current_request = 1 - ms91xx->current_request;
+	goto dev_exit;
+
+request_complete:
+	complete(&current_request->done);
+dev_exit:
+	drm_dev_exit(idx);
+	return ret;
+}
-- 
2.55.0

Re: [PATCH v2 3/4] drm/ms91xx: Add atomic modesetting and USB framebuffer transfers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/ms91xx: lost damage rects in ms91xx_plane_atomic_update()
- [High] drm/ms91xx: unaligned odd x1 and dropped edge damage in ms91xx_fb_send_rect()
- [Medium] drm/ms91xx: devm vs drmm lifetime mismatch for ctrl_lock in ms91xx_usb_probe()
--

commit 1cde8302b787ba6feaf5820dd66da0d09c56074d
Author: Ho Jie Feng <hjf3108@gmail.com>

drm/ms91xx: Add atomic modesetting and USB framebuffer transfers

This commit adds the atomic display pipeline, USB driver probe/disconnect
handling, and double-buffered USB framebuffer transfers with XRGB8888 to
UYVY conversion for MacroSilicon ms91xx display adapters.

> diff --git a/drivers/gpu/drm/ms91xx/ms91xx_drv.c b/drivers/gpu/drm/ms91xx/ms91xx_drv.c
> new file mode 100644
> index 0000000000000..7b8d1f1e30e9d
> --- /dev/null
> +++ b/drivers/gpu/drm/ms91xx/ms91xx_drv.c

[ ... ]

> +static void ms91xx_plane_atomic_update(struct drm_plane *plane,
> +				       struct drm_atomic_commit *state)
> +{
> +	struct drm_plane_state *old_plane_state;
> +	struct drm_plane_state *new_plane_state;
> +	struct drm_crtc_state *new_crtc_state;
> +	struct drm_shadow_plane_state *shadow_plane_state;
> +	struct ms91xx_device *ms91xx = to_ms91xx(plane->dev);
> +	struct ms91xx_usb_request *current_request, *previous_request;
> +	struct drm_rect current_rect, rect;
> +
> +	old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> +	new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> +
> +	if (!new_plane_state->fb)
> +		return;
> +	new_crtc_state = drm_atomic_get_new_crtc_state(state,
> +						       new_plane_state->crtc);
> +	shadow_plane_state = to_drm_shadow_plane_state(new_plane_state);
> +
> +	/* Damage from the previous mode may not fit the new framebuffer. */
> +	if (drm_atomic_crtc_needs_modeset(new_crtc_state))
> +		ms91xx_clear_rect(&ms91xx->update_rect);
> +
> +	if (drm_atomic_helper_damage_merged(old_plane_state, new_plane_state,
> +					    &current_rect)) {
> +		current_request = &ms91xx->requests[ms91xx->current_request];
> +		previous_request = &ms91xx->requests[!ms91xx->current_request];
> +
> +		/* Latency reduction: requeue with the latest frame data. */
> +		if (cancel_work(&previous_request->work)) {
> +			complete(&previous_request->done);
> +			ms91xx_merge_rects(&current_rect, &current_rect,
> +					   &previous_request->transfer_rect);
> +			current_request = previous_request;
> +			ms91xx->current_request = !ms91xx->current_request;
> +		}
> +
> +		current_request->transfer_rect = current_rect;
> +
> +		/*
> +		 * The device double buffers, so we need to send the update
> +		 * rects of the last two frames.
> +		 */
> +		ms91xx_merge_rects(&rect, &current_rect, &ms91xx->update_rect);
> +		if (ms91xx_fb_send_rect(new_plane_state->fb,
> +					&shadow_plane_state->data[0],
> +					&shadow_plane_state->fmtcnv_state,
> +					&rect)) {
> +			/* In case of error, merge the rects to update later */
> +			ms91xx_merge_rects(&ms91xx->update_rect,
> +					   &ms91xx->update_rect, &rect);
> +		} else {
> +			ms91xx->update_rect = current_rect;
> +		}
> +	}
> +}

[Severity: High]
Can ms91xx_plane_atomic_update() drop damage rectangles from one of the
device's two hardware framebuffers when a queued work item is cancelled or
when ms91xx_fb_send_rect() fails?

Because the hardware double-buffers frame updates, each damage rectangle must
be sent in two consecutive completed USB transfers:

1. Frame 1 (damage R1) sets requests[0].transfer_rect = R1, queues
   requests[0].work (which starts executing usb_sg_wait() to update hardware
   buffer A), flips ms91xx->current_request to 1, and sets
   ms91xx->update_rect = R1.

2. Frame 2 (damage R2) arrives while requests[0].work is still running.
   cancel_work(&requests[0].work) returns false. requests[1].transfer_rect is
   set to R2, rect is computed as R2 union R1, requests[1].work is queued
   behind requests[0].work on the ordered workqueue, ms91xx->current_request
   becomes 0, and ms91xx->update_rect is overwritten with R2.

3. Frame 3 (damage R3) arrives while requests[0].work is still running and
   requests[1].work is still pending. cancel_work(&previous_request->work)
   cancels requests[1].work before R2 union R1 is sent to hardware buffer B.
   ms91xx_plane_atomic_update() merges current_rect (R3) with
   previous_request->transfer_rect (R2) and then merges that with
   ms91xx->update_rect (which is also R2). R1 is lost and never sent to
   hardware buffer B.

In addition, current_request->transfer_rect = current_rect is written before
ms91xx_fb_send_rect() waits on wait_for_completion_timeout(&current_request->done, ...),
so a timeout overwrites the in-flight request's transfer_rect. And when
ms91xx_fb_send_rect() fails, merging rect into ms91xx->update_rect only sends
that failed frame's damage in the single next transfer before
ms91xx->update_rect = current_rect overwrites it, leaving the second hardware
buffer stale on the following frame.

[ ... ]

> +static int ms91xx_usb_probe(struct usb_interface *interface,
> +			    const struct usb_device_id *id)
> +{
> +	int ret;
> +	unsigned int i;
> +	struct ms91xx_device *ms91xx;
> +	struct drm_device *dev;
> +	struct device *dma_dev;
> +	struct usb_device *usbdev;
> +
> +	ms91xx = devm_drm_dev_alloc(&interface->dev, &ms91xx_drm_driver,
> +				    struct ms91xx_device, drm);
> +	if (IS_ERR(ms91xx))
> +		return PTR_ERR(ms91xx);
> +
> +	ms91xx->intf = interface;
> +	ret = devm_mutex_init(&interface->dev, &ms91xx->ctrl_lock);
> +	if (ret)
> +		return ret;

[ ... ]

> +	drmm_kms_helper_poll_init(dev);
> +
> +	ret = drm_dev_register(dev, 0);
> +	if (ret)
> +		goto err_free_request_1;

[Severity: Medium]
Should ms91xx->ctrl_lock be initialized with drmm_mutex_init() instead of
devm_mutex_init() in ms91xx_usb_probe()?

In ms91xx_usb_probe(), devm_drm_dev_alloc(&interface->dev, ...) is registered
first on interface->dev, followed by devm_mutex_init(&interface->dev,
&ms91xx->ctrl_lock). Later, drmm_kms_helper_poll_init(dev) starts
dev->mode_config.output_poll_work, whose cleanup is tied to the drm_device
via drm_managed_release().

If drm_dev_register(dev, 0) fails, devres unwinds interface->dev in reverse
order: devm_mutex_release destroys ms91xx->ctrl_lock before
devm_drm_dev_init_release runs drm_dev_put() -> drm_managed_release() ->
drm_kms_helper_poll_init_release() to cancel output_poll_work. If
output_poll_work runs ms91xx_detect() -> ms91xx_read_byte() during that
window, it will lock ms91xx->ctrl_lock after it has already been destroyed.

> diff --git a/drivers/gpu/drm/ms91xx/ms91xx_transfer.c b/drivers/gpu/drm/ms91xx/ms91xx_transfer.c
> new file mode 100644
> index 0000000000000..0e26d5d7aab97
> --- /dev/null
> +++ b/drivers/gpu/drm/ms91xx/ms91xx_transfer.c

[ ... ]

> +static void ms91xx_fb_xrgb8888_to_yuv422(void *dst,
> +					 const struct iosys_map *src,
> +					 struct drm_framebuffer *fb,
> +					 const struct drm_rect *rect,
> +					 struct drm_format_conv_state *fmtcnv_state)
> +{
> +	struct ms91xx_frame_update_header *header = dst;
> +	struct iosys_map dst_map;
> +	u32 position, dimensions;
> +	int x, y1, width;
> +
> +	y1 = rect->y1;
> +	x = rect->x1;
> +	width = drm_rect_width(rect);
> +
> +	header->marker = cpu_to_be16(0xff00);
> +	position = ((x & 0xfff) << 12) | (y1 & 0xfff);
> +	dimensions = ((width & 0xfff) << 12) |
> +		     (drm_rect_height(rect) & 0xfff);
> +	put_unaligned_be24(position, header->position);
> +	put_unaligned_be24(dimensions, header->dimensions);
> +	dst += sizeof(*header);
> +
> +	iosys_map_set_vaddr(&dst_map, dst);
> +	drm_fb_xrgb8888_to_uyvy(&dst_map, NULL, src, fb, rect, fmtcnv_state);
> +	dst += width * drm_rect_height(rect) * 2;
> +
> +	memcpy(dst, ms91xx_end_of_buffer, sizeof(ms91xx_end_of_buffer));
> +}
> +
> +int ms91xx_fb_send_rect(struct drm_framebuffer *fb, const struct iosys_map *map,
> +			struct drm_format_conv_state *fmtcnv_state,
> +			struct drm_rect *rect)
> +{
> +	int ret = 0, idx;
> +	struct ms91xx_device *ms91xx = to_ms91xx(fb->dev);
> +	struct drm_device *drm = &ms91xx->drm;
> +	struct ms91xx_usb_request *current_request;
> +	int x, width;
> +
> +	/* UYVY stores pixels in pairs. Expand damage to a complete pair. */
> +	x = rect->x1;
> +	width = ALIGN(min_t(int, rect->x2, fb->width) - x, 2);
> +	if (x + width > fb->width)
> +		width -= 2;
> +	rect->x2 = x + width;

[Severity: High]
What happens in ms91xx_fb_send_rect() when rect->x1 is odd?

UYVY packs horizontal pixel pairs (2k, 2k + 1) into 4-byte macropixels, and
drm_fb_xrgb8888_to_uyvy() requires clip->x1 and clip->x2 to be even.

In ms91xx_fb_send_rect(), x = rect->x1 is not aligned down to an even pixel
boundary, and only width is aligned to a multiple of 2. When rect->x1 is odd:

1. Both rect->x1 and rect->x2 remain odd when passed to
   ms91xx_fb_xrgb8888_to_yuv422() -> drm_fb_xrgb8888_to_uyvy(), pairing
   misaligned pixels (2k + 1, 2k + 2) instead of (2k, 2k + 1).

2. When the damage clip reaches the right edge of the framebuffer
   (rect->x2 == fb->width, where fb->width is even), fb->width - x is odd, so
   ALIGN(fb->width - x, 2) rounds up to fb->width - x + 1 and x + width
   becomes fb->width + 1. The check:

	if (x + width > fb->width)
		width -= 2;

   then subtracts 2 from width, setting rect->x2 = fb->width - 1 and dropping
   the rightmost pixel column. In particular, if a 1-pixel damage clip is at
   the right edge (rect->x1 == fb->width - 1, rect->x2 == fb->width), width
   becomes 0 and a 0-width frame update is queued over USB.

Should rect->x1 be rounded down to an even coordinate (for example with
round_down(rect->x1, 2)) and rect->x2 be aligned up with ALIGN(min_t(int,
rect->x2, fb->width), 2)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104151652.59135-1-hjf3108@gmail.com?part=3

[PATCH v2 4/4] drm/ms91xx: Add Kconfig and Makefile integration

[Ho Jie Feng]

Expose the MS91xx USB display driver in the DRM build and register its maintainer.

Signed-off-by: Ho Jie Feng <hjf3108@gmail.com>
---
 MAINTAINERS                     |  7 +++++++
 drivers/gpu/drm/Kconfig         |  1 +
 drivers/gpu/drm/Makefile        |  1 +
 drivers/gpu/drm/ms91xx/Kconfig  | 13 +++++++++++++
 drivers/gpu/drm/ms91xx/Makefile |  9 +++++++++
 5 files changed, 31 insertions(+)
 create mode 100644 drivers/gpu/drm/ms91xx/Kconfig
 create mode 100644 drivers/gpu/drm/ms91xx/Makefile

diff --git a/MAINTAINERS b/MAINTAINERS
index 3951f85d2..85d4e7a3a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16034,6 +16034,13 @@ F:	drivers/net/wireless/virtual/mac80211_hwsim*.[ch]
 F:	include/net/mac80211.h
 F:	net/mac80211/
 
+MACROSILICON MS91XX DRM DRIVER
+M:	Ho Jie Feng <hjf3108@gmail.com>
+L:	dri-devel@lists.freedesktop.org
+S:	Maintained
+T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
+F:	drivers/gpu/drm/ms91xx/
+
 MAILBOX API
 M:	Jassi Brar <jassisinghbrar@gmail.com>
 L:	linux-kernel@vger.kernel.org
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 61111d50b..93d73c7b9 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -330,6 +330,7 @@ source "drivers/gpu/drm/mcde/Kconfig"
 source "drivers/gpu/drm/mediatek/Kconfig"
 source "drivers/gpu/drm/meson/Kconfig"
 source "drivers/gpu/drm/mgag200/Kconfig"
+source "drivers/gpu/drm/ms91xx/Kconfig"
 source "drivers/gpu/drm/msm/Kconfig"
 source "drivers/gpu/drm/mxsfb/Kconfig"
 source "drivers/gpu/drm/nouveau/Kconfig"
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 2c6bc093a..cf83e3104 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -180,6 +180,7 @@ obj-$(CONFIG_DRM_I915)	+= i915/
 obj-$(CONFIG_DRM_XE)	+= xe/
 obj-$(CONFIG_DRM_KMB_DISPLAY)  += kmb/
 obj-$(CONFIG_DRM_MGAG200) += mgag200/
+obj-$(CONFIG_DRM_MS91XX) += ms91xx/
 obj-$(CONFIG_DRM_V3D)  += v3d/
 obj-$(CONFIG_DRM_VC4)  += vc4/
 obj-$(CONFIG_DRM_VMWGFX)+= vmwgfx/
diff --git a/drivers/gpu/drm/ms91xx/Kconfig b/drivers/gpu/drm/ms91xx/Kconfig
new file mode 100644
index 000000000..526aaa2ee
--- /dev/null
+++ b/drivers/gpu/drm/ms91xx/Kconfig
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+config DRM_MS91XX
+	tristate "MacroSilicon MS912x/MS913x USB display adapters"
+	depends on DRM && USB && MMU
+	select DRM_CLIENT_SELECTION
+	select DRM_GEM_SHMEM_HELPER
+	select DRM_KMS_HELPER
+	help
+	  This is a KMS driver for MacroSilicon MS912x/MS913x USB display
+	  adapters with VGA or HDMI outputs.
+
+	  If M is selected the module will be called ms91xx.
diff --git a/drivers/gpu/drm/ms91xx/Makefile b/drivers/gpu/drm/ms91xx/Makefile
new file mode 100644
index 000000000..98dac2602
--- /dev/null
+++ b/drivers/gpu/drm/ms91xx/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+ms91xx-y := \
+	ms91xx_registers.o \
+	ms91xx_connector.o \
+	ms91xx_transfer.o \
+	ms91xx_drv.o
+
+obj-$(CONFIG_DRM_MS91XX) := ms91xx.o
-- 
2.55.0

Re: [PATCH v2 4/4] drm/ms91xx: Add Kconfig and Makefile integration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] drm/ms91xx: missing rect->x1 even alignment in ms91xx_fb_send_rect()
- [Medium] drm/ms91xx: lost frame damage on cancel_work() in ms91xx_plane_atomic_update()
- [Medium] drm/ms91xx: devm_mutex_init() destroys ctrl_lock before drmm poll teardown
--

commit 1cde8302b787ba6feaf5820dd66da0d09c56074d
Author: Ho Jie Feng <hjf3108@gmail.com>

drm/ms91xx: Add atomic modesetting and USB framebuffer transfers

This commit adds the atomic modesetting pipeline, double-buffered USB
framebuffer transfer handling, and USB probe/disconnect lifecycle for
MacroSilicon MS912x/MS913x display adapters.

> diff --git a/drivers/gpu/drm/ms91xx/ms91xx_drv.c b/drivers/gpu/drm/ms91xx/ms91xx_drv.c
> new file mode 100644
> index 0000000000000..7b8d1f1e30e9d
> --- /dev/null
> +++ b/drivers/gpu/drm/ms91xx/ms91xx_drv.c
[ ... ]
> +static void ms91xx_plane_atomic_update(struct drm_plane *plane,
> +				       struct drm_atomic_commit *state)
> +{
[ ... ]
> +	if (drm_atomic_helper_damage_merged(old_plane_state, new_plane_state,
> +					    &current_rect)) {
> +		current_request = &ms91xx->requests[ms91xx->current_request];
> +		previous_request = &ms91xx->requests[!ms91xx->current_request];
> +
> +		/* Latency reduction: requeue with the latest frame data. */
> +		if (cancel_work(&previous_request->work)) {
> +			complete(&previous_request->done);
> +			ms91xx_merge_rects(&current_rect, &current_rect,
> +					   &previous_request->transfer_rect);
> +			current_request = previous_request;
> +			ms91xx->current_request = !ms91xx->current_request;
> +		}
> +
> +		current_request->transfer_rect = current_rect;
> +
> +		/*
> +		 * The device double buffers, so we need to send the update
> +		 * rects of the last two frames.
> +		 */
> +		ms91xx_merge_rects(&rect, &current_rect, &ms91xx->update_rect);
> +		if (ms91xx_fb_send_rect(new_plane_state->fb,
> +					&shadow_plane_state->data[0],
> +					&shadow_plane_state->fmtcnv_state,
> +					&rect)) {
> +			/* In case of error, merge the rects to update later */
> +			ms91xx_merge_rects(&ms91xx->update_rect,
> +					   &ms91xx->update_rect, &rect);
> +		} else {
> +			ms91xx->update_rect = current_rect;
> +		}
> +	}
> +}

[Severity: Medium]
Can cancelling a pending work item here lose the damage rectangle from the
earlier in-flight frame on a double-buffered device?

Consider three consecutive atomic updates arriving while the first transfer is
still running usb_sg_wait() on the ordered workqueue:

1. Frame 1 (damage R1) queues requests[0].work with transfer_rect = R1 and
   sets ms91xx->update_rect = R1. requests[0].work starts executing on the
   workqueue.

2. Frame 2 (damage R2) arrives while requests[0].work is still running.
   cancel_work(&requests[0].work) returns false, so requests[1].work is
   queued behind requests[0].work with transfer_rect = R2, and
   ms91xx->update_rect is overwritten with R2.

3. Frame 3 (damage R3) arrives while requests[0].work is still running and
   requests[1].work is still pending. cancel_work(&requests[1].work) returns
   true and cancels Frame 2 before it is sent to the device.

At that point, ms91xx_plane_atomic_update() merges requests[1].transfer_rect
(R2) into current_rect (producing R2 union R3), and then merges current_rect
with ms91xx->update_rect. Because ms91xx->update_rect was already overwritten
with R2 during Frame 2, R1 (which was only written to the first hardware
buffer by requests[0]) is never included in the transfer for the second
hardware buffer, leaving that buffer with stale pixels in R1.

Should ms91xx->update_rect retain the damage of the last frame that was not
cancelled, or also merge the in-flight request's transfer_rect when a queued
request is cancelled?

[ ... ]
> +static int ms91xx_usb_probe(struct usb_interface *interface,
> +			    const struct usb_device_id *id)
> +{
> +	int ret;
> +	unsigned int i;
> +	struct ms91xx_device *ms91xx;
> +	struct drm_device *dev;
> +	struct device *dma_dev;
> +	struct usb_device *usbdev;
> +
> +	ms91xx = devm_drm_dev_alloc(&interface->dev, &ms91xx_drm_driver,
> +				    struct ms91xx_device, drm);
> +	if (IS_ERR(ms91xx))
> +		return PTR_ERR(ms91xx);
> +
> +	ms91xx->intf = interface;
> +	ret = devm_mutex_init(&interface->dev, &ms91xx->ctrl_lock);
> +	if (ret)
> +		return ret;
[ ... ]
> +	drmm_kms_helper_poll_init(dev);
> +
> +	ret = drm_dev_register(dev, 0);
> +	if (ret)
> +		goto err_free_request_1;

[Severity: Medium]
Does initializing ms91xx->ctrl_lock with devm_mutex_init() after
devm_drm_dev_alloc() allow ctrl_lock to be destroyed before the DRM output
poll worker is stopped?

In ms91xx_usb_probe(), devm_drm_dev_alloc() registers its devres release
action on interface->dev first, and devm_mutex_init() registers the mutex
destruction action on interface->dev second. Later in ms91xx_usb_probe(),
drmm_kms_helper_poll_init(dev) starts output_poll_work and registers
drm_kms_helper_poll_fini() as a DRM-managed (drmm) cleanup action.

Because devres actions run in reverse registration order, if
drm_dev_register() fails (or during device teardown before the final
drm_dev_put()), devres destroys ms91xx->ctrl_lock before
devm_drm_dev_init_release() invokes drm_managed_release() and
drm_kms_helper_poll_fini(). While output_poll_work is still active,
output_poll_execute() -> ms91xx_detect() -> ms91xx_read_byte() can lock
ms91xx->ctrl_lock after it has been destroyed.

Should ms91xx->ctrl_lock be initialized with drmm_mutex_init(dev,
&ms91xx->ctrl_lock) instead so its lifetime is tied to the DRM device and
ordered ahead of drmm_kms_helper_poll_init(dev)?

> diff --git a/drivers/gpu/drm/ms91xx/ms91xx_transfer.c b/drivers/gpu/drm/ms91xx/ms91xx_transfer.c
> new file mode 100644
> index 0000000000000..0e26d5d7aab97
> --- /dev/null
> +++ b/drivers/gpu/drm/ms91xx/ms91xx_transfer.c
[ ... ]
> +int ms91xx_fb_send_rect(struct drm_framebuffer *fb, const struct iosys_map *map,
> +			struct drm_format_conv_state *fmtcnv_state,
> +			struct drm_rect *rect)
> +{
> +	int ret = 0, idx;
> +	struct ms91xx_device *ms91xx = to_ms91xx(fb->dev);
> +	struct drm_device *drm = &ms91xx->drm;
> +	struct ms91xx_usb_request *current_request;
> +	int x, width;
> +
> +	/* UYVY stores pixels in pairs. Expand damage to a complete pair. */
> +	x = rect->x1;
> +	width = ALIGN(min_t(int, rect->x2, fb->width) - x, 2);
> +	if (x + width > fb->width)
> +		width -= 2;
> +	rect->x2 = x + width;

[Severity: Medium]
What happens in ms91xx_fb_send_rect() when userspace submits a damage clip
where rect->x1 is odd?

UYVY packs horizontal pixel pairs at even-odd coordinates (2k, 2k + 1), and
drm_fb_xrgb8888_to_uyvy() requires both clip->x1 and clip->x2 to be even.
Here, x is assigned rect->x1 without rounding down to an even pixel boundary,
and width is aligned to a multiple of 2, so both rect->x1 and rect->x2 remain
odd when rect->x1 is odd.

In addition, if rect->x1 is odd and rect->x2 reaches fb->width (which is even
for all supported modes), fb->width - x is odd, so ALIGN(fb->width - x, 2)
produces fb->width - x + 1. The check:

	if (x + width > fb->width)
		width -= 2;

then subtracts 2 from width, setting rect->x2 = fb->width - 1 and dropping the
rightmost column of damaged pixels. In the case of a 1-pixel-wide damage clip
at the right edge of the framebuffer (rect->x1 == fb->width - 1,
rect->x2 == fb->width), width becomes 0 and the pixel update is lost while a
0-width transfer is queued.

Should rect->x1 be rounded down to an even coordinate first (for example with
ALIGN_DOWN(rect->x1, 2)) before computing width and rect->x2?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104151652.59135-1-hjf3108@gmail.com?part=4