Re: [PATCH v7] usb: gadget: uvc: configfs: Add bFrameIndex attributes
Hi, Laurent Pinchart writes: > Hi Felipe, > > On Thursday, 26 July 2018 13:49:07 EEST Felipe Balbi wrote: >> Laurent Pinchart writes: >> > From: Joel Pepper >> > >> > - 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. >> > >> > 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. >> > >> > Signed-off-by: Joel Pepper >> > [Simplified documentation, renamed function, blank space update] >> > Signed-off-by: Laurent Pinchart >> >> please rebase on testing/next, but give me a couple hours to go through >> the rest of my inbox first ;) > > I will. Have you had time to go through the rest of your inbox now ? It's > been > a few hours already I suppose :-) well, pull request was already pushed to Greg. We're already in -rc7, too late for this merge window. -- balbi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] usb: gadget: uvc: configfs: Add bFrameIndex attributes
Hi Felipe, On Thursday, 26 July 2018 13:49:07 EEST Felipe Balbi wrote: > Laurent Pinchart writes: > > From: Joel Pepper > > > > - 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. > > > > 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. > > > > Signed-off-by: Joel Pepper > > [Simplified documentation, renamed function, blank space update] > > Signed-off-by: Laurent Pinchart > > please rebase on testing/next, but give me a couple hours to go through > the rest of my inbox first ;) I will. Have you had time to go through the rest of your inbox now ? It's been a few hours already I suppose :-) > checking file Documentation/ABI/testing/configfs-usb-gadget-uvc > Hunk #1 succeeded at 177 (offset -4 lines). > Hunk #2 succeeded at 228 (offset -8 lines). > checking file drivers/usb/gadget/function/uvc_configfs.c > Hunk #1 succeeded at 720 (offset -121 lines). > Hunk #2 succeeded at 757 (offset -133 lines). > Hunk #3 succeeded at 996 (offset -137 lines). > Hunk #4 succeeded at 1179 (offset -137 lines). > Hunk #5 FAILED at 1395. > 1 out of 5 hunks FAILED -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] usb: gadget: uvc: configfs: Add bFrameIndex attributes
Laurent Pinchart writes: > From: Joel Pepper > > - 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. > > 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. > > Signed-off-by: Joel Pepper > [Simplified documentation, renamed function, blank space update] > Signed-off-by: Laurent Pinchart please rebase on testing/next, but give me a couple hours to go through the rest of my inbox first ;) checking file Documentation/ABI/testing/configfs-usb-gadget-uvc Hunk #1 succeeded at 177 (offset -4 lines). Hunk #2 succeeded at 228 (offset -8 lines). checking file drivers/usb/gadget/function/uvc_configfs.c Hunk #1 succeeded at 720 (offset -121 lines). Hunk #2 succeeded at 757 (offset -133 lines). Hunk #3 succeeded at 996 (offset -137 lines). Hunk #4 succeeded at 1179 (offset -137 lines). Hunk #5 FAILED at 1395. 1 out of 5 hunks FAILED -- balbi signature.asc Description: PGP signature
Re: [PATCH v7] usb: gadget: uvc: configfs: Add bFrameIndex attributes
Hi Laurent,
looks good to me; I have no complaints.
On 13.06.2018 00:58, Laurent Pinchart wrote:
> From: Joel Pepper
>
> - 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.
>
> 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.
>
> Signed-off-by: Joel Pepper
> [Simplified documentation, renamed function, blank space update]
> Signed-off-by: Laurent Pinchart
> ---
> Documentation/ABI/testing/configfs-usb-gadget-uvc | 8
> drivers/usb/gadget/function/uvc_configfs.c| 56
> +++
> 2 files changed, 64 insertions(+)
>
> Hi Joel,
>
> What do you think about this patch ? It retains your approach and
> addresses the few minor reviw comments I sent.
>
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> index 9896c8aa0e14..1efeb2e835ea 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -181,6 +181,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
> @@ -232,6 +236,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 5c3ea5f89201..cac249e8a7ec 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -841,6 +841,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);
> +
> static int uvcg_streaming_header_allow_link(struct config_item *src,
> struct config_item *target)
> {
> @@ -888,6 +890,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;
> @@ -1129,6 +1133,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->
[PATCH v7] usb: gadget: uvc: configfs: Add bFrameIndex attributes
From: Joel Pepper
- 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.
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.
Signed-off-by: Joel Pepper
[Simplified documentation, renamed function, blank space update]
Signed-off-by: Laurent Pinchart
---
Documentation/ABI/testing/configfs-usb-gadget-uvc | 8
drivers/usb/gadget/function/uvc_configfs.c| 56 +++
2 files changed, 64 insertions(+)
Hi Joel,
What do you think about this patch ? It retains your approach and
addresses the few minor reviw comments I sent.
diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc
b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index 9896c8aa0e14..1efeb2e835ea 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
@@ -181,6 +181,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
@@ -232,6 +236,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 5c3ea5f89201..cac249e8a7ec 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -841,6 +841,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);
+
static int uvcg_streaming_header_allow_link(struct config_item *src,
struct config_item *target)
{
@@ -888,6 +890,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;
@@ -1129,6 +1133,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_capa
