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

[sashiko-bot@kernel.org]

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