Sakari Ailus wrote:
> Mauro Carvalho Chehab wrote:
>> Hi Sakari,
>
> Hi Mauro,
>
> ...
>> The idea of having a common file handle at the V4L2 core is interesting, but
>> I see some troubles on it. As you said on this changeset's comment:
>>
>> V4L/DVB: V4L: File handles
>>
>> This patch adds a list of v4l2_fh structures to every video_device.
>> It allows using file handle related information in V4L2. The event interface
>> is one example of such use.
>>
>> Video device drivers should use the v4l2_fh pointer as their
>> file->private_data.
>>
>> You're saying that the drivers should use struct v4l2_fh, but there
>> aren't any patches on your series enforcing this change.
>
> Oh, there should be an "if" there. It is described better in
> Documentation/video4linux/v4l2-framework.txt .
Even there, it is not clear that this is an optional interface. Also, as I am
understanding, you'll likely add other things there, so the better would be
to add some notes at the header files that implement those functions, stating
when and how they should be used.
>
>> Also, you're defining it as just:
>>
>> +struct v4l2_fh {
>> + struct list_head list;
>> + struct video_device *vdev;
>> +};
>>
>> while other drivers need something much more complex. For example, saa7134
>> defines the file handler struct used on file-private_data as:
>>
>> struct saa7134_fh {
>> struct saa7134_dev *dev;
>> unsigned int radio;
>> enum v4l2_buf_type type;
>> unsigned int resources;
>> enum v4l2_priority prio;
>>
>> /* video overlay */
>> struct v4l2_window win;
>> struct v4l2_clip clips[8];
>> unsigned int nclips;
>>
>> /* video capture */
>> struct saa7134_format *fmt;
>> unsigned int width,height;
>> struct videobuf_queue cap;
>> struct saa7134_pgtable pt_cap;
>>
>> /* vbi capture */
>> struct videobuf_queue vbi;
>> struct saa7134_pgtable pt_vbi;
>> };
>>
>> Even the simplest driver we have (vivi) require more than just
>> the video device:
>>
>> struct vivi_fh {
>> struct vivi_dev *dev;
>>
>> /* video capture */
>> struct vivi_fmt *fmt;
>> unsigned int width, height;
>> struct videobuf_queue vb_vidq;
>>
>> enum v4l2_buf_type type;
>> unsigned char bars[8][3];
>> int input; /* Input Number on bars */
>> };
>>
>>
>> So, it is not clear for me how to convert any of the existing
>> drivers to fit on your proposal.
>
> The struct v4l2_fh is meant to be a part of the driver's own file handle
> structure. It does not attempt to offer drivers anything directly but
> generic functionality that depends on file handle specific data such as
> V4L2 events. E.g.
>
> struct my_video_fh {
> blah;
> struct v4l2_fh fh;
> blah;
> };
>
>> Also, the better is if you could provide us a patchset with the needed
>> conversion. It is interesting if you could add some notes there about why
>> this change is needed.
>
> There is no need to change any existing drivers, this change does not
> affect them. V4L2 file handles are only for those drivers that want to
> use them. Other drivers can safely ignore this. But if a driver wants to
> use V4L2 events it also has to use V4L2 file handles since event
> information is stored in V4L2 file handles.
It is always a good idea to add at least one use case when a new API is
added on kernel. So, I prefer if you could send those changes together
with some use case. Hans is planning to use on ivtv (as his email on
this thread). Also Guennadi said me once, at irc, that he has also something
ready or almost ready just waiting for this merge. So, it would be really
nice if you can send either one of the implementations together with the
patch series. This helps to better understand about the usecases.
>
> Cheers,
>
--
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
