Hi Noralf,
super fair call with the BE testing, let's hope for some testing soonish.
I was thinking about my device doing protocol STALL when I try to return 0 bytes, and while it *is* a bug in my device, from a standards point of view it's actually completely valid, if not expected:
--8<-- usb_20.pdf 8.5.3.4 STALL Handshakes Returned by Control Pipes If the device is unable to complete a command, it returns a STALL in the Data and/or Status stages of the control transfer. Unlike the case of a functional stall, protocol stall does not indicate an error with the device. -->8--
I think it's fair to say that a device can't complete the command when it has no data to return.
So how about allowing STALL for optional GUD_REQ_GET_:s to mean the same as a 0 byte response? Should I propose a separate patch for it later?
Noralf Trønnes wrote:
+++ b/drivers/gpu/drm/gud/gud_connector.c
..
+static int gud_connector_get_modes(struct drm_connector *connector)
..
- ret = gud_usb_get(gdrm, GUD_REQ_GET_CONNECTOR_EDID, connector->index,
edid_ctx.buf, GUD_CONNECTOR_MAX_EDID_LEN);
if (ret == -EPIPE) ret = 0;
- 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);- }
+static int gud_connector_add_properties(struct gud_device *gdrm, struct gud_connector *gconn)
..
- ret = gud_usb_get(gdrm, GUD_REQ_GET_CONNECTOR_PROPERTIES, connector->index,
properties, GUD_CONNECTOR_PROPERTIES_MAX_NUM * sizeof(*properties));
if (ret == -EPIPE) ret = 0;
- if (ret <= 0)
goto out;- if (ret % sizeof(*properties)) {
ret = -EIO;goto out;- }
+++ b/drivers/gpu/drm/gud/gud_drv.c
.. ..
+static int gud_get_properties(struct gud_device *gdrm)
..
- ret = gud_usb_get(gdrm, GUD_REQ_GET_PROPERTIES, 0,
properties, GUD_PROPERTIES_MAX_NUM * sizeof(*properties));
if (ret == -EPIPE) ret = 0;
- if (ret <= 0)
goto out;- if (ret % sizeof(*properties)) {
ret = -EIO;goto out;- }
Then I looked whether a device could cause trouble in the driver by returning complex/unexpected data, and found this:
+static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
..
- /* Add room for emulated XRGB8888 */
- formats = devm_kmalloc_array(dev, GUD_FORMATS_MAX_NUM + 1, sizeof(*formats), GFP_KERNEL);
It looks like this +1 and the way xrgb8888_emulation_format works means that an interface will not always work correctly if multiple emulated formats (R1, XRGB1111, RGB565) are returned, because only one emulated mode is added after the loop, with struct drm_format_info for the last emulated format returned by the device. So userspace would only see the last emulated mode and the bulk output would only ever use that particular pixel format, any earlier ones would be unavailable?
If this is EWONTFIX then how about adding an error message if multiple emulated modes are returned and ignore all but the first, rather than all but the last?
Related: Can userspace find out which GUD_PIXEL_FORMAT_* is behind an emulated format? It's needed to decide how the emulated framebuffer should be used, in particular to not use G or B if GUD_PIXEL_FORMAT_R1.
switch (format) {case GUD_DRM_FORMAT_R1:fallthrough;case GUD_DRM_FORMAT_XRGB1111:if (!xrgb8888_emulation_format)xrgb8888_emulation_format = info;break;case DRM_FORMAT_RGB565:rgb565_supported = true;if (!xrgb8888_emulation_format)xrgb8888_emulation_format = info;break;
Could RGB565 go before XRGB111 (or R1) and also fallthrough; in this construct? Not terribly important, but the repetition caught my eye.
Then, in gud_connector.c I saw this, which surprised me:
+int gud_connector_fill_properties(struct drm_connector_state *connector_state, .. + if (prop == GUD_PROPERTY_BACKLIGHT_BRIGHTNESS) { + val = connector_state->tv.brightness; + } else {
Why is this using tv.brightness rather than say gconn->initial_brightness?
It looks like the end result might be the same because tv.brightness is set to gconn->initial_brightness in gud_connector_reset() but it's a little confusing to me, since a GUD backlight isn't a drm/TV thing?
Thanks a lot
//Peter