Hi Pawel,
Thank you for the patch.
On Friday 30 August 2013 11:17:05 Pawel Osciak wrote:
> Add encoding unit definitions and descriptor parsing code and allow them to
> be added to chains.
>
> Signed-off-by: Pawel Osciak <posciak@xxxxxxxxxxxx>
> ---
> drivers/media/usb/uvc/uvc_ctrl.c | 37 ++++++++++++++++++---
> drivers/media/usb/uvc/uvc_driver.c | 67 ++++++++++++++++++++++++++++++-----
> drivers/media/usb/uvc/uvcvideo.h | 14 +++++++-
> include/uapi/linux/usb/video.h | 1 +
> 4 files changed, 105 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> b/drivers/media/usb/uvc/uvc_ctrl.c index ba159a4..72d6724 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -777,6 +777,7 @@ static const __u8 uvc_processing_guid[16] =
> UVC_GUID_UVC_PROCESSING; static const __u8 uvc_camera_guid[16] =
> UVC_GUID_UVC_CAMERA;
> static const __u8 uvc_media_transport_input_guid[16] =
> UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
> +static const __u8 uvc_encoding_guid[16] = UVC_GUID_UVC_ENCODING;
>
> static int uvc_entity_match_guid(const struct uvc_entity *entity,
> const __u8 guid[16])
> @@ -795,6 +796,9 @@ static int uvc_entity_match_guid(const struct uvc_entity
> *entity, return memcmp(entity->extension.guidExtensionCode,
> guid, 16) == 0;
>
> + case UVC_VC_ENCODING_UNIT:
> + return memcmp(uvc_encoding_guid, guid, 16) == 0;
> +
> default:
> return 0;
> }
> @@ -2105,12 +2109,13 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
> {
> struct uvc_entity *entity;
> unsigned int i;
> + int num_found;
>
> /* Walk the entities list and instantiate controls */
> list_for_each_entry(entity, &dev->entities, list) {
> struct uvc_control *ctrl;
> - unsigned int bControlSize = 0, ncontrols;
> - __u8 *bmControls = NULL;
> + unsigned int bControlSize = 0, ncontrols = 0;
> + __u8 *bmControls = NULL, *bmControlsRuntime = NULL;
>
> if (UVC_ENTITY_TYPE(entity) == UVC_VC_EXTENSION_UNIT) {
> bmControls = entity->extension.bmControls;
> @@ -2121,13 +2126,25 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
> } else if (UVC_ENTITY_TYPE(entity) == UVC_ITT_CAMERA) {
> bmControls = entity->camera.bmControls;
> bControlSize = entity->camera.bControlSize;
> + } else if (UVC_ENTITY_TYPE(entity) == UVC_VC_ENCODING_UNIT) {
> + bmControls = entity->encoding.bmControls;
> + bmControlsRuntime = entity->encoding.bmControlsRuntime;
> + bControlSize = entity->encoding.bControlSize;
> }
>
> /* Remove bogus/blacklisted controls */
> uvc_ctrl_prune_entity(dev, entity);
>
> /* Count supported controls and allocate the controls array */
> - ncontrols = memweight(bmControls, bControlSize);
> + for (i = 0; i < bControlSize; ++i) {
> + if (bmControlsRuntime) {
> + ncontrols += hweight8(bmControls[i]
> + | bmControlsRuntime[i]);
Nitpicking, could you move the | to the end of the previous line to match the
style of the rest of the code ?
> + } else {
> + ncontrols += hweight8(bmControls[i]);
> + }
The { } are not needed.
> + }
> +
> if (ncontrols == 0)
> continue;
>
> @@ -2139,8 +2156,17 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
>
> /* Initialize all supported controls */
> ctrl = entity->controls;
> - for (i = 0; i < bControlSize * 8; ++i) {
> - if (uvc_test_bit(bmControls, i) == 0)
> + for (i = 0, num_found = 0;
> + i < bControlSize * 8 && num_found < ncontrols; ++i) {
> + if (uvc_test_bit(bmControls, i) == 1)
> + ctrl->on_init = 1;
> + if (bmControlsRuntime &&
> + uvc_test_bit(bmControlsRuntime, i) == 1)
> + ctrl->in_runtime = 1;
> + else if (!bmControlsRuntime)
> + ctrl->in_runtime = ctrl->on_init;
> +
> + if (ctrl->on_init == 0 && ctrl->in_runtime == 0)
> continue;
Wouldn't it simplify the code if you assigned bmControls to bmControlsRuntime
when bmControlsRuntime is NULL before counting the controls above ? You could
get rid of the if inside the count loop, and you could also simplify this
construct.
>
> ctrl->entity = entity;
> @@ -2148,6 +2174,7 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
>
> uvc_ctrl_init_ctrl(dev, ctrl);
> ctrl++;
> + num_found++;
> }
> }
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index d7ff707..d950b40 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1155,6 +1155,37 @@ static int uvc_parse_standard_control(struct
> uvc_device *dev, list_add_tail(&unit->list, &dev->entities);
> break;
>
> + case UVC_VC_ENCODING_UNIT:
> + n = buflen >= 7 ? buffer[6] : 0;
> +
> + if (buflen < 7 + 2 * n) {
> + uvc_trace(UVC_TRACE_DESCR, "device %d videocontrol "
> + "interface %d ENCODING_UNIT error\n",
> + udev->devnum, alts->desc.bInterfaceNumber);
> + return -EINVAL;
> + }
> +
> + unit = uvc_alloc_entity(buffer[2], buffer[3], 2, 2 * n);
> + if (unit == NULL)
> + return -ENOMEM;
> +
> + memcpy(unit->baSourceID, &buffer[4], 1);
> + unit->encoding.bControlSize = buffer[6];
> + unit->encoding.bmControls = (__u8 *)unit + sizeof(*unit);
> + memcpy(unit->encoding.bmControls, &buffer[7], n);
> + unit->encoding.bmControlsRuntime = unit->encoding.bmControls
> + + n;
> + memcpy(unit->encoding.bmControlsRuntime, &buffer[7 + n], n);
> +
> + if (buffer[5] != 0)
> + usb_string(udev, buffer[5], unit->name,
> + sizeof(unit->name));
> + else
> + sprintf(unit->name, "encoding %u", buffer[3]);
Could you s/encoding/Encoding/ to match the other unit names ?
> +
> + list_add_tail(&unit->list, &dev->entities);
> + break;
> +
> default:
> uvc_trace(UVC_TRACE_DESCR, "Found an unknown CS_INTERFACE "
> "descriptor (%u)\n", buffer[2]);
> @@ -1251,25 +1282,31 @@ static void uvc_delete_chain(struct uvc_video_chain
> *chain) *
> * - one or more Output Terminals (USB Streaming or Display)
> * - zero or one Processing Unit
> + * - zero or one Encoding Unit
> * - zero, one or more single-input Selector Units
> * - zero or one multiple-input Selector Units, provided all inputs are
> * connected to input terminals
> - * - zero, one or mode single-input Extension Units
> + * - zero, one or more single-input Extension Units
> * - one or more Input Terminals (Camera, External or USB Streaming)
> *
> - * The terminal and units must match on of the following structures:
> + * The terminal and units must match one of the following structures:
While we're at it, s/terminal/terminals/ ?
> *
> - * ITT_*(0) -> +---------+ +---------+ +---------+ -> TT_STREAMING(0)
> - * ... | SU{0,1} | -> | PU{0,1} | -> | XU{0,n} | ...
> - * ITT_*(n) -> +---------+ +---------+ +---------+ -> TT_STREAMING(n)
> + * ITT_*(0) -> +---------+ -> TT_STREAMING(0) + *
> ... | SU{0,1} | -> (...) ...
> + * ITT_*(n) -> +---------+ -> TT_STREAMING(n)
> + *
> + * Where (...), in any order:
> + * +---------+ +---------+ +---------+
> + * | PU{0,1} | -> | XU{0,n} | -> | EU{0,1} |
> + * +---------+ +---------+ +---------+
> *
> * +---------+ +---------+ -> OTT_*(0)
> * TT_STREAMING -> | PU{0,1} | -> | XU{0,n} | ...
> * +---------+ +---------+ -> OTT_*(n)
> *
> - * The Processing Unit and Extension Units can be in any order. Additional
> - * Extension Units connected to the main chain as single-unit branches are
> - * also supported. Single-input Selector Units are ignored.
> + * The Processing Unit, the Encoding Unit and Extension Units can be in any
> + * order. Additional Extension Units connected to the main chain as
> single-unit
> + * branches are also supported. Single-input Selector Units are ignored. */
> static int uvc_scan_chain_entity(struct uvc_video_chain *chain,
> struct uvc_entity *entity)
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
