RE: [PATCH v4 1/1] i2c: Add Omnivision OV5670 5M sensor support

[Rapolu, Chiranjeevi]

Raj,

Removed duplicate const.

Thanks,
Chiran
-Original Message-
From: Mani, Rajmohan 
Sent: Friday, July 7, 2017 3:49 PM
To: Rapolu, Chiranjeevi ; 
linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; tf...@chromium.org
Cc: Zheng, Jian Xu ; Yang, Hyungwoo 

Subject: RE: [PATCH v4 1/1] i2c: Add Omnivision OV5670 5M sensor support

Hi Chiran,

> +
> +/* Supported link frequencies */
> +#define OV5670_LINK_FREQ_840MBPS 84000
> +#define OV5670_LINK_FREQ_840MBPS_INDEX   0
> +static const struct ov5670_link_freq_config link_freq_configs[] = {
> + {
> + .pixel_rate = 33600,
> + .reg_list = {
> + .num_of_regs =
> ARRAY_SIZE(mipi_data_rate_840mbps),
> + .regs = mipi_data_rate_840mbps,
> + }
> + }
> +};
> +
> +static const const s64 link_freq_menu_items[] = {

 ^ duplicated const declaration

> + OV5670_LINK_FREQ_840MBPS
> +};
> +

RE: [PATCH v4 1/1] i2c: Add Omnivision OV5670 5M sensor support

[Rapolu, Chiranjeevi]

Sakari,

Thanks for the review. Submitted v5, responses embedded below.

Thanks,
Chiran.
-Original Message-
>Hi Chiranjeevi,
>
>A few relatively minor comments below.
>
>On Sat, Jul 01, 2017 at 01:49:06PM -0700, chiranjeevi.rap...@intel.com wrote:
>> From: Chiranjeevi Rapolu 
>> 
>> Provides single source pad with up to 2592x1944 pixels at 10-bit raw
>> bayer format over MIPI CSI2 two lanes at 840Mbps/lane.
>> The driver supports following features:
>> - up to  30fps at 5M pixels
>> - manual exposure
>> - digital/analog gain
>> - V-blank/H-blank
>> - test pattern
>> - media controller
>> - runtime pm
>> 
>> Signed-off-by: Chiranjeevi Rapolu 
>> ---
>>  drivers/media/i2c/Kconfig  |   12 +
>>  drivers/media/i2c/Makefile |1 +
>>  drivers/media/i2c/ov5670.c | 2591 
>> 
>>  3 files changed, 2604 insertions(+)
>>  create mode 100644 drivers/media/i2c/ov5670.c
>> 
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index 121b3b5..e0f2411 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -593,6 +593,18 @@ config VIDEO_OV5647
>>To compile this driver as a module, choose M here: the
>>module will be called ov5647.
>>  
>> +config VIDEO_OV5670
>> +tristate "OmniVision OV5670 sensor support"
>> +depends on I2C && VIDEO_V4L2
>> +depends on MEDIA_CAMERA_SUPPORT
>> +select V4L2_FWNODE
>> +---help---
>> +  This is a Video4Linux2 sensor-level driver for the OmniVision
>> +  OV5670 camera.
>> +
>> +  To compile this driver as a module, choose M here: the
>> +  module will be called ov5670.
>> +
>>  config VIDEO_OV7640
>>  tristate "OmniVision OV7640 sensor support"
>>  depends on I2C && VIDEO_V4L2
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index 2c0868f..1fb3a1f 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -62,6 +62,7 @@ obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
>>  obj-$(CONFIG_VIDEO_OV5640) += ov5640.o
>>  obj-$(CONFIG_VIDEO_OV5645) += ov5645.o
>>  obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
>> +obj-$(CONFIG_VIDEO_OV5670) += ov5670.o
>>  obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
>>  obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
>>  obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
>> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
>> new file mode 100644
>> index 000..9f1dcb0
>> --- /dev/null
>> +++ b/drivers/media/i2c/ov5670.c
>> @@ -0,0 +1,2591 @@
>> +/*
>> + * Copyright (c) 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 
>> +
>> +#define OV5670_REG_CHIP_ID  0x300a
>> +#define OV5670_CHIP_ID  0x005670
>> +
>> +#define OV5670_REG_MODE_SELECT  0x0100
>> +#define OV5670_MODE_STANDBY 0x00
>> +#define OV5670_MODE_STREAMING   0x01
>> +
>> +#define OV5670_REG_SOFTWARE_RST 0x0103
>> +#define OV5670_SOFTWARE_RST 0x01
>> +
>> +/* vertical-timings from sensor */
>> +#define OV5670_REG_VTS  0x380e
>> +#define OV5670_VTS_30FPS0x0808 /* default for 30 fps */
>> +#define OV5670_VTS_MAX  0x
>> +#define OV5670_VBLANK_MIN   56
>> +
>> +/* horizontal-timings from sensor */
>> +#define OV5670_REG_HTS  0x380c
>> +#define OV5670_DEF_PPL  3360/* Default pixels per 
>> line */
>> +
>> +/* Exposure controls from sensor */
>> +#define OV5670_REG_EXPOSURE 0x3500
>> +#define OV5670_EXPOSURE_MIN 4
>> +#define OV5670_EXPOSURE_STEP1
>> +#define OV5670_EXPOSURE_DEFAULT 1992
>> +
>> +/* Analog gain controls from sensor */
>> +#define OV5670_REG_ANALOG_GAIN  0x3508
>> +#define ANALOG_GAIN_MIN 0
>> +#define ANALOG_GAIN_MAX 8191
>> +#define ANALOG_GAIN_STEP1
>> +#define ANALOG_GAIN_DEFAULT 128
>> +
>> +/* Digital gain controls from sensor */
>> +#define OV5670_REG_R_DGTL_GAIN  0x5032
>> +#define OV5670_REG_G_DGTL_GAIN  0x5034
>> +#define OV5670_REG_B_DGTL_GAIN  0x5036
>> +#define OV5670_DGTL_GAIN_MIN0
>> +#define OV5670_DGTL_GAIN_MAX4095
>> +#define OV5670_DGTL_GAIN_STEP   1
>> +#define OV5670_DGTL_GAIN_DEFAULT1024

cron job: media_tree daily build: ERRORS

[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:   Fri Jul 14 05:00:23 CEST 2017
media-tree git hash:2748e76ddb2967c4030171342ebdd3faa6a5e8e8
media_build git hash:   bc1db0a204a87da86349ea5e64ae0d65e945609d
v4l-utils git hash: 8e68406dae2233e811032dc8e7714c09c818e893
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-arm-stm32: OK
linux-git-blackfin-bf561: OK
linux-git-i686: OK
linux-git-m32r: OK
linux-git-mips: OK
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: 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: OK
linux-3.0.60-i686: OK
linux-3.1.10-i686: OK
linux-3.2.37-i686: OK
linux-3.3.8-i686: OK
linux-3.4.27-i686: ERRORS
linux-3.5.7-i686: OK
linux-3.6.11-i686: OK
linux-3.7.4-i686: OK
linux-3.8-i686: OK
linux-3.9.2-i686: OK
linux-3.10.1-i686: WARNINGS
linux-3.11.1-i686: OK
linux-3.12.67-i686: OK
linux-3.13.11-i686: WARNINGS
linux-3.14.9-i686: ERRORS
linux-3.15.2-i686: ERRORS
linux-3.16.7-i686: ERRORS
linux-3.17.8-i686: ERRORS
linux-3.18.7-i686: ERRORS
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: OK
linux-4.9.26-i686: OK
linux-4.10.14-i686: OK
linux-4.11-i686: OK
linux-4.12.1-i686: OK
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: ERRORS
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: ERRORS
linux-3.15.2-x86_64: ERRORS
linux-3.16.7-x86_64: ERRORS
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
linux-4.12.1-x86_64: WARNINGS
apps: WARNINGS
spec-git: OK
sparse: WARNINGS

Detailed results are available here:

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

Full logs are available here:

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

The Media Infrastructure API from this daily build is here:

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

Re: [PATCH 1/1 V2] media: usb: uvc: Fix incorrect timeout for Get Request

[Jim Lin]

On 2017年07月11日 03:47, Laurent Pinchart wrote:

Hi Jim,

Thank you for the patch.

On Monday 10 Jul 2017 14:43:49 Jim Lin wrote:

Section 9.2.6.4 of USB 2.0/3.x specification describes that
"device must be able to return the first data packet to host within
500 ms of receipt of the request. For subsequent data packet, if any,
the device must be able to return them within 500 ms".

This is to fix incorrect timeout and change it from 300 ms to 500 ms
to meet the timing specified by specification for Get Request.

Signed-off-by: Jim Lin 


The patch looks good to me, so

Reviewed-by: Laurent Pinchart 

but I'm curious, have you noticed issues with some devices in practice ?



Sometimes this device takes about 360 ms to respond.

usb 1-2: new high-speed USB device number 16
usb 1-2: New USB device found, idVendor=045e, idProduct=0772
usb 1-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0
usb 1-2: Product: Microsoft�® LifeCam Studio(TM)
usb 1-2: Manufacturer: Microsoft
:
uvcvideo: Failed to query (GET_DEF) UVC control 2 on unit 4: -110 (exp. 2).

And it will be working well with correct timeout value.

--nvpublic

[PATCH v5 1/1] i2c: Add Omnivision OV5670 5M sensor support

[Chiranjeevi Rapolu]

Provides single source pad with up to 2592x1944 pixels at 10-bit raw
bayer format over MIPI CSI2 two lanes at 840Mbps/lane.
The driver supports following features:
- up to  30fps at 5M pixels
- manual exposure
- digital/analog gain
- V-blank/H-blank
- test pattern
- media controller
- runtime pm

Signed-off-by: Chiranjeevi Rapolu 
---
 drivers/media/i2c/Kconfig  |   12 +
 drivers/media/i2c/Makefile |1 +
 drivers/media/i2c/ov5670.c | 2587 
 3 files changed, 2600 insertions(+)
 create mode 100644 drivers/media/i2c/ov5670.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 121b3b5..e0f2411 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -593,6 +593,18 @@ config VIDEO_OV5647
  To compile this driver as a module, choose M here: the
  module will be called ov5647.
 
+config VIDEO_OV5670
+   tristate "OmniVision OV5670 sensor support"
+   depends on I2C && VIDEO_V4L2
+   depends on MEDIA_CAMERA_SUPPORT
+   select V4L2_FWNODE
+   ---help---
+ This is a Video4Linux2 sensor-level driver for the OmniVision
+ OV5670 camera.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ov5670.
+
 config VIDEO_OV7640
tristate "OmniVision OV7640 sensor support"
depends on I2C && VIDEO_V4L2
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 2c0868f..1fb3a1f 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -62,6 +62,7 @@ obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
 obj-$(CONFIG_VIDEO_OV5640) += ov5640.o
 obj-$(CONFIG_VIDEO_OV5645) += ov5645.o
 obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
+obj-$(CONFIG_VIDEO_OV5670) += ov5670.o
 obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
 obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
 obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
new file mode 100644
index 000..909095a
--- /dev/null
+++ b/drivers/media/i2c/ov5670.c
@@ -0,0 +1,2587 @@
+/*
+ * Copyright (c) 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 
+
+#define OV5670_REG_CHIP_ID 0x300a
+#define OV5670_CHIP_ID 0x005670
+
+#define OV5670_REG_MODE_SELECT 0x0100
+#define OV5670_MODE_STANDBY0x00
+#define OV5670_MODE_STREAMING  0x01
+
+#define OV5670_REG_SOFTWARE_RST0x0103
+#define OV5670_SOFTWARE_RST0x01
+
+/* vertical-timings from sensor */
+#define OV5670_REG_VTS 0x380e
+#define OV5670_VTS_30FPS   0x0808 /* default for 30 fps */
+#define OV5670_VTS_MAX 0x
+#define OV5670_VBLANK_MIN  56
+
+/* horizontal-timings from sensor */
+#define OV5670_REG_HTS 0x380c
+#define OV5670_DEF_PPL 3360/* Default pixels per line */
+
+/* Exposure controls from sensor */
+#define OV5670_REG_EXPOSURE0x3500
+#defineOV5670_EXPOSURE_MIN 4
+#defineOV5670_EXPOSURE_STEP1
+
+/* Analog gain controls from sensor */
+#define OV5670_REG_ANALOG_GAIN 0x3508
+#defineANALOG_GAIN_MIN 0
+#defineANALOG_GAIN_MAX 8191
+#defineANALOG_GAIN_STEP1
+#defineANALOG_GAIN_DEFAULT 128
+
+/* Digital gain controls from sensor */
+#define OV5670_REG_R_DGTL_GAIN 0x5032
+#define OV5670_REG_G_DGTL_GAIN 0x5034
+#define OV5670_REG_B_DGTL_GAIN 0x5036
+#define OV5670_DGTL_GAIN_MIN   0
+#define OV5670_DGTL_GAIN_MAX   4095
+#define OV5670_DGTL_GAIN_STEP  1
+#define OV5670_DGTL_GAIN_DEFAULT   1024
+
+/* Test Pattern Control */
+#define OV5670_REG_TEST_PATTERN0x4303
+#define OV5670_TEST_PATTERN_ENABLE BIT(3)
+#define OV5670_REG_TEST_PATTERN_CTRL   0x4320
+
+#define OV5670_REG_VALUE_08BIT 1
+#define OV5670_REG_VALUE_16BIT 2
+#define OV5670_REG_VALUE_24BIT 3
+
+/* Initial number of frames to skip to avoid possible garbage */
+#define OV5670_NUM_OF_SKIP_FRAMES  2
+
+struct ov5670_reg {
+   u16 address;
+   u8 val;
+};
+
+struct ov5670_reg_list {
+   u32 num_of_regs;
+   const struct ov5670_reg *regs;
+};
+
+struct ov5670_link_freq_config {
+   u32 pixel_rate;
+   const struct ov5670_reg_list reg_list;
+};
+
+struct ov5670_mode {
+   /* Frame 

rc-core: how to use hid (hardware) decoder?

[Antti Palosaari]

Moikka!
Some remote controller receivers uses HID interface. I looked rc-core 
implementation, but failed to find how it could be used for hid. I need 
somehow get scancodes and keycodes out from rc-core and write those to 
hardware which then generate hid events. Also, I am not sure if kernel 
keycodes are same than HID codes, but if not then those should be 
translated somehow.


There is rc_g_keycode_from_table() function, which could be used to dump 
current scancode:keycode mapping, but calling it in a loop millions of 
times is not surely correctly :]


Any ideas?

regards
Antti


--
http://palosaari.fi/

Re: [PATCH v2 00/14] Renesas R-Car VSP: Add H3 ES2.0 support

[Laurent Pinchart]

Hi Kieran,

On Thursday 13 Jul 2017 13:25:55 Kieran Bingham wrote:
> Hi Laurent,
> 
> Thank you for these patches, bringing life to the outputs of my ES2.0 target
> board.
> 
> I have tested them on my board, and including the VSP unit test suite, and
> kmscube utilities.
> 
> Feel free to add a Tested-by: Kieran Bingham
>  to all of the patches if you
> desire, and I'm working through the individual reviews.
> 
> Oh - except now I've just said that - I did some extra testing with both
> HDMI and VGA output connected and ran
>kmstest --flip --sync
> 
> This was soon followed by a kernel error trace [0] shown below.

:-/

> However replicating this is possibly more complicated than just running
> kmstest --flip --sync ... I've had to do various iterations of running
> with/without {flip, sync} but I have reproduced 'issues' about 3 times now.
> 
> I think the key thing is in the VGA connection or regarding trying to output
> to both.
> 
> Interestingly, I haven't been able to make this happen on the ES1.0 platform
> as yet... though --flip --sync is quicker to fail with 'Flip Commit failed:
> -16" there...
> 
> For reference this was tested on your
> pinchartl-media/drm/next/h3-es2/merged, branch which I don't believe has
> the recent work on not needing to wait for a final vblank on shutdown. So
> it is quite possible that the issue I am seeing is simply a symptom of that
> separately repaired issue.

That's possible, or it could also be related to the vblank race and the 
"[PATCH] drm: rcar-du: Wait for flip completion instead of vblank in commit 
tail" patch that I've just posted.

> On that basis I've left my comment regarding my Tested-by: tag above as I
> suspect that this issue I've hit could likely be separate and already
> resolved.
> 
> I'll try to add those patches to this tree to see if the issue resolves
> itself...

I've updated my drm/next/h3-es2/merged branch, could you please test it ? I've 
included "[PATCH] drm: rcar-du: Wait for flip completion instead of vblank in 
commit tail", you may want to try and revert it if it causes issues.

> Regardless of that, this series conflicts with my current developments,
> therefore I will likely rebase my work on top of this series. I don't need
> an immutable branch, but please do let me know if this series changes :-)

I will.

> [0] : Kernel log snippet posted below:
> 
> [  597.471369] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
> [CRTC:57:crtc-3] flip_done timed out
> [  607.711346] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
> [CRTC:57:crtc-3] flip_done timed out
> [  607.711354] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
> [CRTC:57:crtc-3] flip_done timed out
> [  607.749585] vsp1 fea2.vsp: failed to reset wpf.1
> [  617.951311] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
> [CRTC:57:crtc-3] flip_done timed out
> [  618.055358] vsp1 fea2.vsp: failed to reset wpf.1
> [  618.060498] vsp1 fea2.vsp: DRM pipeline stop timeout
> [  628.831762] vsp1 fea2.vsp: failed to reset wpf.1
> [  638.943315] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
> [CRTC:57:crtc-3] flip_done timed out
> [  649.183313] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
> [CRTC:57:crtc-3] flip_done timed out
> [  677.753465] vsp1 fea2.vsp: failed to reset wpf.1
> [  687.839322] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
> [CRTC:57:crtc-3] flip_done timed out
> [  687.935288] vsp1 fea2.vsp: failed to reset wpf.1
> [  687.940360] vsp1 fea2.vsp: DRM pipeline stop timeout
> [  687.945939] Unable to handle kernel NULL pointer dereference at virtual
> address 
> [  687.954206] pgd = ff8009a0a000
> [  687.957680] [] *pgd=00073fffe003, *pud=00073fffe003,
> *pmd=
> [  687.966068] Internal error: Oops: 9606 [#1] PREEMPT SMP
> [  687.971690] Modules linked in:
> [  687.974777] CPU: 0 PID: 10426 Comm: kmstest Not tainted
> 4.12.0-rc5-01343-g1bc824acf27c #5
> [  687.983019] Hardware name: Renesas Salvator-X 2nd version board based on
> r8a7795 ES2.0+ (DT)
> [  687.991525] task: ffc6f97b4080 task.stack: ffc6f51dc000
> [  687.997502] PC is at __media_pipeline_stop+0x24/0xd0
> [  688.002507] LR is at media_pipeline_stop+0x34/0x48
> [  688.007336] pc : [] lr : [] pstate:
> 6145 [  688.014790] sp : ffc6f51df780
> [  688.018129] x29: ffc6f51df780 x28: 
> [  688.023490] x27: 0038 x26: ff8008960088
> [  688.028850] x25: ffc6f98bcb10 x24: ffc6f98b8818
> [  688.034210] x23: 0001 x22: ffc6f9ff1998
> [  688.039570] x21: ffc6f98bc080 x20: 
> [  688.044929] x19: 0008 x18: 0010
> [  688.050288] x17: 007f9bc9e1c8 x16: ff8008165bb8
> [  688.055648] x15: ff80899bb63f x14: 0006
> [  688.061007] x13: ffc6faac6750 x12: ff80087bd078
> [  688.066366] x11:  x10: 

[PATCH v2.1 08/14] v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and VSP2-D instances

[Laurent Pinchart]

New Gen3 SoCs come with two new VSP2 variants names VSP2-BS and VSP2-DL,
as well as a new VSP2-D variant on V3M and V3H SoCs. Add new entries for
them in the VSP device info table.

Signed-off-by: Laurent Pinchart 
Reviewed-by: Kieran Bingham 
---
Changes since v2:

- Added VSP1_HAS_WPF_VFLIP to VSP2-BS
---
 drivers/media/platform/vsp1/vsp1_drv.c  | 24 
 drivers/media/platform/vsp1/vsp1_regs.h | 15 +--
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drv.c 
b/drivers/media/platform/vsp1/vsp1_drv.c
index 6a9aeb71aedf..d1682a9719af 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -710,6 +710,14 @@ static const struct vsp1_device_info vsp1_device_infos[] = 
{
.num_bru_inputs = 5,
.uapi = true,
}, {
+   .version = VI6_IP_VERSION_MODEL_VSPBS_GEN3,
+   .model = "VSP2-BS",
+   .gen = 3,
+   .features = VSP1_HAS_BRS | VSP1_HAS_WPF_VFLIP,
+   .rpf_count = 2,
+   .wpf_count = 1,
+   .uapi = true,
+   }, {
.version = VI6_IP_VERSION_MODEL_VSPD_GEN3,
.model = "VSP2-D",
.gen = 3,
@@ -717,6 +725,22 @@ static const struct vsp1_device_info vsp1_device_infos[] = 
{
.rpf_count = 5,
.wpf_count = 2,
.num_bru_inputs = 5,
+   }, {
+   .version = VI6_IP_VERSION_MODEL_VSPD_V3,
+   .model = "VSP2-D",
+   .gen = 3,
+   .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF,
+   .rpf_count = 5,
+   .wpf_count = 1,
+   .num_bru_inputs = 5,
+   }, {
+   .version = VI6_IP_VERSION_MODEL_VSPDL_GEN3,
+   .model = "VSP2-DL",
+   .gen = 3,
+   .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF,
+   .rpf_count = 5,
+   .wpf_count = 2,
+   .num_bru_inputs = 5,
},
 };
 
diff --git a/drivers/media/platform/vsp1/vsp1_regs.h 
b/drivers/media/platform/vsp1/vsp1_regs.h
index 744217e020b9..ab439a60a100 100644
--- a/drivers/media/platform/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/vsp1/vsp1_regs.h
@@ -699,9 +699,20 @@
 #define VI6_IP_VERSION_MODEL_VSPBD_GEN3(0x15 << 8)
 #define VI6_IP_VERSION_MODEL_VSPBC_GEN3(0x16 << 8)
 #define VI6_IP_VERSION_MODEL_VSPD_GEN3 (0x17 << 8)
+#define VI6_IP_VERSION_MODEL_VSPD_V3   (0x18 << 8)
+#define VI6_IP_VERSION_MODEL_VSPDL_GEN3(0x19 << 8)
+#define VI6_IP_VERSION_MODEL_VSPBS_GEN3(0x1a << 8)
 #define VI6_IP_VERSION_SOC_MASK(0xff << 0)
-#define VI6_IP_VERSION_SOC_H   (0x01 << 0)
-#define VI6_IP_VERSION_SOC_M   (0x02 << 0)
+#define VI6_IP_VERSION_SOC_H2  (0x01 << 0)
+#define VI6_IP_VERSION_SOC_V2H (0x01 << 0)
+#define VI6_IP_VERSION_SOC_V3M (0x01 << 0)
+#define VI6_IP_VERSION_SOC_M2  (0x02 << 0)
+#define VI6_IP_VERSION_SOC_M3W (0x02 << 0)
+#define VI6_IP_VERSION_SOC_V3H (0x02 << 0)
+#define VI6_IP_VERSION_SOC_H3  (0x03 << 0)
+#define VI6_IP_VERSION_SOC_D3  (0x04 << 0)
+#define VI6_IP_VERSION_SOC_M3N (0x04 << 0)
+#define VI6_IP_VERSION_SOC_E3  (0x04 << 0)
 
 /* 
-
  * RPF CLUT Registers
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 08/14] v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and VSP2-D instances

[Laurent Pinchart]

Hi Kieran,

On Thursday 13 Jul 2017 18:49:19 Kieran Bingham wrote:
> On 26/06/17 19:12, Laurent Pinchart wrote:
> > New Gen3 SoCs come with two new VSP2 variants names VSP2-BS and VSP2-DL,
> > as well as a new VSP2-D variant on V3M and V3H SoCs. Add new entries for
> > them in the VSP device info table.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> 
> Code in the patch looks OK - but I can't see where the difference between
> the horizontal widths are supported between VSPD H3/VC
> 
> I see this in the datasheet: (32.1.1.6 in this particular part)
> 
> Direct connection to display module
> — Supporting 4096 pixels in horizontal direction [R-Car H3/R-Car M3-W/ R-Car
> M3-N]
> — Supporting 2048 pixels in horizontal direction [R-Car V3M/R-Car V3H/R-Car
> D3/R-Car E3]
> 
> Do we have this information encoded anywhere? or are they just talking about
> maximum performance capability there?

No, we don't. It's a limit that we should have. I think we should fix that in 
a separate patch, as the 4096 pixels limit isn't implemented either.

> Also some features that are implied as supported aren't mentioned - but
> that's not a blocker to adding in the initial devices at all.
> 
> Therefore:
> 
> Reviewed-by: Kieran Bingham 
> 
> > ---
> > 
> >  drivers/media/platform/vsp1/vsp1_drv.c  | 24 
> >  drivers/media/platform/vsp1/vsp1_regs.h | 15 +--
> >  2 files changed, 37 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> > b/drivers/media/platform/vsp1/vsp1_drv.c index 6a9aeb71aedf..c4f2ac61f7d2
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drv.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> > @@ -710,6 +710,14 @@ static const struct vsp1_device_info
> > vsp1_device_infos[] = {> 
> > .num_bru_inputs = 5,
> > .uapi = true,
> > }, {
> > +   .version = VI6_IP_VERSION_MODEL_VSPBS_GEN3,
> > +   .model = "VSP2-BS",
> > +   .gen = 3,
> > +   .features = VSP1_HAS_BRS,
> 
> 32.1.1.5 implies:
> | VSP1_HAS_WPF_VFLIP
> 
> But Figure 32.5 implies that it doesn't ...

The figures only tell whether the full combination of rotation and H/V flip is 
available. I think you're right, I'll add VSP1_HAS_WPF_VFLIP.

> Figure 32.5 also implies that | VSP1_HAS_CLU is there too on both RPF0, and
> RPF1

Note that CLUT != CLU. I know it's confusing :-)

> > +   .rpf_count = 2,
> > +   .wpf_count = 1,
> > +   .uapi = true,
> > +   }, {
> > .version = VI6_IP_VERSION_MODEL_VSPD_GEN3,
> > .model = "VSP2-D",
> > .gen = 3,
> > @@ -717,6 +725,22 @@ static const struct vsp1_device_info
> > vsp1_device_infos[] = {> 
> > .rpf_count = 5,
> > .wpf_count = 2,
> > .num_bru_inputs = 5,
> > +   }, {
> > +   .version = VI6_IP_VERSION_MODEL_VSPD_V3,
> > +   .model = "VSP2-D",
> > +   .gen = 3,
> > +   .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF,
> > +   .rpf_count = 5,
> > +   .wpf_count = 1,
> > +   .num_bru_inputs = 5,
> > +   }, {
> > +   .version = VI6_IP_VERSION_MODEL_VSPDL_GEN3,
> > +   .model = "VSP2-DL",
> > +   .gen = 3,
> > +   .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF,
> 
> Hrm. 32.1.1.7 says:
> — Vertical flipping in case of output to memory.
> So thats some sort of a conditional : | VSP1_HAS_WPF_VFLIP
> 
> So looking at this and the settings of the existing models, I guess it looks
> like we don't support flip if we have an LIF output (as that would then be
> unsupported)

On Gen3 vertical flipping seems to always be supported, unlike on Gen2 where 
VSPD is specifically documented as not supporting vertical flipping. We could 
add the WFLIP on all VSP2-D* instances. This would create a corresponding 
control, which wouldn't do much harm as the VSPD instances on Gen3 are not 
exposed to userspace, but that would waste a bit of memory for no good purpose 
(beside correctness I suppose). I wonder if that's worth it, what do you think 
? If so, VSP2-D should be fixed too, so I'd prefer doing that in a separate 
patch.

> > +   .rpf_count = 5,
> > +   .wpf_count = 2,
> > +   .num_bru_inputs = 5,
> > },
> >  };
> > 

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 06/14] v4l: vsp1: Add pipe index argument to the VSP-DU API

[Laurent Pinchart]

Hi Kieran,

On Thursday 13 Jul 2017 14:16:03 Kieran Bingham wrote:
> On 26/06/17 19:12, Laurent Pinchart wrote:
> > In the H3 ES2.0 SoC the VSP2-DL instance has two connections to DU
> > channels that need to be configured independently. Extend the VSP-DU API
> > with a pipeline index to identify which pipeline the caller wants to
> > operate on.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> 
> A bit of comment merge between this and the next patch but it's minor and
> not worth the effort to change that ... so I'll happily ignore it if you do
> :)
> 
> Reviewed-by: Kieran Bingham 
> 
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_du_vsp.c  | 12 ++--
> >  drivers/media/platform/vsp1/vsp1_drm.c | 32 +++
> >  include/media/vsp1.h   | 10 ++
> >  3 files changed, 34 insertions(+), 20 deletions(-)

[snip]

> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > b/drivers/media/platform/vsp1/vsp1_drm.c index c72d021ff820..daaafe7885fa
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -58,21 +58,26 @@ EXPORT_SYMBOL_GPL(vsp1_du_init);
> >  /**
> >   * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
> >   * @dev: the VSP device
> > + * @pipe_index: the DRM pipeline index
> >   * @cfg: the LIF configuration
> >   *
> >   * Configure the output part of VSP DRM pipeline for the given frame
> >   @cfg.width
> > - * and @cfg.height. This sets up formats on the BRU source pad, the WPF0
> > sink
> > - * and source pads, and the LIF sink pad.
> > + * and @cfg.height. This sets up formats on the blend unit (BRU or BRS)
> > source
> > + * pad, the WPF sink and source pads, and the LIF sink pad.
> >   *
> > - * As the media bus code on the BRU source pad is conditioned by the
> > - * configuration of the BRU sink 0 pad, we also set up the formats on all
> > BRU
> > + * The @pipe_index argument selects which DRM pipeline to setup. The
> > number of
> > + * available pipelines depend on the VSP instance.
> > + *
> > + * As the media bus code on the blend unit source pad is conditioned by
> > the
> > + * configuration of its sink 0 pad, we also set up the formats on all
> > blend unit
> >   * sinks, even if the configuration will be overwritten later by
> > - * vsp1_du_setup_rpf(). This ensures that the BRU configuration is set to
> > a well
> > - * defined state.
> > + * vsp1_du_setup_rpf(). This ensures that the blend unit configuration is
> > set to
> > + * a well defined state.
> 
> I presume those comment updates for the BRU/ blend-unit configuration are
> actually for the next patch - but I don't think it matters here - and isn't
> worth the effort to move the hunks.

Too late, I've fixed it already :-) Thanks for pointing it out.

> It all reads OK.
> 
> >   *
> >   * Return 0 on success or a negative error code on failure.
> >   */
> > -int vsp1_du_setup_lif(struct device *dev, const struct vsp1_du_lif_config
> > *cfg)
> > +int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
> > + const struct vsp1_du_lif_config *cfg)
> >  {
> > struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> > struct vsp1_pipeline *pipe = >drm->pipe;

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] staging: media: davinci_vpfe: fix spelling mistake in variable

[Laurent Pinchart]

Hi Colin,

Thank you for the patch.

On Thursday 13 Jul 2017 23:34:16 Colin King wrote:
> From: Colin Ian King 
> 
> Trivial fix to spelling mistake, rename the function name
> resizer_configure_in_continious_mode to
> resizer_configure_in_continuous_mode and also remove an extraneous space.
> 
> Signed-off-by: Colin Ian King 

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/staging/media/davinci_vpfe/dm365_resizer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/davinci_vpfe/dm365_resizer.c
> b/drivers/staging/media/davinci_vpfe/dm365_resizer.c index
> 857b0e847c5e..d751d590a894 100644
> --- a/drivers/staging/media/davinci_vpfe/dm365_resizer.c
> +++ b/drivers/staging/media/davinci_vpfe/dm365_resizer.c
> @@ -480,7 +480,7 @@ resizer_configure_common_in_params(struct
> vpfe_resizer_device *resizer) return 0;
>  }
>  static int
> -resizer_configure_in_continious_mode(struct vpfe_resizer_device *resizer)
> +resizer_configure_in_continuous_mode(struct vpfe_resizer_device *resizer)
>  {
>   struct device *dev = resizer->crop_resizer.subdev.v4l2_dev->dev;
>   struct resizer_params *param = >config;
> @@ -1242,7 +1242,7 @@ static int resizer_do_hw_setup(struct
> vpfe_resizer_device *resizer) ipipeif_source == IPIPEIF_OUTPUT_RESIZER)
>   ret = resizer_configure_in_single_shot_mode(resizer);
>   else
> - ret =  resizer_configure_in_continious_mode(resizer);
> + ret = resizer_configure_in_continuous_mode(resizer);
>   if (ret)
>   return ret;
>   ret = config_rsz_hw(resizer, param);

-- 
Regards,

Laurent Pinchart

[PATCH] [media] staging: media: davinci_vpfe: fix spelling mistake in variable

[Colin King]

From: Colin Ian King 

Trivial fix to spelling mistake, rename the function name
resizer_configure_in_continious_mode to
resizer_configure_in_continuous_mode and also remove an extraneous space.

Signed-off-by: Colin Ian King 
---
 drivers/staging/media/davinci_vpfe/dm365_resizer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_resizer.c 
b/drivers/staging/media/davinci_vpfe/dm365_resizer.c
index 857b0e847c5e..d751d590a894 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_resizer.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_resizer.c
@@ -480,7 +480,7 @@ resizer_configure_common_in_params(struct 
vpfe_resizer_device *resizer)
return 0;
 }
 static int
-resizer_configure_in_continious_mode(struct vpfe_resizer_device *resizer)
+resizer_configure_in_continuous_mode(struct vpfe_resizer_device *resizer)
 {
struct device *dev = resizer->crop_resizer.subdev.v4l2_dev->dev;
struct resizer_params *param = >config;
@@ -1242,7 +1242,7 @@ static int resizer_do_hw_setup(struct vpfe_resizer_device 
*resizer)
ipipeif_source == IPIPEIF_OUTPUT_RESIZER)
ret = resizer_configure_in_single_shot_mode(resizer);
else
-   ret =  resizer_configure_in_continious_mode(resizer);
+   ret = resizer_configure_in_continuous_mode(resizer);
if (ret)
return ret;
ret = config_rsz_hw(resizer, param);
-- 
2.11.0

Re: [PATCH 0/2] OMAP3ISP CCP2 support

[Sakari Ailus]

On Thu, Jul 13, 2017 at 11:38:06PM +0200, Pavel Machek wrote:
> On Fri 2017-07-14 00:26:52, Sakari Ailus wrote:
> > On Thu, Jul 13, 2017 at 11:13:35PM +0200, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > I took the liberty of changing your patch a bit. I added another to 
> > > > extract
> > > > the number of lanes from the endpoint instead as it's not really a 
> > > > property
> > > > of the PHY. (Not tested yet, will check with N9.)
> > > 
> > > No problem.
> > > 
> > > Notice that the 1/2 does not apply on top of ccp2 branch; my merge
> > > resolution was this:
> > 
> > The two patches are for the ccp2-prepare branches, not for ccp2; it's
> > somewhat out of date right now and needs a rebase.
> 
> Yes, and 1/2 will need merge resolution when you do that. Fortunately
> it is easy.

Well... the other patches go on top of this. The end result is still the
same.

> 
> > The patches work fine on N9.
> 
> I was able to fix the userspace, and they work for me, too. For both:
> 
> Acked-by: Pavel Machek 
> Tested-by: Pavel Machek 

Thanks! I've applied these on ccp2-prepare. ccp2 branch is rebased, too.

-- 
Regards,

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

Re: [PATCH 0/2] OMAP3ISP CCP2 support

[Pavel Machek]

On Fri 2017-07-14 00:26:52, Sakari Ailus wrote:
> On Thu, Jul 13, 2017 at 11:13:35PM +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > I took the liberty of changing your patch a bit. I added another to 
> > > extract
> > > the number of lanes from the endpoint instead as it's not really a 
> > > property
> > > of the PHY. (Not tested yet, will check with N9.)
> > 
> > No problem.
> > 
> > Notice that the 1/2 does not apply on top of ccp2 branch; my merge
> > resolution was this:
> 
> The two patches are for the ccp2-prepare branches, not for ccp2; it's
> somewhat out of date right now and needs a rebase.

Yes, and 1/2 will need merge resolution when you do that. Fortunately
it is easy.

> The patches work fine on N9.

I was able to fix the userspace, and they work for me, too. For both:

Acked-by: Pavel Machek 
Tested-by: Pavel Machek 

Thanks,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH 0/2] OMAP3ISP CCP2 support

[Sakari Ailus]

On Thu, Jul 13, 2017 at 11:13:35PM +0200, Pavel Machek wrote:
> Hi!
> 
> > I took the liberty of changing your patch a bit. I added another to extract
> > the number of lanes from the endpoint instead as it's not really a property
> > of the PHY. (Not tested yet, will check with N9.)
> 
> No problem.
> 
> Notice that the 1/2 does not apply on top of ccp2 branch; my merge
> resolution was this:

The two patches are for the ccp2-prepare branches, not for ccp2; it's
somewhat out of date right now and needs a rebase.

The patches work fine on N9.

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

Re: [PATCH 0/2] OMAP3ISP CCP2 support

[Pavel Machek]

Hi!

> I took the liberty of changing your patch a bit. I added another to extract
> the number of lanes from the endpoint instead as it's not really a property
> of the PHY. (Not tested yet, will check with N9.)

No problem.

Notice that the 1/2 does not apply on top of ccp2 branch; my merge
resolution was this:

I broke something in my userspace; I'll continue testing tommorow.

Thanks,
Pavel

commit 895f4f28972942d1ee77d98dd38dc3d59afaa5c4
Author: Sakari Ailus 
Date:   Thu Jul 13 19:19:02 2017 +0300

omap3isp: Explicitly set the number of CSI-2 lanes used in lane cfg

The omap3isp driver extracts the CSI-2 lane configuration from the V4L2
fwnode endpoint but misses the number of lanes itself. Get this information
and use it in PHY configuration.

Signed-off-by: Sakari Ailus 

diff --git a/drivers/media/platform/omap3isp/isp.c 
b/drivers/media/platform/omap3isp/isp.c
index b80debf..776f708 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2118,7 +2118,10 @@ static int isp_fwnode_parse(struct device *dev, struct 
fwnode_handle *fwnode,
buscfg->bus.csi2.lanecfg.clk.pol,
buscfg->bus.csi2.lanecfg.clk.pos);
 
-   for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
+   buscfg->bus.csi2.num_data_lanes =
+   vep.bus.mipi_csi2.num_data_lanes;
+
+   for (i = 0; i < buscfg->bus.csi2.num_data_lanes; i++) {
buscfg->bus.csi2.lanecfg.data[i].pos =
vep.bus.mipi_csi2.data_lanes[i];
buscfg->bus.csi2.lanecfg.data[i].pol =
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c 
b/drivers/media/platform/omap3isp/ispcsiphy.c
index 50c0f64..958ac7b 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -181,7 +181,7 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
struct isp_bus_cfg *buscfg;
struct isp_csiphy_lanes_cfg *lanes;
int csi2_ddrclk_khz;
-   unsigned int used_lanes = 0;
+   unsigned int num_data_lanes, used_lanes = 0;
unsigned int i;
u32 reg;
 
@@ -197,13 +197,19 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
}
 
if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
-   || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
+   || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
lanes = >bus.ccp2.lanecfg;
-   else
+   num_data_lanes = 1;
+   } else {
lanes = >bus.csi2.lanecfg;
+   num_data_lanes = buscfg->bus.csi2.num_data_lanes;
+   }
+
+   if (num_data_lanes > phy->num_data_lanes)
+   return -EINVAL;
 
/* Clock and data lanes verification */
-   for (i = 0; i < phy->num_data_lanes; i++) {
+   for (i = 0; i < num_data_lanes; i++) {
if (lanes->data[i].pol > 1 || lanes->data[i].pos > 3)
return -EINVAL;
 
@@ -259,7 +265,7 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
/* DPHY lane configuration */
reg = isp_reg_readl(phy->isp, phy->cfg_regs, ISPCSI2_PHY_CFG);
 
-   for (i = 0; i < phy->num_data_lanes; i++) {
+   for (i = 0; i < num_data_lanes; i++) {
reg &= ~(ISPCSI2_PHY_CFG_DATA_POL_MASK(i + 1) |
 ISPCSI2_PHY_CFG_DATA_POSITION_MASK(i + 1));
reg |= (lanes->data[i].pol <<
diff --git a/drivers/media/platform/omap3isp/omap3isp.h 
b/drivers/media/platform/omap3isp/omap3isp.h
index f6d1d0d..672a9cf 100644
--- a/drivers/media/platform/omap3isp/omap3isp.h
+++ b/drivers/media/platform/omap3isp/omap3isp.h
@@ -115,10 +115,13 @@ struct isp_ccp2_cfg {
 /**
  * struct isp_csi2_cfg - CSI2 interface configuration
  * @crc: Enable the cyclic redundancy check
+ * @lanecfg: CSI-2 lane configuration
+ * @num_data_lanes: The number of data lanes in use
  */
 struct isp_csi2_cfg {
unsigned crc:1;
struct isp_csiphy_lanes_cfg lanecfg;
+   u8 num_data_lanes;
 };
 
 struct isp_bus_cfg {


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH v2 09/14] v4l: vsp1: Add support for multiple LIF instances

[Kieran Bingham]

Hi Laurent,

On 26/06/17 19:12, Laurent Pinchart wrote:
> The VSP2-DL instance (present in the H3 ES2.0 and M3-N SoCs) has two LIF
> instances. Adapt the driver infrastructure to support multiple LIFs.
> Support for multiple display pipelines will be added separately.
> 
> The change to the entity routing table removes the ability to connect
> the LIF output to the HGO or HGT histogram generators. This feature is
> only available on Gen2 hardware, isn't supported by the rest of the
> driver, and has no known use case, so this isn't an issue.
> 
> Signed-off-by: Laurent Pinchart 
This looks good.

Reviewed-by: Kieran Bingham 

> ---
>  drivers/media/platform/vsp1/vsp1.h|  5 ++--
>  drivers/media/platform/vsp1/vsp1_drm.c|  8 ++---
>  drivers/media/platform/vsp1/vsp1_drv.c| 49 
> +++
>  drivers/media/platform/vsp1/vsp1_entity.c |  3 +-
>  drivers/media/platform/vsp1/vsp1_lif.c|  5 ++--
>  drivers/media/platform/vsp1/vsp1_lif.h|  2 +-
>  drivers/media/platform/vsp1/vsp1_regs.h   |  4 ++-
>  7 files changed, 46 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1.h 
> b/drivers/media/platform/vsp1/vsp1.h
> index 73858a0ed35c..78ef838416b3 100644
> --- a/drivers/media/platform/vsp1/vsp1.h
> +++ b/drivers/media/platform/vsp1/vsp1.h
> @@ -41,11 +41,11 @@ struct vsp1_rwpf;
>  struct vsp1_sru;
>  struct vsp1_uds;
>  
> +#define VSP1_MAX_LIF 2
>  #define VSP1_MAX_RPF 5
>  #define VSP1_MAX_UDS 3
>  #define VSP1_MAX_WPF 4
>  
> -#define VSP1_HAS_LIF (1 << 0)

I guess I can re-use this 'bit' for the Writeback prototype which used to be
BIT(9) (which is now the BRS)


>  #define VSP1_HAS_LUT (1 << 1)
>  #define VSP1_HAS_SRU (1 << 2)
>  #define VSP1_HAS_BRU (1 << 3)
> @@ -61,6 +61,7 @@ struct vsp1_device_info {
>   const char *model;
>   unsigned int gen;
>   unsigned int features;
> + unsigned int lif_count;
>   unsigned int rpf_count;
>   unsigned int uds_count;
>   unsigned int wpf_count;
> @@ -84,7 +85,7 @@ struct vsp1_device {
>   struct vsp1_hgt *hgt;
>   struct vsp1_hsit *hsi;
>   struct vsp1_hsit *hst;
> - struct vsp1_lif *lif;
> + struct vsp1_lif *lif[VSP1_MAX_LIF];
>   struct vsp1_lut *lut;
>   struct vsp1_rwpf *rpf[VSP1_MAX_RPF];
>   struct vsp1_sru *sru;
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index daaafe7885fa..4e1b893e8f51 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -181,7 +181,7 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int 
> pipe_index,
>   format.format.code);
>  
>   format.pad = LIF_PAD_SINK;
> - ret = v4l2_subdev_call(>lif->entity.subdev, pad, set_fmt, NULL,
> + ret = v4l2_subdev_call(>lif[0]->entity.subdev, pad, set_fmt, NULL,
>  );
>   if (ret < 0)
>   return ret;
> @@ -599,15 +599,15 @@ int vsp1_drm_init(struct vsp1_device *vsp1)
>  
>   vsp1->bru->entity.sink = >wpf[0]->entity;
>   vsp1->bru->entity.sink_pad = 0;
> - vsp1->wpf[0]->entity.sink = >lif->entity;
> + vsp1->wpf[0]->entity.sink = >lif[0]->entity;
>   vsp1->wpf[0]->entity.sink_pad = 0;
>  
>   list_add_tail(>bru->entity.list_pipe, >entities);
>   list_add_tail(>wpf[0]->entity.list_pipe, >entities);
> - list_add_tail(>lif->entity.list_pipe, >entities);
> + list_add_tail(>lif[0]->entity.list_pipe, >entities);
>  
>   pipe->bru = >bru->entity;
> - pipe->lif = >lif->entity;
> + pipe->lif = >lif[0]->entity;
>   pipe->output = vsp1->wpf[0];
>   pipe->output->pipe = pipe;
>   pipe->frame_end = vsp1_du_pipeline_frame_end;
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c 
> b/drivers/media/platform/vsp1/vsp1_drv.c
> index c4f2ac61f7d2..e875982f02ae 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -168,10 +168,13 @@ static int vsp1_uapi_create_links(struct vsp1_device 
> *vsp1)
>   return ret;
>   }
>  
> - if (vsp1->lif) {
> - ret = media_create_pad_link(>wpf[0]->entity.subdev.entity,
> + for (i = 0; i < vsp1->info->lif_count; ++i) {
> + if (!vsp1->lif[i])
> + continue;
> +
> + ret = media_create_pad_link(>wpf[i]->entity.subdev.entity,
>   RWPF_PAD_SOURCE,
> - >lif->entity.subdev.entity,
> + >lif[i]->entity.subdev.entity,
>   LIF_PAD_SINK, 0);
>   if (ret < 0)
>   return ret;
> @@ -334,18 +337,23 @@ static int vsp1_create_entities(struct vsp1_device 
> *vsp1)
>   }

Re: [PATCH v2 08/14] v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and VSP2-D instances

[Kieran Bingham]

Hi Laurent,

On 26/06/17 19:12, Laurent Pinchart wrote:
> New Gen3 SoCs come with two new VSP2 variants names VSP2-BS and VSP2-DL,
> as well as a new VSP2-D variant on V3M and V3H SoCs. Add new entries for
> them in the VSP device info table.
> 
> Signed-off-by: Laurent Pinchart 

Code in the patch looks OK - but I can't see where the difference between the
horizontal widths are supported between VSPD H3/VC

I see this in the datasheet: (32.1.1.6 in this particular part)

Direct connection to display module
— Supporting 4096 pixels in horizontal direction [R-Car H3/R-Car M3-W/ R-Car 
M3-N]
— Supporting 2048 pixels in horizontal direction [R-Car V3M/R-Car V3H/R-Car
D3/R-Car E3]

Do we have this information encoded anywhere? or are they just talking about
maximum performance capability there?

Also some features that are implied as supported aren't mentioned - but that's
not a blocker to adding in the initial devices at all.

Therefore:

Reviewed-by: Kieran Bingham 

> ---
>  drivers/media/platform/vsp1/vsp1_drv.c  | 24 
>  drivers/media/platform/vsp1/vsp1_regs.h | 15 +--
>  2 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c 
> b/drivers/media/platform/vsp1/vsp1_drv.c
> index 6a9aeb71aedf..c4f2ac61f7d2 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -710,6 +710,14 @@ static const struct vsp1_device_info vsp1_device_infos[] 
> = {
>   .num_bru_inputs = 5,
>   .uapi = true,
>   }, {
> + .version = VI6_IP_VERSION_MODEL_VSPBS_GEN3,
> + .model = "VSP2-BS",
> + .gen = 3,
> + .features = VSP1_HAS_BRS,

32.1.1.5 implies:

| VSP1_HAS_WPF_VFLIP

But Figure 32.5 implies that it doesn't ...

Figure 32.5 also implies that | VSP1_HAS_CLU is there too on both RPF0, and RPF1

> + .rpf_count = 2,
> + .wpf_count = 1,
> + .uapi = true,
> + }, {
>   .version = VI6_IP_VERSION_MODEL_VSPD_GEN3,
>   .model = "VSP2-D",
>   .gen = 3,
> @@ -717,6 +725,22 @@ static const struct vsp1_device_info vsp1_device_infos[] 
> = {
>   .rpf_count = 5,
>   .wpf_count = 2,
>   .num_bru_inputs = 5,
> + }, {
> + .version = VI6_IP_VERSION_MODEL_VSPD_V3,
> + .model = "VSP2-D",
> + .gen = 3,
> + .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF,
> + .rpf_count = 5,
> + .wpf_count = 1,
> + .num_bru_inputs = 5,
> + }, {
> + .version = VI6_IP_VERSION_MODEL_VSPDL_GEN3,
> + .model = "VSP2-DL",
> + .gen = 3,
> + .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF,

Hrm. 32.1.1.7 says:
— Vertical flipping in case of output to memory.
So thats some sort of a conditional : | VSP1_HAS_WPF_VFLIP

So looking at this and the settings of the existing models, I guess it looks
like we don't support flip if we have an LIF output (as that would then be
unsupported)

> + .rpf_count = 5,
> + .wpf_count = 2,
> + .num_bru_inputs = 5,
>   },
>  };
>  
> diff --git a/drivers/media/platform/vsp1/vsp1_regs.h 
> b/drivers/media/platform/vsp1/vsp1_regs.h
> index 744217e020b9..ab439a60a100 100644
> --- a/drivers/media/platform/vsp1/vsp1_regs.h
> +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> @@ -699,9 +699,20 @@
>  #define VI6_IP_VERSION_MODEL_VSPBD_GEN3  (0x15 << 8)
>  #define VI6_IP_VERSION_MODEL_VSPBC_GEN3  (0x16 << 8)
>  #define VI6_IP_VERSION_MODEL_VSPD_GEN3   (0x17 << 8)
> +#define VI6_IP_VERSION_MODEL_VSPD_V3 (0x18 << 8)
> +#define VI6_IP_VERSION_MODEL_VSPDL_GEN3  (0x19 << 8)
> +#define VI6_IP_VERSION_MODEL_VSPBS_GEN3  (0x1a << 8)
>  #define VI6_IP_VERSION_SOC_MASK  (0xff << 0)
> -#define VI6_IP_VERSION_SOC_H (0x01 << 0)
> -#define VI6_IP_VERSION_SOC_M (0x02 << 0)
> +#define VI6_IP_VERSION_SOC_H2(0x01 << 0)
> +#define VI6_IP_VERSION_SOC_V2H   (0x01 << 0)
> +#define VI6_IP_VERSION_SOC_V3M   (0x01 << 0)
> +#define VI6_IP_VERSION_SOC_M2(0x02 << 0)
> +#define VI6_IP_VERSION_SOC_M3W   (0x02 << 0)
> +#define VI6_IP_VERSION_SOC_V3H   (0x02 << 0)
> +#define VI6_IP_VERSION_SOC_H3(0x03 << 0)
> +#define VI6_IP_VERSION_SOC_D3(0x04 << 0)
> +#define VI6_IP_VERSION_SOC_M3N   (0x04 << 0)
> +#define VI6_IP_VERSION_SOC_E3(0x04 << 0)
>  
>  /* 
> -
>   * RPF CLUT Registers
> 

Re: [PATCH v2 02/14] v4l: vsp1: Don't recycle active list at display start

[Kieran Bingham]

Hi Laurent,

On 26/06/17 19:12, Laurent Pinchart wrote:
> When the display start interrupt occurs, we know that the hardware has
> finished loading the active display list. The driver then proceeds to
> recycle the list, assuming it won't be needed anymore.
> 
> This assumption holds true for headerless display lists, as the VSP
> doesn't reload the list for the next frame if it hasn't changed.
> However, this isn't true anymore for header display lists, as they are
> loaded at every frame start regardless of whether they have been
> updated.
> 
> To prepare for header display lists usage in display pipelines, we need
> to postpone recycling the list until it gets replaced by a new one
> through a page flip. The driver already does so in the frame end
> interrupt handler, so all we need is to skip list recycling in the
> display start interrupt handler.
> 
> While the active list can be recycled at display start for headerless
> display lists, there's no real harm in postponing that to the frame end
> interrupt handler in all cases. This simplifies interrupt handling as we
> don't need to process the display start interrupt anymore.
> 
> Signed-off-by: Laurent Pinchart 

Ok, I had skipped this one as I was concerned about its effects in relation to
11/14 but I see how that's working now.

Reviewed-by: Kieran Bingham 

> ---
>  drivers/media/platform/vsp1/vsp1_dl.c  | 16 
>  drivers/media/platform/vsp1/vsp1_dl.h  |  1 -
>  drivers/media/platform/vsp1/vsp1_drm.c | 12 
>  drivers/media/platform/vsp1/vsp1_drm.h |  2 --
>  drivers/media/platform/vsp1/vsp1_drv.c |  8 
>  5 files changed, 4 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
> b/drivers/media/platform/vsp1/vsp1_dl.c
> index dc47e236c780..bb92be4fe0f0 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -547,22 +547,6 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
>   * Display List Manager
>   */
>  
> -/* Interrupt Handling */
> -void vsp1_dlm_irq_display_start(struct vsp1_dl_manager *dlm)
> -{
> - spin_lock(>lock);
> -
> - /*
> -  * The display start interrupt signals the end of the display list
> -  * processing by the device. The active display list, if any, won't be
> -  * accessed anymore and can be reused.
> -  */
> - __vsp1_dl_list_put(dlm->active);
> - dlm->active = NULL;
> -
> - spin_unlock(>lock);
> -}
> -
>  /**
>   * vsp1_dlm_irq_frame_end - Display list handler for the frame end interrupt
>   * @dlm: the display list manager
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h 
> b/drivers/media/platform/vsp1/vsp1_dl.h
> index 6ec1380a10af..ee3508172f0a 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.h
> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> @@ -27,7 +27,6 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device 
> *vsp1,
>   unsigned int prealloc);
>  void vsp1_dlm_destroy(struct vsp1_dl_manager *dlm);
>  void vsp1_dlm_reset(struct vsp1_dl_manager *dlm);
> -void vsp1_dlm_irq_display_start(struct vsp1_dl_manager *dlm);
>  bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm);
>  
>  struct vsp1_dl_list *vsp1_dl_list_get(struct vsp1_dl_manager *dlm);
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index 9377aafa8996..bc3fd9bc7126 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -32,11 +32,6 @@
>   * Interrupt Handling
>   */
>  
> -void vsp1_drm_display_start(struct vsp1_device *vsp1)
> -{
> - vsp1_dlm_irq_display_start(vsp1->drm->pipe.output->dlm);
> -}
> -
>  static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe)
>  {
>   struct vsp1_drm *drm = to_vsp1_drm(pipe);
> @@ -224,6 +219,10 @@ int vsp1_du_setup_lif(struct device *dev, const struct 
> vsp1_du_lif_config *cfg)
>   return ret;
>   }
>  
> + /* Disable the display interrupts. */
> + vsp1_write(vsp1, VI6_DISP_IRQ_STA, 0);
> + vsp1_write(vsp1, VI6_DISP_IRQ_ENB, 0);
> +
>   dev_dbg(vsp1->dev, "%s: pipeline enabled\n", __func__);
>  
>   return 0;
> @@ -529,13 +528,10 @@ void vsp1_du_atomic_flush(struct device *dev)
>  
>   /* Start or stop the pipeline if needed. */
>   if (!vsp1->drm->num_inputs && pipe->num_inputs) {
> - vsp1_write(vsp1, VI6_DISP_IRQ_STA, 0);
> - vsp1_write(vsp1, VI6_DISP_IRQ_ENB, VI6_DISP_IRQ_ENB_DSTE);
>   spin_lock_irqsave(>irqlock, flags);
>   vsp1_pipeline_run(pipe);
>   spin_unlock_irqrestore(>irqlock, flags);
>   } else if (vsp1->drm->num_inputs && !pipe->num_inputs) {
> - vsp1_write(vsp1, VI6_DISP_IRQ_ENB, 0);
>   vsp1_pipeline_stop(pipe);
>   }
>  }
> diff --git 

[PATCH v3] staging: lirc_zilog: Clean up lirc zilog error codes

[Yves Lemée]

According the coding style guidelines, the ENOSYS error code must be returned
in case of a non existent system call. This code has been replaced with
the ENOTTY error code indicating a missing functionality.

Signed-off-by: Yves Lemée 
---
v3: Fixed patch subject
Fixed patch revision description

v2: Improved punctuation

 drivers/staging/media/lirc/lirc_zilog.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
b/drivers/staging/media/lirc/lirc_zilog.c
index 015e41bd036e..26dd32d5b5b2 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -1249,7 +1249,7 @@ static long ioctl(struct file *filep, unsigned int cmd, 
unsigned long arg)
break;
case LIRC_GET_REC_MODE:
if (!(features & LIRC_CAN_REC_MASK))
-   return -ENOSYS;
+   return -ENOTTY;
 
result = put_user(LIRC_REC2MODE
  (features & LIRC_CAN_REC_MASK),
@@ -1257,21 +1257,21 @@ static long ioctl(struct file *filep, unsigned int cmd, 
unsigned long arg)
break;
case LIRC_SET_REC_MODE:
if (!(features & LIRC_CAN_REC_MASK))
-   return -ENOSYS;
+   return -ENOTTY;
 
result = get_user(mode, uptr);
if (!result && !(LIRC_MODE2REC(mode) & features))
-   result = -EINVAL;
+   result = -ENOTTY;
break;
case LIRC_GET_SEND_MODE:
if (!(features & LIRC_CAN_SEND_MASK))
-   return -ENOSYS;
+   return -ENOTTY;
 
result = put_user(LIRC_MODE_PULSE, uptr);
break;
case LIRC_SET_SEND_MODE:
if (!(features & LIRC_CAN_SEND_MASK))
-   return -ENOSYS;
+   return -ENOTTY;
 
result = get_user(mode, uptr);
if (!result && mode != LIRC_MODE_PULSE)
-- 
2.13.2

[PATCH 1/2] omap3isp: Explicitly set the number of CSI-2 lanes used in lane cfg

[Sakari Ailus]

The omap3isp driver extracts the CSI-2 lane configuration from the V4L2
fwnode endpoint but misses the number of lanes itself. Get this information
and use it in PHY configuration.

Signed-off-by: Sakari Ailus 
---
 drivers/media/platform/omap3isp/isp.c   |  5 -
 drivers/media/platform/omap3isp/ispcsiphy.c | 16 +++-
 drivers/media/platform/omap3isp/omap3isp.h  |  3 +++
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c 
b/drivers/media/platform/omap3isp/isp.c
index 088dc8b1b78a..db2cccb57ceb 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2061,7 +2061,10 @@ static int isp_fwnode_parse(struct device *dev, struct 
fwnode_handle *fwnode,
buscfg->bus.csi2.lanecfg.clk.pol,
buscfg->bus.csi2.lanecfg.clk.pos);
 
-   for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
+   buscfg->bus.csi2.num_data_lanes =
+   vep.bus.mipi_csi2.num_data_lanes;
+
+   for (i = 0; i < buscfg->bus.csi2.num_data_lanes; i++) {
buscfg->bus.csi2.lanecfg.data[i].pos =
vep.bus.mipi_csi2.data_lanes[i];
buscfg->bus.csi2.lanecfg.data[i].pol =
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c 
b/drivers/media/platform/omap3isp/ispcsiphy.c
index 83940e9d8291..3efa71396aae 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -169,7 +169,7 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
struct isp_bus_cfg *buscfg = pipe->external->host_priv;
struct isp_csiphy_lanes_cfg *lanes;
int csi2_ddrclk_khz;
-   unsigned int used_lanes = 0;
+   unsigned int num_data_lanes, used_lanes = 0;
unsigned int i;
u32 reg;
 
@@ -181,13 +181,19 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
}
 
if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
-   || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
+   || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
lanes = >bus.ccp2.lanecfg;
-   else
+   num_data_lanes = 1;
+   } else {
lanes = >bus.csi2.lanecfg;
+   num_data_lanes = buscfg->bus.csi2.num_data_lanes;
+   }
+
+   if (num_data_lanes > phy->num_data_lanes)
+   return -EINVAL;
 
/* Clock and data lanes verification */
-   for (i = 0; i < phy->num_data_lanes; i++) {
+   for (i = 0; i < num_data_lanes; i++) {
if (lanes->data[i].pol > 1 || lanes->data[i].pos > 3)
return -EINVAL;
 
@@ -243,7 +249,7 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
/* DPHY lane configuration */
reg = isp_reg_readl(csi2->isp, phy->cfg_regs, ISPCSI2_PHY_CFG);
 
-   for (i = 0; i < phy->num_data_lanes; i++) {
+   for (i = 0; i < num_data_lanes; i++) {
reg &= ~(ISPCSI2_PHY_CFG_DATA_POL_MASK(i + 1) |
 ISPCSI2_PHY_CFG_DATA_POSITION_MASK(i + 1));
reg |= (lanes->data[i].pol <<
diff --git a/drivers/media/platform/omap3isp/omap3isp.h 
b/drivers/media/platform/omap3isp/omap3isp.h
index 443e8f7673e2..3c26f9a3f508 100644
--- a/drivers/media/platform/omap3isp/omap3isp.h
+++ b/drivers/media/platform/omap3isp/omap3isp.h
@@ -114,10 +114,13 @@ struct isp_ccp2_cfg {
 /**
  * struct isp_csi2_cfg - CSI2 interface configuration
  * @crc: Enable the cyclic redundancy check
+ * @lanecfg: CSI-2 lane configuration
+ * @num_data_lanes: The number of data lanes in use
  */
 struct isp_csi2_cfg {
unsigned crc:1;
struct isp_csiphy_lanes_cfg lanecfg;
+   u8 num_data_lanes;
 };
 
 struct isp_bus_cfg {
-- 
2.11.0

[PATCH 0/2] OMAP3ISP CCP2 support

[Sakari Ailus]

Hi Pavel,

I took the liberty of changing your patch a bit. I added another to extract
the number of lanes from the endpoint instead as it's not really a property
of the PHY. (Not tested yet, will check with N9.)

Pavel Machek (1):
  omap3isp: add CSI1 support

Sakari Ailus (1):
  omap3isp: Explicitly set the number of CSI-2 lanes used in lane cfg

 drivers/media/platform/omap3isp/isp.c   |  5 -
 drivers/media/platform/omap3isp/ispccp2.c   |  1 +
 drivers/media/platform/omap3isp/ispcsiphy.c | 35 +++--
 drivers/media/platform/omap3isp/omap3isp.h  |  3 +++
 4 files changed, 31 insertions(+), 13 deletions(-)

-- 
2.11.0

[PATCH 2/2] omap3isp: add CSI1 support

[Sakari Ailus]

From: Pavel Machek 

CSI-2 PHY power management is only needed for major version 15 of the ISP.
Additionally, set the CCP2 PHY for previous ISP versions as well.

These changes are necessary for CCP2 support.

Signed-off-by: Pavel Machek 
Signed-off-by: Sakari Ailus 
---
 drivers/media/platform/omap3isp/ispccp2.c   |  1 +
 drivers/media/platform/omap3isp/ispcsiphy.c | 19 ---
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispccp2.c 
b/drivers/media/platform/omap3isp/ispccp2.c
index ca095238510d..588f67a89f79 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -1141,6 +1141,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
"Could not get regulator vdds_csib\n");
ccp2->vdds_csib = NULL;
}
+   ccp2->phy = >isp_csiphy2;
} else if (isp->revision == ISP_REVISION_15_0) {
ccp2->phy = >isp_csiphy1;
}
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c 
b/drivers/media/platform/omap3isp/ispcsiphy.c
index 3efa71396aae..addc6efbb033 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -292,13 +292,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
if (rval < 0)
goto done;
 
-   rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
-   if (rval) {
-   regulator_disable(phy->vdd);
-   goto done;
+   if (phy->isp->revision == ISP_REVISION_15_0) {
+   rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
+   if (rval) {
+   regulator_disable(phy->vdd);
+   goto done;
+   }
+
+   csiphy_power_autoswitch_enable(phy, true);
}
 
-   csiphy_power_autoswitch_enable(phy, true);
phy->phy_in_use = 1;
 
 done:
@@ -317,8 +320,10 @@ void omap3isp_csiphy_release(struct isp_csiphy *phy)
 
csiphy_routing_cfg(phy, buscfg->interface, false,
   buscfg->bus.ccp2.phy_layer);
-   csiphy_power_autoswitch_enable(phy, false);
-   csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_OFF);
+   if (phy->isp->revision == ISP_REVISION_15_0) {
+   csiphy_power_autoswitch_enable(phy, false);
+   csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_OFF);
+   }
regulator_disable(phy->vdd);
phy->phy_in_use = 0;
}
-- 
2.11.0

[PATCH] [media] staging/atomisp: fix minor coding style issue

[Shy More]

Below was the minor issue flagged by checkpatch.pl:
- ERROR: space prohibited after that open parenthesis '('

Signed-off-by: Shy More 
---
 .../atomisp/pci/atomisp2/css2400/runtime/isys/src/ibuf_ctrl_rmgr.c  | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/ibuf_ctrl_rmgr.c
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/ibuf_ctrl_rmgr.c
index 76d9142..4e61cb7 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/ibuf_ctrl_rmgr.c
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/ibuf_ctrl_rmgr.c
@@ -131,7 +131,7 @@ void ia_css_isys_ibuf_rmgr_release(
for (i = 0; i < ibuf_rsrc.num_allocated; i++) {
handle = getHandle(i);
if ((handle->start_addr == *start_addr)
-   && ( true == handle->active)) {
+   && (true == handle->active)) {
handle->active = false;
ibuf_rsrc.num_active--;
break;
-- 
1.9.1

Re: [PATCH v2 2/2] staging: atomisp2: hmm: Alignment code (rebased)

[Sakari Ailus]

On Thu, Jul 13, 2017 at 08:55:43AM +0200, Philipp Guendisch wrote:
> This patch fixed code alignment to open paranthesis.
> Semantic should not be affected by this patch.
> 
> It has been rebased on top of media_tree atomisp branch
> 
> Signed-off-by: Philipp Guendisch 
> Signed-off-by: Chris Baller 

Hi Philipp,

Neither of the patches still applies?

Are you sure you rebased them on the atomisp branch?

-- 
Regards,

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

Re: [PATCH] media: vimc: cleanup a few warnings

[Javier Martinez Canillas]

On Thu, Jul 13, 2017 at 5:38 PM, Sakari Ailus  wrote:

[snip]

>>
>> -static const struct platform_device_id vimc_sen_driver_ids[] = {
>> +static const __maybe_unused
>> +struct platform_device_id vimc_sen_driver_ids[] = {
>>   {
>>   .name   = VIMC_SEN_DRV_NAME,
>>   },
>
> Shouldn't these be set to the corresponding driver structs' id_table
> fields? Or do I miss something...?
>

Agreed, the real problem is that the .id_table is not set for these
drivers. The match only works because the platform subsystem fallbacks
to the driver's name if an .id_table isn't defined:

http://elixir.free-electrons.com/linux/latest/source/drivers/base/platform.c#L964

Best regards,
Javier

Re: [PATCH] media: vimc: cleanup a few warnings

[Sakari Ailus]

Hi Mauro,

On Wed, Jul 12, 2017 at 08:46:30AM -0300, Mauro Carvalho Chehab wrote:
> The const structs uded by MODULE_DEVICE_TABLE()
> may never be used with COMPILE_TEST:
> 
> drivers/media/platform/vimc/vimc-capture.c:528:40: warning: 
> 'vimc_cap_driver_ids' defined but not used [-Wunused-const-variable=]
>  static const struct platform_device_id vimc_cap_driver_ids[] = {
> ^~~
> drivers/media/platform/vimc/vimc-debayer.c:588:40: warning: 
> 'vimc_deb_driver_ids' defined but not used [-Wunused-const-variable=]
>  static const struct platform_device_id vimc_deb_driver_ids[] = {
> ^~~
> drivers/media/platform/vimc/vimc-scaler.c:442:40: warning: 
> 'vimc_sca_driver_ids' defined but not used [-Wunused-const-variable=]
>  static const struct platform_device_id vimc_sca_driver_ids[] = {
> ^~~
> drivers/media/platform/vimc/vimc-sensor.c:376:40: warning: 
> 'vimc_sen_driver_ids' defined but not used [-Wunused-const-variable=]
>  static const struct platform_device_id vimc_sen_driver_ids[] = {
> ^~~
> 
> So, add the proper notation to avoid warnings.
> 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/media/platform/vimc/vimc-capture.c | 3 ++-
>  drivers/media/platform/vimc/vimc-debayer.c | 3 ++-
>  drivers/media/platform/vimc/vimc-scaler.c  | 3 ++-
>  drivers/media/platform/vimc/vimc-sensor.c  | 3 ++-
>  4 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c 
> b/drivers/media/platform/vimc/vimc-capture.c
> index 14cb32e21130..c6f4a407e019 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -525,7 +525,8 @@ static struct platform_driver vimc_cap_pdrv = {
>   },
>  };
>  
> -static const struct platform_device_id vimc_cap_driver_ids[] = {
> +static const __maybe_unused
> +struct platform_device_id vimc_cap_driver_ids[] = {
>   {
>   .name   = VIMC_CAP_DRV_NAME,
>   },
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c 
> b/drivers/media/platform/vimc/vimc-debayer.c
> index 35b15bd4d61d..428454e33b75 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -585,7 +585,8 @@ static struct platform_driver vimc_deb_pdrv = {
>   },
>  };
>  
> -static const struct platform_device_id vimc_deb_driver_ids[] = {
> +static const __maybe_unused
> +struct platform_device_id vimc_deb_driver_ids[] = {
>   {
>   .name   = VIMC_DEB_DRV_NAME,
>   },
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c 
> b/drivers/media/platform/vimc/vimc-scaler.c
> index fe77505d2679..35bf3b32108f 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -439,7 +439,8 @@ static struct platform_driver vimc_sca_pdrv = {
>   },
>  };
>  
> -static const struct platform_device_id vimc_sca_driver_ids[] = {
> +static const __maybe_unused
> +struct platform_device_id vimc_sca_driver_ids[] = {
>   {
>   .name   = VIMC_SCA_DRV_NAME,
>   },
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c 
> b/drivers/media/platform/vimc/vimc-sensor.c
> index ebdbbe8c05ed..9ad2be111a14 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -373,7 +373,8 @@ static struct platform_driver vimc_sen_pdrv = {
>   },
>  };
>  
> -static const struct platform_device_id vimc_sen_driver_ids[] = {
> +static const __maybe_unused
> +struct platform_device_id vimc_sen_driver_ids[] = {
>   {
>   .name   = VIMC_SEN_DRV_NAME,
>   },

Shouldn't these be set to the corresponding driver structs' id_table
fields? Or do I miss something...?

-- 
Regards,

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

[PATCH v2 3/6] omap3isp: Remove misleading comment

[Sakari Ailus]

The intent of the check the comment is related to is to ensure we are
streaming --- still. Not that streaming wouldn't be enabled yet. Remove
it.

Signed-off-by: Sakari Ailus 
Acked-by: Hans Verkuil 
Reviewed-by: Laurent Pinchart 
---
 drivers/media/platform/omap3isp/ispvideo.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/platform/omap3isp/ispvideo.c 
b/drivers/media/platform/omap3isp/ispvideo.c
index 9e9b18c217bd..a3ca2a480d1a 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1205,7 +1205,6 @@ isp_video_streamoff(struct file *file, void *fh, enum 
v4l2_buf_type type)
 
mutex_lock(>stream_lock);
 
-   /* Make sure we're not streaming yet. */
mutex_lock(>queue_lock);
streaming = vb2_is_streaming(>queue);
mutex_unlock(>queue_lock);
-- 
2.11.0

[PATCH v2 6/6] media: devnode: Rename mdev argument as devnode

[Sakari Ailus]

Historically, mdev argument name was being used on both struct
media_device and struct media_devnode. Recently most occurrences of mdev
referring to struct media_devnode were replaced by devnode, which makes
more sense. Fix the last remaining occurrence.

Fixes: 163f1e93e9950 ("[media] media-devnode: fix namespace mess")
Signed-off-by: Sakari Ailus 
Acked-by: Hans Verkuil 
Reviewed-by: Laurent Pinchart 
---
 drivers/media/media-device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index c51e2e5636f3..fce91b543c14 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -537,9 +537,9 @@ static DEVICE_ATTR(model, S_IRUGO, show_model, NULL);
  * Registration/unregistration
  */
 
-static void media_device_release(struct media_devnode *mdev)
+static void media_device_release(struct media_devnode *devnode)
 {
-   dev_dbg(mdev->parent, "Media device released\n");
+   dev_dbg(devnode->parent, "Media device released\n");
 }
 
 /**
-- 
2.11.0

[PATCH v2 5/6] media: Remove useless curly braces and parentheses

[Sakari Ailus]

Signed-off-by: Sakari Ailus 
Acked-by: Hans Verkuil 
Reviewed-by: Laurent Pinchart 
---
 drivers/media/media-device.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 760e3e424e23..c51e2e5636f3 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -591,9 +591,8 @@ int __must_check media_device_register_entity(struct 
media_device *mdev,
   >pads[i].graph_obj);
 
/* invoke entity_notify callbacks */
-   list_for_each_entry_safe(notify, next, >entity_notify, list) {
-   (notify)->notify(entity, notify->notify_data);
-   }
+   list_for_each_entry_safe(notify, next, >entity_notify, list)
+   notify->notify(entity, notify->notify_data);
 
if (mdev->entity_internal_idx_max
>= mdev->pm_count_walk.ent_enum.idx_max) {
-- 
2.11.0

[PATCH v2 1/6] omap3isp: Don't rely on devm for memory resource management

[Sakari Ailus]

devm functions are fine for managing resources that are directly related
to the device at hand and that have no other dependencies. However, a
process holding a file handle to a device created by a driver for a device
may result in the file handle left behind after the device is long gone.
This will result in accessing released (and potentially reallocated)
memory.

Instead, manage the memory resources in the driver. Releasing the
resources can be later on bound to e.g. by releasing a reference.

Signed-off-by: Sakari Ailus 
Acked-by: Hans Verkuil 
Reviewed-by: Laurent Pinchart 
---
 drivers/media/platform/omap3isp/isp.c | 18 --
 drivers/media/platform/omap3isp/isph3a_aewb.c | 24 +---
 drivers/media/platform/omap3isp/isph3a_af.c   | 24 +---
 drivers/media/platform/omap3isp/isphist.c | 11 +++
 drivers/media/platform/omap3isp/ispstat.c |  2 ++
 5 files changed, 55 insertions(+), 24 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c 
b/drivers/media/platform/omap3isp/isp.c
index 9df64c189883..a03f8b9f8763 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1999,6 +1999,8 @@ static int isp_remove(struct platform_device *pdev)
 
media_entity_enum_cleanup(>crashed);
 
+   kfree(isp);
+
return 0;
 }
 
@@ -2189,7 +2191,7 @@ static int isp_probe(struct platform_device *pdev)
int ret;
int i, m;
 
-   isp = devm_kzalloc(>dev, sizeof(*isp), GFP_KERNEL);
+   isp = kzalloc(sizeof(*isp), GFP_KERNEL);
if (!isp) {
dev_err(>dev, "could not allocate memory\n");
return -ENOMEM;
@@ -2198,21 +2200,23 @@ static int isp_probe(struct platform_device *pdev)
ret = fwnode_property_read_u32(of_fwnode_handle(pdev->dev.of_node),
   "ti,phy-type", >phy_type);
if (ret)
-   return ret;
+   goto error_release_isp;
 
isp->syscon = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
  "syscon");
-   if (IS_ERR(isp->syscon))
-   return PTR_ERR(isp->syscon);
+   if (IS_ERR(isp->syscon)) {
+   ret = PTR_ERR(isp->syscon);
+   goto error_release_isp;
+   }
 
ret = of_property_read_u32_index(pdev->dev.of_node,
 "syscon", 1, >syscon_offset);
if (ret)
-   return ret;
+   goto error_release_isp;
 
ret = isp_fwnodes_parse(>dev, >notifier);
if (ret < 0)
-   return ret;
+   goto error_release_isp;
 
isp->autoidle = autoidle;
 
@@ -2362,6 +2366,8 @@ static int isp_probe(struct platform_device *pdev)
__omap3isp_put(isp, false);
 error:
mutex_destroy(>isp_mutex);
+error_release_isp:
+   kfree(isp);
 
return ret;
 }
diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c 
b/drivers/media/platform/omap3isp/isph3a_aewb.c
index d44626f20ac6..efddafdb999d 100644
--- a/drivers/media/platform/omap3isp/isph3a_aewb.c
+++ b/drivers/media/platform/omap3isp/isph3a_aewb.c
@@ -289,9 +289,10 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
 {
struct ispstat *aewb = >isp_aewb;
struct omap3isp_h3a_aewb_config *aewb_cfg;
-   struct omap3isp_h3a_aewb_config *aewb_recover_cfg;
+   struct omap3isp_h3a_aewb_config *aewb_recover_cfg = NULL;
+   int ret;
 
-   aewb_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_cfg), GFP_KERNEL);
+   aewb_cfg = kzalloc(sizeof(*aewb_cfg), GFP_KERNEL);
if (!aewb_cfg)
return -ENOMEM;
 
@@ -301,12 +302,12 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
aewb->isp = isp;
 
/* Set recover state configuration */
-   aewb_recover_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_recover_cfg),
-   GFP_KERNEL);
+   aewb_recover_cfg = kzalloc(sizeof(*aewb_recover_cfg), GFP_KERNEL);
if (!aewb_recover_cfg) {
dev_err(aewb->isp->dev,
"AEWB: cannot allocate memory for recover 
configuration.\n");
-   return -ENOMEM;
+   ret = -ENOMEM;
+   goto err;
}
 
aewb_recover_cfg->saturation_limit = OMAP3ISP_AEWB_MAX_SATURATION_LIM;
@@ -323,13 +324,22 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
if (h3a_aewb_validate_params(aewb, aewb_recover_cfg)) {
dev_err(aewb->isp->dev,
"AEWB: recover configuration is invalid.\n");
-   return -EINVAL;
+   ret = -EINVAL;
+   goto err;
}
 
aewb_recover_cfg->buf_size = h3a_aewb_get_buf_size(aewb_recover_cfg);
aewb->recover_priv = aewb_recover_cfg;
 
-   return 

[PATCH v2 0/6] Media controller and omap3isp cleanups

[Sakari Ailus]

Hi folks,

These patches prepare for media device refcounting but do not bring those
changes yet. They're worthwhile on their own merits.

since v1:

- Use IS_ERR_OR_NULL() check in error handling for omap3isp_hist_init()
  instead of IS_ERR() --- the argument may be NULL as well.

Sakari Ailus (6):
  omap3isp: Don't rely on devm for memory resource management
  omap3isp: Call video_unregister_device() unconditionally
  omap3isp: Remove misleading comment
  omap3isp: Disable streaming at driver unbind time
  media: Remove useless curly braces and parentheses
  media: devnode: Rename mdev argument as devnode

 drivers/media/media-device.c  |  9 -
 drivers/media/platform/omap3isp/isp.c | 18 --
 drivers/media/platform/omap3isp/isph3a_aewb.c | 24 +---
 drivers/media/platform/omap3isp/isph3a_af.c   | 24 +---
 drivers/media/platform/omap3isp/isphist.c | 11 +++
 drivers/media/platform/omap3isp/ispstat.c |  2 ++
 drivers/media/platform/omap3isp/ispvideo.c| 27 ---
 7 files changed, 75 insertions(+), 40 deletions(-)

-- 
2.11.0

[PATCH v2 2/6] omap3isp: Call video_unregister_device() unconditionally

[Sakari Ailus]

video_unregister_device() can be called on a never or an already
unregistered device. Drop the redundant check.

Signed-off-by: Sakari Ailus 
Acked-by: Hans Verkuil 
Reviewed-by: Laurent Pinchart 
---
 drivers/media/platform/omap3isp/ispvideo.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispvideo.c 
b/drivers/media/platform/omap3isp/ispvideo.c
index 218e6d7ae93a..9e9b18c217bd 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1495,6 +1495,5 @@ int omap3isp_video_register(struct isp_video *video, 
struct v4l2_device *vdev)
 
 void omap3isp_video_unregister(struct isp_video *video)
 {
-   if (video_is_registered(>video))
-   video_unregister_device(>video);
+   video_unregister_device(>video);
 }
-- 
2.11.0

[PATCH v2 4/6] omap3isp: Disable streaming at driver unbind time

[Sakari Ailus]

Once the driver is unbound accessing the hardware is not allowed anymore.
Due to this, disable streaming when the device driver is unbound. The
states of the associated objects related to Media controller and videobuf2
frameworks are updated as well, just like if the application disabled
streaming explicitly.

Signed-off-by: Sakari Ailus 
Acked-by: Hans Verkuil 
Reviewed-by: Laurent Pinchart 
---
 drivers/media/platform/omap3isp/ispvideo.c | 23 +++
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispvideo.c 
b/drivers/media/platform/omap3isp/ispvideo.c
index a3ca2a480d1a..c04d357cd4d0 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1191,22 +1191,17 @@ isp_video_streamon(struct file *file, void *fh, enum 
v4l2_buf_type type)
 }
 
 static int
-isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
+__isp_video_streamoff(struct isp_video *video)
 {
-   struct isp_video_fh *vfh = to_isp_video_fh(fh);
-   struct isp_video *video = video_drvdata(file);
struct isp_pipeline *pipe = to_isp_pipeline(>video.entity);
enum isp_pipeline_state state;
unsigned int streaming;
unsigned long flags;
 
-   if (type != video->type)
-   return -EINVAL;
-
mutex_lock(>stream_lock);
 
mutex_lock(>queue_lock);
-   streaming = vb2_is_streaming(>queue);
+   streaming = video->queue && vb2_is_streaming(video->queue);
mutex_unlock(>queue_lock);
 
if (!streaming)
@@ -1229,7 +1224,7 @@ isp_video_streamoff(struct file *file, void *fh, enum 
v4l2_buf_type type)
omap3isp_video_cancel_stream(video);
 
mutex_lock(>queue_lock);
-   vb2_streamoff(>queue, type);
+   vb2_streamoff(video->queue, video->queue->type);
mutex_unlock(>queue_lock);
video->queue = NULL;
video->error = false;
@@ -1245,6 +1240,17 @@ isp_video_streamoff(struct file *file, void *fh, enum 
v4l2_buf_type type)
 }
 
 static int
+isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
+{
+   struct isp_video *video = video_drvdata(file);
+
+   if (type != video->type)
+   return -EINVAL;
+
+   return __isp_video_streamoff(video);
+}
+
+static int
 isp_video_enum_input(struct file *file, void *fh, struct v4l2_input *input)
 {
if (input->index > 0)
@@ -1494,5 +1500,6 @@ int omap3isp_video_register(struct isp_video *video, 
struct v4l2_device *vdev)
 
 void omap3isp_video_unregister(struct isp_video *video)
 {
+   __isp_video_streamoff(video);
video_unregister_device(>video);
 }
-- 
2.11.0

Re: [PATCH] [media] staging/atomisp: fix minor coding style warnings

[Greg KH]

On Thu, Jul 13, 2017 at 08:06:21AM -0700, smklearn wrote:
> Below were the minor issues flagged by checkpatch.pl:
> - WARNING: Block comments use * on subsequent lines
> - ERROR: space prohibited after that open parenthesis '('

Don't do multiple things in the same patch please, this should be
multiple patches.

> Signed-off-by: Shy More 

This name doesn't match your "From:" name in the email :(

thanks,

greg k-h

[PATCH] [media] staging/atomisp: fix minor coding style warnings

[smklearn]

Below were the minor issues flagged by checkpatch.pl:
- WARNING: Block comments use * on subsequent lines
- ERROR: space prohibited after that open parenthesis '('

Signed-off-by: Shy More 
---
 .../css2400/runtime/isys/src/ibuf_ctrl_rmgr.c  | 26 +++---
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/ibuf_ctrl_rmgr.c
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/ibuf_ctrl_rmgr.c
index 76d9142..856fb6e 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/ibuf_ctrl_rmgr.c
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/ibuf_ctrl_rmgr.c
@@ -14,18 +14,18 @@
  */
 #else
 /**
-Support for Intel Camera Imaging ISP subsystem.
-Copyright (c) 2010 - 2015, Intel Corporation.
-
-This program is free software; you can redistribute it and/or modify it
-under the terms and conditions of the GNU General Public License,
-version 2, as published by the Free Software Foundation.
-
-This program is distributed in the hope 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.
-*/
+ * Support for Intel Camera Imaging ISP subsystem.
+ * Copyright (c) 2010 - 2015, Intel Corporation.
+
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+
+ * This program is distributed in the hope 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.
+ */
 #endif
 
 #include "system_global.h"
@@ -131,7 +131,7 @@ void ia_css_isys_ibuf_rmgr_release(
for (i = 0; i < ibuf_rsrc.num_allocated; i++) {
handle = getHandle(i);
if ((handle->start_addr == *start_addr)
-   && ( true == handle->active)) {
+   && (true == handle->active)) {
handle->active = false;
ibuf_rsrc.num_active--;
break;
-- 
1.9.1

Re: [PATCH] staging drivers fixed coding style error

[Greg KH]

On Thu, Jul 13, 2017 at 07:17:56AM -0700, smklearn wrote:
> Fixed coding style error flagged checkpatch.pl:
>   - ERROR: space prohibited after that open parenthesis '('
>   - WARNING: Block comments use * on subsequent lines
> 
> Signed-off-by: Shy More 
> 
> Output after fixing coding style issues:
> 
> $KERN/scripts/checkpatch.pl -f
>   ./media/atomisp/pci/atomisp2/css2400/runtime/isys/src/ibuf_ctrl_rmgr.c
> 
> total: 0 errors, 0 warnings, 141 lines checked

Please don't put anything below the Signed-off-by: line, you will note
that all other commits are written that way.

Also, your subject: needs a lot of work, again, look at other commits
for the driver you are modifying to get it right.

good luck!

greg k-h

[PATCH] staging drivers fixed coding style error

[smklearn]

Fixed coding style error flagged checkpatch.pl:
- ERROR: space prohibited after that open parenthesis '('
- WARNING: Block comments use * on subsequent lines

Signed-off-by: Shy More 

Output after fixing coding style issues:

$KERN/scripts/checkpatch.pl -f
./media/atomisp/pci/atomisp2/css2400/runtime/isys/src/ibuf_ctrl_rmgr.c

total: 0 errors, 0 warnings, 141 lines checked
---
 .../css2400/runtime/isys/src/ibuf_ctrl_rmgr.c  | 26 +++---
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/ibuf_ctrl_rmgr.c
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/ibuf_ctrl_rmgr.c
index 76d9142..856fb6e 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/ibuf_ctrl_rmgr.c
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/ibuf_ctrl_rmgr.c
@@ -14,18 +14,18 @@
  */
 #else
 /**
-Support for Intel Camera Imaging ISP subsystem.
-Copyright (c) 2010 - 2015, Intel Corporation.
-
-This program is free software; you can redistribute it and/or modify it
-under the terms and conditions of the GNU General Public License,
-version 2, as published by the Free Software Foundation.
-
-This program is distributed in the hope 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.
-*/
+ * Support for Intel Camera Imaging ISP subsystem.
+ * Copyright (c) 2010 - 2015, Intel Corporation.
+
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+
+ * This program is distributed in the hope 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.
+ */
 #endif
 
 #include "system_global.h"
@@ -131,7 +131,7 @@ void ia_css_isys_ibuf_rmgr_release(
for (i = 0; i < ibuf_rsrc.num_allocated; i++) {
handle = getHandle(i);
if ((handle->start_addr == *start_addr)
-   && ( true == handle->active)) {
+   && (true == handle->active)) {
handle->active = false;
ibuf_rsrc.num_active--;
break;
-- 
1.9.1

Re: [PATCH v2 07/14] v4l: vsp1: Add support for the BRS entity

[Kieran Bingham]

On 26/06/17 19:12, Laurent Pinchart wrote:
> The Blend/ROP Sub Unit (BRS) is a stripped-down version of the BRU found
> in several VSP2 instances. Compared to a regular BRU, it supports two
> inputs only, and thus has no ROP unit.
> 
> Add support for the BRS by modeling it as a new entity type, but reuse

s/modeling/modelling/


> the vsp1_bru object underneath. Chaining the BRU and BRS entities seems
> to be supported by the hardware but isn't implemented yet as it isn't
> the primary use case for the BRS.
> 
> Signed-off-by: Laurent Pinchart 

Reviewed-by: Kieran Bingham 

> ---
>  drivers/media/platform/vsp1/vsp1.h|  2 +
>  drivers/media/platform/vsp1/vsp1_bru.c| 45 ++
>  drivers/media/platform/vsp1/vsp1_bru.h|  4 +-
>  drivers/media/platform/vsp1/vsp1_drv.c| 19 +-
>  drivers/media/platform/vsp1/vsp1_entity.c | 13 ++-
>  drivers/media/platform/vsp1/vsp1_entity.h |  1 +
>  drivers/media/platform/vsp1/vsp1_pipe.c   |  7 ++--
>  drivers/media/platform/vsp1/vsp1_regs.h   | 26 +
>  drivers/media/platform/vsp1/vsp1_video.c  | 63 
> ---
>  drivers/media/platform/vsp1/vsp1_wpf.c|  4 +-
>  10 files changed, 130 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1.h 
> b/drivers/media/platform/vsp1/vsp1.h
> index 847963b6e9eb..73858a0ed35c 100644
> --- a/drivers/media/platform/vsp1/vsp1.h
> +++ b/drivers/media/platform/vsp1/vsp1.h
> @@ -54,6 +54,7 @@ struct vsp1_uds;
>  #define VSP1_HAS_WPF_HFLIP   (1 << 6)
>  #define VSP1_HAS_HGO (1 << 7)
>  #define VSP1_HAS_HGT (1 << 8)
> +#define VSP1_HAS_BRS (1 << 9)
>  
>  struct vsp1_device_info {
>   u32 version;
> @@ -76,6 +77,7 @@ struct vsp1_device {
>   struct rcar_fcp_device *fcp;
>   struct device *bus_master;
>  
> + struct vsp1_bru *brs;
>   struct vsp1_bru *bru;
>   struct vsp1_clu *clu;
>   struct vsp1_hgo *hgo;
> diff --git a/drivers/media/platform/vsp1/vsp1_bru.c 
> b/drivers/media/platform/vsp1/vsp1_bru.c
> index 85362c5ef57a..e8fd2ae3b3eb 100644
> --- a/drivers/media/platform/vsp1/vsp1_bru.c
> +++ b/drivers/media/platform/vsp1/vsp1_bru.c
> @@ -33,7 +33,7 @@
>  static inline void vsp1_bru_write(struct vsp1_bru *bru, struct vsp1_dl_list 
> *dl,
> u32 reg, u32 data)
>  {
> - vsp1_dl_list_write(dl, reg, data);
> + vsp1_dl_list_write(dl, bru->base + reg, data);
>  }
>  
>  /* 
> -
> @@ -332,11 +332,14 @@ static void bru_configure(struct vsp1_entity *entity,
>   /*
>* Route BRU input 1 as SRC input to the ROP unit and configure the ROP
>* unit with a NOP operation to make BRU input 1 available as the
> -  * Blend/ROP unit B SRC input.
> +  * Blend/ROP unit B SRC input. Only needed for BRU, the BRS has no ROP
> +  * unit.
>*/
> - vsp1_bru_write(bru, dl, VI6_BRU_ROP, VI6_BRU_ROP_DSTSEL_BRUIN(1) |
> -VI6_BRU_ROP_CROP(VI6_ROP_NOP) |
> -VI6_BRU_ROP_AROP(VI6_ROP_NOP));
> + if (entity->type == VSP1_ENTITY_BRU)
> + vsp1_bru_write(bru, dl, VI6_BRU_ROP,
> +VI6_BRU_ROP_DSTSEL_BRUIN(1) |
> +VI6_BRU_ROP_CROP(VI6_ROP_NOP) |
> +VI6_BRU_ROP_AROP(VI6_ROP_NOP));
>  
>   for (i = 0; i < bru->entity.source_pad; ++i) {
>   bool premultiplied = false;
> @@ -366,12 +369,13 @@ static void bru_configure(struct vsp1_entity *entity,
>   ctrl |= VI6_BRU_CTRL_DSTSEL_VRPF;
>  
>   /*
> -  * Route BRU inputs 0 to 3 as SRC inputs to Blend/ROP units A to
> -  * D in that order. The Blend/ROP unit B SRC is hardwired to the
> -  * ROP unit output, the corresponding register bits must be set
> -  * to 0.
> +  * Route inputs 0 to 3 as SRC inputs to Blend/ROP units A to D
> +  * in that order. In the BRU the Blend/ROP unit B SRC is
> +  * hardwired to the ROP unit output, the corresponding register
> +  * bits must be set to 0. The BRS has no ROP unit and doesn't
> +  * need any special processing.
>*/
> - if (i != 1)
> + if (!(entity->type == VSP1_ENTITY_BRU && i == 1))

If we're using this module for both BRU and BRS, would an is_bru(entity) and
is_brs(entity) be cleaner here ?

Not required - just thinking outloud...

Actaully - it's only this line that would be affected so not really needed.  I
thought there would be more uses/differences.

>   ctrl |= VI6_BRU_CTRL_SRCSEL_BRUIN(i);
>  
>   vsp1_bru_write(bru, dl, VI6_BRU_CTRL(i), ctrl);
> @@ -407,20 +411,31 @@ static const struct vsp1_entity_operations 
> bru_entity_ops = 

Re: [PATCH v3 0/2] [media] videobuf2-dc: Add support for cacheable MMAP

[Christoph Hellwig]

On Thu, Jul 13, 2017 at 02:21:53PM +0100, Russell King - ARM Linux wrote:
> My conclusion of the dma_alloc_noncoherent() and dma_cache_sync() API
> when it was introduced is that it's basically a completely broken
> interface, and I've never seen any point to it.  Maybe some of that is
> because it's badly documented - which in turn makes it badly designed
> (because there's no specification detailing what it's supposed to be
> doing.)
> 
> I'd like to see that thing die...

It's not exactly the best interface ever, so any improvement is welcome.

I've posted a series to kill dma_alloc_noncoherent in favor of
dma_alloc_attrs a short while ago, and a big chunk of it should have
made it into 4.12.  I plan to kill it off entirely for 4.13.

That leaves dma_cache_sync() - it's used by 6 drivers:

drivers/net/ethernet/i825xx/lasi_82596.c
drivers/net/ethernet/seeq/sgiseeq.c
drivers/scsi/53c700.c
drivers/scsi/sgiwd93.c
drivers/sh/maple/maple.c
drivers/tty/serial/mpsc.c

Those are used on parisc, mips for a few old SGI systems, the SH
dreamcast and powerpc marvell mv64x60 devices.

So it shouldn't be too hard to figure out if they could be moved
to the normal dma_sync_* calls.

On parisc dma_cache_sync is equivalent to dma_sync_single_for_cpu,
so that should be fine.

On mips the implementation of dma_sync_single_for_cpu is a little
more complicated, but both dma_sync_single_for_cpu and dma_cache_sync
end up calling __dma_sync_virtual, so they look like the same in
the end as well.

On SH sync_single_for_device is implemented using dma_cache_sync,
and there is no dma_sync_single_for_cpu.

On powerpc both dma_sync_single_for_cpu and dma_sync_single_for_device
are implemented using the same primitive as dma_cache_sync.

In short: killing off dma_cache_sync and using the existing and
better defined syncing primitives looks entirely feasible.

I'll add it to my TODO list for 4.13.

Re: [PATCH v3 0/2] [media] videobuf2-dc: Add support for cacheable MMAP

[Russell King - ARM Linux]

On Thu, Jul 13, 2017 at 01:13:28PM +0200, Marek Szyprowski wrote:
> Hi Christoph,
> 
> On 2017-07-05 19:33, Christoph Hellwig wrote:
> >On Mon, Jul 03, 2017 at 11:27:32AM +0200, Marek Szyprowski wrote:
> >>The main question here if we want to merge incomplete solution or not. As
> >>for now, there is no support in ARM/ARM64 for NON_CONSISTENT attribute.
> >>Also none of the v4l2 drivers use it. Sadly support for NON_CONSISTENT
> >>attribute is not fully implemented nor even defined in mainline.
> >>
> >DMA_ATTR_NON_CONSISTENT is the way to get the dma_alloc_noncoherent
> >semantics through the dma_alloc_attr API, and as such I think it is
> >pretty well defined, although the documentation in
> >Documentation/DMA-attributes.txt is really bad and we need to improve
> >it, by merging it with the dma_alloc_noncoherent description in
> >Documentation/DMA-API.txt. My series to remove dma_alloc_noncoherent
> >updates the latter to mention DMA_ATTR_NON_CONSISTENT, but
> >we should probably merge Documentation/DMA-API.txt,
> >Documentation/DMA-attributes.txt and Documentation/DMA-API-HOWTO.txt
> >into a single coherent document.
> 
> Right. I started conversion of dma_alloc_noncoherent to NON_CONSISTENT
> DMA attribute, but later I got stuck at the details of cache
> synchronization.
> 
> >>I know that it works fine for some vendor kernel trees, but supporting it in
> >>mainline was a bit controversial. There is no proper way to sync cache for
> >>such
> >>buffers. Calling dma_sync_sg worked so far, but it has to be first agreed as
> >>a proper DMA API.
> >As documented in Documentation/DMA-API.txt the proper way to sync
> >noncoherent/nonconsistent regions is to call dma_cache_sync.  It seems
> >like it generally is the same as dma_sync_range/sg so if we could
> >eventually merge these APIs that should reduce the confusion further.
> 
> Original dma_alloc_noncoherent utilized dma_cache_sync() function, which had
> some flaws, which prevented me to continue that task and introduce it to ARM
> architecture. The dma_alloc_noncoherent() and dma_cache_sync() API lacks
> buffer ownership and imprecisely defines how and when the caches has to be
> synchronized. dma_cache_sync() also lacks DMA address argument, what also
> complicates potential lightweight implementation.

My conclusion of the dma_alloc_noncoherent() and dma_cache_sync() API
when it was introduced is that it's basically a completely broken
interface, and I've never seen any point to it.  Maybe some of that is
because it's badly documented - which in turn makes it badly designed
(because there's no specification detailing what it's supposed to be
doing.)

I'd like to see that thing die...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 06/14] v4l: vsp1: Add pipe index argument to the VSP-DU API

[Kieran Bingham]

On 26/06/17 19:12, Laurent Pinchart wrote:
> In the H3 ES2.0 SoC the VSP2-DL instance has two connections to DU
> channels that need to be configured independently. Extend the VSP-DU API
> with a pipeline index to identify which pipeline the caller wants to
> operate on.
> 
> Signed-off-by: Laurent Pinchart 

A bit of comment merge between this and the next patch but it's minor and not
worth the effort to change that ... so I'll happily ignore it if you do :)

Reviewed-by: Kieran Bingham 

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c  | 12 ++--
>  drivers/media/platform/vsp1/vsp1_drm.c | 32 ++--
>  include/media/vsp1.h   | 10 ++
>  3 files changed, 34 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c 
> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index f870445ebc8d..d46dce054442 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -81,22 +81,22 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>*/
>   crtc->group->need_restart = true;
>  
> - vsp1_du_setup_lif(crtc->vsp->vsp, );
> + vsp1_du_setup_lif(crtc->vsp->vsp, 0, );
>  }
>  
>  void rcar_du_vsp_disable(struct rcar_du_crtc *crtc)
>  {
> - vsp1_du_setup_lif(crtc->vsp->vsp, NULL);
> + vsp1_du_setup_lif(crtc->vsp->vsp, 0, NULL);
>  }
>  
>  void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
>  {
> - vsp1_du_atomic_begin(crtc->vsp->vsp);
> + vsp1_du_atomic_begin(crtc->vsp->vsp, 0);
>  }
>  
>  void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
>  {
> - vsp1_du_atomic_flush(crtc->vsp->vsp);
> + vsp1_du_atomic_flush(crtc->vsp->vsp, 0);
>  }
>  
>  /* Keep the two tables in sync. */
> @@ -192,7 +192,7 @@ static void rcar_du_vsp_plane_setup(struct 
> rcar_du_vsp_plane *plane)
>   }
>   }
>  
> - vsp1_du_atomic_update(plane->vsp->vsp, plane->index, );
> + vsp1_du_atomic_update(plane->vsp->vsp, 0, plane->index, );
>  }
>  
>  static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
> @@ -292,7 +292,7 @@ static void rcar_du_vsp_plane_atomic_update(struct 
> drm_plane *plane,
>   if (plane->state->crtc)
>   rcar_du_vsp_plane_setup(rplane);
>   else
> - vsp1_du_atomic_update(rplane->vsp->vsp, rplane->index, NULL);
> + vsp1_du_atomic_update(rplane->vsp->vsp, 0, rplane->index, NULL);
>  }
>  
>  static const struct drm_plane_helper_funcs rcar_du_vsp_plane_helper_funcs = {
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index c72d021ff820..daaafe7885fa 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -58,21 +58,26 @@ EXPORT_SYMBOL_GPL(vsp1_du_init);
>  /**
>   * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
>   * @dev: the VSP device
> + * @pipe_index: the DRM pipeline index
>   * @cfg: the LIF configuration
>   *
>   * Configure the output part of VSP DRM pipeline for the given frame 
> @cfg.width
> - * and @cfg.height. This sets up formats on the BRU source pad, the WPF0 sink
> - * and source pads, and the LIF sink pad.
> + * and @cfg.height. This sets up formats on the blend unit (BRU or BRS) 
> source
> + * pad, the WPF sink and source pads, and the LIF sink pad.
>   *
> - * As the media bus code on the BRU source pad is conditioned by the
> - * configuration of the BRU sink 0 pad, we also set up the formats on all BRU
> + * The @pipe_index argument selects which DRM pipeline to setup. The number 
> of
> + * available pipelines depend on the VSP instance.
> + *
> + * As the media bus code on the blend unit source pad is conditioned by the
> + * configuration of its sink 0 pad, we also set up the formats on all blend 
> unit
>   * sinks, even if the configuration will be overwritten later by
> - * vsp1_du_setup_rpf(). This ensures that the BRU configuration is set to a 
> well
> - * defined state.
> + * vsp1_du_setup_rpf(). This ensures that the blend unit configuration is 
> set to
> + * a well defined state.

I presume those comment updates for the BRU/ blend-unit configuration are
actually for the next patch - but I don't think it matters here - and isn't
worth the effort to move the hunks.

It all reads OK.


>   *
>   * Return 0 on success or a negative error code on failure.
>   */
> -int vsp1_du_setup_lif(struct device *dev, const struct vsp1_du_lif_config 
> *cfg)
> +int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
> +   const struct vsp1_du_lif_config *cfg)
>  {
>   struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>   struct vsp1_pipeline *pipe = >drm->pipe;
> @@ -81,6 +86,9 @@ int vsp1_du_setup_lif(struct device *dev, const struct 
> vsp1_du_lif_config *cfg)
>   unsigned int i;
>   int ret;
>  
> + if (pipe_index > 

Re: [PATCH v2 05/14] v4l: vsp1: Don't create links for DRM pipeline

[Kieran Bingham]

On 26/06/17 19:12, Laurent Pinchart wrote:
> When the VSP1 is used in a DRM pipeline the driver doesn't register the
> media device. Links between entities are not exposed to userspace, but
> are still used internally for the sole purpose of setting up internal
> source to sink pointers through the link setup handler.
> 
> Instead of going through this complex procedure, remove link creation
> and set the sink pointers directly.
> 
> Signed-off-by: Laurent Pinchart 

A whole function removed ... always love code removal...

Reviewed-by: Kieran Bingham 


> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 53 
> --
>  drivers/media/platform/vsp1/vsp1_drm.h |  1 -
>  drivers/media/platform/vsp1/vsp1_drv.c | 16 --
>  3 files changed, 12 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index 2d5a74e95e09..c72d021ff820 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -487,6 +487,7 @@ void vsp1_du_atomic_flush(struct device *dev)
>  
>   vsp1->bru->inputs[i].rpf = rpf;
>   rpf->bru_input = i;
> + rpf->entity.sink = >bru->entity;
>   rpf->entity.sink_pad = i;
>  
>   dev_dbg(vsp1->dev, "%s: connecting RPF.%u to BRU:%u\n",
> @@ -564,53 +565,6 @@ EXPORT_SYMBOL_GPL(vsp1_du_unmap_sg);
>   * Initialization
>   */
>  
> -int vsp1_drm_create_links(struct vsp1_device *vsp1)
> -{
> - const u32 flags = MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE;
> - unsigned int i;
> - int ret;
> -
> - /*
> -  * VSPD instances require a BRU to perform composition and a LIF to
> -  * output to the DU.
> -  */
> - if (!vsp1->bru || !vsp1->lif)
> - return -ENXIO;
> -
> - for (i = 0; i < vsp1->info->rpf_count; ++i) {
> - struct vsp1_rwpf *rpf = vsp1->rpf[i];
> -
> - ret = media_create_pad_link(>entity.subdev.entity,
> - RWPF_PAD_SOURCE,
> - >bru->entity.subdev.entity,
> - i, flags);
> - if (ret < 0)
> - return ret;
> -
> - rpf->entity.sink = >bru->entity;
> - rpf->entity.sink_pad = i;
> - }
> -
> - ret = media_create_pad_link(>bru->entity.subdev.entity,
> - vsp1->bru->entity.source_pad,
> - >wpf[0]->entity.subdev.entity,
> - RWPF_PAD_SINK, flags);
> - if (ret < 0)
> - return ret;
> -
> - vsp1->bru->entity.sink = >wpf[0]->entity;
> - vsp1->bru->entity.sink_pad = RWPF_PAD_SINK;
> -
> - ret = media_create_pad_link(>wpf[0]->entity.subdev.entity,
> - RWPF_PAD_SOURCE,
> - >lif->entity.subdev.entity,
> - LIF_PAD_SINK, flags);
> - if (ret < 0)
> - return ret;
> -
> - return 0;
> -}
> -
>  int vsp1_drm_init(struct vsp1_device *vsp1)
>  {
>   struct vsp1_pipeline *pipe;
> @@ -631,6 +585,11 @@ int vsp1_drm_init(struct vsp1_device *vsp1)
>   list_add_tail(>entity.list_pipe, >entities);
>   }
>  
> + vsp1->bru->entity.sink = >wpf[0]->entity;
> + vsp1->bru->entity.sink_pad = 0;
> + vsp1->wpf[0]->entity.sink = >lif->entity;
> + vsp1->wpf[0]->entity.sink_pad = 0;
> +
>   list_add_tail(>bru->entity.list_pipe, >entities);
>   list_add_tail(>wpf[0]->entity.list_pipe, >entities);
>   list_add_tail(>lif->entity.list_pipe, >entities);
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.h 
> b/drivers/media/platform/vsp1/vsp1_drm.h
> index cbdbb8a39883..67d6549edfad 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.h
> +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> @@ -48,6 +48,5 @@ static inline struct vsp1_drm *to_vsp1_drm(struct 
> vsp1_pipeline *pipe)
>  
>  int vsp1_drm_init(struct vsp1_device *vsp1);
>  void vsp1_drm_cleanup(struct vsp1_device *vsp1);
> -int vsp1_drm_create_links(struct vsp1_device *vsp1);
>  
>  #endif /* __VSP1_DRM_H__ */
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c 
> b/drivers/media/platform/vsp1/vsp1_drv.c
> index 9b3a0790f92a..5a467b118a1c 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -423,19 +423,15 @@ static int vsp1_create_entities(struct vsp1_device 
> *vsp1)
>   goto done;
>   }
>  
> - /* Create links. */
> - if (vsp1->info->uapi)
> - ret = vsp1_uapi_create_links(vsp1);
> - else
> - ret = vsp1_drm_create_links(vsp1);
> - if (ret < 0)
> - goto done;
> -
>   /*
> -  * Register subdev nodes if the userspace API is enabled or 

Re: [PATCH v2 04/14] v4l: vsp1: Store source and sink pointers as vsp1_entity

[Kieran Bingham]

Hi Laurent,

This looks like a good simplification/removal of obfuscation to me!

On 26/06/17 19:12, Laurent Pinchart wrote:
> The internal VSP entity source and sink pointers are stored as
> media_entity pointers, which are then cast to a vsp1_entity. As all
> sources and sinks are vsp1_entity instances, we can store the
> vsp1_entity pointers directly.
> 
> Signed-off-by: Laurent Pinchart 

Reviewed-by: Kieran Bingham 

> ---
>  drivers/media/platform/vsp1/vsp1_drm.c|  4 ++--
>  drivers/media/platform/vsp1/vsp1_drv.c|  2 +-
>  drivers/media/platform/vsp1/vsp1_entity.c | 26 +-
>  drivers/media/platform/vsp1/vsp1_entity.h |  4 ++--
>  4 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index bc3fd9bc7126..2d5a74e95e09 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -587,7 +587,7 @@ int vsp1_drm_create_links(struct vsp1_device *vsp1)
>   if (ret < 0)
>   return ret;
>  
> - rpf->entity.sink = >bru->entity.subdev.entity;
> + rpf->entity.sink = >bru->entity;
>   rpf->entity.sink_pad = i;
>   }
>  
> @@ -598,7 +598,7 @@ int vsp1_drm_create_links(struct vsp1_device *vsp1)
>   if (ret < 0)
>   return ret;
>  
> - vsp1->bru->entity.sink = >wpf[0]->entity.subdev.entity;
> + vsp1->bru->entity.sink = >wpf[0]->entity;
>   vsp1->bru->entity.sink_pad = RWPF_PAD_SINK;
>  
>   ret = media_create_pad_link(>wpf[0]->entity.subdev.entity,
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c 
> b/drivers/media/platform/vsp1/vsp1_drv.c
> index 35087d5573ce..9b3a0790f92a 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -121,7 +121,7 @@ static int vsp1_create_sink_links(struct vsp1_device 
> *vsp1,
>   return ret;
>  
>   if (flags & MEDIA_LNK_FL_ENABLED)
> - source->sink = entity;
> + source->sink = sink;
>   }
>   }
>  
> diff --git a/drivers/media/platform/vsp1/vsp1_entity.c 
> b/drivers/media/platform/vsp1/vsp1_entity.c
> index 4bdb3b141611..71dd903263ad 100644
> --- a/drivers/media/platform/vsp1/vsp1_entity.c
> +++ b/drivers/media/platform/vsp1/vsp1_entity.c
> @@ -24,18 +24,11 @@
>  #include "vsp1_pipe.h"
>  #include "vsp1_rwpf.h"
>  
> -static inline struct vsp1_entity *
> -media_entity_to_vsp1_entity(struct media_entity *entity)
> -{
> - return container_of(entity, struct vsp1_entity, subdev.entity);
> -}
> -
>  void vsp1_entity_route_setup(struct vsp1_entity *entity,
>struct vsp1_pipeline *pipe,
>struct vsp1_dl_list *dl)
>  {
>   struct vsp1_entity *source;
> - struct vsp1_entity *sink;
>  
>   if (entity->type == VSP1_ENTITY_HGO) {
>   u32 smppt;
> @@ -44,7 +37,7 @@ void vsp1_entity_route_setup(struct vsp1_entity *entity,
>* The HGO is a special case, its routing is configured on the
>* sink pad.
>*/
> - source = media_entity_to_vsp1_entity(entity->sources[0]);
> + source = entity->sources[0];
>   smppt = (pipe->output->entity.index << VI6_DPR_SMPPT_TGW_SHIFT)
> | (source->route->output << VI6_DPR_SMPPT_PT_SHIFT);
>  
> @@ -57,7 +50,7 @@ void vsp1_entity_route_setup(struct vsp1_entity *entity,
>* The HGT is a special case, its routing is configured on the
>* sink pad.
>*/
> - source = media_entity_to_vsp1_entity(entity->sources[0]);
> + source = entity->sources[0];
>   smppt = (pipe->output->entity.index << VI6_DPR_SMPPT_TGW_SHIFT)
> | (source->route->output << VI6_DPR_SMPPT_PT_SHIFT);
>  
> @@ -69,9 +62,8 @@ void vsp1_entity_route_setup(struct vsp1_entity *entity,
>   if (source->route->reg == 0)
>   return;
>  
> - sink = media_entity_to_vsp1_entity(source->sink);
>   vsp1_dl_list_write(dl, source->route->reg,
> -sink->route->inputs[source->sink_pad]);
> +source->sink->route->inputs[source->sink_pad]);
>  }
>  
>  /* 
> -
> @@ -316,6 +308,12 @@ int vsp1_subdev_enum_frame_size(struct v4l2_subdev 
> *subdev,
>   * Media Operations
>   */
>  
> +static inline struct vsp1_entity *
> +media_entity_to_vsp1_entity(struct media_entity *entity)
> +{
> + return container_of(entity, struct vsp1_entity, subdev.entity);
> +}
> +
>  static int vsp1_entity_link_setup_source(const struct media_pad *source_pad,
>

Re: [PATCH v2 03/14] v4l: vsp1: Don't set WPF sink pointer

[Kieran Bingham]

On 26/06/17 19:12, Laurent Pinchart wrote:
> The sink pointer is used to configure routing inside the VSP, and as
> such must point to the next VSP entity in the pipeline. The WPF being a
> pipeline terminal sink, its output route can't be configured. The
> routing configuration code already handles this correctly without
> referring to the sink pointer, which thus doesn't need to be set.
> 
> Signed-off-by: Laurent Pinchart 

Reviewed-by: Kieran Bingham 

> ---
>  drivers/media/platform/vsp1/vsp1_drv.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c 
> b/drivers/media/platform/vsp1/vsp1_drv.c
> index 6b35e043b554..35087d5573ce 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -412,7 +412,6 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
>   }
>  
>   list_add_tail(>list, >videos);
> - wpf->entity.sink = >video.entity;
>   }
>   }
>  
> 

Re: [PATCH v2 01/14] v4l: vsp1: Fill display list headers without holding dlm spinlock

[Kieran Bingham]

Hi Laurent,

Starts easy ... (I haven't gone through these in numerical order of course :D)

On 26/06/17 19:12, Laurent Pinchart wrote:
> The display list headers are filled using information from the display
> list only. Lower the display list manager spinlock contention by filling
> the headers without holding the lock.
> 
> Signed-off-by: Laurent Pinchart 

Reviewed-by: Kieran Bingham 

> ---
>  drivers/media/platform/vsp1/vsp1_dl.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
> b/drivers/media/platform/vsp1/vsp1_dl.c
> index aaf17b13fd78..dc47e236c780 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -483,8 +483,6 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
>   unsigned long flags;
>   bool update;
>  
> - spin_lock_irqsave(>lock, flags);
> -
>   if (dl->dlm->mode == VSP1_DL_MODE_HEADER) {
>   struct vsp1_dl_list *dl_child;
>  
> @@ -501,7 +499,11 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
>  
>   vsp1_dl_list_fill_header(dl_child, last);
>   }
> + }
>  
> + spin_lock_irqsave(>lock, flags);
> +
> + if (dl->dlm->mode == VSP1_DL_MODE_HEADER) {
>   /*
>* Commit the head display list to hardware. Chained headers
>* will auto-start.
> 

Re: [PATCH v2 00/14] Renesas R-Car VSP: Add H3 ES2.0 support

[Kieran Bingham]

Hi Laurent,

Thankyou for these patches, bringing life to the outputs of my ES2.0 target 
board.

I have tested them on my board, and including the VSP unit test suite, and
kmscube utilities.

Feel free to add a Tested-by: Kieran Bingham
 to all of the patches if you desire,
and I'm working through the individual reviews.

Oh - except now I've just said that - I did some extra testing with both HDMI
and VGA output connected and ran
   kmstest --flip --sync

This was soon followed by a kernel error trace [0] shown below.

However replicating this is possibly more complicated than just running kmstest
--flip --sync ... I've had to do various iterations of running with/without
{flip, sync} but I have reproduced 'issues' about 3 times now.

I think the key thing is in the VGA connection or regarding trying to output to
both.

Interestingly, I haven't been able to make this happen on the ES1.0 platform as
yet... though --flip --sync is quicker to fail with 'Flip Commit failed: -16"
there...

For reference this was tested on your pinchartl-media/drm/next/h3-es2/merged,
branch which I don't believe has the recent work on not needing to wait for a
final vblank on shutdown. So it is quite possible that the issue I am seeing is
simply a symptom of that separately repaired issue.

On that basis I've left my comment regarding my Tested-by: tag above as I
suspect that this issue I've hit could likely be separate and already resolved.

I'll try to add those patches to this tree to see if the issue resolves 
itself...

Regardless of that, this series conflicts with my current developments,
therefore I will likely rebase my work on top of this series. I don't need an
immutable branch, but please do let me know if this series changes :-)

--
Regards

Kieran.






[0] : Kernel log snippet posted below:

[  597.471369] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
[CRTC:57:crtc-3] flip_done timed out
[  607.711346] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[CRTC:57:crtc-3] flip_done timed out
[  607.711354] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
[CRTC:57:crtc-3] flip_done timed out
[  607.749585] vsp1 fea2.vsp: failed to reset wpf.1
[  617.951311] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
[CRTC:57:crtc-3] flip_done timed out
[  618.055358] vsp1 fea2.vsp: failed to reset wpf.1
[  618.060498] vsp1 fea2.vsp: DRM pipeline stop timeout
[  628.831762] vsp1 fea2.vsp: failed to reset wpf.1
[  638.943315] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
[CRTC:57:crtc-3] flip_done timed out
[  649.183313] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
[CRTC:57:crtc-3] flip_done timed out
[  677.753465] vsp1 fea2.vsp: failed to reset wpf.1
[  687.839322] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
[CRTC:57:crtc-3] flip_done timed out
[  687.935288] vsp1 fea2.vsp: failed to reset wpf.1
[  687.940360] vsp1 fea2.vsp: DRM pipeline stop timeout
[  687.945939] Unable to handle kernel NULL pointer dereference at virtual
address 
[  687.954206] pgd = ff8009a0a000
[  687.957680] [] *pgd=00073fffe003, *pud=00073fffe003,
*pmd=
[  687.966068] Internal error: Oops: 9606 [#1] PREEMPT SMP
[  687.971690] Modules linked in:
[  687.974777] CPU: 0 PID: 10426 Comm: kmstest Not tainted
4.12.0-rc5-01343-g1bc824acf27c #5
[  687.983019] Hardware name: Renesas Salvator-X 2nd version board based on
r8a7795 ES2.0+ (DT)
[  687.991525] task: ffc6f97b4080 task.stack: ffc6f51dc000
[  687.997502] PC is at __media_pipeline_stop+0x24/0xd0
[  688.002507] LR is at media_pipeline_stop+0x34/0x48
[  688.007336] pc : [] lr : [] pstate: 
6145
[  688.014790] sp : ffc6f51df780
[  688.018129] x29: ffc6f51df780 x28: 
[  688.023490] x27: 0038 x26: ff8008960088
[  688.028850] x25: ffc6f98bcb10 x24: ffc6f98b8818
[  688.034210] x23: 0001 x22: ffc6f9ff1998
[  688.039570] x21: ffc6f98bc080 x20: 
[  688.044929] x19: 0008 x18: 0010
[  688.050288] x17: 007f9bc9e1c8 x16: ff8008165bb8
[  688.055648] x15: ff80899bb63f x14: 0006
[  688.061007] x13: ffc6faac6750 x12: ff80087bd078
[  688.066366] x11:  x10: ff80097bb000
[  688.071725] x9 : ff8008afd000 x8 : 
[  688.077085] x7 : ff8008583c1c x6 : ff8008da75c8
[  688.082443] x5 : ff8009585d00 x4 : 2d28ca88
[  688.087803] x3 : 89277f76 x2 : 
[  688.093162] x1 : ffc6f97b4080 x0 : ff8008583c24
[  688.098523] Process kmstest (pid: 10426, stack limit = 0xffc6f51dc000)
[  688.105453] Stack: (0xffc6f51df780 to 0xffc6f51e)
[  688.111245] f780: ffc6f51df7b0 ff8008583c24 ffc6f98b8ae8
ffc6f98bc080
[  688.119138] f7a0: ffc6f98bc818 ff8009d18000 ffc6f51df7e0
ff80085aa688
[  688.127031] f7c0: 

[PATCH] [media] dib0090: make const array dib0090_tuning_table_cband_7090e_aci static

[Colin King]

From: Colin Ian King 

Don't populate array dib0090_tuning_table_cband_7090e_aci on the stack but
instead make it static. Makes the object code smaller by over 180 bytes:

Before:
   textdata bss dec hex filename
  400527320 192   47564b9cc dib0090.o

After:
   textdata bss dec hex filename
  397807408 192   47380b914 dib0090.o

Signed-off-by: Colin Ian King 
---
 drivers/media/dvb-frontends/dib0090.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/dib0090.c 
b/drivers/media/dvb-frontends/dib0090.c
index 33af14df27bd..ae53a199b7af 100644
--- a/drivers/media/dvb-frontends/dib0090.c
+++ b/drivers/media/dvb-frontends/dib0090.c
@@ -2052,7 +2052,7 @@ int dib0090_update_tuning_table_7090(struct dvb_frontend 
*fe,
struct dib0090_state *state = fe->tuner_priv;
const struct dib0090_tuning *tune =
dib0090_tuning_table_cband_7090e_sensitivity;
-   const struct dib0090_tuning dib0090_tuning_table_cband_7090e_aci[] = {
+   static const struct dib0090_tuning 
dib0090_tuning_table_cband_7090e_aci[] = {
{ 30,  0 ,  3,  0x8165, 0x2c0, 0x2d12, 0xb84e, EN_CAB },
{ 65,  0 ,  4,  0x815B, 0x280, 0x2d12, 0xb84e, EN_CAB },
{ 86,  0 ,  5,  0x84EF, 0x280, 0x2d12, 0xb84e, EN_CAB },
-- 
2.11.0

[PATCH] [media] drxj: make several const arrays static

[Colin King]

From: Colin Ian King 

Don't populate const arrays on the stack but instead make them static.
Makes the object code smaller by over 1800 bytes:

Before:
   textdata bss dec hex filename
  941009160   0  103260   1935c drxj.o

After:
   textdata bss dec hex filename
  91044   10400   0  101444   18c44 drxj.o

Signed-off-by: Colin Ian King 
---
 drivers/media/dvb-frontends/drx39xyj/drxj.c | 35 +++--
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/media/dvb-frontends/drx39xyj/drxj.c 
b/drivers/media/dvb-frontends/drx39xyj/drxj.c
index 14040c915dbb..499ccff557bf 100644
--- a/drivers/media/dvb-frontends/drx39xyj/drxj.c
+++ b/drivers/media/dvb-frontends/drx39xyj/drxj.c
@@ -5489,7 +5489,7 @@ static int set_vsb_leak_n_gain(struct drx_demod_instance 
*demod)
struct i2c_device_addr *dev_addr = NULL;
int rc;
 
-   const u8 vsb_ffe_leak_gain_ram0[] = {
+   static const u8 vsb_ffe_leak_gain_ram0[] = {
DRXJ_16TO8(0x8),/* FFETRAINLKRATIO1  */
DRXJ_16TO8(0x8),/* FFETRAINLKRATIO2  */
DRXJ_16TO8(0x8),/* FFETRAINLKRATIO3  */
@@ -5620,7 +5620,7 @@ static int set_vsb_leak_n_gain(struct drx_demod_instance 
*demod)
DRXJ_16TO8(0x1010)  /* FIRRCA1GAIN8 */
};
 
-   const u8 vsb_ffe_leak_gain_ram1[] = {
+   static const u8 vsb_ffe_leak_gain_ram1[] = {
DRXJ_16TO8(0x1010), /* FIRRCA1GAIN9 */
DRXJ_16TO8(0x0808), /* FIRRCA1GAIN10 */
DRXJ_16TO8(0x0808), /* FIRRCA1GAIN11 */
@@ -5710,7 +5710,7 @@ static int set_vsb(struct drx_demod_instance *demod)
struct drxj_data *ext_attr = NULL;
u16 cmd_result = 0;
u16 cmd_param = 0;
-   const u8 vsb_taps_re[] = {
+   static const u8 vsb_taps_re[] = {
DRXJ_16TO8(-2), /* re0  */
DRXJ_16TO8(4),  /* re1  */
DRXJ_16TO8(1),  /* re2  */
@@ -,7 +,7 @@ static int set_qam16(struct drx_demod_instance *demod)
 {
struct i2c_device_addr *dev_addr = demod->my_i2c_dev_addr;
int rc;
-   const u8 qam_dq_qual_fun[] = {
+   static const u8 qam_dq_qual_fun[] = {
DRXJ_16TO8(2),  /* fun0  */
DRXJ_16TO8(2),  /* fun1  */
DRXJ_16TO8(2),  /* fun2  */
@@ -6674,7 +6674,7 @@ static int set_qam16(struct drx_demod_instance *demod)
DRXJ_16TO8(3),  /* fun4  */
DRXJ_16TO8(3),  /* fun5  */
};
-   const u8 qam_eq_cma_rad[] = {
+   static const u8 qam_eq_cma_rad[] = {
DRXJ_16TO8(13517),  /* RAD0  */
DRXJ_16TO8(13517),  /* RAD1  */
DRXJ_16TO8(13517),  /* RAD2  */
@@ -6901,7 +6901,7 @@ static int set_qam32(struct drx_demod_instance *demod)
 {
struct i2c_device_addr *dev_addr = demod->my_i2c_dev_addr;
int rc;
-   const u8 qam_dq_qual_fun[] = {
+   static const u8 qam_dq_qual_fun[] = {
DRXJ_16TO8(3),  /* fun0  */
DRXJ_16TO8(3),  /* fun1  */
DRXJ_16TO8(3),  /* fun2  */
@@ -6909,7 +6909,7 @@ static int set_qam32(struct drx_demod_instance *demod)
DRXJ_16TO8(4),  /* fun4  */
DRXJ_16TO8(4),  /* fun5  */
};
-   const u8 qam_eq_cma_rad[] = {
+   static const u8 qam_eq_cma_rad[] = {
DRXJ_16TO8(6707),   /* RAD0  */
DRXJ_16TO8(6707),   /* RAD1  */
DRXJ_16TO8(6707),   /* RAD2  */
@@ -7136,7 +7136,8 @@ static int set_qam64(struct drx_demod_instance *demod)
 {
struct i2c_device_addr *dev_addr = demod->my_i2c_dev_addr;
int rc;
-   const u8 qam_dq_qual_fun[] = {  /* this is hw reset value. no necessary 
to re-write */
+   static const u8 qam_dq_qual_fun[] = {
+   /* this is hw reset value. no necessary to re-write */
DRXJ_16TO8(4),  /* fun0  */
DRXJ_16TO8(4),  /* fun1  */
DRXJ_16TO8(4),  /* fun2  */
@@ -7144,7 +7145,7 @@ static int set_qam64(struct drx_demod_instance *demod)
DRXJ_16TO8(6),  /* fun4  */
DRXJ_16TO8(6),  /* fun5  */
};
-   const u8 qam_eq_cma_rad[] = {
+   static const u8 qam_eq_cma_rad[] = {
DRXJ_16TO8(13336),  /* RAD0  */
DRXJ_16TO8(12618),  /* RAD1  */
DRXJ_16TO8(11988),  /* RAD2  */
@@ -7371,7 +7372,7 @@ static int set_qam128(struct drx_demod_instance *demod)
 {
struct i2c_device_addr *dev_addr = demod->my_i2c_dev_addr;
int rc;
-   const u8 qam_dq_qual_fun[] = {
+   static const u8 qam_dq_qual_fun[] = {
DRXJ_16TO8(6),  /* fun0  */
DRXJ_16TO8(6),  /* fun1  */
DRXJ_16TO8(6),  /* fun2  */
@@ -7379,7 +7380,7 @@ 

[PATCH] [media] drxd: make const arrays slowIncrDecLUT and fastIncrDecLUT static

[Colin King]

From: Colin Ian King 

Don't populate arrays slowIncrDecLUT and fastIncrDecLUT on the stack but
instead make them static. Makes the object code smaller by over 100 bytes:

   textdata bss dec hex filename
  27776 832  64   286727000 drxd_hard.o

   textdata bss dec hex filename
  27530 976  64   285706f9a drxd_hard.o

Signed-off-by: Colin Ian King 
---
 drivers/media/dvb-frontends/drxd_hard.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-frontends/drxd_hard.c 
b/drivers/media/dvb-frontends/drxd_hard.c
index 17638e08835a..7d04400b18dd 100644
--- a/drivers/media/dvb-frontends/drxd_hard.c
+++ b/drivers/media/dvb-frontends/drxd_hard.c
@@ -638,8 +638,10 @@ static int SetCfgIfAgc(struct drxd_state *state, struct 
SCfgAgc *cfg)
/* == Speed == */
{
const u16 maxRur = 8;
-   const u16 slowIncrDecLUT[] = { 3, 4, 4, 5, 6 };
-   const u16 fastIncrDecLUT[] = { 14, 15, 15, 16,
+   static const u16 slowIncrDecLUT[] = {
+   3, 4, 4, 5, 6 };
+   const u16 fastIncrDecLUT[] = {
+   14, 15, 15, 16,
17, 18, 18, 19,
20, 21, 22, 23,
24, 26, 27, 28,
-- 
2.11.0

Re: [PATCH v3 0/2] [media] videobuf2-dc: Add support for cacheable MMAP

[Marek Szyprowski]

Hi Christoph,

On 2017-07-05 19:33, Christoph Hellwig wrote:

On Mon, Jul 03, 2017 at 11:27:32AM +0200, Marek Szyprowski wrote:

The main question here if we want to merge incomplete solution or not. As
for now, there is no support in ARM/ARM64 for NON_CONSISTENT attribute.
Also none of the v4l2 drivers use it. Sadly support for NON_CONSISTENT
attribute is not fully implemented nor even defined in mainline.


DMA_ATTR_NON_CONSISTENT is the way to get the dma_alloc_noncoherent
semantics through the dma_alloc_attr API, and as such I think it is
pretty well defined, although the documentation in
Documentation/DMA-attributes.txt is really bad and we need to improve
it, by merging it with the dma_alloc_noncoherent description in
Documentation/DMA-API.txt. My series to remove dma_alloc_noncoherent
updates the latter to mention DMA_ATTR_NON_CONSISTENT, but
we should probably merge Documentation/DMA-API.txt,
Documentation/DMA-attributes.txt and Documentation/DMA-API-HOWTO.txt
into a single coherent document.


Right. I started conversion of dma_alloc_noncoherent to NON_CONSISTENT
DMA attribute, but later I got stuck at the details of cache 
synchronization.



I know that it works fine for some vendor kernel trees, but supporting it in
mainline was a bit controversial. There is no proper way to sync cache for
such
buffers. Calling dma_sync_sg worked so far, but it has to be first agreed as
a proper DMA API.

As documented in Documentation/DMA-API.txt the proper way to sync
noncoherent/nonconsistent regions is to call dma_cache_sync.  It seems
like it generally is the same as dma_sync_range/sg so if we could
eventually merge these APIs that should reduce the confusion further.


Original dma_alloc_noncoherent utilized dma_cache_sync() function, which had
some flaws, which prevented me to continue that task and introduce it to ARM
architecture. The dma_alloc_noncoherent() and dma_cache_sync() API lacks
buffer ownership and imprecisely defines how and when the caches has to be
synchronized. dma_cache_sync() also lacks DMA address argument, what also
complicates potential lightweight implementation.

IMHO it would make sense to change it to work similar to the other
dma_sync_*_for_{cpu,device} functions, but I didn't find enough time to
finally take a try.

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

Re: [PATCH 7/8] omap3isp: Check for valid port in endpoints

[Pavel Machek]

Hi!

> > On Thu, Jul 06, 2017 at 02:00:18AM +0300, Sakari Ailus wrote:
> > > Check that we do have a valid port in an endpoint, return an error if not.
> > > 
> > > Signed-off-by: Sakari Ailus 
> > 
> > Reviewed-by: Sebastian Reichel 
> 
> Thanks for the reviews, Sebastian and Pavel!
> 
> I'll send a pull request on these for 4.14 once we have -rc1 in the media
> tree.

I can update my patches when updated ccp2 branch is ready.

And actually, I'd quite like to get rest of support ready for 4.14,
too. I believe we are close enough.

I re-checked, and it seems to work w/o the "smiapp-pll: Take existing
divisor into account in minimum divisor check" patch; so with
"omap3isp: add CSI1 support" plus the other ccp2 patches, plus dts
changes, we should be ok.

Best regards,
Pavel

commit eba2751794239780efb256ce7079294a4d4c6e74
Author: Pavel 
Date:   Mon Feb 13 21:18:27 2017 +0100



From: Sakari Ailus 

Required added multiplier (and divisor) calculation did not take into
account the existing divisor when checking the values against the
minimum divisor. Do just that.

Signed-off-by: Sakari Ailus 
Signed-off-by: Ivaylo Dimitrov 
Signed-off-by: Pavel Machek 

diff --git a/drivers/media/i2c/smiapp-pll.c b/drivers/media/i2c/smiapp-pll.c
index 771db56..0ada972 100644
--- a/drivers/media/i2c/smiapp-pll.c
+++ b/drivers/media/i2c/smiapp-pll.c
@@ -227,7 +227,8 @@ static int __smiapp_pll_calculate(
 
more_mul_factor = lcm(div, pll->pre_pll_clk_div) / div;
dev_dbg(dev, "more_mul_factor: %u\n", more_mul_factor);
-   more_mul_factor = lcm(more_mul_factor, op_limits->min_sys_clk_div);
+   more_mul_factor = lcm(more_mul_factor,
+ DIV_ROUND_UP(op_limits->min_sys_clk_div, div));
dev_dbg(dev, "more_mul_factor: min_op_sys_clk_div: %d\n",
more_mul_factor);
i = roundup(more_mul_min, more_mul_factor);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

[PATCH] omap3isp: add CSI1 support

[Pavel Machek]

omap3isp: add CSI1 support

Use proper code path for csi1/ccp2 support.

Signed-off-by: Pavel Machek 

diff --git a/drivers/media/platform/omap3isp/ispccp2.c 
b/drivers/media/platform/omap3isp/ispccp2.c
index 24a9fc5..47210b1 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -1149,6 +1149,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
"Could not get regulator vdds_csib\n");
ccp2->vdds_csib = NULL;
}
+   ccp2->phy = >isp_csiphy2;
} else if (isp->revision == ISP_REVISION_15_0) {
ccp2->phy = >isp_csiphy1;
}
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c 
b/drivers/media/platform/omap3isp/ispcsiphy.c
index 50c0f64..a5ac2b40 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -197,9 +197,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
}
 
if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
-   || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
+   || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
lanes = >bus.ccp2.lanecfg;
-   else
+   phy->num_data_lanes = 1;
+   } else
lanes = >bus.csi2.lanecfg;
 
/* Clock and data lanes verification */
@@ -302,13 +303,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
if (rval < 0)
goto done;
 
-   rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
-   if (rval) {
-   regulator_disable(phy->vdd);
-   goto done;
+   if (phy->isp->revision == ISP_REVISION_15_0) {
+   rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
+   if (rval) {
+   regulator_disable(phy->vdd);
+   goto done;
+   }
+
+   csiphy_power_autoswitch_enable(phy, true);
}
 
-   csiphy_power_autoswitch_enable(phy, true);
phy->phy_in_use = 1;
 
 done:
@@ -326,8 +330,10 @@ void omap3isp_csiphy_release(struct isp_csiphy *phy)
 
csiphy_routing_cfg(phy, buscfg->interface, false,
   buscfg->bus.ccp2.phy_layer);
-   csiphy_power_autoswitch_enable(phy, false);
-   csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_OFF);
+   if (phy->isp->revision == ISP_REVISION_15_0) {
+   csiphy_power_autoswitch_enable(phy, false);
+   csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_OFF);
+   }
regulator_disable(phy->vdd);
phy->phy_in_use = 0;
}



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [patch] propagating controls in libv4l2 was Re: support autofocus / autogain in libv4l2

[Pavel Machek]

Hi!

> > Now. There is autogain support in libv4lconvert, but it expects to use
> > same fd for camera and for the gain... which does not work with
> > subdevs.
> > 
> > Of course, opening subdevs by name like this is not really
> > acceptable. But can you suggest a method that is?
> 
> There are two separate things here:
> 
> 1) Autofoucs for a device that doesn't use subdev API
> 2) libv4l2 support for devices that require MC and subdev API
> 
> for (1), it should use the /dev/videoX device that was opened with
> v4l2_open().
> 
> For (2), libv4l2 should be aware of MC and subdev APIs. Sakari
> once tried to write a libv4l2 plugin for OMAP3, but never finished it.
> A more recent trial were to add a libv4l2 plugin for Exynos.
> Unfortunately, none of those code got merged. Last time I checked,
> the Exynos plugin was almost ready to be merged, but Sakari asked
> some changes on it. The developer that was working on it got job on
> some other company. Last time I heard from him, he was still interested
> on finishing his work, but in the need to setup a test environment
> using his own devices.

First... we should not have hardware-specific code in the
userspace. Hardware abstraction should be kernel's job.

Second... we really should solve "ioctl propagation" in
libv4l2. Because otherwise existing userspace is unusable in MC/subdev
drivers.

> IMHO, the right thing to do with regards to autofocus is to
> implement it via a processing module, assuming that just one
> video device is opened.

Ok, I have done that.

> The hole idea is that a libv4l2 client, running on a N900 device
> would just open a fake /dev/video0. Internally, libv4l2 will
> open whatever video nodes it needs to control the device, exporting
> all hardware capabilities (video formats, controls, resolutions,
> etc) as if it was a normal V4L2 camera, hiding all dirty details
> about MC and subdev APIs from userspace application.
> 
> This way, a normal application, like xawtv, tvtime, camorama,
> zbar, mplayer, vlc, ... will work without any changes.

Well, yes, we'd like to get there eventually. But we are not there at
the moment, and ioctl() propagation is one of the steps.

(I do have support specific for N900, but currently it is in python;
this is enough to make the camera usable.)

Regards,
Pavel


> > diff --git a/lib/libv4l2/libv4l2-priv.h b/lib/libv4l2/libv4l2-priv.h
> > index 343db5e..a6bc48e 100644
> > --- a/lib/libv4l2/libv4l2-priv.h
> > +++ b/lib/libv4l2/libv4l2-priv.h
> > @@ -26,6 +26,7 @@
> >  #include "../libv4lconvert/libv4lsyscall-priv.h"
> >  
> >  #define V4L2_MAX_DEVICES 16
> > +#define V4L2_MAX_SUBDEVS 8
> >  /* Warning when making this larger the frame_queued and frame_mapped 
> > members of
> > the v4l2_dev_info struct can no longer be a bitfield, so the code needs 
> > to
> > be adjusted! */
> > @@ -104,6 +105,7 @@ struct v4l2_dev_info {
> > void *plugin_library;
> > void *dev_ops_priv;
> > const struct libv4l_dev_ops *dev_ops;
> > +int subdev_fds[V4L2_MAX_SUBDEVS];
> >  };
> >  
> >  /* From v4l2-plugin.c */
> > diff --git a/lib/libv4l2/libv4l2.c b/lib/libv4l2/libv4l2.c
> > index 0ba0a88..edc9642 100644
> > --- a/lib/libv4l2/libv4l2.c
> > +++ b/lib/libv4l2/libv4l2.c
> > @@ -1,3 +1,4 @@
> > +/* -*- c-file-style: "linux" -*- */
> >  /*
> >  # (C) 2008 Hans de Goede 
> >  
> > @@ -789,18 +790,25 @@ no_capture:
> >  
> > /* Note we always tell v4lconvert to optimize src fmt selection for
> >our default fps, the only exception is the app explicitly selecting
> > -  a fram erate using the S_PARM ioctl after a S_FMT */
> > +  a frame rate using the S_PARM ioctl after a S_FMT */
> > if (devices[index].convert)
> > v4lconvert_set_fps(devices[index].convert, V4L2_DEFAULT_FPS);
> > v4l2_update_fps(index, );
> >  
> > +   devices[index].subdev_fds[0] = SYS_OPEN("/dev/video_sensor", O_RDWR, 0);
> > +   devices[index].subdev_fds[1] = SYS_OPEN("/dev/video_focus", O_RDWR, 0);
> > +   devices[index].subdev_fds[2] = -1;
> > +
> > +   printf("Sensor: %d, focus: %d\n", devices[index].subdev_fds[0], 
> > +  devices[index].subdev_fds[1]);
> > +
> > V4L2_LOG("open: %d\n", fd);
> >  
> > return fd;
> >  }
> >  
> >  /* Is this an fd for which we are emulating v4l1 ? */
> > -static int v4l2_get_index(int fd)
> > +int v4l2_get_index(int fd)
> >  {
> > int index;
> >  
> > 
> > commit 1d6a9ce121f53e8f2e38549eed597a3c3dea5233
> > Author: Pavel 
> > Date:   Wed Apr 26 12:34:04 2017 +0200
> > 
> > Enable ioctl propagation.
> > 
> > diff --git a/lib/libv4l2/libv4l2.c b/lib/libv4l2/libv4l2.c
> > index edc9642..6dab661 100644
> > --- a/lib/libv4l2/libv4l2.c
> > +++ b/lib/libv4l2/libv4l2.c
> > @@ -1064,6 +1064,23 @@ static int v4l2_s_fmt(int index, struct v4l2_format 
> > *dest_fmt)

Re: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Sakari Ailus]

Hi Tomasz,

On Thu, Jul 13, 2017 at 05:31:33PM +0900, Tomasz Figa wrote:
> On Thu, Jul 13, 2017 at 5:21 PM, Sakari Ailus  wrote:
> >> >> + ret = v4l2_async_notifier_register(>v4l2_dev, 
> >> >> >notifier);
> >> >> + if (ret) {
> >> >> + cio2->notifier.num_subdevs = 0;
> >> >
> >> > No need to assign num_subdevs as 0.
> >> >
> >> > [YZ] _notifier_exit() will call _unregister() if this is not 0.
> >>
> >> You shouldn't call _exit() if _init() failed. I noticed that many
> >> error paths in your code does this. Please fix it.
> >
> > In general most functions that initialise and clean up something are
> > implemented so that the cleanup function can be called without calling the
> > corresponding init function. This eases driver implementation by reducing
> > complexity in error paths that are difficult to implement and test to begin
> > with, so I don't see anything wrong with that, quite the contrary.
> >
> > I.e. in this case you should call v4l2_async_notifier_unregister() without
> > checking the number of async sub-devices.
> >
> > There are exceptions to that though; not all the framework functions behave
> > this way. Of kernel APIs, e.g. kmalloc() and kfree() do this --- you can
> > pass a NULL pointer to kfree() and it does nothing.
> 
> I'd argue that most of the cleanup paths I've seen in the kernel are
> the opposite. If you properly check for errors in your code, it's
> actually very unlikely that you need to call a cleanup function
> without the init function called... That said, I've seen the pattern
> you describe too, so probably either there is no strict rule or it's
> not strictly enforced. (Still, judging by
> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#centralized-exiting-of-functions,
> which mentions "one err bugs" and suggests "to split it up into two
> error labels", the pattern I'm arguing for might be the recommended
> default.)

I don't really see a problem here; the another label in the example is
needed to avoid referencing a NULL pointer. If there's a reason to add a
label, then a label is just added. :-)

You could check foo as well under a single label. I'd say that approach
generally scales better: if you can handle a difference in error handling
locally in error handling code, this is easier to maintain and easier to
get right in complex error handling code (whereas the example is very
simple); spreading that knowledge over much of the function has the
opposite effect: it's easy to e.g. to go to a wrong label and there have
been plenty of such bugs in the past.

I think we're getting to fine details here. :-)

-- 
Kind regards,

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

Re: [patch, libv4l]: add sdlcam example for testing digital still camera functionality

[Pavel Machek]

Hi!

> > Utilities like v4l2-ctl are tied closely to the kernel and are updated 
> > whenever
> > new APIs appear. But yet another viewer?
> > 
> > Mauro, I find that v4l-utils is a bit polluted with non-core utilities.
> > IMHO it should only contain the core libv4l2, core utilities and 
> > driver-specific
> > utilities. I wonder if we should make a media-utils-contrib for all the 
> > non-core
> > stuff.
> > 
> > What is your opinion?
> 
> One of the purposes the v4l-utils repository has is that the distributions
> get these programs included to their v4l-utils package as it's typically
> called. It's debatable whether or how much it should contain device specific
> or otherwise random projects, but having a common location for such programs
> has clear benefits, too.
> 
> Based on how this one looks it is definitely not an end user application (I
> hope I'm not miscategorising it) and as Pavel mentioned, it has been useful
> in testing automatic focus / gain control on N900.

Well, testing camera drivers without userspace support is not really
possible. Yes, you can use mplayer to display the data in "preview"
mode, and yavta to adjust focus/exposure... But that's really
hard/slow and preview quality does not tell you much about quality of
final photo.

So... yes, I'd like to see this in.

Best regards,
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Tomasz Figa]

On Thu, Jul 13, 2017 at 5:21 PM, Sakari Ailus  wrote:
>> >> + ret = v4l2_async_notifier_register(>v4l2_dev, 
>> >> >notifier);
>> >> + if (ret) {
>> >> + cio2->notifier.num_subdevs = 0;
>> >
>> > No need to assign num_subdevs as 0.
>> >
>> > [YZ] _notifier_exit() will call _unregister() if this is not 0.
>>
>> You shouldn't call _exit() if _init() failed. I noticed that many
>> error paths in your code does this. Please fix it.
>
> In general most functions that initialise and clean up something are
> implemented so that the cleanup function can be called without calling the
> corresponding init function. This eases driver implementation by reducing
> complexity in error paths that are difficult to implement and test to begin
> with, so I don't see anything wrong with that, quite the contrary.
>
> I.e. in this case you should call v4l2_async_notifier_unregister() without
> checking the number of async sub-devices.
>
> There are exceptions to that though; not all the framework functions behave
> this way. Of kernel APIs, e.g. kmalloc() and kfree() do this --- you can
> pass a NULL pointer to kfree() and it does nothing.

I'd argue that most of the cleanup paths I've seen in the kernel are
the opposite. If you properly check for errors in your code, it's
actually very unlikely that you need to call a cleanup function
without the init function called... That said, I've seen the pattern
you describe too, so probably either there is no strict rule or it's
not strictly enforced. (Still, judging by
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#centralized-exiting-of-functions,
which mentions "one err bugs" and suggests "to split it up into two
error labels", the pattern I'm arguing for might be the recommended
default.)

Best regards,
Tomasz

Re: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Sakari Ailus]

Hi Tomasz and Yong,

On Thu, Jul 13, 2017 at 01:51:18PM +0900, Tomasz Figa wrote:
> Hi Yong,
> 
> On Thu, Jul 13, 2017 at 8:20 AM, Zhi, Yong  wrote:
> > Hi, Sakari,
> >
> > Thanks for the time spent on code review, acks to all the comments, except 
> > two places:
> >
> >> +/* .complete() is called after all subdevices have been located */
> >> +static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
> >> +{
> >> + struct cio2_device *cio2 = container_of(notifier, struct cio2_device,
> >> + notifier);
> >> + struct sensor_async_subdev *s_asd;
> >> + struct fwnode_handle *fwn_remote, *fwn_endpt, *fwn_remote_endpt;
> >> + struct cio2_queue *q;
> >> + struct fwnode_endpoint remote_endpt;
> >> + unsigned int i, pad;
> >> + int ret;
> >> +
> >> + for (i = 0; i < notifier->num_subdevs; i++) {
> >> + s_asd = container_of(cio2->notifier.subdevs[i],
> >> + struct sensor_async_subdev,
> >> + asd);
> >> +
> >> + fwn_remote = s_asd->asd.match.fwnode.fwnode;
> >> + fwn_endpt = (struct fwnode_handle *)
> >> + s_asd->vfwn_endpt.base.local_fwnode;
> >
> > Why do you need a cast?
> >
> > [YZ] With a cast results in compilation warning:
> 
> (I think you mean "without".)
> 
> >
> > drivers/media/pci/ipu3/ipu3-cio2.c:1298:13: warning: assignment discards 
> > ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> >fwn_endpt = /*(struct fwnode_handle *)*/
> 
> This is a sign that the code is doing something wrong (in this case
> probably trying to write to a const pointer), so casting just silences
> the unfixed error.

Indeed. The right thing to do would be to make fwn_endpt const.

In that case though I don't think that variable is really used for anything
useful; the same goes for remote_endpt and fwn_remote_endpt. This looks
like leftovers from development time.

Could these be removed, or did I miss something?

> 
> >
> >> + ret = v4l2_async_notifier_register(>v4l2_dev, >notifier);
> >> + if (ret) {
> >> + cio2->notifier.num_subdevs = 0;
> >
> > No need to assign num_subdevs as 0.
> >
> > [YZ] _notifier_exit() will call _unregister() if this is not 0.
> 
> You shouldn't call _exit() if _init() failed. I noticed that many
> error paths in your code does this. Please fix it.

In general most functions that initialise and clean up something are
implemented so that the cleanup function can be called without calling the
corresponding init function. This eases driver implementation by reducing
complexity in error paths that are difficult to implement and test to begin
with, so I don't see anything wrong with that, quite the contrary.

I.e. in this case you should call v4l2_async_notifier_unregister() without
checking the number of async sub-devices.

There are exceptions to that though; not all the framework functions behave
this way. Of kernel APIs, e.g. kmalloc() and kfree() do this --- you can
pass a NULL pointer to kfree() and it does nothing.

-- 
Regards,

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

Re: [patch] autogain support for bayer10 format (was Re: [patch] propagating controls in libv4l2)

[Pavel Machek]

Hi!

> Oh, somehow I got confused that this is kernel code :)
> 
> >But I'd say NEON conversion is not neccessary anytime soon. First,
> >this is just trying to get average luminosity. We can easily skip
> >quite a lot of pixels, and still get reasonable answer.
> >
> >Second, omap3isp actually has a hardware block computing statistics
> >for us. We just don't use it for simplicity.
> >
> 
> Right, I forgot about that.
> 
> >(But if you want to play with camera, I'll get you patches; there's
> >ton of work to be done, both kernel and userspace :-).
> 
> Well, I saw a low hanging fruit I thought I can convert to NEON in a day or
> two, while having some rest from the huge "project" I am devoting all my
> spare time recently (rebasing hildon/maemo 5 on top of devuan Jessie).
> Still, if there is something relatively small to be done, just email me and
> I'll have a look.

Well, there's a ton of work on camera, and some work on
libcmtspeechdata. The later is rather self-contained.
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH v2] [media] lirc_zilog: Clean up lirc zilog error codes

[Frans Klaver]

Almost there on the subject. Stuff between brackets is removed by git,
so you should rather use something like

[PATCH v2] staging: lirc: Clean up zilog error codes

On Wed, Jul 12, 2017 at 9:17 PM, Yves Lemée  wrote:
> According the coding style guidelines, the ENOSYS error code must be returned
> in case of a non existent system call. This code has been replaced with
> the ENOTTY error code indicating a missing functionality.
>
> v2: Improved punctuation
> Fixed patch subject

This change info goes below the --- line and just above the diffstat.


> Signed-off-by: Yves Lemée 
> ---
>  drivers/staging/media/lirc/lirc_zilog.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
> b/drivers/staging/media/lirc/lirc_zilog.c
> index 015e41bd036e..26dd32d5b5b2 100644
> --- a/drivers/staging/media/lirc/lirc_zilog.c
> +++ b/drivers/staging/media/lirc/lirc_zilog.c
> @@ -1249,7 +1249,7 @@ static long ioctl(struct file *filep, unsigned int cmd, 
> unsigned long arg)
> break;
> case LIRC_GET_REC_MODE:
> if (!(features & LIRC_CAN_REC_MASK))
> -   return -ENOSYS;
> +   return -ENOTTY;
>
> result = put_user(LIRC_REC2MODE
>   (features & LIRC_CAN_REC_MASK),
> @@ -1257,21 +1257,21 @@ static long ioctl(struct file *filep, unsigned int 
> cmd, unsigned long arg)
> break;
> case LIRC_SET_REC_MODE:
> if (!(features & LIRC_CAN_REC_MASK))
> -   return -ENOSYS;
> +   return -ENOTTY;
>
> result = get_user(mode, uptr);
> if (!result && !(LIRC_MODE2REC(mode) & features))
> -   result = -EINVAL;
> +   result = -ENOTTY;
> break;
> case LIRC_GET_SEND_MODE:
> if (!(features & LIRC_CAN_SEND_MASK))
> -   return -ENOSYS;
> +   return -ENOTTY;
>
> result = put_user(LIRC_MODE_PULSE, uptr);
> break;
> case LIRC_SET_SEND_MODE:
> if (!(features & LIRC_CAN_SEND_MASK))
> -   return -ENOSYS;
> +   return -ENOTTY;
>
> result = get_user(mode, uptr);
> if (!result && mode != LIRC_MODE_PULSE)
> --
> 2.13.2
>
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH V2 4/9] [media] dvb-core/dvb_ca_en50221.c: Fixed block comments

[Jasmin J.]

Hello Antti!

> actually it reports, when run --strict mode
I checked this once, but I thought this would be too aggressive changes.

>> * many cases where if (ret != 0), which generally should be written as if
>> (ret). If you expect it is just error ret value, then prefer if (ret), but
>> if> ret has some other meaning like it returns number of bytes then if you
>> expect 0-bytes returned (ret != 0) is also valid.
In fact I did no real code changes to keep the impact as little as possible.
But I agree fully with you and in my drivers I used always (ret) or (!ret).
Although this has been changed in my new company when it comes to certified
software ... .

I will try also to compile with GCC 7.1.1, if I get one for my system.

>> * unnecessary looking line split like that:
>> if (a
>>& b)
I am sure I did this because of the 80 col limit, but I will look again.

THX for your review and the valuable input. I will add you the receiver list
next time.

BR,
   Jasmin

[PATCH] cec: move cec_register_cec_notifier to cec-notifier.h

[Hans Verkuil]

The cec_register_cec_notifier function was in media/cec.h, but it
has to be in cec-notifier.h.

While we are at it, also document it and add a stub function for when
the notifier is disabled or the CEC core code is unreachable.

Based on an earlier patch from Jose Abreu .

Signed-off-by: Hans Verkuil 
---
 include/media/cec-notifier.h | 12 
 include/media/cec.h  |  5 -
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/media/cec-notifier.h b/include/media/cec-notifier.h
index 298f996969df..73bc98b90afc 100644
--- a/include/media/cec-notifier.h
+++ b/include/media/cec-notifier.h
@@ -86,6 +86,14 @@ void cec_notifier_register(struct cec_notifier *n,
  */
 void cec_notifier_unregister(struct cec_notifier *n);

+/**
+ * cec_register_cec_notifier - register the notifier with the cec adapter.
+ * @adap: the CEC adapter
+ * @notifier: the CEC notifier
+ */
+void cec_register_cec_notifier(struct cec_adapter *adap,
+  struct cec_notifier *notifier);
+
 #else
 static inline struct cec_notifier *cec_notifier_get(struct device *dev)
 {
@@ -116,6 +124,10 @@ static inline void cec_notifier_unregister(struct 
cec_notifier *n)
 {
 }

+static inline void cec_register_cec_notifier(struct cec_adapter *adap,
+struct cec_notifier *notifier)
+{
+}
 #endif

 #endif
diff --git a/include/media/cec.h b/include/media/cec.h
index 56643b27e4b8..6a1c2515bb91 100644
--- a/include/media/cec.h
+++ b/include/media/cec.h
@@ -311,11 +311,6 @@ u16 cec_phys_addr_for_input(u16 phys_addr, u8 input);
  */
 int cec_phys_addr_validate(u16 phys_addr, u16 *parent, u16 *port);

-#ifdef CONFIG_CEC_NOTIFIER
-void cec_register_cec_notifier(struct cec_adapter *adap,
-  struct cec_notifier *notifier);
-#endif
-
 #else

 static inline int cec_register_adapter(struct cec_adapter *adap,
-- 
2.11.0

Re: [PATCH v8 1/5] [media] cec.h: Add stub function for cec_register_cec_notifier()

[Hans Verkuil]

On 10/07/17 17:46, Jose Abreu wrote:
> Add a new stub function for cec_register_cec_notifier() so that
> we can still call this function when CONFIG_CEC_NOTIFIER and
> CONFIG_CEC_CORE are not set.
> 
> Signed-off-by: Jose Abreu 
> Cc: Carlos Palminha 
> Cc: Hans Verkuil 
> ---
>  include/media/cec.h | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/media/cec.h b/include/media/cec.h
> index 56643b2..8357f60 100644
> --- a/include/media/cec.h
> +++ b/include/media/cec.h
> @@ -365,6 +365,14 @@ static inline int cec_phys_addr_validate(u16 phys_addr, 
> u16 *parent, u16 *port)
>   return 0;
>  }
>  
> +#ifndef CONFIG_CEC_NOTIFIER
> +struct cec_notifier;
> +static inline void cec_register_cec_notifier(struct cec_adapter *adap,
> +  struct cec_notifier *notifier)
> +{
> +}
> +#endif
> +
>  #endif
>  
>  /**
> 

This isn't quite right. This function prototype needs to be moved to 
cec-notifier.h.

I also saw that it isn't documented. I'll make a patch for this which will also 
include
documentation.

Regards,

Hans

[PATCH v2 2/2] staging: atomisp2: hmm: Alignment code (rebased)

[Philipp Guendisch]

This patch fixed code alignment to open paranthesis.
Semantic should not be affected by this patch.

It has been rebased on top of media_tree atomisp branch

Signed-off-by: Philipp Guendisch 
Signed-off-by: Chris Baller 
---
 .../staging/media/atomisp/pci/atomisp2/hmm/hmm.c   | 93 +++---
 1 file changed, 45 insertions(+), 48 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c 
b/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c
index b345025..b8aae4b 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c
@@ -55,7 +55,7 @@ struct _hmm_mem_stat hmm_mem_stat;
 static const char hmm_bo_type_string[] = "psui";
 
 static ssize_t bo_show(struct device *dev, struct device_attribute *attr,
-   char *buf, struct list_head *bo_list, bool active)
+  char *buf, struct list_head *bo_list, bool active)
 {
ssize_t ret = 0;
struct hmm_buffer_object *bo;
@@ -75,10 +75,10 @@ static ssize_t bo_show(struct device *dev, struct 
device_attribute *attr,
spin_lock_irqsave(_device.list_lock, flags);
list_for_each_entry(bo, bo_list, list) {
if ((active && (bo->status & HMM_BO_ALLOCED)) ||
-   (!active && !(bo->status & HMM_BO_ALLOCED))) {
+   (!active && !(bo->status & HMM_BO_ALLOCED))) {
ret = scnprintf(buf + index1, PAGE_SIZE - index1,
-   "%c %d\n",
-   hmm_bo_type_string[bo->type], bo->pgnr);
+   "%c %d\n",
+   hmm_bo_type_string[bo->type], bo->pgnr);
 
total[bo->type] += bo->pgnr;
count[bo->type]++;
@@ -91,9 +91,10 @@ static ssize_t bo_show(struct device *dev, struct 
device_attribute *attr,
for (i = 0; i < HMM_BO_LAST; i++) {
if (count[i]) {
ret = scnprintf(buf + index1 + index2,
-   PAGE_SIZE - index1 - index2,
-   "%ld %c buffer objects: %ld KB\n",
-   count[i], hmm_bo_type_string[i], total[i] * 4);
+   PAGE_SIZE - index1 - index2,
+   "%ld %c buffer objects: %ld KB\n",
+   count[i], hmm_bo_type_string[i],
+   total[i] * 4);
if (ret > 0)
index2 += ret;
}
@@ -103,23 +104,21 @@ static ssize_t bo_show(struct device *dev, struct 
device_attribute *attr,
return index1 + index2 + 1;
 }
 
-static ssize_t active_bo_show(struct device *dev,
-   struct device_attribute *attr,
-   char *buf)
+static ssize_t active_bo_show(struct device *dev, struct device_attribute 
*attr,
+ char *buf)
 {
return bo_show(dev, attr, buf, _device.entire_bo_list, true);
 }
 
-static ssize_t free_bo_show(struct device *dev,
-   struct device_attribute *attr,
-   char *buf)
+static ssize_t free_bo_show(struct device *dev, struct device_attribute *attr,
+   char *buf)
 {
return bo_show(dev, attr, buf, _device.entire_bo_list, false);
 }
 
 static ssize_t reserved_pool_show(struct device *dev,
-   struct device_attribute *attr,
-   char *buf)
+ struct device_attribute *attr,
+ char *buf)
 {
ssize_t ret = 0;
 
@@ -131,7 +130,7 @@ static ssize_t reserved_pool_show(struct device *dev,
 
spin_lock_irqsave(>list_lock, flags);
ret = scnprintf(buf, PAGE_SIZE, "%d out of %d pages available\n",
-   pinfo->index, pinfo->pgnr);
+   pinfo->index, pinfo->pgnr);
spin_unlock_irqrestore(>list_lock, flags);
 
if (ret > 0)
@@ -141,8 +140,8 @@ static ssize_t reserved_pool_show(struct device *dev,
 };
 
 static ssize_t dynamic_pool_show(struct device *dev,
-   struct device_attribute *attr,
-   char *buf)
+struct device_attribute *attr,
+char *buf)
 {
ssize_t ret = 0;
 
@@ -154,7 +153,7 @@ static ssize_t dynamic_pool_show(struct device *dev,
 
spin_lock_irqsave(>list_lock, flags);
ret = scnprintf(buf, PAGE_SIZE, "%d (max %d) pages available\n",
-   pinfo->pgnr, pinfo->pool_size);
+   pinfo->pgnr, pinfo->pool_size);
spin_unlock_irqrestore(>list_lock, flags);
 
if (ret > 0)
@@ -202,7 +201,7 @@ int hmm_init(void)
 
if (!ret) {
ret = 

[PATCH v2 1/2] staging: atomisp2: hmm: Fixed comment style

[Philipp Guendisch]

This patch fixed comment style. Semantic should not be affected.
There are also two warnings left about too long lines, which
reduce readability if changed.

Signed-off-by: Philipp Guendisch 
Signed-off-by: Chris Baller 
---
 .../staging/media/atomisp/pci/atomisp2/hmm/hmm.c   | 44 +++---
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c 
b/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c
index de0426b..b345025 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c
@@ -46,10 +46,12 @@ static ia_css_ptr dummy_ptr;
 static bool hmm_initialized;
 struct _hmm_mem_stat hmm_mem_stat;
 
-/* p: private
-   s: shared
-   u: user
-   i: ion */
+/*
+ * p: private
+ * s: shared
+ * u: user
+ * i: ion
+ */
 static const char hmm_bo_type_string[] = "psui";
 
 static ssize_t bo_show(struct device *dev, struct device_attribute *attr,
@@ -213,9 +215,7 @@ void hmm_cleanup(void)
 {
sysfs_remove_group(_dev->kobj, atomisp_attribute_group);
 
-   /*
-* free dummy memory first
-*/
+   /* free dummy memory first */
hmm_free(dummy_ptr);
dummy_ptr = 0;
 
@@ -230,22 +230,24 @@ ia_css_ptr hmm_alloc(size_t bytes, enum hmm_bo_type type,
struct hmm_buffer_object *bo;
int ret;
 
-   /* Check if we are initialized. In the ideal world we wouldn't need
-  this but we can tackle it once the driver is a lot cleaner */
+   /*
+* Check if we are initialized. In the ideal world we wouldn't need
+* this but we can tackle it once the driver is a lot cleaner
+*/
 
if (!hmm_initialized)
hmm_init();
-   /*Get page number from size*/
+   /* Get page number from size */
pgnr = size_to_pgnr_ceil(bytes);
 
-   /*Buffer object structure init*/
+   /* Buffer object structure init */
bo = hmm_bo_alloc(_device, pgnr);
if (!bo) {
dev_err(atomisp_dev, "hmm_bo_create failed.\n");
goto create_bo_err;
}
 
-   /*Allocate pages for memory*/
+   /* Allocate pages for memory */
ret = hmm_bo_alloc_pages(bo, type, from_highmem, userptr, cached);
if (ret) {
dev_err(atomisp_dev,
@@ -253,7 +255,7 @@ ia_css_ptr hmm_alloc(size_t bytes, enum hmm_bo_type type,
goto alloc_page_err;
}
 
-   /*Combind the virtual address and pages togather*/
+   /* Combind the virtual address and pages togather */
ret = hmm_bo_bind(bo);
if (ret) {
dev_err(atomisp_dev, "hmm_bo_bind failed.\n");
@@ -319,7 +321,7 @@ static inline int hmm_check_bo(struct hmm_buffer_object 
*bo, unsigned int ptr)
return 0;
 }
 
-/*Read function in ISP memory management*/
+/* Read function in ISP memory management */
 static int load_and_flush_by_kmap(ia_css_ptr virt, void *data, unsigned int 
bytes)
 {
struct hmm_buffer_object *bo;
@@ -362,7 +364,7 @@ static int load_and_flush_by_kmap(ia_css_ptr virt, void 
*data, unsigned int byte
return 0;
 }
 
-/*Read function in ISP memory management*/
+/* Read function in ISP memory management */
 static int load_and_flush(ia_css_ptr virt, void *data, unsigned int bytes)
 {
struct hmm_buffer_object *bo;
@@ -397,7 +399,7 @@ static int load_and_flush(ia_css_ptr virt, void *data, 
unsigned int bytes)
return 0;
 }
 
-/*Read function in ISP memory management*/
+/* Read function in ISP memory management */
 int hmm_load(ia_css_ptr virt, void *data, unsigned int bytes)
 {
if (!data) {
@@ -408,13 +410,13 @@ int hmm_load(ia_css_ptr virt, void *data, unsigned int 
bytes)
return load_and_flush(virt, data, bytes);
 }
 
-/*Flush hmm data from the data cache*/
+/* Flush hmm data from the data cache */
 int hmm_flush(ia_css_ptr virt, unsigned int bytes)
 {
return load_and_flush(virt, NULL, bytes);
 }
 
-/*Write function in ISP memory management*/
+/* Write function in ISP memory management */
 int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes)
 {
struct hmm_buffer_object *bo;
@@ -496,7 +498,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned 
int bytes)
return 0;
 }
 
-/*memset function in ISP memory management*/
+/* memset function in ISP memory management */
 int hmm_set(ia_css_ptr virt, int c, unsigned int bytes)
 {
struct hmm_buffer_object *bo;
@@ -556,7 +558,7 @@ int hmm_set(ia_css_ptr virt, int c, unsigned int bytes)
return 0;
 }
 
-/*Virtual address to physical address convert*/
+/* Virtual address to physical address convert */
 phys_addr_t hmm_virt_to_phys(ia_css_ptr virt)
 {
unsigned int idx, offset;
@@ -591,7 +593,7 @@ int hmm_mmap(struct vm_area_struct *vma, ia_css_ptr virt)
return hmm_bo_mmap(vma, bo);
 }
 
-/*Map ISP virtual address 

Re: [PATCH v7 2/3] media: i2c: adv748x: add adv748x driver

[Niklas Söderlund]

Hi Kieran,

Thanks for your hard work.

On 2017-07-06 12:01:16 +0100, Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> Provide support for the ADV7481 and ADV7482.
> 
> The driver is modelled with 4 subdevices to allow simultaneous streaming
> from the AFE (Analog front end) and HDMI inputs though two CSI TX
> entities.
> 
> The HDMI entity is linked to the TXA CSI bus, whilst the AFE is linked
> to the TXB CSI bus.
> 
> The driver is based on a prototype by Koji Matsuoka in the Renesas BSP,
> and an earlier rework by Niklas Söderlund.
> 
> Signed-off-by: Kieran Bingham 

Unfortunately I have hit a regression with v7. I use the latest R-Car 
Gen3 VIN and CSI-2 patches in conjunction with v6 and v7 of this series.
And wile running v4l2-compliance on a VIN which is connected to the CVBS 
input it fails, see output at the end.

If i substitute the adv748x driver patch with v6 version v4l2-compliance 
works as expected. It also works as expected for the HDMI inputs using 
both v6 and v7 of this series. I have tried using both a PAL and NTSC 
source so I don't think that the removal of auto detection by the 
adv748x driver itself in v7 is to blame, but I have not verified this.

To ease replication let me point out the compliance.sh script in 
vin-tests which will replicate the MC setup before running the 
v4l2-compliance.

# v4l2-compliance -s -d /dev/video26
v4l2-compliance SHA   : [  359.935042] rcar-vin e6ef1000.video: 
=  START STATUS  =
d16a17abd1d8d788[  359.944098] rcar-vin e6ef1000.video: ==  END 
STATUS  ==
5ca2f44fb295035278baa89c

Driver Info:
Driver name   : rcar_vin
Card type : R_Car_VIN
Bus info  : platform:e6ef1000.video
Driver version: 4.12.0
Capabilities  : 0x8521
Video Capture
Read/Write
Streaming
Extended Pix Format
Device Capabilities
Device Caps   : 0x0521
Video Capture
Read/Write
Streaming
Extended Pix Format

Compliance test for device /dev/video26 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK
test for unlimited opens: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
test VIDIOC_LOG_STATUS: OK

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)

Test input 0:

Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
test VIDIOC_QUERYCTRL: OK (Not Supported)
test VIDIOC_G/S_CTRL: OK (Not Supported)
test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 0 Private Controls: 0

Format ioctls:
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK (Not Supported)
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
test Cropping: OK (Not Supported)
test Composing: OK (Not Supported)
test Scaling: OK

Codec ioctls:
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
test VIDIOC_G_ENC_INDEX: OK (Not Supported)
test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK

Test input 0:

Streaming ioctls:
test read/write: OK