[RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Hans Verkuil]

Second version of this patch series.

It's the same as RFCv1, except that I dropped the g_frequency and
g_tuner/s_tuner patches (patch 3, 6 and 7 in the original patch series)
because I need to think more on those, and I added a new fix for tuner_resume
which was broken as well.

Regards,

	Hans

[RFCv2 PATCH 1/5] tuner-core: rename check_mode to supported_mode

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The check_mode function checks whether a mode is supported. So calling it
supported_mode is more appropriate. In addition it returned either 0 or
-EINVAL which suggests that the -EINVAL error should be passed on. However,
that's not the case so change the return type to bool.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/tuner-core.c |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index 5748d04..083b9f1 100644
--- a/drivers/media/video/tuner-core.c
+++ b/drivers/media/video/tuner-core.c
@@ -723,22 +723,19 @@ static int tuner_remove(struct i2c_client *client)
  */
 
 /**
- * check_mode - Verify if tuner supports the requested mode
+ * supported_mode - Verify if tuner supports the requested mode
  * @t: a pointer to the module's internal struct_tuner
  *
  * This function checks if the tuner is capable of tuning analog TV,
  * digital TV or radio, depending on what the caller wants. If the
- * tuner can't support that mode, it returns -EINVAL. Otherwise, it
- * returns 0.
+ * tuner can't support that mode, it returns false. Otherwise, it
+ * returns true.
  * This function is needed for boards that have a separate tuner for
  * radio (like devices with tea5767).
  */
-static inline int check_mode(struct tuner *t, enum v4l2_tuner_type mode)
+static bool supported_mode(struct tuner *t, enum v4l2_tuner_type mode)
 {
-	if ((1 << mode & t->mode_mask) == 0)
-		return -EINVAL;
-
-	return 0;
+	return 1 << mode & t->mode_mask;
 }
 
 /**
@@ -759,7 +756,7 @@ static int set_mode_freq(struct i2c_client *client, struct tuner *t,
 	struct analog_demod_ops *analog_ops = &t->fe.ops.analog_ops;
 
 	if (mode != t->mode) {
-		if (check_mode(t, mode) == -EINVAL) {
+		if (!supported_mode(t, mode)) {
 			tuner_dbg("Tuner doesn't support mode %d. "
 				  "Putting tuner to sleep\n", mode);
 			t->standby = true;
@@ -1138,7 +1135,7 @@ static int tuner_g_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
 	struct tuner *t = to_tuner(sd);
 	struct dvb_tuner_ops *fe_tuner_ops = &t->fe.ops.tuner_ops;
 
-	if (check_mode(t, f->type) == -EINVAL)
+	if (!supported_mode(t, f->type))
 		return 0;
 	f->type = t->mode;
 	if (fe_tuner_ops->get_frequency && !t->standby) {
@@ -1161,7 +1158,7 @@ static int tuner_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 	struct analog_demod_ops *analog_ops = &t->fe.ops.analog_ops;
 	struct dvb_tuner_ops *fe_tuner_ops = &t->fe.ops.tuner_ops;
 
-	if (check_mode(t, vt->type) == -EINVAL)
+	if (!supported_mode(t, vt->type))
 		return 0;
 	vt->type = t->mode;
 	if (analog_ops->get_afc)
-- 
1.7.1

[RFCv2 PATCH 2/5] tuner-core: change return type of set_mode_freq to bool

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

set_mode_freq currently returns 0 or -EINVAL. But -EINVAL does not
indicate a error that should be passed on, it just indicates that the
tuner does not support the requested mode. So change the return type to
bool.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/tuner-core.c |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index 083b9f1..ee43e0a 100644
--- a/drivers/media/video/tuner-core.c
+++ b/drivers/media/video/tuner-core.c
@@ -746,11 +746,11 @@ static bool supported_mode(struct tuner *t, enum v4l2_tuner_type mode)
  * @freq:	frequency to set (0 means to use the previous one)
  *
  * If tuner doesn't support the needed mode (radio or TV), prints a
- * debug message and returns -EINVAL, changing its state to standby.
- * Otherwise, changes the state and sets frequency to the last value, if
- * the tuner can sleep or if it supports both Radio and TV.
+ * debug message and returns false, changing its state to standby.
+ * Otherwise, changes the state and sets frequency to the last value
+ * and returns true.
  */
-static int set_mode_freq(struct i2c_client *client, struct tuner *t,
+static bool set_mode_freq(struct i2c_client *client, struct tuner *t,
 			 enum v4l2_tuner_type mode, unsigned int freq)
 {
 	struct analog_demod_ops *analog_ops = &t->fe.ops.analog_ops;
@@ -762,7 +762,7 @@ static int set_mode_freq(struct i2c_client *client, struct tuner *t,
 			t->standby = true;
 			if (analog_ops->standby)
 				analog_ops->standby(&t->fe);
-			return -EINVAL;
+			return false;
 		}
 		t->mode = mode;
 		tuner_dbg("Changing to mode %d\n", mode);
@@ -777,7 +777,7 @@ static int set_mode_freq(struct i2c_client *client, struct tuner *t,
 		set_tv_freq(client, t->tv_freq);
 	}
 
-	return 0;
+	return true;
 }
 
 /*
@@ -1075,8 +1075,7 @@ static int tuner_s_radio(struct v4l2_subdev *sd)
 	struct tuner *t = to_tuner(sd);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	if (set_mode_freq(client, t, V4L2_TUNER_RADIO, 0) == -EINVAL)
-		return 0;
+	set_mode_freq(client, t, V4L2_TUNER_RADIO, 0);
 	return 0;
 }
 
@@ -1110,7 +1109,7 @@ static int tuner_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
 	struct tuner *t = to_tuner(sd);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	if (set_mode_freq(client, t, V4L2_TUNER_ANALOG_TV, 0) == -EINVAL)
+	if (!set_mode_freq(client, t, V4L2_TUNER_ANALOG_TV, 0))
 		return 0;
 
 	t->std = std;
@@ -1124,9 +1123,7 @@ static int tuner_s_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
 	struct tuner *t = to_tuner(sd);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	if (set_mode_freq(client, t, f->type, f->frequency) == -EINVAL)
-		return 0;
-
+	set_mode_freq(client, t, f->type, f->frequency);
 	return 0;
 }
 
@@ -1197,7 +1194,7 @@ static int tuner_s_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 	struct tuner *t = to_tuner(sd);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	if (set_mode_freq(client, t, vt->type, 0) == -EINVAL)
+	if (!set_mode_freq(client, t, vt->type, 0))
 		return 0;
 
 	if (t->mode == V4L2_TUNER_RADIO)
-- 
1.7.1

[RFCv2 PATCH 3/5] tuner-core: simplify the standard fixup.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Get rid of a number of unnecessary tuner_dbg messages by simplifying
the std fixup function.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/tuner-core.c |   92 +++++++++++---------------------------
 1 files changed, 27 insertions(+), 65 deletions(-)

diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index ee43e0a..462a8f4 100644
--- a/drivers/media/video/tuner-core.c
+++ b/drivers/media/video/tuner-core.c
@@ -832,7 +832,7 @@ static void set_tv_freq(struct i2c_client *c, unsigned int freq)
 /**
  * tuner_fixup_std - force a given video standard variant
  *
- * @t:	tuner internal struct
+ * @std:	TV standard
  *
  * A few devices or drivers have problem to detect some standard variations.
  * On other operational systems, the drivers generally have a per-country
@@ -842,57 +842,39 @@ static void set_tv_freq(struct i2c_client *c, unsigned int freq)
  * to distinguish all video standard variations, a modprobe parameter can
  * be used to force a video standard match.
  */
-static int tuner_fixup_std(struct tuner *t)
+static v4l2_std_id tuner_fixup_std(struct tuner *t, v4l2_std_id std)
 {
-	if ((t->std & V4L2_STD_PAL) == V4L2_STD_PAL) {
+	if (pal[0] != '-' && (std & V4L2_STD_PAL) == V4L2_STD_PAL) {
 		switch (pal[0]) {
 		case '6':
-			tuner_dbg("insmod fixup: PAL => PAL-60\n");
-			t->std = V4L2_STD_PAL_60;
-			break;
+			return V4L2_STD_PAL_60;
 		case 'b':
 		case 'B':
 		case 'g':
 		case 'G':
-			tuner_dbg("insmod fixup: PAL => PAL-BG\n");
-			t->std = V4L2_STD_PAL_BG;
-			break;
+			return V4L2_STD_PAL_BG;
 		case 'i':
 		case 'I':
-			tuner_dbg("insmod fixup: PAL => PAL-I\n");
-			t->std = V4L2_STD_PAL_I;
-			break;
+			return V4L2_STD_PAL_I;
 		case 'd':
 		case 'D':
 		case 'k':
 		case 'K':
-			tuner_dbg("insmod fixup: PAL => PAL-DK\n");
-			t->std = V4L2_STD_PAL_DK;
-			break;
+			return V4L2_STD_PAL_DK;
 		case 'M':
 		case 'm':
-			tuner_dbg("insmod fixup: PAL => PAL-M\n");
-			t->std = V4L2_STD_PAL_M;
-			break;
+			return V4L2_STD_PAL_M;
 		case 'N':
 		case 'n':
-			if (pal[1] == 'c' || pal[1] == 'C') {
-				tuner_dbg("insmod fixup: PAL => PAL-Nc\n");
-				t->std = V4L2_STD_PAL_Nc;
-			} else {
-				tuner_dbg("insmod fixup: PAL => PAL-N\n");
-				t->std = V4L2_STD_PAL_N;
-			}
-			break;
-		case '-':
-			/* default parameter, do nothing */
-			break;
+			if (pal[1] == 'c' || pal[1] == 'C')
+				return V4L2_STD_PAL_Nc;
+			return V4L2_STD_PAL_N;
 		default:
 			tuner_warn("pal= argument not recognised\n");
 			break;
 		}
 	}
-	if ((t->std & V4L2_STD_SECAM) == V4L2_STD_SECAM) {
+	if (secam[0] != '-' && (std & V4L2_STD_SECAM) == V4L2_STD_SECAM) {
 		switch (secam[0]) {
 		case 'b':
 		case 'B':
@@ -900,63 +882,42 @@ static int tuner_fixup_std(struct tuner *t)
 		case 'G':
 		case 'h':
 		case 'H':
-			tuner_dbg("insmod fixup: SECAM => SECAM-BGH\n");
-			t->std = V4L2_STD_SECAM_B |
-				 V4L2_STD_SECAM_G |
-				 V4L2_STD_SECAM_H;
-			break;
+			return V4L2_STD_SECAM_B |
+			       V4L2_STD_SECAM_G |
+			       V4L2_STD_SECAM_H;
 		case 'd':
 		case 'D':
 		case 'k':
 		case 'K':
-			tuner_dbg("insmod fixup: SECAM => SECAM-DK\n");
-			t->std = V4L2_STD_SECAM_DK;
-			break;
+			return V4L2_STD_SECAM_DK;
 		case 'l':
 		case 'L':
-			if ((secam[1] == 'C') || (secam[1] == 'c')) {
-				tuner_dbg("insmod fixup: SECAM => SECAM-L'\n");
-				t->std = V4L2_STD_SECAM_LC;
-			} else {
-				tuner_dbg("insmod fixup: SECAM => SECAM-L\n");
-				t->std = V4L2_STD_SECAM_L;
-			}
-			break;
-		case '-':
-			/* default parameter, do nothing */
-			break;
+			if ((secam[1] == 'C') || (secam[1] == 'c'))
+				return V4L2_STD_SECAM_LC;
+			return V4L2_STD_SECAM_L;
 		default:
 			tuner_warn("secam= argument not recognised\n");
 			break;
 		}
 	}
 
-	if ((t->std & V4L2_STD_NTSC) == V4L2_STD_NTSC) {
+	if (ntsc[0] != '-' && (std & V4L2_STD_NTSC) == V4L2_STD_NTSC) {
 		switch (ntsc[0]) {
 		case 'm':
 		case 'M':
-			tuner_dbg("insmod fixup: NTSC => NTSC-M\n");
-			t->std = V4L2_STD_NTSC_M;
-			break;
+			return V4L2_STD_NTSC_M;
 		case 'j':
 		case 'J':
-			tuner_dbg("insmod fixup: NTSC => NTSC_M_JP\n");
-			t->std = V4L2_STD_NTSC_M_JP;
-			break;
+			return V4L2_STD_NTSC_M_JP;
 		case 'k':
 		case 'K':
-			tuner_dbg("insmod fixup: NTSC => NTSC_M_KR\n");
-			t->std = V4L2_STD_NTSC_M_KR;
-			break;
-		case '-':
-			/* default parameter, do nothing */
-			break;
+			return V4L2_STD_NTSC_M_KR;
 		default:
 			tuner_info("ntsc= argument not recognised\n");
 			break;
 		}
 	}
-	return 0;
+	return std;
 }
 
 /*
@@ -1112,8 +1073,9 @@ static int tuner_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
 	if (!set_mode_freq(client, t, V4L2_TUNER_ANALOG_TV, 0))
 		return 0;
 
-	t->std = std;
-	tuner_fixup_std(t);
+	t->std = tuner_fixup_std(t, std);
+	if (t->std != std)
+		tuner_dbg("Fixup standard %llx to %llx\n", std, t->std);
 
 	return 0;
 }
-- 
1.7.1

[RFCv2 PATCH 4/5] tuner-core: fix s_std and s_tuner.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Both s_std and s_tuner are broken because set_mode_freq is called before the
new std (for s_std) and audmode (for s_tuner) are set.

This patch splits set_mode_freq in a set_mode and a set_freq and in s_std
first calls set_mode, and if that returns true (i.e. the mode is supported)
then they set t->std/t->audmode and call set_freq.

This fixes a bug where changing std or audmode would actually change it to
the previous value.

Discovered while testing analog TV standards for cx18 with a tda18271 tuner.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/tuner-core.c |   57 ++++++++++++++++++++-----------------
 1 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index 462a8f4..e5ec145 100644
--- a/drivers/media/video/tuner-core.c
+++ b/drivers/media/video/tuner-core.c
@@ -739,19 +739,15 @@ static bool supported_mode(struct tuner *t, enum v4l2_tuner_type mode)
 }
 
 /**
- * set_mode_freq - Switch tuner to other mode.
- * @client:	struct i2c_client pointer
+ * set_mode - Switch tuner to other mode.
  * @t:		a pointer to the module's internal struct_tuner
  * @mode:	enum v4l2_type (radio or TV)
- * @freq:	frequency to set (0 means to use the previous one)
  *
  * If tuner doesn't support the needed mode (radio or TV), prints a
  * debug message and returns false, changing its state to standby.
- * Otherwise, changes the state and sets frequency to the last value
- * and returns true.
+ * Otherwise, changes the state and returns true.
  */
-static bool set_mode_freq(struct i2c_client *client, struct tuner *t,
-			 enum v4l2_tuner_type mode, unsigned int freq)
+static bool set_mode(struct tuner *t, enum v4l2_tuner_type mode)
 {
 	struct analog_demod_ops *analog_ops = &t->fe.ops.analog_ops;
 
@@ -767,17 +763,27 @@ static bool set_mode_freq(struct i2c_client *client, struct tuner *t,
 		t->mode = mode;
 		tuner_dbg("Changing to mode %d\n", mode);
 	}
+	return true;
+}
+
+/**
+ * set_freq - Set the tuner to the desired frequency.
+ * @t:		a pointer to the module's internal struct_tuner
+ * @freq:	frequency to set (0 means to use the current frequency)
+ */
+static void set_freq(struct tuner *t, unsigned int freq)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&t->sd);
+
 	if (t->mode == V4L2_TUNER_RADIO) {
-		if (freq)
-			t->radio_freq = freq;
-		set_radio_freq(client, t->radio_freq);
+		if (!freq)
+			freq = t->radio_freq;
+		set_radio_freq(client, freq);
 	} else {
-		if (freq)
-			t->tv_freq = freq;
-		set_tv_freq(client, t->tv_freq);
+		if (!freq)
+			freq = t->tv_freq;
+		set_tv_freq(client, freq);
 	}
-
-	return true;
 }
 
 /*
@@ -1034,9 +1040,9 @@ static void tuner_status(struct dvb_frontend *fe)
 static int tuner_s_radio(struct v4l2_subdev *sd)
 {
 	struct tuner *t = to_tuner(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	set_mode_freq(client, t, V4L2_TUNER_RADIO, 0);
+	set_mode(t, V4L2_TUNER_RADIO);
+	set_freq(t, 0);
 	return 0;
 }
 
@@ -1068,24 +1074,23 @@ static int tuner_s_power(struct v4l2_subdev *sd, int on)
 static int tuner_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
 {
 	struct tuner *t = to_tuner(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	if (!set_mode_freq(client, t, V4L2_TUNER_ANALOG_TV, 0))
+	if (!set_mode(t, V4L2_TUNER_ANALOG_TV))
 		return 0;
 
 	t->std = tuner_fixup_std(t, std);
 	if (t->std != std)
 		tuner_dbg("Fixup standard %llx to %llx\n", std, t->std);
-
+	set_freq(t, 0);
 	return 0;
 }
 
 static int tuner_s_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
 {
 	struct tuner *t = to_tuner(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	set_mode_freq(client, t, f->type, f->frequency);
+	if (set_mode(t, f->type))
+		set_freq(t, f->frequency);
 	return 0;
 }
 
@@ -1154,13 +1159,13 @@ static int tuner_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 static int tuner_s_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 {
 	struct tuner *t = to_tuner(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	if (!set_mode_freq(client, t, vt->type, 0))
+	if (!set_mode(t, vt->type))
 		return 0;
 
 	if (t->mode == V4L2_TUNER_RADIO)
 		t->audmode = vt->audmode;
+	set_freq(t, 0);
 
 	return 0;
 }
@@ -1195,8 +1200,8 @@ static int tuner_resume(struct i2c_client *c)
 	tuner_dbg("resume\n");
 
 	if (!t->standby)
-		set_mode_freq(c, t, t->type, 0);
-
+		if (set_mode(t, t->type))
+			set_freq(t, 0);
 	return 0;
 }
 
-- 
1.7.1

[RFCv2 PATCH 5/5] tuner-core: fix tuner_resume: use t->mode instead of t->type.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

set_mode is called with t->type, which is the tuner type. Instead, use
t->mode which is the actual tuner mode (i.e. radio vs tv).

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/tuner-core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index e5ec145..17f6c00 100644
--- a/drivers/media/video/tuner-core.c
+++ b/drivers/media/video/tuner-core.c
@@ -1200,7 +1200,7 @@ static int tuner_resume(struct i2c_client *c)
 	tuner_dbg("resume\n");
 
 	if (!t->standby)
-		if (set_mode(t, t->type))
+		if (set_mode(t, t->mode))
 			set_freq(t, 0);
 	return 0;
 }
-- 
1.7.1

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Devin Heitmueller]

On Sat, Jun 11, 2011 at 11:05 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Second version of this patch series.
>
> It's the same as RFCv1, except that I dropped the g_frequency and
> g_tuner/s_tuner patches (patch 3, 6 and 7 in the original patch series)
> because I need to think more on those, and I added a new fix for tuner_resume
> which was broken as well.

Hi Hans,

I appreciate your taking the time to refactor this code (no doubt it
really needed it).  All that I ask is that you please actually *try*
the resulting patches with VLC and a tuner that supports standby in
order to ensure that it didn't cause any regressions.  This stuff was
brittle to begin with, and there are lots of opportunities for
obscure/unexpected effects resulting from what appear to be sane
changes.

The last series of patches that went in were in response to this stuff
being very broken, and I would hate to see a regression in existing
applications after we finally got it working.

Thanks,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Hans Verkuil]

On Saturday, June 11, 2011 17:32:10 Devin Heitmueller wrote:
> On Sat, Jun 11, 2011 at 11:05 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > Second version of this patch series.
> >
> > It's the same as RFCv1, except that I dropped the g_frequency and
> > g_tuner/s_tuner patches (patch 3, 6 and 7 in the original patch series)
> > because I need to think more on those, and I added a new fix for tuner_resume
> > which was broken as well.
> 
> Hi Hans,
> 
> I appreciate your taking the time to refactor this code (no doubt it
> really needed it).  All that I ask is that you please actually *try*
> the resulting patches with VLC and a tuner that supports standby in
> order to ensure that it didn't cause any regressions.

That's easier said than done. I don't think I have tuners of that type.

Do you happen to know not-too-expensive cards that you can buy that have
this sort of tuners? It may be useful to be able to test this myself.

> This stuff was
> brittle to begin with, and there are lots of opportunities for
> obscure/unexpected effects resulting from what appear to be sane
> changes.
> 
> The last series of patches that went in were in response to this stuff
> being very broken, and I would hate to see a regression in existing
> applications after we finally got it working.

Yeah, it seems that whenever you touch this tuner code something breaks
for at least one card. There is so much legacy here...

Regards,

	Hans

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Andy Walls]

Devin Heitmueller <dheitmueller@kernellabs.com> wrote:

>On Sat, Jun 11, 2011 at 11:05 AM, Hans Verkuil <hverkuil@xs4all.nl>
>wrote:
>> Second version of this patch series.
>>
>> It's the same as RFCv1, except that I dropped the g_frequency and
>> g_tuner/s_tuner patches (patch 3, 6 and 7 in the original patch
>series)
>> because I need to think more on those, and I added a new fix for
>tuner_resume
>> which was broken as well.
>
>Hi Hans,
>
>I appreciate your taking the time to refactor this code (no doubt it
>really needed it).  All that I ask is that you please actually *try*
>the resulting patches with VLC and a tuner that supports standby in
>order to ensure that it didn't cause any regressions.  This stuff was
>brittle to begin with, and there are lots of opportunities for
>obscure/unexpected effects resulting from what appear to be sane
>changes.
>
>The last series of patches that went in were in response to this stuff
>being very broken, and I would hate to see a regression in existing
>applications after we finally got it working.
>
>Thanks,
>
>Devin
>
>-- 
>Devin J. Heitmueller - Kernel Labs
>http://www.kernellabs.com
>--
>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

Its not a refactor; drivers are broken.  Hans noticed it when testing the newer hvr1600s analog tuner with a standard other than NTSC.

I appreciate that we don't want fixes now to break things, but the past changes did indeed break things.

Regards,
Andy

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Andy Walls]

Hans Verkuil <hverkuil@xs4all.nl> wrote:

>On Saturday, June 11, 2011 17:32:10 Devin Heitmueller wrote:
>> On Sat, Jun 11, 2011 at 11:05 AM, Hans Verkuil <hverkuil@xs4all.nl>
>wrote:
>> > Second version of this patch series.
>> >
>> > It's the same as RFCv1, except that I dropped the g_frequency and
>> > g_tuner/s_tuner patches (patch 3, 6 and 7 in the original patch
>series)
>> > because I need to think more on those, and I added a new fix for
>tuner_resume
>> > which was broken as well.
>> 
>> Hi Hans,
>> 
>> I appreciate your taking the time to refactor this code (no doubt it
>> really needed it).  All that I ask is that you please actually *try*
>> the resulting patches with VLC and a tuner that supports standby in
>> order to ensure that it didn't cause any regressions.
>
>That's easier said than done. I don't think I have tuners of that type.
>
>Do you happen to know not-too-expensive cards that you can buy that
>have
>this sort of tuners? It may be useful to be able to test this myself.
>
>> This stuff was
>> brittle to begin with, and there are lots of opportunities for
>> obscure/unexpected effects resulting from what appear to be sane
>> changes.
>> 
>> The last series of patches that went in were in response to this
>stuff
>> being very broken, and I would hate to see a regression in existing
>> applications after we finally got it working.
>
>Yeah, it seems that whenever you touch this tuner code something breaks
>for at least one card. There is so much legacy here...
>
>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

Devin,

I think I have a Gotview or compro card with an xc2028.  Is that tuner capable of standby?  Would the cx18 or ivtv driver need to actively support using stand by?

-Andy

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Devin Heitmueller]

On Sat, Jun 11, 2011 at 12:06 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> Devin,
>
> I think I have a Gotview or compro card with an xc2028.  Is that tuner capable of standby?  Would the cx18 or ivtv driver need to actively support using stand by?

An xc2028/xc3028 should be fine, as that does support standby.  The
problems we saw with VLC were related to calls like G_TUNER returning
prematurely if the device was in standby, leaving the returned
structure populated with garbage.

FWIW, I don't dispute your assertion that Hans found legitimate bugs -
just that we need to be careful to not cause regressions in the cases
that the last round of bug fixes addressed.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Devin Heitmueller]

On Sat, Jun 11, 2011 at 11:53 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Do you happen to know not-too-expensive cards that you can buy that have
> this sort of tuners? It may be useful to be able to test this myself.

Anything with an xc3028 or xc5000 would have this issue.  I don't
really keep close track of current shipping DVB products, but the 3028
was definitely very chip in products from a couple of years ago.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Hans Verkuil]

On Saturday, June 11, 2011 18:57:16 Devin Heitmueller wrote:
> On Sat, Jun 11, 2011 at 12:06 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> > Devin,
> >
> > I think I have a Gotview or compro card with an xc2028.  Is that tuner capable of standby?  Would the cx18 or ivtv driver need to actively support using stand by?
> 
> An xc2028/xc3028 should be fine, as that does support standby.  The
> problems we saw with VLC were related to calls like G_TUNER returning
> prematurely if the device was in standby, leaving the returned
> structure populated with garbage.

OK, but how do you get it into standby in the first place? (I must be missing
something here...)

Regards,

	Hans

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Devin Heitmueller]

On Sat, Jun 11, 2011 at 1:02 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> OK, but how do you get it into standby in the first place? (I must be missing
> something here...)

The tuner core puts the chip into standby when the last V4L filehandle
is closed.  Yes, I realize this violates the V4L spec since you should
be able to make multiple calls with something like v4l2-ctl, but
nobody has ever come up with a better mechanism for knowing when to
put the device to sleep.

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.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [RFCv2 PATCH 4/5] tuner-core: fix s_std and s_tuner.

[Hans Verkuil]

On Saturday, June 11, 2011 17:05:30 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Both s_std and s_tuner are broken because set_mode_freq is called before the
> new std (for s_std) and audmode (for s_tuner) are set.
> 
> This patch splits set_mode_freq in a set_mode and a set_freq and in s_std
> first calls set_mode, and if that returns true (i.e. the mode is supported)
> then they set t->std/t->audmode and call set_freq.
> 
> This fixes a bug where changing std or audmode would actually change it to
> the previous value.
> 
> Discovered while testing analog TV standards for cx18 with a tda18271 tuner.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/video/tuner-core.c |   57 ++++++++++++++++++++-----------------
>  1 files changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
> index 462a8f4..e5ec145 100644
> --- a/drivers/media/video/tuner-core.c
> +++ b/drivers/media/video/tuner-core.c
> @@ -739,19 +739,15 @@ static bool supported_mode(struct tuner *t, enum v4l2_tuner_type mode)
>  }
>  
>  /**
> - * set_mode_freq - Switch tuner to other mode.
> - * @client:	struct i2c_client pointer
> + * set_mode - Switch tuner to other mode.
>   * @t:		a pointer to the module's internal struct_tuner
>   * @mode:	enum v4l2_type (radio or TV)
> - * @freq:	frequency to set (0 means to use the previous one)
>   *
>   * If tuner doesn't support the needed mode (radio or TV), prints a
>   * debug message and returns false, changing its state to standby.
> - * Otherwise, changes the state and sets frequency to the last value
> - * and returns true.
> + * Otherwise, changes the state and returns true.
>   */
> -static bool set_mode_freq(struct i2c_client *client, struct tuner *t,
> -			 enum v4l2_tuner_type mode, unsigned int freq)
> +static bool set_mode(struct tuner *t, enum v4l2_tuner_type mode)
>  {
>  	struct analog_demod_ops *analog_ops = &t->fe.ops.analog_ops;
>  
> @@ -767,17 +763,27 @@ static bool set_mode_freq(struct i2c_client *client, struct tuner *t,
>  		t->mode = mode;
>  		tuner_dbg("Changing to mode %d\n", mode);
>  	}
> +	return true;
> +}
> +
> +/**
> + * set_freq - Set the tuner to the desired frequency.
> + * @t:		a pointer to the module's internal struct_tuner
> + * @freq:	frequency to set (0 means to use the current frequency)
> + */
> +static void set_freq(struct tuner *t, unsigned int freq)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&t->sd);
> +
>  	if (t->mode == V4L2_TUNER_RADIO) {
> -		if (freq)
> -			t->radio_freq = freq;
> -		set_radio_freq(client, t->radio_freq);
> +		if (!freq)
> +			freq = t->radio_freq;
> +		set_radio_freq(client, freq);
>  	} else {
> -		if (freq)
> -			t->tv_freq = freq;
> -		set_tv_freq(client, t->tv_freq);
> +		if (!freq)
> +			freq = t->tv_freq;
> +		set_tv_freq(client, freq);
>  	}
> -
> -	return true;
>  }
>  
>  /*
> @@ -1034,9 +1040,9 @@ static void tuner_status(struct dvb_frontend *fe)
>  static int tuner_s_radio(struct v4l2_subdev *sd)
>  {
>  	struct tuner *t = to_tuner(sd);
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  
> -	set_mode_freq(client, t, V4L2_TUNER_RADIO, 0);
> +	set_mode(t, V4L2_TUNER_RADIO);
> +	set_freq(t, 0);
>  	return 0;
>  }
>  
> @@ -1068,24 +1074,23 @@ static int tuner_s_power(struct v4l2_subdev *sd, int on)
>  static int tuner_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
>  {
>  	struct tuner *t = to_tuner(sd);
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  
> -	if (!set_mode_freq(client, t, V4L2_TUNER_ANALOG_TV, 0))
> +	if (!set_mode(t, V4L2_TUNER_ANALOG_TV))
>  		return 0;
>  
>  	t->std = tuner_fixup_std(t, std);
>  	if (t->std != std)
>  		tuner_dbg("Fixup standard %llx to %llx\n", std, t->std);
> -
> +	set_freq(t, 0);
>  	return 0;
>  }
>  
>  static int tuner_s_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
>  {
>  	struct tuner *t = to_tuner(sd);
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  
> -	set_mode_freq(client, t, f->type, f->frequency);
> +	if (set_mode(t, f->type))
> +		set_freq(t, f->frequency);
>  	return 0;
>  }
>  
> @@ -1154,13 +1159,13 @@ static int tuner_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
>  static int tuner_s_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
>  {
>  	struct tuner *t = to_tuner(sd);
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  
> -	if (!set_mode_freq(client, t, vt->type, 0))
> +	if (!set_mode(t, vt->type))
>  		return 0;
>  
>  	if (t->mode == V4L2_TUNER_RADIO)
>  		t->audmode = vt->audmode;
> +	set_freq(t, 0);

This 'if' is missing {}. I'll fix that.

Regards,

	Hans

>  
>  	return 0;
>  }
> @@ -1195,8 +1200,8 @@ static int tuner_resume(struct i2c_client *c)
>  	tuner_dbg("resume\n");
>  
>  	if (!t->standby)
> -		set_mode_freq(c, t, t->type, 0);
> -
> +		if (set_mode(t, t->type))
> +			set_freq(t, 0);
>  	return 0;
>  }
>  
> 

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Hans Verkuil]

On Saturday, June 11, 2011 19:08:04 Devin Heitmueller wrote:
> On Sat, Jun 11, 2011 at 1:02 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > OK, but how do you get it into standby in the first place? (I must be missing
> > something here...)
> 
> The tuner core puts the chip into standby when the last V4L filehandle
> is closed.  Yes, I realize this violates the V4L spec since you should
> be able to make multiple calls with something like v4l2-ctl, but
> nobody has ever come up with a better mechanism for knowing when to
> put the device to sleep.

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.

> 
> 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.

Devin, Mauro, is there anything wrong with adding support for s_power(1) and
calling it in em28xx? A similar power up would be needed in cx23885, au0828,
cx88 and cx231xx.

Mauro, if you agree with this patch, then I'll add it to the tuner patch series.

Tested with em28xx.

Regards,

	Hans

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;

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Devin Heitmueller]

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

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Hans Verkuil]

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

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Devin Heitmueller]

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

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Hans Verkuil]

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

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Mauro Carvalho Chehab]

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

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Devin Heitmueller]

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

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Marko Ristola]

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
> 

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Devin Heitmueller]

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

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Marko Ristola]

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

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Mauro Carvalho Chehab]

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

Re: [RFCv2 PATCH 0/5] tuner-core: fix s_std and s_tuner

[Marko Ristola]

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