Hi,
I missed this completely until now. Noralf Trønnes noralf@tronnes.org writes:
This adds the gadget side support for the Generic USB Display. It presents a DRM display device as a USB Display configured through configfs.
The display is implemented as a vendor type USB interface with one bulk out endpoint. The protocol is implemented using control requests. lz4 compressed framebuffer data/pixels are sent over the bulk endpoint.
The DRM part of the gadget is placed in the DRM subsystem since it reaches into the DRM internals.
First and foremost, could this be done in userspace using the raw gadget or f_fs?
diff --git a/drivers/usb/gadget/function/f_gud_drm.c b/drivers/usb/gadget/function/f_gud_drm.c new file mode 100644 index 000000000000..9a2d6bb9739f --- /dev/null +++ b/drivers/usb/gadget/function/f_gud_drm.c @@ -0,0 +1,678 @@ +struct f_gud_drm {
- struct usb_function func;
- struct work_struct worker;
why do you need a worker?
- size_t max_buffer_size;
- void *ctrl_req_buf;
- u8 interface_id;
- struct usb_request *ctrl_req;
- struct usb_ep *bulk_ep;
- struct usb_request *bulk_req;
single request? Don't you want to amortize the latency of queue->complete by using a series of requests?
- struct gud_drm_gadget *gdg;
- spinlock_t lock; /* Protects the following members: */
- bool ctrl_pending;
- bool status_pending;
- bool bulk_pending;
- bool disable_pending;
could this be a single u32 with #define'd flags? That would be atomic, right?
- u8 errno;
a global per-function error? Why?
- u16 request;
- u16 value;
also why? Looks odd
+};
+static inline struct f_gud_drm *func_to_f_gud_drm(struct usb_function *f)
let the compiler inline for you
+{
- return container_of(f, struct f_gud_drm, func);
could be a macro, but okay.
+static inline struct f_gud_drm_opts *fi_to_f_gud_drm_opts(const struct usb_function_instance *fi)
ditto
+{
- return container_of(fi, struct f_gud_drm_opts, func_inst);
ditto
+}
+static inline struct f_gud_drm_opts *ci_to_f_gud_drm_opts(struct config_item *item)
ditto
+{
- return container_of(to_config_group(item), struct f_gud_drm_opts,
func_inst.group);
ditto
+}
+#define F_MUD_DEFINE_BULK_ENDPOINT_DESCRIPTOR(name, addr, size) \
- static struct usb_endpoint_descriptor name = { \
const? Also, please check all the others
+static void f_gud_drm_bulk_complete(struct usb_ep *ep, struct usb_request *req) +{
- struct f_gud_drm *fgd = req->context;
- unsigned long flags;
- if (req->status || req->actual != req->length)
return;
so, if we complete with an erroneous status or a short packet, you'll ignore it?
- spin_lock_irqsave(&fgd->lock, flags);
- fgd->bulk_pending = true;
- spin_unlock_irqrestore(&fgd->lock, flags);
- queue_work(system_long_wq, &fgd->worker);
Do you need to offset this to a worker?
+static int f_gud_drm_ctrl_req_set_buffer(struct f_gud_drm *fgd, void *buf, size_t len) +{
- int ret;
- if (len != sizeof(struct gud_drm_req_set_buffer))
return -EINVAL;- ret = gud_drm_gadget_set_buffer(fgd->gdg, buf);
- if (ret < 0)
return ret;- if (ret > fgd->max_buffer_size)
return -EOVERFLOW;- fgd->bulk_req->length = ret;
- return usb_ep_queue(fgd->bulk_ep, fgd->bulk_req, GFP_KERNEL);
+}
+static void f_gud_drm_worker(struct work_struct *work) +{
- struct f_gud_drm *fgd = container_of(work, struct f_gud_drm, worker);
- bool ctrl_pending, bulk_pending, disable_pending;
- struct gud_drm_gadget *gdg = fgd->gdg;
- unsigned long flags;
- u16 request, value;
- int ret;
- spin_lock_irqsave(&fgd->lock, flags);
- request = fgd->request;
- value = fgd->value;
- ctrl_pending = fgd->ctrl_pending;
- bulk_pending = fgd->bulk_pending;
- disable_pending = fgd->disable_pending;
- spin_unlock_irqrestore(&fgd->lock, flags);
- pr_debug("%s: bulk_pending=%u ctrl_pending=%u disable_pending=%u\n",
__func__, bulk_pending, ctrl_pending, disable_pending);
could we use dev_dbg() at least?