Re: [RFC PATCH] tuner-core: fix broken G_TUNER call when tuner is in standby
Em 03-02-2011 21:50, Devin Heitmueller escreveu:
> On Wed, Feb 2, 2011 at 10:21 AM, Mauro Carvalho Chehab
> wrote:
>> 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
>>>
>>> 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
>>> Cc: Hans Verkuil
>>>
>>> --- media_build/linux/drivers/media/video/tuner-core.c2010-10-24
>>> 19:34:59.0 -0400
>>> +++ media_build_950qfixes//linux/drivers/media/video/tuner-core.c
>>> 2011-01-23 17:18:22.381107568 -0500
>>> @@ -90,6 +90,7 @@
>>>
>>> unsigned intmode;
>>> unsigned intmode_mask; /* Combination of allowable modes */
>>> + unsigned intin_standby:1;
>>>
>>> unsigned inttype; /* chip type id */
>>> unsigned intconfig;
>>> @@ -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 ;)
>
> Look again. The target mode being switched to is provided as an
> argument. So the code preserves the old mode in orig_mode, switches
> to the new mode, then calls check_mode() against the new mode. If the
> call fails, we reset it back to the mode it was in before the call.
Ok.
>>> + 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)?
>
> I don't have any radio supported devices.
Without testing on devices with radio, you'll likely break tuner-core.
The way radio applications work is different than the video ones. Some
just opens the device, sets radio frequency and closes it (as there's
no need for the radio app to keep the device node opened).
>>> 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->rang
Re: [RFC PATCH] tuner-core: fix broken G_TUNER call when tuner is in standby
On Wed, Feb 2, 2011 at 10:21 AM, Mauro Carvalho Chehab
wrote:
> 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
>>
>> 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
>> Cc: Hans Verkuil
>>
>> --- media_build/linux/drivers/media/video/tuner-core.c 2010-10-24
>> 19:34:59.0 -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 ;)
Look again. The target mode being switched to is provided as an
argument. So the code preserves the old mode in orig_mode, switches
to the new mode, then calls check_mode() against the new mode. If the
call fails, we reset it back to the mode it was in before the call.
>> + 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)?
I don't have any radio supported devices.
>> 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] * 1
Re: [RFC PATCH] tuner-core: fix broken G_TUNER call when tuner is in standby
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
>
> 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
> Cc: Hans Verkuil
>
> --- media_build/linux/drivers/media/video/tuner-core.c2010-10-24
> 19:34:59.0 -0400
> +++ media_build_950qfixes//linux/drivers/media/video/tuner-core.c
> 2011-01-23 17:18:22.381107568 -0500
> @@ -90,6 +90,7 @@
>
> unsigned intmode;
> unsigned intmode_mask; /* Combination of allowable modes */
> + unsigned intin_standby:1;
>
> unsigned inttype; /* chip type id */
> unsigned intconfig;
> @@ -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.
>
> -
