Re: [RFCv1 PATCH 7/7] tuner-core: s_tuner should not change tuner mode.

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em 12-06-2011 08:59, Mauro Carvalho Chehab escreveu:
> Em 12-06-2011 08:36, Hans Verkuil escreveu:
>> On Saturday, June 11, 2011 21:04:57 Mauro Carvalho Chehab wrote:
>>> Em 11-06-2011 14:27, Hans Verkuil escreveu:
>>>> On Saturday, June 11, 2011 15:54:59 Mauro Carvalho Chehab wrote:
>>>>> Em 11-06-2011 10:34, Hans Verkuil escreveu:
>>>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>>
>>>>>> According to the spec the tuner type field is not used when calling
>>>>>> S_TUNER: index, audmode and reserved are the only writable fields.
>>>>>>
>>>>>> So remove the type check. Instead, just set the audmode if the current
>>>>>> tuner mode is set to radio.
>>>>>
>>>>> I suspect that this patch also breaks support for a separate radio tuner.
>>>>> if tuner-type is not properly filled, then the easiest fix would be to
>>>>> revert some changes done at the tuner cleanup/fixup patches applied on .39.
>>>>> Yet, the previous logic were trying to hint the device mode, with is bad
>>>>> (that's why it was changed).
>>>>>
>>>>> The proper change seems to add a parameter to this callback, set by the
>>>>> bridge driver, informing if the device is using radio or video mode.
>>>>> We need also to patch the V4L API specs, as it allows using a video node
>>>>> for radio, and vice versa. This is not well supported, and it seems silly
>>>>> to keep it at the specs and needing to add hints at the drivers due to
>>>>> that.
>>>>
>>>> So, just to make sure I understand correctly what you want. The bridge or
>>>> platform drivers will fill in the vf->type (for g/s_frequency) or vt->type
>>>> (for g/s_tuner) based on the device node: RADIO if /dev/radio is used,
>>>> TV for anything else.
>>>
>>> Yes. I remember I've reviewed the bridge drivers when I rewrote the tuner code.
>>> Of course, I might have left something else. Btw, the older code were also
>>> requiring it.
>>>
>>> The cx18 implementation were merged after the changes, so maybe it is not doing 
>>> the right thing.
>>
>> Actually, G_TUNER was wrong for bttv, ivtv, cx18 and pvrusb2. So it wasn't
>> just some random driver that failed to set vf/vt->type.
> 
> G_FREQUENCY were broken just on cx18. As I said before, filling the type were
> always required. Anyway, I've added a proper documentation for it. See the
> patch bellow.
> 
> The same patch should be done also for G_TUNER.
> 
>>>> What about VIDIOC_S_FREQUENCY? The spec says that the app needs to fill this
>>>> in. Will we just overwrite vf->type or will we check and return -EINVAL if
>>>> the app tries to set e.g. a TV frequency on /dev/radio?
>>>
>>> That's a very good question. What happens is that the V4L2 API used to allow
>>> opening a /dev/radio device for TV (or even for VBI). IMHO, this were a trouble
>>> at the API specs. I think that this were changed on newer versions of the spec.
>>
>> If you want, then I can add an extra patch to my v4 patch series that returns
>> -EINVAL in video_ioctl2 if S_FREQUENCY is called with an inconsistent tuner type.
>> (inconsistent with the device node's type, that is).
> 
> The reason why check_mode returns -EINVAL is that this error code may need to be
> returned to the caller. You should note, however, that the bridge code may need
> to be fixed if you return the check_mode error code, as otherwise the error will
> simply be discarded or it it will break devices with two tuners.
> 
> For example, currently, bttv driver uses v4l2_device_call_all() for it.
> So, any errors returned by it will be simply discarded.
> 
> If some bridge driver is using v4l2_device_call_until_err(), it will stop on the first
> tuner that gets an error.
> 
> The solution is to implement a v4l2_device_call_until_not_err() and use it for the
> tuner commands.
> 
>>>> Is VIDIOC_S_FREQUENCY allowed to change the tuner mode? E.g. if /dev/radio was
>>>> opened, and now I open /dev/video and call S_FREQUENCY with the TV tuner type,
>>>> should that change the tuner to tv mode?
>>>
>>> Yes. I think that some applications like kradio just keeps the device node opened.
>>> If we return -EBUSY, those applications will break. The reverse is more tricky:
>>> e. g. if /dev/video is streaming, I think that the bridge driver should return
>>> -EBUSY if the device can't do both TV and radio at the same time, but this is
>>> something that it is device-specific, so such logic, if needed, should be implemented
>>> at the bridge driver.
>>
>> Agreed.
>>  
>>>> I think the type passed to S_FREQUENCY should 1) match the device node's type
>>>> (if not, then return -EINVAL) and 2) should match the current mode (if not,
>>>> then return -EBUSY). So attempts to change the TV frequency when in radio
>>>> mode should fail. This second rule should also be valid for S_TUNER.
>>>
>>> See above.
>>>
>>>> What should G_TUNER return on a video node when in radio mode or vice versa?
>>>> For G_FREQUENCY you can still return the last used frequency, but that's
>>>> much more ambiguous for G_TUNER. One option is to set rxsubchans, signal and
>>>> afc all to 0 if you query G_TUNER when 'in the wrong mode'.
>>>
>>> The current logic should handle this case well. I tested it carefully. Basically,
>>> if the device is on Radio mode, and has a separate tuner for TV, the TV tuner
>>> should not touch the structure. The Radio tuner should properly fill the values.
>>
>> I analyzed it again and you are correct.
>>
>>> Calls to G_TUNER/G_FREQUENCY shouldn't switch the device mode, or they may break
>>> applications like kradio, that may be always there during the entire KDE section.
>>
>> Obviously.
>>
>>>> The VIDIOC_G/S_MODULATOR ioctls do not have a type and they are RADIO only,
>>>> so that's OK.
>>>>
>>>> And how do we switch between radio and TV? Right now opening the radio node
>>>> will set the tuner in radio mode, and calling S_STD will change the mode to
>>>> TV again. As mentioned above, what S_FREQUENCY is supposed to do is undefined
>>>> at the moment.
>>>
>>> If S_FREQUENCY is called from /dev/video (or /dev/vbi), it should set it to TV. If
>>> it is called from /dev/radio, it should put the device on radio mode.
>>>
>>> The current logic already does that. I tested it on several devices, with both
>>> tea5767 and without it.
>>>
>>>> What about this:
>>>>
>>>> Opening /dev/radio effectively starts the radio mode. So if there is TV
>>>> capture in progress, then the open should return -EBUSY. Otherwise it
>>>> switches the tuner to radio mode. And it stays in radio mode until the
>>>> last filehandle of /dev/radio is closed. At that point it will automatically
>>>> switch back to TV mode (if there is one, of course).
>>>
>>> No. This would break existing applications. The mode switch should be done
>>> at S_FREQUENCY (e. g. when the radio application is tuning into a channel).
>>
>> This is not what happens today as the switch to radio occurs as soon as you open
>> the radio node. It's the reason for the s_radio op.
> 
> The s_radio op is something that I wanted to remove. It was there in the past to feed
> the TV/radio hint logic. I wrote a patch for it, but I ended by discarding from my
> final queue (I can't remember why).
> 
> I think that the hint logic were completely removed, but we may need to take a look
> on the callers for s_radio. I'll check it right now.
> 

The s_radio callback requires some care, as it is used on several places. It is probably
safe to remove it from tuner, but a few sub-drivers like msp3400 needs it. The actual
troubles seem to happen at the bridge drivers that call it during open(). It should be
called only at s_frequency. I opted to keep the callback just to avoid having a bridge
driver switching its registers to radio mode, and not having the tuner following it.

If we move the radio mode switch at the bridge drivers to s_frequency only, we can just
remove this callback from tuner, letting it to be implemented only at the audio decoders.

Cheers,
Mauro.