UVC property auto update

UVC property auto update

[Edgar Thier]

Hi all,

I have some USB-3.0 cameras that use UVC.
These cameras offer auto updates for various properties.
An example of such a property would be gain, that will be adjusted when activating the auto-gain
property. These property changes are not queried by the UVC driver, unless it already has the
property marked as auto update via UVC_CTRL_FLAG_AUTO_UPDATE.
>From what I have seen, it seems that this flag is not checked when indexing the camera controls.
However it is checked when using extension units, so all properties loaded through such a unit are
being updates as one would hope.

My questions:

Is it intended that properties cannot mark themselves as autoupdate?
If yes:
	Is there a recommended way of working around this?
	Do all autoupdate properties have to be in an extension unit?
If no:
	What should a fix look like?

Regards,

Edgar

Re: UVC property auto update

[Guennadi Liakhovetski]

Hi Edgar,

I'm adding UVC maintainer to CC, he might have missed this email. Could 
you also tell us what cameras have those features? A recent patch from me 
extends the UVC driver use of the Interrupt endpoint for asynchronous 
control completion notifications. This would be another extension for the 
same interface. I guess the way to implement it would be using V4L events.

Thanks
Guennadi

On Mon, 7 Aug 2017, Edgar Thier wrote:

> Hi all,
> 
> I have some USB-3.0 cameras that use UVC.
> These cameras offer auto updates for various properties.
> An example of such a property would be gain, that will be adjusted when activating the auto-gain
> property. These property changes are not queried by the UVC driver, unless it already has the
> property marked as auto update via UVC_CTRL_FLAG_AUTO_UPDATE.
> >From what I have seen, it seems that this flag is not checked when indexing the camera controls.
> However it is checked when using extension units, so all properties loaded through such a unit are
> being updates as one would hope.
> 
> My questions:
> 
> Is it intended that properties cannot mark themselves as autoupdate?
> If yes:
> 	Is there a recommended way of working around this?
> 	Do all autoupdate properties have to be in an extension unit?
> If no:
> 	What should a fix look like?
> 
> Regards,
> 
> Edgar
> 

Re: UVC property auto update

[Edgar Thier]

Hi Guennadi,

The cameras in question are USB-3.0 industrial cameras from The Imaging Source.
The ones I tested were the DFK UX250 and DFK UX264 models.
I do not know if there are other devices that have the AUTO_UPDATE flag for various properties.

Since I received no immediate answer I tried fixing it myself.
The result can be found here:
https://patchwork.linuxtv.org/patch/43289/

Cheers,

Edgar

Re: UVC property auto update

[Guennadi Liakhovetski]

Hi Edgar,

Thanks for the explanation.

On Mon, 4 Sep 2017, Edgar Thier wrote:

> Hi Guennadi,
> 
> The cameras in question are USB-3.0 industrial cameras from The Imaging Source.
> The ones I tested were the DFK UX250 and DFK UX264 models.
> I do not know if there are other devices that have the AUTO_UPDATE flag for various properties.
> 
> Since I received no immediate answer I tried fixing it myself.
> The result can be found here:
> https://patchwork.linuxtv.org/patch/43289/

But that patch only re-reads the flags. What does that give you? Do those 
flags change? In which way? As far as I understand the UVC Autoupdate 
feature, a change in GET_INFO data is only one possibility, (arguably) a 
more important one is changes in GET_CUR data. In either case, this should 
be implemented using the UVC Interrupt endpoint. Here's my latest 
asynchronous control patch:

https://patchwork.linuxtv.org/patch/42800/

As you can see, it only handles the VALUE_CHANGE (GET_CUR) case. I would 
suggest implementing a patch on top of it to add support for INFO_CHANGE 
and you'd be the best person to test it then!

Thanks
Guennadi

> Cheers,
> 
> Edgar

Re: UVC property auto update

[Edgar Thier]

Hi Guennadi,


> But that patch only re-reads the flags. What does that give you? Do those > flags change? In which way? As far as I understand the UVC Autoupdate
> feature, a change in GET_INFO data is only one possibility, (arguably) a 
> more important one is changes in GET_CUR data.

My understanding of the driver is rather narrow, so please correct me if I am wrong.
>From what I can see the uvc driver is currently not asking the device if a property has self
modifying properties (and thus would require the AUTO_UPDATE flag).
This is only done for properties in the extension unit but not for 'standard' properties.
Thus properties exhibit different behavior depending on where they are defined.
By changing this the driver now assumes that a property with AUTO_UPDATE has to be re-read when
getting/setting a property and does not rely on cached values, no matter if extension unit or not.

I did not consider the possibility that a lower level change would be necessary or that a more
previce update mechanism for different property parts was possible.

Basically the entry point for my change would be here:
https://git.linuxtv.org/media_tree.git/tree/drivers/media/usb/uvc/uvc_ctrl.c#n1405

How an update is handled by the driver did not seem important for this patch as the retrieval of a
correct property value seemed more important.

> In either case, this should 
> be implemented using the UVC Interrupt endpoint. Here's my latest 
> asynchronous control patch:
> 
> https://patchwork.linuxtv.org/patch/42800/
> 
> As you can see, it only handles the VALUE_CHANGE (GET_CUR) case. I would 
> suggest implementing a patch on top of it to add support for INFO_CHANGE 
> and you'd be the best person to test it then!

I will try it out. I should be able to give you feedback tomorrow.
I will also ask the firmware developer if only value changes are available or flag changes are also
a possibility.

Cheers,

Edgar

Re: UVC property auto update

[Guennadi Liakhovetski]

On Mon, 4 Sep 2017, Edgar Thier wrote:

> Hi Guennadi,
> 
> 
> > But that patch only re-reads the flags. What does that give you? Do those 
> > flags change? In which way? As far as I understand the UVC Autoupdate
> > feature, a change in GET_INFO data is only one possibility, (arguably) a 
> > more important one is changes in GET_CUR data.

Sorry, I misunderstood, please, ignore that.

> My understanding of the driver is rather narrow, so please correct me if I am wrong.
> From what I can see the uvc driver is currently not asking the device if a property has self
> modifying properties (and thus would require the AUTO_UPDATE flag).
> This is only done for properties in the extension unit but not for 'standard' properties.
> Thus properties exhibit different behavior depending on where they are defined.
> By changing this the driver now assumes that a property with AUTO_UPDATE has to be re-read when
> getting/setting a property and does not rely on cached values, no matter if extension unit or not.
> 
> I did not consider the possibility that a lower level change would be necessary or that a more
> previce update mechanism for different property parts was possible.
> 
> Basically the entry point for my change would be here:
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/usb/uvc/uvc_ctrl.c#n1405
> 
> How an update is handled by the driver did not seem important for this patch as the retrieval of a
> correct property value seemed more important.

Ok, looking more at the spec, the driver and your patch, here's what I 
come up with:

1. UVC defines which standard controls should have which flags. Among 
those flags it specifies, which controls should specify the Autoupdate 
flag. E.g. see the first of them as it appears in my copy of the spec 
"4.2.2.4.8 Average Bit Rate Control"
2. The driver could read out flags from descriptors, but it hard-codes 
them instead. So, (arguably), there's no need to actually read them at 
probe time. XUs on the other hand aren't standard, therefore their flags 
have to be read out.
3. In your patch you provide gain as an example. Do you mean the 
PU_GAIN_CONTROL? The spec doesn't specify, that it should have Autoupdate 
set. Now, whether that means, that using that flag with PU_GAIN_CONTROL is 
a violation of the spec - I'm not sure about.

So, I think, the question really is - are hard-coded flags a proper and 
sufficient approach or should flags be read from descriptors?

> > In either case, this should 
> > be implemented using the UVC Interrupt endpoint. Here's my latest 
> > asynchronous control patch:
> > 
> > https://patchwork.linuxtv.org/patch/42800/
> > 
> > As you can see, it only handles the VALUE_CHANGE (GET_CUR) case. I would 
> > suggest implementing a patch on top of it to add support for INFO_CHANGE 
> > and you'd be the best person to test it then!
> 
> I will try it out. I should be able to give you feedback tomorrow.

Thanks.

> I will also ask the firmware developer if only value changes are available or flag changes are also
> a possibility.

Well, flags aren't likely to change, perhaps. I think min and max values 
are more likely to be updated.

Regards
Guennadi

> Cheers,
> Edgar

Re: UVC property auto update

[Edgar Thier]

> 
> Ok, looking more at the spec, the driver and your patch, here's what I 
> come up with:
> 
> 1. UVC defines which standard controls should have which flags. Among 
> those flags it specifies, which controls should specify the Autoupdate 
> flag. E.g. see the first of them as it appears in my copy of the spec 
> "4.2.2.4.8 Average Bit Rate Control"
> 2. The driver could read out flags from descriptors, but it hard-codes 
> them instead. So, (arguably), there's no need to actually read them at 
> probe time. XUs on the other hand aren't standard, therefore their flags 
> have to be read out.
> 3. In your patch you provide gain as an example. Do you mean the 
> PU_GAIN_CONTROL? The spec doesn't specify, that it should have Autoupdate 
> set. Now, whether that means, that using that flag with PU_GAIN_CONTROL is 
> a violation of the spec - I'm not sure about.
> 
> So, I think, the question really is - are hard-coded flags a proper and 
> sufficient approach or should flags be read from descriptors?
> 

That is the questioned I cannot answer. The current approach (with my patch) enables both.
The cameras I work with either assume no AUTO_UPDATE or try to define the FLAG themselves.
As to what the standard expects, I do not know as IMO it is not clearly enough defined if this flag
is optional or somehow expected. But I think that it makes more sense to ask the device
for its capabilities than the other way around. E.g. I have yet to encounter a camera that has hue
with AUTO_UPDATE yet the driver expects it.

> 
>> I will also ask the firmware developer if only value changes are available or flag changes are also
>> a possibility.
> 
> Well, flags aren't likely to change, perhaps. I think min and max values 
> are more likely to be updated.
> 

I just talked to him. There are no plans to use the auto update functionality for anything besides
GET_CUR. Flags could get messy since auto update itself could be toggled once other properties are
changed. These cross dependencies are not handled in the standard as far as I am aware.

> Well, flags aren't likely to change, perhaps. I think min and max values 
> are more likely to be updated.

That depends. When activating an auto feature, say auto-exposure. it could be interesting to set
exposure to read-only. For boundary changes I would say the question is how many users would
anticipate such a behavior.

>>>
>>> As you can see, it only handles the VALUE_CHANGE (GET_CUR) case. I would 
>>> suggest implementing a patch on top of it to add support for INFO_CHANGE 
>>> and you'd be the best person to test it then!
>>
>> I will try it out. I should be able to give you feedback tomorrow.
> 
> Thanks.
> 

Your patch works in combination with mine. I could not detect any problems.