Re: [RFC PATCH 1/9] media: add request API core and UAPI

[Hans Verkuil]

On 02/02/2018 08:33 AM, Sakari Ailus wrote:



 +struct media_request_entity_data *
 +media_request_get_entity_data(struct media_request *req,
 +   struct media_entity *entity, void *fh)
>>>
>>> This makes the assumption that request data is bound to entities. How does
>>> this work with links?
>>>
>>> I wonder if it should rather be bound to graph objects, or certain graph
>>> objects. Having a standard way to bind request specific information e.g. to
>>> entities is definitely worth having, though.
>>>
>>> V4L2 framework specific information would be needed across the media graph
>>> and it'd be good to store it in a non-driver specific way. What I think
>>> you'd need is an interface that allows storing information based on two
>>> keys --- the request and e.g. a pointer provided by the caller. The V4L2
>>> framework would have one key, e.g. a pointer to an empty struct defined
>>> somewhere in the V4L2 framework could be used for the purpose.
>>>
>>> Going forward, the entire media graph state will be subject to changing
>>> through requests. This includes link state, media bus and pixel formats,
>>> cropping and scaling configurations, everything. Let's not try to go there
>>> yet in this patchset, but what I'm asking is to keep the goal in mind when
>>> implementating the request API.
>>
>> Yeah, I think a similar idea is brought up in the cover letter of the
>> next revision (although for different reasons). Entities are probably
>> not a one-fit for all use-cases.
>>
>> For the case of links though, I believe that the "entity" that would
>> control them would be the media controller itself, since it is the one
>> that takes the MEDIA_IOC_SETUP_LINK ioctl. But even for this case, we
>> cannot use an entity to look up the media_device, so something more
>> generic like an opaque key would probably be needed.
> 
> Perhaps in the near future we still need a little less than that. Changing
> something that has a state in V4L2 will be troublesome and will require
> managing state of what is now stream centric.
> 
> I still think that the framework would need to do the job of managing the
> video buffers related to a request as well as controls without necessarily
> trying to generalise that right now. But how to store these in a meaningful
> way? Putting them to the request itself would be one option: you'll need to
> dig the request up anyway when things are associated to it, and the driver
> needs it when it is queued.
> 
> I wonder what Hans and Laurent think.

I think this is something for the future. I want to avoid delaying the Request
API for endless internal design discussions. The public API should be solid,
but the internal framework will undoubtedly need to change in the future.

That's OK. The reality is that there is a lot of demand for the Request API for
stateless codecs, and that is what we should concentrate on.

Should the framework manage the buffers? I don't even know what is meant with
that exactly, let alone that I can give an answer.

Let's stay focused: 1) solid uAPI, 2) stateless codec support. The internal
framework shouldn't of course make it harder than it needs to to later extend
the support to camera pipelines, but neither should we spend much time on it
or we will never get this in.

Regards,

Hans

Re: [RFC PATCH 1/9] media: add request API core and UAPI

[Hans Verkuil]

On 02/02/2018 08:33 AM, Sakari Ailus wrote:



 +struct media_request_entity_data *
 +media_request_get_entity_data(struct media_request *req,
 +   struct media_entity *entity, void *fh)
>>>
>>> This makes the assumption that request data is bound to entities. How does
>>> this work with links?
>>>
>>> I wonder if it should rather be bound to graph objects, or certain graph
>>> objects. Having a standard way to bind request specific information e.g. to
>>> entities is definitely worth having, though.
>>>
>>> V4L2 framework specific information would be needed across the media graph
>>> and it'd be good to store it in a non-driver specific way. What I think
>>> you'd need is an interface that allows storing information based on two
>>> keys --- the request and e.g. a pointer provided by the caller. The V4L2
>>> framework would have one key, e.g. a pointer to an empty struct defined
>>> somewhere in the V4L2 framework could be used for the purpose.
>>>
>>> Going forward, the entire media graph state will be subject to changing
>>> through requests. This includes link state, media bus and pixel formats,
>>> cropping and scaling configurations, everything. Let's not try to go there
>>> yet in this patchset, but what I'm asking is to keep the goal in mind when
>>> implementating the request API.
>>
>> Yeah, I think a similar idea is brought up in the cover letter of the
>> next revision (although for different reasons). Entities are probably
>> not a one-fit for all use-cases.
>>
>> For the case of links though, I believe that the "entity" that would
>> control them would be the media controller itself, since it is the one
>> that takes the MEDIA_IOC_SETUP_LINK ioctl. But even for this case, we
>> cannot use an entity to look up the media_device, so something more
>> generic like an opaque key would probably be needed.
> 
> Perhaps in the near future we still need a little less than that. Changing
> something that has a state in V4L2 will be troublesome and will require
> managing state of what is now stream centric.
> 
> I still think that the framework would need to do the job of managing the
> video buffers related to a request as well as controls without necessarily
> trying to generalise that right now. But how to store these in a meaningful
> way? Putting them to the request itself would be one option: you'll need to
> dig the request up anyway when things are associated to it, and the driver
> needs it when it is queued.
> 
> I wonder what Hans and Laurent think.

I think this is something for the future. I want to avoid delaying the Request
API for endless internal design discussions. The public API should be solid,
but the internal framework will undoubtedly need to change in the future.

That's OK. The reality is that there is a lot of demand for the Request API for
stateless codecs, and that is what we should concentrate on.

Should the framework manage the buffers? I don't even know what is meant with
that exactly, let alone that I can give an answer.

Let's stay focused: 1) solid uAPI, 2) stateless codec support. The internal
framework shouldn't of course make it harder than it needs to to later extend
the support to camera pipelines, but neither should we spend much time on it
or we will never get this in.

Regards,

Hans

Re: [RFC PATCH 1/9] media: add request API core and UAPI

[Tomasz Figa]

On Fri, Feb 2, 2018 at 4:33 PM, Sakari Ailus
 wrote:
>> >> +/**
>> >> + * struct media_request_queue - queue of requests
>> >> + *
>> >> + * @mdev:media_device that manages this queue
>> >> + * @ops: implementation of the queue
>> >> + * @mutex:   protects requests, active_request, req_id, and all members 
>> >> of
>> >> + *   struct media_request
>> >> + * @active_request: request being currently run by this queue
>> >> + * @requests:list of requests (not in any particular order) that 
>> >> this
>> >> + *   queue owns.
>> >> + * @req_id:  counter used to identify requests for debugging purposes
>> >> + */
>> >> +struct media_request_queue {
>> >> + struct media_device *mdev;
>> >> + const struct media_request_queue_ops *ops;
>> >> +
>> >> + struct mutex mutex;
>> >
>> > Any particular reason for using a mutex? The request queue lock will need
>> > to be acquired from interrupts, too, so this should be changed to a
>> > spinlock.
>>
>> Will it be acquired from interrupts? In any case it should be possible
>> to change this to a spinlock.
>
> Using mutexes will effectively make this impossible, and I don't think we
> can safely say there's not going to be a need for that. So spinlocks,
> please.
>

IMHO whether a mutex or spinlock is the right thing depends on what
kind of critical section it is used for. If it only protects data (and
according to the comment, this one seems to do so), spinlock might
actually have better properties, e.g. not introducing the need to
reschedule, if another CPU is accessing the data at the moment. It
might also depend on how heavy the data accesses are, though. We
shouldn't need to spin for too long time.

Best regards,
Tomasz

Re: [RFC PATCH 1/9] media: add request API core and UAPI

[Tomasz Figa]

On Fri, Feb 2, 2018 at 4:33 PM, Sakari Ailus
 wrote:
>> >> +/**
>> >> + * struct media_request_queue - queue of requests
>> >> + *
>> >> + * @mdev:media_device that manages this queue
>> >> + * @ops: implementation of the queue
>> >> + * @mutex:   protects requests, active_request, req_id, and all members 
>> >> of
>> >> + *   struct media_request
>> >> + * @active_request: request being currently run by this queue
>> >> + * @requests:list of requests (not in any particular order) that 
>> >> this
>> >> + *   queue owns.
>> >> + * @req_id:  counter used to identify requests for debugging purposes
>> >> + */
>> >> +struct media_request_queue {
>> >> + struct media_device *mdev;
>> >> + const struct media_request_queue_ops *ops;
>> >> +
>> >> + struct mutex mutex;
>> >
>> > Any particular reason for using a mutex? The request queue lock will need
>> > to be acquired from interrupts, too, so this should be changed to a
>> > spinlock.
>>
>> Will it be acquired from interrupts? In any case it should be possible
>> to change this to a spinlock.
>
> Using mutexes will effectively make this impossible, and I don't think we
> can safely say there's not going to be a need for that. So spinlocks,
> please.
>

IMHO whether a mutex or spinlock is the right thing depends on what
kind of critical section it is used for. If it only protects data (and
according to the comment, this one seems to do so), spinlock might
actually have better properties, e.g. not introducing the need to
reschedule, if another CPU is accessing the data at the moment. It
might also depend on how heavy the data accesses are, though. We
shouldn't need to spin for too long time.

Best regards,
Tomasz

Re: [RFC PATCH 1/9] media: add request API core and UAPI

[Sakari Ailus]

Hi Alexandre,

On Tue, Jan 30, 2018 at 01:23:05PM +0900, Alexandre Courbot wrote:
> Hi Sakari, thanks for the review!
> 
> The version you reviewed is not the latest one, but I suppose most of
> your comments still apply.
> 
> On Fri, Jan 26, 2018 at 5:39 PM, Sakari Ailus  wrote:
> > Hi Alexandre,
> >
> > I remember it was discussed that the work after the V4L2 jobs API would
> > continue from the existing request API patches. I see that at least the
> > rather important support for events is missing in this version. Why was it
> > left out?
> 
> Request completion is signaled by polling on the request FD, so we
> don't need to rely on V4L2 events to signal this anymore. If we want
> to signal different kinds of events on requests we could implement a
> more sophisticated event system on top of that, but for our current
> needs polling is sufficient.

Right. This works for now indeed. We will need to revisit this when
requests are moved to the media device in the future.

> 
> What other kind of event besides completion could we want to deliver
> to user-space from a request?
> 
> >
> > I also see that variable size IOCTL argument support is no longer included.
> 
> Do we need this for the request API?

Technically there's no strict need for that now. However when the requests
are moved to the media device (i.e. other device nodes are not needed
anymore), then this is a must.

It was proposed and AFAIR agreed on as well that new media device
IOCTLs would not use reserved fields any longer but rely on variable size
IOCTL arguments instead. This is in line with your request argument struct
having no reserved fields and I don't think we should add them there.

> 
> >
> > On Fri, Dec 15, 2017 at 04:56:17PM +0900, Alexandre Courbot wrote:
> >> The request API provides a way to group buffers and device parameters
> >> into units of work to be queued and executed. This patch introduces the
> >> UAPI and core framework.
> >>
> >> This patch is based on the previous work by Laurent Pinchart. The core
> >> has changed considerably, but the UAPI is mostly untouched.
> >>
> >> Signed-off-by: Alexandre Courbot 
> >> ---
> >>  drivers/media/Makefile   |   3 +-
> >>  drivers/media/media-device.c |   6 +
> >>  drivers/media/media-request.c| 390 
> >> +++
> >>  drivers/media/v4l2-core/v4l2-ioctl.c |   2 +-
> >>  include/media/media-device.h |   3 +
> >>  include/media/media-entity.h |   6 +
> >>  include/media/media-request.h| 269 
> >>  include/uapi/linux/media.h   |  11 +
> >>  8 files changed, 688 insertions(+), 2 deletions(-)
> >>  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 e79f72b8b858..045cec7d2de9 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
> >>
> >> @@ -407,6 +408,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_CMD, media_device_request_cmd, 0),
> >>  };
> >>
> >>  static long media_device_ioctl(struct file *filp, unsigned int cmd,
> >> @@ -688,6 +690,10 @@ EXPORT_SYMBOL_GPL(media_device_init);
> >>
> >>  void media_device_cleanup(struct media_device *mdev)
> >>  {
> >> + if (mdev->req_queue) {
> >> + mdev->req_queue->ops->release(mdev->req_queue);
> >> + mdev->req_queue = NULL;
> >> + }
> >>   ida_destroy(>entity_internal_idx);
> >>   mdev->entity_internal_idx_max = 0;
> >>   media_graph_walk_cleanup(>pm_count_walk);
> >> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
> >> new file mode 100644
> >> index ..15dc65ddfe41
> >> --- /dev/null
> >> +++ b/drivers/media/media-request.c
> >> @@ -0,0 +1,390 @@
> >> +/*
> >> + * Request and request queue base management
> >> + *
> >> + * Copyright (C) 2017, The Chromium OS 

Re: [RFC PATCH 1/9] media: add request API core and UAPI

[Sakari Ailus]

Hi Alexandre,

On Tue, Jan 30, 2018 at 01:23:05PM +0900, Alexandre Courbot wrote:
> Hi Sakari, thanks for the review!
> 
> The version you reviewed is not the latest one, but I suppose most of
> your comments still apply.
> 
> On Fri, Jan 26, 2018 at 5:39 PM, Sakari Ailus  wrote:
> > Hi Alexandre,
> >
> > I remember it was discussed that the work after the V4L2 jobs API would
> > continue from the existing request API patches. I see that at least the
> > rather important support for events is missing in this version. Why was it
> > left out?
> 
> Request completion is signaled by polling on the request FD, so we
> don't need to rely on V4L2 events to signal this anymore. If we want
> to signal different kinds of events on requests we could implement a
> more sophisticated event system on top of that, but for our current
> needs polling is sufficient.

Right. This works for now indeed. We will need to revisit this when
requests are moved to the media device in the future.

> 
> What other kind of event besides completion could we want to deliver
> to user-space from a request?
> 
> >
> > I also see that variable size IOCTL argument support is no longer included.
> 
> Do we need this for the request API?

Technically there's no strict need for that now. However when the requests
are moved to the media device (i.e. other device nodes are not needed
anymore), then this is a must.

It was proposed and AFAIR agreed on as well that new media device
IOCTLs would not use reserved fields any longer but rely on variable size
IOCTL arguments instead. This is in line with your request argument struct
having no reserved fields and I don't think we should add them there.

> 
> >
> > On Fri, Dec 15, 2017 at 04:56:17PM +0900, Alexandre Courbot wrote:
> >> The request API provides a way to group buffers and device parameters
> >> into units of work to be queued and executed. This patch introduces the
> >> UAPI and core framework.
> >>
> >> This patch is based on the previous work by Laurent Pinchart. The core
> >> has changed considerably, but the UAPI is mostly untouched.
> >>
> >> Signed-off-by: Alexandre Courbot 
> >> ---
> >>  drivers/media/Makefile   |   3 +-
> >>  drivers/media/media-device.c |   6 +
> >>  drivers/media/media-request.c| 390 
> >> +++
> >>  drivers/media/v4l2-core/v4l2-ioctl.c |   2 +-
> >>  include/media/media-device.h |   3 +
> >>  include/media/media-entity.h |   6 +
> >>  include/media/media-request.h| 269 
> >>  include/uapi/linux/media.h   |  11 +
> >>  8 files changed, 688 insertions(+), 2 deletions(-)
> >>  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 e79f72b8b858..045cec7d2de9 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
> >>
> >> @@ -407,6 +408,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_CMD, media_device_request_cmd, 0),
> >>  };
> >>
> >>  static long media_device_ioctl(struct file *filp, unsigned int cmd,
> >> @@ -688,6 +690,10 @@ EXPORT_SYMBOL_GPL(media_device_init);
> >>
> >>  void media_device_cleanup(struct media_device *mdev)
> >>  {
> >> + if (mdev->req_queue) {
> >> + mdev->req_queue->ops->release(mdev->req_queue);
> >> + mdev->req_queue = NULL;
> >> + }
> >>   ida_destroy(>entity_internal_idx);
> >>   mdev->entity_internal_idx_max = 0;
> >>   media_graph_walk_cleanup(>pm_count_walk);
> >> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
> >> new file mode 100644
> >> index ..15dc65ddfe41
> >> --- /dev/null
> >> +++ b/drivers/media/media-request.c
> >> @@ -0,0 +1,390 @@
> >> +/*
> >> + * Request and request queue base management
> >> + *
> >> + * Copyright (C) 2017, The Chromium OS Authors.  All rights reserved.
> >> + *
> >> + * 

Re: [RFC PATCH 1/9] media: add request API core and UAPI

[Alexandre Courbot]

Hi Sakari, thanks for the review!

The version you reviewed is not the latest one, but I suppose most of
your comments still apply.

On Fri, Jan 26, 2018 at 5:39 PM, Sakari Ailus  wrote:
> Hi Alexandre,
>
> I remember it was discussed that the work after the V4L2 jobs API would
> continue from the existing request API patches. I see that at least the
> rather important support for events is missing in this version. Why was it
> left out?

Request completion is signaled by polling on the request FD, so we
don't need to rely on V4L2 events to signal this anymore. If we want
to signal different kinds of events on requests we could implement a
more sophisticated event system on top of that, but for our current
needs polling is sufficient.

What other kind of event besides completion could we want to deliver
to user-space from a request?

>
> I also see that variable size IOCTL argument support is no longer included.

Do we need this for the request API?

>
> On Fri, Dec 15, 2017 at 04:56:17PM +0900, Alexandre Courbot wrote:
>> The request API provides a way to group buffers and device parameters
>> into units of work to be queued and executed. This patch introduces the
>> UAPI and core framework.
>>
>> This patch is based on the previous work by Laurent Pinchart. The core
>> has changed considerably, but the UAPI is mostly untouched.
>>
>> Signed-off-by: Alexandre Courbot 
>> ---
>>  drivers/media/Makefile   |   3 +-
>>  drivers/media/media-device.c |   6 +
>>  drivers/media/media-request.c| 390 
>> +++
>>  drivers/media/v4l2-core/v4l2-ioctl.c |   2 +-
>>  include/media/media-device.h |   3 +
>>  include/media/media-entity.h |   6 +
>>  include/media/media-request.h| 269 
>>  include/uapi/linux/media.h   |  11 +
>>  8 files changed, 688 insertions(+), 2 deletions(-)
>>  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 e79f72b8b858..045cec7d2de9 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
>>
>> @@ -407,6 +408,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_CMD, media_device_request_cmd, 0),
>>  };
>>
>>  static long media_device_ioctl(struct file *filp, unsigned int cmd,
>> @@ -688,6 +690,10 @@ EXPORT_SYMBOL_GPL(media_device_init);
>>
>>  void media_device_cleanup(struct media_device *mdev)
>>  {
>> + if (mdev->req_queue) {
>> + mdev->req_queue->ops->release(mdev->req_queue);
>> + mdev->req_queue = NULL;
>> + }
>>   ida_destroy(>entity_internal_idx);
>>   mdev->entity_internal_idx_max = 0;
>>   media_graph_walk_cleanup(>pm_count_walk);
>> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
>> new file mode 100644
>> index ..15dc65ddfe41
>> --- /dev/null
>> +++ b/drivers/media/media-request.c
>> @@ -0,0 +1,390 @@
>> +/*
>> + * Request and request queue base management
>> + *
>> + * Copyright (C) 2017, The Chromium OS Authors.  All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +#include 
>
> Alphabetic order, please.

Fixed.

>
>> +
>> +const struct file_operations request_fops;
>> +
>> +void media_request_get(struct media_request *req)
>
> Please return the media request, too.

Fixed.

>
>> +{
>> + kref_get(>kref);
>> +}
>> 

Re: [RFC PATCH 1/9] media: add request API core and UAPI

[Alexandre Courbot]

Hi Sakari, thanks for the review!

The version you reviewed is not the latest one, but I suppose most of
your comments still apply.

On Fri, Jan 26, 2018 at 5:39 PM, Sakari Ailus  wrote:
> Hi Alexandre,
>
> I remember it was discussed that the work after the V4L2 jobs API would
> continue from the existing request API patches. I see that at least the
> rather important support for events is missing in this version. Why was it
> left out?

Request completion is signaled by polling on the request FD, so we
don't need to rely on V4L2 events to signal this anymore. If we want
to signal different kinds of events on requests we could implement a
more sophisticated event system on top of that, but for our current
needs polling is sufficient.

What other kind of event besides completion could we want to deliver
to user-space from a request?

>
> I also see that variable size IOCTL argument support is no longer included.

Do we need this for the request API?

>
> On Fri, Dec 15, 2017 at 04:56:17PM +0900, Alexandre Courbot wrote:
>> The request API provides a way to group buffers and device parameters
>> into units of work to be queued and executed. This patch introduces the
>> UAPI and core framework.
>>
>> This patch is based on the previous work by Laurent Pinchart. The core
>> has changed considerably, but the UAPI is mostly untouched.
>>
>> Signed-off-by: Alexandre Courbot 
>> ---
>>  drivers/media/Makefile   |   3 +-
>>  drivers/media/media-device.c |   6 +
>>  drivers/media/media-request.c| 390 
>> +++
>>  drivers/media/v4l2-core/v4l2-ioctl.c |   2 +-
>>  include/media/media-device.h |   3 +
>>  include/media/media-entity.h |   6 +
>>  include/media/media-request.h| 269 
>>  include/uapi/linux/media.h   |  11 +
>>  8 files changed, 688 insertions(+), 2 deletions(-)
>>  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 e79f72b8b858..045cec7d2de9 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
>>
>> @@ -407,6 +408,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_CMD, media_device_request_cmd, 0),
>>  };
>>
>>  static long media_device_ioctl(struct file *filp, unsigned int cmd,
>> @@ -688,6 +690,10 @@ EXPORT_SYMBOL_GPL(media_device_init);
>>
>>  void media_device_cleanup(struct media_device *mdev)
>>  {
>> + if (mdev->req_queue) {
>> + mdev->req_queue->ops->release(mdev->req_queue);
>> + mdev->req_queue = NULL;
>> + }
>>   ida_destroy(>entity_internal_idx);
>>   mdev->entity_internal_idx_max = 0;
>>   media_graph_walk_cleanup(>pm_count_walk);
>> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
>> new file mode 100644
>> index ..15dc65ddfe41
>> --- /dev/null
>> +++ b/drivers/media/media-request.c
>> @@ -0,0 +1,390 @@
>> +/*
>> + * Request and request queue base management
>> + *
>> + * Copyright (C) 2017, The Chromium OS Authors.  All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +#include 
>
> Alphabetic order, please.

Fixed.

>
>> +
>> +const struct file_operations request_fops;
>> +
>> +void media_request_get(struct media_request *req)
>
> Please return the media request, too.

Fixed.

>
>> +{
>> + kref_get(>kref);
>> +}
>> +EXPORT_SYMBOL_GPL(media_request_get);
>> +
>> +struct 

Re: [RFC PATCH 1/9] media: add request API core and UAPI

[Sakari Ailus]

Hi Alexandre,

I remember it was discussed that the work after the V4L2 jobs API would
continue from the existing request API patches. I see that at least the
rather important support for events is missing in this version. Why was it
left out?

I also see that variable size IOCTL argument support is no longer included.

On Fri, Dec 15, 2017 at 04:56:17PM +0900, Alexandre Courbot wrote:
> The request API provides a way to group buffers and device parameters
> into units of work to be queued and executed. This patch introduces the
> UAPI and core framework.
> 
> This patch is based on the previous work by Laurent Pinchart. The core
> has changed considerably, but the UAPI is mostly untouched.
> 
> Signed-off-by: Alexandre Courbot 
> ---
>  drivers/media/Makefile   |   3 +-
>  drivers/media/media-device.c |   6 +
>  drivers/media/media-request.c| 390 
> +++
>  drivers/media/v4l2-core/v4l2-ioctl.c |   2 +-
>  include/media/media-device.h |   3 +
>  include/media/media-entity.h |   6 +
>  include/media/media-request.h| 269 
>  include/uapi/linux/media.h   |  11 +
>  8 files changed, 688 insertions(+), 2 deletions(-)
>  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 e79f72b8b858..045cec7d2de9 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
>  
> @@ -407,6 +408,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_CMD, media_device_request_cmd, 0),
>  };
>  
>  static long media_device_ioctl(struct file *filp, unsigned int cmd,
> @@ -688,6 +690,10 @@ EXPORT_SYMBOL_GPL(media_device_init);
>  
>  void media_device_cleanup(struct media_device *mdev)
>  {
> + if (mdev->req_queue) {
> + mdev->req_queue->ops->release(mdev->req_queue);
> + mdev->req_queue = NULL;
> + }
>   ida_destroy(>entity_internal_idx);
>   mdev->entity_internal_idx_max = 0;
>   media_graph_walk_cleanup(>pm_count_walk);
> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
> new file mode 100644
> index ..15dc65ddfe41
> --- /dev/null
> +++ b/drivers/media/media-request.c
> @@ -0,0 +1,390 @@
> +/*
> + * Request and request queue base management
> + *
> + * Copyright (C) 2017, The Chromium OS Authors.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 

Alphabetic order, please.

> +
> +const struct file_operations request_fops;
> +
> +void media_request_get(struct media_request *req)

Please return the media request, too.

> +{
> + kref_get(>kref);
> +}
> +EXPORT_SYMBOL_GPL(media_request_get);
> +
> +struct media_request *
> +media_request_get_from_fd(int fd)
> +{
> + struct file *f;
> + struct media_request *req;
> +
> + f = fget(fd);
> + if (!f)
> + return NULL;
> +
> + /* Not a request FD? */
> + if (f->f_op != _fops) {
> + fput(f);
> + return NULL;
> + }
> +
> + req = f->private_data;
> + media_request_get(req);
> + fput(f);
> +
> + return req;
> +}
> +EXPORT_SYMBOL_GPL(media_request_get_from_fd);
> +
> +static void media_request_release(struct kref *kref)
> +{
> + struct media_request_entity_data *data, *next;
> + struct media_request *req =
> + container_of(kref, typeof(*req), kref);
> + struct media_device *mdev = 

Re: [RFC PATCH 1/9] media: add request API core and UAPI

[Sakari Ailus]

Hi Alexandre,

I remember it was discussed that the work after the V4L2 jobs API would
continue from the existing request API patches. I see that at least the
rather important support for events is missing in this version. Why was it
left out?

I also see that variable size IOCTL argument support is no longer included.

On Fri, Dec 15, 2017 at 04:56:17PM +0900, Alexandre Courbot wrote:
> The request API provides a way to group buffers and device parameters
> into units of work to be queued and executed. This patch introduces the
> UAPI and core framework.
> 
> This patch is based on the previous work by Laurent Pinchart. The core
> has changed considerably, but the UAPI is mostly untouched.
> 
> Signed-off-by: Alexandre Courbot 
> ---
>  drivers/media/Makefile   |   3 +-
>  drivers/media/media-device.c |   6 +
>  drivers/media/media-request.c| 390 
> +++
>  drivers/media/v4l2-core/v4l2-ioctl.c |   2 +-
>  include/media/media-device.h |   3 +
>  include/media/media-entity.h |   6 +
>  include/media/media-request.h| 269 
>  include/uapi/linux/media.h   |  11 +
>  8 files changed, 688 insertions(+), 2 deletions(-)
>  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 e79f72b8b858..045cec7d2de9 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
>  
> @@ -407,6 +408,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_CMD, media_device_request_cmd, 0),
>  };
>  
>  static long media_device_ioctl(struct file *filp, unsigned int cmd,
> @@ -688,6 +690,10 @@ EXPORT_SYMBOL_GPL(media_device_init);
>  
>  void media_device_cleanup(struct media_device *mdev)
>  {
> + if (mdev->req_queue) {
> + mdev->req_queue->ops->release(mdev->req_queue);
> + mdev->req_queue = NULL;
> + }
>   ida_destroy(>entity_internal_idx);
>   mdev->entity_internal_idx_max = 0;
>   media_graph_walk_cleanup(>pm_count_walk);
> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
> new file mode 100644
> index ..15dc65ddfe41
> --- /dev/null
> +++ b/drivers/media/media-request.c
> @@ -0,0 +1,390 @@
> +/*
> + * Request and request queue base management
> + *
> + * Copyright (C) 2017, The Chromium OS Authors.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 

Alphabetic order, please.

> +
> +const struct file_operations request_fops;
> +
> +void media_request_get(struct media_request *req)

Please return the media request, too.

> +{
> + kref_get(>kref);
> +}
> +EXPORT_SYMBOL_GPL(media_request_get);
> +
> +struct media_request *
> +media_request_get_from_fd(int fd)
> +{
> + struct file *f;
> + struct media_request *req;
> +
> + f = fget(fd);
> + if (!f)
> + return NULL;
> +
> + /* Not a request FD? */
> + if (f->f_op != _fops) {
> + fput(f);
> + return NULL;
> + }
> +
> + req = f->private_data;
> + media_request_get(req);
> + fput(f);
> +
> + return req;
> +}
> +EXPORT_SYMBOL_GPL(media_request_get_from_fd);
> +
> +static void media_request_release(struct kref *kref)
> +{
> + struct media_request_entity_data *data, *next;
> + struct media_request *req =
> + container_of(kref, typeof(*req), kref);
> + struct media_device *mdev = req->queue->mdev;
> +
> + 

Re: [RFC PATCH 1/9] media: add request API core and UAPI

[Hans Verkuil]

Hi Alexandre,

A quick review: I'm primarily concentrating on the uAPI as that is the most
critical part to get right at this stage.

On 12/15/17 08:56, Alexandre Courbot wrote:
> The request API provides a way to group buffers and device parameters
> into units of work to be queued and executed. This patch introduces the
> UAPI and core framework.
> 
> This patch is based on the previous work by Laurent Pinchart. The core
> has changed considerably, but the UAPI is mostly untouched.
> 
> Signed-off-by: Alexandre Courbot 
> ---
>  drivers/media/Makefile   |   3 +-
>  drivers/media/media-device.c |   6 +
>  drivers/media/media-request.c| 390 
> +++
>  drivers/media/v4l2-core/v4l2-ioctl.c |   2 +-
>  include/media/media-device.h |   3 +
>  include/media/media-entity.h |   6 +
>  include/media/media-request.h| 269 
>  include/uapi/linux/media.h   |  11 +
>  8 files changed, 688 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/media/media-request.c
>  create mode 100644 include/media/media-request.h
> 



> diff --git a/include/media/media-request.h b/include/media/media-request.h
> new file mode 100644
> index ..ead7fd8898c4
> --- /dev/null
> +++ b/include/media/media-request.h
> @@ -0,0 +1,269 @@
> +/*
> + * Generic request queue.
> + *
> + * Copyright (C) 2017, The Chromium OS Authors.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _MEDIA_REQUEST_H
> +#define _MEDIA_REQUEST_H
> +
> +#include 
> +#include 
> +#include 
> +
> +struct media_device;
> +struct media_request_queue;
> +struct media_request_cmd;
> +struct media_entity;
> +struct media_request_entity_data;
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +
> +enum media_request_state {
> + MEDIA_REQUEST_STATE_IDLE,
> + MEDIA_REQUEST_STATE_QUEUED,
> + MEDIA_REQUEST_STATE_COMPLETE,

COMPLETE -> COMPLETED

> + MEDIA_REQUEST_STATE_DELETED,
> +};

Regards,

Hans

Re: [RFC PATCH 1/9] media: add request API core and UAPI

[Hans Verkuil]

Hi Alexandre,

A quick review: I'm primarily concentrating on the uAPI as that is the most
critical part to get right at this stage.

On 12/15/17 08:56, Alexandre Courbot wrote:
> The request API provides a way to group buffers and device parameters
> into units of work to be queued and executed. This patch introduces the
> UAPI and core framework.
> 
> This patch is based on the previous work by Laurent Pinchart. The core
> has changed considerably, but the UAPI is mostly untouched.
> 
> Signed-off-by: Alexandre Courbot 
> ---
>  drivers/media/Makefile   |   3 +-
>  drivers/media/media-device.c |   6 +
>  drivers/media/media-request.c| 390 
> +++
>  drivers/media/v4l2-core/v4l2-ioctl.c |   2 +-
>  include/media/media-device.h |   3 +
>  include/media/media-entity.h |   6 +
>  include/media/media-request.h| 269 
>  include/uapi/linux/media.h   |  11 +
>  8 files changed, 688 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/media/media-request.c
>  create mode 100644 include/media/media-request.h
> 



> diff --git a/include/media/media-request.h b/include/media/media-request.h
> new file mode 100644
> index ..ead7fd8898c4
> --- /dev/null
> +++ b/include/media/media-request.h
> @@ -0,0 +1,269 @@
> +/*
> + * Generic request queue.
> + *
> + * Copyright (C) 2017, The Chromium OS Authors.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _MEDIA_REQUEST_H
> +#define _MEDIA_REQUEST_H
> +
> +#include 
> +#include 
> +#include 
> +
> +struct media_device;
> +struct media_request_queue;
> +struct media_request_cmd;
> +struct media_entity;
> +struct media_request_entity_data;
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +
> +enum media_request_state {
> + MEDIA_REQUEST_STATE_IDLE,
> + MEDIA_REQUEST_STATE_QUEUED,
> + MEDIA_REQUEST_STATE_COMPLETE,

COMPLETE -> COMPLETED

> + MEDIA_REQUEST_STATE_DELETED,
> +};

Regards,

Hans

Re: [RFC PATCH 1/9] media: add request API core and UAPI

[Philippe Ombredanne]

Alexandre:

On Fri, Dec 15, 2017 at 8:56 AM, Alexandre Courbot
 wrote:
> The request API provides a way to group buffers and device parameters
> into units of work to be queued and executed. This patch introduces the
> UAPI and core framework.
>
> This patch is based on the previous work by Laurent Pinchart. The core
> has changed considerably, but the UAPI is mostly untouched.
>
> Signed-off-by: Alexandre Courbot 

 --- /dev/null
> +++ b/drivers/media/media-request.c
> @@ -0,0 +1,390 @@
> +/*
> + * Request and request queue base management
> + *
> + * Copyright (C) 2017, The Chromium OS Authors.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */

Have you considered using the new SPDX tags instead of this fine but
long legalese? And if other Chromium contributors could follow suit
and you could spread the word that would be even better!

See Thomas doc patches [1] for details.
Thanks!

[1] https://lkml.org/lkml/2017/12/4/934
-- 
Cordially
Philippe Ombredanne

Re: [RFC PATCH 1/9] media: add request API core and UAPI

[Philippe Ombredanne]

Alexandre:

On Fri, Dec 15, 2017 at 8:56 AM, Alexandre Courbot
 wrote:
> The request API provides a way to group buffers and device parameters
> into units of work to be queued and executed. This patch introduces the
> UAPI and core framework.
>
> This patch is based on the previous work by Laurent Pinchart. The core
> has changed considerably, but the UAPI is mostly untouched.
>
> Signed-off-by: Alexandre Courbot 

 --- /dev/null
> +++ b/drivers/media/media-request.c
> @@ -0,0 +1,390 @@
> +/*
> + * Request and request queue base management
> + *
> + * Copyright (C) 2017, The Chromium OS Authors.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */

Have you considered using the new SPDX tags instead of this fine but
long legalese? And if other Chromium contributors could follow suit
and you could spread the word that would be even better!

See Thomas doc patches [1] for details.
Thanks!

[1] https://lkml.org/lkml/2017/12/4/934
-- 
Cordially
Philippe Ombredanne

[RFC PATCH 1/9] media: add request API core and UAPI

[Alexandre Courbot]

The request API provides a way to group buffers and device parameters
into units of work to be queued and executed. This patch introduces the
UAPI and core framework.

This patch is based on the previous work by Laurent Pinchart. The core
has changed considerably, but the UAPI is mostly untouched.

Signed-off-by: Alexandre Courbot 
---
 drivers/media/Makefile   |   3 +-
 drivers/media/media-device.c |   6 +
 drivers/media/media-request.c| 390 +++
 drivers/media/v4l2-core/v4l2-ioctl.c |   2 +-
 include/media/media-device.h |   3 +
 include/media/media-entity.h |   6 +
 include/media/media-request.h| 269 
 include/uapi/linux/media.h   |  11 +
 8 files changed, 688 insertions(+), 2 deletions(-)
 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 e79f72b8b858..045cec7d2de9 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
 
@@ -407,6 +408,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_CMD, media_device_request_cmd, 0),
 };
 
 static long media_device_ioctl(struct file *filp, unsigned int cmd,
@@ -688,6 +690,10 @@ EXPORT_SYMBOL_GPL(media_device_init);
 
 void media_device_cleanup(struct media_device *mdev)
 {
+   if (mdev->req_queue) {
+   mdev->req_queue->ops->release(mdev->req_queue);
+   mdev->req_queue = NULL;
+   }
ida_destroy(>entity_internal_idx);
mdev->entity_internal_idx_max = 0;
media_graph_walk_cleanup(>pm_count_walk);
diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
new file mode 100644
index ..15dc65ddfe41
--- /dev/null
+++ b/drivers/media/media-request.c
@@ -0,0 +1,390 @@
+/*
+ * Request and request queue base management
+ *
+ * Copyright (C) 2017, The Chromium OS Authors.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+const struct file_operations request_fops;
+
+void media_request_get(struct media_request *req)
+{
+   kref_get(>kref);
+}
+EXPORT_SYMBOL_GPL(media_request_get);
+
+struct media_request *
+media_request_get_from_fd(int fd)
+{
+   struct file *f;
+   struct media_request *req;
+
+   f = fget(fd);
+   if (!f)
+   return NULL;
+
+   /* Not a request FD? */
+   if (f->f_op != _fops) {
+   fput(f);
+   return NULL;
+   }
+
+   req = f->private_data;
+   media_request_get(req);
+   fput(f);
+
+   return req;
+}
+EXPORT_SYMBOL_GPL(media_request_get_from_fd);
+
+static void media_request_release(struct kref *kref)
+{
+   struct media_request_entity_data *data, *next;
+   struct media_request *req =
+   container_of(kref, typeof(*req), kref);
+   struct media_device *mdev = req->queue->mdev;
+
+   dev_dbg(mdev->dev, "%s: releasing request %u\n", __func__, req->id);
+
+   list_del(>list);
+   list_for_each_entry_safe(data, next, >data, list) {
+   list_del(>list);
+   data->entity->req_ops->release_data(data);
+   }
+
+   req->queue->ops->req_free(req->queue, req);
+}
+
+void media_request_put(struct media_request *req)
+{
+   kref_put(>kref, media_request_release);
+}
+EXPORT_SYMBOL_GPL(media_request_put);
+
+struct media_request_entity_data *
+media_request_get_entity_data(struct media_request *req,
+ struct media_entity *entity, void *fh)
+{
+   struct 

[RFC PATCH 1/9] media: add request API core and UAPI

[Alexandre Courbot]

The request API provides a way to group buffers and device parameters
into units of work to be queued and executed. This patch introduces the
UAPI and core framework.

This patch is based on the previous work by Laurent Pinchart. The core
has changed considerably, but the UAPI is mostly untouched.

Signed-off-by: Alexandre Courbot 
---
 drivers/media/Makefile   |   3 +-
 drivers/media/media-device.c |   6 +
 drivers/media/media-request.c| 390 +++
 drivers/media/v4l2-core/v4l2-ioctl.c |   2 +-
 include/media/media-device.h |   3 +
 include/media/media-entity.h |   6 +
 include/media/media-request.h| 269 
 include/uapi/linux/media.h   |  11 +
 8 files changed, 688 insertions(+), 2 deletions(-)
 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 e79f72b8b858..045cec7d2de9 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
 
@@ -407,6 +408,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_CMD, media_device_request_cmd, 0),
 };
 
 static long media_device_ioctl(struct file *filp, unsigned int cmd,
@@ -688,6 +690,10 @@ EXPORT_SYMBOL_GPL(media_device_init);
 
 void media_device_cleanup(struct media_device *mdev)
 {
+   if (mdev->req_queue) {
+   mdev->req_queue->ops->release(mdev->req_queue);
+   mdev->req_queue = NULL;
+   }
ida_destroy(>entity_internal_idx);
mdev->entity_internal_idx_max = 0;
media_graph_walk_cleanup(>pm_count_walk);
diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
new file mode 100644
index ..15dc65ddfe41
--- /dev/null
+++ b/drivers/media/media-request.c
@@ -0,0 +1,390 @@
+/*
+ * Request and request queue base management
+ *
+ * Copyright (C) 2017, The Chromium OS Authors.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+const struct file_operations request_fops;
+
+void media_request_get(struct media_request *req)
+{
+   kref_get(>kref);
+}
+EXPORT_SYMBOL_GPL(media_request_get);
+
+struct media_request *
+media_request_get_from_fd(int fd)
+{
+   struct file *f;
+   struct media_request *req;
+
+   f = fget(fd);
+   if (!f)
+   return NULL;
+
+   /* Not a request FD? */
+   if (f->f_op != _fops) {
+   fput(f);
+   return NULL;
+   }
+
+   req = f->private_data;
+   media_request_get(req);
+   fput(f);
+
+   return req;
+}
+EXPORT_SYMBOL_GPL(media_request_get_from_fd);
+
+static void media_request_release(struct kref *kref)
+{
+   struct media_request_entity_data *data, *next;
+   struct media_request *req =
+   container_of(kref, typeof(*req), kref);
+   struct media_device *mdev = req->queue->mdev;
+
+   dev_dbg(mdev->dev, "%s: releasing request %u\n", __func__, req->id);
+
+   list_del(>list);
+   list_for_each_entry_safe(data, next, >data, list) {
+   list_del(>list);
+   data->entity->req_ops->release_data(data);
+   }
+
+   req->queue->ops->req_free(req->queue, req);
+}
+
+void media_request_put(struct media_request *req)
+{
+   kref_put(>kref, media_request_release);
+}
+EXPORT_SYMBOL_GPL(media_request_put);
+
+struct media_request_entity_data *
+media_request_get_entity_data(struct media_request *req,
+ struct media_entity *entity, void *fh)
+{
+   struct media_request_queue *queue =