Morning Laurent
On 18/12/2022 23:04, Laurent Pinchart wrote:
On Fri, Dec 16, 2022 at 03:53:05PM +0000, Dan Scally wrote:
On 15/12/2022 11:51, Kieran Bingham wrote:
Quoting Daniel Scally (2022-12-13 08:37:35)
In preparation for allowing more than the default color matching
descriptor, make the color matching attributes writeable.
Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
---
.../ABI/testing/configfs-usb-gadget-uvc | 2 +-
drivers/usb/gadget/function/uvc_configfs.c | 32 ++++++++++++++++++-
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index 611b23e6488d..3512f4899fe3 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
@@ -165,7 +165,7 @@ Date: Dec 2014
KernelVersion: 4.0
Description: Default color matching descriptors
- All attributes read only:
+ All attributes read/write:
Do we need to specify here what acceptable values can now be written at
all?
Yes I guess so...we probably need better documentation for this
somewhere. I don't think this file is the right place to describe it
fully, it's more of an enumeration. We probably need something like
Documentation/usb/gadget_serial.rst
That would make sense. I think you can also heavily quote or reference
the USB video class documentation.
Ack. I've started this.
======================== ======================================
bMatrixCoefficients matrix used to compute luma and
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 6f5932c9f09c..4fbc42d738a4 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -1845,7 +1845,37 @@ static ssize_t uvcg_color_matching_##cname##_show( \
return result; \
} \
\
-UVC_ATTR_RO(uvcg_color_matching_, cname, aname)
+static ssize_t uvcg_color_matching_##cname##_store( \
+ struct config_item *item, const char *page, size_t len) \
+{ \
+ struct config_group *group = to_config_group(item); \
+ struct mutex *su_mutex = &group->cg_subsys->su_mutex; \
+ struct uvcg_cmd *cmd = to_uvcg_cmd(group); \
+ struct f_uvc_opts *opts; \
+ struct config_item *opts_item; \
+ int ret; \
I'd make this one last.
Hah I overthought this quite a lot because following the preprocessing
num might be shorter. No problem though, I'll switch it to last.
+ u##bits num; \
In other places we use
typeof(cmd->desc.aname) num;
Up to you.
+ \
+ ret = kstrtou##bits(page, 0, &num); \
+ if (ret) \
+ return ret; \
I don't know how horrible it would be - or if there's any other
precendence, but I'm weary that setting '1', or '4' in here from
userspace is fairly meaningless.
Of course - the user doing so would have to know from the spec perhaps
what they are configuring - but it makes me wonder if we should support
string matching in here to also convert say "BT.709" to the appropriate
integer value (if a non-integer was set).
It may depend on how 'most' other configfs entries that would be similar
to this would expect to operate.
This might be abusing configfs slightly, though I did something similar
for the custom string descriptors (see [1] and ctrl-f for
"uvcg_language_strings_show") and I think it's a worthwhile thing. What
about an "enumerate options" attribute that's read only and simply
enumerates the settings with their corresponding descriptions? The
problem with string parsing is you've still got to know the exact string
to pass (and googling "BT.709" tells me it can also be called "Rec.709",
"Rec. 709" and "ITU 709" for example) so you'd have to look it up
anyway. I'm thinking something like:
$ cat enumerate_options
bColorPrimaries - This defines the color primaries and the reference white
value desc
0 Unspecified (Image characteristics unknown)
1 BT.709 (sRGB)
2 BT.470-2 (M)
3 BT.470 (B, G)
4 SMPTE 170M
5 SMPTE 240M
6-255 Reserved
bTransferCharacteristics - This field defines the opto-electronic transfer
characteristics of the
source picture, also
called the gamma function
value desc
0 Unspecified (Image characteristics unknown)
1 BT.7-0
2 BT.470-2 M
... ...
... and so on. What do you think?
I think it's overkill. The userspace side of the UVC gadget is expected
to have good knowledge of UVC. This isn't an interface meant for users.
Fair enough
[1] https://lore.kernel.org/linux-usb/20221121092517.225242-6-dan.scally@xxxxxxxxxxxxxxxx/
+ \
+ mutex_lock(su_mutex); /* for navigating configfs hierarchy */ \
+ \
+ opts_item = group->cg_item.ci_parent->ci_parent->ci_parent; \
+ opts = to_f_uvc_opts(opts_item); \
+ \
+ mutex_lock(&opts->lock); \
Don't you need a similar linked check as in uvcg_frame_##cname##_store()
?
Yes, actually. I was halfway through explaining why I had considered it
unnecessary and skipped it, and then realised I was wrong :)
+ \
+ cmd->desc.aname = num; \
+ ret = len; \
+ \
+ mutex_unlock(&opts->lock); \
+ mutex_unlock(su_mutex); \
+ \
+ return ret; \
+} \
+UVC_ATTR(uvcg_color_matching_, cname, aname)
UVCG_COLOR_MATCHING_ATTR(b_color_primaries, bColorPrimaries, 8);
UVCG_COLOR_MATCHING_ATTR(b_transfer_characteristics, bTransferCharacteristics, 8);
