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

[Noralf Trønnes]

Den 15.03.2021 20.37, skrev Peter Stuge:
> 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?
> 

Yeah, that would be nice.

We can't look for -EPIPE though, since GUD_REQ_GET_STATUS will ask for
the actual error. We have these to choose from currently:

  #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

Maybe REQUEST_NOT_SUPPORTED (-EOPNOTSUPP) or add a more fitting status
value.

If the driver sees -EPIPE this means that the device has failed to
respond properly. See gud_usb_transfer().

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

It does ignore all but the first... doesn't it?

You could make a patch if you care about this:

		case GUD_DRM_FORMAT_R1:
			fallthrough;
		case GUD_DRM_FORMAT_XRGB1111:
			if (!xrgb8888_emulation_format)
				xrgb8888_emulation_format = info;
+			else
+				dev_err(dev, "...");
			break;

It's only needed for the formats that are not exported to userspace.

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

There's no way for userspace to know that. drm_fb_xrgb8888_to_gray8()
uses ITU BT.601 rgb conversion so userspace doesn't have to know which
colors to use, but ofc it will need to know there's a monochrome display
for it to look good.

XRGB8888 is the only format that is allowed to be emulated since some
userspace only supports that one format. So we can't have a device that
supports both R1 and XRGB1111.

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

It could but I'd like to keep the increasing bits-per-pixel order.

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

I'm reusing the tv state property since that saves me from subclassing
the connector state. I want to have the value in the state because that
makes it less of a special case. Some time in the future DRM will have
proper backlight support as a DRM property, but so far no one has been
willing to invest the necessary time and effort to make it happen.

Noralf.