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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm: sleeping in atomic context in plane and CRTC callbacks
- [Medium] drm/trigger5: deferred display damage is never flushed if userspace goes idle
--

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

drm/trigger5: Add MCT Trigger 5 USB display driver

This commit introduces a new DRM driver for MCT Trigger 5 USB display adapters.
It implements modesetting, USB framebuffer updates, connector detection, and
EDID retrieval based on reverse-engineered protocols.

> diff --git a/drivers/gpu/drm/tiny/trigger5.c b/drivers/gpu/drm/tiny/trigger5.c
> new file mode 100644
> index 0000000000000..d0ab06bab8d73
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/trigger5.c
[ ... ]
> +static void trigger5_transfer_work(struct work_struct *work)
> +{
[ ... ]
> +	if (transfer->sgr.status)
> +		drm_err_ratelimited(&trigger5->drm,
> +				    "USB transfer failed: %d\n",
> +				    transfer->sgr.status);
> +	else if (transfer->sgr.bytes != transfer->frame_len)
> +		drm_err_ratelimited(&trigger5->drm,
> +				    "short USB transfer: %zu/%zu bytes\n",
> +				    transfer->sgr.bytes, transfer->frame_len);
> +	else if (READ_ONCE(trigger5->display_enabled))
> +		/* Keepalive must only be sent after a frame has been sent */
> +		queue_delayed_work(trigger5->transfer_wq,
> +				   &trigger5->keepalive_work,
> +				   msecs_to_jiffies(TRIGGER5_KEEPALIVE_INTERVAL_MS));
> +
> +exit:
> +	drm_dev_exit(idx);
> +complete:
> +	complete(&transfer->frame_complete);
> +}

[Severity: Medium]
Does this transfer completion need to check for or schedule any work to handle
damage deferred in pending_rect?

If userspace stops sending new frames (e.g., when the system goes idle), the
deferred damage might never get flushed to the screen.

[ ... ]
> +static void trigger5_crtc_atomic_enable(struct drm_crtc *crtc,
> +					struct drm_atomic_commit *state)
> +{
[ ... ]
> +	trigger5_stop_io(trigger5);
> +
> +	/* Sequence recovered from USB captures. */
> +	ret = usb_control_msg_recv(udev, 0,
> +				   TRIGGER5_REQUEST_FIRMWARE_RESET,
> +				   USB_DIR_IN | USB_TYPE_VENDOR |
> +					   USB_RECIP_DEVICE,
> +				   0x0000, 0x0000, data, 1,
> +				   USB_CTRL_GET_TIMEOUT, GFP_KERNEL);

[Severity: High]
Is it safe to use GFP_KERNEL and blocking USB functions here?

CRTC atomic enable callbacks are atomic context paths where sleeping is
strictly forbidden. Calling blocking USB transfers with the GFP_KERNEL flag
in this path violates DRM invariants and can cause kernel warnings or
deadlocks.

[ ... ]
> +static void trigger5_crtc_atomic_disable(struct drm_crtc *crtc,
> +					 struct drm_atomic_commit *state)
> +{
[ ... ]
> +	trigger5_stop_io(trigger5);
> +
> +	ret = usb_control_msg_recv(udev, 0,
> +				   TRIGGER5_REQUEST_FIRMWARE_RESET,
> +				   USB_DIR_IN | USB_TYPE_VENDOR |
> +					   USB_RECIP_DEVICE,
> +				   0x0001, 0x0000, &data, 1,
> +				   USB_CTRL_GET_TIMEOUT, GFP_KERNEL);

[Severity: High]
Similarly to the atomic enable callback, is it safe to use a blocking USB
function with GFP_KERNEL allocation here?

CRTC atomic disable callbacks are also atomic contexts where sleeping is
prohibited.

[ ... ]
> +static void trigger5_plane_atomic_update(struct drm_plane *plane,
> +					 struct drm_atomic_commit *atomic_state)
> +{
[ ... ]
> +	/*
> +	 * This should almost never wait because we have should have a
> +	 * pending transfer ready to be de-queued above in case the transfer
> +	 * hasn't finished, but do a bounded wait just in case it gets stuck
> +	 */
> +	if (!wait_for_completion_timeout(&current_transfer->frame_complete,
> +					 msecs_to_jiffies(20)))
> +		goto exit_save_pending;

[Severity: High]
Can wait_for_completion_timeout() sleep in this context?

Plane atomic update callbacks execute in atomic context where sleeping is
forbidden. Blocking for up to 20ms here will cause system instability.

[Severity: Medium]
When this timeout occurs, the damage is saved to pending_rect, but no
asynchronous flush is queued to present it later.

As mentioned above in trigger5_transfer_work(), if the display update aborts
here and userspace stops sending new frames, the screen might permanently
display stale content.

[ ... ]
> +	iosys_map_set_vaddr(&data_map,
> +			    current_transfer->buf.data + sizeof(*header));
> +
> +	ret = drm_gem_fb_begin_cpu_access(state->fb, DMA_FROM_DEVICE);

[Severity: High]
Could this function acquire DMA reservation locks that might sleep?

Since this is still within the plane atomic update callback, calling a
function that can sleep violates the DRM atomic context rules.

> +	if (ret < 0) {
> +		complete(&current_transfer->frame_complete);
> +		goto exit_save_pending;
> +	}
> +
> +	drm_fb_xrgb8888_to_rgb888(&data_map, NULL,
> +				  &shadow_plane_state->data[0],
> +				  state->fb, &current_rect,
> +				  &shadow_plane_state->fmtcnv_state);
> +
> +	drm_gem_fb_end_cpu_access(state->fb, DMA_FROM_DEVICE);

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