| Message ID | 20221213083736.2284536-1-dan.scally@ideasonboard.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | UVC Gadget: Extend color matching support | expand |
Hi Dan, Thank you for the series. On Tue, Dec 13, 2022 at 08:37:30AM +0000, Daniel Scally wrote: > The current UVC gadget implementation hardcodes a single color matching > descriptor and transmits it a single time following all the format and frame I'm not sure I would use "transmits" in this context. Descriptors are for sure transmitted over the wire, but all in one go, not as individual units (at least within a configuration descriptor). Maybe "includes" would be a better term ? This is nitpicking for the cover letter, but the comment applies more importantly to commit messages and code for the whole series. > descriptors. This is inflexible, and additionally applies only to the _last_ > format in the array of descriptors. > > This series extends the support such that the default descriptor can be amended > and is transmitted once-per-format instead of once-only, it then adds the ability > to create new color matching descriptors and associate them with particular formats. > The default color matching descriptor is retained and used where the user does not > link a new color matching descriptor to the format, so the default interaction > with userspace is unchanged from the current implementation. I wonder if we shouldn't drop the default descriptor. If userspace doesn't specify one, then we really can't know what colorimetry data applies to the frames. Instead of providing a default to the host, not providing any colorimetry information would be better. > Daniel Scally (6): > usb: gadget: usb: Remove "default" from color matching attributes > usb: gadget: uvc: Add struct for color matching in configs > usb: gadget: uvc: Copy color matching descriptor for each frame > usb: gadget: uvc: Remove the hardcoded default color matching > usb: gadget: uvc: Make color matching attributes read/write > usb: gadget: uvc: Allow creating new color matching descriptors > > .../ABI/testing/configfs-usb-gadget-uvc | 6 +- > drivers/usb/gadget/function/f_uvc.c | 9 - > drivers/usb/gadget/function/u_uvc.h | 1 - > drivers/usb/gadget/function/uvc_configfs.c | 247 +++++++++++++++--- > drivers/usb/gadget/function/uvc_configfs.h | 9 + > 5 files changed, 228 insertions(+), 44 deletions(-)
Morning Laurent On 18/12/2022 18:12, Laurent Pinchart wrote: > Hi Dan, > > Thank you for the series. > > On Tue, Dec 13, 2022 at 08:37:30AM +0000, Daniel Scally wrote: >> The current UVC gadget implementation hardcodes a single color matching >> descriptor and transmits it a single time following all the format and frame > I'm not sure I would use "transmits" in this context. Descriptors are > for sure transmitted over the wire, but all in one go, not as individual > units (at least within a configuration descriptor). Maybe "includes" > would be a better term ? This is nitpicking for the cover letter, but > the comment applies more importantly to commit messages and code for the > whole series. I've used the same term I think in the series yes. No problem; I'll swap to includes. > >> descriptors. This is inflexible, and additionally applies only to the _last_ >> format in the array of descriptors. >> >> This series extends the support such that the default descriptor can be amended >> and is transmitted once-per-format instead of once-only, it then adds the ability >> to create new color matching descriptors and associate them with particular formats. >> The default color matching descriptor is retained and used where the user does not >> link a new color matching descriptor to the format, so the default interaction >> with userspace is unchanged from the current implementation. > I wonder if we shouldn't drop the default descriptor. If userspace > doesn't specify one, then we really can't know what colorimetry data > applies to the frames. Instead of providing a default to the host, not > providing any colorimetry information would be better. According to the spec: "In the absence of this descriptor, or in the case of "Unspecified" values within the descriptor, color matching defaults will be assumed. The color matching defaults are compliant with sRGB since the BT.709 transfer function and the sRGB transfer function are very similar" And it goes on to identify the default values for each of the descriptor's fields...which happen to be the values that are set in our default descriptor. So I think that including that default descriptor shouldn't change the host's behaviour, but does give userspace an easy way to see what's set...I think it's fine to keep. > >> Daniel Scally (6): >> usb: gadget: usb: Remove "default" from color matching attributes >> usb: gadget: uvc: Add struct for color matching in configs >> usb: gadget: uvc: Copy color matching descriptor for each frame >> usb: gadget: uvc: Remove the hardcoded default color matching >> usb: gadget: uvc: Make color matching attributes read/write >> usb: gadget: uvc: Allow creating new color matching descriptors >> >> .../ABI/testing/configfs-usb-gadget-uvc | 6 +- >> drivers/usb/gadget/function/f_uvc.c | 9 - >> drivers/usb/gadget/function/u_uvc.h | 1 - >> drivers/usb/gadget/function/uvc_configfs.c | 247 +++++++++++++++--- >> drivers/usb/gadget/function/uvc_configfs.h | 9 + >> 5 files changed, 228 insertions(+), 44 deletions(-)