@Laurent, I accidentally hit "Reply" instead of "Reply All", please
disregard the previous copy of this email
Hi Laurent and Paul,
On 08.06.2018 21:16, Laurent Pinchart wrote:
> Hi Joel and Paul,
>
> On Friday, 8 June 2018 22:12:56 EEST Laurent Pinchart wrote:
>> From: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx>
>>
>> The UVC frame descriptions are numbered using the descriptor's
>> bFrameIndex field. The index is used in UVC requests, and is thus
>> needed to handle requests in userspace. Make it dynamically discoverable
>> by exposing it in a bFrameIndex configfs attribute of the frame
>> config item.
>>
>> However, the bFrameIndex value is never set to begin with. Set these
>> values when linking the stream class header in the configfs hierarchy,
>> and store it in the config item private data.
>>
>> Signed-off-by: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx>
>> ---
>> drivers/usb/gadget/function/uvc_configfs.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
>> b/drivers/usb/gadget/function/uvc_configfs.c index
>> 0ad6ea57d0b7..882d9963b244 100644
>> --- a/drivers/usb/gadget/function/uvc_configfs.c
>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
>> @@ -1152,6 +1152,8 @@ UVC_ATTR(uvcg_frame_, cname, aname);
>>
>> #define noop_conversion(x) (x)
>>
>> +UVCG_FRAME_ATTR_RO(b_frame_index, bFrameIndex, noop_conversion,
>> + noop_conversion, 8);
>> UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
>> noop_conversion, 8);
>> UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16);
>> @@ -1298,6 +1300,7 @@ static ssize_t
>> uvcg_frame_dw_frame_interval_store(struct config_item *item,
>> UVC_ATTR(uvcg_frame_, dw_frame_interval, dwFrameInterval);
>>
>> static struct configfs_attribute *uvcg_frame_attrs[] = {
>> + &uvcg_frame_attr_b_frame_index,
>> &uvcg_frame_attr_bm_capabilities,
>> &uvcg_frame_attr_w_width,
>> &uvcg_frame_attr_w_height,
>> @@ -2130,12 +2133,15 @@ static int __uvcg_fill_strm(void *priv1, void
>> *priv2, void *priv3, int n, sizeof(*frm->dw_frame_interval);
>> memcpy(*dest, frm->dw_frame_interval, sz);
>> *dest += sz;
>> - if (frm->fmt_type == UVCG_UNCOMPRESSED)
>> + if (frm->fmt_type == UVCG_UNCOMPRESSED) {
>> h->bLength = UVC_DT_FRAME_UNCOMPRESSED_SIZE(
>> frm->frame.b_frame_interval_type);
>> - else if (frm->fmt_type == UVCG_MJPEG)
>> + frm->frame.b_frame_index = n + 1;
>> + } else if (frm->fmt_type == UVCG_MJPEG) {
>> h->bLength = UVC_DT_FRAME_MJPEG_SIZE(
>> frm->frame.b_frame_interval_type);
>> + frm->frame.b_frame_index = n + 1;
>> + }
>> }
>> break;
>> }
>
> This approach looks simpler to me than the patch that Joel submitted.
However,
> this patch lacks ABI documentation, which is included in Joel's patch.
>
> Joel, as you were first, I propose taking your patch and modifying it
with the
> approach followed here, using the UVCG_FRAME_ATTR_RO macro and setting
> b_frame_index in __uvcg_fill_strm(). Would that be OK with you ? Or is
there a
> reason why the approach taken in your patch would be better ?
>
I've compared Paul's implementation to mine, and as far as I can tell,
they are functionally equivalent except for two points:
1) I specifically opted against using a generic UVCG_FRAME_ATTR_RO
macro, because the index has no well defined value until they are all
set at once, as such reading the attribute beforehand should return
-EBUSY before that point (instead of 1 for all indexes, which would be
the case now). All other attributes can be read immediately after
creation. Because my approach sets the frame indexes when the parent
format is linked, I check the target_fmt->linked flag; For Paul's
approach we would have to check whether __uvcg_fill_strm() has already
been called. Considering, that the format indices are set in the same
fashion, we should probably do the same for bFormatIndex.
2) My approach creates the bFrameIndexes as soon as a format has been
finalized and linked. Depending on how an application sets up the
gadget, this might make it easier to take note of the created indices at
the earliest possible time instead of rescanning everything at the end.
I do not think however this is a significant enough advantage to warrant
taking a different approach compared to the way format indices are
already being set.
Cheers,
Joël Pepper
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
