Em Mon, 23 Apr 2018 13:49:11 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> On 04/10/2018 11:52 AM, Mauro Carvalho Chehab wrote:
> > Em Mon, 9 Apr 2018 16:20:00 +0200
> > Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> >
> >> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >>
> >> Add support for allocating a new request. This is only supported
> >> if mdev->ops->req_queue is set, i.e. the driver indicates that it
> >> supports queueing requests.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >> ---
> >> drivers/media/Makefile | 3 ++-
> >> drivers/media/media-device.c | 14 ++++++++++++++
> >> drivers/media/media-request.c | 23 +++++++++++++++++++++++
> >> include/media/media-device.h | 13 +++++++++++++
> >> include/media/media-request.h | 22 ++++++++++++++++++++++
> >> 5 files changed, 74 insertions(+), 1 deletion(-)
> >> create mode 100644 drivers/media/media-request.c
> >> create mode 100644 include/media/media-request.h
> >>
> >> diff --git a/drivers/media/Makefile b/drivers/media/Makefile
> >> index 594b462ddf0e..985d35ec6b29 100644
> >> --- a/drivers/media/Makefile
> >> +++ b/drivers/media/Makefile
> >> @@ -3,7 +3,8 @@
> >> # Makefile for the kernel multimedia device drivers.
> >> #
> >>
> >> -media-objs := media-device.o media-devnode.o media-entity.o
> >> +media-objs := media-device.o media-devnode.o media-entity.o \
> >> + media-request.o
> >>
> >> #
> >> # I2C drivers should come before other drivers, otherwise they'll fail
> >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >> index 35e81f7c0d2f..acb583c0eacd 100644
> >> --- a/drivers/media/media-device.c
> >> +++ b/drivers/media/media-device.c
> >> @@ -32,6 +32,7 @@
> >> #include <media/media-device.h>
> >> #include <media/media-devnode.h>
> >> #include <media/media-entity.h>
> >> +#include <media/media-request.h>
> >>
> >> #ifdef CONFIG_MEDIA_CONTROLLER
> >>
> >> @@ -366,6 +367,15 @@ static long media_device_get_topology(struct media_device *mdev,
> >> return ret;
> >> }
> >>
> >> +static long media_device_request_alloc(struct media_device *mdev,
> >> + struct media_request_alloc *alloc)
> >> +{
> >> + if (!mdev->ops || !mdev->ops->req_queue)
> >> + return -ENOTTY;
> >> +
> >> + return media_request_alloc(mdev, alloc);
> >> +}
> >> +
> >> static long copy_arg_from_user(void *karg, void __user *uarg, unsigned int cmd)
> >> {
> >> /* All media IOCTLs are _IOWR() */
> >> @@ -414,6 +424,7 @@ static const struct media_ioctl_info ioctl_info[] = {
> >> MEDIA_IOC(ENUM_LINKS, media_device_enum_links, MEDIA_IOC_FL_GRAPH_MUTEX),
> >> MEDIA_IOC(SETUP_LINK, media_device_setup_link, MEDIA_IOC_FL_GRAPH_MUTEX),
> >> MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
> >> + MEDIA_IOC(REQUEST_ALLOC, media_device_request_alloc, 0),
> >> };
> >>
> >> static long media_device_ioctl(struct file *filp, unsigned int cmd,
> >> @@ -686,6 +697,9 @@ void media_device_init(struct media_device *mdev)
> >> INIT_LIST_HEAD(&mdev->pads);
> >> INIT_LIST_HEAD(&mdev->links);
> >> INIT_LIST_HEAD(&mdev->entity_notify);
> >> +
> >> + spin_lock_init(&mdev->req_lock);
> >> + mutex_init(&mdev->req_queue_mutex);
> >> mutex_init(&mdev->graph_mutex);
> >> ida_init(&mdev->entity_internal_idx);
> >>
> >> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
> >> new file mode 100644
> >> index 000000000000..ead78613fdbe
> >> --- /dev/null
> >> +++ b/drivers/media/media-request.c
> >> @@ -0,0 +1,23 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Media device request objects
> >> + *
> >> + * Copyright (C) 2018 Intel Corporation
> >> + * Copyright (C) 2018, The Chromium OS Authors. All rights reserved.
> >> + *
> >> + * Author: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >> + */
> >> +
> >> +#include <linux/anon_inodes.h>
> >
> > Not needed. You already included it at media_device.h.
>
> Actually, it is needed here, but not in media_device.h
After patch 4, yes it is needed, but not in this patch :-)
>
> >
> >> +#include <linux/file.h>
> >> +#include <linux/mm.h>
> >> +#include <linux/string.h>
> >
> > Why do you need so many includes for a stub function?
>
> I'm going to merge patch 3 and 4. Splitting it up it too confusing for
> reviewers, I've noticed. These headers are needed for patch 4.
OK!
>
> As mentioned in an earlier reply: the req_lock spinlock is unused and is
> now deleted. It was a left-over from older versions and that's what caused
> the locking confusion.
Ok. I'll review the locks again on your next patchset.
>
> Regards,
>
> Hans
>
>
> >
> >> +
> >> +#include <media/media-device.h>
> >> +#include <media/media-request.h>
> >> +
> >> +int media_request_alloc(struct media_device *mdev,
> >> + struct media_request_alloc *alloc)
> >> +{
> >> + return -ENOMEM;
> >> +}
> >> diff --git a/include/media/media-device.h b/include/media/media-device.h
> >> index bcc6ec434f1f..07e323c57202 100644
> >> --- a/include/media/media-device.h
> >> +++ b/include/media/media-device.h
> >> @@ -19,6 +19,7 @@
> >> #ifndef _MEDIA_DEVICE_H
> >> #define _MEDIA_DEVICE_H
> >>
> >> +#include <linux/anon_inodes.h>
> >
> > Why do you need it? I don't see anything below needing it.
> >
> >> #include <linux/list.h>
> >> #include <linux/mutex.h>
> >>
> >> @@ -27,6 +28,7 @@
> >>
> >> struct ida;
> >> struct device;
> >> +struct media_device;
> >>
> >> /**
> >> * struct media_entity_notify - Media Entity Notify
> >> @@ -50,10 +52,16 @@ struct media_entity_notify {
> >> * struct media_device_ops - Media device operations
> >> * @link_notify: Link state change notification callback. This callback is
> >> * called with the graph_mutex held.
> >> + * @req_alloc: Allocate a request
> >> + * @req_free: Free a request
> >> + * @req_queue: Queue a request
> >> */
> >> struct media_device_ops {
> >> int (*link_notify)(struct media_link *link, u32 flags,
> >> unsigned int notification);
> >> + struct media_request *(*req_alloc)(struct media_device *mdev);
> >> + void (*req_free)(struct media_request *req);
> >> + int (*req_queue)(struct media_request *req);
> >> };
> >>
> >> /**
> >> @@ -88,6 +96,8 @@ struct media_device_ops {
> >> * @disable_source: Disable Source Handler function pointer
> >> *
> >> * @ops: Operation handler callbacks
> >> + * @req_lock: Serialise access to requests
> >> + * @req_queue_mutex: Serialise validating and queueing requests
> >
> > IMHO, this would better describe it:
> > Serialise validate and queue requests
> >
> > Yet, IMO, it doesn't let it clear when the spin lock should be
> > used and when the mutex should be used.
> >
> > I mean, what of them protect what variable?
> >
> >> *
> >> * This structure represents an abstract high-level media device. It allows easy
> >> * access to entities and provides basic media device-level support. The
> >> @@ -158,6 +168,9 @@ struct media_device {
> >> void (*disable_source)(struct media_entity *entity);
> >>
> >> const struct media_device_ops *ops;
> >> +
> >> + spinlock_t req_lock;
> >> + struct mutex req_queue_mutex;
> >> };
> >>
> >> /* We don't need to include pci.h or usb.h here */
> >> diff --git a/include/media/media-request.h b/include/media/media-request.h
> >> new file mode 100644
> >> index 000000000000..dae3eccd9aa7
> >> --- /dev/null
> >> +++ b/include/media/media-request.h
> >> @@ -0,0 +1,22 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Media device request objects
> >> + *
> >> + * Copyright (C) 2018 Intel Corporation
> >> + *
> >> + * Author: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >> + */
> >> +
> >> +#ifndef MEDIA_REQUEST_H
> >> +#define MEDIA_REQUEST_H
> >> +
> >> +#include <linux/list.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/spinlock.h>
> >
> > Why those includes are needed?
> >
> >> +
> >> +#include <media/media-device.h>
> >> +
> >> +int media_request_alloc(struct media_device *mdev,
> >> + struct media_request_alloc *alloc);
> >> +
> >> +#endif
> >
> >
> >
> > Thanks,
> > Mauro
> >
>
Thanks,
Mauro
