tda18271 driver power consumption

  On Thu, Sep 20, 2012 at 1:47 PM, Michael Krufky <mkrufky@linuxtv.org> wrote:
> On Mon, Aug 6, 2012 at 3:13 PM, Antti Palosaari <crope@iki.fi> wrote:
>> On 08/06/2012 09:57 PM, Michael Krufky wrote:
>>>
>>> On Mon, Aug 6, 2012 at 2:35 PM, Devin Heitmueller
>>> <dheitmueller@kernellabs.com> wrote:
>>>>
>>>> On Mon, Aug 6, 2012 at 2:19 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>
>>>>> You should understand from DVB driver model:
>>>>> * attach() called only once when driver is loaded
>>>>> * init() called to wake-up device
>>>>> * sleep() called to sleep device
>>>>>
>>>>> What I would like to say is that there is very big risk to shoot own leg
>>>>> when doing some initialization on attach(). For example resuming from
>>>>> the
>>>>> suspend could cause device reset and if you rely some settings that are
>>>>> done
>>>>> during attach() you will likely fail as Kernel / USB-host controller has
>>>>> reset your device.
>>>>>
>>>>> See reset_resume from Kernel documentation:
>>>>> Documentation/usb/power-management.txt
>>>>
>>>>
>>>> Be forewarned:  there is a very high likelihood that this patch will
>>>> cause regressions on hybrid devices due to known race conditions
>>>> related to dvb_frontend sleeping the tuner asynchronously on close.
>>>> This is a hybrid tuner, and unless code is specifically added to the
>>>> bridge or tuner driver, going from digital to analog mode too quickly
>>>> will cause the tuner to be shutdown while it's actively in analog
>>>> mode.
>>>>
>>>> (I discovered this the hard way when working on problems MythTV users
>>>> reported against the HVR-950q).
>>>>
>>>> Description of race:
>>>>
>>>> 1.  User opens DVB frontend tunes
>>>> 2.  User closes DVB frontend
>>>> 3.  User *immediately* opens V4L device using same tuner
>>>> 4.  User performs tuning request for analog
>>>> 5.  DVB frontend issues sleep() call to tuner, causing analog tuning to
>>>> fail.
>>>>
>>>> This class of problem isn't seen on DVB only devices or those that
>>>> have dedicated digital tuners not shared for analog usage.  And in
>>>> some cases it isn't noticed because a delay between closing the DVB
>>>> device and opening the analog devices causes the sleep() call to
>>>> happen before the analog tune (in which case you don't hit the race).
>>>>
>>>> I'm certainly not against improved power management, but it will
>>>> require the race conditions to be fixed first in order to avoid
>>>> regressions.
>>>>
>>>> Devin
>>>>
>>>> --
>>>> Devin J. Heitmueller - Kernel Labs
>>>> http://www.kernellabs.com
>>>
>>>
>>> Devin's right.  I'm sorry, please *don't* merge the patch, Mauro.
>>> Antti, you should just call sleep from your driver after attach(), as
>>> I had previously suggested.  We can revisit this some time in the
>>> future after we can be sure that these race conditions wont occur.
>>
>>
>> OK, maybe it is safer then. I have no any hybrid hardware to test...
>>
>> Anyhow, I wonder how many years it will take to resolve that V4L2/DVB API
>> hybrid usage p?oblem. I ran thinking that recently when looked how to
>> implement DVB SDR for V4L2 API... I could guess problem will not disappear
>> near future even analog TV disappears, because there is surely coming new
>> nasty features which spreads over both V4L2 and DVB APIs.
>
> Guys,
>
> Please take another look at this branch and test if possible -- I
> pushed an additional patch that takes Devin's concerns into account.
> After applying these patches, the tda18271 driver will behave as it
> always has, but it will sleep the tuner after attaching the first
> instance.  If there is only one instance, then this works exactly as
> Antti desires.  If there are more instances, then the tuner will only
> be woken up again during attach if the tda18271_need_cal_on_startup()
> returns non-zero.  The driver does not attempt to re-sleep the
> hardware again during successive attach() calls.
>
> I have not tested this yet myself, but I believe it resolves the
> matter -- please comment.
>
> Regards,
>
> Mike Krufky

...in case the URL got lost:


The following changes since commit 0c7d5a6da75caecc677be1fda207b7578936770d:

  Linux 3.5-rc5 (2012-07-03 22:57:41 +0300)

are available in the git repository at:

  git://linuxtv.org/mkrufky/tuners tda18271

for you to fetch changes up to 4e46c5d1bbb920165fecfe7de18b2c01d9787230:

  tda18271: make 'low-power standby mode after attach' multi-instance
safe (2012-09-20 13:34:29 -0400)

----------------------------------------------------------------
Michael Krufky (2):
      tda18271: enter low-power standby mode at the end of tda18271_attach()
      tda18271: make 'low-power standby mode after attach' multi-instance safe

 drivers/media/common/tuners/tda18271-fe.c |    4 ++++
 1 file changed, 4 insertions(+)

Best regards,

Mike
--
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 09/20/2012 08:49 PM, Michael Krufky wrote:
> On Thu, Sep 20, 2012 at 1:47 PM, Michael Krufky <mkrufky@linuxtv.org> wrote:
>> On Mon, Aug 6, 2012 at 3:13 PM, Antti Palosaari <crope@iki.fi> wrote:
>>> On 08/06/2012 09:57 PM, Michael Krufky wrote:
>>>>
>>>> On Mon, Aug 6, 2012 at 2:35 PM, Devin Heitmueller
>>>> <dheitmueller@kernellabs.com> wrote:
>>>>>
>>>>> On Mon, Aug 6, 2012 at 2:19 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>>
>>>>>> You should understand from DVB driver model:
>>>>>> * attach() called only once when driver is loaded
>>>>>> * init() called to wake-up device
>>>>>> * sleep() called to sleep device
>>>>>>
>>>>>> What I would like to say is that there is very big risk to shoot own leg
>>>>>> when doing some initialization on attach(). For example resuming from
>>>>>> the
>>>>>> suspend could cause device reset and if you rely some settings that are
>>>>>> done
>>>>>> during attach() you will likely fail as Kernel / USB-host controller has
>>>>>> reset your device.
>>>>>>
>>>>>> See reset_resume from Kernel documentation:
>>>>>> Documentation/usb/power-management.txt
>>>>>
>>>>>
>>>>> Be forewarned:  there is a very high likelihood that this patch will
>>>>> cause regressions on hybrid devices due to known race conditions
>>>>> related to dvb_frontend sleeping the tuner asynchronously on close.
>>>>> This is a hybrid tuner, and unless code is specifically added to the
>>>>> bridge or tuner driver, going from digital to analog mode too quickly
>>>>> will cause the tuner to be shutdown while it's actively in analog
>>>>> mode.
>>>>>
>>>>> (I discovered this the hard way when working on problems MythTV users
>>>>> reported against the HVR-950q).
>>>>>
>>>>> Description of race:
>>>>>
>>>>> 1.  User opens DVB frontend tunes
>>>>> 2.  User closes DVB frontend
>>>>> 3.  User *immediately* opens V4L device using same tuner
>>>>> 4.  User performs tuning request for analog
>>>>> 5.  DVB frontend issues sleep() call to tuner, causing analog tuning to
>>>>> fail.
>>>>>
>>>>> This class of problem isn't seen on DVB only devices or those that
>>>>> have dedicated digital tuners not shared for analog usage.  And in
>>>>> some cases it isn't noticed because a delay between closing the DVB
>>>>> device and opening the analog devices causes the sleep() call to
>>>>> happen before the analog tune (in which case you don't hit the race).
>>>>>
>>>>> I'm certainly not against improved power management, but it will
>>>>> require the race conditions to be fixed first in order to avoid
>>>>> regressions.
>>>>>
>>>>> Devin
>>>>>
>>>>> --
>>>>> Devin J. Heitmueller - Kernel Labs
>>>>> http://www.kernellabs.com
>>>>
>>>>
>>>> Devin's right.  I'm sorry, please *don't* merge the patch, Mauro.
>>>> Antti, you should just call sleep from your driver after attach(), as
>>>> I had previously suggested.  We can revisit this some time in the
>>>> future after we can be sure that these race conditions wont occur.
>>>
>>>
>>> OK, maybe it is safer then. I have no any hybrid hardware to test...
>>>
>>> Anyhow, I wonder how many years it will take to resolve that V4L2/DVB API
>>> hybrid usage p?oblem. I ran thinking that recently when looked how to
>>> implement DVB SDR for V4L2 API... I could guess problem will not disappear
>>> near future even analog TV disappears, because there is surely coming new
>>> nasty features which spreads over both V4L2 and DVB APIs.
>>
>> Guys,
>>
>> Please take another look at this branch and test if possible -- I
>> pushed an additional patch that takes Devin's concerns into account.
>> After applying these patches, the tda18271 driver will behave as it
>> always has, but it will sleep the tuner after attaching the first
>> instance.  If there is only one instance, then this works exactly as
>> Antti desires.  If there are more instances, then the tuner will only
>> be woken up again during attach if the tda18271_need_cal_on_startup()
>> returns non-zero.  The driver does not attempt to re-sleep the
>> hardware again during successive attach() calls.
>>
>> I have not tested this yet myself, but I believe it resolves the
>> matter -- please comment.
>>
>> Regards,
>>
>> Mike Krufky
>
> ...in case the URL got lost:
>
>
> The following changes since commit 0c7d5a6da75caecc677be1fda207b7578936770d:
>
>    Linux 3.5-rc5 (2012-07-03 22:57:41 +0300)
>
> are available in the git repository at:
>
>    git://linuxtv.org/mkrufky/tuners tda18271
>
> for you to fetch changes up to 4e46c5d1bbb920165fecfe7de18b2c01d9787230:
>
>    tda18271: make 'low-power standby mode after attach' multi-instance
> safe (2012-09-20 13:34:29 -0400)
>
> ----------------------------------------------------------------
> Michael Krufky (2):
>        tda18271: enter low-power standby mode at the end of tda18271_attach()
>        tda18271: make 'low-power standby mode after attach' multi-instance safe
>
>   drivers/media/common/tuners/tda18271-fe.c |    4 ++++
>   1 file changed, 4 insertions(+)
>
> Best regards,
>
> Mike
>
Tested-by: Antti Palosaari <crope@iki.fi>

Tested with PCTV 290e, but I cannot test multi-instance. I tried to plug 
PCTV 520e as a second stick, but it crashed as there is now that DRX-K 
firmware download problem....

regards
Antti
  

On 09/22/2012 08:21 PM, Antti Palosaari wrote:
> On 09/20/2012 08:49 PM, Michael Krufky wrote:
>> On Thu, Sep 20, 2012 at 1:47 PM, Michael Krufky <mkrufky@linuxtv.org>
>> wrote:
>>> On Mon, Aug 6, 2012 at 3:13 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>> On 08/06/2012 09:57 PM, Michael Krufky wrote:
>>>>>
>>>>> On Mon, Aug 6, 2012 at 2:35 PM, Devin Heitmueller
>>>>> <dheitmueller@kernellabs.com> wrote:
>>>>>>
>>>>>> On Mon, Aug 6, 2012 at 2:19 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>>>
>>>>>>> You should understand from DVB driver model:
>>>>>>> * attach() called only once when driver is loaded
>>>>>>> * init() called to wake-up device
>>>>>>> * sleep() called to sleep device
>>>>>>>
>>>>>>> What I would like to say is that there is very big risk to shoot
>>>>>>> own leg
>>>>>>> when doing some initialization on attach(). For example resuming
>>>>>>> from
>>>>>>> the
>>>>>>> suspend could cause device reset and if you rely some settings
>>>>>>> that are
>>>>>>> done
>>>>>>> during attach() you will likely fail as Kernel / USB-host
>>>>>>> controller has
>>>>>>> reset your device.
>>>>>>>
>>>>>>> See reset_resume from Kernel documentation:
>>>>>>> Documentation/usb/power-management.txt
>>>>>>
>>>>>>
>>>>>> Be forewarned:  there is a very high likelihood that this patch will
>>>>>> cause regressions on hybrid devices due to known race conditions
>>>>>> related to dvb_frontend sleeping the tuner asynchronously on close.
>>>>>> This is a hybrid tuner, and unless code is specifically added to the
>>>>>> bridge or tuner driver, going from digital to analog mode too quickly
>>>>>> will cause the tuner to be shutdown while it's actively in analog
>>>>>> mode.
>>>>>>
>>>>>> (I discovered this the hard way when working on problems MythTV users
>>>>>> reported against the HVR-950q).
>>>>>>
>>>>>> Description of race:
>>>>>>
>>>>>> 1.  User opens DVB frontend tunes
>>>>>> 2.  User closes DVB frontend
>>>>>> 3.  User *immediately* opens V4L device using same tuner
>>>>>> 4.  User performs tuning request for analog
>>>>>> 5.  DVB frontend issues sleep() call to tuner, causing analog
>>>>>> tuning to
>>>>>> fail.
>>>>>>
>>>>>> This class of problem isn't seen on DVB only devices or those that
>>>>>> have dedicated digital tuners not shared for analog usage.  And in
>>>>>> some cases it isn't noticed because a delay between closing the DVB
>>>>>> device and opening the analog devices causes the sleep() call to
>>>>>> happen before the analog tune (in which case you don't hit the race).
>>>>>>
>>>>>> I'm certainly not against improved power management, but it will
>>>>>> require the race conditions to be fixed first in order to avoid
>>>>>> regressions.
>>>>>>
>>>>>> Devin
>>>>>>
>>>>>> --
>>>>>> Devin J. Heitmueller - Kernel Labs
>>>>>> http://www.kernellabs.com
>>>>>
>>>>>
>>>>> Devin's right.  I'm sorry, please *don't* merge the patch, Mauro.
>>>>> Antti, you should just call sleep from your driver after attach(), as
>>>>> I had previously suggested.  We can revisit this some time in the
>>>>> future after we can be sure that these race conditions wont occur.
>>>>
>>>>
>>>> OK, maybe it is safer then. I have no any hybrid hardware to test...
>>>>
>>>> Anyhow, I wonder how many years it will take to resolve that
>>>> V4L2/DVB API
>>>> hybrid usage p?oblem. I ran thinking that recently when looked how to
>>>> implement DVB SDR for V4L2 API... I could guess problem will not
>>>> disappear
>>>> near future even analog TV disappears, because there is surely
>>>> coming new
>>>> nasty features which spreads over both V4L2 and DVB APIs.
>>>
>>> Guys,
>>>
>>> Please take another look at this branch and test if possible -- I
>>> pushed an additional patch that takes Devin's concerns into account.
>>> After applying these patches, the tda18271 driver will behave as it
>>> always has, but it will sleep the tuner after attaching the first
>>> instance.  If there is only one instance, then this works exactly as
>>> Antti desires.  If there are more instances, then the tuner will only
>>> be woken up again during attach if the tda18271_need_cal_on_startup()
>>> returns non-zero.  The driver does not attempt to re-sleep the
>>> hardware again during successive attach() calls.
>>>
>>> I have not tested this yet myself, but I believe it resolves the
>>> matter -- please comment.
>>>
>>> Regards,
>>>
>>> Mike Krufky
>>
>> ...in case the URL got lost:
>>
>>
>> The following changes since commit
>> 0c7d5a6da75caecc677be1fda207b7578936770d:
>>
>>    Linux 3.5-rc5 (2012-07-03 22:57:41 +0300)
>>
>> are available in the git repository at:
>>
>>    git://linuxtv.org/mkrufky/tuners tda18271
>>
>> for you to fetch changes up to 4e46c5d1bbb920165fecfe7de18b2c01d9787230:
>>
>>    tda18271: make 'low-power standby mode after attach' multi-instance
>> safe (2012-09-20 13:34:29 -0400)
>>
>> ----------------------------------------------------------------
>> Michael Krufky (2):
>>        tda18271: enter low-power standby mode at the end of
>> tda18271_attach()
>>        tda18271: make 'low-power standby mode after attach'
>> multi-instance safe
>>
>>   drivers/media/common/tuners/tda18271-fe.c |    4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> Best regards,
>>
>> Mike
>>
> Tested-by: Antti Palosaari <crope@iki.fi>
>
> Tested with PCTV 290e, but I cannot test multi-instance. I tried to plug
> PCTV 520e as a second stick, but it crashed as there is now that DRX-K
> firmware download problem....

Today I tested it more and it seems to work. Anyhow, I don't have any 
hybrid device to test.

regards
Antti