From: Mauro Carvalho Chehab <mchehab@s-opensource.com>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Sakari Ailus <sakari.ailus@iki.fi>,
linux-media@vger.kernel.org,
Hans Verkuil <hans.verkuil@cisco.com>
Subject: Re: [RFCv11 PATCH 02/29] uapi/linux/media.h: add request API
Date: Mon, 23 Apr 2018 10:32:43 -0300 [thread overview]
Message-ID: <20180423103243.23815991@vento.lan> (raw)
In-Reply-To: <c667b621-8cca-c15f-cbd6-34fb92243d55@xs4all.nl>
Em Mon, 23 Apr 2018 13:41:31 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> On 04/10/2018 01:00 PM, Sakari Ailus wrote:
> > On Tue, Apr 10, 2018 at 06:38:56AM -0300, Mauro Carvalho Chehab wrote:
> >> Em Mon, 9 Apr 2018 16:19:59 +0200
> >> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >>
> >>> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>>
> >>> Define the public request API.
> >>>
> >>> This adds the new MEDIA_IOC_REQUEST_ALLOC ioctl to allocate a request
> >>> and two ioctls that operate on a request in order to queue the
> >>> contents of the request to the driver and to re-initialize the
> >>> request.
> >>>
> >>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>> ---
> >>> include/uapi/linux/media.h | 8 ++++++++
> >>> 1 file changed, 8 insertions(+)
> >>>
> >>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> >>> index c7e9a5cba24e..f8769e74f847 100644
> >>> --- a/include/uapi/linux/media.h
> >>> +++ b/include/uapi/linux/media.h
> >>> @@ -342,11 +342,19 @@ struct media_v2_topology {
> >>>
> >>> /* ioctls */
> >>>
> >>> +struct __attribute__ ((packed)) media_request_alloc {
> >>> + __s32 fd;
> >>> +};
> >>> +
> >>> #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_REQUEST_ALLOC _IOWR('|', 0x05, struct media_request_alloc)
> >>> +
> >>
> >> Why use a struct here? Just declare it as:
> >>
> >> #define MEDIA_IOC_REQUEST_ALLOC _IOWR('|', 0x05, int)
> >
> > I'd say it's easier to extend it if it's a struct.
That's not true. Assuming that you declare it as:
#define MEDIA_IOC_REQUEST_ALLOC _IOWR('|', 0x05, int)
If you ever need a struct there, let's say:
struct media_request_alloc {
int request;
int new_field;
};
#define MEDIA_IOC_REQUEST_ALLOC_old _IOWR('|', 0x05, int)
#define MEDIA_IOC_REQUEST_ALLOC _IOWR('|', 0x05, struct media_request_alloc)
And add both MEDIA_IOC_REQUEST_ALLOC_old and MEDIA_IOC_REQUEST_ALLOC
to the ioctl handling logic, where the handler for
MEDIA_IOC_REQUEST_ALLOC_old being something like:
switch (cmd) {
...
case MEDIA_IOC_REQUEST_ALLOC_old:
struct media_request_alloc arg = {};
arg.request = parg;
and calling the same routine as MEDIA_IOC_REQUEST_ALLOC would
call, passing &arg as its input.
One advantage of not passing an struct is that there's no
need to call copy_to_user(), making its handler faster.
> > All other IOCTLs also
> > have a struct as an argument.
That is not true as well, if you consider V4L2 API (or the media
API as a hole). There are several ioctls there that takes just
an integer argument:
include/uapi/linux/videodev2.h:#define VIDIOC_OVERLAY _IOW('V', 14, int)
include/uapi/linux/videodev2.h:#define VIDIOC_STREAMON _IOW('V', 18, int)
include/uapi/linux/videodev2.h:#define VIDIOC_STREAMOFF _IOW('V', 19, int)
include/uapi/linux/videodev2.h:#define VIDIOC_G_INPUT _IOR('V', 38, int)
include/uapi/linux/videodev2.h:#define VIDIOC_S_INPUT _IOWR('V', 39, int)
include/uapi/linux/videodev2.h:#define VIDIOC_G_OUTPUT _IOR('V', 46, int)
include/uapi/linux/videodev2.h:#define VIDIOC_S_OUTPUT _IOWR('V', 47, int)
include/uapi/linux/dvb/frontend.h:#define FE_ENABLE_HIGH_LNB_VOLTAGE _IO('o', 68) /* int */
include/uapi/linux/dvb/frontend.h:#define FE_SET_FRONTEND_TUNE_MODE _IO('o', 81) /* unsigned int */
include/uapi/linux/dvb/frontend.h:#define FE_DISHNETWORK_SEND_LEGACY_CMD _IO('o', 80) /* unsigned int */
include/uapi/linux/lirc.h:#define LIRC_GET_FEATURES _IOR('i', 0x00000000, __u32)
include/uapi/linux/lirc.h:#define LIRC_GET_SEND_MODE _IOR('i', 0x00000001, __u32)
include/uapi/linux/lirc.h:#define LIRC_GET_REC_MODE _IOR('i', 0x00000002, __u32)
include/uapi/linux/lirc.h:#define LIRC_GET_REC_RESOLUTION _IOR('i', 0x00000007, __u32)
include/uapi/linux/lirc.h:#define LIRC_GET_MIN_TIMEOUT _IOR('i', 0x00000008, __u32)
include/uapi/linux/lirc.h:#define LIRC_GET_MAX_TIMEOUT _IOR('i', 0x00000009, __u32)
include/uapi/linux/lirc.h:#define LIRC_GET_LENGTH _IOR('i', 0x0000000f, __u32)
include/uapi/linux/lirc.h:#define LIRC_SET_SEND_MODE _IOW('i', 0x00000011, __u32)
include/uapi/linux/lirc.h:#define LIRC_SET_REC_MODE _IOW('i', 0x00000012, __u32)
include/uapi/linux/lirc.h:#define LIRC_SET_SEND_CARRIER _IOW('i', 0x00000013, __u32)
include/uapi/linux/lirc.h:#define LIRC_SET_REC_CARRIER _IOW('i', 0x00000014, __u32)
include/uapi/linux/lirc.h:#define LIRC_SET_SEND_DUTY_CYCLE _IOW('i', 0x00000015, __u32)
include/uapi/linux/lirc.h:#define LIRC_SET_TRANSMITTER_MASK _IOW('i', 0x00000017, __u32)
include/uapi/linux/lirc.h:#define LIRC_SET_REC_TIMEOUT _IOW('i', 0x00000018, __u32)
include/uapi/linux/lirc.h:#define LIRC_SET_REC_TIMEOUT_REPORTS _IOW('i', 0x00000019, __u32)
include/uapi/linux/lirc.h:#define LIRC_SET_MEASURE_CARRIER_MODE _IOW('i', 0x0000001d, __u32)
include/uapi/linux/lirc.h:#define LIRC_SET_REC_CARRIER_RANGE _IOW('i', 0x0000001f, __u32)
include/uapi/linux/lirc.h:#define LIRC_SET_WIDEBAND_RECEIVER _IOW('i', 0x00000023, __u32)
include/uapi/linux/lirc.h:#define LIRC_GET_REC_TIMEOUT _IOR('i', 0x00000024, __u32)
include/uapi/linux/dvb/frontend.h:#define FE_READ_BER _IOR('o', 70, __u32)
include/uapi/linux/dvb/frontend.h:#define FE_READ_SIGNAL_STRENGTH _IOR('o', 71, __u16)
include/uapi/linux/dvb/frontend.h:#define FE_READ_SNR _IOR('o', 72, __u16)
include/uapi/linux/dvb/frontend.h:#define FE_READ_UNCORRECTED_BLOCKS _IOR('o', 73, __u32)
Btw, at the beginning, I found ugly the way lirc API were designed (there,
*all* ioctls pass/receive integers), but the more I looked into it and
started documenting it, the more I liked!
It is a way easier to understand what each ioctl does, as just one thing
is affected by each ioctl. Btw, sysfs follows the same logic: there, you
really need a very strong reason why not just pass a single value to each
sysfs node.
That makes the design better, and usually avoid the need of changes, as,
if you need something else, just add a new sysfs node, or a new ioctl
with would take just one integer.
> > As a struct member, the parameter (fd) also
> > has a name; this is a plus.
Huh? Why is it a plus?
>
> While I do not have a very strong opinion on this, I do agree with Sakari here.
The thing is: adding a struct there is overdesign. If are there any
real reason why to have it as a struct (e. g. if we know/foresee any other
parameter to be needed there), then
>
> Regards,
>
> Hans
>
> >
> >>
> >>> +#define MEDIA_REQUEST_IOC_QUEUE _IO('|', 0x80)
> >>> +#define MEDIA_REQUEST_IOC_REINIT _IO('|', 0x81)
> >>>
> >>> #if !defined(__KERNEL__) || defined(__NEED_MEDIA_LEGACY_API)
> >>>
> >>
> >> Thanks,
> >> Mauro
> >
>
Thanks,
Mauro
next prev parent reply other threads:[~2018-04-23 13:32 UTC|newest]
Thread overview: 135+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-09 14:19 [RFCv11 PATCH 00/29] Request API Hans Verkuil
2018-04-09 14:19 ` [RFCv11 PATCH 01/29] v4l2-device.h: always expose mdev Hans Verkuil
2018-04-17 4:34 ` Alexandre Courbot
2018-04-09 14:19 ` [RFCv11 PATCH 02/29] uapi/linux/media.h: add request API Hans Verkuil
2018-04-10 5:26 ` Tomasz Figa
2018-04-23 9:55 ` Hans Verkuil
2018-04-10 9:38 ` Mauro Carvalho Chehab
2018-04-10 11:00 ` Sakari Ailus
2018-04-23 11:41 ` Hans Verkuil
2018-04-23 13:32 ` Mauro Carvalho Chehab [this message]
2018-04-17 4:34 ` Alexandre Courbot
2018-04-09 14:20 ` [RFCv11 PATCH 03/29] media-request: allocate media requests Hans Verkuil
2018-04-10 5:35 ` Tomasz Figa
2018-04-10 9:54 ` Mauro Carvalho Chehab
2018-04-23 9:59 ` Hans Verkuil
2018-04-10 9:52 ` Mauro Carvalho Chehab
2018-04-10 11:14 ` Sakari Ailus
2018-04-11 10:13 ` Mauro Carvalho Chehab
2018-04-23 9:46 ` Hans Verkuil
2018-04-23 11:49 ` Hans Verkuil
2018-04-23 13:35 ` Mauro Carvalho Chehab
2018-04-17 4:34 ` Alexandre Courbot
2018-04-09 14:20 ` [RFCv11 PATCH 04/29] media-request: core request support Hans Verkuil
2018-04-10 8:21 ` Tomasz Figa
2018-04-10 13:04 ` Sakari Ailus
2018-04-23 11:24 ` Hans Verkuil
2018-04-23 12:14 ` Hans Verkuil
2018-04-10 10:32 ` Mauro Carvalho Chehab
2018-04-10 12:32 ` Sakari Ailus
2018-04-10 14:51 ` Mauro Carvalho Chehab
2018-04-11 13:21 ` Sakari Ailus
2018-04-11 13:49 ` Mauro Carvalho Chehab
2018-04-11 15:02 ` Sakari Ailus
2018-04-11 15:17 ` Mauro Carvalho Chehab
2018-04-11 15:35 ` Sakari Ailus
2018-04-11 16:13 ` Mauro Carvalho Chehab
2018-04-12 7:11 ` Sakari Ailus
2018-04-23 12:43 ` Hans Verkuil
2018-05-07 10:05 ` Sakari Ailus
2018-04-23 12:23 ` Hans Verkuil
2018-04-23 14:07 ` Mauro Carvalho Chehab
2018-04-10 12:17 ` Sakari Ailus
2018-04-17 4:35 ` Alexandre Courbot
2018-04-23 14:28 ` Hans Verkuil
2018-04-09 14:20 ` [RFCv11 PATCH 05/29] media-request: add request ioctls Hans Verkuil
2018-04-10 8:59 ` Tomasz Figa
2018-04-12 7:35 ` Sakari Ailus
2018-04-23 12:40 ` Hans Verkuil
2018-04-23 11:34 ` Hans Verkuil
2018-04-10 10:57 ` Mauro Carvalho Chehab
2018-04-12 10:40 ` Sakari Ailus
2018-04-23 13:45 ` Hans Verkuil
2018-04-12 7:31 ` Sakari Ailus
2018-04-09 14:20 ` [RFCv11 PATCH 06/29] media-request: add media_request_find Hans Verkuil
2018-04-10 11:04 ` Mauro Carvalho Chehab
2018-04-12 10:52 ` Sakari Ailus
2018-04-12 12:08 ` Sakari Ailus
2018-04-23 13:50 ` Hans Verkuil
2018-04-09 14:20 ` [RFCv11 PATCH 07/29] media-request: add media_request_object_find Hans Verkuil
2018-04-10 11:07 ` Mauro Carvalho Chehab
2018-04-10 11:10 ` Hans Verkuil
2018-04-12 12:23 ` Sakari Ailus
2018-04-23 13:55 ` Hans Verkuil
2018-04-17 4:36 ` Alexandre Courbot
2018-04-23 14:32 ` Hans Verkuil
2018-04-09 14:20 ` [RFCv11 PATCH 08/29] videodev2.h: add request_fd field to v4l2_ext_controls Hans Verkuil
2018-04-10 11:11 ` Mauro Carvalho Chehab
2018-04-12 12:24 ` Sakari Ailus
2018-04-09 14:20 ` [RFCv11 PATCH 09/29] videodev2.h: add V4L2_CTRL_FLAG_IN_REQUEST Hans Verkuil
2018-04-09 14:20 ` [RFCv11 PATCH 10/29] v4l2-ctrls: v4l2_ctrl_add_handler: add from_other_dev Hans Verkuil
2018-04-10 11:42 ` Mauro Carvalho Chehab
2018-04-12 13:35 ` Sakari Ailus
2018-04-09 14:20 ` [RFCv11 PATCH 11/29] v4l2-ctrls: prepare internal structs for request API Hans Verkuil
2018-04-09 14:20 ` [RFCv11 PATCH 12/29] v4l2-ctrls: alloc memory for p_req Hans Verkuil
2018-04-10 9:32 ` Tomasz Figa
2018-04-10 13:57 ` Mauro Carvalho Chehab
2018-04-23 11:39 ` Hans Verkuil
2018-04-09 14:20 ` [RFCv11 PATCH 13/29] v4l2-ctrls: use ref in helper instead of ctrl Hans Verkuil
2018-04-10 12:47 ` Mauro Carvalho Chehab
2018-04-09 14:20 ` [RFCv11 PATCH 14/29] v4l2-ctrls: add core request support Hans Verkuil
2018-04-11 8:37 ` Paul Kocialkowski
2018-04-11 9:43 ` Tomasz Figa
2018-04-23 9:23 ` Hans Verkuil
2018-04-09 14:20 ` [RFCv11 PATCH 15/29] v4l2-ctrls: support g/s_ext_ctrls for requests Hans Verkuil
2018-04-10 13:00 ` Mauro Carvalho Chehab
2018-04-09 14:20 ` [RFCv11 PATCH 16/29] videodev2.h: Add request_fd field to v4l2_buffer Hans Verkuil
2018-04-09 14:20 ` [RFCv11 PATCH 17/29] vb2: store userspace data in vb2_v4l2_buffer Hans Verkuil
2018-04-09 15:11 ` Kieran Bingham
2018-04-23 9:53 ` Hans Verkuil
2018-04-10 13:32 ` Mauro Carvalho Chehab
2018-04-09 14:20 ` [RFCv11 PATCH 18/29] videobuf2-core: embed media_request_object Hans Verkuil
2018-04-09 14:20 ` [RFCv11 PATCH 19/29] videobuf2-core: integrate with media requests Hans Verkuil
2018-04-12 8:13 ` Tomasz Figa
2018-04-23 12:49 ` Hans Verkuil
2018-04-09 14:20 ` [RFCv11 PATCH 20/29] videobuf2-v4l2: " Hans Verkuil
2018-04-10 13:52 ` Mauro Carvalho Chehab
2018-04-17 4:36 ` Alexandre Courbot
2018-04-23 14:53 ` Hans Verkuil
2018-04-09 14:20 ` [RFCv11 PATCH 21/29] videobuf2-core: add vb2_core_request_has_buffers Hans Verkuil
2018-04-10 13:53 ` Mauro Carvalho Chehab
2018-04-09 14:20 ` [RFCv11 PATCH 22/29] videobuf2-v4l2: add vb2_request_queue helper Hans Verkuil
2018-04-12 8:29 ` Tomasz Figa
2018-04-23 12:51 ` Hans Verkuil
2018-04-09 14:20 ` [RFCv11 PATCH 23/29] videobuf2-v4l2: export request_fd Hans Verkuil
2018-04-10 13:59 ` Mauro Carvalho Chehab
2018-04-09 14:20 ` [RFCv11 PATCH 24/29] Documentation: v4l: document request API Hans Verkuil
2018-04-10 14:19 ` Mauro Carvalho Chehab
2018-04-12 8:51 ` Tomasz Figa
2018-04-09 14:20 ` [RFCv11 PATCH 25/29] media: vim2m: add media device Hans Verkuil
2018-04-12 8:54 ` Tomasz Figa
2018-04-23 13:23 ` Hans Verkuil
2018-04-17 4:37 ` Alexandre Courbot
2018-04-09 14:20 ` [RFCv11 PATCH 26/29] vim2m: use workqueue Hans Verkuil
2018-04-11 14:06 ` Paul Kocialkowski
2018-04-30 14:51 ` Hans Verkuil
2018-04-12 9:02 ` Tomasz Figa
2018-04-30 14:58 ` Hans Verkuil
2018-04-09 14:20 ` [RFCv11 PATCH 27/29] vim2m: support requests Hans Verkuil
2018-04-11 14:01 ` Paul Kocialkowski
2018-04-12 9:15 ` Tomasz Figa
2018-04-23 13:30 ` Hans Verkuil
2018-04-17 4:37 ` Alexandre Courbot
2018-04-23 15:06 ` Hans Verkuil
2018-04-17 11:42 ` Paul Kocialkowski
2018-04-23 15:10 ` Hans Verkuil
2018-04-09 14:20 ` [RFCv11 PATCH 28/29] vivid: add mc Hans Verkuil
2018-04-09 14:20 ` [RFCv11 PATCH 29/29] vivid: add request support Hans Verkuil
2018-04-10 14:31 ` [RFCv11 PATCH 00/29] Request API Mauro Carvalho Chehab
2018-04-17 4:33 ` Alexandre Courbot
2018-04-17 6:12 ` Hans Verkuil
2018-04-17 6:17 ` Alexandre Courbot
2018-04-17 11:40 ` Paul Kocialkowski
2018-04-18 2:06 ` Alexandre Courbot
2018-04-19 7:48 ` Paul Kocialkowski
2018-04-23 15:10 ` Hans Verkuil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180423103243.23815991@vento.lan \
--to=mchehab@s-opensource.com \
--cc=hans.verkuil@cisco.com \
--cc=hverkuil@xs4all.nl \
--cc=linux-media@vger.kernel.org \
--cc=sakari.ailus@iki.fi \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.