| Message ID | 20210219121702.50964-1-noralf@tronnes.org |
|---|---|
| Headers | show |
| Series | GUD USB Display driver | expand |
Hi Noralf, Noralf Trønnes wrote: > +++ b/drivers/gpu/drm/gud/gud_connector.c .. > +static int gud_connector_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len) .. > + struct gud_connector *gconn = ctx->gconn; > + size_t start = block * EDID_LENGTH; > + > + if (start + len > gconn->edid_len) > + return -1; > + > + if (!block) { > + struct gud_device *gdrm = to_gud_device(gconn->connector.dev); > + int ret; > + > + /* Check because drm_do_get_edid() will retry on failure */ > + if (!ctx->buf) > + ctx->buf = kmalloc(gconn->edid_len, GFP_KERNEL); > + if (!ctx->buf) > + return -1; > + > + ret = gud_usb_get(gdrm, GUD_REQ_GET_CONNECTOR_EDID, gconn->connector.index, > + ctx->buf, gconn->edid_len); .. > + memcpy(buf, ctx->buf + start, len); Danger, danger? gconn->edid_len in this call to gud_usb_get() comes from the device in gud_connector_status_request() where the only validation is that edid_len % EDID_LENGTH == 0, so a device could write past the buffer if drm_do_get_edid() passes a buffer smaller than edid_len. I guess the buffer passed is just 128, EDID_LENGTH, so a malicious or buggy device could overwrite 64k-128 kernel memory? Ouch! More generally it's not very typical in USB to report the data size separately from the data itself, if reporting size explicitly at all. Sizes can be part of the data structure itself (like in descriptors) but on the application layer (like here) it's convenient to just decide a sensible fixed maximum size and let the host try to always transfer that size while accepting short transfers. Unlike read() a short transfer only ever happens if and when a device intends for it, so that's like an in-band handshake but "for free". Oh, and does/should the GUD EDID change if the panel "behind" the device CPU on a hotpluggable connector changes? It wouldn't be great to require GUD driver reprobe in that case. But maybe DRM requires that anyway? I'm sorry I didn't spot this pattern earlier, I understand that it's late in the game and that changing it needs the gadget to change as well, but I do really think this is a worthwhile change throughout the protocol. And I think it applies to more than EDID, e.g. both GUD and connector properties, maybe formats, something else? Unfortunately, the gud_usb_control_msg() check (ret != len) creates a requirement to know in advance how much data will be transfered. That could be revised at least for the general case, even if not used everywhere; maybe something like adding a size_t required_min_len parameter to gud_usb_control_msg()? > +static int gud_connector_get_modes(struct drm_connector *connector) > +{ > + struct gud_connector *gconn = to_gud_connector(connector); > + struct gud_device *gdrm = to_gud_device(connector->dev); > + struct gud_connector_get_edid_ctx edid_ctx = { > + .gconn = gconn, > + }; > + struct gud_display_mode_req *reqmodes = NULL; > + unsigned int i, num_modes = 0; The error path of this function executes "return num_modes" with num_modes unmodified; ie. 0. Is that intentional? > +static int gud_connector_add_tv_mode(struct gud_device *gdrm, .. > + buf_len = num_modes * GUD_CONNECTOR_TV_MODE_NAME_LEN; > + modes = kmalloc_array(num_modes, sizeof(*modes), GFP_KERNEL); > + buf = kmalloc(buf_len, GFP_KERNEL); Maybe moving the buf assignment immediately following the buf_len assignment would help readability? This is quite minor. > +static int gud_connector_add_properties(struct gud_device *gdrm, struct gud_connector *gconn, > + unsigned int num_properties) > +{ > + struct drm_device *drm = &gdrm->drm; > + struct drm_connector *connector = &gconn->connector; > + struct gud_property_req *properties; > + unsigned int i; > + int ret; > + > + gconn->properties = kcalloc(num_properties, sizeof(*gconn->properties), GFP_KERNEL); > + if (!gconn->properties) > + return -ENOMEM; > + > + properties = kcalloc(num_properties, sizeof(*properties), GFP_KERNEL); > + if (!properties) > + return -ENOMEM; I think this error path leaks gconn->properties? > +int gud_connector_create(struct gud_device *gdrm, unsigned int index) Most error paths in this function seem to leak both gconn and connector? > +++ b/drivers/gpu/drm/gud/gud_drv.c .. > +static int gud_usb_get_status(struct usb_device *usb, u8 ifnum, u8 *status) .. > + ret = gud_usb_control_msg(usb, ifnum, true, GUD_REQ_GET_STATUS, 0, buf, sizeof(*buf)); > + *status = *buf; Maybe make this conditional on 0 == ret. > +static int gud_set_version(struct usb_device *usb, u8 ifnum, u32 flags, u8 version) .. > + if (ret == -EPIPE) > + return -EPROTONOSUPPORT; So yeah, this isn't typical, devices usually describe optional things that the driver may need to know about, unless it's something that can change during operation. Arguably mildly contradictory to the short transfer pattern, but one is capability and the other is "runtime" data. > +static int gud_get_properties(struct gud_device *gdrm, unsigned int num_properties) > +{ > + struct gud_property_req *properties; > + unsigned int i; > + int ret; > + > + if (!num_properties) > + return 0; > + > + gdrm->properties = drmm_kcalloc(&gdrm->drm, num_properties, sizeof(*gdrm->properties), > + GFP_KERNEL); > + if (!gdrm->properties) > + return -ENOMEM; > + > + properties = kcalloc(num_properties, sizeof(*properties), GFP_KERNEL); > + if (!properties) > + return -ENOMEM; It looks like this function leaks gdrm->properties in all error paths? > + default: > + /* New ones might show up in future devices, skip those we don't know. */ > + drm_dbg(&gdrm->drm, "Unknown property: %u\n", prop); Maybe "Ignoring unknown property: %u\n" would be a little more clear? > +static int gud_stats_debugfs(struct seq_file *m, void *data) .. > + seq_puts(m, "Compression: "); > + if (gdrm->compression & GUD_COMPRESSION_LZ4) > + seq_puts(m, " lz4"); > + seq_puts(m, "\n"); Maybe an explicit seq_puts(m, " none") if there are none? > + 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); > + } Will the fraction ever need zero padding? > +static int gud_probe(struct usb_interface *interface, const struct usb_device_id *id) I appreciate very much that GUD works on interface level, so that it can also be used in composite devices at some point. Thanks a lot! \o/ > +++ b/drivers/gpu/drm/gud/gud_pipe.c .. > +int gud_pipe_check(struct drm_simple_display_pipe *pipe, .. > + 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)) > + return -EINVAL; req leaks? > + req->connector = drm_connector_index(connector); > + req->num_properties = num_properties; > + > + num_properties = gud_connector_fill_properties(connector, connector_state, > + req->properties); Following this new assignment to num_properties the new value is used to (hopefully!) append at the first req->properties[] index after the old value was used as index, that doesn't feel great.. I mean, it's harmless as long as gud_connector_fill_properties() is sure to return the same value, but, well, maybe sometime later it doesn't, or is that guaranteed if there is no error? Then maybe at least document that requirement by the function. What do you think? Thanks a lot //Peter
Den 19.02.2021 22.42, skrev Peter Stuge: > Hi Noralf, > > Noralf Trønnes wrote: >> +++ b/drivers/gpu/drm/gud/gud_connector.c > .. >> +static int gud_connector_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len) > .. >> + struct gud_connector *gconn = ctx->gconn; >> + size_t start = block * EDID_LENGTH; >> + >> + if (start + len > gconn->edid_len) >> + return -1; >> + >> + if (!block) { >> + struct gud_device *gdrm = to_gud_device(gconn->connector.dev); >> + int ret; >> + >> + /* Check because drm_do_get_edid() will retry on failure */ >> + if (!ctx->buf) >> + ctx->buf = kmalloc(gconn->edid_len, GFP_KERNEL); >> + if (!ctx->buf) >> + return -1; >> + >> + ret = gud_usb_get(gdrm, GUD_REQ_GET_CONNECTOR_EDID, gconn->connector.index, >> + ctx->buf, gconn->edid_len); > .. >> + memcpy(buf, ctx->buf + start, len); > > Danger, danger? > > gconn->edid_len in this call to gud_usb_get() comes from the device in > gud_connector_status_request() where the only validation is that > edid_len % EDID_LENGTH == 0, so a device could write past the buffer > if drm_do_get_edid() passes a buffer smaller than edid_len. > > I guess the buffer passed is just 128, EDID_LENGTH, so a malicious > or buggy device could overwrite 64k-128 kernel memory? Ouch! > The result goes into ctx->buf which is big enough. Then each edid block is copied from that buffer as the parser runs the callback. Maybe I should add a cap on ->edid_len, but I don't know how big it should be. There's no danger as such not having a cap, the host will either fail to allocate memory (max 4MB usually) or do a very big transfer and error out in the edid parser. > > More generally it's not very typical in USB to report the data size > separately from the data itself, if reporting size explicitly at all. > > Sizes can be part of the data structure itself (like in descriptors) but > on the application layer (like here) it's convenient to just decide a > sensible fixed maximum size and let the host try to always transfer > that size while accepting short transfers. Unlike read() a short > transfer only ever happens if and when a device intends for it, > so that's like an in-band handshake but "for free". > > Oh, and does/should the GUD EDID change if the panel "behind" the device > CPU on a hotpluggable connector changes? It wouldn't be great to require > GUD driver reprobe in that case. But maybe DRM requires that anyway? > If gud_connector_status_req.status has changed since last poll or GUD_CONNECTOR_STATUS_CHANGED is set, DRM will notify userspace which will reprobe the connector. connector->epoch_counter++ in gud_connector_status_request() triggers that. > > I'm sorry I didn't spot this pattern earlier, I understand that it's late > in the game and that changing it needs the gadget to change as well, but I > do really think this is a worthwhile change throughout the protocol. > I see what you mean, I'll give it a try. > And I think it applies to more than EDID, e.g. both GUD and connector > properties, maybe formats, something else? > > > Unfortunately, the gud_usb_control_msg() check (ret != len) creates a > requirement to know in advance how much data will be transfered. > > That could be revised at least for the general case, even if not used > everywhere; maybe something like adding a size_t required_min_len > parameter to gud_usb_control_msg()? > > >> +static int gud_connector_get_modes(struct drm_connector *connector) >> +{ >> + struct gud_connector *gconn = to_gud_connector(connector); >> + struct gud_device *gdrm = to_gud_device(connector->dev); >> + struct gud_connector_get_edid_ctx edid_ctx = { >> + .gconn = gconn, >> + }; >> + struct gud_display_mode_req *reqmodes = NULL; >> + unsigned int i, num_modes = 0; > > The error path of this function executes "return num_modes" with num_modes > unmodified; ie. 0. Is that intentional? > It's not allowed to return an error code so 0 is the only option. The caller drm_helper_probe_single_connector_modes() adds a fallback 1024x768 mode in that case which probably won't fit for a display panel, but the device should reject an illegal mode on GUD_REQ_SET_STATE_CHECK so we're fine. > >> +static int gud_connector_add_tv_mode(struct gud_device *gdrm, > .. >> + buf_len = num_modes * GUD_CONNECTOR_TV_MODE_NAME_LEN; >> + modes = kmalloc_array(num_modes, sizeof(*modes), GFP_KERNEL); >> + buf = kmalloc(buf_len, GFP_KERNEL); > > Maybe moving the buf assignment immediately following the buf_len assignment > would help readability? This is quite minor. > > >> +static int gud_connector_add_properties(struct gud_device *gdrm, struct gud_connector *gconn, >> + unsigned int num_properties) >> +{ >> + struct drm_device *drm = &gdrm->drm; >> + struct drm_connector *connector = &gconn->connector; >> + struct gud_property_req *properties; >> + unsigned int i; >> + int ret; >> + >> + gconn->properties = kcalloc(num_properties, sizeof(*gconn->properties), GFP_KERNEL); >> + if (!gconn->properties) >> + return -ENOMEM; >> + >> + properties = kcalloc(num_properties, sizeof(*properties), GFP_KERNEL); >> + if (!properties) >> + return -ENOMEM; > > I think this error path leaks gconn->properties? > It's freed in gud_connector_destroy() which will be called on error at this stage. > >> +int gud_connector_create(struct gud_device *gdrm, unsigned int index) > > Most error paths in this function seem to leak both gconn and connector? > Everything that happens after the drm_connector_init() call is cleaned up automatically when the DRM device is torn down. devm_drm_dev_alloc() and drmm_mode_config_init() sets this up. > >> +++ b/drivers/gpu/drm/gud/gud_drv.c > .. >> +static int gud_usb_get_status(struct usb_device *usb, u8 ifnum, u8 *status) > .. >> + ret = gud_usb_control_msg(usb, ifnum, true, GUD_REQ_GET_STATUS, 0, buf, sizeof(*buf)); >> + *status = *buf; > > Maybe make this conditional on 0 == ret. > > >> +static int gud_set_version(struct usb_device *usb, u8 ifnum, u32 flags, u8 version) > .. >> + if (ret == -EPIPE) >> + return -EPROTONOSUPPORT; > > So yeah, this isn't typical, devices usually describe optional things that > the driver may need to know about, unless it's something that can change > during operation. > > Arguably mildly contradictory to the short transfer pattern, but one is > capability and the other is "runtime" data. > > >> +static int gud_get_properties(struct gud_device *gdrm, unsigned int num_properties) >> +{ >> + struct gud_property_req *properties; >> + unsigned int i; >> + int ret; >> + >> + if (!num_properties) >> + return 0; >> + >> + gdrm->properties = drmm_kcalloc(&gdrm->drm, num_properties, sizeof(*gdrm->properties), >> + GFP_KERNEL); >> + if (!gdrm->properties) >> + return -ENOMEM; >> + >> + properties = kcalloc(num_properties, sizeof(*properties), GFP_KERNEL); >> + if (!properties) >> + return -ENOMEM; > > It looks like this function leaks gdrm->properties in all error paths? > drmm_kcalloc() is the DRM version of devm_kcalloc() which frees the memory when the last DRM fd is closed. There might be open fd's when the device goes away so the devm_ versions can't be used. > >> + default: >> + /* New ones might show up in future devices, skip those we don't know. */ >> + drm_dbg(&gdrm->drm, "Unknown property: %u\n", prop); > > Maybe "Ignoring unknown property: %u\n" would be a little more clear? > Sure. > >> +static int gud_stats_debugfs(struct seq_file *m, void *data) > .. >> + seq_puts(m, "Compression: "); >> + if (gdrm->compression & GUD_COMPRESSION_LZ4) >> + seq_puts(m, " lz4"); >> + seq_puts(m, "\n"); > > Maybe an explicit seq_puts(m, " none") if there are none? > That makes sense. > >> + 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); >> + } > > Will the fraction ever need zero padding? > No, I don't see why. > >> +static int gud_probe(struct usb_interface *interface, const struct usb_device_id *id) > > I appreciate very much that GUD works on interface level, so that it > can also be used in composite devices at some point. Thanks a lot! \o/ > This was a design requirement since I wanted to make room for at least HID touch and maybe audio. It will also tolerate other vendor class interfaces and just silently ignore them. The Pi images on the wiki has the ability to add a serial console USB interface for debugging. > >> +++ b/drivers/gpu/drm/gud/gud_pipe.c > .. >> +int gud_pipe_check(struct drm_simple_display_pipe *pipe, > .. >> + 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)) >> + return -EINVAL; > > req leaks? > Yep, you're right. > >> + req->connector = drm_connector_index(connector); >> + req->num_properties = num_properties; >> + >> + num_properties = gud_connector_fill_properties(connector, connector_state, >> + req->properties); > > Following this new assignment to num_properties the new value is used > to (hopefully!) append at the first req->properties[] index after the > old value was used as index, that doesn't feel great.. > > I mean, it's harmless as long as gud_connector_fill_properties() is > sure to return the same value, but, well, maybe sometime later it > doesn't, or is that guaranteed if there is no error? Then maybe at > least document that requirement by the function. What do you think? > The number of properties doesn't change, it is returned by GUD_REQ_GET_CONNECTOR during probe. There is a small comment in gud_connector_fill_properties() but I can expand on that and put it before the function definition. Thanks for your scrutiny. Noralf.
Hi Noralf, Noralf Trønnes wrote: > 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. I forgot, but I have a two-tone (black/red) e-ink display here, and I also have a 3-bpp RGB TFT display. Should we add maybe R2 and R3? (or R3/R8 for number of colours?) I'm particularly considering the 3-bpp RGB panel for GUD use now, and while it will surely work with say a 16-bit RGB mode many bits will be wasted in the process. What are your thoughts? Would you take a patch for that now, later, never? //Peter
Den 20.02.2021 18.27, skrev Noralf Trønnes: > > > Den 19.02.2021 22.42, skrev Peter Stuge: >> >> More generally it's not very typical in USB to report the data size >> separately from the data itself, if reporting size explicitly at all. >> >> Sizes can be part of the data structure itself (like in descriptors) but >> on the application layer (like here) it's convenient to just decide a >> sensible fixed maximum size and let the host try to always transfer >> that size while accepting short transfers. Unlike read() a short >> transfer only ever happens if and when a device intends for it, >> so that's like an in-band handshake but "for free". >> >> Oh, and does/should the GUD EDID change if the panel "behind" the device >> CPU on a hotpluggable connector changes? It wouldn't be great to require >> GUD driver reprobe in that case. But maybe DRM requires that anyway? >> > > If gud_connector_status_req.status has changed since last poll or > GUD_CONNECTOR_STATUS_CHANGED is set, DRM will notify userspace which > will reprobe the connector. connector->epoch_counter++ in > gud_connector_status_request() triggers that. > >> >> I'm sorry I didn't spot this pattern earlier, I understand that it's late >> in the game and that changing it needs the gadget to change as well, but I >> do really think this is a worthwhile change throughout the protocol. >> > > I see what you mean, I'll give it a try. > Peter, please have a look at this diff and see if I'm on the right track here: https://gist.github.com/notro/a43a93a3aa0cc75d930890b7b254fc0a I want to avoid waisting a patch version cycle by being way off. Noralf.
Noralf Trønnes wrote: > Peter, please have a look at this diff and see if I'm on the right track > here: https://gist.github.com/notro/a43a93a3aa0cc75d930890b7b254fc0a Yes that's exactly what I meant; this way the possibility for contradicting sizes is eliminated by protocol and not just by implementation - very nice! Some more comments, sorry if this is just because of ongoing work: Perhaps the functions taking usb_device + ifnum could take usb_interface instead - but I don't know if that would simplify or complicate things. Alan mentioned this idea in similar circumstances in another thread. I don't feel strongly, but perhaps it's cleaner. gud_usb_control_msg() now seems almost redundant, maybe it could be removed. In gud_usb_set() if NULL == buf then that's passed to usb_control_msg() along with len, which likely crashes if len > 0, so it may be good to check or enforce that, maybe with else len=0; before the gud_usb_transfer() call. Finally a small style note that I'd personally change a few if (ret > 0) { blocks to have one indent level less and do each check right away, e.g. in gud_connector_get_modes(): ret = gud_usb_get() if (ret % EDID_LENGTH) { drm_err(); } else if (ret > 0) { edid_ctx.len = ret; edid = drm_do_get_edid(); } and later on in the function by the display modes one indent level could be saved with a goto: if (ret <= 0) goto out; but obviously no huge deal. In general it's really helpful for device development to see error messages when the device behaves incorrectly, the "Invalid .. size" errors are great examples of this, but e.g. gud_get_display_descriptor() returns -EIO without a message. Maybe there are opportunities for further helpful error messages? Thanks a lot and kind regards //Peter
Den 28.02.2021 02.52, skrev Peter Stuge: > Noralf Trønnes wrote: >> Peter, please have a look at this diff and see if I'm on the right track >> here: https://gist.github.com/notro/a43a93a3aa0cc75d930890b7b254fc0a > > Yes that's exactly what I meant; this way the possibility for contradicting > sizes is eliminated by protocol and not just by implementation - very nice! > > Some more comments, sorry if this is just because of ongoing work: > > Perhaps the functions taking usb_device + ifnum could take usb_interface > instead - but I don't know if that would simplify or complicate things. > Alan mentioned this idea in similar circumstances in another thread. > I don't feel strongly, but perhaps it's cleaner. > I agree it's cleaner, this way I don't have to store the interface number in gdrm. > gud_usb_control_msg() now seems almost redundant, maybe it could be removed. > There are 4 callers so I think it makes sense still. > In gud_usb_set() if NULL == buf then that's passed to usb_control_msg() > along with len, which likely crashes if len > 0, so it may be good to > check or enforce that, maybe with else len=0; before the gud_usb_transfer() > call. > Ok. > Finally a small style note that I'd personally change a few if (ret > 0) { > blocks to have one indent level less and do each check right away, e.g. in > gud_connector_get_modes(): > > ret = gud_usb_get() > if (ret % EDID_LENGTH) { > drm_err(); > } else if (ret > 0) { > edid_ctx.len = ret; > edid = drm_do_get_edid(); > } > > and later on in the function by the display modes one indent level > could be saved with a goto: > > if (ret <= 0) > goto out; > > but obviously no huge deal. > It makes for a better read so I'll do that. > > In general it's really helpful for device development to see error messages > when the device behaves incorrectly, the "Invalid .. size" errors are great > examples of this, but e.g. gud_get_display_descriptor() returns -EIO without > a message. Maybe there are opportunities for further helpful error messages? > The message is printed by the caller: ret = gud_get_display_descriptor(intf, &desc); if (ret) { DRM_DEV_DEBUG_DRIVER(dev, "Not a display interface: ret=%d\n", ret); return -ENODEV; } It's a debug message enabled by writing to /sys/module/drm/parameters/debug. The reason for not making it an error message, is that I want the driver to just ignore non-display vendor class interfaces so they can co-exist on the device. Someone might make an open protocol gpio (vendor class) interface driver some day, or adc, i2c, spi, rtc, or... Thanks, Noralf.
Hi Noralf,
Peter Stuge wrote:
> I'll prepare a test setup for the RGB-TFT on the weekend.
So implementing a GUD and looking at the protocol from yet another
angle gives more new insights - surprise. :)
Here are some thoughts so far:
* GUD_REQ_SET_VERSION does still rub me wrong; it seems potentially
quite complex to maintain compatibility in two places; both for host
and device. I don't want to insist on removing it, but at a minimum
it's quite unusual.
Part of the idea in USB is that host software updates are easy if
not fully automated but device firmware updates less so, thus
complexity is rather placed in the host.
* It's unclear to me from reading the header files in this v6 patch set
which GUD_REQ:s and which properties are actually mandatory in devices.
I'm getting hints from your STM32 device and reading the driver code in
detail, but what do you think is a good way to document what's required
vs. optional?
* GUD_REQ_SET_BUFFER my old nemesis. :) It's great that it's optional!
But do you see any way to turn it into a bulk message, in order to
remove the memory barrier effect of a control transfer before bulk?
I think it would be possible to noticeably improve performance later,
by changing the host driver to submit asynchronous bulk transfers for
frame data rather than waiting for each transfer to finish; bulk
transfers will then pack optimally on the wire - but with a control
transfer in between there's no chance of achieving that.
Having only one kind of transfer in the hot path would also simplify
canceling still pending transfers (when using async later) if new data
gets flushed before the previous frame is completely transfered.
* A fair bit of the EDID isn't used or has dummy values. Have you already
considered and dismissed some other ways of accomplishing the same?
* Sorry if I've asked before - but what's the purpose of
GUD_REQ_SET_STATE_CHECK and GUD_REQ_SET_STATE_COMMIT? Why/when does
drm do pipe check vs. update?
* How do you feel about passing the parameters for
GUD_REQ_SET_CONTROLLER_ENABLE and GUD_REQ_SET_DISPLAY_ENABLE in wValue?
It would save the transfer data stage.
Kind regards
//Peter