[v3,4/6] drm: Add Generic USB Display driver
diff mbox series

Message ID 20200529175643.46094-5-noralf@tronnes.org
State New
Headers show
Series
  • Generic USB Display driver
Related show

Commit Message

Noralf Trønnes May 29, 2020, 5:56 p.m. UTC
This adds a generic USB display driver with the intention that it can be
used with future USB interfaced low end displays/adapters. The Linux
gadget device driver will serve as the canonical device implementation.

The following DRM properties are supported:
- Plane rotation
- Connector TV properties

There is also support for backlight brightness exposed as a backlight
device.

Display modes can be made available to the host driver either as DRM
display modes or through EDID. If both are present, EDID is just passed
on to userspace.

Performance is preferred over color depth, so if the device supports
RGB565, DRM_CAP_DUMB_PREFERRED_DEPTH will return 16.

If the device transfer buffer can't fit an uncompressed framebuffer
update, the update is split up into parts that do fit.

Optimal user experience is achieved by providing damage reports either by
setting FB_DAMAGE_CLIPS on pageflips or calling DRM_IOCTL_MODE_DIRTYFB.

LZ4 compression is used if the device supports it.

The driver supports a one bit monochrome transfer format: R1. This is not
implemented in the gadget driver. It is added in preparation for future
monochrome e-ink displays.

The driver is MIT licensed to smooth the path for any BSD port of the
driver.

v2:
- Use devm_drm_dev_alloc() and drmm_mode_config_init()
- drm_fbdev_generic_setup: Use preferred_bpp=0, 16 was a copy paste error
- The drm_backlight_helper is dropped, copy in the code
- Support protocol version backwards compatibility for device

v3:
- Use donated Openmoko USB pid
- Use direct compression from framebuffer when pitch matches, not only on
  full frames, so split updates can benefit
- Use __le16 in struct gud_drm_req_get_connector_status
- Set edid property when the device only provides edid
- Clear compression fields in struct gud_drm_req_set_buffer
- Fix protocol version negotiation
- Remove mode->vrefresh, it's calculated

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 MAINTAINERS                             |   8 +
 drivers/gpu/drm/Kconfig                 |   2 +
 drivers/gpu/drm/Makefile                |   1 +
 drivers/gpu/drm/gud/Kconfig             |  14 +
 drivers/gpu/drm/gud/Makefile            |   4 +
 drivers/gpu/drm/gud/gud_drm_connector.c | 726 ++++++++++++++++++++++++
 drivers/gpu/drm/gud/gud_drm_drv.c       | 648 +++++++++++++++++++++
 drivers/gpu/drm/gud/gud_drm_internal.h  |  65 +++
 drivers/gpu/drm/gud/gud_drm_pipe.c      | 426 ++++++++++++++
 include/drm/gud_drm.h                   | 361 ++++++++++++
 10 files changed, 2255 insertions(+)
 create mode 100644 drivers/gpu/drm/gud/Kconfig
 create mode 100644 drivers/gpu/drm/gud/Makefile
 create mode 100644 drivers/gpu/drm/gud/gud_drm_connector.c
 create mode 100644 drivers/gpu/drm/gud/gud_drm_drv.c
 create mode 100644 drivers/gpu/drm/gud/gud_drm_internal.h
 create mode 100644 drivers/gpu/drm/gud/gud_drm_pipe.c
 create mode 100644 include/drm/gud_drm.h

Comments

Peter Stuge May 29, 2020, 10:45 p.m. UTC | #1
Hi Noralf,

Noralf Trønnes wrote:
> This adds a generic USB display driver with the intention that it can be
> used with future USB interfaced low end displays/adapters.

Fun!


> The Linux gadget device driver will serve as the canonical device
> implementation.

That's a great goal, but as proposed it isn't as generic as I would like.

Several Linux/DRM internals have "leaked" into the USB protocol - this
should be avoided if you want device implementations other than your
gadget, because those internals can change within Linux in the future,
while the protocol must not.


> If the device transfer buffer can't fit an uncompressed framebuffer
> update, the update is split up into parts that do fit.

Does "device transfer buffer" refer to something like display RAM on
the device side? If so, its size is a device implementation detail
which shouldn't be exposed over USB.

It's true that the host drives USB communication but the device decides
whether it will accept data or not. If not, it responds with a NAK
handshake in the OUT transaction, and the host controller will then
try to resend the data later, until the transfer timeout given by the
host software expires. Retries are invisible to host software.

The point is: USB has native flow control on the lowest level; that's
far more efficient than anything the application can construct, and
flow control in the application protocol would be redundant.

When using gadgetfs IIRC device controllers NAK as long as the
userspace process doesn't write new data to the ep?out-bulk fd.
Have you tried/seen this?


> The driver supports a one bit monochrome transfer format: R1. This is not
> implemented in the gadget driver. It is added in preparation for future
> monochrome e-ink displays.

The R1 idea is great!


> - Use donated Openmoko USB pid

If Linux will be the reference for this protocol then perhaps a PID
under the Linux Foundation VID (1d6b) makes more sense?

But: A PID applies on device level, not to interfaces.

Until this protocol becomes a USB-IF device class maybe it's better
to create a probe for GUD interface(s) rather than binding to PID?

Does the driver btw. already support a composite device with multiple GUD
interfaces? Say a microcontroller with two independent panels. It seems no?

If yes, would any of the control requests currently sent to the interface
be better directed at the device? If so, then a PID might make sense again,
but it's still not possible to create a composite device which uses this
protocol, without risking collissions with other vendor specific requests
on other (vendor specific) interfaces, that would be a real shame.

I can imagine a composite device wanting to implement HID and GUD,
let's make sure that it's possible.


On to the code.


> +static int gud_drm_usb_control_msg(struct usb_device *usb, u8 ifnum, bool in,
> +				   u8 request, u16 value, void *buf, size_t len,
> +				   bool check_len)
> +{
> +	u8 requesttype = USB_TYPE_VENDOR | USB_RECIP_INTERFACE;

This takes struct usb_device rather than struct usb_interface - again,
would this actually work with a composite device? The driver doesn't
ever claim the interface so I guess no?


> +static int gud_get_vendor_descriptor(struct usb_interface *interface,
> +				     struct gud_drm_display_descriptor *desc)
> +{
..
> +	ret = gud_drm_usb_control_msg(usb, ifnum, true, USB_REQ_GET_DESCRIPTOR,
> +				      GUD_DRM_USB_DT_DISPLAY << 8, buf, sizeof(*desc), false);

GUD_DRM_USB_DT_DISPLAY is defined as (USB_TYPE_VENDOR | 0x4),
but USB_TYPE_VENDOR only applies to bmRequestType[6:5] in control transfers,
nowhere else. I know of no standardized way to introduce vendor-specific
descriptors. Squatting is possible, but I think it would be nice to do
better here. It is easy enough.

It could be argued that the vendor specific interface gives flexibility here,
but actually it just means that the semantics of the standardized and
well-defined USB_REQ_GET_DESCRIPTOR have been duplicated by this protocol,
that is not very common - but if you want to go ahead then at least drop
USB_TYPE_VENDOR from the GUD_DRM_USB_DT_DISPLAY definition.

Maybe it's good to think about the data exchange some more - anything not
transfered by standardized USB_REQ_GET_DESCRIPTOR (bmRequestType 10000000B;
Device-to-host data, Standard type, Device recipient) isn't actually
a descriptor, it's vendor-specific, free-format data. Does that enable
any simplifications?


> +static int gud_usb_get_status(struct usb_device *usb, u8 ifnum)
> +{
> +	struct gud_drm_req_get_status *status;
> +	int ret, status_retries = 2000 / 5; /* maximum wait ~2 seconds */
> +	unsigned long delay = 500;
> +
> +	status = kmalloc(sizeof(*status), GFP_KERNEL);
> +	if (!status)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Poll due to lack of data/status stage control on the gadget side.

I hope we can find something better here.

Doesn't gadgetfs allow userspace to (indirectly) control the status stage,
as I wrote above?


> +	 * If we did not use polling and gave up here after waiting 2 seconds,
> +	 * the worker in the gadget would finally get to queuing up the status
> +	 * respons, but by that time the host has moved on. The gadget side
> +	 * (at least dwc2) would now be left in a non-recoverable state.

Independently of the above, how does the gadget become non-recoverable?

If a transfer times out on the host without other error then the device
has replied with NAK in the data stage transactions sent by the host
until the host stopped trying. The device controller then sees no
further data stage transactions and shouldn't be in a weird state?


> +	 * Worst case commit timeout in DRM can be tens of seconds (wait for
> +	 * various _done completions).
> +	 */
> +	while (status_retries--) {
> +		ret = gud_drm_usb_control_msg(usb, ifnum, true, USB_REQ_GET_STATUS, 0,
> +					      status, sizeof(*status), true);

Instead of this loop a single request with 2000 ms timeout would
avoid software overhead on the host. The header file says that
0 == PENDING is the only exit condition for the loop besides timeout,
as long as there are no other conditions it's better to just wait for
the timeout.


> +		if (!(status->flags & GUD_DRM_STATUS_PENDING)) {
> +			ret = -status->errno;

So this exposes errno (an implementation detail) in the bus protocol,
at an absolute minimum it needs some le_to_cpu() adjustment, but I
think it would be cleaner to define the specific errors that are
application relevant.


If this get_status thing is in fact really really required then how about
adding an interrupt IN endpoint for it? I think that would be cleaner and
it costs less bus time with arguably lower latency. See e.g. usblp.


> +static struct drm_gem_object *
> +gud_drm_driver_gem_create_object(struct drm_device *dev, size_t size)
> +{
> +	struct drm_gem_shmem_object *shmem;
..
> +	/*
> +	 * This doesn't make a difference on x86, but on ARM (pi4) it was
> +	 * necessary to avoid black lines all over and it made it possible to
> +	 * compress directly from the framebuffer without performance drop.
> +	 */
> +	shmem->map_cached = true;

Can you explain what this does exactly?


> +static int gud_drm_probe(struct usb_interface *interface,
> +			 const struct usb_device_id *id)
> +{
..
> +	struct gud_drm_display_descriptor desc;
..
> +		/* Check if the device can support us */
> +		*version = 1;
> +		ret = gud_drm_usb_control_msg(usb, ifnum, false, GUD_DRM_USB_REQ_SET_VERSION,
> +					      0, version, sizeof(*version), true);
> +		if (!ret)
> +			ret = gud_usb_get_status(usb, ifnum);
> +		kfree(version);
> +		if (ret) {
> +			dev_err(dev, "Protocol version %u is not supported\n", desc.bVersion);
> +			return -EPROTONOSUPPORT;

Could't this work without _get_status()? What does usb_control_msg()
return for a STALL handshake in the data stage?

Anyway, usb_get_status() (which I guess inspired gud_usb_get_status()) is
usually for administrative device status rather than application status.
Ideally the application status can be deduced by host software based on
device responses.


..
> +	gdrm->compression = desc.bCompression & GUD_DRM_COMPRESSION_LZ4;

This is a perfect example of doing things right! :)

GUD_DRM_COMPRESSION_LZ4 is specific for this protocol, not DRM/Linux.

What do you think about s,GUD_DRM_,GUD_, for such names?

And would it make sense to expose the protocol (names, structures) in uapi?

Or at least in Documentation/ ?


> +static size_t gud_drm_xrgb8888_to_r124(u8 *dst, const struct drm_format_info *format,
> +				       void *src, struct drm_framebuffer *fb,
> +				       struct drm_rect *rect)
> +{
..
> +	buf = kmalloc(width * height, GFP_KERNEL);
> +	if (!buf)
> +		return len; /* To keep logic simple, just transmit garbage */

Ouch! Shouldn't this bubble up somehow? If there is memory pressure
then I really think something above should fail.


> +static int gud_drm_fb_flush(struct gud_drm_device *gdrm, struct drm_framebuffer *fb,
> +			    const struct drm_format_info *format, struct drm_rect *rect)
> +{

This is in the hot path, right?


> +	ret = gud_drm_usb_set(gdrm, GUD_DRM_USB_REQ_SET_BUFFER, 0, &req, sizeof(req));
> +	if (ret)
> +		goto vunmap;
> +
> +	ret = usb_bulk_msg(gdrm->usb, gdrm->bulk_pipe, gdrm->bulk_buf, trlen,
> +			   &actual_length, msecs_to_jiffies(3000));

I would definitely change this pattern so that the hot path has only bulk
transactions, ideally a single transfer. The control transfer wastes
precious bus time in the hot path.

Maybe it's insignificant with FHD data but then again, the more data
the less overhead we want, and in any case for small R1 data the
control transfer is easily more expensive than the data itself!

A control transfer somehow compares to an ioctl() from userspace with
much context switching, while a bulk transfer is more like mmaped kernel
memory or aio.

Why not just add the values in struct gud_drm_req_set_buffer as a header
before the data instead?

That would mean two bulk transfers, but they could be asynchronous, at least
the first, but making both async would also fit more data onto the bus.
Maybe later.


> + * struct gud_drm_req_get_connector - Connector descriptor
> + * @connector_type: Connector type (DRM_MODE_CONNECTOR_*)
> + * @flags: Flags
> + * @num_properties: Number of supported properties
> + */
> +struct gud_drm_req_get_connector {
> +	__u8 connector_type;

This is intended for the (Generic) USB Display to report the connector type
used for its panel, right? It should not use Linux/DRM-internal values such
as DRM_MODE_CONNECTOR_SPI to do so, if it wants to be generic and stable
over time.

Why does the host software need to know anything about the connector
inside the device, anyway? With a microcontroller that could be anything,
especially with actual R1 displays.

Would it make sense to introduce DRM_MODE_CONNECTOR_USB on the host, and
keep this implementation detail in the device?

Make the protocol application specific and avoid implementation specifics.


> + * struct gud_drm_req_get_connector_status - Connector status

How does this work if and when the status on the device changes?


> +/*
> + * Internal monochrome transfer format presented to userspace as XRGB8888.
> + * Pixel lines are byte aligned.
> + */
> +#define GUD_DRM_FORMAT_R1	fourcc_code('R', '1', ' ', ' ')

This is also a data format over USB, right? Then it's not really internal,
because it also exists on the device. I have several uses for this, but
none will be using Linux gadgets, rather microcontrollers.


> +/* USB Control requests: */
> +
> +/*
> + * If the host driver doesn't support the device protocol version it will send
> + * the versions it supports starting with the latest. If the device isn't
> + * backwards compatible or doesn't support the version the host suggests, it
> + * shall return EPROTONOSUPPORT.
> + */
> +#define GUD_DRM_USB_REQ_SET_VERSION			0x30

USB devices report errors to a control requests with a STALL handshake,
which is passed all the way back to the host software. Please use that?

How/where would the EPROTONOSUPPORT value be returned? It's another
implementation specific, that should be used on the bus.


> +/* Get supported pixel formats as an array of fourcc codes. See include/uapi/drm/drm_fourcc.h */
> +#define GUD_DRM_USB_REQ_GET_FORMATS			0x40

Plus R1, right? I can understand if you don't want to add R1 to uapi, but
then at least document that the array can contain both uapi codes and R1.


> +/* Apply the prevoius _STATE_CHECK configuration */
> +#define GUD_DRM_USB_REQ_SET_STATE_COMMIT		0x62

Typo -> previous



I hope this helps.

Kind regards

//Peter
Noralf Trønnes June 1, 2020, 4:57 p.m. UTC | #2
Hi Peter,

Den 30.05.2020 00.45, skrev Peter Stuge:
> Hi Noralf,
> 
> Noralf Trønnes wrote:
>> This adds a generic USB display driver with the intention that it can be
>> used with future USB interfaced low end displays/adapters.
> 
> Fun!
> 
> 
>> The Linux gadget device driver will serve as the canonical device
>> implementation.
> 
> That's a great goal, but as proposed it isn't as generic as I would like.
> 
> Several Linux/DRM internals have "leaked" into the USB protocol - this
> should be avoided if you want device implementations other than your
> gadget, because those internals can change within Linux in the future,
> while the protocol must not.
> 

That's intentional, I see no point in recreating uapi values that won't
change:

Linux errno: /include/uapi/asm-generic/errno{-base,}.h
Pixel formats: include/uapi/drm/drm_fourcc.h
Connector types and status: include/uapi/drm/drm_mode.h

> 
>> If the device transfer buffer can't fit an uncompressed framebuffer
>> update, the update is split up into parts that do fit.
> 
> Does "device transfer buffer" refer to something like display RAM on
> the device side? If so, its size is a device implementation detail
> which shouldn't be exposed over USB.
> 

The reason for exposing this is that the Linux gadget driver needs to
decompress the transfer into a buffer and the host needs to know how big
this is (the host can choose to lower this if it can't allocate a
continuous buffer of this size).

lz4 (in the kernel) is block compression and can't be used for
decompressing just a stream of bytes. There is a lz4 frame protocol
which looks like it could be useful, but it's not available in the
kernel. I hardly know anything about compression so I'm in no position
to add that to the kernel. Maybe someone will add it at a later time if
it is useful.

> It's true that the host drives USB communication but the device decides
> whether it will accept data or not. If not, it responds with a NAK
> handshake in the OUT transaction, and the host controller will then
> try to resend the data later, until the transfer timeout given by the
> host software expires. Retries are invisible to host software.
> 
> The point is: USB has native flow control on the lowest level; that's
> far more efficient than anything the application can construct, and
> flow control in the application protocol would be redundant.
> 
> When using gadgetfs IIRC device controllers NAK as long as the
> userspace process doesn't write new data to the ep?out-bulk fd.
> Have you tried/seen this?
> 
> 
>> The driver supports a one bit monochrome transfer format: R1. This is not
>> implemented in the gadget driver. It is added in preparation for future
>> monochrome e-ink displays.
> 
> The R1 idea is great!
> 

My plan was to remove R1 support from this version of the patchset, but
I forgot. The reason is that it's cumbersome to test when the gadget
driver doesn't support it. I had some code to test the implementation
when I made it, but it's removed now. It's very easy to add R1 back in
when there's a display that makes use of it giving it a thorough testing.

You mention further down that you have use cases for this, do you have a
timeplan for when you will make use of R1?

> 
>> - Use donated Openmoko USB pid
> 
> If Linux will be the reference for this protocol then perhaps a PID
> under the Linux Foundation VID (1d6b) makes more sense?
> 

Do they hand out pid's? To me it's no big deal, it can be added later if
someones cares about it.

> But: A PID applies on device level, not to interfaces.
> 

Indeed. This is a USB interface driver though, so it only looks at
vendor class interfaces. Then it checks if there's a bulk out endpoint,
if not it returns -ENXIO and the device subsytem moves on to another
interface driver if any. Next it tries to fetch the display descriptor
and if not succesful it returns -ENODEV to give another driver a chance.

I have tried my best to let the driver tolerate other vendor class
interfaces on the device.

> Until this protocol becomes a USB-IF device class maybe it's better
> to create a probe for GUD interface(s) rather than binding to PID?
> 

Not sure what you mean here, this is an interface driver.

There exists a HID Display page:

Request #: HUTRR29
Title: Repurposing the Alphanumeric Display Page (0x14) as a generic
Auxiliary Display Page and adding bitmap and custom segment
capabilities
https://www.usb.org/sites/default/files/hutrr29b_0.pdf

I couldn't find anything that uses it. It could have been interesting if
there was a bulk endpoint here.

> Does the driver btw. already support a composite device with multiple GUD
> interfaces? Say a microcontroller with two independent panels. It seems no?
> 

I haven't tried, but it should work.

> If yes, would any of the control requests currently sent to the interface
> be better directed at the device? If so, then a PID might make sense again,
> but it's still not possible to create a composite device which uses this
> protocol, without risking collissions with other vendor specific requests
> on other (vendor specific) interfaces, that would be a real shame.
> 

I don't understand why PID should not be necessary, I'm using a vendor
class interface and the driver can't probe all of those, so it has to
look at specific vid:pid's.

> I can imagine a composite device wanting to implement HID and GUD,
> let's make sure that it's possible.
> 

I have tried together with a HID interface and that worked. I
specifically want HID to work so it's possible to make touch displays.
The Raspberry Pi images I've made available for testing has a console on
a USB ACM (serial) interface.

> 
> On to the code.
> 
> 
>> +static int gud_drm_usb_control_msg(struct usb_device *usb, u8 ifnum, bool in,
>> +				   u8 request, u16 value, void *buf, size_t len,
>> +				   bool check_len)
>> +{
>> +	u8 requesttype = USB_TYPE_VENDOR | USB_RECIP_INTERFACE;
> 
> This takes struct usb_device rather than struct usb_interface - again,
> would this actually work with a composite device? The driver doesn't
> ever claim the interface so I guess no?
> 

usb_interface has a pointer to usb_device and that's what's used to send
usb messages, so instead of dereferencing each time I just use it directly.

> 
>> +static int gud_get_vendor_descriptor(struct usb_interface *interface,
>> +				     struct gud_drm_display_descriptor *desc)
>> +{
> ..
>> +	ret = gud_drm_usb_control_msg(usb, ifnum, true, USB_REQ_GET_DESCRIPTOR,
>> +				      GUD_DRM_USB_DT_DISPLAY << 8, buf, sizeof(*desc), false);
> 
> GUD_DRM_USB_DT_DISPLAY is defined as (USB_TYPE_VENDOR | 0x4),
> but USB_TYPE_VENDOR only applies to bmRequestType[6:5] in control transfers,
> nowhere else. I know of no standardized way to introduce vendor-specific
> descriptors. Squatting is possible, but I think it would be nice to do
> better here. It is easy enough.
> 
> It could be argued that the vendor specific interface gives flexibility here,
> but actually it just means that the semantics of the standardized and
> well-defined USB_REQ_GET_DESCRIPTOR have been duplicated by this protocol,
> that is not very common - but if you want to go ahead then at least drop
> USB_TYPE_VENDOR from the GUD_DRM_USB_DT_DISPLAY definition.
> 

I liked that it is well defined and understood, so I didn't have to be
clever. It's nice that the size is embedded for future versions of the
protocol that migh expand the descriptor. Well not so important now that
I have the version number in there.

I like it and think I'll keep it, so I'll change the macro:

#define GUD_DRM_USB_DT_DISPLAY 0x44

> Maybe it's good to think about the data exchange some more - anything not
> transfered by standardized USB_REQ_GET_DESCRIPTOR (bmRequestType 10000000B;
> Device-to-host data, Standard type, Device recipient) isn't actually
> a descriptor, it's vendor-specific, free-format data. Does that enable
> any simplifications?
> 

Actually it is:

	u8 requesttype = USB_TYPE_VENDOR | USB_RECIP_INTERFACE;

see gud_drm_usb_control_msg(). I could add
GUD_DRM_USB_REQ_GET_DESCRIPTOR instead of using USB_REQ_GET_DESCRIPTOR
if that makes it any clearer.

> 
>> +static int gud_usb_get_status(struct usb_device *usb, u8 ifnum)
>> +{
>> +	struct gud_drm_req_get_status *status;
>> +	int ret, status_retries = 2000 / 5; /* maximum wait ~2 seconds */
>> +	unsigned long delay = 500;
>> +
>> +	status = kmalloc(sizeof(*status), GFP_KERNEL);
>> +	if (!status)
>> +		return -ENOMEM;
>> +
>> +	/*
>> +	 * Poll due to lack of data/status stage control on the gadget side.
> 
> I hope we can find something better here.
> 
> Doesn't gadgetfs allow userspace to (indirectly) control the status stage,
> as I wrote above?
> 

For read requests the device is stalling in case of error and the host
issues a status request.

For write requests the only thing I've found is a way to delay the
status stage, but only for requests without payload:

drivers/usb/gadget/composite.c: composite_setup():

} else if (value == USB_GADGET_DELAYED_STATUS && w_length != 0) {
	WARN(cdev,
		"%s: Delayed status not supported for w_length != 0",
		__func__);
}

And I can't stall, only continue:

/**
 * usb_composite_setup_continue() - Continue with the control transfer
 * @cdev: the composite device who's control transfer was kept waiting
 *
 * This function must be called by the USB function driver to continue
 * with the control transfer's data/status stage in case it had requested to
 * delay the data/status stages. A USB function's setup handler (e.g.
set_alt())
 * can request the composite framework to delay the setup request's
data/status
 * stages by returning USB_GADGET_DELAYED_STATUS.
 */
void usb_composite_setup_continue(struct usb_composite_dev *cdev)

If I could control the status stage and stall after data was received,
then I wouldn't need my own status stage.

> 
>> +	 * If we did not use polling and gave up here after waiting 2 seconds,
>> +	 * the worker in the gadget would finally get to queuing up the status
>> +	 * respons, but by that time the host has moved on. The gadget side
>> +	 * (at least dwc2) would now be left in a non-recoverable state.
> 
> Independently of the above, how does the gadget become non-recoverable?
> 

It stops responding to requests.

> If a transfer times out on the host without other error then the device
> has replied with NAK in the data stage transactions sent by the host
> until the host stopped trying. The device controller then sees no
> further data stage transactions and shouldn't be in a weird state?
> 

I don't know why it doesn't respond, it could be a controller driver
(dwc2) bug, but I'm no expert here. Actually this is the first work I've
ever done with USB.

> 
>> +	 * Worst case commit timeout in DRM can be tens of seconds (wait for
>> +	 * various _done completions).
>> +	 */
>> +	while (status_retries--) {
>> +		ret = gud_drm_usb_control_msg(usb, ifnum, true, USB_REQ_GET_STATUS, 0,
>> +					      status, sizeof(*status), true);
> 
> Instead of this loop a single request with 2000 ms timeout would
> avoid software overhead on the host. The header file says that
> 0 == PENDING is the only exit condition for the loop besides timeout,
> as long as there are no other conditions it's better to just wait for
> the timeout.
> 
> 
>> +		if (!(status->flags & GUD_DRM_STATUS_PENDING)) {
>> +			ret = -status->errno;
> 
> So this exposes errno (an implementation detail) in the bus protocol,
> at an absolute minimum it needs some le_to_cpu() adjustment, but I
> think it would be cleaner to define the specific errors that are
> application relevant.
> 
> 
> If this get_status thing is in fact really really required then how about
> adding an interrupt IN endpoint for it? I think that would be cleaner and
> it costs less bus time with arguably lower latency. See e.g. usblp.
> 

I couldn't find interrupt ep used in drivers/usb/class/usblp.c, but I've
seen examples of this used for status.
I'm a bit wary of using more endpoints since I suspect that
microcontrollers don't have that many.

I did a test setting delay to fixed 500-1500 usec in the polling loop
instead of the 4500-5500 that will be used when running a movie.
Actual frames per second for a 30 sec movie clip changed from 8.8 to
9.0, so latency isn't a big issue at least.

I also realise now that I don't need the PENDING flag, I can just keep
stalling the status request until the device is done.

> 
>> +static struct drm_gem_object *
>> +gud_drm_driver_gem_create_object(struct drm_device *dev, size_t size)
>> +{
>> +	struct drm_gem_shmem_object *shmem;
> ..
>> +	/*
>> +	 * This doesn't make a difference on x86, but on ARM (pi4) it was
>> +	 * necessary to avoid black lines all over and it made it possible to
>> +	 * compress directly from the framebuffer without performance drop.
>> +	 */
>> +	shmem->map_cached = true;
> 
> Can you explain what this does exactly?
> 

Not excatly, but I can try rather vaguely :-) The memory subsystem is a
black box to me. It has to do with the memory page attribute that
controls how memory access is handled by the CPU cache. The default for
shmem is WriteCombined which makes for slow read access on ARM, but on
x86 there doesn't seem to be any penalty judging by my brief testing.
The black lines are probably cache lines that haven't been
flushed/invalidated or whatever the black box does.

> 
>> +static int gud_drm_probe(struct usb_interface *interface,
>> +			 const struct usb_device_id *id)
>> +{
> ..
>> +	struct gud_drm_display_descriptor desc;
> ..
>> +		/* Check if the device can support us */
>> +		*version = 1;
>> +		ret = gud_drm_usb_control_msg(usb, ifnum, false, GUD_DRM_USB_REQ_SET_VERSION,
>> +					      0, version, sizeof(*version), true);
>> +		if (!ret)
>> +			ret = gud_usb_get_status(usb, ifnum);
>> +		kfree(version);
>> +		if (ret) {
>> +			dev_err(dev, "Protocol version %u is not supported\n", desc.bVersion);
>> +			return -EPROTONOSUPPORT;
> 
> Could't this work without _get_status()? What does usb_control_msg()
> return for a STALL handshake in the data stage?
> 

It returns -EPIPE, but as mentioned above, I can't stall a control write
request with payload. So I need my own status stage.

> Anyway, usb_get_status() (which I guess inspired gud_usb_get_status()) is
> usually for administrative device status rather than application status.
> Ideally the application status can be deduced by host software based on
> device responses.
> 
> 
> ..
>> +	gdrm->compression = desc.bCompression & GUD_DRM_COMPRESSION_LZ4;
> 
> This is a perfect example of doing things right! :)
> 
> GUD_DRM_COMPRESSION_LZ4 is specific for this protocol, not DRM/Linux.
> 
> What do you think about s,GUD_DRM_,GUD_, for such names?
> 
> And would it make sense to expose the protocol (names, structures) in uapi?
> 

No, since it's not for userspace.

> Or at least in Documentation/ ?
> 

Ideally I would have written a spec and made the implementation against
it. However since I suck at writing, I even struggle with getting the
compiler to understand me, so it won't happen. It would have taken me
forever.

> 
>> +static size_t gud_drm_xrgb8888_to_r124(u8 *dst, const struct drm_format_info *format,
>> +				       void *src, struct drm_framebuffer *fb,
>> +				       struct drm_rect *rect)
>> +{
> ..
>> +	buf = kmalloc(width * height, GFP_KERNEL);
>> +	if (!buf)
>> +		return len; /* To keep logic simple, just transmit garbage */
> 
> Ouch! Shouldn't this bubble up somehow? If there is memory pressure
> then I really think something above should fail.
> 
> 
>> +static int gud_drm_fb_flush(struct gud_drm_device *gdrm, struct drm_framebuffer *fb,
>> +			    const struct drm_format_info *format, struct drm_rect *rect)
>> +{
> 
> This is in the hot path, right?
> 

Indeed, the most important one. All the other paths can take "as long"
as they want, but this one "shows up" on the display.

> 
>> +	ret = gud_drm_usb_set(gdrm, GUD_DRM_USB_REQ_SET_BUFFER, 0, &req, sizeof(req));
>> +	if (ret)
>> +		goto vunmap;
>> +
>> +	ret = usb_bulk_msg(gdrm->usb, gdrm->bulk_pipe, gdrm->bulk_buf, trlen,
>> +			   &actual_length, msecs_to_jiffies(3000));
> 
> I would definitely change this pattern so that the hot path has only bulk
> transactions, ideally a single transfer. The control transfer wastes
> precious bus time in the hot path.
> 
> Maybe it's insignificant with FHD data but then again, the more data
> the less overhead we want, and in any case for small R1 data the
> control transfer is easily more expensive than the data itself!
> 
> A control transfer somehow compares to an ioctl() from userspace with
> much context switching, while a bulk transfer is more like mmaped kernel
> memory or aio.
> 
> Why not just add the values in struct gud_drm_req_set_buffer as a header
> before the data instead?
> 
> That would mean two bulk transfers, but they could be asynchronous, at least
> the first, but making both async would also fit more data onto the bus.
> Maybe later.
> 

GUD_DRM_USB_REQ_SET_BUFFER serves one purpose in addition to sending the
header, as mentioned in the comment preceeding it, it waits for any
ongoing decompression/copying on the device from the previous fb flush.
lz4 decompression is much faster than compression, but the host will be
much more powerful than the device, so the host will almost always have
to wait for the device when flushing continously.

Yes you're right, the overhead is insignificant for FHD, 4-500 usec for
the actual control messages (including status message) vs. 50-100ms for
the total operation (depending on how well it compresses).

But why should this overhead be of concern for a display that has a
framebuffer transfer of 4-8kB? It will be more than fast enough.

> 
>> + * struct gud_drm_req_get_connector - Connector descriptor
>> + * @connector_type: Connector type (DRM_MODE_CONNECTOR_*)
>> + * @flags: Flags
>> + * @num_properties: Number of supported properties
>> + */
>> +struct gud_drm_req_get_connector {
>> +	__u8 connector_type;
> 
> This is intended for the (Generic) USB Display to report the connector type
> used for its panel, right? It should not use Linux/DRM-internal values such
> as DRM_MODE_CONNECTOR_SPI to do so, if it wants to be generic and stable
> over time.
> 

DRM_MODE_CONNECTOR_SPI is uabi from include/uapi/drm/drm_mode.h

> Why does the host software need to know anything about the connector
> inside the device, anyway? With a microcontroller that could be anything,
> especially with actual R1 displays.
> 
> Would it make sense to introduce DRM_MODE_CONNECTOR_USB on the host, and
> keep this implementation detail in the device?
> 

For display adapters it makes sense especially when it has more than one
connector like the Raspberry Pi which has an hdmi and a sdtv connector.
For display panels the connector is USB yes, but there's no type for
this yet. There's a discussion going on if we should stop adding
connector types for panels and just add a DRM_MODE_CONNECTOR_PANEL type.
Not sure if there has been a conclusion on this.

> Make the protocol application specific and avoid implementation specifics.
> 
> 
>> + * struct gud_drm_req_get_connector_status - Connector status
> 
> How does this work if and when the status on the device changes?
> 

If that status can change, the device has to set
gud_drm_gadget_connector.flags |= GUD_DRM_CONNECTOR_FLAGS_POLL.
Now the host will poll the status every 10 seconds. Userspace is
notified about change in status.

> 
>> +/*
>> + * Internal monochrome transfer format presented to userspace as XRGB8888.
>> + * Pixel lines are byte aligned.
>> + */
>> +#define GUD_DRM_FORMAT_R1	fourcc_code('R', '1', ' ', ' ')
> 
> This is also a data format over USB, right? Then it's not really internal,
> because it also exists on the device. I have several uses for this, but
> none will be using Linux gadgets, rather microcontrollers.
> 

I can drop the word internal if it confuses, but any non-Linux
implementation will have to copy this header file and resolve the
dependencies. Like I did for my uC hack:
https://github.com/notro/gud/tree/master/STM32F769I-DISCO/USB_Device/MSC_Standalone/Inc

> 
>> +/* USB Control requests: */
>> +
>> +/*
>> + * If the host driver doesn't support the device protocol version it will send
>> + * the versions it supports starting with the latest. If the device isn't
>> + * backwards compatible or doesn't support the version the host suggests, it
>> + * shall return EPROTONOSUPPORT.
>> + */
>> +#define GUD_DRM_USB_REQ_SET_VERSION			0x30
> 
> USB devices report errors to a control requests with a STALL handshake,
> which is passed all the way back to the host software. Please use that?
> 
> How/where would the EPROTONOSUPPORT value be returned? It's another
> implementation specific, that should be used on the bus.
> 
> 
>> +/* Get supported pixel formats as an array of fourcc codes. See include/uapi/drm/drm_fourcc.h */
>> +#define GUD_DRM_USB_REQ_GET_FORMATS			0x40
> 
> Plus R1, right? I can understand if you don't want to add R1 to uapi, but
> then at least document that the array can contain both uapi codes and R1.
> 
> 
>> +/* Apply the prevoius _STATE_CHECK configuration */
>> +#define GUD_DRM_USB_REQ_SET_STATE_COMMIT		0x62
> 
> Typo -> previous
> 
> 
> 
> I hope this helps.
> 

Indeed it I does, it helps me revisit the decisions I've made and look
at them in a new light. I have looked at the code for so long that I
hardly see it anymore.

Noralf.
Peter Stuge June 2, 2020, 12:12 a.m. UTC | #3
Hi Noralf,

Thanks a lot for going into more detail.

Noralf Trønnes wrote:
> > Several Linux/DRM internals have "leaked" into the USB protocol - this
> > should be avoided if you want device implementations other than your
> > gadget, because those internals can change within Linux in the future,
> > while the protocol must not.
> > 
> 
> That's intentional, I see no point in recreating uapi values that won't
> change:
> 
> Linux errno: /include/uapi/asm-generic/errno{-base,}.h
> Pixel formats: include/uapi/drm/drm_fourcc.h
> Connector types and status: include/uapi/drm/drm_mode.h

I understand, and it's good that these are uapi values, but I will still
disagree with using errno and DRM connector types in the USB protocol,
which is a "namespace" of its own.

That is an important point here; GUD is three things: a Linux DRM driver,
a Linux gadget driver and a USB protocol. USB protocols (good ones) are
OS-agnostic.


> >> If the device transfer buffer can't fit an uncompressed framebuffer
> >> update, the update is split up into parts that do fit.
> > 
> > Does "device transfer buffer" refer to something like display RAM on
> > the device side? If so, its size is a device implementation detail
> > which shouldn't be exposed over USB.
> 
> The reason for exposing this is that the Linux gadget driver needs to
> decompress the transfer into a buffer and the host needs to know how big
> this is (the host can choose to lower this if it can't allocate a
> continuous buffer of this size).

I understand; so it's only required for some compression types - then
it should at least be completely optional, but in any case I find
exposing/having to expose this to be awful USB protocol design and I
hope we can find a better way.

Maybe it's premature anyway? How would you feel about skipping compression
to begin with?


> lz4 (in the kernel) is block compression and can't be used for
> decompressing just a stream of bytes. There is a lz4 frame protocol
> which looks like it could be useful, but it's not available in the
> kernel. I hardly know anything about compression so I'm in no position
> to add that to the kernel. Maybe someone will add it at a later time if
> it is useful.

Why did you choose to use lz4?

Whether compression comes now or later, maybe there is a more suitable
algorithm?


> > The R1 idea is great!
> 
> My plan was to remove R1 support from this version of the patchset, but
> I forgot. The reason is that it's cumbersome to test when the gadget
> driver doesn't support it.

Why not support R1 also in the gadget?


> You mention further down that you have use cases for this, do you have a
> timeplan for when you will make use of R1?

No strict plan, but if it helps I could make a demo device and -firmware
without much effort. You mentioned that you would like to have a
microcontroller device for testing?


> >> - Use donated Openmoko USB pid
> > 
> > If Linux will be the reference for this protocol then perhaps a PID
> > under the Linux Foundation VID (1d6b) makes more sense?
> 
> Do they hand out pid's?

I don't know. :) The root hub drivers each have one.


> To me it's no big deal, it can be added later if someones cares about it.

Okay, hopefully we can do without a PID anyway.


> > But: A PID applies on device level, not to interfaces.
> 
> Indeed. This is a USB interface driver though, so it only looks at
> vendor class interfaces. Then it checks if there's a bulk out endpoint,
> if not it returns -ENXIO and the device subsytem moves on to another
> interface driver if any. Next it tries to fetch the display descriptor
> and if not succesful it returns -ENODEV to give another driver a chance.

Thanks for clarifying this flow. It's nice not to require particular
endpoint addresses - that makes the protocol/driver much more generic.


> I have tried my best to let the driver tolerate other vendor class
> interfaces on the device.

Ack, this is clear now.


> I don't understand why PID should not be necessary, I'm using a vendor
> class interface and the driver can't probe all of those, so it has to
> look at specific vid:pid's.

Why can't the driver probe all vendor class interfaces?

To probe fewer interfaces, a criteria other than PID can still be defined,
and doing so would enable immediate plug-and-play for non-gadget and especially
composite devices, without requiring the addition of PIDs in the host driver.

I find this possibility especially attractive for composite devices, which
may already have some VID:PID and a non-GUD primary function/interface that
is handled by another driver, such that a GUD PID effectively can't be adopted
for that device.

One example of such a criteria would be to require that the iInterface
string descriptor contains the (sub)string "Generic USB Display".


> I have tried together with a HID interface and that worked.

Great. Thanks!


> >> +static int gud_get_vendor_descriptor(struct usb_interface *interface,
> >> +				     struct gud_drm_display_descriptor *desc)
> >> +{
> > ..
> >> +	ret = gud_drm_usb_control_msg(usb, ifnum, true, USB_REQ_GET_DESCRIPTOR,
> >> +				      GUD_DRM_USB_DT_DISPLAY << 8, buf, sizeof(*desc), false);
> > 
> > GUD_DRM_USB_DT_DISPLAY is defined as (USB_TYPE_VENDOR | 0x4),
> > but USB_TYPE_VENDOR only applies to bmRequestType[6:5] in control transfers,
> > nowhere else. I know of no standardized way to introduce vendor-specific
> > descriptors. Squatting is possible, but I think it would be nice to do
> > better here. It is easy enough.
> > 
> > It could be argued that the vendor specific interface gives flexibility here,
> > but actually it just means that the semantics of the standardized and
> > well-defined USB_REQ_GET_DESCRIPTOR have been duplicated by this protocol,
> > that is not very common - but if you want to go ahead then at least drop
> > USB_TYPE_VENDOR from the GUD_DRM_USB_DT_DISPLAY definition.
> 
> I liked that it is well defined and understood, so I didn't have to be clever.

I tried to explain that it is only well defined for the standardized
GET_DESCRIPTOR request with device recipient. The concept "descriptor"
isn't used anywhere else by other USB protocols that I know.

There are various class-specific descriptors, but they are all standardized
in USB-IF device class specs, and all of them are only ever retrieved by the
standardized GET_DESCRIPTOR request.

Because of that it's actually rather confusing to me to refer to the display
data structure as a descriptor and even use the standardized descriptor header
and naming convention when it is actually /not/ a descriptor, since it isn't
standardized and isn't retrievable with the standardized GET_DESCRIPTOR
request. Does that make sense?


> I like it and think I'll keep it, so I'll change the macro:
> 
> #define GUD_DRM_USB_DT_DISPLAY 0x44

Okay. The number is arbitrary, since a control request directed to a vendor
specific interface is vendor specific "by inheritance", thus also arbitrary.


> > Maybe it's good to think about the data exchange some more - anything not
> > transfered by standardized USB_REQ_GET_DESCRIPTOR (bmRequestType 10000000B;
> > Device-to-host data, Standard type, Device recipient) isn't actually
> > a descriptor, it's vendor-specific, free-format data. Does that enable
> > any simplifications?
> 
> Actually it is:
> 
> 	u8 requesttype = USB_TYPE_VENDOR | USB_RECIP_INTERFACE;

Right; it's a Vendor-Specific type, Interface recipient request, thus not
the standardized USB_REQ_GET_DESCRIPTOR, and thus the data is not actually
a descriptor, hence why calling it one is confusing to me.


> see gud_drm_usb_control_msg(). I could add
> GUD_DRM_USB_REQ_GET_DESCRIPTOR instead of using USB_REQ_GET_DESCRIPTOR
> if that makes it any clearer.

That would be an improvement, but I would really wish for different
terminology all together.


> >> +static int gud_usb_get_status(struct usb_device *usb, u8 ifnum)
> >> +{
> >> +	struct gud_drm_req_get_status *status;
> >> +	int ret, status_retries = 2000 / 5; /* maximum wait ~2 seconds */
> >> +	unsigned long delay = 500;
> >> +
> >> +	status = kmalloc(sizeof(*status), GFP_KERNEL);
> >> +	if (!status)
> >> +		return -ENOMEM;
> >> +
> >> +	/*
> >> +	 * Poll due to lack of data/status stage control on the gadget side.
> > 
> > I hope we can find something better here.
> > 
> > Doesn't gadgetfs allow userspace to (indirectly) control the status stage,
> > as I wrote above?
> 
> For read requests the device is stalling in case of error and the host
> issues a status request.

Sorry, I can't quite understand this.

Does read mean IN or OUT?

And what does "the host issues a status request" mean? Status is the final
stage in each (control request) transaction. I'm a bit confused here.


> For write requests the only thing I've found is a way to delay the
> status stage, but only for requests without payload:
..
> drivers/usb/gadget/composite.c: composite_setup():

The way I read composite_setup() after try_fun_setup: it calls f->setup()
when available, and that can return < 0 to stall.

I expect that composite_setup() and thus f->setup() run when the
SETUP packet has arrived, thus before the data packet arrives, and if
composite_setup() stalls then the device/function should never see the
data packet.

For an OUT transaction I think the host controller might still send
the DATA packet, but the device controllers that I know don't make it
visible to the application in that case.


> And I can't stall, only continue:
..
> If I could control the status stage and stall after data was received,
> then I wouldn't need my own status stage.

It looks to me like the function can stall.

In any case I think the delayed thing+continue is the wrong path.


> >> +	 * If we did not use polling and gave up here after waiting 2 seconds,
> >> +	 * the worker in the gadget would finally get to queuing up the status
> >> +	 * respons, but by that time the host has moved on. The gadget side
> >> +	 * (at least dwc2) would now be left in a non-recoverable state.
> > 
> > Independently of the above, how does the gadget become non-recoverable?
> 
> It stops responding to requests.
> 
> > If a transfer times out on the host without other error then the device
> > has replied with NAK in the data stage transactions sent by the host
> > until the host stopped trying. The device controller then sees no
> > further data stage transactions and shouldn't be in a weird state?
> 
> I don't know why it doesn't respond, it could be a controller driver
> (dwc2) bug, but I'm no expert here.

It would be useful to investigate this in more detail. 

And I think it would be useful to have another, independent, device
implementation. I could help with that.


> Actually this is the first work I've ever done with USB.

I think it's a very ambitious first USB project, but it is a fun idea! :)

I would like it if we can iterate to something really generic that takes
advantage of all that USB offers.


> > If this get_status thing is in fact really really required then how about
> > adding an interrupt IN endpoint for it? I think that would be cleaner and
> > it costs less bus time with arguably lower latency. See e.g. usblp.
> 
> I couldn't find interrupt ep used in drivers/usb/class/usblp.c, but I've
> seen examples of this used for status.

Sorry, yes, in usblp it is a bulk endpoint, it was more about the pattern
of a status endpoint.


> I'm a bit wary of using more endpoints since I suspect that
> microcontrollers don't have that many.

It was true for some early microcontrollers with USB, but I haven't
seen one in many years with less than six endpoints, and usually they
have more. Some really old 8-bit ones even allowed using all 32 possible
endpoints, more than many modern IP blocks.


To me it is critical to remove this status polling, because as I wrote,
the lowest level protocol already implements very good flow control,
the application protocol just needs to use it, not invent its own.


> I also realise now that I don't need the PENDING flag, I can just keep
> stalling the status request until the device is done.

That's heading in the right direction, but the transaction should be
NAKed rather than STALLed.

Think of NAK as EAGAIN and STALL more like EPIPE. The former is "not yet"
while the latter signals that there was an actual error.

It looks like composite_setup() might indeed not support NAK, that would
be a bug/missing feature in the gadget code, but I may also just be reading
it wrong. Either way it doesn't justify polling in the application protocol.

In addition to being redundant, such polling generates constant application-
level noise on the bus, causes unneccessary wakeups and is very annoying when
looking at packet captures e.g. from usbmon. We should avoid that.


> >> +	shmem->map_cached = true;
> > 
> > Can you explain what this does exactly?
> 
> Not excatly, but I can try rather vaguely :-) The memory subsystem is a
> black box to me. It has to do with the memory page attribute that
> controls how memory access is handled by the CPU cache. The default for
> shmem is WriteCombined which makes for slow read access on ARM, but on
> x86 there doesn't seem to be any penalty judging by my brief testing.

Hmm. ARM often (always?) can't do byte-aligned access, only 32-bit access.
Do you know if and how the buffer is aligned? Maybe this can make a difference?


> >> +static int gud_drm_probe(struct usb_interface *interface,
> >> +			 const struct usb_device_id *id)
> >> +{
..
> >> +		/* Check if the device can support us */
> >> +		*version = 1;
> >> +		ret = gud_drm_usb_control_msg(usb, ifnum, false, GUD_DRM_USB_REQ_SET_VERSION,
> >> +					      0, version, sizeof(*version), true);

A more USB:y way would btw. be to report the highest supported protocol
version to the host in some data structure.

This too could be the iInterface string descriptor.


> > Could't this work without _get_status()? What does usb_control_msg()
> > return for a STALL handshake in the data stage?
> 
> It returns -EPIPE, but as mentioned above, I can't stall a control write
> request with payload.

Please check this? It looks to me like it would be possible.


> > And would it make sense to expose the protocol (names, structures) in uapi?
> 
> No, since it's not for userspace.

Okay.


> > Or at least in Documentation/ ?
> 
> Ideally I would have written a spec and made the implementation against it.
> However since I suck at writing, I even struggle with getting the compiler
> to understand me, so it won't happen. It would have taken me forever.

I have a template for USB protocols that I could use, perhaps we can make
that spec a reality. I think it would be quite valuable, to help folks
outside Linux who may also want to create a generic usb display.


> >> +static size_t gud_drm_xrgb8888_to_r124(u8 *dst, const struct drm_format_info *format,
> >> +				       void *src, struct drm_framebuffer *fb,
> >> +				       struct drm_rect *rect)
> >> +{
> > ..
> >> +	buf = kmalloc(width * height, GFP_KERNEL);
> >> +	if (!buf)
> >> +		return len; /* To keep logic simple, just transmit garbage */
> > 
> > Ouch! Shouldn't this bubble up somehow? If there is memory pressure
> > then I really think something above should fail.

No comment on this?



> >> +static int gud_drm_fb_flush(struct gud_drm_device *gdrm, struct drm_framebuffer *fb,
> >> +			    const struct drm_format_info *format, struct drm_rect *rect)
> >> +{
> > 
> > This is in the hot path, right?
> 
> Indeed, the most important one. All the other paths can take "as long"
> as they want, but this one "shows up" on the display.
> 
> > 
> >> +	ret = gud_drm_usb_set(gdrm, GUD_DRM_USB_REQ_SET_BUFFER, 0, &req, sizeof(req));
> >> +	if (ret)
> >> +		goto vunmap;
> >> +
> >> +	ret = usb_bulk_msg(gdrm->usb, gdrm->bulk_pipe, gdrm->bulk_buf, trlen,
> >> +			   &actual_length, msecs_to_jiffies(3000));
> > 
> > I would definitely change this pattern so that the hot path has only bulk
> > transactions, ideally a single transfer. The control transfer wastes
> > precious bus time in the hot path.
> > 
> > Maybe it's insignificant with FHD data but then again, the more data
> > the less overhead we want, and in any case for small R1 data the
> > control transfer is easily more expensive than the data itself!
> > 
> > A control transfer somehow compares to an ioctl() from userspace with
> > much context switching, while a bulk transfer is more like mmaped kernel
> > memory or aio.
> > 
> > Why not just add the values in struct gud_drm_req_set_buffer as a header
> > before the data instead?
> > 
> > That would mean two bulk transfers, but they could be asynchronous, at least
> > the first, but making both async would also fit more data onto the bus.
> > Maybe later.
> 
> GUD_DRM_USB_REQ_SET_BUFFER serves one purpose in addition to sending the
> header, as mentioned in the comment preceeding it, it waits for any
> ongoing decompression/copying on the device from the previous fb flush.

So even more application-level flow control. Please let's make sure that
the protocol will simply rely on the low-level flow control here as well.


> lz4 decompression is much faster than compression, but the host will be
> much more powerful than the device, so the host will almost always have
> to wait for the device when flushing continously.

One design objective for USB is indeed strong host+weak device and the
low-level flow control applies particularly well in this case. The device
explicitly informs the host that it is not ready for data using NAK.


> Yes you're right, the overhead is insignificant for FHD, 4-500 usec for
> the actual control messages (including status message) vs. 50-100ms for
> the total operation (depending on how well it compresses).
> 
> But why should this overhead be of concern for a display that has a
> framebuffer transfer of 4-8kB? It will be more than fast enough.

Mm, insignificant there as well, but in both cases it's not optimal,
and that without good reason.

Besides that, it also causes unneccessary synchronization/bus scheduling
complexity and wastes bandwidth. Control transfers and bulk transfers are
scheduled to wire time according to different rules, with control requests
taking priority over bulk in case of contention.

A control request occupies more bus time than a NAKed bulk data transaction.

Finally, besides application level flow control simply being wrong it's doubly
wrong to use the control pipe to implement flow control for a bulk pipe; those
two pipes are completely independent bus channels, and shouldn't be interlocked
without an exceptionally good reason and flow control isn't it.


> >> + * struct gud_drm_req_get_connector - Connector descriptor
> >> + * @connector_type: Connector type (DRM_MODE_CONNECTOR_*)
> >> + * @flags: Flags
> >> + * @num_properties: Number of supported properties
> >> + */
> >> +struct gud_drm_req_get_connector {
> >> +	__u8 connector_type;
> > 
> > This is intended for the (Generic) USB Display to report the connector type
> > used for its panel, right? It should not use Linux/DRM-internal values such
> > as DRM_MODE_CONNECTOR_SPI to do so, if it wants to be generic and stable
> > over time.
> 
> DRM_MODE_CONNECTOR_SPI is uabi from include/uapi/drm/drm_mode.h

Sorry - that wasn't clear to me, and that's better than internal values,
but it's still not great to declare that this USB protocol value is
locked to anything else.

It would be better to define these values independently. As long as it is
possible they can of course use the same values as the uapi, but I will
argue that they should be able to diverge, because this USB protocol idea
doesn't inherently have anything to do with Linux.


> > Why does the host software need to know anything about the connector
> > inside the device, anyway? With a microcontroller that could be anything,
> > especially with actual R1 displays.
> > 
> > Would it make sense to introduce DRM_MODE_CONNECTOR_USB on the host, and
> > keep this implementation detail in the device?
> 
> For display adapters it makes sense especially when it has more than one
> connector like the Raspberry Pi which has an hdmi and a sdtv connector.

Right, a GUD device could have multiple connectors, I guess with one USB
interface per connector, but again, what does the host really need to know
about the connector beyond the supported pixel format(s)?

Thanks for mentioning SDTV - I wasn't sure about the rationale for those
TV parts in the patch. Can you elaborate on that?

If the idea is in fact to create "Linux DRM over USB" then by all means
go for it, but in that case please don't call it generic.


> For display panels the connector is USB yes, but there's no type for
> this yet. There's a discussion going on if we should stop adding
> connector types for panels and just add a DRM_MODE_CONNECTOR_PANEL type.
> Not sure if there has been a conclusion on this.

Ack. That does sound like it could be useful for the DRM GUD driver.


> > Make the protocol application specific and avoid implementation specifics.
> > 
> > 
> >> + * struct gud_drm_req_get_connector_status - Connector status
> > 
> > How does this work if and when the status on the device changes?
> 
> If that status can change, the device has to set
> gud_drm_gadget_connector.flags |= GUD_DRM_CONNECTOR_FLAGS_POLL.
> Now the host will poll the status every 10 seconds.

I understand - this is another place where the application requirements call
for an interrupt status endpoint. I think it should be one and the same
endpoint for all status changes.


> >> +/*
> >> + * Internal monochrome transfer format presented to userspace as XRGB8888.
> >> + * Pixel lines are byte aligned.
> >> + */
> >> +#define GUD_DRM_FORMAT_R1	fourcc_code('R', '1', ' ', ' ')
> > 
> > This is also a data format over USB, right? Then it's not really internal,
> > because it also exists on the device. I have several uses for this, but
> > none will be using Linux gadgets, rather microcontrollers.
> 
> I can drop the word internal if it confuses,

I think that would be a big improvement.


> but any non-Linux implementation will have to copy this header file
> and resolve the dependencies. Like I did for my uC hack:
> https://github.com/notro/gud/tree/master/STM32F769I-DISCO/USB_Device/MSC_Standalone/Inc

I think it would be better to document the value instead.


> > I hope this helps.
> 
> Indeed it I does, it helps me revisit the decisions I've made and look
> at them in a new light. I have looked at the code for so long that I
> hardly see it anymore.

I know the feeling and I'm glad to help, but only if the goal is indeed
to create a generic USB:y protocol, mostly if not completely free of Linux
details.

If you're actually after something more closely tied to Linux/DRM then
that's also a fun idea, but much less relevant for me.



Thanks and kind regards

//Peter
Alan Stern June 2, 2020, 2:32 a.m. UTC | #4
On Tue, Jun 02, 2020 at 12:12:07AM +0000, Peter Stuge wrote:

...

> The way I read composite_setup() after try_fun_setup: it calls f->setup()
> when available, and that can return < 0 to stall.
> 
> I expect that composite_setup() and thus f->setup() run when the
> SETUP packet has arrived, thus before the data packet arrives, and if
> composite_setup() stalls then the device/function should never see the
> data packet.
> 
> For an OUT transaction I think the host controller might still send
> the DATA packet, but the device controllers that I know don't make it
> visible to the application in that case.

...

Are you guys interested in comments from other people who know more
about the kernel and how it works with USB?  Your messages have been
far too long to go into in any detail, but I will address this one issue.

The USB protocol forbids a device from sending a STALL response to a
SETUP packet.  The only valid response is ACK.  Thus, there is no way
to prevent the host from sending its DATA packet for a control-OUT
transfer.

A gadget driver can STALL in response to a control-OUT data packet,
but only before it has seen the packet.  Once the driver knows what
the data packet contains, the gadget API doesn't provide any way to
STALL the status stage.  There has been a proposal to change the API
to make this possible, but so far it hasn't gone forward.

Alan Stern
Peter Stuge June 2, 2020, 5:21 a.m. UTC | #5
Hi Alan,

Alan Stern wrote:
> > The way I read composite_setup() after try_fun_setup: it calls f->setup()
> > when available, and that can return < 0 to stall.
> > 
> > I expect that composite_setup() and thus f->setup() run when the
> > SETUP packet has arrived, thus before the data packet arrives, and if
> > composite_setup() stalls then the device/function should never see the
> > data packet.
> > 
> > For an OUT transaction I think the host controller might still send
> > the DATA packet, but the device controllers that I know don't make it
> > visible to the application in that case.
> 
> ...
> 
> Are you guys interested in comments from other people who know more
> about the kernel and how it works with USB?

I am, especially when it comes to the gadget code.


> The USB protocol forbids a device from sending a STALL response to a
> SETUP packet.  The only valid response is ACK.  Thus, there is no way
> to prevent the host from sending its DATA packet for a control-OUT
> transfer.

Right; a STALL handshake only after a DATA packet, but a udc could silently
drop the first DATA packet if instructed to STALL during SETUP processing.
I don't know how common that is for the udc:s supported by gadget, but some
MCU:s behave like that.


> A gadget driver can STALL in response to a control-OUT data packet,
> but only before it has seen the packet.

How can it do that for OUT, and IN if possible there too?

Is it related to f->setup() returning < 0 ?


The spec also allows NAK, but the gadget code seems to not (need to?)
explicitly support that. Can you comment on this as well?


> Once the driver knows what the data packet contains, the gadget API
> doesn't provide any way to STALL the status stage.

Thanks. I think this particular gadget driver doesn't need to decide late.

Ideally the control transfers can even be avoided.


Thanks and kind regards

//Peter
Noralf Trønnes June 2, 2020, 11:46 a.m. UTC | #6
Den 02.06.2020 04.32, skrev Alan Stern:
> On Tue, Jun 02, 2020 at 12:12:07AM +0000, Peter Stuge wrote:
> 
> ...
> 
>> The way I read composite_setup() after try_fun_setup: it calls f->setup()
>> when available, and that can return < 0 to stall.
>>
>> I expect that composite_setup() and thus f->setup() run when the
>> SETUP packet has arrived, thus before the data packet arrives, and if
>> composite_setup() stalls then the device/function should never see the
>> data packet.
>>
>> For an OUT transaction I think the host controller might still send
>> the DATA packet, but the device controllers that I know don't make it
>> visible to the application in that case.
> 
> ...
> 
> Are you guys interested in comments from other people who know more
> about the kernel and how it works with USB?

Absolutely, I want something thats works well in the kernel and doesn't
look odd to kernel USB people.

> Your messages have been
> far too long to go into in any detail, but I will address this one issue.
> 
> The USB protocol forbids a device from sending a STALL response to a
> SETUP packet.  The only valid response is ACK.  Thus, there is no way
> to prevent the host from sending its DATA packet for a control-OUT
> transfer.
> 
> A gadget driver can STALL in response to a control-OUT data packet,
> but only before it has seen the packet.  Once the driver knows what
> the data packet contains, the gadget API doesn't provide any way to
> STALL the status stage.  There has been a proposal to change the API
> to make this possible, but so far it hasn't gone forward.
> 

This confirms what I have seen in the kernel and the reason I added a
status request so I can know the result of the operation the device has
performed.

I have a problem that I've encountered with this status request.
In my first version the gadget would usb_ep_queue() the status value
when the operation was done and as long as this happended within the
host timeout (5s) everything worked fine.

Then I hit a 10s timeout in the gadget when performing a display modeset
operation (wait on missing vblank). The result of this was that the host
timed out and moved on. The gadget however didn't know that the host
gave up, so it queued up the status value. The result of this was that
all further requests from the host would time out.
Do you know a solution to this?
My work around is to just poll on the status request, which returns a
value immediately, until there's a result. The udc driver I use is dwc2.

Noralf.

> Alan Stern
>
Alan Stern June 2, 2020, 3:27 p.m. UTC | #7
On Tue, Jun 02, 2020 at 05:21:50AM +0000, Peter Stuge wrote:
> > The USB protocol forbids a device from sending a STALL response to a
> > SETUP packet.  The only valid response is ACK.  Thus, there is no way
> > to prevent the host from sending its DATA packet for a control-OUT
> > transfer.
> 
> Right; a STALL handshake only after a DATA packet, but a udc could silently
> drop the first DATA packet if instructed to STALL during SETUP processing.
> I don't know how common that is for the udc:s supported by gadget, but some
> MCU:s behave like that.

It happens from time to time, such as when the host sends a SETUP packet 
that the gadget driver doesn't understand.

> > A gadget driver can STALL in response to a control-OUT data packet,
> > but only before it has seen the packet.
> 
> How can it do that for OUT, and IN if possible there too?

In the way described just above: The gadget driver's SETUP handler tells 
the UDC to stall the data packet.

> Is it related to f->setup() returning < 0 ?

Yes; the composite core interprets such a value as meaning to STALL 
endpoint 0.

> The spec also allows NAK, but the gadget code seems to not (need to?)
> explicitly support that. Can you comment on this as well?

If the gadget driver doesn't submit a usb_request then the UDC will 
reply with NAK.

> > Once the driver knows what the data packet contains, the gadget API
> > doesn't provide any way to STALL the status stage.
> 
> Thanks. I think this particular gadget driver doesn't need to decide late.
> 
> Ideally the control transfers can even be avoided.


On Tue, Jun 02, 2020 at 01:46:38PM +0200, Noralf Trønnes wrote:

> > A gadget driver can STALL in response to a control-OUT data packet,
> > but only before it has seen the packet.  Once the driver knows what
> > the data packet contains, the gadget API doesn't provide any way to
> > STALL the status stage.  There has been a proposal to change the API
> > to make this possible, but so far it hasn't gone forward.
> > 
> 
> This confirms what I have seen in the kernel and the reason I added a
> status request so I can know the result of the operation the device has
> performed.

Does this status request use ep0 or some other endpoint?

> I have a problem that I've encountered with this status request.
> In my first version the gadget would usb_ep_queue() the status value
> when the operation was done and as long as this happended within the
> host timeout (5s) everything worked fine.
> 
> Then I hit a 10s timeout in the gadget when performing a display modeset
> operation (wait on missing vblank). The result of this was that the host
> timed out and moved on. The gadget however didn't know that the host
> gave up, so it queued up the status value. The result of this was that
> all further requests from the host would time out.
> Do you know a solution to this?
> My work around is to just poll on the status request, which returns a
> value immediately, until there's a result. The udc driver I use is dwc2.

It's hard to give a precise answer without knowing the details of how 
your driver works.

There are two reasonable approaches you could use.  One is to have a 
vendor-specific control request to get the result of the preceding 
operation.  This works but it has high overhead -- which may not matter 
if it happens infrequently and isn't sensitive to latency.

The other approach is to send the status data over a bulk-IN endpoint.  
It would have to be formatted in such a way that the host could 
recognize it as a status packet and not some other sort of data packet.  
That way, if the host received a stale status value, it could ignore it 
and move on.

You also may want to give some thought to a "resynchronization" 
protocol, for use in situations where the host times out waiting for a 
response from the device while the device is waiting for something else 
(the host, a vblank, or whatever).  This could be a special control 
request, or you could rely on the host doing a complete USB reset.

Alan Stern
Peter Stuge June 2, 2020, 6:38 p.m. UTC | #8
Alan Stern wrote:
> > > A gadget driver can STALL in response to a control-OUT data packet,
> > > but only before it has seen the packet.
> > 
> > How can it do that for OUT, and IN if possible there too?
> 
> In the way described just above: The gadget driver's SETUP handler tells 
> the UDC to stall the data packet.
> 
> > Is it related to f->setup() returning < 0 ?
> 
> Yes; the composite core interprets such a value as meaning to STALL 
> endpoint 0.

Thanks a lot for confirming this.


> > The spec also allows NAK, but the gadget code seems to not (need to?)
> > explicitly support that. Can you comment on this as well?
> 
> If the gadget driver doesn't submit a usb_request then the UDC will 
> reply with NAK.

And thanks for this as well.


> > a status request so I can know the result of the operation the device has
> > performed.
..
> There are two reasonable approaches you could use.  One is to have a 
> vendor-specific control request to get the result of the preceding 
> operation.
..
> The other approach is to send the status data over a bulk-IN endpoint.

I've tried to explain a third approach, which I think fits well because the
status is only a "Ready" flag - ie. a memory barrier or flow control,
to make the host not send data OUT.

I'm proposing that the gadget should NAK on data OUT when it isn't Ready, and
that the host driver shouldn't query status but simply send data when it has.

Per Alans description the NAK happens automatically if the gadget driver has
no usb_request pending while it is processing previously received data.

On the host that NAK makes the host controller retry automatically until
transfer success, timeout or fatal error.


> It would have to be formatted in such a way that the host could 
> recognize it as a status packet and not some other sort of data packet.

For host notification of status changes other than Ready I really like
such an IN endpoint, but preferably an interrupt endpoint.

To avoid the formatting problem each data packet could be one full status
message. That way the host would always receive a known data structure.
Interrupt packets can be max 64 byte. Noralf, do you think that's enough
for everyone in the first version?


//Peter
Noralf Trønnes June 3, 2020, 7:17 p.m. UTC | #9
Den 02.06.2020 02.12, skrev Peter Stuge:
> Hi Noralf,
> 
> Thanks a lot for going into more detail.
> 
> Noralf Trønnes wrote:
>>> Several Linux/DRM internals have "leaked" into the USB protocol - this
>>> should be avoided if you want device implementations other than your
>>> gadget, because those internals can change within Linux in the future,
>>> while the protocol must not.
>>>
>>
>> That's intentional, I see no point in recreating uapi values that won't
>> change:
>>
>> Linux errno: /include/uapi/asm-generic/errno{-base,}.h
>> Pixel formats: include/uapi/drm/drm_fourcc.h
>> Connector types and status: include/uapi/drm/drm_mode.h
> 
> I understand, and it's good that these are uapi values, but I will still
> disagree with using errno and DRM connector types in the USB protocol,
> which is a "namespace" of its own.
> 
> That is an important point here; GUD is three things: a Linux DRM driver,
> a Linux gadget driver and a USB protocol. USB protocols (good ones) are
> OS-agnostic.
> 

I need to be more clear here about the word 'Generic' that I've used.

This is not a OS-agnostic protocol. It's written for Linux. I have had
the BSD's in mind, hence the MIT license, FreeBSD for instance has a DRM
subsystem. Other OS'es might not support all DRM properties, but should
be able to use it as a basic display. They will ofc need to translate
the Linux DRM specifics into their own environment. But first and
foremost this is for Linux.

The host driver is written against the capabilities of the Linux gadget
framework and the DRM subsystem. This will add some peculiarities that a
microcontroller implementation won't face. The focal point of the
project is providing as good performance as possible for Full HD desktop
use.

The word 'Generic' means that it's (should be) possible to make a USB
display for use on Linux without having to write a graphics driver. This
includes all kinds of tiny/small displays that currently are SPI or I2C
interfaced. These will probably use a microcontroller instead of a Linux
SoC to keep cost low.

> 
>>>> If the device transfer buffer can't fit an uncompressed framebuffer
>>>> update, the update is split up into parts that do fit.
>>>
>>> Does "device transfer buffer" refer to something like display RAM on
>>> the device side? If so, its size is a device implementation detail
>>> which shouldn't be exposed over USB.
>>
>> The reason for exposing this is that the Linux gadget driver needs to
>> decompress the transfer into a buffer and the host needs to know how big
>> this is (the host can choose to lower this if it can't allocate a
>> continuous buffer of this size).
> 
> I understand; so it's only required for some compression types - then
> it should at least be completely optional, but in any case I find
> exposing/having to expose this to be awful USB protocol design and I
> hope we can find a better way.
> 
> Maybe it's premature anyway? How would you feel about skipping compression
> to begin with?
> 

Performance is not good without compression so I need to keep that.

> 
>> lz4 (in the kernel) is block compression and can't be used for
>> decompressing just a stream of bytes. There is a lz4 frame protocol
>> which looks like it could be useful, but it's not available in the
>> kernel. I hardly know anything about compression so I'm in no position
>> to add that to the kernel. Maybe someone will add it at a later time if
>> it is useful.
> 
> Why did you choose to use lz4?
> 

The kernel supports it and in actually performs quite well.
Decompression is much faster than compression which fits nicely with not
so powerful USB devices. Low resolution displays might not need
compression at all.

> Whether compression comes now or later, maybe there is a more suitable
> algorithm?
> 

Could be, it's possible to support other compression methods. I'll leave
that to the professionals that need it for their display.

> 
>>> The R1 idea is great!
>>
>> My plan was to remove R1 support from this version of the patchset, but
>> I forgot. The reason is that it's cumbersome to test when the gadget
>> driver doesn't support it.
> 
> Why not support R1 also in the gadget?
> 

The DRM subsystem doesn't have support for it so the gadget wouldn't
know when to use it. Monochrome DRM drivers advertise a XRGB8888 format
and converts this into monochrome. The reason for using this format is
because all userspace supports it. AFAIK no DRM userspace support
monochrome.

> 
>> You mention further down that you have use cases for this, do you have a
>> timeplan for when you will make use of R1?
> 
> No strict plan, but if it helps I could make a demo device and -firmware
> without much effort. You mentioned that you would like to have a
> microcontroller device for testing?
> 

I have done a microcontroller implementation, so I have tried it. It's
quite a different environment to work in than Linux for sure :-) It
might have been a more pleasant experience if I'd spent money on a
professional compiler/IDE I guess.

> 
>>>> - Use donated Openmoko USB pid
>>>
>>> If Linux will be the reference for this protocol then perhaps a PID
>>> under the Linux Foundation VID (1d6b) makes more sense?
>>
>> Do they hand out pid's?
> 
> I don't know. :) The root hub drivers each have one.
> 
> 
>> To me it's no big deal, it can be added later if someones cares about it.
> 
> Okay, hopefully we can do without a PID anyway.
> 

That can't happen AFAIK. This would require the driver to probe all USB
devices with a vendor interface. Who knows what kind a havoc this might
cause on the device in question.

> 
>>> But: A PID applies on device level, not to interfaces.
>>
>> Indeed. This is a USB interface driver though, so it only looks at
>> vendor class interfaces. Then it checks if there's a bulk out endpoint,
>> if not it returns -ENXIO and the device subsytem moves on to another
>> interface driver if any. Next it tries to fetch the display descriptor
>> and if not succesful it returns -ENODEV to give another driver a chance.
> 
> Thanks for clarifying this flow. It's nice not to require particular
> endpoint addresses - that makes the protocol/driver much more generic.
> 
> 
>> I have tried my best to let the driver tolerate other vendor class
>> interfaces on the device.
> 
> Ack, this is clear now.
> 
> 
>> I don't understand why PID should not be necessary, I'm using a vendor
>> class interface and the driver can't probe all of those, so it has to
>> look at specific vid:pid's.
> 
> Why can't the driver probe all vendor class interfaces?
> 
> To probe fewer interfaces, a criteria other than PID can still be defined,
> and doing so would enable immediate plug-and-play for non-gadget and especially
> composite devices, without requiring the addition of PIDs in the host driver.
> 
> I find this possibility especially attractive for composite devices, which
> may already have some VID:PID and a non-GUD primary function/interface that
> is handled by another driver, such that a GUD PID effectively can't be adopted
> for that device.
> 
> One example of such a criteria would be to require that the iInterface
> string descriptor contains the (sub)string "Generic USB Display".
> 

I can consider a driver that looks at all vendor interfaces if you can
find a driver in the kernel that does this.

>>>> +static int gud_get_vendor_descriptor(struct usb_interface *interface,
>>>> +				     struct gud_drm_display_descriptor *desc)
>>>> +{
>>> ..
>>>> +	ret = gud_drm_usb_control_msg(usb, ifnum, true, USB_REQ_GET_DESCRIPTOR,
>>>> +				      GUD_DRM_USB_DT_DISPLAY << 8, buf, sizeof(*desc), false);
>>>
>>> GUD_DRM_USB_DT_DISPLAY is defined as (USB_TYPE_VENDOR | 0x4),
>>> but USB_TYPE_VENDOR only applies to bmRequestType[6:5] in control transfers,
>>> nowhere else. I know of no standardized way to introduce vendor-specific
>>> descriptors. Squatting is possible, but I think it would be nice to do
>>> better here. It is easy enough.
>>>
>>> It could be argued that the vendor specific interface gives flexibility here,
>>> but actually it just means that the semantics of the standardized and
>>> well-defined USB_REQ_GET_DESCRIPTOR have been duplicated by this protocol,
>>> that is not very common - but if you want to go ahead then at least drop
>>> USB_TYPE_VENDOR from the GUD_DRM_USB_DT_DISPLAY definition.
>>
>> I liked that it is well defined and understood, so I didn't have to be clever.
> 
> I tried to explain that it is only well defined for the standardized
> GET_DESCRIPTOR request with device recipient. The concept "descriptor"
> isn't used anywhere else by other USB protocols that I know.
> 
> There are various class-specific descriptors, but they are all standardized
> in USB-IF device class specs, and all of them are only ever retrieved by the
> standardized GET_DESCRIPTOR request.
> 
> Because of that it's actually rather confusing to me to refer to the display
> data structure as a descriptor and even use the standardized descriptor header
> and naming convention when it is actually /not/ a descriptor, since it isn't
> standardized and isn't retrievable with the standardized GET_DESCRIPTOR
> request. Does that make sense?
> 
> 
>> I like it and think I'll keep it, so I'll change the macro:
>>
>> #define GUD_DRM_USB_DT_DISPLAY 0x44
> 
> Okay. The number is arbitrary, since a control request directed to a vendor
> specific interface is vendor specific "by inheritance", thus also arbitrary.
> 
> 
>>> Maybe it's good to think about the data exchange some more - anything not
>>> transfered by standardized USB_REQ_GET_DESCRIPTOR (bmRequestType 10000000B;
>>> Device-to-host data, Standard type, Device recipient) isn't actually
>>> a descriptor, it's vendor-specific, free-format data. Does that enable
>>> any simplifications?
>>
>> Actually it is:
>>
>> 	u8 requesttype = USB_TYPE_VENDOR | USB_RECIP_INTERFACE;
> 
> Right; it's a Vendor-Specific type, Interface recipient request, thus not
> the standardized USB_REQ_GET_DESCRIPTOR, and thus the data is not actually
> a descriptor, hence why calling it one is confusing to me.
> 
> 
>> see gud_drm_usb_control_msg(). I could add
>> GUD_DRM_USB_REQ_GET_DESCRIPTOR instead of using USB_REQ_GET_DESCRIPTOR
>> if that makes it any clearer.
> 
> That would be an improvement, but I would really wish for different
> terminology all together.
> 
> 

Ok, if this is really confusing I need to fix it and I couldn't find a
driver to support my case in the kernel either.

>>>> +static int gud_usb_get_status(struct usb_device *usb, u8 ifnum)

I'm skipping the status poll discussion since I've discovered a bug in
my previous code that broke on timeouts. I will to try and fix that
first and see how it turns out.

>>>> +	shmem->map_cached = true;
>>>
>>> Can you explain what this does exactly?
>>
>> Not excatly, but I can try rather vaguely :-) The memory subsystem is a
>> black box to me. It has to do with the memory page attribute that
>> controls how memory access is handled by the CPU cache. The default for
>> shmem is WriteCombined which makes for slow read access on ARM, but on
>> x86 there doesn't seem to be any penalty judging by my brief testing.
> 
> Hmm. ARM often (always?) can't do byte-aligned access, only 32-bit access.
> Do you know if and how the buffer is aligned? Maybe this can make a difference?
> 
> 

The start of the buffer is word aligned to match the arch, the memory
subsystem takes care of that, otherwise we would have all kinds of trouble.

>>>> +static int gud_drm_probe(struct usb_interface *interface,
>>>> +			 const struct usb_device_id *id)
>>>> +{
> ..
>>>> +		/* Check if the device can support us */
>>>> +		*version = 1;
>>>> +		ret = gud_drm_usb_control_msg(usb, ifnum, false, GUD_DRM_USB_REQ_SET_VERSION,
>>>> +					      0, version, sizeof(*version), true);
> 
> A more USB:y way would btw. be to report the highest supported protocol
> version to the host in some data structure.
> 
> This too could be the iInterface string descriptor.
> 
> 
>>> Could't this work without _get_status()? What does usb_control_msg()
>>> return for a STALL handshake in the data stage?
>>
>> It returns -EPIPE, but as mentioned above, I can't stall a control write
>> request with payload.
> 
> Please check this? It looks to me like it would be possible.
> 

Alan confirmed what I read in the kernel source in his reply.

>>> Or at least in Documentation/ ?
>>
>> Ideally I would have written a spec and made the implementation against it.
>> However since I suck at writing, I even struggle with getting the compiler
>> to understand me, so it won't happen. It would have taken me forever.
> 
> I have a template for USB protocols that I could use, perhaps we can make
> that spec a reality. I think it would be quite valuable, to help folks
> outside Linux who may also want to create a generic usb display.
> 

I'm sorry but that's outside of the scope of the project and the effort
I can put into it.

> 
>>>> +static size_t gud_drm_xrgb8888_to_r124(u8 *dst, const struct drm_format_info *format,
>>>> +				       void *src, struct drm_framebuffer *fb,
>>>> +				       struct drm_rect *rect)
>>>> +{
>>> ..
>>>> +	buf = kmalloc(width * height, GFP_KERNEL);
>>>> +	if (!buf)
>>>> +		return len; /* To keep logic simple, just transmit garbage */
>>>
>>> Ouch! Shouldn't this bubble up somehow? If there is memory pressure
>>> then I really think something above should fail.
> 
> No comment on this?
> 

I wanted to see if you were going to use R1 in the near future,
otherwise I will remove this in the next version. I guess I'll fix this
if it stays.

>>>> +static int gud_drm_fb_flush(struct gud_drm_device *gdrm, struct drm_framebuffer *fb,

I need to delay commenting on the flow control issues for now. I'll pick
it up later after some more study.

>>> Why does the host software need to know anything about the connector
>>> inside the device, anyway? With a microcontroller that could be anything,
>>> especially with actual R1 displays.
>>>
>>> Would it make sense to introduce DRM_MODE_CONNECTOR_USB on the host, and
>>> keep this implementation detail in the device?
>>
>> For display adapters it makes sense especially when it has more than one
>> connector like the Raspberry Pi which has an hdmi and a sdtv connector.
> 
> Right, a GUD device could have multiple connectors, I guess with one USB
> interface per connector, but again, what does the host really need to know
> about the connector beyond the supported pixel format(s)?
> 
> Thanks for mentioning SDTV - I wasn't sure about the rationale for those
> TV parts in the patch. Can you elaborate on that?
> 

Those are properties exposed by the DRM subsystem for use on TV
connectors. I actually don't know much details, I've just exposed them
and tested that I can set margins.

> If the idea is in fact to create "Linux DRM over USB" then by all means
> go for it, but in that case please don't call it generic.
> 
I disagree, I believe this is generic, within the Linux world.

>>> I hope this helps.
>>
>> Indeed it I does, it helps me revisit the decisions I've made and look
>> at them in a new light. I have looked at the code for so long that I
>> hardly see it anymore.
> 
> I know the feeling and I'm glad to help, but only if the goal is indeed
> to create a generic USB:y protocol, mostly if not completely free of Linux
> details.
> 
> If you're actually after something more closely tied to Linux/DRM then
> that's also a fun idea, but much less relevant for me.
> 

I'm sorry, but this is Linux first and foremost.

Noralf.

> 
> 
> Thanks and kind regards
> 
> //Peter
>
Noralf Trønnes June 5, 2020, 12:03 p.m. UTC | #10
Den 02.06.2020 20.38, skrev Peter Stuge:
> Alan Stern wrote:
>>>> A gadget driver can STALL in response to a control-OUT data packet,
>>>> but only before it has seen the packet.
>>>
>>> How can it do that for OUT, and IN if possible there too?
>>
>> In the way described just above: The gadget driver's SETUP handler tells 
>> the UDC to stall the data packet.
>>
>>> Is it related to f->setup() returning < 0 ?
>>
>> Yes; the composite core interprets such a value as meaning to STALL 
>> endpoint 0.
> 
> Thanks a lot for confirming this.
> 
> 
>>> The spec also allows NAK, but the gadget code seems to not (need to?)
>>> explicitly support that. Can you comment on this as well?
>>
>> If the gadget driver doesn't submit a usb_request then the UDC will 
>> reply with NAK.
> 
> And thanks for this as well.
> 
> 
>>> a status request so I can know the result of the operation the device has
>>> performed.
> ..
>> There are two reasonable approaches you could use.  One is to have a 
>> vendor-specific control request to get the result of the preceding 
>> operation.
> ..
>> The other approach is to send the status data over a bulk-IN endpoint.
> 
> I've tried to explain a third approach, which I think fits well because the
> status is only a "Ready" flag - ie. a memory barrier or flow control,
> to make the host not send data OUT.
> 
> I'm proposing that the gadget should NAK on data OUT when it isn't Ready, and
> that the host driver shouldn't query status but simply send data when it has.
> 
> Per Alans description the NAK happens automatically if the gadget driver has
> no usb_request pending while it is processing previously received data.
> 
> On the host that NAK makes the host controller retry automatically until
> transfer success, timeout or fatal error.
> 
> 
>> It would have to be formatted in such a way that the host could 
>> recognize it as a status packet and not some other sort of data packet.
> 
> For host notification of status changes other than Ready I really like
> such an IN endpoint, but preferably an interrupt endpoint.
> 
> To avoid the formatting problem each data packet could be one full status
> message. That way the host would always receive a known data structure.
> Interrupt packets can be max 64 byte. Noralf, do you think that's enough
> for everyone in the first version?
> 

I'm going through some treatment that turned out to be worse than
expected, so sadly I have to put this project on hold until my body
recovers.

Noralf.

Patch
diff mbox series

diff --git a/MAINTAINERS b/MAINTAINERS
index caeeac9f6b46..b15bf9b2229b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5291,6 +5291,14 @@  S:	Maintained
 F:	drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c
 F:	Documentation/devicetree/bindings/display/panel/feiyang,fy07024di26a30d.yaml
 
+DRM DRIVER FOR GENERIC USB DISPLAY
+M:	Noralf Trønnes <noralf@tronnes.org>
+S:	Maintained
+W:	https://github.com/notro/gud/wiki
+T:	git git://anongit.freedesktop.org/drm/drm-misc
+F:	drivers/gpu/drm/gud/
+F:	include/drm/gud_drm.h
+
 DRM DRIVER FOR GRAIN MEDIA GM12U320 PROJECTORS
 M:	Hans de Goede <hdegoede@redhat.com>
 S:	Maintained
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 4bffa08f610a..33e63b68a82d 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -391,6 +391,8 @@  source "drivers/gpu/drm/mcde/Kconfig"
 
 source "drivers/gpu/drm/tidss/Kconfig"
 
+source "drivers/gpu/drm/gud/Kconfig"
+
 # Keep legacy drivers last
 
 menuconfig DRM_LEGACY
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 53d8fa170143..41a87763f0e5 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -124,3 +124,4 @@  obj-$(CONFIG_DRM_PANFROST) += panfrost/
 obj-$(CONFIG_DRM_ASPEED_GFX) += aspeed/
 obj-$(CONFIG_DRM_MCDE) += mcde/
 obj-$(CONFIG_DRM_TIDSS) += tidss/
+obj-y			+= gud/
diff --git a/drivers/gpu/drm/gud/Kconfig b/drivers/gpu/drm/gud/Kconfig
new file mode 100644
index 000000000000..767f1c067ed9
--- /dev/null
+++ b/drivers/gpu/drm/gud/Kconfig
@@ -0,0 +1,14 @@ 
+# SPDX-License-Identifier: GPL-2.0
+
+config DRM_GUD
+	tristate "Generic USB Display"
+	depends on DRM && USB
+	select LZ4_COMPRESS
+	select DRM_KMS_HELPER
+	select DRM_GEM_SHMEM_HELPER
+	select BACKLIGHT_CLASS_DEVICE
+	help
+	  This is a DRM display driver for Generic USB Displays or display
+	  adapters.
+
+	  If M is selected the module will be called gud_drm.
diff --git a/drivers/gpu/drm/gud/Makefile b/drivers/gpu/drm/gud/Makefile
new file mode 100644
index 000000000000..73ed7ef3da94
--- /dev/null
+++ b/drivers/gpu/drm/gud/Makefile
@@ -0,0 +1,4 @@ 
+# SPDX-License-Identifier: GPL-2.0
+
+gud_drm-objs			:= gud_drm_drv.o gud_drm_pipe.o gud_drm_connector.o
+obj-$(CONFIG_DRM_GUD)		+= gud_drm.o
diff --git a/drivers/gpu/drm/gud/gud_drm_connector.c b/drivers/gpu/drm/gud/gud_drm_connector.c
new file mode 100644
index 000000000000..33cc4e13ffcc
--- /dev/null
+++ b/drivers/gpu/drm/gud/gud_drm_connector.c
@@ -0,0 +1,726 @@ 
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright 2020 Noralf Trønnes
+ */
+
+#include <linux/backlight.h>
+
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_state_helper.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_encoder.h>
+#include <drm/drm_file.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_print.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+#include <drm/gud_drm.h>
+
+#include "gud_drm_internal.h"
+
+struct gud_drm_connector {
+	struct drm_connector connector;
+	struct drm_encoder encoder;
+	struct backlight_device *backlight;
+
+	u32 flags;
+
+	/* Supported properties */
+	u16 *properties;
+	unsigned int num_properties;
+
+	/* Initial gadget tv state if applicable, applied on state reset */
+	struct drm_tv_connector_state initial_tv_state;
+
+	/*
+	 * Initial gadget backlight brightness if applicable, applied on state reset.
+	 * The value -ENODEV is used internally to signal no backlight.
+	 */
+	int initial_brightness;
+
+	/* Supported display modes in transfer format */
+	struct gud_drm_display_mode *modes;
+	unsigned int num_modes;
+
+	/* EDID */
+	void *edid;
+	size_t edid_len;
+};
+
+static inline struct gud_drm_connector *to_gud_drm_connector(struct drm_connector *connector)
+{
+	return container_of(connector, struct gud_drm_connector, connector);
+}
+
+static int gud_drm_connector_backlight_update_status(struct backlight_device *bd)
+{
+	struct drm_connector *connector = bl_get_data(bd);
+	struct drm_connector_state *connector_state;
+	struct drm_device *dev = connector->dev;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_atomic_state *state;
+	int ret;
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return -ENOMEM;
+
+	drm_modeset_acquire_init(&ctx, 0);
+	state->acquire_ctx = &ctx;
+retry:
+	connector_state = drm_atomic_get_connector_state(state, connector);
+	if (IS_ERR(connector_state)) {
+		ret = PTR_ERR(connector_state);
+		goto out;
+	}
+
+	/* Reuse tv.brightness to avoid having to subclass */
+	connector_state->tv.brightness = bd->props.brightness;
+
+	ret = drm_atomic_commit(state);
+out:
+	if (ret == -EDEADLK) {
+		drm_atomic_state_clear(state);
+		drm_modeset_backoff(&ctx);
+		goto retry;
+	}
+
+	drm_atomic_state_put(state);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
+	return ret;
+}
+
+static int gud_drm_connector_backlight_get_brightness(struct backlight_device *bd)
+{
+	struct drm_connector *connector = bl_get_data(bd);
+	struct drm_device *dev = connector->dev;
+	int brightness;
+
+	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+	brightness = connector->state->tv.brightness;
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
+	return brightness;
+}
+
+static const struct backlight_ops gud_drm_connector_backlight_ops = {
+	.get_brightness = gud_drm_connector_backlight_get_brightness,
+	.update_status	= gud_drm_connector_backlight_update_status,
+};
+
+static int gud_drm_connector_backlight_register(struct gud_drm_connector *gconn)
+{
+	struct drm_connector *connector = &gconn->connector;
+	struct backlight_device *bd;
+	const char *name;
+	const struct backlight_properties props = {
+		.type = BACKLIGHT_RAW,
+		.scale = BACKLIGHT_SCALE_NON_LINEAR,
+		.max_brightness = 100,
+	};
+
+	name = kasprintf(GFP_KERNEL, "card%d-%s-backlight",
+			 connector->dev->primary->index, connector->name);
+	if (!name)
+		return -ENOMEM;
+
+	bd = backlight_device_register(name, connector->kdev, connector,
+				       &gud_drm_connector_backlight_ops, &props);
+	kfree(name);
+	if (IS_ERR(bd))
+		return PTR_ERR(bd);
+
+	gconn->backlight = bd;
+
+	return 0;
+}
+
+static void gud_drm_connector_modes_clear(struct gud_drm_connector *gconn)
+{
+	kfree(gconn->modes);
+	gconn->modes = NULL;
+	gconn->num_modes = 0;
+}
+
+static int gud_drm_connector_modes_get(struct gud_drm_connector *gconn, unsigned int num_modes)
+{
+	struct gud_drm_device *gdrm = to_gud_drm_device(gconn->connector.dev);
+	unsigned int index = gconn->connector.index;
+	int ret = 0;
+
+	if (!num_modes)
+		goto clear;
+
+	gud_drm_connector_modes_clear(gconn);
+
+	gconn->modes = kmalloc_array(num_modes, sizeof(*gconn->modes), GFP_KERNEL);
+	if (!gconn->modes)
+		return -ENOMEM;
+
+	gconn->num_modes = num_modes;
+
+	ret = gud_drm_usb_get(gdrm, GUD_DRM_USB_REQ_GET_CONNECTOR_MODES, index,
+			      gconn->modes, num_modes * sizeof(*gconn->modes));
+	if (ret)
+		goto clear;
+
+	return 0;
+clear:
+	gud_drm_connector_modes_clear(gconn);
+
+	return ret;
+}
+
+static void gud_drm_connector_edid_clear(struct gud_drm_connector *gconn)
+{
+	kfree(gconn->edid);
+	gconn->edid = NULL;
+	gconn->edid_len = 0;
+}
+
+static int gud_drm_connector_edid_get(struct gud_drm_connector *gconn, size_t len)
+{
+	struct gud_drm_device *gdrm = to_gud_drm_device(gconn->connector.dev);
+	unsigned int index = gconn->connector.index;
+	int ret = 0;
+
+	if (!len)
+		goto clear;
+
+	gud_drm_connector_edid_clear(gconn);
+
+	gconn->edid = kmalloc(len, GFP_KERNEL);
+	if (!gconn->edid)
+		return -ENOMEM;
+
+	gconn->edid_len = len;
+
+	ret = gud_drm_usb_get(gdrm, GUD_DRM_USB_REQ_GET_CONNECTOR_EDID, index, gconn->edid, len);
+	if (ret)
+		goto clear;
+
+	return 0;
+clear:
+	gud_drm_connector_edid_clear(gconn);
+
+	return ret;
+}
+
+static int gud_drm_connector_detect_safe(struct drm_connector *connector, bool force)
+{
+	struct gud_drm_connector *gconn = to_gud_drm_connector(connector);
+	struct gud_drm_device *gdrm = to_gud_drm_device(connector->dev);
+	struct gud_drm_req_get_connector_status req;
+	u16 num_modes, edid_len;
+	int status, ret;
+	bool changed;
+
+	if (force) {
+		ret = gud_drm_usb_set(gdrm, GUD_DRM_USB_REQ_SET_CONNECTOR_FORCE_DETECT,
+				      connector->index, NULL, 0);
+		if (ret)
+			goto free;
+	}
+
+	ret = gud_drm_usb_get(gdrm, GUD_DRM_USB_REQ_GET_CONNECTOR_STATUS,
+			      connector->index, &req, sizeof(req));
+	if (ret)
+		goto free;
+
+	changed = req.status & GUD_DRM_CONNECTOR_STATUS_CHANGED;
+	status = req.status & GUD_DRM_CONNECTOR_STATUS_MASK;
+	if (status > connector_status_unknown)
+		status = connector_status_unknown;
+	num_modes = le16_to_cpu(req.num_modes);
+	edid_len = le16_to_cpu(req.edid_len);
+
+	if (!num_modes && !edid_len) {
+		ret = connector_status_disconnected;
+		goto free;
+	}
+
+	if (!changed && connector->status == status &&
+	    gconn->num_modes == num_modes && gconn->edid_len == edid_len)
+		return status;
+
+	ret = gud_drm_connector_modes_get(gconn, num_modes);
+	if (ret)
+		goto free;
+
+	ret = gud_drm_connector_edid_get(gconn, edid_len);
+	if (ret)
+		goto free;
+
+	return status;
+free:
+	gud_drm_connector_modes_clear(gconn);
+	gud_drm_connector_edid_clear(gconn);
+
+	return ret < 0 ? connector_status_unknown : ret;
+}
+
+static int gud_drm_connector_detect(struct drm_connector *connector,
+				    struct drm_modeset_acquire_ctx *ctx, bool force)
+{
+	int idx, ret;
+
+	if (!drm_dev_enter(connector->dev, &idx))
+		return -ENODEV;
+
+	ret = gud_drm_connector_detect_safe(connector, force);
+
+	drm_dev_exit(idx);
+
+	return ret;
+}
+
+static int gud_drm_connector_get_modes(struct drm_connector *connector)
+{
+	struct gud_drm_connector *gconn = to_gud_drm_connector(connector);
+	unsigned int i, num_modes = 0;
+
+	if (!gconn->num_modes) {
+		num_modes = drm_add_edid_modes(connector, gconn->edid);
+		goto out;
+	}
+
+	for (i = 0; i < gconn->num_modes; i++) {
+		struct drm_display_mode *mode;
+
+		mode = drm_mode_create(connector->dev);
+		if (!mode)
+			goto out;
+
+		gud_drm_to_display_mode(mode, &gconn->modes[i]);
+
+		drm_mode_probed_add(connector, mode);
+		num_modes++;
+	}
+out:
+	if (gconn->edid_len)
+		drm_connector_update_edid_property(connector, gconn->edid);
+
+	return num_modes;
+}
+
+static int gud_drm_connector_atomic_check(struct drm_connector *connector,
+					  struct drm_atomic_state *state)
+{
+	struct drm_connector_state *new_state;
+	struct drm_crtc_state *new_crtc_state;
+	struct drm_connector_state *old_state;
+
+	new_state = drm_atomic_get_new_connector_state(state, connector);
+	if (!new_state->crtc)
+		return 0;
+
+	old_state = drm_atomic_get_old_connector_state(state, connector);
+	new_crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
+
+	if (old_state->tv.subconnector != new_state->tv.subconnector ||
+	    old_state->tv.margins.left != new_state->tv.margins.left ||
+	    old_state->tv.margins.right != new_state->tv.margins.right ||
+	    old_state->tv.margins.top != new_state->tv.margins.top ||
+	    old_state->tv.margins.bottom != new_state->tv.margins.bottom ||
+	    old_state->tv.mode != new_state->tv.mode ||
+	    old_state->tv.brightness != new_state->tv.brightness ||
+	    old_state->tv.contrast != new_state->tv.contrast ||
+	    old_state->tv.flicker_reduction != new_state->tv.flicker_reduction ||
+	    old_state->tv.overscan != new_state->tv.overscan ||
+	    old_state->tv.saturation != new_state->tv.saturation ||
+	    old_state->tv.hue != new_state->tv.hue)
+		new_crtc_state->connectors_changed = true;
+
+	return 0;
+}
+
+static const struct drm_connector_helper_funcs gud_drm_connector_helper_funcs = {
+	.detect_ctx = gud_drm_connector_detect,
+	.get_modes = gud_drm_connector_get_modes,
+	.atomic_check = gud_drm_connector_atomic_check,
+};
+
+static int gud_drm_connector_late_register(struct drm_connector *connector)
+{
+	struct gud_drm_connector *gconn = to_gud_drm_connector(connector);
+
+	if (gconn->initial_brightness < 0)
+		return 0;
+
+	return gud_drm_connector_backlight_register(gconn);
+}
+
+static void gud_drm_connector_early_unregister(struct drm_connector *connector)
+{
+	struct gud_drm_connector *gconn = to_gud_drm_connector(connector);
+
+	backlight_device_unregister(gconn->backlight);
+}
+
+static void gud_drm_connector_destroy(struct drm_connector *connector)
+{
+	struct gud_drm_connector *gconn = to_gud_drm_connector(connector);
+
+	drm_connector_cleanup(connector);
+	gud_drm_connector_modes_clear(gconn);
+	gud_drm_connector_edid_clear(gconn);
+	kfree(gconn->properties);
+	kfree(gconn);
+}
+
+static void gud_drm_connector_reset(struct drm_connector *connector)
+{
+	struct gud_drm_connector *gconn = to_gud_drm_connector(connector);
+
+	drm_atomic_helper_connector_reset(connector);
+	connector->state->tv = gconn->initial_tv_state;
+	/* Set margins from command line */
+	drm_atomic_helper_connector_tv_reset(connector);
+	if (gconn->initial_brightness >= 0)
+		connector->state->tv.brightness = gconn->initial_brightness;
+}
+
+static const struct drm_connector_funcs gud_drm_connector_funcs = {
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.late_register = gud_drm_connector_late_register,
+	.early_unregister = gud_drm_connector_early_unregister,
+	.destroy = gud_drm_connector_destroy,
+	.reset = gud_drm_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+/*
+ * The tv.mode property is shared among the connectors and its enum names are
+ * driver specific. This means that if more than one connector uses tv.mode,
+ * the enum names has to be the same.
+ */
+static int gud_drm_connector_add_tv_mode(struct gud_drm_device *gdrm,
+					 struct drm_connector *connector, u64 val)
+{
+	unsigned int i, num_modes;
+	const char **modes;
+	size_t buf_len;
+	char *buf;
+	int ret;
+
+	num_modes = val >> GUD_DRM_USB_CONNECTOR_TV_MODE_NUM_SHIFT;
+
+	if (!num_modes)
+		return -EINVAL;
+
+	buf_len = num_modes * DRM_PROP_NAME_LEN;
+	modes = kmalloc_array(num_modes, sizeof(*modes), GFP_KERNEL);
+	buf = kmalloc(buf_len, GFP_KERNEL);
+	if (!modes || !buf) {
+		ret = -ENOMEM;
+		goto free;
+	}
+
+	ret = gud_drm_usb_get(gdrm, GUD_DRM_USB_REQ_GET_CONNECTOR_TV_MODE_VALUES,
+			      connector->index, buf, buf_len);
+	if (ret)
+		goto free;
+
+	for (i = 0; i < num_modes; i++)
+		modes[i] = &buf[i * DRM_PROP_NAME_LEN];
+
+	ret = drm_mode_create_tv_properties(connector->dev, num_modes, modes);
+free:
+	kfree(modes);
+	kfree(buf);
+
+	return ret;
+}
+
+static struct drm_property *
+gud_drm_connector_property_lookup(struct drm_connector *connector, u16 prop)
+{
+	struct drm_mode_config *config = &connector->dev->mode_config;
+
+	switch (prop) {
+	case GUD_DRM_PROPERTY_TV_SELECT_SUBCONNECTOR:
+		return config->tv_select_subconnector_property;
+	case GUD_DRM_PROPERTY_TV_LEFT_MARGIN:
+		return config->tv_left_margin_property;
+	case GUD_DRM_PROPERTY_TV_RIGHT_MARGIN:
+		return config->tv_right_margin_property;
+	case GUD_DRM_PROPERTY_TV_TOP_MARGIN:
+		return config->tv_top_margin_property;
+	case GUD_DRM_PROPERTY_TV_BOTTOM_MARGIN:
+		return config->tv_bottom_margin_property;
+	case GUD_DRM_PROPERTY_TV_MODE:
+		return config->tv_mode_property;
+	case GUD_DRM_PROPERTY_TV_BRIGHTNESS:
+		return config->tv_brightness_property;
+	case GUD_DRM_PROPERTY_TV_CONTRAST:
+		return config->tv_contrast_property;
+	case GUD_DRM_PROPERTY_TV_FLICKER_REDUCTION:
+		return config->tv_flicker_reduction_property;
+	case GUD_DRM_PROPERTY_TV_OVERSCAN:
+		return config->tv_overscan_property;
+	case GUD_DRM_PROPERTY_TV_SATURATION:
+		return config->tv_saturation_property;
+	case GUD_DRM_PROPERTY_TV_HUE:
+		return config->tv_hue_property;
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+}
+
+static unsigned int *gud_drm_connector_tv_state_val(u16 prop, struct drm_tv_connector_state *state)
+{
+	switch (prop) {
+	case GUD_DRM_PROPERTY_TV_SELECT_SUBCONNECTOR:
+		return &state->subconnector;
+	case GUD_DRM_PROPERTY_TV_LEFT_MARGIN:
+		return &state->margins.left;
+	case GUD_DRM_PROPERTY_TV_RIGHT_MARGIN:
+		return &state->margins.right;
+	case GUD_DRM_PROPERTY_TV_TOP_MARGIN:
+		return &state->margins.top;
+	case GUD_DRM_PROPERTY_TV_BOTTOM_MARGIN:
+		return &state->margins.bottom;
+	case GUD_DRM_PROPERTY_TV_MODE:
+		return &state->mode;
+	case GUD_DRM_PROPERTY_TV_BRIGHTNESS:
+		return &state->brightness;
+	case GUD_DRM_PROPERTY_TV_CONTRAST:
+		return &state->contrast;
+	case GUD_DRM_PROPERTY_TV_FLICKER_REDUCTION:
+		return &state->flicker_reduction;
+	case GUD_DRM_PROPERTY_TV_OVERSCAN:
+		return &state->overscan;
+	case GUD_DRM_PROPERTY_TV_SATURATION:
+		return &state->saturation;
+	case GUD_DRM_PROPERTY_TV_HUE:
+		return &state->hue;
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+}
+
+static int gud_drm_connector_add_properties(struct gud_drm_device *gdrm,
+					    struct gud_drm_connector *gconn,
+					    unsigned int num_properties)
+{
+	struct drm_device *drm = &gdrm->drm;
+	struct drm_connector *connector = &gconn->connector;
+	struct gud_drm_property *properties;
+	bool need_tv_props = false;
+	unsigned int i;
+	int ret;
+
+	gconn->properties = kcalloc(num_properties, sizeof(*gconn->properties), GFP_KERNEL);
+	if (!gconn->properties)
+		return -ENOMEM;
+
+	properties = kcalloc(num_properties, sizeof(*properties), GFP_KERNEL);
+	if (!properties)
+		return -ENOMEM;
+
+	ret = gud_drm_usb_get(gdrm, GUD_DRM_USB_REQ_GET_CONNECTOR_PROPERTIES, connector->index,
+			      properties, num_properties * sizeof(*properties));
+	if (ret)
+		goto out;
+
+	for (i = 0; i < num_properties; i++) {
+		u16 prop = le16_to_cpu(properties[i].prop);
+		u64 val = le64_to_cpu(properties[i].val);
+
+		drm_dbg(drm, "property: %u = %llu(0x%llx)\n", prop, val, val);
+
+		switch (prop) {
+		case GUD_DRM_PROPERTY_TV_LEFT_MARGIN:
+			fallthrough;
+		case GUD_DRM_PROPERTY_TV_RIGHT_MARGIN:
+			fallthrough;
+		case GUD_DRM_PROPERTY_TV_TOP_MARGIN:
+			fallthrough;
+		case GUD_DRM_PROPERTY_TV_BOTTOM_MARGIN:
+			ret = drm_mode_create_tv_margin_properties(drm);
+			if (ret)
+				goto out;
+			break;
+		case GUD_DRM_PROPERTY_TV_MODE:
+			ret = gud_drm_connector_add_tv_mode(gdrm, connector, val);
+			if (ret)
+				goto out;
+			break;
+		case GUD_DRM_PROPERTY_TV_SELECT_SUBCONNECTOR:
+			fallthrough;
+		case GUD_DRM_PROPERTY_TV_BRIGHTNESS:
+			fallthrough;
+		case GUD_DRM_PROPERTY_TV_CONTRAST:
+			fallthrough;
+		case GUD_DRM_PROPERTY_TV_FLICKER_REDUCTION:
+			fallthrough;
+		case GUD_DRM_PROPERTY_TV_OVERSCAN:
+			fallthrough;
+		case GUD_DRM_PROPERTY_TV_SATURATION:
+			fallthrough;
+		case GUD_DRM_PROPERTY_TV_HUE:
+			need_tv_props = true;
+			break;
+		case GUD_DRM_PROPERTY_BACKLIGHT_BRIGHTNESS:
+			if (val > 100) {
+				ret = -EINVAL;
+				goto out;
+			}
+			gconn->initial_brightness = val;
+			break;
+		default:
+			/* New ones might show up in future devices, skip those we don't know. */
+			drm_dbg(drm, "Unknown property: %u\n", prop);
+			continue;
+		}
+
+		gconn->properties[gconn->num_properties++] = prop;
+	}
+
+	if (!gconn->num_properties)
+		goto out;
+
+	if (need_tv_props) {
+		/* This is a no-op if already added. */
+		ret = drm_mode_create_tv_properties(drm, 0, NULL);
+		if (ret)
+			goto out;
+	}
+
+	for (i = 0; i < num_properties; i++) {
+		u16 prop = le16_to_cpu(properties[i].prop);
+		u64 val = le64_to_cpu(properties[i].val);
+		struct drm_property *property;
+		unsigned int *state_val;
+
+		switch (prop) {
+		case GUD_DRM_PROPERTY_BACKLIGHT_BRIGHTNESS:
+			/* not a DRM property */
+			continue;
+		case GUD_DRM_PROPERTY_TV_MODE:
+			val = val & (BIT(GUD_DRM_USB_CONNECTOR_TV_MODE_NUM_SHIFT) - 1);
+			break;
+		}
+
+		property = gud_drm_connector_property_lookup(connector, prop);
+		if (IS_ERR(property))
+			continue;
+
+		state_val = gud_drm_connector_tv_state_val(prop, &gconn->initial_tv_state);
+		if (IS_ERR(state_val))
+			continue;
+
+		*state_val = val;
+		drm_object_attach_property(&connector->base, property, 0);
+	}
+out:
+	kfree(properties);
+
+	return ret;
+}
+
+int gud_drm_connector_fill_properties(struct drm_connector *connector,
+				      struct drm_connector_state *connector_state,
+				      struct gud_drm_property *properties)
+{
+	struct gud_drm_connector *gconn;
+	unsigned int i;
+
+	gconn = to_gud_drm_connector(connector);
+
+	/* Only interested in the count? */
+	if (!connector_state)
+		return gconn->num_properties;
+
+	for (i = 0; i < gconn->num_properties; i++) {
+		u16 prop = gconn->properties[i];
+		u64 val;
+
+		if (prop == GUD_DRM_PROPERTY_BACKLIGHT_BRIGHTNESS) {
+			val = connector_state->tv.brightness;
+		} else {
+			unsigned int *state_val;
+
+			state_val = gud_drm_connector_tv_state_val(prop, &connector_state->tv);
+			if (WARN_ON_ONCE(IS_ERR(state_val)))
+				return PTR_ERR(state_val);
+
+			val = *state_val;
+		}
+
+		properties[i].prop = cpu_to_le16(prop);
+		properties[i].val = cpu_to_le64(val);
+	}
+
+	return gconn->num_properties;
+}
+
+int gud_drm_connector_create(struct gud_drm_device *gdrm, unsigned int index)
+{
+	struct gud_drm_req_get_connector desc;
+	struct drm_device *drm = &gdrm->drm;
+	struct gud_drm_connector *gconn;
+	struct drm_connector *connector;
+	struct drm_encoder *encoder;
+	int ret;
+
+	ret = gud_drm_usb_get(gdrm, GUD_DRM_USB_REQ_GET_CONNECTOR, index, &desc, sizeof(desc));
+	if (ret)
+		return ret;
+
+	drm_dbg(drm, "index=%u type=%u flags=0x%x num_properties=%u\n", index,
+		desc.connector_type, le32_to_cpu(desc.flags), desc.num_properties);
+
+	/* REVISIT: This needs to be updated as new types are added */
+	if (desc.connector_type > DRM_MODE_CONNECTOR_SPI)
+		return -EINVAL;
+
+	gconn = kzalloc(sizeof(*gconn), GFP_KERNEL);
+	if (!gconn)
+		return -ENOMEM;
+
+	gconn->initial_brightness = -ENODEV;
+	gconn->flags = le32_to_cpu(desc.flags);
+	connector = &gconn->connector;
+
+	drm_connector_helper_add(connector, &gud_drm_connector_helper_funcs);
+	ret = drm_connector_init(drm, connector, &gud_drm_connector_funcs, desc.connector_type);
+	if (ret) {
+		kfree(connector);
+		return ret;
+	}
+
+	if (WARN_ON(connector->index != index))
+		return -EINVAL;
+
+	if (gconn->flags & GUD_DRM_CONNECTOR_FLAGS_POLL)
+		connector->polled = (DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT);
+
+	if (desc.num_properties) {
+		ret = gud_drm_connector_add_properties(gdrm, gconn, desc.num_properties);
+		if (ret) {
+			dev_err(drm->dev, "Failed to add connector/%u properties\n", index);
+			return ret;
+		}
+	}
+
+	/* The first connector is attached to the existing simple pipe encoder */
+	if (!connector->index) {
+		encoder = &gdrm->pipe.encoder;
+	} else {
+		encoder = &gconn->encoder;
+
+		ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE);
+		if (ret)
+			return ret;
+
+		encoder->possible_crtcs = 1;
+	}
+
+	return drm_connector_attach_encoder(connector, encoder);
+}
diff --git a/drivers/gpu/drm/gud/gud_drm_drv.c b/drivers/gpu/drm/gud/gud_drm_drv.c
new file mode 100644
index 000000000000..e2d72e2ec219
--- /dev/null
+++ b/drivers/gpu/drm/gud/gud_drm_drv.c
@@ -0,0 +1,648 @@ 
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright 2020 Noralf Trønnes
+ */
+
+#include <linux/dma-buf.h>
+#include <linux/lz4.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/string_helpers.h>
+#include <linux/usb.h>
+#include <linux/workqueue.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_debugfs.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_managed.h>
+#include <drm/drm_print.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+#include <drm/gud_drm.h>
+
+#include "gud_drm_internal.h"
+
+/* Only used internally */
+static const struct drm_format_info gud_drm_format_r1 = {
+	.format = GUD_DRM_FORMAT_R1,
+	.num_planes = 1,
+	.char_per_block = { 1, 0, 0 },
+	.block_w = { 8, 0, 0 },
+	.block_h = { 1, 0, 0 },
+	.hsub = 1,
+	.vsub = 1,
+};
+
+static int gud_drm_usb_control_msg(struct usb_device *usb, u8 ifnum, bool in,
+				   u8 request, u16 value, void *buf, size_t len,
+				   bool check_len)
+{
+	u8 requesttype = USB_TYPE_VENDOR | USB_RECIP_INTERFACE;
+	unsigned int pipe;
+	int ret;
+
+	if (in) {
+		pipe = usb_rcvctrlpipe(usb, 0);
+		requesttype |= USB_DIR_IN;
+	} else {
+		pipe = usb_sndctrlpipe(usb, 0);
+		requesttype |= USB_DIR_OUT;
+	}
+
+	ret = usb_control_msg(usb, pipe, request, requesttype, value,
+			      ifnum, buf, len, USB_CTRL_GET_TIMEOUT);
+
+	if (check_len && ret >= 0) {
+		if (ret != len)
+			ret = -EIO;
+		else
+			ret = 0;
+	}
+
+	return ret;
+}
+
+static int gud_get_vendor_descriptor(struct usb_interface *interface,
+				     struct gud_drm_display_descriptor *desc)
+{
+	u8 ifnum = interface->cur_altsetting->desc.bInterfaceNumber;
+	struct usb_device *usb = interface_to_usbdev(interface);
+	void *buf;
+	int ret;
+
+	buf = kmalloc(sizeof(*desc), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = gud_drm_usb_control_msg(usb, ifnum, true, USB_REQ_GET_DESCRIPTOR,
+				      GUD_DRM_USB_DT_DISPLAY << 8, buf, sizeof(*desc), false);
+
+	memcpy(desc, buf, sizeof(*desc));
+	kfree(buf);
+
+	if (ret < 0)
+		return ret;
+
+	if (ret != sizeof(*desc) || desc->bLength != sizeof(*desc) ||
+	    desc->bDescriptorType != GUD_DRM_USB_DT_DISPLAY)
+		return -ENODATA;
+
+	DRM_DEV_DEBUG_DRIVER(&interface->dev,
+			     "Version=%u Compression=0x%x NumFormats=%u NumConnectors=%u MaxBufferSizeOrder=%u\n",
+			     desc->bVersion, desc->bCompression, desc->bNumFormats,
+			     desc->bNumConnectors, desc->bMaxBufferSizeOrder);
+
+	if (!desc->bVersion || !desc->bNumFormats || !desc->bNumConnectors ||
+	    !desc->bMaxBufferSizeOrder || !desc->dwMaxWidth || !desc->dwMaxHeight ||
+	    le32_to_cpu(desc->dwMinWidth) > le32_to_cpu(desc->dwMaxWidth) ||
+	    le32_to_cpu(desc->dwMinHeight) > le32_to_cpu(desc->dwMaxHeight))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int gud_usb_get_status(struct usb_device *usb, u8 ifnum)
+{
+	struct gud_drm_req_get_status *status;
+	int ret, status_retries = 2000 / 5; /* maximum wait ~2 seconds */
+	unsigned long delay = 500;
+
+	status = kmalloc(sizeof(*status), GFP_KERNEL);
+	if (!status)
+		return -ENOMEM;
+
+	/*
+	 * Poll due to lack of data/status stage control on the gadget side.
+	 *
+	 * If we did not use polling and gave up here after waiting 2 seconds,
+	 * the worker in the gadget would finally get to queuing up the status
+	 * respons, but by that time the host has moved on. The gadget side
+	 * (at least dwc2) would now be left in a non-recoverable state.
+	 *
+	 * Worst case commit timeout in DRM can be tens of seconds (wait for
+	 * various _done completions).
+	 */
+	while (status_retries--) {
+		ret = gud_drm_usb_control_msg(usb, ifnum, true, USB_REQ_GET_STATUS, 0,
+					      status, sizeof(*status), true);
+		if (ret)
+			goto out;
+
+		if (!(status->flags & GUD_DRM_STATUS_PENDING)) {
+			ret = -status->errno;
+			goto out;
+		}
+
+		usleep_range(delay, delay + 1000);
+
+		if (delay < 4500)
+			delay += 1000;
+	}
+
+	ret = -ETIMEDOUT;
+out:
+	kfree(status);
+
+	return ret;
+}
+
+static int gud_usb_transfer(struct gud_drm_device *gdrm, bool in, u8 request,
+			    u16 index, void *buf, size_t len)
+{
+	int idx, ret;
+
+	drm_dbg(&gdrm->drm, "%s: request=0x%x index=%u len=%zu\n",
+		in ? "get" : "set", request, index, len);
+
+	if (len > GUD_DRM_MAX_TRANSFER_SIZE)
+		return -ENOMEM;
+
+	if (!drm_dev_enter(&gdrm->drm, &idx))
+		return -ENODEV;
+
+	mutex_lock(&gdrm->ctrl_lock);
+
+	if (!in && buf)
+		memcpy(gdrm->ctrl_msg_buf, buf, len);
+
+	ret = gud_drm_usb_control_msg(gdrm->usb, gdrm->ifnum, in, request, index,
+				      gdrm->ctrl_msg_buf, len, true);
+
+	/*
+	 * OUT transfers are processed in a worker on the gadget side after
+	 * reception so we always need to check status. IN transfers are
+	 * processed in the interrupt handler and will halt on error letting us
+	 * know something went wrong.
+	 */
+	if (ret || !in) {
+		ret = gud_usb_get_status(gdrm->usb, gdrm->ifnum);
+		if (ret)
+			goto error;
+	}
+
+	if (in && buf)
+		memcpy(buf, gdrm->ctrl_msg_buf, len);
+error:
+	if (ret) {
+		drm_dbg(&gdrm->drm, "ret=%d\n", ret);
+		gdrm->stats_num_errors++;
+	}
+
+	mutex_unlock(&gdrm->ctrl_lock);
+	drm_dev_exit(idx);
+
+	return ret;
+}
+
+int gud_drm_usb_get(struct gud_drm_device *gdrm, u8 request, u16 index, void *buf, size_t len)
+{
+	return gud_usb_transfer(gdrm, true, request, index, buf, len);
+}
+
+int gud_drm_usb_set(struct gud_drm_device *gdrm, u8 request, u16 index, void *buf, size_t len)
+{
+	return gud_usb_transfer(gdrm, false, request, index, buf, len);
+}
+
+int gud_drm_usb_write8(struct gud_drm_device *gdrm, u8 request, u8 val)
+{
+	return gud_drm_usb_set(gdrm, request, 0, &val, sizeof(val));
+}
+
+static int gud_drm_usb_read32(struct gud_drm_device *gdrm, u8 request,
+			      u32 *vals, unsigned int num_vals)
+{
+	unsigned int i;
+	int ret;
+
+	ret = gud_drm_usb_get(gdrm, request, 0, vals, num_vals * sizeof(*vals));
+	if (ret)
+		return ret;
+
+	for (i = 0; i < num_vals; i++)
+		vals[i] = le32_to_cpu((__le32)vals[i]);
+
+	return 0;
+}
+
+static int gud_drm_get_properties(struct gud_drm_device *gdrm, unsigned int num_properties)
+{
+	struct gud_drm_property *properties;
+	unsigned int i;
+	int ret;
+
+	if (!num_properties)
+		return 0;
+
+	gdrm->properties = kcalloc(num_properties, sizeof(*gdrm->properties), GFP_KERNEL);
+	if (!gdrm->properties)
+		return -ENOMEM;
+
+	properties = kcalloc(num_properties, sizeof(*properties), GFP_KERNEL);
+	if (!properties)
+		return -ENOMEM;
+
+	ret = gud_drm_usb_get(gdrm, GUD_DRM_USB_REQ_GET_PROPERTIES, 0,
+			      properties, num_properties * sizeof(*properties));
+	if (ret)
+		goto out;
+
+	for (i = 0; i < num_properties; i++) {
+		u16 prop = le16_to_cpu(properties[i].prop);
+		u64 val = le64_to_cpu(properties[i].val);
+
+		switch (prop) {
+		case GUD_DRM_PROPERTY_ROTATION:
+			ret = drm_plane_create_rotation_property(&gdrm->pipe.plane,
+								 DRM_MODE_ROTATE_0, val);
+			break;
+		default:
+			/* New ones might show up in future devices, skip those we don't know. */
+			drm_dbg(&gdrm->drm, "Unknown property: %u\n", prop);
+			continue;
+		}
+
+		if (ret)
+			goto out;
+
+		gdrm->properties[gdrm->num_properties++] = prop;
+	}
+out:
+	kfree(properties);
+
+	return ret;
+}
+
+static struct drm_gem_object *
+gud_drm_driver_gem_create_object(struct drm_device *dev, size_t size)
+{
+	struct drm_gem_shmem_object *shmem;
+
+	shmem = kzalloc(sizeof(*shmem), GFP_KERNEL);
+	if (!shmem)
+		return NULL;
+
+	/*
+	 * This doesn't make a difference on x86, but on ARM (pi4) it was
+	 * necessary to avoid black lines all over and it made it possible to
+	 * compress directly from the framebuffer without performance drop.
+	 */
+	shmem->map_cached = true;
+
+	return &shmem->base;
+}
+
+static int gud_drm_stats_debugfs(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = m->private;
+	struct gud_drm_device *gdrm = to_gud_drm_device(node->minor->dev);
+	char buf[10];
+
+	string_get_size(gdrm->bulk_len, 1, STRING_UNITS_2, buf, sizeof(buf));
+	seq_printf(m, "Max buffer size: %s\n", buf);
+	seq_printf(m, "Number of errors:  %u\n", gdrm->stats_num_errors);
+
+	seq_puts(m, "Compression:      ");
+	if (gdrm->compression & GUD_DRM_COMPRESSION_LZ4)
+		seq_puts(m, " lz4");
+	seq_puts(m, "\n");
+
+	if (gdrm->compression) {
+		u64 remainder;
+		u64 ratio = div64_u64_rem(gdrm->stats_length, gdrm->stats_actual_length,
+					  &remainder);
+		u64 ratio_frac = div64_u64(remainder * 10, gdrm->stats_actual_length);
+
+		seq_printf(m, "Compression ratio: %llu.%llu\n", ratio, ratio_frac);
+	}
+
+	return 0;
+}
+
+static const struct drm_info_list gud_drm_debugfs_list[] = {
+	{ "stats", gud_drm_stats_debugfs, 0, NULL },
+};
+
+static void gud_drm_driver_debugfs_init(struct drm_minor *minor)
+{
+	drm_debugfs_create_files(gud_drm_debugfs_list, ARRAY_SIZE(gud_drm_debugfs_list),
+				 minor->debugfs_root, minor);
+}
+
+static const struct drm_simple_display_pipe_funcs gud_drm_pipe_funcs = {
+	.check      = gud_drm_pipe_check,
+	.update	    = gud_drm_pipe_update,
+	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
+};
+
+static const struct drm_mode_config_funcs gud_drm_mode_config_funcs = {
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+static const uint64_t gud_drm_pipe_modifiers[] = {
+	DRM_FORMAT_MOD_LINEAR,
+	DRM_FORMAT_MOD_INVALID
+};
+
+DEFINE_DRM_GEM_FOPS(gud_drm_fops);
+
+static struct drm_driver gud_drm_driver = {
+	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
+	.fops			= &gud_drm_fops,
+	.gem_create_object	= gud_drm_driver_gem_create_object,
+	DRM_GEM_SHMEM_DRIVER_OPS,
+	.debugfs_init		= gud_drm_driver_debugfs_init,
+
+	.name			= "gud_drm",
+	.desc			= "Generic USB Display",
+	.date			= "20200422",
+	.major			= 1,
+	.minor			= 0,
+};
+
+static void gud_drm_free_buffers_and_mutex(void *data)
+{
+	struct gud_drm_device *gdrm = data;
+
+	/* Access to these are protected by drm_dev_enter/exit */
+
+	kfree(gdrm->properties);
+	vfree(gdrm->compress_buf);
+	kfree(gdrm->bulk_buf);
+	kfree(gdrm->ctrl_msg_buf);
+	gdrm->properties = NULL;
+	gdrm->compress_buf = NULL;
+	gdrm->bulk_buf = NULL;
+	gdrm->ctrl_msg_buf = NULL;
+
+	mutex_destroy(&gdrm->ctrl_lock);
+	mutex_destroy(&gdrm->damage_lock);
+}
+
+static int gud_drm_probe(struct usb_interface *interface,
+			 const struct usb_device_id *id)
+{
+	u8 ifnum = interface->cur_altsetting->desc.bInterfaceNumber;
+	struct usb_device *usb = interface_to_usbdev(interface);
+	struct device *dev = &interface->dev;
+	const struct drm_format_info *xrgb8888_emulation_format = NULL;
+	u32 *formats, *formats_dev, num_connectors, num_formats = 0;
+	bool rgb565_supported = false, rgb8888_supported = false;
+	struct usb_endpoint_descriptor *bulk_out;
+	struct gud_drm_display_descriptor desc;
+	struct gud_drm_device *gdrm;
+	struct drm_device *drm;
+	size_t max_buffer_size;
+	int ret, i;
+
+	ret = usb_find_bulk_out_endpoint(interface->cur_altsetting, &bulk_out);
+	if (ret)
+		return ret;
+
+	ret = gud_get_vendor_descriptor(interface, &desc);
+	if (ret) {
+		DRM_DEV_DEBUG_DRIVER(dev, "Not a display interface: ret=%d\n", ret);
+		return -ENODEV;
+	}
+
+	if (desc.bVersion > 1) {
+		u8 *version = kmalloc(sizeof(*version), GFP_KERNEL);
+
+		if (!version)
+			return -ENOMEM;
+
+		/* Check if the device can support us */
+		*version = 1;
+		ret = gud_drm_usb_control_msg(usb, ifnum, false, GUD_DRM_USB_REQ_SET_VERSION,
+					      0, version, sizeof(*version), true);
+		if (!ret)
+			ret = gud_usb_get_status(usb, ifnum);
+		kfree(version);
+		if (ret) {
+			dev_err(dev, "Protocol version %u is not supported\n", desc.bVersion);
+			return -EPROTONOSUPPORT;
+		}
+
+		desc.bVersion = 1;
+	}
+
+	num_connectors = desc.bNumConnectors;
+	max_buffer_size = 1 << desc.bMaxBufferSizeOrder;
+
+	gdrm = devm_drm_dev_alloc(dev, &gud_drm_driver, struct gud_drm_device, drm);
+	if (IS_ERR(gdrm))
+		return PTR_ERR(gdrm);
+
+	drm = &gdrm->drm;
+	drm->mode_config.funcs = &gud_drm_mode_config_funcs;
+	ret = drmm_mode_config_init(drm);
+	if (ret)
+		return ret;
+
+	gdrm->usb = usb;
+	gdrm->ifnum = ifnum;
+	gdrm->compression = desc.bCompression & GUD_DRM_COMPRESSION_LZ4;
+
+	mutex_init(&gdrm->ctrl_lock);
+	mutex_init(&gdrm->damage_lock);
+	INIT_WORK(&gdrm->work, gud_drm_work);
+	gud_drm_clear_damage(gdrm);
+
+	/*
+	 * devm_kmalloc() places struct devres at the beginning of the buffer it
+	 * allocates. This can waste a lot of memory when allocating
+	 * power-of-two sized buffers. Asking for 4k would actually allocate 8k.
+	 */
+
+	ret = devm_add_action_or_reset(dev, gud_drm_free_buffers_and_mutex, gdrm);
+	if (ret)
+		return ret;
+
+	gdrm->ctrl_msg_buf = kmalloc(GUD_DRM_MAX_TRANSFER_SIZE, GFP_KERNEL);
+	if (!gdrm->ctrl_msg_buf)
+		return -ENOMEM;
+retry:
+	gdrm->bulk_buf = kmalloc(max_buffer_size, GFP_KERNEL);
+	if (!gdrm->bulk_buf) {
+		max_buffer_size /= 2;
+		if (max_buffer_size < SZ_2M) { /* Give up if we can't do 1024x768 RGB565 */
+			return -ENOMEM;
+		}
+		goto retry;
+	}
+
+	gdrm->bulk_pipe = usb_sndbulkpipe(gdrm->usb, usb_endpoint_num(bulk_out));
+	gdrm->bulk_len = max_buffer_size;
+
+	if (gdrm->compression & GUD_DRM_COMPRESSION_LZ4) {
+		gdrm->lz4_comp_mem = devm_kmalloc(dev, LZ4_MEM_COMPRESS, GFP_KERNEL);
+		if (!gdrm->lz4_comp_mem)
+			return -ENOMEM;
+
+		gdrm->compress_buf = vmalloc(gdrm->bulk_len);
+		if (!gdrm->compress_buf)
+			return -ENOMEM;
+	}
+
+	drm->mode_config.min_width = le32_to_cpu(desc.dwMinWidth);
+	drm->mode_config.max_width = le32_to_cpu(desc.dwMaxWidth);
+	drm->mode_config.min_height = le32_to_cpu(desc.dwMinHeight);
+	drm->mode_config.max_height = le32_to_cpu(desc.dwMaxHeight);
+
+	formats_dev = devm_kmalloc_array(dev, desc.bNumFormats, sizeof(u32), GFP_KERNEL);
+	/* Add room for emulated XRGB8888 */
+	formats = devm_kmalloc_array(dev, desc.bNumFormats + 1, sizeof(u32), GFP_KERNEL);
+	if (!formats_dev || !formats)
+		return -ENOMEM;
+
+	ret = gud_drm_usb_read32(gdrm, GUD_DRM_USB_REQ_GET_FORMATS, formats_dev, desc.bNumFormats);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < desc.bNumFormats; i++) {
+		const struct drm_format_info *fmt_info;
+		u32 format = formats_dev[i];
+
+		if (format == GUD_DRM_FORMAT_R1) {
+			fmt_info = &gud_drm_format_r1;
+		} else {
+			/* This will trigger a WARN for unknown formats... */
+			fmt_info = drm_format_info(format);
+			if (!fmt_info) {
+				drm_dbg(drm, "Unknown format: 0x%x\n", format);
+				continue;
+			}
+		}
+
+		switch (format) {
+		case DRM_FORMAT_XRGB8888:
+			fallthrough;
+		case DRM_FORMAT_ARGB8888:
+			rgb8888_supported = true;
+			break;
+		case DRM_FORMAT_RGB888:
+			fallthrough;
+		case DRM_FORMAT_BGR888:
+			drm_dbg(drm, "24-bit formats are not supported.\n");
+			continue;
+		case DRM_FORMAT_RGB565:
+			rgb565_supported = true;
+			if (!xrgb8888_emulation_format)
+				xrgb8888_emulation_format = fmt_info;
+			break;
+		case GUD_DRM_FORMAT_R1:
+			if (!xrgb8888_emulation_format)
+				xrgb8888_emulation_format = fmt_info;
+			/* Internal, not for userspace */
+			continue;
+		}
+
+		formats[num_formats++] = format;
+	}
+
+	if (!num_formats && !xrgb8888_emulation_format) {
+		dev_err(dev, "No supported formats found\n");
+		return -ENOENT;
+	}
+
+	/* Prefer speed over color depth */
+	if (rgb565_supported)
+		drm->mode_config.preferred_depth = 16;
+
+	if (!rgb8888_supported && xrgb8888_emulation_format) {
+		gdrm->xrgb8888_emulation_format = xrgb8888_emulation_format;
+		formats[num_formats++] = DRM_FORMAT_XRGB8888;
+	}
+
+	ret = drm_simple_display_pipe_init(drm, &gdrm->pipe, &gud_drm_pipe_funcs,
+					   formats, num_formats,
+					   gud_drm_pipe_modifiers, NULL);
+	if (ret)
+		return ret;
+
+	devm_kfree(dev, formats);
+	devm_kfree(dev, formats_dev);
+
+	ret = gud_drm_get_properties(gdrm, desc.bNumProperties);
+	if (ret)
+		return ret;
+
+	drm_plane_enable_fb_damage_clips(&gdrm->pipe.plane);
+
+	for (i = 0; i < num_connectors; i++) {
+		ret = gud_drm_connector_create(gdrm, i);
+		if (ret)
+			return ret;
+	}
+
+	drm_mode_config_reset(drm);
+
+	usb_set_intfdata(interface, gdrm);
+
+	ret = drm_dev_register(drm, 0);
+	if (ret)
+		return ret;
+
+	drm_kms_helper_poll_init(drm);
+
+	drm_fbdev_generic_setup(drm, 0);
+
+	return 0;
+}
+
+static void gud_drm_disconnect(struct usb_interface *interface)
+{
+	struct gud_drm_device *gdrm = usb_get_intfdata(interface);
+	struct drm_device *drm = &gdrm->drm;
+
+	drm_dbg(drm, "%s:\n", __func__);
+
+	drm_kms_helper_poll_fini(drm);
+	drm_dev_unplug(drm);
+	drm_atomic_helper_shutdown(drm);
+}
+
+static int gud_drm_suspend(struct usb_interface *interface, pm_message_t message)
+{
+	struct gud_drm_device *gdrm = usb_get_intfdata(interface);
+
+	return drm_mode_config_helper_suspend(&gdrm->drm);
+}
+
+static int gud_drm_resume(struct usb_interface *interface)
+{
+	struct gud_drm_device *gdrm = usb_get_intfdata(interface);
+
+	drm_mode_config_helper_resume(&gdrm->drm);
+
+	return 0;
+}
+
+static const struct usb_device_id gud_drm_table[] = {
+	{ USB_DEVICE_INTERFACE_CLASS(0x1d50, 0x614d, USB_CLASS_VENDOR_SPEC) },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(usb, gud_drm_table);
+
+static struct usb_driver gud_drm_usb_driver = {
+	.name		= "gud_drm",
+	.probe		= gud_drm_probe,
+	.disconnect	= gud_drm_disconnect,
+	.id_table	= gud_drm_table,
+	.suspend	= gud_drm_suspend,
+	.resume		= gud_drm_resume,
+	.reset_resume	= gud_drm_resume,
+};
+
+module_usb_driver(gud_drm_usb_driver);
+
+MODULE_AUTHOR("Noralf Trønnes");
+MODULE_LICENSE("Dual MIT/GPL");
diff --git a/drivers/gpu/drm/gud/gud_drm_internal.h b/drivers/gpu/drm/gud/gud_drm_internal.h
new file mode 100644
index 000000000000..2cca502d444e
--- /dev/null
+++ b/drivers/gpu/drm/gud/gud_drm_internal.h
@@ -0,0 +1,65 @@ 
+/* SPDX-License-Identifier: MIT */
+
+#ifndef __LINUX_GUD_DRM_INTERNAL_H
+#define __LINUX_GUD_DRM_INTERNAL_H
+
+#include <linux/workqueue.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+
+#include <drm/drm_simple_kms_helper.h>
+
+struct gud_drm_device {
+	struct drm_device drm;
+	struct drm_simple_display_pipe pipe;
+	struct work_struct work;
+	struct usb_device *usb;
+	u8 ifnum;
+	const struct drm_format_info *xrgb8888_emulation_format;
+
+	u16 *properties;
+	unsigned int num_properties;
+
+	unsigned int bulk_pipe;
+	void *bulk_buf;
+	size_t bulk_len;
+
+	u8 compression;
+	void *lz4_comp_mem;
+	void *compress_buf;
+
+	u64 stats_length;
+	u64 stats_actual_length;
+	unsigned int stats_num_errors;
+
+	struct mutex ctrl_lock; /* Serialize req and status transfers */
+	void *ctrl_msg_buf;
+
+	struct mutex damage_lock; /* Protects the following members: */
+	struct drm_framebuffer *fb;
+	struct drm_rect damage;
+};
+
+static inline struct gud_drm_device *to_gud_drm_device(struct drm_device *drm)
+{
+	return container_of(drm, struct gud_drm_device, drm);
+}
+
+int gud_drm_usb_get(struct gud_drm_device *gdrm, u8 request, u16 index, void *buf, size_t len);
+int gud_drm_usb_set(struct gud_drm_device *gdrm, u8 request, u16 index, void *buf, size_t len);
+int gud_drm_usb_write8(struct gud_drm_device *gdrm, u8 request, u8 val);
+
+void gud_drm_clear_damage(struct gud_drm_device *gdrm);
+void gud_drm_work(struct work_struct *work);
+int gud_drm_pipe_check(struct drm_simple_display_pipe *pipe,
+		       struct drm_plane_state *new_plane_state,
+		       struct drm_crtc_state *new_crtc_state);
+void gud_drm_pipe_update(struct drm_simple_display_pipe *pipe,
+			 struct drm_plane_state *old_state);
+
+int gud_drm_connector_fill_properties(struct drm_connector *connector,
+				      struct drm_connector_state *connector_state,
+				      struct gud_drm_property *properties);
+int gud_drm_connector_create(struct gud_drm_device *gdrm, unsigned int index);
+
+#endif
diff --git a/drivers/gpu/drm/gud/gud_drm_pipe.c b/drivers/gpu/drm/gud/gud_drm_pipe.c
new file mode 100644
index 000000000000..d8a3e0ba38fd
--- /dev/null
+++ b/drivers/gpu/drm/gud/gud_drm_pipe.c
@@ -0,0 +1,426 @@ 
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright 2020 Noralf Trønnes
+ */
+
+#include <linux/dma-buf.h>
+#include <linux/lz4.h>
+#include <linux/usb.h>
+#include <linux/workqueue.h>
+
+#include <drm/drm_atomic.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_format_helper.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_print.h>
+#include <drm/drm_rect.h>
+#include <drm/drm_simple_kms_helper.h>
+#include <drm/gud_drm.h>
+
+#include "gud_drm_internal.h"
+
+static bool gud_drm_is_big_endian(void)
+{
+#if defined(__BIG_ENDIAN)
+	return true;
+#else
+	return false;
+#endif
+}
+
+static size_t gud_drm_xrgb8888_to_r124(u8 *dst, const struct drm_format_info *format,
+				       void *src, struct drm_framebuffer *fb,
+				       struct drm_rect *rect)
+{
+	unsigned int block_width = drm_format_info_block_width(format, 0);
+	unsigned int bits_per_pixel = 8 / block_width;
+	unsigned int x, y, width, height;
+	u8 *p, *block = dst; /* Assign to silence compiler warning */
+	size_t len;
+	void *buf;
+
+	WARN_ON_ONCE(format->char_per_block[0] != 1);
+
+	/* Start on a byte boundary */
+	rect->x1 = ALIGN_DOWN(rect->x1, block_width);
+	width = drm_rect_width(rect);
+	height = drm_rect_height(rect);
+	len = drm_format_info_min_pitch(format, 0, width) * height;
+
+	buf = kmalloc(width * height, GFP_KERNEL);
+	if (!buf)
+		return len; /* To keep logic simple, just transmit garbage */
+
+	drm_fb_xrgb8888_to_gray8(buf, src, fb, rect);
+
+	p = buf;
+	for (y = 0; y < drm_rect_height(rect); y++) {
+		for (x = 0; x < drm_rect_width(rect); x++) {
+			if (!(x % block_width)) {
+				block = dst++;
+				*block = 0;
+			}
+
+			*block <<= bits_per_pixel;
+			*block |= (*p++) >> (8 - bits_per_pixel);
+		}
+	}
+
+	kfree(buf);
+
+	return len;
+}
+
+static int gud_drm_fb_flush(struct gud_drm_device *gdrm, struct drm_framebuffer *fb,
+			    const struct drm_format_info *format, struct drm_rect *rect)
+{
+	struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach;
+	struct gud_drm_req_set_buffer req;
+	size_t pitch, len, trlen;
+	int actual_length;
+	void *vaddr, *buf;
+	int ret = 0;
+
+	drm_dbg(&gdrm->drm, "Flushing [FB:%d] " DRM_RECT_FMT " imported=%s\n",
+		fb->base.id, DRM_RECT_ARG(rect), import_attach ? "yes" : "no");
+
+	pitch = drm_format_info_min_pitch(format, 0, drm_rect_width(rect));
+	len = pitch * drm_rect_height(rect);
+	if (len > gdrm->bulk_len)
+		return -E2BIG;
+
+	vaddr = drm_gem_shmem_vmap(fb->obj[0]);
+	if (!vaddr)
+		return -ENOMEM;
+
+	if (import_attach) {
+		ret = dma_buf_begin_cpu_access(import_attach->dmabuf, DMA_FROM_DEVICE);
+		if (ret)
+			goto vunmap;
+	}
+
+	if (gdrm->compression & GUD_DRM_COMPRESSION_LZ4)
+		buf = gdrm->compress_buf;
+	else
+		buf = gdrm->bulk_buf;
+
+	/*
+	 * Imported buffers are assumed to be write-combined and thus uncached
+	 * with slow reads (at least on ARM).
+	 */
+	if (format != fb->format) {
+		if (format->format == GUD_DRM_FORMAT_R1)
+			len = gud_drm_xrgb8888_to_r124(buf, format, vaddr, fb, rect);
+		else if (format->format == DRM_FORMAT_RGB565)
+			drm_fb_xrgb8888_to_rgb565(buf, vaddr, fb, rect, gud_drm_is_big_endian());
+	} else if (gud_drm_is_big_endian() && format->cpp[0] > 1) {
+		drm_fb_swab(buf, vaddr, fb, rect, !import_attach);
+	} else if (gdrm->compression && !import_attach && pitch == fb->pitches[0]) {
+		/* can compress directly from the framebuffer */
+		buf = vaddr + rect->y1 * pitch;
+	} else {
+		drm_fb_memcpy(buf, vaddr, fb, rect);
+	}
+
+	req.x = cpu_to_le32(rect->x1);
+	req.y = cpu_to_le32(rect->y1);
+	req.width = cpu_to_le32(drm_rect_width(rect));
+	req.height = cpu_to_le32(drm_rect_height(rect));
+	req.length = cpu_to_le32(len);
+	req.compression = 0;
+	req.compressed_length = 0;
+
+	if (gdrm->compression & GUD_DRM_COMPRESSION_LZ4) {
+		ret = LZ4_compress_default(buf, gdrm->bulk_buf, len, len, gdrm->lz4_comp_mem);
+		if (ret > 0)
+			req.compression = GUD_DRM_COMPRESSION_LZ4;
+	}
+
+	trlen = len;
+
+	if (ret > 0) {
+		req.compressed_length = cpu_to_le32(ret);
+		trlen = ret;
+	} else if (buf == gdrm->compress_buf) {
+		/*
+		 * Compression failed (buffer didn't compress well).
+		 * compress_buf is vmalloc'ed so we need to copy.
+		 */
+		memcpy(gdrm->bulk_buf, gdrm->compress_buf, len);
+	}
+
+	if (import_attach)
+		dma_buf_end_cpu_access(import_attach->dmabuf, DMA_FROM_DEVICE);
+
+	gdrm->stats_length += len;
+	/* Did it wrap around? */
+	if (gdrm->stats_length <= len && gdrm->stats_actual_length) {
+		gdrm->stats_length = len;
+		gdrm->stats_actual_length = 0;
+	}
+	gdrm->stats_actual_length += trlen;
+
+	/*
+	 * This will wait if decompress/copy from the previous flush is still in
+	 * process on the gadget side.
+	 */
+	ret = gud_drm_usb_set(gdrm, GUD_DRM_USB_REQ_SET_BUFFER, 0, &req, sizeof(req));
+	if (ret)
+		goto vunmap;
+
+	ret = usb_bulk_msg(gdrm->usb, gdrm->bulk_pipe, gdrm->bulk_buf, trlen,
+			   &actual_length, msecs_to_jiffies(3000));
+	if (!ret && trlen != actual_length)
+		ret = -EIO;
+	if (ret)
+		gdrm->stats_num_errors++;
+vunmap:
+	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
+
+	return ret;
+}
+
+void gud_drm_clear_damage(struct gud_drm_device *gdrm)
+{
+	gdrm->damage.x1 = INT_MAX;
+	gdrm->damage.y1 = INT_MAX;
+	gdrm->damage.x2 = 0;
+	gdrm->damage.y2 = 0;
+}
+
+void gud_drm_work(struct work_struct *work)
+{
+	struct gud_drm_device *gdrm = container_of(work, struct gud_drm_device, work);
+	const struct drm_format_info *format;
+	struct drm_framebuffer *fb;
+	struct drm_rect damage;
+	unsigned int i, lines;
+	int idx, ret = 0;
+	size_t pitch;
+
+	if (!drm_dev_enter(&gdrm->drm, &idx))
+		return;
+
+	mutex_lock(&gdrm->damage_lock);
+	fb = gdrm->fb;
+	gdrm->fb = NULL;
+	damage = gdrm->damage;
+	gud_drm_clear_damage(gdrm);
+	mutex_unlock(&gdrm->damage_lock);
+
+	if (!fb)
+		goto out;
+
+	format = fb->format;
+	if (format->format == DRM_FORMAT_XRGB8888 && gdrm->xrgb8888_emulation_format)
+		format = gdrm->xrgb8888_emulation_format;
+
+	/* Split update if it's too big */
+	pitch = drm_format_info_min_pitch(format, 0, drm_rect_width(&damage));
+	lines = drm_rect_height(&damage);
+
+	if (gdrm->bulk_len < lines * pitch)
+		lines = gdrm->bulk_len / pitch;
+
+	for (i = 0; i < DIV_ROUND_UP(drm_rect_height(&damage), lines); i++) {
+		struct drm_rect rect = damage;
+
+		rect.y1 += i * lines;
+		rect.y2 = min_t(u32, rect.y1 + lines, damage.y2);
+
+		ret = gud_drm_fb_flush(gdrm, fb, format, &rect);
+		if (ret &&
+		    (ret != -ENODEV && ret != -ECONNRESET && ret != -ESHUTDOWN && ret != -EPROTO))
+			dev_err_once(fb->dev->dev, "Failed to flush framebuffer: error=%d\n", ret);
+	}
+
+	drm_framebuffer_put(fb);
+out:
+	drm_dev_exit(idx);
+}
+
+static void gud_drm_fb_queue_damage(struct gud_drm_device *gdrm,
+				    struct drm_framebuffer *fb,
+				    struct drm_rect *damage)
+{
+	struct drm_framebuffer *old_fb = NULL;
+
+	mutex_lock(&gdrm->damage_lock);
+
+	if (fb != gdrm->fb) {
+		old_fb = gdrm->fb;
+		drm_framebuffer_get(fb);
+		gdrm->fb = fb;
+	}
+
+	gdrm->damage.x1 = min(gdrm->damage.x1, damage->x1);
+	gdrm->damage.y1 = min(gdrm->damage.y1, damage->y1);
+	gdrm->damage.x2 = max(gdrm->damage.x2, damage->x2);
+	gdrm->damage.y2 = max(gdrm->damage.y2, damage->y2);
+
+	mutex_unlock(&gdrm->damage_lock);
+
+	queue_work(system_long_wq, &gdrm->work);
+
+	if (old_fb)
+		drm_framebuffer_put(old_fb);
+}
+
+int gud_drm_pipe_check(struct drm_simple_display_pipe *pipe,
+		       struct drm_plane_state *new_plane_state,
+		       struct drm_crtc_state *new_crtc_state)
+{
+	struct gud_drm_device *gdrm = to_gud_drm_device(pipe->crtc.dev);
+	struct drm_plane_state *old_plane_state = pipe->plane.state;
+	const struct drm_display_mode *mode = &new_crtc_state->mode;
+	struct drm_atomic_state *state = new_plane_state->state;
+	struct drm_framebuffer *old_fb = old_plane_state->fb;
+	struct drm_connector_state *connector_state = NULL;
+	struct drm_framebuffer *fb = new_plane_state->fb;
+	const struct drm_format_info *format = fb->format;
+	struct gud_drm_req_set_state *req;
+	struct drm_connector *connector;
+	int idx, ret, num_properties;
+	unsigned int i;
+	size_t len;
+
+	if (WARN_ON_ONCE(!fb))
+		return -EINVAL;
+
+	if (old_plane_state->rotation != new_plane_state->rotation)
+		new_crtc_state->mode_changed = true;
+
+	if (old_fb && old_fb->format != format)
+		new_crtc_state->mode_changed = true;
+
+	if (!new_crtc_state->mode_changed && !new_crtc_state->connectors_changed)
+		return 0;
+
+	/* Only one connector is supported */
+	if (hweight32(new_crtc_state->connector_mask) != 1)
+		return -EINVAL;
+
+	if (format->format == DRM_FORMAT_XRGB8888 && gdrm->xrgb8888_emulation_format)
+		format = gdrm->xrgb8888_emulation_format;
+
+	for_each_new_connector_in_state(state, connector, connector_state, i)
+		break;
+
+	if (!connector_state) {
+		struct drm_connector_list_iter conn_iter;
+
+		/* We always send the full state to the device, so get the connector state */
+
+		drm_connector_list_iter_begin(pipe->crtc.dev, &conn_iter);
+		drm_for_each_connector_iter(connector, &conn_iter) {
+			if (new_crtc_state->connector_mask & drm_connector_mask(connector))
+				break;
+		}
+		drm_connector_list_iter_end(&conn_iter);
+
+		if (WARN_ON_ONCE(!connector))
+			return -ENOENT;
+
+		connector_state = drm_atomic_get_connector_state(state, connector);
+		if (IS_ERR(connector_state))
+			return PTR_ERR(connector_state);
+	}
+
+	num_properties = gud_drm_connector_fill_properties(connector, NULL, NULL);
+	if (num_properties < 0)
+		return num_properties;
+
+	num_properties += gdrm->num_properties;
+
+	len = struct_size(req, properties, num_properties);
+	req = kzalloc(len, GFP_KERNEL);
+	if (!req)
+		return -ENOMEM;
+
+	gud_drm_from_display_mode(&req->mode, mode);
+
+	req->format = cpu_to_le32(format->format);
+	req->connector = drm_connector_index(connector);
+	req->num_properties = num_properties;
+
+	num_properties = gud_drm_connector_fill_properties(connector, connector_state,
+							   req->properties);
+
+	for (i = 0; i < gdrm->num_properties; i++) {
+		u16 prop = gdrm->properties[i];
+		u64 val;
+
+		switch (prop) {
+		case GUD_DRM_PROPERTY_ROTATION:
+			val = new_plane_state->rotation;
+			break;
+		default:
+			WARN_ON_ONCE(1);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		req->properties[num_properties + i].prop = cpu_to_le16(prop);
+		req->properties[num_properties + i].val = cpu_to_le64(val);
+	}
+
+	if (!drm_dev_enter(fb->dev, &idx)) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	ret = gud_drm_usb_set(gdrm, GUD_DRM_USB_REQ_SET_STATE_CHECK, 0, req, len);
+
+	drm_dev_exit(idx);
+out:
+	kfree(req);
+
+	return ret;
+}
+
+void gud_drm_pipe_update(struct drm_simple_display_pipe *pipe,
+			 struct drm_plane_state *old_state)
+{
+	struct drm_device *drm = pipe->crtc.dev;
+	struct gud_drm_device *gdrm = to_gud_drm_device(drm);
+	struct drm_plane_state *state = pipe->plane.state;
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct drm_rect damage;
+	int idx;
+
+	if (!drm_dev_enter(drm, &idx))
+		return;
+
+	if (!old_state->fb)
+		gud_drm_usb_write8(gdrm, GUD_DRM_USB_REQ_SET_CONTROLLER_ENABLE, 1);
+
+	if (fb && (crtc->state->mode_changed || crtc->state->connectors_changed))
+		gud_drm_usb_set(gdrm, GUD_DRM_USB_REQ_SET_STATE_COMMIT, 0, NULL, 0);
+
+	if (crtc->state->active_changed)
+		gud_drm_usb_write8(gdrm, GUD_DRM_USB_REQ_SET_DISPLAY_ENABLE, crtc->state->active);
+
+	if (drm_atomic_helper_damage_merged(old_state, state, &damage))
+		gud_drm_fb_queue_damage(gdrm, fb, &damage);
+
+	if (!fb) {
+		cancel_work_sync(&gdrm->work);
+
+		mutex_lock(&gdrm->damage_lock);
+		if (gdrm->fb) {
+			drm_framebuffer_put(gdrm->fb);
+			gdrm->fb = NULL;
+		}
+		gud_drm_clear_damage(gdrm);
+		mutex_unlock(&gdrm->damage_lock);
+
+		gud_drm_usb_write8(gdrm, GUD_DRM_USB_REQ_SET_CONTROLLER_ENABLE, 0);
+	}
+
+	drm_dev_exit(idx);
+}
diff --git a/include/drm/gud_drm.h b/include/drm/gud_drm.h
new file mode 100644
index 000000000000..7876d2e3cbcf
--- /dev/null
+++ b/include/drm/gud_drm.h
@@ -0,0 +1,361 @@ 
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright 2020 Noralf Trønnes
+ */
+
+#ifndef __LINUX_GUD_DRM_H
+#define __LINUX_GUD_DRM_H
+
+#include <drm/drm_modes.h>
+#include <linux/types.h>
+#include <uapi/drm/drm_fourcc.h>
+#include <uapi/linux/usb/ch9.h>
+
+/*
+ * Maximum size of a control message, fits 120 display modes.
+ * If this needs to increase, the IN side in f_gud_drm_setup()
+ * needs fixing.
+ */
+#define GUD_DRM_MAX_TRANSFER_SIZE	SZ_4K
+
+#define GUD_DRM_USB_DT_DISPLAY		(USB_TYPE_VENDOR | 0x4)
+
+/*
+ * struct gud_drm_display_descriptor - Display descriptor
+ * @bLength: Size of descriptor in bytes
+ * @bDescriptorType: DescriptorType (GUD_DRM_USB_DT_DISPLAY)
+ * @bVersion: Protocol version
+ * @bMaxBufferSizeOrder: Maximum buffer size the device can handle as log2
+ * @bmFlags: Currently unused, should be set to zero
+ * @bCompression: Supported compression types
+ * @dwMinWidth: Minimum pixel width the controller can handle
+ * @dwMaxWidth: Maximum width
+ * @dwMinHeight: Minimum height
+ * @dwMaxHeight: Maximum height
+ * @bNumFormats: Number of supported pixel formats
+ * @bNumProperties: Number of properties that are not connector porperties
+ * @bNumConnectors: Number of connectors
+ *
+ * Devices that have only one display mode will have dwMinWidth == dwMaxWidth
+ * and dwMinHeight == dwMaxHeight.
+ *
+ */
+struct gud_drm_display_descriptor {
+	__u8 bLength;
+	__u8 bDescriptorType;
+
+	__u8 bVersion;
+	__u8 bMaxBufferSizeOrder;
+	__le32 bmFlags;
+
+	__u8 bCompression;
+#define GUD_DRM_COMPRESSION_LZ4		BIT(0)
+
+	__le32 dwMinWidth;
+	__le32 dwMaxWidth;
+	__le32 dwMinHeight;
+	__le32 dwMaxHeight;
+
+	__u8 bNumFormats;
+	__u8 bNumProperties;
+	__u8 bNumConnectors;
+} __packed;
+
+/*
+ * struct gud_drm_req_get_status - Status request
+ * @flags: Flags
+ * @errno: Linux errno value
+ *
+ * The host keeps polling for status as long as the GUD_DRM_STATUS_PENDING flag
+ * is set (or until timeout). Requested using: USB_REQ_GET_STATUS.
+ */
+struct gud_drm_req_get_status {
+	__u8 flags;
+#define GUD_DRM_STATUS_PENDING	BIT(0)
+	__u8 errno;
+} __packed;
+
+/*
+ * struct gud_drm_property - Property
+ * @prop: Property
+ * @val: Value
+ */
+struct gud_drm_property {
+	__le16 prop;
+	__le64 val;
+} __packed;
+
+/* See &drm_display_mode for the meaning of these fields */
+struct gud_drm_display_mode {
+	__le32 clock;
+	__le16 hdisplay;
+	__le16 hsync_start;
+	__le16 hsync_end;
+	__le16 htotal;
+	__le16 hskew;
+	__le16 vdisplay;
+	__le16 vsync_start;
+	__le16 vsync_end;
+	__le16 vtotal;
+	__le16 vscan;
+	__le32 flags;
+	__u8 type;
+} __packed;
+
+/*
+ * struct gud_drm_req_get_connector - Connector descriptor
+ * @connector_type: Connector type (DRM_MODE_CONNECTOR_*)
+ * @flags: Flags
+ * @num_properties: Number of supported properties
+ */
+struct gud_drm_req_get_connector {
+	__u8 connector_type;
+
+	__le32 flags;
+#define GUD_DRM_CONNECTOR_FLAGS_POLL	BIT(0)
+
+	__u8 num_properties;
+} __packed;
+
+/*
+ * struct gud_drm_req_get_connector_status - Connector status
+ * @status: Status, see &drm_connector_status
+ * @num_modes: Number of available display modes
+ * @modes_array_checksum: CRC-CCITT checksum of the display mode array in little endian format
+ * @edid_len: Length of EDID data
+ * @edid_checksum: CRC-CCITT checksum of EDID data
+ *
+ * If both @num_modes and @edid_len are zero, connector status is set to
+ * disconnected. If @num_modes is zero, edid is used to create display modes.
+ * If both are set, edid is just passed on to userspace in the EDID connector
+ * property.
+ *
+ * Display modes and EDID are only requested if number/length or crc differs.
+ */
+struct gud_drm_req_get_connector_status {
+	__u8 status;
+#define GUD_DRM_CONNECTOR_STATUS_MASK		0xf /* Only 2 bits are currently used for status */
+#define GUD_DRM_CONNECTOR_STATUS_CHANGED	BIT(7)
+	__le16 num_modes;
+	__le16 edid_len;
+} __packed;
+
+/*
+ * struct gud_drm_req_set_buffer - Set buffer transfer info
+ * @x: X position of rectangle
+ * @y: Y position
+ * @width: Pixel width of rectangle
+ * @height: Pixel height
+ * @length: Buffer length in bytes
+ * @compression: Transfer compression
+ * @compressed_length: Compressed buffer length
+ *
+ * @x, @y, @width and @height specifies the rectangle where the buffer should be
+ * placed inside the framebuffer.
+ */
+struct gud_drm_req_set_buffer {
+	__le32 x;
+	__le32 y;
+	__le32 width;
+	__le32 height;
+
+	__le32 length;
+	__u8 compression;
+	__le32 compressed_length;
+} __packed;
+
+/*
+ * struct gud_drm_req_set_state - Set display state
+ * @mode: Display mode
+ * @format: Pixel format
+ * @connector: Connector index
+ * @num_properties: Number of properties in the state
+ * @properties: Array of properties
+ *
+ * The entire state is transferred each time there's a change.
+ */
+struct gud_drm_req_set_state {
+	struct gud_drm_display_mode mode;
+	__le32 format;
+	__u8 connector;
+	__u8 num_properties;
+	struct gud_drm_property properties[];
+} __packed;
+
+/*
+ * Internal monochrome transfer format presented to userspace as XRGB8888.
+ * Pixel lines are byte aligned.
+ */
+#define GUD_DRM_FORMAT_R1	fourcc_code('R', '1', ' ', ' ')
+
+/* List of supported connector properties: */
+
+/* TV related properties, see &drm_connector and &drm_tv_connector_state */
+#define GUD_DRM_PROPERTY_TV_SELECT_SUBCONNECTOR		1
+#define GUD_DRM_PROPERTY_TV_LEFT_MARGIN			2
+#define GUD_DRM_PROPERTY_TV_RIGHT_MARGIN		3
+#define GUD_DRM_PROPERTY_TV_TOP_MARGIN			4
+#define GUD_DRM_PROPERTY_TV_BOTTOM_MARGIN		5
+/* Number of modes are placed at _SHIFT in val on retrieval */
+#define GUD_DRM_PROPERTY_TV_MODE			6
+  #define GUD_DRM_USB_CONNECTOR_TV_MODE_NUM_SHIFT   16
+#define GUD_DRM_PROPERTY_TV_BRIGHTNESS			7
+#define GUD_DRM_PROPERTY_TV_CONTRAST			8
+#define GUD_DRM_PROPERTY_TV_FLICKER_REDUCTION		9
+#define GUD_DRM_PROPERTY_TV_OVERSCAN			10
+#define GUD_DRM_PROPERTY_TV_SATURATION			11
+#define GUD_DRM_PROPERTY_TV_HUE				12
+
+/*
+ * Backlight brightness is in the range 0-100 inclusive. The value represents
+ * the human perceptual brightness and not a linear PWM value. 0 is minimum
+ * brightness which should not turn the backlight completely off. The DPMS
+ * connector property should be used to control power which will trigger a
+ * GUD_DRM_USB_REQ_SET_DISPLAY_ENABLE request.
+ *
+ * This is not a real DRM property, but rather a fake one used for the backlight
+ * device. See drm_backlight_register() for more details.
+ */
+#define GUD_DRM_PROPERTY_BACKLIGHT_BRIGHTNESS		13
+
+/* List of supported properties that are not connector propeties: */
+
+/*
+ * Plane rotation. Should return the supported bitmask on
+ * GUD_DRM_USB_REQ_GET_PROPERTIES, see drm_plane_create_rotation_property().
+ */
+#define GUD_DRM_PROPERTY_ROTATION			50
+
+/* USB Control requests: */
+
+/*
+ * If the host driver doesn't support the device protocol version it will send
+ * the versions it supports starting with the latest. If the device isn't
+ * backwards compatible or doesn't support the version the host suggests, it
+ * shall return EPROTONOSUPPORT.
+ */
+#define GUD_DRM_USB_REQ_SET_VERSION			0x30
+
+/* Get supported pixel formats as an array of fourcc codes. See include/uapi/drm/drm_fourcc.h */
+#define GUD_DRM_USB_REQ_GET_FORMATS			0x40
+
+/* Get supported properties that are not connector propeties as a &gud_drm_property array */
+#define GUD_DRM_USB_REQ_GET_PROPERTIES			0x41
+
+/* Get connector descriptor */
+#define GUD_DRM_USB_REQ_GET_CONNECTOR			0x50
+
+/* Get properties supported by the connector as a &gud_drm_property array */
+#define GUD_DRM_USB_REQ_GET_CONNECTOR_PROPERTIES	0x51
+
+/*
+ * Issued when there's a tv.mode property present.
+ * Gets an array of tv.mode enum names each entry of length DRM_PROP_NAME_LEN.
+ */
+#define GUD_DRM_USB_REQ_GET_CONNECTOR_TV_MODE_VALUES	0x52
+
+/* When userspace checks status, this is issued first, not used for poll requests. */
+#define GUD_DRM_USB_REQ_SET_CONNECTOR_FORCE_DETECT	0x53
+
+/* Get connector status as &gud_drm_req_get_connector_status. */
+#define GUD_DRM_USB_REQ_GET_CONNECTOR_STATUS		0x54
+
+/* Get &gud_drm_display_mode array of supported display modes */
+#define GUD_DRM_USB_REQ_GET_CONNECTOR_MODES		0x55
+
+#define GUD_DRM_USB_REQ_GET_CONNECTOR_EDID		0x56
+
+/* Set buffer properties before bulk transfer as &gud_drm_req_set_buffer */
+#define GUD_DRM_USB_REQ_SET_BUFFER			0x60
+
+/* Check display configuration as &gud_drm_req_set_state */
+#define GUD_DRM_USB_REQ_SET_STATE_CHECK			0x61
+
+/* Apply the prevoius _STATE_CHECK configuration */
+#define GUD_DRM_USB_REQ_SET_STATE_COMMIT		0x62
+
+ /* Enable/disable the display controller, value is u8 0/1 */
+#define GUD_DRM_USB_REQ_SET_CONTROLLER_ENABLE		0x63
+
+/* Enable/disable display/output (DPMS), value is u8 0/1 */
+#define GUD_DRM_USB_REQ_SET_DISPLAY_ENABLE		0x64
+
+static inline void gud_drm_from_display_mode(struct gud_drm_display_mode *dst,
+					     const struct drm_display_mode *src)
+{
+	u32 flags = src->flags;
+
+	switch (src->picture_aspect_ratio) {
+	case HDMI_PICTURE_ASPECT_4_3:
+		flags |= DRM_MODE_FLAG_PIC_AR_4_3;
+		break;
+	case HDMI_PICTURE_ASPECT_16_9:
+		flags |= DRM_MODE_FLAG_PIC_AR_16_9;
+		break;
+	case HDMI_PICTURE_ASPECT_64_27:
+		flags |= DRM_MODE_FLAG_PIC_AR_64_27;
+		break;
+	case HDMI_PICTURE_ASPECT_256_135:
+		flags |= DRM_MODE_FLAG_PIC_AR_256_135;
+		break;
+	default:
+		flags |= DRM_MODE_FLAG_PIC_AR_NONE;
+		break;
+	}
+
+	dst->clock = cpu_to_le32(src->clock);
+	dst->hdisplay = cpu_to_le16(src->hdisplay);
+	dst->hsync_start = cpu_to_le16(src->hsync_start);
+	dst->hsync_end = cpu_to_le16(src->hsync_end);
+	dst->htotal = cpu_to_le16(src->htotal);
+	dst->hskew = cpu_to_le16(src->hskew);
+	dst->vdisplay = cpu_to_le16(src->vdisplay);
+	dst->vsync_start = cpu_to_le16(src->vsync_start);
+	dst->vsync_end = cpu_to_le16(src->vsync_end);
+	dst->vtotal = cpu_to_le16(src->vtotal);
+	dst->vscan = cpu_to_le16(src->vscan);
+	dst->flags = cpu_to_le32(flags);
+	dst->type = src->type;
+}
+
+static inline void gud_drm_to_display_mode(struct drm_display_mode *dst,
+					   const struct gud_drm_display_mode *src)
+{
+	u32 flags = le32_to_cpu(src->flags);
+
+	dst->clock = le32_to_cpu(src->clock);
+	dst->hdisplay = le16_to_cpu(src->hdisplay);
+	dst->hsync_start = le16_to_cpu(src->hsync_start);
+	dst->hsync_end = le16_to_cpu(src->hsync_end);
+	dst->htotal = le16_to_cpu(src->htotal);
+	dst->hskew = le16_to_cpu(src->hskew);
+	dst->vdisplay = le16_to_cpu(src->vdisplay);
+	dst->vsync_start = le16_to_cpu(src->vsync_start);
+	dst->vsync_end = le16_to_cpu(src->vsync_end);
+	dst->vtotal = le16_to_cpu(src->vtotal);
+	dst->vscan = le16_to_cpu(src->vscan);
+	dst->flags = flags & ~DRM_MODE_FLAG_PIC_AR_MASK;
+	dst->type = src->type;
+
+	switch (flags & DRM_MODE_FLAG_PIC_AR_MASK) {
+	case DRM_MODE_FLAG_PIC_AR_4_3:
+		dst->picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3;
+		break;
+	case DRM_MODE_FLAG_PIC_AR_16_9:
+		dst->picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9;
+		break;
+	case DRM_MODE_FLAG_PIC_AR_64_27:
+		dst->picture_aspect_ratio = HDMI_PICTURE_ASPECT_64_27;
+		break;
+	case DRM_MODE_FLAG_PIC_AR_256_135:
+		dst->picture_aspect_ratio = HDMI_PICTURE_ASPECT_256_135;
+		break;
+	default:
+		dst->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
+		break;
+	}
+
+	drm_mode_set_name(dst);
+}
+
+#endif