Hi Laurent, Joel
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(+)
>
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> index a6cc8d6d398e..809765bd9573 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -189,6 +189,10 @@ Date: Dec 2014
> KernelVersion: 4.0
> Description: Specific MJPEG frame descriptors
>
> + bFrameIndex - unique id for this framedescriptor;
> + only defined after parent format is
> + linked into the streaming header;
> + read-only
> dwFrameInterval - indicates how frame interval can be
> programmed; a number of values
> separated by newline can be specified
> @@ -240,6 +244,10 @@ Date: Dec 2014
> KernelVersion: 4.0
> Description: Specific uncompressed frame descriptors
>
> + bFrameIndex - unique id for this framedescriptor;
> + only defined after parent format is
> + linked into the streaming header;
> + read-only
> dwFrameInterval - indicates how frame interval can be
> programmed; a number of values
> separated by newline can be specified
> 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.
With a decision made on that, and the typo fixed in the commit message:
Reviewed-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> 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
