Re: tda18271 driver power consumption

[Antti Palosaari <crope@iki.fi>]

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

-- 
http://palosaari.fi/