Hi Laurent,
On 24/09/18 17:00, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Monday, 24 September 2018 15:22:57 EEST Kieran Bingham wrote:
>> On 01/08/18 22:55, Laurent Pinchart wrote:
>>> From: Joel Pepper <joel.pepper@xxxxxxxxxxxxxx>
>>>
>>> - Add bFrameIndex as a UVCG_FRAME_ATTR_RO for each frame size.
>>> - Automatically assign ascending bFrameIndex to each frame in a format.
>>>
>>> Before all "bFrameindex" attributes were set to "1" with no way to
>>> configure the gadget otherwise. This resulted in the host always
>>> negotiating for bFrameIndex 1 (i.e. the first framesize of the gadget).
>>> After the negotiation the host driver will set the user or application
>>> selected framesize, while the gadget is actually set to the first
>>> framesize.
>>
>> s/framesize/frame size/ *3 above ? (or alternatively frame-size?)
>>
>>> Now, when the containing format is linked into the streaming header,
>>> iterate over all child frame descriptors and assign ascending indices.
>>> The automatically assigned indices can be read from the new read only
>>> bFrameIndex configsfs attribute in each frame descriptor item.
>>
>> s/configsfs/configfs/
>>
>>> Signed-off-by: Joel Pepper <joel.pepper@xxxxxxxxxxxxxx>
>>> [Simplified documentation, renamed function, blank space update]
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>> ---
>>>
>>> Documentation/ABI/testing/configfs-usb-gadget-uvc | 8 ++++
>>> drivers/usb/gadget/function/uvc_configfs.c | 56 ++++++++++++++++++
>>> 2 files changed, 64 insertions(+)
>
> [snip]
>
>>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
>>> b/drivers/usb/gadget/function/uvc_configfs.c index
>>> 5cee8aca3734..b8763343dcae 100644
>>> --- a/drivers/usb/gadget/function/uvc_configfs.c
>>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
>>> @@ -868,6 +868,8 @@ static struct uvcg_streaming_header
>>> *to_uvcg_streaming_header(struct config_item>
>>> return container_of(item, struct uvcg_streaming_header, item);
>>>
>>> }
>>>
>>> +static void uvcg_format_set_indices(struct config_group *fmt);
>>> +
>>
>> Do we need to forward declare here?
>>
>> Couldn't we move the uvcg_format_set_indices() implementation up ?
>> Or is it preferred to keep that code down in the lower section.
>
> You know how much I dislike forward-declarations. I tried to fix this one, but
> uvcg_format_set_indices() can't be moved up as-is as it depends on the
> definition of the uvcg_frame structure. I attempted to reorganize the code in
> a more logical way but gave up given how large the resulting change was even
> before I got it to compile, and how the new organization was less logical :-(
Aha - I missed that it was because of the structure definition. I had
only looked at the call hierarchy :-)
>
>> With a decision made on that, and the typo fixed in the commit message:
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
>
> Thank you. I'll keep the forward declaration for now. I might give it a try
> again later.
No problem.
--
Kieran
>
>>> static int uvcg_streaming_header_allow_link(struct config_item *src,
>>>
>>> struct config_item *target)
>>>
>>> {
>>>
>>> @@ -915,6 +917,8 @@ static int uvcg_streaming_header_allow_link(struct
>>> config_item *src,>
>>> if (!target_fmt)
>>>
>>> goto out;
>>>
>>> + uvcg_format_set_indices(to_config_group(target));
>>> +
>>>
>>> format_ptr = kzalloc(sizeof(*format_ptr), GFP_KERNEL);
>>> if (!format_ptr) {
>>>
>>> ret = -ENOMEM;
>>>
>>> @@ -1146,6 +1150,41 @@ end: \
>>>
>>> \
>>>
>>> UVC_ATTR(uvcg_frame_, cname, aname);
>>>
>>> +static ssize_t uvcg_frame_b_frame_index_show(struct config_item *item,
>>> + char *page)
>>> +{
>>> + struct uvcg_frame *f = to_uvcg_frame(item);
>>> + struct uvcg_format *fmt;
>>> + struct f_uvc_opts *opts;
>>> + struct config_item *opts_item;
>>> + struct config_item *fmt_item;
>>> + struct mutex *su_mutex = &f->item.ci_group->cg_subsys->su_mutex;
>>> + int result;
>>> +
>>> + mutex_lock(su_mutex); /* for navigating configfs hierarchy */
>>> +
>>> + fmt_item = f->item.ci_parent;
>>> + fmt = to_uvcg_format(fmt_item);
>>> +
>>> + if (!fmt->linked) {
>>> + result = -EBUSY;
>>> + goto out;
>>> + }
>>> +
>>> + opts_item = fmt_item->ci_parent->ci_parent->ci_parent;
>>> + opts = to_f_uvc_opts(opts_item);
>>> +
>>> + mutex_lock(&opts->lock);
>>> + result = sprintf(page, "%d\n", f->frame.b_frame_index);
>>> + mutex_unlock(&opts->lock);
>>> +
>>> +out:
>>> + mutex_unlock(su_mutex);
>>> + return result;
>>> +}
>>> +
>>> +UVC_ATTR_RO(uvcg_frame_, b_frame_index, bFrameIndex);
>>> +
>>>
>>> #define noop_conversion(x) (x)
>>>
>>> UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
>>>
>>> @@ -1294,6 +1333,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,
>>>
>>> @@ -1373,6 +1413,22 @@ static void uvcg_frame_drop(struct config_group
>>> *group, struct config_item *item>
>>> config_item_put(item);
>>>
>>> }
>>>
>>> +static void uvcg_format_set_indices(struct config_group *fmt)
>>> +{
>>> + struct config_item *ci;
>>> + unsigned int i = 1;
>>> +
>>> + list_for_each_entry(ci, &fmt->cg_children, ci_entry) {
>>> + struct uvcg_frame *frm;
>>> +
>>> + if (ci->ci_type != &uvcg_frame_type)
>>> + continue;
>>> +
>>> + frm = to_uvcg_frame(ci);
>>> + frm->frame.b_frame_index = i++;
>>> + }
>>> +}
>>> +
>>>
>>> /*
>>> ------------------------------------------------------------------------
>>> ----->
>>> * streaming/uncompressed/<NAME>
>>> */
>
>
--
Regards
--
Kieran
