[PATCHv3] media: add tuner standby op, use where needed

[Hans Verkuil]

The v4l2_subdev core s_power op was used for two different things: power on/off
sensors or video decoders/encoders and to put a tuner in standby (and only the
tuner!). There is no 'tuner wakeup' op, that's done automatically when the tuner
is accessed.

The danger with calling (s_power, 0) to put a tuner into standby is that it is
usually broadcast for all subdevs. So a video receiver subdev that supports
s_power will also be powered off, and since there is no corresponding (s_power, 
1)
they will never be powered on again.

In addition, this is specifically meant for tuners only since they draw the most
current.

This patch adds a new tuner op called 'standby' and replaces all calls to
(core, s_power, 0) by (tuner, standby). This prevents confusion between the two
uses of s_power. Note that there is no overlap: bridge drivers either just want
to put the tuner into standby, or they deal with powering on/off sensors. Never
both.

This also makes it easier to replace s_power for the remaining bridge drivers
with some PM code later.

Whether we want something cleaner for tuners in the future is a separate topic.
There is a lot of legacy code surrounding tuners, and I am very hesitant about
making changes there.

Signed-off-by: Hans Verkuil 
---
Changes since v2:
- update the tuner_standby documentation
Changes since v1:
- move the standby op to the tuner_ops, which makes much more sense.
---
 drivers/media/pci/cx23885/cx23885-core.c  |  2 +-
 drivers/media/pci/cx23885/cx23885-dvb.c   |  4 ++--
 drivers/media/pci/cx88/cx88-cards.c   |  2 +-
 drivers/media/pci/cx88/cx88-dvb.c |  4 ++--
 drivers/media/pci/saa7134/saa7134-video.c |  2 +-
 drivers/media/tuners/e4000.c  | 15 +++
 drivers/media/tuners/fc2580.c | 16 +++-
 drivers/media/tuners/msi001.c | 19 +++
 drivers/media/usb/au0828/au0828-video.c   |  4 ++--
 drivers/media/usb/cx231xx/cx231xx-video.c |  2 +-
 drivers/media/usb/em28xx/em28xx-video.c   |  4 ++--
 drivers/media/v4l2-core/tuner-core.c  | 15 +++
 include/media/v4l2-subdev.h   |  4 
 13 files changed, 28 insertions(+), 65 deletions(-)

diff --git a/drivers/media/pci/cx23885/cx23885-core.c 
b/drivers/media/pci/cx23885/cx23885-core.c
index 8f63df1cb418..b8394cdf030b 100644
--- a/drivers/media/pci/cx23885/cx23885-core.c
+++ b/drivers/media/pci/cx23885/cx23885-core.c
@@ -965,7 +965,7 @@ static int cx23885_dev_setup(struct cx23885_dev *dev)
cx23885_i2c_register(>i2c_bus[1]);
cx23885_i2c_register(>i2c_bus[2]);
cx23885_card_setup(dev);
-   call_all(dev, core, s_power, 0);
+   call_all(dev, tuner, standby);
cx23885_ir_init(dev);

if (dev->board == CX23885_BOARD_VIEWCAST_460E) {
diff --git a/drivers/media/pci/cx23885/cx23885-dvb.c 
b/drivers/media/pci/cx23885/cx23885-dvb.c
index 700422b538c0..ba5429877d1f 100644
--- a/drivers/media/pci/cx23885/cx23885-dvb.c
+++ b/drivers/media/pci/cx23885/cx23885-dvb.c
@@ -2514,8 +2514,8 @@ static int dvb_register(struct cx23885_tsport *port)
fe1->dvb.frontend->ops.ts_bus_ctrl = cx23885_dvb_bus_ctrl;
 #endif

-   /* Put the analog decoder in standby to keep it quiet */
-   call_all(dev, core, s_power, 0);
+   /* Put the tuner in standby to keep it quiet */
+   call_all(dev, tuner, standby);

if (fe0->dvb.frontend->ops.analog_ops.standby)
fe0->dvb.frontend->ops.analog_ops.standby(fe0->dvb.frontend);
diff --git a/drivers/media/pci/cx88/cx88-cards.c 
b/drivers/media/pci/cx88/cx88-cards.c
index 6df21b29ea17..4c92d2388c26 100644
--- a/drivers/media/pci/cx88/cx88-cards.c
+++ b/drivers/media/pci/cx88/cx88-cards.c
@@ -3592,7 +3592,7 @@ static void cx88_card_setup(struct cx88_core *core)
ctl.fname);
call_all(core, tuner, s_config, _cfg);
}
-   call_all(core, core, s_power, 0);
+   call_all(core, tuner, standby);
 }

 /* -- */
diff --git a/drivers/media/pci/cx88/cx88-dvb.c 
b/drivers/media/pci/cx88/cx88-dvb.c
index 49a335f4603e..3abef0b106be 100644
--- a/drivers/media/pci/cx88/cx88-dvb.c
+++ b/drivers/media/pci/cx88/cx88-dvb.c
@@ -1631,8 +1631,8 @@ static int dvb_register(struct cx8802_dev *dev)
if (fe1)
fe1->dvb.frontend->ops.ts_bus_ctrl = cx88_dvb_bus_ctrl;

-   /* Put the analog decoder in standby to keep it quiet */
-   call_all(core, core, s_power, 0);
+   /* Put the tuner in standby to keep it quiet */
+   call_all(core, tuner, standby);

/* register everything */
res = vb2_dvb_register_bus(>frontends, THIS_MODULE, dev,
diff --git a/drivers/media/pci/saa7134/saa7134-video.c 
b/drivers/media/pci/saa7134/saa7134-video.c
index 1ca6a32ad10e..4f1091a11e91 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -1200,7 

Re: [PATCHv2] media: add tuner standby op, use where needed

[Hans Verkuil]

On 02/21/2018 01:02 AM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Tuesday, 20 February 2018 11:44:20 EET Hans Verkuil wrote:
>> The v4l2_subdev core s_power op was used for two different things: power
>> on/off sensors or video decoders/encoders and to put a tuner in standby
>> (and only the tuner!). There is no 'tuner wakeup' op, that's done
>> automatically when the tuner is accessed.
>>
>> The danger with calling (s_power, 0) to put a tuner into standby is that it
>> is usually broadcast for all subdevs. So a video receiver subdev that
>> supports s_power will also be powered off, and since there is no
>> corresponding (s_power, 1) they will never be powered on again.
>>
>> In addition, this is specifically meant for tuners only since they draw the
>> most current.
>>
>> This patch adds a new tuner op called 'standby' and replaces all calls to
>> (core, s_power, 0) by (tuner, standby). This prevents confusion between the
>> two uses of s_power. Note that there is no overlap: bridge drivers either
>> just want to put the tuner into standby, or they deal with powering on/off
>> sensors. Never both.
>>
>> This also makes it easier to replace s_power for the remaining bridge
>> drivers with some PM code later.
>>
>> Whether we want something cleaner for tuners in the future is a separate
>> topic. There is a lot of legacy code surrounding tuners, and I am very
>> hesitant about making changes there.
>>
>> Signed-off-by: Hans Verkuil 
>> ---
>> Changes since v1:
>> - move the standby op to the tuner_ops, which makes much more sense.
>> ---
> 
> [snip]
> 
>> diff --git a/drivers/media/v4l2-core/tuner-core.c
>> b/drivers/media/v4l2-core/tuner-core.c index 82852f23a3b6..cb126baf8771
>> 100644
>> --- a/drivers/media/v4l2-core/tuner-core.c
>> +++ b/drivers/media/v4l2-core/tuner-core.c
>> @@ -1099,23 +1099,15 @@ static int tuner_s_radio(struct v4l2_subdev *sd)
>>   */
>>
>>  /**
>> - * tuner_s_power - controls the power state of the tuner
>> + * tuner_standby - controls the power state of the tuner
> 
> I'd update the description too.
> 
>>   * @sd: pointer to struct v4l2_subdev
>>   * @on: a zero value puts the tuner to sleep, non-zero wakes it up
> 
> And this parameter doesn't exist anymore. You could have caught that by 
> compiling the documentation.

Oops! I'll make a v3. Thanks for catching this.

> 
>>   */
>> -static int tuner_s_power(struct v4l2_subdev *sd, int on)
>> +static int tuner_standby(struct v4l2_subdev *sd)
>>  {
>>  struct tuner *t = to_tuner(sd);
>>  struct analog_demod_ops *analog_ops = >fe.ops.analog_ops;
>>
>> -if (on) {
>> -if (t->standby && set_mode(t, t->mode) == 0) {
>> -dprintk("Waking up tuner\n");
>> -set_freq(t, 0);
>> -}
>> -return 0;
>> -}
>> -
> 
> Interesting how this code was not used. I've had a look at tuner-core driver 
> out of curiosity, it clearly shows its age :/ I2C address probing belongs to 
> another century.

Not really. It's still needed for USB/PCI devices.

I was also surprised that it wasn't used. I expected to see some internal calls
to tuner_s_power(sd, 1) to turn things on, but that's not what happened.

Regards,

Hans

> 
>>  dprintk("Putting tuner to sleep\n");
>>  t->standby = true;
>>  if (analog_ops->standby)
>> @@ -1328,10 +1320,10 @@ static int tuner_command(struct i2c_client *client,
>> unsigned cmd, void *arg)
>>
>>  static const struct v4l2_subdev_core_ops tuner_core_ops = {
>>  .log_status = tuner_log_status,
>> -.s_power = tuner_s_power,
>>  };
>>
>>  static const struct v4l2_subdev_tuner_ops tuner_tuner_ops = {
>> +.standby = tuner_standby,
>>  .s_radio = tuner_s_radio,
>>  .g_tuner = tuner_g_tuner,
>>  .s_tuner = tuner_s_tuner,
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 980a86c08fce..62429cd89620 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -224,6 +224,9 @@ struct v4l2_subdev_core_ops {
>>   * struct v4l2_subdev_tuner_ops - Callbacks used when v4l device was opened
>>   *  in radio mode.
>>   *
>> + * @standby: puts the tuner in standby mode. It will be woken up
>> + *   automatically the next time it is used.
>> + *
> 
> I wouldn't have dared making such a statement as I don't trust myself as 
> being 
> able to give such a guarantee after reading the code :-)
> 
>>   * @s_radio: callback that switches the tuner to radio mode.
>>   *   drivers should explicitly call it when a tuner ops should
>>   *   operate on radio mode, before being able to handle it.
>> @@ -268,6 +271,7 @@ struct v4l2_subdev_core_ops {
>>   *}
>>   */
>>  struct v4l2_subdev_tuner_ops {
>> +int (*standby)(struct v4l2_subdev *sd);
>>  int (*s_radio)(struct v4l2_subdev *sd);
>>  int (*s_frequency)(struct v4l2_subdev *sd, const struct v4l2_frequency
>> *freq);
>>  int 

Re: [RFCv4 16/21] v4l2: video_device: support for creating requests

[Hans Verkuil]

On 02/21/2018 07:01 AM, Alexandre Courbot wrote:
> On Wed, Feb 21, 2018 at 1:35 AM, Hans Verkuil  wrote:
>> On 02/20/2018 05:44 AM, Alexandre Courbot wrote:
>>> Add a new VIDIOC_NEW_REQUEST ioctl, which allows to instanciate requests
>>> on devices that support the request API. Requests created that way can
>>> only control the device they originate from, making them suitable for
>>> simple devices, but not complex pipelines.
>>>
>>> Signed-off-by: Alexandre Courbot 
>>> ---
>>>  Documentation/ioctl/ioctl-number.txt |  1 +
>>>  drivers/media/v4l2-core/v4l2-dev.c   |  2 ++
>>>  drivers/media/v4l2-core/v4l2-ioctl.c | 25 +
>>>  include/media/v4l2-dev.h |  2 ++
>>>  include/uapi/linux/videodev2.h   |  3 +++
>>>  5 files changed, 33 insertions(+)
>>>
>>> diff --git a/Documentation/ioctl/ioctl-number.txt 
>>> b/Documentation/ioctl/ioctl-number.txt
>>> index 6501389d55b9..afdc9ed255b0 100644
>>> --- a/Documentation/ioctl/ioctl-number.txt
>>> +++ b/Documentation/ioctl/ioctl-number.txt
>>> @@ -286,6 +286,7 @@ Code  Seq#(hex)   Include FileComments
>>>   
>>>  'z'  10-4F   drivers/s390/crypto/zcrypt_api.hconflict!
>>>  '|'  00-7F   linux/media.h
>>> +'|'  80-9F   linux/media-request.h
>>>  0x80 00-1F   linux/fb.h
>>>  0x89 00-06   arch/x86/include/asm/sockios.h
>>>  0x89 0B-DF   linux/sockios.h
>>
>> This  doesn't belong in this patch.
> 
> Do you mean we need a separate patch for this?

Yes.

I now also see why you started at 0x80. Let's keep that for now.

> 
>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
>>> b/drivers/media/v4l2-core/v4l2-dev.c
>>> index 0301fe426a43..062ebee5bffc 100644
>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>> @@ -559,6 +559,8 @@ static void determine_valid_ioctls(struct video_device 
>>> *vdev)
>>>   set_bit(_IOC_NR(VIDIOC_TRY_EXT_CTRLS), valid_ioctls);
>>>   if (vdev->ctrl_handler || ops->vidioc_querymenu)
>>>   set_bit(_IOC_NR(VIDIOC_QUERYMENU), valid_ioctls);
>>> + if (vdev->req_mgr)
>>> + set_bit(_IOC_NR(VIDIOC_NEW_REQUEST), valid_ioctls);
>>>   SET_VALID_IOCTL(ops, VIDIOC_G_FREQUENCY, vidioc_g_frequency);
>>>   SET_VALID_IOCTL(ops, VIDIOC_S_FREQUENCY, vidioc_s_frequency);
>>>   SET_VALID_IOCTL(ops, VIDIOC_LOG_STATUS, vidioc_log_status);
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
>>> b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index ab4968ea443f..a45fe078f8ae 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -21,6 +21,7 @@
>>>
>>>  #include 
>>>
>>> +#include 
>>>  #include 
>>>  #include 
>>>  #include 
>>> @@ -842,6 +843,13 @@ static void v4l_print_freq_band(const void *arg, bool 
>>> write_only)
>>>   p->rangehigh, p->modulation);
>>>  }
>>>
>>> +static void vidioc_print_new_request(const void *arg, bool write_only)
>>> +{
>>> + const struct media_request_new *new = arg;
>>> +
>>> + pr_cont("fd=0x%x\n", new->fd);
>>
>> I'd use %d since fds are typically shown as signed integers.
> 
> Right.
> 
>>
>>> +}
>>> +
>>>  static void v4l_print_edid(const void *arg, bool write_only)
>>>  {
>>>   const struct v4l2_edid *p = arg;
>>> @@ -2486,6 +2494,22 @@ static int v4l_enum_freq_bands(const struct 
>>> v4l2_ioctl_ops *ops,
>>>   return -ENOTTY;
>>>  }
>>>
>>> +static int vidioc_new_request(const struct v4l2_ioctl_ops *ops,
>>> +   struct file *file, void *fh, void *arg)
>>> +{
>>> +#if IS_ENABLED(CONFIG_MEDIA_REQUEST_API)
>>> + struct media_request_new *new = arg;
>>> + struct video_device *vfd = video_devdata(file);
>>> +
>>> + if (!vfd->req_mgr)
>>> + return -ENOTTY;
>>> +
>>> + return media_request_ioctl_new(vfd->req_mgr, new);
>>> +#else
>>> + return -ENOTTY;
>>> +#endif
>>> +}
>>
>> You don't need the #ifdef's here. media_request_ioctl_new() will be stubbed 
>> if
>> CONFIG_MEDIA_REQUEST_API isn't set.
> 
> Correct.
> 
>>
>>> +
>>>  struct v4l2_ioctl_info {
>>>   unsigned int ioctl;
>>>   u32 flags;
>>> @@ -2617,6 +2641,7 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>>>   IOCTL_INFO_FNC(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, 
>>> v4l_print_freq_band, 0),
>>>   IOCTL_INFO_FNC(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, 
>>> v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
>>>   IOCTL_INFO_FNC(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, 
>>> v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, 
>>> id)),
>>> + IOCTL_INFO_FNC(VIDIOC_NEW_REQUEST, vidioc_new_request, 
>>> vidioc_print_new_request, 0),
>>>  };
>>>  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>>>
>>> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
>>> index 53f32022fabe..e6c4e10889bc 100644
>>> --- 

Re: [RFCv4 01/21] media: add request API core and UAPI

[Hans Verkuil]

On 02/21/2018 07:01 AM, Alexandre Courbot wrote:
> Hi Hans,
> 
> On Tue, Feb 20, 2018 at 7:36 PM, Hans Verkuil  wrote:
>> On 02/20/18 05:44, Alexandre Courbot wrote:



>>> +#define MEDIA_REQUEST_IOC(__cmd, func) 
>>>   \
>>> + [_IOC_NR(MEDIA_REQUEST_IOC_##__cmd) - 0x80] = { \
>>> + .cmd = MEDIA_REQUEST_IOC_##__cmd,   \
>>> + .fn = func, \
>>> + }
>>> +
>>> +struct media_request_ioctl_info {
>>> + unsigned int cmd;
>>> + long (*fn)(struct media_request *req);
>>> +};
>>> +
>>> +static const struct media_request_ioctl_info ioctl_info[] = {
>>> + MEDIA_REQUEST_IOC(SUBMIT, media_request_ioctl_submit),
>>> + MEDIA_REQUEST_IOC(REINIT, media_request_ioctl_reinit),
>>
>> There are only two ioctls, so there is really no need for the
>> MEDIA_REQUEST_IOC define. Just keep it simple.
> 
> The number of times it is used doesn't change the fact that it helps
> with readability IMHO.

But this macro just boils down to:

static const struct media_request_ioctl_info ioctl_info[] = {
{ MEDIA_REQUEST_IOC_SUBMIT, media_request_ioctl_submit },
{ MEDIA_REQUEST_IOC_REINIT, media_request_ioctl_reinit },
};

It's absolutely identical! So it seems senseless to me.

> 
>>
>>> +};
>>> +
>>> +static long media_request_ioctl(struct file *filp, unsigned int cmd,
>>> + unsigned long __arg)
>>> +{
>>> + struct media_request *req = filp->private_data;
>>> + const struct media_request_ioctl_info *info;
>>> +
>>> + if ((_IOC_NR(cmd) < 0x80) ||
>>
>> Why start the ioctl number at 0x80? Why not just 0?
>> It avoids all this hassle with the 0x80 offset.

There is no clash with the MC ioctls, so I really don't believe the 0x80
offset is needed.

>>
>>> +  _IOC_NR(cmd) >= 0x80 + ARRAY_SIZE(ioctl_info) ||
>>> +  ioctl_info[_IOC_NR(cmd) - 0x80].cmd != cmd)
>>> + return -ENOIOCTLCMD;
>>> +
>>> + info = _info[_IOC_NR(cmd) - 0x80];
>>> +
>>> + return info->fn(req);
>>> +}



>>> diff --git a/include/uapi/linux/media-request.h 
>>> b/include/uapi/linux/media-request.h
>>> new file mode 100644
>>> index ..5d30f731a442
>>> --- /dev/null
>>> +++ b/include/uapi/linux/media-request.h
>>> @@ -0,0 +1,37 @@
>>> +/*
>>> + * Media requests UAPI
>>> + *
>>> + * Copyright (C) 2018, 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 __LINUX_MEDIA_REQUEST_H
>>> +#define __LINUX_MEDIA_REQUEST_H
>>> +
>>> +#ifndef __KERNEL__
>>> +#include 
>>> +#endif
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +/* Only check that requests can be used, do not allocate */
>>> +#define MEDIA_REQUEST_FLAG_TEST  0x0001
>>> +
>>> +struct media_request_new {
>>> + __u32 flags;
>>> + __s32 fd;
>>> +} __attribute__ ((packed));
>>> +
>>> +#define MEDIA_REQUEST_IOC_SUBMIT   _IO('|',  128)
>>> +#define MEDIA_REQUEST_IOC_REINIT   _IO('|',  129)
>>> +
>>> +#endif
>>>
>>
>> I need to think a bit more on this internal API, so I might come back
>> to this patch for more comments.
> 
> I think I should probably elaborate on why I think it is advantageous
> to have these ioctls handled here.

Sorry for the confusion, I was not actually referring to these ioctls.
In fact, I really like them. It was more a general comment about the
request API core.

I should have been more clear.

Regards,

Hans

> 
> One of the reasons if that it does not force user-space to keep track
> of who issued the request to operate on it. Semantically, the only
> device a request could be submitted to is the device that produced it
> anyway, so since that argument is constant we may as well get rid of
> it (and we also don't need to pass the request FD as argument
> anymore).
> 
> It also gives us more freedom when designing new request-related
> ioctls: before, all request-related operations were multiplexed under
> a single MEDIA_IOC_REQUEST_CMD ioctl, which cmd field indicated the
> actual operation to perform. With this design, all the arguments must
> fit within the media_request_cmd structure, which may cause confusion
> as it will have to be variable-sized. I am thinking in particular
> about a future atomic-like API to set topology, controls and buffers
> related to a request all at the same time. Having it as a request
> ioctl seems perfectly fitting to me.
> 

Re: [RFCv4 01/21] media: add request API core and UAPI

[Alexandre Courbot]

Hi Hans,

On Tue, Feb 20, 2018 at 7:36 PM, Hans Verkuil  wrote:
> On 02/20/18 05:44, 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/Kconfig  |   3 +
>>  drivers/media/Makefile |   6 +
>>  drivers/media/media-request.c  | 341 
>>  include/media/media-request.h  | 349 +
>>  include/uapi/linux/media-request.h |  37 +++
>>  5 files changed, 736 insertions(+)
>>  create mode 100644 drivers/media/media-request.c
>>  create mode 100644 include/media/media-request.h
>>  create mode 100644 include/uapi/linux/media-request.h
>>
>> diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
>> index 145e12bfb819..db30fc9547d2 100644
>> --- a/drivers/media/Kconfig
>> +++ b/drivers/media/Kconfig
>> @@ -130,6 +130,9 @@ config VIDEO_V4L2_SUBDEV_API
>>
>> This API is mostly used by camera interfaces in embedded platforms.
>>
>> +config MEDIA_REQUEST_API
>> + tristate
>> +
>>  source "drivers/media/v4l2-core/Kconfig"
>>
>>  #
>> diff --git a/drivers/media/Makefile b/drivers/media/Makefile
>> index 594b462ddf0e..03c0a39ad344 100644
>> --- a/drivers/media/Makefile
>> +++ b/drivers/media/Makefile
>> @@ -5,6 +5,12 @@
>>
>>  media-objs   := media-device.o media-devnode.o media-entity.o
>>
>> +#
>> +# Request API support comes as its own module since it can be used by
>> +# both media and video devices
>> +#
>> +obj-$(CONFIG_MEDIA_REQUEST_API) += media-request.o
>> +
>>  #
>>  # I2C drivers should come before other drivers, otherwise they'll fail
>>  # when compiled as builtin drivers
>> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
>> new file mode 100644
>> index ..b88362028561
>> --- /dev/null
>> +++ b/drivers/media/media-request.c
>> @@ -0,0 +1,341 @@
>> +/*
>> + * Request base implementation
>> + *
>> + * Copyright (C) 2018, 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 
>> +#include 
>> +
>> +#include 
>> +
>> +const struct file_operations request_fops;
>> +
>> +static const char * const media_request_states[] __maybe_unused = {
>
> Why 'maybe_unused'?
>
>> + "IDLE",
>> + "SUBMITTED",
>> + "COMPLETED",
>> + "INVALID",
>
> I don't like to yell. I prefer "Idle", "Submitted", etc.

Sure.

>
>> +};
>> +
>> +struct media_request *media_request_get(struct media_request *req)
>> +{
>> + get_file(req->file);
>> + return req;
>> +}
>> +EXPORT_SYMBOL_GPL(media_request_get);
>> +
>> +struct media_request *media_request_get_from_fd(int fd)
>> +{
>> + struct file *f;
>> +
>> + f = fget(fd);
>> + if (!f)
>> + return NULL;
>> +
>> + /* Not a request FD? */
>> + if (f->f_op != _fops) {
>> + fput(f);
>> + return NULL;
>> + }
>> +
>> + return f->private_data;
>> +}
>> +EXPORT_SYMBOL_GPL(media_request_get_from_fd);
>> +
>> +void media_request_put(struct media_request *req)
>> +{
>> + if (WARN_ON(req == NULL))
>> + return;
>> +
>> + fput(req->file);
>> +}
>> +EXPORT_SYMBOL_GPL(media_request_put);
>> +
>> +struct media_request_entity_data *
>> +media_request_get_entity_data(struct media_request *req,
>> +   struct media_request_entity *entity)
>> +{
>> + struct media_request_entity_data *data;
>> +
>> + /* First check that this entity is valid for this request at all */
>> + if (!req->mgr->ops->entity_valid(req, entity))
>> + return ERR_PTR(-EINVAL);
>> +
>> + mutex_lock(>lock);
>> +
>> + /* Lookup whether we already have entity data */
>> + list_for_each_entry(data, >data, list) {
>> + if (data->entity == entity)
>> + goto out;
>> + }
>> +
>> + /* No entity data found, let's create it */
>> + data = entity->ops->data_alloc(req, entity);
>> + if (IS_ERR(data))
>> + goto out;
>> +
>> + data->entity = entity;
>> + list_add_tail(>list, >data);
>> +
>> +out:
>> + mutex_unlock(>lock);
>> +
>> 

Re: [RFCv4 10/21] videodev2.h: Add request_fd field to v4l2_buffer

[Alexandre Courbot]

On Wed, Feb 21, 2018 at 12:20 AM, Hans Verkuil  wrote:
> On 02/20/18 05:44, Alexandre Courbot wrote:
>> From: Hans Verkuil 
>>
>> When queuing buffers allow for passing the request that should
>> be associated with this buffer.
>>
>> Signed-off-by: Hans Verkuil 
>> [acour...@chromium.org: make request ID 32-bit]
>> Signed-off-by: Alexandre Courbot 
>> ---
>>  drivers/media/common/videobuf2/videobuf2-v4l2.c | 2 +-
>>  drivers/media/usb/cpia2/cpia2_v4l.c | 2 +-
>>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c   | 9 ++---
>>  drivers/media/v4l2-core/v4l2-ioctl.c| 4 ++--
>>  include/uapi/linux/videodev2.h  | 3 ++-
>>  5 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c 
>> b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 886a2d8d5c6c..6d4d184aa68e 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -203,7 +203,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, 
>> void *pb)
>>   b->timestamp = ns_to_timeval(vb->timestamp);
>>   b->timecode = vbuf->timecode;
>>   b->sequence = vbuf->sequence;
>> - b->reserved2 = 0;
>> + b->request_fd = 0;
>>   b->reserved = 0;
>>
>>   if (q->is_multiplanar) {
>> diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c 
>> b/drivers/media/usb/cpia2/cpia2_v4l.c
>> index 99f106b13280..af42ce3ceb48 100644
>> --- a/drivers/media/usb/cpia2/cpia2_v4l.c
>> +++ b/drivers/media/usb/cpia2/cpia2_v4l.c
>> @@ -948,7 +948,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, 
>> struct v4l2_buffer *buf)
>>   buf->sequence = cam->buffers[buf->index].seq;
>>   buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
>>   buf->length = cam->frame_size;
>> - buf->reserved2 = 0;
>> + buf->request_fd = 0;
>>   buf->reserved = 0;
>>   memset(>timecode, 0, sizeof(buf->timecode));
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c 
>> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> index 5198c9eeb348..32bf47489a2e 100644
>> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> @@ -386,7 +386,7 @@ struct v4l2_buffer32 {
>>   __s32   fd;
>>   } m;
>>   __u32   length;
>> - __u32   reserved2;
>> + __s32   request_fd;
>>   __u32   reserved;
>>  };
>>
>> @@ -486,6 +486,7 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user 
>> *kp,
>>  {
>>   u32 type;
>>   u32 length;
>> + s32 request_fd;
>>   enum v4l2_memory memory;
>>   struct v4l2_plane32 __user *uplane32;
>>   struct v4l2_plane __user *uplane;
>> @@ -500,7 +501,9 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user 
>> *kp,
>>   get_user(memory, >memory) ||
>>   put_user(memory, >memory) ||
>>   get_user(length, >length) ||
>> - put_user(length, >length))
>> + put_user(length, >length) ||
>> + get_user(request_fd, >request_fd) ||
>> + put_user(request_fd, >request_fd))
>>   return -EFAULT;
>>
>>   if (V4L2_TYPE_IS_OUTPUT(type))
>> @@ -604,7 +607,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user 
>> *kp,
>>   assign_in_user(>timestamp.tv_usec, >timestamp.tv_usec) ||
>>   copy_in_user(>timecode, >timecode, sizeof(kp->timecode)) ||
>>   assign_in_user(>sequence, >sequence) ||
>> - assign_in_user(>reserved2, >reserved2) ||
>> + assign_in_user(>request_fd, >request_fd) ||
>>   assign_in_user(>reserved, >reserved) ||
>>   get_user(length, >length) ||
>>   put_user(length, >length))
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
>> b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 260288ca4f55..7bfeaf233d5a 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -437,13 +437,13 @@ static void v4l_print_buffer(const void *arg, bool 
>> write_only)
>>   const struct v4l2_plane *plane;
>>   int i;
>>
>> - pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, flags=0x%08x, 
>> field=%s, sequence=%d, memory=%s",
>> + pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, request_fd=%u, 
>> flags=0x%08x, field=%s, sequence=%d, memory=%s",
>>   p->timestamp.tv_sec / 3600,
>>   (int)(p->timestamp.tv_sec / 60) % 60,
>>   (int)(p->timestamp.tv_sec % 60),
>>   (long)p->timestamp.tv_usec,
>>   p->index,
>> - prt_names(p->type, v4l2_type_names),
>> + prt_names(p->type, v4l2_type_names), p->request_fd,
>>   p->flags, prt_names(p->field, v4l2_field_names),

Re: [RFCv4 13/21] media: videobuf2-v4l2: support for requests

[Alexandre Courbot]

On Wed, Feb 21, 2018 at 1:18 AM, Hans Verkuil  wrote:
> On 02/20/2018 05:44 AM, Alexandre Courbot wrote:
>> Add a new vb2_qbuf_request() (a request-aware version of vb2_qbuf())
>> that request-aware drivers can call to queue a buffer into a request
>> instead of directly into the vb2 queue if relevent.
>>
>> This function expects that drivers invoking it are using instances of
>> v4l2_request_entity and v4l2_request_entity_data to describe their
>> entity and entity data respectively.
>>
>> Also add the vb2_request_submit() helper function which drivers can
>> invoke in order to queue all the buffers previously queued into a
>> request into the regular vb2 queue.
>>
>> Signed-off-by: Alexandre Courbot 
>> ---
>>  .../media/common/videobuf2/videobuf2-v4l2.c   | 129 +-
>>  include/media/videobuf2-v4l2.h|  59 
>>  2 files changed, 187 insertions(+), 1 deletion(-)
>>
>
> 
>
>> @@ -776,10 +899,14 @@ EXPORT_SYMBOL_GPL(vb2_ioctl_querybuf);
>>  int vb2_ioctl_qbuf(struct file *file, void *priv, struct v4l2_buffer *p)
>>  {
>>   struct video_device *vdev = video_devdata(file);
>> + struct v4l2_fh *fh = NULL;
>> +
>> + if (test_bit(V4L2_FL_USES_V4L2_FH, >flags))
>> + fh = file->private_data;
>
> No need for this. All drivers using vb2 will also use v4l2_fh.

Fixed.

>
>>
>>   if (vb2_queue_is_busy(vdev, file))
>>   return -EBUSY;
>> - return vb2_qbuf(vdev->queue, p);
>> + return vb2_qbuf_request(vdev->queue, p, fh ? fh->entity : NULL);
>>  }
>>  EXPORT_SYMBOL_GPL(vb2_ioctl_qbuf);
>>
>> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
>> index 3d5e2d739f05..d4dfa266a0da 100644
>> --- a/include/media/videobuf2-v4l2.h
>> +++ b/include/media/videobuf2-v4l2.h
>> @@ -23,6 +23,12 @@
>>  #error VB2_MAX_PLANES != VIDEO_MAX_PLANES
>>  #endif
>>
>> +struct media_entity;
>> +struct v4l2_fh;
>> +struct media_request;
>> +struct media_request_entity;
>> +struct v4l2_request_entity_data;
>> +
>>  /**
>>   * struct vb2_v4l2_buffer - video buffer information for v4l2.
>>   *
>> @@ -116,6 +122,59 @@ int vb2_prepare_buf(struct vb2_queue *q, struct 
>> v4l2_buffer *b);
>>   */
>>  int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b);
>>
>> +#if IS_ENABLED(CONFIG_MEDIA_REQUEST_API)
>> +
>> +/**
>> + * vb2_qbuf_request() - Queue a buffer, with request support
>> + * @q:   pointer to  vb2_queue with videobuf2 queue.
>> + * @b:   buffer structure passed from userspace to
>> + *   _ioctl_ops->vidioc_qbuf handler in driver
>> + * @entity:  request entity to queue for if requests are used.
>> + *
>> + * Should be called from _ioctl_ops->vidioc_qbuf handler of a driver.
>> + *
>> + * If requests are not in use, calling this is equivalent to calling 
>> vb2_qbuf().
>> + *
>> + * If the request_fd member of b is set, then the buffer represented by b is
>> + * queued in the request instead of the vb2 queue. The buffer will be passed
>> + * to the vb2 queue when the request is submitted.
>
> I would definitely also prepare the buffer at this time. That way you'll see 
> any
> errors relating to the prepare early on.

I was wondering about that, so glad to have your opinion on this. Will
make sure buffers are prepared before queuing them to a request.

>
>> + *
>> + * The return values from this function are intended to be directly returned
>> + * from _ioctl_ops->vidioc_qbuf handler in driver.
>> + */
>> +int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer *b,
>> +  struct media_request_entity *entity);
>> +
>> +/**
>> + * vb2_request_submit() - Queue all the buffers in a v4l2 request.
>> + * @data:request entity data to queue buffers of
>> + *
>> + * This function should be called from the media_request_entity_ops::submit
>> + * hook for instances of media_request_v4l2_entity_data. It will immediately
>> + * queue all the request-bound buffers to their respective vb2 queues.
>> + *
>> + * Errors from vb2_core_qbuf() are returned if something happened. Also, 
>> since
>> + * v4l2 request entities require at least one buffer for the request to 
>> trigger,
>> + * this function will return -EINVAL if no buffer have been bound at all for
>> + * this entity.
>> + */
>> +int vb2_request_submit(struct v4l2_request_entity_data *data);
>> +
>> +#else /* CONFIG_MEDIA_REQUEST_API */
>> +
>> +static inline int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer 
>> *b,
>> +struct media_request_entity *entity)
>> +{
>> + return vb2_qbuf(q, b);
>> +}
>> +
>> +static inline int vb2_request_submit(struct v4l2_request_entity_data *data)
>> +{
>> + return -ENOTSUPP;
>> +}
>> +
>> +#endif /* CONFIG_MEDIA_REQUEST_API */
>> +
>>  /**
>>   * vb2_expbuf() - Export a buffer as a file descriptor
>>   * @q:   pointer to  vb2_queue with videobuf2 queue.
>>
>
> Regards,
>
> Hans

Re: [RFCv4 11/21] media: v4l2_fh: add request entity field

[Alexandre Courbot]

On Wed, Feb 21, 2018 at 12:24 AM, Hans Verkuil  wrote:
> On 02/20/18 05:44, Alexandre Courbot wrote:
>> Allow drivers to assign a request entity to v4l2_fh. This will be useful
>> for request-aware ioctls to find out which request entity to use.
>>
>> Signed-off-by: Alexandre Courbot 
>> ---
>>  include/media/v4l2-fh.h | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
>> index ea73fef8bdc0..f54cb319dd64 100644
>> --- a/include/media/v4l2-fh.h
>> +++ b/include/media/v4l2-fh.h
>> @@ -28,6 +28,7 @@
>>
>>  struct video_device;
>>  struct v4l2_ctrl_handler;
>> +struct media_request_entity;
>>
>>  /**
>>   * struct v4l2_fh - Describes a V4L2 file handler
>> @@ -43,6 +44,7 @@ struct v4l2_ctrl_handler;
>>   * @navailable: number of available events at @available list
>>   * @sequence: event sequence number
>>   * @m2m_ctx: pointer to  v4l2_m2m_ctx
>> + * @entity: the request entity this fh operates on behalf of
>>   */
>>  struct v4l2_fh {
>>   struct list_headlist;
>> @@ -60,6 +62,7 @@ struct v4l2_fh {
>>  #if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
>>   struct v4l2_m2m_ctx *m2m_ctx;
>>  #endif
>> + struct media_request_entity *entity;
>
> The name 'media_request_entity' is very confusing.
>
> In the media controller API terminology an entity represents a piece
> of hardware with inputs and outputs (very rough description), but a
> request is not an entity. It may be associated with an entity, though.
>
> So calling this field 'entity' is also very misleading.

Note that this field is not a request though, it is a pointer to a
piece of hardware referenced by a request, which is closer to the MC
terminology. Or do you mean this should just be renamed
"request_entity"?

If we go all the way, the media_ prefix is also misleading - it
implies a dependency to the media controller framework, while there is
none (in this patchset at least).

However I thought that 'request' alone (instead of media_request) may
name-conflict with something else, and since 'media' is also the
umbrella term for anything under drivers/media it sounds fitting on
the other hand. Suggestions are welcome though.

>
> As with previous patches, I'll have to think about this and try and
> come up with better, less confusing names.

I will gladly take suggestions, have been trying to come with a better
name to reply to your comment above but could not find any. :)

Re: [RFCv4 16/21] v4l2: video_device: support for creating requests

[Alexandre Courbot]

On Wed, Feb 21, 2018 at 1:35 AM, Hans Verkuil  wrote:
> On 02/20/2018 05:44 AM, Alexandre Courbot wrote:
>> Add a new VIDIOC_NEW_REQUEST ioctl, which allows to instanciate requests
>> on devices that support the request API. Requests created that way can
>> only control the device they originate from, making them suitable for
>> simple devices, but not complex pipelines.
>>
>> Signed-off-by: Alexandre Courbot 
>> ---
>>  Documentation/ioctl/ioctl-number.txt |  1 +
>>  drivers/media/v4l2-core/v4l2-dev.c   |  2 ++
>>  drivers/media/v4l2-core/v4l2-ioctl.c | 25 +
>>  include/media/v4l2-dev.h |  2 ++
>>  include/uapi/linux/videodev2.h   |  3 +++
>>  5 files changed, 33 insertions(+)
>>
>> diff --git a/Documentation/ioctl/ioctl-number.txt 
>> b/Documentation/ioctl/ioctl-number.txt
>> index 6501389d55b9..afdc9ed255b0 100644
>> --- a/Documentation/ioctl/ioctl-number.txt
>> +++ b/Documentation/ioctl/ioctl-number.txt
>> @@ -286,6 +286,7 @@ Code  Seq#(hex)   Include FileComments
>>   
>>  'z'  10-4F   drivers/s390/crypto/zcrypt_api.hconflict!
>>  '|'  00-7F   linux/media.h
>> +'|'  80-9F   linux/media-request.h
>>  0x80 00-1F   linux/fb.h
>>  0x89 00-06   arch/x86/include/asm/sockios.h
>>  0x89 0B-DF   linux/sockios.h
>
> This  doesn't belong in this patch.

Do you mean we need a separate patch for this?

>
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
>> b/drivers/media/v4l2-core/v4l2-dev.c
>> index 0301fe426a43..062ebee5bffc 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -559,6 +559,8 @@ static void determine_valid_ioctls(struct video_device 
>> *vdev)
>>   set_bit(_IOC_NR(VIDIOC_TRY_EXT_CTRLS), valid_ioctls);
>>   if (vdev->ctrl_handler || ops->vidioc_querymenu)
>>   set_bit(_IOC_NR(VIDIOC_QUERYMENU), valid_ioctls);
>> + if (vdev->req_mgr)
>> + set_bit(_IOC_NR(VIDIOC_NEW_REQUEST), valid_ioctls);
>>   SET_VALID_IOCTL(ops, VIDIOC_G_FREQUENCY, vidioc_g_frequency);
>>   SET_VALID_IOCTL(ops, VIDIOC_S_FREQUENCY, vidioc_s_frequency);
>>   SET_VALID_IOCTL(ops, VIDIOC_LOG_STATUS, vidioc_log_status);
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
>> b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index ab4968ea443f..a45fe078f8ae 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -21,6 +21,7 @@
>>
>>  #include 
>>
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -842,6 +843,13 @@ static void v4l_print_freq_band(const void *arg, bool 
>> write_only)
>>   p->rangehigh, p->modulation);
>>  }
>>
>> +static void vidioc_print_new_request(const void *arg, bool write_only)
>> +{
>> + const struct media_request_new *new = arg;
>> +
>> + pr_cont("fd=0x%x\n", new->fd);
>
> I'd use %d since fds are typically shown as signed integers.

Right.

>
>> +}
>> +
>>  static void v4l_print_edid(const void *arg, bool write_only)
>>  {
>>   const struct v4l2_edid *p = arg;
>> @@ -2486,6 +2494,22 @@ static int v4l_enum_freq_bands(const struct 
>> v4l2_ioctl_ops *ops,
>>   return -ENOTTY;
>>  }
>>
>> +static int vidioc_new_request(const struct v4l2_ioctl_ops *ops,
>> +   struct file *file, void *fh, void *arg)
>> +{
>> +#if IS_ENABLED(CONFIG_MEDIA_REQUEST_API)
>> + struct media_request_new *new = arg;
>> + struct video_device *vfd = video_devdata(file);
>> +
>> + if (!vfd->req_mgr)
>> + return -ENOTTY;
>> +
>> + return media_request_ioctl_new(vfd->req_mgr, new);
>> +#else
>> + return -ENOTTY;
>> +#endif
>> +}
>
> You don't need the #ifdef's here. media_request_ioctl_new() will be stubbed if
> CONFIG_MEDIA_REQUEST_API isn't set.

Correct.

>
>> +
>>  struct v4l2_ioctl_info {
>>   unsigned int ioctl;
>>   u32 flags;
>> @@ -2617,6 +2641,7 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>>   IOCTL_INFO_FNC(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, 
>> v4l_print_freq_band, 0),
>>   IOCTL_INFO_FNC(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, 
>> v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
>>   IOCTL_INFO_FNC(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, 
>> v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, 
>> id)),
>> + IOCTL_INFO_FNC(VIDIOC_NEW_REQUEST, vidioc_new_request, 
>> vidioc_print_new_request, 0),
>>  };
>>  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>>
>> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
>> index 53f32022fabe..e6c4e10889bc 100644
>> --- a/include/media/v4l2-dev.h
>> +++ b/include/media/v4l2-dev.h
>> @@ -209,6 +209,7 @@ struct v4l2_file_operations {
>>   * @entity:  media_entity
>>   * @intf_devnode: pointer to  media_intf_devnode
>>   * @pipe:  media_pipeline
>> + * @req_mgr: request manager to use if 

Re: [RFCv4 09/21] v4l2: add request API support

[Alexandre Courbot]

On Tue, Feb 20, 2018 at 10:25 PM, Hans Verkuil  wrote:
> On 02/20/18 05:44, Alexandre Courbot wrote:
>> Add a v4l2 request entity data structure that takes care of storing the
>> request-related state of a V4L2 device ; in this case, its controls.
>>
>> Signed-off-by: Alexandre Courbot 
>> ---
>>  drivers/media/v4l2-core/Makefile   |   1 +
>>  drivers/media/v4l2-core/v4l2-request.c | 178 +
>>  include/media/v4l2-request.h   | 159 ++
>>  3 files changed, 338 insertions(+)
>>  create mode 100644 drivers/media/v4l2-core/v4l2-request.c
>>  create mode 100644 include/media/v4l2-request.h
>>
>> diff --git a/drivers/media/v4l2-core/Makefile 
>> b/drivers/media/v4l2-core/Makefile
>> index 80de2cb9c476..13d0477535bd 100644
>> --- a/drivers/media/v4l2-core/Makefile
>> +++ b/drivers/media/v4l2-core/Makefile
>> @@ -16,6 +16,7 @@ ifeq ($(CONFIG_TRACEPOINTS),y)
>>videodev-objs += vb2-trace.o v4l2-trace.o
>>  endif
>>  videodev-$(CONFIG_MEDIA_CONTROLLER) += v4l2-mc.o
>> +videodev-$(CONFIG_MEDIA_REQUEST_API) += v4l2-request.o
>>
>>  obj-$(CONFIG_VIDEO_V4L2) += videodev.o
>>  obj-$(CONFIG_VIDEO_V4L2) += v4l2-common.o
>> diff --git a/drivers/media/v4l2-core/v4l2-request.c 
>> b/drivers/media/v4l2-core/v4l2-request.c
>> new file mode 100644
>> index ..e8ad10e2f525
>> --- /dev/null
>> +++ b/drivers/media/v4l2-core/v4l2-request.c
>> @@ -0,0 +1,178 @@
>> +/*
>> + * Media requests support for V4L2
>> + *
>> + * Copyright (C) 2018, 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 
>> +
>> +void v4l2_request_entity_init(struct v4l2_request_entity *entity,
>> +   const struct media_request_entity_ops *ops,
>> +   struct video_device *vdev)
>> +{
>> + media_request_entity_init(>base, 
>> MEDIA_REQUEST_ENTITY_TYPE_V4L2, ops);
>> + entity->vdev = vdev;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_request_entity_init);
>> +
>> +struct media_request_entity_data *
>> +v4l2_request_entity_data_alloc(struct media_request *req,
>> +struct v4l2_ctrl_handler *hdl)
>> +{
>> + struct v4l2_request_entity_data *data;
>> + int ret;
>> +
>> + data = kzalloc(sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + ret = v4l2_ctrl_request_init(>ctrls);
>> + if (ret) {
>> + kfree(data);
>> + return ERR_PTR(ret);
>> + }
>> + ret = v4l2_ctrl_request_clone(>ctrls, hdl, NULL);
>> + if (ret) {
>> + kfree(data);
>> + return ERR_PTR(ret);
>> + }
>> +
>> + INIT_LIST_HEAD(>queued_buffers);
>> +
>> + return >base;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_request_entity_data_alloc);
>> +
>> +void v4l2_request_entity_data_free(struct media_request_entity_data *_data)
>> +{
>> + struct v4l2_request_entity_data *data;
>> + struct v4l2_vb2_request_buffer *qb, *n;
>> +
>> + data = to_v4l2_entity_data(_data);
>> +
>> + list_for_each_entry_safe(qb, n, >queued_buffers, node) {
>> + struct vb2_buffer *buf;
>> + dev_warn(_data->request->mgr->dev,
>> +  "entity data freed while buffer still queued!\n");
>> +
>> + /* give buffer back to user-space */
>> + buf = qb->queue->bufs[qb->v4l2_buf.index];
>> + buf->state = qb->pre_req_state;
>> + buf->request = NULL;
>> +
>> + kfree(qb);
>> + }
>> +
>> + v4l2_ctrl_handler_free(>ctrls);
>> + kfree(data);
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_request_entity_data_free);
>> +
>> +
>> +
>> +
>> +
>> +static struct media_request *v4l2_request_alloc(struct media_request_mgr 
>> *mgr)
>> +{
>> + struct media_request *req;
>> +
>> + req = kzalloc(sizeof(*req), GFP_KERNEL);
>> + if (!req)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + req->mgr = mgr;
>> + req->state = MEDIA_REQUEST_STATE_IDLE;
>> + INIT_LIST_HEAD(>data);
>> + init_waitqueue_head(>complete_wait);
>> + mutex_init(>lock);
>> +
>> + mutex_lock(>mutex);
>> + list_add_tail(>list, >requests);
>> + mutex_unlock(>mutex);
>> +
>> + return req;
>> +}
>> +
>> +static void v4l2_request_free(struct media_request *req)
>> +{
>> + struct media_request_mgr *mgr = req->mgr;
>> + struct media_request_entity_data *data, *next;
>> +
>> + mutex_lock(>mutex);
>> +  

cron job: media_tree daily build: ABI WARNING

[Hans Verkuil]

This message is generated daily by a cron job that builds media_tree for
the kernels and architectures in the list below.

Results of the daily build of media_tree:

date:   Wed Feb 21 05:00:11 CET 2018
media-tree git hash:29422737017b866d4a51014cc7522fa3a99e8852
media_build git hash:   d144cfe4b3c37ece55ae27778c99765d4943c4fa
v4l-utils git hash: 0e2c3e52d0e0310ae31dd0704f31856a5746913d
gcc version:i686-linux-gcc (GCC) 7.3.0
sparse version: v0.5.0-3994-g45eb2282
smatch version: v0.5.0-3994-g45eb2282
host hardware:  x86_64
host os:4.14.0-3-amd64

linux-git-arm-at91: OK
linux-git-arm-davinci: OK
linux-git-arm-multi: OK
linux-git-arm-pxa: OK
linux-git-arm-stm32: OK
linux-git-arm64: OK
linux-git-blackfin-bf561: OK
linux-git-i686: OK
linux-git-m32r: OK
linux-git-mips: OK
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: OK
linux-2.6.36.4-i686: WARNINGS
linux-2.6.36.4-x86_64: WARNINGS
linux-2.6.37.6-i686: WARNINGS
linux-2.6.37.6-x86_64: WARNINGS
linux-2.6.38.8-i686: WARNINGS
linux-2.6.38.8-x86_64: WARNINGS
linux-2.6.39.4-i686: WARNINGS
linux-2.6.39.4-x86_64: WARNINGS
linux-3.0.60-i686: WARNINGS
linux-3.0.60-x86_64: WARNINGS
linux-3.1.10-i686: WARNINGS
linux-3.1.10-x86_64: WARNINGS
linux-3.2.98-i686: WARNINGS
linux-3.2.98-x86_64: WARNINGS
linux-3.3.8-i686: WARNINGS
linux-3.3.8-x86_64: WARNINGS
linux-3.4.27-i686: WARNINGS
linux-3.4.27-x86_64: WARNINGS
linux-3.5.7-i686: WARNINGS
linux-3.5.7-x86_64: WARNINGS
linux-3.6.11-i686: WARNINGS
linux-3.6.11-x86_64: WARNINGS
linux-3.7.4-i686: WARNINGS
linux-3.7.4-x86_64: WARNINGS
linux-3.8-i686: WARNINGS
linux-3.8-x86_64: WARNINGS
linux-3.9.2-i686: WARNINGS
linux-3.9.2-x86_64: WARNINGS
linux-3.10.1-i686: WARNINGS
linux-3.10.1-x86_64: WARNINGS
linux-3.11.1-i686: WARNINGS
linux-3.11.1-x86_64: WARNINGS
linux-3.12.67-i686: WARNINGS
linux-3.12.67-x86_64: WARNINGS
linux-3.13.11-i686: WARNINGS
linux-3.13.11-x86_64: WARNINGS
linux-3.14.9-i686: WARNINGS
linux-3.14.9-x86_64: WARNINGS
linux-3.15.2-i686: WARNINGS
linux-3.15.2-x86_64: WARNINGS
linux-3.16.53-i686: WARNINGS
linux-3.16.53-x86_64: WARNINGS
linux-3.17.8-i686: WARNINGS
linux-3.17.8-x86_64: WARNINGS
linux-3.18.93-i686: WARNINGS
linux-3.18.93-x86_64: WARNINGS
linux-3.19-i686: WARNINGS
linux-3.19-x86_64: WARNINGS
linux-4.0.9-i686: WARNINGS
linux-4.0.9-x86_64: WARNINGS
linux-4.1.49-i686: WARNINGS
linux-4.1.49-x86_64: WARNINGS
linux-4.2.8-i686: WARNINGS
linux-4.2.8-x86_64: WARNINGS
linux-4.3.6-i686: WARNINGS
linux-4.3.6-x86_64: WARNINGS
linux-4.4.115-i686: OK
linux-4.4.115-x86_64: OK
linux-4.5.7-i686: WARNINGS
linux-4.5.7-x86_64: WARNINGS
linux-4.6.7-i686: OK
linux-4.6.7-x86_64: WARNINGS
linux-4.7.5-i686: OK
linux-4.7.5-x86_64: WARNINGS
linux-4.8-i686: OK
linux-4.8-x86_64: WARNINGS
linux-4.9.80-i686: OK
linux-4.9.80-x86_64: OK
linux-4.10.14-i686: OK
linux-4.10.14-x86_64: WARNINGS
linux-4.11-i686: OK
linux-4.11-x86_64: WARNINGS
linux-4.12.1-i686: OK
linux-4.12.1-x86_64: WARNINGS
linux-4.13-i686: OK
linux-4.13-x86_64: OK
linux-4.14.17-i686: OK
linux-4.14.17-x86_64: OK
linux-4.15.2-i686: OK
linux-4.15.2-x86_64: OK
linux-4.16-rc1-i686: OK
linux-4.16-rc1-x86_64: OK
apps: WARNINGS
spec-git: OK
ABI WARNING: change for blackfin-bf561
sparse: WARNINGS
smatch: OK

Detailed results are available here:

http://www.xs4all.nl/~hverkuil/logs/Wednesday.log

Full logs are available here:

http://www.xs4all.nl/~hverkuil/logs/Wednesday.tar.bz2

The Media Infrastructure API from this daily build is here:

http://www.xs4all.nl/~hverkuil/spec/index.html

RE: [PATCH v4 00/12] Intel IPU3 ImgU patchset

[Mani, Rajmohan]

Hi Mauro,

> > > Subject: Re: [PATCH v4 00/12] Intel IPU3 ImgU patchset
> > >
> > > Hi,
> > >
> > > Em Fri, 17 Nov 2017 02:58:56 +
> > > "Mani, Rajmohan"  escreveu:
> > >
> > > > Here is an update on the IPU3 documentation that we are currently
> > > > working
> > > on.
> > > >
> > > > Image processing in IPU3 relies on the following.
> > > >
> > > > 1) HW configuration to enable ISP and
> > > > 2) setting customer specific 3A Tuning / Algorithm Parameters to
> > > > achieve
> > > desired image quality.
> > > >
> > > > We intend to provide documentation on ImgU driver programming
> > > > interface
> > > to help users of this driver to configure and enable ISP HW to meet
> > > their needs.  This documentation will include details on complete
> > > V4L2 Kernel driver interface and IO-Control parameters, except for
> > > the ISP internal algorithm and its parameters (which is Intel proprietary 
> > > IP).
> > >
> > > Sakari asked me to take a look on this thread, specifically on this
> > > email. I took a look on the other e-mails from this thread that are
> > > discussing about this IP block.
> > >
> > > I understand that Intel wants to keep their internal 3A algorithm
> > > protected, just like other vendors protect their own algos. It was
> > > never a requirement to open whatever algorithm are used inside a
> > > hardware (or firmware). The only requirement is that firmwares
> > > should be licensed with redistribution permission, ideally merged at
> > > linux-firmware
> > git tree.
> > >
> > > Yet, what I don't understand is why Intel also wants to also protect
> > > the interface for such 3A hardware/firmware algorithm. The
> > > parameters that are passed from an userspace application to Intel
> > > ISP logic doesn't contain the algorithm itself. What's the issue of
> > > documenting the meaning of each parameter?
> > >
> >
> > Thanks for looking into this.
> >
> > To achieve improved image quality using IPU3, 3A (Auto White balance,
> > Auto Focus and Auto Exposure) Tuning parameters specific to a given
> > camera sensor module, are converted to Intel ISP algorithm parameters
> > in user space camera HAL using AIC (Automatic ISP Configuration) library.
> >
> > As a unique design of Intel ISP, it exposes very detailed algorithm
> > parameters (~ 1 parameters) to configure ISP's image processing
> > algorithm per each image fame in runtime. Typical Camera SW developers
> > (including those at
> > Intel) are not expected to fully understand and directly set these
> > parameters to configure the ISP algorithm blocks. Due to the above, a
> > user space AIC library (in binary form) is provided to generate ISP
> > Algorithm specific parameters, for a given set of 3A tuning
> > parameters. It significantly reduces the efforts of SW development in ISP HW
> configuration.
> >
> > On the other hand, the ISP algorithm details could be deduced readily
> > through these detailed parameters by other ISP experts outside Intel.
> > This is the reason that we want to keep these parameter definitions as Intel
> proprietary IP.
> >
> > We are fully aware of your concerns on how to enable open source
> > developers to use Intel ISP through up-streamed Kernel Driver.
> > Internally, we are working on the license for this AIC library release
> > now (as Hans said NDA license is not acceptable). We believe this will
> > be more efficient way to help open source developers.
> >
> > This AIC library release would be a binary-only release. This AIC
> > library does not use any kernel uAPIs directly. The user space Camera
> > HAL that uses kernel uAPIs is available at
> > https://chromium.googlesource.com/chromiumos/platform/arc-
> > camera/+/master
> >

The AIC library (in binary form) is available here.
https://storage.googleapis.com/chromeos-localmirror/distfiles/intel-3a-libs-bin-2017.09.27.tbz2

Licensing information can be found in ./LICENSE.intel_3a_library file after 
unzipping the tar file.

> 
> Just pinging to know your thoughts on this.
> 
> Thanks
> Raj

[PATCH] media: dw9714: Update to SPDX license identifier

[Rajmohan Mani]

Remove the GPL v2 license boilerplate and update with the SPDX license
identifier.

Signed-off-by: Rajmohan Mani 
---
 drivers/media/i2c/dw9714.c | 14 ++
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c
index 8dbbf0f..91fae01 100644
--- a/drivers/media/i2c/dw9714.c
+++ b/drivers/media/i2c/dw9714.c
@@ -1,15 +1,5 @@
-/*
- * Copyright (c) 2015--2017 Intel Corporation.
- *
- * 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.
- */
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2015--2017 Intel Corporation.
 
 #include 
 #include 
-- 
1.9.1

Re: [PATCHv2] media: add tuner standby op, use where needed

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tuesday, 20 February 2018 11:44:20 EET Hans Verkuil wrote:
> The v4l2_subdev core s_power op was used for two different things: power
> on/off sensors or video decoders/encoders and to put a tuner in standby
> (and only the tuner!). There is no 'tuner wakeup' op, that's done
> automatically when the tuner is accessed.
> 
> The danger with calling (s_power, 0) to put a tuner into standby is that it
> is usually broadcast for all subdevs. So a video receiver subdev that
> supports s_power will also be powered off, and since there is no
> corresponding (s_power, 1) they will never be powered on again.
> 
> In addition, this is specifically meant for tuners only since they draw the
> most current.
> 
> This patch adds a new tuner op called 'standby' and replaces all calls to
> (core, s_power, 0) by (tuner, standby). This prevents confusion between the
> two uses of s_power. Note that there is no overlap: bridge drivers either
> just want to put the tuner into standby, or they deal with powering on/off
> sensors. Never both.
> 
> This also makes it easier to replace s_power for the remaining bridge
> drivers with some PM code later.
> 
> Whether we want something cleaner for tuners in the future is a separate
> topic. There is a lot of legacy code surrounding tuners, and I am very
> hesitant about making changes there.
> 
> Signed-off-by: Hans Verkuil 
> ---
> Changes since v1:
> - move the standby op to the tuner_ops, which makes much more sense.
> ---

[snip]

> diff --git a/drivers/media/v4l2-core/tuner-core.c
> b/drivers/media/v4l2-core/tuner-core.c index 82852f23a3b6..cb126baf8771
> 100644
> --- a/drivers/media/v4l2-core/tuner-core.c
> +++ b/drivers/media/v4l2-core/tuner-core.c
> @@ -1099,23 +1099,15 @@ static int tuner_s_radio(struct v4l2_subdev *sd)
>   */
> 
>  /**
> - * tuner_s_power - controls the power state of the tuner
> + * tuner_standby - controls the power state of the tuner

I'd update the description too.

>   * @sd: pointer to struct v4l2_subdev
>   * @on: a zero value puts the tuner to sleep, non-zero wakes it up

And this parameter doesn't exist anymore. You could have caught that by 
compiling the documentation.

>   */
> -static int tuner_s_power(struct v4l2_subdev *sd, int on)
> +static int tuner_standby(struct v4l2_subdev *sd)
>  {
>   struct tuner *t = to_tuner(sd);
>   struct analog_demod_ops *analog_ops = >fe.ops.analog_ops;
> 
> - if (on) {
> - if (t->standby && set_mode(t, t->mode) == 0) {
> - dprintk("Waking up tuner\n");
> - set_freq(t, 0);
> - }
> - return 0;
> - }
> -

Interesting how this code was not used. I've had a look at tuner-core driver 
out of curiosity, it clearly shows its age :/ I2C address probing belongs to 
another century.

>   dprintk("Putting tuner to sleep\n");
>   t->standby = true;
>   if (analog_ops->standby)
> @@ -1328,10 +1320,10 @@ static int tuner_command(struct i2c_client *client,
> unsigned cmd, void *arg)
> 
>  static const struct v4l2_subdev_core_ops tuner_core_ops = {
>   .log_status = tuner_log_status,
> - .s_power = tuner_s_power,
>  };
> 
>  static const struct v4l2_subdev_tuner_ops tuner_tuner_ops = {
> + .standby = tuner_standby,
>   .s_radio = tuner_s_radio,
>   .g_tuner = tuner_g_tuner,
>   .s_tuner = tuner_s_tuner,
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 980a86c08fce..62429cd89620 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -224,6 +224,9 @@ struct v4l2_subdev_core_ops {
>   * struct v4l2_subdev_tuner_ops - Callbacks used when v4l device was opened
>   *   in radio mode.
>   *
> + * @standby: puts the tuner in standby mode. It will be woken up
> + *automatically the next time it is used.
> + *

I wouldn't have dared making such a statement as I don't trust myself as being 
able to give such a guarantee after reading the code :-)

>   * @s_radio: callback that switches the tuner to radio mode.
>   *drivers should explicitly call it when a tuner ops should
>   *operate on radio mode, before being able to handle it.
> @@ -268,6 +271,7 @@ struct v4l2_subdev_core_ops {
>   * }
>   */
>  struct v4l2_subdev_tuner_ops {
> + int (*standby)(struct v4l2_subdev *sd);
>   int (*s_radio)(struct v4l2_subdev *sd);
>   int (*s_frequency)(struct v4l2_subdev *sd, const struct v4l2_frequency
> *freq);
>   int (*g_frequency)(struct v4l2_subdev *sd, struct v4l2_frequency *freq);

-- 
Regards,

Laurent Pinchart

Re: Bug: Two device nodes created in /dev for a single UVC webcam

[Laurent Pinchart]

Hi Devin,

On Tuesday, 20 February 2018 20:18:16 EET Devin Heitmueller wrote:
> On Mon, Feb 19, 2018 at 11:19 AM, Laurent Pinchart wrote:
> > I've tested VLC (2.2.8) and haven't noticed any issue. If a program is
> > directed to the metadata video node and tries to capture video from it it
> > will obviously fail. That being said, software that work today should
> > continue working, otherwise it's a regression, and we'll have to handle
> > that.
> 
> Perhaps it shouldn't be a video node then (as we do with VBI devices).
> Would something like /dev/videometadataX would be more appropriate?

We've thought about it, and the initial implementation created a metadata 
device node instead of a video device node. This has been rejected, see 
https://www.mail-archive.com/linux-media@vger.kernel.org/msg97454.html and 
https://www.mail-archive.com/linux-media@vger.kernel.org/msg97446.html.

> People have for years operated under the expectation that /dev/videoX
> nodes are video nodes.  If we're going to be creating things with that
> name which aren't video nodes then that is going to cause considerable
> confusion as well as messing up all sorts of existing applications
> which operate under that expectation.
> 
> I know that some of the older PCI boards have always exposed a bunch
> of video nodes for various things (i.e. raw video vs. mpeg, etc), but
> because USB devices have traditionally been simpler they generally
> expose only one node of each type (i.e. one /dev/videoX, /dev/vbiX
> /dev/radioX).  I've already gotten an email from a customer who has a
> ton of scripts which depend on this behavior, so please seriously
> consider the implications of this design decision.

While I can't speak about other USB devices as I'm not too familiar with them, 
please note that the UVC driver already exposes multiple video nodes related 
to video capture (or video output) for some devices, and will posssibly do so 
increasingly in the future when we add support for UVC 1.5. We can reconsider 
the decision of exposing metadata through a video node, but adding new video 
nodes to expose additional compressed video streams for UVC 1.5 support is 
something that userspace has to live with the same way it already has to live 
with multiple video nodes for older PCI boards.

> It's easy to brush this off as "all the existing applications will
> eventually be updated", but you're talking about changing the basic
> behavior of how these device nodes have been presented for over a
> decade.

That's not what I meant, I might have not expressed myself correctly. Updating 
applications is something we should strive for when we want to get rid of an 
undesired userspace behaviour, but that's in no way an excuse for breaking 
anything. Regarding the issue reported by Alexandre-Xavier, it looks to me 
like he might be suffering from another problem, possibly part of the same 
patch series, but not caused by the extra video device node. That's why I 
asked for more information before taking any decision.

-- 
Regards,

Laurent Pinchart

drivers/media/common/videobuf2/videobuf2-v4l2.c:678: undefined reference to `video_devdata'

[kbuild test robot]

Hi Hans,

FYI, the error/warning still remains.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   af3e79d29555b97dd096e2f8e36a0f50213808a8
commit: 7952be9b6ece3d3c4d61f9811d7e5a984580064a media: 
drivers/media/common/videobuf2: rename from videobuf
date:   4 weeks ago
config: x86_64-randconfig-u0-02210105 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
git checkout 7952be9b6ece3d3c4d61f9811d7e5a984580064a
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/media/common/videobuf2/videobuf2-v4l2.o: In function `vb2_poll':
>> drivers/media/common/videobuf2/videobuf2-v4l2.c:678: undefined reference to 
>> `video_devdata'
>> drivers/media/common/videobuf2/videobuf2-v4l2.c:685: undefined reference to 
>> `v4l2_event_pending'
   drivers/media/common/videobuf2/videobuf2-v4l2.o: In function 
`vb2_ioctl_reqbufs':
   drivers/media/common/videobuf2/videobuf2-v4l2.c:714: undefined reference to 
`video_devdata'
   drivers/media/common/videobuf2/videobuf2-v4l2.o: In function 
`vb2_ioctl_create_bufs':
   drivers/media/common/videobuf2/videobuf2-v4l2.c:733: undefined reference to 
`video_devdata'
   drivers/media/common/videobuf2/videobuf2-v4l2.o: In function 
`vb2_ioctl_prepare_buf':
   drivers/media/common/videobuf2/videobuf2-v4l2.c:759: undefined reference to 
`video_devdata'
   drivers/media/common/videobuf2/videobuf2-v4l2.o: In function 
`vb2_ioctl_querybuf':
   drivers/media/common/videobuf2/videobuf2-v4l2.c:769: undefined reference to 
`video_devdata'
   drivers/media/common/videobuf2/videobuf2-v4l2.o: In function 
`vb2_ioctl_qbuf':
   drivers/media/common/videobuf2/videobuf2-v4l2.c:778: undefined reference to 
`video_devdata'
   
drivers/media/common/videobuf2/videobuf2-v4l2.o:drivers/media/common/videobuf2/videobuf2-v4l2.c:788:
 more undefined references to `video_devdata' follow
   drivers/media/common/videobuf2/videobuf2-v4l2.o: In function 
`_vb2_fop_release':
>> drivers/media/common/videobuf2/videobuf2-v4l2.c:848: undefined reference to 
>> `v4l2_fh_release'

vim +678 drivers/media/common/videobuf2/videobuf2-v4l2.c

3c5be988 drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-10-06  675  
49d8ab9f drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-11-03  676  
unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
49d8ab9f drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-11-03  677  
{
49d8ab9f drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-11-03 @678  
struct video_device *vfd = video_devdata(file);
49d8ab9f drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-11-03  679  
unsigned long req_events = poll_requested_events(wait);
49d8ab9f drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-11-03  680  
unsigned int res = 0;
49d8ab9f drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-11-03  681  
49d8ab9f drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-11-03  682  
if (test_bit(V4L2_FL_USES_V4L2_FH, >flags)) {
49d8ab9f drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-11-03  683  
struct v4l2_fh *fh = file->private_data;
49d8ab9f drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-11-03  684  
49d8ab9f drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-11-03 @685  
if (v4l2_event_pending(fh))
49d8ab9f drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-11-03  686  
res = POLLPRI;
49d8ab9f drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-11-03  687  
else if (req_events & POLLPRI)
49d8ab9f drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-11-03  688  
poll_wait(file, >wait, wait);
3c5be988 drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-10-06  689  
}
49d8ab9f drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-11-03  690  
49d8ab9f drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-11-03  691  
return res | vb2_core_poll(q, file, wait);
3c5be988 drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-10-06  692  
}
3c5be988 drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-10-06  693  
EXPORT_SYMBOL_GPL(vb2_poll);
3c5be988 drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-10-06  694  
3c5be988 drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-10-06  695  
/*
3c5be988 drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-10-06  696  
 * The following functions are not part of the vb2 core API, but are helper
3c5be988 drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-10-06  697  
 * functions that plug into struct v4l2_ioctl_ops, struct v4l2_file_operations
3c5be988 drivers/media/v4l2-core/videobuf2-v4l2.c Junghak Sung 2015-10-06  698  
 * and struct vb2_ops.
3c5be988 

Re: Bug: Two device nodes created in /dev for a single UVC webcam

[Devin Heitmueller]

Hi Laurent,

On Mon, Feb 19, 2018 at 11:19 AM, Laurent Pinchart
 wrote:
> I've tested VLC (2.2.8) and haven't noticed any issue. If a program is
> directed to the metadata video node and tries to capture video from it it will
> obviously fail. That being said, software that work today should continue
> working, otherwise it's a regression, and we'll have to handle that.

Perhaps it shouldn't be a video node then (as we do with VBI devices).
Would something like /dev/videometadataX would be more appropriate?

People have for years operated under the expectation that /dev/videoX
nodes are video nodes.  If we're going to be creating things with that
name which aren't video nodes then that is going to cause considerable
confusion as well as messing up all sorts of existing applications
which operate under that expectation.

I know that some of the older PCI boards have always exposed a bunch
of video nodes for various things (i.e. raw video vs. mpeg, etc), but
because USB devices have traditionally been simpler they generally
expose only one node of each type (i.e. one /dev/videoX, /dev/vbiX
/dev/radioX).  I've already gotten an email from a customer who has a
ton of scripts which depend on this behavior, so please seriously
consider the implications of this design decision.

It's easy to brush this off as "all the existing applications will
eventually be updated", but you're talking about changing the basic
behavior of how these device nodes have been presented for over a
decade.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [RFCv4 16/21] v4l2: video_device: support for creating requests

[Hans Verkuil]

On 02/20/2018 05:44 AM, Alexandre Courbot wrote:
> Add a new VIDIOC_NEW_REQUEST ioctl, which allows to instanciate requests
> on devices that support the request API. Requests created that way can
> only control the device they originate from, making them suitable for
> simple devices, but not complex pipelines.
> 
> Signed-off-by: Alexandre Courbot 
> ---
>  Documentation/ioctl/ioctl-number.txt |  1 +
>  drivers/media/v4l2-core/v4l2-dev.c   |  2 ++
>  drivers/media/v4l2-core/v4l2-ioctl.c | 25 +
>  include/media/v4l2-dev.h |  2 ++
>  include/uapi/linux/videodev2.h   |  3 +++
>  5 files changed, 33 insertions(+)
> 
> diff --git a/Documentation/ioctl/ioctl-number.txt 
> b/Documentation/ioctl/ioctl-number.txt
> index 6501389d55b9..afdc9ed255b0 100644
> --- a/Documentation/ioctl/ioctl-number.txt
> +++ b/Documentation/ioctl/ioctl-number.txt
> @@ -286,6 +286,7 @@ Code  Seq#(hex)   Include FileComments
>   
>  'z'  10-4F   drivers/s390/crypto/zcrypt_api.hconflict!
>  '|'  00-7F   linux/media.h
> +'|'  80-9F   linux/media-request.h
>  0x80 00-1F   linux/fb.h
>  0x89 00-06   arch/x86/include/asm/sockios.h
>  0x89 0B-DF   linux/sockios.h

This  doesn't belong in this patch.

> diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
> b/drivers/media/v4l2-core/v4l2-dev.c
> index 0301fe426a43..062ebee5bffc 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -559,6 +559,8 @@ static void determine_valid_ioctls(struct video_device 
> *vdev)
>   set_bit(_IOC_NR(VIDIOC_TRY_EXT_CTRLS), valid_ioctls);
>   if (vdev->ctrl_handler || ops->vidioc_querymenu)
>   set_bit(_IOC_NR(VIDIOC_QUERYMENU), valid_ioctls);
> + if (vdev->req_mgr)
> + set_bit(_IOC_NR(VIDIOC_NEW_REQUEST), valid_ioctls);
>   SET_VALID_IOCTL(ops, VIDIOC_G_FREQUENCY, vidioc_g_frequency);
>   SET_VALID_IOCTL(ops, VIDIOC_S_FREQUENCY, vidioc_s_frequency);
>   SET_VALID_IOCTL(ops, VIDIOC_LOG_STATUS, vidioc_log_status);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index ab4968ea443f..a45fe078f8ae 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -21,6 +21,7 @@
>  
>  #include 
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -842,6 +843,13 @@ static void v4l_print_freq_band(const void *arg, bool 
> write_only)
>   p->rangehigh, p->modulation);
>  }
>  
> +static void vidioc_print_new_request(const void *arg, bool write_only)
> +{
> + const struct media_request_new *new = arg;
> +
> + pr_cont("fd=0x%x\n", new->fd);

I'd use %d since fds are typically shown as signed integers.

> +}
> +
>  static void v4l_print_edid(const void *arg, bool write_only)
>  {
>   const struct v4l2_edid *p = arg;
> @@ -2486,6 +2494,22 @@ static int v4l_enum_freq_bands(const struct 
> v4l2_ioctl_ops *ops,
>   return -ENOTTY;
>  }
>  
> +static int vidioc_new_request(const struct v4l2_ioctl_ops *ops,
> +   struct file *file, void *fh, void *arg)
> +{
> +#if IS_ENABLED(CONFIG_MEDIA_REQUEST_API)
> + struct media_request_new *new = arg;
> + struct video_device *vfd = video_devdata(file);
> +
> + if (!vfd->req_mgr)
> + return -ENOTTY;
> +
> + return media_request_ioctl_new(vfd->req_mgr, new);
> +#else
> + return -ENOTTY;
> +#endif
> +}

You don't need the #ifdef's here. media_request_ioctl_new() will be stubbed if
CONFIG_MEDIA_REQUEST_API isn't set.

> +
>  struct v4l2_ioctl_info {
>   unsigned int ioctl;
>   u32 flags;
> @@ -2617,6 +2641,7 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>   IOCTL_INFO_FNC(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, 
> v4l_print_freq_band, 0),
>   IOCTL_INFO_FNC(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, 
> v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
>   IOCTL_INFO_FNC(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, 
> v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, 
> id)),
> + IOCTL_INFO_FNC(VIDIOC_NEW_REQUEST, vidioc_new_request, 
> vidioc_print_new_request, 0),
>  };
>  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>  
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 53f32022fabe..e6c4e10889bc 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -209,6 +209,7 @@ struct v4l2_file_operations {
>   * @entity:  media_entity
>   * @intf_devnode: pointer to  media_intf_devnode
>   * @pipe:  media_pipeline
> + * @req_mgr: request manager to use if this device supports creating requests
>   * @fops: pointer to  v4l2_file_operations for the video device
>   * @device_caps: device capabilities as used in v4l2_capabilities
>   * @dev:  device for the video device
> @@ -251,6 +252,7 @@ struct video_device
>  

Re: [RFCv4 13/21] media: videobuf2-v4l2: support for requests

[Hans Verkuil]

On 02/20/2018 05:44 AM, Alexandre Courbot wrote:
> Add a new vb2_qbuf_request() (a request-aware version of vb2_qbuf())
> that request-aware drivers can call to queue a buffer into a request
> instead of directly into the vb2 queue if relevent.
> 
> This function expects that drivers invoking it are using instances of
> v4l2_request_entity and v4l2_request_entity_data to describe their
> entity and entity data respectively.
> 
> Also add the vb2_request_submit() helper function which drivers can
> invoke in order to queue all the buffers previously queued into a
> request into the regular vb2 queue.
> 
> Signed-off-by: Alexandre Courbot 
> ---
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 129 +-
>  include/media/videobuf2-v4l2.h|  59 
>  2 files changed, 187 insertions(+), 1 deletion(-)
> 



> @@ -776,10 +899,14 @@ EXPORT_SYMBOL_GPL(vb2_ioctl_querybuf);
>  int vb2_ioctl_qbuf(struct file *file, void *priv, struct v4l2_buffer *p)
>  {
>   struct video_device *vdev = video_devdata(file);
> + struct v4l2_fh *fh = NULL;
> +
> + if (test_bit(V4L2_FL_USES_V4L2_FH, >flags))
> + fh = file->private_data;

No need for this. All drivers using vb2 will also use v4l2_fh.

>  
>   if (vb2_queue_is_busy(vdev, file))
>   return -EBUSY;
> - return vb2_qbuf(vdev->queue, p);
> + return vb2_qbuf_request(vdev->queue, p, fh ? fh->entity : NULL);
>  }
>  EXPORT_SYMBOL_GPL(vb2_ioctl_qbuf);
>  
> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> index 3d5e2d739f05..d4dfa266a0da 100644
> --- a/include/media/videobuf2-v4l2.h
> +++ b/include/media/videobuf2-v4l2.h
> @@ -23,6 +23,12 @@
>  #error VB2_MAX_PLANES != VIDEO_MAX_PLANES
>  #endif
>  
> +struct media_entity;
> +struct v4l2_fh;
> +struct media_request;
> +struct media_request_entity;
> +struct v4l2_request_entity_data;
> +
>  /**
>   * struct vb2_v4l2_buffer - video buffer information for v4l2.
>   *
> @@ -116,6 +122,59 @@ int vb2_prepare_buf(struct vb2_queue *q, struct 
> v4l2_buffer *b);
>   */
>  int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b);
>  
> +#if IS_ENABLED(CONFIG_MEDIA_REQUEST_API)
> +
> +/**
> + * vb2_qbuf_request() - Queue a buffer, with request support
> + * @q:   pointer to  vb2_queue with videobuf2 queue.
> + * @b:   buffer structure passed from userspace to
> + *   _ioctl_ops->vidioc_qbuf handler in driver
> + * @entity:  request entity to queue for if requests are used.
> + *
> + * Should be called from _ioctl_ops->vidioc_qbuf handler of a driver.
> + *
> + * If requests are not in use, calling this is equivalent to calling 
> vb2_qbuf().
> + *
> + * If the request_fd member of b is set, then the buffer represented by b is
> + * queued in the request instead of the vb2 queue. The buffer will be passed
> + * to the vb2 queue when the request is submitted.

I would definitely also prepare the buffer at this time. That way you'll see any
errors relating to the prepare early on.

> + *
> + * The return values from this function are intended to be directly returned
> + * from _ioctl_ops->vidioc_qbuf handler in driver.
> + */
> +int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer *b,
> +  struct media_request_entity *entity);
> +
> +/**
> + * vb2_request_submit() - Queue all the buffers in a v4l2 request.
> + * @data:request entity data to queue buffers of
> + *
> + * This function should be called from the media_request_entity_ops::submit
> + * hook for instances of media_request_v4l2_entity_data. It will immediately
> + * queue all the request-bound buffers to their respective vb2 queues.
> + *
> + * Errors from vb2_core_qbuf() are returned if something happened. Also, 
> since
> + * v4l2 request entities require at least one buffer for the request to 
> trigger,
> + * this function will return -EINVAL if no buffer have been bound at all for
> + * this entity.
> + */
> +int vb2_request_submit(struct v4l2_request_entity_data *data);
> +
> +#else /* CONFIG_MEDIA_REQUEST_API */
> +
> +static inline int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer 
> *b,
> +struct media_request_entity *entity)
> +{
> + return vb2_qbuf(q, b);
> +}
> +
> +static inline int vb2_request_submit(struct v4l2_request_entity_data *data)
> +{
> + return -ENOTSUPP;
> +}
> +
> +#endif /* CONFIG_MEDIA_REQUEST_API */
> +
>  /**
>   * vb2_expbuf() - Export a buffer as a file descriptor
>   * @q:   pointer to  vb2_queue with videobuf2 queue.
> 

Regards,

Hans

Re: [RFCv4 11/21] media: v4l2_fh: add request entity field

[Hans Verkuil]

On 02/20/18 05:44, Alexandre Courbot wrote:
> Allow drivers to assign a request entity to v4l2_fh. This will be useful
> for request-aware ioctls to find out which request entity to use.
> 
> Signed-off-by: Alexandre Courbot 
> ---
>  include/media/v4l2-fh.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> index ea73fef8bdc0..f54cb319dd64 100644
> --- a/include/media/v4l2-fh.h
> +++ b/include/media/v4l2-fh.h
> @@ -28,6 +28,7 @@
>  
>  struct video_device;
>  struct v4l2_ctrl_handler;
> +struct media_request_entity;
>  
>  /**
>   * struct v4l2_fh - Describes a V4L2 file handler
> @@ -43,6 +44,7 @@ struct v4l2_ctrl_handler;
>   * @navailable: number of available events at @available list
>   * @sequence: event sequence number
>   * @m2m_ctx: pointer to  v4l2_m2m_ctx
> + * @entity: the request entity this fh operates on behalf of
>   */
>  struct v4l2_fh {
>   struct list_headlist;
> @@ -60,6 +62,7 @@ struct v4l2_fh {
>  #if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
>   struct v4l2_m2m_ctx *m2m_ctx;
>  #endif
> + struct media_request_entity *entity;

The name 'media_request_entity' is very confusing.

In the media controller API terminology an entity represents a piece
of hardware with inputs and outputs (very rough description), but a
request is not an entity. It may be associated with an entity, though.

So calling this field 'entity' is also very misleading.

As with previous patches, I'll have to think about this and try and
come up with better, less confusing names.

Regards,

Hans

>  };
>  
>  /**
> 

Re: stable-rc build: 3 warnings 0 failures (stable-rc/v4.14.20-119-g1b1ab1d)

[Arnd Bergmann]

On Tue, Feb 20, 2018 at 1:47 PM, Olof's autobuilder  wrote:

> Warnings:
>
> arm64.allmodconfig:
> drivers/media/tuners/r820t.c:1334:1: warning: the frame size of 2896 bytes is 
> larger than 2048 bytes [-Wframe-larger-than=]

Hi Greg,

please add

16c3ada89cff ("media: r820t: fix r820t_write_reg for KASAN")

This is an old bug, but hasn't shown up before as the stack warning
limit was turned off
in allmodconfig kernels. The fix is also on the backport lists I sent
for 4.9 and 4.4.

Arnd

Re: [RFCv4 10/21] videodev2.h: Add request_fd field to v4l2_buffer

[Hans Verkuil]

On 02/20/18 05:44, Alexandre Courbot wrote:
> From: Hans Verkuil 
> 
> When queuing buffers allow for passing the request that should
> be associated with this buffer.
> 
> Signed-off-by: Hans Verkuil 
> [acour...@chromium.org: make request ID 32-bit]
> Signed-off-by: Alexandre Courbot 
> ---
>  drivers/media/common/videobuf2/videobuf2-v4l2.c | 2 +-
>  drivers/media/usb/cpia2/cpia2_v4l.c | 2 +-
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c   | 9 ++---
>  drivers/media/v4l2-core/v4l2-ioctl.c| 4 ++--
>  include/uapi/linux/videodev2.h  | 3 ++-
>  5 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c 
> b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 886a2d8d5c6c..6d4d184aa68e 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -203,7 +203,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, 
> void *pb)
>   b->timestamp = ns_to_timeval(vb->timestamp);
>   b->timecode = vbuf->timecode;
>   b->sequence = vbuf->sequence;
> - b->reserved2 = 0;
> + b->request_fd = 0;
>   b->reserved = 0;
>  
>   if (q->is_multiplanar) {
> diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c 
> b/drivers/media/usb/cpia2/cpia2_v4l.c
> index 99f106b13280..af42ce3ceb48 100644
> --- a/drivers/media/usb/cpia2/cpia2_v4l.c
> +++ b/drivers/media/usb/cpia2/cpia2_v4l.c
> @@ -948,7 +948,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, 
> struct v4l2_buffer *buf)
>   buf->sequence = cam->buffers[buf->index].seq;
>   buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
>   buf->length = cam->frame_size;
> - buf->reserved2 = 0;
> + buf->request_fd = 0;
>   buf->reserved = 0;
>   memset(>timecode, 0, sizeof(buf->timecode));
>  
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c 
> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index 5198c9eeb348..32bf47489a2e 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -386,7 +386,7 @@ struct v4l2_buffer32 {
>   __s32   fd;
>   } m;
>   __u32   length;
> - __u32   reserved2;
> + __s32   request_fd;
>   __u32   reserved;
>  };
>  
> @@ -486,6 +486,7 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user 
> *kp,
>  {
>   u32 type;
>   u32 length;
> + s32 request_fd;
>   enum v4l2_memory memory;
>   struct v4l2_plane32 __user *uplane32;
>   struct v4l2_plane __user *uplane;
> @@ -500,7 +501,9 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user 
> *kp,
>   get_user(memory, >memory) ||
>   put_user(memory, >memory) ||
>   get_user(length, >length) ||
> - put_user(length, >length))
> + put_user(length, >length) ||
> + get_user(request_fd, >request_fd) ||
> + put_user(request_fd, >request_fd))
>   return -EFAULT;
>  
>   if (V4L2_TYPE_IS_OUTPUT(type))
> @@ -604,7 +607,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user 
> *kp,
>   assign_in_user(>timestamp.tv_usec, >timestamp.tv_usec) ||
>   copy_in_user(>timecode, >timecode, sizeof(kp->timecode)) ||
>   assign_in_user(>sequence, >sequence) ||
> - assign_in_user(>reserved2, >reserved2) ||
> + assign_in_user(>request_fd, >request_fd) ||
>   assign_in_user(>reserved, >reserved) ||
>   get_user(length, >length) ||
>   put_user(length, >length))
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 260288ca4f55..7bfeaf233d5a 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -437,13 +437,13 @@ static void v4l_print_buffer(const void *arg, bool 
> write_only)
>   const struct v4l2_plane *plane;
>   int i;
>  
> - pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, flags=0x%08x, 
> field=%s, sequence=%d, memory=%s",
> + pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, request_fd=%u, 
> flags=0x%08x, field=%s, sequence=%d, memory=%s",
>   p->timestamp.tv_sec / 3600,
>   (int)(p->timestamp.tv_sec / 60) % 60,
>   (int)(p->timestamp.tv_sec % 60),
>   (long)p->timestamp.tv_usec,
>   p->index,
> - prt_names(p->type, v4l2_type_names),
> + prt_names(p->type, v4l2_type_names), p->request_fd,
>   p->flags, prt_names(p->field, v4l2_field_names),
>   p->sequence, prt_names(p->memory, v4l2_memory_names));
>  
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 

Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions

[Robin Murphy]

Hi Marek,

On 20/02/18 09:36, Marek Szyprowski wrote:

Hi Robin,

On 2018-02-19 17:29, Robin Murphy wrote:

Hi Maciej,

On 19/02/18 15:43, Maciej Purski wrote:

When a driver is going to use clk_bulk_get() function, it has to
initialize an array of clk_bulk_data, by filling its id fields.

Add a new function to the core, which dynamically allocates
clk_bulk_data array and fills its id fields. Add clk_bulk_free()
function, which frees the array allocated by clk_bulk_alloc() function.
Add a managed version of clk_bulk_alloc().


Seeing how every subsequent patch ends up with the roughly this same 
stanza:


x = devm_clk_bulk_alloc(dev, num, names);
if (IS_ERR(x)
    return PTR_ERR(x);
ret = devm_clk_bulk_get(dev, x, num);

I wonder if it might be better to simply implement:

int devm_clk_bulk_alloc_get(dev, , num, names)

that does the whole lot in one go, and let drivers that want to do 
more fiddly things continue to open-code the allocation.


But perhaps that's an abstraction too far... I'm not all that familiar 
with the lie of the land here.


Hmmm. This patchset clearly shows, that it would be much simpler if we
get rid of clk_bulk_data structure at all and let clk_bulk_* functions
to operate on struct clk *array[]. Typically the list of clock names
is already in some kind of array (taken from driver data or statically
embedded into driver), so there is little benefit from duplicating it
in clk_bulk_data. Sadly, I missed clk_bulk_* api discussion and maybe
there are other benefits from this approach.

If not, I suggest simplifying clk_bulk_* api by dropping clk_bulk_data
structure and switching to clock ptr array:

int clk_bulk_get(struct device *dev, int num_clock, struct clk *clocks[],
              const char *clk_names[]);
int clk_bulk_prepare(int num_clks, struct clk *clks[]);
int clk_bulk_enable(int num_clks, struct clk *clks[]);
...


Yes, that's certainly a possibility; if on the other hand there are 
desirable reasons for the encapsulation (personally, I do think it's 
quite neat), then maybe num_clocks should get pushed down into 
clk_bulk_data as well - then with dedicated alloc/free functions as 
proposed here it could become a simple opaque cookie as far as callers 
are concerned.


I also haven't looked into the origins of the bulk API design, though; 
I've just been familiarising myself from the perspective of reviewing 
general clk API usage in drivers.


Robin.


Signed-off-by: Maciej Purski 
---
  drivers/clk/clk-bulk.c   | 16 
  drivers/clk/clk-devres.c | 37 +---
  include/linux/clk.h  | 64 


  3 files changed, 113 insertions(+), 4 deletions(-)



[...]
@@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char *dev_id, 
const char *con_id);

    #else /* !CONFIG_HAVE_CLK */
  +static inline struct clk_bulk_data *clk_bulk_alloc(int num_clks,
+   const char **clk_ids)
+{
+    return NULL;


Either way, is it intentional not returning an ERR_PTR() value in this 
case? Since NULL will pass an IS_ERR() check, it seems a bit fragile 
for an allocation call to apparently succeed when the whole API is 
configured out (and I believe introducing new uses of IS_ERR_OR_NULL() 
is in general strongly discouraged.)


Robin.


+}
+
+static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct 
device *dev,

+    int num_clks,
+    const char **clk_ids)
+{
+    return NULL;
+}
+
+static inline void clk_bulk_free(struct clk_bulk_data *clks)
+{
+}
+
  static inline struct clk *clk_get(struct device *dev, const char *id)
  {
  return NULL;



Best regards

Re: [RFCv4 09/21] v4l2: add request API support

[Hans Verkuil]

On 02/20/18 05:44, Alexandre Courbot wrote:
> Add a v4l2 request entity data structure that takes care of storing the
> request-related state of a V4L2 device ; in this case, its controls.
> 
> Signed-off-by: Alexandre Courbot 
> ---
>  drivers/media/v4l2-core/Makefile   |   1 +
>  drivers/media/v4l2-core/v4l2-request.c | 178 +
>  include/media/v4l2-request.h   | 159 ++
>  3 files changed, 338 insertions(+)
>  create mode 100644 drivers/media/v4l2-core/v4l2-request.c
>  create mode 100644 include/media/v4l2-request.h
> 
> diff --git a/drivers/media/v4l2-core/Makefile 
> b/drivers/media/v4l2-core/Makefile
> index 80de2cb9c476..13d0477535bd 100644
> --- a/drivers/media/v4l2-core/Makefile
> +++ b/drivers/media/v4l2-core/Makefile
> @@ -16,6 +16,7 @@ ifeq ($(CONFIG_TRACEPOINTS),y)
>videodev-objs += vb2-trace.o v4l2-trace.o
>  endif
>  videodev-$(CONFIG_MEDIA_CONTROLLER) += v4l2-mc.o
> +videodev-$(CONFIG_MEDIA_REQUEST_API) += v4l2-request.o
>  
>  obj-$(CONFIG_VIDEO_V4L2) += videodev.o
>  obj-$(CONFIG_VIDEO_V4L2) += v4l2-common.o
> diff --git a/drivers/media/v4l2-core/v4l2-request.c 
> b/drivers/media/v4l2-core/v4l2-request.c
> new file mode 100644
> index ..e8ad10e2f525
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-request.c
> @@ -0,0 +1,178 @@
> +/*
> + * Media requests support for V4L2
> + *
> + * Copyright (C) 2018, 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 
> +
> +void v4l2_request_entity_init(struct v4l2_request_entity *entity,
> +   const struct media_request_entity_ops *ops,
> +   struct video_device *vdev)
> +{
> + media_request_entity_init(>base, 
> MEDIA_REQUEST_ENTITY_TYPE_V4L2, ops);
> + entity->vdev = vdev;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_request_entity_init);
> +
> +struct media_request_entity_data *
> +v4l2_request_entity_data_alloc(struct media_request *req,
> +struct v4l2_ctrl_handler *hdl)
> +{
> + struct v4l2_request_entity_data *data;
> + int ret;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return ERR_PTR(-ENOMEM);
> +
> + ret = v4l2_ctrl_request_init(>ctrls);
> + if (ret) {
> + kfree(data);
> + return ERR_PTR(ret);
> + }
> + ret = v4l2_ctrl_request_clone(>ctrls, hdl, NULL);
> + if (ret) {
> + kfree(data);
> + return ERR_PTR(ret);
> + }
> +
> + INIT_LIST_HEAD(>queued_buffers);
> +
> + return >base;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_request_entity_data_alloc);
> +
> +void v4l2_request_entity_data_free(struct media_request_entity_data *_data)
> +{
> + struct v4l2_request_entity_data *data;
> + struct v4l2_vb2_request_buffer *qb, *n;
> +
> + data = to_v4l2_entity_data(_data);
> +
> + list_for_each_entry_safe(qb, n, >queued_buffers, node) {
> + struct vb2_buffer *buf;
> + dev_warn(_data->request->mgr->dev,
> +  "entity data freed while buffer still queued!\n");
> +
> + /* give buffer back to user-space */
> + buf = qb->queue->bufs[qb->v4l2_buf.index];
> + buf->state = qb->pre_req_state;
> + buf->request = NULL;
> +
> + kfree(qb);
> + }
> +
> + v4l2_ctrl_handler_free(>ctrls);
> + kfree(data);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_request_entity_data_free);
> +
> +
> +
> +
> +
> +static struct media_request *v4l2_request_alloc(struct media_request_mgr 
> *mgr)
> +{
> + struct media_request *req;
> +
> + req = kzalloc(sizeof(*req), GFP_KERNEL);
> + if (!req)
> + return ERR_PTR(-ENOMEM);
> +
> + req->mgr = mgr;
> + req->state = MEDIA_REQUEST_STATE_IDLE;
> + INIT_LIST_HEAD(>data);
> + init_waitqueue_head(>complete_wait);
> + mutex_init(>lock);
> +
> + mutex_lock(>mutex);
> + list_add_tail(>list, >requests);
> + mutex_unlock(>mutex);
> +
> + return req;
> +}
> +
> +static void v4l2_request_free(struct media_request *req)
> +{
> + struct media_request_mgr *mgr = req->mgr;
> + struct media_request_entity_data *data, *next;
> +
> + mutex_lock(>mutex);
> + list_del(>list);
> + mutex_unlock(>mutex);
> +
> + list_for_each_entry_safe(data, next, >data, list) {
> + list_del(>list);
> + data->entity->ops->data_free(data);
> + }
> +
> + 

Re: [RFCv4 08/21] [WAR] v4l2-ctrls: do not clone non-standard controls

[Hans Verkuil]

On 02/20/18 05:44, Alexandre Courbot wrote:
> Only standard controls can be successfully cloned: handler_new_ref, used
> by v4l2_ctrl_request_clone(), forcibly calls v4l2_ctrl_new_std() which
> fails to find custom controls names, and we eventually hit the condition
> that name == NULL in v4l2_ctrl_new().

Hmm, the core reason is that handler_new_ref tries to automatically create
a new control class if it didn't exist yet. Which is OK for standard control
classes but not for non-standard control classes such as is used in vivid.

I will have to think about this.

Regards,

Hans

> 
> This prevents us from using non-standard controls with requests, but
> that is enough for testing purposes.
> 
> Signed-off-by: Alexandre Courbot 
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
> b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 166647817efb..7a81aa5959c3 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -2772,6 +2772,11 @@ int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler 
> *hdl,
>   if (filter && !filter(ctrl))
>   continue;
>   err = handler_new_ref(hdl, ctrl, _ref, false);
> + if (err) {
> + printk("%s: handler_new_ref on control %x (%s) returned 
> %d\n", __func__, ctrl->id, ctrl->name, err);
> + err = 0;
> + continue;
> + }
>   if (err)
>   break;
>   if (from->is_request)
> 

Re: cron job: media_tree daily build: ABI WARNING

[Hans Verkuil]

On 02/20/18 05:44, Hans Verkuil wrote:
> This message is generated daily by a cron job that builds media_tree for
> the kernels and architectures in the list below.
> 
> Results of the daily build of media_tree:
> 
> date: Tue Feb 20 05:00:11 CET 2018
> media-tree git hash:  29422737017b866d4a51014cc7522fa3a99e8852
> media_build git hash: d144cfe4b3c37ece55ae27778c99765d4943c4fa
> v4l-utils git hash:   4665ab1fbab1ddaa5696bc3f5865ec6fc83eaf84
> gcc version:  i686-linux-gcc (GCC) 7.3.0
> sparse version:   v0.5.0-3994-g45eb2282
> smatch version:   v0.5.0-3994-g45eb2282
> host hardware:x86_64
> host os:  4.14.0-3-amd64
> 
> linux-git-arm-at91: OK
> linux-git-arm-davinci: OK
> linux-git-arm-multi: OK
> linux-git-arm-pxa: OK
> linux-git-arm-stm32: OK
> linux-git-arm64: OK
> linux-git-blackfin-bf561: OK
> linux-git-i686: OK
> linux-git-m32r: OK
> linux-git-mips: OK
> linux-git-powerpc64: OK
> linux-git-sh: OK
> linux-git-x86_64: OK
> linux-2.6.36.4-i686: WARNINGS
> linux-2.6.36.4-x86_64: WARNINGS
> linux-2.6.37.6-i686: WARNINGS
> linux-2.6.37.6-x86_64: WARNINGS
> linux-2.6.38.8-i686: WARNINGS
> linux-2.6.38.8-x86_64: WARNINGS
> linux-2.6.39.4-i686: WARNINGS
> linux-2.6.39.4-x86_64: WARNINGS
> linux-3.0.60-i686: WARNINGS
> linux-3.0.60-x86_64: WARNINGS
> linux-3.1.10-i686: WARNINGS
> linux-3.1.10-x86_64: WARNINGS
> linux-3.2.98-i686: WARNINGS
> linux-3.2.98-x86_64: WARNINGS
> linux-3.3.8-i686: WARNINGS
> linux-3.3.8-x86_64: WARNINGS
> linux-3.4.27-i686: WARNINGS
> linux-3.4.27-x86_64: WARNINGS
> linux-3.5.7-i686: WARNINGS
> linux-3.5.7-x86_64: WARNINGS
> linux-3.6.11-i686: WARNINGS
> linux-3.6.11-x86_64: WARNINGS
> linux-3.7.4-i686: WARNINGS
> linux-3.7.4-x86_64: WARNINGS
> linux-3.8-i686: WARNINGS
> linux-3.8-x86_64: WARNINGS
> linux-3.9.2-i686: WARNINGS
> linux-3.9.2-x86_64: WARNINGS
> linux-3.10.1-i686: WARNINGS
> linux-3.10.1-x86_64: WARNINGS
> linux-3.11.1-i686: WARNINGS
> linux-3.11.1-x86_64: WARNINGS
> linux-3.12.67-i686: WARNINGS
> linux-3.12.67-x86_64: WARNINGS
> linux-3.13.11-i686: WARNINGS
> linux-3.13.11-x86_64: WARNINGS
> linux-3.14.9-i686: WARNINGS
> linux-3.14.9-x86_64: WARNINGS
> linux-3.15.2-i686: WARNINGS
> linux-3.15.2-x86_64: WARNINGS
> linux-3.16.53-i686: WARNINGS
> linux-3.16.53-x86_64: WARNINGS
> linux-3.17.8-i686: WARNINGS
> linux-3.17.8-x86_64: WARNINGS
> linux-3.18.93-i686: WARNINGS
> linux-3.18.93-x86_64: WARNINGS
> linux-3.19-i686: WARNINGS
> linux-3.19-x86_64: WARNINGS
> linux-4.0.9-i686: WARNINGS
> linux-4.0.9-x86_64: WARNINGS
> linux-4.1.49-i686: WARNINGS
> linux-4.1.49-x86_64: WARNINGS
> linux-4.2.8-i686: WARNINGS
> linux-4.2.8-x86_64: WARNINGS
> linux-4.3.6-i686: WARNINGS
> linux-4.3.6-x86_64: WARNINGS
> linux-4.4.115-i686: OK
> linux-4.4.115-x86_64: OK
> linux-4.5.7-i686: WARNINGS
> linux-4.5.7-x86_64: WARNINGS
> linux-4.6.7-i686: OK
> linux-4.6.7-x86_64: WARNINGS
> linux-4.7.5-i686: OK
> linux-4.7.5-x86_64: WARNINGS
> linux-4.8-i686: OK
> linux-4.8-x86_64: WARNINGS
> linux-4.9.80-i686: OK
> linux-4.9.80-x86_64: OK
> linux-4.10.14-i686: OK
> linux-4.10.14-x86_64: WARNINGS
> linux-4.11-i686: OK
> linux-4.11-x86_64: WARNINGS
> linux-4.12.1-i686: OK
> linux-4.12.1-x86_64: WARNINGS
> linux-4.13-i686: OK
> linux-4.13-x86_64: OK
> linux-4.14.17-i686: OK
> linux-4.14.17-x86_64: OK
> linux-4.15.2-i686: OK
> linux-4.15.2-x86_64: OK
> linux-4.16-rc1-i686: OK
> linux-4.16-rc1-x86_64: OK
> apps: WARNINGS
> spec-git: OK
> ABI WARNING: change for blackfin-bf561

Ignore this. There are some issues with the blackfin-bf561 debugging output
and I'm looking at it.

Regards,

Hans

Re: [RFCv4 01/21] media: add request API core and UAPI

[Hans Verkuil]

On 02/20/18 05:44, 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/Kconfig  |   3 +
>  drivers/media/Makefile |   6 +
>  drivers/media/media-request.c  | 341 
>  include/media/media-request.h  | 349 +
>  include/uapi/linux/media-request.h |  37 +++
>  5 files changed, 736 insertions(+)
>  create mode 100644 drivers/media/media-request.c
>  create mode 100644 include/media/media-request.h
>  create mode 100644 include/uapi/linux/media-request.h
> 
> diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> index 145e12bfb819..db30fc9547d2 100644
> --- a/drivers/media/Kconfig
> +++ b/drivers/media/Kconfig
> @@ -130,6 +130,9 @@ config VIDEO_V4L2_SUBDEV_API
>  
> This API is mostly used by camera interfaces in embedded platforms.
>  
> +config MEDIA_REQUEST_API
> + tristate
> +
>  source "drivers/media/v4l2-core/Kconfig"
>  
>  #
> diff --git a/drivers/media/Makefile b/drivers/media/Makefile
> index 594b462ddf0e..03c0a39ad344 100644
> --- a/drivers/media/Makefile
> +++ b/drivers/media/Makefile
> @@ -5,6 +5,12 @@
>  
>  media-objs   := media-device.o media-devnode.o media-entity.o
>  
> +#
> +# Request API support comes as its own module since it can be used by
> +# both media and video devices
> +#
> +obj-$(CONFIG_MEDIA_REQUEST_API) += media-request.o
> +
>  #
>  # I2C drivers should come before other drivers, otherwise they'll fail
>  # when compiled as builtin drivers
> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
> new file mode 100644
> index ..b88362028561
> --- /dev/null
> +++ b/drivers/media/media-request.c
> @@ -0,0 +1,341 @@
> +/*
> + * Request base implementation
> + *
> + * Copyright (C) 2018, 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 
> +#include 
> +
> +#include 
> +
> +const struct file_operations request_fops;
> +
> +static const char * const media_request_states[] __maybe_unused = {

Why 'maybe_unused'?

> + "IDLE",
> + "SUBMITTED",
> + "COMPLETED",
> + "INVALID",

I don't like to yell. I prefer "Idle", "Submitted", etc.

> +};
> +
> +struct media_request *media_request_get(struct media_request *req)
> +{
> + get_file(req->file);
> + return req;
> +}
> +EXPORT_SYMBOL_GPL(media_request_get);
> +
> +struct media_request *media_request_get_from_fd(int fd)
> +{
> + struct file *f;
> +
> + f = fget(fd);
> + if (!f)
> + return NULL;
> +
> + /* Not a request FD? */
> + if (f->f_op != _fops) {
> + fput(f);
> + return NULL;
> + }
> +
> + return f->private_data;
> +}
> +EXPORT_SYMBOL_GPL(media_request_get_from_fd);
> +
> +void media_request_put(struct media_request *req)
> +{
> + if (WARN_ON(req == NULL))
> + return;
> +
> + fput(req->file);
> +}
> +EXPORT_SYMBOL_GPL(media_request_put);
> +
> +struct media_request_entity_data *
> +media_request_get_entity_data(struct media_request *req,
> +   struct media_request_entity *entity)
> +{
> + struct media_request_entity_data *data;
> +
> + /* First check that this entity is valid for this request at all */
> + if (!req->mgr->ops->entity_valid(req, entity))
> + return ERR_PTR(-EINVAL);
> +
> + mutex_lock(>lock);
> +
> + /* Lookup whether we already have entity data */
> + list_for_each_entry(data, >data, list) {
> + if (data->entity == entity)
> + goto out;
> + }
> +
> + /* No entity data found, let's create it */
> + data = entity->ops->data_alloc(req, entity);
> + if (IS_ERR(data))
> + goto out;
> +
> + data->entity = entity;
> + list_add_tail(>list, >data);
> +
> +out:
> + mutex_unlock(>lock);
> +
> + return data;
> +}
> +EXPORT_SYMBOL_GPL(media_request_get_entity_data);
> +
> +static unsigned int media_request_poll(struct file *file, poll_table *wait)
> +{
> + struct media_request *req = file->private_data;
> +
> + 

[PATCHv2] media: add tuner standby op, use where needed

[Hans Verkuil]

The v4l2_subdev core s_power op was used for two different things: power on/off
sensors or video decoders/encoders and to put a tuner in standby (and only the
tuner!). There is no 'tuner wakeup' op, that's done automatically when the tuner
is accessed.

The danger with calling (s_power, 0) to put a tuner into standby is that it is
usually broadcast for all subdevs. So a video receiver subdev that supports
s_power will also be powered off, and since there is no corresponding (s_power, 
1)
they will never be powered on again.

In addition, this is specifically meant for tuners only since they draw the most
current.

This patch adds a new tuner op called 'standby' and replaces all calls to
(core, s_power, 0) by (tuner, standby). This prevents confusion between the two
uses of s_power. Note that there is no overlap: bridge drivers either just want
to put the tuner into standby, or they deal with powering on/off sensors. Never
both.

This also makes it easier to replace s_power for the remaining bridge drivers
with some PM code later.

Whether we want something cleaner for tuners in the future is a separate topic.
There is a lot of legacy code surrounding tuners, and I am very hesitant about
making changes there.

Signed-off-by: Hans Verkuil 
---
Changes since v1:
- move the standby op to the tuner_ops, which makes much more sense.
---
diff --git a/drivers/media/pci/cx23885/cx23885-core.c 
b/drivers/media/pci/cx23885/cx23885-core.c
index 8f63df1cb418..b8394cdf030b 100644
--- a/drivers/media/pci/cx23885/cx23885-core.c
+++ b/drivers/media/pci/cx23885/cx23885-core.c
@@ -965,7 +965,7 @@ static int cx23885_dev_setup(struct cx23885_dev *dev)
cx23885_i2c_register(>i2c_bus[1]);
cx23885_i2c_register(>i2c_bus[2]);
cx23885_card_setup(dev);
-   call_all(dev, core, s_power, 0);
+   call_all(dev, tuner, standby);
cx23885_ir_init(dev);

if (dev->board == CX23885_BOARD_VIEWCAST_460E) {
diff --git a/drivers/media/pci/cx23885/cx23885-dvb.c 
b/drivers/media/pci/cx23885/cx23885-dvb.c
index 700422b538c0..ba5429877d1f 100644
--- a/drivers/media/pci/cx23885/cx23885-dvb.c
+++ b/drivers/media/pci/cx23885/cx23885-dvb.c
@@ -2514,8 +2514,8 @@ static int dvb_register(struct cx23885_tsport *port)
fe1->dvb.frontend->ops.ts_bus_ctrl = cx23885_dvb_bus_ctrl;
 #endif

-   /* Put the analog decoder in standby to keep it quiet */
-   call_all(dev, core, s_power, 0);
+   /* Put the tuner in standby to keep it quiet */
+   call_all(dev, tuner, standby);

if (fe0->dvb.frontend->ops.analog_ops.standby)
fe0->dvb.frontend->ops.analog_ops.standby(fe0->dvb.frontend);
diff --git a/drivers/media/pci/cx88/cx88-cards.c 
b/drivers/media/pci/cx88/cx88-cards.c
index 6df21b29ea17..4c92d2388c26 100644
--- a/drivers/media/pci/cx88/cx88-cards.c
+++ b/drivers/media/pci/cx88/cx88-cards.c
@@ -3592,7 +3592,7 @@ static void cx88_card_setup(struct cx88_core *core)
ctl.fname);
call_all(core, tuner, s_config, _cfg);
}
-   call_all(core, core, s_power, 0);
+   call_all(core, tuner, standby);
 }

 /* -- */
diff --git a/drivers/media/pci/cx88/cx88-dvb.c 
b/drivers/media/pci/cx88/cx88-dvb.c
index 49a335f4603e..3abef0b106be 100644
--- a/drivers/media/pci/cx88/cx88-dvb.c
+++ b/drivers/media/pci/cx88/cx88-dvb.c
@@ -1631,8 +1631,8 @@ static int dvb_register(struct cx8802_dev *dev)
if (fe1)
fe1->dvb.frontend->ops.ts_bus_ctrl = cx88_dvb_bus_ctrl;

-   /* Put the analog decoder in standby to keep it quiet */
-   call_all(core, core, s_power, 0);
+   /* Put the tuner in standby to keep it quiet */
+   call_all(core, tuner, standby);

/* register everything */
res = vb2_dvb_register_bus(>frontends, THIS_MODULE, dev,
diff --git a/drivers/media/pci/saa7134/saa7134-video.c 
b/drivers/media/pci/saa7134/saa7134-video.c
index 1ca6a32ad10e..4f1091a11e91 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -1200,7 +1200,7 @@ static int video_release(struct file *file)
saa_andorb(SAA7134_OFMT_DATA_A, 0x1f, 0);
saa_andorb(SAA7134_OFMT_DATA_B, 0x1f, 0);

-   saa_call_all(dev, core, s_power, 0);
+   saa_call_all(dev, tuner, standby);
if (vdev->vfl_type == VFL_TYPE_RADIO)
saa_call_all(dev, core, ioctl, SAA6588_CMD_CLOSE, );
mutex_unlock(>lock);
diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
index 564a000f503e..a17d4853ad40 100644
--- a/drivers/media/tuners/e4000.c
+++ b/drivers/media/tuners/e4000.c
@@ -293,28 +293,19 @@ static inline struct e4000_dev 
*e4000_subdev_to_dev(struct v4l2_subdev *sd)
return container_of(sd, struct e4000_dev, sd);
 }

-static int e4000_s_power(struct v4l2_subdev *sd, int on)
+static int e4000_standby(struct v4l2_subdev *sd)
 {
  

Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions

[Marek Szyprowski]

Hi Robin,

On 2018-02-19 17:29, Robin Murphy wrote:

Hi Maciej,

On 19/02/18 15:43, Maciej Purski wrote:

When a driver is going to use clk_bulk_get() function, it has to
initialize an array of clk_bulk_data, by filling its id fields.

Add a new function to the core, which dynamically allocates
clk_bulk_data array and fills its id fields. Add clk_bulk_free()
function, which frees the array allocated by clk_bulk_alloc() function.
Add a managed version of clk_bulk_alloc().


Seeing how every subsequent patch ends up with the roughly this same 
stanza:


x = devm_clk_bulk_alloc(dev, num, names);
if (IS_ERR(x)
    return PTR_ERR(x);
ret = devm_clk_bulk_get(dev, x, num);

I wonder if it might be better to simply implement:

int devm_clk_bulk_alloc_get(dev, , num, names)

that does the whole lot in one go, and let drivers that want to do 
more fiddly things continue to open-code the allocation.


But perhaps that's an abstraction too far... I'm not all that familiar 
with the lie of the land here.


Hmmm. This patchset clearly shows, that it would be much simpler if we
get rid of clk_bulk_data structure at all and let clk_bulk_* functions
to operate on struct clk *array[]. Typically the list of clock names
is already in some kind of array (taken from driver data or statically
embedded into driver), so there is little benefit from duplicating it
in clk_bulk_data. Sadly, I missed clk_bulk_* api discussion and maybe
there are other benefits from this approach.

If not, I suggest simplifying clk_bulk_* api by dropping clk_bulk_data
structure and switching to clock ptr array:

int clk_bulk_get(struct device *dev, int num_clock, struct clk *clocks[],
             const char *clk_names[]);
int clk_bulk_prepare(int num_clks, struct clk *clks[]);
int clk_bulk_enable(int num_clks, struct clk *clks[]);
...






Signed-off-by: Maciej Purski 
---
  drivers/clk/clk-bulk.c   | 16 
  drivers/clk/clk-devres.c | 37 +---
  include/linux/clk.h  | 64 


  3 files changed, 113 insertions(+), 4 deletions(-)



[...]
@@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char *dev_id, 
const char *con_id);

    #else /* !CONFIG_HAVE_CLK */
  +static inline struct clk_bulk_data *clk_bulk_alloc(int num_clks,
+   const char **clk_ids)
+{
+    return NULL;


Either way, is it intentional not returning an ERR_PTR() value in this 
case? Since NULL will pass an IS_ERR() check, it seems a bit fragile 
for an allocation call to apparently succeed when the whole API is 
configured out (and I believe introducing new uses of IS_ERR_OR_NULL() 
is in general strongly discouraged.)


Robin.


+}
+
+static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct 
device *dev,

+    int num_clks,
+    const char **clk_ids)
+{
+    return NULL;
+}
+
+static inline void clk_bulk_free(struct clk_bulk_data *clks)
+{
+}
+
  static inline struct clk *clk_get(struct device *dev, const char *id)
  {
  return NULL;



Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland

[GIT PULL v2 FOR v4.17] media: replace g/s_parm by g/s_frame_interval

[Hans Verkuil]

There are currently two subdev ops variants to get/set the frame interval:
g/s_parm and g/s_frame_interval.

This patch series replaces all g/s_parm calls by g/s_frame_interval.

The first patch clears the reserved fields in v4l2-subdev.c. It's taken
from the "Media Controller compliance fixes" series.

The second patch adds helper functions that can be used by bridge drivers.
Only em28xx can't use it and it needs custom code (it uses v4l2_device_call()
to try all subdevs instead of calling a specific subdev).

The next patch converts all non-staging drivers, then come Sakari's
atomisp staging fixes.

The v4l2-subdev.h patch removes the now obsolete g/s_parm ops and the
final patch clarifies the documentation a bit (the core allows for
_MPLANE to be used as well).

I would really like to take the next step and introduce two new ioctls
VIDIOC_G/S_FRAME_INTERVAL (just like the SUBDEV variants that already
exist) and convert all bridge drivers to use that and just have helper
functions in the core for VIDIOC_G/S_PARM.

I hate that ioctl and it always confuses driver developers. It would
also prevent the type of abuse that was present in the atomisp driver.

But that's for later, let's simplify the subdev drivers first.

Regards,

Hans

Changes since the previous pull request:

- added patch 1 (v4l2-subdev: clear reserved fields). This is the same
  patch as is included in the "Media Controller compliance fixes" series.
- because of patch 1 we can now drop the memsets in patch 3 as suggested
  by Mauro.
- updated commit message of patch "staging: atomisp: Kill subdev s_parm abuse"
- updated patch "staging: atomisp: i2c: Disable non-preview configurations"
  to the 2.1 version posted by Sakari, with a slight rewording of the commit
  message.


The following changes since commit 29422737017b866d4a51014cc7522fa3a99e8852:

  media: rc: get start time just before calling driver tx (2018-02-14 14:17:21 
-0500)

are available in the git repository at:

  git://linuxtv.org/hverkuil/media_tree.git parm

for you to fetch changes up to e26ba313fbb2d1b9eca813b490869d22f76b6eb5:

  vidioc-g-parm.rst: also allow _MPLANE buffer types (2018-02-19 15:00:48 +0100)


Hans Verkuil (5):
  v4l2-subdev: clear reserved fields
  v4l2-common: create v4l2_g/s_parm_cap helpers
  media: convert g/s_parm to g/s_frame_interval in subdevs
  v4l2-subdev.h: remove obsolete g/s_parm
  vidioc-g-parm.rst: also allow _MPLANE buffer types

Sakari Ailus (5):
  staging: atomisp: Kill subdev s_parm abuse
  staging: atomisp: i2c: Disable non-preview configurations
  staging: atomisp: i2c: Drop g_parm support in sensor drivers
  staging: atomisp: mt9m114: Drop empty s_parm callback
  staging: atomisp: Drop g_parm and s_parm subdev ops use

 Documentation/media/uapi/v4l/vidioc-g-parm.rst  |  7 ++---
 drivers/media/i2c/mt9v011.c | 29 
+++--
 drivers/media/i2c/ov6650.c  | 33 

 drivers/media/i2c/ov7670.c  | 22 
++--
 drivers/media/i2c/ov7740.c  | 29 
++---
 drivers/media/i2c/tvp514x.c | 37 
+--
 drivers/media/i2c/vs6624.c  | 27 
+++-
 drivers/media/platform/atmel/atmel-isc.c| 10 ++--
 drivers/media/platform/atmel/atmel-isi.c| 12 ++---
 drivers/media/platform/blackfin/bfin_capture.c  | 14 +++---
 drivers/media/platform/marvell-ccic/mcam-core.c | 12 -
 drivers/media/platform/soc_camera/soc_camera.c  | 10 +---
 drivers/media/platform/via-camera.c |  4 +--
 drivers/media/usb/em28xx/em28xx-video.c | 36 
++
 drivers/media/v4l2-core/v4l2-common.c   | 48 
++
 drivers/media/v4l2-core/v4l2-subdev.c   | 13 ++
 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c  | 53 
--
 drivers/staging/media/atomisp/i2c/atomisp-gc2235.c  | 53 
--
 drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c |  6 -
 drivers/staging/media/atomisp/i2c/atomisp-ov2680.c  | 56 

 drivers/staging/media/atomisp/i2c/atomisp-ov2722.c  | 53 
--
 drivers/staging/media/atomisp/i2c/gc0310.h  | 43 
---
 drivers/staging/media/atomisp/i2c/gc2235.h  |  7 -
 drivers/staging/media/atomisp/i2c/ov2680.h  | 68 
-
 

Re: [RFC PATCH] Add core tuner_standby op, use where needed

[Hans Verkuil]

On 02/19/18 20:27, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Monday, 19 February 2018 15:12:05 EET Hans Verkuil wrote:
>> The v4l2_subdev core s_power op was used to two different things: power
>> on/off sensors or video decoders/encoders and to put a tuner in standby
>> (and only the tuner). There is no 'tuner wakeup' op, that's done
>> automatically when the tuner is accessed.
>>
>> The danger with calling (s_power, 0) to put a tuner into standby is that it
>> is usually broadcast for all subdevs. So a video receiver subdev that also
>> supports s_power will also be powered off, and since there is no
>> corresponding (s_power, 1) they will never be powered on again.
>>
>> In addition, this is specifically meant for tuners only since they draw the
>> most current.
>>
>> This patch adds a new core op called 'tuner_standby' and replaces all calls
>> to (s_power, 0) by tuner_standby. This prevents confusion between the two
>> uses of s_power. Note that there is no overlap: bridge drivers either just
>> want to put the tuner into standby, or they deal with powering on/off
>> sensors. Never both.
>>
>> This also makes it easier to replace s_power for the remaining bridge
>> drivers with some PM code later.
>>
>> Whether we want something similar for tuners in the future is a separate
>> topic. There is a lot of legacy code surrounding tuners, and I am very
>> hesitant about making changes there.
> 
> While I don't request you to make changes, someone should. I assume the 
> tuners 
> are still maintained, aren't they ? :-)

Theoretically. DVB tuners are certainly maintained, but analog TV? I haven't
seen patches for such tuners in years.

>> Signed-off-by: Hans Verkuil 
>> ---
> 
> [snip]
>  
>>  static const struct v4l2_subdev_tuner_ops tuner_tuner_ops = {
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 980a86c08fce..b214da92a5c0 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -184,6 +184,9 @@ struct v4l2_subdev_io_pin_config {
>>   * @s_power: puts subdevice in power saving mode (on == 0) or normal
>> operation *  mode (on == 1).
>>   *
>> + * @tuner_standby: puts the tuner in standby mode. It will be woken up
>> + *  automatically the next time it is used.
>> + *
>>   * @interrupt_service_routine: Called by the bridge chip's interrupt
>> service
>>   *  handler, when an interrupt status has be raised due to this subdev,
>>   *  so that this subdev can handle the details.  It may schedule work to be
>> @@ -212,6 +215,7 @@ struct v4l2_subdev_core_ops {
>>  int (*s_register)(struct v4l2_subdev *sd, const struct v4l2_dbg_register
>> *reg); #endif
>>  int (*s_power)(struct v4l2_subdev *sd, int on);
>> +int (*tuner_standby)(struct v4l2_subdev *sd);
> 
> If it's a tuner operation, how about moving it to v4l2_subdev_tuner_ops ? 
> That 
> would at least make it clear that it shouldn't be used by other drivers (and 
> I 
> think we should also mention in the documentation that this is a legacy 
> operation that shouldn't be used for any new purpose).

That makes a lot of sense. I'll move it.

> 
>>  int (*interrupt_service_routine)(struct v4l2_subdev *sd,
>>  u32 status, bool *handled);
>>  int (*subscribe_event)(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> 
> I'd prefer the bridge drivers to be fixed to use s_power in a balanced way, 
> but I understand that it might be difficult to achieve in a timely fashion. 
> I'm thus not against this patch, but I don't think it makes too much sense to 
> merge it without a user, that is a patch series that works on removing 
> s_power.
> 

I actually think it is a nice cleanup/clarification regardless of removing
s_power. The s_power op was really abused for tuners. And let's be honest:
having s_power(0) and never s_power(1) was very confusing!

So I believe that this can go in regardless of the other work.

Regards,

Hans

Re: [PATCH v9 11/11] media: i2c: ov7670: Fully set mbus frame fmt

[jacopo mondi]

Hi Laurent,

On Mon, Feb 19, 2018 at 09:19:32PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Monday, 19 February 2018 18:59:44 EET Jacopo Mondi wrote:
> > The sensor driver sets mbus format colorspace information and sizes,
> > but not ycbcr encoding, quantization and xfer function. When supplied
> > with an badly initialized mbus frame format structure, those fields
> > need to be set explicitly not to leave them uninitialized. This is
> > tested by v4l2-compliance, which supplies a mbus format description
> > structure and checks for all fields to be properly set.
> >
> > Without this commit, v4l2-compliance fails when testing formats with:
> > fail: v4l2-test-formats.cpp(335): ycbcr_enc >= 0xff
> >
> > Signed-off-by: Jacopo Mondi 
> > ---
> >  drivers/media/i2c/ov7670.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> > index 25b26d4..61c472e 100644
> > --- a/drivers/media/i2c/ov7670.c
> > +++ b/drivers/media/i2c/ov7670.c
> > @@ -996,6 +996,10 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev
> > *sd, fmt->height = wsize->height;
> > fmt->colorspace = ov7670_formats[index].colorspace;
>
> On a side note, if I'm not mistaken the colorspace field is set to SRGB for
> all entries. Shouldn't you hardcode it here and remove the field ?
>
> > +   fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > +   fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> > +   fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;
>
> How about setting the values explicitly instead of relying on defaults ? That
> would be V4L2_YCBCR_ENC_601, V4L2_QUANTIZATION_LIM_RANGE and
> V4L2_XFER_FUNC_SRGB. And could you then check a captured frame to see if the
> sensor outputs limited or full range ?
>

This actually makes me wonder if those informations (ycbcb_enc,
quantization and xfer_func) shouldn't actually be part of the
supported format list... I blindly added those default fields in the
try_fmt function, but I doubt they applies to all supported formats.

Eg. the sensor supports YUYV as well as 2 RGB encodings (RGB444 and
RGB 565) and 1 raw format (BGGR). I now have a question here:

1) ycbcr_enc transforms non-linear R'G'B' to Y'CbCr: does this
applies to RGB and raw formats? I don't think so, and what value is
the correct one for the ycbcr_enc field in this case? I assume
xfer_func and quantization applies to all formats instead..

Thanks
   j

> > info->format = *fmt;
> >
> > return 0;
>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v2] videodev2.h: add helper to validate colorspace

[Hans Verkuil]

Hi Niklas,

On 02/19/2018 11:28 PM, Niklas Söderlund wrote:
> Hi Hans,
> 
> Thanks for your feedback.
> 
> [snip]
> 
>> Can you then fix v4l2-compliance to stop testing colorspace 
>> against 0xff
>> ?
>
> For now I can simply relax this test for subdevs with sources and sinks.

 You also need to relax it for video nodes with MC drivers, as the DMA
 engines don't care about colorspaces.
>>>
>>> Yes, they do. Many DMA engines can at least do RGB <-> YUV conversions, so
>>> they should get the colorspace info from their source and pass it on to
>>> userspace (after correcting for any conversions done by the DMA engine).
>>
>> Not in the MC case. Video nodes there only model the DMA engine, and are 
>> thus 
>> not aware of colorspaces. What MC drivers do is check at stream on time when 
>> validating the pipeline that the colorspace set by userspace on the video 
>> node 
>> corresponds to the colorspace on the source pad of the connected subdev, but 
>> that's only to ensure that userspace gets a coherent view of colorspace 
>> across 
>> the pipeline, not to program the hardware. There could be exceptions, but in 
>> the general case, the video node implementation of an MC driver will accept 
>> any colorspace and only validate it at stream on time, similarly to how it 
>> does for the frame size format instance (and in the frame size case it will 
>> usually enforce min/max limits when the DMA engine limits the frame size).
> 
> I'm afraid the issue described above by Laurent is what sparked me to 
> write this commit to begin with. In my never ending VIN Gen3 patch-set I 
> currency need to carry a patch [1] to implement a hack to make sure 
> v4l2-compliance do not fail for the VIN Gen3 MC-centric use-case. This 
> patch was an attempt to be able to validate the colorspace using the 
> magic value 0xff.

This is NOT a magic value. The test that's done here is to memset the
format structure with 0xff, then call the ioctl. Afterwards it checks
if there are any remaining 0xff bytes left in the struct since it expects
the driver to have overwritten it by something else. That's where the 0xff
comes from.

> 
> I don't feel strongly for this patch in particular and I'm happy to drop 
> it.  But I would like to receive some guidance on how to then properly 
> be able to handle this problem for the MC-centric VIN driver use-case.  
> One option is as you suggested to relax the test in v4l-compliance to 
> not check colorspace, but commit [2] is not enough to resolve the issue 
> for my MC use-case.
> 
> As Laurent stated above, the use-case is that the video device shall 
> accept any colorspace set from user-space. This colorspace is then only 
> used as stream on time to validate the MC pipeline. The VIN driver do 
> not care about colorspace, but I care about not breaking v4l2-compliance 
> as I find it's a very useful tool :-)

I think part of my confusion here is that there are two places where you
deal with colorspaces in a DMA engine: first there is a input pad of the
DMA engine entity, secondly there is the v4l2_pix_format for the memory
description.

The second is set by the driver based on what userspace specified for the
input pad, together with any changes due to additional conversions such
as quantization range and RGB <-> YUV by the DMA engine.

So any colorspace validation is done for the input pad. The question is
what that validation should be. It's never been defined.

Also the handling of COLORSPACE_DEFAULT for pad formats needs to be defined.

This is not the first time this cropped up, see e.g. this RFC patch:

https://patchwork.linuxtv.org/patch/41734/

> I'm basing the following on the latest v4l-utils master 
> (4665ab1fbab1ddaa)  which contains commit [2]. The core issue is that if 
> I do not have a patch like [1] the v4l2-compliance run fails for format 
> ioctls:
> 
> Format ioctls (Input 0):
>   test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
>   test VIDIOC_G/S_PARM: OK (Not Supported)
>   test VIDIOC_G_FBUF: OK (Not Supported)
>   fail: v4l2-test-formats.cpp(330): !colorspace
>   fail: v4l2-test-formats.cpp(439): 
> testColorspace(pix.pixelformat, pix.colorspace, pix.ycbcr_enc, 
> pix.quantization)
>   test VIDIOC_G_FMT: FAIL
>   test VIDIOC_TRY_FMT: OK (Not Supported)
>   test VIDIOC_S_FMT: OK (Not Supported)
>   test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
>   test Cropping: OK (Not Supported)
>   test Composing: OK (Not Supported)
>   test Scaling: OK
> 
> Well that is OK as that fails when colorspace is V4L2_COLORSPACE_DEFAULT 
> and that is not valid. If I instead of reverting [1] only test for 
> V4L2_COLORSPACE_DEFAULT which would not require this patch to implement:
> 
> -   if (!pix->colorspace || pix->colorspace >= 0xff)
> +   if (pix->colorspace == V4L2_COLORSPACE_DEFAULT)
> 
> I still fail for the format ioctls:
> 
> Format ioctls (Input 0):
>   test 

Re: [PATCH v2] [media] Use common error handling code in 20 functions

[Todor Tomov]

Hi Markus,

Thank you for the patch.

On 19.02.2018 20:11, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Mon, 19 Feb 2018 18:50:40 +0100
> 
> Adjust jump targets so that a bit of exception handling can be better
> reused at the end of these functions.
> 
> This issue was partly detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring 
> ---
> 
> v2:
> Hans Verkuil insisted on patch squashing. Thus several changes
> were recombined based on source files from Linux next-20180216.
> 
> The implementation of the function "tda8261_set_params" was improved
> after a notification by Christoph Böhmwalder on 2017-09-26.
> 
>  drivers/media/dvb-core/dmxdev.c| 16 
>  drivers/media/dvb-frontends/tda1004x.c | 20 ++
>  drivers/media/dvb-frontends/tda8261.c  | 19 ++
>  drivers/media/pci/bt8xx/dst.c  | 19 ++
>  drivers/media/pci/bt8xx/dst_ca.c   | 30 +++
>  drivers/media/pci/cx88/cx88-input.c| 17 +
>  drivers/media/platform/omap3isp/ispvideo.c | 29 +++
>  .../media/platform/qcom/camss-8x16/camss-csid.c| 20 +-
>  drivers/media/tuners/tuner-xc2028.c| 30 +++
>  drivers/media/usb/cpia2/cpia2_usb.c| 13 ---
>  drivers/media/usb/gspca/gspca.c| 17 +
>  drivers/media/usb/gspca/sn9c20x.c  | 17 +
>  drivers/media/usb/pvrusb2/pvrusb2-ioread.c | 10 +++--
>  drivers/media/usb/tm6000/tm6000-cards.c|  7 ++--
>  drivers/media/usb/tm6000/tm6000-dvb.c  | 11 --
>  drivers/media/usb/tm6000/tm6000-video.c| 13 ---
>  drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c  | 13 +++
>  drivers/media/usb/ttusb-dec/ttusb_dec.c| 43 
> --
>  drivers/media/usb/uvc/uvc_v4l2.c   | 13 ---
>  19 files changed, 180 insertions(+), 177 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dmxdev.c b/drivers/media/dvb-core/dmxdev.c
> index 6d53af00190e..6a0411c91195 100644



> diff --git a/drivers/media/platform/qcom/camss-8x16/camss-csid.c 
> b/drivers/media/platform/qcom/camss-8x16/camss-csid.c
> index 64df82817de3..92d4dc6b4a66 100644
> --- a/drivers/media/platform/qcom/camss-8x16/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss-8x16/camss-csid.c
> @@ -328,16 +328,12 @@ static int csid_set_power(struct v4l2_subdev *sd, int 
> on)
>   return ret;
>  
>   ret = csid_set_clock_rates(csid);
> - if (ret < 0) {
> - regulator_disable(csid->vdda);
> - return ret;
> - }
> + if (ret < 0)
> + goto disable_regulator;
>  
>   ret = camss_enable_clocks(csid->nclocks, csid->clock, dev);
> - if (ret < 0) {
> - regulator_disable(csid->vdda);
> - return ret;
> - }
> + if (ret < 0)
> + goto disable_regulator;
>  
>   enable_irq(csid->irq);
>  
> @@ -345,8 +341,7 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
>   if (ret < 0) {
>   disable_irq(csid->irq);
>   camss_disable_clocks(csid->nclocks, csid->clock);
> - regulator_disable(csid->vdda);
> - return ret;
> + goto disable_regulator;
>   }
>  
>   hw_version = readl_relaxed(csid->base + CAMSS_CSID_HW_VERSION);
> @@ -357,6 +352,11 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
>   ret = regulator_disable(csid->vdda);
>   }
>  
> + goto exit;

I think it will be cleaner if you remove the exit label and return here instead.

> +
> +disable_regulator:
> + regulator_disable(csid->vdda);
> +exit:
>   return ret;
>  }

-- 
Best regards,
Todor Tomov

Re: [RFCv4 09/21] v4l2: add request API support

[Alexandre Courbot]

Hi Philippe,

On Tue, Feb 20, 2018 at 4:36 PM, Philippe Ombredanne
 wrote:
> On Tue, Feb 20, 2018 at 5:44 AM, Alexandre Courbot
>  wrote:
>> Add a v4l2 request entity data structure that takes care of storing the
>> request-related state of a V4L2 device ; in this case, its controls.
>>
>> Signed-off-by: Alexandre Courbot 
>
> 
>
>> --- /dev/null
>> +++ b/drivers/media/v4l2-core/v4l2-request.c
>> @@ -0,0 +1,178 @@
>> +/*
>> + * Media requests support for V4L2
>> + *
>> + * Copyright (C) 2018, 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.
>> + */
>
> Do you mind using SPDX tags per [1] rather that this fine but long
> legalese. (Here and in the whole patch series)
>
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/license-rules.rst

I need to check what is the stance of Chromium OS regarding these, but
will gladly comply if possible.