[1/3] si2157: change command for sleep

  Instead of sending command 13 to the tuner, send command 16 when sleeping. This 
behaviour is observed when using manufacturer provided binary-only Linux driver 
for TechnoTrend CT2-4400 (Windows driver does not do power management).

The issue with command 13 is that firmware loading is necessary after that. 
This is not an issue with tuners that do not require firmware, but resuming 
from sleep on an Si2158 takes noticeable time as firmware is loaded on resume.

Signed-off-by: Olli Salonen <olli.salonen@iki.fi>
---
 drivers/media/tuners/si2157.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
  

Moikka Olli

I ran some PM tests for that patch set, using PCTV 292e, which is em28xx 
+ Si2168 B40 + Si2157 A30. Here is results:

current impementation
-------------------------------------
cold plugin     40 mA
streaming      235 mA
sleeping        42 mA

si2157: change command for sleep
-------------------------------------
cold plugin     40 mA
streaming      235 mA
sleeping        60 mA

So it increases sleep power usage surprisingly much, almost 20mA from 
the USB, nominal 5V.

It is also funny that you will not lose firmware for Si2168 when sleep 
with command 13, but that Si2157 tuner behaves differently.

I think I will still apply that, it is just firmware download time vs. 
current use in sleep.

Antti


On 08/25/2014 09:07 PM, Olli Salonen wrote:
> Instead of sending command 13 to the tuner, send command 16 when sleeping. This
> behaviour is observed when using manufacturer provided binary-only Linux driver
> for TechnoTrend CT2-4400 (Windows driver does not do power management).
>
> The issue with command 13 is that firmware loading is necessary after that.
> This is not an issue with tuners that do not require firmware, but resuming
> from sleep on an Si2158 takes noticeable time as firmware is loaded on resume.
>
> Signed-off-by: Olli Salonen <olli.salonen@iki.fi>
> ---
>   drivers/media/tuners/si2157.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
> index efb5cce..c84f7b8 100644
> --- a/drivers/media/tuners/si2157.c
> +++ b/drivers/media/tuners/si2157.c
> @@ -197,9 +197,10 @@ static int si2157_sleep(struct dvb_frontend *fe)
>
>   	s->active = false;
>
> -	memcpy(cmd.args, "\x13", 1);
> -	cmd.wlen = 1;
> -	cmd.rlen = 0;
> +	/* standby */
> +	memcpy(cmd.args, "\x16\x00", 2);
> +	cmd.wlen = 2;
> +	cmd.rlen = 1;
>   	ret = si2157_cmd_execute(s, &cmd);
>   	if (ret)
>   		goto err;
>
  

Em Fri, 05 Sep 2014 11:34:01 +0300
Antti Palosaari <crope@iki.fi> escreveu:

> Moikka Olli
> 
> I ran some PM tests for that patch set, using PCTV 292e, which is em28xx 
> + Si2168 B40 + Si2157 A30. Here is results:
> 
> current impementation
> -------------------------------------
> cold plugin     40 mA
> streaming      235 mA
> sleeping        42 mA
> 
> si2157: change command for sleep
> -------------------------------------
> cold plugin     40 mA
> streaming      235 mA
> sleeping        60 mA
> 
> So it increases sleep power usage surprisingly much, almost 20mA from 
> the USB, nominal 5V.
> 
> It is also funny that you will not lose firmware for Si2168 when sleep 
> with command 13, but that Si2157 tuner behaves differently.
> 
> I think I will still apply that, it is just firmware download time vs. 
> current use in sleep.

IMHO, the best is to keep it saving more power. Ok, it will take more
time to wake up but so what? If someone is putting the machine to sleep,
it is because he/she wants to save power.

So, IMHO, we should keep the default behavior as-is. Nothing prevents
that we would add a modprobe parameter or to use some other method that
would allow the user to choose between those two different ways.

Regards,
Mauro

> 
> Antti
> 
> 
> On 08/25/2014 09:07 PM, Olli Salonen wrote:
> > Instead of sending command 13 to the tuner, send command 16 when sleeping. This
> > behaviour is observed when using manufacturer provided binary-only Linux driver
> > for TechnoTrend CT2-4400 (Windows driver does not do power management).
> >
> > The issue with command 13 is that firmware loading is necessary after that.
> > This is not an issue with tuners that do not require firmware, but resuming
> > from sleep on an Si2158 takes noticeable time as firmware is loaded on resume.
> >
> > Signed-off-by: Olli Salonen <olli.salonen@iki.fi>
> > ---
> >   drivers/media/tuners/si2157.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
> > index efb5cce..c84f7b8 100644
> > --- a/drivers/media/tuners/si2157.c
> > +++ b/drivers/media/tuners/si2157.c
> > @@ -197,9 +197,10 @@ static int si2157_sleep(struct dvb_frontend *fe)
> >
> >   	s->active = false;
> >
> > -	memcpy(cmd.args, "\x13", 1);
> > -	cmd.wlen = 1;
> > -	cmd.rlen = 0;
> > +	/* standby */
> > +	memcpy(cmd.args, "\x16\x00", 2);
> > +	cmd.wlen = 2;
> > +	cmd.rlen = 1;
> >   	ret = si2157_cmd_execute(s, &cmd);
> >   	if (ret)
> >   		goto err;
> >
> 
--
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 Thu, 18 Sep 2014 08:22:33 -0300
Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu:

> Em Fri, 05 Sep 2014 11:34:01 +0300
> Antti Palosaari <crope@iki.fi> escreveu:
> 
> > Moikka Olli
> > 
> > I ran some PM tests for that patch set, using PCTV 292e, which is em28xx 
> > + Si2168 B40 + Si2157 A30. Here is results:
> > 
> > current impementation
> > -------------------------------------
> > cold plugin     40 mA
> > streaming      235 mA
> > sleeping        42 mA
> > 
> > si2157: change command for sleep
> > -------------------------------------
> > cold plugin     40 mA
> > streaming      235 mA
> > sleeping        60 mA
> > 
> > So it increases sleep power usage surprisingly much, almost 20mA from 
> > the USB, nominal 5V.
> > 
> > It is also funny that you will not lose firmware for Si2168 when sleep 
> > with command 13, but that Si2157 tuner behaves differently.
> > 
> > I think I will still apply that, it is just firmware download time vs. 
> > current use in sleep.
> 
> IMHO, the best is to keep it saving more power. Ok, it will take more
> time to wake up but so what? If someone is putting the machine to sleep,
> it is because he/she wants to save power.
> 
> So, IMHO, we should keep the default behavior as-is. Nothing prevents
> that we would add a modprobe parameter or to use some other method that
> would allow the user to choose between those two different ways.

Sorry, I misunderstood this patch. 

There are actually two different things, each requiring a different PM
setting:

1) to put the tuner to sleep while it is not in usage;

2) put the machine to suspend.

This patch is for (1). That's FINE. I'll apply it.

Yet, for (2), assuming a suspend to ram, the best is to save more
power. 

In the past, the DVB core didn't make any distinction between those
two, but we recently added a hook for suspend there.

So, it makes sense to keep the tuner powerdown mode for suspend.

Regards,
Mauro.


> 
> Regards,
> Mauro
> 
> > 
> > Antti
> > 
> > 
> > On 08/25/2014 09:07 PM, Olli Salonen wrote:
> > > Instead of sending command 13 to the tuner, send command 16 when sleeping. This
> > > behaviour is observed when using manufacturer provided binary-only Linux driver
> > > for TechnoTrend CT2-4400 (Windows driver does not do power management).
> > >
> > > The issue with command 13 is that firmware loading is necessary after that.
> > > This is not an issue with tuners that do not require firmware, but resuming
> > > from sleep on an Si2158 takes noticeable time as firmware is loaded on resume.
> > >
> > > Signed-off-by: Olli Salonen <olli.salonen@iki.fi>
> > > ---
> > >   drivers/media/tuners/si2157.c | 7 ++++---
> > >   1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
> > > index efb5cce..c84f7b8 100644
> > > --- a/drivers/media/tuners/si2157.c
> > > +++ b/drivers/media/tuners/si2157.c
> > > @@ -197,9 +197,10 @@ static int si2157_sleep(struct dvb_frontend *fe)
> > >
> > >   	s->active = false;
> > >
> > > -	memcpy(cmd.args, "\x13", 1);
> > > -	cmd.wlen = 1;
> > > -	cmd.rlen = 0;
> > > +	/* standby */
> > > +	memcpy(cmd.args, "\x16\x00", 2);
> > > +	cmd.wlen = 2;
> > > +	cmd.rlen = 1;
> > >   	ret = si2157_cmd_execute(s, &cmd);
> > >   	if (ret)
> > >   		goto err;
> > >
> > 
> --
> 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
--
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/18/2014 02:50 PM, Mauro Carvalho Chehab wrote:
> Sorry, I misunderstood this patch.
>
> There are actually two different things, each requiring a different PM
> setting:
>
> 1) to put the tuner to sleep while it is not in usage;
>
> 2) put the machine to suspend.
>
> This patch is for (1). That's FINE. I'll apply it.
>
> Yet, for (2), assuming a suspend to ram, the best is to save more
> power.
>
> In the past, the DVB core didn't make any distinction between those
> two, but we recently added a hook for suspend there.
>
> So, it makes sense to keep the tuner powerdown mode for suspend.


I think old deep sleep should be used for suspend, whilst on warm state 
that new is OK.

I2C driver has suspend/resume support and it is one thing I am going to 
study in near future. Currently I have 128GB SSD and no swap at all, so 
I cannot test it now. I have already bought 256GB SSD and I am just 
waiting for Fedora 21 alpha version, which I will install with proper 
swap. Unfortunately they have delayed many times already Fedora 21 alpha...

I am not even sure if DVB frontend needs any special suspend/resume 
handling, isn't it possible to use standard kernel PM here? (I already 
added some initial PM to DVB FE ~2 years ago, but IIRC those will only 
kill threads and so which are not allowed when suspend).

So is there some reason DVB FE needs suspend/resume hooks?


regards
Antti
  

Hi Antti,

Em Thu, 18 Sep 2014 15:00:59 +0300
Antti Palosaari <crope@iki.fi> escreveu:

> On 09/18/2014 02:50 PM, Mauro Carvalho Chehab wrote:
> > Sorry, I misunderstood this patch.
> >
> > There are actually two different things, each requiring a different PM
> > setting:
> >
> > 1) to put the tuner to sleep while it is not in usage;
> >
> > 2) put the machine to suspend.
> >
> > This patch is for (1). That's FINE. I'll apply it.
> >
> > Yet, for (2), assuming a suspend to ram, the best is to save more
> > power.
> >
> > In the past, the DVB core didn't make any distinction between those
> > two, but we recently added a hook for suspend there.
> >
> > So, it makes sense to keep the tuner powerdown mode for suspend.
> 
> 
> I think old deep sleep should be used for suspend, whilst on warm state 
> that new is OK.

Agreed.

> I2C driver has suspend/resume support and it is one thing I am going to 
> study in near future.

The problem with an I2C core driven suspend/resume call is that the
suspend/resume order will be random. This will likely break support
on complex devices where resuming require a certain order to happen,
especially if the device is streaming.

> Currently I have 128GB SSD and no swap at all, so 
> I cannot test it now. I have already bought 256GB SSD and I am just 
> waiting for Fedora 21 alpha version, which I will install with proper 
> swap. Unfortunately they have delayed many times already Fedora 21 alpha...

Fedora 21 alpha will likely be too experimental for my taste ;)

The changes on Fedora next means that too much needs to be touched,
so, I'll likely wait for a few months after Fedora 21 release to
migrate most of my machines.

> I am not even sure if DVB frontend needs any special suspend/resume 
> handling, isn't it possible to use standard kernel PM here? (I already 
> added some initial PM to DVB FE ~2 years ago, but IIRC those will only 
> kill threads and so which are not allowed when suspend).

Well, dvb uses the standard Kernel PM. The way PM works on most places
is that the subsystem provides their own PM methods that are called
by the PM subsystem.

> So is there some reason DVB FE needs suspend/resume hooks?

Yes. See above.

Basically, at suspend, the tuner and demod should be stopped (including
all kthreads), and they need to be restarted and firmware may need to be
reloaded at resume time.

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