Re: [PATCH v2] dw9714: Initial driver for dw9714 VCM

[Tomasz Figa]

Hi Raj,

On Thu, May 11, 2017 at 12:12 PM, Mani, Rajmohan
 wrote:
> Hi Tomasz,
> Thanks for the reviews. Please see comments inline.
>
>> -Original Message-
>> From: Tomasz Figa [mailto:tf...@chromium.org]
>> Sent: Tuesday, May 09, 2017 4:44 PM
>> To: Mani, Rajmohan 
>> Cc: linux-media@vger.kernel.org; mche...@kernel.org; Hans Verkuil
>> 
>> Subject: Re: [PATCH v2] dw9714: Initial driver for dw9714 VCM
>>
>> Hi Rajmohan,
>>
>> Some comments below.
>>
>> On Mon, May 8, 2017 at 10:36 PM, Rajmohan Mani
>>  wrote:
>> > DW9714 is a 10 bit DAC, designed for linear control of voice coil
>> > motor.
>> >
>> > This driver creates a V4L2 subdevice and provides control to set the
>> > desired focus.
>> >
>> > Signed-off-by: Rajmohan Mani 
>> > ---
>> > Changes in v2:
>> > - Addressed review comments from Hans Verkuil
>> > - Fixed a debug message typo
>> > - Got rid of a return variable
>> > ---
>> >  drivers/media/i2c/Kconfig  |   9 ++
>> >  drivers/media/i2c/Makefile |   1 +
>> >  drivers/media/i2c/dw9714.c | 320
>> > +
>> >  3 files changed, 330 insertions(+)
>> >  create mode 100644 drivers/media/i2c/dw9714.c
>> [snip]
>> > diff --git a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c
>> > new file mode 100644 index 000..cd6cde7
>> > --- /dev/null
>> > +++ b/drivers/media/i2c/dw9714.c
>> > @@ -0,0 +1,320 @@
>> > +/*
>> > + * 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.
>> > + */
>> > +
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +
>> > +#define DW9714_NAME"dw9714"
>> > +#define DW9714_MAX_FOCUS_POS   1023
>> > +#define DW9714_CTRL_STEPS  16  /* Keep this value power of 2 */
>>
>> Because?
>>
>
> This acts as the minimum granularity of lens movement.
> Keep this value power of 2, so the control steps can be uniformly adjusted 
> for gradual lens movement, with desired number of control steps.

I mean, the comment should explain the reason.

>
>> > +#define DW9714_CTRL_DELAY_US   1000
>> > +/*
>> > + * S[3:2] = 0x00, codes per step for "Linear Slope Control"
>> > + * S[1:0] = 0x00, step period
>> > + */
>> > +#define DW9714_DEFAULT_S 0x0
>> > +#define DW9714_VAL(data, s) (u16)((data) << 4 | (s))
>>
>> Do we need this cast?
>>
> Yes. This is a write to a 2 byte register

Still, I'm not sure what this cast really gives us. If we want strict
type checking we should make this a static inline that has all types
specified, but that's probably not necessary either.

Best regards,
Tomasz

[PATCH v4] dw9714: Initial driver for dw9714 VCM

[Rajmohan Mani]

DW9714 is a 10 bit DAC, designed for linear
control of voice coil motor.

This driver creates a V4L2 subdevice and
provides control to set the desired focus.

Signed-off-by: Rajmohan Mani 
---
Changes in v4:
- Addressed review comments from Tomasz
Changes in v3:
- Addressed most of the review comments from Sakari
  on v1 of this patch
Changes in v2:
- Addressed review comments from Hans Verkuil
- Fixed a debug message typo
- Got rid of a return variable
---
 drivers/media/i2c/Kconfig  |   9 ++
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/dw9714.c | 332 +
 3 files changed, 342 insertions(+)
 create mode 100644 drivers/media/i2c/dw9714.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index fd181c9..516e2f2 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -300,6 +300,15 @@ config VIDEO_AD5820
  This is a driver for the AD5820 camera lens voice coil.
  It is used for example in Nokia N900 (RX-51).
 
+config VIDEO_DW9714
+   tristate "DW9714 lens voice coil support"
+   depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER && 
VIDEO_V4L2_SUBDEV_API
+   ---help---
+ This is a driver for the DW9714 camera lens voice coil.
+ DW9714 is a 10 bit DAC with 120mA output current sink
+ capability. This is designed for linear control of
+ voice coil motors, controlled via I2C serial interface.
+
 config VIDEO_SAA7110
tristate "Philips SAA7110 video decoder"
depends on VIDEO_V4L2 && I2C
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 62323ec..987bd1f 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
 obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
 obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
 obj-$(CONFIG_VIDEO_AD5820)  += ad5820.o
+obj-$(CONFIG_VIDEO_DW9714)  += dw9714.o
 obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
 obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
 obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o
diff --git a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c
new file mode 100644
index 000..fefd5d2
--- /dev/null
+++ b/drivers/media/i2c/dw9714.c
@@ -0,0 +1,332 @@
+/*
+ * 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.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DW9714_NAME"dw9714"
+#define DW9714_MAX_FOCUS_POS   1023
+/*
+ * This acts as the minimum granularity of lens movement.
+ * Keep this value power of 2, so the control steps can be
+ * uniformly adjusted for gradual lens movement, with desired
+ * number of control steps.
+ */
+#define DW9714_CTRL_STEPS  16
+#define DW9714_CTRL_DELAY_US   1000
+/*
+ * S[3:2] = 0x00, codes per step for "Linear Slope Control"
+ * S[1:0] = 0x00, step period
+ */
+#define DW9714_DEFAULT_S 0x0
+#define DW9714_VAL(data, s) (u16)((data) << 4 | (s))
+
+/* dw9714 device structure */
+struct dw9714_device {
+   struct i2c_client *client;
+   struct v4l2_ctrl_handler ctrls_vcm;
+   struct v4l2_subdev sd;
+   u16 current_val;
+};
+
+static inline struct dw9714_device *to_dw9714_vcm(struct v4l2_ctrl *ctrl)
+{
+   return container_of(ctrl->handler, struct dw9714_device, ctrls_vcm);
+}
+
+static int dw9714_i2c_write(struct i2c_client *client, u16 data)
+{
+   int ret;
+   u16 val = cpu_to_be16(data);
+   const int num_bytes = sizeof(val);
+
+   ret = i2c_master_send(client, (const char *) , sizeof(val));
+
+   /*One retry */
+   if (ret != num_bytes)
+   ret = i2c_master_send(client, (const char *) , sizeof(val));
+
+   if (ret != num_bytes) {
+   dev_err(>dev, "I2C write fail\n");
+   return -EIO;
+   }
+   return 0;
+}
+
+static int dw9714_t_focus_vcm(struct dw9714_device *dw9714_dev, u16 val)
+{
+   struct i2c_client *client = dw9714_dev->client;
+
+   dw9714_dev->current_val = val;
+
+   return dw9714_i2c_write(client, DW9714_VAL(val, DW9714_DEFAULT_S));
+}
+
+static int dw9714_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+   struct dw9714_device *dev_vcm = to_dw9714_vcm(ctrl);
+
+   if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE)
+   return dw9714_t_focus_vcm(dev_vcm, ctrl->val);
+
+   return -EINVAL;
+}
+
+static const struct v4l2_ctrl_ops dw9714_vcm_ctrl_ops = {
+   .s_ctrl = dw9714_set_ctrl,
+};
+
+static int 

cron job: media_tree daily build: WARNINGS

[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:   Thu May 11 05:00:34 CEST 2017
media-tree git hash:3622d3e77ecef090b5111e3c5423313f11711dfa
media_build git hash:   ab988a3d089232ce9e1aec2f259e947c06983dbc
v4l-utils git hash: 6acac5cec698de39b9398b66c4f5f4db6b2730d8
gcc version:i686-linux-gcc (GCC) 7.1.0
sparse version: v0.5.0-3553-g78b2ea6
smatch version: v0.5.0-3553-g78b2ea6
host hardware:  x86_64
host os:4.9.0-164

linux-git-arm-at91: WARNINGS
linux-git-arm-davinci: WARNINGS
linux-git-arm-multi: WARNINGS
linux-git-arm-pxa: OK
linux-git-blackfin-bf561: OK
linux-git-i686: WARNINGS
linux-git-m32r: OK
linux-git-mips: WARNINGS
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: WARNINGS
linux-2.6.36.4-i686: WARNINGS
linux-2.6.37.6-i686: WARNINGS
linux-2.6.38.8-i686: WARNINGS
linux-2.6.39.4-i686: WARNINGS
linux-3.0.60-i686: WARNINGS
linux-3.1.10-i686: WARNINGS
linux-3.2.37-i686: WARNINGS
linux-3.3.8-i686: WARNINGS
linux-3.4.27-i686: WARNINGS
linux-3.5.7-i686: WARNINGS
linux-3.6.11-i686: WARNINGS
linux-3.7.4-i686: WARNINGS
linux-3.8-i686: WARNINGS
linux-3.9.2-i686: WARNINGS
linux-3.10.1-i686: WARNINGS
linux-3.11.1-i686: WARNINGS
linux-3.12.67-i686: WARNINGS
linux-3.13.11-i686: WARNINGS
linux-3.14.9-i686: WARNINGS
linux-3.15.2-i686: WARNINGS
linux-3.16.7-i686: WARNINGS
linux-3.17.8-i686: WARNINGS
linux-3.18.7-i686: WARNINGS
linux-3.19-i686: WARNINGS
linux-4.0.9-i686: WARNINGS
linux-4.1.33-i686: WARNINGS
linux-4.2.8-i686: WARNINGS
linux-4.3.6-i686: WARNINGS
linux-4.4.22-i686: WARNINGS
linux-4.5.7-i686: WARNINGS
linux-4.6.7-i686: WARNINGS
linux-4.7.5-i686: WARNINGS
linux-4.8-i686: WARNINGS
linux-4.9.26-i686: WARNINGS
linux-4.10.14-i686: WARNINGS
linux-4.11-i686: WARNINGS
linux-2.6.36.4-x86_64: WARNINGS
linux-2.6.37.6-x86_64: WARNINGS
linux-2.6.38.8-x86_64: WARNINGS
linux-2.6.39.4-x86_64: WARNINGS
linux-3.0.60-x86_64: WARNINGS
linux-3.1.10-x86_64: WARNINGS
linux-3.2.37-x86_64: WARNINGS
linux-3.3.8-x86_64: WARNINGS
linux-3.4.27-x86_64: WARNINGS
linux-3.5.7-x86_64: WARNINGS
linux-3.6.11-x86_64: WARNINGS
linux-3.7.4-x86_64: WARNINGS
linux-3.8-x86_64: WARNINGS
linux-3.9.2-x86_64: WARNINGS
linux-3.10.1-x86_64: WARNINGS
linux-3.11.1-x86_64: WARNINGS
linux-3.12.67-x86_64: WARNINGS
linux-3.13.11-x86_64: WARNINGS
linux-3.14.9-x86_64: WARNINGS
linux-3.15.2-x86_64: WARNINGS
linux-3.16.7-x86_64: WARNINGS
linux-3.17.8-x86_64: WARNINGS
linux-3.18.7-x86_64: WARNINGS
linux-3.19-x86_64: WARNINGS
linux-4.0.9-x86_64: WARNINGS
linux-4.1.33-x86_64: WARNINGS
linux-4.2.8-x86_64: WARNINGS
linux-4.3.6-x86_64: WARNINGS
linux-4.4.22-x86_64: WARNINGS
linux-4.5.7-x86_64: WARNINGS
linux-4.6.7-x86_64: WARNINGS
linux-4.7.5-x86_64: WARNINGS
linux-4.8-x86_64: WARNINGS
linux-4.9.26-x86_64: WARNINGS
linux-4.10.14-x86_64: WARNINGS
linux-4.11-x86_64: WARNINGS
apps: WARNINGS
spec-git: OK
sparse: WARNINGS

Detailed results are available here:

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

Full logs are available here:

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

The Media Infrastructure API from this daily build is here:

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

RE: [PATCH v2] dw9714: Initial driver for dw9714 VCM

[Mani, Rajmohan]

Hi Tomasz,
Thanks for the reviews. Please see comments inline.

> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Tuesday, May 09, 2017 4:44 PM
> To: Mani, Rajmohan 
> Cc: linux-media@vger.kernel.org; mche...@kernel.org; Hans Verkuil
> 
> Subject: Re: [PATCH v2] dw9714: Initial driver for dw9714 VCM
> 
> Hi Rajmohan,
> 
> Some comments below.
> 
> On Mon, May 8, 2017 at 10:36 PM, Rajmohan Mani
>  wrote:
> > DW9714 is a 10 bit DAC, designed for linear control of voice coil
> > motor.
> >
> > This driver creates a V4L2 subdevice and provides control to set the
> > desired focus.
> >
> > Signed-off-by: Rajmohan Mani 
> > ---
> > Changes in v2:
> > - Addressed review comments from Hans Verkuil
> > - Fixed a debug message typo
> > - Got rid of a return variable
> > ---
> >  drivers/media/i2c/Kconfig  |   9 ++
> >  drivers/media/i2c/Makefile |   1 +
> >  drivers/media/i2c/dw9714.c | 320
> > +
> >  3 files changed, 330 insertions(+)
> >  create mode 100644 drivers/media/i2c/dw9714.c
> [snip]
> > diff --git a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c
> > new file mode 100644 index 000..cd6cde7
> > --- /dev/null
> > +++ b/drivers/media/i2c/dw9714.c
> > @@ -0,0 +1,320 @@
> > +/*
> > + * 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.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define DW9714_NAME"dw9714"
> > +#define DW9714_MAX_FOCUS_POS   1023
> > +#define DW9714_CTRL_STEPS  16  /* Keep this value power of 2 */
> 
> Because?
> 

This acts as the minimum granularity of lens movement.
Keep this value power of 2, so the control steps can be uniformly adjusted for 
gradual lens movement, with desired number of control steps.

> > +#define DW9714_CTRL_DELAY_US   1000
> > +/*
> > + * S[3:2] = 0x00, codes per step for "Linear Slope Control"
> > + * S[1:0] = 0x00, step period
> > + */
> > +#define DW9714_DEFAULT_S 0x0
> > +#define DW9714_VAL(data, s) (u16)((data) << 4 | (s))
> 
> Do we need this cast?
> 
Yes. This is a write to a 2 byte register

> > +
> > +/* dw9714 device structure */
> > +struct dw9714_device {
> > +   struct i2c_client *client;
> > +   struct v4l2_ctrl_handler ctrls_vcm;
> > +   struct v4l2_subdev sd;
> > +   u16 current_val;
> > +};
> > +
> > +#define to_dw9714_vcm(_ctrl)   \
> > +   container_of(_ctrl->handler, struct dw9714_device, ctrls_vcm)
> 
> Please use a static inline function for type safety.
> 

Ack

> > +
> > +static int dw9714_i2c_write(struct i2c_client *client, u16 data) {
> > +   const int num_msg = 1;
> > +   int ret;
> > +   u16 val = cpu_to_be16(data);
> > +   struct i2c_msg msg = {
> > +   .addr = client->addr,
> > +   .flags = 0,
> > +   .len = sizeof(val),
> > +   .buf = (u8 *) ,
> > +   };
> > +
> > +   ret = i2c_transfer(client->adapter, , num_msg);
> > +
> > +   /*One retry */
> > +   if (ret != num_msg)
> > +   ret = i2c_transfer(client->adapter, , num_msg);
> > +
> > +   if (ret != num_msg) {
> > +   dev_err(>dev, "I2C write fail\n");
> > +   return -EIO;
> > +   }
> 
> I believe i2c_master_send() would simplify this function significantly.
> 

Ack

> > +   return 0;
> > +}
> > +
> > +static int dw9714_t_focus_vcm(struct dw9714_device *dw9714_dev, u16
> > +val) {
> > +   struct i2c_client *client = dw9714_dev->client;
> > +
> > +   dev_dbg(>dev, "Setting new value VCM: %d\n", val);
> > +   dw9714_dev->current_val = val;
> > +
> > +   return dw9714_i2c_write(client, DW9714_VAL(val,
> > +DW9714_DEFAULT_S)); }
> > +
> > +static int dw9714_set_ctrl(struct v4l2_ctrl *ctrl) {
> > +   struct dw9714_device *dev_vcm = to_dw9714_vcm(ctrl);
> > +
> > +   if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE)
> > +   return dw9714_t_focus_vcm(dev_vcm, ctrl->val);
> > +
> > +   return -EINVAL;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops dw9714_vcm_ctrl_ops = {
> > +   .s_ctrl = dw9714_set_ctrl,
> > +};
> > +
> > +static int dw9714_init_controls(struct dw9714_device *dev_vcm) {
> > +   struct v4l2_ctrl_handler *hdl = _vcm->ctrls_vcm;
> > +   const struct v4l2_ctrl_ops *ops = _vcm_ctrl_ops;
> > +   

Re: [PATCH] v4l2-subdev: Remove of_node

[Sakari Ailus]

Hi Kieran,

Thanks for the patch.

On Wed, May 10, 2017 at 05:45:54PM +0100, Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> With the fwnode implementation, of_node is no longer used.
> 
> Remove it.
> 
> Signed-off-by: Kieran Bingham 
> ---
>  include/media/v4l2-subdev.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 5f1669c45642..a40760174797 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -787,7 +787,6 @@ struct v4l2_subdev_platform_data {
>   *   is attached.
>   * @devnode: subdev device node
>   * @dev: pointer to the physical device, if any
> - * @of_node: The device_node of the subdev, usually the same as dev->of_node.
>   * @fwnode: The fwnode_handle of the subdev, usually the same as
>   *   either dev->of_node->fwnode or dev->fwnode (whichever is non-NULL).
>   * @async_list: Links this subdev to a global subdev_list or @notifier->done
> @@ -820,7 +819,6 @@ struct v4l2_subdev {
>   void *host_priv;
>   struct video_device *devnode;
>   struct device *dev;
> - struct device_node *of_node;
>   struct fwnode_handle *fwnode;
>   struct list_head async_list;
>   struct v4l2_async_subdev *asd;

This is actually the difference my local v4l2-acpi branch and what I have in
my git.linuxtv.org tree. :-) So the change is there, embedded in the same
patch that converts the users.

I'll upload it later tonight.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

[PATCH] v4l2-subdev: Remove of_node

[Kieran Bingham]

From: Kieran Bingham 

With the fwnode implementation, of_node is no longer used.

Remove it.

Signed-off-by: Kieran Bingham 
---
 include/media/v4l2-subdev.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 5f1669c45642..a40760174797 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -787,7 +787,6 @@ struct v4l2_subdev_platform_data {
  * is attached.
  * @devnode: subdev device node
  * @dev: pointer to the physical device, if any
- * @of_node: The device_node of the subdev, usually the same as dev->of_node.
  * @fwnode: The fwnode_handle of the subdev, usually the same as
  * either dev->of_node->fwnode or dev->fwnode (whichever is non-NULL).
  * @async_list: Links this subdev to a global subdev_list or @notifier->done
@@ -820,7 +819,6 @@ struct v4l2_subdev {
void *host_priv;
struct video_device *devnode;
struct device *dev;
-   struct device_node *of_node;
struct fwnode_handle *fwnode;
struct list_head async_list;
struct v4l2_async_subdev *asd;
-- 
2.7.4

Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure

[Ezequiel Garcia]

Hi Krzysztof,

On 10 May 2017 at 06:51, Krzysztof Hałasa  wrote:
> Signed-off-by: Krzysztof Hałasa 
>
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c 
> b/drivers/media/pci/tw686x/tw686x-video.c
> index c3fafa9..d637f47 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -1190,6 +1190,13 @@ int tw686x_video_init(struct tw686x_dev *dev)
> return err;
> }
>
> +   /* Initialize vc->dev and vc->ch for the error path first */
> +   for (ch = 0; ch < max_channels(dev); ch++) {
> +   struct tw686x_video_channel *vc = >video_channels[ch];
> +   vc->dev = dev;
> +   vc->ch = ch;
> +   }
> +

I'm not sure where is the oops this commit fixes, care to explain it to me?

> for (ch = 0; ch < max_channels(dev); ch++) {
> struct tw686x_video_channel *vc = >video_channels[ch];
> struct video_device *vdev;
> @@ -1198,9 +1205,6 @@ int tw686x_video_init(struct tw686x_dev *dev)
> spin_lock_init(>qlock);
> INIT_LIST_HEAD(>vidq_queued);
>
> -   vc->dev = dev;
> -   vc->ch = ch;
> -
> /* default settings */
> err = tw686x_set_standard(vc, V4L2_STD_NTSC);
> if (err)

Thanks,
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 12/16] rcar-vin: allow switch between capturing modes when stalling

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:53 Niklas Söderlund wrote:
> If userspace can't feed the driver with buffers as fast as the driver
> consumes them the driver will stop video capturing and wait for more
> buffers from userspace, the driver is stalled. Once it have been feed
> one or more free buffers it will recover from the stall and resume
> capturing.
> 
> Instead of of continue to capture using the same capture mode as before

s/of of continue/of continuing/

> the stall allow the driver to choose between single and continuous mode
> base on free buffer availability. Do this by stopping capturing when the

s/base/based/

> driver becomes stalled and restart capturing once it continues. By doing
> this the capture mode will be evaluated each time the driver is
> recovering from a stall.
> 
> This behavior is needed to fix a bug where continuous capturing mode is
> used, userspace is about to stop the stream and is waiting for the last
> buffers to be returned from the driver and is not queuing any new
> buffers. In this case the driver becomes stalled when there are only 3
> buffers remaining streaming will never resume since the driver is
> waiting for userspace to feed it more buffers before it can continue
> streaming.  With this fix the driver will then switch to single capture
> mode for the last 3 buffers and a deadlock is avoided. The issue can be
> demonstrated using yavta.
> 
> $ yavta -f RGB565 -s 640x480 -n 4 --capture=10  /dev/video22
> Device /dev/video22 opened.
> Device `R_Car_VIN' on `platform:e6ef1000.video' (driver 'rcar_vin') supports
> video, capture, without mplanes. Video format set: RGB565 (50424752)
> 640x480 (stride 1280) field interlaced buffer size 614400 Video format:
> RGB565 (50424752) 640x480 (stride 1280) field interlaced buffer size 614400
> 4 buffers requested.
> length: 614400 offset: 0 timestamp type/source: mono/EoF
> Buffer 0/0 mapped at address 0xb6cc7000.
> length: 614400 offset: 614400 timestamp type/source: mono/EoF
> Buffer 1/0 mapped at address 0xb6c31000.
> length: 614400 offset: 1228800 timestamp type/source: mono/EoF
> Buffer 2/0 mapped at address 0xb6b9b000.
> length: 614400 offset: 1843200 timestamp type/source: mono/EoF
> Buffer 3/0 mapped at address 0xb6b05000.
> 0 (0) [-] interlaced 0 614400 B 38.240285 38.240303 12.421 fps ts mono/EoF
> 1 (1) [-] interlaced 1 614400 B 38.282329 38.282346 23.785 fps ts mono/EoF
> 2 (2) [-] interlaced 2 614400 B 38.322324 38.322338 25.003 fps ts mono/EoF
> 3 (3) [-] interlaced 3 614400 B 38.362318 38.362333 25.004 fps ts mono/EoF
> 4 (0) [-] interlaced 4 614400 B 38.402313 38.402328 25.003 fps ts mono/EoF
> 5 (1) [-] interlaced 5 614400 B 38.442307 38.442321 25.004 fps ts mono/EoF
> 6 (2) [-] interlaced 6 614400 B 38.482301 38.482316 25.004 fps ts mono/EoF
> 7 (3) [-] interlaced 7 614400 B 38.522295 38.522312 25.004 fps ts mono/EoF
> 8 (0) [-] interlaced 8 614400 B 38.562290 38.562306 25.003 fps ts mono/EoF
> 
> 
> This fix also allow the driver to switch to single capture mode if
> userspace don't feed it buffers fast enough. Or the other way around, if

s/don't/doesn't/

> userspace suddenly feeds the driver buffers faster it can switch to
> continues capturing mode.
> 
> Signed-off-by: Niklas Söderlund 

I have a feeling that the streaming code is a bit fragile, but it doesn't seem 
that this patch is making it worse, so we can rework it later.

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 22 +-
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> f7776592b9a13d41..bd1ccb70ae2bc47e 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -428,6 +428,8 @@ static int rvin_capture_start(struct rvin_dev *vin)
> 
>   rvin_capture_on(vin);
> 
> + vin->state = RUNNING;
> +
>   return 0;
>  }
> 
> @@ -906,7 +908,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
>   struct rvin_dev *vin = data;
>   u32 int_status, vnms;
>   int slot;
> - unsigned int sequence, handled = 0;
> + unsigned int i, sequence, handled = 0;
>   unsigned long flags;
> 
>   spin_lock_irqsave(>qlock, flags);
> @@ -968,8 +970,20 @@ static irqreturn_t rvin_irq(int irq, void *data)
>* the VnMBm registers.
>*/
>   if (vin->continuous) {
> - rvin_capture_off(vin);
> + rvin_capture_stop(vin);
>   vin_dbg(vin, "IRQ %02d: hw not ready stop\n", 
sequence);
> +
> + /* Maybe we can continue in single capture mode */
> + for (i = 0; i < HW_BUFFER_NUM; i++) {
> + if (vin->queue_buf[i]) {
> +  

Re: [PATCH 16/16] rcar-vin: fix bug in pixelformat selection

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:57 Niklas Söderlund wrote:
> If the requested pixelformat is not supported only revert to the current
> pixelformat, do not revert the entire format. Also if the pixelformat
> needs to be reverted the pixel information needs to be fetched once
> more.
> 
> Signed-off-by: Niklas Söderlund 
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> 956092ba6ef9bc6f..27b7733e96afe3e9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -226,9 +226,8 @@ static int __rvin_try_format(struct rvin_dev *vin,
>   if (!info) {
>   vin_dbg(vin, "Format %x not found, keeping %x\n",
>   pix->pixelformat, vin->format.pixelformat);
> - *pix = vin->format;
> - pix->width = rwidth;
> - pix->height = rheight;
> + pix->pixelformat = vin->format.pixelformat;

You should set a fixed default in this case to achieve a more deterministic 
behaviour. You can for instance pick the first entry of, which will also save 
you from calling rvin_format_from_pixel() as you can then replace it with 
_formats[0].

> + info = rvin_format_from_pixel(pix->pixelformat);
>   }
> 
>   /* Always recalculate */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 13/16] rcar-vin: refactor and fold in function after stall handling rework

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:54 Niklas Söderlund wrote:
> With the driver stopping and starting the stream each time the driver is
> stalled rvin_capture_off() can be folded in to the only caller.
> 
> Signed-off-by: Niklas Söderlund 

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++---
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> bd1ccb70ae2bc47e..c5fa176ac9d8cc4a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -396,12 +396,6 @@ static void rvin_capture_on(struct rvin_dev *vin)
>   rvin_write(vin, VNFC_S_FRAME, VNFC_REG);
>  }
> 
> -static void rvin_capture_off(struct rvin_dev *vin)
> -{
> - /* Set continuous & single transfer off */
> - rvin_write(vin, 0, VNFC_REG);
> -}
> -
>  static int rvin_capture_start(struct rvin_dev *vin)
>  {
>   struct rvin_buffer *buf, *node;
> @@ -435,7 +429,8 @@ static int rvin_capture_start(struct rvin_dev *vin)
> 
>  static void rvin_capture_stop(struct rvin_dev *vin)
>  {
> - rvin_capture_off(vin);
> + /* Set continuous & single transfer off */
> + rvin_write(vin, 0, VNFC_REG);
> 
>   /* Disable module */
>   rvin_write(vin, rvin_read(vin, VNMC_REG) & ~VNMC_ME, VNMC_REG);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 15/16] rcar-vin: add missing error check to propagate error

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:56 Niklas Söderlund wrote:
> The return value of __rvin_try_format_source is not checked, add a check
> and propagate the error.
> 
> Signed-off-by: Niklas Söderlund 

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> c40f5bc3e3d26472..956092ba6ef9bc6f 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -208,6 +208,7 @@ static int __rvin_try_format(struct rvin_dev *vin,
>  {
>   const struct rvin_video_format *info;
>   u32 rwidth, rheight, walign;
> + int ret;
> 
>   /* Requested */
>   rwidth = pix->width;
> @@ -235,7 +236,9 @@ static int __rvin_try_format(struct rvin_dev *vin,
>   pix->sizeimage = 0;
> 
>   /* Limit to source capabilities */
> - __rvin_try_format_source(vin, which, pix, source);
> + ret = __rvin_try_format_source(vin, which, pix, source);
> + if (ret)
> + return ret;
> 
>   switch (pix->field) {
>   case V4L2_FIELD_TOP:

-- 
Regards,

Laurent Pinchart

Re: [PATCH 14/16] rcar-vin: make use of video_device_alloc() and video_device_release()

[Laurent Pinchart]

Hi Niklas,

On Tuesday 14 Mar 2017 19:59:55 Niklas Söderlund wrote:
> Make use of the helper functions video_device_alloc() and
> video_device_release() to control the lifetime of the struct
> video_device.

It's nice to see you considering lifetime management issues, but this isn't 
enough. The rvin_release() function accesses the rvin_dev structure, so you 
need to keep this around until all references to the video device have been 
dropped. This patch won't do so.

I would instead keep the video_device instance embedded in rvin_dev, and 
implement a custom release handler that will kfree() the rvin_dev instance. 
You will obviously need to replace devm_kzalloc() with kzalloc() to allocate 
the rvin_dev.

> Signed-off-by: Niklas Söderlund 
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 44 ++
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  2 +-
>  2 files changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> be6f41bf82ac3bc5..c40f5bc3e3d26472 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -489,7 +489,7 @@ static int rvin_enum_input(struct file *file, void
> *priv, i->std = 0;
>   } else {
>   i->capabilities = V4L2_IN_CAP_STD;
> - i->std = vin->vdev.tvnorms;
> + i->std = vin->vdev->tvnorms;
>   }
> 
>   strlcpy(i->name, "Camera", sizeof(i->name));
> @@ -752,8 +752,8 @@ static int rvin_initialize_device(struct file *file)
>   if (ret < 0)
>   return ret;
> 
> - pm_runtime_enable(>vdev.dev);
> - ret = pm_runtime_resume(>vdev.dev);
> + pm_runtime_enable(>vdev->dev);
> + ret = pm_runtime_resume(>vdev->dev);
>   if (ret < 0 && ret != -ENOSYS)
>   goto eresume;
> 
> @@ -771,7 +771,7 @@ static int rvin_initialize_device(struct file *file)
> 
>   return 0;
>  esfmt:
> - pm_runtime_disable(>vdev.dev);
> + pm_runtime_disable(>vdev->dev);
>  eresume:
>   rvin_power_off(vin);
> 
> @@ -823,8 +823,8 @@ static int rvin_release(struct file *file)
>* Then de-initialize hw module.
>*/
>   if (fh_singular) {
> - pm_runtime_suspend(>vdev.dev);
> - pm_runtime_disable(>vdev.dev);
> + pm_runtime_suspend(>vdev->dev);
> + pm_runtime_disable(>vdev->dev);
>   rvin_power_off(vin);
>   }
> 
> @@ -846,13 +846,13 @@ static const struct v4l2_file_operations rvin_fops = {
> void rvin_v4l2_remove(struct rvin_dev *vin)
>  {
>   v4l2_info(>v4l2_dev, "Removing %s\n",
> -   video_device_node_name(>vdev));
> +   video_device_node_name(vin->vdev));
> 
>   /* Checks internaly if handlers have been init or not */
>   v4l2_ctrl_handler_free(>ctrl_handler);
> 
>   /* Checks internaly if vdev have been init or not */
> - video_unregister_device(>vdev);
> + video_unregister_device(vin->vdev);
>  }
> 
>  static void rvin_notify(struct v4l2_subdev *sd,
> @@ -863,7 +863,7 @@ static void rvin_notify(struct v4l2_subdev *sd,
> 
>   switch (notification) {
>   case V4L2_DEVICE_NOTIFY_EVENT:
> - v4l2_event_queue(>vdev, arg);
> + v4l2_event_queue(vin->vdev, arg);
>   break;
>   default:
>   break;
> @@ -872,7 +872,7 @@ static void rvin_notify(struct v4l2_subdev *sd,
> 
>  int rvin_v4l2_probe(struct rvin_dev *vin)
>  {
> - struct video_device *vdev = >vdev;
> + struct video_device *vdev;
>   struct v4l2_subdev *sd = vin_to_source(vin);
>   int ret;
> 
> @@ -880,16 +880,18 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
> 
>   vin->v4l2_dev.notify = rvin_notify;
> 
> - ret = v4l2_subdev_call(sd, video, g_tvnorms, >vdev.tvnorms);
> + vdev = video_device_alloc();
> +
> + ret = v4l2_subdev_call(sd, video, g_tvnorms, >tvnorms);
>   if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
>   return ret;
> 
> - if (vin->vdev.tvnorms == 0) {
> + if (vdev->tvnorms == 0) {
>   /* Disable the STD API if there are no tvnorms defined */
> - v4l2_disable_ioctl(>vdev, VIDIOC_G_STD);
> - v4l2_disable_ioctl(>vdev, VIDIOC_S_STD);
> - v4l2_disable_ioctl(>vdev, VIDIOC_QUERYSTD);
> - v4l2_disable_ioctl(>vdev, VIDIOC_ENUMSTD);
> + v4l2_disable_ioctl(vdev, VIDIOC_G_STD);
> + v4l2_disable_ioctl(vdev, VIDIOC_S_STD);
> + v4l2_disable_ioctl(vdev, VIDIOC_QUERYSTD);
> + v4l2_disable_ioctl(vdev, VIDIOC_ENUMSTD);
>   }
> 
>   /* Add the controls */
> @@ -913,7 +915,7 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
>   vdev->v4l2_dev = >v4l2_dev;
>   vdev->queue = >queue;
>   strlcpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name));
> - vdev->release = 

Re: [PATCH 11/16] rcar-vin: select capture mode based on free buffers

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:52 Niklas Söderlund wrote:
> Instead of selecting single or continuous capture mode based on how many
> buffers userspace intends to give us select capture mode based on number
> of free buffers we can allocate to hardware when the stream is started.
> 
> This change is a prerequisite to enable the driver to switch from
> continuous to single capture mode (or the other way around) when the
> driver is stalled by userspace not feeding it buffers as fast as it
> consumes it.
> 
> Signed-off-by: Niklas Söderlund 

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 31 ---
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> c10d75aa7e71d665..f7776592b9a13d41 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -404,7 +404,21 @@ static void rvin_capture_off(struct rvin_dev *vin)
> 
>  static int rvin_capture_start(struct rvin_dev *vin)
>  {
> - int ret;
> + struct rvin_buffer *buf, *node;
> + int bufs, ret;
> +
> + /* Count number of free buffers */
> + bufs = 0;
> + list_for_each_entry_safe(buf, node, >buf_list, list)
> + bufs++;
> +
> + /* Continuous capture requires more buffers then there are HW slots */
> + vin->continuous = bufs > HW_BUFFER_NUM;
> +
> + if (!rvin_fill_hw(vin)) {
> + vin_err(vin, "HW not ready to start, not enough buffers 
available\n");
> + return -EINVAL;
> + }
> 
>   rvin_crop_scale_comp(vin);
> 
> @@ -1061,22 +1075,7 @@ static int rvin_start_streaming(struct vb2_queue *vq,
> unsigned int count) vin->state = RUNNING;
>   vin->sequence = 0;
> 
> - /* Continuous capture requires more buffers then there are HW slots */
> - vin->continuous = count > HW_BUFFER_NUM;
> -
> - /*
> -  * This should never happen but if we don't have enough
> -  * buffers for HW bail out
> -  */
> - if (!rvin_fill_hw(vin)) {
> - vin_err(vin, "HW not ready to start, not enough buffers 
available\n");
> - ret = -EINVAL;
> - goto out;
> - }
> -
>   ret = rvin_capture_start(vin);
> -out:
> - /* Return all buffers if something went wrong */
>   if (ret) {
>   return_all_buffers(vin, VB2_BUF_STATE_QUEUED);
>   v4l2_subdev_call(sd, video, s_stream, 0);

-- 
Regards,

Laurent Pinchart

Re: [RFC 0/4] Exynos DRM: add Picture Processor extension

[Marek Szyprowski]

Hi Daniel,

On 2017-05-10 09:55, Daniel Vetter wrote:

On Wed, May 10, 2017 at 03:27:02PM +0900, Inki Dae wrote:

Hi Tomasz,

2017년 05월 10일 14:38에 Tomasz Figa 이(가) 쓴 글:

Hi Everyone,

On Wed, May 10, 2017 at 9:24 AM, Inki Dae  wrote:


2017년 04월 26일 07:21에 Sakari Ailus 이(가) 쓴 글:

Hi Marek,

On Thu, Apr 20, 2017 at 01:23:09PM +0200, Marek Szyprowski wrote:

Hi Laurent,

On 2017-04-20 12:25, Laurent Pinchart wrote:

Hi Marek,

(CC'ing Sakari Ailus)

Thank you for the patches.

On Thursday 20 Apr 2017 11:13:36 Marek Szyprowski wrote:

Dear all,

This is an updated proposal for extending EXYNOS DRM API with generic
support for hardware modules, which can be used for processing image data

>from the one memory buffer to another. Typical memory-to-memory operations

are: rotation, scaling, colour space conversion or mix of them. This is a
follow-up of my previous proposal "[RFC 0/2] New feature: Framebuffer
processors", which has been rejected as "not really needed in the DRM
core":
http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg146286.html

In this proposal I moved all the code to Exynos DRM driver, so now this
will be specific only to Exynos DRM. I've also changed the name from
framebuffer processor (fbproc) to picture processor (pp) to avoid confusion
with fbdev API.

Here is a bit more information what picture processors are:

Embedded SoCs are known to have a number of hardware blocks, which perform
such operations. They can be used in paralel to the main GPU module to
offload CPU from processing grapics or video data. One of example use of
such modules is implementing video overlay, which usually requires color
space conversion from NV12 (or similar) to RGB32 color space and scaling to
target window size.

The proposed API is heavily inspired by atomic KMS approach - it is also
based on DRM objects and their properties. A new DRM object is introduced:
picture processor (called pp for convenience). Such objects have a set of
standard DRM properties, which describes the operation to be performed by
respective hardware module. In typical case those properties are a source
fb id and rectangle (x, y, width, height) and destination fb id and
rectangle. Optionally a rotation property can be also specified if
supported by the given hardware. To perform an operation on image data,
userspace provides a set of properties and their values for given fbproc
object in a similar way as object and properties are provided for
performing atomic page flip / mode setting.

The proposed API consists of the 3 new ioctls:
- DRM_IOCTL_EXYNOS_PP_GET_RESOURCES: to enumerate all available picture
   processors,
- DRM_IOCTL_EXYNOS_PP_GET: to query capabilities of given picture
   processor,
- DRM_IOCTL_EXYNOS_PP_COMMIT: to perform operation described by given
   property set.

The proposed API is extensible. Drivers can attach their own, custom
properties to add support for more advanced picture processing (for example
blending).

This proposal aims to replace Exynos DRM IPP (Image Post Processing)
subsystem. IPP API is over-engineered in general, but not really extensible
on the other side. It is also buggy, with significant design flaws - the
biggest issue is the fact that the API covers memory-2-memory picture
operations together with CRTC writeback and duplicating features, which
belongs to video plane. Comparing with IPP subsystem, the PP framework is
smaller (1807 vs 778 lines) and allows driver simplification (Exynos
rotator driver smaller by over 200 lines).

This seems to be the kind of hardware that is typically supported by V4L2.
Stupid question, why DRM ?

Let me elaborate a bit on the reasons for implementing it in Exynos DRM:

1. we want to replace existing Exynos IPP subsystem:
  - it is used only in some internal/vendor trees, not in open-source
  - we want it to have sane and potentially extensible userspace API
  - but we don't want to loose its functionality

2. we want to have simple API for performing single image processing
operation:
  - typically it will be used by compositing window manager, this means that
some parameters of the processing might change on each vblank (like
destination rectangle for example). This api allows such change on each
operation without any additional cost. V4L2 requires to reinitialize
queues with new configuration on such change, what means that a bunch of
ioctls has to be called.

What do you mean by re-initialising the queue? Format, buffers or something
else?

If you need a larger buffer than what you have already allocated, you'll
need to re-allocate, V4L2 or not.

We also do lack a way to destroy individual buffers in V4L2. It'd be up to
implementing that and some work in videobuf2.

Another thing is that V4L2 is very stream oriented. For most devices that's
fine as a lot of the parameters are not changeable during streaming,
especially if the pipeline is handled by multiple drivers. That said, for
devices that process 

Re: [PATCH 10/16] rcar-vin: move functions which acts on hardware

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:51 Niklas Söderlund wrote:
> This only moves whole structs, defines and functions around, no code is
> changed inside any function. The reason for moving this code around is
> to prepare for refactoring and fixing of a start/stop stream bug without
> having to use forward declarations.
> 
> Signed-off-by: Niklas Söderlund 
> ---
> drivers/media/platform/rcar-vin/rcar-dma.c | 181 -
> 1 file changed, 90 insertions(+), 91 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> c37f7a2993fb5565..c10d75aa7e71d665 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -119,6 +119,15 @@
>  #define VNDMR2_FTEV  (1 << 17)
>  #define VNDMR2_VLV(n)((n & 0xf) << 12)
> 
> +struct rvin_buffer {
> + struct vb2_v4l2_buffer vb;
> + struct list_head list;
> +};
> +
> +#define to_buf_list(vb2_buffer) (_of(vb2_buffer, \
> +struct rvin_buffer, \
> +vb)->list)
> +
>  static void rvin_write(struct rvin_dev *vin, u32 value, u32 offset)
>  {
>   iowrite32(value, vin->base + offset);
> @@ -269,48 +278,6 @@ static int rvin_setup(struct rvin_dev *vin)
>   return 0;
>  }
> 
> -static void rvin_capture_on(struct rvin_dev *vin)
> -{
> - vin_dbg(vin, "Capture on in %s mode\n",
> - vin->continuous ? "continuous" : "single");
> -
> - if (vin->continuous)
> - /* Continuous Frame Capture Mode */
> - rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
> - else
> - /* Single Frame Capture Mode */
> - rvin_write(vin, VNFC_S_FRAME, VNFC_REG);
> -}
> -
> -static void rvin_capture_off(struct rvin_dev *vin)
> -{
> - /* Set continuous & single transfer off */
> - rvin_write(vin, 0, VNFC_REG);
> -}
> -
> -static int rvin_capture_start(struct rvin_dev *vin)
> -{
> - int ret;
> -
> - rvin_crop_scale_comp(vin);
> -
> - ret = rvin_setup(vin);
> - if (ret)
> - return ret;
> -
> - rvin_capture_on(vin);
> -
> - return 0;
> -}
> -
> -static void rvin_capture_stop(struct rvin_dev *vin)
> -{
> - rvin_capture_off(vin);
> -
> - /* Disable module */
> - rvin_write(vin, rvin_read(vin, VNMC_REG) & ~VNMC_ME, VNMC_REG);
> -}
> -
>  static void rvin_disable_interrupts(struct rvin_dev *vin)
>  {
>   rvin_write(vin, 0, VNIE_REG);
> @@ -377,6 +344,87 @@ static void rvin_set_slot_addr(struct rvin_dev *vin,
> int slot, dma_addr_t addr) rvin_write(vin, offset, VNMB_REG(slot));
>  }
> 
> +static bool rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
> +{
> + struct rvin_buffer *buf;
> + struct vb2_v4l2_buffer *vbuf;
> + dma_addr_t phys_addr_top;
> +
> + if (vin->queue_buf[slot] != NULL)
> + return true;
> +
> + if (list_empty(>buf_list))
> + return false;
> +
> + vin_dbg(vin, "Filling HW slot: %d\n", slot);
> +
> + /* Keep track of buffer we give to HW */
> + buf = list_entry(vin->buf_list.next, struct rvin_buffer, list);
> + vbuf = >vb;
> + list_del_init(to_buf_list(vbuf));
> + vin->queue_buf[slot] = vbuf;
> +
> + /* Setup DMA */
> + phys_addr_top = vb2_dma_contig_plane_dma_addr(>vb2_buf, 0);
> + rvin_set_slot_addr(vin, slot, phys_addr_top);
> +
> + return true;
> +}
> +
> +static bool rvin_fill_hw(struct rvin_dev *vin)
> +{
> + int slot, limit;
> +
> + limit = vin->continuous ? HW_BUFFER_NUM : 1;
> +
> + for (slot = 0; slot < limit; slot++)
> + if (!rvin_fill_hw_slot(vin, slot))
> + return false;
> + return true;
> +}
> +
> +static void rvin_capture_on(struct rvin_dev *vin)
> +{
> + vin_dbg(vin, "Capture on in %s mode\n",
> + vin->continuous ? "continuous" : "single");
> +
> + if (vin->continuous)
> + /* Continuous Frame Capture Mode */
> + rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
> + else
> + /* Single Frame Capture Mode */
> + rvin_write(vin, VNFC_S_FRAME, VNFC_REG);
> +}
> +
> +static void rvin_capture_off(struct rvin_dev *vin)
> +{
> + /* Set continuous & single transfer off */
> + rvin_write(vin, 0, VNFC_REG);
> +}
> +
> +static int rvin_capture_start(struct rvin_dev *vin)
> +{
> + int ret;
> +
> + rvin_crop_scale_comp(vin);
> +
> + ret = rvin_setup(vin);
> + if (ret)
> + return ret;
> +
> + rvin_capture_on(vin);
> +
> + return 0;
> +}
> +
> +static void rvin_capture_stop(struct rvin_dev *vin)
> +{
> + rvin_capture_off(vin);
> +
> + /* Disable module */
> + rvin_write(vin, rvin_read(vin, VNMC_REG) & ~VNMC_ME, VNMC_REG);
> +}
> +
>  /*
> ---

Re: [PATCH 09/16] rcar-vin: decrease buffers needed to capture

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:50 Niklas Söderlund wrote:
> It's possible to grab frames using only one buffer, this should never
> have been set to anything else then 1.
> 
> Signed-off-by: Niklas Söderlund 

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> 9ccd5ff55e192514..c37f7a2993fb5565 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1183,7 +1183,7 @@ int rvin_dma_probe(struct rvin_dev *vin, int irq)
>   q->ops = _qops;
>   q->mem_ops = _dma_contig_memops;
>   q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> - q->min_buffers_needed = 2;
> + q->min_buffers_needed = 1;
>   q->dev = vin->dev;
> 
>   ret = vb2_queue_init(q);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 07/16] rcar-vin: move pad lookup to async bound handler

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:48 Niklas Söderlund wrote:
> Information about pads will be needed when enumerating the media bus
> codes in the async complete handler which is run before
> rvin_v4l2_probe(). Move the pad lookup to the async bound handler so
> they are available when needed.
> 
> Signed-off-by: Niklas Söderlund 
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 32 +-
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 24 --
> 2 files changed, 31 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> 098a0b1cc10a26ba..d7aba15f6761259b 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -31,6 +31,20 @@
> 
>  #define notifier_to_vin(n) container_of(n, struct rvin_dev, notifier)
> 
> +static int rvin_find_pad(struct v4l2_subdev *sd, int direction)
> +{
> + unsigned int pad;
> +
> + if (sd->entity.num_pads <= 1)
> + return 0;
> +
> + for (pad = 0; pad < sd->entity.num_pads; pad++)
> + if (sd->entity.pads[pad].flags & direction)
> + return pad;
> +
> + return -EINVAL;
> +}
> +
>  static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
>  {
>   struct v4l2_subdev *sd = entity->subdev;
> @@ -101,12 +115,28 @@ static int rvin_digital_notify_bound(struct
> v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd)
>  {
>   struct rvin_dev *vin = notifier_to_vin(notifier);
> + int ret;
> 
>   v4l2_set_subdev_hostdata(subdev, vin);
> 
>   if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
> - vin_dbg(vin, "bound digital subdev %s\n", subdev->name);
> + /* Find surce and sink pad of remote subdevice */
> +
> + ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
> + if (ret < 0)
> + return ret;
> + vin->digital.source_pad = ret;
> +
> + ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
> + if (ret < 0)
> + return ret;
> + vin->digital.sink_pad = ret;
> +
>   vin->digital.subdev = subdev;
> +
> + vin_dbg(vin, "bound subdev %s source pad: %d sink pad: %d\n",
> + subdev->name, vin->digital.source_pad,
> + vin->digital.sink_pad);

As source_pad and sink_pad are unsigned, s/%d/%u/g

>   return 0;
>   }
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> ce29a21888da48d5..be6f41bf82ac3bc5 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -870,20 +870,6 @@ static void rvin_notify(struct v4l2_subdev *sd,
>   }
>  }
> 
> -static int rvin_find_pad(struct v4l2_subdev *sd, int direction)
> -{
> - unsigned int pad;
> -
> - if (sd->entity.num_pads <= 1)
> - return 0;
> -
> - for (pad = 0; pad < sd->entity.num_pads; pad++)
> - if (sd->entity.pads[pad].flags & direction)
> - return pad;
> -
> - return -EINVAL;
> -}
> -
>  int rvin_v4l2_probe(struct rvin_dev *vin)
>  {
>   struct video_device *vdev = >vdev;
> @@ -934,16 +920,6 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
>   vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
>   V4L2_CAP_READWRITE;
> 
> - ret = rvin_find_pad(sd, MEDIA_PAD_FL_SOURCE);
> - if (ret < 0)
> - return ret;
> - vin->digital.source_pad = ret;
> -
> - ret = rvin_find_pad(sd, MEDIA_PAD_FL_SINK);
> - if (ret < 0)
> - return ret;
> - vin->digital.sink_pad = ret;
> -
>   vin->format.pixelformat = RVIN_DEFAULT_FORMAT;
>   rvin_reset_format(vin);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 02/16] rcar-vin: use rvin_reset_format() in S_DV_TIMINGS

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:43 Niklas Söderlund wrote:
> Use rvin_reset_format() in rvin_s_dv_timings() instead of just resetting
> a few fields. This fixes an issue where the field format was not
> properly set after S_DV_TIMINGS.
> 
> Signed-off-by: Niklas Söderlund 
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 8 ++--
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> 69bc4cfea6a8aeb5..7ca27599b9982ffc 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -573,12 +573,8 @@ static int rvin_s_dv_timings(struct file *file, void
> *priv_fh, if (ret)
>   return ret;
> 
> - vin->source.width = timings->bt.width;
> - vin->source.height = timings->bt.height;
> - vin->format.width = timings->bt.width;
> - vin->format.height = timings->bt.height;
> -
> - return 0;
> + /* Changing the timings will change the width/height */
> + return rvin_reset_format(vin);

vin->source won't be updated anymore. Is this intentional ?

>  }
> 
>  static int rvin_g_dv_timings(struct file *file, void *priv_fh,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 04/16] rcar-vin: fix standard in input enumeration

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:45 Niklas Söderlund wrote:
> If the subdevice supports dv_timings_cap the driver should not fill in
> the standard.

A subdev could have analog TV and digital TV inputs. However, as the rcar-vin 
driver supports a single input only, this patch is correct. I'd mention this 
fact in the commit message. Apart from that,

Reviewed-by: Laurent Pinchart 

> Signed-off-by: Niklas Söderlund 
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> 610f59e2a9142622..7be52c2036bb35fc 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -483,10 +483,14 @@ static int rvin_enum_input(struct file *file, void
> *priv, return ret;
> 
>   i->type = V4L2_INPUT_TYPE_CAMERA;
> - i->std = vin->vdev.tvnorms;
> 
> - if (v4l2_subdev_has_op(sd, pad, dv_timings_cap))
> + if (v4l2_subdev_has_op(sd, pad, dv_timings_cap)) {
>   i->capabilities = V4L2_IN_CAP_DV_TIMINGS;
> + i->std = 0;
> + } else {
> + i->capabilities = V4L2_IN_CAP_STD;
> + i->std = vin->vdev.tvnorms;
> + }
> 
>   strlcpy(i->name, "Camera", sizeof(i->name));

-- 
Regards,

Laurent Pinchart

Re: [PATCH 01/16] rcar-vin: reset bytesperline and sizeimage when resetting format

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:42 Niklas Söderlund wrote:
> These two where forgotten when refactoring the format reset code. If
> they are not also reset at the same time as width and height the format
> returned from G_FMT will not match reality.
> 
> Signed-off-by: Niklas Söderlund 

With the commit message typo fixed,

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> 2bbe6d495fa634da..69bc4cfea6a8aeb5 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -151,6 +151,9 @@ static int rvin_reset_format(struct rvin_dev *vin)
> 
>   rvin_reset_crop_compose(vin);
> 
> + vin->format.bytesperline = rvin_format_bytesperline(>format);
> + vin->format.sizeimage = rvin_format_sizeimage(>format);
> +
>   return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 05/16] rcar-vin: move subdev source and sink pad index to rvin_graph_entity

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:46 Niklas Söderlund wrote:
> It makes more sens to store the sink and source pads in struct
> rvin_graph_entity since that contains other subdevice related
> information.
> 
> The data type to store pad information in is unsigned int and not int,
> change this. While we are at it drop the _idx suffix from the names,
> this never made sens.

s/sens/sense/

> Signed-off-by: Niklas Söderlund 

With the typo fixed,

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 20 ++--
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  9 +
>  2 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> 7be52c2036bb35fc..1a75191539b0e7d7 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -111,7 +111,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
>   struct v4l2_mbus_framefmt *mf = 
>   int ret;
> 
> - fmt.pad = vin->src_pad_idx;
> + fmt.pad = vin->digital.source_pad;
> 
>   ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, );
>   if (ret)
> @@ -178,7 +178,7 @@ static int __rvin_try_format_source(struct rvin_dev
> *vin, if (pad_cfg == NULL)
>   return -ENOMEM;
> 
> - format.pad = vin->src_pad_idx;
> + format.pad = vin->digital.source_pad;
> 
>   field = pix->field;
> 
> @@ -559,7 +559,7 @@ static int rvin_enum_dv_timings(struct file *file, void
> *priv_fh, if (timings->pad)
>   return -EINVAL;
> 
> - timings->pad = vin->sink_pad_idx;
> + timings->pad = vin->digital.sink_pad;
> 
>   ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
> 
> @@ -611,7 +611,7 @@ static int rvin_dv_timings_cap(struct file *file, void
> *priv_fh, if (cap->pad)
>   return -EINVAL;
> 
> - cap->pad = vin->sink_pad_idx;
> + cap->pad = vin->digital.sink_pad;
> 
>   ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
> 
> @@ -629,7 +629,7 @@ static int rvin_g_edid(struct file *file, void *fh,
> struct v4l2_edid *edid) if (edid->pad)
>   return -EINVAL;
> 
> - edid->pad = vin->sink_pad_idx;
> + edid->pad = vin->digital.sink_pad;
> 
>   ret = v4l2_subdev_call(sd, pad, get_edid, edid);
> 
> @@ -647,7 +647,7 @@ static int rvin_s_edid(struct file *file, void *fh,
> struct v4l2_edid *edid) if (edid->pad)
>   return -EINVAL;
> 
> - edid->pad = vin->sink_pad_idx;
> + edid->pad = vin->digital.sink_pad;
> 
>   ret = v4l2_subdev_call(sd, pad, set_edid, edid);
> 
> @@ -920,19 +920,19 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
>   vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
>   V4L2_CAP_READWRITE;
> 
> - vin->src_pad_idx = 0;
> + vin->digital.source_pad = 0;
>   for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
>   if (sd->entity.pads[pad_idx].flags == MEDIA_PAD_FL_SOURCE)
>   break;
>   if (pad_idx >= sd->entity.num_pads)
>   return -EINVAL;
> 
> - vin->src_pad_idx = pad_idx;
> + vin->digital.source_pad = pad_idx;
> 
> - vin->sink_pad_idx = 0;
> + vin->digital.sink_pad = 0;
>   for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
>   if (sd->entity.pads[pad_idx].flags == MEDIA_PAD_FL_SINK) {
> - vin->sink_pad_idx = pad_idx;
> + vin->digital.sink_pad = pad_idx;
>   break;
>   }
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> b/drivers/media/platform/rcar-vin/rcar-vin.h index
> 727e215c08718eb9..9bfb5a7c4dc4f215 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -74,6 +74,8 @@ struct rvin_video_format {
>   * @subdev:  subdevice matched using async framework
>   * @code:Media bus format from source
>   * @mbus_cfg:Media bus format from DT
> + * @source_pad:  source pad of remote subdevice
> + * @sink_pad:sink pad of remote subdevice
>   */
>  struct rvin_graph_entity {
>   struct v4l2_async_subdev asd;
> @@ -81,6 +83,9 @@ struct rvin_graph_entity {
> 
>   u32 code;
>   struct v4l2_mbus_config mbus_cfg;
> +
> + unsigned int source_pad;
> + unsigned int sink_pad;
>  };
> 
>  /**
> @@ -91,8 +96,6 @@ struct rvin_graph_entity {
>   *
>   * @vdev:V4L2 video device associated with VIN
>   * @v4l2_dev:V4L2 device
> - * @src_pad_idx: source pad index for media controller drivers
> - * @sink_pad_idx:sink pad index for media controller drivers
>   * @ctrl_handler:V4L2 control handler
>   * @notifier:V4L2 asynchronous subdevs notifier
>   * 

Re: [PATCH 06/16] rcar-vin: refactor pad lookup code

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 14 Mar 2017 19:59:47 Niklas Söderlund wrote:
> The pad lookup code can be broken out to increase readability and to
> reduce code duplication.
> 
> Signed-off-by: Niklas Söderlund 
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 38 +
>  1 file changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> 1a75191539b0e7d7..ce29a21888da48d5 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -870,11 +870,25 @@ static void rvin_notify(struct v4l2_subdev *sd,
>   }
>  }
> 
> +static int rvin_find_pad(struct v4l2_subdev *sd, int direction)
> +{
> + unsigned int pad;
> +
> + if (sd->entity.num_pads <= 1)
> + return 0;
> +
> + for (pad = 0; pad < sd->entity.num_pads; pad++)
> + if (sd->entity.pads[pad].flags & direction)
> + return pad;
> +
> + return -EINVAL;
> +}
> +
>  int rvin_v4l2_probe(struct rvin_dev *vin)
>  {
>   struct video_device *vdev = >vdev;
>   struct v4l2_subdev *sd = vin_to_source(vin);
> - int pad_idx, ret;
> + int ret;
> 
>   v4l2_set_subdev_hostdata(sd, vin);
> 
> @@ -920,21 +934,15 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
>   vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
>   V4L2_CAP_READWRITE;
> 
> - vin->digital.source_pad = 0;
> - for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
> - if (sd->entity.pads[pad_idx].flags == MEDIA_PAD_FL_SOURCE)
> - break;
> - if (pad_idx >= sd->entity.num_pads)
> - return -EINVAL;
> -
> - vin->digital.source_pad = pad_idx;
> + ret = rvin_find_pad(sd, MEDIA_PAD_FL_SOURCE);
> + if (ret < 0)
> + return ret;
> + vin->digital.source_pad = ret;
> 
> - vin->digital.sink_pad = 0;
> - for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
> - if (sd->entity.pads[pad_idx].flags == MEDIA_PAD_FL_SINK) {
> - vin->digital.sink_pad = pad_idx;
> - break;
> - }
> + ret = rvin_find_pad(sd, MEDIA_PAD_FL_SINK);
> + if (ret < 0)
> + return ret;
> + vin->digital.sink_pad = ret;

The driver didn't previously consider the lack of a sink pad as an error. As 
camera sensor subdevs typically have no sink pad, I don't think you should 
change this.

>   vin->format.pixelformat = RVIN_DEFAULT_FORMAT;
>   rvin_reset_format(vin);

-- 
Regards,

Laurent Pinchart

Re: [RFC 0/4] Exynos DRM: add Picture Processor extension

[Daniel Vetter]

On Wed, May 10, 2017 at 12:29 PM, Inki Dae  wrote:
>> This kind of contradicts with response Marek received from DRM
>> community about his proposal. Which drivers in particular you have in
>> mind?
>
> You can check vmw_overlay_ioctl of vmwgfx driver and 
> intel_overlay_put_image_ioctl of i915 driver. These was all I could find in 
> mainline.
> Seems the boundaries of whether we have to implement pre/post post processing 
> mem2mem driver in V4L2 or DRM are really vague.

These aren't picture processors, but overlay plane support merged
before we had the core drm overlay support. Please do not emulate them
at all, your patch will be rejected :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v9 3/4] arm: dts: mt2701: Add node for Mediatek JPEG Decoder

[Matthias Brugger]

On 14/12/16 09:04, Rick Chang wrote:

Signed-off-by: Rick Chang 
Signed-off-by: Minghsiu Tsai 
---
This patch depends on:
   CCF "Add clock support for Mediatek MT2701"[1]
   iommu and smi "Add the dtsi node of iommu and smi for mt2701"[2]

[1] http://lists.infradead.org/pipermail/linux-mediatek/2016-October/007271.html
[2] https://patchwork.kernel.org/patch/9164013/


Now queued for v4.12-next/dts32

Thanks and sorry for the delay.
Matthias


---
  arch/arm/boot/dts/mt2701.dtsi | 14 ++
  1 file changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/mt2701.dtsi b/arch/arm/boot/dts/mt2701.dtsi
index 8f13c70..4dd5048 100644
--- a/arch/arm/boot/dts/mt2701.dtsi
+++ b/arch/arm/boot/dts/mt2701.dtsi
@@ -298,6 +298,20 @@
power-domains = < MT2701_POWER_DOMAIN_ISP>;
};
  
+	jpegdec: jpegdec@15004000 {

+   compatible = "mediatek,mt2701-jpgdec";
+   reg = <0 0x15004000 0 0x1000>;
+   interrupts = ;
+   clocks =  < CLK_IMG_JPGDEC_SMI>,
+ < CLK_IMG_JPGDEC>;
+   clock-names = "jpgdec-smi",
+ "jpgdec";
+   power-domains = < MT2701_POWER_DOMAIN_ISP>;
+   mediatek,larb = <>;
+   iommus = < MT2701_M4U_PORT_JPGDEC_WDMA>,
+< MT2701_M4U_PORT_JPGDEC_BSDMA>;
+   };
+
vdecsys: syscon@1600 {
compatible = "mediatek,mt2701-vdecsys", "syscon";
reg = <0 0x1600 0 0x1000>;

Re: [RFC v4 13/18] vb2: Don't sync cache for a buffer if so requested

[Tomasz Figa]

Hi Sakari,

Few comments inline.

On Mon, May 8, 2017 at 11:03 PM, Sakari Ailus
 wrote:
> From: Samu Onkalo 
>
> The user may request to the driver (vb2) to skip the cache maintenance
> operations in case the buffer does not need cache synchronisation, e.g. in
> cases where the buffer is passed between hardware blocks without it being
> touched by the CPU.
[snip]
> @@ -1199,6 +1236,11 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, 
> const void *pb)
> dprintk(1, "buffer initialization failed\n");
> goto err;
> }
> +
> +   /* This is new buffer memory --- always synchronise cache. */
> +   __mem_prepare_planes(vb);
> +   } else if (!no_cache_sync) {
> +   __mem_prepare_planes(vb);

Do we actually need this at all for DMABUF, given that respective
callbacks in both vb2_dc and vb2_sg actually bail out if so?

> }
>
> ret = call_vb_qop(vb, buf_prepare, vb);
[snip]
> @@ -568,7 +571,11 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> }
>
> ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> -   return ret ? ret : vb2_core_qbuf(q, b->index, b);
> +   if (ret)
> +   return ret;
> +
> +   return vb2_core_qbuf(q, b->index, b,
> +b->flags & V4L2_BUF_FLAG_NO_CACHE_SYNC);

Can we really let the userspace alone control this? I believe there
are drivers that need to do some fixup in OUTPUT buffers before
sending to the hardware or in CAPTURE buffer after getting from the
hardware, respectively in buf_prepare or buf_finish. I believe such
driver needs to be able to override this behavior.

Actually I'm wondering if we really need this buffer flag at all.
Wouldn't the following work for typical use cases that we actually
care about performance of?

buffer_needs_cache_sync = (buffer_type_is_MMAP &&
buffer_is_non_coherent && (buffer_is_mmapped ||
buffer_has_kernel_mapping)) || buffer_is_USERPTR

The above should cover all the fast paths that are used only to
exchange data between devices, without the CPU involved, assuming that
drivers that don't need the fixups I mentioned before are properly
updated to request no kernel mapping for allocated buffers.

I've added (buffer_is_USERPTR) to the equation as it's really hard to
imagine a use case where there is no CPU access to the buffer, but
USERPTR needs to be used (instead of DMABUF). I might be missing
something, though.

Best regards,
Tomasz

Re: [RFC v4 10/18] vb2: dma-contig: Fix DMA attribute and cache management

[Tomasz Figa]

Hi Sakari,

Some comments inline.

On Mon, May 8, 2017 at 11:03 PM, Sakari Ailus
 wrote:
> Patch ccc66e73 ("ARM: 8508/2: videobuf2-dc: Let drivers specify DMA
> attrs") added support for driver specific DMA attributes to
> videobuf2-dma-contig but it had several issues in it.
>
> In particular,
>
> - cache operations were only performed on USERPTR buffers,
>
> - DMA attributes were set only for MMAP buffers and
>
> - it did not provide begin_cpu_access() and end_cpu_access() dma_buf_ops
>   callbacks for cache syncronisation on exported MMAP buffers.
>
> This patch corrects these issues.

I think the above are not really issues for the use cases the original
commit added the support for, i.e. disabling kernel mapping. There was
no intention of allowing cached MMAP buffers. Although I guess the
code added by it was not strict enough and didn't check the flags for
allowed ones.

>
> Also arrange the header files alphabetically.
>
> Fixes: ccc66e73 ("ARM: 8508/2: videobuf2-dc: Let drivers specify DMA attrs")
> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 94 
> --
>  1 file changed, 72 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c 
> b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index 0afc3da..8b0298a 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -11,12 +11,12 @@
>   */
>
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
>  #include 
> -#include 
>
>  #include 
>  #include 
> @@ -97,12 +97,13 @@ static void vb2_dc_prepare(void *buf_priv)
> struct vb2_dc_buf *buf = buf_priv;
> struct sg_table *sgt = buf->dma_sgt;
>
> -   /* DMABUF exporter will flush the cache for us */

Ah, this annoying comment goes away here! Thanks and sorry for the
noise in previous patch.

> -   if (!buf->vec)
> -   return;
> -
> -   dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents,
> -  buf->dma_dir);
> +   /*
> +* DMABUF exporter will flush the cache for us; only USERPTR
> +* and MMAP buffers with non-coherent memory will be flushed.
> +*/

Sounds much better.

> +   if (buf->attrs & DMA_ATTR_NON_CONSISTENT)

Hmm, we change it for USERPTR to rely on presence of
DMA_ATTR_NON_CONSISTENT in buf->attrs, but I don't see it being set
for such anywhere by previous patches.

> +   dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents,
> +  buf->dma_dir);
>  }
>
>  static void vb2_dc_finish(void *buf_priv)
> @@ -110,11 +111,13 @@ static void vb2_dc_finish(void *buf_priv)
> struct vb2_dc_buf *buf = buf_priv;
> struct sg_table *sgt = buf->dma_sgt;
>
> -   /* DMABUF exporter will flush the cache for us */
> -   if (!buf->vec)
> -   return;
> -
> -   dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, 
> buf->dma_dir);
> +   /*
> +* DMABUF exporter will flush the cache for us; only USERPTR
> +* and MMAP buffers with non-coherent memory will be flushed.
> +*/
> +   if (buf->attrs & DMA_ATTR_NON_CONSISTENT)

Ditto.

> +   dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents,
> +   buf->dma_dir);
>  }
>
>  /*/
> @@ -142,6 +145,7 @@ static void *vb2_dc_alloc(struct device *dev, unsigned 
> long attrs,
>   gfp_t gfp_flags)
>  {
> struct vb2_dc_buf *buf;
> +   int ret;
>
> if (WARN_ON(!dev))
> return ERR_PTR(-EINVAL);
> @@ -152,9 +156,9 @@ static void *vb2_dc_alloc(struct device *dev, unsigned 
> long attrs,
>
> buf->attrs = attrs;
> buf->cookie = dma_alloc_attrs(dev, size, >dma_addr,
> -   GFP_KERNEL | gfp_flags, buf->attrs);
> + GFP_KERNEL | gfp_flags, buf->attrs);

White space change, not sure if intended.

> if (!buf->cookie) {
> -   dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);
> +   dev_err(dev, "dma_alloc_attrs of size %ld failed\n", size);
> kfree(buf);
> return ERR_PTR(-ENOMEM);
> }
> @@ -167,6 +171,16 @@ static void *vb2_dc_alloc(struct device *dev, unsigned 
> long attrs,
> buf->size = size;
> buf->dma_dir = dma_dir;
>
> +   ret = dma_get_sgtable_attrs(buf->dev, >__dma_sgt, buf->cookie,
> +   buf->dma_addr, buf->size, buf->attrs);
> +   if (ret < 0) {
> +   dma_free_attrs(dev, size, buf->cookie, buf->dma_addr,
> +  buf->attrs);
> +   put_device(dev);
> +   return ERR_PTR(-ENOMEM);
> +   

Re: [RFC 0/4] Exynos DRM: add Picture Processor extension

[Inki Dae]

2017년 05월 10일 16:55에 Daniel Vetter 이(가) 쓴 글:
> On Wed, May 10, 2017 at 03:27:02PM +0900, Inki Dae wrote:
>> Hi Tomasz,
>>
>> 2017년 05월 10일 14:38에 Tomasz Figa 이(가) 쓴 글:
>>> Hi Everyone,
>>>
>>> On Wed, May 10, 2017 at 9:24 AM, Inki Dae  wrote:


 2017년 04월 26일 07:21에 Sakari Ailus 이(가) 쓴 글:
> Hi Marek,
>
> On Thu, Apr 20, 2017 at 01:23:09PM +0200, Marek Szyprowski wrote:
>> Hi Laurent,
>>
>> On 2017-04-20 12:25, Laurent Pinchart wrote:
>>> Hi Marek,
>>>
>>> (CC'ing Sakari Ailus)
>>>
>>> Thank you for the patches.
>>>
>>> On Thursday 20 Apr 2017 11:13:36 Marek Szyprowski wrote:
 Dear all,

 This is an updated proposal for extending EXYNOS DRM API with generic
 support for hardware modules, which can be used for processing image 
 data
>>> >from the one memory buffer to another. Typical memory-to-memory 
>>> >operations
 are: rotation, scaling, colour space conversion or mix of them. This 
 is a
 follow-up of my previous proposal "[RFC 0/2] New feature: Framebuffer
 processors", which has been rejected as "not really needed in the DRM
 core":
 http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg146286.html

 In this proposal I moved all the code to Exynos DRM driver, so now this
 will be specific only to Exynos DRM. I've also changed the name from
 framebuffer processor (fbproc) to picture processor (pp) to avoid 
 confusion
 with fbdev API.

 Here is a bit more information what picture processors are:

 Embedded SoCs are known to have a number of hardware blocks, which 
 perform
 such operations. They can be used in paralel to the main GPU module to
 offload CPU from processing grapics or video data. One of example use 
 of
 such modules is implementing video overlay, which usually requires 
 color
 space conversion from NV12 (or similar) to RGB32 color space and 
 scaling to
 target window size.

 The proposed API is heavily inspired by atomic KMS approach - it is 
 also
 based on DRM objects and their properties. A new DRM object is 
 introduced:
 picture processor (called pp for convenience). Such objects have a set 
 of
 standard DRM properties, which describes the operation to be performed 
 by
 respective hardware module. In typical case those properties are a 
 source
 fb id and rectangle (x, y, width, height) and destination fb id and
 rectangle. Optionally a rotation property can be also specified if
 supported by the given hardware. To perform an operation on image data,
 userspace provides a set of properties and their values for given 
 fbproc
 object in a similar way as object and properties are provided for
 performing atomic page flip / mode setting.

 The proposed API consists of the 3 new ioctls:
 - DRM_IOCTL_EXYNOS_PP_GET_RESOURCES: to enumerate all available picture
   processors,
 - DRM_IOCTL_EXYNOS_PP_GET: to query capabilities of given picture
   processor,
 - DRM_IOCTL_EXYNOS_PP_COMMIT: to perform operation described by given
   property set.

 The proposed API is extensible. Drivers can attach their own, custom
 properties to add support for more advanced picture processing (for 
 example
 blending).

 This proposal aims to replace Exynos DRM IPP (Image Post Processing)
 subsystem. IPP API is over-engineered in general, but not really 
 extensible
 on the other side. It is also buggy, with significant design flaws - 
 the
 biggest issue is the fact that the API covers memory-2-memory picture
 operations together with CRTC writeback and duplicating features, which
 belongs to video plane. Comparing with IPP subsystem, the PP framework 
 is
 smaller (1807 vs 778 lines) and allows driver simplification (Exynos
 rotator driver smaller by over 200 lines).
>>> This seems to be the kind of hardware that is typically supported by 
>>> V4L2.
>>> Stupid question, why DRM ?
>>
>> Let me elaborate a bit on the reasons for implementing it in Exynos DRM:
>>
>> 1. we want to replace existing Exynos IPP subsystem:
>>  - it is used only in some internal/vendor trees, not in open-source
>>  - we want it to have sane and potentially extensible userspace API
>>  - but we don't want to loose its functionality
>>
>> 2. we want to have simple API for performing single image processing
>> operation:
>>  - typically it will be used by compositing window manager, this means 

Re: [RFC 0/4] Exynos DRM: add Picture Processor extension

[Inki Dae]

2017년 05월 10일 15:38에 Tomasz Figa 이(가) 쓴 글:
> On Wed, May 10, 2017 at 2:27 PM, Inki Dae  wrote:
>> Hi Tomasz,
>>
>> 2017년 05월 10일 14:38에 Tomasz Figa 이(가) 쓴 글:
>>> Hi Everyone,
>>>
>>> On Wed, May 10, 2017 at 9:24 AM, Inki Dae  wrote:


 2017년 04월 26일 07:21에 Sakari Ailus 이(가) 쓴 글:
> Hi Marek,
>
> On Thu, Apr 20, 2017 at 01:23:09PM +0200, Marek Szyprowski wrote:
>> Hi Laurent,
>>
>> On 2017-04-20 12:25, Laurent Pinchart wrote:
>>> Hi Marek,
>>>
>>> (CC'ing Sakari Ailus)
>>>
>>> Thank you for the patches.
>>>
>>> On Thursday 20 Apr 2017 11:13:36 Marek Szyprowski wrote:
 Dear all,

 This is an updated proposal for extending EXYNOS DRM API with generic
 support for hardware modules, which can be used for processing image 
 data
>>> >from the one memory buffer to another. Typical memory-to-memory 
>>> >operations
 are: rotation, scaling, colour space conversion or mix of them. This 
 is a
 follow-up of my previous proposal "[RFC 0/2] New feature: Framebuffer
 processors", which has been rejected as "not really needed in the DRM
 core":
 http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg146286.html

 In this proposal I moved all the code to Exynos DRM driver, so now this
 will be specific only to Exynos DRM. I've also changed the name from
 framebuffer processor (fbproc) to picture processor (pp) to avoid 
 confusion
 with fbdev API.

 Here is a bit more information what picture processors are:

 Embedded SoCs are known to have a number of hardware blocks, which 
 perform
 such operations. They can be used in paralel to the main GPU module to
 offload CPU from processing grapics or video data. One of example use 
 of
 such modules is implementing video overlay, which usually requires 
 color
 space conversion from NV12 (or similar) to RGB32 color space and 
 scaling to
 target window size.

 The proposed API is heavily inspired by atomic KMS approach - it is 
 also
 based on DRM objects and their properties. A new DRM object is 
 introduced:
 picture processor (called pp for convenience). Such objects have a set 
 of
 standard DRM properties, which describes the operation to be performed 
 by
 respective hardware module. In typical case those properties are a 
 source
 fb id and rectangle (x, y, width, height) and destination fb id and
 rectangle. Optionally a rotation property can be also specified if
 supported by the given hardware. To perform an operation on image data,
 userspace provides a set of properties and their values for given 
 fbproc
 object in a similar way as object and properties are provided for
 performing atomic page flip / mode setting.

 The proposed API consists of the 3 new ioctls:
 - DRM_IOCTL_EXYNOS_PP_GET_RESOURCES: to enumerate all available picture
   processors,
 - DRM_IOCTL_EXYNOS_PP_GET: to query capabilities of given picture
   processor,
 - DRM_IOCTL_EXYNOS_PP_COMMIT: to perform operation described by given
   property set.

 The proposed API is extensible. Drivers can attach their own, custom
 properties to add support for more advanced picture processing (for 
 example
 blending).

 This proposal aims to replace Exynos DRM IPP (Image Post Processing)
 subsystem. IPP API is over-engineered in general, but not really 
 extensible
 on the other side. It is also buggy, with significant design flaws - 
 the
 biggest issue is the fact that the API covers memory-2-memory picture
 operations together with CRTC writeback and duplicating features, which
 belongs to video plane. Comparing with IPP subsystem, the PP framework 
 is
 smaller (1807 vs 778 lines) and allows driver simplification (Exynos
 rotator driver smaller by over 200 lines).
>>> This seems to be the kind of hardware that is typically supported by 
>>> V4L2.
>>> Stupid question, why DRM ?
>>
>> Let me elaborate a bit on the reasons for implementing it in Exynos DRM:
>>
>> 1. we want to replace existing Exynos IPP subsystem:
>>  - it is used only in some internal/vendor trees, not in open-source
>>  - we want it to have sane and potentially extensible userspace API
>>  - but we don't want to loose its functionality
>>
>> 2. we want to have simple API for performing single image processing
>> operation:
>>  - typically it will be used by compositing window manager, 

Re: [RFC v4 07/18] vb2: dma-contig: Remove redundant sgt_base field

[Tomasz Figa]

Hi Sakari,

Some comments inline.

On Mon, May 8, 2017 at 11:03 PM, Sakari Ailus
 wrote:
> The struct vb2_dc_buf contains two struct sg_table fields: sgt_base and
> dma_sgt. The former is used by DMA-BUF buffers whereas the latter is used
> by USERPTR.
>
> Unify the two, leaving dma_sgt.
>
> MMAP buffers do not need cache flushing since they have been allocated
> using dma_alloc_coherent().
>
> Signed-off-by: Sakari Ailus 
> Acked-by: Hans Verkuil 
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 25 +
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c 
> b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index a8a46a8..ddbbcf0 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -31,12 +31,13 @@ struct vb2_dc_buf {
> unsigned long   attrs;
> enum dma_data_direction dma_dir;
> struct sg_table *dma_sgt;
> -   struct frame_vector *vec;
>
> /* MMAP related */
> struct vb2_vmarea_handler   handler;
> refcount_t  refcount;
> -   struct sg_table *sgt_base;
> +
> +   /* USERPTR related */
> +   struct frame_vector *vec;
>
> /* DMABUF related */
> struct dma_buf_attachment   *db_attach;
> @@ -96,7 +97,7 @@ static void vb2_dc_prepare(void *buf_priv)
> struct sg_table *sgt = buf->dma_sgt;
>
> /* DMABUF exporter will flush the cache for us */
> -   if (!sgt || buf->db_attach)
> +   if (!buf->vec)

While at it, can we change the comment above to actually refer to what
this condition is checking? Maybe it's just me, but it's very
confusing, as the condition is actually (!USERPTR), while the comment
mentions DMABUF alone and not even mentioning about MMAP. Maybe we
could have something like this:

/*
 * Only USERPTR needs cache maintenance. DMABUF exporter will flush
 * the cache for us, while MMAP buffers are coherent by design.
 */

I guess it could be done as a separate patch after this series,
especially considering the message might actually change, since we are
going to allow cached MMAP buffers.

> return;
>
> dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents,
> @@ -109,7 +110,7 @@ static void vb2_dc_finish(void *buf_priv)
> struct sg_table *sgt = buf->dma_sgt;
>
> /* DMABUF exporter will flush the cache for us */

Here too.

Best regards,
Tomasz

[PATCH] TW686x: Fix OOPS on buffer alloc failure

[Krzysztof Hałasa]

Signed-off-by: Krzysztof Hałasa 

diff --git a/drivers/media/pci/tw686x/tw686x-video.c 
b/drivers/media/pci/tw686x/tw686x-video.c
index c3fafa9..d637f47 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -1190,6 +1190,13 @@ int tw686x_video_init(struct tw686x_dev *dev)
return err;
}
 
+   /* Initialize vc->dev and vc->ch for the error path first */
+   for (ch = 0; ch < max_channels(dev); ch++) {
+   struct tw686x_video_channel *vc = >video_channels[ch];
+   vc->dev = dev;
+   vc->ch = ch;
+   }
+
for (ch = 0; ch < max_channels(dev); ch++) {
struct tw686x_video_channel *vc = >video_channels[ch];
struct video_device *vdev;
@@ -1198,9 +1205,6 @@ int tw686x_video_init(struct tw686x_dev *dev)
spin_lock_init(>qlock);
INIT_LIST_HEAD(>vidq_queued);
 
-   vc->dev = dev;
-   vc->ch = ch;
-
/* default settings */
err = tw686x_set_standard(vc, V4L2_STD_NTSC);
if (err)

Re: [PCIE device driver: tw686x] I have some problems with tw686x and I need help.

[Krzysztof Hałasa]

Hello Singh,

I've added linux-media as well as the current TW686x driver maintainer
to Cc: list. Perhaps they will be better prepared to help you, the
driver differs much from what it was when I originally wrote it.

 writes:

> First, I download source code from website
> https://github.com/torvalds/linux/tree/master/drivers/media/pci/tw686x,
> build the driver in my project. when i start the machine the machine
> printk the log
> "[ 5.557782] tw6869: PCI :01:00.0, IRQ 170, MMIO 0xc800 (memcpy mode)
> [ 5.565010] tw686x :01:00.0: enabling device ( -> 0002)".
> from the log we can see that pcie-bus read the ID is 6869, but out
> chip is tw6865. this is not our main problem.

The truth is I don't have a real TW6865, I simply assumed its PCI ID
would be 0x6865 because it worked this way for TW6864 and TW6869.
Could you please do a "lspci -vvn" on the machine with this card and
send me (with CC: linux-media@vger.kernel.org) this command's output
(you may omit other devices, I'm only interested in the TW chip
information). Perhaps the invalid identification can be fixed.

Thanks.

> After we booted the machine. then we lunch out application to display
> video that we capture from tw6865. But there are a lot of log , such
> as
> "[ 24.671551] tw686x :01:00.0: video10: unexpected p-b buffer!
> [ 24.671561] tw686x :01:00.0: video11: unexpected p-b buffer!
> [ 24.671566] tw686x :01:00.0: video12: unexpected p-b buffer!
> [ 24.671570] tw686x :01:00.0: video13: unexpected p-b buffer!"
> and the log print always.
> those log means tw6865 always reset dma channel.
> sometimes we can see the right picture, but the video moves slowly, about 2 
> frame per 5 seconds.
> after a few minute, the linux kernel crashed. I post the crash log
> below.

Unfortunately there is no meaningful stack dump. Perhaps the crashes
will go away when the "unexpected p-b buffer" issue is fixed so I
wouldn't worry too much about the dumps at the moment.

> Sometimes, after I booted the machine, and lunched the application. To
> the begin, we can see a few frame displaying on screen. after a few
> seconds, the monitor are freezed. and I use "cat /proc/interrupts" to
> look up tw6865's interrput. and we find that there is no tw6865
> interrupt any more. after a few minutes, most probely the machine will
> crash and reboot(crash log is same with the before which I have
> posted).
> we use command "dmesg" to look up log. we found that there ever
> happend "tw686x :01:00.0: video10: FIFO error" or "we found that
> there ever happend" frequently.

This could be caused by the invalid identification (and handling) of
the chip.

OTOH, the "memcpy" mode of this driver may be too slow on your machine
(ARM usually doesn't have hardware-coherent cache memory and certain
cache sync operations are apparently very slow). You may want to try the
DMA mode instead (modprobe tw686x dma_mode=contig or sg). Be aware that
the DMA modes aren't as flexible as memcpy mode, the frame formats are
more limited.

I don't have experience with any ARM64, though.

Also I think the current code will OOPS on any video initialization
error (which will most probably happen on ARM in contig mode because
ARM machines don't allow that much continuous allocation). At least this
can be easily fixed, I'll attach the patch.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH v5 3/7] media: i2c: max2175: Add MAX2175 support

[Sakari Ailus]

Hi Ramesh,

On Tue, May 09, 2017 at 02:37:34PM +0100, Ramesh Shanmugasundaram wrote:
...
> +#include 
> +#include 
> +#include 

Could you rebase this on the V4L2 fwnode patchset here, please?



It depends on other patches which will reach media-tree master in next rc1,
for now I've merged them here:



I'll send a pull request for media-tree once we have 4.12rc1 in media-tree
master.

Thanks.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH[ v4l2-ioctl.c: always copy G/S_EDID result

[Sakari Ailus]

Hi Hans,

On Wed, May 10, 2017 at 08:36:56AM +0200, Hans Verkuil wrote:
> The VIDIOC_G/S_EDID ioctls can return valid data even if an error is returned.
> 
> Mark those ioctls accordingly. Rather than using an explicit 'if' to check 
> for the
> ioctl (as was done until now for VIDIOC_QUERY_DV_TIMINGS) just set a new flag 
> in the
> v4l2_ioctls array.
> 
> Signed-off-by: Hans Verkuil 

Acked-by: Sakari Ailus 

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [RFC 0/4] Exynos DRM: add Picture Processor extension

[Daniel Vetter]

On Wed, May 10, 2017 at 03:27:02PM +0900, Inki Dae wrote:
> Hi Tomasz,
> 
> 2017년 05월 10일 14:38에 Tomasz Figa 이(가) 쓴 글:
> > Hi Everyone,
> > 
> > On Wed, May 10, 2017 at 9:24 AM, Inki Dae  wrote:
> >>
> >>
> >> 2017년 04월 26일 07:21에 Sakari Ailus 이(가) 쓴 글:
> >>> Hi Marek,
> >>>
> >>> On Thu, Apr 20, 2017 at 01:23:09PM +0200, Marek Szyprowski wrote:
>  Hi Laurent,
> 
>  On 2017-04-20 12:25, Laurent Pinchart wrote:
> > Hi Marek,
> >
> > (CC'ing Sakari Ailus)
> >
> > Thank you for the patches.
> >
> > On Thursday 20 Apr 2017 11:13:36 Marek Szyprowski wrote:
> >> Dear all,
> >>
> >> This is an updated proposal for extending EXYNOS DRM API with generic
> >> support for hardware modules, which can be used for processing image 
> >> data
> > >from the one memory buffer to another. Typical memory-to-memory 
> > >operations
> >> are: rotation, scaling, colour space conversion or mix of them. This 
> >> is a
> >> follow-up of my previous proposal "[RFC 0/2] New feature: Framebuffer
> >> processors", which has been rejected as "not really needed in the DRM
> >> core":
> >> http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg146286.html
> >>
> >> In this proposal I moved all the code to Exynos DRM driver, so now this
> >> will be specific only to Exynos DRM. I've also changed the name from
> >> framebuffer processor (fbproc) to picture processor (pp) to avoid 
> >> confusion
> >> with fbdev API.
> >>
> >> Here is a bit more information what picture processors are:
> >>
> >> Embedded SoCs are known to have a number of hardware blocks, which 
> >> perform
> >> such operations. They can be used in paralel to the main GPU module to
> >> offload CPU from processing grapics or video data. One of example use 
> >> of
> >> such modules is implementing video overlay, which usually requires 
> >> color
> >> space conversion from NV12 (or similar) to RGB32 color space and 
> >> scaling to
> >> target window size.
> >>
> >> The proposed API is heavily inspired by atomic KMS approach - it is 
> >> also
> >> based on DRM objects and their properties. A new DRM object is 
> >> introduced:
> >> picture processor (called pp for convenience). Such objects have a set 
> >> of
> >> standard DRM properties, which describes the operation to be performed 
> >> by
> >> respective hardware module. In typical case those properties are a 
> >> source
> >> fb id and rectangle (x, y, width, height) and destination fb id and
> >> rectangle. Optionally a rotation property can be also specified if
> >> supported by the given hardware. To perform an operation on image data,
> >> userspace provides a set of properties and their values for given 
> >> fbproc
> >> object in a similar way as object and properties are provided for
> >> performing atomic page flip / mode setting.
> >>
> >> The proposed API consists of the 3 new ioctls:
> >> - DRM_IOCTL_EXYNOS_PP_GET_RESOURCES: to enumerate all available picture
> >>   processors,
> >> - DRM_IOCTL_EXYNOS_PP_GET: to query capabilities of given picture
> >>   processor,
> >> - DRM_IOCTL_EXYNOS_PP_COMMIT: to perform operation described by given
> >>   property set.
> >>
> >> The proposed API is extensible. Drivers can attach their own, custom
> >> properties to add support for more advanced picture processing (for 
> >> example
> >> blending).
> >>
> >> This proposal aims to replace Exynos DRM IPP (Image Post Processing)
> >> subsystem. IPP API is over-engineered in general, but not really 
> >> extensible
> >> on the other side. It is also buggy, with significant design flaws - 
> >> the
> >> biggest issue is the fact that the API covers memory-2-memory picture
> >> operations together with CRTC writeback and duplicating features, which
> >> belongs to video plane. Comparing with IPP subsystem, the PP framework 
> >> is
> >> smaller (1807 vs 778 lines) and allows driver simplification (Exynos
> >> rotator driver smaller by over 200 lines).
> > This seems to be the kind of hardware that is typically supported by 
> > V4L2.
> > Stupid question, why DRM ?
> 
>  Let me elaborate a bit on the reasons for implementing it in Exynos DRM:
> 
>  1. we want to replace existing Exynos IPP subsystem:
>   - it is used only in some internal/vendor trees, not in open-source
>   - we want it to have sane and potentially extensible userspace API
>   - but we don't want to loose its functionality
> 
>  2. we want to have simple API for performing single image processing
>  operation:
>   - typically it will be used by compositing window manager, this means 
>  that
> some parameters of the 

Re: [RFC 0/4] Exynos DRM: add Picture Processor extension

[Tomasz Figa]

On Wed, May 10, 2017 at 2:27 PM, Inki Dae  wrote:
> Hi Tomasz,
>
> 2017년 05월 10일 14:38에 Tomasz Figa 이(가) 쓴 글:
>> Hi Everyone,
>>
>> On Wed, May 10, 2017 at 9:24 AM, Inki Dae  wrote:
>>>
>>>
>>> 2017년 04월 26일 07:21에 Sakari Ailus 이(가) 쓴 글:
 Hi Marek,

 On Thu, Apr 20, 2017 at 01:23:09PM +0200, Marek Szyprowski wrote:
> Hi Laurent,
>
> On 2017-04-20 12:25, Laurent Pinchart wrote:
>> Hi Marek,
>>
>> (CC'ing Sakari Ailus)
>>
>> Thank you for the patches.
>>
>> On Thursday 20 Apr 2017 11:13:36 Marek Szyprowski wrote:
>>> Dear all,
>>>
>>> This is an updated proposal for extending EXYNOS DRM API with generic
>>> support for hardware modules, which can be used for processing image 
>>> data
>> >from the one memory buffer to another. Typical memory-to-memory 
>> >operations
>>> are: rotation, scaling, colour space conversion or mix of them. This is 
>>> a
>>> follow-up of my previous proposal "[RFC 0/2] New feature: Framebuffer
>>> processors", which has been rejected as "not really needed in the DRM
>>> core":
>>> http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg146286.html
>>>
>>> In this proposal I moved all the code to Exynos DRM driver, so now this
>>> will be specific only to Exynos DRM. I've also changed the name from
>>> framebuffer processor (fbproc) to picture processor (pp) to avoid 
>>> confusion
>>> with fbdev API.
>>>
>>> Here is a bit more information what picture processors are:
>>>
>>> Embedded SoCs are known to have a number of hardware blocks, which 
>>> perform
>>> such operations. They can be used in paralel to the main GPU module to
>>> offload CPU from processing grapics or video data. One of example use of
>>> such modules is implementing video overlay, which usually requires color
>>> space conversion from NV12 (or similar) to RGB32 color space and 
>>> scaling to
>>> target window size.
>>>
>>> The proposed API is heavily inspired by atomic KMS approach - it is also
>>> based on DRM objects and their properties. A new DRM object is 
>>> introduced:
>>> picture processor (called pp for convenience). Such objects have a set 
>>> of
>>> standard DRM properties, which describes the operation to be performed 
>>> by
>>> respective hardware module. In typical case those properties are a 
>>> source
>>> fb id and rectangle (x, y, width, height) and destination fb id and
>>> rectangle. Optionally a rotation property can be also specified if
>>> supported by the given hardware. To perform an operation on image data,
>>> userspace provides a set of properties and their values for given fbproc
>>> object in a similar way as object and properties are provided for
>>> performing atomic page flip / mode setting.
>>>
>>> The proposed API consists of the 3 new ioctls:
>>> - DRM_IOCTL_EXYNOS_PP_GET_RESOURCES: to enumerate all available picture
>>>   processors,
>>> - DRM_IOCTL_EXYNOS_PP_GET: to query capabilities of given picture
>>>   processor,
>>> - DRM_IOCTL_EXYNOS_PP_COMMIT: to perform operation described by given
>>>   property set.
>>>
>>> The proposed API is extensible. Drivers can attach their own, custom
>>> properties to add support for more advanced picture processing (for 
>>> example
>>> blending).
>>>
>>> This proposal aims to replace Exynos DRM IPP (Image Post Processing)
>>> subsystem. IPP API is over-engineered in general, but not really 
>>> extensible
>>> on the other side. It is also buggy, with significant design flaws - the
>>> biggest issue is the fact that the API covers memory-2-memory picture
>>> operations together with CRTC writeback and duplicating features, which
>>> belongs to video plane. Comparing with IPP subsystem, the PP framework 
>>> is
>>> smaller (1807 vs 778 lines) and allows driver simplification (Exynos
>>> rotator driver smaller by over 200 lines).
>> This seems to be the kind of hardware that is typically supported by 
>> V4L2.
>> Stupid question, why DRM ?
>
> Let me elaborate a bit on the reasons for implementing it in Exynos DRM:
>
> 1. we want to replace existing Exynos IPP subsystem:
>  - it is used only in some internal/vendor trees, not in open-source
>  - we want it to have sane and potentially extensible userspace API
>  - but we don't want to loose its functionality
>
> 2. we want to have simple API for performing single image processing
> operation:
>  - typically it will be used by compositing window manager, this means 
> that
>some parameters of the processing might change on each vblank (like
>destination rectangle for example). This api allows such change on each
>operation 

[PATCH[ v4l2-ioctl.c: always copy G/S_EDID result

[Hans Verkuil]

The VIDIOC_G/S_EDID ioctls can return valid data even if an error is returned.

Mark those ioctls accordingly. Rather than using an explicit 'if' to check for 
the
ioctl (as was done until now for VIDIOC_QUERY_DV_TIMINGS) just set a new flag 
in the
v4l2_ioctls array.

Signed-off-by: Hans Verkuil 
---
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
b/drivers/media/v4l2-core/v4l2-ioctl.c
index e5a2187381db..4f27cfa134a1 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2472,20 +2472,22 @@ struct v4l2_ioctl_info {
 };

 /* This control needs a priority check */
-#define INFO_FL_PRIO   (1 << 0)
+#define INFO_FL_PRIO   (1 << 0)
 /* This control can be valid if the filehandle passes a control handler. */
-#define INFO_FL_CTRL   (1 << 1)
+#define INFO_FL_CTRL   (1 << 1)
 /* This is a standard ioctl, no need for special code */
-#define INFO_FL_STD(1 << 2)
+#define INFO_FL_STD(1 << 2)
 /* This is ioctl has its own function */
-#define INFO_FL_FUNC   (1 << 3)
+#define INFO_FL_FUNC   (1 << 3)
 /* Queuing ioctl */
-#define INFO_FL_QUEUE  (1 << 4)
+#define INFO_FL_QUEUE  (1 << 4)
+/* Always copy back result, even on error */
+#define INFO_FL_ALWAYS_COPY(1 << 5)
 /* Zero struct from after the field to the end */
 #define INFO_FL_CLEAR(v4l2_struct, field)  \
((offsetof(struct v4l2_struct, field) + \
  sizeof(((struct v4l2_struct *)0)->field)) << 16)
-#define INFO_FL_CLEAR_MASK (_IOC_SIZEMASK << 16)
+#define INFO_FL_CLEAR_MASK (_IOC_SIZEMASK << 16)

 #define IOCTL_INFO_STD(_ioctl, _vidioc, _debug, _flags)
\
[_IOC_NR(_ioctl)] = {   \
@@ -2536,8 +2538,8 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
IOCTL_INFO_FNC(VIDIOC_QUERYMENU, v4l_querymenu, v4l_print_querymenu, 
INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_querymenu, index)),
IOCTL_INFO_STD(VIDIOC_G_INPUT, vidioc_g_input, v4l_print_u32, 0),
IOCTL_INFO_FNC(VIDIOC_S_INPUT, v4l_s_input, v4l_print_u32, 
INFO_FL_PRIO),
-   IOCTL_INFO_STD(VIDIOC_G_EDID, vidioc_g_edid, v4l_print_edid, 0),
-   IOCTL_INFO_STD(VIDIOC_S_EDID, vidioc_s_edid, v4l_print_edid, 
INFO_FL_PRIO),
+   IOCTL_INFO_STD(VIDIOC_G_EDID, vidioc_g_edid, v4l_print_edid, 
INFO_FL_ALWAYS_COPY),
+   IOCTL_INFO_STD(VIDIOC_S_EDID, vidioc_s_edid, v4l_print_edid, 
INFO_FL_PRIO | INFO_FL_ALWAYS_COPY),
IOCTL_INFO_STD(VIDIOC_G_OUTPUT, vidioc_g_output, v4l_print_u32, 0),
IOCTL_INFO_FNC(VIDIOC_S_OUTPUT, v4l_s_output, v4l_print_u32, 
INFO_FL_PRIO),
IOCTL_INFO_FNC(VIDIOC_ENUMOUTPUT, v4l_enumoutput, v4l_print_enumoutput, 
INFO_FL_CLEAR(v4l2_output, index)),
@@ -2583,7 +2585,7 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
IOCTL_INFO_FNC(VIDIOC_CREATE_BUFS, v4l_create_bufs, 
v4l_print_create_buffers, INFO_FL_PRIO | INFO_FL_QUEUE),
IOCTL_INFO_FNC(VIDIOC_PREPARE_BUF, v4l_prepare_buf, v4l_print_buffer, 
INFO_FL_QUEUE),
IOCTL_INFO_STD(VIDIOC_ENUM_DV_TIMINGS, vidioc_enum_dv_timings, 
v4l_print_enum_dv_timings, INFO_FL_CLEAR(v4l2_enum_dv_timings, pad)),
-   IOCTL_INFO_STD(VIDIOC_QUERY_DV_TIMINGS, vidioc_query_dv_timings, 
v4l_print_dv_timings, 0),
+   IOCTL_INFO_STD(VIDIOC_QUERY_DV_TIMINGS, vidioc_query_dv_timings, 
v4l_print_dv_timings, INFO_FL_ALWAYS_COPY),
IOCTL_INFO_STD(VIDIOC_DV_TIMINGS_CAP, vidioc_dv_timings_cap, 
v4l_print_dv_timings_cap, INFO_FL_CLEAR(v4l2_dv_timings_cap, type)),
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)),
@@ -2801,6 +2803,7 @@ video_usercopy(struct file *file, unsigned int cmd, 
unsigned long arg,
void*parg = (void *)arg;
longerr  = -EINVAL;
boolhas_array_args;
+   boolalways_copy = false;
size_t  array_size = 0;
void __user *user_ptr = NULL;
void**kernel_ptr = NULL;
@@ -2830,8 +2833,10 @@ video_usercopy(struct file *file, unsigned int cmd, 
unsigned long arg,
 */
if (v4l2_is_known_ioctl(cmd)) {
u32 flags = v4l2_ioctls[_IOC_NR(cmd)].flags;
+
if (flags & INFO_FL_CLEAR_MASK)
n = (flags & INFO_FL_CLEAR_MASK) >> 16;
+   always_copy = flags & INFO_FL_ALWAYS_COPY;
}

if (copy_from_user(parg, (void __user *)arg, n))
@@ -2885,9 +2890,11 @@ video_usercopy(struct file *file, unsigned int cmd, 
unsigned long arg,
err = -EFAULT;
goto out_array_args;
}
-   /* VIDIOC_QUERY_DV_TIMINGS can return an error, but still have valid
-  results 

Re: [RFC 0/4] Exynos DRM: add Picture Processor extension

[Inki Dae]

Hi Tomasz,

2017년 05월 10일 14:38에 Tomasz Figa 이(가) 쓴 글:
> Hi Everyone,
> 
> On Wed, May 10, 2017 at 9:24 AM, Inki Dae  wrote:
>>
>>
>> 2017년 04월 26일 07:21에 Sakari Ailus 이(가) 쓴 글:
>>> Hi Marek,
>>>
>>> On Thu, Apr 20, 2017 at 01:23:09PM +0200, Marek Szyprowski wrote:
 Hi Laurent,

 On 2017-04-20 12:25, Laurent Pinchart wrote:
> Hi Marek,
>
> (CC'ing Sakari Ailus)
>
> Thank you for the patches.
>
> On Thursday 20 Apr 2017 11:13:36 Marek Szyprowski wrote:
>> Dear all,
>>
>> This is an updated proposal for extending EXYNOS DRM API with generic
>> support for hardware modules, which can be used for processing image data
> >from the one memory buffer to another. Typical memory-to-memory 
> >operations
>> are: rotation, scaling, colour space conversion or mix of them. This is a
>> follow-up of my previous proposal "[RFC 0/2] New feature: Framebuffer
>> processors", which has been rejected as "not really needed in the DRM
>> core":
>> http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg146286.html
>>
>> In this proposal I moved all the code to Exynos DRM driver, so now this
>> will be specific only to Exynos DRM. I've also changed the name from
>> framebuffer processor (fbproc) to picture processor (pp) to avoid 
>> confusion
>> with fbdev API.
>>
>> Here is a bit more information what picture processors are:
>>
>> Embedded SoCs are known to have a number of hardware blocks, which 
>> perform
>> such operations. They can be used in paralel to the main GPU module to
>> offload CPU from processing grapics or video data. One of example use of
>> such modules is implementing video overlay, which usually requires color
>> space conversion from NV12 (or similar) to RGB32 color space and scaling 
>> to
>> target window size.
>>
>> The proposed API is heavily inspired by atomic KMS approach - it is also
>> based on DRM objects and their properties. A new DRM object is 
>> introduced:
>> picture processor (called pp for convenience). Such objects have a set of
>> standard DRM properties, which describes the operation to be performed by
>> respective hardware module. In typical case those properties are a source
>> fb id and rectangle (x, y, width, height) and destination fb id and
>> rectangle. Optionally a rotation property can be also specified if
>> supported by the given hardware. To perform an operation on image data,
>> userspace provides a set of properties and their values for given fbproc
>> object in a similar way as object and properties are provided for
>> performing atomic page flip / mode setting.
>>
>> The proposed API consists of the 3 new ioctls:
>> - DRM_IOCTL_EXYNOS_PP_GET_RESOURCES: to enumerate all available picture
>>   processors,
>> - DRM_IOCTL_EXYNOS_PP_GET: to query capabilities of given picture
>>   processor,
>> - DRM_IOCTL_EXYNOS_PP_COMMIT: to perform operation described by given
>>   property set.
>>
>> The proposed API is extensible. Drivers can attach their own, custom
>> properties to add support for more advanced picture processing (for 
>> example
>> blending).
>>
>> This proposal aims to replace Exynos DRM IPP (Image Post Processing)
>> subsystem. IPP API is over-engineered in general, but not really 
>> extensible
>> on the other side. It is also buggy, with significant design flaws - the
>> biggest issue is the fact that the API covers memory-2-memory picture
>> operations together with CRTC writeback and duplicating features, which
>> belongs to video plane. Comparing with IPP subsystem, the PP framework is
>> smaller (1807 vs 778 lines) and allows driver simplification (Exynos
>> rotator driver smaller by over 200 lines).
> This seems to be the kind of hardware that is typically supported by V4L2.
> Stupid question, why DRM ?

 Let me elaborate a bit on the reasons for implementing it in Exynos DRM:

 1. we want to replace existing Exynos IPP subsystem:
  - it is used only in some internal/vendor trees, not in open-source
  - we want it to have sane and potentially extensible userspace API
  - but we don't want to loose its functionality

 2. we want to have simple API for performing single image processing
 operation:
  - typically it will be used by compositing window manager, this means that
some parameters of the processing might change on each vblank (like
destination rectangle for example). This api allows such change on each
operation without any additional cost. V4L2 requires to reinitialize
queues with new configuration on such change, what means that a bunch of
ioctls has to be called.
>>>
>>> What do you mean by re-initialising the queue? Format, buffers or