Re: [RFCv9 PATCH 03/29] media-request: allocate media requests

[Hans Verkuil]

On 29/03/18 11:52, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, Mar 29, 2018 at 11:16:45AM +0200, Hans Verkuil wrote:
>> On 29/03/18 11:01, Sakari Ailus wrote:
>>> On Thu, Mar 29, 2018 at 10:57:44AM +0200, Hans Verkuil wrote:
 On 29/03/18 10:45, Sakari Ailus wrote:
> Hi Hans,
>
> On Wed, Mar 28, 2018 at 03:50:04PM +0200, Hans Verkuil wrote:
> ...
>> @@ -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
>
> s/validating and//
>
> As there's no more a separate validation step. Then,

 Well, you validate before queuing. It's not a separate step, but
 part of the queue operation. See patch 23 where this is implemented
 in the vb2_request_helper function.
>>>
>>> Works for me. I think we'll need the validate op sooner or later anyway.
>>>
>>
>>
>> There is one. Request objects have prepare, unprepare and queue ops.
>>
>> The request req_queue op will prepare all objects, and only queue if the
>> prepare (aka validate) step succeeds for all objects. If not, then the
> 
> You can't validate the objects in a request separately from other objects
> in it, the validation needs to happen at the request level. There are two
> reasons for this:
> 
> - The objects in a request must be compatible with all other objects in the
>   request and
> 
> - The request must contain the required objects in order to be valid ---
>   e.g. for a device producing multiple capture buffers from one output
>   buffer, the output buffer is mandatory whereas one or more of the capture
>   buffers are not.
> 
> The latter could presumably be implemented separately for each object but
> then the driver needs to go fishing for the related objects which may not
> be very efficient.
> 
> What I'm proposing is to put this at the level of a request, at least for
> now.
> 

The driver implements req_queue. It can do whatever it wants there, it has
full control.

However, as part of that process objects still have to be prepared (for
buffers that literally means calling buf_prepare). That step does the
validation on an object level and also possible memory/resource allocations
that can fail.

The helper function in patch 23 is however sufficient for simple drivers
like vim2m, vivid, codec drivers, etc. that do not need to validate the
request as a whole.

Drivers that do need to validate the request as a whole will not use that
helper function, they will do this themselves.

Regards,

Hans

Re: [RFCv9 PATCH 03/29] media-request: allocate media requests

[Sakari Ailus]

Hi Hans,

On Thu, Mar 29, 2018 at 11:16:45AM +0200, Hans Verkuil wrote:
> On 29/03/18 11:01, Sakari Ailus wrote:
> > On Thu, Mar 29, 2018 at 10:57:44AM +0200, Hans Verkuil wrote:
> >> On 29/03/18 10:45, Sakari Ailus wrote:
> >>> Hi Hans,
> >>>
> >>> On Wed, Mar 28, 2018 at 03:50:04PM +0200, Hans Verkuil wrote:
> >>> ...
>  @@ -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
> >>>
> >>> s/validating and//
> >>>
> >>> As there's no more a separate validation step. Then,
> >>
> >> Well, you validate before queuing. It's not a separate step, but
> >> part of the queue operation. See patch 23 where this is implemented
> >> in the vb2_request_helper function.
> > 
> > Works for me. I think we'll need the validate op sooner or later anyway.
> > 
> 
> 
> There is one. Request objects have prepare, unprepare and queue ops.
> 
> The request req_queue op will prepare all objects, and only queue if the
> prepare (aka validate) step succeeds for all objects. If not, then the

You can't validate the objects in a request separately from other objects
in it, the validation needs to happen at the request level. There are two
reasons for this:

- The objects in a request must be compatible with all other objects in the
  request and

- The request must contain the required objects in order to be valid ---
  e.g. for a device producing multiple capture buffers from one output
  buffer, the output buffer is mandatory whereas one or more of the capture
  buffers are not.

The latter could presumably be implemented separately for each object but
then the driver needs to go fishing for the related objects which may not
be very efficient.

What I'm proposing is to put this at the level of a request, at least for
now.

-- 
Sakari Ailus
sakari.ai...@linux.intel.com

Re: [RFCv9 PATCH 03/29] media-request: allocate media requests

[Hans Verkuil]

On 29/03/18 11:01, Sakari Ailus wrote:
> On Thu, Mar 29, 2018 at 10:57:44AM +0200, Hans Verkuil wrote:
>> On 29/03/18 10:45, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Wed, Mar 28, 2018 at 03:50:04PM +0200, Hans Verkuil wrote:
>>> ...
 @@ -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
>>>
>>> s/validating and//
>>>
>>> As there's no more a separate validation step. Then,
>>
>> Well, you validate before queuing. It's not a separate step, but
>> part of the queue operation. See patch 23 where this is implemented
>> in the vb2_request_helper function.
> 
> Works for me. I think we'll need the validate op sooner or later anyway.
> 


There is one. Request objects have prepare, unprepare and queue ops.

The request req_queue op will prepare all objects, and only queue if the
prepare (aka validate) step succeeds for all objects. If not, then the
objects that were prepared will be rolled back with the unprepare op
and req_queue returns an error.

Note that the object ops are not documented at the moment, it's one of
my TODOs.

Regards,

Hans

Re: [RFCv9 PATCH 03/29] media-request: allocate media requests

[Sakari Ailus]

On Thu, Mar 29, 2018 at 10:57:44AM +0200, Hans Verkuil wrote:
> On 29/03/18 10:45, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Wed, Mar 28, 2018 at 03:50:04PM +0200, Hans Verkuil wrote:
> > ...
> >> @@ -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
> > 
> > s/validating and//
> > 
> > As there's no more a separate validation step. Then,
> 
> Well, you validate before queuing. It's not a separate step, but
> part of the queue operation. See patch 23 where this is implemented
> in the vb2_request_helper function.

Works for me. I think we'll need the validate op sooner or later anyway.

-- 
Sakari Ailus
sakari.ai...@linux.intel.com

Re: [RFCv9 PATCH 03/29] media-request: allocate media requests

[Hans Verkuil]

On 29/03/18 10:45, Sakari Ailus wrote:
> Hi Hans,
> 
> On Wed, Mar 28, 2018 at 03:50:04PM +0200, Hans Verkuil wrote:
> ...
>> @@ -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
> 
> s/validating and//
> 
> As there's no more a separate validation step. Then,

Well, you validate before queuing. It's not a separate step, but
part of the queue operation. See patch 23 where this is implemented
in the vb2_request_helper function.

Regards,

Hans

> 
> Acked-by: Sakari Ailus 
> 

Re: [RFCv9 PATCH 03/29] media-request: allocate media requests

[Sakari Ailus]

Hi Hans,

On Wed, Mar 28, 2018 at 03:50:04PM +0200, Hans Verkuil wrote:
...
> @@ -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

s/validating and//

As there's no more a separate validation step. Then,

Acked-by: Sakari Ailus 

-- 
Sakari Ailus
sakari.ai...@linux.intel.com

[RFCv9 PATCH 03/29] media-request: allocate media requests

[Hans Verkuil]

From: Hans Verkuil 

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 
---
 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 
 #include 
 #include 
+#include 
 
 #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(>pads);
INIT_LIST_HEAD(>links);
INIT_LIST_HEAD(>entity_notify);
+
+   spin_lock_init(>req_lock);
+   mutex_init(>req_queue_mutex);
mutex_init(>graph_mutex);
ida_init(>entity_internal_idx);
 
diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
new file mode 100644
index ..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 
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+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 
 #include 
 #include 
 
@@ -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
  *
  * 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