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 <dheitmueller@xxxxxxxxxxxxxx> > > 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 <dheitmueller@xxxxxxxxxxxxxx> > Cc: Hans Verkuil <hverkuil@xxxxxxxxx> > > --- 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
