Re: [PATCH v2 1/9] usb: gadget: uvc: Make bSourceID read/write

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dan,

On Sun, Jan 01, 2023 at 09:18:13PM +0000, Dan Scally wrote:
> On 29/12/2022 00:30, Laurent Pinchart wrote:
> > On Thu, Dec 29, 2022 at 02:30:04AM +0200, Laurent Pinchart wrote:
> >> On Mon, Nov 21, 2022 at 09:25:09AM +0000, Daniel Scally wrote:
> >>> At the moment, the UVC function graph is hardcoded IT -> PU -> OT.
> >>> To add XU support we need the ability to insert the XU descriptors
> >>> into the chain. To facilitate that, make the output terminal's
> >>> bSourceID attribute writeable so that we can configure its source.
> >>>
> >>> Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
> >>> ---
> >>> Changes in v2:
> >>>
> >>> 	- Updated the ABI Documentation to reflect the change.
> >>>
> >>>   .../ABI/testing/configfs-usb-gadget-uvc       |  2 +-
> >>>   drivers/usb/gadget/function/uvc_configfs.c    | 57 ++++++++++++++++++-
> >>>   2 files changed, 57 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >>> index 611b23e6488d..feb3f2cc0c16 100644
> >>> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >>> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >>> @@ -52,7 +52,7 @@ Date:		Dec 2014
> >>>   KernelVersion:	4.0
> >>>   Description:	Default output terminal descriptors
> >>>   
> >>> -		All attributes read only:
> >>> +		All attributes read only except bSourceID:
> >>>   
> >>>   		==============	=============================================
> >>>   		iTerminal	index of string descriptor
> >>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> >>> index 4303a3283ba0..af4258120f9a 100644
> >>> --- a/drivers/usb/gadget/function/uvc_configfs.c
> >>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> >>> @@ -483,11 +483,66 @@ UVC_ATTR_RO(uvcg_default_output_, cname, aname)
> >>>   UVCG_DEFAULT_OUTPUT_ATTR(b_terminal_id, bTerminalID, 8);
> >>>   UVCG_DEFAULT_OUTPUT_ATTR(w_terminal_type, wTerminalType, 16);
> >>>   UVCG_DEFAULT_OUTPUT_ATTR(b_assoc_terminal, bAssocTerminal, 8);
> >>> -UVCG_DEFAULT_OUTPUT_ATTR(b_source_id, bSourceID, 8);
> >>>   UVCG_DEFAULT_OUTPUT_ATTR(i_terminal, iTerminal, 8);
> >>>   
> >>>   #undef UVCG_DEFAULT_OUTPUT_ATTR
> >>>   
> >>> +static ssize_t uvcg_default_output_b_source_id_show(struct config_item *item,
> >>> +						    char *page)
> >>> +{
> >>> +	struct config_group *group = to_config_group(item);
> >>> +	struct f_uvc_opts *opts;
> >>> +	struct config_item *opts_item;
> >>> +	struct mutex *su_mutex = &group->cg_subsys->su_mutex;
> >>> +	struct uvc_output_terminal_descriptor *cd;
> >>> +	int result;
> >>> +
> >>> +	mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> >>> +
> >>> +	opts_item = group->cg_item.ci_parent->ci_parent->ci_parent->ci_parent;
> >>> +	opts = to_f_uvc_opts(opts_item);
> >>> +	cd = &opts->uvc_output_terminal;
> >>> +
> >>> +	mutex_lock(&opts->lock);
> >>> +	result = sprintf(page, "%u\n", le8_to_cpu(cd->bSourceID));
> >>> +	mutex_unlock(&opts->lock);
> >>> +
> >>> +	mutex_unlock(su_mutex);
> >>> +
> >>> +	return result;
> >>> +}
> >>> +
> >>> +static ssize_t uvcg_default_output_b_source_id_store(struct config_item *item,
> >>> +						     const char *page, size_t len)
> >>> +{
> >>> +	struct config_group *group = to_config_group(item);
> >>> +	struct f_uvc_opts *opts;
> >>> +	struct config_item *opts_item;
> >>> +	struct mutex *su_mutex = &group->cg_subsys->su_mutex;
> >>> +	struct uvc_output_terminal_descriptor *cd;
> >>> +	int result;
> >>> +	u8 num;
> >>> +
> >>> +	mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> >>> +
> >>> +	opts_item = group->cg_item.ci_parent->ci_parent->ci_parent->ci_parent;
> >>> +	opts = to_f_uvc_opts(opts_item);
> >>> +	cd = &opts->uvc_output_terminal;
> >>> +
> >>> +	result = kstrtou8(page, 0, &num);
> >>> +	if (result)
> >>> +		return result;
> >>> +
> >>> +	mutex_lock(&opts->lock);
> >>> +	cd->bSourceID = num;
> >>> +	mutex_unlock(&opts->lock);
> >>> +
> >>> +	mutex_unlock(su_mutex);
> >>> +
> >>> +	return len;
> >>> +}
> >>> +UVC_ATTR(uvcg_default_output_, b_source_id, bSourceID);
> >>
> >> Feel free to shoot this idea down if it's a bad one: given that the
> >> bSourceID attributes serve to create a pipeline by linking entities,
> >> would it make sense to model these links with symlinks ?
> >
> > I forgot to mention that this would handle the bSourceID attribute
> > automatically, avoiding mistakes. But maybe we're over-engineering all
> > this...
> 
> Hmmmm, maybe. I lean towards over-engineered, but not strongly so. 
> Assuming the string descriptors stand as is, the .allow_link() for XUs 
> would have to account for linking to both a string and another unit. The 
> position of the Source ID field in the Unit Descriptors differs, and for 
> the XUs is nested behind another struct...and properly supporting XUs as 
> specified means we'd need to allow multiple links in to an XU, all of 
> which might make it a bit more complicated than it is helpful. Happy to 
> be convinced otherwise though; I'm on the fence about it.

So am I :-) I agree about the multiple links issue, and the fact that an
XU can have multiple sources makes it more complicated. That's what made
me think we may be over-engineering all this, it's all about passing a
set of descriptors from userspace to the host, without a real need for
the kernel to access that data. configfs really starts feeling like a
bad solution for this, at least in its current form :-(

> >>> +
> >>>   static struct configfs_attribute *uvcg_default_output_attrs[] = {
> >>>   	&uvcg_default_output_attr_b_terminal_id,
> >>>   	&uvcg_default_output_attr_w_terminal_type,

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux