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.
