Re: Correct way to do s_ctrl ioctl taking into account subdev framework?
Em 27-06-2010 00:26, Devin Heitmueller escreveu: > On Sat, Jun 26, 2010 at 9:34 PM, Mauro Carvalho Chehab > wrote: >> would do the trick. Yet, the application is broken, as it is considering a >> positive >> return as an error. A positive code should never be considered as an error. >> So, we >> need to fix v4l2-ctl as well (ok, returning 1 is wrong as well, as this is a >> non-v4l2 >> compliance in this case). > > A strict interpretation of the spec would read that returning zero is > success, -1 is an well-formed error condition, and *ANYTHING* else is > a violation of the spec and an application used for testing compliance > should complain very loudly (which is exactly what it does). > > In effect, the only patch I would consider valid for v4l2-ctl would be > one that makes the error even more LOUD than it already is. It should output it as an API violation, not as a failure on setting the value. That's said, I think that a few ioctl calls used to return positive values under certain special conditions. Not sure if this is a non-compliance or if the API allows it. Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Correct way to do s_ctrl ioctl taking into account subdev framework?
On Saturday 26 June 2010 21:04:51 Devin Heitmueller wrote: > On Sat, Jun 26, 2010 at 2:51 PM, Hans Verkuil wrote: > > There really is no good way at the moment to handle cases like this, or at > > least not without a lot of work. > > Ok, it's good to know I'm not missing something obvious. > > > The plan is to have the framework merged in time for 2.6.36. My last patch > > series for the framework already converts a bunch of subdevs to use it. Your > > best bet is to take the patch series and convert any remaining subdevs used > > by em28xx and em28xx itself. I'd be happy to add those patches to my patch > > series, so that when I get the go ahead the em28xx driver will be fixed > > automatically. > > > > It would be useful for me anyway to have someone else use it: it's a good > > check whether my documentation is complete. > > Sure, could you please point me to the tree in question and I'll take a look? http://git.linuxtv.org/hverkuil/v4l-dvb.git, branch ctrlfw5. This tree is based on the latest v4l-dvb master. Laurent, when you start working on moving UVC over to the control framework, please use this new branch. The old patch series no longer applies cleanly due to changes in the master. Regards, Hans > > Given I've got applications failing, for the short term I will likely > just submit a patch which makes the s_ctrl always return zero > regardless of the subdev response, instead of returning 1. > > Thanks, > > Devin > > -- Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Correct way to do s_ctrl ioctl taking into account subdev framework?
On Sat, Jun 26, 2010 at 9:34 PM, Mauro Carvalho Chehab wrote: > would do the trick. Yet, the application is broken, as it is considering a > positive > return as an error. A positive code should never be considered as an error. > So, we > need to fix v4l2-ctl as well (ok, returning 1 is wrong as well, as this is a > non-v4l2 > compliance in this case). A strict interpretation of the spec would read that returning zero is success, -1 is an well-formed error condition, and *ANYTHING* else is a violation of the spec and an application used for testing compliance should complain very loudly (which is exactly what it does). In effect, the only patch I would consider valid for v4l2-ctl would be one that makes the error even more LOUD than it already is. > We might add a new handler at subdev, but, as Laurent is reworking > it, the above trick would be an acceptable workaround. Great. I'll submit a patch to this effect, which would be applicable until we have a final solution in place. Thanks, Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Correct way to do s_ctrl ioctl taking into account subdev framework?
Em 26-06-2010 16:04, Devin Heitmueller escreveu:
> On Sat, Jun 26, 2010 at 2:51 PM, Hans Verkuil wrote:
>> There really is no good way at the moment to handle cases like this, or at
>> least not without a lot of work.
>
> Ok, it's good to know I'm not missing something obvious.
>
>> The plan is to have the framework merged in time for 2.6.36. My last patch
>> series for the framework already converts a bunch of subdevs to use it. Your
>> best bet is to take the patch series and convert any remaining subdevs used
>> by em28xx and em28xx itself. I'd be happy to add those patches to my patch
>> series, so that when I get the go ahead the em28xx driver will be fixed
>> automatically.
>>
>> It would be useful for me anyway to have someone else use it: it's a good
>> check whether my documentation is complete.
>
> Sure, could you please point me to the tree in question and I'll take a look?
>
> Given I've got applications failing, for the short term I will likely
> just submit a patch which makes the s_ctrl always return zero
> regardless of the subdev response, instead of returning 1.
Yeah, something like:
if (rc = 1) {
rc = 0;
...
would do the trick. Yet, the application is broken, as it is considering a
positive
return as an error. A positive code should never be considered as an error. So,
we
need to fix v4l2-ctl as well (ok, returning 1 is wrong as well, as this is a
non-v4l2
compliance in this case).
We might add a new handler at subdev, but, as Laurent is reworking
it, the above trick would be an acceptable workaround.
Cheers,
Mauro.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Correct way to do s_ctrl ioctl taking into account subdev framework?
On Sat, Jun 26, 2010 at 2:51 PM, Hans Verkuil wrote: > There really is no good way at the moment to handle cases like this, or at > least not without a lot of work. Ok, it's good to know I'm not missing something obvious. > The plan is to have the framework merged in time for 2.6.36. My last patch > series for the framework already converts a bunch of subdevs to use it. Your > best bet is to take the patch series and convert any remaining subdevs used > by em28xx and em28xx itself. I'd be happy to add those patches to my patch > series, so that when I get the go ahead the em28xx driver will be fixed > automatically. > > It would be useful for me anyway to have someone else use it: it's a good > check whether my documentation is complete. Sure, could you please point me to the tree in question and I'll take a look? Given I've got applications failing, for the short term I will likely just submit a patch which makes the s_ctrl always return zero regardless of the subdev response, instead of returning 1. Thanks, Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Correct way to do s_ctrl ioctl taking into account subdev framework?
Hi Devin, On Saturday 26 June 2010 20:37:51 Devin Heitmueller wrote: > First, a bit of background: > > A bug in the em28xx implementation of s_ctrl() was present where it > would always return 1, even in success cases, regardless of what the > subdev servicing the request said (in this case the video decoder). > It was using v4l2_device_call_all(), and disregarding the return value > from any of the subdevs. > > This prompted me to change the code so that it started using > v4l2_device_call_until_err(), figuring that subdevs that did not > support it would simply return -ENOIOCTLCMD. However, as Mauro > correctly pointed out, subdevs that do implement s_ctrl, but not the > desired control will return -EINVAL, which would cause the bridge to > stop sending the command to other subdevs and return an error. > > I looked at various other bridges, and don't see any consistent > approach for this case. Some of the bridges always return zero > (regardless of what happened during the call). Some of them look at > the content of the resulting struct for some value that suggests it > was changed. Others feed the call to different classes of subdevice > depending on what the actual control being set was. > > So what's the "right" approach? I'm willing to conform to whatever > the recommendation is here, since it will obviously be an improvement > over always returning 1 (even always returning zero would be better > since at least applications wouldn't treat it as a failure). > > Hans? The correct approach is to wait until the control framework is merged. This depends on Laurent implementing it first in UVC so we can be sure that the framework can handle the special UVC requirements. There really is no good way at the moment to handle cases like this, or at least not without a lot of work. The plan is to have the framework merged in time for 2.6.36. My last patch series for the framework already converts a bunch of subdevs to use it. Your best bet is to take the patch series and convert any remaining subdevs used by em28xx and em28xx itself. I'd be happy to add those patches to my patch series, so that when I get the go ahead the em28xx driver will be fixed automatically. It would be useful for me anyway to have someone else use it: it's a good check whether my documentation is complete. I don't see any point in attempting to fix this in another way, it's just plain broken at the moment. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Correct way to do s_ctrl ioctl taking into account subdev framework?
First, a bit of background: A bug in the em28xx implementation of s_ctrl() was present where it would always return 1, even in success cases, regardless of what the subdev servicing the request said (in this case the video decoder). It was using v4l2_device_call_all(), and disregarding the return value from any of the subdevs. This prompted me to change the code so that it started using v4l2_device_call_until_err(), figuring that subdevs that did not support it would simply return -ENOIOCTLCMD. However, as Mauro correctly pointed out, subdevs that do implement s_ctrl, but not the desired control will return -EINVAL, which would cause the bridge to stop sending the command to other subdevs and return an error. I looked at various other bridges, and don't see any consistent approach for this case. Some of the bridges always return zero (regardless of what happened during the call). Some of them look at the content of the resulting struct for some value that suggests it was changed. Others feed the call to different classes of subdevice depending on what the actual control being set was. So what's the "right" approach? I'm willing to conform to whatever the recommendation is here, since it will obviously be an improvement over always returning 1 (even always returning zero would be better since at least applications wouldn't treat it as a failure). Hans? Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
