Re: [PATCH v2 2/3] usb: gadget/uvc: also handle v4l2 ioctl ENUM_FMT

Re: [PATCH v2 2/3] usb: gadget/uvc: also handle v4l2 ioctl ENUM_FMT

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

(CC'ing Hans Verkuil and the linux-media mailing list)

On Friday 08 August 2014 17:38:58 Michael Grzeschik wrote:
> This patch adds ENUM_FMT as possible ioctl to the uvc v4l2 device.
> That makes userspace applications with a generic IOCTL calling
> convention make also use of it.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> v1 -> v2:
>  - changed first switch case to simple if
>  - added separate function
>  - added description field
>  - bail out on array boundaries
> 
>  drivers/usb/gadget/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/uvc_v4l2.c b/drivers/usb/gadget/uvc_v4l2.c
> index ad48e81..58633bf 100644
> --- a/drivers/usb/gadget/uvc_v4l2.c
> +++ b/drivers/usb/gadget/uvc_v4l2.c
> @@ -55,14 +55,30 @@ struct uvc_format
>  {
>  	u8 bpp;
>  	u32 fcc;
> +	char *description;
>  };
> 
>  static struct uvc_format uvc_formats[] = {
> -	{ 16, V4L2_PIX_FMT_YUYV  },
> -	{ 0,  V4L2_PIX_FMT_MJPEG },
> +	{ 16, V4L2_PIX_FMT_YUYV, "YUV 4:2:2" },
> +	{ 0,  V4L2_PIX_FMT_MJPEG, "MJPEG" },

Format descriptions are currently duplicated in every driver, causing higher 
memory usage and different descriptions for the same format depending on the 
driver. Hans, should we try to fix this ?

>  };
> 
>  static int
> +uvc_v4l2_enum_format(struct uvc_video *video, struct v4l2_fmtdesc *fmt)
> +{
> +

There's an extra blank line here.

> +	int index = fmt->index;

You can use fmt->index directly below, there's no need for a local variable.

> +	if (index >= ARRAY_SIZE(uvc_formats))
> +		return -EINVAL;
> +
> +	strcpy(fmt->description, uvc_formats[index].description);

How about strlcpy to make sure we don't overflow the buffer ?

> +	fmt->pixelformat = uvc_formats[index].fcc;
> +
> +	return 0;
> +}
> +
> +static int
>  uvc_v4l2_get_format(struct uvc_video *video, struct v4l2_format *fmt)
>  {
>  	fmt->fmt.pix.pixelformat = video->fcc;
> @@ -183,6 +199,16 @@ uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd,
> void *arg) break;
>  	}
> 
> +	case VIDIOC_ENUM_FMT:
> +	{
> +		struct v4l2_fmtdesc *fmt = arg;
> +
> +		if (fmt->type != video->queue.queue.type)
> +			return -EINVAL;
> +
> +		return uvc_v4l2_enum_format(video, fmt);
> +	}
> +
>  	/* Get & Set format */
>  	case VIDIOC_G_FMT:

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] usb: gadget/uvc: also handle v4l2 ioctl ENUM_FMT

[Hans Verkuil]

On 08/19/2014 05:01 PM, Laurent Pinchart wrote:
> Hi Michael,
> 
> Thank you for the patch.
> 
> (CC'ing Hans Verkuil and the linux-media mailing list)
> 
> On Friday 08 August 2014 17:38:58 Michael Grzeschik wrote:
>> This patch adds ENUM_FMT as possible ioctl to the uvc v4l2 device.
>> That makes userspace applications with a generic IOCTL calling
>> convention make also use of it.
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> ---
>> v1 -> v2:
>>  - changed first switch case to simple if
>>  - added separate function
>>  - added description field
>>  - bail out on array boundaries
>>
>>  drivers/usb/gadget/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++--
>>  1 file changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/uvc_v4l2.c b/drivers/usb/gadget/uvc_v4l2.c
>> index ad48e81..58633bf 100644
>> --- a/drivers/usb/gadget/uvc_v4l2.c
>> +++ b/drivers/usb/gadget/uvc_v4l2.c
>> @@ -55,14 +55,30 @@ struct uvc_format
>>  {
>>  	u8 bpp;
>>  	u32 fcc;
>> +	char *description;
>>  };
>>
>>  static struct uvc_format uvc_formats[] = {
>> -	{ 16, V4L2_PIX_FMT_YUYV  },
>> -	{ 0,  V4L2_PIX_FMT_MJPEG },
>> +	{ 16, V4L2_PIX_FMT_YUYV, "YUV 4:2:2" },
>> +	{ 0,  V4L2_PIX_FMT_MJPEG, "MJPEG" },
> 
> Format descriptions are currently duplicated in every driver, causing higher 
> memory usage and different descriptions for the same format depending on the 
> driver. Hans, should we try to fix this ?

Yes, we should. It's been on my todo list for ages, but at a very low priority.
I'm not planning to work on this in the near future, but if someone else wants
to work on this, then just go ahead.

Regards,

	Hans

> 
>>  };
>>
>>  static int
>> +uvc_v4l2_enum_format(struct uvc_video *video, struct v4l2_fmtdesc *fmt)
>> +{
>> +
> 
> There's an extra blank line here.
> 
>> +	int index = fmt->index;
> 
> You can use fmt->index directly below, there's no need for a local variable.
> 
>> +	if (index >= ARRAY_SIZE(uvc_formats))
>> +		return -EINVAL;
>> +
>> +	strcpy(fmt->description, uvc_formats[index].description);
> 
> How about strlcpy to make sure we don't overflow the buffer ?
> 
>> +	fmt->pixelformat = uvc_formats[index].fcc;
>> +
>> +	return 0;
>> +}
>> +
>> +static int
>>  uvc_v4l2_get_format(struct uvc_video *video, struct v4l2_format *fmt)
>>  {
>>  	fmt->fmt.pix.pixelformat = video->fcc;
>> @@ -183,6 +199,16 @@ uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd,
>> void *arg) break;
>>  	}
>>
>> +	case VIDIOC_ENUM_FMT:
>> +	{
>> +		struct v4l2_fmtdesc *fmt = arg;
>> +
>> +		if (fmt->type != video->queue.queue.type)
>> +			return -EINVAL;
>> +
>> +		return uvc_v4l2_enum_format(video, fmt);
>> +	}
>> +
>>  	/* Get & Set format */
>>  	case VIDIOC_G_FMT:
> 

Re: [PATCH v2 2/3] usb: gadget/uvc: also handle v4l2 ioctl ENUM_FMT

[Laurent Pinchart]

Hi Hans and Michael,

On Wednesday 20 August 2014 02:06:54 Hans Verkuil wrote:
> On 08/19/2014 05:01 PM, Laurent Pinchart wrote:
> > Hi Michael,
> > 
> > Thank you for the patch.
> > 
> > (CC'ing Hans Verkuil and the linux-media mailing list)
> > 
> > On Friday 08 August 2014 17:38:58 Michael Grzeschik wrote:
> >> This patch adds ENUM_FMT as possible ioctl to the uvc v4l2 device.
> >> That makes userspace applications with a generic IOCTL calling
> >> convention make also use of it.
> >> 
> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> >> ---
> >> 
> >> v1 -> v2:
> >>  - changed first switch case to simple if
> >>  - added separate function
> >>  - added description field
> >>  - bail out on array boundaries
> >>  
> >>  drivers/usb/gadget/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++--
> >>  1 file changed, 28 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/usb/gadget/uvc_v4l2.c
> >> b/drivers/usb/gadget/uvc_v4l2.c
> >> index ad48e81..58633bf 100644
> >> --- a/drivers/usb/gadget/uvc_v4l2.c
> >> +++ b/drivers/usb/gadget/uvc_v4l2.c
> >> @@ -55,14 +55,30 @@ struct uvc_format
> >>  {
> >>  	u8 bpp;
> >>  	u32 fcc;
> >> +	char *description;
> >>  };
> >>  
> >>  static struct uvc_format uvc_formats[] = {
> >> -	{ 16, V4L2_PIX_FMT_YUYV  },
> >> -	{ 0,  V4L2_PIX_FMT_MJPEG },
> >> +	{ 16, V4L2_PIX_FMT_YUYV, "YUV 4:2:2" },
> >> +	{ 0,  V4L2_PIX_FMT_MJPEG, "MJPEG" },
> > 
> > Format descriptions are currently duplicated in every driver, causing
> > higher memory usage and different descriptions for the same format
> > depending on the driver. Hans, should we try to fix this ?
> 
> Yes, we should. It's been on my todo list for ages, but at a very low
> priority. I'm not planning to work on this in the near future, but if
> someone else wants to work on this, then just go ahead.

Michael, would you like to give this a try, or should I do it ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] usb: gadget/uvc: also handle v4l2 ioctl ENUM_FMT

[Michael Grzeschik]

Hi Laurent,

On Wed, Aug 20, 2014 at 07:05:30PM +0200, Laurent Pinchart wrote:
> Hi Hans and Michael,
> 
> On Wednesday 20 August 2014 02:06:54 Hans Verkuil wrote:
> > On 08/19/2014 05:01 PM, Laurent Pinchart wrote:
> > > Hi Michael,
> > > 
> > > Thank you for the patch.
> > > 
> > > (CC'ing Hans Verkuil and the linux-media mailing list)
> > > 
> > > On Friday 08 August 2014 17:38:58 Michael Grzeschik wrote:
> > >> This patch adds ENUM_FMT as possible ioctl to the uvc v4l2 device.
> > >> That makes userspace applications with a generic IOCTL calling
> > >> convention make also use of it.
> > >> 
> > >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > >> ---
> > >> 
> > >> v1 -> v2:
> > >>  - changed first switch case to simple if
> > >>  - added separate function
> > >>  - added description field
> > >>  - bail out on array boundaries
> > >>  
> > >>  drivers/usb/gadget/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++--
> > >>  1 file changed, 28 insertions(+), 2 deletions(-)
> > >> 
> > >> diff --git a/drivers/usb/gadget/uvc_v4l2.c
> > >> b/drivers/usb/gadget/uvc_v4l2.c
> > >> index ad48e81..58633bf 100644
> > >> --- a/drivers/usb/gadget/uvc_v4l2.c
> > >> +++ b/drivers/usb/gadget/uvc_v4l2.c
> > >> @@ -55,14 +55,30 @@ struct uvc_format
> > >>  {
> > >>  	u8 bpp;
> > >>  	u32 fcc;
> > >> +	char *description;
> > >>  };
> > >>  
> > >>  static struct uvc_format uvc_formats[] = {
> > >> -	{ 16, V4L2_PIX_FMT_YUYV  },
> > >> -	{ 0,  V4L2_PIX_FMT_MJPEG },
> > >> +	{ 16, V4L2_PIX_FMT_YUYV, "YUV 4:2:2" },
> > >> +	{ 0,  V4L2_PIX_FMT_MJPEG, "MJPEG" },
> > > 
> > > Format descriptions are currently duplicated in every driver, causing
> > > higher memory usage and different descriptions for the same format
> > > depending on the driver. Hans, should we try to fix this ?
> > 
> > Yes, we should. It's been on my todo list for ages, but at a very low
> > priority. I'm not planning to work on this in the near future, but if
> > someone else wants to work on this, then just go ahead.
> 
> Michael, would you like to give this a try, or should I do it ?

It seems Philipp is already taking the chance! :)

Regards,
Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 2/3] usb: gadget/uvc: also handle v4l2 ioctl ENUM_FMT

[Laurent Pinchart]

Hi Michael,

On Monday 25 August 2014 15:59:57 Michael Grzeschik wrote:
> On Wed, Aug 20, 2014 at 07:05:30PM +0200, Laurent Pinchart wrote:
> > On Wednesday 20 August 2014 02:06:54 Hans Verkuil wrote:
> > > On 08/19/2014 05:01 PM, Laurent Pinchart wrote:
> > > > Hi Michael,
> > > > 
> > > > Thank you for the patch.
> > > > 
> > > > (CC'ing Hans Verkuil and the linux-media mailing list)
> > > > 
> > > > On Friday 08 August 2014 17:38:58 Michael Grzeschik wrote:
> > > >> This patch adds ENUM_FMT as possible ioctl to the uvc v4l2 device.
> > > >> That makes userspace applications with a generic IOCTL calling
> > > >> convention make also use of it.
> > > >> 
> > > >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > > >> ---
> > > >> 
> > > >> v1 -> v2:
> > > >>  - changed first switch case to simple if
> > > >>  - added separate function
> > > >>  - added description field
> > > >>  - bail out on array boundaries
> > > >>  
> > > >>  drivers/usb/gadget/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++--
> > > >>  1 file changed, 28 insertions(+), 2 deletions(-)
> > > >> 
> > > >> diff --git a/drivers/usb/gadget/uvc_v4l2.c
> > > >> b/drivers/usb/gadget/uvc_v4l2.c
> > > >> index ad48e81..58633bf 100644
> > > >> --- a/drivers/usb/gadget/uvc_v4l2.c
> > > >> +++ b/drivers/usb/gadget/uvc_v4l2.c
> > > >> @@ -55,14 +55,30 @@ struct uvc_format
> > > >>  {
> > > >>  	u8 bpp;
> > > >>  	u32 fcc;
> > > >> +	char *description;
> > > >>  };
> > > >>  
> > > >>  static struct uvc_format uvc_formats[] = {
> > > >> -	{ 16, V4L2_PIX_FMT_YUYV  },
> > > >> -	{ 0,  V4L2_PIX_FMT_MJPEG },
> > > >> +	{ 16, V4L2_PIX_FMT_YUYV, "YUV 4:2:2" },
> > > >> +	{ 0,  V4L2_PIX_FMT_MJPEG, "MJPEG" },
> > > > 
> > > > Format descriptions are currently duplicated in every driver, causing
> > > > higher memory usage and different descriptions for the same format
> > > > depending on the driver. Hans, should we try to fix this ?
> > > 
> > > Yes, we should. It's been on my todo list for ages, but at a very low
> > > priority. I'm not planning to work on this in the near future, but if
> > > someone else wants to work on this, then just go ahead.
> > 
> > Michael, would you like to give this a try, or should I do it ?
> 
> It seems Philipp is already taking the chance! :)

Perfect timing, I wonder if that's just a coincidence ;-)

I don't think this patch is very urgent, would you be fine with rebasing it on 
top of Philipp's patch when it will be accepted ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] usb: gadget/uvc: also handle v4l2 ioctl ENUM_FMT

[Philipp Zabel]

Hi Laurent,

Am Montag, den 25.08.2014, 16:48 +0200 schrieb Laurent Pinchart:
[...]
> > > > > Format descriptions are currently duplicated in every driver, causing
> > > > > higher memory usage and different descriptions for the same format
> > > > > depending on the driver. Hans, should we try to fix this ?
> > > > 
> > > > Yes, we should. It's been on my todo list for ages, but at a very low
> > > > priority. I'm not planning to work on this in the near future, but if
> > > > someone else wants to work on this, then just go ahead.
> > > 
> > > Michael, would you like to give this a try, or should I do it ?
> > 
> > It seems Philipp is already taking the chance! :)
> 
> Perfect timing, I wonder if that's just a coincidence ;-)

It felt like my own idea this weekend, but I strongly suspect that I
took up enough information to trigger it, when scanning mail last week.

I shouldn't be so fast to dismiss uvc/gadget mails as "Michael's
business"...

regards
Philipp