On 02/21/2018 07:01 AM, Alexandre Courbot wrote: > On Wed, Feb 21, 2018 at 1:35 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote: >> On 02/20/2018 05:44 AM, Alexandre Courbot wrote: >>> Add a new VIDIOC_NEW_REQUEST ioctl, which allows to instanciate requests >>> on devices that support the request API. Requests created that way can >>> only control the device they originate from, making them suitable for >>> simple devices, but not complex pipelines. >>> >>> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxxxx> >>> --- >>> Documentation/ioctl/ioctl-number.txt | 1 + >>> drivers/media/v4l2-core/v4l2-dev.c | 2 ++ >>> drivers/media/v4l2-core/v4l2-ioctl.c | 25 +++++++++++++++++++++++++ >>> include/media/v4l2-dev.h | 2 ++ >>> include/uapi/linux/videodev2.h | 3 +++ >>> 5 files changed, 33 insertions(+) >>> >>> diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt >>> index 6501389d55b9..afdc9ed255b0 100644 >>> --- a/Documentation/ioctl/ioctl-number.txt >>> +++ b/Documentation/ioctl/ioctl-number.txt >>> @@ -286,6 +286,7 @@ Code Seq#(hex) Include File Comments >>> <mailto:oe@xxxxxxx> >>> 'z' 10-4F drivers/s390/crypto/zcrypt_api.h conflict! >>> '|' 00-7F linux/media.h >>> +'|' 80-9F linux/media-request.h >>> 0x80 00-1F linux/fb.h >>> 0x89 00-06 arch/x86/include/asm/sockios.h >>> 0x89 0B-DF linux/sockios.h >> >> This ^^^^ doesn't belong in this patch. > > Do you mean we need a separate patch for this? Yes. I now also see why you started at 0x80. Let's keep that for now. > >> >>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c >>> index 0301fe426a43..062ebee5bffc 100644 >>> --- a/drivers/media/v4l2-core/v4l2-dev.c >>> +++ b/drivers/media/v4l2-core/v4l2-dev.c >>> @@ -559,6 +559,8 @@ 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); >>> + if (vdev->req_mgr) >>> + set_bit(_IOC_NR(VIDIOC_NEW_REQUEST), valid_ioctls); >>> 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); >>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >>> index ab4968ea443f..a45fe078f8ae 100644 >>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >>> @@ -21,6 +21,7 @@ >>> >>> #include <linux/videodev2.h> >>> >>> +#include <media/media-request.h> >>> #include <media/v4l2-common.h> >>> #include <media/v4l2-ioctl.h> >>> #include <media/v4l2-ctrls.h> >>> @@ -842,6 +843,13 @@ static void v4l_print_freq_band(const void *arg, bool write_only) >>> p->rangehigh, p->modulation); >>> } >>> >>> +static void vidioc_print_new_request(const void *arg, bool write_only) >>> +{ >>> + const struct media_request_new *new = arg; >>> + >>> + pr_cont("fd=0x%x\n", new->fd); >> >> I'd use %d since fds are typically shown as signed integers. > > Right. > >> >>> +} >>> + >>> static void v4l_print_edid(const void *arg, bool write_only) >>> { >>> const struct v4l2_edid *p = arg; >>> @@ -2486,6 +2494,22 @@ static int v4l_enum_freq_bands(const struct v4l2_ioctl_ops *ops, >>> return -ENOTTY; >>> } >>> >>> +static int vidioc_new_request(const struct v4l2_ioctl_ops *ops, >>> + struct file *file, void *fh, void *arg) >>> +{ >>> +#if IS_ENABLED(CONFIG_MEDIA_REQUEST_API) >>> + struct media_request_new *new = arg; >>> + struct video_device *vfd = video_devdata(file); >>> + >>> + if (!vfd->req_mgr) >>> + return -ENOTTY; >>> + >>> + return media_request_ioctl_new(vfd->req_mgr, new); >>> +#else >>> + return -ENOTTY; >>> +#endif >>> +} >> >> You don't need the #ifdef's here. media_request_ioctl_new() will be stubbed if >> CONFIG_MEDIA_REQUEST_API isn't set. > > Correct. > >> >>> + >>> struct v4l2_ioctl_info { >>> unsigned int ioctl; >>> u32 flags; >>> @@ -2617,6 +2641,7 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = { >>> 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)), >>> + IOCTL_INFO_FNC(VIDIOC_NEW_REQUEST, vidioc_new_request, vidioc_print_new_request, 0), >>> }; >>> #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls) >>> >>> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h >>> index 53f32022fabe..e6c4e10889bc 100644 >>> --- a/include/media/v4l2-dev.h >>> +++ b/include/media/v4l2-dev.h >>> @@ -209,6 +209,7 @@ struct v4l2_file_operations { >>> * @entity: &struct media_entity >>> * @intf_devnode: pointer to &struct media_intf_devnode >>> * @pipe: &struct media_pipeline >>> + * @req_mgr: request manager to use if this device supports creating requests >>> * @fops: pointer to &struct v4l2_file_operations for the video device >>> * @device_caps: device capabilities as used in v4l2_capabilities >>> * @dev: &struct device for the video device >>> @@ -251,6 +252,7 @@ struct video_device >>> struct media_intf_devnode *intf_devnode; >>> struct media_pipeline pipe; >>> #endif >>> + struct media_request_mgr *req_mgr; >>> const struct v4l2_file_operations *fops; >>> >>> u32 device_caps; >>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >>> index 91cfe0cbd5c5..35706204e81d 100644 >>> --- a/include/uapi/linux/videodev2.h >>> +++ b/include/uapi/linux/videodev2.h >>> @@ -63,6 +63,7 @@ >>> #include <linux/compiler.h> >>> #include <linux/ioctl.h> >>> #include <linux/types.h> >>> +#include <linux/media-request.h> >>> #include <linux/v4l2-common.h> >>> #include <linux/v4l2-controls.h> >>> >>> @@ -2407,6 +2408,8 @@ struct v4l2_create_buffers { >>> >>> #define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct v4l2_query_ext_ctrl) >>> >>> +#define VIDIOC_NEW_REQUEST _IOWR('V', 104, struct media_request_new) >> >> Hmm, I probably call this VIDIOC_CREATE_REQUEST (analogous to CREATE_BUFS). >> Ditto struct media_create_request and MEDIA_IOC_CREATE_REQUEST. > > Agree, it is better to keep consistency somehow. > >> >> I'm still not convinced this is the right approach (as opposed to using the media >> device node). I plan to dig deeper into the data structures tomorrow morning. > > I see no reason why this would not work. This seems more elegant than > relying on the media node for this, and also does not require pulling > the whole media controller support just to use requests on a simple > codec or m2m device. > > But of course, I may not be seeing everything. :) > > Thanks for the review! > Alex. > Regards, Hans
