[PATCH v6 3/3] drm: Add GUD USB Display driver

[Noralf Trønnes]

Den 01.03.2021 19.31, skrev Peter Stuge:
> Hi Noralf,
> 
> Peter Stuge wrote:
>> I'll prepare a test setup for the RGB-TFT on the weekend.
> 
> So implementing a GUD and looking at the protocol from yet another
> angle gives more new insights - surprise. :)
> 

Yep, my Raspberry Pi Pico implementation discovered a bug in the
compression code. Running X on a 240x135 display produced some 2x2
buffer rectangles which was so small that they failed to compress.

> Here are some thoughts so far:
> 
> * GUD_REQ_SET_VERSION does still rub me wrong; it seems potentially
>   quite complex to maintain compatibility in two places; both for host
>   and device. I don't want to insist on removing it, but at a minimum
>   it's quite unusual.
>   Part of the idea in USB is that host software updates are easy if
>   not fully automated but device firmware updates less so, thus
>   complexity is rather placed in the host.
> 

Alright, I'll remove it :) This can also be accomplished in the device
by having a switch that turns it into a version 1 device.

> * It's unclear to me from reading the header files in this v6 patch set
>   which GUD_REQ:s and which properties are actually mandatory in devices.
>   I'm getting hints from your STM32 device and reading the driver code in
>   detail, but what do you think is a good way to document what's required
>   vs. optional?
> 

The device must respond to all requests, but can return zero on some
like the property requests. I intend to do a writeup on the wiki later
with some more details around the protocol and how DRM works for this
usecase.

My RPi Pico implementation will also serve as a guide. I've put the
protocol handling in one file for possible reuse:
- gud.h: https://gist.github.com/notro/26bbf918fa59fb89caf155d51d57a40f
- gud.c: https://gist.github.com/notro/c1b32cea591f84d3d1c94f30812c1ba6

I will publish it when I have cleaned up the rest of the code.

> * GUD_REQ_SET_BUFFER my old nemesis. :) It's great that it's optional!
>   But do you see any way to turn it into a bulk message, in order to
>   remove the memory barrier effect of a control transfer before bulk?
> 

On a 1920x1080 RGB565 full update (4MB), the control request is
negligible. I did measure it (USB 2.0), but I don't remember the
numbers, a few milliseconds.

> I think it would be possible to noticeably improve performance later,
> by changing the host driver to submit asynchronous bulk transfers for
> frame data rather than waiting for each transfer to finish; bulk
> transfers will then pack optimally on the wire - but with a control
> transfer in between there's no chance of achieving that.
> 
> Having only one kind of transfer in the hot path would also simplify
> canceling still pending transfers (when using async later) if new data
> gets flushed before the previous frame is completely transfered.
> 

The device is the bottle neck (unless it's powerful like a Pi4):

Host: compresses buffer
Host: sends ctrl transfer
Host: sends bulk transfer
Device: Decompresses buffer, much slower than a desktop CPU
Host: compresses buffer
Host: sends ctrl transfer, waits for device to respond
Device: Has decompressed, and is ready
Host: sends bulk transfer

I would have preferred to do a test on a USB 3.0 device which would put
more stress on the host, but I couldn't find an affordable one with
mainline Linux support.

As you say this can be optimized later. By using double buffering it's
possible to compress the next buffer while the previous is in transit.

lz4 compression has worked really well in my testing, always 2x or better.

I will reconsider the ctrl req if you provide me with numbers that show
it's a performance problem.

> * A fair bit of the EDID isn't used or has dummy values. Have you already
>   considered and dismissed some other ways of accomplishing the same?
> 

EDID is optional (return zero in v7), but useful if you want userspace
to give a name to the monitor in the GUI for instance.

This comment will be present in version 7:

/*
 * Display modes can be fetched as either EDID data or an array of
&gud_display_mode_req.
 *
 * If GUD_REQ_GET_CONNECTOR_MODES returns zero, EDID is used to create
display modes.
 * If both display modes and EDID are returned, EDID is just passed on
to userspace
 * in the EDID connector property.
 */

> * Sorry if I've asked before - but what's the purpose of
>   GUD_REQ_SET_STATE_CHECK and GUD_REQ_SET_STATE_COMMIT? Why/when does
>   drm do pipe check vs. update?
> 

DRM atomic mode setting has 2 stages:
- Check to see if the mode is supported, return -EINVAL if not
- Commit the mode, this is not allowed to fail.

Userspace can also do just the check phase when figuring out which
combination of options that is supported.

> * How do you feel about passing the parameters for
>   GUD_REQ_SET_CONTROLLER_ENABLE and GUD_REQ_SET_DISPLAY_ENABLE in wValue?
>   It would save the transfer data stage.
> 

That would make the request different form the others, wValue is only
used by the connector requests for the index. I prefer to use the data
stage for data. If there was any performance gain I would have looked
into it, but these requests are only used on init and when turning
on/off the display.

Noralf.