Re: [PATCH v6] drm/tiny: trigger5: Add MCT Trigger 5 USB display driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] drm/tiny: trigger5: broken USB reset lifecycle in
  trigger5_reset()
- [Medium] drm/tiny: trigger5: unclipped src_rect in
  trigger5_plane_atomic_update()
- [Medium] drm/tiny: trigger5: bulk stream desync on mid-frame
  trigger5_submit_urb() error
--

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

drm/tiny: trigger5: Add MCT Trigger 5 USB display driver

This commit adds a DRM driver for MCT Trigger 5 USB 3 to HDMI/VGA display
adapters, implementing modesetting, EDID retrieval, and bulk URB frame
updates.

> diff --git a/drivers/gpu/drm/tiny/trigger5.c b/drivers/gpu/drm/tiny/trigger5.c
> new file mode 100644
> index 0000000000000..46fa6e6babe9c
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/trigger5.c

[ ... ]

> +static void trigger5_reset(struct trigger5_device *trigger5, int err)
> +{
> +	if (!atomic_xchg(&trigger5->needs_reset, 1))
> +		usb_queue_reset_device(trigger5->intf);
> +
> +	drm_err(&trigger5->drm, "USB transfer failed: %d, resetting adapter\n",
> +		err);
> +}
> +
> +static void trigger5_urb_complete(struct urb *urb)
> +{
> +	struct trigger5_transfer *transfer = urb->context;
> +	struct trigger5_device *trigger5 = transfer->trigger5;
> +
> +	switch (urb->status) {
> +	case -ENOENT:
> +	case -ECONNRESET:
> +	case -ESHUTDOWN:
> +		break;
> +	case 0:
> +		if (urb->actual_length == urb->transfer_buffer_length)
> +			break;
> +		fallthrough;
> +	default:
> +		trigger5_reset(trigger5, urb->status);
> +		break;
> +	}
> +
> +	complete(&transfer->done);
> +}
> +
> +static struct trigger5_transfer *
> +trigger5_get_next_urb(struct trigger5_device *trigger5)
> +{
> +	struct trigger5_transfer *transfer =
> +		&trigger5->transfers[trigger5->next_transfer];
> +
> +	if (atomic_read(&trigger5->needs_reset))
> +		return ERR_PTR(-EIO);
> +
> +	if (!wait_for_completion_timeout(&transfer->done,
> +					 msecs_to_jiffies(TRIGGER5_BULK_TIMEOUT_MS))) {
> +		trigger5_reset(trigger5, -ETIMEDOUT);
> +		return ERR_PTR(-ETIMEDOUT);
> +	}
> +
> +	trigger5->next_transfer =
> +		(trigger5->next_transfer + 1) % TRIGGER5_NUM_URBS;
> +
> +	return transfer;
> +}

[Severity: High]
Does trigger5_reset() cause the active DRM device to be permanently
unplugged or stuck returning -EIO after any USB transfer failure?

trigger5_driver does not define .pre_reset and .post_reset callbacks
(.reset_resume is only used when resuming a suspended device), so
usb_reset_device() falls back to usb_forced_unbind_intf(), which runs
trigger5_usb_disconnect() -> drm_dev_unplug() and kills the active DRM
device instead of resetting the adapter in place.

In addition, nothing in the driver ever clears trigger5->needs_reset once
trigger5_reset() sets it to 1, so if the queued reset does not unbind the
device (for example if usb_lock_device_for_reset() fails or after a
reset_resume), trigger5_get_next_urb() rejects all future display updates
with -EIO.

Two smaller issues in this path: trigger5_get_next_urb() only checks
trigger5->needs_reset before wait_for_completion_timeout(), so if the URB
being waited on completes with an error it still returns the transfer to
the caller; and on a short transfer in trigger5_urb_complete(), falling
through from case 0 passes urb->status (0) to trigger5_reset().

[ ... ]

> +static void trigger5_plane_atomic_update(struct drm_plane *plane,
> +					 struct drm_atomic_commit *atomic_state)
> +{

[ ... ]

> +	src_rect = drm_plane_state_src(state);
> +
> +	/* Match drm_atomic_helper_damage_iter_init() rounding. */
> +	src_rect.x1 >>= 16;
> +	src_rect.y1 >>= 16;
> +	src_rect.x2 = (src_rect.x2 >> 16) + !!(src_rect.x2 & 0xffff);
> +	src_rect.y2 = (src_rect.y2 >> 16) + !!(src_rect.y2 & 0xffff);
> +
> +	/* Clip merged damage to the new resolution. */
> +	if (!drm_rect_intersect(&current_rect, &src_rect))
> +		goto exit;
> +
> +	width = drm_rect_width(&current_rect);
> +	height = drm_rect_height(&current_rect);
> +	payload_len = array3_size(width, height, 3);
> +	lines_per_urb = TRIGGER5_URB_BUF_SIZE / (width * 3);
> +
> +	if (drm_gem_fb_begin_cpu_access(state->fb, DMA_FROM_DEVICE) < 0)
> +		goto exit;
> +
> +	transfer = trigger5_get_next_urb(trigger5);
> +	if (IS_ERR(transfer))
> +		goto exit_end_cpu_access;
> +
> +	header = transfer->urb->transfer_buffer;
> +	header->magic = 0xfb;
> +	header->length = 0x14;
> +	/* flags 0: uncompressed 24-bit RGB888. */
> +	header->counter =
> +		cpu_to_le16((trigger5->frame_counter++) & 0xfff);
> +	header->horizontal_offset = cpu_to_le16(current_rect.x1 - src_rect.x1);
> +	header->vertical_offset = cpu_to_le16(current_rect.y1 - src_rect.y1);
> +	header->width = cpu_to_le16(width);
> +	header->height = cpu_to_le16(height);
> +	header->payload_length = cpu_to_le32((u32)payload_len);

[Severity: Medium]
Should src_rect come from the clipped state->src (via
drm_rect_fp_to_int(&src_rect, &state->src)) rather than the unclipped
drm_plane_state_src(state)?

In trigger5_plane_atomic_check(), drm_atomic_helper_check_plane_state()
clips state->dst to the active CRTC mode and stores the visible source
rectangle in state->src, while leaving state->src_x/y/w/h unclipped.
Because drm_atomic_helper_damage_iter_init() also initializes its plane_src
from the unclipped drm_plane_state_src(state), intersecting current_rect
with drm_plane_state_src(state) here does not clip damage to the active
CRTC mode when a larger framebuffer (up to 8191x8191) is panned with
negative crtc_x/crtc_y, and outward rounding of fractional 16.16 src
coordinates expands src_rect by an extra pixel.  This computes
horizontal_offset and vertical_offset relative to the off-screen origin and
can send frame dimensions and payload_length exceeding the active display
mode and TRIGGER5_MAX_FRAME_SIZE.

> +	header->flags = 0x1;
> +	header->unknown1 = 0;
> +	header->unknown2 = 0;
> +	header->checksum = trigger5_bulk_header_checksum(header);
> +
> +	if (trigger5_submit_urb(transfer, sizeof(*header)))
> +		goto exit_end_cpu_access;
> +
> +	clip = current_rect;
> +	for (y = current_rect.y1; y < current_rect.y2; y += lines_per_urb) {
> +		clip.y1 = y;
> +		clip.y2 = min(y + lines_per_urb, current_rect.y2);
> +
> +		transfer = trigger5_get_next_urb(trigger5);
> +		if (IS_ERR(transfer))
> +			break;
> +
> +		iosys_map_set_vaddr(&data_map, transfer->urb->transfer_buffer);
> +		drm_fb_xrgb8888_to_rgb888(&data_map, NULL,
> +					  &shadow_plane_state->data[0],
> +					  state->fb, &clip,
> +					  &shadow_plane_state->fmtcnv_state);
> +
> +		if (trigger5_submit_urb(transfer,
> +					width * 3 * drm_rect_height(&clip)))
> +			break;
> +	}

[Severity: Medium]
Does breaking out of this loop when trigger5_submit_urb() fails leave the
adapter's bulk stream out of sync?

At that point the 20-byte trigger5_bulk_header URB declaring
header->payload_length bytes has already been submitted, and unlike
trigger5_urb_complete() or trigger5_get_next_urb(), trigger5_submit_urb()
does not call trigger5_reset() on usb_submit_urb() failure.  The device
will keep waiting for the remaining bytes of the aborted frame and treat
subsequent frame headers and pixel data as part of the old payload.

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