Re: [PATCH v14 5/6] au0828: fix enable and disable source audio and video inconsistencies

[shuah <shuah@kernel.org>]

On 4/1/19 5:14 AM, Hans Verkuil wrote:
> Hi Shuah,
> 
> On 3/30/19 2:54 AM, Shuah Khan wrote:
>> Enable and disable source interfaces aren't consistent in enforcing
>> how video and audio share the tuner resource.
>>
>> Fix these issues to enforce the following rules and allow
>> sharing between audio and video applications.
>>
>> - When DVB is streaming, audio/video/vbi/s-video/composite
>>    should find the resource busy. DVB holds the tuner in
>>    exclusive mode.
>> - When video is streaming, audio can share the tuner and vice versa.
>> - v4l2 allows multiple applications to open video device.
>> - Video applications call enable source multiple times during their
>>    run-time. Resource should stay locked until the last application
>>    releases it.
>> - A shared resource should stay in shared state and locked when it is
>>    in use by audio and video. More than one video application is allowed
>>    to use the tuner as long as video streaming protocol allows such usage.
>>    Resource is released when the last video/audio application releases it.
>> - S-Video and Composite hold the resource in exclusive mode.
>> - VBI allows more than vbi applications sharing and will not share
>>    with another type. When resource is locked by VBI and in use by
>>    multiple VBI applications, it should stay locked until the last
>>    application disables it.
> 
> This isn't correct: only one application at most can stream VBI, but
> VBI can be shared with video and audio streaming.
> 
> All three come from the same source (analog TV), so as long as one of
> these is streaming the analog tuner resource is in use.
> 
> With this patch it seems that streaming VBI blocks streaming audio/video
> and vice versa. That's wrong.

Right. The reason I went this route is that VBI streaming stops even
when another VBI starts to stream. I am debugging this. Looks like
you have a fix for this from reading below.

> 
> Otherwise everything looks OK, except for a typo below.
> 

Will fix the typos and also I noticed, I forgot to catch updating
Copyright in one of the files.

>>
>> Signed-off-by: Shuah Khan <shuah@kernel.org>
>> ---
>>   drivers/media/usb/au0828/au0828-core.c | 182 +++++++++++++++++++------
>>   drivers/media/usb/au0828/au0828.h      |   5 +-
>>   2 files changed, 148 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
>> index 4f8ba6f64d3c..b8bcab2fa1dc 100644
>> --- a/drivers/media/usb/au0828/au0828-core.c
>> +++ b/drivers/media/usb/au0828/au0828-core.c
>> @@ -272,6 +272,27 @@ static void au0828_media_graph_notify(struct media_entity *new,
>>   	}
>>   }
>>   
>> +static bool au0828_is_link_sharable(struct media_entity *owner,
>> +				    struct media_entity *entity)
>> +{
>> +	bool sharable = false;
> 
> typo: sharable -> shareable
> 
> Also in the function name: do a search-and-replace for this.

Will do.

> 
>> +
>> +	/* Tuner link can be shared by audio and video */
> 
> add: and VBI
> 
> I think the core issue is here since VBI should be included with
> IO_V4L and AUDIO_CAPTURE.
> 
>> +	switch (owner->function) {
>> +	case MEDIA_ENT_F_IO_V4L:
>> +	case MEDIA_ENT_F_AUDIO_CAPTURE:
>> +		if (entity->function == MEDIA_ENT_F_IO_V4L ||
>> +		    entity->function == MEDIA_ENT_F_AUDIO_CAPTURE)
>> +			sharable = true;
>> +		break;
>> +	case MEDIA_ENT_F_DTV_DEMOD:
>> +	case MEDIA_ENT_F_IO_VBI:
>> +	default:
>> +		break;
>> +	}
>> +	return sharable;
> 
> I hacked this function so VBI is included with IO_V4L/AUDIO_CAPTURE
> and now it works.
> 

Yes. Y have been debugging with VBI included in the sharing, however
running into the below problem. Do you have the patch I can use to
test with this series?

> There is one remaining issue (but that was always a problem with au0828):
> when streaming is in progress from both video and vbi, and then you stop
> the video streaming, then vbi stops as well.
> 
> au0828_stop_streaming() calls v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 0);
> which it should only do when the last user disappears. I'll post a patch
> for au0828 to fix this.
> 

Thanks for finding this problem. I narrowed it to close and didn't get
a chance to play with it.

thanks,
-- Shuah