[PATCH/RFC 0/2] Support controls at the subdev file handler level

[PATCH/RFC 0/2] Support controls at the subdev file handler level

[Laurent Pinchart]

Hi everybody,

Here's a patch set that adds support for per-file-handle controls on V4L2
subdevs. The patches are work in progress, but I'm still sending them as
Samsung expressed interest in a similar feature on V4L2 device nodes.

Laurent Pinchart (2):
  v4l: subdev: Move file handle support to drivers
  v4l: subdev: Add support for file handler control handler

 drivers/media/video/v4l2-subdev.c |  144 +++++++++++++++++++-----------------
 include/media/v4l2-subdev.h       |   10 ++-
 2 files changed, 84 insertions(+), 70 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH/RFC 1/2] v4l: subdev: Move file handle support to drivers

[Laurent Pinchart]

Drivers sometimes need to embed the v4l2_subdev_fh structure into a
higher level driver-specific structure. As the v4l2_subdev_fh structure
is allocated by subdev core, this isn't possible.

Fix the problem by moving allocation and free of the structure to the
subdevices. Two helper functions v4l2_subdev_fh_open() and
v4l2_subdev_fh_close() can be used to fill the subdev file operations by
drivers that don't need to perform any specific operation in their open
and close handlers.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/v4l2-subdev.c |  126 ++++++++++++++++++++-----------------
 include/media/v4l2-subdev.h       |    5 ++
 2 files changed, 72 insertions(+), 59 deletions(-)

diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c
index 0b80644..5f23c9f 100644
--- a/drivers/media/video/v4l2-subdev.c
+++ b/drivers/media/video/v4l2-subdev.c
@@ -31,8 +31,10 @@
 #include <media/v4l2-fh.h>
 #include <media/v4l2-event.h>
 
-static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
+int v4l2_subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
 {
+	int ret;
+
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	/* Allocate try format and crop in the same memory block */
 	fh->try_fmt = kzalloc((sizeof(*fh->try_fmt) + sizeof(*fh->try_crop))
@@ -43,104 +45,110 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
 	fh->try_crop = (struct v4l2_rect *)
 		(fh->try_fmt + sd->entity.num_pads);
 #endif
+
+	ret = v4l2_fh_init(&fh->vfh, &sd->devnode);
+	if (ret)
+		goto error;
+
+	if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) {
+		ret = v4l2_event_init(&fh->vfh);
+		if (ret)
+			goto error;
+
+		ret = v4l2_event_alloc(&fh->vfh, sd->nevents);
+		if (ret)
+			goto error;
+	}
+
+	v4l2_fh_add(&fh->vfh);
 	return 0;
+
+error:
+	v4l2_subdev_fh_exit(fh);
+	return ret;
 }
+EXPORT_SYMBOL_GPL(v4l2_subdev_fh_init);
 
-static void subdev_fh_free(struct v4l2_subdev_fh *fh)
+void v4l2_subdev_fh_exit(struct v4l2_subdev_fh *fh)
 {
+	v4l2_fh_del(&fh->vfh);
+	v4l2_fh_exit(&fh->vfh);
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	kfree(fh->try_fmt);
 	fh->try_fmt = NULL;
 	fh->try_crop = NULL;
 #endif
 }
+EXPORT_SYMBOL_GPL(v4l2_subdev_fh_exit);
 
-static int subdev_open(struct file *file)
+int v4l2_subdev_fh_open(struct v4l2_subdev *sd, struct file *file)
 {
-	struct video_device *vdev = video_devdata(file);
-	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
-	struct v4l2_subdev_fh *subdev_fh;
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	struct media_entity *entity = NULL;
-#endif
+	struct v4l2_subdev_fh *fh;
 	int ret;
 
-	subdev_fh = kzalloc(sizeof(*subdev_fh), GFP_KERNEL);
-	if (subdev_fh == NULL)
+	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
+	if (fh == NULL)
 		return -ENOMEM;
 
-	ret = subdev_fh_init(subdev_fh, sd);
-	if (ret) {
-		kfree(subdev_fh);
-		return ret;
-	}
+	ret = v4l2_subdev_fh_init(fh, sd);
+	if (ret < 0)
+		goto error;
 
-	ret = v4l2_fh_init(&subdev_fh->vfh, vdev);
-	if (ret)
-		goto err;
+	file->private_data = &fh->vfh;
+	return 0;
 
-	if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) {
-		ret = v4l2_event_init(&subdev_fh->vfh);
-		if (ret)
-			goto err;
+error:
+	v4l2_subdev_fh_exit(fh);
+	kfree(fh);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_fh_open);
 
-		ret = v4l2_event_alloc(&subdev_fh->vfh, sd->nevents);
-		if (ret)
-			goto err;
-	}
+int v4l2_subdev_fh_close(struct v4l2_subdev *sd, struct file *file)
+{
+	struct v4l2_subdev_fh *fh = to_v4l2_subdev_fh(file->private_data);
+
+	v4l2_subdev_fh_exit(fh);
+	kfree(fh);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_fh_close);
+
+static int subdev_open(struct file *file)
+{
+	struct video_device *vdev = video_devdata(file);
+	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
+	int ret;
 
-	v4l2_fh_add(&subdev_fh->vfh);
-	file->private_data = &subdev_fh->vfh;
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	if (sd->v4l2_dev->mdev) {
-		entity = media_entity_get(&sd->entity);
-		if (!entity) {
-			ret = -EBUSY;
-			goto err;
-		}
+		if (!media_entity_get(&sd->entity))
+			return -EBUSY;
 	}
 #endif
-
 	if (sd->internal_ops && sd->internal_ops->open) {
-		ret = sd->internal_ops->open(sd, subdev_fh);
-		if (ret < 0)
-			goto err;
+		ret = sd->internal_ops->open(sd, file);
+		if (ret < 0) {
+			media_entity_put(&sd->entity);
+			return ret;
+		}
 	}
 
 	return 0;
-
-err:
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	if (entity)
-		media_entity_put(entity);
-#endif
-	v4l2_fh_del(&subdev_fh->vfh);
-	v4l2_fh_exit(&subdev_fh->vfh);
-	subdev_fh_free(subdev_fh);
-	kfree(subdev_fh);
-
-	return ret;
 }
 
 static int subdev_close(struct file *file)
 {
 	struct video_device *vdev = video_devdata(file);
 	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
-	struct v4l2_fh *vfh = file->private_data;
-	struct v4l2_subdev_fh *subdev_fh = to_v4l2_subdev_fh(vfh);
 
 	if (sd->internal_ops && sd->internal_ops->close)
-		sd->internal_ops->close(sd, subdev_fh);
+		sd->internal_ops->close(sd, file);
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	if (sd->v4l2_dev->mdev)
 		media_entity_put(&sd->entity);
 #endif
-	v4l2_fh_del(vfh);
-	v4l2_fh_exit(vfh);
-	subdev_fh_free(subdev_fh);
-	kfree(subdev_fh);
-	file->private_data = NULL;
-
 	return 0;
 }
 
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 1562c4f..4671459 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -571,6 +571,11 @@ static inline void *v4l2_get_subdev_hostdata(const struct v4l2_subdev *sd)
 void v4l2_subdev_init(struct v4l2_subdev *sd,
 		      const struct v4l2_subdev_ops *ops);
 
+int v4l2_subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd);
+void v4l2_subdev_fh_exit(struct v4l2_subdev_fh *fh);
+int v4l2_subdev_fh_open(struct v4l2_subdev *sd, struct file *file);
+int v4l2_subdev_fh_close(struct v4l2_subdev *sd, struct file *file);
+
 /* Call an ops of a v4l2_subdev, doing the right checks against
    NULL pointers.
 
-- 
1.7.3.4

[PATCH/RFC 2/2] v4l: subdev: Add support for file handler control handler

[Laurent Pinchart]

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/v4l2-subdev.c |   18 +++++++++---------
 include/media/v4l2-subdev.h       |    5 +++--
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c
index 5f23c9f..5d3a4bd 100644
--- a/drivers/media/video/v4l2-subdev.c
+++ b/drivers/media/video/v4l2-subdev.c
@@ -157,31 +157,31 @@ 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 *vfh = file->private_data;
-#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	struct v4l2_subdev_fh *subdev_fh = to_v4l2_subdev_fh(vfh);
-#endif
+	struct v4l2_ctrl_handler *ctrl_handler = subdev_fh->ctrl_handler
+		? subdev_fh->ctrl_handler : sd->ctrl_handler;
 
 	switch (cmd) {
 	case VIDIOC_QUERYCTRL:
-		return v4l2_subdev_queryctrl(sd, arg);
+		return v4l2_queryctrl(ctrl_handler, arg);
 
 	case VIDIOC_QUERYMENU:
-		return v4l2_subdev_querymenu(sd, arg);
+		return v4l2_querymenu(ctrl_handler, arg);
 
 	case VIDIOC_G_CTRL:
-		return v4l2_subdev_g_ctrl(sd, arg);
+		return v4l2_g_ctrl(ctrl_handler, arg);
 
 	case VIDIOC_S_CTRL:
-		return v4l2_subdev_s_ctrl(sd, arg);
+		return v4l2_s_ctrl(ctrl_handler, arg);
 
 	case VIDIOC_G_EXT_CTRLS:
-		return v4l2_subdev_g_ext_ctrls(sd, arg);
+		return v4l2_g_ext_ctrls(ctrl_handler, arg);
 
 	case VIDIOC_S_EXT_CTRLS:
-		return v4l2_subdev_s_ext_ctrls(sd, arg);
+		return v4l2_s_ext_ctrls(ctrl_handler, arg);
 
 	case VIDIOC_TRY_EXT_CTRLS:
-		return v4l2_subdev_try_ext_ctrls(sd, arg);
+		return v4l2_try_ext_ctrls(ctrl_handler, arg);
 
 	case VIDIOC_DQEVENT:
 		if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 4671459..6229ffc 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -469,8 +469,8 @@ struct v4l2_subdev_ops {
 struct v4l2_subdev_internal_ops {
 	int (*registered)(struct v4l2_subdev *sd);
 	void (*unregistered)(struct v4l2_subdev *sd);
-	int (*open)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
-	int (*close)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
+	int (*open)(struct v4l2_subdev *sd, struct file *file);
+	int (*close)(struct v4l2_subdev *sd, struct file *file);
 };
 
 #define V4L2_SUBDEV_NAME_SIZE 32
@@ -527,6 +527,7 @@ struct v4l2_subdev_fh {
 	struct v4l2_mbus_framefmt *try_fmt;
 	struct v4l2_rect *try_crop;
 #endif
+	struct v4l2_ctrl_handler *ctrl_handler;
 };
 
 #define to_v4l2_subdev_fh(fh)	\
-- 
1.7.3.4

RE: [PATCH/RFC 0/2] Support controls at the subdev file handler level

[Jeongtae Park]

Hi, all.

Some hardware need to handle per-filehandle level controls.
Hans suggests add a v4l2_ctrl_handler struct v4l2_fh. It will be work fine.
Although below patch series are for subdev, but it's great start point.
I will try to make a patch.

If v4l2 control framework can be handle per-filehandle controls,
a driver could be handle per-buffer level controls also. (with VB2 callback
operation)

Best Regards,

/jtpark

> -----Original Message-----
> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> owner@vger.kernel.org] On Behalf Of Laurent Pinchart
> Sent: Thursday, March 10, 2011 6:27 AM
> To: linux-media@vger.kernel.org
> Cc: hverkuil@xs4all.nl; jaeryul.oh@samsung.com
> Subject: [PATCH/RFC 0/2] Support controls at the subdev file handler level
> 
> Hi everybody,
> 
> Here's a patch set that adds support for per-file-handle controls on V4L2
> subdevs. The patches are work in progress, but I'm still sending them as
> Samsung expressed interest in a similar feature on V4L2 device nodes.
> 
> Laurent Pinchart (2):
>   v4l: subdev: Move file handle support to drivers
>   v4l: subdev: Add support for file handler control handler
> 
>  drivers/media/video/v4l2-subdev.c |  144 +++++++++++++++++++-------------
----
>  include/media/v4l2-subdev.h       |   10 ++-
>  2 files changed, 84 insertions(+), 70 deletions(-)
> 
> --
> Regards,
> 
> Laurent Pinchart
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 0/2] Support controls at the subdev file handler level

[Laurent Pinchart]

Hi,

On Thursday 10 March 2011 10:56:40 Jeongtae Park wrote:
> Hi, all.
> 
> Some hardware need to handle per-filehandle level controls.
> Hans suggests add a v4l2_ctrl_handler struct v4l2_fh. It will be work fine.
> Although below patch series are for subdev, but it's great start point.
> I will try to make a patch.
> 
> If v4l2 control framework can be handle per-filehandle controls,
> a driver could be handle per-buffer level controls also. (with VB2 callback
> operation)

Per-buffer controls would likely be implemented as a meta-data plane. This 
definitely needs to be brainstormed.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 0/2] Support controls at the subdev file handler level

[Sylwester Nawrocki]

Hi,

On 03/10/2011 10:56 AM, Jeongtae Park wrote:
> Hi, all.
> 
> Some hardware need to handle per-filehandle level controls.
> Hans suggests add a v4l2_ctrl_handler struct v4l2_fh. It will be work fine.
> Although below patch series are for subdev, but it's great start point.
> I will try to make a patch.
> 
> If v4l2 control framework can be handle per-filehandle controls,
> a driver could be handle per-buffer level controls also. (with VB2 callback
> operation)
> 

Can you elaborate to what kind of per buffer controls are you referring to?
Using optional meta data planes is expected to be a proper way to do such
things. Either to pass meta data from application to driver or in the opposite
direction.

I can't see how the per buffer controls can be dependent on a per file handle
control support. Perhaps this is only specific to some selected devices.

--
Regards,
Sylwester Nawrocki

RE: [PATCH/RFC 0/2] Support controls at the subdev file handler level

[Jaeryul Oh]

Hi, 

> -----Original Message-----
> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> owner@vger.kernel.org] On Behalf Of Sylwester Nawrocki
> Sent: Friday, March 11, 2011 4:44 AM
> To: jtp.park@samsung.com
> Cc: linux-media@vger.kernel.org; laurent.pinchart@ideasonboard.com
> Subject: Re: [PATCH/RFC 0/2] Support controls at the subdev file handler
> level
> 
> Hi,
> 
> On 03/10/2011 10:56 AM, Jeongtae Park wrote:
> > Hi, all.
> >
> > Some hardware need to handle per-filehandle level controls.
> > Hans suggests add a v4l2_ctrl_handler struct v4l2_fh. It will be work
> fine.
> > Although below patch series are for subdev, but it's great start point.
> > I will try to make a patch.
> >
> > If v4l2 control framework can be handle per-filehandle controls,
> > a driver could be handle per-buffer level controls also. (with VB2
> callback
> > operation)
> >
> 
> Can you elaborate to what kind of per buffer controls are you referring to?
> Using optional meta data planes is expected to be a proper way to do such
> things. Either to pass meta data from application to driver or in the
> opposite
> direction.
> 
> I can't see how the per buffer controls can be dependent on a per file
> handle
> control support. Perhaps this is only specific to some selected devices.
> 

There might be some missing point about this issue. To support full feature of 
codec, hierarchy should be done as below

Per-node(decoder or encoder)
   |
   |---- Per-file for multi-instance
                     |
                     |--------Per-buffer control for setting dynamic configuration 

Some cases are using per-node handling is enough, but some cases are for per-file
And because of codec characteristics, we need to support per-buffer at this 
hierarchy as above. 
Let's talk more about this at Warsaw.


> --
> Regards,
> Sylwester Nawrocki
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html