Re: [PATCHv13 03/28] media-request: implement media requests

[Hans Verkuil]

On 08/05/18 12:21, Mauro Carvalho Chehab wrote:
> Em Fri, 4 May 2018 15:27:50 +0300
> Sakari Ailus  escreveu:
> 
>> Hi Hans,
>>
>> I've read this patch a large number of times and I think also the details
>> begin to seem sound. A few comments below.
> 
> I'm sending this after analyzing the other patches in this series,
> as this is the core of the changes. So, although I wrote the comments
> early, I wanted to read first all other patches before sending it.
> 
>>
>> On Thu, May 03, 2018 at 04:52:53PM +0200, Hans Verkuil wrote:
>>> From: Hans Verkuil 
>>>
>>> Add initial media request support:
>>>
>>> 1) Add MEDIA_IOC_REQUEST_ALLOC ioctl support to media-device.c
>>> 2) Add struct media_request to store request objects.
>>> 3) Add struct media_request_object to represent a request object.
>>> 4) Add MEDIA_REQUEST_IOC_QUEUE/REINIT ioctl support.
>>>
>>> Basic lifecycle: the application allocates a request, adds
>>> objects to it, queues the request, polls until it is completed
>>> and can then read the final values of the objects at the time
>>> of completion. When it closes the file descriptor the request
>>> memory will be freed (actually, when the last user of that request
>>> releases the request).
>>>
>>> Drivers will bind an object to a request (the 'adds objects to it'
>>> phase), when MEDIA_REQUEST_IOC_QUEUE is called the request is
>>> validated (req_validate op), then queued (the req_queue op).
>>>
>>> When done with an object it can either be unbound from the request
>>> (e.g. when the driver has finished with a vb2 buffer) or marked as
>>> completed (e.g. for controls associated with a buffer). When all
>>> objects in the request are completed (or unbound), then the request
>>> fd will signal an exception (poll).
>>>
>>> Signed-off-by: Hans Verkuil 
> 
> Hmm... As you're adding Copyrights from Intel/Google in this patch, that
> indicates that part of the stuff you're adding here were authored by
> others. So, you should use Co-developed-by: tag here, and get the SOBs
> from the other developers that did part of the work[1].
> 
> [1] except if your work was sponsored by Cisco, Intel and Google, but
> I think this is not the case.

I'll check this with Sakari and Google.

> 
>>> ---
>>>  drivers/media/Makefile|   3 +-
>>>  drivers/media/media-device.c  |  14 ++
>>>  drivers/media/media-request.c | 407 ++
>>>  include/media/media-device.h  |  16 ++
>>>  include/media/media-request.h | 244 
>>>  5 files changed, 683 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..bb6a64acd3f0 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_validate || !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,8 @@ void media_device_init(struct media_device *mdev)
>>> INIT_LIST_HEAD(>pads);
>>> INIT_LIST_HEAD(>links);
>>> INIT_LIST_HEAD(>entity_notify);
>>> +
>>> +   mutex_init(>req_queue_mutex);
>>> mutex_init(>graph_mutex);
>>> ida_init(>entity_internal_idx);
>>>  
>>> @@ -699,6 

Re: [PATCHv13 03/28] media-request: implement media requests

[Hans Verkuil]

On 04/05/18 14:27, Sakari Ailus wrote:
> Hi Hans,
> 
> I've read this patch a large number of times and I think also the details
> begin to seem sound. A few comments below.
> 
> On Thu, May 03, 2018 at 04:52:53PM +0200, Hans Verkuil wrote:
>> From: Hans Verkuil 
>>
>> Add initial media request support:
>>
>> 1) Add MEDIA_IOC_REQUEST_ALLOC ioctl support to media-device.c
>> 2) Add struct media_request to store request objects.
>> 3) Add struct media_request_object to represent a request object.
>> 4) Add MEDIA_REQUEST_IOC_QUEUE/REINIT ioctl support.
>>
>> Basic lifecycle: the application allocates a request, adds
>> objects to it, queues the request, polls until it is completed
>> and can then read the final values of the objects at the time
>> of completion. When it closes the file descriptor the request
>> memory will be freed (actually, when the last user of that request
>> releases the request).
>>
>> Drivers will bind an object to a request (the 'adds objects to it'
>> phase), when MEDIA_REQUEST_IOC_QUEUE is called the request is
>> validated (req_validate op), then queued (the req_queue op).
>>
>> When done with an object it can either be unbound from the request
>> (e.g. when the driver has finished with a vb2 buffer) or marked as
>> completed (e.g. for controls associated with a buffer). When all
>> objects in the request are completed (or unbound), then the request
>> fd will signal an exception (poll).
>>
>> Signed-off-by: Hans Verkuil 
>> ---
>>  drivers/media/Makefile|   3 +-
>>  drivers/media/media-device.c  |  14 ++
>>  drivers/media/media-request.c | 407 ++
>>  include/media/media-device.h  |  16 ++
>>  include/media/media-request.h | 244 
>>  5 files changed, 683 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..bb6a64acd3f0 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_validate || !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,8 @@ void media_device_init(struct media_device *mdev)
>>  INIT_LIST_HEAD(>pads);
>>  INIT_LIST_HEAD(>links);
>>  INIT_LIST_HEAD(>entity_notify);
>> +
>> +mutex_init(>req_queue_mutex);
>>  mutex_init(>graph_mutex);
>>  ida_init(>entity_internal_idx);
>>  
>> @@ -699,6 +712,7 @@ void media_device_cleanup(struct media_device *mdev)
>>  mdev->entity_internal_idx_max = 0;
>>  media_graph_walk_cleanup(>pm_count_walk);
>>  mutex_destroy(>graph_mutex);
>> +mutex_destroy(>req_queue_mutex);
>>  }
>>  EXPORT_SYMBOL_GPL(media_device_cleanup);
>>  
>> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
>> new file mode 100644
>> index ..c216c4ab628b
>> --- /dev/null
>> +++ b/drivers/media/media-request.c
>> @@ -0,0 +1,407 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Media device request objects
>> + *
>> + * Copyright 2018 Cisco Systems, Inc. and/or its affiliates. All rights 
>> reserved.
>> + * Copyright (C) 2018 Intel Corporation
>> + * Copyright (C) 2018 Google, Inc.
>> + *
>> + * Author: Hans Verkuil 

Re: [PATCHv13 03/28] media-request: implement media requests

[Sakari Ailus]

Hi Mauro,

On Tue, May 08, 2018 at 09:41:03AM -0300, Mauro Carvalho Chehab wrote:
> Em Tue, 8 May 2018 13:52:33 +0300
> Sakari Ailus  escreveu:
> 
> > Hi Mauro, Hans,
> > 
> > On Tue, May 08, 2018 at 07:21:16AM -0300, Mauro Carvalho Chehab wrote:
> > > Em Fri, 4 May 2018 15:27:50 +0300
> > > Sakari Ailus  escreveu:
> > >   
> > > > Hi Hans,
> > > > 
> > > > I've read this patch a large number of times and I think also the 
> > > > details
> > > > begin to seem sound. A few comments below.  
> > > 
> > > I'm sending this after analyzing the other patches in this series,
> > > as this is the core of the changes. So, although I wrote the comments
> > > early, I wanted to read first all other patches before sending it.
> > >   
> > > > 
> > > > On Thu, May 03, 2018 at 04:52:53PM +0200, Hans Verkuil wrote:  
> > > > > From: Hans Verkuil 
> > > > > 
> > > > > Add initial media request support:
> > > > > 
> > > > > 1) Add MEDIA_IOC_REQUEST_ALLOC ioctl support to media-device.c
> > > > > 2) Add struct media_request to store request objects.
> > > > > 3) Add struct media_request_object to represent a request object.
> > > > > 4) Add MEDIA_REQUEST_IOC_QUEUE/REINIT ioctl support.
> > > > > 
> > > > > Basic lifecycle: the application allocates a request, adds
> > > > > objects to it, queues the request, polls until it is completed
> > > > > and can then read the final values of the objects at the time
> > > > > of completion. When it closes the file descriptor the request
> > > > > memory will be freed (actually, when the last user of that request
> > > > > releases the request).
> > > > > 
> > > > > Drivers will bind an object to a request (the 'adds objects to it'
> > > > > phase), when MEDIA_REQUEST_IOC_QUEUE is called the request is
> > > > > validated (req_validate op), then queued (the req_queue op).
> > > > > 
> > > > > When done with an object it can either be unbound from the request
> > > > > (e.g. when the driver has finished with a vb2 buffer) or marked as
> > > > > completed (e.g. for controls associated with a buffer). When all
> > > > > objects in the request are completed (or unbound), then the request
> > > > > fd will signal an exception (poll).
> > > > > 
> > > > > Signed-off-by: Hans Verkuil   
> > > 
> > > Hmm... As you're adding Copyrights from Intel/Google in this patch, that
> > > indicates that part of the stuff you're adding here were authored by
> > > others. So, you should use Co-developed-by: tag here, and get the SOBs
> > > from the other developers that did part of the work[1].
> > > 
> > > [1] except if your work was sponsored by Cisco, Intel and Google, but
> > > I think this is not the case.  
> > 
> > I think this could be appropriate:
> > 
> > Co-developed-by: Laurent Pinchart 
> > 
> > Co-developed-by: Sakari Ailus 
> > Co-developed-by: Alexandre Courbot 
> > 
> > ...
> > 
> > > > > +static long media_request_ioctl_queue(struct media_request *req)
> > > > > +{
> > > > > + struct media_device *mdev = req->mdev;
> > > > > + enum media_request_state state;
> > > > > + unsigned long flags;
> > > > > + int ret = 0;
> > > > 
> > > > ret is unconditionally assigned below, no need to initialise here.
> > > >   
> > > > > +
> > > > > + dev_dbg(mdev->dev, "request: queue %s\n", req->debug_str);
> > > > > +
> > > > > + /*
> > > > > +  * Ensure the request that is validated will be the one that 
> > > > > gets queued
> > > > > +  * next by serialising the queueing process. This mutex is also 
> > > > > used
> > > > > +  * to serialize with canceling a vb2 queue and with setting 
> > > > > values such
> > > > > +  * as controls in a request.
> > > > > +  */
> > > > > + mutex_lock(>req_queue_mutex);
> > > > > +
> > > > > + spin_lock_irqsave(>lock, flags);
> > > > > + state = atomic_cmpxchg(>state, MEDIA_REQUEST_STATE_IDLE,
> > > > > +MEDIA_REQUEST_STATE_VALIDATING);
> > > > > + spin_unlock_irqrestore(>lock, flags);  
> > > 
> > > It looks weird to serialize access to it with a mutex, a spin lock and 
> > > an atomic call.  
> > 
> > req->lock is needed for state changes from idle to other states as also
> > other struct members need to be serialised with that, with the request
> > state. Which is kind of in line with my earlier point: there's little
> > if any benefit in making this field atomic.
> 
> In this case, only req->state change is serialized - although atomic
> also serializes it.

Correct. But other fields also need to be serialised --- together with the
state change. Therefore relying on atomic only is not an option.

> 
> > The mutex is there to ensure only a single request remains in validating
> > state, i.e. we will be changing the state of the device request tip one
> > request at a time.

Re: [PATCHv13 03/28] media-request: implement media requests

[Mauro Carvalho Chehab]

Em Tue, 8 May 2018 13:52:33 +0300
Sakari Ailus  escreveu:

> Hi Mauro, Hans,
> 
> On Tue, May 08, 2018 at 07:21:16AM -0300, Mauro Carvalho Chehab wrote:
> > Em Fri, 4 May 2018 15:27:50 +0300
> > Sakari Ailus  escreveu:
> >   
> > > Hi Hans,
> > > 
> > > I've read this patch a large number of times and I think also the details
> > > begin to seem sound. A few comments below.  
> > 
> > I'm sending this after analyzing the other patches in this series,
> > as this is the core of the changes. So, although I wrote the comments
> > early, I wanted to read first all other patches before sending it.
> >   
> > > 
> > > On Thu, May 03, 2018 at 04:52:53PM +0200, Hans Verkuil wrote:  
> > > > From: Hans Verkuil 
> > > > 
> > > > Add initial media request support:
> > > > 
> > > > 1) Add MEDIA_IOC_REQUEST_ALLOC ioctl support to media-device.c
> > > > 2) Add struct media_request to store request objects.
> > > > 3) Add struct media_request_object to represent a request object.
> > > > 4) Add MEDIA_REQUEST_IOC_QUEUE/REINIT ioctl support.
> > > > 
> > > > Basic lifecycle: the application allocates a request, adds
> > > > objects to it, queues the request, polls until it is completed
> > > > and can then read the final values of the objects at the time
> > > > of completion. When it closes the file descriptor the request
> > > > memory will be freed (actually, when the last user of that request
> > > > releases the request).
> > > > 
> > > > Drivers will bind an object to a request (the 'adds objects to it'
> > > > phase), when MEDIA_REQUEST_IOC_QUEUE is called the request is
> > > > validated (req_validate op), then queued (the req_queue op).
> > > > 
> > > > When done with an object it can either be unbound from the request
> > > > (e.g. when the driver has finished with a vb2 buffer) or marked as
> > > > completed (e.g. for controls associated with a buffer). When all
> > > > objects in the request are completed (or unbound), then the request
> > > > fd will signal an exception (poll).
> > > > 
> > > > Signed-off-by: Hans Verkuil   
> > 
> > Hmm... As you're adding Copyrights from Intel/Google in this patch, that
> > indicates that part of the stuff you're adding here were authored by
> > others. So, you should use Co-developed-by: tag here, and get the SOBs
> > from the other developers that did part of the work[1].
> > 
> > [1] except if your work was sponsored by Cisco, Intel and Google, but
> > I think this is not the case.  
> 
> I think this could be appropriate:
> 
> Co-developed-by: Laurent Pinchart 
> 
> Co-developed-by: Sakari Ailus 
> Co-developed-by: Alexandre Courbot 
> 
> ...
> 
> > > > +static long media_request_ioctl_queue(struct media_request *req)
> > > > +{
> > > > +   struct media_device *mdev = req->mdev;
> > > > +   enum media_request_state state;
> > > > +   unsigned long flags;
> > > > +   int ret = 0;
> > > 
> > > ret is unconditionally assigned below, no need to initialise here.
> > >   
> > > > +
> > > > +   dev_dbg(mdev->dev, "request: queue %s\n", req->debug_str);
> > > > +
> > > > +   /*
> > > > +* Ensure the request that is validated will be the one that 
> > > > gets queued
> > > > +* next by serialising the queueing process. This mutex is also 
> > > > used
> > > > +* to serialize with canceling a vb2 queue and with setting 
> > > > values such
> > > > +* as controls in a request.
> > > > +*/
> > > > +   mutex_lock(>req_queue_mutex);
> > > > +
> > > > +   spin_lock_irqsave(>lock, flags);
> > > > +   state = atomic_cmpxchg(>state, MEDIA_REQUEST_STATE_IDLE,
> > > > +  MEDIA_REQUEST_STATE_VALIDATING);
> > > > +   spin_unlock_irqrestore(>lock, flags);  
> > 
> > It looks weird to serialize access to it with a mutex, a spin lock and 
> > an atomic call.  
> 
> req->lock is needed for state changes from idle to other states as also
> other struct members need to be serialised with that, with the request
> state. Which is kind of in line with my earlier point: there's little
> if any benefit in making this field atomic.

In this case, only req->state change is serialized - although atomic
also serializes it.

> The mutex is there to ensure only a single request remains in validating
> state, i.e. we will be changing the state of the device request tip one
> request at a time.

Better to add a comment explaining it.

> > 
> > IMHO, locking is still an issue here. I would love to test the 
> > locks with some tool that would randomize syscalls, issuing close(),
> > poll() and read() at wrong times and inverting the order of some calls, 
> > in order to do some empiric test that all locks are at the right places.
> > 
> > Complex locking schemas like that usually tend to cause a lot of

Re: [PATCHv13 03/28] media-request: implement media requests

[Sakari Ailus]

Hi Mauro, Hans,

On Tue, May 08, 2018 at 07:21:16AM -0300, Mauro Carvalho Chehab wrote:
> Em Fri, 4 May 2018 15:27:50 +0300
> Sakari Ailus  escreveu:
> 
> > Hi Hans,
> > 
> > I've read this patch a large number of times and I think also the details
> > begin to seem sound. A few comments below.
> 
> I'm sending this after analyzing the other patches in this series,
> as this is the core of the changes. So, although I wrote the comments
> early, I wanted to read first all other patches before sending it.
> 
> > 
> > On Thu, May 03, 2018 at 04:52:53PM +0200, Hans Verkuil wrote:
> > > From: Hans Verkuil 
> > > 
> > > Add initial media request support:
> > > 
> > > 1) Add MEDIA_IOC_REQUEST_ALLOC ioctl support to media-device.c
> > > 2) Add struct media_request to store request objects.
> > > 3) Add struct media_request_object to represent a request object.
> > > 4) Add MEDIA_REQUEST_IOC_QUEUE/REINIT ioctl support.
> > > 
> > > Basic lifecycle: the application allocates a request, adds
> > > objects to it, queues the request, polls until it is completed
> > > and can then read the final values of the objects at the time
> > > of completion. When it closes the file descriptor the request
> > > memory will be freed (actually, when the last user of that request
> > > releases the request).
> > > 
> > > Drivers will bind an object to a request (the 'adds objects to it'
> > > phase), when MEDIA_REQUEST_IOC_QUEUE is called the request is
> > > validated (req_validate op), then queued (the req_queue op).
> > > 
> > > When done with an object it can either be unbound from the request
> > > (e.g. when the driver has finished with a vb2 buffer) or marked as
> > > completed (e.g. for controls associated with a buffer). When all
> > > objects in the request are completed (or unbound), then the request
> > > fd will signal an exception (poll).
> > > 
> > > Signed-off-by: Hans Verkuil 
> 
> Hmm... As you're adding Copyrights from Intel/Google in this patch, that
> indicates that part of the stuff you're adding here were authored by
> others. So, you should use Co-developed-by: tag here, and get the SOBs
> from the other developers that did part of the work[1].
> 
> [1] except if your work was sponsored by Cisco, Intel and Google, but
> I think this is not the case.

I think this could be appropriate:

Co-developed-by: Laurent Pinchart 

Co-developed-by: Sakari Ailus 
Co-developed-by: Alexandre Courbot 

...

> > > +static long media_request_ioctl_queue(struct media_request *req)
> > > +{
> > > + struct media_device *mdev = req->mdev;
> > > + enum media_request_state state;
> > > + unsigned long flags;
> > > + int ret = 0;  
> > 
> > ret is unconditionally assigned below, no need to initialise here.
> > 
> > > +
> > > + dev_dbg(mdev->dev, "request: queue %s\n", req->debug_str);
> > > +
> > > + /*
> > > +  * Ensure the request that is validated will be the one that gets queued
> > > +  * next by serialising the queueing process. This mutex is also used
> > > +  * to serialize with canceling a vb2 queue and with setting values such
> > > +  * as controls in a request.
> > > +  */
> > > + mutex_lock(>req_queue_mutex);
> > > +
> > > + spin_lock_irqsave(>lock, flags);
> > > + state = atomic_cmpxchg(>state, MEDIA_REQUEST_STATE_IDLE,
> > > +MEDIA_REQUEST_STATE_VALIDATING);
> > > + spin_unlock_irqrestore(>lock, flags);
> 
> It looks weird to serialize access to it with a mutex, a spin lock and 
> an atomic call.

req->lock is needed for state changes from idle to other states as also
other struct members need to be serialised with that, with the request
state. Which is kind of in line with my earlier point: there's little
if any benefit in making this field atomic.

The mutex is there to ensure only a single request remains in validating
state, i.e. we will be changing the state of the device request tip one
request at a time.

> 
> IMHO, locking is still an issue here. I would love to test the 
> locks with some tool that would randomize syscalls, issuing close(),
> poll() and read() at wrong times and inverting the order of some calls, 
> in order to do some empiric test that all locks are at the right places.
> 
> Complex locking schemas like that usually tend to cause a lot of
> troubles.
> 
> > > + if (state != MEDIA_REQUEST_STATE_IDLE) {
> > > + dev_dbg(mdev->dev,
> > > + "request: unable to queue %s, request in state %s\n",
> > > + req->debug_str, media_request_state_str(state));
> > > + mutex_unlock(>req_queue_mutex);
> > > + return -EBUSY;
> > > + }
> > > +
> > > + ret = mdev->ops->req_validate(req);
> > > +
> > > + /*
> > > +  * If the req_validate was successful, then we mark the state as QUEUED
> > > +  * and call req_queue. The reason we set the state first is 

Re: [PATCHv13 03/28] media-request: implement media requests

[Mauro Carvalho Chehab]

Em Fri, 4 May 2018 15:27:50 +0300
Sakari Ailus  escreveu:

> Hi Hans,
> 
> I've read this patch a large number of times and I think also the details
> begin to seem sound. A few comments below.

I'm sending this after analyzing the other patches in this series,
as this is the core of the changes. So, although I wrote the comments
early, I wanted to read first all other patches before sending it.

> 
> On Thu, May 03, 2018 at 04:52:53PM +0200, Hans Verkuil wrote:
> > From: Hans Verkuil 
> > 
> > Add initial media request support:
> > 
> > 1) Add MEDIA_IOC_REQUEST_ALLOC ioctl support to media-device.c
> > 2) Add struct media_request to store request objects.
> > 3) Add struct media_request_object to represent a request object.
> > 4) Add MEDIA_REQUEST_IOC_QUEUE/REINIT ioctl support.
> > 
> > Basic lifecycle: the application allocates a request, adds
> > objects to it, queues the request, polls until it is completed
> > and can then read the final values of the objects at the time
> > of completion. When it closes the file descriptor the request
> > memory will be freed (actually, when the last user of that request
> > releases the request).
> > 
> > Drivers will bind an object to a request (the 'adds objects to it'
> > phase), when MEDIA_REQUEST_IOC_QUEUE is called the request is
> > validated (req_validate op), then queued (the req_queue op).
> > 
> > When done with an object it can either be unbound from the request
> > (e.g. when the driver has finished with a vb2 buffer) or marked as
> > completed (e.g. for controls associated with a buffer). When all
> > objects in the request are completed (or unbound), then the request
> > fd will signal an exception (poll).
> > 
> > Signed-off-by: Hans Verkuil 

Hmm... As you're adding Copyrights from Intel/Google in this patch, that
indicates that part of the stuff you're adding here were authored by
others. So, you should use Co-developed-by: tag here, and get the SOBs
from the other developers that did part of the work[1].

[1] except if your work was sponsored by Cisco, Intel and Google, but
I think this is not the case.

> > ---
> >  drivers/media/Makefile|   3 +-
> >  drivers/media/media-device.c  |  14 ++
> >  drivers/media/media-request.c | 407 ++
> >  include/media/media-device.h  |  16 ++
> >  include/media/media-request.h | 244 
> >  5 files changed, 683 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..bb6a64acd3f0 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_validate || !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,8 @@ void media_device_init(struct media_device *mdev)
> > INIT_LIST_HEAD(>pads);
> > INIT_LIST_HEAD(>links);
> > INIT_LIST_HEAD(>entity_notify);
> > +
> > +   mutex_init(>req_queue_mutex);
> > mutex_init(>graph_mutex);
> > ida_init(>entity_internal_idx);
> >  
> > @@ -699,6 +712,7 @@ void media_device_cleanup(struct media_device *mdev)
> > mdev->entity_internal_idx_max = 0;
> > 

Re: [PATCHv13 03/28] media-request: implement media requests

[Sakari Ailus]

Hi Hans,

I've read this patch a large number of times and I think also the details
begin to seem sound. A few comments below.

On Thu, May 03, 2018 at 04:52:53PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> Add initial media request support:
> 
> 1) Add MEDIA_IOC_REQUEST_ALLOC ioctl support to media-device.c
> 2) Add struct media_request to store request objects.
> 3) Add struct media_request_object to represent a request object.
> 4) Add MEDIA_REQUEST_IOC_QUEUE/REINIT ioctl support.
> 
> Basic lifecycle: the application allocates a request, adds
> objects to it, queues the request, polls until it is completed
> and can then read the final values of the objects at the time
> of completion. When it closes the file descriptor the request
> memory will be freed (actually, when the last user of that request
> releases the request).
> 
> Drivers will bind an object to a request (the 'adds objects to it'
> phase), when MEDIA_REQUEST_IOC_QUEUE is called the request is
> validated (req_validate op), then queued (the req_queue op).
> 
> When done with an object it can either be unbound from the request
> (e.g. when the driver has finished with a vb2 buffer) or marked as
> completed (e.g. for controls associated with a buffer). When all
> objects in the request are completed (or unbound), then the request
> fd will signal an exception (poll).
> 
> Signed-off-by: Hans Verkuil 
> ---
>  drivers/media/Makefile|   3 +-
>  drivers/media/media-device.c  |  14 ++
>  drivers/media/media-request.c | 407 ++
>  include/media/media-device.h  |  16 ++
>  include/media/media-request.h | 244 
>  5 files changed, 683 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..bb6a64acd3f0 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_validate || !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,8 @@ void media_device_init(struct media_device *mdev)
>   INIT_LIST_HEAD(>pads);
>   INIT_LIST_HEAD(>links);
>   INIT_LIST_HEAD(>entity_notify);
> +
> + mutex_init(>req_queue_mutex);
>   mutex_init(>graph_mutex);
>   ida_init(>entity_internal_idx);
>  
> @@ -699,6 +712,7 @@ void media_device_cleanup(struct media_device *mdev)
>   mdev->entity_internal_idx_max = 0;
>   media_graph_walk_cleanup(>pm_count_walk);
>   mutex_destroy(>graph_mutex);
> + mutex_destroy(>req_queue_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_device_cleanup);
>  
> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
> new file mode 100644
> index ..c216c4ab628b
> --- /dev/null
> +++ b/drivers/media/media-request.c
> @@ -0,0 +1,407 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Media device request objects
> + *
> + * Copyright 2018 Cisco Systems, Inc. and/or its affiliates. All rights 
> reserved.
> + * Copyright (C) 2018 Intel Corporation
> + * Copyright (C) 2018 Google, Inc.
> + *
> + * Author: Hans Verkuil 
> + * Author: Sakari Ailus 
> + */
> +
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +static const