[PATCH v3 0/6] Generic USB Display driver

[PATCH v3 0/6] Generic USB Display driver

[Noralf Trønnes]

Hi,

A while back I had the idea to turn a Raspberry Pi Zero into a $5
USB to HDMI/SDTV/DSI/DPI display adapter.

This series adds a USB host driver and a device/gadget driver to achieve
that.

The reason for calling it 'Generic' is so anyone can make a USB
display/adapter against this driver, all that's needed is to add a USB
vid:pid. I have done a microcontroller implementation hack just to see
how that would work out[1] (which unconvered a couple of bugs in the
host driver).

The contents of the previous cover letter has been moved to the wiki[2]
since it was getting rather long.

I've made an image[3] with the gadget side set up for the Raspberry Pi
for easy testing. Patch 4 is the only one needed for the host side.

Merge plan
I'm hoping to apply the remaining drm_client patches in time for 5.9.
With that in place it's much easier to apply the patch for the USB
subsystem the following merge window (5.10). Doing both in the same
cycle is possible ofc, but due to the high rate of change in DRM this
_can_ turn out to be tricky. There's no hurry to get the gadget side
merged since I will provide images for the Raspberry Pi. The host driver
I hope to apply in time for 5.9.
Reviews and testing are very much welcome!

Changes since version 2:
- Use donated Openmoko USB pid: 1d50:614d
- 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
- drm_client_init_from_id(): remove locking
- Applied reviewed patches, thanks Sam.

Dependency:
- backlight: Add backlight_device_get_by_name()[4]

Noralf.

[1] https://github.com/notro/gud/tree/master/STM32F769I-DISCO
[2] https://github.com/notro/gud/wiki
[3] https://github.com/notro/gud/wiki/rpi-image
[4] https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git/commit/?h=for-backlight-next&id=479da1f538a2f3547e15f9d5922c611b69ec2fbc


Noralf Trønnes (6):
  drm/client: Add drm_client_init_from_id()
  drm/client: Add drm_client_modeset_disable()
  drm/client: Add a way to set modeset, properties and rotation
  drm: Add Generic USB Display driver
  drm/gud: Add functionality for the USB gadget side
  usb: gadget: function: Add Generic USB Display support

 .../ABI/testing/configfs-usb-gadget-gud_drm   |   10 +
 MAINTAINERS                                   |   10 +
 drivers/gpu/drm/Kconfig                       |    2 +
 drivers/gpu/drm/Makefile                      |    1 +
 drivers/gpu/drm/drm_client.c                  |   44 +-
 drivers/gpu/drm/drm_client_modeset.c          |  157 +++
 drivers/gpu/drm/gud/Kconfig                   |   20 +
 drivers/gpu/drm/gud/Makefile                  |    5 +
 drivers/gpu/drm/gud/gud_drm_connector.c       |  726 ++++++++++
 drivers/gpu/drm/gud/gud_drm_drv.c             |  648 +++++++++
 drivers/gpu/drm/gud/gud_drm_gadget.c          | 1167 +++++++++++++++++
 drivers/gpu/drm/gud/gud_drm_internal.h        |   65 +
 drivers/gpu/drm/gud/gud_drm_pipe.c            |  426 ++++++
 drivers/usb/gadget/Kconfig                    |   12 +
 drivers/usb/gadget/function/Makefile          |    2 +
 drivers/usb/gadget/function/f_gud_drm.c       |  678 ++++++++++
 include/drm/drm_client.h                      |   43 +-
 include/drm/gud_drm.h                         |  374 ++++++
 18 files changed, 4387 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/ABI/testing/configfs-usb-gadget-gud_drm
 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_gadget.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 drivers/usb/gadget/function/f_gud_drm.c
 create mode 100644 include/drm/gud_drm.h

-- 
2.23.0

[PATCH v3 1/6] drm/client: Add drm_client_init_from_id()

[Noralf Trønnes]

drm_client_init_from_id() provides a way for clients to add a client based
on the minor. drm_client_register() is changed to return whether it was
registered or not depending on the unplugged status of the DRM device.
Its only caller drm_fbdev_generic_setup() runs inside probe() so it
doesn't have to check.

v2:
- Move drm_client_modeset_set() to a separate patch with added functions.
- Previous version had drm_client_init_from_id() call
  drm_client_register(). This put the client in a position where it could
  receive hotplugs during init in addition to akward error paths. Instead
  let drm_client_register() return status so clients can know if the DRM
  device is gone or not.

v3:
- Forgot to remove locking with the change in the previous version.
  No need for locking when drm_client_register() is not called.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_client.c | 44 +++++++++++++++++++++++++++++++++++-
 include/drm/drm_client.h     |  4 +++-
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index 8f3d08dc996d..b7a310fef7e2 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -112,6 +112,36 @@ int drm_client_init(struct drm_device *dev, struct drm_client_dev *client,
 }
 EXPORT_SYMBOL(drm_client_init);
 
+/**
+ * drm_client_init_from_id - Initialise a DRM client
+ * @minor_id: DRM minor id
+ * @client: DRM client
+ * @name: Client name
+ * @funcs: DRM client functions (optional)
+ *
+ * This function looks up the drm_device using the minor id and initializes the
+ * client using drm_client_init().
+ *
+ * Returns:
+ * Zero on success or negative error code on failure.
+ */
+int drm_client_init_from_id(unsigned int minor_id, struct drm_client_dev *client,
+			    const char *name, const struct drm_client_funcs *funcs)
+{
+	struct drm_minor *minor;
+	int ret;
+
+	minor = drm_minor_acquire(minor_id);
+	if (IS_ERR(minor))
+		return PTR_ERR(minor);
+
+	ret = drm_client_init(minor->dev, client, name, funcs);
+	drm_minor_release(minor);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_client_init_from_id);
+
 /**
  * drm_client_register - Register client
  * @client: DRM client
@@ -121,14 +151,26 @@ EXPORT_SYMBOL(drm_client_init);
  * drm_client_register() it is no longer permissible to call drm_client_release()
  * directly (outside the unregister callback), instead cleanup will happen
  * automatically on driver unload.
+ *
+ * Returns:
+ * True if the client has been registered, false if the DRM device has already
+ * been unregistered.
  */
-void drm_client_register(struct drm_client_dev *client)
+bool drm_client_register(struct drm_client_dev *client)
 {
 	struct drm_device *dev = client->dev;
+	int idx;
+
+	if (!drm_dev_enter(client->dev, &idx))
+		return false;
 
 	mutex_lock(&dev->clientlist_mutex);
 	list_add(&client->list, &dev->clientlist);
 	mutex_unlock(&dev->clientlist_mutex);
+
+	drm_dev_exit(idx);
+
+	return true;
 }
 EXPORT_SYMBOL(drm_client_register);
 
diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
index 9f5a36a8ef24..76704f48fc46 100644
--- a/include/drm/drm_client.h
+++ b/include/drm/drm_client.h
@@ -109,8 +109,10 @@ struct drm_client_dev {
 
 int drm_client_init(struct drm_device *dev, struct drm_client_dev *client,
 		    const char *name, const struct drm_client_funcs *funcs);
+int drm_client_init_from_id(unsigned int minor_id, struct drm_client_dev *client,
+			    const char *name, const struct drm_client_funcs *funcs);
 void drm_client_release(struct drm_client_dev *client);
-void drm_client_register(struct drm_client_dev *client);
+bool drm_client_register(struct drm_client_dev *client);
 
 void drm_client_dev_unregister(struct drm_device *dev);
 void drm_client_dev_hotplug(struct drm_device *dev);
-- 
2.23.0

[PATCH v3 2/6] drm/client: Add drm_client_modeset_disable()

[Noralf Trønnes]

Add a way for clients to disable all outputs.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_client_modeset.c | 20 ++++++++++++++++++++
 include/drm/drm_client.h             |  1 +
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index 27e2fb41f14d..977bcd063520 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -1225,3 +1225,23 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode)
 	return ret;
 }
 EXPORT_SYMBOL(drm_client_modeset_dpms);
+
+/**
+ * drm_client_modeset_disable() - Disable all outputs
+ * @client: DRM client
+ *
+ * This function disables all outputs by first clearing the modeset array and
+ * then committing the empty modesets.
+ *
+ * Returns:
+ * Zero on success or negative error code on failure.
+ */
+int drm_client_modeset_disable(struct drm_client_dev *client)
+{
+	mutex_lock(&client->modeset_mutex);
+	drm_client_modeset_release(client);
+	mutex_unlock(&client->modeset_mutex);
+
+	return drm_client_modeset_commit(client);
+}
+EXPORT_SYMBOL(drm_client_modeset_disable);
diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
index 76704f48fc46..498089b647da 100644
--- a/include/drm/drm_client.h
+++ b/include/drm/drm_client.h
@@ -168,6 +168,7 @@ int drm_client_modeset_check(struct drm_client_dev *client);
 int drm_client_modeset_commit_locked(struct drm_client_dev *client);
 int drm_client_modeset_commit(struct drm_client_dev *client);
 int drm_client_modeset_dpms(struct drm_client_dev *client, int mode);
+int drm_client_modeset_disable(struct drm_client_dev *client);
 
 /**
  * drm_client_for_each_modeset() - Iterate over client modesets
-- 
2.23.0

[PATCH v3 3/6] drm/client: Add a way to set modeset, properties and rotation

[Noralf Trønnes]

This adds functions for clients that need more control over the
configuration than what's setup by drm_client_modeset_probe().
Connector, fb and display mode can be set using drm_client_modeset_set().
Plane rotation can be set using drm_client_modeset_set_rotation() and
other properties using drm_client_modeset_set_property(). Property
setting is only implemented for atomic drivers.

v2:
- drm_client_modeset_set(): Remove undocumented functionality to clear
  modesets. A disable function takes care of that need now.
- drm_client_modeset_set_property(): Return -EOPNOTSUPP if driver is not
  atomic (Sam)

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_client_modeset.c | 137 +++++++++++++++++++++++++++
 include/drm/drm_client.h             |  38 +++++++-
 2 files changed, 174 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index 977bcd063520..9eee9567b9aa 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -83,6 +83,10 @@ static void drm_client_modeset_release(struct drm_client_dev *client)
 		}
 		modeset->num_connectors = 0;
 	}
+
+	kfree(client->properties);
+	client->properties = NULL;
+	client->num_properties = 0;
 }
 
 void drm_client_modeset_free(struct drm_client_dev *client)
@@ -879,6 +883,130 @@ int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width,
 }
 EXPORT_SYMBOL(drm_client_modeset_probe);
 
+/**
+ * drm_client_modeset_set() - Set modeset configuration
+ * @client: DRM client
+ * @connector: Connector
+ * @mode: Display mode
+ * @fb: Framebuffer
+ *
+ * This function releases any current modeset info, including properties, and
+ * sets the new modeset in the client's modeset array.
+ *
+ * Returns:
+ * Zero on success or negative error code on failure.
+ */
+int drm_client_modeset_set(struct drm_client_dev *client, struct drm_connector *connector,
+			   struct drm_display_mode *mode, struct drm_framebuffer *fb)
+{
+	struct drm_mode_set *modeset;
+	int ret = -ENOENT;
+
+	mutex_lock(&client->modeset_mutex);
+
+	drm_client_modeset_release(client);
+
+	drm_client_for_each_modeset(modeset, client) {
+		if (!connector_has_possible_crtc(connector, modeset->crtc))
+			continue;
+
+		modeset->mode = drm_mode_duplicate(client->dev, mode);
+		if (!modeset->mode) {
+			ret = -ENOMEM;
+			break;
+		}
+
+		drm_mode_set_crtcinfo(modeset->mode, CRTC_INTERLACE_HALVE_V);
+
+		drm_connector_get(connector);
+		modeset->connectors[modeset->num_connectors++] = connector;
+
+		modeset->fb = fb;
+		ret = 0;
+		break;
+	}
+
+	mutex_unlock(&client->modeset_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_client_modeset_set);
+
+/**
+ * drm_client_modeset_set_property() - Set a property on the current configuration
+ * @client: DRM client
+ * @obj: DRM Mode Object
+ * @prop: DRM Property
+ * @value: Property value
+ *
+ * Note: Currently only implemented for atomic drivers.
+ *
+ * Returns:
+ * Zero on success or negative error code on failure.
+ */
+int drm_client_modeset_set_property(struct drm_client_dev *client, struct drm_mode_object *obj,
+				    struct drm_property *prop, u64 value)
+{
+	struct drm_client_property *properties;
+	int ret = 0;
+
+	if (!prop)
+		return -EINVAL;
+
+	if (!drm_drv_uses_atomic_modeset(client->dev))
+		return -EOPNOTSUPP;
+
+	mutex_lock(&client->modeset_mutex);
+
+	properties = krealloc(client->properties,
+			      (client->num_properties + 1) * sizeof(*properties), GFP_KERNEL);
+	if (!properties) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	properties[client->num_properties].obj = obj;
+	properties[client->num_properties].prop = prop;
+	properties[client->num_properties].value = value;
+	client->properties = properties;
+	client->num_properties++;
+unlock:
+	mutex_unlock(&client->modeset_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_client_modeset_set_property);
+
+/**
+ * drm_client_modeset_set_rotation() - Set rotation on the current configuration
+ * @client: DRM client
+ * @value: Rotation value
+ *
+ * Returns:
+ * Zero on success or negative error code on failure.
+ */
+int drm_client_modeset_set_rotation(struct drm_client_dev *client, u64 value)
+{
+	struct drm_plane *plane = NULL;
+	struct drm_mode_set *modeset;
+
+	mutex_lock(&client->modeset_mutex);
+	drm_client_for_each_modeset(modeset, client) {
+		if (modeset->mode) {
+			plane = modeset->crtc->primary;
+			break;
+		}
+	}
+	mutex_unlock(&client->modeset_mutex);
+
+	if (!plane)
+		return -ENOENT;
+
+	return drm_client_modeset_set_property(client, &plane->base,
+					       plane->rotation_property, value);
+}
+EXPORT_SYMBOL(drm_client_modeset_set_rotation);
+
 /**
  * drm_client_rotation() - Check the initial rotation value
  * @modeset: DRM modeset
@@ -973,6 +1101,7 @@ static int drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool
 	struct drm_atomic_state *state;
 	struct drm_modeset_acquire_ctx ctx;
 	struct drm_mode_set *mode_set;
+	unsigned int i;
 	int ret;
 
 	drm_modeset_acquire_init(&ctx, 0);
@@ -1033,6 +1162,14 @@ static int drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool
 		}
 	}
 
+	for (i = 0; i < client->num_properties; i++) {
+		struct drm_client_property *prop = &client->properties[i];
+
+		ret = drm_atomic_set_property(state, NULL, prop->obj, prop->prop, prop->value);
+		if (ret)
+			goto out_state;
+	}
+
 	if (check)
 		ret = drm_atomic_check_only(state);
 	else
diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
index 498089b647da..bdcdc30519e6 100644
--- a/include/drm/drm_client.h
+++ b/include/drm/drm_client.h
@@ -16,6 +16,7 @@ struct drm_file;
 struct drm_framebuffer;
 struct drm_gem_object;
 struct drm_minor;
+struct drm_property;
 struct module;
 
 /**
@@ -64,6 +65,26 @@ struct drm_client_funcs {
 	int (*hotplug)(struct drm_client_dev *client);
 };
 
+/**
+ * struct drm_client_property - DRM client property
+ */
+struct drm_client_property {
+	/**
+	 * @obj: DRM Mode Object to which the property belongs.
+	 */
+	struct drm_mode_object *obj;
+
+	/**
+	 * @prop: DRM Property.
+	 */
+	struct drm_property *prop;
+
+	/**
+	 * @value: Property value.
+	 */
+	u64 value;
+};
+
 /**
  * struct drm_client_dev - DRM client instance
  */
@@ -97,7 +118,7 @@ struct drm_client_dev {
 	struct drm_file *file;
 
 	/**
-	 * @modeset_mutex: Protects @modesets.
+	 * @modeset_mutex: Protects @modesets and @properties.
 	 */
 	struct mutex modeset_mutex;
 
@@ -105,6 +126,16 @@ struct drm_client_dev {
 	 * @modesets: CRTC configurations
 	 */
 	struct drm_mode_set *modesets;
+
+	/**
+	 * @properties: DRM properties attached to the configuration.
+	 */
+	struct drm_client_property *properties;
+
+	/**
+	 * @num_properties: Number of attached properties.
+	 */
+	unsigned int num_properties;
 };
 
 int drm_client_init(struct drm_device *dev, struct drm_client_dev *client,
@@ -163,6 +194,11 @@ void drm_client_buffer_vunmap(struct drm_client_buffer *buffer);
 int drm_client_modeset_create(struct drm_client_dev *client);
 void drm_client_modeset_free(struct drm_client_dev *client);
 int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width, unsigned int height);
+int drm_client_modeset_set(struct drm_client_dev *client, struct drm_connector *connector,
+			   struct drm_display_mode *mode, struct drm_framebuffer *fb);
+int drm_client_modeset_set_property(struct drm_client_dev *client, struct drm_mode_object *obj,
+				    struct drm_property *prop, u64 value);
+int drm_client_modeset_set_rotation(struct drm_client_dev *client, u64 value);
 bool drm_client_rotation(struct drm_mode_set *modeset, unsigned int *rotation);
 int drm_client_modeset_check(struct drm_client_dev *client);
 int drm_client_modeset_commit_locked(struct drm_client_dev *client);
-- 
2.23.0

[PATCH v3 4/6] drm: Add Generic USB Display driver

[Noralf Trønnes]

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

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

[PATCH v3 5/6] drm/gud: Add functionality for the USB gadget side

[Noralf Trønnes]

Since the USB gadget/device has to reach into the DRM internals, part of
the code is placed in the DRM subsystem as a separate module. All calls
into this module runs in process context and are serialized, except one
function that runs in interrupt context.

Since both the gadget side and the DRM side can disable the gadget,
special care has been taken to make that safe.

v2:
- Use drm_client_modeset_disable()

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/gud/Kconfig          |    6 +
 drivers/gpu/drm/gud/Makefile         |    1 +
 drivers/gpu/drm/gud/gud_drm_gadget.c | 1167 ++++++++++++++++++++++++++
 include/drm/gud_drm.h                |   13 +
 4 files changed, 1187 insertions(+)
 create mode 100644 drivers/gpu/drm/gud/gud_drm_gadget.c

diff --git a/drivers/gpu/drm/gud/Kconfig b/drivers/gpu/drm/gud/Kconfig
index 767f1c067ed9..9bbf39ae6191 100644
--- a/drivers/gpu/drm/gud/Kconfig
+++ b/drivers/gpu/drm/gud/Kconfig
@@ -12,3 +12,9 @@ config DRM_GUD
 	  adapters.
 
 	  If M is selected the module will be called gud_drm.
+
+config DRM_GUD_GADGET
+	tristate
+	depends on DRM
+	select LZ4_DECOMPRESS
+	select BACKLIGHT_CLASS_DEVICE
diff --git a/drivers/gpu/drm/gud/Makefile b/drivers/gpu/drm/gud/Makefile
index 73ed7ef3da94..fb5cfbd93d0f 100644
--- a/drivers/gpu/drm/gud/Makefile
+++ b/drivers/gpu/drm/gud/Makefile
@@ -2,3 +2,4 @@
 
 gud_drm-objs			:= gud_drm_drv.o gud_drm_pipe.o gud_drm_connector.o
 obj-$(CONFIG_DRM_GUD)		+= gud_drm.o
+obj-$(CONFIG_DRM_GUD_GADGET)	+= gud_drm_gadget.o
diff --git a/drivers/gpu/drm/gud/gud_drm_gadget.c b/drivers/gpu/drm/gud/gud_drm_gadget.c
new file mode 100644
index 000000000000..913aa5a8d075
--- /dev/null
+++ b/drivers/gpu/drm/gud/gud_drm_gadget.c
@@ -0,0 +1,1167 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2020 Noralf Trønnes
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/lz4.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+
+#include <drm/drm_client.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_mode_object.h>
+#include <drm/drm_plane.h>
+#include <drm/drm_rect.h>
+#include <drm/gud_drm.h>
+
+#include "gud_drm_internal.h"
+
+/*
+ * Concurrency:
+ * Calls into this module from f_gud_drm are serialized and run in process
+ * context except gud_drm_gadget_ctrl_get() which is run in interrupt context.
+ *
+ * Termination:
+ * A DRM client can not release itself, only the DRM driver which resources the
+ * client uses can do that.
+ * This means that there are 2 paths to stop the gadget function:
+ * - Unregistering the DRM driver (module unload)
+ * - Disabling the USB gadget (configfs unbind)
+ *
+ * A use counter protects the gadget should the client go away. A kref is used
+ * to control the release of the gud_drm_gadget structure shared by the 2 actors.
+ *
+ * Backlight:
+ * If there's a backlight device it's attached to the first connector.
+ */
+
+struct gud_drm_gadget_connector {
+	struct drm_connector *connector;
+
+	const struct gud_drm_property *properties;
+	unsigned int num_properties;
+	const char *tv_mode_enum_names;
+	unsigned int num_tv_mode_enum_names;
+	struct backlight_device *backlight;
+
+	spinlock_t lock; /* Protects the following members: */
+	enum drm_connector_status status;
+	unsigned int width_mm;
+	unsigned int height_mm;
+	struct gud_drm_display_mode *modes;
+	unsigned int num_modes;
+	void *edid;
+	size_t edid_len;
+	bool changed;
+};
+
+struct gud_drm_gadget {
+	struct kref refcount;
+	refcount_t usecnt;
+	struct drm_client_dev client;
+	struct backlight_device *backlight;
+
+	const u32 *formats;
+	unsigned int format_count;
+
+	const struct gud_drm_property *properties;
+	unsigned int num_properties;
+
+	struct gud_drm_gadget_connector *connectors;
+	unsigned int connector_count;
+
+	struct drm_rect set_buffer_rect;
+	u32 set_buffer_length;
+	u8 set_buffer_compression;
+	u32 set_buffer_compressed_length;
+
+	struct drm_client_buffer *buffer;
+	struct drm_client_buffer *buffer_check;
+	u8 brightness;
+	bool check_ok;
+
+	size_t max_buffer_size;
+	void *work_buf;
+};
+
+static int gud_drm_gadget_probe_connector(struct gud_drm_gadget_connector *gconn)
+{
+	struct drm_connector *connector = gconn->connector;
+	struct gud_drm_display_mode *modes = NULL;
+	struct drm_device *drm = connector->dev;
+	struct drm_display_mode *mode;
+	void *edid_data, *edid = NULL;
+	unsigned int num_modes = 0;
+	size_t edid_len = 0;
+	unsigned long flags;
+	unsigned int i = 0;
+	int ret = 0;
+
+	mutex_lock(&drm->mode_config.mutex);
+
+	connector->funcs->fill_modes(connector,
+				     drm->mode_config.max_width,
+				     drm->mode_config.max_height);
+
+	list_for_each_entry(mode, &connector->modes, head)
+		num_modes++;
+
+	if (!num_modes)
+		goto update;
+
+	modes = kmalloc_array(num_modes, sizeof(*modes), GFP_KERNEL);
+	if (!modes) {
+		ret = -ENOMEM;
+		num_modes = 0;
+		goto update;
+	}
+
+	list_for_each_entry(mode, &connector->modes, head)
+		gud_drm_from_display_mode(&modes[i++], mode);
+
+	if (!connector->edid_blob_ptr)
+		goto update;
+
+	edid_data = connector->edid_blob_ptr->data;
+	edid_len = connector->edid_blob_ptr->length;
+	if (!edid_data || !edid_len) {
+		edid_len = 0;
+		goto update;
+	}
+
+	edid = kmemdup(edid_data, edid_len, GFP_KERNEL);
+	if (!edid) {
+		ret = -ENOMEM;
+		edid_len = 0;
+	}
+
+update:
+	spin_lock_irqsave(&gconn->lock, flags);
+	if (gconn->status != connector->status || gconn->num_modes != num_modes ||
+	    gconn->edid_len != edid_len ||
+	    (gconn->modes && modes && memcmp(gconn->modes, modes, num_modes * sizeof(*modes))) ||
+	    (gconn->edid && edid && memcmp(gconn->edid, edid, edid_len)))
+		gconn->changed = true;
+	swap(gconn->modes, modes);
+	gconn->num_modes = num_modes;
+	swap(gconn->edid, edid);
+	gconn->edid_len = edid_len;
+	gconn->width_mm = connector->display_info.width_mm;
+	gconn->height_mm = connector->display_info.height_mm;
+	gconn->status = connector->status;
+	spin_unlock_irqrestore(&gconn->lock, flags);
+
+	mutex_unlock(&drm->mode_config.mutex);
+
+	kfree(edid);
+	kfree(modes);
+
+	return ret;
+}
+
+static void gud_drm_gadget_probe_connectors(struct gud_drm_gadget *gdg)
+{
+	unsigned int i;
+
+	for (i = 0; i < gdg->connector_count; i++)
+		gud_drm_gadget_probe_connector(&gdg->connectors[i]);
+}
+
+static bool gud_drm_gadget_check_buffer(struct gud_drm_gadget *gdg,
+					struct drm_client_buffer *buffer,
+					struct drm_display_mode *mode,
+					u32 format)
+{
+	struct drm_framebuffer *fb;
+
+	if (!buffer)
+		return false;
+
+	fb = buffer->fb;
+
+	return fb->format->format == format &&
+	       fb->width == mode->hdisplay && fb->height == mode->vdisplay;
+}
+
+static bool gud_drm_gadget_set_connector_property(struct drm_client_dev *client,
+						  struct drm_connector *connector,
+						  u16 prop, u64 val, int *ret)
+{
+	struct drm_mode_config *config = &connector->dev->mode_config;
+	struct drm_property *property;
+
+	switch (prop) {
+	case GUD_DRM_PROPERTY_TV_SELECT_SUBCONNECTOR:
+		property = config->tv_select_subconnector_property;
+		break;
+	case GUD_DRM_PROPERTY_TV_LEFT_MARGIN:
+		property = config->tv_left_margin_property;
+		break;
+	case GUD_DRM_PROPERTY_TV_RIGHT_MARGIN:
+		property = config->tv_right_margin_property;
+		break;
+	case GUD_DRM_PROPERTY_TV_TOP_MARGIN:
+		property = config->tv_top_margin_property;
+		break;
+	case GUD_DRM_PROPERTY_TV_BOTTOM_MARGIN:
+		property = config->tv_bottom_margin_property;
+		break;
+	case GUD_DRM_PROPERTY_TV_MODE:
+		property = config->tv_mode_property;
+		break;
+	case GUD_DRM_PROPERTY_TV_BRIGHTNESS:
+		property = config->tv_brightness_property;
+		break;
+	case GUD_DRM_PROPERTY_TV_CONTRAST:
+		property = config->tv_contrast_property;
+		break;
+	case GUD_DRM_PROPERTY_TV_FLICKER_REDUCTION:
+		property = config->tv_flicker_reduction_property;
+		break;
+	case GUD_DRM_PROPERTY_TV_OVERSCAN:
+		property = config->tv_overscan_property;
+		break;
+	case GUD_DRM_PROPERTY_TV_SATURATION:
+		property = config->tv_saturation_property;
+		break;
+	case GUD_DRM_PROPERTY_TV_HUE:
+		property = config->tv_hue_property;
+		break;
+	default:
+		return false;
+	}
+
+	*ret = drm_client_modeset_set_property(client, &connector->base, property, val);
+
+	return true;
+}
+
+static int gud_drm_gadget_check(struct gud_drm_gadget *gdg, struct gud_drm_req_set_state *req,
+				size_t size)
+{
+	struct drm_client_dev *client = &gdg->client;
+	u32 format = le32_to_cpu(req->format);
+	struct drm_client_buffer *buffer;
+	struct drm_connector *connector;
+	struct drm_display_mode mode;
+	unsigned int i;
+	void *vaddr;
+	int ret;
+
+	if (size < sizeof(struct gud_drm_req_set_state))
+		return -EINVAL;
+
+	if (size != struct_size(req, properties, req->num_properties))
+		return -EINVAL;
+
+	memset(&mode, 0, sizeof(mode));
+	gud_drm_to_display_mode(&mode, &req->mode);
+
+	gdg->check_ok = false;
+
+	if (!mode.hdisplay || !format)
+		return -EINVAL;
+
+	if (req->connector >= gdg->connector_count)
+		return -EINVAL;
+
+	connector = gdg->connectors[req->connector].connector;
+
+	if (gdg->buffer_check) {
+		drm_client_framebuffer_delete(gdg->buffer_check);
+		gdg->buffer_check = NULL;
+	}
+
+	if (!gud_drm_gadget_check_buffer(gdg, gdg->buffer, &mode, format)) {
+		buffer = drm_client_framebuffer_create(client, mode.hdisplay, mode.vdisplay,
+						       format);
+		if (IS_ERR(buffer))
+			return PTR_ERR(buffer);
+
+		vaddr = drm_client_buffer_vmap(buffer);
+		if (IS_ERR(vaddr)) {
+			drm_client_framebuffer_delete(buffer);
+			return PTR_ERR(vaddr);
+		}
+
+		gdg->buffer_check = buffer;
+	} else {
+		buffer = gdg->buffer;
+	}
+
+	ret = drm_client_modeset_set(client, connector, &mode, buffer->fb);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < req->num_properties; i++) {
+		u16 prop = le16_to_cpu(req->properties[i].prop);
+		u64 val = le64_to_cpu(req->properties[i].val);
+
+		if (gud_drm_gadget_set_connector_property(client, connector, prop, val, &ret)) {
+			if (ret)
+				return ret;
+			continue;
+		}
+
+		switch (prop) {
+		case GUD_DRM_PROPERTY_BACKLIGHT_BRIGHTNESS:
+			if (val > 100)
+				return -EINVAL;
+			gdg->brightness = val;
+			break;
+		case GUD_DRM_PROPERTY_ROTATION:
+			ret = drm_client_modeset_set_rotation(client, val);
+			break;
+		default:
+			pr_err("%s: Unknown property: %u\n", __func__, prop);
+			continue;
+		}
+
+		if (ret)
+			return ret;
+	}
+
+	ret = drm_client_modeset_check(&gdg->client);
+	if (ret)
+		return ret;
+
+	gdg->check_ok = true;
+
+	return 0;
+}
+
+static int gud_drm_gadget_commit(struct gud_drm_gadget *gdg)
+{
+	int ret;
+
+	if (!gdg->check_ok)
+		return -EINVAL;
+
+	if (gdg->backlight) {
+		int val, max_brightness = gdg->backlight->props.max_brightness;
+
+		val = DIV64_U64_ROUND_UP(gdg->brightness * max_brightness, 100);
+		ret = backlight_device_set_brightness(gdg->backlight, val);
+		if (ret)
+			return ret;
+	}
+
+	ret = drm_client_modeset_commit(&gdg->client);
+	if (ret)
+		return ret;
+
+	if (gdg->buffer_check) {
+		drm_client_framebuffer_delete(gdg->buffer);
+		gdg->buffer = gdg->buffer_check;
+		gdg->buffer_check = NULL;
+	}
+
+	return 0;
+}
+
+static size_t gud_drm_gadget_write_buffer_memcpy(struct drm_client_buffer *buffer,
+						 const void *src, size_t len,
+						 struct drm_rect *rect)
+{
+	unsigned int cpp = buffer->fb->format->cpp[0];
+	size_t dst_pitch = buffer->fb->pitches[0];
+	size_t src_pitch = drm_rect_width(rect) * cpp;
+	unsigned int y;
+	void *dst;
+
+	/* Get the address, it's already mapped */
+	dst = drm_client_buffer_vmap(buffer);
+	dst += rect->y1 * dst_pitch;
+	dst += rect->x1 * cpp;
+
+	for (y = 0; y < drm_rect_height(rect) && len; y++) {
+		src_pitch = min(src_pitch, len);
+		memcpy(dst, src, src_pitch);
+		src += src_pitch;
+		dst += dst_pitch;
+		len -= src_pitch;
+	}
+
+	return len;
+}
+
+static bool gud_drm_gadget_check_rect(struct drm_client_buffer *buffer, struct drm_rect *rect)
+{
+	return buffer->fb && rect->x1 < rect->x2 && rect->y1 < rect->y2 &&
+	       rect->x2 <= buffer->fb->width && rect->y2 <= buffer->fb->height;
+}
+
+int gud_drm_gadget_write_buffer(struct gud_drm_gadget *gdg, const void *buf, size_t len)
+{
+	struct drm_client_buffer *buffer = gdg->buffer ? gdg->buffer : gdg->buffer_check;
+	struct drm_rect *rect = &gdg->set_buffer_rect;
+	u8 compression = gdg->set_buffer_compression;
+	struct drm_framebuffer *fb;
+	size_t remain;
+	int ret;
+
+	pr_debug("%s: len=%zu compression=0x%x\n", __func__, len, compression);
+
+	if (WARN_ON_ONCE(!buffer))
+		return -ENOMEM;
+
+	if (!gud_drm_gadget_check_rect(buffer, rect)) {
+		pr_err("%s: Rectangle doesn't fit: " DRM_RECT_FMT "\n",
+		       __func__, DRM_RECT_ARG(rect));
+		return -EINVAL;
+	}
+
+	fb = buffer->fb;
+
+	if (compression & GUD_DRM_COMPRESSION_LZ4) {
+		if (len != gdg->set_buffer_compressed_length) {
+			pr_err("%s: Buffer compressed len differs: %zu != %zu\n",
+			       __func__, len, gdg->set_buffer_compressed_length);
+			return -EINVAL;
+		}
+
+		ret = LZ4_decompress_safe(buf, gdg->work_buf, len, gdg->max_buffer_size);
+		if (ret < 0) {
+			pr_err("%s: Failed to decompress buffer\n", __func__);
+			return -EIO;
+		}
+
+		buf = gdg->work_buf;
+		len = ret;
+	}
+
+	if (len != gdg->set_buffer_length) {
+		pr_err("%s: Buffer len differs: %zu != %zu\n",
+		       __func__, len, gdg->set_buffer_length);
+		return -EINVAL;
+	}
+
+	if (len > (drm_rect_width(rect) * drm_rect_height(rect) * fb->format->cpp[0])) {
+		pr_err("%s: Buffer is too big for rectangle: " DRM_RECT_FMT " len=%zu\n",
+		       __func__, DRM_RECT_ARG(rect), len);
+		return -EINVAL;
+	}
+
+	remain = gud_drm_gadget_write_buffer_memcpy(buffer, buf, len, rect);
+	if (remain) {
+		pr_err("%s: Failed to write buffer: remain=%zu\n", __func__, remain);
+		return -EIO;
+	}
+
+	return drm_client_framebuffer_flush(buffer, rect);
+}
+EXPORT_SYMBOL(gud_drm_gadget_write_buffer);
+
+int gud_drm_gadget_set_buffer(struct gud_drm_gadget *gdg, struct gud_drm_req_set_buffer *req)
+{
+	u32 compressed_length = le32_to_cpu(req->compressed_length);
+	u32 length = le32_to_cpu(req->length);
+	struct drm_client_buffer *buffer;
+	struct drm_rect rect;
+	int ret = 0;
+
+	if (!refcount_inc_not_zero(&gdg->usecnt))
+		return -ENODEV;
+
+	buffer = gdg->buffer ? gdg->buffer : gdg->buffer_check;
+	if (!buffer) {
+		ret = -ENOENT;
+		goto out;
+	}
+
+	drm_rect_init(&rect, le32_to_cpu(req->x), le32_to_cpu(req->y),
+		      le32_to_cpu(req->width), le32_to_cpu(req->height));
+
+	pr_debug("%s: " DRM_RECT_FMT "\n", __func__, DRM_RECT_ARG(&rect));
+
+	if (!gud_drm_gadget_check_rect(buffer, &rect)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (req->compression & ~GUD_DRM_COMPRESSION_LZ4) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	gdg->set_buffer_rect = rect;
+	gdg->set_buffer_length = length;
+
+	if (req->compression) {
+		if (!compressed_length) {
+			ret = -EINVAL;
+			goto out;
+		}
+		gdg->set_buffer_compression = req->compression;
+		gdg->set_buffer_compressed_length = compressed_length;
+		length = compressed_length;
+	} else {
+		gdg->set_buffer_compression = 0;
+		gdg->set_buffer_compressed_length = 0;
+	}
+out:
+	refcount_dec(&gdg->usecnt);
+
+	return ret ? ret : length;
+}
+EXPORT_SYMBOL(gud_drm_gadget_set_buffer);
+
+static void gud_drm_gadget_delete_buffers(struct gud_drm_gadget *gdg)
+{
+	drm_client_framebuffer_delete(gdg->buffer_check);
+	drm_client_framebuffer_delete(gdg->buffer);
+	gdg->buffer_check = NULL;
+	gdg->buffer = NULL;
+}
+
+int gud_drm_gadget_disable_pipe(struct gud_drm_gadget *gdg)
+{
+	int ret;
+
+	ret = drm_client_modeset_disable(&gdg->client);
+	gud_drm_gadget_delete_buffers(gdg);
+
+	return ret;
+}
+EXPORT_SYMBOL(gud_drm_gadget_disable_pipe);
+
+static int gud_drm_gadget_ctrl_get_display_descriptor(struct gud_drm_gadget *gdg, u16 value,
+						      void *data, size_t size)
+{
+	struct drm_device *drm = gdg->client.dev;
+	struct gud_drm_display_descriptor desc;
+	u8 type = value >> 8;
+	u8 index = value & 0xff;
+
+	if (type != GUD_DRM_USB_DT_DISPLAY || index)
+		return -EINVAL;
+
+	desc.bLength = sizeof(desc);
+	desc.bDescriptorType = GUD_DRM_USB_DT_DISPLAY;
+
+	desc.bVersion = 1;
+	desc.bMaxBufferSizeOrder = ilog2(gdg->max_buffer_size);
+	desc.bmFlags = 0;
+	desc.bCompression = GUD_DRM_COMPRESSION_LZ4;
+
+	desc.dwMinWidth = cpu_to_le32(drm->mode_config.min_width);
+	desc.dwMaxWidth = cpu_to_le32(drm->mode_config.max_width);
+	desc.dwMinHeight = cpu_to_le32(drm->mode_config.min_height);
+	desc.dwMaxHeight = cpu_to_le32(drm->mode_config.max_height);
+	desc.bNumFormats = gdg->format_count;
+	desc.bNumProperties = gdg->num_properties;
+	desc.bNumConnectors = gdg->connector_count;
+
+	size = min(size, sizeof(desc));
+	memcpy(data, &desc, size);
+
+	return size;
+}
+
+static void gud_drm_gadget_ctrl_get_formats(struct gud_drm_gadget *gdg, __le32 *formats)
+{
+	unsigned int i;
+
+	for (i = 0; i < gdg->format_count; i++)
+		formats[i] = cpu_to_le32(gdg->formats[i]);
+}
+
+static int gud_drm_gadget_ctrl_get_connector(struct gud_drm_gadget *gdg, unsigned int index,
+					     struct gud_drm_req_get_connector *desc)
+{
+	struct gud_drm_gadget_connector *gconn;
+
+	if (index >= gdg->connector_count)
+		return -EINVAL;
+
+	memset(desc, 0, sizeof(*desc));
+
+	gconn = &gdg->connectors[index];
+
+	desc->connector_type = gconn->connector->connector_type;
+	desc->flags = cpu_to_le32(GUD_DRM_CONNECTOR_FLAGS_POLL);
+	desc->num_properties = gconn->num_properties;
+
+	return 0;
+}
+
+static struct gud_drm_gadget_connector *
+gud_drm_gadget_get_gconn(struct gud_drm_gadget *gdg, unsigned int index)
+{
+	if (index >= gdg->connector_count)
+		return NULL;
+
+	return &gdg->connectors[index];
+}
+
+static int gud_drm_gadget_ctrl_get_connector_status(struct gud_drm_gadget *gdg, unsigned int index,
+						    struct gud_drm_req_get_connector_status *status)
+{
+	struct gud_drm_gadget_connector *gconn;
+	unsigned long flags;
+
+	gconn = gud_drm_gadget_get_gconn(gdg, index);
+	if (!gconn)
+		return -ENOENT;
+
+	memset(status, 0, sizeof(*status));
+
+	spin_lock_irqsave(&gconn->lock, flags);
+	status->status = gconn->status;
+	if (gconn->changed) {
+		status->status |= GUD_DRM_CONNECTOR_STATUS_CHANGED;
+		gconn->changed = false;
+	}
+	if (gconn->num_modes)
+		status->num_modes = cpu_to_le16(gconn->num_modes);
+	if (gconn->edid_len)
+		status->edid_len = cpu_to_le16(gconn->edid_len);
+	spin_unlock_irqrestore(&gconn->lock, flags);
+
+	return 0;
+}
+
+/* This runs in interrupt context */
+int gud_drm_gadget_ctrl_get(struct gud_drm_gadget *gdg, u8 request,
+			    u16 index, void *data, size_t size)
+{
+	struct gud_drm_gadget_connector *gconn;
+	unsigned long flags;
+	int ret = -EINVAL;
+
+	pr_debug("%s: request=0x%x index=%u size=%zu\n", __func__, request, index, size);
+
+	if (!refcount_inc_not_zero(&gdg->usecnt))
+		return -ENODEV;
+
+	if (!size)
+		goto out;
+
+	switch (request) {
+	case USB_REQ_GET_DESCRIPTOR:
+		ret = gud_drm_gadget_ctrl_get_display_descriptor(gdg, index, data, size);
+		break;
+	case GUD_DRM_USB_REQ_GET_FORMATS:
+		if (!index && size == gdg->format_count * sizeof(u32)) {
+			gud_drm_gadget_ctrl_get_formats(gdg, data);
+			ret = 0;
+		}
+		break;
+	case GUD_DRM_USB_REQ_GET_PROPERTIES:
+		if (!index && size == gdg->num_properties * sizeof(*gdg->properties)) {
+			memcpy(data, gdg->properties, size);
+			ret = 0;
+		}
+		break;
+	case GUD_DRM_USB_REQ_GET_CONNECTOR:
+		if (size == sizeof(struct gud_drm_req_get_connector))
+			ret = gud_drm_gadget_ctrl_get_connector(gdg, index, data);
+		break;
+	case GUD_DRM_USB_REQ_GET_CONNECTOR_PROPERTIES:
+		gconn = gud_drm_gadget_get_gconn(gdg, index);
+		if (gconn && size == gconn->num_properties * sizeof(*gconn->properties)) {
+			memcpy(data, gconn->properties, size);
+			ret = 0;
+		}
+		break;
+	case GUD_DRM_USB_REQ_GET_CONNECTOR_TV_MODE_VALUES:
+		gconn = gud_drm_gadget_get_gconn(gdg, index);
+		if (gconn && size == gconn->num_tv_mode_enum_names * DRM_PROP_NAME_LEN) {
+			memcpy(data, gconn->tv_mode_enum_names, size);
+			ret = 0;
+		}
+		break;
+	case GUD_DRM_USB_REQ_GET_CONNECTOR_STATUS:
+		if (size == sizeof(struct gud_drm_req_get_connector_status))
+			ret = gud_drm_gadget_ctrl_get_connector_status(gdg, index, data);
+		break;
+	case GUD_DRM_USB_REQ_GET_CONNECTOR_MODES:
+		gconn = gud_drm_gadget_get_gconn(gdg, index);
+		spin_lock_irqsave(&gconn->lock, flags);
+		if (gconn && size == gconn->num_modes * sizeof(*gconn->modes)) {
+			memcpy(data, gconn->modes, size);
+			ret = 0;
+		}
+		spin_unlock_irqrestore(&gconn->lock, flags);
+		break;
+	case GUD_DRM_USB_REQ_GET_CONNECTOR_EDID:
+		gconn = gud_drm_gadget_get_gconn(gdg, index);
+		spin_lock_irqsave(&gconn->lock, flags);
+		if (gconn && size == gconn->edid_len) {
+			memcpy(data, gconn->edid, size);
+			ret = 0;
+		}
+		spin_unlock_irqrestore(&gconn->lock, flags);
+		break;
+	}
+out:
+	refcount_dec(&gdg->usecnt);
+
+	return !ret ? size : ret;
+}
+EXPORT_SYMBOL(gud_drm_gadget_ctrl_get);
+
+int gud_drm_gadget_ctrl_set(struct gud_drm_gadget *gdg, u8 request,
+			    u16 index, void *data, size_t size)
+{
+	struct gud_drm_gadget_connector *gconn;
+	int dpms, ret = -EINVAL;
+
+	pr_debug("%s: request=0x%x index=%u size=%zu\n", __func__, request, index, size);
+
+	if (!refcount_inc_not_zero(&gdg->usecnt))
+		return -ENODEV;
+
+	switch (request) {
+	case GUD_DRM_USB_REQ_SET_CONNECTOR_FORCE_DETECT:
+		gconn = gud_drm_gadget_get_gconn(gdg, index);
+		if (gconn)
+			ret = gud_drm_gadget_probe_connector(gconn);
+		break;
+	case GUD_DRM_USB_REQ_SET_STATE_CHECK:
+		ret = gud_drm_gadget_check(gdg, data, size);
+		break;
+	case GUD_DRM_USB_REQ_SET_STATE_COMMIT:
+		if (!size)
+			ret = gud_drm_gadget_commit(gdg);
+		break;
+	case GUD_DRM_USB_REQ_SET_CONTROLLER_ENABLE:
+		if (size == sizeof(u8)) {
+			if (*(u8 *)data == 0)
+				ret = gud_drm_gadget_disable_pipe(gdg);
+			else
+				ret = 0;
+		}
+		break;
+	case GUD_DRM_USB_REQ_SET_DISPLAY_ENABLE:
+		if (size == sizeof(u8)) {
+			dpms = *(u8 *)data ? DRM_MODE_DPMS_ON : DRM_MODE_DPMS_OFF;
+			ret = drm_client_modeset_dpms(&gdg->client, dpms);
+		}
+		break;
+	}
+
+	refcount_dec(&gdg->usecnt);
+
+	return ret;
+}
+EXPORT_SYMBOL(gud_drm_gadget_ctrl_set);
+
+static int gud_drm_gadget_get_formats(struct gud_drm_gadget *gdg, u8 *max_cpp)
+{
+	struct drm_device *drm = gdg->client.dev;
+	struct drm_plane *plane;
+	unsigned int i;
+	u32 *formats;
+	int ret;
+
+	*max_cpp = 0;
+
+	drm_for_each_plane(plane, drm) {
+		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
+			break;
+	}
+
+	formats = kcalloc(plane->format_count, sizeof(u32), GFP_KERNEL);
+	if (!formats)
+		return -ENOMEM;
+
+	for (i = 0; i < plane->format_count; i++) {
+		const struct drm_format_info *fmt;
+
+		fmt = drm_format_info(plane->format_types[i]);
+		if (fmt->num_planes != 1)
+			continue;
+
+		/*
+		 * Supporting 24-bit bpp would add complexity so don't bother.
+		 * It's hardly used and compression removes much of the gain.
+		 */
+		if (fmt->cpp[0] == 3)
+			continue;
+
+		if (*max_cpp < fmt->cpp[0])
+			*max_cpp = fmt->cpp[0];
+
+		formats[gdg->format_count++] = plane->format_types[i];
+	}
+
+	if (!gdg->format_count) {
+		ret = -ENOENT;
+		goto err_free;
+	}
+
+	gdg->formats = formats;
+
+	return 0;
+
+err_free:
+	kfree(formats);
+
+	return ret;
+}
+
+static int gud_drm_gadget_get_rotation_property(struct drm_device *drm, u16 *prop, u64 *val)
+{
+	struct drm_property_enum *prop_enum;
+	struct drm_plane *plane;
+	unsigned int num_props = 0;
+	u16 bitmask = 0;
+
+	drm_for_each_plane(plane, drm) {
+		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
+			break;
+	}
+
+	if (!plane->rotation_property)
+		return 0;
+
+	list_for_each_entry(prop_enum, &plane->rotation_property->enum_list, head) {
+		num_props++;
+		bitmask |= BIT(prop_enum->value);
+	}
+
+	*prop = GUD_DRM_PROPERTY_ROTATION;
+	*val = bitmask;
+
+	return 1;
+}
+
+static int gud_drm_gadget_get_properties(struct gud_drm_gadget *gdg)
+{
+	struct gud_drm_property *properties;
+	u16 prop;
+	u64 val;
+	int ret;
+
+	ret = gud_drm_gadget_get_rotation_property(gdg->client.dev, &prop, &val);
+	if (ret <= 0)
+		return ret;
+
+	properties = kcalloc(1, sizeof(*properties), GFP_KERNEL);
+	if (!properties)
+		return -ENOMEM;
+
+	gdg->properties = properties;
+	gdg->num_properties++;
+
+	properties[0].prop = cpu_to_le16(prop);
+	properties[0].val = cpu_to_le64(val);
+
+	return 0;
+}
+
+static int gud_drm_gadget_get_connector_properties(struct gud_drm_gadget *gdg,
+						   struct gud_drm_gadget_connector *gconn)
+{
+	struct drm_device *drm = gdg->client.dev;
+	struct drm_mode_config *config = &drm->mode_config;
+	struct drm_connector *connector = gconn->connector;
+	struct drm_object_properties *conn_props = connector->base.properties;
+	struct gud_drm_property *properties;
+	unsigned int i, ret = 0;
+	u16 prop;
+	u64 val;
+
+	mutex_lock(&drm->mode_config.mutex);
+
+	if (!conn_props->count)
+		goto unlock;
+
+	/* Add room for possible backlight */
+	properties = kcalloc(conn_props->count + 1, sizeof(*properties), GFP_KERNEL);
+	if (!properties) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	gconn->properties = properties;
+
+	for (i = 0; i < conn_props->count; i++) {
+		struct drm_property *property = conn_props->properties[i];
+
+		if (property == config->tv_select_subconnector_property) {
+			prop = GUD_DRM_PROPERTY_TV_SELECT_SUBCONNECTOR;
+			val = connector->state->tv.subconnector;
+		} else if (property == config->tv_left_margin_property) {
+			prop = GUD_DRM_PROPERTY_TV_LEFT_MARGIN;
+			val = connector->state->tv.margins.left;
+		} else if (property == config->tv_right_margin_property) {
+			prop = GUD_DRM_PROPERTY_TV_RIGHT_MARGIN;
+			val = connector->state->tv.margins.right;
+		} else if (property == config->tv_top_margin_property) {
+			prop = GUD_DRM_PROPERTY_TV_TOP_MARGIN;
+			val = connector->state->tv.margins.top;
+		} else if (property == config->tv_bottom_margin_property) {
+			prop = GUD_DRM_PROPERTY_TV_BOTTOM_MARGIN;
+			val = connector->state->tv.margins.bottom;
+		} else if (property == config->tv_mode_property) {
+			struct drm_property_enum *prop_enum;
+			char *buf;
+
+			list_for_each_entry(prop_enum, &property->enum_list, head)
+				gconn->num_tv_mode_enum_names++;
+
+			if (WARN_ON(!gconn->num_tv_mode_enum_names)) {
+				ret = -EINVAL;
+				goto unlock;
+			}
+
+			buf = kcalloc(gconn->num_tv_mode_enum_names, DRM_PROP_NAME_LEN, GFP_KERNEL);
+			if (!buf) {
+				ret = -ENOMEM;
+				goto unlock;
+			}
+
+			gconn->tv_mode_enum_names = buf;
+
+			list_for_each_entry(prop_enum, &property->enum_list, head) {
+				strncpy(buf, prop_enum->name, DRM_PROP_NAME_LEN);
+				buf += DRM_PROP_NAME_LEN;
+			}
+
+			prop = GUD_DRM_PROPERTY_TV_MODE;
+			val = connector->state->tv.mode;
+			val |= gconn->num_tv_mode_enum_names << GUD_DRM_USB_CONNECTOR_TV_MODE_NUM_SHIFT;
+		} else if (property == config->tv_brightness_property) {
+			prop = GUD_DRM_PROPERTY_TV_BRIGHTNESS;
+			val = connector->state->tv.brightness;
+		} else if (property == config->tv_contrast_property) {
+			prop = GUD_DRM_PROPERTY_TV_CONTRAST;
+			val = connector->state->tv.contrast;
+		} else if (property == config->tv_flicker_reduction_property) {
+			prop = GUD_DRM_PROPERTY_TV_FLICKER_REDUCTION;
+			val = connector->state->tv.flicker_reduction;
+		} else if (property == config->tv_overscan_property) {
+			prop = GUD_DRM_PROPERTY_TV_OVERSCAN;
+			val = connector->state->tv.overscan;
+		} else if (property == config->tv_saturation_property) {
+			prop = GUD_DRM_PROPERTY_TV_SATURATION;
+			val = connector->state->tv.saturation;
+		} else if (property == config->tv_hue_property) {
+			prop = GUD_DRM_PROPERTY_TV_HUE;
+			val = connector->state->tv.hue;
+		} else {
+			continue;
+		}
+
+		properties[gconn->num_properties].prop = cpu_to_le16(prop);
+		properties[gconn->num_properties++].val = cpu_to_le64(val);
+	}
+
+	if (!connector->index && gdg->backlight) {
+		struct backlight_properties *props = &gdg->backlight->props;
+
+		prop = GUD_DRM_PROPERTY_BACKLIGHT_BRIGHTNESS;
+		val = DIV64_U64_ROUND_UP(props->brightness * 100, props->max_brightness);
+		properties[gconn->num_properties].prop = cpu_to_le16(prop);
+		properties[gconn->num_properties++].val = cpu_to_le64(val);
+		gconn->backlight = gdg->backlight;
+	}
+unlock:
+	mutex_unlock(&drm->mode_config.mutex);
+
+	return ret;
+}
+
+static int gud_drm_gadget_get_connectors(struct gud_drm_gadget *gdg)
+{
+	struct gud_drm_gadget_connector *connectors = NULL;
+	struct drm_connector_list_iter conn_iter;
+	struct drm_device *drm = gdg->client.dev;
+	unsigned int connector_count = 0;
+	struct drm_connector *connector;
+	int ret = 0;
+
+	drm_connector_list_iter_begin(drm, &conn_iter);
+	drm_client_for_each_connector_iter(connector, &conn_iter) {
+		struct gud_drm_gadget_connector *tmp, *gconn;
+
+		tmp = krealloc(connectors, (connector_count + 1) * sizeof(*connectors),
+			       GFP_KERNEL | __GFP_ZERO);
+		if (!tmp) {
+			ret = -ENOMEM;
+			break;
+		}
+
+		connectors = tmp;
+		drm_connector_get(connector);
+		gconn = &connectors[connector_count++];
+		gconn->connector = connector;
+		spin_lock_init(&gconn->lock);
+
+		ret = gud_drm_gadget_get_connector_properties(gdg, gconn);
+		if (ret)
+			break;
+	}
+	drm_connector_list_iter_end(&conn_iter);
+
+	gdg->connectors = connectors;
+	gdg->connector_count = connector_count;
+
+	return ret;
+}
+
+static void gud_drm_gadget_release(struct kref *kref)
+{
+	struct gud_drm_gadget *gdg = container_of(kref, struct gud_drm_gadget, refcount);
+
+	kfree(gdg);
+}
+
+static void gud_drm_gadget_put(struct gud_drm_gadget *gdg)
+{
+	kref_put(&gdg->refcount, gud_drm_gadget_release);
+}
+
+static void gud_drm_gadget_client_unregister(struct drm_client_dev *client)
+{
+	struct gud_drm_gadget *gdg = container_of(client, struct gud_drm_gadget, client);
+	int timeout = 10000 / 50;
+	unsigned int i;
+
+	/*
+	 * If usecnt doesn't drop to zero, try waiting for the gadget, but we
+	 * can't block the DRM driver forever. The worst case wait the gadget side
+	 * can hit are tens of seconds through the call to drm_client_modeset_commit().
+	 */
+	if (refcount_dec_and_test(&gdg->usecnt)) {
+		for (; timeout && refcount_read(&gdg->usecnt); timeout--)
+			msleep(50);
+	}
+
+	if (!timeout) {
+		pr_err("%s: Timeout waiting for gadget side, will leak memory\n", __func__);
+		return;
+	}
+
+	vfree(gdg->work_buf);
+	kfree(gdg->formats);
+	kfree(gdg->properties);
+
+	for (i = 0; i < gdg->connector_count; i++) {
+		struct gud_drm_gadget_connector *gconn = &gdg->connectors[i];
+
+		drm_connector_put(gconn->connector);
+		kfree(gconn->properties);
+		kfree(gconn->tv_mode_enum_names);
+		kfree(gconn->modes);
+		kfree(gconn->edid);
+	}
+	kfree(gdg->connectors);
+
+	gud_drm_gadget_delete_buffers(gdg);
+	drm_client_release(client);
+	gud_drm_gadget_put(gdg);
+}
+
+static int gud_drm_gadget_client_hotplug(struct drm_client_dev *client)
+{
+	struct gud_drm_gadget *gdg = container_of(client, struct gud_drm_gadget, client);
+
+	gud_drm_gadget_probe_connectors(gdg);
+
+	return 0;
+}
+
+static const struct drm_client_funcs gdg_client_funcs = {
+	.owner		= THIS_MODULE,
+	.unregister	= gud_drm_gadget_client_unregister,
+	.hotplug	= gud_drm_gadget_client_hotplug,
+};
+
+struct gud_drm_gadget *gud_drm_gadget_init(unsigned int minor_id, const char *backlight,
+					   size_t *max_buffer_size)
+{
+	struct gud_drm_gadget *gdg;
+	u8 max_cpp;
+	int ret;
+
+	gdg = kzalloc(sizeof(*gdg), GFP_KERNEL);
+	if (!gdg)
+		return ERR_PTR(-ENOMEM);
+
+	ret = drm_client_init_from_id(minor_id, &gdg->client, "gud-drm-gadget", &gdg_client_funcs);
+	if (ret) {
+		pr_err("Failed to aquire minor=%u\n", minor_id);
+		kfree(gdg);
+		return ERR_PTR(ret);
+	}
+
+	refcount_set(&gdg->usecnt, 1);
+	/* The DRM driver (through the client) and f_gud_drm need one ref each */
+	kref_init(&gdg->refcount);
+	kref_get(&gdg->refcount);
+
+	if (backlight) {
+		gdg->backlight = backlight_device_get_by_name(backlight);
+		if (!gdg->backlight) {
+			pr_err("Failed to find backlight: %s\n", backlight);
+			ret = -ENODEV;
+			goto error_release;
+		}
+	}
+
+	ret = gud_drm_gadget_get_formats(gdg, &max_cpp);
+	if (ret) {
+		pr_err("Failed to get formats\n");
+		goto error_release;
+	}
+
+	*max_buffer_size = gdg->client.dev->mode_config.max_width *
+			   gdg->client.dev->mode_config.max_height * max_cpp;
+	/* f_gud_drm will kmalloc a buffer of this size */
+	*max_buffer_size = min_t(size_t, *max_buffer_size, KMALLOC_MAX_SIZE);
+
+	gdg->max_buffer_size = *max_buffer_size;
+	gdg->work_buf = vmalloc(gdg->max_buffer_size);
+	if (!gdg->work_buf) {
+		ret = -ENOMEM;
+		goto error_release;
+	}
+
+	ret = gud_drm_gadget_get_properties(gdg);
+	if (ret) {
+		pr_err("Failed to get properties\n");
+		goto error_release;
+	}
+
+	ret = gud_drm_gadget_get_connectors(gdg);
+	if (ret) {
+		pr_err("Failed to get connectors\n");
+		goto error_release;
+	}
+
+	if (!drm_client_register(&gdg->client)) {
+		pr_err("DRM device is gone\n");
+		ret = -ENODEV;
+		goto error_release;
+	}
+
+	gud_drm_gadget_probe_connectors(gdg);
+
+	return gdg;
+
+error_release:
+	gud_drm_gadget_client_unregister(&gdg->client);
+	gud_drm_gadget_fini(gdg);
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL(gud_drm_gadget_init);
+
+void gud_drm_gadget_fini(struct gud_drm_gadget *gdg)
+{
+	backlight_put(gdg->backlight);
+	gud_drm_gadget_put(gdg);
+}
+EXPORT_SYMBOL(gud_drm_gadget_fini);
+
+MODULE_AUTHOR("Noralf Trønnes");
+MODULE_LICENSE("GPL");
diff --git a/include/drm/gud_drm.h b/include/drm/gud_drm.h
index 7876d2e3cbcf..df8e16c2de4b 100644
--- a/include/drm/gud_drm.h
+++ b/include/drm/gud_drm.h
@@ -358,4 +358,17 @@ static inline void gud_drm_to_display_mode(struct drm_display_mode *dst,
 	drm_mode_set_name(dst);
 }
 
+struct gud_drm_gadget;
+
+struct gud_drm_gadget *gud_drm_gadget_init(unsigned int minor_id, const char *backlight,
+					   size_t *max_buffer_size);
+void gud_drm_gadget_fini(struct gud_drm_gadget *gdg);
+int gud_drm_gadget_disable_pipe(struct gud_drm_gadget *gdg);
+int gud_drm_gadget_ctrl_get(struct gud_drm_gadget *gdg, u8 request,
+			    u16 index, void *data, size_t size);
+int gud_drm_gadget_ctrl_set(struct gud_drm_gadget *gdg, u8 request,
+			    u16 index, void *data, size_t size);
+int gud_drm_gadget_set_buffer(struct gud_drm_gadget *gdg, struct gud_drm_req_set_buffer *req);
+int gud_drm_gadget_write_buffer(struct gud_drm_gadget *gdg, const void *buf, size_t len);
+
 #endif
-- 
2.23.0

[PATCH v3 6/6] usb: gadget: function: Add Generic USB Display support

[Noralf Trønnes]

This adds the gadget side support for the Generic USB Display. It presents
a DRM display device as a USB Display configured through configfs.

The display is implemented as a vendor type USB interface with one bulk
out endpoint. The protocol is implemented using control requests.
lz4 compressed framebuffer data/pixels are sent over the bulk endpoint.

The DRM part of the gadget is placed in the DRM subsystem since it reaches
into the DRM internals.

Cc: Felipe Balbi <balbi@kernel.org>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 .../ABI/testing/configfs-usb-gadget-gud_drm   |  10 +
 MAINTAINERS                                   |   2 +
 drivers/usb/gadget/Kconfig                    |  12 +
 drivers/usb/gadget/function/Makefile          |   2 +
 drivers/usb/gadget/function/f_gud_drm.c       | 678 ++++++++++++++++++
 5 files changed, 704 insertions(+)
 create mode 100644 Documentation/ABI/testing/configfs-usb-gadget-gud_drm
 create mode 100644 drivers/usb/gadget/function/f_gud_drm.c

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-gud_drm b/Documentation/ABI/testing/configfs-usb-gadget-gud_drm
new file mode 100644
index 000000000000..2ac2c12b33aa
--- /dev/null
+++ b/Documentation/ABI/testing/configfs-usb-gadget-gud_drm
@@ -0,0 +1,10 @@
+What:		/config/usb-gadget/gadget/functions/gud_drm.name
+Date:		Dec 2020
+KernelVersion:	5.10
+Description:
+		The attributes:
+
+		drm_dev - DRM device number
+		backlight_dev - Backlight device name (optional)
+				The backlight brightness scale should be
+				perceptual not linear.
diff --git a/MAINTAINERS b/MAINTAINERS
index b15bf9b2229b..fe3382e07316 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5296,7 +5296,9 @@ 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:	Documentation/ABI/testing/configfs-usb-gadget-gud_drm
 F:	drivers/gpu/drm/gud/
+F:	drivers/usb/gadget/function/f_gud_drm.c
 F:	include/drm/gud_drm.h
 
 DRM DRIVER FOR GRAIN MEDIA GM12U320 PROJECTORS
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index c6db0a0a340c..8d90add495b6 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -216,6 +216,9 @@ config USB_F_PRINTER
 config USB_F_TCM
 	tristate
 
+config USB_F_GUD_DRM
+	tristate
+
 # this first set of drivers all depend on bulk-capable hardware.
 
 config USB_CONFIGFS
@@ -483,6 +486,15 @@ config USB_CONFIGFS_F_TCM
 	  Both protocols can work on USB2.0 and USB3.0.
 	  UAS utilizes the USB 3.0 feature called streams support.
 
+config USB_CONFIGFS_F_GUD_DRM
+	bool "Generic USB Display Gadget function"
+	depends on USB_CONFIGFS
+	depends on DRM
+	select DRM_GUD_GADGET
+	select USB_F_GUD_DRM
+	help
+	  This presents a DRM display device as a Generic USB Display.
+
 source "drivers/usb/gadget/legacy/Kconfig"
 
 endif # USB_GADGET
diff --git a/drivers/usb/gadget/function/Makefile b/drivers/usb/gadget/function/Makefile
index 5d3a6cf02218..cd71caa2a34c 100644
--- a/drivers/usb/gadget/function/Makefile
+++ b/drivers/usb/gadget/function/Makefile
@@ -50,3 +50,5 @@ usb_f_printer-y			:= f_printer.o
 obj-$(CONFIG_USB_F_PRINTER)	+= usb_f_printer.o
 usb_f_tcm-y			:= f_tcm.o
 obj-$(CONFIG_USB_F_TCM)		+= usb_f_tcm.o
+usb_f_gud_drm-y			:= f_gud_drm.o
+obj-$(CONFIG_USB_F_GUD_DRM)	+= usb_f_gud_drm.o
diff --git a/drivers/usb/gadget/function/f_gud_drm.c b/drivers/usb/gadget/function/f_gud_drm.c
new file mode 100644
index 000000000000..9a2d6bb9739f
--- /dev/null
+++ b/drivers/usb/gadget/function/f_gud_drm.c
@@ -0,0 +1,678 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Noralf Trønnes
+ */
+
+#include <linux/configfs.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/usb/composite.h>
+#include <linux/usb/gadget.h>
+#include <linux/workqueue.h>
+
+#include <drm/gud_drm.h>
+
+struct f_gud_drm {
+	struct usb_function func;
+	struct work_struct worker;
+	size_t max_buffer_size;
+	void *ctrl_req_buf;
+
+	u8 interface_id;
+	struct usb_request *ctrl_req;
+
+	struct usb_ep *bulk_ep;
+	struct usb_request *bulk_req;
+
+	struct gud_drm_gadget *gdg;
+
+	spinlock_t lock; /* Protects the following members: */
+	bool ctrl_pending;
+	bool status_pending;
+	bool bulk_pending;
+	bool disable_pending;
+	u8 errno;
+	u16 request;
+	u16 value;
+};
+
+static inline struct f_gud_drm *func_to_f_gud_drm(struct usb_function *f)
+{
+	return container_of(f, struct f_gud_drm, func);
+}
+
+struct f_gud_drm_opts {
+	struct usb_function_instance func_inst;
+	struct mutex lock;
+	int refcnt;
+
+	unsigned int drm_dev;
+	const char *backlight_dev;
+};
+
+static inline struct f_gud_drm_opts *fi_to_f_gud_drm_opts(const struct usb_function_instance *fi)
+{
+	return container_of(fi, struct f_gud_drm_opts, func_inst);
+}
+
+static inline struct f_gud_drm_opts *ci_to_f_gud_drm_opts(struct config_item *item)
+{
+	return container_of(to_config_group(item), struct f_gud_drm_opts,
+			    func_inst.group);
+}
+
+#define F_MUD_DEFINE_BULK_ENDPOINT_DESCRIPTOR(name, addr, size)	\
+	static struct usb_endpoint_descriptor name = {		\
+		.bLength =		USB_DT_ENDPOINT_SIZE,	\
+		.bDescriptorType =	USB_DT_ENDPOINT,	\
+		.bEndpointAddress =	addr,			\
+		.bmAttributes =		USB_ENDPOINT_XFER_BULK,	\
+		.wMaxPacketSize =	cpu_to_le16(size),	\
+	}
+
+static struct usb_interface_descriptor f_gud_drm_intf = {
+	.bLength =		USB_DT_INTERFACE_SIZE,
+	.bDescriptorType =	USB_DT_INTERFACE,
+	.bNumEndpoints =	1,
+	.bInterfaceClass =	USB_CLASS_VENDOR_SPEC,
+};
+
+F_MUD_DEFINE_BULK_ENDPOINT_DESCRIPTOR(f_gud_drm_fs_out_desc, USB_DIR_OUT, 0);
+
+static struct usb_descriptor_header *f_gud_drm_fs_function[] = {
+	(struct usb_descriptor_header *)&f_gud_drm_intf,
+	(struct usb_descriptor_header *)&f_gud_drm_fs_out_desc,
+	NULL,
+};
+
+F_MUD_DEFINE_BULK_ENDPOINT_DESCRIPTOR(f_gud_drm_hs_out_desc, USB_DIR_OUT, 512);
+
+static struct usb_descriptor_header *f_gud_drm_hs_function[] = {
+	(struct usb_descriptor_header *)&f_gud_drm_intf,
+	(struct usb_descriptor_header *)&f_gud_drm_hs_out_desc,
+	NULL,
+};
+
+F_MUD_DEFINE_BULK_ENDPOINT_DESCRIPTOR(f_gud_drm_ss_out_desc, USB_DIR_OUT, 1024);
+
+static struct usb_ss_ep_comp_descriptor f_gud_drm_ss_bulk_comp_desc = {
+	.bLength =		USB_DT_SS_EP_COMP_SIZE,
+	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
+};
+
+static struct usb_descriptor_header *f_gud_drm_ss_function[] = {
+	(struct usb_descriptor_header *)&f_gud_drm_intf,
+	(struct usb_descriptor_header *)&f_gud_drm_ss_out_desc,
+	(struct usb_descriptor_header *)&f_gud_drm_ss_bulk_comp_desc,
+	NULL,
+};
+
+static struct usb_string f_gud_drm_string_defs[] = {
+	[0].s = "Generic USB Display",
+	{  } /* end of list */
+};
+
+static struct usb_gadget_strings f_gud_drm_string_table = {
+	.language =	0x0409,	/* en-us */
+	.strings =	f_gud_drm_string_defs,
+};
+
+static struct usb_gadget_strings *f_gud_drm_strings[] = {
+	&f_gud_drm_string_table,
+	NULL,
+};
+
+static void f_gud_drm_bulk_complete(struct usb_ep *ep, struct usb_request *req)
+{
+	struct f_gud_drm *fgd = req->context;
+	unsigned long flags;
+
+	if (req->status || req->actual != req->length)
+		return;
+
+	spin_lock_irqsave(&fgd->lock, flags);
+	fgd->bulk_pending = true;
+	spin_unlock_irqrestore(&fgd->lock, flags);
+
+	queue_work(system_long_wq, &fgd->worker);
+}
+
+static int f_gud_drm_ctrl_req_set_buffer(struct f_gud_drm *fgd, void *buf, size_t len)
+{
+	int ret;
+
+	if (len != sizeof(struct gud_drm_req_set_buffer))
+		return -EINVAL;
+
+	ret = gud_drm_gadget_set_buffer(fgd->gdg, buf);
+	if (ret < 0)
+		return ret;
+
+	if (ret > fgd->max_buffer_size)
+		return -EOVERFLOW;
+
+	fgd->bulk_req->length = ret;
+
+	return usb_ep_queue(fgd->bulk_ep, fgd->bulk_req, GFP_KERNEL);
+}
+
+static void f_gud_drm_worker(struct work_struct *work)
+{
+	struct f_gud_drm *fgd = container_of(work, struct f_gud_drm, worker);
+	bool ctrl_pending, bulk_pending, disable_pending;
+	struct gud_drm_gadget *gdg = fgd->gdg;
+	unsigned long flags;
+	u16 request, value;
+	int ret;
+
+	spin_lock_irqsave(&fgd->lock, flags);
+	request = fgd->request;
+	value = fgd->value;
+	ctrl_pending = fgd->ctrl_pending;
+	bulk_pending = fgd->bulk_pending;
+	disable_pending = fgd->disable_pending;
+	spin_unlock_irqrestore(&fgd->lock, flags);
+
+	pr_debug("%s: bulk_pending=%u ctrl_pending=%u disable_pending=%u\n",
+		 __func__, bulk_pending, ctrl_pending, disable_pending);
+
+	if (disable_pending) {
+		gud_drm_gadget_disable_pipe(gdg);
+
+		spin_lock_irqsave(&fgd->lock, flags);
+		fgd->disable_pending = false;
+		spin_unlock_irqrestore(&fgd->lock, flags);
+		return;
+	}
+
+	if (bulk_pending) {
+		struct usb_request *req = fgd->bulk_req;
+
+		ret = gud_drm_gadget_write_buffer(gdg, req->buf, req->actual);
+		if (ret)
+			pr_err("%s: Failed to write buffer, error=%d\n", __func__, ret);
+
+		spin_lock_irqsave(&fgd->lock, flags);
+		fgd->bulk_pending = false;
+		spin_unlock_irqrestore(&fgd->lock, flags);
+	}
+
+	if (ctrl_pending) {
+		unsigned int length = fgd->ctrl_req->length;
+		void *buf = fgd->ctrl_req->buf;
+
+		if (request == GUD_DRM_USB_REQ_SET_BUFFER)
+			ret = f_gud_drm_ctrl_req_set_buffer(fgd, buf, length);
+		else
+			ret = gud_drm_gadget_ctrl_set(gdg, request, value, buf, length);
+
+		spin_lock_irqsave(&fgd->lock, flags);
+		if (!fgd->errno) /* Don't scribble over an EBUSY or ESHUTDOWN */
+			fgd->errno = -ret;
+		fgd->ctrl_pending = false;
+		fgd->status_pending = false;
+		spin_unlock_irqrestore(&fgd->lock, flags);
+	}
+}
+
+static void f_gud_drm_ctrl_req_complete(struct usb_ep *ep, struct usb_request *req)
+{
+	struct f_gud_drm *fgd = req->context;
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&fgd->lock, flags);
+
+	if (req->status)
+		ret = req->status;
+	else if (req->actual != req->length)
+		ret = -EREMOTEIO;
+	if (ret) {
+		fgd->errno = -ret;
+		fgd->status_pending = false;
+	} else {
+		fgd->ctrl_pending = true;
+	}
+
+	spin_unlock_irqrestore(&fgd->lock, flags);
+
+	if (!ret)
+		queue_work(system_long_wq, &fgd->worker);
+}
+
+static int f_gud_drm_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
+{
+	struct usb_composite_dev *cdev = f->config->cdev;
+	struct f_gud_drm *fgd = func_to_f_gud_drm(f);
+	bool in = ctrl->bRequestType & USB_DIR_IN;
+	u16 length = le16_to_cpu(ctrl->wLength);
+	u16 value = le16_to_cpu(ctrl->wValue);
+	unsigned long flags;
+	int ret;
+
+	if (ctrl->bRequest == USB_REQ_GET_STATUS) {
+		struct gud_drm_req_get_status *status = cdev->req->buf;
+
+		if (!in || length != sizeof(*status))
+			return -EINVAL;
+
+		spin_lock_irqsave(&fgd->lock, flags);
+		status->flags = 0;
+		if (fgd->status_pending)
+			status->flags |= GUD_DRM_STATUS_PENDING;
+		status->errno = fgd->errno;
+		spin_unlock_irqrestore(&fgd->lock, flags);
+	} else if (in) {
+		if (length > USB_COMP_EP0_BUFSIZ) /* 4k */
+			return -EOVERFLOW;
+
+		ret = gud_drm_gadget_ctrl_get(fgd->gdg, ctrl->bRequest, value,
+					      cdev->req->buf, length);
+		spin_lock_irqsave(&fgd->lock, flags);
+		fgd->status_pending = false;
+		fgd->errno = ret < 0 ? -ret : 0;
+		spin_unlock_irqrestore(&fgd->lock, flags);
+		if (ret < 0)
+			return ret;
+
+		length = ret;
+	} else {
+		if (length > GUD_DRM_MAX_TRANSFER_SIZE)
+			return -EOVERFLOW;
+
+		spin_lock_irqsave(&fgd->lock, flags);
+		if (fgd->ctrl_pending) {
+			/* If we get here the host has timed out on the previous request */
+			ret = -EBUSY;
+			fgd->status_pending = false;
+			fgd->errno = -ret;
+		} else {
+			ret = 0;
+			fgd->errno = 0;
+			fgd->request = ctrl->bRequest;
+			fgd->value = value;
+			fgd->status_pending = true;
+		}
+		spin_unlock_irqrestore(&fgd->lock, flags);
+
+		if (ret)
+			return ret;
+
+		fgd->ctrl_req->length = length;
+
+		return usb_ep_queue(cdev->gadget->ep0, fgd->ctrl_req, GFP_ATOMIC);
+	}
+
+	cdev->req->length = length;
+
+	return usb_ep_queue(cdev->gadget->ep0, cdev->req, GFP_ATOMIC);
+}
+
+static bool f_gud_drm_req_match(struct usb_function *f, const struct usb_ctrlrequest *ctrl,
+				bool config0)
+{
+	struct f_gud_drm *fgd = func_to_f_gud_drm(f);
+
+	if (config0)
+		return false;
+
+	if ((ctrl->bRequestType & USB_TYPE_MASK) != USB_TYPE_VENDOR)
+		return false;
+
+	if ((ctrl->bRequestType & USB_RECIP_MASK) != USB_RECIP_INTERFACE)
+		return false;
+
+	return fgd->interface_id == le16_to_cpu(ctrl->wIndex);
+}
+
+static int f_gud_drm_set_alt(struct usb_function *f, unsigned int intf, unsigned int alt)
+{
+	struct usb_composite_dev *cdev = f->config->cdev;
+	struct f_gud_drm *fgd = func_to_f_gud_drm(f);
+	unsigned long flags;
+
+	if (alt || intf != fgd->interface_id)
+		return -EINVAL;
+
+	if (!fgd->bulk_ep->desc) {
+		pr_debug("%s: init\n", __func__);
+		if (config_ep_by_speed(cdev->gadget, f, fgd->bulk_ep)) {
+			fgd->bulk_ep->desc = NULL;
+			return -EINVAL;
+		}
+	}
+
+	pr_debug("%s: reset\n", __func__);
+
+	usb_ep_disable(fgd->bulk_ep);
+	usb_ep_enable(fgd->bulk_ep);
+
+	spin_lock_irqsave(&fgd->lock, flags);
+	fgd->ctrl_pending = false;
+	fgd->bulk_pending = false;
+	fgd->disable_pending = false;
+	spin_unlock_irqrestore(&fgd->lock, flags);
+
+	return 0;
+}
+
+static void f_gud_drm_disable(struct usb_function *f)
+{
+	struct f_gud_drm *fgd = func_to_f_gud_drm(f);
+	unsigned long flags;
+
+	pr_debug("%s\n", __func__);
+
+	usb_ep_disable(fgd->bulk_ep);
+
+	spin_lock_irqsave(&fgd->lock, flags);
+	fgd->ctrl_pending = false;
+	fgd->bulk_pending = false;
+	fgd->status_pending = false;
+	fgd->disable_pending = true;
+	fgd->errno = ESHUTDOWN;
+	spin_unlock_irqrestore(&fgd->lock, flags);
+
+	queue_work(system_long_wq, &fgd->worker);
+}
+
+static void f_gud_drm_unbind(struct usb_configuration *c, struct usb_function *f)
+{
+	struct f_gud_drm *fgd = func_to_f_gud_drm(f);
+	struct usb_composite_dev *cdev = fgd->func.config->cdev;
+
+	flush_work(&fgd->worker);
+
+	gud_drm_gadget_fini(fgd->gdg);
+	fgd->gdg = NULL;
+
+	kfree(fgd->bulk_req->buf);
+	usb_ep_free_request(fgd->bulk_ep, fgd->bulk_req);
+	usb_ep_free_request(cdev->gadget->ep0, fgd->ctrl_req);
+	fgd->ctrl_req = NULL;
+	fgd->bulk_req = NULL;
+	fgd->bulk_ep = NULL;
+
+	usb_free_all_descriptors(f);
+}
+
+static int f_gud_drm_bind(struct usb_configuration *c, struct usb_function *f)
+{
+	struct f_gud_drm_opts *opts = fi_to_f_gud_drm_opts(f->fi);
+	struct usb_composite_dev *cdev = c->cdev;
+	struct f_gud_drm *fgd = func_to_f_gud_drm(f);
+	struct usb_request *ctrl_req, *bulk_req;
+	struct gud_drm_gadget *gdg;
+	struct usb_string *us;
+	void *buf;
+	int ret;
+
+	us = usb_gstrings_attach(cdev, f_gud_drm_strings,
+				 ARRAY_SIZE(f_gud_drm_string_defs));
+	if (IS_ERR(us))
+		return PTR_ERR(us);
+
+	f_gud_drm_intf.iInterface = us[0].id;
+
+	ret = usb_interface_id(c, f);
+	if (ret < 0)
+		return ret;
+
+	fgd->interface_id = ret;
+	f_gud_drm_intf.bInterfaceNumber = fgd->interface_id;
+
+	fgd->bulk_ep = usb_ep_autoconfig(cdev->gadget, &f_gud_drm_fs_out_desc);
+	if (!fgd->bulk_ep)
+		return -ENODEV;
+
+	f_gud_drm_hs_out_desc.bEndpointAddress = f_gud_drm_fs_out_desc.bEndpointAddress;
+
+	f_gud_drm_ss_out_desc.bEndpointAddress = f_gud_drm_fs_out_desc.bEndpointAddress;
+
+	ret = usb_assign_descriptors(f, f_gud_drm_fs_function, f_gud_drm_hs_function,
+				     f_gud_drm_ss_function, NULL);
+	if (ret)
+		return ret;
+
+	ctrl_req = usb_ep_alloc_request(cdev->gadget->ep0, GFP_KERNEL);
+	if (!ctrl_req) {
+		ret = -ENOMEM;
+		goto fail_free_descs;
+	}
+
+	ctrl_req->buf = fgd->ctrl_req_buf;
+	ctrl_req->complete = f_gud_drm_ctrl_req_complete;
+	ctrl_req->context = fgd;
+
+	gdg = gud_drm_gadget_init(opts->drm_dev, opts->backlight_dev, &fgd->max_buffer_size);
+	if (IS_ERR(gdg)) {
+		ret = PTR_ERR(gdg);
+		goto fail_free_ctrl_req;
+	}
+
+	bulk_req = usb_ep_alloc_request(fgd->bulk_ep, GFP_KERNEL);
+	if (!bulk_req) {
+		ret = -ENOMEM;
+		goto fail_free_ctrl_req;
+	}
+
+	buf = kmalloc(fgd->max_buffer_size, GFP_KERNEL);
+	if (!buf) {
+		ret = -ENOMEM;
+		goto fail_free_bulk_req;
+	}
+
+	bulk_req->complete = f_gud_drm_bulk_complete;
+	bulk_req->context = fgd;
+	bulk_req->buf = buf;
+
+	fgd->ctrl_req = ctrl_req;
+	fgd->bulk_req = bulk_req;
+	fgd->gdg = gdg;
+
+	return 0;
+
+fail_free_bulk_req:
+	usb_ep_free_request(fgd->bulk_ep, bulk_req);
+fail_free_ctrl_req:
+	usb_ep_free_request(cdev->gadget->ep0, ctrl_req);
+fail_free_descs:
+	usb_free_all_descriptors(f);
+
+	return ret;
+}
+
+static void f_gud_drm_free_func(struct usb_function *f)
+{
+	struct f_gud_drm_opts *opts = container_of(f->fi, struct f_gud_drm_opts, func_inst);
+	struct f_gud_drm *fgd = func_to_f_gud_drm(f);
+
+	mutex_lock(&opts->lock);
+	opts->refcnt--;
+	mutex_unlock(&opts->lock);
+
+	kfree(fgd->ctrl_req_buf);
+	kfree(fgd);
+}
+
+static struct usb_function *f_gud_drm_alloc_func(struct usb_function_instance *fi)
+{
+	struct f_gud_drm_opts *opts = fi_to_f_gud_drm_opts(fi);
+	struct usb_function *func;
+	struct f_gud_drm *fgd;
+
+	fgd = kzalloc(sizeof(*fgd), GFP_KERNEL);
+	if (!fgd)
+		return ERR_PTR(-ENOMEM);
+
+	fgd->ctrl_req_buf = kmalloc(GUD_DRM_MAX_TRANSFER_SIZE, GFP_KERNEL);
+	if (!fgd->ctrl_req_buf)
+		goto error;
+
+	spin_lock_init(&fgd->lock);
+	INIT_WORK(&fgd->worker, f_gud_drm_worker);
+
+	mutex_lock(&opts->lock);
+	opts->refcnt++;
+	mutex_unlock(&opts->lock);
+
+	func = &fgd->func;
+	func->name = "gud_drm";
+	func->bind = f_gud_drm_bind;
+	func->unbind = f_gud_drm_unbind;
+	func->set_alt = f_gud_drm_set_alt;
+	func->req_match = f_gud_drm_req_match;
+	func->setup = f_gud_drm_setup;
+	func->disable = f_gud_drm_disable;
+	func->free_func = f_gud_drm_free_func;
+
+	return func;
+
+error:
+	kfree(fgd);
+
+	return ERR_PTR(-ENOMEM);
+}
+
+static ssize_t f_gud_drm_opts_drm_dev_show(struct config_item *item, char *page)
+{
+	struct f_gud_drm_opts *opts = ci_to_f_gud_drm_opts(item);
+	int result;
+
+	mutex_lock(&opts->lock);
+	result = sprintf(page, "%u\n", opts->drm_dev);
+	mutex_unlock(&opts->lock);
+
+	return result;
+}
+
+static ssize_t f_gud_drm_opts_drm_dev_store(struct config_item *item,
+					    const char *page, size_t len)
+{
+	struct f_gud_drm_opts *opts = ci_to_f_gud_drm_opts(item);
+	unsigned int num;
+	int ret;
+
+	mutex_lock(&opts->lock);
+	if (opts->refcnt) {
+		ret = -EBUSY;
+		goto unlock;
+	}
+
+	ret = kstrtouint(page, 0, &num);
+	if (ret)
+		goto unlock;
+
+	opts->drm_dev = num;
+	ret = len;
+unlock:
+	mutex_unlock(&opts->lock);
+
+	return ret;
+}
+
+CONFIGFS_ATTR(f_gud_drm_opts_, drm_dev);
+
+static ssize_t f_gud_drm_opts_backlight_dev_show(struct config_item *item, char *page)
+{
+	struct f_gud_drm_opts *opts = ci_to_f_gud_drm_opts(item);
+	ssize_t ret = 0;
+
+	mutex_lock(&opts->lock);
+	if (opts->backlight_dev)
+		ret = strscpy(page, opts->backlight_dev, PAGE_SIZE);
+	else
+		page[0] = '\0';
+	mutex_unlock(&opts->lock);
+
+	return ret;
+}
+
+static ssize_t f_gud_drm_opts_backlight_dev_store(struct config_item *item,
+						  const char *page, size_t len)
+{
+	struct f_gud_drm_opts *opts = ci_to_f_gud_drm_opts(item);
+	ssize_t ret;
+	char *name;
+
+	mutex_lock(&opts->lock);
+	if (opts->refcnt) {
+		ret = -EBUSY;
+		goto unlock;
+	}
+
+	name = kstrndup(page, len, GFP_KERNEL);
+	if (!name) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	kfree(opts->backlight_dev);
+	opts->backlight_dev = name;
+	ret = len;
+unlock:
+	mutex_unlock(&opts->lock);
+
+	return ret;
+}
+
+CONFIGFS_ATTR(f_gud_drm_opts_, backlight_dev);
+
+static struct configfs_attribute *f_gud_drm_attrs[] = {
+	&f_gud_drm_opts_attr_drm_dev,
+	&f_gud_drm_opts_attr_backlight_dev,
+	NULL,
+};
+
+static void f_gud_drm_attr_release(struct config_item *item)
+{
+	struct f_gud_drm_opts *opts = ci_to_f_gud_drm_opts(item);
+
+	usb_put_function_instance(&opts->func_inst);
+}
+
+static struct configfs_item_operations f_gud_drm_item_ops = {
+	.release	= f_gud_drm_attr_release,
+};
+
+static const struct config_item_type f_gud_drm_func_type = {
+	.ct_item_ops	= &f_gud_drm_item_ops,
+	.ct_attrs	= f_gud_drm_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static void f_gud_drm_free_func_inst(struct usb_function_instance *fi)
+{
+	struct f_gud_drm_opts *opts = fi_to_f_gud_drm_opts(fi);
+
+	mutex_destroy(&opts->lock);
+	kfree(opts->backlight_dev);
+	kfree(opts);
+}
+
+static struct usb_function_instance *f_gud_drm_alloc_func_inst(void)
+{
+	struct f_gud_drm_opts *opts;
+
+	opts = kzalloc(sizeof(*opts), GFP_KERNEL);
+	if (!opts)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_init(&opts->lock);
+	opts->func_inst.free_func_inst = f_gud_drm_free_func_inst;
+
+	config_group_init_type_name(&opts->func_inst.group, "", &f_gud_drm_func_type);
+
+	return &opts->func_inst;
+}
+
+DECLARE_USB_FUNCTION_INIT(gud_drm, f_gud_drm_alloc_func_inst, f_gud_drm_alloc_func);
+
+MODULE_DESCRIPTION("Generic USB Display Gadget");
+MODULE_AUTHOR("Noralf Trønnes");
+MODULE_LICENSE("GPL");
-- 
2.23.0

Re: [PATCH v3 4/6] drm: Add Generic USB Display driver

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


> 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

Re: [PATCH v3 4/6] drm: Add Generic USB Display driver

[Noralf Trønnes]

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.

Re: [PATCH v3 4/6] drm: Add Generic USB Display driver

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


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

Re: [PATCH v3 4/6] drm: Add Generic USB Display driver

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

Re: [PATCH v3 4/6] drm: Add Generic USB Display driver

[Peter Stuge]

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

Re: [PATCH v3 4/6] drm: Add Generic USB Display driver

[Noralf Trønnes]

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
> 

Re: [PATCH v3 4/6] drm: Add Generic USB Display driver

[Alan Stern]

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

Re: [PATCH v3 6/6] usb: gadget: function: Add Generic USB Display support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4315 bytes --]


Hi,

I missed this completely until now.
Noralf Trønnes <noralf@tronnes.org> writes:
> This adds the gadget side support for the Generic USB Display. It presents
> a DRM display device as a USB Display configured through configfs.
>
> The display is implemented as a vendor type USB interface with one bulk
> out endpoint. The protocol is implemented using control requests.
> lz4 compressed framebuffer data/pixels are sent over the bulk endpoint.
>
> The DRM part of the gadget is placed in the DRM subsystem since it reaches
> into the DRM internals.

First and foremost, could this be done in userspace using the raw gadget
or f_fs?

> diff --git a/drivers/usb/gadget/function/f_gud_drm.c b/drivers/usb/gadget/function/f_gud_drm.c
> new file mode 100644
> index 000000000000..9a2d6bb9739f
> --- /dev/null
> +++ b/drivers/usb/gadget/function/f_gud_drm.c
> @@ -0,0 +1,678 @@
> +struct f_gud_drm {
> +	struct usb_function func;
> +	struct work_struct worker;

why do you need a worker?

> +	size_t max_buffer_size;
> +	void *ctrl_req_buf;
> +
> +	u8 interface_id;
> +	struct usb_request *ctrl_req;
> +
> +	struct usb_ep *bulk_ep;
> +	struct usb_request *bulk_req;

single request? Don't you want to amortize the latency of
queue->complete by using a series of requests?

> +	struct gud_drm_gadget *gdg;
> +
> +	spinlock_t lock; /* Protects the following members: */
> +	bool ctrl_pending;
> +	bool status_pending;
> +	bool bulk_pending;
> +	bool disable_pending;

could this be a single u32 with #define'd flags? That would be atomic,
right?

> +	u8 errno;

a global per-function error? Why?

> +	u16 request;
> +	u16 value;

also why? Looks odd

> +};
> +
> +static inline struct f_gud_drm *func_to_f_gud_drm(struct usb_function *f)

let the compiler inline for you

> +{
> +	return container_of(f, struct f_gud_drm, func);

could be a macro, but okay.

> +static inline struct f_gud_drm_opts *fi_to_f_gud_drm_opts(const struct usb_function_instance *fi)

ditto

> +{
> +	return container_of(fi, struct f_gud_drm_opts, func_inst);

ditto

> +}
> +
> +static inline struct f_gud_drm_opts *ci_to_f_gud_drm_opts(struct config_item *item)

ditto

> +{
> +	return container_of(to_config_group(item), struct f_gud_drm_opts,
> +			    func_inst.group);

ditto

> +}
> +
> +#define F_MUD_DEFINE_BULK_ENDPOINT_DESCRIPTOR(name, addr, size)	\
> +	static struct usb_endpoint_descriptor name = {		\

const? Also, please check all the others

> +static void f_gud_drm_bulk_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct f_gud_drm *fgd = req->context;
> +	unsigned long flags;
> +
> +	if (req->status || req->actual != req->length)
> +		return;

so, if we complete with an erroneous status or a short packet, you'll
ignore it?

> +	spin_lock_irqsave(&fgd->lock, flags);
> +	fgd->bulk_pending = true;
> +	spin_unlock_irqrestore(&fgd->lock, flags);
> +
> +	queue_work(system_long_wq, &fgd->worker);

Do you need to offset this to a worker?

> +static int f_gud_drm_ctrl_req_set_buffer(struct f_gud_drm *fgd, void *buf, size_t len)
> +{
> +	int ret;
> +
> +	if (len != sizeof(struct gud_drm_req_set_buffer))
> +		return -EINVAL;
> +
> +	ret = gud_drm_gadget_set_buffer(fgd->gdg, buf);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret > fgd->max_buffer_size)
> +		return -EOVERFLOW;
> +
> +	fgd->bulk_req->length = ret;
> +
> +	return usb_ep_queue(fgd->bulk_ep, fgd->bulk_req, GFP_KERNEL);
> +}
> +
> +static void f_gud_drm_worker(struct work_struct *work)
> +{
> +	struct f_gud_drm *fgd = container_of(work, struct f_gud_drm, worker);
> +	bool ctrl_pending, bulk_pending, disable_pending;
> +	struct gud_drm_gadget *gdg = fgd->gdg;
> +	unsigned long flags;
> +	u16 request, value;
> +	int ret;
> +
> +	spin_lock_irqsave(&fgd->lock, flags);
> +	request = fgd->request;
> +	value = fgd->value;
> +	ctrl_pending = fgd->ctrl_pending;
> +	bulk_pending = fgd->bulk_pending;
> +	disable_pending = fgd->disable_pending;
> +	spin_unlock_irqrestore(&fgd->lock, flags);
> +
> +	pr_debug("%s: bulk_pending=%u ctrl_pending=%u disable_pending=%u\n",
> +		 __func__, bulk_pending, ctrl_pending, disable_pending);

could we use dev_dbg() at least?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v3 4/6] drm: Add Generic USB Display driver

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


//Peter

Re: [PATCH v3 6/6] usb: gadget: function: Add Generic USB Display support

[Noralf Trønnes]

Hi,

Den 02.06.2020 19.05, skrev Felipe Balbi:
> 
> Hi,
> 
> I missed this completely until now.
> Noralf Trønnes <noralf@tronnes.org> writes:
>> This adds the gadget side support for the Generic USB Display. It presents
>> a DRM display device as a USB Display configured through configfs.
>>
>> The display is implemented as a vendor type USB interface with one bulk
>> out endpoint. The protocol is implemented using control requests.
>> lz4 compressed framebuffer data/pixels are sent over the bulk endpoint.
>>
>> The DRM part of the gadget is placed in the DRM subsystem since it reaches
>> into the DRM internals.
> 
> First and foremost, could this be done in userspace using the raw gadget
> or f_fs?
> 

An uncompressed 1920x1080-RGB565 frame is ~4MB. All frames can be
compressed (lz4) even if just a little, so this is decompressed into the
framebuffer of the attached DRM device. AFAIU I would need to be able to
mmap the received bulk buffer if I were to do this from userspace
without killing performance. So it doesn't look like I can use raw
gadget or f_fs.

>> diff --git a/drivers/usb/gadget/function/f_gud_drm.c b/drivers/usb/gadget/function/f_gud_drm.c
>> new file mode 100644
>> index 000000000000..9a2d6bb9739f
>> --- /dev/null
>> +++ b/drivers/usb/gadget/function/f_gud_drm.c
>> @@ -0,0 +1,678 @@
>> +struct f_gud_drm {
>> +	struct usb_function func;
>> +	struct work_struct worker;
> 
> why do you need a worker?
> 

The gadget runs in interrupt context and I need to call into the DRM
subsystem which can sleep.

>> +	size_t max_buffer_size;
>> +	void *ctrl_req_buf;
>> +
>> +	u8 interface_id;
>> +	struct usb_request *ctrl_req;
>> +
>> +	struct usb_ep *bulk_ep;
>> +	struct usb_request *bulk_req;
> 
> single request? Don't you want to amortize the latency of
> queue->complete by using a series of requests?
> 

I use only one request per update or partial update.
I kmalloc the biggest buffer I can get (default 4MB) and tell the host
about this size. If a frame doesn't fit, the host splits it up into
partial updates. I already support partial updates so this is built in.
Userspace can tell the graphics driver which portion of the framebuffer
it has touched to avoid sending the full frame each time.
Having one continous buffer simplifies decompression.

There's a control request preceding the bulk request which tells the
area the update is for and whether it is compressed or not.
I did some testing to see if I should avoid the control request overhead
for split updates, but it turns out that the bottleneck is the
decompression. The control request is just 400-500us, while the total
time from control request to buffer is decompressed is 50-100ms
(depending on how well the frame compresses).

>> +	struct gud_drm_gadget *gdg;
>> +
>> +	spinlock_t lock; /* Protects the following members: */
>> +	bool ctrl_pending;
>> +	bool status_pending;
>> +	bool bulk_pending;
>> +	bool disable_pending;
> 
> could this be a single u32 with #define'd flags? That would be atomic,
> right?
> 

I have never grasped all the concurrency issues, but wouldn't I need
memory barriers as well?

>> +	u8 errno;
> 
> a global per-function error? Why?
> 

This is the result of the previously request operation. The host will
request this value to see how it went. I might switch to using a bulk
endpoint for status following a discussion with Alan Stern in the host
driver thread in this patch series. If so I might not need this.

>> +	u16 request;
>> +	u16 value;
> 
> also why? Looks odd
> 

These values contains the operation (in addition to the payload) that
the worker shall perform following the control-OUT requests.

control-IN requests can run in interrupt context so in that case the
payload is queued up immediately.

<snip>

>> +static void f_gud_drm_bulk_complete(struct usb_ep *ep, struct usb_request *req)
>> +{
>> +	struct f_gud_drm *fgd = req->context;
>> +	unsigned long flags;
>> +
>> +	if (req->status || req->actual != req->length)
>> +		return;
> 
> so, if we complete with an erroneous status or a short packet, you'll
> ignore it?
> 

Hmm yeah. When I wrote this I thought that the bottleneck was the USB
transfers, so I didn't want the host to slow down performance by
requesting this status. Now I know it's the decompression that takes
time, so I could actually do a status check and retry the frame if the
device detects an error.

>> +	spin_lock_irqsave(&fgd->lock, flags);
>> +	fgd->bulk_pending = true;
>> +	spin_unlock_irqrestore(&fgd->lock, flags);
>> +
>> +	queue_work(system_long_wq, &fgd->worker);
> 
> Do you need to offset this to a worker?
> 

Yes, long running (one operation can be >100ms) and can sleep.

>> +static int f_gud_drm_ctrl_req_set_buffer(struct f_gud_drm *fgd, void *buf, size_t len)
>> +{
>> +	int ret;
>> +
>> +	if (len != sizeof(struct gud_drm_req_set_buffer))
>> +		return -EINVAL;
>> +
>> +	ret = gud_drm_gadget_set_buffer(fgd->gdg, buf);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (ret > fgd->max_buffer_size)
>> +		return -EOVERFLOW;
>> +
>> +	fgd->bulk_req->length = ret;
>> +
>> +	return usb_ep_queue(fgd->bulk_ep, fgd->bulk_req, GFP_KERNEL);
>> +}
>> +
>> +static void f_gud_drm_worker(struct work_struct *work)
>> +{
>> +	struct f_gud_drm *fgd = container_of(work, struct f_gud_drm, worker);
>> +	bool ctrl_pending, bulk_pending, disable_pending;
>> +	struct gud_drm_gadget *gdg = fgd->gdg;
>> +	unsigned long flags;
>> +	u16 request, value;
>> +	int ret;
>> +
>> +	spin_lock_irqsave(&fgd->lock, flags);
>> +	request = fgd->request;
>> +	value = fgd->value;
>> +	ctrl_pending = fgd->ctrl_pending;
>> +	bulk_pending = fgd->bulk_pending;
>> +	disable_pending = fgd->disable_pending;
>> +	spin_unlock_irqrestore(&fgd->lock, flags);
>> +
>> +	pr_debug("%s: bulk_pending=%u ctrl_pending=%u disable_pending=%u\n",
>> +		 __func__, bulk_pending, ctrl_pending, disable_pending);
> 
> could we use dev_dbg() at least?
> 

Sure.

Noralf.

Re: [PATCH v3 6/6] usb: gadget: function: Add Generic USB Display support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 5359 bytes --]


Hi,

Noralf Trønnes <noralf@tronnes.org> writes:
>> I missed this completely until now.
>> Noralf Trønnes <noralf@tronnes.org> writes:
>>> This adds the gadget side support for the Generic USB Display. It presents
>>> a DRM display device as a USB Display configured through configfs.
>>>
>>> The display is implemented as a vendor type USB interface with one bulk
>>> out endpoint. The protocol is implemented using control requests.
>>> lz4 compressed framebuffer data/pixels are sent over the bulk endpoint.
>>>
>>> The DRM part of the gadget is placed in the DRM subsystem since it reaches
>>> into the DRM internals.
>> 
>> First and foremost, could this be done in userspace using the raw gadget
>> or f_fs?
>> 
>
> An uncompressed 1920x1080-RGB565 frame is ~4MB. All frames can be
> compressed (lz4) even if just a little, so this is decompressed into the
> framebuffer of the attached DRM device. AFAIU I would need to be able to
> mmap the received bulk buffer if I were to do this from userspace
> without killing performance. So it doesn't look like I can use raw
> gadget or f_fs.

oh, yeah we couldn't map that much. I was thinking that maybe we could
transfer several small buffers instead of a single large one, but
perhaps that would complicate decompression?

>>> diff --git a/drivers/usb/gadget/function/f_gud_drm.c b/drivers/usb/gadget/function/f_gud_drm.c
>>> new file mode 100644
>>> index 000000000000..9a2d6bb9739f
>>> --- /dev/null
>>> +++ b/drivers/usb/gadget/function/f_gud_drm.c
>>> @@ -0,0 +1,678 @@
>>> +struct f_gud_drm {
>>> +	struct usb_function func;
>>> +	struct work_struct worker;
>> 
>> why do you need a worker?
>> 
>
> The gadget runs in interrupt context and I need to call into the DRM
> subsystem which can sleep.

At some point someone wanted to provide a patch to run endpoint giveback
routine in process context, much like usb host stack does if
requested. That's currently not implemented, but should be doable by
modifying usb_gadget_giveback_request().

>>> +	size_t max_buffer_size;
>>> +	void *ctrl_req_buf;
>>> +
>>> +	u8 interface_id;
>>> +	struct usb_request *ctrl_req;
>>> +
>>> +	struct usb_ep *bulk_ep;
>>> +	struct usb_request *bulk_req;
>> 
>> single request? Don't you want to amortize the latency of
>> queue->complete by using a series of requests?
>> 
>
> I use only one request per update or partial update.
> I kmalloc the biggest buffer I can get (default 4MB) and tell the host
> about this size. If a frame doesn't fit, the host splits it up into
> partial updates. I already support partial updates so this is built in.
> Userspace can tell the graphics driver which portion of the framebuffer
> it has touched to avoid sending the full frame each time.
> Having one continous buffer simplifies decompression.

got it

> There's a control request preceding the bulk request which tells the
> area the update is for and whether it is compressed or not.
> I did some testing to see if I should avoid the control request overhead
> for split updates, but it turns out that the bottleneck is the
> decompression. The control request is just 400-500us, while the total
> time from control request to buffer is decompressed is 50-100ms
> (depending on how well the frame compresses).

yeah, that makes sense.

>>> +	struct gud_drm_gadget *gdg;
>>> +
>>> +	spinlock_t lock; /* Protects the following members: */
>>> +	bool ctrl_pending;
>>> +	bool status_pending;
>>> +	bool bulk_pending;
>>> +	bool disable_pending;
>> 
>> could this be a single u32 with #define'd flags? That would be atomic,
>> right?
>> 
>
> I have never grasped all the concurrency issues, but wouldn't I need
> memory barriers as well?

As far as I understand, {test_and_,}{set,clear,change}_bit() handle all
the required steps to guarantee proper atomic behavior. I haven't looked
at the implementation myself, though.

>>> +	u8 errno;
>> 
>> a global per-function error? Why?
>> 
>
> This is the result of the previously request operation. The host will
> request this value to see how it went. I might switch to using a bulk
> endpoint for status following a discussion with Alan Stern in the host
> driver thread in this patch series. If so I might not need this.

got it.

>>> +	u16 request;
>>> +	u16 value;
>> 
>> also why? Looks odd
>> 
>
> These values contains the operation (in addition to the payload) that
> the worker shall perform following the control-OUT requests.
>
> control-IN requests can run in interrupt context so in that case the
> payload is queued up immediately.

cool

>>> +static void f_gud_drm_bulk_complete(struct usb_ep *ep, struct usb_request *req)
>>> +{
>>> +	struct f_gud_drm *fgd = req->context;
>>> +	unsigned long flags;
>>> +
>>> +	if (req->status || req->actual != req->length)
>>> +		return;
>> 
>> so, if we complete with an erroneous status or a short packet, you'll
>> ignore it?
>> 
>
> Hmm yeah. When I wrote this I thought that the bottleneck was the USB
> transfers, so I didn't want the host to slow down performance by
> requesting this status. Now I know it's the decompression that takes
> time, so I could actually do a status check and retry the frame if the
> device detects an error.

sounds good :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v3 4/6] drm: Add Generic USB Display driver

[Noralf Trønnes]

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
> 

Re: [PATCH v3 4/6] drm: Add Generic USB Display driver

[Noralf Trønnes]

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.

Re: [PATCH v3 0/6] Generic USB Display driver

[Lubomir Rintel]

Hello,

On 29 May 2020 Noralf Trønnes wrote:
...
> This series adds a USB host driver and a device/gadget driver to achieve
> that.
> 
> The reason for calling it 'Generic' is so anyone can make a USB
> display/adapter against this driver, all that's needed is to add a USB
> vid:pid. I have done a microcontroller implementation hack just to see
> how that would work out[1] (which unconvered a couple of bugs in the
> host driver).
...

This is actually very cool; finally a good way to drive the cheapo
SPI/I2C displays from computers whose only means of expansion is USB
with a little help from a microcontroller. I've actually had some
success doing just that [1].

[1] https://assets.octodon.social/media_attachments/files/009/983/960/original/64ad8ea46c1b06c5.jpg

I suppose you can add:

Tested-by: Lubomir Rintel <lkundrak@v3.sk>

I've had to jump through some hoops though.

My OLED display is a 128x64 SSD1306 [1] driven from the SPI bus. The frame
buffer SRAM is normally scanned out in stripes of 8 vertical pixels called
"pages". When the display is turned on its side, with "vertical
addressing mode" and "segment remapping" enabled and bytes being sent LSB
first, it appears linear -- it's easy to repaint the whole display from
what is now the top left corner to the bottom right. This is very
convenient for painting pixels as they come, without bufferring them or
doing any conversions (assuming that memory and cpu cycles are at
premium on MCUs).

[1] https://cdn-shop.adafruit.com/datasheets/SSD1306.pdf

There doesn't seem a comfortable way to do partial redraws though. Would
you find it objectionable if the device could indicate that needs full
frames instead of just the damaged areas? Perhaps then the driver
wouldn't even need to bother issuing GUD_DRM_USB_REQ_SET_BUFFER to
displays dumb enough to be incapable of partial redraws and decompression.

My work-in-progress code that works on STM32F103 (e.g. "Blue Pill"
boards), or GD32VF103 (RISC-V "Polos Alef") is at [2]. The partial redraws
that don't start from column zero or are not "page aligned" don't work
correctly for the time being; X11 doesn't seem to care.

[2] https://github.com/hackerspace/libopencm3-gf32v-examples/tree/lr/gd32v/examples/gd32v/f103/polos-alef/usb-display

Thank you!
Lubo

Re: [PATCH v3 0/6] Generic USB Display driver

[Noralf Trønnes]

Den 09.07.2020 18.32, skrev Lubomir Rintel:
> Hello,
> 
> On 29 May 2020 Noralf Trønnes wrote:
> ...
>> This series adds a USB host driver and a device/gadget driver to achieve
>> that.
>>
>> The reason for calling it 'Generic' is so anyone can make a USB
>> display/adapter against this driver, all that's needed is to add a USB
>> vid:pid. I have done a microcontroller implementation hack just to see
>> how that would work out[1] (which unconvered a couple of bugs in the
>> host driver).
> ...
> 
> This is actually very cool; finally a good way to drive the cheapo
> SPI/I2C displays from computers whose only means of expansion is USB
> with a little help from a microcontroller. I've actually had some
> success doing just that [1].

Nice to see a monochrome implementation, I actually planned to remove
the R1 format from v3 since the gadget driver doesn't implement it.
Having unused (and poorly tested) code isn't that great, but I forgot.
It turns out it was a good thing that I forgot that :-)

> 
> [1] https://assets.octodon.social/media_attachments/files/009/983/960/original/64ad8ea46c1b06c5.jpg
> 
> I suppose you can add:
> 
> Tested-by: Lubomir Rintel <lkundrak@v3.sk>
> 
> I've had to jump through some hoops though.
> 
> My OLED display is a 128x64 SSD1306 [1] driven from the SPI bus. The frame
> buffer SRAM is normally scanned out in stripes of 8 vertical pixels called
> "pages". When the display is turned on its side, with "vertical
> addressing mode" and "segment remapping" enabled and bytes being sent LSB
> first, it appears linear -- it's easy to repaint the whole display from
> what is now the top left corner to the bottom right. This is very
> convenient for painting pixels as they come, without bufferring them or
> doing any conversions (assuming that memory and cpu cycles are at
> premium on MCUs).
> 
> [1] https://cdn-shop.adafruit.com/datasheets/SSD1306.pdf
> 
> There doesn't seem a comfortable way to do partial redraws though. Would
> you find it objectionable if the device could indicate that needs full
> frames instead of just the damaged areas? Perhaps then the driver
> wouldn't even need to bother issuing GUD_DRM_USB_REQ_SET_BUFFER to
> displays dumb enough to be incapable of partial redraws and decompression.
> 

Yeah I figured always having full updates might benefit/simplify
monochrome devices, but I'd wait for an actual device needing it before
adding it. Now that you need it, I'll add a flag for it in the next
version.

I would like to keep the SET_BUFFER request since it will serve as a
syncing point between the host and the device. I'm no USB expert but I
assume that a bulk transfer can fail halfway through and result in the
next update starting where the previous one failed and thus writing
beyond the end of the display buffer.

Noralf.

> My work-in-progress code that works on STM32F103 (e.g. "Blue Pill"
> boards), or GD32VF103 (RISC-V "Polos Alef") is at [2]. The partial redraws
> that don't start from column zero or are not "page aligned" don't work
> correctly for the time being; X11 doesn't seem to care.
> 
> [2] https://github.com/hackerspace/libopencm3-gf32v-examples/tree/lr/gd32v/examples/gd32v/f103/polos-alef/usb-display
> 
> Thank you!
> Lubo
> 

Re: [PATCH v3 4/6] drm: Add Generic USB Display driver

[Noralf Trønnes]

Den 02.06.2020 13.46, skrev Noralf Trønnes:
> 
> 
> 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.
> 

I have now tried this on a Beaglebone Black (musb udc driver) and it
works just fine there (it displays an error message on the next
request). So it has to be a dwc2 driver problem. I will try and chase
down this problem when I get the time.

This means I don't need this status request polling in my host driver.

Noralf.

Re: [PATCH v3 0/6] Generic USB Display driver

[Peter Stuge]

Hi Noralf,

Noralf Trønnes wrote:
> I would like to keep the SET_BUFFER request since it will serve as a
> syncing point between the host and the device. I'm no USB expert but I
> assume that a bulk transfer can fail halfway through and result in the
> next update starting where the previous one failed and thus writing
> beyond the end of the display buffer.

Transfers either succeed completely (possibly after many retries),
time out (after zero or more transfered bytes) or fail catastrophically
(e.g. from device disconnect).

In all cases, the driver on the host knows/has available how many bytes
were successfully transfered.


//Peter

Re: [PATCH v3 0/6] Generic USB Display driver

[Noralf Trønnes]

Den 14.07.2020 19.40, skrev Peter Stuge:
> Hi Noralf,
> 
> Noralf Trønnes wrote:
>> I would like to keep the SET_BUFFER request since it will serve as a
>> syncing point between the host and the device. I'm no USB expert but I
>> assume that a bulk transfer can fail halfway through and result in the
>> next update starting where the previous one failed and thus writing
>> beyond the end of the display buffer.
> 
> Transfers either succeed completely (possibly after many retries),
> time out (after zero or more transfered bytes) or fail catastrophically
> (e.g. from device disconnect).
> 
> In all cases, the driver on the host knows/has available how many bytes
> were successfully transfered.
> 

I was thinking about the device, that it could get out of sync. Let's
say the host sends a 1k framebuffer and half of it gets transferred and
the rest fails for some reason. Lubomir's MCU implementation has an
endpoint max size of 64 bytes and a callback is called for each packet.
If the 1k transfer fails at some point, will the device be able to
detect this and know that the next time the rx callback is called that
this is the start of a new framebuffer update?

Noralf.

Re: [PATCH v3 0/6] Generic USB Display driver

[Peter Stuge]

Noralf Trønnes wrote:
> > In all cases, the driver on the host knows/has available how many bytes
> > were successfully transfered.
> 
> I was thinking about the device, that it could get out of sync. Let's
> say the host sends a 1k framebuffer and half of it gets transferred and
> the rest fails for some reason. Lubomir's MCU implementation has an
> endpoint max size of 64 bytes and a callback is called for each packet.
> If the 1k transfer fails at some point, will the device be able to
> detect this and know that the next time the rx callback is called that
> this is the start of a new framebuffer update?

Ah! No, a device can not detect that the host intended to send more (bulk)
packets but e.g. timed out.

I can't immediately think of other reasons for a larger transfer to fail,
which still allow communication to continue.

When the host recognizes a timeout with partial data transfer it could
simply send the remaining data in a new transfer.


While the device can not detect host intent, the protocol could allow
devices to specify requirements, e.g. that the host always sends full frames.

In any case, please avoid making a control request mandatory for frame
transfer.

Because control requests are scheduled differently onto the wire and
because they consist of more packets than bulk data, a control request
will interrupt a bulk data stream and likely introduce unneccessary latency.

If synchronization is always required then I'd suggest to place it inline
with frame data, e.g. in the first packet byte.

If synchronization is only required in rare cases then a control transfer
is probably the better choice, to not waste any inline bytes.

But the optimum would be that the device can describe its needs to the host
and the host driver ensures that the device always receives the data it needs.

Do you think this is possible?


Kind regards

//Peter

Re: [PATCH v3 0/6] Generic USB Display driver

[Lubomir Rintel]

On Tue, Jul 14, 2020 at 09:03:14PM +0200, Noralf Trønnes wrote:
> 
> 
> Den 14.07.2020 19.40, skrev Peter Stuge:
> > Hi Noralf,
> > 
> > Noralf Trønnes wrote:
> >> I would like to keep the SET_BUFFER request since it will serve as a
> >> syncing point between the host and the device. I'm no USB expert but I
> >> assume that a bulk transfer can fail halfway through and result in the
> >> next update starting where the previous one failed and thus writing
> >> beyond the end of the display buffer.
> > 
> > Transfers either succeed completely (possibly after many retries),
> > time out (after zero or more transfered bytes) or fail catastrophically
> > (e.g. from device disconnect).
> > 
> > In all cases, the driver on the host knows/has available how many bytes
> > were successfully transfered.
> > 
> 
> I was thinking about the device, that it could get out of sync. Let's
> say the host sends a 1k framebuffer and half of it gets transferred and
> the rest fails for some reason. Lubomir's MCU implementation has an
> endpoint max size of 64 bytes and a callback is called for each packet.

Note that 64 bytes was chosen totally arbitrarily, without any thought.
Perhaps the full frame of 1024 bytes would work just fine. I'm not
familiar with USB at all.

> If the 1k transfer fails at some point, will the device be able to
> detect this and know that the next time the rx callback is called that
> this is the start of a new framebuffer update?
> 
> Noralf.

Lubo

Re: [PATCH v3 0/6] Generic USB Display driver

[Noralf Trønnes]

Den 14.07.2020 21.38, skrev Peter Stuge:
> Noralf Trønnes wrote:
>>> In all cases, the driver on the host knows/has available how many bytes
>>> were successfully transfered.
>>
>> I was thinking about the device, that it could get out of sync. Let's
>> say the host sends a 1k framebuffer and half of it gets transferred and
>> the rest fails for some reason. Lubomir's MCU implementation has an
>> endpoint max size of 64 bytes and a callback is called for each packet.
>> If the 1k transfer fails at some point, will the device be able to
>> detect this and know that the next time the rx callback is called that
>> this is the start of a new framebuffer update?
> 
> Ah! No, a device can not detect that the host intended to send more (bulk)
> packets but e.g. timed out.
> 
> I can't immediately think of other reasons for a larger transfer to fail,
> which still allow communication to continue.
> 
> When the host recognizes a timeout with partial data transfer it could
> simply send the remaining data in a new transfer.
> 
> 
> While the device can not detect host intent, the protocol could allow
> devices to specify requirements, e.g. that the host always sends full frames.
> 
> In any case, please avoid making a control request mandatory for frame
> transfer.
> 
> Because control requests are scheduled differently onto the wire and
> because they consist of more packets than bulk data, a control request
> will interrupt a bulk data stream and likely introduce unneccessary latency.
> 
> If synchronization is always required then I'd suggest to place it inline
> with frame data, e.g. in the first packet byte.
> 
> If synchronization is only required in rare cases then a control transfer
> is probably the better choice, to not waste any inline bytes.
> 
> But the optimum would be that the device can describe its needs to the host
> and the host driver ensures that the device always receives the data it needs.
> 
> Do you think this is possible?
> 

Looking at the host driver I see that I need to fix it so that it
requeues the update if it fails (on SET_BUFFER or bulk out). Currently
it just goes back to sleep waiting for userspace to announce a new change.

I would like to avoid having a special case for retrying the failing
part of a bulk transfer for devices that only want full updates, I would
prefer to use the common error path of retrying the whole update.

This is my suggestion for the new flag:

/*
 * Always send the entire framebuffer when flushing changes.
 * The GUD_DRM_USB_REQ_SET_BUFFER request will not be sent before each
bulk transfer,
 * it will only be sent if the previous bulk transfer had failed. This
is done to
 * inform the device that the previous update failed and that a new one
is started.
 *
 * This flag can not be used in combination with compression.
 */
#define GUD_DRM_DISPLAY_FLAG_FULL_UPDATE	BIT(1)


Noralf.