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

[Jie Feng]

On Wed, 11 Nov 2026 at 23:21, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> Hi
>
> Am 10.11.26 um 16:49 schrieb Jie Feng:
> > On Tue, 10 Nov 2026 at 22:23, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
> >> Hi,
> >>
> >> sorry for letting you wait for so long. You'll find a review below.
> > Thanks for the review! Since this driver shares many similarities
> > with the ms91xx driver I am submitting too, I will be making
> > the changes in that driver too.
>
> I've briefly seen that other driver. Let's focus on one of them first.
> The other will then be easier, I guess.
Sure, let's focus on the review here, this is a much smaller driver.
>
> >
> >>> +     ret = drm_gem_fb_begin_cpu_access(state->fb, DMA_FROM_DEVICE);
> >>> +     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);
> >>> +
> >>> +     flush_kernel_vmap_range(current_transfer->buf.data, frame_len);
> >>> +
> >>> +     queue_work(trigger5->transfer_wq, &current_transfer->transfer_work);
> >>> +     trigger5->current_transfer = !trigger5->current_transfer;
> >> Why do you need a separate work item for the transfer. Can't you just
> >> send from here?  We do this in udl.
> > I want to make the send async, so drm_fb_xrgb8888_to_rgb888 can run
> > in parallel with the usb transfer. This is done since drm_fb_*_to_*
> > functions are slow when copying on write-combined memory due to
> > dma-buf sharing between the main GPU and this device.
>
> We do this in other drivers as well. Neither is supposed to be fast.
> There's always this memcpy from the imported buffer.
>
>
> >
> > udl does usb_submit_urb, which is async, but the usb_sg_wait has no async
> > counterpart, so I opted for putting it in a worker.
>
> Why do you use usb_sg_wait() in the first place? It appears to be a loop
This part is referenced from gud. I thought it makes sense since I need a large
vmalloc buffer, so using usb_sg_* looked like the right approach instead of
asking for kmalloc(8MB).

Alternative I considered are:
- usb_alloc_noncoherent: but that requires implementing a fallback if a
  controller doesn't support scatter gather DMA, while usb_sg_* does
  handle all of these cases.
- pool of fixed kmalloc buffers: 64K seems too small, too many URBs
  for a 1080p frame, and tuning the number so that kmalloc will likely
  to succeed on a fragmented system while keeping the number
  of URBs per 1080p frame small doesn't seem like the right
  approach to me.

Are there other possible approaches to this?

> around usb_submit_urb() plus the waiting. (The comment at [1] is
> actually quite funny.
>
> [1]
> https://elixir.bootlin.com/linux/v7.3.1/source/drivers/usb/core/message.c#L761
Indeed!
> >
> > I saw there's a TODO within the format helpers for io memory, so
> > maybe in a future patch I could change it to use drm_memcpy_from_wc
> > to speed it up. Currently it couldn't run at full 1080p at 60fps on bare-metal,
> > but on a VM it is fine due to not copying from WC memory.
> >
> > There's also the other issue of some compositors calling _update
> > in the render loop or something that causes visible stutter when a device's
> > _update callback is slow.
> >
> > The ideal way to solve this, with both low latency and low memory footprint
> > is to take inspiration from gm12u320 and store the fb and update rect in
> > the device struct, and let a worker obtain the latest one from the struct.
>
> It's not. None of our USB-based drivers are well optimized. You can take
> some inspiration from them, but don't take them as they are.
>
> At the end of each page flip, DRM sends out a FLIP_COMPLETE event, [2]
> telling the compositor that it's done with the display update. This
> gives the compositor an idea about the performance of the device.  It
> can adapt to that and end out less updates. By moving out the actual
> work into a separate thread, you break that feedback. The compositor
> will see an early complete from the page flip and queue up the next
> frame immediately. The driver will pile up updates for the worker to
> send out. Hence, leave the copying and sending operations in the regular
> update code.  If you build this with usb_submit_urb(), there's no
> waiting necessary.
In that case, assuming I keep the usb_sg_wait, doing something like
wait_for_completion(previous_transfer) would be more correct.

> [2]
> https://elixir.bootlin.com/linux/v7.3.1/source/include/uapi/drm/drm.h#L1395
>
> What I have in mind for udl is this logic:
>
> 1. send the current frame's updates as usual using usb_submit_urb()
>    - transfer runs in parallel to current display update
>    - count the number of submitted urbs
> 2. in each complete handler, decrement the urb count.
> 3. in the next frame's atomic_begin, wait until the urb counter reaches zero
>    - most parallelism
>    - max out USB
>    - maintains correct transfer statistics and events
> 4. if the second frame takes longer than expected, the FLIP_COMPLETE
> event wil be late; the compositor can throttle the page-flip rate
For this one, wait_for_completion(previous_transfer) looks to be
similar in behaviour.

> I made a nicely working prototype from this.
>
> Best regards
> Thomas
>
>
> > The transfer is then sent in blocks, while overlapping the drm copy
> > and usb transfer in parallel. I think this is a bit too much complexity
> > for a drm/tiny driver though.
>
>
>
>
> > ...