Re: [PATCH 5/6] usb: gadget: uvc: Make color matching attributes read/write

[Dan Scally <dan.scally@ideasonboard.com>]

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@ideasonboard.com>
>> ---
>>   .../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

>
>>   
>>                  ========================  ======================================
>>                  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;                                                        \
>> +       u##bits num;                                                    \
>> +                                                                       \
>> +       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?


[1] 
https://lore.kernel.org/linux-usb/20221121092517.225242-6-dan.scally@ideasonboard.com/

>
>> +                                                                       \
>> +       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);                                        \
>> +                                                                       \
>> +       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);
>> -- 
>> 2.34.1
>>