Em Mon, 23 Apr 2018 13:41:31 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> 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@xxxxxxxxx> escreveu:
> >>
> >>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxx>
> >>> ---
> >>> 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
