Hi Hans,
On 10/10/2014 10:54 AM, Hans Verkuil wrote:
Hi Jacek,
I didn't do an in-depth review, but one thing caught my eye:
On 10/08/2014 10:46 AM, Jacek Anaszewski wrote:
The plugin provides support for the media device on Exynos4 SoC.
Added is also a media device configuration file parser.
The media configuration file is used for conveying information
about media device links that need to be established as well
as V4L2 user control ioctls redirection to a particular
sub-device.
The plugin performs single plane <-> multi plane API conversion,
video pipeline linking and takes care of automatic data format
negotiation for the whole pipeline, after intercepting
VIDIOC_S_FMT or VIDIOC_TRY_FMT ioctls.
Signed-off-by: Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx>
Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx>
Cc: Hans Verkuil <hans.verkuil@xxxxxxxxx>
---
configure.ac | 1 +
lib/Makefile.am | 5 +-
lib/libv4l-exynos4-camera/Makefile.am | 7 +
.../libv4l-devconfig-parser.h | 145 ++
lib/libv4l-exynos4-camera/libv4l-exynos4-camera.c | 2486
++++++++++++++++++++
5 files changed, 2642 insertions(+), 2 deletions(-)
create mode 100644 lib/libv4l-exynos4-camera/Makefile.am
create mode 100644 lib/libv4l-exynos4-camera/libv4l-devconfig-parser.h
create mode 100644 lib/libv4l-exynos4-camera/libv4l-exynos4-camera.c
<snip>
+static int is_control_supported(struct media_device *mdev,
+ struct libv4l_media_ctrl_conf *ctrl_cfg)
+{
+ struct v4l2_query_ext_ctrl queryctrl;
+ struct media_entity *entity;
+
+ entity = get_entity_by_name(mdev, ctrl_cfg->entity_name);
+ if (entity == NULL)
+ return 0;
+
+ /* Iterate through control ids */
+
+ queryctrl.id = V4L2_CID_BASE | V4L2_CTRL_FLAG_NEXT_CTRL;
+
+ while (!SYS_IOCTL(entity->fd, VIDIOC_QUERY_EXT_CTRL, &queryctrl)) {
+ if (queryctrl.flags & V4L2_CTRL_FLAG_DISABLED)
+ continue;
+
+ if (!strcmp((char *) queryctrl.name, ctrl_cfg->control_name)) {
+ ctrl_cfg->cid = queryctrl.id &
+ ~V4L2_CTRL_FLAG_NEXT_CTRL;
+ ctrl_cfg->entity = entity;
+
+ return 1;
+ }
+
+ queryctrl.id = queryctrl.id | V4L2_CTRL_FLAG_NEXT_CTRL;
+ }
+
+ queryctrl.id = V4L2_CID_BASE | V4L2_CTRL_FLAG_NEXT_COMPOUND;
Why not combine V4L2_CTRL_FLAG_NEXT_CTRL and V4L2_CTRL_FLAG_NEXT_COMPOUND?
That way you iterate over both 'normal' and compound controls. But do you
really want to look at compound controls? First of all, to my knowledge
the exynos driver doesn't use them and secondly compound controls typically
do not have simple values that can easily be parsed.
I messed up few things here. I will remove checking for compound
controls
[...]
+static int querycap_ioctl(struct exynos4_camera_plugin *plugin,
+ struct v4l2_capability *arg)
+{
+ int ret;
+
+ ret = SYS_IOCTL(plugin->vid_fd, VIDIOC_QUERYCAP, arg);
+
+ if (arg->capabilities & V4L2_CAP_VIDEO_CAPTURE_MPLANE)
+ arg->capabilities |= V4L2_CAP_VIDEO_CAPTURE;
Check device_caps instead of capabilities. That way you can check
what this specific device supports. I hope that the exynos driver
sets device_caps. If not, that should be added.
If you want to make this generic you could do:
__u32 caps = arg->capabilities;
if (caps & V4L2_CAP_DEVICE_CAPS)
caps = arg->device_caps;
Thanks, I'll conform to this.
+static int plugin_ioctl(void *dev_ops_priv, int fd, unsigned long int
cmd,
+ void *arg)
+{
+ struct exynos4_camera_plugin *plugin = dev_ops_priv;
+
+ if (plugin == NULL || arg == NULL)
+ return -EINVAL;
+
+ switch (cmd) {
+ case VIDIOC_S_CTRL:
+ case VIDIOC_G_CTRL:
+ return ctrl_ioctl(plugin, VIDIOC_S_CTRL, arg);
Support for VIDIOC_S/G/TRY_EXT_CTRLS should be added.
I'll cover it.
+ case VIDIOC_TRY_FMT:
+ return set_fmt_ioctl(plugin, VIDIOC_S_FMT, arg,
+ V4L2_SUBDEV_FORMAT_TRY);
+ case VIDIOC_S_FMT:
+ return set_fmt_ioctl(plugin, VIDIOC_S_FMT, arg,
+ V4L2_SUBDEV_FORMAT_ACTIVE);
+ case VIDIOC_G_FMT:
+ return get_fmt_ioctl(plugin, VIDIOC_G_FMT, arg);
+ case VIDIOC_ENUM_FMT:
+ return enum_fmt_ioctl(plugin, VIDIOC_ENUM_FMT, arg);
+ case VIDIOC_QUERYCAP:
+ return querycap_ioctl(plugin, arg);
+ case VIDIOC_QBUF:
+ case VIDIOC_DQBUF:
+ case VIDIOC_QUERYBUF:
+ case VIDIOC_PREPARE_BUF:
+ return buf_ioctl(plugin, cmd, arg);
+ case VIDIOC_REQBUFS:
+ return SIMPLE_CONVERT_IOCTL(fd, cmd, arg,
+ v4l2_requestbuffers);
+ case VIDIOC_STREAMON:
+ case VIDIOC_STREAMOFF:
+ {
+ int *arg_type = (int *) arg;
+ int type;
+
+ type = convert_type(*arg_type);
+
+ if (type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
+ type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
+ V4L2_EXYNOS4_ERR("Invalid buffer type.");
+ return -EINVAL;
+ }
+
+ return SYS_IOCTL(fd, cmd, &type);
+ }
+
+ default:
+ return SYS_IOCTL(fd, cmd, arg);
+ }
+}
+
+PLUGIN_PUBLIC const struct libv4l_dev_ops libv4l2_plugin = {
+ .init = &plugin_init,
+ .close = &plugin_close,
+ .ioctl = &plugin_ioctl,
+};
A lot of this looks like it is exynos independent. I would move such
code to
a separate source and make it easy to reuse elsewhere.
Yes, I was thinking about it, I'm planning to enclose this code
into SO library.
Best Regards,
Jacek Anaszewski
--
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
