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 <[email protected]> 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, > + ¤t_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(¤t_rect, ¤t_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, ¤t_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/[email protected]?part=4
