Em 19-09-2010 18:10, Hans Verkuil escreveu:
> On Sunday, September 19, 2010 22:20:18 Mauro Carvalho Chehab wrote:
>> Em 19-09-2010 16:06, Hans Verkuil escreveu:
>>> On Sunday, September 19, 2010 20:29:58 Mauro Carvalho Chehab wrote:
>>>> Em 19-09-2010 11:58, Hans Verkuil escreveu:
>>>>> On Sunday, September 19, 2010 13:43:43 Mauro Carvalho Chehab wrote:
>>>>
>>> Multiple stream per device node: if you are talking about allowing e.g. 
>>> both VBI streaming
>>> and video streaming from one device node, then that is indeed allowed by 
>>> the current spec.
>>> Few drivers support this though, and it is a really bad idea. During the 
>>> Helsinki meeting we
>>> agreed to remove this from the spec (point 10a in the mini summit report).
>>
>> I'm talking about read(), overlay and mmap(). The spec says, at "Multiple 
>> Opens" [1]:
>>      "When a device supports multiple functions like capturing and overlay 
>> simultaneously,
>>       multiple opens allow concurrent use of the device by forked processes 
>> or specialized applications."
>>
>> [1] http://linuxtv.org/downloads/v4l-dvb-apis/ch01.html#id2717880
>>
>> So, it is allowed by the spec. What is forbidden is to have some copy logic 
>> in kernel to duplicate data.
> 
> There is no streaming involved with overlays, is there? It is all handled in 
> the driver and
> userspace just tells the driver where the window is. I may be wrong, I'm much 
> more familiar
> with output overlays (OSD). Are overlays actually still working anywhere 
> these days?

It depends on what you call streaming. On overlay mode, there's a PCI2PCI 
transfer stream, from video 
capture memory into the video adapter memory. It is still a stream, even though 
it is not "visible"
after started.

>> Besides that, not all device drivers will work with all applications or 
>> provide the complete set of
>> functionalities. For example, there are only three drivers (ivtv, cx18 and 
>> pvrusb2), as far as I remember, 
>> that only implements read() method. By using your logic that "only a few 
>> drivers allow feature X", maybe
>> we should deprecate read() as well ;)
> 
> There's nothing wrong with read. But reading while streaming at the same time 
> from the same source,
> that's another matter. 

You may not like its implementation, but it is currently in use, and there's 
nothing at spec
forbidding it.

> And I'm hoping that vb2 will make it possible to implement streaming in
> ivtv and cx18.

Ok. That's another reason why we should lock poll/read.

> <snip>
> 
>>>> The problem with the current implementation of v4l2_fh() is that there's 
>>>> no way for the core
>>>> to get per-fh info.
>>>
>>> You mean how to get from a struct file to a v4l2_fh? That should be done 
>>> through
>>> filp->private_data, but since many driver still put other things there, 
>>> that is not
>>> really usable at the moment without changing all those drivers first.
>>
>> It should be drivers decision to put whatever they want on a "priv_data". If 
>> you want to have
>> core data, then you need to use embeed for the core, but keeping priv_data 
>> for private driver's
>> internal usage. That's the standard way used on Linux. You're doing just the 
>> reverse ;)
> 
> I don't follow your reasoning here.

What kernel does, in general, is to use a "class inheritance" by embeding one 
struct into another.
This is used mainly on the core structs. For drivers, it provides void *priv 
data for internal driver-only
usage.

The way you're wanting to do with v4l2_fh is just the reverse: use priv_data 
for core usage, and embed 
struct for the drivers.

>>> This actually will work correctly. When a device node is registered in 
>>> cx88, it is already
>>> hooked into the v4l2_device of the core struct. This was needed to handle 
>>> the i2c subdevs
>>> in the right place: the cx88 core struct. So refcounting will also be done 
>>> in the core struct.
>>
>> No. Look at the actual code. For example, this is what cx88-mpeg does:
>>
>> struct cx8802_dev *dev = pci_get_drvdata(pci_dev);
>>
>> cx88 core is at dev->core.
>>
>> The same happens with cx88-video, using struct cx8800:
>>
>> struct cx8800_dev *dev = pci_get_drvdata(pci_dev);
>>
>> cx88 core is also at dev->core.
>>
>> This device is implemented using multiple PCI devices, one for each 
>> function. Function 0 (video) and Function 2
>> (used for TS devices, like mpeg encoders) can be used independently, but 
>> there are some data that are concurrent.
>> So, drivers will likely need to use two locks, one for the core and one for 
>> the function.
> 
> I was talking about refcounting in cx88 using my patch, not locking. Locking 
> in
> cx88 will almost certainly need two locks.

Depending on the way the cx88 core lock will be implemented, you may need to 
unlock it before release.

I just arguing that it needs to take more care/review at cx88, in order to 
avoid having a dead lock there.

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

Reply via email to