| Message ID | 201106152155.57978.hverkuil@xs4all.nl (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On Wed, Jun 15, 2011 at 3:55 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > Why would that violate the spec? If the last filehandle is closed, then > you can safely poweroff the tuner. The only exception is when you have a radio > tuner whose audio output is hooked up to some line-in: there you can't power off > the tuner. The use case that some expect to work is: v4l2-ctl <set standard> v4l2-ctl <set frequency> cat /dev/video0 > out.mpg By powering off the tuner after v4l2-ctl closes the device node, the cat won't work as expected because the tuner will be powered down. >> We've been forced to choose between the purist perspective, which is >> properly preserving state, never powering down the tuner and sucking >> up 500ma on the USB port when not using the tuner, versus powering >> down the tuner when the last party closes the filehandle, which >> preserves power but breaks v4l2 conformance and in some cases is >> highly noticeable with tuners that require firmware to be reloaded >> when being powered back up. > > Seems fine to me. What isn't fine is that a driver like e.g. em28xx powers off > the tuner but doesn't power it on again on the next open. It won't do that > until the first S_FREQUENCY/S_TUNER/S_STD call. You don't want to power up the tuner unless you know the user intends to use it. For example, you don't want to power up the tuner if the user intends to capture on composite/s-video input (as this consumes considerably more power). Devin
On Wednesday, June 15, 2011 22:09:45 Devin Heitmueller wrote: > On Wed, Jun 15, 2011 at 3:55 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > Why would that violate the spec? If the last filehandle is closed, then > > you can safely poweroff the tuner. The only exception is when you have a radio > > tuner whose audio output is hooked up to some line-in: there you can't power off > > the tuner. > > The use case that some expect to work is: > > v4l2-ctl <set standard> > v4l2-ctl <set frequency> > cat /dev/video0 > out.mpg > > By powering off the tuner after v4l2-ctl closes the device node, the > cat won't work as expected because the tuner will be powered down. > > >> We've been forced to choose between the purist perspective, which is > >> properly preserving state, never powering down the tuner and sucking > >> up 500ma on the USB port when not using the tuner, versus powering > >> down the tuner when the last party closes the filehandle, which > >> preserves power but breaks v4l2 conformance and in some cases is > >> highly noticeable with tuners that require firmware to be reloaded > >> when being powered back up. > > > > Seems fine to me. What isn't fine is that a driver like e.g. em28xx powers off > > the tuner but doesn't power it on again on the next open. It won't do that > > until the first S_FREQUENCY/S_TUNER/S_STD call. > > You don't want to power up the tuner unless you know the user intends > to use it. For example, you don't want to power up the tuner if the > user intends to capture on composite/s-video input (as this consumes > considerably more power). But the driver has that information, so it should act accordingly. So on first open you can check whether the current input has a tuner and power on the tuner in that case. On S_INPUT you can also poweron/off accordingly (bit iffy against the spec, though). So in that case the first use case would actually work. It does require that tuner-core.c supports s_power(1), of course. BTW, I noticed in tuner-core.c that the g_tuner op doesn't wake-up the tuner when called. I think it should be added, even though most (all?) tuners will need time before they can return anything sensible. BTW2: it's not a good idea to just broadcast s_power to all subdevs. That should be done to the tuner(s) only since other subdevs might also implement s_power. For now it's pretty much just tuners and some sensors, though. You know, this really needs to be a standardized module option and/or sysfs entry: 'always on', 'wake up on first open', 'wake up on first use'. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 15, 2011 at 4:37 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > But the driver has that information, so it should act accordingly. > > So on first open you can check whether the current input has a tuner and > power on the tuner in that case. On S_INPUT you can also poweron/off accordingly > (bit iffy against the spec, though). So in that case the first use case would > actually work. It does require that tuner-core.c supports s_power(1), of course. This will get messy, and is almost certain to get screwed up and cause regressions at least on some devices. > BTW, I noticed in tuner-core.c that the g_tuner op doesn't wake-up the tuner > when called. I think it should be added, even though most (all?) tuners will > need time before they can return anything sensible. Bear in mind that some tuners can take several seconds to load firmware when powered up. You don't want a situation where the tuner is reloading firmware continuously, the net result being that calls to v4l2-ctl that used to take milliseconds now take multiple seconds. > BTW2: it's not a good idea to just broadcast s_power to all subdevs. That should > be done to the tuner(s) only since other subdevs might also implement s_power. > For now it's pretty much just tuners and some sensors, though. > > You know, this really needs to be a standardized module option and/or sysfs > entry: 'always on', 'wake up on first open', 'wake up on first use'. That would definitely be useful, but it shouldn't be a module option since you can have multiple devices using the same module. It really should be an addition to the V4L API. Devin
On Wednesday, June 15, 2011 22:49:39 Devin Heitmueller wrote: > On Wed, Jun 15, 2011 at 4:37 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > But the driver has that information, so it should act accordingly. > > > > So on first open you can check whether the current input has a tuner and > > power on the tuner in that case. On S_INPUT you can also poweron/off accordingly > > (bit iffy against the spec, though). So in that case the first use case would > > actually work. It does require that tuner-core.c supports s_power(1), of course. > > This will get messy, and is almost certain to get screwed up and cause > regressions at least on some devices. I don't see why this should be messy. Anyway, this is all theoretical as long as tuner-core doesn't support s_power(1). Let's get that in first. > > BTW, I noticed in tuner-core.c that the g_tuner op doesn't wake-up the tuner > > when called. I think it should be added, even though most (all?) tuners will > > need time before they can return anything sensible. > > Bear in mind that some tuners can take several seconds to load > firmware when powered up. You don't want a situation where the tuner > is reloading firmware continuously, the net result being that calls to > v4l2-ctl that used to take milliseconds now take multiple seconds. Yes, but calling VIDIOC_G_TUNER is a good time to wake up the tuner :-) > > BTW2: it's not a good idea to just broadcast s_power to all subdevs. That should > > be done to the tuner(s) only since other subdevs might also implement s_power. > > For now it's pretty much just tuners and some sensors, though. > > > > You know, this really needs to be a standardized module option and/or sysfs > > entry: 'always on', 'wake up on first open', 'wake up on first use'. > > That would definitely be useful, but it shouldn't be a module option > since you can have multiple devices using the same module. Of course, I forgot about that. > It really > should be an addition to the V4L API. This would actually for once be a good use of sysfs. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 16-06-2011 03:21, Hans Verkuil escreveu: > On Wednesday, June 15, 2011 22:49:39 Devin Heitmueller wrote: >> On Wed, Jun 15, 2011 at 4:37 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >>> But the driver has that information, so it should act accordingly. >>> >>> So on first open you can check whether the current input has a tuner and >>> power on the tuner in that case. On S_INPUT you can also poweron/off accordingly >>> (bit iffy against the spec, though). So in that case the first use case would >>> actually work. It does require that tuner-core.c supports s_power(1), of course. >> >> This will get messy, and is almost certain to get screwed up and cause >> regressions at least on some devices. > > I don't see why this should be messy. Anyway, this is all theoretical as long > as tuner-core doesn't support s_power(1). Let's get that in first. Adding s_power to tuner-core is the easiest part. The hardest one is to decide what would be the proper behaviour. See bellow. >>> BTW, I noticed in tuner-core.c that the g_tuner op doesn't wake-up the tuner >>> when called. I think it should be added, even though most (all?) tuners will >>> need time before they can return anything sensible. >> >> Bear in mind that some tuners can take several seconds to load >> firmware when powered up. You don't want a situation where the tuner >> is reloading firmware continuously, the net result being that calls to >> v4l2-ctl that used to take milliseconds now take multiple seconds. The question is not when to wake up the tuner, but when to put it to sleep. Really, the big issue is on USB devices, where we don't want to spend lots of power while the device is not active. Some devices even become hot when the tuner is not sleeping. As Devin pointed, some devices like tm6000 take about 30-45 seconds for loading a firmware (ok, it is a broken design. We should not take it as a good example). Currently, there's no way to know when a radio device is being used or not. Even for video, scripts that call v4l-ctl or v4l2-ctl and then start some userspace application are there for years. We have even an example for that at the v4l-utils: contrib/v4l_rec.pl. One possible logic that would solve the scripting would be to use a watchdog to monitor ioctl activities. If not used for a while, it could send a s_power to put the device to sleep, but this may not solve all our problems. So, I agree with Devin: we need to add an option to explicitly control the power management logic of the device, having 3 modes of operation: POWER_AUTO - use the watchdogs to poweroff POWER_ON - explicitly powers on whatever subdevices are needed in order to make the V4L ready to stream; POWER_OFF - Put all subdevices to power-off if they support it. After implementing such logic, and keeping the default as POWER_ON, we may announce that the default will change to POWER_AUTO, and give some time for userspace apps/scripts that need to use a different mode to change their behaviour. That means that, for example, "radio -qf" will need to change to POWER_ON mode, and "radio -m" should call POWER_OFF. It would be good if the API would also provide a way to warn userspace that a given device supports it or not (maybe at VIDIOC_QUERYCTL?). I think that such API can be implemented as a new V4L control. > Yes, but calling VIDIOC_G_TUNER is a good time to wake up the tuner :-) Not really. Several popular devices load firmware based on the standard (the ones based on xc2028/xc3028/xc4000). So, before sending any tuner command, a VIDIOC_S_STD is needed. >>> BTW2: it's not a good idea to just broadcast s_power to all subdevs. That should >>> be done to the tuner(s) only since other subdevs might also implement s_power. >>> For now it's pretty much just tuners and some sensors, though. >>> >>> You know, this really needs to be a standardized module option and/or sysfs >>> entry: 'always on', 'wake up on first open', 'wake up on first use'. >> >> That would definitely be useful, but it shouldn't be a module option >> since you can have multiple devices using the same module. > > Of course, I forgot about that. > >> It really >> should be an addition to the V4L API. > > This would actually for once be a good use of sysfs. > > Regards, > > Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 16, 2011 at 7:14 AM, Mauro Carvalho Chehab <mchehab@redhat.com> wrote: > One possible logic that would solve the scripting would be to use a watchdog > to monitor ioctl activities. If not used for a while, it could send a s_power > to put the device to sleep, but this may not solve all our problems. > > So, I agree with Devin: we need to add an option to explicitly control the > power management logic of the device, having 3 modes of operation: > POWER_AUTO - use the watchdogs to poweroff > POWER_ON - explicitly powers on whatever subdevices are needed in > order to make the V4L ready to stream; > POWER_OFF - Put all subdevices to power-off if they support it. > > After implementing such logic, and keeping the default as POWER_ON, we may > announce that the default will change to POWER_AUTO, and give some time for > userspace apps/scripts that need to use a different mode to change their > behaviour. That means that, for example, "radio -qf" will need to change to > POWER_ON mode, and "radio -m" should call POWER_OFF. I've considered this idea before, and it's not bad in theory. The one thing you will definitely have to watch out for is causing a race between DVB and V4L for hybrid tuners. In other words, you can have a user switching from analog to digital and you don't want the tuner to get powered down a few seconds after they started streaming video from DVB. Any such solution would have to take the above into account. We've got a history of race conditions like this and I definitely don't want to see a new one introduced. Devin
Hi. I think that you could reuse lots of code with smart suspend / resume. What do you think about this DVB power saving case (about the concept, don't look at details, please): - One device has responsibility to do the power off when it can be done (mantis_core.ko) - In my case there is only one frontend tda10021.ko to take care of. - dvb_frontend.c would call fe->sleep(fe). The callback goes into mantis_core.ko. - mantis_core.ko will traverse all devices on it's responsibility, all (tda10021.ko) are idle. => suspend tda10021.ko by calling tda10021->sleep() and do additional power off things. - When dvb_frontend.c wants tuner to be woken up, mantis_core.ko does hardware resets and power on first and then resumes tda10021->init(fe). I implemented something that worked a few years ago with suspend / resume. In mantis_dvb.c's driver probe function I modified tuner_ops to enable these runtime powersaving features: + mantis->fe->ops.tuner_ops.sleep = mantis_dvb_tuner_sleep; + mantis->fe->ops.tuner_ops.init = mantis_dvb_tuner_init; That way mantis_core.ko had all needed details to do any advanced power savings I wanted. Suspend / resume worked well: During resume there was only a glitch at the picture and sound and the TV channel watching continued. tda10021 (was cu1216 at that time) restored the original TV channel. It took DVB FE_LOCK during resume. Suspended DMA transfer was recovered before returning into userspace. So I think that you need a single device (mantis_core.ko) that can see the whole picture, in what states the subdevices are: can you touch the power button?. With DVB this is easy because dvb_frontend.c tells when a frontend is idle and when it is not. The similar idea of some kind of watchdog that is able to track when a single device (frontend) is used and when it is not used, would be sufficient. The topmost driver (mantis_core.ko in my case) would then be responsible to track multiple frontends (subdevices), if they all are idle or not, with suitable mutex protection. Then this driver could easilly suspend/resume these subdevices and press power switch when necessary. So the clash between DVB and V4L devices would be solved: Both DVB and V4L calls a (different) sleep() function on mantis_core.ko mantis_core.ko will turn power off when both "frontends" are sleeping. If only one sleeps, the one can be put to sleep or suspended, but power button can't be touched. What do you think? I did this easy case mantis_core.ko solution in about Summer 2007. It needs a rewrite and testing, if I take it from the dust. Regards, Marko Ristola 16.06.2011 15:51, Devin Heitmueller wrote: > On Thu, Jun 16, 2011 at 7:14 AM, Mauro Carvalho Chehab > <mchehab@redhat.com> wrote: >> One possible logic that would solve the scripting would be to use a watchdog >> to monitor ioctl activities. If not used for a while, it could send a s_power >> to put the device to sleep, but this may not solve all our problems. >> >> So, I agree with Devin: we need to add an option to explicitly control the >> power management logic of the device, having 3 modes of operation: >> POWER_AUTO - use the watchdogs to poweroff >> POWER_ON - explicitly powers on whatever subdevices are needed in >> order to make the V4L ready to stream; >> POWER_OFF - Put all subdevices to power-off if they support it. >> >> After implementing such logic, and keeping the default as POWER_ON, we may >> announce that the default will change to POWER_AUTO, and give some time for >> userspace apps/scripts that need to use a different mode to change their >> behaviour. That means that, for example, "radio -qf" will need to change to >> POWER_ON mode, and "radio -m" should call POWER_OFF. > > I've considered this idea before, and it's not bad in theory. The one > thing you will definitely have to watch out for is causing a race > between DVB and V4L for hybrid tuners. In other words, you can have a > user switching from analog to digital and you don't want the tuner to > get powered down a few seconds after they started streaming video from > DVB. > > Any such solution would have to take the above into account. We've > got a history of race conditions like this and I definitely don't want > to see a new one introduced. > > Devin > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 6, 2011 at 1:53 PM, Marko Ristola <marko.ristola@kolumbus.fi> wrote: > > Hi. > > I think that you could reuse lots of code with smart suspend / resume. > > What do you think about this DVB power saving case (about the concept, don't look at details, please): > > - One device has responsibility to do the power off when it can be done (mantis_core.ko) > - In my case there is only one frontend tda10021.ko to take care of. > > - dvb_frontend.c would call fe->sleep(fe). The callback goes into mantis_core.ko. > - mantis_core.ko will traverse all devices on it's responsibility, all (tda10021.ko) are idle. > => suspend tda10021.ko by calling tda10021->sleep() and do additional power off things. > > - When dvb_frontend.c wants tuner to be woken up, > mantis_core.ko does hardware resets and power on first and then resumes tda10021->init(fe). > > I implemented something that worked a few years ago with suspend / resume. > In mantis_dvb.c's driver probe function I modified tuner_ops to enable these runtime powersaving features: > + mantis->fe->ops.tuner_ops.sleep = mantis_dvb_tuner_sleep; > + mantis->fe->ops.tuner_ops.init = mantis_dvb_tuner_init; > > That way mantis_core.ko had all needed details to do any advanced power savings I wanted. > > Suspend / resume worked well: During resume there was only a glitch at the picture and sound > and the TV channel watching continued. tda10021 (was cu1216 at that time) > restored the original TV channel. It took DVB FE_LOCK during resume. > Suspended DMA transfer was recovered before returning into userspace. > > So I think that you need a single device (mantis_core.ko) that can see the whole picture, > in what states the subdevices are: can you touch the power button?. > With DVB this is easy because dvb_frontend.c tells when a frontend is idle and when it is not. > > The similar idea of some kind of watchdog that is able to track when a single > device (frontend) is used and when it is not used, would be sufficient. > > The topmost driver (mantis_core.ko in my case) would then be responsible to track multiple frontends > (subdevices), if they all are idle or not, with suitable mutex protection. > Then this driver could easilly suspend/resume these subdevices and press power switch when necessary. > > > So the clash between DVB and V4L devices would be solved: > Both DVB and V4L calls a (different) sleep() function on mantis_core.ko > mantis_core.ko will turn power off when both "frontends" are sleeping. > If only one sleeps, the one can be put to sleep or suspended, but power > button can't be touched. > > What do you think? > > I did this easy case mantis_core.ko solution in about Summer 2007. > It needs a rewrite and testing, if I take it from the dust. > > Regards, > Marko Ristola Hi Marko, This is one of those ideas that sounds good in theory but in practice getting it to work with all the different types of boards is really a challenge. It would need to take into account devices with multiple frontends, shared tuners between V4L and DVB, shared tuners between different DVB frontends, etc. For example, the DVB frontend call to sleep the demod is actually deferred. This exposes all sorts of fun races with applications such as MythTV which switch between DVB and V4L modes in fast succession. For example, on the HVR-950Q I debugged an issue last year where switching from DVB to V4L would close the DVB frontend device, immediately open the V4L device, start tuning the V4L device, and then a couple hundred milliseconds later the sleep call would arrive from the DVB frontend thread which would screw up the tuner state. In other words, the locking is not trivial and you need to take into account the various threads that can be running. Taking the above example, if you had deferred freeing up the device until the sleep call arrived, then the DVB close would have returned immediately, and the V4L open would have failed because the device was "still in use". Also, you need to take into consideration that bringing some devices out of sleep can be a *very* time consuming operation. This is mostly due to loading of firmware, which on some devices can take upwards of ten seconds due to large blobs and slow i2c busses. Hence if you have too granular a power management strategy you end up with a situation where you are continuously calling a resume routine which takes ten seconds. This adds up quickly if for example you call v4l2-ctl half a dozen times before starting streaming. All that said, I believe that you are correct in that the business logic needs to ultimately be decided by the bridge driver, rather than having the dvb/tuner core blindly calling the sleep routines against the tuner and demod drivers without a full understanding of what impact it has on the board as a whole. Cheers, Devin
06.07.2011 21:17, Devin Heitmueller kirjoitti: > On Wed, Jul 6, 2011 at 1:53 PM, Marko Ristola <marko.ristola@kolumbus.fi> wrote: >> > > All that said, I believe that you are correct in that the business > logic needs to ultimately be decided by the bridge driver, rather than > having the dvb/tuner core blindly calling the sleep routines against > the tuner and demod drivers without a full understanding of what > impact it has on the board as a whole. You wrote it nicely and compactly. What do you think about tracking coarse last busy time rather than figuring out accurate idle time? dvb_frontend.c and V4L side would just poll the device: "bridge->wake()". wake() will just store current "busy" timestamp to the bridge device with coarse accuracy, if subdevices are already at active state. If subdevices are powered off, it will first power them on and resume them, and then store "busy" timestamp. Bridge device would have a "delayed task": "Check after 3 minutes: If I haven't been busy for three minutes, I'll go to sleep. I'll suspend the subdevices and power them off." The "delayed task" would refresh itself: check again after last awake time + 3 minutes. "Delayed task" could be further developed to support multiple suspend states. > > Cheers, > > Devin > Marko -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 06-07-2011 16:59, Marko Ristola escreveu: > 06.07.2011 21:17, Devin Heitmueller kirjoitti: >> On Wed, Jul 6, 2011 at 1:53 PM, Marko Ristola <marko.ristola@kolumbus.fi> wrote: >>> >> >> All that said, I believe that you are correct in that the business >> logic needs to ultimately be decided by the bridge driver, rather than >> having the dvb/tuner core blindly calling the sleep routines against >> the tuner and demod drivers without a full understanding of what >> impact it has on the board as a whole. > > You wrote it nicely and compactly. > > What do you think about tracking coarse last busy time rather than figuring out accurate idle time? > > dvb_frontend.c and V4L side would just poll the device: > "bridge->wake()". wake() will just store current "busy" timestamp to the bridge device > with coarse accuracy, if subdevices are already at active state. > If subdevices are powered off, it will first power them on and resume them, and then store "busy" timestamp. > > Bridge device would have a "delayed task": "Check after 3 minutes: If I haven't been busy > for three minutes, I'll go to sleep. I'll suspend the subdevices and power them off." > > The "delayed task" would refresh itself: check again after last awake time + 3 minutes. > > "Delayed task" could be further developed to support multiple suspend states. There is still an issue: Devices that support FM radio may stay closed, but with radio powered on. This is supported since the beginning by radio application (part of xawtv package). If the device is on radio mode, the only reliable way to power the device off is if the device is muted. IMO, the proper solution is to provide a core resource locking mechanism, that will keep track about what device resources are in usage (tuner, analog audio/video demods, digital demods, sec, radio reception, i2c buses, etc), and some mechanisms like the above that will power the subdevice off when it is not being used for a given amount of time (a Kconfig option can be added so set the time to X minutes or to disable such mechanism, in order to preserve back compatibility). Having a core mechanism helps to assure that it will be properly implemented on all places. This locking mechanism will be controlled by the bridge driver. It is easy to say, but it may be hard to implement, as it may require some changes at both the V4L/DVB core and at the drivers. One special case for the locking mechanisms that may or may not be covered by such core resource locking is the access to the I2C buses. Currently, the DVB, V4L and remote controller stacks may try to use resources behind I2C at the same time. The I2C core has a locking schema, but it works only if a transaction is atomically commanded. So, if it requires multiple I2C transfers, all of them need to be grouped into one i2c xfer call. Complex operations like firmware transfers are protected by the I2C locking, so one driver might be generating RC polling events while a firmware is being transferring, causing it to fail. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi. IMO, your thoughts about core resource locking mechanism sound good. I say here some thoughts and examples how the resource locking mechanism might work. IMHO, It's good enough now if we are heading to a solution. At least I would not alone find a proper concept. 07.07.2011 18:14, Mauro Carvalho Chehab kirjoitti: > Em 06-07-2011 16:59, Marko Ristola escreveu: >> 06.07.2011 21:17, Devin Heitmueller kirjoitti: >>> On Wed, Jul 6, 2011 at 1:53 PM, Marko Ristola <marko.ristola@kolumbus.fi> wrote: >>>> > > IMO, the proper solution is to provide a core resource locking mechanism, that will keep > track about what device resources are in usage (tuner, analog audio/video demods, digital > demods, sec, radio reception, i2c buses, etc), and some mechanisms like the above that will > power the subdevice off when it is not being used for a given amount of time (a Kconfig option > can be added so set the time to X minutes or to disable such mechanism, in order to preserve > back compatibility). > > One special case for the locking mechanisms that may or may not be covered by such core > resource locking is the access to the I2C buses. Currently, the DVB, V4L and remote controller > stacks may try to use resources behind I2C at the same time. The I2C core has a locking schema, > but it works only if a transaction is atomically commanded. So, if it requires multiple I2C > transfers, all of them need to be grouped into one i2c xfer call. Complex operations like firmware > transfers are protected by the I2C locking, so one driver might be generating RC polling events > while a firmware is being transferring, causing it to fail. A generic locking schema could have shared/exclusive locks by name (device name, pointer ?). A generic resource "set of named locks" could contain these locks. Firmware load would take an exclusive lock on "I2C master" for the whole transfer. RC polling would take a shared lock on "I2C master" and an exclusive lock on "RC UART" device. Or if there is no need to share I2C device, RC polling would just take exclusive lock on "I2C Master". If I2C bus must be quiesced for 10ms after frontend's tuning action, "I2C master" exclusive lock could be held 10ms after last I2C transfer. "i2c/bridge1" state should be protected if the frontend is behind an I2C bridge. The existing I2C xfer mutex would be as it is now: it is a lower level lock, no regressions to come. Taking a shared lock of "tuner_power_switch" would mark that the device must not be turned off. While holding the shared lock, no "deferred watch" would be needed. While releasing "tuner_power_switch" lock, usage timestamp on that name should be updated. If there are no more "tuner_power_switch" holders, "delayed task" could be activated and run after 3 minutes to power the device off if needed. Bridge drivers that don't have full runtime power saving support, would not introduce a callback which a "delayed task" would run to turn power off / on. > > Regards, > Mauro IMO, suspend / resume code must be a separate thing. We might want to suspend the laptop while watching a DVB channel. We don't want to have runtime power management active while watching a DVB channel. Suspend quiesces the device possibly in one phase. It needs to have the device in a good state before suspending: take an exclusive "I2C Master" lock before going to suspend. While resuming, release "I2C Master" lock. So even though these details are incomplete, suspend/resume could perhaps be integrated with the generic advanced locking scheme. Regards, Marko -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/video/em28xx/em28xx-video.c b/drivers/media/video/em28xx/em28xx-video.c index 7b6461d..6f3f51b 100644 --- a/drivers/media/video/em28xx/em28xx-video.c +++ b/drivers/media/video/em28xx/em28xx-video.c @@ -2111,6 +2111,7 @@ static int em28xx_v4l2_open(struct file *filp) em28xx_wake_i2c(dev); } + v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 1); if (fh->radio) { em28xx_videodbg("video_open: setting radio device\n"); v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, s_radio); diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c index 9b0d833..9006c9a 100644 --- a/drivers/media/video/tuner-core.c +++ b/drivers/media/video/tuner-core.c @@ -1057,16 +1057,20 @@ static int tuner_s_radio(struct v4l2_subdev *sd) /** * tuner_s_power - controls the power state of the tuner * @sd: pointer to struct v4l2_subdev - * @on: a zero value puts the tuner to sleep + * @on: a zero value puts the tuner to sleep, non-zero wakes it up */ static int tuner_s_power(struct v4l2_subdev *sd, int on) { struct tuner *t = to_tuner(sd); struct analog_demod_ops *analog_ops = &t->fe.ops.analog_ops; - /* FIXME: Why this function don't wake the tuner if on != 0 ? */ - if (on) + if (on) { + if (t->standby && set_mode(t, t->mode) == 0) { + tuner_dbg("Waking up tuner\n"); + set_freq(t, 0); + } return 0; + } tuner_dbg("Putting tuner to sleep\n"); t->standby = true;