* [PATCH v7 0/3] GUD USB Display driver
@ 2021-03-05 16:31 Noralf Trønnes
2021-03-05 16:31 ` [PATCH v7 1/3] drm/uapi: Add USB connector type Noralf Trønnes
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Noralf Trønnes @ 2021-03-05 16:31 UTC (permalink / raw)
To: dri-devel
Cc: linux-usb, sam, peter, lkundrak, markus, pontus.fuchs, hudson,
th020394, Noralf Trønnes
Hi,
A while back I had the idea to turn a Raspberry Pi Zero into a $5
USB to HDMI/SDTV/DPI display adapter.
The protocol is open so people are free to make displays implementing it and
use this driver, all that's needed is to add a USB vid:pid to the driver for
the display.
See the wiki[1] for more information and images for the Raspberry Pi Zero/4.
Changes in this version:
- LZ4_compress_default() can return zero, check for that
- Fix memory leak in gud_pipe_check() error path (Peter)
- Improve debug and error messages (Peter)
- Don't pass length in protocol structs (Peter)
- Pass USB interface to gud_usb_control_msg() et al. (Peter)
- Improve gud_connector_fill_properties() (Peter)
- Add GUD_PIXEL_FORMAT_RGB111 (Peter)
- Remove GUD_REQ_SET_VERSION (Peter)
- Fix DRM_IOCTL_MODE_OBJ_SETPROPERTY and the rotation property
- Fix dma-buf import (Thomas)
Note: There are several changes to the protocol.
I plan to apply the patches in a week or so unless something turns out to be
broken.
Dependency:
drm: Use USB controller's DMA mask when importing dmabufs[2]
(currently in drm-misc-fixes but not in drm-misc-next yet, also present in
drm-tip and linux-next)
Noralf.
[1] https://github.com/notro/gud/wiki
[2] https://patchwork.freedesktop.org/patch/msgid/20210303133229.3288-1-tzimmermann@suse.de
Noralf Trønnes (3):
drm/uapi: Add USB connector type
drm/probe-helper: Check epoch counter in output_poll_execute()
drm: Add GUD USB Display driver
MAINTAINERS | 8 +
drivers/gpu/drm/Kconfig | 2 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_connector.c | 1 +
drivers/gpu/drm/drm_probe_helper.c | 7 +-
drivers/gpu/drm/gud/Kconfig | 14 +
drivers/gpu/drm/gud/Makefile | 4 +
drivers/gpu/drm/gud/gud_connector.c | 729 ++++++++++++++++++++++++++++
drivers/gpu/drm/gud/gud_drv.c | 659 +++++++++++++++++++++++++
drivers/gpu/drm/gud/gud_internal.h | 154 ++++++
drivers/gpu/drm/gud/gud_pipe.c | 535 ++++++++++++++++++++
include/drm/gud.h | 333 +++++++++++++
include/uapi/drm/drm_mode.h | 1 +
13 files changed, 2447 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/gud/Kconfig
create mode 100644 drivers/gpu/drm/gud/Makefile
create mode 100644 drivers/gpu/drm/gud/gud_connector.c
create mode 100644 drivers/gpu/drm/gud/gud_drv.c
create mode 100644 drivers/gpu/drm/gud/gud_internal.h
create mode 100644 drivers/gpu/drm/gud/gud_pipe.c
create mode 100644 include/drm/gud.h
--
2.23.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v7 1/3] drm/uapi: Add USB connector type
2021-03-05 16:31 [PATCH v7 0/3] GUD USB Display driver Noralf Trønnes
@ 2021-03-05 16:31 ` Noralf Trønnes
2021-03-05 16:31 ` [PATCH v7 2/3] drm/probe-helper: Check epoch counter in output_poll_execute() Noralf Trønnes
2021-03-05 16:31 ` [PATCH v7 3/3] drm: Add GUD USB Display driver Noralf Trønnes
2 siblings, 0 replies; 18+ messages in thread
From: Noralf Trønnes @ 2021-03-05 16:31 UTC (permalink / raw)
To: dri-devel
Cc: linux-usb, sam, peter, lkundrak, markus, pontus.fuchs, hudson,
th020394, Noralf Trønnes, Daniel Vetter
Add a connector type for USB connected display panels.
Some examples of what current userspace will name the connector:
- Weston: "UNNAMED-%d"
- Mutter: "Unknown20-%d"
- X: "Unknown20-%d"
v2:
- Update drm_connector_enum_list
- Add examples to commit message
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/gpu/drm/drm_connector.c | 1 +
include/uapi/drm/drm_mode.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 717c4e7271b0..52712daed3bc 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -94,6 +94,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = {
{ DRM_MODE_CONNECTOR_DPI, "DPI" },
{ DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
{ DRM_MODE_CONNECTOR_SPI, "SPI" },
+ { DRM_MODE_CONNECTOR_USB, "USB" },
};
void drm_connector_ida_init(void)
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index fed66a03c7ae..33024cc5d26e 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -367,6 +367,7 @@ enum drm_mode_subconnector {
#define DRM_MODE_CONNECTOR_DPI 17
#define DRM_MODE_CONNECTOR_WRITEBACK 18
#define DRM_MODE_CONNECTOR_SPI 19
+#define DRM_MODE_CONNECTOR_USB 20
/**
* struct drm_mode_get_connector - Get connector metadata.
--
2.23.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v7 2/3] drm/probe-helper: Check epoch counter in output_poll_execute()
2021-03-05 16:31 [PATCH v7 0/3] GUD USB Display driver Noralf Trønnes
2021-03-05 16:31 ` [PATCH v7 1/3] drm/uapi: Add USB connector type Noralf Trønnes
@ 2021-03-05 16:31 ` Noralf Trønnes
2021-03-05 16:31 ` [PATCH v7 3/3] drm: Add GUD USB Display driver Noralf Trønnes
2 siblings, 0 replies; 18+ messages in thread
From: Noralf Trønnes @ 2021-03-05 16:31 UTC (permalink / raw)
To: dri-devel
Cc: linux-usb, sam, peter, lkundrak, markus, pontus.fuchs, hudson,
th020394, Noralf Trønnes, Daniel Vetter
drm_helper_hpd_irq_event() checks the epoch counter to determine
connector status change. This was introduced in
commit 5186421cbfe2 ("drm: Introduce epoch counter to drm_connector").
Do the same for output_poll_execute() so it can detect other changes
beside connection status value changes.
v2:
- Add Fixes tag (Daniel)
Fixes: 5186421cbfe2 ("drm: Introduce epoch counter to drm_connector")
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/gpu/drm/drm_probe_helper.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index d6017726cc2a..e5432dcf6999 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -623,6 +623,7 @@ static void output_poll_execute(struct work_struct *work)
struct drm_connector_list_iter conn_iter;
enum drm_connector_status old_status;
bool repoll = false, changed;
+ u64 old_epoch_counter;
if (!dev->mode_config.poll_enabled)
return;
@@ -659,8 +660,9 @@ static void output_poll_execute(struct work_struct *work)
repoll = true;
+ old_epoch_counter = connector->epoch_counter;
connector->status = drm_helper_probe_detect(connector, NULL, false);
- if (old_status != connector->status) {
+ if (old_epoch_counter != connector->epoch_counter) {
const char *old, *new;
/*
@@ -689,6 +691,9 @@ static void output_poll_execute(struct work_struct *work)
connector->base.id,
connector->name,
old, new);
+ DRM_DEBUG_KMS("[CONNECTOR:%d:%s] epoch counter %llu -> %llu\n",
+ connector->base.id, connector->name,
+ old_epoch_counter, connector->epoch_counter);
changed = true;
}
--
2.23.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v7 3/3] drm: Add GUD USB Display driver
2021-03-05 16:31 [PATCH v7 0/3] GUD USB Display driver Noralf Trønnes
2021-03-05 16:31 ` [PATCH v7 1/3] drm/uapi: Add USB connector type Noralf Trønnes
2021-03-05 16:31 ` [PATCH v7 2/3] drm/probe-helper: Check epoch counter in output_poll_execute() Noralf Trønnes
@ 2021-03-05 16:31 ` Noralf Trønnes
2021-03-08 18:13 ` Noralf Trønnes
2021-03-09 14:02 ` Peter Stuge
2 siblings, 2 replies; 18+ messages in thread
From: Noralf Trønnes @ 2021-03-05 16:31 UTC (permalink / raw)
To: dri-devel
Cc: linux-usb, sam, peter, lkundrak, markus, pontus.fuchs, hudson,
th020394, Noralf Trønnes, Daniel Vetter
This adds a 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
v4:
- Drop the status req polling which was a workaround for something that
turned out to be a dwc2 udc driver problem
- Add a flag for the Linux gadget to require a status request on
SET operations. Other devices will only get status req on STALL errors
- Use protocol specific error codes (Peter)
- Add a flag for devices that want to receive the entire framebuffer on
each flush (Lubomir)
- Retry a failed framebuffer flush
- If mode has changed wait for worker and clear pending damage before
queuing up new damage, fb width/height might have changed
- Increase error counter on bulk transfer failures
- Use DRM_MODE_CONNECTOR_USB
- Handle R1 kmalloc error (Peter)
- Don't try and replicate the USB get descriptor request standard for the
display descriptor (Peter)
- Make max_buffer_size optional (Peter), drop the pow2 requirement since
it's not necessary anymore.
- Don't pre-alloc a control request buffer, it was only 4k
- Let gud.h describe the whole protocol explicitly and don't let DRM
leak into it (Peter)
- Drop display mode .hskew and .vscan from the protocol
- Shorten names: s/GUD_DRM_/GUD_/ s/gud_drm_/gud_/ (Peter)
- Fix gud_pipe_check() connector picking when switching connector
- Drop gud_drm_driver_gem_create_object() cached is default now
- Retrieve USB device from struct drm_device.dev instead of keeping a
pointer
- Honour fb->offsets[0]
- Fix mode fetching when connector status is forced
- Check EDID length reported by the device
- Use drm_do_get_edid() so userspace can overrride EDID
- Set epoch counter to signal connector status change
- gud_drm_driver can be const now
v5:
- GUD_DRM_FORMAT_R1: Use non-human ascii values (Daniel)
- Change name to: GUD USB Display (Thomas, Simon)
- Change one __u32 -> __le32 in protocol header
- Always log fb flush errors, unless the previous one failed
- Run backlight update in a worker to avoid upsetting lockdep (Daniel)
- Drop backlight_ops.get_brightness, there's no readback from the device
so it doesn't really add anything.
- Set dma mask, needed by dma-buf importers
v6:
- Use obj-y in Makefile (Peter)
- Fix missing le32_to_cpu() when using GUD_DISPLAY_MAGIC (Peter)
- Set initial brightness on backlight device
v7:
- LZ4_compress_default() can return zero, check for that
- Fix memory leak in gud_pipe_check() error path (Peter)
- Improve debug and error messages (Peter)
- Don't pass length in protocol structs (Peter)
- Pass USB interface to gud_usb_control_msg() et al. (Peter)
- Improve gud_connector_fill_properties() (Peter)
- Add GUD_PIXEL_FORMAT_RGB111 (Peter)
- Remove GUD_REQ_SET_VERSION (Peter)
- Fix DRM_IOCTL_MODE_OBJ_SETPROPERTY and the rotation property
- Fix dma-buf import (Thomas)
Cc: Peter Stuge <peter@stuge.se>
Cc: Lubomir Rintel <lkundrak@v3.sk>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
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_connector.c | 729 ++++++++++++++++++++++++++++
drivers/gpu/drm/gud/gud_drv.c | 659 +++++++++++++++++++++++++
drivers/gpu/drm/gud/gud_internal.h | 154 ++++++
drivers/gpu/drm/gud/gud_pipe.c | 535 ++++++++++++++++++++
include/drm/gud.h | 333 +++++++++++++
10 files changed, 2439 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_connector.c
create mode 100644 drivers/gpu/drm/gud/gud_drv.c
create mode 100644 drivers/gpu/drm/gud/gud_internal.h
create mode 100644 drivers/gpu/drm/gud/gud_pipe.c
create mode 100644 include/drm/gud.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 00112c044608..e7f71ac55f08 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5525,6 +5525,14 @@ S: Maintained
F: Documentation/devicetree/bindings/display/panel/feiyang,fy07024di26a30d.yaml
F: drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c
+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.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 147d61b9674e..6d3f47933e51 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -388,6 +388,8 @@ source "drivers/gpu/drm/tidss/Kconfig"
source "drivers/gpu/drm/xlnx/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 81569009f884..78dd8e12525d 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -124,3 +124,4 @@ obj-$(CONFIG_DRM_ASPEED_GFX) += aspeed/
obj-$(CONFIG_DRM_MCDE) += mcde/
obj-$(CONFIG_DRM_TIDSS) += tidss/
obj-y += xlnx/
+obj-y += gud/
diff --git a/drivers/gpu/drm/gud/Kconfig b/drivers/gpu/drm/gud/Kconfig
new file mode 100644
index 000000000000..1c8601bf4d91
--- /dev/null
+++ b/drivers/gpu/drm/gud/Kconfig
@@ -0,0 +1,14 @@
+# SPDX-License-Identifier: GPL-2.0
+
+config DRM_GUD
+ tristate "GUD 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 GUD USB Displays or display
+ adapters.
+
+ If M is selected the module will be called gud.
diff --git a/drivers/gpu/drm/gud/Makefile b/drivers/gpu/drm/gud/Makefile
new file mode 100644
index 000000000000..68a1c622cf33
--- /dev/null
+++ b/drivers/gpu/drm/gud/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0
+
+gud-y := gud_drv.o gud_pipe.o gud_connector.o
+obj-$(CONFIG_DRM_GUD) += gud.o
diff --git a/drivers/gpu/drm/gud/gud_connector.c b/drivers/gpu/drm/gud/gud_connector.c
new file mode 100644
index 000000000000..ec495dcd6122
--- /dev/null
+++ b/drivers/gpu/drm/gud/gud_connector.c
@@ -0,0 +1,729 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright 2020 Noralf Trønnes
+ */
+
+#include <linux/backlight.h>
+#include <linux/workqueue.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.h>
+
+#include "gud_internal.h"
+
+struct gud_connector {
+ struct drm_connector connector;
+ struct drm_encoder encoder;
+ struct backlight_device *backlight;
+ struct work_struct backlight_work;
+
+ /* 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 to signal no backlight.
+ */
+ int initial_brightness;
+};
+
+static inline struct gud_connector *to_gud_connector(struct drm_connector *connector)
+{
+ return container_of(connector, struct gud_connector, connector);
+}
+
+static void gud_conn_err(struct drm_connector *connector, const char *msg, int ret)
+{
+ dev_err(connector->dev->dev, "%s: %s (ret=%d)\n", connector->name, msg, ret);
+}
+
+/*
+ * Use a worker to avoid taking kms locks inside the backlight lock.
+ * Other display drivers use backlight within their kms locks.
+ * This avoids inconsistent locking rules, which would upset lockdep.
+ */
+static void gud_connector_backlight_update_status_work(struct work_struct *work)
+{
+ struct gud_connector *gconn = container_of(work, struct gud_connector, backlight_work);
+ struct drm_connector *connector = &gconn->connector;
+ struct drm_connector_state *connector_state;
+ struct drm_device *drm = connector->dev;
+ struct drm_modeset_acquire_ctx ctx;
+ struct drm_atomic_state *state;
+ int idx, ret;
+
+ if (!drm_dev_enter(drm, &idx))
+ return;
+
+ state = drm_atomic_state_alloc(drm);
+ if (!state) {
+ ret = -ENOMEM;
+ goto exit;
+ }
+
+ 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 = gconn->backlight->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);
+exit:
+ drm_dev_exit(idx);
+
+ if (ret)
+ dev_err(drm->dev, "Failed to update backlight, err=%d\n", ret);
+}
+
+static int gud_connector_backlight_update_status(struct backlight_device *bd)
+{
+ struct drm_connector *connector = bl_get_data(bd);
+ struct gud_connector *gconn = to_gud_connector(connector);
+
+ /* The USB timeout is 5 seconds so use system_long_wq for worst case scenario */
+ queue_work(system_long_wq, &gconn->backlight_work);
+
+ return 0;
+}
+
+static const struct backlight_ops gud_connector_backlight_ops = {
+ .update_status = gud_connector_backlight_update_status,
+};
+
+static int gud_connector_backlight_register(struct gud_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,
+ .brightness = gconn->initial_brightness,
+ };
+
+ 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_connector_backlight_ops, &props);
+ kfree(name);
+ if (IS_ERR(bd))
+ return PTR_ERR(bd);
+
+ gconn->backlight = bd;
+
+ return 0;
+}
+
+static int gud_connector_detect(struct drm_connector *connector,
+ struct drm_modeset_acquire_ctx *ctx, bool force)
+{
+ struct gud_device *gdrm = to_gud_device(connector->dev);
+ int idx, ret;
+ u8 status;
+
+ if (!drm_dev_enter(connector->dev, &idx))
+ return connector_status_disconnected;
+
+ if (force) {
+ ret = gud_usb_set(gdrm, GUD_REQ_SET_CONNECTOR_FORCE_DETECT,
+ connector->index, NULL, 0);
+ if (ret) {
+ ret = connector_status_unknown;
+ goto exit;
+ }
+ }
+
+ ret = gud_usb_get_u8(gdrm, GUD_REQ_GET_CONNECTOR_STATUS, connector->index, &status);
+ if (ret) {
+ ret = connector_status_unknown;
+ goto exit;
+ }
+
+ switch (status & GUD_CONNECTOR_STATUS_CONNECTED_MASK) {
+ case GUD_CONNECTOR_STATUS_DISCONNECTED:
+ ret = connector_status_disconnected;
+ break;
+ case GUD_CONNECTOR_STATUS_CONNECTED:
+ ret = connector_status_connected;
+ break;
+ default:
+ ret = connector_status_unknown;
+ break;
+ };
+
+ if (status & GUD_CONNECTOR_STATUS_CHANGED)
+ connector->epoch_counter += 1;
+exit:
+ drm_dev_exit(idx);
+
+ return ret;
+}
+
+struct gud_connector_get_edid_ctx {
+ void *buf;
+ size_t len;
+ bool edid_override;
+};
+
+static int gud_connector_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
+{
+ struct gud_connector_get_edid_ctx *ctx = data;
+ size_t start = block * EDID_LENGTH;
+
+ ctx->edid_override = false;
+
+ if (start + len > ctx->len)
+ return -1;
+
+ memcpy(buf, ctx->buf + start, len);
+
+ return 0;
+}
+
+static int gud_connector_get_modes(struct drm_connector *connector)
+{
+ struct gud_device *gdrm = to_gud_device(connector->dev);
+ struct gud_display_mode_req *reqmodes = NULL;
+ struct gud_connector_get_edid_ctx edid_ctx;
+ unsigned int i, num_modes = 0;
+ struct edid *edid = NULL;
+ int idx, ret;
+
+ if (!drm_dev_enter(connector->dev, &idx))
+ return 0;
+
+ edid_ctx.edid_override = true;
+ edid_ctx.buf = kmalloc(GUD_CONNECTOR_MAX_EDID_LEN, GFP_KERNEL);
+ if (!edid_ctx.buf)
+ goto out;
+
+ ret = gud_usb_get(gdrm, GUD_REQ_GET_CONNECTOR_EDID, connector->index,
+ edid_ctx.buf, GUD_CONNECTOR_MAX_EDID_LEN);
+ if (ret > 0 && ret % EDID_LENGTH) {
+ gud_conn_err(connector, "Invalid EDID size", ret);
+ } else if (ret > 0) {
+ edid_ctx.len = ret;
+ edid = drm_do_get_edid(connector, gud_connector_get_edid_block, &edid_ctx);
+ }
+
+ kfree(edid_ctx.buf);
+ drm_connector_update_edid_property(connector, edid);
+
+ if (edid && edid_ctx.edid_override)
+ goto out;
+
+ reqmodes = kmalloc_array(GUD_CONNECTOR_MAX_NUM_MODES, sizeof(*reqmodes), GFP_KERNEL);
+ if (!reqmodes)
+ goto out;
+
+ ret = gud_usb_get(gdrm, GUD_REQ_GET_CONNECTOR_MODES, connector->index,
+ reqmodes, GUD_CONNECTOR_MAX_NUM_MODES * sizeof(*reqmodes));
+ if (ret <= 0)
+ goto out;
+ if (ret % sizeof(*reqmodes)) {
+ gud_conn_err(connector, "Invalid display mode array size", ret);
+ goto out;
+ }
+
+ num_modes = ret / sizeof(*reqmodes);
+
+ for (i = 0; i < num_modes; i++) {
+ struct drm_display_mode *mode;
+
+ mode = drm_mode_create(connector->dev);
+ if (!mode) {
+ num_modes = i;
+ goto out;
+ }
+
+ gud_to_display_mode(mode, &reqmodes[i]);
+ drm_mode_probed_add(connector, mode);
+ }
+out:
+ if (!num_modes)
+ num_modes = drm_add_edid_modes(connector, edid);
+
+ kfree(reqmodes);
+ kfree(edid);
+ drm_dev_exit(idx);
+
+ return num_modes;
+}
+
+static int gud_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.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_connector_helper_funcs = {
+ .detect_ctx = gud_connector_detect,
+ .get_modes = gud_connector_get_modes,
+ .atomic_check = gud_connector_atomic_check,
+};
+
+static int gud_connector_late_register(struct drm_connector *connector)
+{
+ struct gud_connector *gconn = to_gud_connector(connector);
+
+ if (gconn->initial_brightness < 0)
+ return 0;
+
+ return gud_connector_backlight_register(gconn);
+}
+
+static void gud_connector_early_unregister(struct drm_connector *connector)
+{
+ struct gud_connector *gconn = to_gud_connector(connector);
+
+ backlight_device_unregister(gconn->backlight);
+ cancel_work_sync(&gconn->backlight_work);
+}
+
+static void gud_connector_destroy(struct drm_connector *connector)
+{
+ struct gud_connector *gconn = to_gud_connector(connector);
+
+ drm_connector_cleanup(connector);
+ kfree(gconn->properties);
+ kfree(gconn);
+}
+
+static void gud_connector_reset(struct drm_connector *connector)
+{
+ struct gud_connector *gconn = to_gud_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_connector_funcs = {
+ .fill_modes = drm_helper_probe_single_connector_modes,
+ .late_register = gud_connector_late_register,
+ .early_unregister = gud_connector_early_unregister,
+ .destroy = gud_connector_destroy,
+ .reset = gud_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_connector_add_tv_mode(struct gud_device *gdrm, struct drm_connector *connector)
+{
+ size_t buf_len = GUD_CONNECTOR_TV_MODE_MAX_NUM * GUD_CONNECTOR_TV_MODE_NAME_LEN;
+ const char *modes[GUD_CONNECTOR_TV_MODE_MAX_NUM];
+ unsigned int i, num_modes;
+ char *buf;
+ int ret;
+
+ buf = kmalloc(buf_len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = gud_usb_get(gdrm, GUD_REQ_GET_CONNECTOR_TV_MODE_VALUES,
+ connector->index, buf, buf_len);
+ if (ret < 0)
+ goto free;
+ if (!ret || ret % GUD_CONNECTOR_TV_MODE_NAME_LEN) {
+ ret = -EIO;
+ goto free;
+ }
+
+ num_modes = ret / GUD_CONNECTOR_TV_MODE_NAME_LEN;
+ for (i = 0; i < num_modes; i++)
+ modes[i] = &buf[i * GUD_CONNECTOR_TV_MODE_NAME_LEN];
+
+ ret = drm_mode_create_tv_properties(connector->dev, num_modes, modes);
+free:
+ kfree(buf);
+ if (ret < 0)
+ gud_conn_err(connector, "Failed to add TV modes", ret);
+
+ return ret;
+}
+
+static struct drm_property *
+gud_connector_property_lookup(struct drm_connector *connector, u16 prop)
+{
+ struct drm_mode_config *config = &connector->dev->mode_config;
+
+ switch (prop) {
+ case GUD_PROPERTY_TV_LEFT_MARGIN:
+ return config->tv_left_margin_property;
+ case GUD_PROPERTY_TV_RIGHT_MARGIN:
+ return config->tv_right_margin_property;
+ case GUD_PROPERTY_TV_TOP_MARGIN:
+ return config->tv_top_margin_property;
+ case GUD_PROPERTY_TV_BOTTOM_MARGIN:
+ return config->tv_bottom_margin_property;
+ case GUD_PROPERTY_TV_MODE:
+ return config->tv_mode_property;
+ case GUD_PROPERTY_TV_BRIGHTNESS:
+ return config->tv_brightness_property;
+ case GUD_PROPERTY_TV_CONTRAST:
+ return config->tv_contrast_property;
+ case GUD_PROPERTY_TV_FLICKER_REDUCTION:
+ return config->tv_flicker_reduction_property;
+ case GUD_PROPERTY_TV_OVERSCAN:
+ return config->tv_overscan_property;
+ case GUD_PROPERTY_TV_SATURATION:
+ return config->tv_saturation_property;
+ case GUD_PROPERTY_TV_HUE:
+ return config->tv_hue_property;
+ default:
+ return ERR_PTR(-EINVAL);
+ }
+}
+
+static unsigned int *gud_connector_tv_state_val(u16 prop, struct drm_tv_connector_state *state)
+{
+ switch (prop) {
+ case GUD_PROPERTY_TV_LEFT_MARGIN:
+ return &state->margins.left;
+ case GUD_PROPERTY_TV_RIGHT_MARGIN:
+ return &state->margins.right;
+ case GUD_PROPERTY_TV_TOP_MARGIN:
+ return &state->margins.top;
+ case GUD_PROPERTY_TV_BOTTOM_MARGIN:
+ return &state->margins.bottom;
+ case GUD_PROPERTY_TV_MODE:
+ return &state->mode;
+ case GUD_PROPERTY_TV_BRIGHTNESS:
+ return &state->brightness;
+ case GUD_PROPERTY_TV_CONTRAST:
+ return &state->contrast;
+ case GUD_PROPERTY_TV_FLICKER_REDUCTION:
+ return &state->flicker_reduction;
+ case GUD_PROPERTY_TV_OVERSCAN:
+ return &state->overscan;
+ case GUD_PROPERTY_TV_SATURATION:
+ return &state->saturation;
+ case GUD_PROPERTY_TV_HUE:
+ return &state->hue;
+ default:
+ return ERR_PTR(-EINVAL);
+ }
+}
+
+static int gud_connector_add_properties(struct gud_device *gdrm, struct gud_connector *gconn)
+{
+ struct drm_connector *connector = &gconn->connector;
+ struct drm_device *drm = &gdrm->drm;
+ struct gud_property_req *properties;
+ unsigned int i, num_properties;
+ int ret;
+
+ properties = kcalloc(GUD_CONNECTOR_PROPERTIES_MAX_NUM, sizeof(*properties), GFP_KERNEL);
+ if (!properties)
+ return -ENOMEM;
+
+ ret = gud_usb_get(gdrm, GUD_REQ_GET_CONNECTOR_PROPERTIES, connector->index,
+ properties, GUD_CONNECTOR_PROPERTIES_MAX_NUM * sizeof(*properties));
+ if (ret <= 0)
+ goto out;
+ if (ret % sizeof(*properties)) {
+ ret = -EIO;
+ goto out;
+ }
+
+ num_properties = ret / sizeof(*properties);
+ ret = 0;
+
+ gconn->properties = kcalloc(num_properties, sizeof(*gconn->properties), GFP_KERNEL);
+ if (!gconn->properties) {
+ ret = -ENOMEM;
+ 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;
+
+ drm_dbg(drm, "property: %u = %llu(0x%llx)\n", prop, val, val);
+
+ switch (prop) {
+ case GUD_PROPERTY_TV_LEFT_MARGIN:
+ fallthrough;
+ case GUD_PROPERTY_TV_RIGHT_MARGIN:
+ fallthrough;
+ case GUD_PROPERTY_TV_TOP_MARGIN:
+ fallthrough;
+ case GUD_PROPERTY_TV_BOTTOM_MARGIN:
+ ret = drm_mode_create_tv_margin_properties(drm);
+ if (ret)
+ goto out;
+ break;
+ case GUD_PROPERTY_TV_MODE:
+ ret = gud_connector_add_tv_mode(gdrm, connector);
+ if (ret)
+ goto out;
+ break;
+ case GUD_PROPERTY_TV_BRIGHTNESS:
+ fallthrough;
+ case GUD_PROPERTY_TV_CONTRAST:
+ fallthrough;
+ case GUD_PROPERTY_TV_FLICKER_REDUCTION:
+ fallthrough;
+ case GUD_PROPERTY_TV_OVERSCAN:
+ fallthrough;
+ case GUD_PROPERTY_TV_SATURATION:
+ fallthrough;
+ case GUD_PROPERTY_TV_HUE:
+ /* This is a no-op if already added. */
+ ret = drm_mode_create_tv_properties(drm, 0, NULL);
+ if (ret)
+ goto out;
+ break;
+ case GUD_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, "Ignoring unknown property: %u\n", prop);
+ continue;
+ }
+
+ gconn->properties[gconn->num_properties++] = prop;
+
+ if (prop == GUD_PROPERTY_BACKLIGHT_BRIGHTNESS)
+ continue; /* not a DRM property */
+
+ property = gud_connector_property_lookup(connector, prop);
+ if (WARN_ON(IS_ERR(property)))
+ continue;
+
+ state_val = gud_connector_tv_state_val(prop, &gconn->initial_tv_state);
+ if (WARN_ON(IS_ERR(state_val)))
+ continue;
+
+ *state_val = val;
+ drm_object_attach_property(&connector->base, property, 0);
+ }
+out:
+ kfree(properties);
+
+ return ret;
+}
+
+int gud_connector_fill_properties(struct drm_connector_state *connector_state,
+ struct gud_property_req *properties)
+{
+ struct gud_connector *gconn = to_gud_connector(connector_state->connector);
+ unsigned int i;
+
+ for (i = 0; i < gconn->num_properties; i++) {
+ u16 prop = gconn->properties[i];
+ u64 val;
+
+ if (prop == GUD_PROPERTY_BACKLIGHT_BRIGHTNESS) {
+ val = connector_state->tv.brightness;
+ } else {
+ unsigned int *state_val;
+
+ state_val = gud_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;
+}
+
+static int gud_connector_create(struct gud_device *gdrm, unsigned int index,
+ struct gud_connector_descriptor_req *desc)
+{
+ struct drm_device *drm = &gdrm->drm;
+ struct gud_connector *gconn;
+ struct drm_connector *connector;
+ struct drm_encoder *encoder;
+ int ret, connector_type;
+ u32 flags;
+
+ gconn = kzalloc(sizeof(*gconn), GFP_KERNEL);
+ if (!gconn)
+ return -ENOMEM;
+
+ INIT_WORK(&gconn->backlight_work, gud_connector_backlight_update_status_work);
+ gconn->initial_brightness = -ENODEV;
+ flags = le32_to_cpu(desc->flags);
+ connector = &gconn->connector;
+
+ drm_dbg(drm, "Connector: index=%u type=%u flags=0x%x\n", index, desc->connector_type, flags);
+
+ switch (desc->connector_type) {
+ case GUD_CONNECTOR_TYPE_PANEL:
+ connector_type = DRM_MODE_CONNECTOR_USB;
+ break;
+ case GUD_CONNECTOR_TYPE_VGA:
+ connector_type = DRM_MODE_CONNECTOR_VGA;
+ break;
+ case GUD_CONNECTOR_TYPE_DVI:
+ connector_type = DRM_MODE_CONNECTOR_DVID;
+ break;
+ case GUD_CONNECTOR_TYPE_COMPOSITE:
+ connector_type = DRM_MODE_CONNECTOR_Composite;
+ break;
+ case GUD_CONNECTOR_TYPE_SVIDEO:
+ connector_type = DRM_MODE_CONNECTOR_SVIDEO;
+ break;
+ case GUD_CONNECTOR_TYPE_COMPONENT:
+ connector_type = DRM_MODE_CONNECTOR_Component;
+ break;
+ case GUD_CONNECTOR_TYPE_DISPLAYPORT:
+ connector_type = DRM_MODE_CONNECTOR_DisplayPort;
+ break;
+ case GUD_CONNECTOR_TYPE_HDMI:
+ connector_type = DRM_MODE_CONNECTOR_HDMIA;
+ break;
+ default: /* future types */
+ connector_type = DRM_MODE_CONNECTOR_USB;
+ break;
+ };
+
+ drm_connector_helper_add(connector, &gud_connector_helper_funcs);
+ ret = drm_connector_init(drm, connector, &gud_connector_funcs, connector_type);
+ if (ret) {
+ kfree(connector);
+ return ret;
+ }
+
+ if (WARN_ON(connector->index != index))
+ return -EINVAL;
+
+ if (flags & GUD_CONNECTOR_FLAGS_POLL_STATUS)
+ connector->polled = (DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT);
+ if (flags & GUD_CONNECTOR_FLAGS_INTERLACE)
+ connector->interlace_allowed = true;
+ if (flags & GUD_CONNECTOR_FLAGS_DOUBLESCAN)
+ connector->doublescan_allowed = true;
+
+ ret = gud_connector_add_properties(gdrm, gconn);
+ if (ret) {
+ gud_conn_err(connector, "Failed to add properties", ret);
+ 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);
+}
+
+int gud_get_connectors(struct gud_device *gdrm)
+{
+ struct gud_connector_descriptor_req *descs;
+ unsigned int i, num_connectors;
+ int ret;
+
+ descs = kmalloc_array(GUD_CONNECTORS_MAX_NUM, sizeof(*descs), GFP_KERNEL);
+ if (!descs)
+ return -ENOMEM;
+
+ ret = gud_usb_get(gdrm, GUD_REQ_GET_CONNECTORS, 0,
+ descs, GUD_CONNECTORS_MAX_NUM * sizeof(descs));
+ if (ret < 0)
+ goto free;
+ if (!ret || ret % sizeof(*descs)) {
+ ret = -EIO;
+ goto free;
+ }
+
+ num_connectors = ret / sizeof(*descs);
+
+ for (i = 0; i < num_connectors; i++) {
+ ret = gud_connector_create(gdrm, i, &descs[i]);
+ if (ret)
+ goto free;
+ }
+free:
+ kfree(descs);
+
+ return ret;
+}
diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
new file mode 100644
index 000000000000..2e3056e4d271
--- /dev/null
+++ b/drivers/gpu/drm/gud/gud_drv.c
@@ -0,0 +1,659 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright 2020 Noralf Trønnes
+ */
+
+#include <linux/dma-buf.h>
+#include <linux/dma-mapping.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/vmalloc.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_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_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.h>
+
+#include "gud_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 const struct drm_format_info gud_drm_format_rgb111 = {
+ .format = GUD_DRM_FORMAT_RGB111,
+ .num_planes = 1,
+ .char_per_block = { 1, 0, 0 },
+ .block_w = { 2, 0, 0 },
+ .block_h = { 1, 0, 0 },
+ .hsub = 1,
+ .vsub = 1,
+};
+
+static int gud_usb_control_msg(struct usb_interface *intf, bool in,
+ u8 request, u16 value, void *buf, size_t len)
+{
+ u8 requesttype = USB_TYPE_VENDOR | USB_RECIP_INTERFACE;
+ u8 ifnum = intf->cur_altsetting->desc.bInterfaceNumber;
+ struct usb_device *usb = interface_to_usbdev(intf);
+ unsigned int pipe;
+
+ if (len && !buf)
+ return -EINVAL;
+
+ if (in) {
+ pipe = usb_rcvctrlpipe(usb, 0);
+ requesttype |= USB_DIR_IN;
+ } else {
+ pipe = usb_sndctrlpipe(usb, 0);
+ requesttype |= USB_DIR_OUT;
+ }
+
+ return usb_control_msg(usb, pipe, request, requesttype, value,
+ ifnum, buf, len, USB_CTRL_GET_TIMEOUT);
+}
+
+static int gud_get_display_descriptor(struct usb_interface *intf,
+ struct gud_display_descriptor_req *desc)
+{
+ void *buf;
+ int ret;
+
+ buf = kmalloc(sizeof(*desc), GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = gud_usb_control_msg(intf, true, GUD_REQ_GET_DESCRIPTOR, 0, buf, sizeof(*desc));
+ memcpy(desc, buf, sizeof(*desc));
+ kfree(buf);
+ if (ret < 0)
+ return ret;
+ if (ret != sizeof(*desc))
+ return -EIO;
+
+ if (desc->magic != le32_to_cpu(GUD_DISPLAY_MAGIC))
+ return -ENODATA;
+
+ DRM_DEV_DEBUG_DRIVER(&intf->dev,
+ "version=%u flags=0x%x compression=0x%x max_buffer_size=%u\n",
+ desc->version, le32_to_cpu(desc->flags), desc->compression,
+ le32_to_cpu(desc->max_buffer_size));
+
+ if (!desc->version || !desc->max_width || !desc->max_height ||
+ le32_to_cpu(desc->min_width) > le32_to_cpu(desc->max_width) ||
+ le32_to_cpu(desc->min_height) > le32_to_cpu(desc->max_height))
+ return -EINVAL;
+
+ return 0;
+}
+
+static int gud_status_to_errno(u8 status)
+{
+ switch (status) {
+ case GUD_STATUS_OK:
+ return 0;
+ case GUD_STATUS_BUSY:
+ return -EBUSY;
+ case GUD_STATUS_REQUEST_NOT_SUPPORTED:
+ return -EOPNOTSUPP;
+ case GUD_STATUS_PROTOCOL_ERROR:
+ return -EPROTO;
+ case GUD_STATUS_INVALID_PARAMETER:
+ return -EINVAL;
+ case GUD_STATUS_ERROR:
+ return -EREMOTEIO;
+ default:
+ return -EREMOTEIO;
+ }
+}
+
+static int gud_usb_get_status(struct usb_interface *intf)
+{
+ int ret, status = -EIO;
+ u8 *buf;
+
+ buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = gud_usb_control_msg(intf, true, GUD_REQ_GET_STATUS, 0, buf, sizeof(*buf));
+ if (ret == sizeof(*buf))
+ status = gud_status_to_errno(*buf);
+ kfree(buf);
+
+ if (ret < 0)
+ return ret;
+
+ return status;
+}
+
+static int gud_usb_transfer(struct gud_device *gdrm, bool in, u8 request, u16 index,
+ void *buf, size_t len)
+{
+ struct usb_interface *intf = to_usb_interface(gdrm->drm.dev);
+ int idx, ret;
+
+ drm_dbg(&gdrm->drm, "%s: request=0x%x index=%u len=%zu\n",
+ in ? "get" : "set", request, index, len);
+
+ if (!drm_dev_enter(&gdrm->drm, &idx))
+ return -ENODEV;
+
+ mutex_lock(&gdrm->ctrl_lock);
+
+ ret = gud_usb_control_msg(intf, in, request, index, buf, len);
+ if (ret == -EPIPE || ((gdrm->flags & GUD_DISPLAY_FLAG_STATUS_ON_SET) && !in && ret >= 0)) {
+ int status;
+
+ status = gud_usb_get_status(intf);
+ if (status < 0) {
+ ret = status;
+ } else if (ret < 0) {
+ dev_err_once(gdrm->drm.dev,
+ "Unexpected status OK for failed transfer\n");
+ ret = -EPIPE;
+ }
+ }
+
+ if (ret < 0) {
+ drm_dbg(&gdrm->drm, "ret=%d\n", ret);
+ gdrm->stats_num_errors++;
+ }
+
+ mutex_unlock(&gdrm->ctrl_lock);
+ drm_dev_exit(idx);
+
+ return ret;
+}
+
+/*
+ * @buf cannot be allocated on the stack.
+ * Returns number of bytes received or negative error code on failure.
+ */
+int gud_usb_get(struct gud_device *gdrm, u8 request, u16 index, void *buf, size_t max_len)
+{
+ return gud_usb_transfer(gdrm, true, request, index, buf, max_len);
+}
+
+/*
+ * @buf can be allocated on the stack or NULL.
+ * Returns zero on success or negative error code on failure.
+ */
+int gud_usb_set(struct gud_device *gdrm, u8 request, u16 index, void *buf, size_t len)
+{
+ void *trbuf = NULL;
+ int ret;
+
+ if (buf && len) {
+ trbuf = kmemdup(buf, len, GFP_KERNEL);
+ if (!trbuf)
+ return -ENOMEM;
+ }
+
+ ret = gud_usb_transfer(gdrm, false, request, index, trbuf, len);
+ kfree(trbuf);
+ if (ret < 0)
+ return ret;
+
+ return ret != len ? -EIO : 0;
+}
+
+/*
+ * @val can be allocated on the stack.
+ * Returns zero on success or negative error code on failure.
+ */
+int gud_usb_get_u8(struct gud_device *gdrm, u8 request, u16 index, u8 *val)
+{
+ u8 *buf;
+ int ret;
+
+ buf = kmalloc(sizeof(*val), GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = gud_usb_get(gdrm, request, index, buf, sizeof(*val));
+ *val = *buf;
+ kfree(buf);
+ if (ret < 0)
+ return ret;
+
+ return ret != sizeof(*val) ? -EIO : 0;
+}
+
+/* Returns zero on success or negative error code on failure. */
+int gud_usb_set_u8(struct gud_device *gdrm, u8 request, u8 val)
+{
+ return gud_usb_set(gdrm, request, 0, &val, sizeof(val));
+}
+
+static int gud_get_properties(struct gud_device *gdrm)
+{
+ struct gud_property_req *properties;
+ unsigned int i, num_properties;
+ int ret;
+
+ properties = kcalloc(GUD_PROPERTIES_MAX_NUM, sizeof(*properties), GFP_KERNEL);
+ if (!properties)
+ return -ENOMEM;
+
+ ret = gud_usb_get(gdrm, GUD_REQ_GET_PROPERTIES, 0,
+ properties, GUD_PROPERTIES_MAX_NUM * sizeof(*properties));
+ if (ret <= 0)
+ goto out;
+ if (ret % sizeof(*properties)) {
+ ret = -EIO;
+ goto out;
+ }
+
+ num_properties = ret / sizeof(*properties);
+
+ gdrm->properties = drmm_kcalloc(&gdrm->drm, num_properties, sizeof(*gdrm->properties),
+ GFP_KERNEL);
+ if (!gdrm->properties) {
+ ret = -ENOMEM;
+ 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_PROPERTY_ROTATION:
+ /*
+ * DRM UAPI matches the protocol so use the value directly,
+ * but mask out any additions on future devices.
+ */
+ val &= GUD_ROTATION_MASK;
+ 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, "Ignoring unknown property: %u\n", prop);
+ continue;
+ }
+
+ if (ret)
+ goto out;
+
+ gdrm->properties[gdrm->num_properties++] = prop;
+ }
+out:
+ kfree(properties);
+
+ return ret;
+}
+
+/*
+ * FIXME: Dma-buf sharing requires DMA support by the importing device.
+ * This function is a workaround to make USB devices work as well.
+ * See todo.rst for how to fix the issue in the dma-buf framework.
+ */
+static struct drm_gem_object *gud_gem_prime_import(struct drm_device *drm, struct dma_buf *dma_buf)
+{
+ struct gud_device *gdrm = to_gud_device(drm);
+
+ if (!gdrm->dmadev)
+ return ERR_PTR(-ENODEV);
+
+ return drm_gem_prime_import_dev(drm, dma_buf, gdrm->dmadev);
+}
+
+static int gud_stats_debugfs(struct seq_file *m, void *data)
+{
+ struct drm_info_node *node = m->private;
+ struct gud_device *gdrm = to_gud_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_COMPRESSION_LZ4)
+ seq_puts(m, " lz4");
+ if (!gdrm->compression)
+ seq_puts(m, " none");
+ 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_debugfs_list[] = {
+ { "stats", gud_stats_debugfs, 0, NULL },
+};
+
+static void gud_debugfs_init(struct drm_minor *minor)
+{
+ drm_debugfs_create_files(gud_debugfs_list, ARRAY_SIZE(gud_debugfs_list),
+ minor->debugfs_root, minor);
+}
+
+static const struct drm_simple_display_pipe_funcs gud_pipe_funcs = {
+ .check = gud_pipe_check,
+ .update = gud_pipe_update,
+ .prepare_fb = drm_gem_simple_display_pipe_prepare_fb,
+};
+
+static const struct drm_mode_config_funcs gud_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 u64 gud_pipe_modifiers[] = {
+ DRM_FORMAT_MOD_LINEAR,
+ DRM_FORMAT_MOD_INVALID
+};
+
+DEFINE_DRM_GEM_FOPS(gud_fops);
+
+static const struct drm_driver gud_drm_driver = {
+ .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
+ .fops = &gud_fops,
+ DRM_GEM_SHMEM_DRIVER_OPS,
+ .gem_prime_import = gud_gem_prime_import,
+ .debugfs_init = gud_debugfs_init,
+
+ .name = "gud",
+ .desc = "Generic USB Display",
+ .date = "20200422",
+ .major = 1,
+ .minor = 0,
+};
+
+static void gud_free_buffers_and_mutex(struct drm_device *drm, void *unused)
+{
+ struct gud_device *gdrm = to_gud_device(drm);
+
+ vfree(gdrm->compress_buf);
+ kfree(gdrm->bulk_buf);
+ mutex_destroy(&gdrm->ctrl_lock);
+ mutex_destroy(&gdrm->damage_lock);
+}
+
+static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
+{
+ const struct drm_format_info *xrgb8888_emulation_format = NULL;
+ bool rgb565_supported = false, xrgb8888_supported = false;
+ unsigned int num_formats_dev, num_formats = 0;
+ struct usb_endpoint_descriptor *bulk_out;
+ struct gud_display_descriptor_req desc;
+ struct device *dev = &intf->dev;
+ size_t max_buffer_size = 0;
+ struct gud_device *gdrm;
+ struct drm_device *drm;
+ u8 *formats_dev;
+ u32 *formats;
+ int ret, i;
+
+ ret = usb_find_bulk_out_endpoint(intf->cur_altsetting, &bulk_out);
+ if (ret)
+ return ret;
+
+ ret = gud_get_display_descriptor(intf, &desc);
+ if (ret) {
+ DRM_DEV_DEBUG_DRIVER(dev, "Not a display interface: ret=%d\n", ret);
+ return -ENODEV;
+ }
+
+ if (desc.version > 1) {
+ dev_err(dev, "Protocol version %u is not supported\n", desc.version);
+ return -ENODEV;
+ }
+
+ gdrm = devm_drm_dev_alloc(dev, &gud_drm_driver, struct gud_device, drm);
+ if (IS_ERR(gdrm))
+ return PTR_ERR(gdrm);
+
+ drm = &gdrm->drm;
+ drm->mode_config.funcs = &gud_mode_config_funcs;
+ ret = drmm_mode_config_init(drm);
+ if (ret)
+ return ret;
+
+ gdrm->flags = le32_to_cpu(desc.flags);
+ gdrm->compression = desc.compression & GUD_COMPRESSION_LZ4;
+
+ if (gdrm->flags & GUD_DISPLAY_FLAG_FULL_UPDATE && gdrm->compression)
+ return -EINVAL;
+
+ mutex_init(&gdrm->ctrl_lock);
+ mutex_init(&gdrm->damage_lock);
+ INIT_WORK(&gdrm->work, gud_flush_work);
+ gud_clear_damage(gdrm);
+
+ ret = drmm_add_action_or_reset(drm, gud_free_buffers_and_mutex, NULL);
+ if (ret)
+ return ret;
+
+ drm->mode_config.min_width = le32_to_cpu(desc.min_width);
+ drm->mode_config.max_width = le32_to_cpu(desc.max_width);
+ drm->mode_config.min_height = le32_to_cpu(desc.min_height);
+ drm->mode_config.max_height = le32_to_cpu(desc.max_height);
+
+ formats_dev = devm_kmalloc(dev, GUD_FORMATS_MAX_NUM, GFP_KERNEL);
+ /* Add room for emulated XRGB8888 */
+ formats = devm_kmalloc_array(dev, GUD_FORMATS_MAX_NUM + 1, sizeof(*formats), GFP_KERNEL);
+ if (!formats_dev || !formats)
+ return -ENOMEM;
+
+ ret = gud_usb_get(gdrm, GUD_REQ_GET_FORMATS, 0, formats_dev, GUD_FORMATS_MAX_NUM);
+ if (ret < 0)
+ return ret;
+
+ num_formats_dev = ret;
+ for (i = 0; i < num_formats_dev; i++) {
+ const struct drm_format_info *info;
+ size_t fmt_buf_size;
+ u32 format;
+
+ format = gud_to_fourcc(formats_dev[i]);
+ if (!format) {
+ drm_dbg(drm, "Unsupported format: 0x%02x\n", formats_dev[i]);
+ continue;
+ }
+
+ if (format == GUD_DRM_FORMAT_R1)
+ info = &gud_drm_format_r1;
+ else if (format == GUD_DRM_FORMAT_RGB111)
+ info = &gud_drm_format_rgb111;
+ else
+ info = drm_format_info(format);
+
+ switch (format) {
+ case GUD_DRM_FORMAT_R1:
+ fallthrough;
+ case GUD_DRM_FORMAT_RGB111:
+ xrgb8888_emulation_format = info;
+ break;
+ case DRM_FORMAT_RGB565:
+ rgb565_supported = true;
+ if (!xrgb8888_emulation_format)
+ xrgb8888_emulation_format = info;
+ break;
+ case DRM_FORMAT_XRGB8888:
+ xrgb8888_supported = true;
+ break;
+ };
+
+ fmt_buf_size = drm_format_info_min_pitch(info, 0, drm->mode_config.max_width) *
+ drm->mode_config.max_height;
+ max_buffer_size = max(max_buffer_size, fmt_buf_size);
+
+ if (format == GUD_DRM_FORMAT_R1)
+ continue; /* Internal not for userspace */
+
+ formats[num_formats++] = format;
+ }
+
+ if (!num_formats && !xrgb8888_emulation_format) {
+ dev_err(dev, "No supported pixel formats found\n");
+ return -EINVAL;
+ }
+
+ /* Prefer speed over color depth */
+ if (rgb565_supported)
+ drm->mode_config.preferred_depth = 16;
+
+ if (!xrgb8888_supported && xrgb8888_emulation_format) {
+ gdrm->xrgb8888_emulation_format = xrgb8888_emulation_format;
+ formats[num_formats++] = DRM_FORMAT_XRGB8888;
+ }
+
+ if (desc.max_buffer_size)
+ max_buffer_size = le32_to_cpu(desc.max_buffer_size);
+retry:
+ /*
+ * Use plain kmalloc here since devm_kmalloc() places struct devres at the beginning
+ * of the buffer it allocates. This wastes a lot of memory when allocating big buffers.
+ * Asking for 2M would actually allocate 4M. This would also prevent getting the biggest
+ * possible buffer potentially leading to split transfers.
+ */
+ gdrm->bulk_buf = kmalloc(max_buffer_size, GFP_KERNEL | __GFP_NOWARN);
+ if (!gdrm->bulk_buf) {
+ max_buffer_size = roundup_pow_of_two(max_buffer_size) / 2;
+ if (max_buffer_size < SZ_512K)
+ return -ENOMEM;
+ goto retry;
+ }
+
+ gdrm->bulk_pipe = usb_sndbulkpipe(interface_to_usbdev(intf), usb_endpoint_num(bulk_out));
+ gdrm->bulk_len = max_buffer_size;
+
+ if (gdrm->compression & GUD_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;
+ }
+
+ ret = drm_simple_display_pipe_init(drm, &gdrm->pipe, &gud_pipe_funcs,
+ formats, num_formats,
+ gud_pipe_modifiers, NULL);
+ if (ret)
+ return ret;
+
+ devm_kfree(dev, formats);
+ devm_kfree(dev, formats_dev);
+
+ ret = gud_get_properties(gdrm);
+ if (ret) {
+ dev_err(dev, "Failed to get properties (error=%d)\n", ret);
+ return ret;
+ }
+
+ drm_plane_enable_fb_damage_clips(&gdrm->pipe.plane);
+
+ ret = gud_get_connectors(gdrm);
+ if (ret) {
+ dev_err(dev, "Failed to get connectors (error=%d)\n", ret);
+ return ret;
+ }
+
+ drm_mode_config_reset(drm);
+
+ usb_set_intfdata(intf, gdrm);
+
+ gdrm->dmadev = usb_intf_get_dma_device(intf);
+ if (!gdrm->dmadev)
+ dev_warn(dev, "buffer sharing not supported");
+
+ ret = drm_dev_register(drm, 0);
+ if (ret) {
+ put_device(gdrm->dmadev);
+ return ret;
+ }
+
+ drm_kms_helper_poll_init(drm);
+
+ drm_fbdev_generic_setup(drm, 0);
+
+ return 0;
+}
+
+static void gud_disconnect(struct usb_interface *interface)
+{
+ struct gud_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);
+ put_device(gdrm->dmadev);
+ gdrm->dmadev = NULL;
+}
+
+static int gud_suspend(struct usb_interface *intf, pm_message_t message)
+{
+ struct gud_device *gdrm = usb_get_intfdata(intf);
+
+ return drm_mode_config_helper_suspend(&gdrm->drm);
+}
+
+static int gud_resume(struct usb_interface *intf)
+{
+ struct gud_device *gdrm = usb_get_intfdata(intf);
+
+ drm_mode_config_helper_resume(&gdrm->drm);
+
+ return 0;
+}
+
+static const struct usb_device_id gud_id_table[] = {
+ { USB_DEVICE_INTERFACE_CLASS(0x1d50, 0x614d, USB_CLASS_VENDOR_SPEC) },
+ { }
+};
+
+MODULE_DEVICE_TABLE(usb, gud_id_table);
+
+static struct usb_driver gud_usb_driver = {
+ .name = "gud",
+ .probe = gud_probe,
+ .disconnect = gud_disconnect,
+ .id_table = gud_id_table,
+ .suspend = gud_suspend,
+ .resume = gud_resume,
+ .reset_resume = gud_resume,
+};
+
+module_usb_driver(gud_usb_driver);
+
+MODULE_AUTHOR("Noralf Trønnes");
+MODULE_LICENSE("Dual MIT/GPL");
diff --git a/drivers/gpu/drm/gud/gud_internal.h b/drivers/gpu/drm/gud/gud_internal.h
new file mode 100644
index 000000000000..b3fd2bdd4901
--- /dev/null
+++ b/drivers/gpu/drm/gud/gud_internal.h
@@ -0,0 +1,154 @@
+/* SPDX-License-Identifier: MIT */
+
+#ifndef __LINUX_GUD_INTERNAL_H
+#define __LINUX_GUD_INTERNAL_H
+
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/usb.h>
+#include <linux/workqueue.h>
+#include <uapi/drm/drm_fourcc.h>
+
+#include <drm/drm_modes.h>
+#include <drm/drm_simple_kms_helper.h>
+
+struct gud_device {
+ struct drm_device drm;
+ struct drm_simple_display_pipe pipe;
+ struct device *dmadev;
+ struct work_struct work;
+ u32 flags;
+ 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 get/set and status transfers */
+
+ struct mutex damage_lock; /* Protects the following members: */
+ struct drm_framebuffer *fb;
+ struct drm_rect damage;
+ bool prev_flush_failed;
+};
+
+static inline struct gud_device *to_gud_device(struct drm_device *drm)
+{
+ return container_of(drm, struct gud_device, drm);
+}
+
+static inline struct usb_device *gud_to_usb_device(struct gud_device *gdrm)
+{
+ return interface_to_usbdev(to_usb_interface(gdrm->drm.dev));
+}
+
+int gud_usb_get(struct gud_device *gdrm, u8 request, u16 index, void *buf, size_t len);
+int gud_usb_set(struct gud_device *gdrm, u8 request, u16 index, void *buf, size_t len);
+int gud_usb_get_u8(struct gud_device *gdrm, u8 request, u16 index, u8 *val);
+int gud_usb_set_u8(struct gud_device *gdrm, u8 request, u8 val);
+
+void gud_clear_damage(struct gud_device *gdrm);
+void gud_flush_work(struct work_struct *work);
+int gud_pipe_check(struct drm_simple_display_pipe *pipe,
+ struct drm_plane_state *new_plane_state,
+ struct drm_crtc_state *new_crtc_state);
+void gud_pipe_update(struct drm_simple_display_pipe *pipe,
+ struct drm_plane_state *old_state);
+int gud_connector_fill_properties(struct drm_connector_state *connector_state,
+ struct gud_property_req *properties);
+int gud_get_connectors(struct gud_device *gdrm);
+
+/* Driver internal fourcc transfer formats */
+#define GUD_DRM_FORMAT_R1 0x00000122
+#define GUD_DRM_FORMAT_RGB111 0x03121722
+
+static inline u8 gud_from_fourcc(u32 fourcc)
+{
+ switch (fourcc) {
+ case GUD_DRM_FORMAT_R1:
+ return GUD_PIXEL_FORMAT_R1;
+ case GUD_DRM_FORMAT_RGB111:
+ return GUD_PIXEL_FORMAT_RGB111;
+ case DRM_FORMAT_RGB565:
+ return GUD_PIXEL_FORMAT_RGB565;
+ case DRM_FORMAT_XRGB8888:
+ return GUD_PIXEL_FORMAT_XRGB8888;
+ case DRM_FORMAT_ARGB8888:
+ return GUD_PIXEL_FORMAT_ARGB8888;
+ };
+
+ return 0;
+}
+
+static inline u32 gud_to_fourcc(u8 format)
+{
+ switch (format) {
+ case GUD_PIXEL_FORMAT_R1:
+ return GUD_DRM_FORMAT_R1;
+ case GUD_PIXEL_FORMAT_RGB111:
+ return GUD_DRM_FORMAT_RGB111;
+ case GUD_PIXEL_FORMAT_RGB565:
+ return DRM_FORMAT_RGB565;
+ case GUD_PIXEL_FORMAT_XRGB8888:
+ return DRM_FORMAT_XRGB8888;
+ case GUD_PIXEL_FORMAT_ARGB8888:
+ return DRM_FORMAT_ARGB8888;
+ };
+
+ return 0;
+}
+
+static inline void gud_from_display_mode(struct gud_display_mode_req *dst,
+ const struct drm_display_mode *src)
+{
+ u32 flags = src->flags & GUD_DISPLAY_MODE_FLAG_USER_MASK;
+
+ if (src->type & DRM_MODE_TYPE_PREFERRED)
+ flags |= GUD_DISPLAY_MODE_FLAG_PREFERRED;
+
+ 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->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->flags = cpu_to_le32(flags);
+}
+
+static inline void gud_to_display_mode(struct drm_display_mode *dst,
+ const struct gud_display_mode_req *src)
+{
+ u32 flags = le32_to_cpu(src->flags);
+
+ memset(dst, 0, sizeof(*dst));
+ 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->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->flags = flags & GUD_DISPLAY_MODE_FLAG_USER_MASK;
+ dst->type = DRM_MODE_TYPE_DRIVER;
+ if (flags & GUD_DISPLAY_MODE_FLAG_PREFERRED)
+ dst->type |= DRM_MODE_TYPE_PREFERRED;
+ drm_mode_set_name(dst);
+}
+
+#endif
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
new file mode 100644
index 000000000000..1b036c7d781e
--- /dev/null
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -0,0 +1,535 @@
+// 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.h>
+
+#include "gud_internal.h"
+
+static bool gud_is_big_endian(void)
+{
+#if defined(__BIG_ENDIAN)
+ return true;
+#else
+ return false;
+#endif
+}
+
+static size_t gud_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 0;
+
+ 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 size_t gud_xrgb8888_to_color(u8 *dst, const struct drm_format_info *format,
+ u32 *src, struct drm_framebuffer *fb,
+ struct drm_rect *rect)
+{
+ unsigned int block_width = drm_format_info_block_width(format, 0);
+ unsigned int x, y, width, height;
+ u8 r, g, b, *block = dst; /* Assign to silence compiler warning */
+ size_t len;
+
+ /* 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;
+
+ for (y = 0; y < height; y++) {
+ for (x = 0; x < width; x++) {
+ if (!(x % block_width)) {
+ block = dst++;
+ *block = 0;
+ }
+
+ r = *src >> 16;
+ g = *src >> 8;
+ b = *src++;
+
+ switch (format->format) {
+ case GUD_DRM_FORMAT_RGB111:
+ *block <<= 4;
+ *block |= ((r >> 7) << 2) | ((g >> 7) << 1) | (b >> 7);
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ return len;
+ };
+ }
+ }
+
+ return len;
+}
+
+static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
+ const struct drm_format_info *format, struct drm_rect *rect,
+ struct gud_set_buffer_req *req)
+{
+ struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach;
+ u8 compression = gdrm->compression;
+ struct dma_buf_map map;
+ void *vaddr, *buf;
+ size_t pitch, len;
+ int ret = 0;
+
+ 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;
+
+ ret = drm_gem_shmem_vmap(fb->obj[0], &map);
+ if (ret)
+ return ret;
+
+ vaddr = map.vaddr + fb->offsets[0];
+
+ if (import_attach) {
+ ret = dma_buf_begin_cpu_access(import_attach->dmabuf, DMA_FROM_DEVICE);
+ if (ret)
+ goto vunmap;
+ }
+retry:
+ if (compression)
+ 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_xrgb8888_to_r124(buf, format, vaddr, fb, rect);
+ if (!len) {
+ ret = -ENOMEM;
+ goto end_cpu_access;
+ }
+ } else if (format->format == DRM_FORMAT_RGB565) {
+ drm_fb_xrgb8888_to_rgb565(buf, vaddr, fb, rect, gud_is_big_endian());
+ } else {
+ len = gud_xrgb8888_to_color(buf, format, vaddr, fb, rect);
+ }
+ } else if (gud_is_big_endian() && format->cpp[0] > 1) {
+ drm_fb_swab(buf, vaddr, fb, rect, !import_attach);
+ } else if (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);
+ }
+
+ memset(req, 0, sizeof(*req));
+ 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);
+
+ if (compression & GUD_COMPRESSION_LZ4) {
+ int complen;
+
+ complen = LZ4_compress_default(buf, gdrm->bulk_buf, len, len, gdrm->lz4_comp_mem);
+ if (complen <= 0) {
+ compression = 0;
+ goto retry;
+ }
+
+ req->compression = GUD_COMPRESSION_LZ4;
+ req->compressed_length = cpu_to_le32(complen);
+ }
+
+end_cpu_access:
+ if (import_attach)
+ dma_buf_end_cpu_access(import_attach->dmabuf, DMA_FROM_DEVICE);
+vunmap:
+ drm_gem_shmem_vunmap(fb->obj[0], &map);
+
+ return ret;
+}
+
+static int gud_flush_rect(struct gud_device *gdrm, struct drm_framebuffer *fb,
+ const struct drm_format_info *format, struct drm_rect *rect)
+{
+ struct usb_device *usb = gud_to_usb_device(gdrm);
+ struct gud_set_buffer_req req;
+ int ret, actual_length;
+ size_t len, trlen;
+
+ drm_dbg(&gdrm->drm, "Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
+
+ ret = gud_prep_flush(gdrm, fb, format, rect, &req);
+ if (ret)
+ return ret;
+
+ len = le32_to_cpu(req.length);
+
+ if (req.compression)
+ trlen = le32_to_cpu(req.compressed_length);
+ else
+ trlen = len;
+
+ 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;
+
+ if (!(gdrm->flags & GUD_DISPLAY_FLAG_FULL_UPDATE) || gdrm->prev_flush_failed) {
+ ret = gud_usb_set(gdrm, GUD_REQ_SET_BUFFER, 0, &req, sizeof(req));
+ if (ret)
+ return ret;
+ }
+
+ ret = usb_bulk_msg(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++;
+
+ return ret;
+}
+
+void gud_clear_damage(struct gud_device *gdrm)
+{
+ gdrm->damage.x1 = INT_MAX;
+ gdrm->damage.y1 = INT_MAX;
+ gdrm->damage.x2 = 0;
+ gdrm->damage.y2 = 0;
+}
+
+static void gud_add_damage(struct gud_device *gdrm, struct drm_rect *damage)
+{
+ 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);
+}
+
+static void gud_retry_failed_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
+ struct drm_rect *damage)
+{
+ /*
+ * pipe_update waits for the worker when the display mode is going to change.
+ * This ensures that the width and height is still the same making it safe to
+ * add back the damage.
+ */
+
+ mutex_lock(&gdrm->damage_lock);
+ if (!gdrm->fb) {
+ drm_framebuffer_get(fb);
+ gdrm->fb = fb;
+ }
+ gud_add_damage(gdrm, damage);
+ mutex_unlock(&gdrm->damage_lock);
+
+ /* Retry only once to avoid a possible storm in case of continues errors. */
+ if (!gdrm->prev_flush_failed)
+ queue_work(system_long_wq, &gdrm->work);
+ gdrm->prev_flush_failed = true;
+}
+
+void gud_flush_work(struct work_struct *work)
+{
+ struct gud_device *gdrm = container_of(work, struct gud_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_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_flush_rect(gdrm, fb, format, &rect);
+ if (ret) {
+ if (ret != -ENODEV && ret != -ECONNRESET &&
+ ret != -ESHUTDOWN && ret != -EPROTO) {
+ bool prev_flush_failed = gdrm->prev_flush_failed;
+
+ gud_retry_failed_flush(gdrm, fb, &damage);
+ if (!prev_flush_failed)
+ dev_err_ratelimited(fb->dev->dev,
+ "Failed to flush framebuffer: error=%d\n", ret);
+ }
+ break;
+ }
+
+ gdrm->prev_flush_failed = false;
+ }
+
+ drm_framebuffer_put(fb);
+out:
+ drm_dev_exit(idx);
+}
+
+static void gud_fb_queue_damage(struct gud_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;
+ }
+
+ gud_add_damage(gdrm, damage);
+
+ mutex_unlock(&gdrm->damage_lock);
+
+ queue_work(system_long_wq, &gdrm->work);
+
+ if (old_fb)
+ drm_framebuffer_put(old_fb);
+}
+
+int gud_pipe_check(struct drm_simple_display_pipe *pipe,
+ struct drm_plane_state *new_plane_state,
+ struct drm_crtc_state *new_crtc_state)
+{
+ struct gud_device *gdrm = to_gud_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 drm_connector *connector;
+ unsigned int i, num_properties;
+ struct gud_state_req *req;
+ int idx, ret;
+ 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) {
+ if (connector_state->crtc)
+ break;
+ }
+
+ /*
+ * DRM_IOCTL_MODE_OBJ_SETPROPERTY on the rotation property will not have
+ * the connector included in the state.
+ */
+ if (!connector_state) {
+ struct drm_connector_list_iter conn_iter;
+
+ drm_connector_list_iter_begin(pipe->crtc.dev, &conn_iter);
+ drm_for_each_connector_iter(connector, &conn_iter) {
+ if (connector->state->crtc) {
+ connector_state = connector->state;
+ break;
+ }
+ }
+ drm_connector_list_iter_end(&conn_iter);
+ }
+
+ if (WARN_ON_ONCE(!connector_state))
+ return -ENOENT;
+
+ len = struct_size(req, properties,
+ GUD_PROPERTIES_MAX_NUM + GUD_CONNECTOR_PROPERTIES_MAX_NUM);
+ req = kzalloc(len, GFP_KERNEL);
+ if (!req)
+ return -ENOMEM;
+
+ gud_from_display_mode(&req->mode, mode);
+
+ req->format = gud_from_fourcc(format->format);
+ if (WARN_ON_ONCE(!req->format)) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ req->connector = drm_connector_index(connector_state->connector);
+
+ ret = gud_connector_fill_properties(connector_state, req->properties);
+ if (ret < 0)
+ goto out;
+
+ num_properties = ret;
+ for (i = 0; i < gdrm->num_properties; i++) {
+ u16 prop = gdrm->properties[i];
+ u64 val;
+
+ switch (prop) {
+ case GUD_PROPERTY_ROTATION:
+ /* DRM UAPI matches the protocol so use value directly */
+ 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);
+ num_properties++;
+ }
+
+ if (drm_dev_enter(fb->dev, &idx)) {
+ len = struct_size(req, properties, num_properties);
+ ret = gud_usb_set(gdrm, GUD_REQ_SET_STATE_CHECK, 0, req, len);
+ drm_dev_exit(idx);
+ } else {
+ ret = -ENODEV;
+ }
+out:
+ kfree(req);
+
+ return ret;
+}
+
+void gud_pipe_update(struct drm_simple_display_pipe *pipe,
+ struct drm_plane_state *old_state)
+{
+ struct drm_device *drm = pipe->crtc.dev;
+ struct gud_device *gdrm = to_gud_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 (crtc->state->mode_changed || !crtc->state->enable) {
+ cancel_work_sync(&gdrm->work);
+ mutex_lock(&gdrm->damage_lock);
+ if (gdrm->fb) {
+ drm_framebuffer_put(gdrm->fb);
+ gdrm->fb = NULL;
+ }
+ gud_clear_damage(gdrm);
+ mutex_unlock(&gdrm->damage_lock);
+ }
+
+ if (!drm_dev_enter(drm, &idx))
+ return;
+
+ if (!old_state->fb)
+ gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 1);
+
+ if (fb && (crtc->state->mode_changed || crtc->state->connectors_changed))
+ gud_usb_set(gdrm, GUD_REQ_SET_STATE_COMMIT, 0, NULL, 0);
+
+ if (crtc->state->active_changed)
+ gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, crtc->state->active);
+
+ if (drm_atomic_helper_damage_merged(old_state, state, &damage)) {
+ if (gdrm->flags & GUD_DISPLAY_FLAG_FULL_UPDATE)
+ drm_rect_init(&damage, 0, 0, fb->width, fb->height);
+ gud_fb_queue_damage(gdrm, fb, &damage);
+ }
+
+ if (!crtc->state->enable)
+ gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 0);
+
+ drm_dev_exit(idx);
+}
diff --git a/include/drm/gud.h b/include/drm/gud.h
new file mode 100644
index 000000000000..a078aad63a31
--- /dev/null
+++ b/include/drm/gud.h
@@ -0,0 +1,333 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright 2020 Noralf Trønnes
+ */
+
+#ifndef __LINUX_GUD_H
+#define __LINUX_GUD_H
+
+#include <linux/types.h>
+
+/*
+ * struct gud_display_descriptor_req - Display descriptor
+ * @magic: Magic value GUD_DISPLAY_MAGIC
+ * @version: Protocol version
+ * @flags: Flags
+ * - STATUS_ON_SET: Always do a status request after a SET request.
+ * This is used by the Linux gadget driver since it has
+ * no way to control the status stage of a control OUT
+ * request that has a payload.
+ * - FULL_UPDATE: Always send the entire framebuffer when flushing changes.
+ * The GUD_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 gives the device
+ * a chance to reset its state machine if needed.
+ * This flag can not be used in combination with compression.
+ * @compression: Supported compression types
+ * - GUD_COMPRESSION_LZ4: LZ4 lossless compression.
+ * @max_buffer_size: Maximum buffer size the device can handle (optional).
+ * This is useful for devices that don't have a big enough
+ * buffer to decompress the entire framebuffer in one go.
+ * @min_width: Minimum pixel width the controller can handle
+ * @max_width: Maximum width
+ * @min_height: Minimum height
+ * @max_height: Maximum height
+ *
+ * Devices that have only one display mode will have min_width == max_width
+ * and min_height == max_height.
+ */
+struct gud_display_descriptor_req {
+ __le32 magic;
+#define GUD_DISPLAY_MAGIC 0x1d50614d
+ __u8 version;
+ __le32 flags;
+#define GUD_DISPLAY_FLAG_STATUS_ON_SET BIT(0)
+#define GUD_DISPLAY_FLAG_FULL_UPDATE BIT(1)
+ __u8 compression;
+#define GUD_COMPRESSION_LZ4 BIT(0)
+ __le32 max_buffer_size;
+ __le32 min_width;
+ __le32 max_width;
+ __le32 min_height;
+ __le32 max_height;
+} __packed;
+
+/*
+ * struct gud_property_req - Property
+ * @prop: Property
+ * @val: Value
+ */
+struct gud_property_req {
+ __le16 prop;
+ __le64 val;
+} __packed;
+
+/*
+ * struct gud_display_mode_req - Display mode
+ * @clock: Pixel clock in kHz
+ * @hdisplay: Horizontal display size
+ * @hsync_start: Horizontal sync start
+ * @hsync_end: Horizontal sync end
+ * @htotal: Horizontal total size
+ * @vdisplay: Vertical display size
+ * @vsync_start: Vertical sync start
+ * @vsync_end: Vertical sync end
+ * @vtotal: Vertical total size
+ * @flags: Bits 0-13 are the same as in the RandR protocol and also what DRM uses.
+ * The deprecated bits are reused for internal protocol flags leaving us
+ * free to follow DRM for the other bits in the future.
+ * - FLAG_PREFERRED: Set on the preferred display mode.
+ */
+struct gud_display_mode_req {
+ __le32 clock;
+ __le16 hdisplay;
+ __le16 hsync_start;
+ __le16 hsync_end;
+ __le16 htotal;
+ __le16 vdisplay;
+ __le16 vsync_start;
+ __le16 vsync_end;
+ __le16 vtotal;
+ __le32 flags;
+#define GUD_DISPLAY_MODE_FLAG_PHSYNC BIT(0)
+#define GUD_DISPLAY_MODE_FLAG_NHSYNC BIT(1)
+#define GUD_DISPLAY_MODE_FLAG_PVSYNC BIT(2)
+#define GUD_DISPLAY_MODE_FLAG_NVSYNC BIT(3)
+#define GUD_DISPLAY_MODE_FLAG_INTERLACE BIT(4)
+#define GUD_DISPLAY_MODE_FLAG_DBLSCAN BIT(5)
+#define GUD_DISPLAY_MODE_FLAG_CSYNC BIT(6)
+#define GUD_DISPLAY_MODE_FLAG_PCSYNC BIT(7)
+#define GUD_DISPLAY_MODE_FLAG_NCSYNC BIT(8)
+#define GUD_DISPLAY_MODE_FLAG_HSKEW BIT(9)
+/* BCast and PixelMultiplex are deprecated */
+#define GUD_DISPLAY_MODE_FLAG_DBLCLK BIT(12)
+#define GUD_DISPLAY_MODE_FLAG_CLKDIV2 BIT(13)
+#define GUD_DISPLAY_MODE_FLAG_USER_MASK \
+ (GUD_DISPLAY_MODE_FLAG_PHSYNC | GUD_DISPLAY_MODE_FLAG_NHSYNC | \
+ GUD_DISPLAY_MODE_FLAG_PVSYNC | GUD_DISPLAY_MODE_FLAG_NVSYNC | \
+ GUD_DISPLAY_MODE_FLAG_INTERLACE | GUD_DISPLAY_MODE_FLAG_DBLSCAN | \
+ GUD_DISPLAY_MODE_FLAG_CSYNC | GUD_DISPLAY_MODE_FLAG_PCSYNC | \
+ GUD_DISPLAY_MODE_FLAG_NCSYNC | GUD_DISPLAY_MODE_FLAG_HSKEW | \
+ GUD_DISPLAY_MODE_FLAG_DBLCLK | GUD_DISPLAY_MODE_FLAG_CLKDIV2)
+/* Internal protocol flags */
+#define GUD_DISPLAY_MODE_FLAG_PREFERRED BIT(10)
+} __packed;
+
+/*
+ * struct gud_connector_descriptor_req - Connector descriptor
+ * @connector_type: Connector type (GUD_CONNECTOR_TYPE_*).
+ * If the host doesn't support the type it should fall back to PANEL.
+ * @flags: Flags
+ * - POLL_STATUS: Connector status can change (polled every 10 seconds)
+ * - INTERLACE: Interlaced modes are supported
+ * - DOUBLESCAN: Doublescan modes are supported
+ */
+struct gud_connector_descriptor_req {
+ __u8 connector_type;
+#define GUD_CONNECTOR_TYPE_PANEL 0
+#define GUD_CONNECTOR_TYPE_VGA 1
+#define GUD_CONNECTOR_TYPE_COMPOSITE 2
+#define GUD_CONNECTOR_TYPE_SVIDEO 3
+#define GUD_CONNECTOR_TYPE_COMPONENT 4
+#define GUD_CONNECTOR_TYPE_DVI 5
+#define GUD_CONNECTOR_TYPE_DISPLAYPORT 6
+#define GUD_CONNECTOR_TYPE_HDMI 7
+ __le32 flags;
+#define GUD_CONNECTOR_FLAGS_POLL_STATUS BIT(0)
+#define GUD_CONNECTOR_FLAGS_INTERLACE BIT(1)
+#define GUD_CONNECTOR_FLAGS_DOUBLESCAN BIT(2)
+} __packed;
+
+/*
+ * struct gud_set_buffer_req - 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
+ *
+ * This request is issued right before the bulk transfer.
+ * @x, @y, @width and @height specifies the rectangle where the buffer should be
+ * placed inside the framebuffer.
+ */
+struct gud_set_buffer_req {
+ __le32 x;
+ __le32 y;
+ __le32 width;
+ __le32 height;
+ __le32 length;
+ __u8 compression;
+ __le32 compressed_length;
+} __packed;
+
+/*
+ * struct gud_state_req - Display state
+ * @mode: Display mode
+ * @format: Pixel format GUD_PIXEL_FORMAT_*
+ * @connector: Connector index
+ * @properties: Array of properties
+ *
+ * The entire state is transferred each time there's a change.
+ */
+struct gud_state_req {
+ struct gud_display_mode_req mode;
+ __u8 format;
+ __u8 connector;
+ struct gud_property_req properties[];
+} __packed;
+
+/* List of supported connector properties: */
+
+/* Margins in pixels to deal with overscan, range 0-100 */
+#define GUD_PROPERTY_TV_LEFT_MARGIN 1
+#define GUD_PROPERTY_TV_RIGHT_MARGIN 2
+#define GUD_PROPERTY_TV_TOP_MARGIN 3
+#define GUD_PROPERTY_TV_BOTTOM_MARGIN 4
+#define GUD_PROPERTY_TV_MODE 5
+/* Brightness in percent, range 0-100 */
+#define GUD_PROPERTY_TV_BRIGHTNESS 6
+/* Contrast in percent, range 0-100 */
+#define GUD_PROPERTY_TV_CONTRAST 7
+/* Flicker reduction in percent, range 0-100 */
+#define GUD_PROPERTY_TV_FLICKER_REDUCTION 8
+/* Overscan in percent, range 0-100 */
+#define GUD_PROPERTY_TV_OVERSCAN 9
+/* Saturation in percent, range 0-100 */
+#define GUD_PROPERTY_TV_SATURATION 10
+/* Hue in percent, range 0-100 */
+#define GUD_PROPERTY_TV_HUE 11
+
+/*
+ * 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_REQ_SET_DISPLAY_ENABLE request.
+ *
+ * This does not map to a DRM property, it is used with the backlight device.
+ */
+#define GUD_PROPERTY_BACKLIGHT_BRIGHTNESS 12
+
+/* List of supported properties that are not connector propeties: */
+
+/*
+ * Plane rotation. Should return the supported bitmask on
+ * GUD_REQ_GET_PROPERTIES. GUD_ROTATION_0 is mandatory.
+ *
+ * Note: This is not display rotation so 90/270 will need scaling to make it fit (unless squared).
+ */
+#define GUD_PROPERTY_ROTATION 50
+ #define GUD_ROTATION_0 BIT(0)
+ #define GUD_ROTATION_90 BIT(1)
+ #define GUD_ROTATION_180 BIT(2)
+ #define GUD_ROTATION_270 BIT(3)
+ #define GUD_ROTATION_REFLECT_X BIT(4)
+ #define GUD_ROTATION_REFLECT_Y BIT(5)
+ #define GUD_ROTATION_MASK (GUD_ROTATION_0 | GUD_ROTATION_90 | \
+ GUD_ROTATION_180 | GUD_ROTATION_270 | \
+ GUD_ROTATION_REFLECT_X | GUD_ROTATION_REFLECT_Y)
+
+/* USB Control requests: */
+
+/* Get status from the last GET/SET control request. Value is u8. */
+#define GUD_REQ_GET_STATUS 0x00
+ /* Status values: */
+ #define GUD_STATUS_OK 0x00
+ #define GUD_STATUS_BUSY 0x01
+ #define GUD_STATUS_REQUEST_NOT_SUPPORTED 0x02
+ #define GUD_STATUS_PROTOCOL_ERROR 0x03
+ #define GUD_STATUS_INVALID_PARAMETER 0x04
+ #define GUD_STATUS_ERROR 0x05
+
+/* Get display descriptor as a &gud_display_descriptor_req */
+#define GUD_REQ_GET_DESCRIPTOR 0x01
+
+/* Get supported pixel formats as a byte array of GUD_PIXEL_FORMAT_* */
+#define GUD_REQ_GET_FORMATS 0x40
+ #define GUD_FORMATS_MAX_NUM 32
+ /* R1 is a 1-bit monochrome transfer format presented to userspace as XRGB8888 */
+ #define GUD_PIXEL_FORMAT_R1 0x01
+ #define GUD_PIXEL_FORMAT_RGB111 0x20
+ #define GUD_PIXEL_FORMAT_RGB565 0x40
+ #define GUD_PIXEL_FORMAT_XRGB8888 0x80
+ #define GUD_PIXEL_FORMAT_ARGB8888 0x81
+
+/*
+ * Get supported properties that are not connector propeties as a &gud_property_req array.
+ * gud_property_req.val often contains the initial value for the property.
+ */
+#define GUD_REQ_GET_PROPERTIES 0x41
+ #define GUD_PROPERTIES_MAX_NUM 32
+
+/* Connector requests have the connector index passed in the wValue field */
+
+/* Get connector descriptors as an array of &gud_connector_descriptor_req */
+#define GUD_REQ_GET_CONNECTORS 0x50
+ #define GUD_CONNECTORS_MAX_NUM 32
+
+/*
+ * Get properties supported by the connector as a &gud_property_req array.
+ * gud_property_req.val often contains the initial value for the property.
+ */
+#define GUD_REQ_GET_CONNECTOR_PROPERTIES 0x51
+ #define GUD_CONNECTOR_PROPERTIES_MAX_NUM 32
+
+/*
+ * Issued when there's a TV_MODE property present.
+ * Gets an array of the supported TV_MODE names each entry of length
+ * GUD_CONNECTOR_TV_MODE_NAME_LEN. Names must be NUL-terminated.
+ */
+#define GUD_REQ_GET_CONNECTOR_TV_MODE_VALUES 0x52
+ #define GUD_CONNECTOR_TV_MODE_NAME_LEN 16
+ #define GUD_CONNECTOR_TV_MODE_MAX_NUM 16
+
+/* When userspace checks connector status, this is issued first, not used for poll requests. */
+#define GUD_REQ_SET_CONNECTOR_FORCE_DETECT 0x53
+
+/*
+ * Get connector status. Value is u8.
+ *
+ * Userspace will get a HOTPLUG uevent if one of the following is true:
+ * - Connection status has changed since last
+ * - CHANGED is set
+ */
+#define GUD_REQ_GET_CONNECTOR_STATUS 0x54
+ #define GUD_CONNECTOR_STATUS_DISCONNECTED 0x00
+ #define GUD_CONNECTOR_STATUS_CONNECTED 0x01
+ #define GUD_CONNECTOR_STATUS_UNKNOWN 0x02
+ #define GUD_CONNECTOR_STATUS_CONNECTED_MASK 0x03
+ #define GUD_CONNECTOR_STATUS_CHANGED BIT(7)
+
+/*
+ * Display modes can be fetched as either EDID data or an array of &gud_display_mode_req.
+ *
+ * If GUD_REQ_GET_CONNECTOR_MODES returns zero, EDID is used to create display modes.
+ * If both display modes and EDID are returned, EDID is just passed on to userspace
+ * in the EDID connector property.
+ */
+
+/* Get &gud_display_mode_req array of supported display modes */
+#define GUD_REQ_GET_CONNECTOR_MODES 0x55
+ #define GUD_CONNECTOR_MAX_NUM_MODES 128
+
+/* Get Extended Display Identification Data */
+#define GUD_REQ_GET_CONNECTOR_EDID 0x56
+ #define GUD_CONNECTOR_MAX_EDID_LEN 2048
+
+/* Set buffer properties before bulk transfer as &gud_set_buffer_req */
+#define GUD_REQ_SET_BUFFER 0x60
+
+/* Check display configuration as &gud_state_req */
+#define GUD_REQ_SET_STATE_CHECK 0x61
+
+/* Apply the previous STATE_CHECK configuration */
+#define GUD_REQ_SET_STATE_COMMIT 0x62
+
+/* Enable/disable the display controller, value is u8: 0/1 */
+#define GUD_REQ_SET_CONTROLLER_ENABLE 0x63
+
+/* Enable/disable display/output (DPMS), value is u8: 0/1 */
+#define GUD_REQ_SET_DISPLAY_ENABLE 0x64
+
+#endif
--
2.23.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
2021-03-05 16:31 ` [PATCH v7 3/3] drm: Add GUD USB Display driver Noralf Trønnes
@ 2021-03-08 18:13 ` Noralf Trønnes
2021-03-09 14:02 ` Peter Stuge
1 sibling, 0 replies; 18+ messages in thread
From: Noralf Trønnes @ 2021-03-08 18:13 UTC (permalink / raw)
To: dri-devel
Cc: linux-usb, sam, peter, lkundrak, markus, pontus.fuchs, hudson,
th020394, Daniel Vetter
Den 05.03.2021 17.31, skrev Noralf Trønnes:
> This adds a 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.
>
> diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
> +static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
> +{
> + num_formats_dev = ret;
> + for (i = 0; i < num_formats_dev; i++) {
> + const struct drm_format_info *info;
> + size_t fmt_buf_size;
> + u32 format;
> +
> + format = gud_to_fourcc(formats_dev[i]);
> + if (!format) {
> + drm_dbg(drm, "Unsupported format: 0x%02x\n", formats_dev[i]);
> + continue;
> + }
> +
> + if (format == GUD_DRM_FORMAT_R1)
> + info = &gud_drm_format_r1;
> + else if (format == GUD_DRM_FORMAT_RGB111)
> + info = &gud_drm_format_rgb111;
> + else
> + info = drm_format_info(format);
> +
> + switch (format) {
> + case GUD_DRM_FORMAT_R1:
> + fallthrough;
> + case GUD_DRM_FORMAT_RGB111:
> + xrgb8888_emulation_format = info;
> + break;
> + case DRM_FORMAT_RGB565:
> + rgb565_supported = true;
> + if (!xrgb8888_emulation_format)
> + xrgb8888_emulation_format = info;
> + break;
> + case DRM_FORMAT_XRGB8888:
> + xrgb8888_supported = true;
> + break;
> + };
> +
> + fmt_buf_size = drm_format_info_min_pitch(info, 0, drm->mode_config.max_width) *
> + drm->mode_config.max_height;
> + max_buffer_size = max(max_buffer_size, fmt_buf_size);
> +
> + if (format == GUD_DRM_FORMAT_R1)
This line should be:
if (format == GUD_DRM_FORMAT_R1 || format == GUD_DRM_FORMAT_RGB111)
There's also a bug in the format conversion functions that shows up on
transfers with widths that are not pixels per byte aligned. I've fixed
R1, I'll look at RGB111 tomorrow.
Noralf.
> + continue; /* Internal not for userspace */
> +
> + formats[num_formats++] = format;
> + }
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
2021-03-05 16:31 ` [PATCH v7 3/3] drm: Add GUD USB Display driver Noralf Trønnes
2021-03-08 18:13 ` Noralf Trønnes
@ 2021-03-09 14:02 ` Peter Stuge
2021-03-09 18:07 ` Noralf Trønnes
1 sibling, 1 reply; 18+ messages in thread
From: Peter Stuge @ 2021-03-09 14:02 UTC (permalink / raw)
To: Noralf Trønnes
Cc: dri-devel, hudson, markus, sam, linux-usb, th020394, lkundrak,
pontus.fuchs, Daniel Vetter
Hello Noralf,
I've made some progress with my test device. I'm implementing R1
first and once that works I'll test RGB111 as well. Along the way
I've found a couple of things in the code:
Noralf Trønnes wrote:
> +++ b/drivers/gpu/drm/gud/gud_drv.c
..
> +static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
..
> + if (format == GUD_DRM_FORMAT_R1)
> + continue; /* Internal not for userspace */
You already found RGB111 missing here.
> +static int gud_usb_control_msg(struct usb_interface *intf, bool in,
> + u8 request, u16 value, void *buf, size_t len)
..
> +static int gud_usb_transfer(struct gud_device *gdrm, bool in, u8 request, u16 index,
..
> + ret = gud_usb_control_msg(intf, in, request, index, buf, len);
The u16 index parameter to gud_usb_transfer() and at least also
gud_usb_{get,set,get_u8,set_u8}() is eventually passed in u16 value
in the call to gud_usb_control_msg(), which had me confused for a bit.
What do you think about renaming all of those parameters to wValue,
to show that and where they are part of the control request? I think
it would help make the protocol more clear.
Finally, an actual bug:
> + ret = gud_get_properties(gdrm);
> + if (ret) {
> + dev_err(dev, "Failed to get properties (error=%d)\n", ret);
> + return ret;
> + }
If gud_get_properties() and gud_connector_add_properties() receive
and process (only!) one or more unknown properties then they return
the number of bytes received from the device rather than 0.
I fixed this by setting ret = 0; before the for() loop, but maybe you
want to do it another way.
I found this because I can't get my device to send 0 bytes IN when
the host requests more, if I provide no data the request STALLs. This
is for sure a bug in my device and I'll come back to it, but for now
I added a dummy 65535 property as a workaround.
What do you think about formalizing this, adding an actual dummy property?
Or maybe adding flags to the display descriptor for "I have properties",
"I have connector properties" and "I have EDID" ?
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
..
> +static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
..
> + /*
> + * 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_xrgb8888_to_r124(buf, format, vaddr, fb, rect);
> + if (!len) {
> + ret = -ENOMEM;
> + goto end_cpu_access;
> + }
> + } else if (format->format == DRM_FORMAT_RGB565) {
> + drm_fb_xrgb8888_to_rgb565(buf, vaddr, fb, rect, gud_is_big_endian());
> + } else {
> + len = gud_xrgb8888_to_color(buf, format, vaddr, fb, rect);
> + }
Does this section also need a RGB111 case?
> +void gud_pipe_update(struct drm_simple_display_pipe *pipe,
..
> + if (fb && (crtc->state->mode_changed || crtc->state->connectors_changed))
> + gud_usb_set(gdrm, GUD_REQ_SET_STATE_COMMIT, 0, NULL, 0);
You mentioned that commit must not fail; what happens/should happen
if a request does fail in pipe_update()? Some reasons could be that
the device was unplugged, a bad cable is glitchy or even that some
device doesn't even implement STATE_COMMIT or does it incorrectly and
will report back failure?
> +++ b/include/drm/gud.h
..
> + #define GUD_STATUS_REQUEST_NOT_SUPPORTED 0x02
Maybe this can be removed? SET_VERSION has been removed so it's no
longer used anywhere, and in any case devices typically signal that
requests are unsupported using a protocol STALL, which comes back
as -EPIPE from the USB stack.
Finally, here's the drm debug output when I connect my device:
Mar 09 14:57:19 vm kernel: usb 1-1: new full-speed USB device number 24 using uhci_hcd
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_probe] version=1 flags=0x2 compression=0x0 max_buffer_size=0
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get: request=0x40 index=0 len=32
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get: request=0x41 index=0 len=320
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_probe] Ignoring unknown property: 65535
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get: request=0x50 index=0 len=256
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_get_connectors] Connector: index=0 type=0 flags=0x0
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get: request=0x51 index=0 len=320
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_get_connectors] property: 65535 = 0(0x0)
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_get_connectors] Ignoring unknown property: 65535
Mar 09 14:57:19 vm kernel: [drm:drm_minor_register]
Mar 09 14:57:19 vm kernel: [drm:drm_minor_register]
Mar 09 14:57:19 vm kernel: [drm:drm_minor_register] new minor registered 0
Mar 09 14:57:19 vm kernel: [drm:drm_sysfs_connector_add] adding "USB-1" to sysfs
Mar 09 14:57:19 vm kernel: [drm:drm_sysfs_hotplug_event] generating hotplug event
Mar 09 14:57:19 vm kernel: [drm] Initialized gud 1.0.0 20200422 for 1-1:27.0 on minor 0
Mar 09 14:57:19 vm kernel: [drm:drm_client_modeset_probe]
Mar 09 14:57:19 vm kernel: [drm:drm_mode_object_get] OBJ ID: 35 (2)
Mar 09 14:57:19 vm kernel: [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:35:USB-1]
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] set: request=0x53 index=0 len=0
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get: request=0x54 index=0 len=1
Mar 09 14:57:19 vm kernel: [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:35:USB-1] status updated from unknown to connected
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get: request=0x56 index=0 len=2048
Mar 09 14:57:19 vm kernel: [drm:drm_sysfs_hotplug_event] generating hotplug event
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: USB-1: Invalid EDID size (ret=1)
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get: request=0x55 index=0 len=3072
Mar 09 14:57:19 vm kernel: [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:35:USB-1] probed modes :
Mar 09 14:57:19 vm kernel: [drm:drm_mode_debug_printmodeline] Modeline "400x240": 104 10000 400 400 400 400 240 240 240 240 0x40 0x0
Mar 09 14:57:19 vm kernel: [drm:drm_client_modeset_probe] connector 35 enabled? yes
Mar 09 14:57:19 vm kernel: [drm:drm_client_modeset_probe] Not using firmware configuration
Mar 09 14:57:19 vm kernel: [drm:drm_client_modeset_probe] looking for cmdline mode on connector 35
Mar 09 14:57:19 vm kernel: [drm:drm_client_modeset_probe] looking for preferred mode on connector 35 0
Mar 09 14:57:19 vm kernel: [drm:drm_client_modeset_probe] found mode 400x240
Mar 09 14:57:19 vm kernel: [drm:drm_client_modeset_probe] picking CRTCs for 400x240 config
Mar 09 14:57:19 vm kernel: [drm:drm_client_modeset_probe] desired mode 400x240 set on crtc 33 (0,0)
Mar 09 14:57:19 vm kernel: [drm:drm_mode_object_get] OBJ ID: 35 (2)
Mar 09 14:57:19 vm kernel: [drm:drm_mode_object_put.part.0] OBJ ID: 35 (3)
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:__drm_fb_helper_initial_config_and_unlock] test CRTC 0 primary plane
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:drm_fb_helper_generic_probe] surface width(400), height(240) and bpp(32)
Mar 09 14:57:19 vm kernel: [drm:drm_mode_addfb2] [FB:36]
Mar 09 14:57:19 vm kernel: [drm:drm_mode_object_put.part.0] OBJ ID: 36 (2)
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm] fb0: guddrmfb frame buffer device
It looks good, I think? However, neither GUD_REQ_SET_CONTROLLER_ENABLE
nor GUD_REQ_SET_DISPLAY_ENABLE is ever called, and there is no bulk
output if I write to /dev/fb0.
Can you tell what's missing?
Many thanks and kind regards
//Peter
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
2021-03-09 14:02 ` Peter Stuge
@ 2021-03-09 18:07 ` Noralf Trønnes
2021-03-09 18:16 ` Noralf Trønnes
2021-03-10 4:55 ` Peter Stuge
0 siblings, 2 replies; 18+ messages in thread
From: Noralf Trønnes @ 2021-03-09 18:07 UTC (permalink / raw)
To: Peter Stuge
Cc: dri-devel, hudson, markus, sam, linux-usb, th020394, lkundrak,
pontus.fuchs, Daniel Vetter
Den 09.03.2021 15.02, skrev Peter Stuge:
> Hello Noralf,
>
> I've made some progress with my test device. I'm implementing R1
> first and once that works I'll test RGB111 as well. Along the way
> I've found a couple of things in the code:
>
> Noralf Trønnes wrote:
>> +++ b/drivers/gpu/drm/gud/gud_drv.c
> ..
>> +static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
> ..
>> + if (format == GUD_DRM_FORMAT_R1)
>> + continue; /* Internal not for userspace */
>
> You already found RGB111 missing here.
>
>
>> +static int gud_usb_control_msg(struct usb_interface *intf, bool in,
>> + u8 request, u16 value, void *buf, size_t len)
> ..
>> +static int gud_usb_transfer(struct gud_device *gdrm, bool in, u8 request, u16 index,
> ..
>> + ret = gud_usb_control_msg(intf, in, request, index, buf, len);
>
> The u16 index parameter to gud_usb_transfer() and at least also
> gud_usb_{get,set,get_u8,set_u8}() is eventually passed in u16 value
> in the call to gud_usb_control_msg(), which had me confused for a bit.
>
> What do you think about renaming all of those parameters to wValue,
> to show that and where they are part of the control request? I think
> it would help make the protocol more clear.
>
Only connector requests use this value and in that case it's the
connector index. I need to get this driver applied now, and can't spend
more time polishing it, so I'll just keep it as-is.
>
> Finally, an actual bug:
>
>> + ret = gud_get_properties(gdrm);
>> + if (ret) {
>> + dev_err(dev, "Failed to get properties (error=%d)\n", ret);
>> + return ret;
>> + }
>
> If gud_get_properties() and gud_connector_add_properties() receive
> and process (only!) one or more unknown properties then they return
> the number of bytes received from the device rather than 0.
>
> I fixed this by setting ret = 0; before the for() loop, but maybe you
> want to do it another way.
>
I'll do that.
>
> I found this because I can't get my device to send 0 bytes IN when
> the host requests more, if I provide no data the request STALLs. This
> is for sure a bug in my device and I'll come back to it, but for now
> I added a dummy 65535 property as a workaround.
>
> What do you think about formalizing this, adding an actual dummy property?
>
I want to avoid that. I'd rather add a descriptor flag
GUD_DISPLAY_FLAG_CANT_RETURN_ZERO or something for such devices.
Also this property warning is just a debug message so not a very big
problem. Worse is EDID if you don't want to support that since it prints
an error in that case.
> Or maybe adding flags to the display descriptor for "I have properties",
> "I have connector properties" and "I have EDID" ?
>
Then I might as well go back to the previous version and have count in
the structs won't I ;)
>
>> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> ..
>> +static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
> ..
>> + /*
>> + * 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_xrgb8888_to_r124(buf, format, vaddr, fb, rect);
>> + if (!len) {
>> + ret = -ENOMEM;
>> + goto end_cpu_access;
>> + }
>> + } else if (format->format == DRM_FORMAT_RGB565) {
>> + drm_fb_xrgb8888_to_rgb565(buf, vaddr, fb, rect, gud_is_big_endian());
>> + } else {
>> + len = gud_xrgb8888_to_color(buf, format, vaddr, fb, rect);
>> + }
>
> Does this section also need a RGB111 case?
>
gud_xrgb8888_to_color() handles that, although broken in this version
for partial updates. Here's the fix:
https://gist.github.com/notro/a94d381cf98b7e15fcddbc90e4af0a21
>
>> +void gud_pipe_update(struct drm_simple_display_pipe *pipe,
> ..
>> + if (fb && (crtc->state->mode_changed || crtc->state->connectors_changed))
>> + gud_usb_set(gdrm, GUD_REQ_SET_STATE_COMMIT, 0, NULL, 0);
>
> You mentioned that commit must not fail; what happens/should happen
> if a request does fail in pipe_update()? Some reasons could be that
> the device was unplugged, a bad cable is glitchy or even that some
> device doesn't even implement STATE_COMMIT or does it incorrectly and
> will report back failure?
>
The failure is just ignored since there's nothing to be done. The error
counter for the debufgs file is increased, making it somewhat visible.
I've chosen to only print probe errors, flush errors and devices
returning incorrect reply lengths. I hate drivers that fill the logs
with errors, but I might have gone to far on the silent side, I don't
know. But one nice thing about DRM is that debug is always builtin and
can be enabled.
>
>> +++ b/include/drm/gud.h
> ..
>> + #define GUD_STATUS_REQUEST_NOT_SUPPORTED 0x02
>
> Maybe this can be removed? SET_VERSION has been removed so it's no
> longer used anywhere, and in any case devices typically signal that
> requests are unsupported using a protocol STALL, which comes back
> as -EPIPE from the USB stack.
>
The driver issues a GUD_REQ_GET_STATUS on -EPIPE to find out what the
exact problem is. GUD_STATUS_REQUEST_NOT_SUPPORTED is returned by the
device on an unsupported/unrecognised request.
>
>
> Finally, here's the drm debug output when I connect my device:
>
> Mar 09 14:57:19 vm kernel: usb 1-1: new full-speed USB device number 24 using uhci_hcd
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_probe] version=1 flags=0x2 compression=0x0 max_buffer_size=0
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get: request=0x40 index=0 len=32
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get: request=0x41 index=0 len=320
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_probe] Ignoring unknown property: 65535
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get: request=0x50 index=0 len=256
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_get_connectors] Connector: index=0 type=0 flags=0x0
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get: request=0x51 index=0 len=320
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_get_connectors] property: 65535 = 0(0x0)
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_get_connectors] Ignoring unknown property: 65535
> Mar 09 14:57:19 vm kernel: [drm:drm_minor_register]
> Mar 09 14:57:19 vm kernel: [drm:drm_minor_register]
> Mar 09 14:57:19 vm kernel: [drm:drm_minor_register] new minor registered 0
> Mar 09 14:57:19 vm kernel: [drm:drm_sysfs_connector_add] adding "USB-1" to sysfs
> Mar 09 14:57:19 vm kernel: [drm:drm_sysfs_hotplug_event] generating hotplug event
> Mar 09 14:57:19 vm kernel: [drm] Initialized gud 1.0.0 20200422 for 1-1:27.0 on minor 0
> Mar 09 14:57:19 vm kernel: [drm:drm_client_modeset_probe]
> Mar 09 14:57:19 vm kernel: [drm:drm_mode_object_get] OBJ ID: 35 (2)
> Mar 09 14:57:19 vm kernel: [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:35:USB-1]
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] set: request=0x53 index=0 len=0
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get: request=0x54 index=0 len=1
> Mar 09 14:57:19 vm kernel: [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:35:USB-1] status updated from unknown to connected
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get: request=0x56 index=0 len=2048
> Mar 09 14:57:19 vm kernel: [drm:drm_sysfs_hotplug_event] generating hotplug event
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: USB-1: Invalid EDID size (ret=1)
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get: request=0x55 index=0 len=3072
> Mar 09 14:57:19 vm kernel: [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:35:USB-1] probed modes :
> Mar 09 14:57:19 vm kernel: [drm:drm_mode_debug_printmodeline] Modeline "400x240": 104 10000 400 400 400 400 240 240 240 240 0x40 0x0
> Mar 09 14:57:19 vm kernel: [drm:drm_client_modeset_probe] connector 35 enabled? yes
> Mar 09 14:57:19 vm kernel: [drm:drm_client_modeset_probe] Not using firmware configuration
> Mar 09 14:57:19 vm kernel: [drm:drm_client_modeset_probe] looking for cmdline mode on connector 35
> Mar 09 14:57:19 vm kernel: [drm:drm_client_modeset_probe] looking for preferred mode on connector 35 0
> Mar 09 14:57:19 vm kernel: [drm:drm_client_modeset_probe] found mode 400x240
> Mar 09 14:57:19 vm kernel: [drm:drm_client_modeset_probe] picking CRTCs for 400x240 config
> Mar 09 14:57:19 vm kernel: [drm:drm_client_modeset_probe] desired mode 400x240 set on crtc 33 (0,0)
> Mar 09 14:57:19 vm kernel: [drm:drm_mode_object_get] OBJ ID: 35 (2)
> Mar 09 14:57:19 vm kernel: [drm:drm_mode_object_put.part.0] OBJ ID: 35 (3)
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:__drm_fb_helper_initial_config_and_unlock] test CRTC 0 primary plane
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:drm_fb_helper_generic_probe] surface width(400), height(240) and bpp(32)
> Mar 09 14:57:19 vm kernel: [drm:drm_mode_addfb2] [FB:36]
> Mar 09 14:57:19 vm kernel: [drm:drm_mode_object_put.part.0] OBJ ID: 36 (2)
> Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm] fb0: guddrmfb frame buffer device
>
>
> It looks good, I think? However, neither GUD_REQ_SET_CONTROLLER_ENABLE
> nor GUD_REQ_SET_DISPLAY_ENABLE is ever called, and there is no bulk
> output if I write to /dev/fb0.
>
> Can you tell what's missing?
>
IIRC you need ioctl FBIOPUT_VSCREENINFO to enable the pipeline. You can
use 'con2fbmap' to put a console on the fbdev or use 'fbi' to show an
image. They will do the necessary ioctls.
I use modetest for simple testing:
https://github.com/notro/tinydrm/wiki/Development#modetest
I have made tool that employs usbmon to show what happens just outside
the wire (runs on the host):
https://github.com/notro/gud/blob/master/tools/monitor.py
I haven't got a proper USB analyser, so this was second best. I was
primarily looking for performance numbers, but it has been helpful in
debugging problems as well. It's not perfect,
GUD_DISPLAY_FLAG_FULL_UPDATE is not handled for instance.
Depending on how long it takes for the DMA mask dependency patch to show
up in drm-misc-next, I will either publish a new version or apply the
current and provide patches with the necessary fixes. It's tiresome to
stay out-of-tree for a long time, DRM is constantly changing. And I've
spent way to long on this project already. This is one of those things
that would never have happened if I knew how long it would take :)
Thanks for helping me improve it, especially the protocol which is
difficult to change after the fact.
Noralf.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
2021-03-09 18:07 ` Noralf Trønnes
@ 2021-03-09 18:16 ` Noralf Trønnes
2021-03-10 4:55 ` Peter Stuge
1 sibling, 0 replies; 18+ messages in thread
From: Noralf Trønnes @ 2021-03-09 18:16 UTC (permalink / raw)
To: Peter Stuge
Cc: hudson, markus, Daniel Vetter, linux-usb, dri-devel, th020394,
lkundrak, pontus.fuchs, sam
Den 09.03.2021 19.07, skrev Noralf Trønnes:
>
>
> Den 09.03.2021 15.02, skrev Peter Stuge:
>> Hello Noralf,
>>
>> I've made some progress with my test device. I'm implementing R1
>> first and once that works I'll test RGB111 as well. Along the way
>> I've found a couple of things in the code:
>>
> Depending on how long it takes for the DMA mask dependency patch to show
> up in drm-misc-next, I will either publish a new version or apply the
> current and provide patches with the necessary fixes.
In case I apply this version, are you happy enough with it that you want
to give an ack or r-b?
Noralf.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
2021-03-09 18:07 ` Noralf Trønnes
2021-03-09 18:16 ` Noralf Trønnes
@ 2021-03-10 4:55 ` Peter Stuge
2021-03-10 11:43 ` Noralf Trønnes
1 sibling, 1 reply; 18+ messages in thread
From: Peter Stuge @ 2021-03-10 4:55 UTC (permalink / raw)
To: Noralf Trønnes
Cc: hudson, markus, Daniel Vetter, linux-usb, dri-devel, th020394,
lkundrak, pontus.fuchs, sam
Noralf Trønnes wrote:
> > The u16 index parameter to gud_usb_transfer() and at least also
> > gud_usb_{get,set,get_u8,set_u8}() is eventually passed in u16 value
> > in the call to gud_usb_control_msg(), which had me confused for a bit.
> >
> > What do you think about renaming all of those parameters to wValue,
>
> Only connector requests use this value and in that case it's the
> connector index. I need to get this driver applied now, and can't
> spend more time polishing it, so I'll just keep it as-is.
Okay.
> > I found this because I can't get my device to send 0 bytes IN when
> > the host requests more, if I provide no data the request STALLs. This
> > is for sure a bug in my device
..
> > What do you think about formalizing this, adding an actual dummy property?
>
> I want to avoid that.
Ack. I too prefer not adding workarounds, this needs to be fixed in
my device, but I wanted to bring it up.
> I'd rather add a descriptor flag GUD_DISPLAY_FLAG_CANT_RETURN_ZERO or
> something for such devices.
> Also this property warning is just a debug message so not a very big
> problem.
I'd rather not squat a currently-unused property that may be allocated
later.
> Worse is EDID if you don't want to support that since it prints
> an error in that case.
I'll fix my device. If that proves impossible I can still add EDID bytes.
> > Or maybe adding flags to the display descriptor for "I have properties",
> > "I have connector properties" and "I have EDID" ?
>
> Then I might as well go back to the previous version and have count in
> the structs won't I ;)
That's true. Forget the workarounds.
> gud_xrgb8888_to_color() handles that, although broken in this version
> for partial updates. Here's the fix:
> https://gist.github.com/notro/a94d381cf98b7e15fcddbc90e4af0a21
Thanks.
> I've chosen to only print probe errors, flush errors and devices
> returning incorrect reply lengths. I hate drivers that fill the logs
> with errors, but I might have gone to far on the silent side, I don't
> know. But one nice thing about DRM is that debug is always builtin and
> can be enabled.
I think you've struck a good balance. The error messages I've seen were
all unique and immediately led me to the right place in the driver.
That's really helpful.
> > Finally, here's the drm debug output when I connect my device:
..
> > It looks good, I think? However, neither GUD_REQ_SET_CONTROLLER_ENABLE
> > nor GUD_REQ_SET_DISPLAY_ENABLE is ever called, and there is no bulk
> > output if I write to /dev/fb0.
> >
> > Can you tell what's missing?
>
> IIRC you need ioctl FBIOPUT_VSCREENINFO to enable the pipeline. You can
> use 'con2fbmap' to put a console on the fbdev or use 'fbi' to show an
> image. They will do the necessary ioctls.
Thanks for the hints! I've used fbi, but remembered writing directly
to /dev/fb0 without ioctl:s. I think that must have been after the
console had already been moved to the framebuffer and thus activated
the pipe. I dug up a version of fbset which calls that ioctl.
> I use modetest for simple testing:
> https://github.com/notro/tinydrm/wiki/Development#modetest
Ack, libdrm just has an inconveniently long dependency tail for my
test environment. But fbset + dd works.
> I have made tool that employs usbmon to show what happens just outside
> the wire (runs on the host):
> https://github.com/notro/gud/blob/master/tools/monitor.py
> I haven't got a proper USB analyser, so this was second best.
Nice idea! I've been using usbmon as well during development, it's great.
I find a hardware analyzer is only really neccessary for lowlevel problems
like my no-0-bytes bug.
> Depending on how long it takes for the DMA mask dependency patch to show
> up in drm-misc-next, I will either publish a new version or apply the
> current and provide patches with the necessary fixes.
I look forward to it landing.
> This is one of those things that would never have happened if I knew
> how long it would take :)
Thanks again for working on this great idea! I'm happy that I can help.
Noralf Trønnes wrote:
> > Depending on how long it takes for the DMA mask dependency patch to show
> > up in drm-misc-next, I will either publish a new version or apply the
> > current and provide patches with the necessary fixes.
>
> In case I apply this version, are you happy enough with it that you want
> to give an ack or r-b?
I've now tested R1 and RGB111 and I think I've found two more things:
I didn't receive the expected bits/bytes for RGB111 on the bulk endpoint,
I think because of how components were extracted in gud_xrgb8888_to_color().
Changing to the following gets me the expected (X R1 G1 B1 X R2 G2 B2) bytes:
r = (*pix32 >> 8) & 0xff;
g = (*pix32 >> 16) & 0xff;
b = (*pix32++ >> 24) & 0xff;
Then, gud_xrgb8888_to_color() and maybe even gud_xrgb8888_to_r124() seem
to be host endian dependent, at least because of that pix32, but maybe more?
I don't know whether drm guarantees "native" XRGB byte sequence in memory,
then I guess the pix32 is okay? Please take a look?
Finally my very last ask: Please consider renaming GUD_PIXEL_FORMAT_RGB111
to GUD_PIXEL_FORMAT_XRGB1111?
Someone may want to add the bit fiddling to output actual RGB111 data
later, then that name would be occupied, and XRGB1111 would describe
the current data format more accurately.
With the two ret=0 fixes, your gist and the three things above addressed
I'm happy to say:
Reviewed-By: Peter Stuge <peter@stuge.se>
R1 and RGB111 are: (if v8 perhaps for a separate RGB commit)
Tested-By: Peter Stuge <peter@stuge.se>
Later I think there's good potential for performance optimization. I
noticed that the drm pipe flush submits the frame as 64 byte bulk
transfers, which becomes slow. The USB stack can take at least 16kb
if not far more and will split as needed. The split into single
64-byte bulk packets is even done by the host controller hardware
if it is given the chance. Some numbers:
usbmon during drm flush of 400x240 RGB111 (48000 byte) shows 750ms
from first submit to last complete:
c1b883c0 0.503674 S Bo:4:022:1 - 64 =
4f7f2900 1.254213 C Bo:4:022:1 0 64 >
A simple userspace program with one libusb_bulk_transfer() call needs
just 300ms for the same data (libusb splits it into 3x16kb transfers):
$ time ./bo < test266.xrgb1111
will output 48000 bytes
real 0m0.298s
user 0m0.002s
sys 0m0.008s
$
The display is slow, but the difference is noticeable. Something for later.
Kind regards
//Peter
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
2021-03-10 4:55 ` Peter Stuge
@ 2021-03-10 11:43 ` Noralf Trønnes
2021-03-11 14:48 ` Peter Stuge
0 siblings, 1 reply; 18+ messages in thread
From: Noralf Trønnes @ 2021-03-10 11:43 UTC (permalink / raw)
To: Peter Stuge
Cc: hudson, markus, Daniel Vetter, linux-usb, dri-devel, th020394,
lkundrak, pontus.fuchs, sam
Den 10.03.2021 05.55, skrev Peter Stuge:
> Noralf Trønnes wrote:
>>> Depending on how long it takes for the DMA mask dependency patch to show
>>> up in drm-misc-next, I will either publish a new version or apply the
>>> current and provide patches with the necessary fixes.
>>
>> In case I apply this version, are you happy enough with it that you want
>> to give an ack or r-b?
>
> I've now tested R1 and RGB111 and I think I've found two more things:
>
> I didn't receive the expected bits/bytes for RGB111 on the bulk endpoint,
> I think because of how components were extracted in gud_xrgb8888_to_color().
>
> Changing to the following gets me the expected (X R1 G1 B1 X R2 G2 B2) bytes:
>
> r = (*pix32 >> 8) & 0xff;
> g = (*pix32 >> 16) & 0xff;
> b = (*pix32++ >> 24) & 0xff;
>
We're accessing the whole word here through pix32, no byte access, so
endianess doesn't come into play. This change will flip r and b, which
gives: XRGB8888 -> XBGR1111
BGR is a common thing on controllers, are you sure yours are set to RGB
and not BGR?
And the 0xff masking isn't necessary since we're assigning to a byte, right?
I haven't got a native R1G1B1 display so I have emulated and I do get
the expected colors. This is the conversion function I use on the device
which I think is correct:
static size_t rgb111_to_rgb565(uint16_t *dst, uint8_t *src,
uint16_t src_width, uint16_t src_height)
{
uint8_t rgb111, val = 0;
size_t len = 0;
for (uint16_t y = 0; y < src_height; y++) {
for (uint16_t x = 0; x < src_width; x++) {
if (!(x % 2))
val = *src++;
rgb111 = val >> 4;
*dst++ = ((rgb111 & 0x04) << 13) | ((rgb111 & 0x02) << 9) |
((rgb111 & 0x01) << 4);
len += sizeof(*dst);
val <<= 4;
}
}
return len;
}
>
> Then, gud_xrgb8888_to_color() and maybe even gud_xrgb8888_to_r124() seem
> to be host endian dependent, at least because of that pix32, but maybe more?
> I don't know whether drm guarantees "native" XRGB byte sequence in memory,
> then I guess the pix32 is okay? Please take a look?
>
>
> Finally my very last ask: Please consider renaming GUD_PIXEL_FORMAT_RGB111
> to GUD_PIXEL_FORMAT_XRGB1111?
>
It has crossed my mind, I'll change it.
Noralf.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
2021-03-10 11:43 ` Noralf Trønnes
@ 2021-03-11 14:48 ` Peter Stuge
2021-03-11 17:16 ` Noralf Trønnes
0 siblings, 1 reply; 18+ messages in thread
From: Peter Stuge @ 2021-03-11 14:48 UTC (permalink / raw)
To: Noralf Trønnes
Cc: hudson, markus, Daniel Vetter, linux-usb, dri-devel, th020394,
lkundrak, pontus.fuchs, sam
Noralf Trønnes wrote:
> > I didn't receive the expected bits/bytes for RGB111 on the bulk endpoint,
> > I think because of how components were extracted in gud_xrgb8888_to_color().
> >
> > Changing to the following gets me the expected (X R1 G1 B1 X R2 G2 B2) bytes:
> >
> > r = (*pix32 >> 8) & 0xff;
> > g = (*pix32 >> 16) & 0xff;
> > b = (*pix32++ >> 24) & 0xff;
>
> We're accessing the whole word here through pix32, no byte access, so
> endianess doesn't come into play.
Endianness matters because parts of pix32 are used.
Software only sees bytes (or larger) because addresses are byte granular,
but must pay attention to the bit order when dealing with smaller values
inside larger memory accesses.
Given 4 bytes of memory {0x11, 0x22, 0x33, 0x44} at address A, both LE
and BE machines appear the same when accessing individual bytes, but with
uint32_t *a32 = A then a32[0] is 0x44332211 on LE and 0x11223344 on BE.
Hence the question: What does DRM promise about the XRGB8888 mode?
Is it guaranteed that the first byte in memory is always unused, the second
represents red, the third green and the fourth blue (endianess agnostic)?
I'd expect this and I guess that it is the case, but I don't know DRM?
Or is it instead guaranteed that when accessed natively as one 32-bit
value the blue component is always in the most significant byte (endianess
abstracted, always LE in memory) or always in the least significant byte
(endianess abstracted, always BE in memory)?
This would be annoying for userspace, but well, it's possible.
In the abstracted (latter) case pix32 would work, but could still be
questioned on style, and in fact, pix32 didn't work for me, so at a
minimum the byte order would be the reverse.
In the agnostic (former) case your code was correct for BE and mine
for LE, but I'd then suggest using a u8 * to both work correctly
everywhere and be obvious.
> This change will flip r and b, which gives: XRGB8888 -> XBGR1111
The old code was:
r = *pix32 >> 16;
g = *pix32 >> 8;
b = *pix32++;
On my LE machine this set r to the third byte (G), g to the second (R)
and b to the first (X), explaining the color confusion that I saw.
> BGR is a common thing on controllers, are you sure yours are set to RGB
> and not BGR?
Yes; I've verified that my display takes red in MSB both in its data
sheet and by writing raw bits to it on a system without the gud driver.
> And the 0xff masking isn't necessary since we're assigning to a byte, right?
Not strictly neccessary but I like to do it anyway, both to be explicit
and also to ensure that the compiler will never sign extend, if types
are changed or if values become treated as signed and/or larger by the
compiler because the code is changed.
It's frustrating to debug such unexpected changes in behavior due to
a type change or calculation change, but if you find it too defensive
then go ahead and remove it, if pix32 does stay.
> I haven't got a native R1G1B1 display so I have emulated and I do get
> the expected colors. This is the conversion function I use on the device
> which I think is correct:
>
> static size_t rgb111_to_rgb565(uint16_t *dst, uint8_t *src,
> uint16_t src_width, uint16_t src_height)
> {
> uint8_t rgb111, val = 0;
> size_t len = 0;
>
> for (uint16_t y = 0; y < src_height; y++) {
> for (uint16_t x = 0; x < src_width; x++) {
> if (!(x % 2))
> val = *src++;
> rgb111 = val >> 4;
> *dst++ = ((rgb111 & 0x04) << 13) | ((rgb111 & 0x02) << 9) |
> ((rgb111 & 0x01) << 4);
I'm afraid this isn't correct. Two wrongs end up cancelling each other
out and it's not so obvious because the destination has symmetric (565)
components.
If you were to convert to xrgb8888 in the same way I think you'd also
see some color confusion, and in any case blue is getting lost already
in gud_xrgb8888_to_color() on LE.
//Peter
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
2021-03-11 14:48 ` Peter Stuge
@ 2021-03-11 17:16 ` Noralf Trønnes
2021-03-11 20:02 ` Peter Stuge
0 siblings, 1 reply; 18+ messages in thread
From: Noralf Trønnes @ 2021-03-11 17:16 UTC (permalink / raw)
To: Peter Stuge
Cc: hudson, markus, Daniel Vetter, linux-usb, dri-devel, th020394,
lkundrak, pontus.fuchs, sam
Den 11.03.2021 15.48, skrev Peter Stuge:
> Noralf Trønnes wrote:
>>> I didn't receive the expected bits/bytes for RGB111 on the bulk endpoint,
>>> I think because of how components were extracted in gud_xrgb8888_to_color().
>>>
>>> Changing to the following gets me the expected (X R1 G1 B1 X R2 G2 B2) bytes:
>>>
>>> r = (*pix32 >> 8) & 0xff;
>>> g = (*pix32 >> 16) & 0xff;
>>> b = (*pix32++ >> 24) & 0xff;
>>
>> We're accessing the whole word here through pix32, no byte access, so
>> endianess doesn't come into play.
>
> Endianness matters because parts of pix32 are used.
>
This code:
#include <stdio.h>
#include <stdint.h>
void main()
{
volatile uint32_t endian = 0x01234567;
uint32_t v = 0xaabbccdd;
uint32_t *pix32 = &v;
uint8_t r, g, b, *p;
r = *pix32 >> 16;
g = *pix32 >> 8;
b = *pix32++;
printf("xrgb8888=%08x\n", v);
printf("32-bit access:\n");
printf("r=%02x\n", r);
printf("g=%02x\n", g);
printf("b=%02x\n", b);
printf("Byte access on %s:\n", (*((uint8_t*)(&endian))) == 0x67 ? "LE"
: "BE");
p = (uint8_t *)&v;
printf("r=%02x\n", p[1]);
printf("g=%02x\n", p[2]);
printf("b=%02x\n", p[3]);
}
prints:
xrgb8888=aabbccdd
32-bit access:
r=bb
g=cc
b=dd
Byte access on LE:
r=cc
g=bb
b=aa
> Software only sees bytes (or larger) because addresses are byte granular,
> but must pay attention to the bit order when dealing with smaller values
> inside larger memory accesses.
>
> Given 4 bytes of memory {0x11, 0x22, 0x33, 0x44} at address A, both LE
> and BE machines appear the same when accessing individual bytes, but with
> uint32_t *a32 = A then a32[0] is 0x44332211 on LE and 0x11223344 on BE.
>
>
> Hence the question: What does DRM promise about the XRGB8888 mode?
>
That it's a 32-bit value. From include/uapi/drm/drm_fourcc.h:
/* 32 bpp RGB */
#define DRM_FORMAT_XRGB8888 fourcc_code('X', 'R', '2', '4') /* [31:0]
x:R:G:B 8:8:8:8 little endian */
If a raw buffer was passed from a BE to an LE machine, there would be
problems because of how the value is stored, but here it's the same
endianness in userspace and kernel space.
There is code in gud_prep_flush() that handles a BE host with a
multibyte format:
} else if (gud_is_big_endian() && format->cpp[0] > 1) {
drm_fb_swab(buf, vaddr, fb, rect, !import_attach);
In this case we can't just pass on the raw buffer to the device since
the protocol is LE, and thus have to swap the bytes to match up how
they're stored in memory on the device.
I'm not loosing any of the colors when running modetest. This is the
test image that modetest uses and it comes through just like that:
https://commons.wikimedia.org/wiki/File:SMPTE_Color_Bars.svg
Noralf.
> Is it guaranteed that the first byte in memory is always unused, the second
> represents red, the third green and the fourth blue (endianess agnostic)?
> I'd expect this and I guess that it is the case, but I don't know DRM?
>
> Or is it instead guaranteed that when accessed natively as one 32-bit
> value the blue component is always in the most significant byte (endianess
> abstracted, always LE in memory) or always in the least significant byte
> (endianess abstracted, always BE in memory)?
> This would be annoying for userspace, but well, it's possible.
>
> In the abstracted (latter) case pix32 would work, but could still be
> questioned on style, and in fact, pix32 didn't work for me, so at a
> minimum the byte order would be the reverse.
>
>
> In the agnostic (former) case your code was correct for BE and mine
> for LE, but I'd then suggest using a u8 * to both work correctly
> everywhere and be obvious.
>
>
>> This change will flip r and b, which gives: XRGB8888 -> XBGR1111
>
> The old code was:
> r = *pix32 >> 16;
> g = *pix32 >> 8;
> b = *pix32++;
>
> On my LE machine this set r to the third byte (G), g to the second (R)
> and b to the first (X), explaining the color confusion that I saw.
>
>
>> BGR is a common thing on controllers, are you sure yours are set to RGB
>> and not BGR?
>
> Yes; I've verified that my display takes red in MSB both in its data
> sheet and by writing raw bits to it on a system without the gud driver.
>
>
>> And the 0xff masking isn't necessary since we're assigning to a byte, right?
>
> Not strictly neccessary but I like to do it anyway, both to be explicit
> and also to ensure that the compiler will never sign extend, if types
> are changed or if values become treated as signed and/or larger by the
> compiler because the code is changed.
>
> It's frustrating to debug such unexpected changes in behavior due to
> a type change or calculation change, but if you find it too defensive
> then go ahead and remove it, if pix32 does stay.
>
>
>> I haven't got a native R1G1B1 display so I have emulated and I do get
>> the expected colors. This is the conversion function I use on the device
>> which I think is correct:
>>
>> static size_t rgb111_to_rgb565(uint16_t *dst, uint8_t *src,
>> uint16_t src_width, uint16_t src_height)
>> {
>> uint8_t rgb111, val = 0;
>> size_t len = 0;
>>
>> for (uint16_t y = 0; y < src_height; y++) {
>> for (uint16_t x = 0; x < src_width; x++) {
>> if (!(x % 2))
>> val = *src++;
>> rgb111 = val >> 4;
>> *dst++ = ((rgb111 & 0x04) << 13) | ((rgb111 & 0x02) << 9) |
>> ((rgb111 & 0x01) << 4);
>
> I'm afraid this isn't correct. Two wrongs end up cancelling each other
> out and it's not so obvious because the destination has symmetric (565)
> components.
>
> If you were to convert to xrgb8888 in the same way I think you'd also
> see some color confusion, and in any case blue is getting lost already
> in gud_xrgb8888_to_color() on LE.
>
>
> //Peter
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
2021-03-11 17:16 ` Noralf Trønnes
@ 2021-03-11 20:02 ` Peter Stuge
2021-03-11 21:36 ` Ilia Mirkin
0 siblings, 1 reply; 18+ messages in thread
From: Peter Stuge @ 2021-03-11 20:02 UTC (permalink / raw)
To: Noralf Trønnes
Cc: hudson, markus, Daniel Vetter, linux-usb, dri-devel, th020394,
lkundrak, pontus.fuchs, sam
Noralf Trønnes wrote:
> > Endianness matters because parts of pix32 are used.
>
> This code:
..
> prints:
>
> xrgb8888=aabbccdd
> 32-bit access:
> r=bb
> g=cc
> b=dd
> Byte access on LE:
> r=cc
> g=bb
> b=aa
As expected, and:
xrgb8888=aabbccdd
32-bit access:
r=bb
g=cc
b=dd
Byte access on BE:
r=bb
g=cc
b=dd
I've done similar tests in the past and did another before my last mail.
We agree about endian effects. Apologies if I came across as overbearing!
> > Hence the question: What does DRM promise about the XRGB8888 mode?
>
> That it's a 32-bit value. From include/uapi/drm/drm_fourcc.h:
>
> /* 32 bpp RGB */
> #define DRM_FORMAT_XRGB8888 fourcc_code('X', 'R', '2', '4') /* [31:0]
> x:R:G:B 8:8:8:8 little endian */
Okay, "[31:0] x:R:G:B 8:8:8:8" can certainly mean
[31:24]=x [23:16]=R [15:8]=G [7:0]=B, which when stored "little endian"
becomes B G R X in memory, for which your pix32 code is correct.
That's the reverse *memory* layout of what the name says :) but yes,
the name then matches the representation seen by software. That's the
"abstracted" case that I didn't expect, because I thought the name was
refering to memory layout and because I was thinking about how traditional
graphics adapter video memory has the R component at the lower
address, at least in early linear modes.
I also didn't pay attention to the fbset output:
rgba 8/16,8/8,8/0,0/0
With drm format describing software pixel representation and per the
fbset rgba description my test file was incorrect. I've recreated it
with B G R X bytes and it shows correctly with your pix32 code.
Sending data directly to the device without the gud driver uses
different data, so isn't actually a fair comparison, but I didn't
change the device at all now, and that still works.
> If a raw buffer was passed from a BE to an LE machine, there would be
> problems because of how the value is stored,
And swab would be required on a LE machine with a graphics adapter in
a mode with X R G B memory layout, or that system would just never
present XRGB8888 for that adapter/mode but perhaps something called
BGRX8888 instead? I see.
> but here it's the same endianness in userspace and kernel space.
Ack.
> There is code in gud_prep_flush() that handles a BE host with a
> multibyte format:
>
> } else if (gud_is_big_endian() && format->cpp[0] > 1) {
> drm_fb_swab(buf, vaddr, fb, rect, !import_attach);
>
> In this case we can't just pass on the raw buffer to the device since
> the protocol is LE, and thus have to swap the bytes to match up how
> they're stored in memory on the device.
Ack.
> I'm not loosing any of the colors when running modetest. This is the
> test image that modetest uses and it comes through just like that:
> https://commons.wikimedia.org/wiki/File:SMPTE_Color_Bars.svg
So your destination rgb565 buffer has a [15:11]=R [10:5]=G [4:0]=B
pixel format, which stores as B+G G+R in memory, as opposed to R+G G+B.
All right.
Thanks a lot for clearing up my misunderstanding of drm format names
and my endianess concerns!
//Peter
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
2021-03-11 20:02 ` Peter Stuge
@ 2021-03-11 21:36 ` Ilia Mirkin
2021-03-11 22:57 ` Peter Stuge
0 siblings, 1 reply; 18+ messages in thread
From: Ilia Mirkin @ 2021-03-11 21:36 UTC (permalink / raw)
To: Peter Stuge
Cc: Noralf Trønnes, hudson, markus, Daniel Vetter, linux-usb,
dri-devel, th020394, lkundrak, pontus.fuchs, Sam Ravnborg
On Thu, Mar 11, 2021 at 3:02 PM Peter Stuge <peter@stuge.se> wrote:
> > > Hence the question: What does DRM promise about the XRGB8888 mode?
> >
> > That it's a 32-bit value. From include/uapi/drm/drm_fourcc.h:
> >
> > /* 32 bpp RGB */
> > #define DRM_FORMAT_XRGB8888 fourcc_code('X', 'R', '2', '4') /* [31:0]
> > x:R:G:B 8:8:8:8 little endian */
>
> Okay, "[31:0] x:R:G:B 8:8:8:8" can certainly mean
> [31:24]=x [23:16]=R [15:8]=G [7:0]=B, which when stored "little endian"
> becomes B G R X in memory, for which your pix32 code is correct.
>
> That's the reverse *memory* layout of what the name says :) but yes,
> the name then matches the representation seen by software. That's the
> "abstracted" case that I didn't expect, because I thought the name was
> refering to memory layout and because I was thinking about how traditional
> graphics adapter video memory has the R component at the lower
> address, at least in early linear modes.
The definition of the formats is memory layout in little endian. The
definition you see is of a 32-bit packed little-endian integer, which
is a fixed memory layout.
Now, if you're on an actual big-endian platform, and you want to
accept big-endian-packed formats, there's a bit of unpleasantness that
goes on. Basically there are two options:
1. Ignore the above definition and interpret the formats as
*big-endian* layouts. This is what nouveau and radeon do. They also
don't support AddFB2 (which is what allows supplying a format) -- only
AddFB which just has depth (and bpp). That's fine for nouveau and
radeon because the relevant userspace just uses AddFB, and knows what
the drivers want, so it all works out.
2. Comply with the above definition and set
dev->mode_config.quirk_addfb_prefer_host_byte_order to false. This
loses you native host packing of RGB565/etc, since they're just not
defined as formats. There's a DRM_FORMAT_BIG_ENDIAN bit but it's not
properly supported for anything but the 8888 formats.
I'm not sure why you guys were talking about BE in the first place,
but since this is a topic I've looked into (in the context of moving
nouveau from 1 to 2 - but that can't happen due to the reduced format
availability), figured I'd share some of the current sad state.
Cheers,
-ilia
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
2021-03-11 21:36 ` Ilia Mirkin
@ 2021-03-11 22:57 ` Peter Stuge
2021-03-12 0:19 ` Ilia Mirkin
0 siblings, 1 reply; 18+ messages in thread
From: Peter Stuge @ 2021-03-11 22:57 UTC (permalink / raw)
To: Ilia Mirkin
Cc: hudson, markus, Daniel Vetter, linux-usb, dri-devel, th020394,
lkundrak, pontus.fuchs, Sam Ravnborg
Ilia Mirkin wrote:
> > > #define DRM_FORMAT_XRGB8888 fourcc_code('X', 'R', '2', '4') /* [31:0]
> > > x:R:G:B 8:8:8:8 little endian */
> >
> > Okay, "[31:0] x:R:G:B 8:8:8:8" can certainly mean
> > [31:24]=x [23:16]=R [15:8]=G [7:0]=B, which when stored "little endian"
> > becomes B G R X in memory, for which your pix32 code is correct.
> >
> > That's the reverse *memory* layout of what the name says :)
>
> The definition of the formats is memory layout in little endian.
To clarify, my new (hopefully correct?) understanding is this:
XRGB8888 does *not* mean that address 0=X, 1=R, 2=G, 3=B, but that
the most significant byte in a packed XRGB8888 32-bit integer is X
and the least significant byte is B, and that this is the case both
on LE and BE machines.
I previously thought that XRGB8888 indicated the memory byte order of
components being X R G B regardless of machine endianess, but now
understand XRGB to mean the MSB..LSB order of component bytes within
the 32-bit integer, as seen by software, not the order of bytes in memory.
> The definition you see is of a 32-bit packed little-endian integer,
> which is a fixed memory layout.
In the header definition I'm not completely sure what the "little endian"
means - I guess it refers to how the 32-bit integer will be stored in memory,
but it could also refer to the order of component packing within.
Noralf's code and testing and also what fbset tells me seems to support
this understanding, at least on LE machines.
> Now, if you're on an actual big-endian platform, and you want to
> accept big-endian-packed formats, there's a bit of unpleasantness that
> goes on.
In the particular case of XRGB8888 that Noralf has implemented and
I've tested every pixel is translated "manually" anyway; each
component byte is downconverted to a single bit, but this use case
is mostly for smaller resolutions, so no too big deal.
> I'm not sure why you guys were talking about BE in the first place,
I was worried that the translation didn't consider endianess.
Noralf, looking at the 3/3 patch again now, drm_fb_swab() gets called
on BE when format == fb->format, but does it also need to be called
on BE they are different, or will circumstances be such that it's
never neccessary then?
Thanks and sorry if I'm confusing things needlessly
//Peter
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
2021-03-11 22:57 ` Peter Stuge
@ 2021-03-12 0:19 ` Ilia Mirkin
2021-03-12 4:32 ` Peter Stuge
0 siblings, 1 reply; 18+ messages in thread
From: Ilia Mirkin @ 2021-03-12 0:19 UTC (permalink / raw)
To: Peter Stuge
Cc: hudson, markus, Daniel Vetter, linux-usb, dri-devel, th020394,
lkundrak, pontus.fuchs, Sam Ravnborg
On Thu, Mar 11, 2021 at 5:58 PM Peter Stuge <peter@stuge.se> wrote:
>
> Ilia Mirkin wrote:
> > > > #define DRM_FORMAT_XRGB8888 fourcc_code('X', 'R', '2', '4') /* [31:0]
> > > > x:R:G:B 8:8:8:8 little endian */
> > >
> > > Okay, "[31:0] x:R:G:B 8:8:8:8" can certainly mean
> > > [31:24]=x [23:16]=R [15:8]=G [7:0]=B, which when stored "little endian"
> > > becomes B G R X in memory, for which your pix32 code is correct.
> > >
> > > That's the reverse *memory* layout of what the name says :)
> >
> > The definition of the formats is memory layout in little endian.
>
> To clarify, my new (hopefully correct?) understanding is this:
>
> XRGB8888 does *not* mean that address 0=X, 1=R, 2=G, 3=B, but that
> the most significant byte in a packed XRGB8888 32-bit integer is X
> and the least significant byte is B, and that this is the case both
> on LE and BE machines.
Not quite.
XRGB8888 means that the memory layout should match a 32-bit integer,
stored as LE, with the low bits being B, next bits being G, etc. This
translates to byte 0 = B, byte 1 = G, etc. If you're on a BE system,
and you're handed a XRGB8888 buffer, it still expects that byte 0 = B,
etc (except as I outlined, some drivers which are from before these
formats were a thing, sort of do their own thing). Thankfully this is
equivalent to BGRX8888 (big-endian packing), so you can just munge the
format. Not so with e.g. RGB565 though (since the components don't
fall on byte boundaries).
> I previously thought that XRGB8888 indicated the memory byte order of
> components being X R G B regardless of machine endianess, but now
> understand XRGB to mean the MSB..LSB order of component bytes within
> the 32-bit integer, as seen by software, not the order of bytes in memory.
There are about 100 conventions, and they all manage to be different
from each other. Packed vs array. BE vs LE. If you're *not* confused,
that should be a red flag.
[...]
> > I'm not sure why you guys were talking about BE in the first place,
>
> I was worried that the translation didn't consider endianess.
The translation in gud_xrgb8888_to_color definitely seems suspect.
There's also a gud_is_big_endian, but I'm guessing this applies to the
downstream device rather than the host system. I didn't check if
dev->mode_config.quirk_addfb_prefer_host_byte_order is set -- that
setting dictates whether these formats are in host-byte-order (and
AddFB2 is disabled, so buffers can only be specified with depth/bpp
and ambiguous component orders) or in LE byte order (and userspace can
use AddFB2 which gives allows precise formats for these buffers). Not
100% sure what something like Xorg's modesetting driver does, TBH.
This is a very poorly-tested scenario.
-ilia
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
2021-03-12 0:19 ` Ilia Mirkin
@ 2021-03-12 4:32 ` Peter Stuge
2021-03-12 11:53 ` Noralf Trønnes
0 siblings, 1 reply; 18+ messages in thread
From: Peter Stuge @ 2021-03-12 4:32 UTC (permalink / raw)
To: Ilia Mirkin
Cc: hudson, markus, Daniel Vetter, linux-usb, dri-devel, th020394,
lkundrak, pontus.fuchs, Sam Ravnborg
Ilia Mirkin wrote:
> XRGB8888 means that the memory layout should match a 32-bit integer,
> stored as LE, with the low bits being B, next bits being G, etc. This
> translates to byte 0 = B, byte 1 = G, etc. If you're on a BE system,
> and you're handed a XRGB8888 buffer, it still expects that byte 0 = B,
> etc (except as I outlined, some drivers which are from before these
> formats were a thing, sort of do their own thing). Thankfully this is
> equivalent to BGRX8888 (big-endian packing), so you can just munge the
> format.
I understand! Thanks a lot for clarifying.
It makes much more sense to me that the format indeed describes
what is in memory rather than how pixels look to software.
> > > I'm not sure why you guys were talking about BE in the first place,
> >
> > I was worried that the translation didn't consider endianess.
>
> The translation in gud_xrgb8888_to_color definitely seems suspect.
So to me this means that the gud_pipe translations from XRGB8888 to the
1-bit formats *do* have to adjust for the reversed order on BE.
> There's also a gud_is_big_endian, but I'm guessing this applies to the
> downstream device rather than the host system.
gud_is_big_endian() is a static bool wrapper around defined(__BIG_ENDIAN)
so yes, it applies to the host.
With memory layout being constant I again think gud_xrgb8888_to_color()
needs to take further steps to work correctly also on BE hosts. (Maybe
that's le32_to_cpu(*pix32), maybe drm_fb_swab(), maybe something else?)
> I didn't check if dev->mode_config.quirk_addfb_prefer_host_byte_order
> is set
I can't tell if that's helpful, probably Noralf can.
Thanks a lot
//Peter
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
2021-03-12 4:32 ` Peter Stuge
@ 2021-03-12 11:53 ` Noralf Trønnes
0 siblings, 0 replies; 18+ messages in thread
From: Noralf Trønnes @ 2021-03-12 11:53 UTC (permalink / raw)
To: Peter Stuge, Ilia Mirkin
Cc: hudson, markus, Daniel Vetter, linux-usb, dri-devel, th020394,
lkundrak, pontus.fuchs, Sam Ravnborg
Den 12.03.2021 05.32, skrev Peter Stuge:
> Ilia Mirkin wrote:
>> XRGB8888 means that the memory layout should match a 32-bit integer,
>> stored as LE, with the low bits being B, next bits being G, etc. This
>> translates to byte 0 = B, byte 1 = G, etc. If you're on a BE system,
>> and you're handed a XRGB8888 buffer, it still expects that byte 0 = B,
>> etc (except as I outlined, some drivers which are from before these
>> formats were a thing, sort of do their own thing). Thankfully this is
>> equivalent to BGRX8888 (big-endian packing), so you can just munge the
>> format.
>
> I understand! Thanks a lot for clarifying.
>
> It makes much more sense to me that the format indeed describes
> what is in memory rather than how pixels look to software.
>
>
>>>> I'm not sure why you guys were talking about BE in the first place,
>>>
>>> I was worried that the translation didn't consider endianess.
>>
>> The translation in gud_xrgb8888_to_color definitely seems suspect.
>
> So to me this means that the gud_pipe translations from XRGB8888 to the
> 1-bit formats *do* have to adjust for the reversed order on BE.
>
>
>> There's also a gud_is_big_endian, but I'm guessing this applies to the
>> downstream device rather than the host system.
>
> gud_is_big_endian() is a static bool wrapper around defined(__BIG_ENDIAN)
> so yes, it applies to the host.
>
> With memory layout being constant I again think gud_xrgb8888_to_color()
> needs to take further steps to work correctly also on BE hosts. (Maybe
> that's le32_to_cpu(*pix32), maybe drm_fb_swab(), maybe something else?)
>
>
>> I didn't check if dev->mode_config.quirk_addfb_prefer_host_byte_order
>> is set
>
> I can't tell if that's helpful, probably Noralf can.
>
I skimmed a discussion a while back around BE and all it's problems
through the whole graphics stack and my take away was that I needed a BE
machine so I could test the whole stack to make sure it works.
Judging from what Ilia says I've been wrong in my assumptions so the
driver is probably broken on BE (along with my SPI display drivers).
I'm not going to try and fix this now, I need someone with a BE machine
that can test the whole stack and make sure it actually works. Me doing
more guesswork is not going to work out well I think :)
I'll add a FIXME in gud_pipe.c that BE is probably broken and link to
this discussion.
It seems Gerd did some work to fix BE in the kernel:
drm: byteorder fixes
https://patchwork.freedesktop.org/series/49073/
But that seems to deal with the format value itself and not the buffer
contents.
Noralf.
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-03-12 11:54 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-05 16:31 [PATCH v7 0/3] GUD USB Display driver Noralf Trønnes
2021-03-05 16:31 ` [PATCH v7 1/3] drm/uapi: Add USB connector type Noralf Trønnes
2021-03-05 16:31 ` [PATCH v7 2/3] drm/probe-helper: Check epoch counter in output_poll_execute() Noralf Trønnes
2021-03-05 16:31 ` [PATCH v7 3/3] drm: Add GUD USB Display driver Noralf Trønnes
2021-03-08 18:13 ` Noralf Trønnes
2021-03-09 14:02 ` Peter Stuge
2021-03-09 18:07 ` Noralf Trønnes
2021-03-09 18:16 ` Noralf Trønnes
2021-03-10 4:55 ` Peter Stuge
2021-03-10 11:43 ` Noralf Trønnes
2021-03-11 14:48 ` Peter Stuge
2021-03-11 17:16 ` Noralf Trønnes
2021-03-11 20:02 ` Peter Stuge
2021-03-11 21:36 ` Ilia Mirkin
2021-03-11 22:57 ` Peter Stuge
2021-03-12 0:19 ` Ilia Mirkin
2021-03-12 4:32 ` Peter Stuge
2021-03-12 11:53 ` Noralf Trønnes
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).