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