On 9/17/19 11:54 AM, Hans Verkuil wrote:
> On 9/13/19 8:57 AM, Vandana BN wrote:
>> Add VFL_TYPE_METADATA, to detect devices of type metadata and
>> to disable unneeded ioctls.
>> Without this change, format ioctls for both video and metadata devices
>> could be called on both device nodes.This is true for other ioctls as
>> well.
>>
>> Signed-off-by: Vandana BN <bnvandana@xxxxxxxxx>
>> ---
>> V3 Updated commit message
>>
>> drivers/media/v4l2-core/v4l2-dev.c | 57 ++++++++++++++++++----------
>> drivers/media/v4l2-core/v4l2-ioctl.c | 5 ++-
>> include/media/v4l2-dev.h | 2 +
>> 3 files changed, 41 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index 4037689a945a..5f2ead772c5f 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -112,6 +112,7 @@ static inline unsigned long *devnode_bits(enum vfl_devnode_type vfl_type)
>> one single bitmap for the purposes of finding a free node number
>> since all those unassigned types use the same minor range. */
>> int idx = (vfl_type > VFL_TYPE_RADIO) ? VFL_TYPE_MAX - 1 : vfl_type;
>> + idx = (vfl_type == VFL_TYPE_METADATA) ? VFL_TYPE_GRABBER : vfl_type;
>>
>> return devnode_nums[idx];
>> }
>> @@ -119,7 +120,9 @@ static inline unsigned long *devnode_bits(enum vfl_devnode_type vfl_type)
>> /* Return the bitmap corresponding to vfl_type. */
>> static inline unsigned long *devnode_bits(enum vfl_devnode_type vfl_type)
>> {
>> - return devnode_nums[vfl_type];
>> + int idx = (vfl_type == VFL_TYPE_METADATA) ? VFL_TYPE_GRABBER : vfl_type;
>> +
>> + return devnode_nums[idx];
>> }
>> #endif
>>
>> @@ -540,6 +543,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>> bool is_radio = vdev->vfl_type == VFL_TYPE_RADIO;
>> bool is_sdr = vdev->vfl_type == VFL_TYPE_SDR;
>> bool is_tch = vdev->vfl_type == VFL_TYPE_TOUCH;
>> + bool is_meta = vdev->vfl_type == VFL_TYPE_METADATA;
>> bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
>> bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
>>
>> @@ -571,8 +575,10 @@ static void determine_valid_ioctls(struct video_device *vdev)
>> set_bit(_IOC_NR(VIDIOC_TRY_EXT_CTRLS), valid_ioctls);
>> if (vdev->ctrl_handler || ops->vidioc_querymenu)
>> set_bit(_IOC_NR(VIDIOC_QUERYMENU), valid_ioctls);
>> - SET_VALID_IOCTL(ops, VIDIOC_G_FREQUENCY, vidioc_g_frequency);
>> - SET_VALID_IOCTL(ops, VIDIOC_S_FREQUENCY, vidioc_s_frequency);
>> + if (!is_meta) {
>> + SET_VALID_IOCTL(ops, VIDIOC_G_FREQUENCY, vidioc_g_frequency);
>> + SET_VALID_IOCTL(ops, VIDIOC_S_FREQUENCY, vidioc_s_frequency);
>> + }
>> SET_VALID_IOCTL(ops, VIDIOC_LOG_STATUS, vidioc_log_status);
>> #ifdef CONFIG_VIDEO_ADV_DEBUG
>> set_bit(_IOC_NR(VIDIOC_DBG_G_CHIP_INFO), valid_ioctls);
>> @@ -589,37 +595,29 @@ static void determine_valid_ioctls(struct video_device *vdev)
>> if (is_vid || is_tch) {
>> /* video and metadata specific ioctls */
>> if ((is_rx && (ops->vidioc_enum_fmt_vid_cap ||
>> - ops->vidioc_enum_fmt_vid_overlay ||
>> - ops->vidioc_enum_fmt_meta_cap)) ||
>> - (is_tx && (ops->vidioc_enum_fmt_vid_out ||
>> - ops->vidioc_enum_fmt_meta_out)))
>> + ops->vidioc_enum_fmt_vid_overlay)) ||
>> + (is_tx && ops->vidioc_enum_fmt_vid_out))
>> set_bit(_IOC_NR(VIDIOC_ENUM_FMT), valid_ioctls);
>> if ((is_rx && (ops->vidioc_g_fmt_vid_cap ||
>> ops->vidioc_g_fmt_vid_cap_mplane ||
>> - ops->vidioc_g_fmt_vid_overlay ||
>> - ops->vidioc_g_fmt_meta_cap)) ||
>> + ops->vidioc_g_fmt_vid_overlay)) ||
>> (is_tx && (ops->vidioc_g_fmt_vid_out ||
>> ops->vidioc_g_fmt_vid_out_mplane ||
>> - ops->vidioc_g_fmt_vid_out_overlay ||
>> - ops->vidioc_g_fmt_meta_out)))
>> + ops->vidioc_g_fmt_vid_out_overlay)))
>> set_bit(_IOC_NR(VIDIOC_G_FMT), valid_ioctls);
>> if ((is_rx && (ops->vidioc_s_fmt_vid_cap ||
>> ops->vidioc_s_fmt_vid_cap_mplane ||
>> - ops->vidioc_s_fmt_vid_overlay ||
>> - ops->vidioc_s_fmt_meta_cap)) ||
>> + ops->vidioc_s_fmt_vid_overlay)) ||
>> (is_tx && (ops->vidioc_s_fmt_vid_out ||
>> ops->vidioc_s_fmt_vid_out_mplane ||
>> - ops->vidioc_s_fmt_vid_out_overlay ||
>> - ops->vidioc_s_fmt_meta_out)))
>> + ops->vidioc_s_fmt_vid_out_overlay)))
>> set_bit(_IOC_NR(VIDIOC_S_FMT), valid_ioctls);
>> if ((is_rx && (ops->vidioc_try_fmt_vid_cap ||
>> ops->vidioc_try_fmt_vid_cap_mplane ||
>> - ops->vidioc_try_fmt_vid_overlay ||
>> - ops->vidioc_try_fmt_meta_cap)) ||
>> + ops->vidioc_try_fmt_vid_overlay)) ||
>> (is_tx && (ops->vidioc_try_fmt_vid_out ||
>> ops->vidioc_try_fmt_vid_out_mplane ||
>> - ops->vidioc_try_fmt_vid_out_overlay ||
>> - ops->vidioc_try_fmt_meta_out)))
>> + ops->vidioc_try_fmt_vid_out_overlay)))
>> set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls);
>> SET_VALID_IOCTL(ops, VIDIOC_OVERLAY, vidioc_overlay);
>> SET_VALID_IOCTL(ops, VIDIOC_G_FBUF, vidioc_g_fbuf);
>> @@ -679,9 +677,23 @@ static void determine_valid_ioctls(struct video_device *vdev)
>> set_bit(_IOC_NR(VIDIOC_S_FMT), valid_ioctls);
>> if (ops->vidioc_try_fmt_sdr_out)
>> set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls);
>> + } else if (is_meta) {
>> + /* metadata specific ioctls */
>> + if ((is_rx && ops->vidioc_enum_fmt_meta_cap) ||
>> + (is_tx && ops->vidioc_enum_fmt_meta_out))
>> + set_bit(_IOC_NR(VIDIOC_ENUM_FMT), valid_ioctls);
>> + if ((is_rx && ops->vidioc_g_fmt_meta_cap) ||
>> + (is_tx && ops->vidioc_g_fmt_meta_out))
>> + set_bit(_IOC_NR(VIDIOC_G_FMT), valid_ioctls);
>> + if ((is_rx && ops->vidioc_s_fmt_meta_cap) ||
>> + (is_tx && ops->vidioc_s_fmt_meta_out))
>> + set_bit(_IOC_NR(VIDIOC_S_FMT), valid_ioctls);
>> + if ((is_rx && ops->vidioc_try_fmt_meta_cap) ||
>> + (is_tx && ops->vidioc_try_fmt_meta_out))
>> + set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls);
>
> After some more testing I realized that metadata devices should still support
> ENUM/G/S_INPUT for capture and ENUM/S/G_OUTPUT for output.
>
> Can you post a v4 with support for that?
Hmm, now the compliance test fails if that's added. Let me dig a bit deeper
into this.
Regards,
Hans
>
> Thanks!
>
> Hans
>
>> }
>>
>> - if (is_vid || is_vbi || is_sdr || is_tch) {
>> + if (is_vid || is_vbi || is_sdr || is_tch || is_meta) {
>> /* ioctls valid for video, metadata, vbi or sdr */
>> SET_VALID_IOCTL(ops, VIDIOC_REQBUFS, vidioc_reqbufs);
>> SET_VALID_IOCTL(ops, VIDIOC_QUERYBUF, vidioc_querybuf);
>> @@ -734,7 +746,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>> SET_VALID_IOCTL(ops, VIDIOC_G_MODULATOR, vidioc_g_modulator);
>> SET_VALID_IOCTL(ops, VIDIOC_S_MODULATOR, vidioc_s_modulator);
>> }
>> - if (is_rx) {
>> + if (is_rx && !is_meta) {
>> /* receiver only ioctls */
>> SET_VALID_IOCTL(ops, VIDIOC_G_TUNER, vidioc_g_tuner);
>> SET_VALID_IOCTL(ops, VIDIOC_S_TUNER, vidioc_s_tuner);
>> @@ -762,6 +774,7 @@ static int video_register_media_controller(struct video_device *vdev)
>>
>> switch (vdev->vfl_type) {
>> case VFL_TYPE_GRABBER:
>> + case VFL_TYPE_METADATA:
>> intf_type = MEDIA_INTF_T_V4L_VIDEO;
>> vdev->entity.function = MEDIA_ENT_F_IO_V4L;
>> break;
>> @@ -870,6 +883,7 @@ int __video_register_device(struct video_device *vdev,
>> /* Part 1: check device type */
>> switch (type) {
>> case VFL_TYPE_GRABBER:
>> + case VFL_TYPE_METADATA:
>> name_base = "video";
>> break;
>> case VFL_TYPE_VBI:
>> @@ -914,6 +928,7 @@ int __video_register_device(struct video_device *vdev,
>> * (new style). */
>> switch (type) {
>> case VFL_TYPE_GRABBER:
>> + case VFL_TYPE_METADATA:
>> minor_offset = 0;
>> minor_cnt = 64;
>> break;
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 51b912743f0f..0d71c06c82cf 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -938,6 +938,7 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type)
>> bool is_vbi = vfd->vfl_type == VFL_TYPE_VBI;
>> bool is_sdr = vfd->vfl_type == VFL_TYPE_SDR;
>> bool is_tch = vfd->vfl_type == VFL_TYPE_TOUCH;
>> + bool is_meta = vfd->vfl_type == VFL_TYPE_METADATA;
>> bool is_rx = vfd->vfl_dir != VFL_DIR_TX;
>> bool is_tx = vfd->vfl_dir != VFL_DIR_RX;
>>
>> @@ -996,11 +997,11 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type)
>> return 0;
>> break;
>> case V4L2_BUF_TYPE_META_CAPTURE:
>> - if (is_vid && is_rx && ops->vidioc_g_fmt_meta_cap)
>> + if (is_meta && is_rx && ops->vidioc_g_fmt_meta_cap)
>> return 0;
>> break;
>> case V4L2_BUF_TYPE_META_OUTPUT:
>> - if (is_vid && is_tx && ops->vidioc_g_fmt_meta_out)
>> + if (is_meta && is_tx && ops->vidioc_g_fmt_meta_out)
>> return 0;
>> break;
>> default:
>> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
>> index 48531e57cc5a..2da91d454c10 100644
>> --- a/include/media/v4l2-dev.h
>> +++ b/include/media/v4l2-dev.h
>> @@ -30,6 +30,7 @@
>> * @VFL_TYPE_SUBDEV: for V4L2 subdevices
>> * @VFL_TYPE_SDR: for Software Defined Radio tuners
>> * @VFL_TYPE_TOUCH: for touch sensors
>> + * @VFL_TYPE_METADATA: for Metadata device
>> * @VFL_TYPE_MAX: number of VFL types, must always be last in the enum
>> */
>> enum vfl_devnode_type {
>> @@ -39,6 +40,7 @@ enum vfl_devnode_type {
>> VFL_TYPE_SUBDEV,
>> VFL_TYPE_SDR,
>> VFL_TYPE_TOUCH,
>> + VFL_TYPE_METADATA,
>> VFL_TYPE_MAX /* Shall be the last one */
>> };
>>
>>
>
