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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 11.11.26 um 18:07 schrieb Jie Feng:
> On Wed, 11 Nov 2026 at 23:21, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 10.11.26 um 16:49 schrieb Jie Feng:
>>> On Tue, 10 Nov 2026 at 22:23, Thomas Zimmermann <tzimmermann@suse.de> 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

Gud isn't really up to date and needs maintenance.  IIRC pushing the 
transfer out of the regular display pipeline worked around limitations 
in user space, which couldn't handle the long transfer times over USB.  
But that problem has been fixed long ago.


> 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?

Most updates are small. Optimizing or full-screen redraws might not be 
worth the effort. Apart from that, set a reasonable size limit for the 
send buffer, and put in as many scan lines as possible. Then do this for 
the next until the damage rectangle has been covered.


>
>> 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.

Please do away with the worker and the USB waiting.

>
>> [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.

That's not in your current patch AFAICT. And it makes the worker pointless.

Another issue is that your driver might want to add wait timeouts at 
some point, so that stuck devices can dealt with. Having the worker in 
the loop make it only harder.

Best regards
Thomas


>
>> 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.
>>
>>
>>
>>> ...

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)