Question about drivers/media/usb/uvc/uvc_v4l2.c

Question about drivers/media/usb/uvc/uvc_v4l2.c

[Shaobo He]

Hello everyone,

This is Shaobo from Utah again. I've been bugging the mailing list with my 
patches. I have a quick question about a function in 
`drivers/media/usb/uvc/uvc_v4l2.c`. In `uvc_v4l2_try_format`, can 
`stream->nformats` be 0? I saw that in other files, this field could be zero 
which is considered as error cases. I was wondering if it's true for this 
function, too.

Thanks,
Shaobo

Re: Question about drivers/media/usb/uvc/uvc_v4l2.c

[Laurent Pinchart]

Hi Shaobo,

On Sat, Mar 02, 2019 at 01:22:49PM -0700, Shaobo He wrote:
> Hello everyone,
> 
> This is Shaobo from Utah again. I've been bugging the mailing list with my 
> patches. I have a quick question about a function in 
> `drivers/media/usb/uvc/uvc_v4l2.c`. In `uvc_v4l2_try_format`, can 
> `stream->nformats` be 0? I saw that in other files, this field could be zero 
> which is considered as error cases. I was wondering if it's true for this 
> function, too.

The uvc_parse_streaming() function should answer this question :-)

-- 
Regards,

Laurent Pinchart

Re: Question about drivers/media/usb/uvc/uvc_v4l2.c

[Shaobo He]

Hi Laurent,

Thank you very much for your reply. This is what I thought, too. It seems that 
there's an implicit order of execution that is not clearly implied in the code, 
meaning `uvc_parse_streaming` is called before `uvc_v4l2_try_format`.

That being said, I was wondering maybe a better practice to write the loop in 
`uvc_v4l2_try_format` would be like the following,

```
format=NULL;
...
for (i = 0; i < stream->nformats; ++i) {
		format = &stream->format[i];
		if (format->fcc == fmt->fmt.pix.pixelformat)
			break;
}
// dereferencing format
```
to
```
// just declaration
format;
i=0;
do {
		format = &stream->format[i];
		if (format->fcc == fmt->fmt.pix.pixelformat)
			break;
		++i;
} while (i<stream->nformats)
// dereferencing format
```
I mean you can save one initialization, provided compiler does it and one branch.

Shaobo
On 2019/3/2 14:43, Laurent Pinchart wrote:
> Hi Shaobo,
> 
> On Sat, Mar 02, 2019 at 01:22:49PM -0700, Shaobo He wrote:
>> Hello everyone,
>>
>> This is Shaobo from Utah again. I've been bugging the mailing list with my
>> patches. I have a quick question about a function in
>> `drivers/media/usb/uvc/uvc_v4l2.c`. In `uvc_v4l2_try_format`, can
>> `stream->nformats` be 0? I saw that in other files, this field could be zero
>> which is considered as error cases. I was wondering if it's true for this
>> function, too.
> 
> The uvc_parse_streaming() function should answer this question :-)
> 

Re: Question about drivers/media/usb/uvc/uvc_v4l2.c

[Laurent Pinchart]

Hello Shoabo,

On Sat, Mar 02, 2019 at 03:29:25PM -0700, Shaobo He wrote:
> Hi Laurent,
> 
> Thank you very much for your reply. This is what I thought, too. It seems that 
> there's an implicit order of execution that is not clearly implied in the code, 
> meaning `uvc_parse_streaming` is called before `uvc_v4l2_try_format`.

It is implied by the logic of the driver that calls
uvc_parse_streaming() at probe time before registering video nodes, and
with uvc_v4l2_try_format() only being called from userspace through the
video nodes.

> That being said, I was wondering maybe a better practice to write the loop in 
> `uvc_v4l2_try_format` would be like the following,
> 
> ```
> format=NULL;
> ...
> for (i = 0; i < stream->nformats; ++i) {
> 		format = &stream->format[i];
> 		if (format->fcc == fmt->fmt.pix.pixelformat)
> 			break;
> }
> // dereferencing format
> ```
> to
> ```
> // just declaration
> format;
> i=0;
> do {
> 		format = &stream->format[i];
> 		if (format->fcc == fmt->fmt.pix.pixelformat)
> 			break;
> 		++i;
> } while (i<stream->nformats)
> // dereferencing format
> ```
> I mean you can save one initialization, provided compiler does it and one branch.

I like for loops better in general, they convey the meaning in a cleaner
way. The compiler should be able to do its job here and optimize the
code correctly, I don't think a change is worth it, especially as we're
not dealing with a hot path.

> On 2019/3/2 14:43, Laurent Pinchart wrote:
> > On Sat, Mar 02, 2019 at 01:22:49PM -0700, Shaobo He wrote:
> >> Hello everyone,
> >>
> >> This is Shaobo from Utah again. I've been bugging the mailing list with my
> >> patches. I have a quick question about a function in
> >> `drivers/media/usb/uvc/uvc_v4l2.c`. In `uvc_v4l2_try_format`, can
> >> `stream->nformats` be 0? I saw that in other files, this field could be zero
> >> which is considered as error cases. I was wondering if it's true for this
> >> function, too.
> > 
> > The uvc_parse_streaming() function should answer this question :-)

-- 
Regards,

Laurent Pinchart