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

[sashiko-bot@kernel.org]

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