[PATCHv2 00/15] Media Controller compliance fixes

[PATCHv2 00/15] Media Controller compliance fixes

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Hi all,

I've been posting random patches fixing various MC problems, but it is
easier to see them all in a single patch series.

All patches except 13 and 14 are identical to was I posted earlier.
For 13 and 14 I decided to drop the requirement that the application
clears the reserved field. Only the driver will clear it.

The only ioctl for which this might be a problem is SETUP_LINK. I don't
think it is worth it adding this requirement for userspace, and I also
think it is too late to do so.

A quick overview of the series:

Patch 1 and 2 fix two vimc bugs. Patch 3 fixes a v4l2-ioctl.c bug.
Patches 4-6 fix v4l2-subdev.c bugs (arguably patch 6 is more of an
enhancement than a bug fix). Patches 7-10 fix documentation bugs.
Patches 11-14 fix various bugs. Please check 13 and 14 if you agree
with how the reserved fields should be handled.

The final patch reorganizes media.h so it is actually understandable.
See here for how it looks after the patch is applied:

https://git.linuxtv.org/hverkuil/media_tree.git/plain/include/uapi/linux/media.h?h=media-fixes

The patch itself is hard to read, so looking at the reorganized header
is easier.

The two core changes in media.h are:

1) all functions are now grouped together
2) all legacy defines are now all moved to the end of the header

I would really like to see this merged soon. This fixes the most immediate
problems that I found.

Regards,

	Hans

Alexandre Courbot (1):
  media: media-types.rst: fix typo

Hans Verkuil (14):
  vimc: fix control event handling
  vimc: use correct subdev functions
  v4l2-ioctl.c: fix VIDIOC_DV_TIMINGS_CAP: don't clear pad
  v4l2-subdev: without controls return -ENOTTY
  v4l2-subdev: clear reserved fields
  v4l2-subdev: implement VIDIOC_DBG_G_CHIP_INFO ioctl
  subdev-formats.rst: fix incorrect types
  media-ioc-g-topology.rst: fix interface-to-entity link description
  media-types.rst: fix type, small improvements
  media-device.c: zero reserved field
  media.h: fix confusing typo in comment
  media: document and zero reservedX fields in media_v2_topology
  media-ioc-enum-entities/links.rst: document reserved fields
  media.h: reorganize header to make it easier to understand

 .../uapi/mediactl/media-ioc-enum-entities.rst      |  19 +-
 .../media/uapi/mediactl/media-ioc-enum-links.rst   |  18 ++
 .../media/uapi/mediactl/media-ioc-g-topology.rst   |  54 +++-
 Documentation/media/uapi/mediactl/media-types.rst  |  12 +-
 Documentation/media/uapi/v4l/subdev-formats.rst    |   6 +-
 drivers/media/media-device.c                       |   6 +
 drivers/media/media-entity.c                       |  16 -
 drivers/media/platform/vimc/vimc-common.c          |   4 +-
 drivers/media/platform/vimc/vimc-debayer.c         |   2 +-
 drivers/media/platform/vimc/vimc-scaler.c          |   2 +-
 drivers/media/platform/vimc/vimc-sensor.c          |  10 +-
 drivers/media/v4l2-core/v4l2-ioctl.c               |   2 +-
 drivers/media/v4l2-core/v4l2-subdev.c              |  42 +++
 include/uapi/linux/media.h                         | 345 ++++++++++-----------
 14 files changed, 314 insertions(+), 224 deletions(-)

-- 
2.15.1

[PATCHv2 01/15] vimc: fix control event handling

[Hans Verkuil]

The sensor subdev didn't handle control events. Add support for this.
Found with v4l2-compliance.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/vimc/vimc-common.c | 4 +++-
 drivers/media/platform/vimc/vimc-sensor.c | 8 ++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
index 9d63c84a9876..617415c224fe 100644
--- a/drivers/media/platform/vimc/vimc-common.c
+++ b/drivers/media/platform/vimc/vimc-common.c
@@ -434,7 +434,9 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
 	v4l2_set_subdevdata(sd, ved);
 
 	/* Expose this subdev to user space */
-	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	if (sd->ctrl_handler)
+		sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;
 
 	/* Initialize the media entity */
 	ret = media_entity_pads_init(&sd->entity, num_pads, ved->pads);
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 457e211514c6..54184cd9e0ff 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -23,6 +23,7 @@
 #include <linux/v4l2-mediabus.h>
 #include <linux/vmalloc.h>
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-subdev.h>
 #include <media/tpg/v4l2-tpg.h>
 
@@ -284,11 +285,18 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable)
 	return 0;
 }
 
+static struct v4l2_subdev_core_ops vimc_sen_core_ops = {
+	.log_status = v4l2_ctrl_subdev_log_status,
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
 static const struct v4l2_subdev_video_ops vimc_sen_video_ops = {
 	.s_stream = vimc_sen_s_stream,
 };
 
 static const struct v4l2_subdev_ops vimc_sen_ops = {
+	.core = &vimc_sen_core_ops,
 	.pad = &vimc_sen_pad_ops,
 	.video = &vimc_sen_video_ops,
 };
-- 
2.15.1

[PATCHv2 02/15] vimc: use correct subdev functions

[Hans Verkuil]

Instead of calling everything a MEDIA_ENT_F_ATV_DECODER, pick the
correct functions for these blocks.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/vimc/vimc-debayer.c | 2 +-
 drivers/media/platform/vimc/vimc-scaler.c  | 2 +-
 drivers/media/platform/vimc/vimc-sensor.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index 4d663e89d33f..6e10b63ba9ec 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -533,7 +533,7 @@ static int vimc_deb_comp_bind(struct device *comp, struct device *master,
 	/* Initialize ved and sd */
 	ret = vimc_ent_sd_register(&vdeb->ved, &vdeb->sd, v4l2_dev,
 				   pdata->entity_name,
-				   MEDIA_ENT_F_ATV_DECODER, 2,
+				   MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2,
 				   (const unsigned long[2]) {MEDIA_PAD_FL_SINK,
 				   MEDIA_PAD_FL_SOURCE},
 				   &vimc_deb_ops);
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index e1602e0bc230..e583ec7a91da 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -395,7 +395,7 @@ static int vimc_sca_comp_bind(struct device *comp, struct device *master,
 	/* Initialize ved and sd */
 	ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev,
 				   pdata->entity_name,
-				   MEDIA_ENT_F_ATV_DECODER, 2,
+				   MEDIA_ENT_F_PROC_VIDEO_SCALER, 2,
 				   (const unsigned long[2]) {MEDIA_PAD_FL_SINK,
 				   MEDIA_PAD_FL_SOURCE},
 				   &vimc_sca_ops);
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 54184cd9e0ff..605e2a2d5dd5 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -386,7 +386,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
 	/* Initialize ved and sd */
 	ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev,
 				   pdata->entity_name,
-				   MEDIA_ENT_F_ATV_DECODER, 1,
+				   MEDIA_ENT_F_CAM_SENSOR, 1,
 				   (const unsigned long[1]) {MEDIA_PAD_FL_SOURCE},
 				   &vimc_sen_ops);
 	if (ret)
-- 
2.15.1

[PATCHv2 03/15] v4l2-ioctl.c: fix VIDIOC_DV_TIMINGS_CAP: don't clear pad

[Hans Verkuil]

The pad field should be passed on to the subdev driver, but it is cleared in
v4l2-ioctl.c so the subdev driver always sees a 0 pad.

Found with v4l2-compliance.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reported-by: Tim Harvey <tharvey@gateworks.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 260288ca4f55..f697c235f698 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2611,7 +2611,7 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
 	IOCTL_INFO_FNC(VIDIOC_PREPARE_BUF, v4l_prepare_buf, v4l_print_buffer, INFO_FL_QUEUE),
 	IOCTL_INFO_STD(VIDIOC_ENUM_DV_TIMINGS, vidioc_enum_dv_timings, v4l_print_enum_dv_timings, INFO_FL_CLEAR(v4l2_enum_dv_timings, pad)),
 	IOCTL_INFO_STD(VIDIOC_QUERY_DV_TIMINGS, vidioc_query_dv_timings, v4l_print_dv_timings, INFO_FL_ALWAYS_COPY),
-	IOCTL_INFO_STD(VIDIOC_DV_TIMINGS_CAP, vidioc_dv_timings_cap, v4l_print_dv_timings_cap, INFO_FL_CLEAR(v4l2_dv_timings_cap, type)),
+	IOCTL_INFO_STD(VIDIOC_DV_TIMINGS_CAP, vidioc_dv_timings_cap, v4l_print_dv_timings_cap, INFO_FL_CLEAR(v4l2_dv_timings_cap, pad)),
 	IOCTL_INFO_FNC(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, v4l_print_freq_band, 0),
 	IOCTL_INFO_FNC(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
 	IOCTL_INFO_FNC(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, id)),
-- 
2.15.1

[PATCHv2 04/15] v4l2-subdev: without controls return -ENOTTY

[Hans Verkuil]

If the subdev did not define any controls, then return -ENOTTY if
userspace attempts to call these ioctls.

The control framework functions will return -EINVAL, not -ENOTTY if
vfh->ctrl_handler is NULL.

Several of these framework functions are also called directly from
drivers, so I don't want to change the error code there.

Found with vimc and v4l2-compliance.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 43fefa73e0a3..be7a19272614 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -187,27 +187,43 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 
 	switch (cmd) {
 	case VIDIOC_QUERYCTRL:
+		if (!vfh->ctrl_handler)
+			return -ENOTTY;
 		return v4l2_queryctrl(vfh->ctrl_handler, arg);
 
 	case VIDIOC_QUERY_EXT_CTRL:
+		if (!vfh->ctrl_handler)
+			return -ENOTTY;
 		return v4l2_query_ext_ctrl(vfh->ctrl_handler, arg);
 
 	case VIDIOC_QUERYMENU:
+		if (!vfh->ctrl_handler)
+			return -ENOTTY;
 		return v4l2_querymenu(vfh->ctrl_handler, arg);
 
 	case VIDIOC_G_CTRL:
+		if (!vfh->ctrl_handler)
+			return -ENOTTY;
 		return v4l2_g_ctrl(vfh->ctrl_handler, arg);
 
 	case VIDIOC_S_CTRL:
+		if (!vfh->ctrl_handler)
+			return -ENOTTY;
 		return v4l2_s_ctrl(vfh, vfh->ctrl_handler, arg);
 
 	case VIDIOC_G_EXT_CTRLS:
+		if (!vfh->ctrl_handler)
+			return -ENOTTY;
 		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg);
 
 	case VIDIOC_S_EXT_CTRLS:
+		if (!vfh->ctrl_handler)
+			return -ENOTTY;
 		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
 
 	case VIDIOC_TRY_EXT_CTRLS:
+		if (!vfh->ctrl_handler)
+			return -ENOTTY;
 		return v4l2_try_ext_ctrls(vfh->ctrl_handler, arg);
 
 	case VIDIOC_DQEVENT:
-- 
2.15.1

[PATCHv2 05/15] v4l2-subdev: clear reserved fields

[Hans Verkuil]

Clear the reserved fields for these ioctls according to the specification:

VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL
VIDIOC_SUBDEV_ENUM_FRAME_SIZE
VIDIOC_SUBDEV_ENUM_MBUS_CODE
VIDIOC_SUBDEV_G_CROP, VIDIOC_SUBDEV_S_CROP
VIDIOC_SUBDEV_G_FMT, VIDIOC_SUBDEV_S_FMT
VIDIOC_SUBDEV_G_FRAME_INTERVAL, VIDIOC_SUBDEV_S_FRAME_INTERVAL
VIDIOC_SUBDEV_G_SELECTION, VIDIOC_SUBDEV_S_SELECTION

Found with v4l2-compliance.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index be7a19272614..6cabfa32d2ed 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -276,6 +276,8 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		if (rval)
 			return rval;
 
+		memset(format->reserved, 0, sizeof(format->reserved));
+		memset(format->format.reserved, 0, sizeof(format->format.reserved));
 		return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format);
 	}
 
@@ -286,6 +288,8 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		if (rval)
 			return rval;
 
+		memset(format->reserved, 0, sizeof(format->reserved));
+		memset(format->format.reserved, 0, sizeof(format->format.reserved));
 		return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
 	}
 
@@ -293,6 +297,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		struct v4l2_subdev_crop *crop = arg;
 		struct v4l2_subdev_selection sel;
 
+		memset(crop->reserved, 0, sizeof(crop->reserved));
 		rval = check_crop(sd, crop);
 		if (rval)
 			return rval;
@@ -318,6 +323,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		if (rval)
 			return rval;
 
+		memset(crop->reserved, 0, sizeof(crop->reserved));
 		memset(&sel, 0, sizeof(sel));
 		sel.which = crop->which;
 		sel.pad = crop->pad;
@@ -342,6 +348,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		if (code->pad >= sd->entity.num_pads)
 			return -EINVAL;
 
+		memset(code->reserved, 0, sizeof(code->reserved));
 		return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad,
 					code);
 	}
@@ -356,6 +363,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		if (fse->pad >= sd->entity.num_pads)
 			return -EINVAL;
 
+		memset(fse->reserved, 0, sizeof(fse->reserved));
 		return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad,
 					fse);
 	}
@@ -366,6 +374,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		if (fi->pad >= sd->entity.num_pads)
 			return -EINVAL;
 
+		memset(fi->reserved, 0, sizeof(fi->reserved));
 		return v4l2_subdev_call(sd, video, g_frame_interval, arg);
 	}
 
@@ -375,6 +384,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		if (fi->pad >= sd->entity.num_pads)
 			return -EINVAL;
 
+		memset(fi->reserved, 0, sizeof(fi->reserved));
 		return v4l2_subdev_call(sd, video, s_frame_interval, arg);
 	}
 
@@ -388,6 +398,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		if (fie->pad >= sd->entity.num_pads)
 			return -EINVAL;
 
+		memset(fie->reserved, 0, sizeof(fie->reserved));
 		return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad,
 					fie);
 	}
@@ -399,6 +410,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		if (rval)
 			return rval;
 
+		memset(sel->reserved, 0, sizeof(sel->reserved));
 		return v4l2_subdev_call(
 			sd, pad, get_selection, subdev_fh->pad, sel);
 	}
@@ -410,6 +422,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		if (rval)
 			return rval;
 
+		memset(sel->reserved, 0, sizeof(sel->reserved));
 		return v4l2_subdev_call(
 			sd, pad, set_selection, subdev_fh->pad, sel);
 	}
-- 
2.15.1

[PATCHv2 06/15] v4l2-subdev: implement VIDIOC_DBG_G_CHIP_INFO ioctl

[Hans Verkuil]

The VIDIOC_DBG_G/S_REGISTER ioctls imply that VIDIOC_DBG_G_CHIP_INFO is also
present, since without that you cannot use v4l2-dbg.

Just like the implementation in v4l2-ioctl.c this can be implemented in the
core and no drivers need to be modified.

It also makes it possible for v4l2-compliance to properly test the
VIDIOC_DBG_G/S_REGISTER ioctls.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 6cabfa32d2ed..2a5b5a3fa7a3 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -255,6 +255,19 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 			return -EPERM;
 		return v4l2_subdev_call(sd, core, s_register, p);
 	}
+	case VIDIOC_DBG_G_CHIP_INFO:
+	{
+		struct v4l2_dbg_chip_info *p = arg;
+
+		if (p->match.type != V4L2_CHIP_MATCH_SUBDEV || p->match.addr)
+			return -EINVAL;
+		if (sd->ops->core && sd->ops->core->s_register)
+			p->flags |= V4L2_CHIP_FL_WRITABLE;
+		if (sd->ops->core && sd->ops->core->g_register)
+			p->flags |= V4L2_CHIP_FL_READABLE;
+		strlcpy(p->name, sd->name, sizeof(p->name));
+		return 0;
+	}
 #endif
 
 	case VIDIOC_LOG_STATUS: {
-- 
2.15.1

[PATCHv2 07/15] subdev-formats.rst: fix incorrect types

[Hans Verkuil]

The ycbcr_enc, quantization and xfer_func fields are __u16 and not enums.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/media/uapi/v4l/subdev-formats.rst | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/media/uapi/v4l/subdev-formats.rst b/Documentation/media/uapi/v4l/subdev-formats.rst
index b1eea44550e1..4f0c0b282f98 100644
--- a/Documentation/media/uapi/v4l/subdev-formats.rst
+++ b/Documentation/media/uapi/v4l/subdev-formats.rst
@@ -33,17 +33,17 @@ Media Bus Formats
       - Image colorspace, from enum
 	:c:type:`v4l2_colorspace`. See
 	:ref:`colorspaces` for details.
-    * - enum :c:type:`v4l2_ycbcr_encoding`
+    * - __u16
       - ``ycbcr_enc``
       - This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
 	streams, see :ref:`colorspaces`.
-    * - enum :c:type:`v4l2_quantization`
+    * - __u16
       - ``quantization``
       - This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
 	streams, see :ref:`colorspaces`.
-    * - enum :c:type:`v4l2_xfer_func`
+    * - __u16
       - ``xfer_func``
       - This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
-- 
2.15.1

[PATCHv2 08/15] media-ioc-g-topology.rst: fix interface-to-entity link description

[Hans Verkuil]

The source_id and sink_id descriptions were the same for interface-to-entity
links. The source_id is the interface ID, not the entity ID. Fix this.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/media/uapi/mediactl/media-ioc-g-topology.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
index 997e6b17440d..870a6c0d1f7a 100644
--- a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
+++ b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
@@ -334,7 +334,7 @@ desired arrays with the media graph elements.
 
        -  On pad to pad links: unique ID for the source pad.
 
-	  On interface to entity links: unique ID for the entity.
+	  On interface to entity links: unique ID for the interface.
 
     -  .. row 3
 
-- 
2.15.1

[PATCHv2 09/15] media-types.rst: fix type, small improvements

[Hans Verkuil]

data conector -> connector

... -> etc.

'...' looked odd when my browser put the ... by itself on the next line, 'etc.'
is clearer IMHO.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/media/uapi/mediactl/media-types.rst | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/media/uapi/mediactl/media-types.rst b/Documentation/media/uapi/mediactl/media-types.rst
index 8d64b0c06ebc..9c1e3d3f590c 100644
--- a/Documentation/media/uapi/mediactl/media-types.rst
+++ b/Documentation/media/uapi/mediactl/media-types.rst
@@ -293,7 +293,7 @@ Types and flags used to represent the media graph elements
 
        -  ``MEDIA_ENT_F_PROC_VIDEO_STATISTICS``
 
-       -  Video statistics computation (histogram, 3A, ...). An entity
+       -  Video statistics computation (histogram, 3A, etc.). An entity
 	  capable of statistics computation must have one sink pad and
 	  one source pad. It computes statistics over the frames
 	  received on its sink pad and outputs the statistics data on
@@ -318,8 +318,8 @@ Types and flags used to represent the media graph elements
        - Video interface bridge. A video interface bridge entity must have at
          least one sink pad and at least one source pad. It receives video
          frames on its sink pad from an input video bus of one type (HDMI, eDP,
-         MIPI CSI-2, ...), and outputs them on its source pad to an output
-         video bus of another type (eDP, MIPI CSI-2, parallel, ...).
+         MIPI CSI-2, etc.), and outputs them on its source pad to an output
+         video bus of another type (eDP, MIPI CSI-2, parallel, etc.).
 
 ..  tabularcolumns:: |p{5.5cm}|p{12.0cm}|
 
@@ -337,7 +337,7 @@ Types and flags used to represent the media graph elements
        -  ``MEDIA_ENT_FL_DEFAULT``
 
        -  Default entity for its type. Used to discover the default audio,
-	  VBI and video devices, the default camera sensor, ...
+	  VBI and video devices, the default camera sensor, etc.
 
     -  .. row 2
 
@@ -345,7 +345,7 @@ Types and flags used to represent the media graph elements
 
        -  ``MEDIA_ENT_FL_CONNECTOR``
 
-       -  The entity represents a data conector
+       -  The entity represents a connector.
 
 
 ..  tabularcolumns:: |p{6.5cm}|p{6.0cm}|p{5.0cm}|
-- 
2.15.1

[PATCHv2 10/15] media: media-types.rst: fix typo

[Hans Verkuil]

From: Alexandre Courbot <acourbot@chromium.org>

with -> which

Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 Documentation/media/uapi/mediactl/media-types.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/media/uapi/mediactl/media-types.rst b/Documentation/media/uapi/mediactl/media-types.rst
index 9c1e3d3f590c..01b0b60771dd 100644
--- a/Documentation/media/uapi/mediactl/media-types.rst
+++ b/Documentation/media/uapi/mediactl/media-types.rst
@@ -26,7 +26,7 @@ Types and flags used to represent the media graph elements
 	  ``MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN``
 
        -  Unknown entity. That generally indicates that a driver didn't
-	  initialize properly the entity, with is a Kernel bug
+	  initialize properly the entity, which is a Kernel bug
 
     -  .. row 2
 
-- 
2.15.1

[PATCHv2 11/15] media-device.c: zero reserved field

[Hans Verkuil]

MEDIA_IOC_SETUP_LINK didn't zero the reserved field of the media_link_desc
struct. Do so in media_device_setup_link().

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-device.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index e79f72b8b858..afbf23a19e16 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -218,6 +218,8 @@ static long media_device_setup_link(struct media_device *mdev,
 	if (link == NULL)
 		return -EINVAL;
 
+	memset(linkd->reserved, 0, sizeof(linkd->reserved));
+
 	/* Setup the link on both entities. */
 	return __media_entity_setup_link(link, linkd->flags);
 }
-- 
2.15.1

[PATCHv2 12/15] media.h: fix confusing typo in comment

[Hans Verkuil]

Subdevs are initialized with MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN, not
MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 include/uapi/linux/media.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index b9b9446095e9..573da38a21c3 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -131,7 +131,7 @@ struct media_device_info {
  * with the legacy v1 API.The number range is out of range by purpose:
  * several previously reserved numbers got excluded from this range.
  *
- * Subdevs are initialized with MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN,
+ * Subdevs are initialized with MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN,
  * in order to preserve backward compatibility.
  * Drivers must change to the proper subdev type before
  * registering the entity.
-- 
2.15.1

[PATCHv2 13/15] media: document and zero reservedX fields in media_v2_topology

[Hans Verkuil]

The MEDIA_IOC_G_TOPOLOGY documentation didn't document the reservedX fields.
Related to that was that the documented type of the num_* fields was also
wrong.

The reservedX fields were not set to 0, that is now also fixed.

Found with v4l2-compliance.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 .../media/uapi/mediactl/media-ioc-g-topology.rst   | 52 +++++++++++++++++-----
 drivers/media/media-device.c                       |  4 ++
 2 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
index 870a6c0d1f7a..c8f9ea37db2d 100644
--- a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
+++ b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
@@ -68,7 +68,7 @@ desired arrays with the media graph elements.
 
     -  .. row 2
 
-       -  __u64
+       -  __u32
 
        -  ``num_entities``
 
@@ -76,6 +76,14 @@ desired arrays with the media graph elements.
 
     -  .. row 3
 
+       -  __u32
+
+       -  ``reserved1``
+
+       -  Applications and drivers shall set this to 0.
+
+    -  .. row 4
+
        -  __u64
 
        -  ``ptr_entities``
@@ -85,15 +93,23 @@ desired arrays with the media graph elements.
 	  the ioctl won't store the entities. It will just update
 	  ``num_entities``
 
-    -  .. row 4
+    -  .. row 5
 
-       -  __u64
+       -  __u32
 
        -  ``num_interfaces``
 
        -  Number of interfaces in the graph
 
-    -  .. row 5
+    -  .. row 6
+
+       -  __u32
+
+       -  ``reserved2``
+
+       -  Applications and drivers shall set this to 0.
+
+    -  .. row 7
 
        -  __u64
 
@@ -104,15 +120,23 @@ desired arrays with the media graph elements.
 	  the ioctl won't store the interfaces. It will just update
 	  ``num_interfaces``
 
-    -  .. row 6
+    -  .. row 8
 
-       -  __u64
+       -  __u32
 
        -  ``num_pads``
 
        -  Total number of pads in the graph
 
-    -  .. row 7
+    -  .. row 9
+
+       -  __u32
+
+       -  ``reserved3``
+
+       -  Applications and drivers shall set this to 0.
+
+    -  .. row 10
 
        -  __u64
 
@@ -122,15 +146,23 @@ desired arrays with the media graph elements.
 	  converted to a 64-bits integer. It can be zero. if zero, the ioctl
 	  won't store the pads. It will just update ``num_pads``
 
-    -  .. row 8
+    -  .. row 11
 
-       -  __u64
+       -  __u32
 
        -  ``num_links``
 
        -  Total number of data and interface links in the graph
 
-    -  .. row 9
+    -  .. row 12
+
+       -  __u32
+
+       -  ``reserved4``
+
+       -  Applications and drivers shall set this to 0.
+
+    -  .. row 13
 
        -  __u64
 
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index afbf23a19e16..d0e3f61dde52 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -265,6 +265,7 @@ static long media_device_get_topology(struct media_device *mdev,
 		uentity++;
 	}
 	topo->num_entities = i;
+	topo->reserved1 = 0;
 
 	/* Get interfaces and number of interfaces */
 	i = 0;
@@ -300,6 +301,7 @@ static long media_device_get_topology(struct media_device *mdev,
 		uintf++;
 	}
 	topo->num_interfaces = i;
+	topo->reserved2 = 0;
 
 	/* Get pads and number of pads */
 	i = 0;
@@ -326,6 +328,7 @@ static long media_device_get_topology(struct media_device *mdev,
 		upad++;
 	}
 	topo->num_pads = i;
+	topo->reserved3 = 0;
 
 	/* Get links and number of links */
 	i = 0;
@@ -357,6 +360,7 @@ static long media_device_get_topology(struct media_device *mdev,
 		ulink++;
 	}
 	topo->num_links = i;
+	topo->reserved4 = 0;
 
 	return ret;
 }
-- 
2.15.1

[PATCHv2 14/15] media-ioc-enum-entities/links.rst: document reserved fields

[Hans Verkuil]

These structures have reserved fields that were never documented.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 .../media/uapi/mediactl/media-ioc-enum-entities.rst   | 19 +++++++++++++++----
 .../media/uapi/mediactl/media-ioc-enum-links.rst      | 18 ++++++++++++++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst b/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst
index b59ce149efb5..45e76e5bc1ea 100644
--- a/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst
+++ b/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst
@@ -144,10 +144,21 @@ id's until they get an error.
 
     -  .. row 9
 
-       -  union
+       -  __u32
+
+       -  ``reserved[4]``
+
+       -
+       -
+       -  Reserved for future extensions. Drivers and applications must set
+          the array to zero.
 
     -  .. row 10
 
+       -  union
+
+    -  .. row 11
+
        -
        -  struct
 
@@ -156,7 +167,7 @@ id's until they get an error.
        -
        -  Valid for (sub-)devices that create a single device node.
 
-    -  .. row 11
+    -  .. row 12
 
        -
        -
@@ -166,7 +177,7 @@ id's until they get an error.
 
        -  Device node major number.
 
-    -  .. row 12
+    -  .. row 13
 
        -
        -
@@ -176,7 +187,7 @@ id's until they get an error.
 
        -  Device node minor number.
 
-    -  .. row 13
+    -  .. row 14
 
        -
        -  __u8
diff --git a/Documentation/media/uapi/mediactl/media-ioc-enum-links.rst b/Documentation/media/uapi/mediactl/media-ioc-enum-links.rst
index d05be16ffaf6..256168b3c3be 100644
--- a/Documentation/media/uapi/mediactl/media-ioc-enum-links.rst
+++ b/Documentation/media/uapi/mediactl/media-ioc-enum-links.rst
@@ -125,6 +125,15 @@ returned during the enumeration process.
 
        -  Pad flags, see :ref:`media-pad-flag` for more details.
 
+    -  .. row 4
+
+       -  __u32
+
+       -  ``reserved[2]``
+
+       -  Reserved for future extensions. Drivers and applications must set
+          the array to zero.
+
 
 
 .. c:type:: media_link_desc
@@ -161,6 +170,15 @@ returned during the enumeration process.
 
        -  Link flags, see :ref:`media-link-flag` for more details.
 
+    -  .. row 4
+
+       -  __u32
+
+       -  ``reserved[4]``
+
+       -  Reserved for future extensions. Drivers and applications must set
+          the array to zero.
+
 
 Return Value
 ============
-- 
2.15.1

[PATCHv2 15/15] media.h: reorganize header to make it easier to understand

[Hans Verkuil]

The media.h public header is very messy. It mixes legacy and 'new' defines
and it is not easy to figure out what should and what shouldn't be used. It
also contains confusing comment that are either out of date or completely
uninteresting for anyone that needs to use this header.

The patch groups all entity functions together, including the 'old' defines
based on the old range base. The reader just wants to know about the available
functions and doesn't care about what range is used.

All legacy defines are moved to the end of the header, so it is easier to
locate them and just ignore them.

The legacy structs in the struct media_entity_desc are put under
#if !defined(__KERNEL__) to prevent the kernel from using them, and this is
also a much more effective signal to the reader that they shouldn't be used
compared to the old method of relying on '#if 1' followed by a comment.

The unused MEDIA_INTF_T_ALSA_* defines are also moved to the end of the header
in the legacy area. They are also dropped from intf_type() in media-entity.c.

All defines are also aligned at the same tab making the header easier to read.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-entity.c |  16 --
 include/uapi/linux/media.h   | 345 +++++++++++++++++++++----------------------
 2 files changed, 166 insertions(+), 195 deletions(-)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index f7c6d64e6031..3498551e618e 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -64,22 +64,6 @@ static inline const char *intf_type(struct media_interface *intf)
 		return "v4l-swradio";
 	case MEDIA_INTF_T_V4L_TOUCH:
 		return "v4l-touch";
-	case MEDIA_INTF_T_ALSA_PCM_CAPTURE:
-		return "alsa-pcm-capture";
-	case MEDIA_INTF_T_ALSA_PCM_PLAYBACK:
-		return "alsa-pcm-playback";
-	case MEDIA_INTF_T_ALSA_CONTROL:
-		return "alsa-control";
-	case MEDIA_INTF_T_ALSA_COMPRESS:
-		return "alsa-compress";
-	case MEDIA_INTF_T_ALSA_RAWMIDI:
-		return "alsa-rawmidi";
-	case MEDIA_INTF_T_ALSA_HWDEP:
-		return "alsa-hwdep";
-	case MEDIA_INTF_T_ALSA_SEQUENCER:
-		return "alsa-sequencer";
-	case MEDIA_INTF_T_ALSA_TIMER:
-		return "alsa-timer";
 	default:
 		return "unknown-intf";
 	}
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 573da38a21c3..febe28054579 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -15,10 +15,6 @@
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
 #ifndef __LINUX_MEDIA_H
@@ -42,108 +38,65 @@ struct media_device_info {
 	__u32 reserved[31];
 };
 
-#define MEDIA_ENT_ID_FLAG_NEXT		(1 << 31)
-
-/*
- * Initial value to be used when a new entity is created
- * Drivers should change it to something useful
- */
-#define MEDIA_ENT_F_UNKNOWN	0x00000000
-
 /*
  * Base number ranges for entity functions
  *
- * NOTE: those ranges and entity function number are phased just to
- * make it easier to maintain this file. Userspace should not rely on
- * the ranges to identify a group of function types, as newer
- * functions can be added with any name within the full u32 range.
+ * NOTE: Userspace should not rely on these ranges to identify a group
+ * of function types, as newer functions can be added with any name within
+ * the full u32 range.
+ *
+ * Some older functions use the MEDIA_ENT_F_OLD_*_BASE range. Do not
+ * changes this, this is for backwards compatibility. When adding new
+ * functions always use MEDIA_ENT_F_BASE.
  */
-#define MEDIA_ENT_F_BASE		0x00000000
-#define MEDIA_ENT_F_OLD_BASE		0x00010000
-#define MEDIA_ENT_F_OLD_SUBDEV_BASE	0x00020000
+#define MEDIA_ENT_F_BASE			0x00000000
+#define MEDIA_ENT_F_OLD_BASE			0x00010000
+#define MEDIA_ENT_F_OLD_SUBDEV_BASE		0x00020000
 
 /*
- * DVB entities
+ * Initial value to be used when a new entity is created
+ * Drivers should change it to something useful.
  */
-#define MEDIA_ENT_F_DTV_DEMOD		(MEDIA_ENT_F_BASE + 0x00001)
-#define MEDIA_ENT_F_TS_DEMUX		(MEDIA_ENT_F_BASE + 0x00002)
-#define MEDIA_ENT_F_DTV_CA		(MEDIA_ENT_F_BASE + 0x00003)
-#define MEDIA_ENT_F_DTV_NET_DECAP	(MEDIA_ENT_F_BASE + 0x00004)
+#define MEDIA_ENT_F_UNKNOWN			MEDIA_ENT_F_BASE
 
 /*
- * I/O entities
+ * Subdevs are initialized with MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN in order
+ * to preserve backward compatibility. Drivers must change to the proper
+ * subdev type before registering the entity.
  */
-#define MEDIA_ENT_F_IO_DTV		(MEDIA_ENT_F_BASE + 0x01001)
-#define MEDIA_ENT_F_IO_VBI		(MEDIA_ENT_F_BASE + 0x01002)
-#define MEDIA_ENT_F_IO_SWRADIO		(MEDIA_ENT_F_BASE + 0x01003)
+#define MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN		MEDIA_ENT_F_OLD_SUBDEV_BASE
 
 /*
- * Analog TV IF-PLL decoders
- *
- * It is a responsibility of the master/bridge drivers to create links
- * for MEDIA_ENT_F_IF_VID_DECODER and MEDIA_ENT_F_IF_AUD_DECODER.
+ * DVB entity functions
  */
-#define MEDIA_ENT_F_IF_VID_DECODER	(MEDIA_ENT_F_BASE + 0x02001)
-#define MEDIA_ENT_F_IF_AUD_DECODER	(MEDIA_ENT_F_BASE + 0x02002)
+#define MEDIA_ENT_F_DTV_DEMOD			(MEDIA_ENT_F_BASE + 0x00001)
+#define MEDIA_ENT_F_TS_DEMUX			(MEDIA_ENT_F_BASE + 0x00002)
+#define MEDIA_ENT_F_DTV_CA			(MEDIA_ENT_F_BASE + 0x00003)
+#define MEDIA_ENT_F_DTV_NET_DECAP		(MEDIA_ENT_F_BASE + 0x00004)
 
 /*
- * Audio Entity Functions
+ * I/O entity functions
  */
-#define MEDIA_ENT_F_AUDIO_CAPTURE	(MEDIA_ENT_F_BASE + 0x03001)
-#define MEDIA_ENT_F_AUDIO_PLAYBACK	(MEDIA_ENT_F_BASE + 0x03002)
-#define MEDIA_ENT_F_AUDIO_MIXER		(MEDIA_ENT_F_BASE + 0x03003)
+#define MEDIA_ENT_F_IO_V4L  			(MEDIA_ENT_F_OLD_BASE + 1)
+#define MEDIA_ENT_F_IO_DTV			(MEDIA_ENT_F_BASE + 0x01001)
+#define MEDIA_ENT_F_IO_VBI			(MEDIA_ENT_F_BASE + 0x01002)
+#define MEDIA_ENT_F_IO_SWRADIO			(MEDIA_ENT_F_BASE + 0x01003)
 
 /*
- * Processing entities
+ * Sensor functions
  */
-#define MEDIA_ENT_F_PROC_VIDEO_COMPOSER		(MEDIA_ENT_F_BASE + 0x4001)
-#define MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER	(MEDIA_ENT_F_BASE + 0x4002)
-#define MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV	(MEDIA_ENT_F_BASE + 0x4003)
-#define MEDIA_ENT_F_PROC_VIDEO_LUT		(MEDIA_ENT_F_BASE + 0x4004)
-#define MEDIA_ENT_F_PROC_VIDEO_SCALER		(MEDIA_ENT_F_BASE + 0x4005)
-#define MEDIA_ENT_F_PROC_VIDEO_STATISTICS	(MEDIA_ENT_F_BASE + 0x4006)
+#define MEDIA_ENT_F_CAM_SENSOR			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 1)
+#define MEDIA_ENT_F_FLASH			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 2)
+#define MEDIA_ENT_F_LENS			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 3)
 
 /*
- * Switch and bridge entitites
+ * Analog video decoder functions
  */
-#define MEDIA_ENT_F_VID_MUX			(MEDIA_ENT_F_BASE + 0x5001)
-#define MEDIA_ENT_F_VID_IF_BRIDGE		(MEDIA_ENT_F_BASE + 0x5002)
+#define MEDIA_ENT_F_ATV_DECODER			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 4)
 
 /*
- * Connectors
- */
-/* It is a responsibility of the entity drivers to add connectors and links */
-#ifdef __KERNEL__
-	/*
-	 * For now, it should not be used in userspace, as some
-	 * definitions may change
-	 */
-
-#define MEDIA_ENT_F_CONN_RF		(MEDIA_ENT_F_BASE + 0x30001)
-#define MEDIA_ENT_F_CONN_SVIDEO		(MEDIA_ENT_F_BASE + 0x30002)
-#define MEDIA_ENT_F_CONN_COMPOSITE	(MEDIA_ENT_F_BASE + 0x30003)
-
-#endif
-
-/*
- * Don't touch on those. The ranges MEDIA_ENT_F_OLD_BASE and
- * MEDIA_ENT_F_OLD_SUBDEV_BASE are kept to keep backward compatibility
- * with the legacy v1 API.The number range is out of range by purpose:
- * several previously reserved numbers got excluded from this range.
+ * Digital TV, analog TV, radio and/or software defined radio tuner functions.
  *
- * Subdevs are initialized with MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN,
- * in order to preserve backward compatibility.
- * Drivers must change to the proper subdev type before
- * registering the entity.
- */
-
-#define MEDIA_ENT_F_IO_V4L  		(MEDIA_ENT_F_OLD_BASE + 1)
-
-#define MEDIA_ENT_F_CAM_SENSOR		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 1)
-#define MEDIA_ENT_F_FLASH		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 2)
-#define MEDIA_ENT_F_LENS		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 3)
-#define MEDIA_ENT_F_ATV_DECODER		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 4)
-/*
  * It is a responsibility of the master/bridge drivers to add connectors
  * and links for MEDIA_ENT_F_TUNER. Please notice that some old tuners
  * may require the usage of separate I2C chips to decode analog TV signals,
@@ -151,49 +104,46 @@ struct media_device_info {
  * On such cases, the IF-PLL staging is mapped via one or two entities:
  * MEDIA_ENT_F_IF_VID_DECODER and/or MEDIA_ENT_F_IF_AUD_DECODER.
  */
-#define MEDIA_ENT_F_TUNER		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 5)
+#define MEDIA_ENT_F_TUNER			(MEDIA_ENT_F_OLD_SUBDEV_BASE + 5)
 
-#define MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN	MEDIA_ENT_F_OLD_SUBDEV_BASE
+/*
+ * Analog TV IF-PLL decoder functions
+ *
+ * It is a responsibility of the master/bridge drivers to create links
+ * for MEDIA_ENT_F_IF_VID_DECODER and MEDIA_ENT_F_IF_AUD_DECODER.
+ */
+#define MEDIA_ENT_F_IF_VID_DECODER		(MEDIA_ENT_F_BASE + 0x02001)
+#define MEDIA_ENT_F_IF_AUD_DECODER		(MEDIA_ENT_F_BASE + 0x02002)
 
-#if !defined(__KERNEL__) || defined(__NEED_MEDIA_LEGACY_API)
+/*
+ * Audio entity functions
+ */
+#define MEDIA_ENT_F_AUDIO_CAPTURE		(MEDIA_ENT_F_BASE + 0x03001)
+#define MEDIA_ENT_F_AUDIO_PLAYBACK		(MEDIA_ENT_F_BASE + 0x03002)
+#define MEDIA_ENT_F_AUDIO_MIXER			(MEDIA_ENT_F_BASE + 0x03003)
 
 /*
- * Legacy symbols used to avoid userspace compilation breakages
- *
- * Those symbols map the entity function into types and should be
- * used only on legacy programs for legacy hardware. Don't rely
- * on those for MEDIA_IOC_G_TOPOLOGY.
+ * Processing entity functions
  */
-#define MEDIA_ENT_TYPE_SHIFT		16
-#define MEDIA_ENT_TYPE_MASK		0x00ff0000
-#define MEDIA_ENT_SUBTYPE_MASK		0x0000ffff
-
-/* End of the old subdev reserved numberspace */
-#define MEDIA_ENT_T_DEVNODE_UNKNOWN	(MEDIA_ENT_T_DEVNODE | \
-					 MEDIA_ENT_SUBTYPE_MASK)
-
-#define MEDIA_ENT_T_DEVNODE		MEDIA_ENT_F_OLD_BASE
-#define MEDIA_ENT_T_DEVNODE_V4L		MEDIA_ENT_F_IO_V4L
-#define MEDIA_ENT_T_DEVNODE_FB		(MEDIA_ENT_T_DEVNODE + 2)
-#define MEDIA_ENT_T_DEVNODE_ALSA	(MEDIA_ENT_T_DEVNODE + 3)
-#define MEDIA_ENT_T_DEVNODE_DVB		(MEDIA_ENT_T_DEVNODE + 4)
-
-#define MEDIA_ENT_T_UNKNOWN		MEDIA_ENT_F_UNKNOWN
-#define MEDIA_ENT_T_V4L2_VIDEO		MEDIA_ENT_F_IO_V4L
-#define MEDIA_ENT_T_V4L2_SUBDEV		MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN
-#define MEDIA_ENT_T_V4L2_SUBDEV_SENSOR	MEDIA_ENT_F_CAM_SENSOR
-#define MEDIA_ENT_T_V4L2_SUBDEV_FLASH	MEDIA_ENT_F_FLASH
-#define MEDIA_ENT_T_V4L2_SUBDEV_LENS	MEDIA_ENT_F_LENS
-#define MEDIA_ENT_T_V4L2_SUBDEV_DECODER	MEDIA_ENT_F_ATV_DECODER
-#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER	MEDIA_ENT_F_TUNER
+#define MEDIA_ENT_F_PROC_VIDEO_COMPOSER		(MEDIA_ENT_F_BASE + 0x4001)
+#define MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER	(MEDIA_ENT_F_BASE + 0x4002)
+#define MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV	(MEDIA_ENT_F_BASE + 0x4003)
+#define MEDIA_ENT_F_PROC_VIDEO_LUT		(MEDIA_ENT_F_BASE + 0x4004)
+#define MEDIA_ENT_F_PROC_VIDEO_SCALER		(MEDIA_ENT_F_BASE + 0x4005)
+#define MEDIA_ENT_F_PROC_VIDEO_STATISTICS	(MEDIA_ENT_F_BASE + 0x4006)
 
-/* Obsolete symbol for media_version, no longer used in the kernel */
-#define MEDIA_API_VERSION		KERNEL_VERSION(0, 1, 0)
-#endif
+/*
+ * Switch and bridge entity functions
+ */
+#define MEDIA_ENT_F_VID_MUX			(MEDIA_ENT_F_BASE + 0x5001)
+#define MEDIA_ENT_F_VID_IF_BRIDGE		(MEDIA_ENT_F_BASE + 0x5002)
 
 /* Entity flags */
-#define MEDIA_ENT_FL_DEFAULT		(1 << 0)
-#define MEDIA_ENT_FL_CONNECTOR		(1 << 1)
+#define MEDIA_ENT_FL_DEFAULT			(1 << 0)
+#define MEDIA_ENT_FL_CONNECTOR			(1 << 1)
+
+/* OR with the entity id value to find the next entity */
+#define MEDIA_ENT_ID_FLAG_NEXT			(1 << 31)
 
 struct media_entity_desc {
 	__u32 id;
@@ -214,7 +164,7 @@ struct media_entity_desc {
 			__u32 minor;
 		} dev;
 
-#if 1
+#if !defined(__KERNEL__)
 		/*
 		 * TODO: this shouldn't have been added without
 		 * actual drivers that use this. When the first real driver
@@ -225,24 +175,17 @@ struct media_entity_desc {
 		 * contain the subdevice information. In addition, struct dev
 		 * can only refer to a single device, and not to multiple (e.g.
 		 * pcm and mixer devices).
-		 *
-		 * So for now mark this as a to do.
 		 */
 		struct {
 			__u32 card;
 			__u32 device;
 			__u32 subdevice;
 		} alsa;
-#endif
 
-#if 1
 		/*
 		 * DEPRECATED: previous node specifications. Kept just to
-		 * avoid breaking compilation, but media_entity_desc.dev
-		 * should be used instead. In particular, alsa and dvb
-		 * fields below are wrong: for all devnodes, there should
-		 * be just major/minor inside the struct, as this is enough
-		 * to represent any devnode, no matter what type.
+		 * avoid breaking compilation. Use media_entity_desc.dev
+		 * instead.
 		 */
 		struct {
 			__u32 major;
@@ -261,9 +204,9 @@ struct media_entity_desc {
 	};
 };
 
-#define MEDIA_PAD_FL_SINK		(1 << 0)
-#define MEDIA_PAD_FL_SOURCE		(1 << 1)
-#define MEDIA_PAD_FL_MUST_CONNECT	(1 << 2)
+#define MEDIA_PAD_FL_SINK			(1 << 0)
+#define MEDIA_PAD_FL_SOURCE			(1 << 1)
+#define MEDIA_PAD_FL_MUST_CONNECT		(1 << 2)
 
 struct media_pad_desc {
 	__u32 entity;		/* entity ID */
@@ -272,13 +215,13 @@ struct media_pad_desc {
 	__u32 reserved[2];
 };
 
-#define MEDIA_LNK_FL_ENABLED		(1 << 0)
-#define MEDIA_LNK_FL_IMMUTABLE		(1 << 1)
-#define MEDIA_LNK_FL_DYNAMIC		(1 << 2)
+#define MEDIA_LNK_FL_ENABLED			(1 << 0)
+#define MEDIA_LNK_FL_IMMUTABLE			(1 << 1)
+#define MEDIA_LNK_FL_DYNAMIC			(1 << 2)
 
-#define MEDIA_LNK_FL_LINK_TYPE		(0xf << 28)
-#  define MEDIA_LNK_FL_DATA_LINK	(0 << 28)
-#  define MEDIA_LNK_FL_INTERFACE_LINK	(1 << 28)
+#define MEDIA_LNK_FL_LINK_TYPE			(0xf << 28)
+#  define MEDIA_LNK_FL_DATA_LINK		(0 << 28)
+#  define MEDIA_LNK_FL_INTERFACE_LINK		(1 << 28)
 
 struct media_link_desc {
 	struct media_pad_desc source;
@@ -298,57 +241,47 @@ struct media_links_enum {
 
 /* Interface type ranges */
 
-#define MEDIA_INTF_T_DVB_BASE	0x00000100
-#define MEDIA_INTF_T_V4L_BASE	0x00000200
-#define MEDIA_INTF_T_ALSA_BASE	0x00000300
+#define MEDIA_INTF_T_DVB_BASE			0x00000100
+#define MEDIA_INTF_T_V4L_BASE			0x00000200
 
 /* Interface types */
 
-#define MEDIA_INTF_T_DVB_FE    	(MEDIA_INTF_T_DVB_BASE)
-#define MEDIA_INTF_T_DVB_DEMUX  (MEDIA_INTF_T_DVB_BASE + 1)
-#define MEDIA_INTF_T_DVB_DVR    (MEDIA_INTF_T_DVB_BASE + 2)
-#define MEDIA_INTF_T_DVB_CA     (MEDIA_INTF_T_DVB_BASE + 3)
-#define MEDIA_INTF_T_DVB_NET    (MEDIA_INTF_T_DVB_BASE + 4)
-
-#define MEDIA_INTF_T_V4L_VIDEO  (MEDIA_INTF_T_V4L_BASE)
-#define MEDIA_INTF_T_V4L_VBI    (MEDIA_INTF_T_V4L_BASE + 1)
-#define MEDIA_INTF_T_V4L_RADIO  (MEDIA_INTF_T_V4L_BASE + 2)
-#define MEDIA_INTF_T_V4L_SUBDEV (MEDIA_INTF_T_V4L_BASE + 3)
-#define MEDIA_INTF_T_V4L_SWRADIO (MEDIA_INTF_T_V4L_BASE + 4)
-#define MEDIA_INTF_T_V4L_TOUCH	(MEDIA_INTF_T_V4L_BASE + 5)
-
-#define MEDIA_INTF_T_ALSA_PCM_CAPTURE   (MEDIA_INTF_T_ALSA_BASE)
-#define MEDIA_INTF_T_ALSA_PCM_PLAYBACK  (MEDIA_INTF_T_ALSA_BASE + 1)
-#define MEDIA_INTF_T_ALSA_CONTROL       (MEDIA_INTF_T_ALSA_BASE + 2)
-#define MEDIA_INTF_T_ALSA_COMPRESS      (MEDIA_INTF_T_ALSA_BASE + 3)
-#define MEDIA_INTF_T_ALSA_RAWMIDI       (MEDIA_INTF_T_ALSA_BASE + 4)
-#define MEDIA_INTF_T_ALSA_HWDEP         (MEDIA_INTF_T_ALSA_BASE + 5)
-#define MEDIA_INTF_T_ALSA_SEQUENCER     (MEDIA_INTF_T_ALSA_BASE + 6)
-#define MEDIA_INTF_T_ALSA_TIMER         (MEDIA_INTF_T_ALSA_BASE + 7)
+#define MEDIA_INTF_T_DVB_FE    			(MEDIA_INTF_T_DVB_BASE)
+#define MEDIA_INTF_T_DVB_DEMUX  		(MEDIA_INTF_T_DVB_BASE + 1)
+#define MEDIA_INTF_T_DVB_DVR    		(MEDIA_INTF_T_DVB_BASE + 2)
+#define MEDIA_INTF_T_DVB_CA     		(MEDIA_INTF_T_DVB_BASE + 3)
+#define MEDIA_INTF_T_DVB_NET    		(MEDIA_INTF_T_DVB_BASE + 4)
+
+#define MEDIA_INTF_T_V4L_VIDEO  		(MEDIA_INTF_T_V4L_BASE)
+#define MEDIA_INTF_T_V4L_VBI    		(MEDIA_INTF_T_V4L_BASE + 1)
+#define MEDIA_INTF_T_V4L_RADIO  		(MEDIA_INTF_T_V4L_BASE + 2)
+#define MEDIA_INTF_T_V4L_SUBDEV 		(MEDIA_INTF_T_V4L_BASE + 3)
+#define MEDIA_INTF_T_V4L_SWRADIO 		(MEDIA_INTF_T_V4L_BASE + 4)
+#define MEDIA_INTF_T_V4L_TOUCH			(MEDIA_INTF_T_V4L_BASE + 5)
+
+#if defined(__KERNEL__)
 
 /*
- * MC next gen API definitions
+ * Connector functions
  *
- * NOTE: The declarations below are close to the MC RFC for the Media
- *	 Controller, the next generation. Yet, there are a few adjustments
- *	 to do, as we want to be able to have a functional API before
- *	 the MC properties change. Those will be properly marked below.
- *	 Please also notice that I removed "num_pads", "num_links",
- *	 from the proposal, as a proper userspace application will likely
- *	 use lists for pads/links, just as we intend to do in Kernelspace.
- *	 The API definition should be freed from fields that are bound to
- *	 some specific data structure.
+ * For now these should not be used in userspace, as some definitions may
+ * change.
  *
- * FIXME: Currently, I opted to name the new types as "media_v2", as this
- *	  won't cause any conflict with the Kernelspace namespace, nor with
- *	  the previous kAPI media_*_desc namespace. This can be changed
- *	  later, before the adding this API upstream.
+ * It is the responsibility of the entity drivers to add connectors and links.
  */
+#define MEDIA_ENT_F_CONN_RF			(MEDIA_ENT_F_BASE + 0x30001)
+#define MEDIA_ENT_F_CONN_SVIDEO			(MEDIA_ENT_F_BASE + 0x30002)
+#define MEDIA_ENT_F_CONN_COMPOSITE		(MEDIA_ENT_F_BASE + 0x30003)
 
+#endif
+
+/*
+ * MC next gen API definitions
+ */
 
 struct media_v2_entity {
 	__u32 id;
-	char name[64];		/* FIXME: move to a property? (RFC says so) */
+	char name[64];
 	__u32 function;		/* Main function of the entity */
 	__u32 reserved[6];
 } __attribute__ ((packed));
@@ -406,12 +339,66 @@ struct media_v2_topology {
 	__u64 ptr_links;
 } __attribute__ ((packed));
 
+
 /* ioctls */
 
-#define MEDIA_IOC_DEVICE_INFO		_IOWR('|', 0x00, struct media_device_info)
-#define MEDIA_IOC_ENUM_ENTITIES		_IOWR('|', 0x01, struct media_entity_desc)
-#define MEDIA_IOC_ENUM_LINKS		_IOWR('|', 0x02, struct media_links_enum)
-#define MEDIA_IOC_SETUP_LINK		_IOWR('|', 0x03, struct media_link_desc)
-#define MEDIA_IOC_G_TOPOLOGY		_IOWR('|', 0x04, struct media_v2_topology)
+#define MEDIA_IOC_DEVICE_INFO	_IOWR('|', 0x00, struct media_device_info)
+#define MEDIA_IOC_ENUM_ENTITIES	_IOWR('|', 0x01, struct media_entity_desc)
+#define MEDIA_IOC_ENUM_LINKS	_IOWR('|', 0x02, struct media_links_enum)
+#define MEDIA_IOC_SETUP_LINK	_IOWR('|', 0x03, struct media_link_desc)
+#define MEDIA_IOC_G_TOPOLOGY	_IOWR('|', 0x04, struct media_v2_topology)
+
+
+#if !defined(__KERNEL__) || defined(__NEED_MEDIA_LEGACY_API)
+
+/*
+ * Legacy symbols used to avoid userspace compilation breakages.
+ * Do not use any of this in new applications!
+ *
+ * Those symbols map the entity function into types and should be
+ * used only on legacy programs for legacy hardware. Don't rely
+ * on those for MEDIA_IOC_G_TOPOLOGY.
+ */
+#define MEDIA_ENT_TYPE_SHIFT			16
+#define MEDIA_ENT_TYPE_MASK			0x00ff0000
+#define MEDIA_ENT_SUBTYPE_MASK			0x0000ffff
+
+#define MEDIA_ENT_T_DEVNODE_UNKNOWN		(MEDIA_ENT_F_OLD_BASE | \
+						 MEDIA_ENT_SUBTYPE_MASK)
+
+#define MEDIA_ENT_T_DEVNODE			MEDIA_ENT_F_OLD_BASE
+#define MEDIA_ENT_T_DEVNODE_V4L			MEDIA_ENT_F_IO_V4L
+#define MEDIA_ENT_T_DEVNODE_FB			(MEDIA_ENT_F_OLD_BASE + 2)
+#define MEDIA_ENT_T_DEVNODE_ALSA		(MEDIA_ENT_F_OLD_BASE + 3)
+#define MEDIA_ENT_T_DEVNODE_DVB			(MEDIA_ENT_F_OLD_BASE + 4)
+
+#define MEDIA_ENT_T_UNKNOWN			MEDIA_ENT_F_UNKNOWN
+#define MEDIA_ENT_T_V4L2_VIDEO			MEDIA_ENT_F_IO_V4L
+#define MEDIA_ENT_T_V4L2_SUBDEV			MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN
+#define MEDIA_ENT_T_V4L2_SUBDEV_SENSOR		MEDIA_ENT_F_CAM_SENSOR
+#define MEDIA_ENT_T_V4L2_SUBDEV_FLASH		MEDIA_ENT_F_FLASH
+#define MEDIA_ENT_T_V4L2_SUBDEV_LENS		MEDIA_ENT_F_LENS
+#define MEDIA_ENT_T_V4L2_SUBDEV_DECODER		MEDIA_ENT_F_ATV_DECODER
+#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER		MEDIA_ENT_F_TUNER
+
+/*
+ * There is still no ALSA support in the media controller. These
+ * defines should not have been added and we leave them here only
+ * in case some application tries to use these defines.
+ */
+#define MEDIA_INTF_T_ALSA_BASE			0x00000300
+#define MEDIA_INTF_T_ALSA_PCM_CAPTURE   	(MEDIA_INTF_T_ALSA_BASE)
+#define MEDIA_INTF_T_ALSA_PCM_PLAYBACK  	(MEDIA_INTF_T_ALSA_BASE + 1)
+#define MEDIA_INTF_T_ALSA_CONTROL       	(MEDIA_INTF_T_ALSA_BASE + 2)
+#define MEDIA_INTF_T_ALSA_COMPRESS      	(MEDIA_INTF_T_ALSA_BASE + 3)
+#define MEDIA_INTF_T_ALSA_RAWMIDI       	(MEDIA_INTF_T_ALSA_BASE + 4)
+#define MEDIA_INTF_T_ALSA_HWDEP         	(MEDIA_INTF_T_ALSA_BASE + 5)
+#define MEDIA_INTF_T_ALSA_SEQUENCER     	(MEDIA_INTF_T_ALSA_BASE + 6)
+#define MEDIA_INTF_T_ALSA_TIMER         	(MEDIA_INTF_T_ALSA_BASE + 7)
+
+/* Obsolete symbol for media_version, no longer used in the kernel */
+#define MEDIA_API_VERSION			KERNEL_VERSION(0, 1, 0)
+
+#endif
 
 #endif /* __LINUX_MEDIA_H */
-- 
2.15.1

Re: [PATCHv2 04/15] v4l2-subdev: without controls return -ENOTTY

[Sakari Ailus]

Hi Hans,

On Thu, Feb 08, 2018 at 09:36:44AM +0100, Hans Verkuil wrote:
> If the subdev did not define any controls, then return -ENOTTY if
> userspace attempts to call these ioctls.
> 
> The control framework functions will return -EINVAL, not -ENOTTY if
> vfh->ctrl_handler is NULL.
> 
> Several of these framework functions are also called directly from
> drivers, so I don't want to change the error code there.
> 
> Found with vimc and v4l2-compliance.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks for the patch.

If the handler is NULL, can there be support for the IOCTL at all? I.e.
should the missing handler as such result in returning -ENOTTY from these
functions instead of -EINVAL?

> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 43fefa73e0a3..be7a19272614 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -187,27 +187,43 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  
>  	switch (cmd) {
>  	case VIDIOC_QUERYCTRL:
> +		if (!vfh->ctrl_handler)
> +			return -ENOTTY;
>  		return v4l2_queryctrl(vfh->ctrl_handler, arg);
>  
>  	case VIDIOC_QUERY_EXT_CTRL:
> +		if (!vfh->ctrl_handler)
> +			return -ENOTTY;
>  		return v4l2_query_ext_ctrl(vfh->ctrl_handler, arg);
>  
>  	case VIDIOC_QUERYMENU:
> +		if (!vfh->ctrl_handler)
> +			return -ENOTTY;
>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
>  
>  	case VIDIOC_G_CTRL:
> +		if (!vfh->ctrl_handler)
> +			return -ENOTTY;
>  		return v4l2_g_ctrl(vfh->ctrl_handler, arg);
>  
>  	case VIDIOC_S_CTRL:
> +		if (!vfh->ctrl_handler)
> +			return -ENOTTY;
>  		return v4l2_s_ctrl(vfh, vfh->ctrl_handler, arg);
>  
>  	case VIDIOC_G_EXT_CTRLS:
> +		if (!vfh->ctrl_handler)
> +			return -ENOTTY;
>  		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg);
>  
>  	case VIDIOC_S_EXT_CTRLS:
> +		if (!vfh->ctrl_handler)
> +			return -ENOTTY;
>  		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
>  
>  	case VIDIOC_TRY_EXT_CTRLS:
> +		if (!vfh->ctrl_handler)
> +			return -ENOTTY;
>  		return v4l2_try_ext_ctrls(vfh->ctrl_handler, arg);
>  
>  	case VIDIOC_DQEVENT:

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCHv2 04/15] v4l2-subdev: without controls return -ENOTTY

[Hans Verkuil]

On 02/09/18 12:46, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, Feb 08, 2018 at 09:36:44AM +0100, Hans Verkuil wrote:
>> If the subdev did not define any controls, then return -ENOTTY if
>> userspace attempts to call these ioctls.
>>
>> The control framework functions will return -EINVAL, not -ENOTTY if
>> vfh->ctrl_handler is NULL.
>>
>> Several of these framework functions are also called directly from
>> drivers, so I don't want to change the error code there.
>>
>> Found with vimc and v4l2-compliance.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Thanks for the patch.
> 
> If the handler is NULL, can there be support for the IOCTL at all? I.e.
> should the missing handler as such result in returning -ENOTTY from these
> functions instead of -EINVAL?

I didn't dare change the control framework. Some of these v4l2_... functions
are called by drivers and I didn't want to analyze them all. If these
functions were only called by v4l2-ioctl.c and v4l2-subdev.c, then I'd have
changed it in v4l2-ctrls.c, but that's not the case.

It would be a useful project to replace all calls from drivers to these
functions (they really shouldn't be used by drivers), but that is out-of-scope
of this patch.

Regards,

	Hans

> 
>> ---
>>  drivers/media/v4l2-core/v4l2-subdev.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 43fefa73e0a3..be7a19272614 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -187,27 +187,43 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>  
>>  	switch (cmd) {
>>  	case VIDIOC_QUERYCTRL:
>> +		if (!vfh->ctrl_handler)
>> +			return -ENOTTY;
>>  		return v4l2_queryctrl(vfh->ctrl_handler, arg);
>>  
>>  	case VIDIOC_QUERY_EXT_CTRL:
>> +		if (!vfh->ctrl_handler)
>> +			return -ENOTTY;
>>  		return v4l2_query_ext_ctrl(vfh->ctrl_handler, arg);
>>  
>>  	case VIDIOC_QUERYMENU:
>> +		if (!vfh->ctrl_handler)
>> +			return -ENOTTY;
>>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
>>  
>>  	case VIDIOC_G_CTRL:
>> +		if (!vfh->ctrl_handler)
>> +			return -ENOTTY;
>>  		return v4l2_g_ctrl(vfh->ctrl_handler, arg);
>>  
>>  	case VIDIOC_S_CTRL:
>> +		if (!vfh->ctrl_handler)
>> +			return -ENOTTY;
>>  		return v4l2_s_ctrl(vfh, vfh->ctrl_handler, arg);
>>  
>>  	case VIDIOC_G_EXT_CTRLS:
>> +		if (!vfh->ctrl_handler)
>> +			return -ENOTTY;
>>  		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg);
>>  
>>  	case VIDIOC_S_EXT_CTRLS:
>> +		if (!vfh->ctrl_handler)
>> +			return -ENOTTY;
>>  		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
>>  
>>  	case VIDIOC_TRY_EXT_CTRLS:
>> +		if (!vfh->ctrl_handler)
>> +			return -ENOTTY;
>>  		return v4l2_try_ext_ctrls(vfh->ctrl_handler, arg);
>>  
>>  	case VIDIOC_DQEVENT:
> 

Re: [PATCHv2 06/15] v4l2-subdev: implement VIDIOC_DBG_G_CHIP_INFO ioctl

[Sakari Ailus]

Hi Hans,

On Thu, Feb 08, 2018 at 09:36:46AM +0100, Hans Verkuil wrote:
> The VIDIOC_DBG_G/S_REGISTER ioctls imply that VIDIOC_DBG_G_CHIP_INFO is also
> present, since without that you cannot use v4l2-dbg.
> 
> Just like the implementation in v4l2-ioctl.c this can be implemented in the
> core and no drivers need to be modified.
> 
> It also makes it possible for v4l2-compliance to properly test the
> VIDIOC_DBG_G/S_REGISTER ioctls.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 6cabfa32d2ed..2a5b5a3fa7a3 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -255,6 +255,19 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  			return -EPERM;
>  		return v4l2_subdev_call(sd, core, s_register, p);
>  	}
> +	case VIDIOC_DBG_G_CHIP_INFO:
> +	{
> +		struct v4l2_dbg_chip_info *p = arg;
> +
> +		if (p->match.type != V4L2_CHIP_MATCH_SUBDEV || p->match.addr)
> +			return -EINVAL;
> +		if (sd->ops->core && sd->ops->core->s_register)
> +			p->flags |= V4L2_CHIP_FL_WRITABLE;
> +		if (sd->ops->core && sd->ops->core->g_register)
> +			p->flags |= V4L2_CHIP_FL_READABLE;
> +		strlcpy(p->name, sd->name, sizeof(p->name));
> +		return 0;
> +	}

This is effectively doing the same as debugfs except that it's specific to
V4L2. I don't think we should endorse its use, and especially not without a
real use case.

>  #endif
>  
>  	case VIDIOC_LOG_STATUS: {
> -- 
> 2.15.1
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCHv2 11/15] media-device.c: zero reserved field

[Sakari Ailus]

On Thu, Feb 08, 2018 at 09:36:51AM +0100, Hans Verkuil wrote:
> MEDIA_IOC_SETUP_LINK didn't zero the reserved field of the media_link_desc
> struct. Do so in media_device_setup_link().
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/media-device.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index e79f72b8b858..afbf23a19e16 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -218,6 +218,8 @@ static long media_device_setup_link(struct media_device *mdev,
>  	if (link == NULL)
>  		return -EINVAL;
>  
> +	memset(linkd->reserved, 0, sizeof(linkd->reserved));
> +

Doesn't media_device_enum_links() need the same for its reserved field?

>  	/* Setup the link on both entities. */
>  	return __media_entity_setup_link(link, linkd->flags);
>  }
> -- 
> 2.15.1
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCHv2 06/15] v4l2-subdev: implement VIDIOC_DBG_G_CHIP_INFO ioctl

[Hans Verkuil]

On 02/09/18 13:01, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, Feb 08, 2018 at 09:36:46AM +0100, Hans Verkuil wrote:
>> The VIDIOC_DBG_G/S_REGISTER ioctls imply that VIDIOC_DBG_G_CHIP_INFO is also
>> present, since without that you cannot use v4l2-dbg.
>>
>> Just like the implementation in v4l2-ioctl.c this can be implemented in the
>> core and no drivers need to be modified.
>>
>> It also makes it possible for v4l2-compliance to properly test the
>> VIDIOC_DBG_G/S_REGISTER ioctls.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-subdev.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 6cabfa32d2ed..2a5b5a3fa7a3 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -255,6 +255,19 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>  			return -EPERM;
>>  		return v4l2_subdev_call(sd, core, s_register, p);
>>  	}
>> +	case VIDIOC_DBG_G_CHIP_INFO:
>> +	{
>> +		struct v4l2_dbg_chip_info *p = arg;
>> +
>> +		if (p->match.type != V4L2_CHIP_MATCH_SUBDEV || p->match.addr)
>> +			return -EINVAL;
>> +		if (sd->ops->core && sd->ops->core->s_register)
>> +			p->flags |= V4L2_CHIP_FL_WRITABLE;
>> +		if (sd->ops->core && sd->ops->core->g_register)
>> +			p->flags |= V4L2_CHIP_FL_READABLE;
>> +		strlcpy(p->name, sd->name, sizeof(p->name));
>> +		return 0;
>> +	}
> 
> This is effectively doing the same as debugfs except that it's specific to
> V4L2. I don't think we should endorse its use, and especially not without a
> real use case.

We (Cisco) use it all the time. Furthermore, this works for any bus, not just
i2c. Also spi, internal register busses, etc.

It's been in use for many years. More importantly, there is no excuse to have
only half the API implemented.

It's all fine to talk about debugfs, but are you going to make that? This API
works, it's supported by v4l2-dbg, it's in use. Now, let's at least make it
pass v4l2-compliance.

I agree, if we would redesign it, we would use debugfs. But I think it didn't
even exist when this was made. So this API is here to stay and all it takes
is this ioctl of code to add the missing piece for subdevs.

Nobody is going to make a replacement for this using debugfs. Why spend effort
on it if we already have an API for this?

Regards,

	Hans

> 
>>  #endif
>>  
>>  	case VIDIOC_LOG_STATUS: {
>> -- 
>> 2.15.1
>>
> 

Re: [PATCHv2 11/15] media-device.c: zero reserved field

[Hans Verkuil]

On 02/09/18 13:17, Sakari Ailus wrote:
> On Thu, Feb 08, 2018 at 09:36:51AM +0100, Hans Verkuil wrote:
>> MEDIA_IOC_SETUP_LINK didn't zero the reserved field of the media_link_desc
>> struct. Do so in media_device_setup_link().
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/media-device.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index e79f72b8b858..afbf23a19e16 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -218,6 +218,8 @@ static long media_device_setup_link(struct media_device *mdev,
>>  	if (link == NULL)
>>  		return -EINVAL;
>>  
>> +	memset(linkd->reserved, 0, sizeof(linkd->reserved));
>> +
> 
> Doesn't media_device_enum_links() need the same for its reserved field?

enum_links() already zeroes this (actually the whole media_link_desc struct is zeroed).

Regards,

	Hans

> 
>>  	/* Setup the link on both entities. */
>>  	return __media_entity_setup_link(link, linkd->flags);
>>  }
>> -- 
>> 2.15.1
>>
> 

Re: [PATCHv2 13/15] media: document and zero reservedX fields in media_v2_topology

[Sakari Ailus]

Hi Hans,

On Thu, Feb 08, 2018 at 09:36:53AM +0100, Hans Verkuil wrote:
> The MEDIA_IOC_G_TOPOLOGY documentation didn't document the reservedX fields.
> Related to that was that the documented type of the num_* fields was also
> wrong.
> 
> The reservedX fields were not set to 0, that is now also fixed.
> 
> Found with v4l2-compliance.

How about splitting the patch between documentation and code? The two are
largely unrelated, and there could be a need to backport the patch zeroing
the fields (but not necessarily the documentation).

> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  .../media/uapi/mediactl/media-ioc-g-topology.rst   | 52 +++++++++++++++++-----
>  drivers/media/media-device.c                       |  4 ++
>  2 files changed, 46 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
> index 870a6c0d1f7a..c8f9ea37db2d 100644
> --- a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
> +++ b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
> @@ -68,7 +68,7 @@ desired arrays with the media graph elements.
>  
>      -  .. row 2
>  
> -       -  __u64
> +       -  __u32
>  
>         -  ``num_entities``
>  
> @@ -76,6 +76,14 @@ desired arrays with the media graph elements.
>  
>      -  .. row 3
>  
> +       -  __u32
> +
> +       -  ``reserved1``
> +
> +       -  Applications and drivers shall set this to 0.
> +
> +    -  .. row 4
> +
>         -  __u64
>  
>         -  ``ptr_entities``
> @@ -85,15 +93,23 @@ desired arrays with the media graph elements.
>  	  the ioctl won't store the entities. It will just update
>  	  ``num_entities``
>  
> -    -  .. row 4
> +    -  .. row 5
>  
> -       -  __u64
> +       -  __u32
>  
>         -  ``num_interfaces``
>  
>         -  Number of interfaces in the graph
>  
> -    -  .. row 5
> +    -  .. row 6
> +
> +       -  __u32
> +
> +       -  ``reserved2``
> +
> +       -  Applications and drivers shall set this to 0.
> +
> +    -  .. row 7
>  
>         -  __u64
>  
> @@ -104,15 +120,23 @@ desired arrays with the media graph elements.
>  	  the ioctl won't store the interfaces. It will just update
>  	  ``num_interfaces``
>  
> -    -  .. row 6
> +    -  .. row 8
>  
> -       -  __u64
> +       -  __u32
>  
>         -  ``num_pads``
>  
>         -  Total number of pads in the graph
>  
> -    -  .. row 7
> +    -  .. row 9
> +
> +       -  __u32
> +
> +       -  ``reserved3``
> +
> +       -  Applications and drivers shall set this to 0.
> +
> +    -  .. row 10
>  
>         -  __u64
>  
> @@ -122,15 +146,23 @@ desired arrays with the media graph elements.
>  	  converted to a 64-bits integer. It can be zero. if zero, the ioctl
>  	  won't store the pads. It will just update ``num_pads``
>  
> -    -  .. row 8
> +    -  .. row 11
>  
> -       -  __u64
> +       -  __u32
>  
>         -  ``num_links``
>  
>         -  Total number of data and interface links in the graph
>  
> -    -  .. row 9
> +    -  .. row 12
> +
> +       -  __u32
> +
> +       -  ``reserved4``
> +
> +       -  Applications and drivers shall set this to 0.
> +
> +    -  .. row 13
>  
>         -  __u64
>  
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index afbf23a19e16..d0e3f61dde52 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -265,6 +265,7 @@ static long media_device_get_topology(struct media_device *mdev,
>  		uentity++;
>  	}
>  	topo->num_entities = i;
> +	topo->reserved1 = 0;
>  
>  	/* Get interfaces and number of interfaces */
>  	i = 0;
> @@ -300,6 +301,7 @@ static long media_device_get_topology(struct media_device *mdev,
>  		uintf++;
>  	}
>  	topo->num_interfaces = i;
> +	topo->reserved2 = 0;
>  
>  	/* Get pads and number of pads */
>  	i = 0;
> @@ -326,6 +328,7 @@ static long media_device_get_topology(struct media_device *mdev,
>  		upad++;
>  	}
>  	topo->num_pads = i;
> +	topo->reserved3 = 0;
>  
>  	/* Get links and number of links */
>  	i = 0;
> @@ -357,6 +360,7 @@ static long media_device_get_topology(struct media_device *mdev,
>  		ulink++;
>  	}
>  	topo->num_links = i;
> +	topo->reserved4 = 0;
>  
>  	return ret;
>  }
> -- 
> 2.15.1
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCHv2 04/15] v4l2-subdev: without controls return -ENOTTY

[Sakari Ailus]

Hi Hans,

On Fri, Feb 09, 2018 at 12:56:37PM +0100, Hans Verkuil wrote:
> On 02/09/18 12:46, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Thu, Feb 08, 2018 at 09:36:44AM +0100, Hans Verkuil wrote:
> >> If the subdev did not define any controls, then return -ENOTTY if
> >> userspace attempts to call these ioctls.
> >>
> >> The control framework functions will return -EINVAL, not -ENOTTY if
> >> vfh->ctrl_handler is NULL.
> >>
> >> Several of these framework functions are also called directly from
> >> drivers, so I don't want to change the error code there.
> >>
> >> Found with vimc and v4l2-compliance.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > Thanks for the patch.
> > 
> > If the handler is NULL, can there be support for the IOCTL at all? I.e.
> > should the missing handler as such result in returning -ENOTTY from these
> > functions instead of -EINVAL?
> 
> I didn't dare change the control framework. Some of these v4l2_... functions
> are called by drivers and I didn't want to analyze them all. If these
> functions were only called by v4l2-ioctl.c and v4l2-subdev.c, then I'd have
> changed it in v4l2-ctrls.c, but that's not the case.
> 
> It would be a useful project to replace all calls from drivers to these
> functions (they really shouldn't be used by drivers), but that is out-of-scope
> of this patch.

Is your concern that the caller could check the return value and do
something based on particular error code it gets?

Based on a quick glance there are a few tens of places these functions are
used in drivers. Some seems legitimate; the caller having another device
where a control needs to be accessed, for instance.

And if handler is NULL, -ENOTTY appears to be a more suitable return value
in a lot of the cases (and in many others it makes no difference).

I wouldn't say this is something that should hold back addressing this in
the control framework instead.

I can submit a patch if you'd prefer that instead.

> 
> Regards,
> 
> 	Hans
> 
> > 
> >> ---
> >>  drivers/media/v4l2-core/v4l2-subdev.c | 16 ++++++++++++++++
> >>  1 file changed, 16 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >> index 43fefa73e0a3..be7a19272614 100644
> >> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >> @@ -187,27 +187,43 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >>  
> >>  	switch (cmd) {
> >>  	case VIDIOC_QUERYCTRL:
> >> +		if (!vfh->ctrl_handler)
> >> +			return -ENOTTY;
> >>  		return v4l2_queryctrl(vfh->ctrl_handler, arg);
> >>  
> >>  	case VIDIOC_QUERY_EXT_CTRL:
> >> +		if (!vfh->ctrl_handler)
> >> +			return -ENOTTY;
> >>  		return v4l2_query_ext_ctrl(vfh->ctrl_handler, arg);
> >>  
> >>  	case VIDIOC_QUERYMENU:
> >> +		if (!vfh->ctrl_handler)
> >> +			return -ENOTTY;
> >>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
> >>  
> >>  	case VIDIOC_G_CTRL:
> >> +		if (!vfh->ctrl_handler)
> >> +			return -ENOTTY;
> >>  		return v4l2_g_ctrl(vfh->ctrl_handler, arg);
> >>  
> >>  	case VIDIOC_S_CTRL:
> >> +		if (!vfh->ctrl_handler)
> >> +			return -ENOTTY;
> >>  		return v4l2_s_ctrl(vfh, vfh->ctrl_handler, arg);
> >>  
> >>  	case VIDIOC_G_EXT_CTRLS:
> >> +		if (!vfh->ctrl_handler)
> >> +			return -ENOTTY;
> >>  		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg);
> >>  
> >>  	case VIDIOC_S_EXT_CTRLS:
> >> +		if (!vfh->ctrl_handler)
> >> +			return -ENOTTY;
> >>  		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
> >>  
> >>  	case VIDIOC_TRY_EXT_CTRLS:
> >> +		if (!vfh->ctrl_handler)
> >> +			return -ENOTTY;
> >>  		return v4l2_try_ext_ctrls(vfh->ctrl_handler, arg);
> >>  
> >>  	case VIDIOC_DQEVENT:
> > 
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCHv2 06/15] v4l2-subdev: implement VIDIOC_DBG_G_CHIP_INFO ioctl

[Sakari Ailus]

On Fri, Feb 09, 2018 at 01:18:18PM +0100, Hans Verkuil wrote:
> On 02/09/18 13:01, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Thu, Feb 08, 2018 at 09:36:46AM +0100, Hans Verkuil wrote:
> >> The VIDIOC_DBG_G/S_REGISTER ioctls imply that VIDIOC_DBG_G_CHIP_INFO is also
> >> present, since without that you cannot use v4l2-dbg.
> >>
> >> Just like the implementation in v4l2-ioctl.c this can be implemented in the
> >> core and no drivers need to be modified.
> >>
> >> It also makes it possible for v4l2-compliance to properly test the
> >> VIDIOC_DBG_G/S_REGISTER ioctls.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> ---
> >>  drivers/media/v4l2-core/v4l2-subdev.c | 13 +++++++++++++
> >>  1 file changed, 13 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >> index 6cabfa32d2ed..2a5b5a3fa7a3 100644
> >> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >> @@ -255,6 +255,19 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >>  			return -EPERM;
> >>  		return v4l2_subdev_call(sd, core, s_register, p);
> >>  	}
> >> +	case VIDIOC_DBG_G_CHIP_INFO:
> >> +	{
> >> +		struct v4l2_dbg_chip_info *p = arg;
> >> +
> >> +		if (p->match.type != V4L2_CHIP_MATCH_SUBDEV || p->match.addr)
> >> +			return -EINVAL;
> >> +		if (sd->ops->core && sd->ops->core->s_register)
> >> +			p->flags |= V4L2_CHIP_FL_WRITABLE;
> >> +		if (sd->ops->core && sd->ops->core->g_register)
> >> +			p->flags |= V4L2_CHIP_FL_READABLE;
> >> +		strlcpy(p->name, sd->name, sizeof(p->name));
> >> +		return 0;
> >> +	}
> > 
> > This is effectively doing the same as debugfs except that it's specific to
> > V4L2. I don't think we should endorse its use, and especially not without a
> > real use case.
> 
> We (Cisco) use it all the time. Furthermore, this works for any bus, not just
> i2c. Also spi, internal register busses, etc.
> 
> It's been in use for many years. More importantly, there is no excuse to have
> only half the API implemented.
> 
> It's all fine to talk about debugfs, but are you going to make that? This API
> works, it's supported by v4l2-dbg, it's in use. Now, let's at least make it
> pass v4l2-compliance.
> 
> I agree, if we would redesign it, we would use debugfs. But I think it didn't
> even exist when this was made. So this API is here to stay and all it takes
> is this ioctl of code to add the missing piece for subdevs.
> 
> Nobody is going to make a replacement for this using debugfs. Why spend effort
> on it if we already have an API for this?

It's not the first case when a more generic API replaces a subsystem
specific one. We have another conversion to make, switching from
implementing s_power() callback in drivers to runtime PM for instance.

I simply want to point out that this patch is endorsing something which is
obsolete and not needed: no-one has complained about the lack of this for
sub-devices, haven't they?

I'd just remove the check from v4l-compliance or make it optional. New
drivers should use debugfs instead if something like that is needed.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCHv2 11/15] media-device.c: zero reserved field

[Sakari Ailus]

On Fri, Feb 09, 2018 at 01:20:41PM +0100, Hans Verkuil wrote:
> On 02/09/18 13:17, Sakari Ailus wrote:
> > On Thu, Feb 08, 2018 at 09:36:51AM +0100, Hans Verkuil wrote:
> >> MEDIA_IOC_SETUP_LINK didn't zero the reserved field of the media_link_desc
> >> struct. Do so in media_device_setup_link().
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> ---
> >>  drivers/media/media-device.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >> index e79f72b8b858..afbf23a19e16 100644
> >> --- a/drivers/media/media-device.c
> >> +++ b/drivers/media/media-device.c
> >> @@ -218,6 +218,8 @@ static long media_device_setup_link(struct media_device *mdev,
> >>  	if (link == NULL)
> >>  		return -EINVAL;
> >>  
> >> +	memset(linkd->reserved, 0, sizeof(linkd->reserved));
> >> +
> > 
> > Doesn't media_device_enum_links() need the same for its reserved field?
> 
> enum_links() already zeroes this (actually the whole media_link_desc struct is zeroed).

I can't see that being done in here and I also don't mean the compat
variant. Can you point me to it?

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCHv2 00/15] Media Controller compliance fixes

[Sakari Ailus]

On Thu, Feb 08, 2018 at 09:36:40AM +0100, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Hi all,
> 
> I've been posting random patches fixing various MC problems, but it is
> easier to see them all in a single patch series.
> 
> All patches except 13 and 14 are identical to was I posted earlier.
> For 13 and 14 I decided to drop the requirement that the application
> clears the reserved field. Only the driver will clear it.

Thanks!

For patches 5, 7, 8, 10, 12 and 14:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCHv2 04/15] v4l2-subdev: without controls return -ENOTTY

[Hans Verkuil]

On 02/09/18 13:38, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Feb 09, 2018 at 12:56:37PM +0100, Hans Verkuil wrote:
>> On 02/09/18 12:46, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Thu, Feb 08, 2018 at 09:36:44AM +0100, Hans Verkuil wrote:
>>>> If the subdev did not define any controls, then return -ENOTTY if
>>>> userspace attempts to call these ioctls.
>>>>
>>>> The control framework functions will return -EINVAL, not -ENOTTY if
>>>> vfh->ctrl_handler is NULL.
>>>>
>>>> Several of these framework functions are also called directly from
>>>> drivers, so I don't want to change the error code there.
>>>>
>>>> Found with vimc and v4l2-compliance.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> Thanks for the patch.
>>>
>>> If the handler is NULL, can there be support for the IOCTL at all? I.e.
>>> should the missing handler as such result in returning -ENOTTY from these
>>> functions instead of -EINVAL?
>>
>> I didn't dare change the control framework. Some of these v4l2_... functions
>> are called by drivers and I didn't want to analyze them all. If these
>> functions were only called by v4l2-ioctl.c and v4l2-subdev.c, then I'd have
>> changed it in v4l2-ctrls.c, but that's not the case.
>>
>> It would be a useful project to replace all calls from drivers to these
>> functions (they really shouldn't be used by drivers), but that is out-of-scope
>> of this patch.
> 
> Is your concern that the caller could check the return value and do
> something based on particular error code it gets?

Or that the handler is NULL and it returns ENOTTY to userspace. You can have
multiple control handlers, some of which might be NULL. It's all unlikely,
but the code needs to be analyzed and that takes time. Hmm, atomisp is
definitely a big user of these functions.

Also, the real issue is the use of these functions by drivers. What I want
to do is to have the drivers use the proper functions, then I can move those
functions to the core and stop exporting them. And at that moment they can
return -ENOTTY instead of -EINVAL.

A worthwhile project, but right now I just want to fix v4l2-subdev.c.

Regards,

	Hans

> Based on a quick glance there are a few tens of places these functions are
> used in drivers. Some seems legitimate; the caller having another device
> where a control needs to be accessed, for instance.
> 
> And if handler is NULL, -ENOTTY appears to be a more suitable return value
> in a lot of the cases (and in many others it makes no difference).
> 
> I wouldn't say this is something that should hold back addressing this in
> the control framework instead.
> 
> I can submit a patch if you'd prefer that instead.
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>>> ---
>>>>  drivers/media/v4l2-core/v4l2-subdev.c | 16 ++++++++++++++++
>>>>  1 file changed, 16 insertions(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>>> index 43fefa73e0a3..be7a19272614 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>> @@ -187,27 +187,43 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>>>  
>>>>  	switch (cmd) {
>>>>  	case VIDIOC_QUERYCTRL:
>>>> +		if (!vfh->ctrl_handler)
>>>> +			return -ENOTTY;
>>>>  		return v4l2_queryctrl(vfh->ctrl_handler, arg);
>>>>  
>>>>  	case VIDIOC_QUERY_EXT_CTRL:
>>>> +		if (!vfh->ctrl_handler)
>>>> +			return -ENOTTY;
>>>>  		return v4l2_query_ext_ctrl(vfh->ctrl_handler, arg);
>>>>  
>>>>  	case VIDIOC_QUERYMENU:
>>>> +		if (!vfh->ctrl_handler)
>>>> +			return -ENOTTY;
>>>>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
>>>>  
>>>>  	case VIDIOC_G_CTRL:
>>>> +		if (!vfh->ctrl_handler)
>>>> +			return -ENOTTY;
>>>>  		return v4l2_g_ctrl(vfh->ctrl_handler, arg);
>>>>  
>>>>  	case VIDIOC_S_CTRL:
>>>> +		if (!vfh->ctrl_handler)
>>>> +			return -ENOTTY;
>>>>  		return v4l2_s_ctrl(vfh, vfh->ctrl_handler, arg);
>>>>  
>>>>  	case VIDIOC_G_EXT_CTRLS:
>>>> +		if (!vfh->ctrl_handler)
>>>> +			return -ENOTTY;
>>>>  		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg);
>>>>  
>>>>  	case VIDIOC_S_EXT_CTRLS:
>>>> +		if (!vfh->ctrl_handler)
>>>> +			return -ENOTTY;
>>>>  		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
>>>>  
>>>>  	case VIDIOC_TRY_EXT_CTRLS:
>>>> +		if (!vfh->ctrl_handler)
>>>> +			return -ENOTTY;
>>>>  		return v4l2_try_ext_ctrls(vfh->ctrl_handler, arg);
>>>>  
>>>>  	case VIDIOC_DQEVENT:
>>>
>>
> 

Re: [PATCHv2 00/15] Media Controller compliance fixes

[Sakari Ailus]

On Fri, Feb 09, 2018 at 02:48:07PM +0200, Sakari Ailus wrote:
> On Thu, Feb 08, 2018 at 09:36:40AM +0100, Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > Hi all,
> > 
> > I've been posting random patches fixing various MC problems, but it is
> > easier to see them all in a single patch series.
> > 
> > All patches except 13 and 14 are identical to was I posted earlier.
> > For 13 and 14 I decided to drop the requirement that the application
> > clears the reserved field. Only the driver will clear it.
> 
> Thanks!
> 
> For patches 5, 7, 8, 10, 12 and 14:

And patches 1 and 2 as well.

> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> -- 
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCHv2 11/15] media-device.c: zero reserved field

[Hans Verkuil]

On 02/09/18 13:46, Sakari Ailus wrote:
> On Fri, Feb 09, 2018 at 01:20:41PM +0100, Hans Verkuil wrote:
>> On 02/09/18 13:17, Sakari Ailus wrote:
>>> On Thu, Feb 08, 2018 at 09:36:51AM +0100, Hans Verkuil wrote:
>>>> MEDIA_IOC_SETUP_LINK didn't zero the reserved field of the media_link_desc
>>>> struct. Do so in media_device_setup_link().
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>> ---
>>>>  drivers/media/media-device.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>>> index e79f72b8b858..afbf23a19e16 100644
>>>> --- a/drivers/media/media-device.c
>>>> +++ b/drivers/media/media-device.c
>>>> @@ -218,6 +218,8 @@ static long media_device_setup_link(struct media_device *mdev,
>>>>  	if (link == NULL)
>>>>  		return -EINVAL;
>>>>  
>>>> +	memset(linkd->reserved, 0, sizeof(linkd->reserved));
>>>> +
>>>
>>> Doesn't media_device_enum_links() need the same for its reserved field?
>>
>> enum_links() already zeroes this (actually the whole media_link_desc struct is zeroed).
> 
> I can't see that being done in here and I also don't mean the compat
> variant. Can you point me to it?
> 

static long media_device_enum_links(struct media_device *mdev,
                                    struct media_links_enum *links)
{
        struct media_entity *entity;

        entity = find_entity(mdev, links->entity);
        if (entity == NULL)
                return -EINVAL;

        if (links->pads) {
...
        }

        if (links->links) {
                struct media_link *link;
                struct media_link_desc __user *ulink_desc = links->links;

                list_for_each_entry(link, &entity->links, list) {
                        struct media_link_desc klink_desc;

                        /* Ignore backlinks. */
                        if (link->source->entity != entity)
                                continue;
                        memset(&klink_desc, 0, sizeof(klink_desc));
			// ^^^^^^^^^^^ zeroed here
	
                        media_device_kpad_to_upad(link->source,
                                                  &klink_desc.source);
                        media_device_kpad_to_upad(link->sink,
                                                  &klink_desc.sink);
                        klink_desc.flags = link->flags;
                        if (copy_to_user(ulink_desc, &klink_desc,
                                         sizeof(*ulink_desc)))
			// ^^^^^^^ copied back to userspace (including zeroed reserved array) here
                                return -EFAULT;
                        ulink_desc++;
                }
        }

        return 0;
}

Regards,

	Hans

Re: [PATCHv2 06/15] v4l2-subdev: implement VIDIOC_DBG_G_CHIP_INFO ioctl

[Laurent Pinchart]

Hello,

On Friday, 9 February 2018 14:44:07 EET Sakari Ailus wrote:
> On Fri, Feb 09, 2018 at 01:18:18PM +0100, Hans Verkuil wrote:
> > On 02/09/18 13:01, Sakari Ailus wrote:
> >> On Thu, Feb 08, 2018 at 09:36:46AM +0100, Hans Verkuil wrote:
> >>> The VIDIOC_DBG_G/S_REGISTER ioctls imply that VIDIOC_DBG_G_CHIP_INFO is
> >>> also present, since without that you cannot use v4l2-dbg.
> >>> 
> >>> Just like the implementation in v4l2-ioctl.c this can be implemented in
> >>> the core and no drivers need to be modified.
> >>> 
> >>> It also makes it possible for v4l2-compliance to properly test the
> >>> VIDIOC_DBG_G/S_REGISTER ioctls.
> >>> 
> >>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>> ---
> >>> 
> >>>  drivers/media/v4l2-core/v4l2-subdev.c | 13 +++++++++++++
> >>>  1 file changed, 13 insertions(+)
> >>> 
> >>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
> >>> b/drivers/media/v4l2-core/v4l2-subdev.c index
> >>> 6cabfa32d2ed..2a5b5a3fa7a3 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>> @@ -255,6 +255,19 @@ static long subdev_do_ioctl(struct file *file,
> >>> unsigned int cmd, void *arg)
> >>>  			return -EPERM;
> >>>  		
> >>>  		return v4l2_subdev_call(sd, core, s_register, p);
> >>>  	}
> >>> 
> >>> +	case VIDIOC_DBG_G_CHIP_INFO:
> >>> +	{
> >>> +		struct v4l2_dbg_chip_info *p = arg;
> >>> +
> >>> +		if (p->match.type != V4L2_CHIP_MATCH_SUBDEV || p->match.addr)
> >>> +			return -EINVAL;
> >>> +		if (sd->ops->core && sd->ops->core->s_register)
> >>> +			p->flags |= V4L2_CHIP_FL_WRITABLE;
> >>> +		if (sd->ops->core && sd->ops->core->g_register)
> >>> +			p->flags |= V4L2_CHIP_FL_READABLE;
> >>> +		strlcpy(p->name, sd->name, sizeof(p->name));
> >>> +		return 0;
> >>> +	}
> >> 
> >> This is effectively doing the same as debugfs except that it's specific
> >> to V4L2. I don't think we should endorse its use, and especially not
> >> without a real use case.
> > 
> > We (Cisco) use it all the time. Furthermore, this works for any bus, not
> > just i2c. Also spi, internal register busses, etc.
> > 
> > It's been in use for many years. More importantly, there is no excuse to
> > have only half the API implemented.
> > 
> > It's all fine to talk about debugfs, but are you going to make that? This
> > API works, it's supported by v4l2-dbg, it's in use. Now, let's at least
> > make it pass v4l2-compliance.
> > 
> > I agree, if we would redesign it, we would use debugfs. But I think it
> > didn't even exist when this was made. So this API is here to stay and all
> > it takes is this ioctl of code to add the missing piece for subdevs.
> > 
> > Nobody is going to make a replacement for this using debugfs. Why spend
> > effort on it if we already have an API for this?
> 
> It's not the first case when a more generic API replaces a subsystem
> specific one. We have another conversion to make, switching from
> implementing s_power() callback in drivers to runtime PM for instance.
> 
> I simply want to point out that this patch is endorsing something which is
> obsolete and not needed: no-one has complained about the lack of this for
> sub-devices, haven't they?
> 
> I'd just remove the check from v4l-compliance or make it optional. New
> drivers should use debugfs instead if something like that is needed.

I second Sakari's opinion here. While I don't like the log status operation as 
we now have debugfs to implement the same functionality, I like the get/set 
register operations exposed through the V4L2 API even less. Let's not endorse 
usage of something we know is obsolete and work on replacing it with proper 
upstream APIs. For this specific case the first step is to not implement 
VIDIOC_DBG_G_CHIP_INFO and not consider the lack of support for this ioctl as 
a compliance issue. We can do so as the API has never documented its usage for 
subdevs.

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 06/15] v4l2-subdev: implement VIDIOC_DBG_G_CHIP_INFO ioctl

[Hans Verkuil]

On 02/09/18 13:44, Sakari Ailus wrote:
> On Fri, Feb 09, 2018 at 01:18:18PM +0100, Hans Verkuil wrote:
>> On 02/09/18 13:01, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Thu, Feb 08, 2018 at 09:36:46AM +0100, Hans Verkuil wrote:
>>>> The VIDIOC_DBG_G/S_REGISTER ioctls imply that VIDIOC_DBG_G_CHIP_INFO is also
>>>> present, since without that you cannot use v4l2-dbg.
>>>>
>>>> Just like the implementation in v4l2-ioctl.c this can be implemented in the
>>>> core and no drivers need to be modified.
>>>>
>>>> It also makes it possible for v4l2-compliance to properly test the
>>>> VIDIOC_DBG_G/S_REGISTER ioctls.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>> ---
>>>>  drivers/media/v4l2-core/v4l2-subdev.c | 13 +++++++++++++
>>>>  1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>>> index 6cabfa32d2ed..2a5b5a3fa7a3 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>> @@ -255,6 +255,19 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>>>  			return -EPERM;
>>>>  		return v4l2_subdev_call(sd, core, s_register, p);
>>>>  	}
>>>> +	case VIDIOC_DBG_G_CHIP_INFO:
>>>> +	{
>>>> +		struct v4l2_dbg_chip_info *p = arg;
>>>> +
>>>> +		if (p->match.type != V4L2_CHIP_MATCH_SUBDEV || p->match.addr)
>>>> +			return -EINVAL;
>>>> +		if (sd->ops->core && sd->ops->core->s_register)
>>>> +			p->flags |= V4L2_CHIP_FL_WRITABLE;
>>>> +		if (sd->ops->core && sd->ops->core->g_register)
>>>> +			p->flags |= V4L2_CHIP_FL_READABLE;
>>>> +		strlcpy(p->name, sd->name, sizeof(p->name));
>>>> +		return 0;
>>>> +	}
>>>
>>> This is effectively doing the same as debugfs except that it's specific to
>>> V4L2. I don't think we should endorse its use, and especially not without a
>>> real use case.
>>
>> We (Cisco) use it all the time. Furthermore, this works for any bus, not just
>> i2c. Also spi, internal register busses, etc.
>>
>> It's been in use for many years. More importantly, there is no excuse to have
>> only half the API implemented.
>>
>> It's all fine to talk about debugfs, but are you going to make that? This API
>> works, it's supported by v4l2-dbg, it's in use. Now, let's at least make it
>> pass v4l2-compliance.
>>
>> I agree, if we would redesign it, we would use debugfs. But I think it didn't
>> even exist when this was made. So this API is here to stay and all it takes
>> is this ioctl of code to add the missing piece for subdevs.
>>
>> Nobody is going to make a replacement for this using debugfs. Why spend effort
>> on it if we already have an API for this?
> 
> It's not the first case when a more generic API replaces a subsystem
> specific one. We have another conversion to make, switching from
> implementing s_power() callback in drivers to runtime PM for instance.
> 
> I simply want to point out that this patch is endorsing something which is
> obsolete and not needed: no-one has complained about the lack of this for
> sub-devices, haven't they?
> 
> I'd just remove the check from v4l-compliance or make it optional. New
> drivers should use debugfs instead if something like that is needed.
> 

You are correct in one respect: we use this API, but with video devices.
So subdevices support the g/s_register ops, and they are called via /dev/videoX.

We can remove the ioctl support from v4l2-subdev.c (not the g/s_register ops!).
Without VIDIOC_DBG_G_CHIP_INFO I don't think v4l2-dbg is usable. Although it
is always possible to call the ioctl directly, of course.

So if Mauro would agree to this, the DBG ioctl support in v4l2-subdev can be
removed.

But either remove them, or add this ioctl. Don't leave it in a zombie state.

Personally I see no harm whatsoever in just adding VIDIOC_DBG_G_CHIP_INFO.
If someone ever makes a patch to switch over to debugfs then these ioctls
can be removed.

BTW, how would new drivers use debugfs for this? Does regmap provide such
access?

Regards,

	Hans

Re: [PATCHv2 11/15] media-device.c: zero reserved field

[Sakari Ailus]

Hi Hans,

On Fri, Feb 09, 2018 at 01:52:50PM +0100, Hans Verkuil wrote:
> On 02/09/18 13:46, Sakari Ailus wrote:
> > On Fri, Feb 09, 2018 at 01:20:41PM +0100, Hans Verkuil wrote:
> >> On 02/09/18 13:17, Sakari Ailus wrote:
> >>> On Thu, Feb 08, 2018 at 09:36:51AM +0100, Hans Verkuil wrote:
> >>>> MEDIA_IOC_SETUP_LINK didn't zero the reserved field of the media_link_desc
> >>>> struct. Do so in media_device_setup_link().
> >>>>
> >>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>> ---
> >>>>  drivers/media/media-device.c | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >>>> index e79f72b8b858..afbf23a19e16 100644
> >>>> --- a/drivers/media/media-device.c
> >>>> +++ b/drivers/media/media-device.c
> >>>> @@ -218,6 +218,8 @@ static long media_device_setup_link(struct media_device *mdev,
> >>>>  	if (link == NULL)
> >>>>  		return -EINVAL;
> >>>>  
> >>>> +	memset(linkd->reserved, 0, sizeof(linkd->reserved));
> >>>> +
> >>>
> >>> Doesn't media_device_enum_links() need the same for its reserved field?
> >>
> >> enum_links() already zeroes this (actually the whole media_link_desc struct is zeroed).
> > 
> > I can't see that being done in here and I also don't mean the compat
> > variant. Can you point me to it?
> > 
> 
> static long media_device_enum_links(struct media_device *mdev,
>                                     struct media_links_enum *links)
> {
>         struct media_entity *entity;
> 
>         entity = find_entity(mdev, links->entity);
>         if (entity == NULL)
>                 return -EINVAL;
> 
>         if (links->pads) {
> ...
>         }
> 
>         if (links->links) {
>                 struct media_link *link;
>                 struct media_link_desc __user *ulink_desc = links->links;
> 
>                 list_for_each_entry(link, &entity->links, list) {
>                         struct media_link_desc klink_desc;
> 
>                         /* Ignore backlinks. */
>                         if (link->source->entity != entity)
>                                 continue;
>                         memset(&klink_desc, 0, sizeof(klink_desc));
> 			// ^^^^^^^^^^^ zeroed here
> 	
>                         media_device_kpad_to_upad(link->source,
>                                                   &klink_desc.source);
>                         media_device_kpad_to_upad(link->sink,
>                                                   &klink_desc.sink);
>                         klink_desc.flags = link->flags;
>                         if (copy_to_user(ulink_desc, &klink_desc,
>                                          sizeof(*ulink_desc)))
> 			// ^^^^^^^ copied back to userspace (including zeroed reserved array) here

We are indeed talking about a different reserved field. I mean the one in
struct media_links_enum .

>                                 return -EFAULT;
>                         ulink_desc++;
>                 }
>         }
> 
>         return 0;
> }
> 
> Regards,
> 
> 	Hans

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCHv2 06/15] v4l2-subdev: implement VIDIOC_DBG_G_CHIP_INFO ioctl

[Laurent Pinchart]

Hi Hans,

On Friday, 9 February 2018 15:00:53 EET Hans Verkuil wrote:
> On 02/09/18 13:44, Sakari Ailus wrote:
> > On Fri, Feb 09, 2018 at 01:18:18PM +0100, Hans Verkuil wrote:
> >> On 02/09/18 13:01, Sakari Ailus wrote:
> >>> On Thu, Feb 08, 2018 at 09:36:46AM +0100, Hans Verkuil wrote:
> >>>> The VIDIOC_DBG_G/S_REGISTER ioctls imply that VIDIOC_DBG_G_CHIP_INFO is
> >>>> also present, since without that you cannot use v4l2-dbg.
> >>>> 
> >>>> Just like the implementation in v4l2-ioctl.c this can be implemented in
> >>>> the
> >>>> core and no drivers need to be modified.
> >>>> 
> >>>> It also makes it possible for v4l2-compliance to properly test the
> >>>> VIDIOC_DBG_G/S_REGISTER ioctls.
> >>>> 
> >>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>> ---
> >>>> 
> >>>>  drivers/media/v4l2-core/v4l2-subdev.c | 13 +++++++++++++
> >>>>  1 file changed, 13 insertions(+)
> >>>> 
> >>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
> >>>> b/drivers/media/v4l2-core/v4l2-subdev.c index
> >>>> 6cabfa32d2ed..2a5b5a3fa7a3 100644
> >>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>> @@ -255,6 +255,19 @@ static long subdev_do_ioctl(struct file *file,
> >>>> unsigned int cmd, void *arg)>>>> 
> >>>>  			return -EPERM;
> >>>>  		
> >>>>  		return v4l2_subdev_call(sd, core, s_register, p);
> >>>>  	
> >>>>  	}
> >>>> 
> >>>> +	case VIDIOC_DBG_G_CHIP_INFO:
> >>>> +	{
> >>>> +		struct v4l2_dbg_chip_info *p = arg;
> >>>> +
> >>>> +		if (p->match.type != V4L2_CHIP_MATCH_SUBDEV || p->match.addr)
> >>>> +			return -EINVAL;
> >>>> +		if (sd->ops->core && sd->ops->core->s_register)
> >>>> +			p->flags |= V4L2_CHIP_FL_WRITABLE;
> >>>> +		if (sd->ops->core && sd->ops->core->g_register)
> >>>> +			p->flags |= V4L2_CHIP_FL_READABLE;
> >>>> +		strlcpy(p->name, sd->name, sizeof(p->name));
> >>>> +		return 0;
> >>>> +	}
> >>> 
> >>> This is effectively doing the same as debugfs except that it's specific
> >>> to V4L2. I don't think we should endorse its use, and especially not
> >>> without a real use case.
> >> 
> >> We (Cisco) use it all the time. Furthermore, this works for any bus, not
> >> just i2c. Also spi, internal register busses, etc.
> >> 
> >> It's been in use for many years. More importantly, there is no excuse to
> >> have only half the API implemented.
> >> 
> >> It's all fine to talk about debugfs, but are you going to make that? This
> >> API works, it's supported by v4l2-dbg, it's in use. Now, let's at least
> >> make it pass v4l2-compliance.
> >> 
> >> I agree, if we would redesign it, we would use debugfs. But I think it
> >> didn't even exist when this was made. So this API is here to stay and
> >> all it takes is this ioctl of code to add the missing piece for subdevs.
> >> 
> >> Nobody is going to make a replacement for this using debugfs. Why spend
> >> effort on it if we already have an API for this?
> > 
> > It's not the first case when a more generic API replaces a subsystem
> > specific one. We have another conversion to make, switching from
> > implementing s_power() callback in drivers to runtime PM for instance.
> > 
> > I simply want to point out that this patch is endorsing something which is
> > obsolete and not needed: no-one has complained about the lack of this for
> > sub-devices, haven't they?
> > 
> > I'd just remove the check from v4l-compliance or make it optional. New
> > drivers should use debugfs instead if something like that is needed.
> 
> You are correct in one respect: we use this API, but with video devices.
> So subdevices support the g/s_register ops, and they are called via
> /dev/videoX.
> 
> We can remove the ioctl support from v4l2-subdev.c (not the g/s_register
> ops!). Without VIDIOC_DBG_G_CHIP_INFO I don't think v4l2-dbg is usable.
> Although it is always possible to call the ioctl directly, of course.
> 
> So if Mauro would agree to this, the DBG ioctl support in v4l2-subdev can be
> removed.

That would be my preferred option.

> But either remove them, or add this ioctl. Don't leave it in a zombie state.
> 
> Personally I see no harm whatsoever in just adding VIDIOC_DBG_G_CHIP_INFO.
> If someone ever makes a patch to switch over to debugfs then these ioctls
> can be removed.
> 
> BTW, how would new drivers use debugfs for this? Does regmap provide such
> access?

Before attempting to provide an answer, as I've never used those ioctls 
myself, could you please give us a bit more information about the use cases 
you have at Cisco for this ?

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 04/15] v4l2-subdev: without controls return -ENOTTY

[Sakari Ailus]

On Fri, Feb 09, 2018 at 01:48:49PM +0100, Hans Verkuil wrote:
> On 02/09/18 13:38, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Fri, Feb 09, 2018 at 12:56:37PM +0100, Hans Verkuil wrote:
> >> On 02/09/18 12:46, Sakari Ailus wrote:
> >>> Hi Hans,
> >>>
> >>> On Thu, Feb 08, 2018 at 09:36:44AM +0100, Hans Verkuil wrote:
> >>>> If the subdev did not define any controls, then return -ENOTTY if
> >>>> userspace attempts to call these ioctls.
> >>>>
> >>>> The control framework functions will return -EINVAL, not -ENOTTY if
> >>>> vfh->ctrl_handler is NULL.
> >>>>
> >>>> Several of these framework functions are also called directly from
> >>>> drivers, so I don't want to change the error code there.
> >>>>
> >>>> Found with vimc and v4l2-compliance.
> >>>>
> >>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>
> >>> Thanks for the patch.
> >>>
> >>> If the handler is NULL, can there be support for the IOCTL at all? I.e.
> >>> should the missing handler as such result in returning -ENOTTY from these
> >>> functions instead of -EINVAL?
> >>
> >> I didn't dare change the control framework. Some of these v4l2_... functions
> >> are called by drivers and I didn't want to analyze them all. If these
> >> functions were only called by v4l2-ioctl.c and v4l2-subdev.c, then I'd have
> >> changed it in v4l2-ctrls.c, but that's not the case.
> >>
> >> It would be a useful project to replace all calls from drivers to these
> >> functions (they really shouldn't be used by drivers), but that is out-of-scope
> >> of this patch.
> > 
> > Is your concern that the caller could check the return value and do
> > something based on particular error code it gets?
> 
> Or that the handler is NULL and it returns ENOTTY to userspace. You can have
> multiple control handlers, some of which might be NULL. It's all unlikely,
> but the code needs to be analyzed and that takes time. Hmm, atomisp is
> definitely a big user of these functions.
> 
> Also, the real issue is the use of these functions by drivers. What I want
> to do is to have the drivers use the proper functions, then I can move those
> functions to the core and stop exporting them. And at that moment they can
> return -ENOTTY instead of -EINVAL.
> 
> A worthwhile project, but right now I just want to fix v4l2-subdev.c.

Fair enough. How about adding a TODO comment on this in either in the
control framework or where the additional checks are now put, to avoid
forgetting it?

With that,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> 
> Regards,
> 
> 	Hans
> 
> > Based on a quick glance there are a few tens of places these functions are
> > used in drivers. Some seems legitimate; the caller having another device
> > where a control needs to be accessed, for instance.
> > 
> > And if handler is NULL, -ENOTTY appears to be a more suitable return value
> > in a lot of the cases (and in many others it makes no difference).
> > 
> > I wouldn't say this is something that should hold back addressing this in
> > the control framework instead.
> > 
> > I can submit a patch if you'd prefer that instead.
> > 
> >>
> >> Regards,
> >>
> >> 	Hans
> >>
> >>>
> >>>> ---
> >>>>  drivers/media/v4l2-core/v4l2-subdev.c | 16 ++++++++++++++++
> >>>>  1 file changed, 16 insertions(+)
> >>>>
> >>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>> index 43fefa73e0a3..be7a19272614 100644
> >>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>> @@ -187,27 +187,43 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >>>>  
> >>>>  	switch (cmd) {
> >>>>  	case VIDIOC_QUERYCTRL:
> >>>> +		if (!vfh->ctrl_handler)
> >>>> +			return -ENOTTY;
> >>>>  		return v4l2_queryctrl(vfh->ctrl_handler, arg);
> >>>>  
> >>>>  	case VIDIOC_QUERY_EXT_CTRL:
> >>>> +		if (!vfh->ctrl_handler)
> >>>> +			return -ENOTTY;
> >>>>  		return v4l2_query_ext_ctrl(vfh->ctrl_handler, arg);
> >>>>  
> >>>>  	case VIDIOC_QUERYMENU:
> >>>> +		if (!vfh->ctrl_handler)
> >>>> +			return -ENOTTY;
> >>>>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
> >>>>  
> >>>>  	case VIDIOC_G_CTRL:
> >>>> +		if (!vfh->ctrl_handler)
> >>>> +			return -ENOTTY;
> >>>>  		return v4l2_g_ctrl(vfh->ctrl_handler, arg);
> >>>>  
> >>>>  	case VIDIOC_S_CTRL:
> >>>> +		if (!vfh->ctrl_handler)
> >>>> +			return -ENOTTY;
> >>>>  		return v4l2_s_ctrl(vfh, vfh->ctrl_handler, arg);
> >>>>  
> >>>>  	case VIDIOC_G_EXT_CTRLS:
> >>>> +		if (!vfh->ctrl_handler)
> >>>> +			return -ENOTTY;
> >>>>  		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg);
> >>>>  
> >>>>  	case VIDIOC_S_EXT_CTRLS:
> >>>> +		if (!vfh->ctrl_handler)
> >>>> +			return -ENOTTY;
> >>>>  		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
> >>>>  
> >>>>  	case VIDIOC_TRY_EXT_CTRLS:
> >>>> +		if (!vfh->ctrl_handler)
> >>>> +			return -ENOTTY;
> >>>>  		return v4l2_try_ext_ctrls(vfh->ctrl_handler, arg);
> >>>>  
> >>>>  	case VIDIOC_DQEVENT:
> >>>
> >>
> > 
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCHv2 06/15] v4l2-subdev: implement VIDIOC_DBG_G_CHIP_INFO ioctl

[Hans Verkuil]

On 02/09/18 14:04, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Friday, 9 February 2018 15:00:53 EET Hans Verkuil wrote:
>> On 02/09/18 13:44, Sakari Ailus wrote:
>>> On Fri, Feb 09, 2018 at 01:18:18PM +0100, Hans Verkuil wrote:
>>>> On 02/09/18 13:01, Sakari Ailus wrote:
>>>>> On Thu, Feb 08, 2018 at 09:36:46AM +0100, Hans Verkuil wrote:
>>>>>> The VIDIOC_DBG_G/S_REGISTER ioctls imply that VIDIOC_DBG_G_CHIP_INFO is
>>>>>> also present, since without that you cannot use v4l2-dbg.
>>>>>>
>>>>>> Just like the implementation in v4l2-ioctl.c this can be implemented in
>>>>>> the
>>>>>> core and no drivers need to be modified.
>>>>>>
>>>>>> It also makes it possible for v4l2-compliance to properly test the
>>>>>> VIDIOC_DBG_G/S_REGISTER ioctls.
>>>>>>
>>>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>> ---
>>>>>>
>>>>>>  drivers/media/v4l2-core/v4l2-subdev.c | 13 +++++++++++++
>>>>>>  1 file changed, 13 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> b/drivers/media/v4l2-core/v4l2-subdev.c index
>>>>>> 6cabfa32d2ed..2a5b5a3fa7a3 100644
>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> @@ -255,6 +255,19 @@ static long subdev_do_ioctl(struct file *file,
>>>>>> unsigned int cmd, void *arg)>>>> 
>>>>>>  			return -EPERM;
>>>>>>  		
>>>>>>  		return v4l2_subdev_call(sd, core, s_register, p);
>>>>>>  	
>>>>>>  	}
>>>>>>
>>>>>> +	case VIDIOC_DBG_G_CHIP_INFO:
>>>>>> +	{
>>>>>> +		struct v4l2_dbg_chip_info *p = arg;
>>>>>> +
>>>>>> +		if (p->match.type != V4L2_CHIP_MATCH_SUBDEV || p->match.addr)
>>>>>> +			return -EINVAL;
>>>>>> +		if (sd->ops->core && sd->ops->core->s_register)
>>>>>> +			p->flags |= V4L2_CHIP_FL_WRITABLE;
>>>>>> +		if (sd->ops->core && sd->ops->core->g_register)
>>>>>> +			p->flags |= V4L2_CHIP_FL_READABLE;
>>>>>> +		strlcpy(p->name, sd->name, sizeof(p->name));
>>>>>> +		return 0;
>>>>>> +	}
>>>>>
>>>>> This is effectively doing the same as debugfs except that it's specific
>>>>> to V4L2. I don't think we should endorse its use, and especially not
>>>>> without a real use case.
>>>>
>>>> We (Cisco) use it all the time. Furthermore, this works for any bus, not
>>>> just i2c. Also spi, internal register busses, etc.
>>>>
>>>> It's been in use for many years. More importantly, there is no excuse to
>>>> have only half the API implemented.
>>>>
>>>> It's all fine to talk about debugfs, but are you going to make that? This
>>>> API works, it's supported by v4l2-dbg, it's in use. Now, let's at least
>>>> make it pass v4l2-compliance.
>>>>
>>>> I agree, if we would redesign it, we would use debugfs. But I think it
>>>> didn't even exist when this was made. So this API is here to stay and
>>>> all it takes is this ioctl of code to add the missing piece for subdevs.
>>>>
>>>> Nobody is going to make a replacement for this using debugfs. Why spend
>>>> effort on it if we already have an API for this?
>>>
>>> It's not the first case when a more generic API replaces a subsystem
>>> specific one. We have another conversion to make, switching from
>>> implementing s_power() callback in drivers to runtime PM for instance.
>>>
>>> I simply want to point out that this patch is endorsing something which is
>>> obsolete and not needed: no-one has complained about the lack of this for
>>> sub-devices, haven't they?
>>>
>>> I'd just remove the check from v4l-compliance or make it optional. New
>>> drivers should use debugfs instead if something like that is needed.
>>
>> You are correct in one respect: we use this API, but with video devices.
>> So subdevices support the g/s_register ops, and they are called via
>> /dev/videoX.
>>
>> We can remove the ioctl support from v4l2-subdev.c (not the g/s_register
>> ops!). Without VIDIOC_DBG_G_CHIP_INFO I don't think v4l2-dbg is usable.
>> Although it is always possible to call the ioctl directly, of course.
>>
>> So if Mauro would agree to this, the DBG ioctl support in v4l2-subdev can be
>> removed.
> 
> That would be my preferred option.
> 
>> But either remove them, or add this ioctl. Don't leave it in a zombie state.
>>
>> Personally I see no harm whatsoever in just adding VIDIOC_DBG_G_CHIP_INFO.
>> If someone ever makes a patch to switch over to debugfs then these ioctls
>> can be removed.
>>
>> BTW, how would new drivers use debugfs for this? Does regmap provide such
>> access?
> 
> Before attempting to provide an answer, as I've never used those ioctls 
> myself, could you please give us a bit more information about the use cases 
> you have at Cisco for this ?
> 

Exactly what it was made for: debugging issues by reading/writing registers
on the fly. It's very similar to using i2cget/set for i2c devices, except it
can also be used for registers in e.g. an IP block. It's also a bit safer
since you can filter (if needed) which addresses can be written.

One thing I have seen in a code review from Sakari for a sensor driver that
implemented the g/s_dbg_register ops: I think it is not right to reject a
patch based on that. It is used (albeit not through a v4l-subdev device
node) and unless someone can provide a working alternative (and 'make something
in debugfs' is not a valid alternative) it should not be a reason for
rejecting it.

It works, it's been there for ages, and we never said that it is suddenly
no longer allowed to be used. And we certainly have not provided an alternative
to this API.

Regards,

	Hans

Re: [PATCHv2 04/15] v4l2-subdev: without controls return -ENOTTY

[Hans Verkuil]

On 02/09/18 14:09, Sakari Ailus wrote:
> On Fri, Feb 09, 2018 at 01:48:49PM +0100, Hans Verkuil wrote:
>> On 02/09/18 13:38, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Fri, Feb 09, 2018 at 12:56:37PM +0100, Hans Verkuil wrote:
>>>> On 02/09/18 12:46, Sakari Ailus wrote:
>>>>> Hi Hans,
>>>>>
>>>>> On Thu, Feb 08, 2018 at 09:36:44AM +0100, Hans Verkuil wrote:
>>>>>> If the subdev did not define any controls, then return -ENOTTY if
>>>>>> userspace attempts to call these ioctls.
>>>>>>
>>>>>> The control framework functions will return -EINVAL, not -ENOTTY if
>>>>>> vfh->ctrl_handler is NULL.
>>>>>>
>>>>>> Several of these framework functions are also called directly from
>>>>>> drivers, so I don't want to change the error code there.
>>>>>>
>>>>>> Found with vimc and v4l2-compliance.
>>>>>>
>>>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>
>>>>> Thanks for the patch.
>>>>>
>>>>> If the handler is NULL, can there be support for the IOCTL at all? I.e.
>>>>> should the missing handler as such result in returning -ENOTTY from these
>>>>> functions instead of -EINVAL?
>>>>
>>>> I didn't dare change the control framework. Some of these v4l2_... functions
>>>> are called by drivers and I didn't want to analyze them all. If these
>>>> functions were only called by v4l2-ioctl.c and v4l2-subdev.c, then I'd have
>>>> changed it in v4l2-ctrls.c, but that's not the case.
>>>>
>>>> It would be a useful project to replace all calls from drivers to these
>>>> functions (they really shouldn't be used by drivers), but that is out-of-scope
>>>> of this patch.
>>>
>>> Is your concern that the caller could check the return value and do
>>> something based on particular error code it gets?
>>
>> Or that the handler is NULL and it returns ENOTTY to userspace. You can have
>> multiple control handlers, some of which might be NULL. It's all unlikely,
>> but the code needs to be analyzed and that takes time. Hmm, atomisp is
>> definitely a big user of these functions.
>>
>> Also, the real issue is the use of these functions by drivers. What I want
>> to do is to have the drivers use the proper functions, then I can move those
>> functions to the core and stop exporting them. And at that moment they can
>> return -ENOTTY instead of -EINVAL.
>>
>> A worthwhile project, but right now I just want to fix v4l2-subdev.c.
> 
> Fair enough. How about adding a TODO comment on this in either in the
> control framework or where the additional checks are now put, to avoid
> forgetting it?

It's certainly worth a TODO comment, I'll add that. Very good point.

Regards,

	Hans

> 
> With that,
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>> Based on a quick glance there are a few tens of places these functions are
>>> used in drivers. Some seems legitimate; the caller having another device
>>> where a control needs to be accessed, for instance.
>>>
>>> And if handler is NULL, -ENOTTY appears to be a more suitable return value
>>> in a lot of the cases (and in many others it makes no difference).
>>>
>>> I wouldn't say this is something that should hold back addressing this in
>>> the control framework instead.
>>>
>>> I can submit a patch if you'd prefer that instead.
>>>
>>>>
>>>> Regards,
>>>>
>>>> 	Hans
>>>>
>>>>>
>>>>>> ---
>>>>>>  drivers/media/v4l2-core/v4l2-subdev.c | 16 ++++++++++++++++
>>>>>>  1 file changed, 16 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> index 43fefa73e0a3..be7a19272614 100644
>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> @@ -187,27 +187,43 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>>>>>  
>>>>>>  	switch (cmd) {
>>>>>>  	case VIDIOC_QUERYCTRL:
>>>>>> +		if (!vfh->ctrl_handler)
>>>>>> +			return -ENOTTY;
>>>>>>  		return v4l2_queryctrl(vfh->ctrl_handler, arg);
>>>>>>  
>>>>>>  	case VIDIOC_QUERY_EXT_CTRL:
>>>>>> +		if (!vfh->ctrl_handler)
>>>>>> +			return -ENOTTY;
>>>>>>  		return v4l2_query_ext_ctrl(vfh->ctrl_handler, arg);
>>>>>>  
>>>>>>  	case VIDIOC_QUERYMENU:
>>>>>> +		if (!vfh->ctrl_handler)
>>>>>> +			return -ENOTTY;
>>>>>>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
>>>>>>  
>>>>>>  	case VIDIOC_G_CTRL:
>>>>>> +		if (!vfh->ctrl_handler)
>>>>>> +			return -ENOTTY;
>>>>>>  		return v4l2_g_ctrl(vfh->ctrl_handler, arg);
>>>>>>  
>>>>>>  	case VIDIOC_S_CTRL:
>>>>>> +		if (!vfh->ctrl_handler)
>>>>>> +			return -ENOTTY;
>>>>>>  		return v4l2_s_ctrl(vfh, vfh->ctrl_handler, arg);
>>>>>>  
>>>>>>  	case VIDIOC_G_EXT_CTRLS:
>>>>>> +		if (!vfh->ctrl_handler)
>>>>>> +			return -ENOTTY;
>>>>>>  		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg);
>>>>>>  
>>>>>>  	case VIDIOC_S_EXT_CTRLS:
>>>>>> +		if (!vfh->ctrl_handler)
>>>>>> +			return -ENOTTY;
>>>>>>  		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
>>>>>>  
>>>>>>  	case VIDIOC_TRY_EXT_CTRLS:
>>>>>> +		if (!vfh->ctrl_handler)
>>>>>> +			return -ENOTTY;
>>>>>>  		return v4l2_try_ext_ctrls(vfh->ctrl_handler, arg);
>>>>>>  
>>>>>>  	case VIDIOC_DQEVENT:
>>>>>
>>>>
>>>
>>
> 

Re: [PATCHv2 06/15] v4l2-subdev: implement VIDIOC_DBG_G_CHIP_INFO ioctl

[Laurent Pinchart]

Hi Hans,

On Friday, 9 February 2018 15:13:26 EET Hans Verkuil wrote:
> On 02/09/18 14:04, Laurent Pinchart wrote:
> > On Friday, 9 February 2018 15:00:53 EET Hans Verkuil wrote:
> >> On 02/09/18 13:44, Sakari Ailus wrote:
> >>> On Fri, Feb 09, 2018 at 01:18:18PM +0100, Hans Verkuil wrote:
> >>>> On 02/09/18 13:01, Sakari Ailus wrote:
> >>>>> On Thu, Feb 08, 2018 at 09:36:46AM +0100, Hans Verkuil wrote:
> >>>>>> The VIDIOC_DBG_G/S_REGISTER ioctls imply that VIDIOC_DBG_G_CHIP_INFO
> >>>>>> is also present, since without that you cannot use v4l2-dbg.
> >>>>>> 
> >>>>>> Just like the implementation in v4l2-ioctl.c this can be implemented
> >>>>>> in the core and no drivers need to be modified.
> >>>>>> 
> >>>>>> It also makes it possible for v4l2-compliance to properly test the
> >>>>>> VIDIOC_DBG_G/S_REGISTER ioctls.
> >>>>>> 
> >>>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>>>> ---
> >>>>>> 
> >>>>>>  drivers/media/v4l2-core/v4l2-subdev.c | 13 +++++++++++++
> >>>>>>  1 file changed, 13 insertions(+)
> >>>>>> 
> >>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>> b/drivers/media/v4l2-core/v4l2-subdev.c index
> >>>>>> 6cabfa32d2ed..2a5b5a3fa7a3 100644
> >>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>> @@ -255,6 +255,19 @@ static long subdev_do_ioctl(struct file *file,
> >>>>>> unsigned int cmd, void *arg)>>>>
> >>>>>> 
> >>>>>>  			return -EPERM;
> >>>>>>  		return v4l2_subdev_call(sd, core, s_register, p);
> >>>>>>  	}
> >>>>>> +	case VIDIOC_DBG_G_CHIP_INFO:
> >>>>>> +	{
> >>>>>> +		struct v4l2_dbg_chip_info *p = arg;
> >>>>>> +
> >>>>>> +		if (p->match.type != V4L2_CHIP_MATCH_SUBDEV || p->match.addr)
> >>>>>> +			return -EINVAL;
> >>>>>> +		if (sd->ops->core && sd->ops->core->s_register)
> >>>>>> +			p->flags |= V4L2_CHIP_FL_WRITABLE;
> >>>>>> +		if (sd->ops->core && sd->ops->core->g_register)
> >>>>>> +			p->flags |= V4L2_CHIP_FL_READABLE;
> >>>>>> +		strlcpy(p->name, sd->name, sizeof(p->name));
> >>>>>> +		return 0;
> >>>>>> +	}
> >>>>> 
> >>>>> This is effectively doing the same as debugfs except that it's
> >>>>> specific to V4L2. I don't think we should endorse its use, and
> >>>>> especially not without a real use case.
> >>>> 
> >>>> We (Cisco) use it all the time. Furthermore, this works for any bus,
> >>>> not just i2c. Also spi, internal register busses, etc.
> >>>> 
> >>>> It's been in use for many years. More importantly, there is no excuse
> >>>> to have only half the API implemented.
> >>>> 
> >>>> It's all fine to talk about debugfs, but are you going to make that?
> >>>> This API works, it's supported by v4l2-dbg, it's in use. Now, let's at
> >>>> least make it pass v4l2-compliance.
> >>>> 
> >>>> I agree, if we would redesign it, we would use debugfs. But I think it
> >>>> didn't even exist when this was made. So this API is here to stay and
> >>>> all it takes is this ioctl of code to add the missing piece for
> >>>> subdevs.
> >>>> 
> >>>> Nobody is going to make a replacement for this using debugfs. Why spend
> >>>> effort on it if we already have an API for this?
> >>> 
> >>> It's not the first case when a more generic API replaces a subsystem
> >>> specific one. We have another conversion to make, switching from
> >>> implementing s_power() callback in drivers to runtime PM for instance.
> >>> 
> >>> I simply want to point out that this patch is endorsing something which
> >>> is obsolete and not needed: no-one has complained about the lack of this
> >>> for sub-devices, haven't they?
> >>> 
> >>> I'd just remove the check from v4l-compliance or make it optional. New
> >>> drivers should use debugfs instead if something like that is needed.
> >> 
> >> You are correct in one respect: we use this API, but with video devices.
> >> So subdevices support the g/s_register ops, and they are called via
> >> /dev/videoX.
> >> 
> >> We can remove the ioctl support from v4l2-subdev.c (not the g/s_register
> >> ops!). Without VIDIOC_DBG_G_CHIP_INFO I don't think v4l2-dbg is usable.
> >> Although it is always possible to call the ioctl directly, of course.
> >> 
> >> So if Mauro would agree to this, the DBG ioctl support in v4l2-subdev can
> >> be removed.
> > 
> > That would be my preferred option.
> > 
> >> But either remove them, or add this ioctl. Don't leave it in a zombie
> >> state.
> >> 
> >> Personally I see no harm whatsoever in just adding
> >> VIDIOC_DBG_G_CHIP_INFO. If someone ever makes a patch to switch over to
> >> debugfs then these ioctls can be removed.
> >> 
> >> BTW, how would new drivers use debugfs for this? Does regmap provide such
> >> access?
> > 
> > Before attempting to provide an answer, as I've never used those ioctls
> > myself, could you please give us a bit more information about the use
> > cases you have at Cisco for this ?
> 
> Exactly what it was made for: debugging issues by reading/writing registers
> on the fly. It's very similar to using i2cget/set for i2c devices, except it
> can also be used for registers in e.g. an IP block. It's also a bit safer
> since you can filter (if needed) which addresses can be written.

We're talking about debugging during development, not in production, right ?

> One thing I have seen in a code review from Sakari for a sensor driver that
> implemented the g/s_dbg_register ops: I think it is not right to reject a
> patch based on that. It is used (albeit not through a v4l-subdev device
> node) and unless someone can provide a working alternative (and 'make
> something in debugfs' is not a valid alternative) it should not be a reason
> for rejecting it.
> 
> It works, it's been there for ages, and we never said that it is suddenly
> no longer allowed to be used. And we certainly have not provided an
> alternative to this API.

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 11/15] media-device.c: zero reserved field

[Hans Verkuil]

On 02/09/18 14:04, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Feb 09, 2018 at 01:52:50PM +0100, Hans Verkuil wrote:
>> On 02/09/18 13:46, Sakari Ailus wrote:
>>> On Fri, Feb 09, 2018 at 01:20:41PM +0100, Hans Verkuil wrote:
>>>> On 02/09/18 13:17, Sakari Ailus wrote:
>>>>> On Thu, Feb 08, 2018 at 09:36:51AM +0100, Hans Verkuil wrote:
>>>>>> MEDIA_IOC_SETUP_LINK didn't zero the reserved field of the media_link_desc
>>>>>> struct. Do so in media_device_setup_link().
>>>>>>
>>>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>> ---
>>>>>>  drivers/media/media-device.c | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>>>>> index e79f72b8b858..afbf23a19e16 100644
>>>>>> --- a/drivers/media/media-device.c
>>>>>> +++ b/drivers/media/media-device.c
>>>>>> @@ -218,6 +218,8 @@ static long media_device_setup_link(struct media_device *mdev,
>>>>>>  	if (link == NULL)
>>>>>>  		return -EINVAL;
>>>>>>  
>>>>>> +	memset(linkd->reserved, 0, sizeof(linkd->reserved));
>>>>>> +
>>>>>
>>>>> Doesn't media_device_enum_links() need the same for its reserved field?
>>>>
>>>> enum_links() already zeroes this (actually the whole media_link_desc struct is zeroed).
>>>
>>> I can't see that being done in here and I also don't mean the compat
>>> variant. Can you point me to it?
>>>
>>
>> static long media_device_enum_links(struct media_device *mdev,
>>                                     struct media_links_enum *links)
>> {
>>         struct media_entity *entity;
>>
>>         entity = find_entity(mdev, links->entity);
>>         if (entity == NULL)
>>                 return -EINVAL;
>>
>>         if (links->pads) {
>> ...
>>         }
>>
>>         if (links->links) {
>>                 struct media_link *link;
>>                 struct media_link_desc __user *ulink_desc = links->links;
>>
>>                 list_for_each_entry(link, &entity->links, list) {
>>                         struct media_link_desc klink_desc;
>>
>>                         /* Ignore backlinks. */
>>                         if (link->source->entity != entity)
>>                                 continue;
>>                         memset(&klink_desc, 0, sizeof(klink_desc));
>> 			// ^^^^^^^^^^^ zeroed here
>> 	
>>                         media_device_kpad_to_upad(link->source,
>>                                                   &klink_desc.source);
>>                         media_device_kpad_to_upad(link->sink,
>>                                                   &klink_desc.sink);
>>                         klink_desc.flags = link->flags;
>>                         if (copy_to_user(ulink_desc, &klink_desc,
>>                                          sizeof(*ulink_desc)))
>> 			// ^^^^^^^ copied back to userspace (including zeroed reserved array) here
> 
> We are indeed talking about a different reserved field. I mean the one in
> struct media_links_enum .

Ah! I missed that one. I added a check to v4l2-compliance and posted a patch
fixing this.

I thought you meant the reserved field in the media_link_desc structs. That was
very confusing :-)

Regards,

	Hans

Re: [PATCHv2 06/15] v4l2-subdev: implement VIDIOC_DBG_G_CHIP_INFO ioctl

[Hans Verkuil]

On 02/09/18 14:21, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Friday, 9 February 2018 15:13:26 EET Hans Verkuil wrote:
>> On 02/09/18 14:04, Laurent Pinchart wrote:
>>> On Friday, 9 February 2018 15:00:53 EET Hans Verkuil wrote:
>>>> On 02/09/18 13:44, Sakari Ailus wrote:
>>>>> On Fri, Feb 09, 2018 at 01:18:18PM +0100, Hans Verkuil wrote:
>>>>>> On 02/09/18 13:01, Sakari Ailus wrote:
>>>>>>> On Thu, Feb 08, 2018 at 09:36:46AM +0100, Hans Verkuil wrote:
>>>>>>>> The VIDIOC_DBG_G/S_REGISTER ioctls imply that VIDIOC_DBG_G_CHIP_INFO
>>>>>>>> is also present, since without that you cannot use v4l2-dbg.
>>>>>>>>
>>>>>>>> Just like the implementation in v4l2-ioctl.c this can be implemented
>>>>>>>> in the core and no drivers need to be modified.
>>>>>>>>
>>>>>>>> It also makes it possible for v4l2-compliance to properly test the
>>>>>>>> VIDIOC_DBG_G/S_REGISTER ioctls.
>>>>>>>>
>>>>>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>  drivers/media/v4l2-core/v4l2-subdev.c | 13 +++++++++++++
>>>>>>>>  1 file changed, 13 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>>>> b/drivers/media/v4l2-core/v4l2-subdev.c index
>>>>>>>> 6cabfa32d2ed..2a5b5a3fa7a3 100644
>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>>>> @@ -255,6 +255,19 @@ static long subdev_do_ioctl(struct file *file,
>>>>>>>> unsigned int cmd, void *arg)>>>>
>>>>>>>>
>>>>>>>>  			return -EPERM;
>>>>>>>>  		return v4l2_subdev_call(sd, core, s_register, p);
>>>>>>>>  	}
>>>>>>>> +	case VIDIOC_DBG_G_CHIP_INFO:
>>>>>>>> +	{
>>>>>>>> +		struct v4l2_dbg_chip_info *p = arg;
>>>>>>>> +
>>>>>>>> +		if (p->match.type != V4L2_CHIP_MATCH_SUBDEV || p->match.addr)
>>>>>>>> +			return -EINVAL;
>>>>>>>> +		if (sd->ops->core && sd->ops->core->s_register)
>>>>>>>> +			p->flags |= V4L2_CHIP_FL_WRITABLE;
>>>>>>>> +		if (sd->ops->core && sd->ops->core->g_register)
>>>>>>>> +			p->flags |= V4L2_CHIP_FL_READABLE;
>>>>>>>> +		strlcpy(p->name, sd->name, sizeof(p->name));
>>>>>>>> +		return 0;
>>>>>>>> +	}
>>>>>>>
>>>>>>> This is effectively doing the same as debugfs except that it's
>>>>>>> specific to V4L2. I don't think we should endorse its use, and
>>>>>>> especially not without a real use case.
>>>>>>
>>>>>> We (Cisco) use it all the time. Furthermore, this works for any bus,
>>>>>> not just i2c. Also spi, internal register busses, etc.
>>>>>>
>>>>>> It's been in use for many years. More importantly, there is no excuse
>>>>>> to have only half the API implemented.
>>>>>>
>>>>>> It's all fine to talk about debugfs, but are you going to make that?
>>>>>> This API works, it's supported by v4l2-dbg, it's in use. Now, let's at
>>>>>> least make it pass v4l2-compliance.
>>>>>>
>>>>>> I agree, if we would redesign it, we would use debugfs. But I think it
>>>>>> didn't even exist when this was made. So this API is here to stay and
>>>>>> all it takes is this ioctl of code to add the missing piece for
>>>>>> subdevs.
>>>>>>
>>>>>> Nobody is going to make a replacement for this using debugfs. Why spend
>>>>>> effort on it if we already have an API for this?
>>>>>
>>>>> It's not the first case when a more generic API replaces a subsystem
>>>>> specific one. We have another conversion to make, switching from
>>>>> implementing s_power() callback in drivers to runtime PM for instance.
>>>>>
>>>>> I simply want to point out that this patch is endorsing something which
>>>>> is obsolete and not needed: no-one has complained about the lack of this
>>>>> for sub-devices, haven't they?
>>>>>
>>>>> I'd just remove the check from v4l-compliance or make it optional. New
>>>>> drivers should use debugfs instead if something like that is needed.
>>>>
>>>> You are correct in one respect: we use this API, but with video devices.
>>>> So subdevices support the g/s_register ops, and they are called via
>>>> /dev/videoX.
>>>>
>>>> We can remove the ioctl support from v4l2-subdev.c (not the g/s_register
>>>> ops!). Without VIDIOC_DBG_G_CHIP_INFO I don't think v4l2-dbg is usable.
>>>> Although it is always possible to call the ioctl directly, of course.
>>>>
>>>> So if Mauro would agree to this, the DBG ioctl support in v4l2-subdev can
>>>> be removed.
>>>
>>> That would be my preferred option.
>>>
>>>> But either remove them, or add this ioctl. Don't leave it in a zombie
>>>> state.
>>>>
>>>> Personally I see no harm whatsoever in just adding
>>>> VIDIOC_DBG_G_CHIP_INFO. If someone ever makes a patch to switch over to
>>>> debugfs then these ioctls can be removed.
>>>>
>>>> BTW, how would new drivers use debugfs for this? Does regmap provide such
>>>> access?
>>>
>>> Before attempting to provide an answer, as I've never used those ioctls
>>> myself, could you please give us a bit more information about the use
>>> cases you have at Cisco for this ?
>>
>> Exactly what it was made for: debugging issues by reading/writing registers
>> on the fly. It's very similar to using i2cget/set for i2c devices, except it
>> can also be used for registers in e.g. an IP block. It's also a bit safer
>> since you can filter (if needed) which addresses can be written.
> 
> We're talking about debugging during development, not in production, right ?

Sometimes you need to debug issues even when it is in production.

A major difference between sensors and video receivers is that you (generally)
know what the sensor sends you, but for video receivers you have no control
whatsoever of what it is being sent by a source. Being able to read and in
rare cases write registers can be very helpful to get to the bottom of the
problem.

So yes, this is important functionality. Hardly something you use every day,
but it's great that you have the option.

Regards,

	Hans