Re: [RFC/PATCH v2 6/7] v4l: subdev: Events support

[Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>]

Hi Mauro,

Mauro Carvalho Chehab wrote:
> Em 09-07-2010 12:31, Laurent Pinchart escreveu:
>> From: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
>>
>> Provide v4l2_subdevs with v4l2_event support. Subdev drivers only need very
>> little to support events.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
>> Signed-off-by: David Cohen <david.cohen@nokia.com>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>  Documentation/video4linux/v4l2-framework.txt |   18 +++++++
>>  drivers/media/video/v4l2-subdev.c            |   71 +++++++++++++++++++++++++-
>>  include/media/v4l2-subdev.h                  |   10 ++++
>>  3 files changed, 98 insertions(+), 1 deletions(-)
>>
>> diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
>> index 9c3f33c..89bd881 100644
>> --- a/Documentation/video4linux/v4l2-framework.txt
>> +++ b/Documentation/video4linux/v4l2-framework.txt
>> @@ -347,6 +347,24 @@ VIDIOC_TRY_EXT_CTRLS
>>  	controls can be also be accessed through one (or several) V4L2 device
>>  	nodes.
>>  
>> +VIDIOC_DQEVENT
>> +VIDIOC_SUBSCRIBE_EVENT
>> +VIDIOC_UNSUBSCRIBE_EVENT
>> +
>> +	The events ioctls are identical to the ones defined in V4L2. They
>> +	behave identically, with the only exception that they deal only with
>> +	events generated by the sub-device. Depending on the driver, those
>> +	events can also be reported by one (or several) V4L2 device nodes.
>> +
>> +	Sub-device drivers that want to use events need to set the
>> +	V4L2_SUBDEV_USES_EVENTS v4l2_subdev::flags and initialize
>> +	v4l2_subdev::nevents to events queue depth before registering the
>> +	sub-device. After registration events can be queued as usual on the
>> +	v4l2_subdev::devnode device node.
>> +
>> +	To properly support events, the poll() file operation is also
>> +	implemented.
>> +
>>  
>>  I2C sub-device drivers
>>  ----------------------
>> diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c
>> index 0ebd760..31bec67 100644
>> --- a/drivers/media/video/v4l2-subdev.c
>> +++ b/drivers/media/video/v4l2-subdev.c
>> @@ -18,26 +18,64 @@
>>   *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>>   */
>>  
>> -#include <linux/types.h>
>>  #include <linux/ioctl.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>>  #include <linux/videodev2.h>
>>  
>>  #include <media/v4l2-device.h>
>>  #include <media/v4l2-ioctl.h>
>> +#include <media/v4l2-fh.h>
>> +#include <media/v4l2-event.h>
>>  
>>  static int subdev_open(struct file *file)
>>  {
>>  	struct video_device *vdev = video_devdata(file);
>>  	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
>> +	struct v4l2_fh *vfh;
>> +	int ret;
>>  
>>  	if (!sd->initialized)
>>  		return -EAGAIN;
>>  
>> +	vfh = kzalloc(sizeof(*vfh), GFP_KERNEL);
>> +	if (vfh == NULL)
>> +		return -ENOMEM;
>> +
>> +	ret = v4l2_fh_init(vfh, vdev);
>> +	if (ret)
>> +		goto err;
> 
> Why to allocate/init the file handler for devices that don't need it?
> I would just move the above logic to happen only if V4L2_SUBDEV_FL_HAS_EVENTS.

This patch actually adds subdev support for V4L2 file handles AND
events. v4l2_fh is also used to support probe formats on subdevs (not
contained in this patchset).

That v4l2_fh_init() function just initialises a few fields, there is no
allocation being done. The v4l2_fh structure will be part of v4l2_subdev_fh:

struct v4l2_subdev_fh {
        struct v4l2_fh vfh;
        struct v4l2_mbus_framefmt *probe_fmt;
        struct v4l2_rect *probe_crop;
};

(Again not yet in this patchset.) The probe formats are a new concept to
allow trying formats across the whole pipeline for which the old try_fmt
wasn't suitable for: memory vs. bus format and pads matter.

>> +
>> +	if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) {
>> +		ret = v4l2_event_init(vfh);
>> +		if (ret)
>> +			goto err;
>> +
>> +		ret = v4l2_event_alloc(vfh, sd->nevents);
>> +		if (ret)
>> +			goto err;
>> +	}
>> +
>> +	v4l2_fh_add(vfh);
>> +	file->private_data = vfh;
>> +
>>  	return 0;
>> +
>> +err:
>> +	v4l2_fh_exit(vfh);
>> +	kfree(vfh);
>> +
>> +	return ret;
>>  }
>>  
>>  static int subdev_close(struct file *file)
>>  {
>> +	struct v4l2_fh *vfh = file->private_data;
>> +
>> +	v4l2_fh_del(vfh);
>> +	v4l2_fh_exit(vfh);
>> +	kfree(vfh);
>> +
>>  	return 0;
>>  }
>>  
>> @@ -45,6 +83,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>  {
>>  	struct video_device *vdev = video_devdata(file);
>>  	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
>> +	struct v4l2_fh *fh = file->private_data;
>>  
>>  	switch (cmd) {
>>  	case VIDIOC_QUERYCTRL:
>> @@ -68,6 +107,18 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>  	case VIDIOC_TRY_EXT_CTRLS:
>>  		return v4l2_subdev_call(sd, core, try_ext_ctrls, arg);
>>  
>> +	case VIDIOC_DQEVENT:
>> +		if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
>> +			return -ENOIOCTLCMD;
>> +
>> +		return v4l2_event_dequeue(fh, arg, file->f_flags & O_NONBLOCK);
>> +
>> +	case VIDIOC_SUBSCRIBE_EVENT:
>> +		return v4l2_subdev_call(sd, core, subscribe_event, fh, arg);
>> +
>> +	case VIDIOC_UNSUBSCRIBE_EVENT:
>> +		return v4l2_subdev_call(sd, core, unsubscribe_event, fh, arg);
> 
> Hmm... shouldn't it test also for V4L2_SUBDEV_FL_HAS_EVENTS?
> 
> I would do, instead:
> 
> 	if (fh) {
> 		switch(cmd) {
> 			/* FH events logic */
> 		}
> 	}

No need to. If the subdev doesn't support events it likely should not
define handlers for event specific calls, right? v4l2_subdev_call()
behaves well if the handler is NULL.

Both would work equally well, I guess.

Best regards,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com