tda18271 driver power consumption
Message ID | CAOcJUbwCkNZCbNv=JHKhSMiuBre+cqWqhF5ihocRP9jbo1iEkw@mail.gmail.com (mailing list archive) |
---|---|
State | RFC, archived |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1TEkst-0003SO-Uq for patchwork@linuxtv.org; Thu, 20 Sep 2012 19:49:55 +0200 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.75/mailfrontend-4) with esmtp for <patchwork@linuxtv.org> id 1TEkst-0003co-9g; Thu, 20 Sep 2012 19:49:55 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753337Ab2ITRtw (ORCPT <rfc822;patchwork@linuxtv.org>); Thu, 20 Sep 2012 13:49:52 -0400 Received: from mail-oa0-f46.google.com ([209.85.219.46]:34774 "EHLO mail-oa0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753200Ab2ITRtw convert rfc822-to-8bit (ORCPT <rfc822;linux-media@vger.kernel.org>); Thu, 20 Sep 2012 13:49:52 -0400 Received: by oago6 with SMTP id o6so2459095oag.19 for <linux-media@vger.kernel.org>; Thu, 20 Sep 2012 10:49:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=p4TbQd11zRYAOQn0hBQKBOZQEVndtkpMbYxJcS9xsnM=; b=uYCsqiB8TNeDOY7ACT5dg18H3B40/I/6lCq+9CsyczlHlWkjSbVuDe5BQDxdlTYJzQ YEOWpmg0v5ZMJSDb8AogEdw9Zm/GGMn6Br1UQQ3uVt7HOj35CCtoisozgmqbkoiJrT/H Y/JIrYlhHo6PnLiSZKcyMnfmTyPmZDu9ECKzKP28ZKxZK9kdDgJ4DHMaEKnyvEFeHPzp 2C9zkFLDi5u9okTBBve82BQuttjNMSP5SthZOzZSl4Z0VgYFG0K+7gkZCeNNykQv1EIh yU6Eta0WYkHmBeTmrdIeZ5LaMr0Bk8+a1laZL4Bxl5E7lYyDBFTms2d1veKOI1/FCnNY p/FQ== MIME-Version: 1.0 Received: by 10.60.170.76 with SMTP id ak12mr271241oec.22.1348163391542; Thu, 20 Sep 2012 10:49:51 -0700 (PDT) Received: by 10.60.35.72 with HTTP; Thu, 20 Sep 2012 10:49:51 -0700 (PDT) In-Reply-To: <CAOcJUbw5+wfaZ6Os5gKbPzCf0_d_rh=apatQwA=0k5gKm_FfOQ@mail.gmail.com> References: <500C5B9B.8000303@iki.fi> <CAOcJUbw-8zG-j7YobgKy7k5vp-k_trkaB5fYGz605KdUQHKTGQ@mail.gmail.com> <500F1DC5.1000608@iki.fi> <CAOcJUbzXoLx10o8oprxPM1TELFxyGE7_wodcWsBr8MX4OR0N_w@mail.gmail.com> <50200AC9.9080203@iki.fi> <CAGoCfixmre37ph46E6U9mJ+tyt+OL7+RbDwg+W6DkL01im2nCg@mail.gmail.com> <CAOcJUbwyNBEoPyE_6QZQ-6tbUsHFzurYBEavegQO1aVYNsQ_kA@mail.gmail.com> <5020175D.3070601@iki.fi> <CAOcJUbw5+wfaZ6Os5gKbPzCf0_d_rh=apatQwA=0k5gKm_FfOQ@mail.gmail.com> Date: Thu, 20 Sep 2012 13:49:51 -0400 X-Google-Sender-Auth: Dokck-IRZkj8xuSpH-RyCGTuUSY Message-ID: <CAOcJUbwCkNZCbNv=JHKhSMiuBre+cqWqhF5ihocRP9jbo1iEkw@mail.gmail.com> Subject: Re: tda18271 driver power consumption From: Michael Krufky <mkrufky@linuxtv.org> To: Antti Palosaari <crope@iki.fi> Cc: Devin Heitmueller <dheitmueller@kernellabs.com>, linux-media <linux-media@vger.kernel.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 5.6.1.2065439, Antispam-Engine: 2.7.2.376379, Antispam-Data: 2012.9.20.173615 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODY_SIZE_4000_4999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, CT_TEXT_PLAIN_UTF8_CAPS 0, DATE_TZ_NA 0, DKIM_SIGNATURE 0, URI_ENDS_IN_HTML 0, WEBMAIL_SOURCE 0, __ANY_URI 0, __BOUNCE_CHALLENGE_SUBJ 0, __BOUNCE_NDR_SUBJ_EXEMPT 0, __C230066_P3_4 0, __CP_URI_IN_BODY 0, __CT 0, __CTE 0, __CT_TEXT_PLAIN 0, __FRAUD_MISC 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILING_LIST 0, __INT_PROD_TV 0, __MIME_TEXT_ONLY 0, __MIME_VERSION 0, __MULTIPLE_RCPTS_CC_X2 0, __PHISH_SPEAR_HTTP_RECEIVED 0, __PHISH_SPEAR_STRUCTURE_1 0, __PHISH_SPEAR_STRUCTURE_2 0, __SANE_MSGID 0, __TO_MALFORMED_2 0, __URI_NS ' |
Pull-request
git://linuxtv.org/mkrufky/tuners tda18271Message
Michael Ira Krufky
Sept. 20, 2012, 5:49 p.m. UTC
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
Comments
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