Em 23-01-2011 20:22, Devin Heitmueller escreveu:
> Hello all,
>
> The following patch addresses a V4L2 specification violation where the
> G_TUNER call would return complete garbage in the event that the tuner
> is asleep. Per Hans' suggestion, I have split out the tuner
> operational mode from whether it is in standby, and fixed the G_TUNER
> call to return as much as possible when the tuner is in standby.
>
> Without this change, products that have tuners which support standby
> mode cannot be tuned from within VLC.
>
> I recognize that changes to tuner-core tend to be pretty hairy, so I
> welcome suggestions/feedback on this patch.
>
> Regards,
>
> Devin
>
> -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com
>
>
> tuner_standby_mode.patch
>
>
> tuner-core: fix broken G_TUNER call when tuner is in standby
>
> From: Devin Heitmueller <dheitmuel...@kernellabs.com>
>
> The logic for determining the supported device modes was combined with the
> logic which dictates whether the tuner was asleep. This resulted in calls
> such as G_TUNER returning complete garbage in the event that the tuner was
> in standby mode (a violation of the V4L2 specification, and causing VLC to
> be broken for such tuners).
>
> This patch reworks the logic so the current tuner mode is maintained
> separately
> from whether it is in standby (per Hans Verkuil's suggestion). It also
> restructures the G_TUNER call such that all the staticly defined information
> related to the tuner is returned regardless of whether it is in standby (e.g.
> the supported frequency range, etc).
>
> Priority: normal
>
> Signed-off-by: Devin Heitmueller <dheitmuel...@kernellabs.com>
> Cc: Hans Verkuil <hverk...@xs4all.nl>
>
> --- media_build/linux/drivers/media/video/tuner-core.c 2010-10-24
> 19:34:59.000000000 -0400
> +++ media_build_950qfixes//linux/drivers/media/video/tuner-core.c
> 2011-01-23 17:18:22.381107568 -0500
> @@ -90,6 +90,7 @@
>
> unsigned int mode;
> unsigned int mode_mask; /* Combination of allowable modes */
> + unsigned int in_standby:1;
>
> unsigned int type; /* chip type id */
> unsigned int config;
> @@ -700,6 +701,7 @@
> static inline int set_mode(struct i2c_client *client, struct tuner *t, int
> mode, char *cmd)
> {
> struct analog_demod_ops *analog_ops = &t->fe.ops.analog_ops;
> + unsigned int orig_mode = t->mode;
>
> if (mode == t->mode)
> return 0;
> @@ -709,7 +711,8 @@
> if (check_mode(t, cmd) == -EINVAL) {
> tuner_dbg("Tuner doesn't support this mode. "
> "Putting tuner to sleep\n");
> - t->mode = T_STANDBY;
> + t->mode = orig_mode;
Hmm... as orig_mode = t->mode, this is:
t->mode = t->mode...
This doesn't make sense ;)
> + t->in_standby = 1;
> if (analog_ops->standby)
> analog_ops->standby(&t->fe);
> return -EINVAL;
> @@ -769,7 +772,7 @@
>
> if (check_mode(t, "s_power") == -EINVAL)
> return 0;
> - t->mode = T_STANDBY;
> + t->in_standby = 1;
> if (analog_ops->standby)
> analog_ops->standby(&t->fe);
> return 0;
> @@ -854,47 +857,54 @@
> 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, "g_tuner") == -EINVAL)
> - return 0;
Some of those checks are to allow switching between radio and TV
mode. Had you test to switch between video/radio mode on your tests
(e. g. alternating video streaming on/off with radio tune)?
> switch_v4l2();
>
> + /* First populate everything that doesn't require talking to the
> + actual hardware */
> vt->type = t->mode;
> - if (analog_ops->get_afc)
> - vt->afc = analog_ops->get_afc(&t->fe);
> if (t->mode == V4L2_TUNER_ANALOG_TV)
> + {
> vt->capability |= V4L2_TUNER_CAP_NORM;
> - if (t->mode != V4L2_TUNER_RADIO) {
> vt->rangelow = tv_range[0] * 16;
> vt->rangehigh = tv_range[1] * 16;
> - return 0;
> + } else {
> + /* radio mode */
> + vt->rxsubchans = V4L2_TUNER_SUB_MONO | V4L2_TUNER_SUB_STEREO;
> + vt->capability |= V4L2_TUNER_CAP_LOW | V4L2_TUNER_CAP_STEREO;
> + vt->audmode = t->audmode;
> + vt->rangelow = radio_range[0] * 16000;
> + vt->rangehigh = radio_range[1] * 16000;
> }
>
> - /* radio mode */
> - vt->rxsubchans =
> - V4L2_TUNER_SUB_MONO | V4L2_TUNER_SUB_STEREO;
> - if (fe_tuner_ops->get_status) {
> - u32 tuner_status;
> + /* If the hardware is in sleep mode, bail out at this point */
> + if (t->in_standby)
> + return 0;
This doesn't seem right. Instead, the proper behaviour should be
to turn tuner on (likely waiting for some time) and then get a status.
>
> - fe_tuner_ops->get_status(&t->fe, &tuner_status);
> - vt->rxsubchans =
> - (tuner_status & TUNER_STATUS_STEREO) ?
> - V4L2_TUNER_SUB_STEREO :
> - V4L2_TUNER_SUB_MONO;
> + /* Now populate the fields that requires the hardware to be alive */
> + if (t->mode == V4L2_TUNER_ANALOG_TV) {
> + if (analog_ops->get_afc)
> + vt->afc = analog_ops->get_afc(&t->fe);
> } else {
> - if (analog_ops->is_stereo) {
> + if (fe_tuner_ops->get_status) {
Hmm... get_status() taking procedence over is_stereo... not sure if this
work as expected. On the other hand:
$ git grep is_stereo drivers/media/
drivers/media/dvb/dvb-core/dvb_frontend.h: int (*is_stereo)(struct
dvb_frontend *fe);
drivers/media/radio/radio-zoltrix.c:static int zol_is_stereo(struct zoltrix
*zol)
drivers/media/radio/radio-zoltrix.c: if (zol_is_stereo(zol))
drivers/media/video/msp3400-kthreads.c: int is_stereo = status & 0x40;
drivers/media/video/msp3400-kthreads.c: if (is_stereo)
drivers/media/video/msp3400-kthreads.c: status, is_stereo,
is_bilingual, state->rxsubchans);
drivers/media/video/tuner-core.c: if (analog_ops->is_stereo)
drivers/media/video/tuner-core.c: analog_ops->is_stereo(fe)
? "yes" : "no");
drivers/media/video/tuner-core.c: if (analog_ops->is_stereo) {
drivers/media/video/tuner-core.c:
analog_ops->is_stereo(&t->fe) ?
It seems that nobody is using is_stereo() callback anymore. So, we can just
get rid of it. This is part of an old API that was there before mkrufky patches
that unified analog and digital tuners.
> + u32 tuner_status;
> +
> + fe_tuner_ops->get_status(&t->fe, &tuner_status);
> vt->rxsubchans =
> - analog_ops->is_stereo(&t->fe) ?
> + (tuner_status & TUNER_STATUS_STEREO) ?
> V4L2_TUNER_SUB_STEREO :
> V4L2_TUNER_SUB_MONO;
> + } else {
> + if (analog_ops->is_stereo) {
> + vt->rxsubchans =
> + analog_ops->is_stereo(&t->fe) ?
> + V4L2_TUNER_SUB_STEREO :
> + V4L2_TUNER_SUB_MONO;
> + }
> }
> + if (analog_ops->has_signal)
> + vt->signal = analog_ops->has_signal(&t->fe);
has_signal() seems to be another callback asking for its removal. Only tda8290
has it:
$ git grep has_signal drivers/media/
drivers/media/common/tuners/tda8290.c:static int tda8295_has_signal(struct
dvb_frontend *fe)
drivers/media/common/tuners/tda8290.c: if (tda8295_has_signal(fe))
drivers/media/common/tuners/tda8290.c:static int tda8290_has_signal(struct
dvb_frontend *fe)
drivers/media/common/tuners/tda8290.c: .has_signal = tda8290_has_signal,
drivers/media/common/tuners/tda8290.c: .has_signal = tda8295_has_signal,
drivers/media/dvb/dvb-core/dvb_frontend.h: int (*has_signal)(struct
dvb_frontend *fe);
drivers/media/video/tuner-core.c:static int fe_has_signal(struct dvb_frontend
*fe)
drivers/media/video/tuner-core.c: .has_signal = fe_has_signal,
drivers/media/video/tuner-core.c: if (analog_ops->has_signal)
drivers/media/video/tuner-core.c:
analog_ops->has_signal(fe));
drivers/media/video/tuner-core.c: if (analog_ops->has_signal)
drivers/media/video/tuner-core.c: vt->signal =
analog_ops->has_signal(&t->fe);
Other drivers use get_rf_strength():
$ git grep get_rf_strength drivers/media/
drivers/media/common/tuners/tea5761.c:static int tea5761_get_rf_strength(struct
dvb_frontend *fe, u16 *strength)
drivers/media/common/tuners/tea5761.c: .get_rf_strength =
tea5761_get_rf_strength,
drivers/media/common/tuners/tea5767.c:static int tea5767_get_rf_strength(struct
dvb_frontend *fe, u16 *strength)
drivers/media/common/tuners/tea5767.c: .get_rf_strength =
tea5767_get_rf_strength,
drivers/media/common/tuners/tuner-simple.c:static int
simple_get_rf_strength(struct dvb_frontend *fe, u16 *strength)
drivers/media/common/tuners/tuner-simple.c: .get_rf_strength =
simple_get_rf_strength,
drivers/media/common/tuners/tuner-xc2028.c: .get_rf_strength =
xc2028_signal,
drivers/media/dvb/dvb-core/dvb_frontend.h: int (*get_rf_strength)(struct
dvb_frontend *fe, u16 *strength);
drivers/media/video/tuner-core.c: if (fe->ops.tuner_ops.get_rf_strength)
drivers/media/video/tuner-core.c: fe->ops.tuner_ops.get_rf_strength(fe,
&strength);
So, I think we should replace has_signal() at tda8290, with get_rf_strength(),
and remove its call from tuner-core.
> }
> - if (analog_ops->has_signal)
> - vt->signal = analog_ops->has_signal(&t->fe);
> - vt->capability |=
> - V4L2_TUNER_CAP_LOW | V4L2_TUNER_CAP_STEREO;
> - vt->audmode = t->audmode;
> - vt->rangelow = radio_range[0] * 16000;
> - vt->rangehigh = radio_range[1] * 16000;
> +
> return 0;
> }
>
> @@ -911,6 +921,11 @@
> /* do nothing unless we're a radio tuner */
> if (t->mode != V4L2_TUNER_RADIO)
> return 0;
> +
> + /* Should this really fail silently if the device is asleep? */
> + if (t->in_standby == 1)
> + return 0;
> +
Same comment as before: it should wake up the device. That's
the expected behaviour by radio applications like console/radio.
> t->audmode = vt->audmode;
> set_radio_freq(client, t->radio_freq);
> return 0;
> @@ -1004,14 +1019,11 @@
> *tv = NULL;
>
> list_for_each_entry(pos, &tuner_list, list) {
> - int mode_mask;
> -
> if (pos->i2c->adapter != adap ||
> strcmp(pos->i2c->driver->driver.name, "tuner"))
> continue;
>
> - mode_mask = pos->mode_mask & ~T_STANDBY;
> - if (*radio == NULL && mode_mask == T_RADIO)
> + if (*radio == NULL && pos->mode_mask == T_RADIO)
> *radio = pos;
> /* Note: currently TDA9887 is the only demod-only
> device. If other devices appear then we need to
> @@ -1063,7 +1075,8 @@
> t->i2c->addr) >= 0) {
> t->type = TUNER_TEA5761;
> t->mode_mask = T_RADIO;
> - t->mode = T_STANDBY;
> + t->mode = T_RADIO;
> + t->in_standby = 1;
> /* Sets freq to FM range */
> t->radio_freq = 87.5 * 16000;
> tuner_lookup(t->i2c->adapter, &radio, &tv);
> @@ -1088,7 +1101,8 @@
> t->type = TUNER_TDA9887;
> t->mode_mask = T_RADIO | T_ANALOG_TV |
> T_DIGITAL_TV;
> - t->mode = T_STANDBY;
> + t->mode = T_ANALOG_TV;
> + t->in_standby = 1;
> goto register_client;
> }
> break;
> @@ -1098,7 +1112,8 @@
> >= 0) {
> t->type = TUNER_TEA5767;
> t->mode_mask = T_RADIO;
> - t->mode = T_STANDBY;
> + t->mode = T_RADIO;
> + t->in_standby = 1;
> /* Sets freq to FM range */
> t->radio_freq = 87.5 * 16000;
> tuner_lookup(t->i2c->adapter, &radio, &tv);
Not sure about the above. In the past, T_STANDBY were used at the
initial setup to tell the core that the tuner is not in usage. I
think we need to do some tests to see if none of the above changes
would break it. Complex devices to probe are those with multiple
tuners, like devices with a tea5767 for FM and a separate tuner for
TV. I think we need to do some tests with those devices.
Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
