Re: [PATCH 0/2 v6] uvcvideo: asynchronous controls

[Guennadi Liakhovetski]

Hi Laurent,

Any update on this?

Thanks
Guennadi

On Wed, 13 Dec 2017, Guennadi Liakhovetski wrote:

> This is an update of the two patches, adding asynchronous control
> support to the uvcvideo driver. If a control is sent, while the camera
> is still processing an earlier control, it will generate a protocol
> STALL condition on the control pipe.
> 
> Thanks
> Guennadi
> 
> Guennadi Liakhovetski (2):
>   uvcvideo: send a control event when a Control Change interrupt arrives
>   uvcvideo: handle control pipe protocol STALLs
> 
>  drivers/media/usb/uvc/uvc_ctrl.c   | 166 
> +
>  drivers/media/usb/uvc/uvc_status.c | 111 ++---
>  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
>  drivers/media/usb/uvc/uvc_video.c  |  59 +++--
>  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
>  include/uapi/linux/uvcvideo.h  |   2 +
>  6 files changed, 322 insertions(+), 35 deletions(-)
> 
> -- 
> 1.9.3
> 

Re: Please help test the new v4l-subdev support in v4l2-compliance

[Hans Verkuil]

On 02/06/2018 08:16 AM, Tim Harvey wrote:
> On Sat, Feb 3, 2018 at 7:56 AM, Hans Verkuil  wrote:
>> Hi Tim, Jacopo,
>>
>> I have now finished writing the v4l2-compliance tests for the various 
>> v4l-subdev
>> ioctls. I managed to test some with the vimc driver, but that doesn't 
>> implement all
>> ioctls, so I could use some help testing my test code :-)
>>
>> To test you first need to apply these patches to your kernel:
>>
>> https://patchwork.linuxtv.org/patch/46817/
>> https://patchwork.linuxtv.org/patch/46822/
>>
>> Otherwise the compliance test will fail a lot.
>>
>> Now run v4l2-compliance -u /dev/v4l-subdevX (or -uX as a shortcut) and see 
>> what
>> happens.
>>
> 
> Hans,
> 
> Here's the compliance results for my tda1997x driver:
> 
> v4l2-compliance SHA   : b2f8f9049056eb6f9e028927dacb2c715a062df8
> Media Driver Info:
> Driver name  : imx-media
> Model: imx-media
> Serial   :
> Bus info :
> Media version: 4.15.0
> Hardware revision: 0x (0)
> Driver version   : 4.15.0
> Interface Info:
> ID   : 0x038f
> Type : V4L Sub-Device
> Entity Info:
> ID   : 0x0003 (3)
> Name : tda19971 2-0048
> Function : Unknown
> Pad 0x0104   : Source
>   Link 0x026f: to remote pad 0x163 of entity
> 'ipu1_csi0_mux': Data, Enabled
> 
> Compliance test for device /dev/v4l-subdev1:
> 
> Allow for multiple opens:
> test second /dev/v4l-subdev1 open: OK
> test for unlimited opens: OK
> 
> Debug ioctls:
> 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 (Not Supported)
> test VIDIOC_G/S_AUDIO: OK (Not Supported)
> Inputs: 0 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
> fail: v4l2-test-io-config.cpp(375): doioctl(node,
> VIDIOC_DV_TIMINGS_CAP, ) != EINVAL
> fail: v4l2-test-io-config.cpp(392): EDID check failed
> for source pad 0.
> test VIDIOC_DV_TIMINGS_CAP: FAIL
> fail: v4l2-test-io-config.cpp(454): ret && ret != EINVAL
> fail: v4l2-test-io-config.cpp(533): EDID check failed
> for source pad 0.
> test VIDIOC_G/S_EDID: FAIL
> 
> Sub-Device ioctls (Source Pad 0):
> test Try VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK
> fail: v4l2-test-subdevs.cpp(303): fmt.code == 0 ||
> fmt.code == ~0U
> fail: v4l2-test-subdevs.cpp(342):
> checkMBusFrameFmt(node, fmt.format)
> test Try VIDIOC_SUBDEV_G/S_FMT: FAIL
> test Try VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported)
> test Active VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK
> test Active VIDIOC_SUBDEV_G/S_FMT: OK
> test Active VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported)
> test VIDIOC_SUBDEV_G/S_FRAME_INTERVAL: OK (Not Supported)
> 
> 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 (Not Supported)
> root@ventana:~# cat foo
> v4l2-compliance SHA   : b2f8f9049056eb6f9e028927dacb2c715a062df8
> Media Driver Info:
> Driver name  : imx-media
> Model: imx-media
> Serial   :
> Bus info :
> Media version: 4.15.0
> Hardware revision: 0x (0)
> Driver version   : 4.15.0
> Interface Info:
> ID   : 0x038f
> Type : V4L Sub-Device
> Entity Info:
> ID   : 0x0003 (3)
> Name : tda19971 2-0048
> Function : Unknown
> Pad 0x0104   : Source
>   Link 0x026f: to remote pad 0x163 of entity
> 'ipu1_csi0_mux': 

Re: Please help test the new v4l-subdev support in v4l2-compliance

[Tim Harvey]

On Sat, Feb 3, 2018 at 7:56 AM, Hans Verkuil  wrote:
> Hi Tim, Jacopo,
>
> I have now finished writing the v4l2-compliance tests for the various 
> v4l-subdev
> ioctls. I managed to test some with the vimc driver, but that doesn't 
> implement all
> ioctls, so I could use some help testing my test code :-)
>
> To test you first need to apply these patches to your kernel:
>
> https://patchwork.linuxtv.org/patch/46817/
> https://patchwork.linuxtv.org/patch/46822/
>
> Otherwise the compliance test will fail a lot.
>
> Now run v4l2-compliance -u /dev/v4l-subdevX (or -uX as a shortcut) and see 
> what
> happens.
>

Hans,

Here's the compliance results for my tda1997x driver:

v4l2-compliance SHA   : b2f8f9049056eb6f9e028927dacb2c715a062df8
Media Driver Info:
Driver name  : imx-media
Model: imx-media
Serial   :
Bus info :
Media version: 4.15.0
Hardware revision: 0x (0)
Driver version   : 4.15.0
Interface Info:
ID   : 0x038f
Type : V4L Sub-Device
Entity Info:
ID   : 0x0003 (3)
Name : tda19971 2-0048
Function : Unknown
Pad 0x0104   : Source
  Link 0x026f: to remote pad 0x163 of entity
'ipu1_csi0_mux': Data, Enabled

Compliance test for device /dev/v4l-subdev1:

Allow for multiple opens:
test second /dev/v4l-subdev1 open: OK
test for unlimited opens: OK

Debug ioctls:
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 (Not Supported)
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 0 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
fail: v4l2-test-io-config.cpp(375): doioctl(node,
VIDIOC_DV_TIMINGS_CAP, ) != EINVAL
fail: v4l2-test-io-config.cpp(392): EDID check failed
for source pad 0.
test VIDIOC_DV_TIMINGS_CAP: FAIL
fail: v4l2-test-io-config.cpp(454): ret && ret != EINVAL
fail: v4l2-test-io-config.cpp(533): EDID check failed
for source pad 0.
test VIDIOC_G/S_EDID: FAIL

Sub-Device ioctls (Source Pad 0):
test Try VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK
fail: v4l2-test-subdevs.cpp(303): fmt.code == 0 ||
fmt.code == ~0U
fail: v4l2-test-subdevs.cpp(342):
checkMBusFrameFmt(node, fmt.format)
test Try VIDIOC_SUBDEV_G/S_FMT: FAIL
test Try VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported)
test Active VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK
test Active VIDIOC_SUBDEV_G/S_FMT: OK
test Active VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK (Not Supported)
test VIDIOC_SUBDEV_G/S_FRAME_INTERVAL: OK (Not Supported)

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 (Not Supported)
root@ventana:~# cat foo
v4l2-compliance SHA   : b2f8f9049056eb6f9e028927dacb2c715a062df8
Media Driver Info:
Driver name  : imx-media
Model: imx-media
Serial   :
Bus info :
Media version: 4.15.0
Hardware revision: 0x (0)
Driver version   : 4.15.0
Interface Info:
ID   : 0x038f
Type : V4L Sub-Device
Entity Info:
ID   : 0x0003 (3)
Name : tda19971 2-0048
Function : Unknown
Pad 0x0104   : Source
  Link 0x026f: to remote pad 0x163 of entity
'ipu1_csi0_mux': Data, Enabled

Compliance test for device /dev/v4l-subdev1:

Allow for multiple opens:
test second /dev/v4l-subdev1 open: OK
test for unlimited opens: OK

Debug ioctls:
test VIDIOC_LOG_STATUS: OK

Input ioctls:
test 

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:   Tue Feb  6 05:00:24 CET 2018
media-tree git hash:273caa260035c03d89ad63d72d8cd3d9e5c5e3f1
media_build git hash:   391263966e7729af299e257e9c5c0ff51aec32f3
v4l-utils git hash: 9c3669241762a128cd896e8799aae210fc8b7214
gcc version:i686-linux-gcc (GCC) 7.3.0
sparse version: v0.5.0-3994-g45eb2282
smatch version: v0.5.0-3994-g45eb2282
host hardware:  x86_64
host os:4.14.0-364

linux-git-arm-at91: OK
linux-git-arm-davinci: OK
linux-git-arm-multi: OK
linux-git-arm-pxa: OK
linux-git-arm-stm32: OK
linux-git-arm64: OK
linux-git-blackfin-bf561: OK
linux-git-i686: OK
linux-git-m32r: OK
linux-git-mips: OK
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: OK
linux-2.6.36.4-i686: ERRORS
linux-2.6.37.6-i686: ERRORS
linux-2.6.38.8-i686: ERRORS
linux-2.6.39.4-i686: ERRORS
linux-3.0.60-i686: ERRORS
linux-3.1.10-i686: ERRORS
linux-3.2.37-i686: ERRORS
linux-3.3.8-i686: ERRORS
linux-3.4.27-i686: ERRORS
linux-3.5.7-i686: ERRORS
linux-3.6.11-i686: ERRORS
linux-3.7.4-i686: ERRORS
linux-3.8-i686: ERRORS
linux-3.9.2-i686: ERRORS
linux-3.10.1-i686: ERRORS
linux-3.11.1-i686: ERRORS
linux-3.12.67-i686: ERRORS
linux-3.13.11-i686: ERRORS
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: ERRORS
linux-4.0.9-i686: ERRORS
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: ERRORS
linux-4.7.5-i686: ERRORS
linux-4.8-i686: ERRORS
linux-4.9.26-i686: WARNINGS
linux-4.10.14-i686: WARNINGS
linux-4.11-i686: WARNINGS
linux-4.12.1-i686: WARNINGS
linux-4.13-i686: WARNINGS
linux-4.14-i686: WARNINGS
linux-4.15-i686: ERRORS
linux-2.6.36.4-x86_64: ERRORS
linux-2.6.37.6-x86_64: ERRORS
linux-2.6.38.8-x86_64: ERRORS
linux-2.6.39.4-x86_64: ERRORS
linux-3.0.60-x86_64: ERRORS
linux-3.1.10-x86_64: ERRORS
linux-3.2.37-x86_64: ERRORS
linux-3.3.8-x86_64: ERRORS
linux-3.4.27-x86_64: ERRORS
linux-3.5.7-x86_64: ERRORS
linux-3.6.11-x86_64: ERRORS
linux-3.7.4-x86_64: ERRORS
linux-3.8-x86_64: ERRORS
linux-3.9.2-x86_64: ERRORS
linux-3.10.1-x86_64: ERRORS
linux-3.11.1-x86_64: ERRORS
linux-3.12.67-x86_64: ERRORS
linux-3.13.11-x86_64: ERRORS
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: ERRORS
linux-3.18.7-x86_64: ERRORS
linux-3.19-x86_64: ERRORS
linux-4.0.9-x86_64: ERRORS
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: ERRORS
linux-4.7.5-x86_64: ERRORS
linux-4.8-x86_64: ERRORS
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
linux-4.13-x86_64: WARNINGS
linux-4.14-x86_64: WARNINGS
linux-4.15-x86_64: ERRORS
apps: WARNINGS
spec-git: OK
smatch: OK

Detailed results are available here:

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

Full logs are available here:

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

The Media Infrastructure API from this daily build is here:

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

[PATCH] media: intel-ipu3: cio2: Use SPDX license headers

[Yong Zhi]

Adopt SPDX license headers and update year to 2018.

Signed-off-by: Yong Zhi 
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 12 ++--
 drivers/media/pci/intel/ipu3/ipu3-cio2.h | 14 ++
 2 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c 
b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 6cb..725973f 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -1,14 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
- * 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.
+ * Copyright (C) 2018 Intel Corporation
  *
  * Based partially on Intel IPU4 driver written by
  *  Sakari Ailus 
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h 
b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
index 78a5799..6a11051 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
@@ -1,15 +1,5 @@
-/*
- * 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.
- */
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2018 Intel Corporation */
 
 #ifndef __IPU3_CIO2_H
 #define __IPU3_CIO2_H
-- 
1.9.1

Re: [PATCH v2 8/8] platform: vivid-cec: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Hi Hans,

Quoting Hans Verkuil :


On 02/05/2018 09:36 PM, Gustavo A. R. Silva wrote:

Add suffix ULL to constant 10 in order to give the compiler complete
information about the proper arithmetic to use. Notice that this
constant is used in a context that expects an expression of type
u64 (64 bits, unsigned).

The expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is currently being
evaluated using 32-bit arithmetic.

Also, remove unnecessary parentheses and add a code comment to make it
clear what is the reason of the code change.

Addresses-Coverity-ID: 1454996
Signed-off-by: Gustavo A. R. Silva 
---
Changes in v2:
 - Update subject and changelog to better reflect the proposed code changes.
 - Add suffix ULL to constant instead of casting a variable.
 - Remove unncessary parentheses.


unncessary -> unnecessary



Thanks for this.


 - Add code comment.

 drivers/media/platform/vivid/vivid-cec.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vivid/vivid-cec.c  
b/drivers/media/platform/vivid/vivid-cec.c

index b55d278..614787b 100644
--- a/drivers/media/platform/vivid/vivid-cec.c
+++ b/drivers/media/platform/vivid/vivid-cec.c
@@ -82,8 +82,15 @@ static void vivid_cec_pin_adap_events(struct  
cec_adapter *adap, ktime_t ts,


if (adap == NULL)
return;
-   ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL +
-  len * 10 * CEC_TIM_DATA_BIT_TOTAL));
+
+   /*
+* Suffix ULL on constant 10 makes the expression
+* CEC_TIM_START_BIT_TOTAL + 10ULL * len * CEC_TIM_DATA_BIT_TOTAL
+* be evaluated using 64-bit unsigned arithmetic (u64), which
+* is what ktime_sub_us expects as second argument.
+*/


That's not really the comment that I was looking for. It still doesn't
explain *why* this is needed at all. How about something like this:



In MHO the reason for the change is simply the discrepancy between the  
arithmetic expected by
the function ktime_sub_us and the arithmetic in which the expression  
is being evaluated. And this

has nothing to do with any particular tool.


/*
 * Add the ULL suffix to the constant 10 to work around a false Coverity
 * "Unintentional integer overflow" warning. Coverity isn't smart enough
 * to understand that len is always <= 16, so there is no chance of an
 * integer overflow.
 */



:P

In my opinion it is not a good idea to tie the code to a particular tool.
There are only three appearances of the word 'Coverity' in the whole  
code base, and, honestly I don't want to add more.


So I think I will document this issue as a FP in the Coverity platform.

Thanks!
--
Gustavo


Regards,

Hans


+   ts = ktime_sub_us(ts, CEC_TIM_START_BIT_TOTAL +
+  10ULL * len * CEC_TIM_DATA_BIT_TOTAL);
cec_queue_pin_cec_event(adap, false, ts);
ts = ktime_add_us(ts, CEC_TIM_START_BIT_LOW);
cec_queue_pin_cec_event(adap, true, ts);

Re: media_device.c question: can this workaround be removed?

[Sakari Ailus]

Hi Mauro and Hans,

On Mon, Feb 05, 2018 at 02:32:28PM -0200, Mauro Carvalho Chehab wrote:
> Em Mon, 5 Feb 2018 16:19:28 +0100
> Hans Verkuil  escreveu:
> 
> > On 02/05/2018 03:30 PM, Sakari Ailus wrote:
> > > Hi Hans,
> > > 
> > > On Mon, Feb 05, 2018 at 01:30:04PM +0100, Hans Verkuil wrote:  
> > >> On 02/05/2018 12:59 PM, Sakari Ailus wrote:  
> > >>> Hi Hans,
> > >>>
> > >>> On Mon, Feb 05, 2018 at 11:26:47AM +0100, Hans Verkuil wrote:  
> >  The function media_device_enum_entities() has this workaround:
> > 
> >  /*
> >   * Workaround for a bug at media-ctl <= v1.10 that makes it to
> >   * do the wrong thing if the entity function doesn't belong to
> >   * either MEDIA_ENT_F_OLD_BASE or MEDIA_ENT_F_OLD_SUBDEV_BASE
> >   * Ranges.
> >   *
> >   * Non-subdevices are expected to be at the 
> >  MEDIA_ENT_F_OLD_BASE,
> >   * or, otherwise, will be silently ignored by media-ctl when
> >   * printing the graphviz diagram. So, map them into the devnode
> >   * old range.
> >   */
> >  if (ent->function < MEDIA_ENT_F_OLD_BASE ||
> >  ent->function > MEDIA_ENT_F_TUNER) {
> >  if (is_media_entity_v4l2_subdev(ent))
> >  entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
> >  else if (ent->function != MEDIA_ENT_F_IO_V4L)
> >  entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
> >  }
> > 
> >  But this means that the entity type returned by ENUM_ENTITIES just 
> >  overwrites
> >  perfectly fine types by bogus values and thus the returned value 
> >  differs
> >  from that returned by G_TOPOLOGY.
> > 
> >  Can we please, please remove this workaround? I have no idea why a 
> >  workaround
> >  for media-ctl of all things ever made it in the kernel.  
> > >>>
> > >>> The entity types were replaced by entity functions back in 2015 with the
> > >>> big Media controller reshuffle. While I agree functions are better for
> > >>> describing entities than types (and those types had Linux specific
> > >>> interfaces in them), the new function-based API still may support a 
> > >>> single
> > >>> value, i.e. a single function per device.
> > >>>
> > >>> This also created two different name spaces for describing entities: the
> > >>> old types used by the MC API and the new functions used by MC v2 API.
> > >>>
> > >>> This doesn't go well with the fact that, as you noticed, the pad
> > >>> information is not available through MC v2 API. The pad information is
> > >>> required by the user space so software continues to use the original MC
> > >>> API.
> > >>>
> > >>> I don't think there's a way to avoid maintaining two name spaces (types 
> > >>> and
> > >>> functions) without breaking at least one of the APIs.  
> > >>
> > >> The comment specifically claims that this workaround is for media-ctl and
> > >> it suggests that it is fixed after v1.10. Is that comment bogus? I can't
> > >> really tell which commit fixed media-ctl. Does anyone know?
> > >>
> > >> As far as I can tell the function defines have been chosen in such a way 
> > >> that
> > >> they will equally well work with the old name space. There should be no
> > >> problem there whatsoever and media-ctl should switch to use the new 
> > >> defines.  
> > > 
> > > The old interface (type) was centered around the uAPI for the entity.
> > > That's no longer the case for functions. The entity type
> > > (MEDIA_ENT_TYPE_MASK) tells the uAPI which affects the interpretation of
> > > the dev union in struct media_entity_struct as well as the interface that
> > > device node implements. With the new function field that's no longer the
> > > case.
> > > 
> > > Also, the new MC v2 API makes a separation between the entity function and
> > > the uAPI (interface) which was lacking in the old API.
> > >   
> > >>
> > >> We now have a broken ENUM_ENTITIES ioctl (it rudely overwrites 
> > >> VBI/DVB/etc types)
> > >> and a broken G_TOPOLOGY ioctl (no pad index).
> > >>
> > >> This sucks. Let's fix both so that they at least report consistent 
> > >> information.  
> > > 
> > > The existing user space may assume that the type field of the entity
> > > conveys that the entity does provide a V4L2 sub-device interface if that's
> > > the case actually.
> > > 
> > > This is what media-ctl does and I presume if existing user space checks 
> > > for
> > > the type field, it may well have similar checks: it was how the API was
> > > defined. Therefore it's not entirely accurate to say that only media-ctl
> > > has this "bug", I'd generally assume programs that use MC (v1) API do 
> > > this.
> > > 
> > > You could argue about the merits (or lack of them) of the old API, no
> > > disagrement there.  
> > 
> > The old API is already broken. E.g. using 

Re: media_device.c question: can this workaround be removed?

[Sakari Ailus]

Hi Hans,

On Mon, Feb 05, 2018 at 04:19:28PM +0100, Hans Verkuil wrote:
> On 02/05/2018 03:30 PM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Mon, Feb 05, 2018 at 01:30:04PM +0100, Hans Verkuil wrote:
> >> On 02/05/2018 12:59 PM, Sakari Ailus wrote:
> >>> Hi Hans,
> >>>
> >>> On Mon, Feb 05, 2018 at 11:26:47AM +0100, Hans Verkuil wrote:
>  The function media_device_enum_entities() has this workaround:
> 
>  /*
>   * Workaround for a bug at media-ctl <= v1.10 that makes it to
>   * do the wrong thing if the entity function doesn't belong to
>   * either MEDIA_ENT_F_OLD_BASE or MEDIA_ENT_F_OLD_SUBDEV_BASE
>   * Ranges.
>   *
>   * Non-subdevices are expected to be at the MEDIA_ENT_F_OLD_BASE,
>   * or, otherwise, will be silently ignored by media-ctl when
>   * printing the graphviz diagram. So, map them into the devnode
>   * old range.
>   */
>  if (ent->function < MEDIA_ENT_F_OLD_BASE ||
>  ent->function > MEDIA_ENT_F_TUNER) {
>  if (is_media_entity_v4l2_subdev(ent))
>  entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
>  else if (ent->function != MEDIA_ENT_F_IO_V4L)
>  entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
>  }
> 
>  But this means that the entity type returned by ENUM_ENTITIES just 
>  overwrites
>  perfectly fine types by bogus values and thus the returned value differs
>  from that returned by G_TOPOLOGY.
> 
>  Can we please, please remove this workaround? I have no idea why a 
>  workaround
>  for media-ctl of all things ever made it in the kernel.
> >>>
> >>> The entity types were replaced by entity functions back in 2015 with the
> >>> big Media controller reshuffle. While I agree functions are better for
> >>> describing entities than types (and those types had Linux specific
> >>> interfaces in them), the new function-based API still may support a single
> >>> value, i.e. a single function per device.
> >>>
> >>> This also created two different name spaces for describing entities: the
> >>> old types used by the MC API and the new functions used by MC v2 API.
> >>>
> >>> This doesn't go well with the fact that, as you noticed, the pad
> >>> information is not available through MC v2 API. The pad information is
> >>> required by the user space so software continues to use the original MC
> >>> API.
> >>>
> >>> I don't think there's a way to avoid maintaining two name spaces (types 
> >>> and
> >>> functions) without breaking at least one of the APIs.
> >>
> >> The comment specifically claims that this workaround is for media-ctl and
> >> it suggests that it is fixed after v1.10. Is that comment bogus? I can't
> >> really tell which commit fixed media-ctl. Does anyone know?
> >>
> >> As far as I can tell the function defines have been chosen in such a way 
> >> that
> >> they will equally well work with the old name space. There should be no
> >> problem there whatsoever and media-ctl should switch to use the new 
> >> defines.
> > 
> > The old interface (type) was centered around the uAPI for the entity.
> > That's no longer the case for functions. The entity type
> > (MEDIA_ENT_TYPE_MASK) tells the uAPI which affects the interpretation of
> > the dev union in struct media_entity_struct as well as the interface that
> > device node implements. With the new function field that's no longer the
> > case.
> > 
> > Also, the new MC v2 API makes a separation between the entity function and
> > the uAPI (interface) which was lacking in the old API.
> > 
> >>
> >> We now have a broken ENUM_ENTITIES ioctl (it rudely overwrites VBI/DVB/etc 
> >> types)
> >> and a broken G_TOPOLOGY ioctl (no pad index).
> >>
> >> This sucks. Let's fix both so that they at least report consistent 
> >> information.
> > 
> > The existing user space may assume that the type field of the entity
> > conveys that the entity does provide a V4L2 sub-device interface if that's
> > the case actually.
> > 
> > This is what media-ctl does and I presume if existing user space checks for
> > the type field, it may well have similar checks: it was how the API was
> > defined. Therefore it's not entirely accurate to say that only media-ctl
> > has this "bug", I'd generally assume programs that use MC (v1) API do this.
> > 
> > You could argue about the merits (or lack of them) of the old API, no
> > disagrement there.
> 
> The old API is already broken. E.g. using MEDIA_ENT_F_PROC_VIDEO_SCALER in
> vimc/vimc-scaler.c (instead of the current - and bogus - 
> MEDIA_ENT_F_ATV_DECODER)
> gives me this with media-ctl:
> 
> - entity 21: Scaler (2 pads, 4 links)
>  type V4L2 subdev subtype Unknown flags 0
>  device node name /dev/v4l-subdev5
> pad0: Sink
> [fmt:RGB888_1X24/640x480 field:none]

Re: [PATCH 4/5] add V4L2 control functions

[Hans Verkuil]

On 02/05/2018 10:36 PM, Florian Echtler wrote:
> Hello Hans,
> 
> On Mon, 5 Feb 2018, Hans Verkuil wrote:
>> On 02/05/2018 03:29 PM, Florian Echtler wrote:
>>> +
>>> +static int sur40_vidioc_queryctrl(struct file *file, void *fh,
>>> +  struct v4l2_queryctrl *qc)
>>
>> Sorry, but this is very wrong. Use the control framework instead. See
>> https://hverkuil.home.xs4all.nl/spec/kapi/v4l2-controls.html and pretty much 
>> all
>> media drivers (with the exception of uvc). See for example this driver:
>> drivers/media/pci/tw68/tw68-video.c (randomly chosen).
>>
>> It actually makes life a lot easier for you as you don't have to perform any
>> range checks and all control ioctls are automatically supported for you.
> 
> thanks for the quick reply, I wasn't aware of that framework at all :-) 
> Will certainly use it.
> 
> What's the earliest kernel version this is supported on, in case we want 
> to backport this for our standalone module, too?

Initial commit was in August 2010, so it's been around for quite some time :-)

Regards,

Hans

Re: [PATCH 4/5] add V4L2 control functions

[Florian Echtler]

Hello Hans,

On Mon, 5 Feb 2018, Hans Verkuil wrote:

On 02/05/2018 03:29 PM, Florian Echtler wrote:

+
+static int sur40_vidioc_queryctrl(struct file *file, void *fh,
+  struct v4l2_queryctrl *qc)


Sorry, but this is very wrong. Use the control framework instead. See
https://hverkuil.home.xs4all.nl/spec/kapi/v4l2-controls.html and pretty much all
media drivers (with the exception of uvc). See for example this driver:
drivers/media/pci/tw68/tw68-video.c (randomly chosen).

It actually makes life a lot easier for you as you don't have to perform any
range checks and all control ioctls are automatically supported for you.


thanks for the quick reply, I wasn't aware of that framework at all :-) 
Will certainly use it.


What's the earliest kernel version this is supported on, in case we want 
to backport this for our standalone module, too?


Best regards, Florian
--
"_Nothing_ brightens up my morning. Coffee simply provides a shade of
grey just above the pitch-black of the infinite depths of the _abyss_."

Re: [PATCH v2 8/8] platform: vivid-cec: use 64-bit arithmetic instead of 32-bit

[Hans Verkuil]

On 02/05/2018 09:36 PM, Gustavo A. R. Silva wrote:
> Add suffix ULL to constant 10 in order to give the compiler complete
> information about the proper arithmetic to use. Notice that this
> constant is used in a context that expects an expression of type
> u64 (64 bits, unsigned).
> 
> The expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is currently being
> evaluated using 32-bit arithmetic.
> 
> Also, remove unnecessary parentheses and add a code comment to make it
> clear what is the reason of the code change.
> 
> Addresses-Coverity-ID: 1454996
> Signed-off-by: Gustavo A. R. Silva 
> ---
> Changes in v2:
>  - Update subject and changelog to better reflect the proposed code changes.
>  - Add suffix ULL to constant instead of casting a variable.
>  - Remove unncessary parentheses.

unncessary -> unnecessary

>  - Add code comment.
> 
>  drivers/media/platform/vivid/vivid-cec.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vivid/vivid-cec.c 
> b/drivers/media/platform/vivid/vivid-cec.c
> index b55d278..614787b 100644
> --- a/drivers/media/platform/vivid/vivid-cec.c
> +++ b/drivers/media/platform/vivid/vivid-cec.c
> @@ -82,8 +82,15 @@ static void vivid_cec_pin_adap_events(struct cec_adapter 
> *adap, ktime_t ts,
>  
>   if (adap == NULL)
>   return;
> - ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL +
> -len * 10 * CEC_TIM_DATA_BIT_TOTAL));
> +
> + /*
> +  * Suffix ULL on constant 10 makes the expression
> +  * CEC_TIM_START_BIT_TOTAL + 10ULL * len * CEC_TIM_DATA_BIT_TOTAL
> +  * be evaluated using 64-bit unsigned arithmetic (u64), which
> +  * is what ktime_sub_us expects as second argument.
> +  */

That's not really the comment that I was looking for. It still doesn't
explain *why* this is needed at all. How about something like this:

/*
 * Add the ULL suffix to the constant 10 to work around a false Coverity
 * "Unintentional integer overflow" warning. Coverity isn't smart enough
 * to understand that len is always <= 16, so there is no chance of an
 * integer overflow.
 */

Regards,

Hans

> + ts = ktime_sub_us(ts, CEC_TIM_START_BIT_TOTAL +
> +10ULL * len * CEC_TIM_DATA_BIT_TOTAL);
>   cec_queue_pin_cec_event(adap, false, ts);
>   ts = ktime_add_us(ts, CEC_TIM_START_BIT_LOW);
>   cec_queue_pin_cec_event(adap, true, ts);
> 

[PATCH v2 8/8] platform: vivid-cec: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Add suffix ULL to constant 10 in order to give the compiler complete
information about the proper arithmetic to use. Notice that this
constant is used in a context that expects an expression of type
u64 (64 bits, unsigned).

The expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is currently being
evaluated using 32-bit arithmetic.

Also, remove unnecessary parentheses and add a code comment to make it
clear what is the reason of the code change.

Addresses-Coverity-ID: 1454996
Signed-off-by: Gustavo A. R. Silva 
---
Changes in v2:
 - Update subject and changelog to better reflect the proposed code changes.
 - Add suffix ULL to constant instead of casting a variable.
 - Remove unncessary parentheses.
 - Add code comment.

 drivers/media/platform/vivid/vivid-cec.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vivid/vivid-cec.c 
b/drivers/media/platform/vivid/vivid-cec.c
index b55d278..614787b 100644
--- a/drivers/media/platform/vivid/vivid-cec.c
+++ b/drivers/media/platform/vivid/vivid-cec.c
@@ -82,8 +82,15 @@ static void vivid_cec_pin_adap_events(struct cec_adapter 
*adap, ktime_t ts,
 
if (adap == NULL)
return;
-   ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL +
-  len * 10 * CEC_TIM_DATA_BIT_TOTAL));
+
+   /*
+* Suffix ULL on constant 10 makes the expression
+* CEC_TIM_START_BIT_TOTAL + 10ULL * len * CEC_TIM_DATA_BIT_TOTAL
+* be evaluated using 64-bit unsigned arithmetic (u64), which
+* is what ktime_sub_us expects as second argument.
+*/
+   ts = ktime_sub_us(ts, CEC_TIM_START_BIT_TOTAL +
+  10ULL * len * CEC_TIM_DATA_BIT_TOTAL);
cec_queue_pin_cec_event(adap, false, ts);
ts = ktime_add_us(ts, CEC_TIM_START_BIT_LOW);
cec_queue_pin_cec_event(adap, true, ts);
-- 
2.7.4

[PATCH v2 7/8] platform: sh_veu: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Cast left and top to dma_addr_t in order to give the compiler complete
information about the proper arithmetic to use. Notice that these
variables are being used in contexts that expect expressions of
type dma_addr_t (64 bit, unsigned).

Such expressions are currently being evaluated using 32-bit arithmetic.

Also, move the expression (((dma_addr_t)left * veu->vfmt_out.fmt->depth) >> 3)
at the end in order to avoid a line wrapping checkpatch.pl warning.

Addresses-Coverity-ID: 1056807
Addresses-Coverity-ID: 1056808
Signed-off-by: Gustavo A. R. Silva 
---
Changes in v2:
 - Update subject and changelog to better reflect the proposed code changes.
 - Move the expression (((dma_addr_t)left * veu->vfmt_out.fmt->depth) >> 3)
   at the end in order to avoid a line wrapping checkpatch.pl warning.

 drivers/media/platform/sh_veu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/sh_veu.c b/drivers/media/platform/sh_veu.c
index 976ea0b..1a0cde0 100644
--- a/drivers/media/platform/sh_veu.c
+++ b/drivers/media/platform/sh_veu.c
@@ -520,8 +520,8 @@ static void sh_veu_colour_offset(struct sh_veu_dev *veu, 
struct sh_veu_vfmt *vfm
/* dst_left and dst_top validity will be verified in CROP / COMPOSE */
unsigned int left = vfmt->frame.left & ~0x03;
unsigned int top = vfmt->frame.top;
-   dma_addr_t offset = ((left * veu->vfmt_out.fmt->depth) >> 3) +
-   top * veu->vfmt_out.bytesperline;
+   dma_addr_t offset = (dma_addr_t)top * veu->vfmt_out.bytesperline +
+   (((dma_addr_t)left * veu->vfmt_out.fmt->depth) >> 3);
unsigned int y_line;
 
vfmt->offset_y = offset;
-- 
2.7.4

[PATCH v2 6/8] rockchip/rga: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Cast p to dma_addr_t in order to avoid a potential integer overflow.
This variable is being used in a context that expects an expression
of type dma_addr_t (u64).

The expression p << PAGE_SHIFT is currently being evaluated
using 32-bit arithmetic.

Addresses-Coverity-ID: 1458347
Signed-off-by: Gustavo A. R. Silva 
---
Changes in v2:
 - Update subject and changelog to better reflect the proposed code change.

 drivers/media/platform/rockchip/rga/rga-buf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rockchip/rga/rga-buf.c 
b/drivers/media/platform/rockchip/rga/rga-buf.c
index 49cacc7..a43b57a 100644
--- a/drivers/media/platform/rockchip/rga/rga-buf.c
+++ b/drivers/media/platform/rockchip/rga/rga-buf.c
@@ -140,7 +140,8 @@ void rga_buf_map(struct vb2_buffer *vb)
address = sg_phys(sgl);
 
for (p = 0; p < len; p++) {
-   dma_addr_t phys = address + (p << PAGE_SHIFT);
+   dma_addr_t phys = address +
+ ((dma_addr_t)p << PAGE_SHIFT);
 
pages[mapped_size + p] = phys;
}
-- 
2.7.4

[PATCH v2 2/8] dvb-frontends: ves1820: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Add suffix ULL to constant 10 in order to give the compiler complete
information about the proper arithmetic to use. Notice that this
constant is used in a context that expects an expression of type
u64 (64 bits, unsigned).

The expression fpxin = state->config->xin * 10 is currently being
evaluated using 32-bit arithmetic.

Addresses-Coverity-ID: 200604
Signed-off-by: Gustavo A. R. Silva 
---
Changes in v2:
 - Update subject and changelog to better reflect the proposed code changes.
 - Add suffix ULL to constant instead of casting a variable.

 drivers/media/dvb-frontends/ves1820.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/ves1820.c 
b/drivers/media/dvb-frontends/ves1820.c
index 1d89792..1760098 100644
--- a/drivers/media/dvb-frontends/ves1820.c
+++ b/drivers/media/dvb-frontends/ves1820.c
@@ -137,7 +137,7 @@ static int ves1820_set_symbolrate(struct ves1820_state 
*state, u32 symbolrate)
NDEC = 3;
 
/* yeuch! */
-   fpxin = state->config->xin * 10;
+   fpxin = state->config->xin * 10ULL;
fptmp = fpxin; do_div(fptmp, 123);
if (symbolrate < fptmp)
SFIL = 1;
-- 
2.7.4

[PATCH v2 5/8] pci: cx88-input: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Add suffix LL to constant 100 in order to give the compiler
complete information about the proper arithmetic to use. Notice
that this constant is used in a context that expects an expression
of type ktime_t (64 bits, signed).

The expression ir->polling * 100 is currently being evaluated
using 32-bit arithmetic.

Addresses-Coverity-ID: 1392628
Addresses-Coverity-ID: 1392630
Signed-off-by: Gustavo A. R. Silva 
---
Changes in v2:
 - Update subject and changelog to better reflect the proposed code changes.
 - Add suffix LL to constant instead of casting a variable.

 drivers/media/pci/cx88/cx88-input.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/cx88/cx88-input.c 
b/drivers/media/pci/cx88/cx88-input.c
index 4e9953e..6f4e692 100644
--- a/drivers/media/pci/cx88/cx88-input.c
+++ b/drivers/media/pci/cx88/cx88-input.c
@@ -180,7 +180,7 @@ static enum hrtimer_restart cx88_ir_work(struct hrtimer 
*timer)
struct cx88_IR *ir = container_of(timer, struct cx88_IR, timer);
 
cx88_ir_handle_key(ir);
-   missed = hrtimer_forward_now(>timer, ir->polling * 100);
+   missed = hrtimer_forward_now(>timer, ir->polling * 100LL);
if (missed > 1)
ir_dprintk("Missed ticks %ld\n", missed - 1);
 
@@ -200,7 +200,7 @@ static int __cx88_ir_start(void *priv)
if (ir->polling) {
hrtimer_init(>timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
ir->timer.function = cx88_ir_work;
-   hrtimer_start(>timer, ir->polling * 100,
+   hrtimer_start(>timer, ir->polling * 100LL,
  HRTIMER_MODE_REL);
}
if (ir->sampling) {
-- 
2.7.4

[PATCH 3/4] v4l: dvb-frontends: stb0899: fix comparison to bitshift when dealing with a mask

[Wolfram Sang]

Due to a typo, the mask was destroyed by a comparison instead of a bit
shift.

Signed-off-by: Wolfram Sang 
---
Only build tested. To be applied individually per subsystem.

 drivers/media/dvb-frontends/stb0899_reg.h | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb-frontends/stb0899_reg.h 
b/drivers/media/dvb-frontends/stb0899_reg.h
index ba1ed56304a0f4..f564269249a682 100644
--- a/drivers/media/dvb-frontends/stb0899_reg.h
+++ b/drivers/media/dvb-frontends/stb0899_reg.h
@@ -374,22 +374,22 @@
 
 #define STB0899_OFF0_IF_AGC_GAIN   0xf30c
 #define STB0899_BASE_IF_AGC_GAIN   0x
-#define STB0899_IF_AGC_GAIN(0x3fff < 0)
+#define STB0899_IF_AGC_GAIN(0x3fff << 0)
 #define STB0899_OFFST_IF_AGC_GAIN  0
 #define STB0899_WIDTH_IF_AGC_GAIN  14
 
 #define STB0899_OFF0_BB_AGC_GAIN   0xf310
 #define STB0899_BASE_BB_AGC_GAIN   0x
-#define STB0899_BB_AGC_GAIN(0x3fff < 0)
+#define STB0899_BB_AGC_GAIN(0x3fff << 0)
 #define STB0899_OFFST_BB_AGC_GAIN  0
 #define STB0899_WIDTH_BB_AGC_GAIN  14
 
 #define STB0899_OFF0_DC_OFFSET 0xf314
 #define STB0899_BASE_DC_OFFSET 0x
-#define STB0899_I  (0xff < 8)
+#define STB0899_I  (0xff << 8)
 #define STB0899_OFFST_I8
 #define STB0899_WIDTH_I8
-#define STB0899_Q  (0xff < 0)
+#define STB0899_Q  (0xff << 0)
 #define STB0899_OFFST_Q8
 #define STB0899_WIDTH_Q8
 
-- 
2.11.0

[PATCH 1/4] v4l: vsp1: fix mask creation for MULT_ALPHA_RATIO

[Wolfram Sang]

Due to a typo, the mask was destroyed by a comparison instead of a bit
shift. No regression since the mask has not been used yet.

Signed-off-by: Wolfram Sang 
---
Only build tested. To be applied individually per subsystem.

 drivers/media/platform/vsp1/vsp1_regs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/vsp1/vsp1_regs.h 
b/drivers/media/platform/vsp1/vsp1_regs.h
index 26c4ffad2f4656..b1912c83a1dae2 100644
--- a/drivers/media/platform/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/vsp1/vsp1_regs.h
@@ -225,7 +225,7 @@
 #define VI6_RPF_MULT_ALPHA_P_MMD_RATIO (1 << 8)
 #define VI6_RPF_MULT_ALPHA_P_MMD_IMAGE (2 << 8)
 #define VI6_RPF_MULT_ALPHA_P_MMD_BOTH  (3 << 8)
-#define VI6_RPF_MULT_ALPHA_RATIO_MASK  (0xff < 0)
+#define VI6_RPF_MULT_ALPHA_RATIO_MASK  (0xff << 0)
 #define VI6_RPF_MULT_ALPHA_RATIO_SHIFT 0
 
 /* 
-
-- 
2.11.0

[PATCH 0/4] tree-wide: fix comparison to bitshift when dealing with a mask

[Wolfram Sang]

In one Renesas driver, I found a typo which turned an intended bit shift ('<<')
into a comparison ('<'). Because this is a subtle issue, I looked tree wide for
similar patterns. This small patch series is the outcome.

Buildbot and checkpatch are happy. Only compile-tested. To be applied
individually per sub-system, I think. I'd think only the net: amd: patch needs
to be conisdered for stable, but I leave this to people who actually know this
driver.

CCing Dan. Maybe he has an idea how to add a test to smatch? In my setup, only
cppcheck reported a 'coding style' issue with a low prio.

Wolfram Sang (4):
  v4l: vsp1: fix mask creation for MULT_ALPHA_RATIO
  drm/exynos: fix comparison to bitshift when dealing with a mask
  v4l: dvb-frontends: stb0899: fix comparison to bitshift when dealing
with a mask
  net: amd-xgbe: fix comparison to bitshift when dealing with a mask

 drivers/gpu/drm/exynos/regs-fimc.h| 2 +-
 drivers/media/dvb-frontends/stb0899_reg.h | 8 
 drivers/media/platform/vsp1/vsp1_regs.h   | 2 +-
 drivers/net/ethernet/amd/xgbe/xgbe-drv.c  | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.11.0

[PATCH v2 4/8] i2c: ov9650: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Add suffix ULL to constants 1 and 100 in order to give the
compiler complete information about the proper arithmetic to use.
Notice that these constants are used in contexts that expect
expressions of type u64 (64 bits, unsigned).

The following expressions:

(u64)(fi->interval.numerator * 1)
(u64)(iv->interval.numerator * 1)
fiv->interval.numerator * 100 / fiv->interval.denominator

are currently being evaluated using 32-bit arithmetic.

Notice that those casts to u64 for the first two expressions are only
effective after such expressions are evaluated using 32-bit arithmetic,
which leads to potential integer overflows. So based on those casts, it
seems that the original intention of the code is to actually use 64-bit
arithmetic instead of 32-bit.

Also, notice that once the suffix ULL is added to the constants, the
outer casts to u64 are no longer needed.

Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
Fixes: 84a15ded76ec ("[media] V4L: Add driver for OV9650/52 image sensors")
Fixes: 79211c8ed19c ("remove abs64()")
Signed-off-by: Gustavo A. R. Silva 
---
Changes in v2:
 - Update subject and changelog to better reflect the proposed code changes.
 - Add suffix ULL to constants instead of casting variables.
 - Remove unnecessary casts to u64 as part of the code change.
 - Extend the same code change to other similar expressions.

 drivers/media/i2c/ov9650.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
index e519f27..e716e98 100644
--- a/drivers/media/i2c/ov9650.c
+++ b/drivers/media/i2c/ov9650.c
@@ -1130,7 +1130,7 @@ static int __ov965x_set_frame_interval(struct ov965x 
*ov965x,
if (fi->interval.denominator == 0)
return -EINVAL;
 
-   req_int = (u64)(fi->interval.numerator * 1) /
+   req_int = fi->interval.numerator * 1ULL /
fi->interval.denominator;
 
for (i = 0; i < ARRAY_SIZE(ov965x_intervals); i++) {
@@ -1139,7 +1139,7 @@ static int __ov965x_set_frame_interval(struct ov965x 
*ov965x,
if (mbus_fmt->width != iv->size.width ||
mbus_fmt->height != iv->size.height)
continue;
-   err = abs((u64)(iv->interval.numerator * 1) /
+   err = abs(iv->interval.numerator * 1ULL /
iv->interval.denominator - req_int);
if (err < min_err) {
fiv = iv;
@@ -1148,8 +1148,9 @@ static int __ov965x_set_frame_interval(struct ov965x 
*ov965x,
}
ov965x->fiv = fiv;
 
-   v4l2_dbg(1, debug, >sd, "Changed frame interval to %u us\n",
-fiv->interval.numerator * 100 / fiv->interval.denominator);
+   v4l2_dbg(1, debug, >sd, "Changed frame interval to %llu us\n",
+fiv->interval.numerator * 100ULL /
+fiv->interval.denominator);
 
return 0;
 }
-- 
2.7.4

[PATCH v2 3/8] i2c: max2175: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Add suffix LL to constant 2 in order to give the compiler complete
information about the proper arithmetic to use. Notice that this
constant is used in a context that expects an expression of type
s64 (64 bits, signed).

The expression 2 * (clock_rate - abs_nco_freq) is currently being
evaluated using 32-bit arithmetic.

Addresses-Coverity-ID: 1446589
Signed-off-by: Gustavo A. R. Silva 
---
Changes in v2:
 - Update subject and changelog to better reflect the proposed code changes.
 - Add suffix LL to constant instead of casting a variable.

 drivers/media/i2c/max2175.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/max2175.c b/drivers/media/i2c/max2175.c
index 2f1966b..87cba15 100644
--- a/drivers/media/i2c/max2175.c
+++ b/drivers/media/i2c/max2175.c
@@ -643,7 +643,7 @@ static int max2175_set_nco_freq(struct max2175 *ctx, s32 
nco_freq)
if (abs_nco_freq < clock_rate / 2) {
nco_val_desired = 2 * nco_freq;
} else {
-   nco_val_desired = 2 * (clock_rate - abs_nco_freq);
+   nco_val_desired = 2LL * (clock_rate - abs_nco_freq);
if (nco_freq < 0)
nco_val_desired = -nco_val_desired;
}
-- 
2.7.4

[PATCH v2 1/8] rtl2832: use 64-bit arithmetic instead of 32-bit in rtl2832_set_frontend

[Gustavo A. R. Silva]

Add suffix ULL to constant 7 in order to give the compiler complete
information about the proper arithmetic to use. Notice that this
constant is used in a context that expects an expression of type
u64 (64 bits, unsigned).

The expression dev->pdata->clk * 7 is currently being evaluated
using 32-bit arithmetic.

Addresses-Coverity-ID: 1271223
Signed-off-by: Gustavo A. R. Silva 
---
Changes in v2:
 - Update subject and changelog to better reflect the proposed code changes.
 - Add suffix ULL to constant instead of casting a variable.

 drivers/media/dvb-frontends/rtl2832.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-frontends/rtl2832.c 
b/drivers/media/dvb-frontends/rtl2832.c
index 94bf5b7..fa3b816 100644
--- a/drivers/media/dvb-frontends/rtl2832.c
+++ b/drivers/media/dvb-frontends/rtl2832.c
@@ -498,7 +498,7 @@ static int rtl2832_set_frontend(struct dvb_frontend *fe)
* RSAMP_RATIO = floor(CrystalFreqHz * 7 * pow(2, 22)
*   / ConstWithBandwidthMode)
*/
-   num = dev->pdata->clk * 7;
+   num = dev->pdata->clk * 7ULL;
num *= 0x40;
num = div_u64(num, bw_mode);
resamp_ratio =  num & 0x3ff;
@@ -511,7 +511,7 @@ static int rtl2832_set_frontend(struct dvb_frontend *fe)
*   / (CrystalFreqHz * 7))
*/
num = bw_mode << 20;
-   num2 = dev->pdata->clk * 7;
+   num2 = dev->pdata->clk * 7ULL;
num = div_u64(num, num2);
num = -num;
cfreq_off_ratio = num & 0xf;
-- 
2.7.4

[PATCH v2 0/8] use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Add suffix LL and ULL to various constants in order to give the compiler
complete information about the proper arithmetic to use. Such constants
are used in contexts that expect expressions of type u64 (64 bits, unsigned)
and s64 (64 bits, signed).

The mentioned expressions are currently being evaluated using 32-bit
arithmetic, wich is some cases can lead to unintentional integer
overflows.

This patchset addresses the following Coverity IDs:
CIDs: 200604, 1056807, 1056808, 1271223, 1324146
CIDs: 1392628, 1392630, 1446589, 1454996, 1458347

Thank you

Changes in v2:
 - Update subject and changelog to better reflect the proposed code changes.
 - Add suffix ULL and LL to constants instead of casting variables.
 - Extend the proposed code changes to other similar cases that had not
   previously been considered in v1 of this patchset.

Gustavo A. R. Silva (8):
  rtl2832: use 64-bit arithmetic instead of 32-bit in
rtl2832_set_frontend
  dvb-frontends: ves1820: use 64-bit arithmetic instead of 32-bit
  i2c: max2175: use 64-bit arithmetic instead of 32-bit
  i2c: ov9650: use 64-bit arithmetic instead of 32-bit
  pci: cx88-input: use 64-bit arithmetic instead of 32-bit
  rockchip/rga: use 64-bit arithmetic instead of 32-bit
  platform: sh_veu: use 64-bit arithmetic instead of 32-bit
  platform: vivid-cec: use 64-bit arithmetic instead of 32-bit

 drivers/media/dvb-frontends/rtl2832.c |  4 ++--
 drivers/media/dvb-frontends/ves1820.c |  2 +-
 drivers/media/i2c/max2175.c   |  2 +-
 drivers/media/i2c/ov9650.c|  9 +
 drivers/media/pci/cx88/cx88-input.c   |  4 ++--
 drivers/media/platform/rockchip/rga/rga-buf.c |  3 ++-
 drivers/media/platform/sh_veu.c   |  4 ++--
 drivers/media/platform/vivid/vivid-cec.c  | 11 +--
 8 files changed, 24 insertions(+), 15 deletions(-)

-- 
2.7.4

Re: Please help test the new v4l-subdev support in v4l2-compliance

[Mauro Carvalho Chehab]

Em Mon, 5 Feb 2018 18:01:34 +0100
Hans Verkuil  escreveu:

> On 02/05/2018 05:59 PM, Hans Verkuil wrote:
> > On 02/05/2018 05:55 PM, Mauro Carvalho Chehab wrote:  
> >> Em Mon, 5 Feb 2018 14:37:29 -0200
> >> Mauro Carvalho Chehab  escreveu:
> >>  
> >>> Em Mon, 5 Feb 2018 08:21:54 -0800
> >>> Tim Harvey  escreveu:
> >>>  
>  Hans,
> 
>  I'm failing compile (of master 4ee9911) with:
> 
>    CXX  v4l2_compliance-media-info.o
>  media-info.cpp: In function ‘media_type media_detect_type(const char*)’:
>  media-info.cpp:79:39: error: no matching function for call to
>  ‘std::basic_ifstream::basic_ifstream(std::__cxx11::string&)’
>    std::ifstream uevent_file(uevent_path);
> ^
> >>>
> >>> Btw, while on it, a few days ago, I noticed several class warnings when
> >>> building v4l-utils on Raspbian, saying that it was using some features
> >>> that future versions of gcc would stop using at qv4l2. See enclosed.
> >>>
> >>> I didn't have time to look on them.  
> >>
> >> FYI, it still happens with today's upstream's version:
> >>
> >>4ee99116d0ec (HEAD, origin/master, origin/HEAD) v4l2-ctl: improve the 
> >> fps calculation when streaming
> >>
> >> $ gcc --version
> >> gcc (Raspbian 6.3.0-18+rpi1) 6.3.0 20170516
> >> Copyright (C) 2016 Free Software Foundation, Inc.
> >> This is free software; see the source for copying conditions.  There is NO
> >> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR 
> >> PURPOSE.  
> > 
> > My guess is that it is a bogus message from gcc 6.
> > 
> > Regards,
> > 
> > Hans
> >   
> 
> See: https://gcc.gnu.org/ml/gcc/2017-05/msg00073.html
> 
> Nothing to worry about.

Hmm... as suggested there, it could be worth to add -Wno-psabi at qv4l2
Makefile if arch is arm[1], in order to avoid those warnings there.

[1] from what's said at https://gcc.gnu.org/gcc-7/changes.html, this is
due to a bug on gcc 5 for ARM.

> 
>   Hans



Thanks,
Mauro

Re: Please help test the new v4l-subdev support in v4l2-compliance

[Hans Verkuil]

On 02/05/2018 05:59 PM, Hans Verkuil wrote:
> On 02/05/2018 05:55 PM, Mauro Carvalho Chehab wrote:
>> Em Mon, 5 Feb 2018 14:37:29 -0200
>> Mauro Carvalho Chehab  escreveu:
>>
>>> Em Mon, 5 Feb 2018 08:21:54 -0800
>>> Tim Harvey  escreveu:
>>>
 Hans,

 I'm failing compile (of master 4ee9911) with:

   CXX  v4l2_compliance-media-info.o
 media-info.cpp: In function ‘media_type media_detect_type(const char*)’:
 media-info.cpp:79:39: error: no matching function for call to
 ‘std::basic_ifstream::basic_ifstream(std::__cxx11::string&)’
   std::ifstream uevent_file(uevent_path);
^  
>>>
>>> Btw, while on it, a few days ago, I noticed several class warnings when
>>> building v4l-utils on Raspbian, saying that it was using some features
>>> that future versions of gcc would stop using at qv4l2. See enclosed.
>>>
>>> I didn't have time to look on them.
>>
>> FYI, it still happens with today's upstream's version:
>>
>>  4ee99116d0ec (HEAD, origin/master, origin/HEAD) v4l2-ctl: improve the 
>> fps calculation when streaming
>>
>> $ gcc --version
>> gcc (Raspbian 6.3.0-18+rpi1) 6.3.0 20170516
>> Copyright (C) 2016 Free Software Foundation, Inc.
>> This is free software; see the source for copying conditions.  There is NO
>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
> My guess is that it is a bogus message from gcc 6.
> 
> Regards,
> 
>   Hans
> 

See: https://gcc.gnu.org/ml/gcc/2017-05/msg00073.html

Nothing to worry about.

Hans

Re: Please help test the new v4l-subdev support in v4l2-compliance

[Hans Verkuil]

On 02/05/2018 05:55 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 5 Feb 2018 14:37:29 -0200
> Mauro Carvalho Chehab  escreveu:
> 
>> Em Mon, 5 Feb 2018 08:21:54 -0800
>> Tim Harvey  escreveu:
>>
>>> Hans,
>>>
>>> I'm failing compile (of master 4ee9911) with:
>>>
>>>   CXX  v4l2_compliance-media-info.o
>>> media-info.cpp: In function ‘media_type media_detect_type(const char*)’:
>>> media-info.cpp:79:39: error: no matching function for call to
>>> ‘std::basic_ifstream::basic_ifstream(std::__cxx11::string&)’
>>>   std::ifstream uevent_file(uevent_path);
>>>^  
>>
>> Btw, while on it, a few days ago, I noticed several class warnings when
>> building v4l-utils on Raspbian, saying that it was using some features
>> that future versions of gcc would stop using at qv4l2. See enclosed.
>>
>> I didn't have time to look on them.
> 
> FYI, it still happens with today's upstream's version:
> 
>   4ee99116d0ec (HEAD, origin/master, origin/HEAD) v4l2-ctl: improve the 
> fps calculation when streaming
> 
> $ gcc --version
> gcc (Raspbian 6.3.0-18+rpi1) 6.3.0 20170516
> Copyright (C) 2016 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.  There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

My guess is that it is a bogus message from gcc 6.

Regards,

Hans

Re: Please help test the new v4l-subdev support in v4l2-compliance

[Hans Verkuil]

On 02/05/2018 05:37 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 5 Feb 2018 08:21:54 -0800
> Tim Harvey  escreveu:
> 
>> Hans,
>>
>> I'm failing compile (of master 4ee9911) with:
>>
>>   CXX  v4l2_compliance-media-info.o
>> media-info.cpp: In function ‘media_type media_detect_type(const char*)’:
>> media-info.cpp:79:39: error: no matching function for call to
>> ‘std::basic_ifstream::basic_ifstream(std::__cxx11::string&)’
>>   std::ifstream uevent_file(uevent_path);
>>^
> 
> Btw, while on it, a few days ago, I noticed several class warnings when
> building v4l-utils on Raspbian, saying that it was using some features
> that future versions of gcc would stop using at qv4l2. See enclosed.
> 
> I didn't have time to look on them.
> 
> Thanks,
> Mauro
> 
> In file included from /usr/include/c++/6/map:60:0,
>  from 
> /usr/include/arm-linux-gnueabihf/qt5/QtCore/qmetatype.h:57,
>  from 
> /usr/include/arm-linux-gnueabihf/qt5/QtCore/qobject.h:54,
>  from 
> /usr/include/arm-linux-gnueabihf/qt5/QtWidgets/qwidget.h:44,
>  from 
> /usr/include/arm-linux-gnueabihf/qt5/QtWidgets/qmainwindow.h:43,
>  from 
> /usr/include/arm-linux-gnueabihf/qt5/QtWidgets/QMainWindow:1,
>  from qv4l2.h:25,
>  from ctrl-tab.cpp:20:
> /usr/include/c++/6/bits/stl_tree.h: In member function 
> ‘std::pair 
> std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, 
> _Alloc>::_M_get_insert_hint_unique_pos(std::_Rb_tree<_Key, _Val, _KeyOfValue, 
> _Compare, _Alloc>::const_iterator, const key_type&) [with _Key = unsigned 
> int; _Val = std::pair; _KeyOfValue = 
> std::_Select1st; 
> _Compare = std::less; _Alloc = std::allocator >]’:
> /usr/include/c++/6/bits/stl_tree.h:1928:5: note: parameter passing for 
> argument of type ‘std::_Rb_tree v4l2_query_ext_ctrl>, std::_Select1st >, std::less, 
> std::allocator 
> >::const_iterator {aka std::_Rb_tree_const_iterator >}’ will change in GCC 7.1
>  _Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::
>  ^~~

I've seen it too, but I have no idea what to do with it. I'm not even sure that 
it is in my
code instead of in Qt headers or even c++ header.

It's not exactly an informative message.

Since I compile with g++ version 7.2 it appears that it is just fine since it 
doesn't complain.

Regards,

Hans

Re: Please help test the new v4l-subdev support in v4l2-compliance

[Mauro Carvalho Chehab]

Em Mon, 5 Feb 2018 14:37:29 -0200
Mauro Carvalho Chehab  escreveu:

> Em Mon, 5 Feb 2018 08:21:54 -0800
> Tim Harvey  escreveu:
> 
> > Hans,
> > 
> > I'm failing compile (of master 4ee9911) with:
> > 
> >   CXX  v4l2_compliance-media-info.o
> > media-info.cpp: In function ‘media_type media_detect_type(const char*)’:
> > media-info.cpp:79:39: error: no matching function for call to
> > ‘std::basic_ifstream::basic_ifstream(std::__cxx11::string&)’
> >   std::ifstream uevent_file(uevent_path);
> >^  
> 
> Btw, while on it, a few days ago, I noticed several class warnings when
> building v4l-utils on Raspbian, saying that it was using some features
> that future versions of gcc would stop using at qv4l2. See enclosed.
> 
> I didn't have time to look on them.

FYI, it still happens with today's upstream's version:

4ee99116d0ec (HEAD, origin/master, origin/HEAD) v4l2-ctl: improve the 
fps calculation when streaming

$ gcc --version
gcc (Raspbian 6.3.0-18+rpi1) 6.3.0 20170516
Copyright (C) 2016 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.


> 
> Thanks,
> Mauro
> 
> In file included from /usr/include/c++/6/map:60:0,
>  from 
> /usr/include/arm-linux-gnueabihf/qt5/QtCore/qmetatype.h:57,
>  from 
> /usr/include/arm-linux-gnueabihf/qt5/QtCore/qobject.h:54,
>  from 
> /usr/include/arm-linux-gnueabihf/qt5/QtWidgets/qwidget.h:44,
>  from 
> /usr/include/arm-linux-gnueabihf/qt5/QtWidgets/qmainwindow.h:43,
>  from 
> /usr/include/arm-linux-gnueabihf/qt5/QtWidgets/QMainWindow:1,
>  from qv4l2.h:25,
>  from ctrl-tab.cpp:20:
> /usr/include/c++/6/bits/stl_tree.h: In member function 
> ‘std::pair 
> std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, 
> _Alloc>::_M_get_insert_hint_unique_pos(std::_Rb_tree<_Key, _Val, _KeyOfValue, 
> _Compare, _Alloc>::const_iterator, const key_type&) [with _Key = unsigned 
> int; _Val = std::pair; _KeyOfValue = 
> std::_Select1st; 
> _Compare = std::less; _Alloc = std::allocator >]’:
> /usr/include/c++/6/bits/stl_tree.h:1928:5: note: parameter passing for 
> argument of type ‘std::_Rb_tree v4l2_query_ext_ctrl>, std::_Select1st >, std::less, 
> std::allocator 
> >::const_iterator {aka std::_Rb_tree_const_iterator >}’ will change in GCC 7.1
>  _Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::
>  ^~~
> /usr/include/c++/6/bits/stl_tree.h: In function ‘std::_Rb_tree<_Key, _Val, 
> _KeyOfValue, _Compare, _Alloc>::iterator std::_Rb_tree<_Key, _Val, 
> _KeyOfValue, _Compare, _Alloc>::_M_emplace_hint_unique(std::_Rb_tree<_Key, 
> _Val, _KeyOfValue, _Compare, _Alloc>::const_iterator, _Args&& ...) [with 
> _Args = {const std::piecewise_construct_t&, std::tuple, 
> std::tuple<>}; _Key = unsigned int; _Val = std::pair v4l2_query_ext_ctrl>; _KeyOfValue = std::_Select1st >; _Compare = std::less; _Alloc = 
> std::allocator]’:
> /usr/include/c++/6/bits/stl_tree.h:2193:7: note: parameter passing for 
> argument of type ‘std::_Rb_tree v4l2_query_ext_ctrl>, std::_Select1st >, std::less, 
> std::allocator 
> >::const_iterator {aka std::_Rb_tree_const_iterator >}’ will change in GCC 7.1
>_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::
>^~~
> In file included from /usr/include/c++/6/map:61:0,
>  from 
> /usr/include/arm-linux-gnueabihf/qt5/QtCore/qmetatype.h:57,
>  from 
> /usr/include/arm-linux-gnueabihf/qt5/QtCore/qobject.h:54,
>  from 
> /usr/include/arm-linux-gnueabihf/qt5/QtWidgets/qwidget.h:44,
>  from 
> /usr/include/arm-linux-gnueabihf/qt5/QtWidgets/qmainwindow.h:43,
>  from 
> /usr/include/arm-linux-gnueabihf/qt5/QtWidgets/QMainWindow:1,
>  from qv4l2.h:25,
>  from ctrl-tab.cpp:20:
> /usr/include/c++/6/bits/stl_map.h: In member function ‘void 
> ApplicationWindow::setWhat(QWidget*, unsigned int, const QString&)’:
> /usr/include/c++/6/bits/stl_map.h:483:4: note: parameter passing for argument 
> of type ‘std::_Rb_tree v4l2_query_ext_ctrl>, std::_Select1st >, std::less, 
> std::allocator 
> >::const_iterator {aka std::_Rb_tree_const_iterator >}’ will change in GCC 7.1
> __i = _M_t._M_emplace_hint_unique(__i, std::piecewise_construct,
> ^~~
> 

Re: Please help test the new v4l-subdev support in v4l2-compliance

[Tim Harvey]

On Mon, Feb 5, 2018 at 8:27 AM, Hans Verkuil  wrote:
> On 02/05/2018 05:21 PM, Tim Harvey wrote:
>
> 
>
>>
>> I ran a 'make distclean; ./bootstrap.sh && ./configure && make'
>>
>> last version I built successfully was '1bb8c70 v4l2-ctl: mention that
>> --set-subdev-fps is for testing only'
>
> That's a lot of revisions ago. I've been busy last weekend :-)

right... I was up to date Thursday morning! ;)

>
> Do a new git pull and try again. I remember hitting something similar during
> the weekend where I was missing a C++ include.
>

Yes, I tried that on my x86 dev host - same failure as from my target.

>>
>> I haven't dug into the failure at all. Are you using something new
>> with c++ requiring a new lib or specific version of something that
>> needs to be added to configure?
>
> Nope, bog standard C++. Real C++ pros are probably appalled by the code.
>

Google to the rescue: The ifstream constructor expects a const char*,
so you need to do ifstream file(filename.c_str()); to make it work.

the following patch fixes it:

diff --git a/utils/common/media-info.cpp b/utils/common/media-info.cpp
index eef743e..39da9b8 100644
--- a/utils/common/media-info.cpp
+++ b/utils/common/media-info.cpp
@@ -76,7 +76,7 @@ media_type media_detect_type(const char *device)
uevent_path += num2s(major(sb.st_rdev), false) + ":" +
num2s(minor(sb.st_rdev), false) + "/uevent";

-   std::ifstream uevent_file(uevent_path);
+   std::ifstream uevent_file(uevent_path.c_str());
if (uevent_file.fail())
return MEDIA_TYPE_UNKNOWN;

@@ -117,7 +117,7 @@ std::string media_get_device(__u32 major, __u32 minor)
sprintf(fmt, "%d:%d", major, minor);
uevent_path += std::string(fmt) + "/uevent";

-   std::ifstream uevent_file(uevent_path);
+   std::ifstream uevent_file(uevent_path.c_str());
if (uevent_file.fail())
return "";

Tim

Re: Please help test the new v4l-subdev support in v4l2-compliance

[Mauro Carvalho Chehab]

Em Mon, 5 Feb 2018 08:21:54 -0800
Tim Harvey  escreveu:

> Hans,
> 
> I'm failing compile (of master 4ee9911) with:
> 
>   CXX  v4l2_compliance-media-info.o
> media-info.cpp: In function ‘media_type media_detect_type(const char*)’:
> media-info.cpp:79:39: error: no matching function for call to
> ‘std::basic_ifstream::basic_ifstream(std::__cxx11::string&)’
>   std::ifstream uevent_file(uevent_path);
>^

Btw, while on it, a few days ago, I noticed several class warnings when
building v4l-utils on Raspbian, saying that it was using some features
that future versions of gcc would stop using at qv4l2. See enclosed.

I didn't have time to look on them.

Thanks,
Mauro

In file included from /usr/include/c++/6/map:60:0,
 from 
/usr/include/arm-linux-gnueabihf/qt5/QtCore/qmetatype.h:57,
 from /usr/include/arm-linux-gnueabihf/qt5/QtCore/qobject.h:54,
 from 
/usr/include/arm-linux-gnueabihf/qt5/QtWidgets/qwidget.h:44,
 from 
/usr/include/arm-linux-gnueabihf/qt5/QtWidgets/qmainwindow.h:43,
 from 
/usr/include/arm-linux-gnueabihf/qt5/QtWidgets/QMainWindow:1,
 from qv4l2.h:25,
 from ctrl-tab.cpp:20:
/usr/include/c++/6/bits/stl_tree.h: In member function 
‘std::pair 
std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, 
_Alloc>::_M_get_insert_hint_unique_pos(std::_Rb_tree<_Key, _Val, _KeyOfValue, 
_Compare, _Alloc>::const_iterator, const key_type&) [with _Key = unsigned int; 
_Val = std::pair; _KeyOfValue = 
std::_Select1st; _Compare 
= std::less; _Alloc = std::allocator]’:
/usr/include/c++/6/bits/stl_tree.h:1928:5: note: parameter passing for argument 
of type ‘std::_Rb_tree, std::_Select1st, std::less, std::allocator >::const_iterator {aka 
std::_Rb_tree_const_iterator}’ will change in GCC 7.1
 _Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::
 ^~~
/usr/include/c++/6/bits/stl_tree.h: In function ‘std::_Rb_tree<_Key, _Val, 
_KeyOfValue, _Compare, _Alloc>::iterator std::_Rb_tree<_Key, _Val, _KeyOfValue, 
_Compare, _Alloc>::_M_emplace_hint_unique(std::_Rb_tree<_Key, _Val, 
_KeyOfValue, _Compare, _Alloc>::const_iterator, _Args&& ...) [with _Args = 
{const std::piecewise_construct_t&, std::tuple, 
std::tuple<>}; _Key = unsigned int; _Val = std::pair; _KeyOfValue = std::_Select1st; _Compare = std::less; _Alloc = 
std::allocator]’:
/usr/include/c++/6/bits/stl_tree.h:2193:7: note: parameter passing for argument 
of type ‘std::_Rb_tree, std::_Select1st, std::less, std::allocator >::const_iterator {aka 
std::_Rb_tree_const_iterator}’ will change in GCC 7.1
   _Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::
   ^~~
In file included from /usr/include/c++/6/map:61:0,
 from 
/usr/include/arm-linux-gnueabihf/qt5/QtCore/qmetatype.h:57,
 from /usr/include/arm-linux-gnueabihf/qt5/QtCore/qobject.h:54,
 from 
/usr/include/arm-linux-gnueabihf/qt5/QtWidgets/qwidget.h:44,
 from 
/usr/include/arm-linux-gnueabihf/qt5/QtWidgets/qmainwindow.h:43,
 from 
/usr/include/arm-linux-gnueabihf/qt5/QtWidgets/QMainWindow:1,
 from qv4l2.h:25,
 from ctrl-tab.cpp:20:
/usr/include/c++/6/bits/stl_map.h: In member function ‘void 
ApplicationWindow::setWhat(QWidget*, unsigned int, const QString&)’:
/usr/include/c++/6/bits/stl_map.h:483:4: note: parameter passing for argument 
of type ‘std::_Rb_tree, std::_Select1st, std::less, std::allocator >::const_iterator {aka 
std::_Rb_tree_const_iterator}’ will change in GCC 7.1
__i = _M_t._M_emplace_hint_unique(__i, std::piecewise_construct,
^~~
/usr/include/c++/6/bits/stl_map.h: In member function ‘void 
ApplicationWindow::setWhat(QWidget*, unsigned int, long long int)’:
/usr/include/c++/6/bits/stl_map.h:483:4: note: parameter passing for argument 
of type ‘std::_Rb_tree, std::_Select1st, std::less, std::allocator >::const_iterator {aka 
std::_Rb_tree_const_iterator}’ will change in GCC 7.1
__i = _M_t._M_emplace_hint_unique(__i, std::piecewise_construct,
^~~
In file included from /usr/include/c++/6/map:60:0,
 from 
/usr/include/arm-linux-gnueabihf/qt5/QtCore/qmetatype.h:57,
 from /usr/include/arm-linux-gnueabihf/qt5/QtCore/qobject.h:54,
 from 
/usr/include/arm-linux-gnueabihf/qt5/QtWidgets/qwidget.h:44,
 from 
/usr/include/arm-linux-gnueabihf/qt5/QtWidgets/qmainwindow.h:43,
 from 
/usr/include/arm-linux-gnueabihf/qt5/QtWidgets/QMainWindow:1,
 from qv4l2.h:25,
   

Re: media_device.c question: can this workaround be removed?

[Mauro Carvalho Chehab]

Em Mon, 5 Feb 2018 16:19:28 +0100
Hans Verkuil  escreveu:

> On 02/05/2018 03:30 PM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Mon, Feb 05, 2018 at 01:30:04PM +0100, Hans Verkuil wrote:  
> >> On 02/05/2018 12:59 PM, Sakari Ailus wrote:  
> >>> Hi Hans,
> >>>
> >>> On Mon, Feb 05, 2018 at 11:26:47AM +0100, Hans Verkuil wrote:  
>  The function media_device_enum_entities() has this workaround:
> 
>  /*
>   * Workaround for a bug at media-ctl <= v1.10 that makes it to
>   * do the wrong thing if the entity function doesn't belong to
>   * either MEDIA_ENT_F_OLD_BASE or MEDIA_ENT_F_OLD_SUBDEV_BASE
>   * Ranges.
>   *
>   * Non-subdevices are expected to be at the MEDIA_ENT_F_OLD_BASE,
>   * or, otherwise, will be silently ignored by media-ctl when
>   * printing the graphviz diagram. So, map them into the devnode
>   * old range.
>   */
>  if (ent->function < MEDIA_ENT_F_OLD_BASE ||
>  ent->function > MEDIA_ENT_F_TUNER) {
>  if (is_media_entity_v4l2_subdev(ent))
>  entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
>  else if (ent->function != MEDIA_ENT_F_IO_V4L)
>  entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
>  }
> 
>  But this means that the entity type returned by ENUM_ENTITIES just 
>  overwrites
>  perfectly fine types by bogus values and thus the returned value differs
>  from that returned by G_TOPOLOGY.
> 
>  Can we please, please remove this workaround? I have no idea why a 
>  workaround
>  for media-ctl of all things ever made it in the kernel.  
> >>>
> >>> The entity types were replaced by entity functions back in 2015 with the
> >>> big Media controller reshuffle. While I agree functions are better for
> >>> describing entities than types (and those types had Linux specific
> >>> interfaces in them), the new function-based API still may support a single
> >>> value, i.e. a single function per device.
> >>>
> >>> This also created two different name spaces for describing entities: the
> >>> old types used by the MC API and the new functions used by MC v2 API.
> >>>
> >>> This doesn't go well with the fact that, as you noticed, the pad
> >>> information is not available through MC v2 API. The pad information is
> >>> required by the user space so software continues to use the original MC
> >>> API.
> >>>
> >>> I don't think there's a way to avoid maintaining two name spaces (types 
> >>> and
> >>> functions) without breaking at least one of the APIs.  
> >>
> >> The comment specifically claims that this workaround is for media-ctl and
> >> it suggests that it is fixed after v1.10. Is that comment bogus? I can't
> >> really tell which commit fixed media-ctl. Does anyone know?
> >>
> >> As far as I can tell the function defines have been chosen in such a way 
> >> that
> >> they will equally well work with the old name space. There should be no
> >> problem there whatsoever and media-ctl should switch to use the new 
> >> defines.  
> > 
> > The old interface (type) was centered around the uAPI for the entity.
> > That's no longer the case for functions. The entity type
> > (MEDIA_ENT_TYPE_MASK) tells the uAPI which affects the interpretation of
> > the dev union in struct media_entity_struct as well as the interface that
> > device node implements. With the new function field that's no longer the
> > case.
> > 
> > Also, the new MC v2 API makes a separation between the entity function and
> > the uAPI (interface) which was lacking in the old API.
> >   
> >>
> >> We now have a broken ENUM_ENTITIES ioctl (it rudely overwrites VBI/DVB/etc 
> >> types)
> >> and a broken G_TOPOLOGY ioctl (no pad index).
> >>
> >> This sucks. Let's fix both so that they at least report consistent 
> >> information.  
> > 
> > The existing user space may assume that the type field of the entity
> > conveys that the entity does provide a V4L2 sub-device interface if that's
> > the case actually.
> > 
> > This is what media-ctl does and I presume if existing user space checks for
> > the type field, it may well have similar checks: it was how the API was
> > defined. Therefore it's not entirely accurate to say that only media-ctl
> > has this "bug", I'd generally assume programs that use MC (v1) API do this.
> > 
> > You could argue about the merits (or lack of them) of the old API, no
> > disagrement there.  
> 
> The old API is already broken. E.g. using MEDIA_ENT_F_PROC_VIDEO_SCALER in
> vimc/vimc-scaler.c (instead of the current - and bogus - 
> MEDIA_ENT_F_ATV_DECODER)
> gives me this with media-ctl:
> 
> - entity 21: Scaler (2 pads, 4 links)
>  type V4L2 subdev subtype Unknown flags 0
>  device node name /dev/v4l-subdev5
> pad0: Sink
> 

Re: Please help test the new v4l-subdev support in v4l2-compliance

[Hans Verkuil]

On 02/05/2018 05:21 PM, Tim Harvey wrote:



> 
> I ran a 'make distclean; ./bootstrap.sh && ./configure && make'
> 
> last version I built successfully was '1bb8c70 v4l2-ctl: mention that
> --set-subdev-fps is for testing only'

That's a lot of revisions ago. I've been busy last weekend :-)

Do a new git pull and try again. I remember hitting something similar during
the weekend where I was missing a C++ include.

> 
> I haven't dug into the failure at all. Are you using something new
> with c++ requiring a new lib or specific version of something that
> needs to be added to configure?

Nope, bog standard C++. Real C++ pros are probably appalled by the code.

Regards,

Hans

Re: Please help test the new v4l-subdev support in v4l2-compliance

[Tim Harvey]

On Sat, Feb 3, 2018 at 7:56 AM, Hans Verkuil  wrote:
> Hi Tim, Jacopo,
>
> I have now finished writing the v4l2-compliance tests for the various 
> v4l-subdev
> ioctls. I managed to test some with the vimc driver, but that doesn't 
> implement all
> ioctls, so I could use some help testing my test code :-)
>
> To test you first need to apply these patches to your kernel:
>
> https://patchwork.linuxtv.org/patch/46817/
> https://patchwork.linuxtv.org/patch/46822/
>
> Otherwise the compliance test will fail a lot.
>
> Now run v4l2-compliance -u /dev/v4l-subdevX (or -uX as a shortcut) and see 
> what
> happens.
>
> I have tested the following ioctls with vimc, so they are likely to be 
> correct:
>
> #define VIDIOC_SUBDEV_G_FMT _IOWR('V',  4, struct 
> v4l2_subdev_format)
> #define VIDIOC_SUBDEV_S_FMT _IOWR('V',  5, struct 
> v4l2_subdev_format)
> #define VIDIOC_SUBDEV_ENUM_MBUS_CODE_IOWR('V',  2, struct 
> v4l2_subdev_mbus_code_enum)
> #define VIDIOC_SUBDEV_ENUM_FRAME_SIZE   _IOWR('V', 74, struct 
> v4l2_subdev_frame_size_enum)
>
> All others are untested:
>
> #define VIDIOC_SUBDEV_G_FRAME_INTERVAL  _IOWR('V', 21, struct 
> v4l2_subdev_frame_interval)
> #define VIDIOC_SUBDEV_S_FRAME_INTERVAL  _IOWR('V', 22, struct 
> v4l2_subdev_frame_interval)
> #define VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL   _IOWR('V', 75, struct 
> v4l2_subdev_frame_interval_enum)
> #define VIDIOC_SUBDEV_G_CROP_IOWR('V', 59, struct 
> v4l2_subdev_crop)
> #define VIDIOC_SUBDEV_S_CROP_IOWR('V', 60, struct 
> v4l2_subdev_crop)
> #define VIDIOC_SUBDEV_G_SELECTION   _IOWR('V', 61, struct 
> v4l2_subdev_selection)
> #define VIDIOC_SUBDEV_S_SELECTION   _IOWR('V', 62, struct 
> v4l2_subdev_selection)
> #define VIDIOC_SUBDEV_G_EDID_IOWR('V', 40, struct 
> v4l2_edid)
> #define VIDIOC_SUBDEV_S_EDID_IOWR('V', 41, struct 
> v4l2_edid)
> #define VIDIOC_SUBDEV_S_DV_TIMINGS  _IOWR('V', 87, struct 
> v4l2_dv_timings)
> #define VIDIOC_SUBDEV_G_DV_TIMINGS  _IOWR('V', 88, struct 
> v4l2_dv_timings)
> #define VIDIOC_SUBDEV_ENUM_DV_TIMINGS   _IOWR('V', 98, struct 
> v4l2_enum_dv_timings)
> #define VIDIOC_SUBDEV_QUERY_DV_TIMINGS  _IOR('V', 99, struct 
> v4l2_dv_timings)
> #define VIDIOC_SUBDEV_DV_TIMINGS_CAP_IOWR('V', 100, struct 
> v4l2_dv_timings_cap)
>
> I did the best I could, but there may very well be bugs in the test code.
>
> I will also test the timings and edid ioctls myself later next week at work.
>
> The v4l2-compliance utility can now also test media devices (-m option), 
> although that's
> early days yet. Eventually I want to be able to walk the graph and test each 
> device in
> turn.
>
> I have this idea of making v4l2-compliance, cec-compliance and 
> media-compliance
> frontends that can all share the actual test code. And perhaps that can 
> include a new
> dvb-compliance as well.
>
> However, that's future music, for now I just want to get proper ioctl test 
> coverage
> so driver authors can at least have some confidence in their code by running 
> these
> tests.
>

Hans,

I'm failing compile (of master 4ee9911) with:

  CXX  v4l2_compliance-media-info.o
media-info.cpp: In function ‘media_type media_detect_type(const char*)’:
media-info.cpp:79:39: error: no matching function for call to
‘std::basic_ifstream::basic_ifstream(std::__cxx11::string&)’
  std::ifstream uevent_file(uevent_path);
   ^
In file included from media-info.cpp:35:0:
/usr/include/c++/5/fstream:495:7: note: candidate:
std::basic_ifstream<_CharT, _Traits>::basic_ifstream(const char*,
std::ios_base::openmode) [with _CharT = char; _Traits =
std::char_traits; std::ios_base::openmode = std::_Ios_Openmode]
   basic_ifstream(const char* __s, ios_base::openmode __mode = ios_base::in)
   ^
/usr/include/c++/5/fstream:495:7: note:   no known conversion for
argument 1 from ‘std::__cxx11::string {aka
std::__cxx11::basic_string}’ to ‘const char*’
In file included from media-info.cpp:35:0:
/usr/include/c++/5/fstream:481:7: note: candidate:
std::basic_ifstream<_CharT, _Traits>::basic_ifstream() [with _CharT =
char; _Traits = std::char_traits]
   basic_ifstream() : __istream_type(), _M_filebuf()
   ^
/usr/include/c++/5/fstream:481:7: note:   candidate expects 0
arguments, 1 provided
In file included from media-info.cpp:35:0:
/usr/include/c++/5/fstream:455:11: note: candidate:
std::basic_ifstream::basic_ifstream(const
std::basic_ifstream&)
 class basic_ifstream : public basic_istream<_CharT, _Traits>
   ^
/usr/include/c++/5/fstream:455:11: note:   no known conversion for
argument 1 from ‘std::__cxx11::string {aka
std::__cxx11::basic_string}’ to ‘const
std::basic_ifstream&’
media-info.cpp: In function ‘std::__cxx11::string
media_get_device(__u32, __u32)’:

Re: media_device.c question: can this workaround be removed?

[Hans Verkuil]

On 02/05/2018 03:30 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Feb 05, 2018 at 01:30:04PM +0100, Hans Verkuil wrote:
>> On 02/05/2018 12:59 PM, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Mon, Feb 05, 2018 at 11:26:47AM +0100, Hans Verkuil wrote:
 The function media_device_enum_entities() has this workaround:

 /*
  * Workaround for a bug at media-ctl <= v1.10 that makes it to
  * do the wrong thing if the entity function doesn't belong to
  * either MEDIA_ENT_F_OLD_BASE or MEDIA_ENT_F_OLD_SUBDEV_BASE
  * Ranges.
  *
  * Non-subdevices are expected to be at the MEDIA_ENT_F_OLD_BASE,
  * or, otherwise, will be silently ignored by media-ctl when
  * printing the graphviz diagram. So, map them into the devnode
  * old range.
  */
 if (ent->function < MEDIA_ENT_F_OLD_BASE ||
 ent->function > MEDIA_ENT_F_TUNER) {
 if (is_media_entity_v4l2_subdev(ent))
 entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
 else if (ent->function != MEDIA_ENT_F_IO_V4L)
 entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
 }

 But this means that the entity type returned by ENUM_ENTITIES just 
 overwrites
 perfectly fine types by bogus values and thus the returned value differs
 from that returned by G_TOPOLOGY.

 Can we please, please remove this workaround? I have no idea why a 
 workaround
 for media-ctl of all things ever made it in the kernel.
>>>
>>> The entity types were replaced by entity functions back in 2015 with the
>>> big Media controller reshuffle. While I agree functions are better for
>>> describing entities than types (and those types had Linux specific
>>> interfaces in them), the new function-based API still may support a single
>>> value, i.e. a single function per device.
>>>
>>> This also created two different name spaces for describing entities: the
>>> old types used by the MC API and the new functions used by MC v2 API.
>>>
>>> This doesn't go well with the fact that, as you noticed, the pad
>>> information is not available through MC v2 API. The pad information is
>>> required by the user space so software continues to use the original MC
>>> API.
>>>
>>> I don't think there's a way to avoid maintaining two name spaces (types and
>>> functions) without breaking at least one of the APIs.
>>
>> The comment specifically claims that this workaround is for media-ctl and
>> it suggests that it is fixed after v1.10. Is that comment bogus? I can't
>> really tell which commit fixed media-ctl. Does anyone know?
>>
>> As far as I can tell the function defines have been chosen in such a way that
>> they will equally well work with the old name space. There should be no
>> problem there whatsoever and media-ctl should switch to use the new defines.
> 
> The old interface (type) was centered around the uAPI for the entity.
> That's no longer the case for functions. The entity type
> (MEDIA_ENT_TYPE_MASK) tells the uAPI which affects the interpretation of
> the dev union in struct media_entity_struct as well as the interface that
> device node implements. With the new function field that's no longer the
> case.
> 
> Also, the new MC v2 API makes a separation between the entity function and
> the uAPI (interface) which was lacking in the old API.
> 
>>
>> We now have a broken ENUM_ENTITIES ioctl (it rudely overwrites VBI/DVB/etc 
>> types)
>> and a broken G_TOPOLOGY ioctl (no pad index).
>>
>> This sucks. Let's fix both so that they at least report consistent 
>> information.
> 
> The existing user space may assume that the type field of the entity
> conveys that the entity does provide a V4L2 sub-device interface if that's
> the case actually.
> 
> This is what media-ctl does and I presume if existing user space checks for
> the type field, it may well have similar checks: it was how the API was
> defined. Therefore it's not entirely accurate to say that only media-ctl
> has this "bug", I'd generally assume programs that use MC (v1) API do this.
> 
> You could argue about the merits (or lack of them) of the old API, no
> disagrement there.

The old API is already broken. E.g. using MEDIA_ENT_F_PROC_VIDEO_SCALER in
vimc/vimc-scaler.c (instead of the current - and bogus - 
MEDIA_ENT_F_ATV_DECODER)
gives me this with media-ctl:

- entity 21: Scaler (2 pads, 4 links)
 type V4L2 subdev subtype Unknown flags 0
 device node name /dev/v4l-subdev5
pad0: Sink
[fmt:RGB888_1X24/640x480 field:none]
<- "Debayer A":1 [ENABLED]
<- "Debayer B":1 []
<- "RGB/YUV Input":0 []
pad1: Source
[fmt:RGB888_1X24/1920x1440 field:none]
-> "RGB/YUV Capture":0 [ENABLED,IMMUTABLE]

Useless. We now have an old API that 

Re: [PATCH 5/5] add default control values as module parameters

[Hans Verkuil]

On 02/05/2018 03:29 PM, Florian Echtler wrote:
> Signed-off-by: Florian Echtler 

Please add a change log when you make a patch.

I for one would like to know why this has to be supplied as a module option.
Some documentation in the code would be helpful as well (e.g. I have no idea
what a 'vsvideo' is).

Regards,

Hans

> ---
>  drivers/input/touchscreen/sur40.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/sur40.c 
> b/drivers/input/touchscreen/sur40.c
> index c4b7cf1..d612f3f 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
> @@ -173,6 +173,14 @@ int sur40_v4l2_contrast   = SUR40_CONTRAST_DEF;   /* 
> blacklevel   */
>  int sur40_v4l2_gain   = SUR40_GAIN_DEF;   /* gain */
>  int sur40_v4l2_backlight  = 1;/* preprocessor */
>  
> +/* module parameters */
> +static uint irlevel = SUR40_BRIGHTNESS_DEF;
> +module_param(irlevel, uint, 0644);
> +MODULE_PARM_DESC(irlevel, "set default irlevel");
> +static uint vsvideo = SUR40_VSVIDEO_DEF;
> +module_param(vsvideo, uint, 0644);
> +MODULE_PARM_DESC(vsvideo, "set default vsvideo");
> +
>  static const struct v4l2_pix_format sur40_pix_format[] = {
>   {
>   .pixelformat = V4L2_TCH_FMT_TU08,
> @@ -372,6 +380,11 @@ static void sur40_open(struct input_polled_dev *polldev)
>  
>   dev_dbg(sur40->dev, "open\n");
>   sur40_init(sur40);
> +
> + /* set default values */
> + sur40_set_irlevel(sur40, irlevel);
> + sur40_set_vsvideo(sur40, vsvideo);
> + sur40_set_preprocessor(sur40, SUR40_BACKLIGHT_DEF);
>  }
>  
>  /* Disable device, polling has stopped. */
> 

Re: [PATCH 4/5] add V4L2 control functions

[Hans Verkuil]

On 02/05/2018 03:29 PM, Florian Echtler wrote:
> Signed-off-by: Florian Echtler 
> ---
>  drivers/input/touchscreen/sur40.c | 114 
> ++
>  1 file changed, 114 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/sur40.c 
> b/drivers/input/touchscreen/sur40.c
> index 63c7264b..c4b7cf1 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
> @@ -953,6 +953,119 @@ static int sur40_vidioc_g_fmt(struct file *file, void 
> *priv,
>   return 0;
>  }
>  
> +
> +static int sur40_vidioc_queryctrl(struct file *file, void *fh,
> +struct v4l2_queryctrl *qc)
> +{
> +
> + switch (qc->id) {
> + case V4L2_CID_BRIGHTNESS:
> + qc->flags = 0;
> + sprintf(qc->name, "Brightness");
> + qc->type = V4L2_CTRL_TYPE_INTEGER;
> + qc->minimum = SUR40_BRIGHTNESS_MIN;
> + qc->default_value = SUR40_BRIGHTNESS_DEF;
> + qc->maximum = SUR40_BRIGHTNESS_MAX;
> + qc->step = 8;
> + return 0;
> + case V4L2_CID_CONTRAST:
> + qc->flags = 0;
> + sprintf(qc->name, "Contrast");
> + qc->type = V4L2_CTRL_TYPE_INTEGER;
> + qc->minimum = SUR40_CONTRAST_MIN;
> + qc->default_value = SUR40_CONTRAST_DEF;
> + qc->maximum = SUR40_CONTRAST_MAX;
> + qc->step = 1;
> + return 0;
> + case V4L2_CID_GAIN:
> + qc->flags = 0;
> + sprintf(qc->name, "Gain");
> + qc->type = V4L2_CTRL_TYPE_INTEGER;
> + qc->minimum = SUR40_GAIN_MIN;
> + qc->default_value = SUR40_GAIN_DEF;
> + qc->maximum = SUR40_GAIN_MAX;
> + qc->step = 1;
> + return 0;
> + case V4L2_CID_BACKLIGHT_COMPENSATION:
> + qc->flags = 0;
> + sprintf(qc->name, "Preprocessor");
> + qc->type = V4L2_CTRL_TYPE_INTEGER;
> + qc->minimum = SUR40_BACKLIGHT_MIN;
> + qc->default_value = SUR40_BACKLIGHT_DEF;
> + qc->maximum = SUR40_BACKLIGHT_MAX;
> + qc->step = 1;
> + return 0;
> + default:
> + qc->flags = V4L2_CTRL_FLAG_DISABLED;
> + return -EINVAL;
> + }
> +}
> +
> +static int sur40_vidioc_g_ctrl(struct file *file, void *fh,
> + struct v4l2_control *ctrl)
> +{
> +
> + switch (ctrl->id) {
> + case V4L2_CID_BRIGHTNESS:
> + ctrl->value = sur40_v4l2_brightness;
> + return 0;
> + case V4L2_CID_CONTRAST:
> + ctrl->value = sur40_v4l2_contrast;
> + return 0;
> + case V4L2_CID_GAIN:
> + ctrl->value = sur40_v4l2_gain;
> + return 0;
> + case V4L2_CID_BACKLIGHT_COMPENSATION:
> + ctrl->value = sur40_v4l2_backlight;
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int sur40_vidioc_s_ctrl(struct file *file, void *fh,
> + struct v4l2_control *ctrl)
> +{
> + u8 value = 0;
> + struct sur40_state *sur40 = video_drvdata(file);
> +
> + switch (ctrl->id) {
> + case V4L2_CID_BRIGHTNESS:
> + sur40_v4l2_brightness = ctrl->value;
> + if (sur40_v4l2_brightness < SUR40_BRIGHTNESS_MIN)
> + sur40_v4l2_brightness = SUR40_BRIGHTNESS_MIN;
> + else if (sur40_v4l2_brightness > SUR40_BRIGHTNESS_MAX)
> + sur40_v4l2_brightness = SUR40_BRIGHTNESS_MAX;
> + sur40_set_irlevel(sur40, sur40_v4l2_brightness);
> + return 0;
> + case V4L2_CID_CONTRAST:
> + sur40_v4l2_contrast = ctrl->value;
> + if (sur40_v4l2_contrast < SUR40_CONTRAST_MIN)
> + sur40_v4l2_contrast = SUR40_CONTRAST_MIN;
> + else if (sur40_v4l2_contrast > SUR40_CONTRAST_MAX)
> + sur40_v4l2_contrast = SUR40_CONTRAST_MAX;
> + value = (sur40_v4l2_contrast << 4) + sur40_v4l2_gain;
> + sur40_set_vsvideo(sur40, value);
> + return 0;
> + case V4L2_CID_GAIN:
> + sur40_v4l2_gain = ctrl->value;
> + if (sur40_v4l2_gain < SUR40_GAIN_MIN)
> + sur40_v4l2_gain = SUR40_GAIN_MIN;
> + else if (sur40_v4l2_gain > SUR40_GAIN_MAX)
> + sur40_v4l2_gain = SUR40_GAIN_MAX;
> + value = (sur40_v4l2_contrast << 4) + sur40_v4l2_gain;
> + sur40_set_vsvideo(sur40, value);
> + return 0;
> + case V4L2_CID_BACKLIGHT_COMPENSATION:
> + sur40_v4l2_backlight = ctrl->value;
> + sur40_set_preprocessor(sur40, sur40_v4l2_backlight);
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +
>  static int sur40_ioctl_parm(struct file *file, void *priv,
>   struct 

[PATCH 4/5] add V4L2 control functions

[Florian Echtler]

Signed-off-by: Florian Echtler 
---
 drivers/input/touchscreen/sur40.c | 114 ++
 1 file changed, 114 insertions(+)

diff --git a/drivers/input/touchscreen/sur40.c 
b/drivers/input/touchscreen/sur40.c
index 63c7264b..c4b7cf1 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -953,6 +953,119 @@ static int sur40_vidioc_g_fmt(struct file *file, void 
*priv,
return 0;
 }
 
+
+static int sur40_vidioc_queryctrl(struct file *file, void *fh,
+  struct v4l2_queryctrl *qc)
+{
+
+   switch (qc->id) {
+   case V4L2_CID_BRIGHTNESS:
+   qc->flags = 0;
+   sprintf(qc->name, "Brightness");
+   qc->type = V4L2_CTRL_TYPE_INTEGER;
+   qc->minimum = SUR40_BRIGHTNESS_MIN;
+   qc->default_value = SUR40_BRIGHTNESS_DEF;
+   qc->maximum = SUR40_BRIGHTNESS_MAX;
+   qc->step = 8;
+   return 0;
+   case V4L2_CID_CONTRAST:
+   qc->flags = 0;
+   sprintf(qc->name, "Contrast");
+   qc->type = V4L2_CTRL_TYPE_INTEGER;
+   qc->minimum = SUR40_CONTRAST_MIN;
+   qc->default_value = SUR40_CONTRAST_DEF;
+   qc->maximum = SUR40_CONTRAST_MAX;
+   qc->step = 1;
+   return 0;
+   case V4L2_CID_GAIN:
+   qc->flags = 0;
+   sprintf(qc->name, "Gain");
+   qc->type = V4L2_CTRL_TYPE_INTEGER;
+   qc->minimum = SUR40_GAIN_MIN;
+   qc->default_value = SUR40_GAIN_DEF;
+   qc->maximum = SUR40_GAIN_MAX;
+   qc->step = 1;
+   return 0;
+   case V4L2_CID_BACKLIGHT_COMPENSATION:
+   qc->flags = 0;
+   sprintf(qc->name, "Preprocessor");
+   qc->type = V4L2_CTRL_TYPE_INTEGER;
+   qc->minimum = SUR40_BACKLIGHT_MIN;
+   qc->default_value = SUR40_BACKLIGHT_DEF;
+   qc->maximum = SUR40_BACKLIGHT_MAX;
+   qc->step = 1;
+   return 0;
+   default:
+   qc->flags = V4L2_CTRL_FLAG_DISABLED;
+   return -EINVAL;
+   }
+}
+
+static int sur40_vidioc_g_ctrl(struct file *file, void *fh,
+   struct v4l2_control *ctrl)
+{
+
+   switch (ctrl->id) {
+   case V4L2_CID_BRIGHTNESS:
+   ctrl->value = sur40_v4l2_brightness;
+   return 0;
+   case V4L2_CID_CONTRAST:
+   ctrl->value = sur40_v4l2_contrast;
+   return 0;
+   case V4L2_CID_GAIN:
+   ctrl->value = sur40_v4l2_gain;
+   return 0;
+   case V4L2_CID_BACKLIGHT_COMPENSATION:
+   ctrl->value = sur40_v4l2_backlight;
+   return 0;
+   default:
+   return -EINVAL;
+   }
+}
+
+static int sur40_vidioc_s_ctrl(struct file *file, void *fh,
+   struct v4l2_control *ctrl)
+{
+   u8 value = 0;
+   struct sur40_state *sur40 = video_drvdata(file);
+
+   switch (ctrl->id) {
+   case V4L2_CID_BRIGHTNESS:
+   sur40_v4l2_brightness = ctrl->value;
+   if (sur40_v4l2_brightness < SUR40_BRIGHTNESS_MIN)
+   sur40_v4l2_brightness = SUR40_BRIGHTNESS_MIN;
+   else if (sur40_v4l2_brightness > SUR40_BRIGHTNESS_MAX)
+   sur40_v4l2_brightness = SUR40_BRIGHTNESS_MAX;
+   sur40_set_irlevel(sur40, sur40_v4l2_brightness);
+   return 0;
+   case V4L2_CID_CONTRAST:
+   sur40_v4l2_contrast = ctrl->value;
+   if (sur40_v4l2_contrast < SUR40_CONTRAST_MIN)
+   sur40_v4l2_contrast = SUR40_CONTRAST_MIN;
+   else if (sur40_v4l2_contrast > SUR40_CONTRAST_MAX)
+   sur40_v4l2_contrast = SUR40_CONTRAST_MAX;
+   value = (sur40_v4l2_contrast << 4) + sur40_v4l2_gain;
+   sur40_set_vsvideo(sur40, value);
+   return 0;
+   case V4L2_CID_GAIN:
+   sur40_v4l2_gain = ctrl->value;
+   if (sur40_v4l2_gain < SUR40_GAIN_MIN)
+   sur40_v4l2_gain = SUR40_GAIN_MIN;
+   else if (sur40_v4l2_gain > SUR40_GAIN_MAX)
+   sur40_v4l2_gain = SUR40_GAIN_MAX;
+   value = (sur40_v4l2_contrast << 4) + sur40_v4l2_gain;
+   sur40_set_vsvideo(sur40, value);
+   return 0;
+   case V4L2_CID_BACKLIGHT_COMPENSATION:
+   sur40_v4l2_backlight = ctrl->value;
+   sur40_set_preprocessor(sur40, sur40_v4l2_backlight);
+   return 0;
+   default:
+   return -EINVAL;
+   }
+}
+
+
 static int sur40_ioctl_parm(struct file *file, void *priv,
struct v4l2_streamparm *p)
 {
@@ -1071,6 +1181,10 @@ static const struct v4l2_ioctl_ops sur40_video_ioctl_ops 
= {
  

[PATCH 3/5] add video control register handlers

[Florian Echtler]

Signed-off-by: Florian Echtler 
---
 drivers/input/touchscreen/sur40.c | 70 +++
 1 file changed, 70 insertions(+)

diff --git a/drivers/input/touchscreen/sur40.c 
b/drivers/input/touchscreen/sur40.c
index 0dbb004..63c7264b 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -247,6 +255,80 @@ static int sur40_command(struct sur40_state *dev,
   0x00, index, buffer, size, 1000);
 }
 
+/* poke a byte in the panel register space */
+static int sur40_poke(struct sur40_state *dev, u8 offset, u8 value)
+{
+   int result;
+   u8 index = 0x96; // 0xae for permanent write
+
+   result = usb_control_msg(dev->usbdev, usb_sndctrlpipe(dev->usbdev, 0),
+   SUR40_POKE, USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
+   0x32, index, NULL, 0, 1000);
+   if (result < 0)
+   goto error;
+   msleep(5);
+
+   result = usb_control_msg(dev->usbdev, usb_sndctrlpipe(dev->usbdev, 0),
+   SUR40_POKE, USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
+   0x72, offset, NULL, 0, 1000);
+   if (result < 0)
+   goto error;
+   msleep(5);
+
+   result = usb_control_msg(dev->usbdev, usb_sndctrlpipe(dev->usbdev, 0),
+   SUR40_POKE, USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
+   0xb2, value, NULL, 0, 1000);
+   if (result < 0)
+   goto error;
+   msleep(5);
+
+error:
+   return result;
+}
+
+static int sur40_set_preprocessor(struct sur40_state *dev, u8 value)
+{
+   u8 setting_07[2] = { 0x01, 0x00 };
+   u8 setting_17[2] = { 0x85, 0x80 };
+   int result;
+
+   if (value > 1)
+   return -ERANGE;
+
+   result = usb_control_msg(dev->usbdev, usb_sndctrlpipe(dev->usbdev, 0),
+   SUR40_POKE, USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
+   0x07, setting_07[value], NULL, 0, 1000);
+   if (result < 0)
+   goto error;
+   msleep(5);
+
+   result = usb_control_msg(dev->usbdev, usb_sndctrlpipe(dev->usbdev, 0),
+   SUR40_POKE, USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
+   0x17, setting_17[value], NULL, 0, 1000);
+   if (result < 0)
+   goto error;
+   msleep(5);
+
+error:
+   return result;
+}
+
+static void sur40_set_vsvideo(struct sur40_state *handle, u8 value)
+{
+   int i;
+
+   for (i = 0; i < 4; i++)
+   sur40_poke(handle, 0x1c+i, value);
+}
+
+static void sur40_set_irlevel(struct sur40_state *handle, u8 value)
+{
+   int i;
+
+   for (i = 0; i < 8; i++)
+   sur40_poke(handle, 0x08+(2*i), value);
+}
+
 /* Initialization routine, called from sur40_open */
 static int sur40_init(struct sur40_state *dev)
 {
-- 
2.7.4

[PATCH 5/5] add default control values as module parameters

[Florian Echtler]

Signed-off-by: Florian Echtler 
---
 drivers/input/touchscreen/sur40.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/input/touchscreen/sur40.c 
b/drivers/input/touchscreen/sur40.c
index c4b7cf1..d612f3f 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -173,6 +173,14 @@ int sur40_v4l2_contrast   = SUR40_CONTRAST_DEF;   /* 
blacklevel   */
 int sur40_v4l2_gain   = SUR40_GAIN_DEF;   /* gain */
 int sur40_v4l2_backlight  = 1;/* preprocessor */
 
+/* module parameters */
+static uint irlevel = SUR40_BRIGHTNESS_DEF;
+module_param(irlevel, uint, 0644);
+MODULE_PARM_DESC(irlevel, "set default irlevel");
+static uint vsvideo = SUR40_VSVIDEO_DEF;
+module_param(vsvideo, uint, 0644);
+MODULE_PARM_DESC(vsvideo, "set default vsvideo");
+
 static const struct v4l2_pix_format sur40_pix_format[] = {
{
.pixelformat = V4L2_TCH_FMT_TU08,
@@ -372,6 +380,11 @@ static void sur40_open(struct input_polled_dev *polldev)
 
dev_dbg(sur40->dev, "open\n");
sur40_init(sur40);
+
+   /* set default values */
+   sur40_set_irlevel(sur40, irlevel);
+   sur40_set_vsvideo(sur40, vsvideo);
+   sur40_set_preprocessor(sur40, SUR40_BACKLIGHT_DEF);
 }
 
 /* Disable device, polling has stopped. */
-- 
2.7.4

[PATCH 0/5] [RFC] add video controls to SUR40 driver

[Florian Echtler]

The SUR40 (aka Pixelsense) has internal registers that expose sensor
parameters such as brightness, gain etc. This patch creates V4L2
control items and maps them to the appropriate parameters.

This is an initial submission for review, comments welcome!

Best regards, Florian

[PATCH 1/5] add missing blob structure tag field

[Florian Echtler]

Signed-off-by: Florian Echtler 
---
 drivers/input/touchscreen/sur40.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/sur40.c 
b/drivers/input/touchscreen/sur40.c
index f16f835..8375b06 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -81,7 +81,10 @@ struct sur40_blob {
 
__le32 area;   /* size in pixels/pressure (?) */
 
-   u8 padding[32];
+   u8 padding[24];
+
+   __le32 tag_id; /* valid when type == 0x04 (SUR40_TAG) */
+   __le32 unknown;
 
 } __packed;
 
-- 
2.7.4

[PATCH 2/5] add video control register definitions

[Florian Echtler]

Signed-off-by: Florian Echtler 
---
 drivers/input/touchscreen/sur40.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/drivers/input/touchscreen/sur40.c 
b/drivers/input/touchscreen/sur40.c
index 8375b06..0dbb004 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -149,6 +149,30 @@ struct sur40_image_header {
 #define SUR40_TOUCH0x02
 #define SUR40_TAG  0x04
 
+/* video controls */
+#define SUR40_BRIGHTNESS_MAX 0xFF
+#define SUR40_BRIGHTNESS_MIN 0x00
+#define SUR40_BRIGHTNESS_DEF 0xFF
+
+#define SUR40_CONTRAST_MAX 0x0F
+#define SUR40_CONTRAST_MIN 0x00
+#define SUR40_CONTRAST_DEF 0x0A
+
+#define SUR40_GAIN_MAX 0x09
+#define SUR40_GAIN_MIN 0x00
+#define SUR40_GAIN_DEF 0x08
+
+#define SUR40_VSVIDEO_DEF 0x86
+
+#define SUR40_BACKLIGHT_MAX 0x01
+#define SUR40_BACKLIGHT_MIN 0x00
+#define SUR40_BACKLIGHT_DEF 0x01
+
+int sur40_v4l2_brightness = SUR40_BRIGHTNESS_DEF; /* infrared */
+int sur40_v4l2_contrast   = SUR40_CONTRAST_DEF;   /* blacklevel   */
+int sur40_v4l2_gain   = SUR40_GAIN_DEF;   /* gain */
+int sur40_v4l2_backlight  = 1;/* preprocessor */
+
 static const struct v4l2_pix_format sur40_pix_format[] = {
{
.pixelformat = V4L2_TCH_FMT_TU08,
-- 
2.7.4

Re: [PATCH 1/1] vb2: core: Finish buffers at the end of the stream

[Devin Heitmueller]

Hi Sakari,

I tried this patch, and I no longer see the messages in dmesg output
when closing the V4L2 device node.

Tested-by: Devin Heitmueller 

Thanks,

Devin

On Fri, Feb 2, 2018 at 8:57 AM, Devin Heitmueller
 wrote:
> Hello Sakari,
>
> Thanks for proposing this patch.  I'll give it a try this weekend.
>
> Regards,
>
> Devin
>
> On Fri, Feb 2, 2018 at 5:08 AM, Sakari Ailus
>  wrote:
>> If buffers were prepared or queued and the buffers were released without
>> starting the queue, the finish mem op (corresponding to the prepare mem
>> op) was never called to the buffers.
>>
>> Before commit a136f59c0a1f there was no need to do this as in such a case
>> the prepare mem op had not been called yet. Address the problem by
>> explicitly calling finish mem op when the queue is stopped if the buffer
>> is in either prepared or queued state.
>>
>> Fixes: a136f59c0a1f ("[media] vb2: Move buffer cache synchronisation to 
>> prepare from queue")
>> Cc: sta...@vger.kernel.org # for v4.13 and up
>> Signed-off-by: Sakari Ailus 
>> ---
>> Hi Devin,
>>
>> Could you check whether this will resolve the problem you've found?
>>
>> Thanks.
>>
>>  drivers/media/common/videobuf2/videobuf2-core.c | 9 +
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
>> b/drivers/media/common/videobuf2/videobuf2-core.c
>> index f7109f827f6e..52a7c1d0a79a 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1696,6 +1696,15 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>> for (i = 0; i < q->num_buffers; ++i) {
>> struct vb2_buffer *vb = q->bufs[i];
>>
>> +   if (vb->state == VB2_BUF_STATE_PREPARED ||
>> +   vb->state == VB2_BUF_STATE_QUEUED) {
>> +   unsigned int plane;
>> +
>> +   for (plane = 0; plane < vb->num_planes; ++plane)
>> +   call_void_memop(vb, finish,
>> +   vb->planes[plane].mem_priv);
>> +   }
>> +
>> if (vb->state != VB2_BUF_STATE_DEQUEUED) {
>> vb->state = VB2_BUF_STATE_PREPARED;
>> call_void_vb_qop(vb, buf_finish, vb);
>> --
>> 2.11.0
>>
>
>
>
> --
> Devin J. Heitmueller - Kernel Labs
> http://www.kernellabs.com



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

Re: media_device.c question: can this workaround be removed?

[Sakari Ailus]

Hi Hans,

On Mon, Feb 05, 2018 at 01:30:04PM +0100, Hans Verkuil wrote:
> On 02/05/2018 12:59 PM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Mon, Feb 05, 2018 at 11:26:47AM +0100, Hans Verkuil wrote:
> >> The function media_device_enum_entities() has this workaround:
> >>
> >> /*
> >>  * Workaround for a bug at media-ctl <= v1.10 that makes it to
> >>  * do the wrong thing if the entity function doesn't belong to
> >>  * either MEDIA_ENT_F_OLD_BASE or MEDIA_ENT_F_OLD_SUBDEV_BASE
> >>  * Ranges.
> >>  *
> >>  * Non-subdevices are expected to be at the MEDIA_ENT_F_OLD_BASE,
> >>  * or, otherwise, will be silently ignored by media-ctl when
> >>  * printing the graphviz diagram. So, map them into the devnode
> >>  * old range.
> >>  */
> >> if (ent->function < MEDIA_ENT_F_OLD_BASE ||
> >> ent->function > MEDIA_ENT_F_TUNER) {
> >> if (is_media_entity_v4l2_subdev(ent))
> >> entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
> >> else if (ent->function != MEDIA_ENT_F_IO_V4L)
> >> entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
> >> }
> >>
> >> But this means that the entity type returned by ENUM_ENTITIES just 
> >> overwrites
> >> perfectly fine types by bogus values and thus the returned value differs
> >> from that returned by G_TOPOLOGY.
> >>
> >> Can we please, please remove this workaround? I have no idea why a 
> >> workaround
> >> for media-ctl of all things ever made it in the kernel.
> > 
> > The entity types were replaced by entity functions back in 2015 with the
> > big Media controller reshuffle. While I agree functions are better for
> > describing entities than types (and those types had Linux specific
> > interfaces in them), the new function-based API still may support a single
> > value, i.e. a single function per device.
> > 
> > This also created two different name spaces for describing entities: the
> > old types used by the MC API and the new functions used by MC v2 API.
> > 
> > This doesn't go well with the fact that, as you noticed, the pad
> > information is not available through MC v2 API. The pad information is
> > required by the user space so software continues to use the original MC
> > API.
> > 
> > I don't think there's a way to avoid maintaining two name spaces (types and
> > functions) without breaking at least one of the APIs.
> 
> The comment specifically claims that this workaround is for media-ctl and
> it suggests that it is fixed after v1.10. Is that comment bogus? I can't
> really tell which commit fixed media-ctl. Does anyone know?
> 
> As far as I can tell the function defines have been chosen in such a way that
> they will equally well work with the old name space. There should be no
> problem there whatsoever and media-ctl should switch to use the new defines.

The old interface (type) was centered around the uAPI for the entity.
That's no longer the case for functions. The entity type
(MEDIA_ENT_TYPE_MASK) tells the uAPI which affects the interpretation of
the dev union in struct media_entity_struct as well as the interface that
device node implements. With the new function field that's no longer the
case.

Also, the new MC v2 API makes a separation between the entity function and
the uAPI (interface) which was lacking in the old API.

> 
> We now have a broken ENUM_ENTITIES ioctl (it rudely overwrites VBI/DVB/etc 
> types)
> and a broken G_TOPOLOGY ioctl (no pad index).
> 
> This sucks. Let's fix both so that they at least report consistent 
> information.

The existing user space may assume that the type field of the entity
conveys that the entity does provide a V4L2 sub-device interface if that's
the case actually.

This is what media-ctl does and I presume if existing user space checks for
the type field, it may well have similar checks: it was how the API was
defined. Therefore it's not entirely accurate to say that only media-ctl
has this "bug", I'd generally assume programs that use MC (v1) API do this.

You could argue about the merits (or lack of them) of the old API, no
disagrement there.

> 
> > 
> >>
> >> I'm adding media support to the vivid driver and because of this media-ctl 
> >> -p
> >> gives me this:
> >>
> >> Device topology
> >> - entity 1: vivid-000-vid-cap (1 pad, 0 link)
> >> type Node subtype V4L flags 0
> >> device node name /dev/video0
> >> pad0: Source
> >>
> >> - entity 5: vivid-000-vid-out (1 pad, 0 link)
> >> type Node subtype V4L flags 0
> >> device node name /dev/video1
> >> pad0: Sink
> >>
> >> - entity 9: vivid-000-vbi-cap (1 pad, 0 link)
> >> type Unknown subtype Unknown flags 0
> >> pad0: Source
> >>
> >> - entity 13: vivid-000-vbi-out (1 pad, 0 link)
> >>  type Unknown subtype Unknown flags 0
> >> pad0: Sink
> >>
> >> - entity 17: 

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Mauro Carvalho Chehab]

Em Mon, 5 Feb 2018 14:47:51 +0100
Hans Verkuil  escreveu:

> On 02/05/2018 02:17 PM, Mauro Carvalho Chehab wrote:
> > Em Mon, 5 Feb 2018 12:55:21 +0100
> > Hans Verkuil  escreveu:
> >   
> >> On 02/05/2018 12:15 PM, Mauro Carvalho Chehab wrote:  
> >>> Hi Hans,
> >>>
> >>> Em Sun, 4 Feb 2018 14:06:42 +0100
> >>> Hans Verkuil  escreveu:
> >>> 
>  Hi Mauro,
> 
>  I'm working on adding proper compliance tests for the MC but I think 
>  something
>  is missing in the G_TOPOLOGY ioctl w.r.t. pads.
> 
>  In several v4l-subdev ioctls you need to pass the pad. There the pad is 
>  an index
>  for the corresponding entity. I.e. an entity has 3 pads, so the pad 
>  argument is
>  [0-2].
> 
>  The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't 
>  use that
>  in the v4l-subdev ioctls, so how do I translate that to a pad index in 
>  my application?
> 
>  It seems to be a missing feature in the API. I assume this information 
>  is available
>  in the core, so then I would add a field to struct media_v2_pad with the 
>  pad index
>  for the entity.
> >>>
> >>> No, this was designed this way by purpose, back in 2015, when this was
> >>> discussed at the first MC workshop. It was a consensus on that time that
> >>> parameters like PAD index, PAD name, etc should be implemented via the
> >>> properties API, as proposed by Sakari [1]. We also had a few discussions 
> >>> about that on both IRC and ML.
> >>>
> >>> [1] See: https://linuxtv.org/news.php?entry=2015-08-17.mchehab
> >>>
> >>>   3.3 Action items
> >>>   ...
> >>>   media_properties: RFC userspace API: Sakari
> >>>
> >>> Unfortunately, Sakari never found the time to send us a patch series
> >>> implementing a properties API, even as RFC.
> >>>
> >>> One of the other missing features is to add a new version for setting
> >>> links (I guess we called it as S_LINKS_V2 at the meetings there).
> >>> The hole idea is to provide a way to setup the entire pipeline using
> >>> a single ioctl, on an atomic way.
> >>>
> >>> The big problem with pad indexes happen on entities that have PADs with
> >>> different types of signal input or output, like for example a TV tuner.
> >>>
> >>> On almost all PC consumers hardware supported by the Kernel, the same
> >>> PCI/USB ID may come with different types of hardware. This may also
> >>> happen with embedded TV sets, as, depending on the market is is
> >>> sold, the same product may come with a different tuner.
> >>>
> >>> A "classic" TV tuner usually has those types of output signals:
> >>>
> >>>   - analog TV luminance IF;
> >>>   - analog TV chrominance IF [1];
> >>>   - digital TV OFDM IF;
> >>>   - audio IF;
> >>>
> >>> [1] As both luminance and chrominance should go to the same TV
> >>> demod, in practice, we can group both signals together on a
> >>> single pad.
> >>>
> >>> More modern tuners also have an audio demod integrated, meaning that
> >>> they also have another PAD:
> >>>   - digital mono or stereo audio.
> >>>
> >>> At the simplest possible scenario, let's say that we have a TV device
> >>> has those entities (among others), each with a single PAD input:
> >>>
> >>>   - entity #0: a TV tuner;
> >>>   - entity #1: an audio demod;
> >>>   - entity #2: an analog TV demod;
> >>>   - entity #3: a digital TV demod.
> >>>
> >>> And the TV tuner has 4 output pads:
> >>>
> >>>   - pad #0 -> analog TV luminance/chrominance;
> >>>   - pad #1 -> digital TV IF;
> >>>   - pad #2 -> audio IF;
> >>>   - pad #3 -> digital audio.
> >>>
> >>> There, pad #0 can only be connected to entity #2. You cannot
> >>> connect any other pad to entity #2. The same apply to every other
> >>> TV tuner output PAD.
> >>>
> >>> In this specific scenario, entity #1 can only be connected to pad #2,
> >>> and pad #3 can't be connected anywhere, as, on this model opted to
> >>> have a separate audio demod, and didn't wired the digital audio output.
> >>> Yet, the subdev has it.
> >>>
> >>> Another TV set may have different pad numbers - placing them even on a
> >>> different order, and opting to use the audio demod tuner, wiring the
> >>> digital audio output.
> >>>
> >>> Currently, there's no way for an userspace application to know what pads
> >>> can be linked to each entity, as there's no way to tell userspace what
> >>> kind of signal each pad supports.
> >>>
> >>> In any case, the Kernel should either detect the tuner model or know
> >>> it (via a different DT entry), but userspace need such information,
> >>> in order to be able to properly set the pipelines.
> >>>
> >>> So, what we really need here is passing an optional set of properties
> >>> to userspace in order to allow it to do the right thing.
> >>
> >> I fail to see the problem. Entities have pads. Pads have both a unique
> >> ID and an index within the entity pad list. I.e. the pad ID and the
> >> 

Re: [PATCH v5 4/5] media: ov5640: add support of DVP parallel interface

[Maxime Ripard]

On Mon, Feb 05, 2018 at 11:42:11AM +, Fabrizio Castro wrote:
> Hello Maxime,
> 
> thank you for your feedback.
> 
> > > > +/*
> > > > + * configure parallel port control lines polarity
> > > > + *
> > > > + * POLARITY CTRL0
> > > > + * - [5]:PCLK polarity (0: active low, 1: active high)
> > > > + * - [1]:HREF polarity (0: active low, 1: active high)
> > > > + * - [0]:VSYNC polarity (mismatch here between
> > > > + *datasheet and hardware, 0 is active high
> > > > + *and 1 is active low...)
> > >
> > > I know that yourself and Maxime have both confirmed that VSYNC
> > > polarity is inverted, but I am looking at HSYNC and VSYNC with a
> > > logic analyser and I am dumping the values written to
> > > OV5640_REG_POLARITY_CTRL00 and to me it looks like that HSYNC is
> > > active HIGH when hsync_pol == 0, and VSYNC is active HIGH when
> > > vsync_pol == 1.
> >
> > Which mode are you testing this on?
> 
> My testing environment is made of the sensor connected to a SoC with
> 8 data lines, vsync, hsync, and pclk, and of course I am specifying
> hsync-active, vsync-active, and pclk-sample in the device tree on
> both ends so that they configure themselves accordingly to work in
> DVP mode (V4L2_MBUS_PARALLEL), with the correct polarities.
>
> Between the sensor and the SoC there is a noninverting bus
> transceiver (voltage translator), for my experiments I have plugged
> myself onto the outputs of this transceiver only to be compliant
> with the voltage level of my logic analyser.

Sorry, my question was more about the resolution, refresh rate, etc.

> > The non-active periods are insanely high in most modes (1896 for an
> > active horizontal length of 640 in 640x480 for example), especially
> > hsync, and it's really easy to invert the two.
> 
> I am looking at all the data lines, so that I don't confuse the
> non-active periods with the active periods, and with my setup what I
> reported is what I get. I wonder if this difference comes from the
> sensor revision at all? Unfortunately I can only test the one camera
> I have got.

I don't really know then. I've had issues with the polarities on my
side, but it was on the receiver side and the sensor part looked like
what is documented.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH] [BUGFIX] media: vb2: Fix videobuf2 to map correct area

[Masami Hiramatsu]

On Mon, 05 Feb 2018 09:47:46 +0100
Marek Szyprowski  wrote:

> Hi Masami,
> 
> On 2018-02-05 03:30, Masami Hiramatsu wrote:
> > Fixes vb2_vmalloc_get_userptr() to ioremap correct area.
> > Since the current code does ioremap the page address, if the offset > 0,
> > it does not do ioremap the last page and results in kernel panic.
> >
> > This fixes to pass the page address + offset to ioremap so that ioremap
> > can map correct area. Also, this uses __pfn_to_phys() to get the physical
> > address of given PFN.
> >
> > Signed-off-by: Masami Hiramatsu 
> > Reported-by: Takao Orito 
> > ---
> >   drivers/media/v4l2-core/videobuf2-vmalloc.c |2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c 
> > b/drivers/media/v4l2-core/videobuf2-vmalloc.c
> > index 3a7c80cd1a17..896f2f378b40 100644
> > --- a/drivers/media/v4l2-core/videobuf2-vmalloc.c
> > +++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c
> > @@ -106,7 +106,7 @@ static void *vb2_vmalloc_get_userptr(struct device 
> > *dev, unsigned long vaddr,
> > if (nums[i-1] + 1 != nums[i])
> > goto fail_map;
> > buf->vaddr = (__force void *)
> > -   ioremap_nocache(nums[0] << PAGE_SHIFT, size);
> > +   ioremap_nocache(__pfn_to_phys(nums[0]) + offset, size);
> 
> Thanks for reporting this issue. However the above line doesn't look like
> a proper fix. Please note that at the end of that function there is already
> "buf->vaddr += offset;".

I see.

> 
> IMHO the proper fix is to create a larger mapping, which would include the
> in-page start offset:
> 
> ioremap_nocache(__pfn_to_phys(nums[0]), offset + size);

Yes, sorry it's my mistake. I misunderstood ioremap_nocache(addr, size) remaps
the PAGE of addr to PAGE of (addr+size). Yours is correct.

Thank you,

> BTW, thanks for updating "<< PAGE_SHIFT" to better __pfn_to_phys() macro!
> 
> > } else {
> > buf->vaddr = vm_map_ram(frame_vector_pages(vec), n_pages, -1,
> > PAGE_KERNEL);
> >
> >
> >
> >
> 
> Best regards
> -- 
> Marek Szyprowski, PhD
> Samsung R Institute Poland
> 


-- 
Masami Hiramatsu 

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Hans Verkuil]

On 02/05/2018 02:17 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 5 Feb 2018 12:55:21 +0100
> Hans Verkuil  escreveu:
> 
>> On 02/05/2018 12:15 PM, Mauro Carvalho Chehab wrote:
>>> Hi Hans,
>>>
>>> Em Sun, 4 Feb 2018 14:06:42 +0100
>>> Hans Verkuil  escreveu:
>>>   
 Hi Mauro,

 I'm working on adding proper compliance tests for the MC but I think 
 something
 is missing in the G_TOPOLOGY ioctl w.r.t. pads.

 In several v4l-subdev ioctls you need to pass the pad. There the pad is an 
 index
 for the corresponding entity. I.e. an entity has 3 pads, so the pad 
 argument is
 [0-2].

 The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use 
 that
 in the v4l-subdev ioctls, so how do I translate that to a pad index in my 
 application?

 It seems to be a missing feature in the API. I assume this information is 
 available
 in the core, so then I would add a field to struct media_v2_pad with the 
 pad index
 for the entity.  
>>>
>>> No, this was designed this way by purpose, back in 2015, when this was
>>> discussed at the first MC workshop. It was a consensus on that time that
>>> parameters like PAD index, PAD name, etc should be implemented via the
>>> properties API, as proposed by Sakari [1]. We also had a few discussions 
>>> about that on both IRC and ML.
>>>
>>> [1] See: https://linuxtv.org/news.php?entry=2015-08-17.mchehab
>>>
>>> 3.3 Action items
>>> ...
>>> media_properties: RFC userspace API: Sakari
>>>
>>> Unfortunately, Sakari never found the time to send us a patch series
>>> implementing a properties API, even as RFC.
>>>
>>> One of the other missing features is to add a new version for setting
>>> links (I guess we called it as S_LINKS_V2 at the meetings there).
>>> The hole idea is to provide a way to setup the entire pipeline using
>>> a single ioctl, on an atomic way.
>>>
>>> The big problem with pad indexes happen on entities that have PADs with
>>> different types of signal input or output, like for example a TV tuner.
>>>
>>> On almost all PC consumers hardware supported by the Kernel, the same
>>> PCI/USB ID may come with different types of hardware. This may also
>>> happen with embedded TV sets, as, depending on the market is is
>>> sold, the same product may come with a different tuner.
>>>
>>> A "classic" TV tuner usually has those types of output signals:
>>>
>>> - analog TV luminance IF;
>>> - analog TV chrominance IF [1];
>>> - digital TV OFDM IF;
>>> - audio IF;
>>>
>>> [1] As both luminance and chrominance should go to the same TV
>>> demod, in practice, we can group both signals together on a
>>> single pad.
>>>
>>> More modern tuners also have an audio demod integrated, meaning that
>>> they also have another PAD:
>>> - digital mono or stereo audio.
>>>
>>> At the simplest possible scenario, let's say that we have a TV device
>>> has those entities (among others), each with a single PAD input:
>>>
>>> - entity #0: a TV tuner;
>>> - entity #1: an audio demod;
>>> - entity #2: an analog TV demod;
>>> - entity #3: a digital TV demod.
>>>
>>> And the TV tuner has 4 output pads:
>>>
>>> - pad #0 -> analog TV luminance/chrominance;
>>> - pad #1 -> digital TV IF;
>>> - pad #2 -> audio IF;
>>> - pad #3 -> digital audio.
>>>
>>> There, pad #0 can only be connected to entity #2. You cannot
>>> connect any other pad to entity #2. The same apply to every other
>>> TV tuner output PAD.
>>>
>>> In this specific scenario, entity #1 can only be connected to pad #2,
>>> and pad #3 can't be connected anywhere, as, on this model opted to
>>> have a separate audio demod, and didn't wired the digital audio output.
>>> Yet, the subdev has it.
>>>
>>> Another TV set may have different pad numbers - placing them even on a
>>> different order, and opting to use the audio demod tuner, wiring the
>>> digital audio output.
>>>
>>> Currently, there's no way for an userspace application to know what pads
>>> can be linked to each entity, as there's no way to tell userspace what
>>> kind of signal each pad supports.
>>>
>>> In any case, the Kernel should either detect the tuner model or know
>>> it (via a different DT entry), but userspace need such information,
>>> in order to be able to properly set the pipelines.
>>>
>>> So, what we really need here is passing an optional set of properties
>>> to userspace in order to allow it to do the right thing.  
>>
>> I fail to see the problem. Entities have pads. Pads have both a unique
>> ID and an index within the entity pad list. I.e. the pad ID and the
>> (entity ID + pad index) tuple both uniquely identify the pad.
>>
>> Whether a pad is connected to anything or what type it is is unrelated
>> to this. Passing the pad index of an unconnected pad to e.g. SUBDEV_S_FMT
>> will just result in an error. All I need is to be able to obtain the
>> pad 

Re: [RFC PATCH] media-device: add index field to media_v2_pad

[Mauro Carvalho Chehab]

Em Mon, 5 Feb 2018 13:42:48 +0100
Hans Verkuil  escreveu:

> On 02/05/2018 01:39 PM, Mauro Carvalho Chehab wrote:
> > Em Sun, 4 Feb 2018 14:53:31 +0100
> > Hans Verkuil  escreveu:
> >   
> >> Userspace has no way of knowing the pad index for the entity that
> >> owns the pad with the MEDIA_IOC_G_TOPOLOGY ioctl. However, various
> >> v4l-subdev ioctls need to pass this as an argument.  
> > 
> > While I'm OK on adding a pad index, it still misses a way for Kernelspace
> > to inform the kind of signal it is expected for the cases where an entity
> > provides multiple PAD inputs or outputs with different meanings, e. g.
> > for cases like TV tuner, where different PAD outputs have different
> > signals and should be connected to different entities, based on the PAD
> > type.
> > 
> > In other words, we need also either a:
> > - pad name;
> > - pad type;
> > - pad signal.  
> 
> As mentioned, I agree but it is unrelated to this issue.
> 
> >   
> >>
> >> Add this missing information.
> >>
> >> Signed-off-by: Hans Verkuil 
> >> ---
> >> RFC, so no documentation yet. This works fine, but how would applications
> >> know that media_v2_pad has been extended with a new index field? Currently
> >> this is 0, which is a valid index.
> >>
> >> If no one is using this API (or only for DVB devices) then we can do that.
> >> The other alternative is to add a new pad flag MEDIA_PAD_FL_HAS_INDEX.  
> 
> Any comment on this?

Already answered on another e-mail. IMHO, the best here is to increment
the media API version and rely on it.

> 
> Regards,
> 
>   Hans
> 
> >> ---
> >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >> index e79f72b8b858..16964d3dfb1e 100644
> >> --- a/drivers/media/media-device.c
> >> +++ b/drivers/media/media-device.c
> >> @@ -318,6 +320,7 @@ static long media_device_get_topology(struct 
> >> media_device *mdev,
> >>kpad.id = pad->graph_obj.id;
> >>kpad.entity_id = pad->entity->graph_obj.id;
> >>kpad.flags = pad->flags;
> >> +  kpad.index = pad->index;
> >>
> >>if (copy_to_user(upad, , sizeof(kpad)))
> >>ret = -EFAULT;
> >> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> >> index b9b9446095e9..c3e7a668e122 100644
> >> --- a/include/uapi/linux/media.h
> >> +++ b/include/uapi/linux/media.h
> >> @@ -375,7 +375,8 @@ struct media_v2_pad {
> >>__u32 id;
> >>__u32 entity_id;
> >>__u32 flags;
> >> -  __u32 reserved[5];
> >> +  __u16 index;
> >> +  __u16 reserved[9];
> >>  } __attribute__ ((packed));
> >>
> >>  struct media_v2_link {  
> > 
> > 
> > 
> > Thanks,
> > Mauro
> >   
> 



Thanks,
Mauro

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Mauro Carvalho Chehab]

Em Mon, 5 Feb 2018 12:38:29 +0100
Hans Verkuil  escreveu:

> On 02/05/2018 12:15 PM, Mauro Carvalho Chehab wrote:
> > Hi Hans,
> > 
> > Em Sun, 4 Feb 2018 14:06:42 +0100
> > Hans Verkuil  escreveu:
> >   
> >> Hi Mauro,
> >>
> >> I'm working on adding proper compliance tests for the MC but I think 
> >> something
> >> is missing in the G_TOPOLOGY ioctl w.r.t. pads.
> >>
> >> In several v4l-subdev ioctls you need to pass the pad. There the pad is an 
> >> index
> >> for the corresponding entity. I.e. an entity has 3 pads, so the pad 
> >> argument is
> >> [0-2].
> >>
> >> The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use 
> >> that
> >> in the v4l-subdev ioctls, so how do I translate that to a pad index in my 
> >> application?
> >>
> >> It seems to be a missing feature in the API. I assume this information is 
> >> available
> >> in the core, so then I would add a field to struct media_v2_pad with the 
> >> pad index
> >> for the entity.  
> > 
> > No, this was designed this way by purpose, back in 2015, when this was
> > discussed at the first MC workshop. It was a consensus on that time that
> > parameters like PAD index, PAD name, etc should be implemented via the
> > properties API, as proposed by Sakari [1]. We also had a few discussions 
> > about that on both IRC and ML.  
> 
> I'll read up on this and reply to this later.
> 
> 
> 
> >> Next time we add new public API features I want to see compliance tests 
> >> before
> >> accepting it. It's much too easy to overlook something, either in the 
> >> design or
> >> in a driver or in the documentation, so this is really, really needed 
> >> IMHO.  
> > 
> > We added a test tool for G_TOPOLOGY on that time.
> > 
> > I doubt that writing test/compliance tools in advance will solve all API
> > gaps. The thing is that new features will take some time to be used on
> > real apps. Some things are only visible when real apps start using the
> > new API features.  
> 
> This is no excuse whatsoever NOT to write compliance tests. Sure, they will
> never find everything, but for sure they find a LOT! Especially all the
> little stupid things that can make something unusable for certain use-cases.

I agree that either a compliance or a test app is important.

> And equally important, driver developers can use it to check that they didn't
> forget anything.
> 
> A media-ctl/v4l2-ctl/whatever utility is no substitute for proper compliance
> testing. The media API is complex because video is complex. It is impossible
> to review code and catch all the little mistakes, but compliance tests can
> go far in verifying driver code and also catching core code regressions.
> 
> I have yet to see a new driver that didn't fail at least one v4l2-compliance
> test, and I very much doubt that existing subdev/media drivers would do any
> different.
> 
> It would be very interesting if you would test it as well on DVB devices.
> You should be able to run v4l2-compliance on the media device. There may
> well be bugs in the tests for DVB devices. But that might also be caused
> by incomplete documentation in the spec.

I can surely do some tests on DVB devices too. Please remind me after a
couple of weeks. I'm very busy this week, and usually the first week
after a merge window is also a busy one.


Regards,
Mauro

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Mauro Carvalho Chehab]

Em Mon, 5 Feb 2018 12:55:21 +0100
Hans Verkuil  escreveu:

> On 02/05/2018 12:15 PM, Mauro Carvalho Chehab wrote:
> > Hi Hans,
> > 
> > Em Sun, 4 Feb 2018 14:06:42 +0100
> > Hans Verkuil  escreveu:
> >   
> >> Hi Mauro,
> >>
> >> I'm working on adding proper compliance tests for the MC but I think 
> >> something
> >> is missing in the G_TOPOLOGY ioctl w.r.t. pads.
> >>
> >> In several v4l-subdev ioctls you need to pass the pad. There the pad is an 
> >> index
> >> for the corresponding entity. I.e. an entity has 3 pads, so the pad 
> >> argument is
> >> [0-2].
> >>
> >> The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use 
> >> that
> >> in the v4l-subdev ioctls, so how do I translate that to a pad index in my 
> >> application?
> >>
> >> It seems to be a missing feature in the API. I assume this information is 
> >> available
> >> in the core, so then I would add a field to struct media_v2_pad with the 
> >> pad index
> >> for the entity.  
> > 
> > No, this was designed this way by purpose, back in 2015, when this was
> > discussed at the first MC workshop. It was a consensus on that time that
> > parameters like PAD index, PAD name, etc should be implemented via the
> > properties API, as proposed by Sakari [1]. We also had a few discussions 
> > about that on both IRC and ML.
> > 
> > [1] See: https://linuxtv.org/news.php?entry=2015-08-17.mchehab
> > 
> > 3.3 Action items
> > ...
> > media_properties: RFC userspace API: Sakari
> > 
> > Unfortunately, Sakari never found the time to send us a patch series
> > implementing a properties API, even as RFC.
> > 
> > One of the other missing features is to add a new version for setting
> > links (I guess we called it as S_LINKS_V2 at the meetings there).
> > The hole idea is to provide a way to setup the entire pipeline using
> > a single ioctl, on an atomic way.
> > 
> > The big problem with pad indexes happen on entities that have PADs with
> > different types of signal input or output, like for example a TV tuner.
> > 
> > On almost all PC consumers hardware supported by the Kernel, the same
> > PCI/USB ID may come with different types of hardware. This may also
> > happen with embedded TV sets, as, depending on the market is is
> > sold, the same product may come with a different tuner.
> > 
> > A "classic" TV tuner usually has those types of output signals:
> > 
> > - analog TV luminance IF;
> > - analog TV chrominance IF [1];
> > - digital TV OFDM IF;
> > - audio IF;
> > 
> > [1] As both luminance and chrominance should go to the same TV
> > demod, in practice, we can group both signals together on a
> > single pad.
> > 
> > More modern tuners also have an audio demod integrated, meaning that
> > they also have another PAD:
> > - digital mono or stereo audio.
> > 
> > At the simplest possible scenario, let's say that we have a TV device
> > has those entities (among others), each with a single PAD input:
> > 
> > - entity #0: a TV tuner;
> > - entity #1: an audio demod;
> > - entity #2: an analog TV demod;
> > - entity #3: a digital TV demod.
> > 
> > And the TV tuner has 4 output pads:
> > 
> > - pad #0 -> analog TV luminance/chrominance;
> > - pad #1 -> digital TV IF;
> > - pad #2 -> audio IF;
> > - pad #3 -> digital audio.
> > 
> > There, pad #0 can only be connected to entity #2. You cannot
> > connect any other pad to entity #2. The same apply to every other
> > TV tuner output PAD.
> > 
> > In this specific scenario, entity #1 can only be connected to pad #2,
> > and pad #3 can't be connected anywhere, as, on this model opted to
> > have a separate audio demod, and didn't wired the digital audio output.
> > Yet, the subdev has it.
> > 
> > Another TV set may have different pad numbers - placing them even on a
> > different order, and opting to use the audio demod tuner, wiring the
> > digital audio output.
> > 
> > Currently, there's no way for an userspace application to know what pads
> > can be linked to each entity, as there's no way to tell userspace what
> > kind of signal each pad supports.
> > 
> > In any case, the Kernel should either detect the tuner model or know
> > it (via a different DT entry), but userspace need such information,
> > in order to be able to properly set the pipelines.
> > 
> > So, what we really need here is passing an optional set of properties
> > to userspace in order to allow it to do the right thing.  
> 
> I fail to see the problem. Entities have pads. Pads have both a unique
> ID and an index within the entity pad list. I.e. the pad ID and the
> (entity ID + pad index) tuple both uniquely identify the pad.
> 
> Whether a pad is connected to anything or what type it is is unrelated
> to this. Passing the pad index of an unconnected pad to e.g. SUBDEV_S_FMT
> will just result in an error. All I need is to be able to obtain the
> pad index so I can call SUBDEV_S_FMT at all!

The 

[PATCH] drivers: staging: media: atomisp: pci: atomisp2: css2400: fix misspellings

[Alona]

From: Alona Solntseva 

Misspelled words are fixed in several places.

Signed-off-by: Alona Solntseva 
---
 .../staging/media/atomisp/pci/atomisp2/css2400/sh_css.c  | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
index 322bb3d..de712fa 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
@@ -194,7 +194,7 @@ sh_css_pipe_start(struct ia_css_stream *stream);
  * @param[in] stream   Point to the target "ia_css_stream" instance.
  *
  * @return
- * - IA_CSS_SUCCESS, if the "stop" requests have been sucessfully sent out.
+ * - IA_CSS_SUCCESS, if the "stop" requests have been successfully sent out.
  * - CSS error code, otherwise.
  *
  *
@@ -1054,7 +1054,7 @@ sh_css_config_input_network(struct ia_css_stream *stream)
if (stream->last_pipe->config.mode == IA_CSS_PIPE_MODE_CAPTURE) 
{
/*
 * We need to poll the ISYS HW in capture_indication 
itself
-* for "non-continous" capture usecase for getting 
accurate
+* for "non-continuous" capture usecase for getting 
accurate
 * isys frame capture timestamps.
 * This is because the capturepipe propcessing takes 
longer
 * to execute than the input system frame capture.
@@ -3657,7 +3657,7 @@ static enum ia_css_err create_host_video_pipeline(struct 
ia_css_pipe *pipe)
in_frame = me->stages->args.out_frame[0];
} else if (pipe->stream->config.continuous) {
 #ifdef USE_INPUT_SYSTEM_VERSION_2401
-   /* When continous is enabled, configure in_frame with the
+   /* When continuous is enabled, configure in_frame with the
 * last pipe, which is the copy pipe.
 */
in_frame = pipe->stream->last_pipe->continuous_frames[0];
@@ -3854,7 +3854,7 @@ create_host_preview_pipeline(struct ia_css_pipe *pipe)
 * - Direct Sensor Mode Online Preview
 * - Buffered Sensor Mode Online Preview
 * - Direct Sensor Mode Continuous Preview
-* - Buffered Sensor Mode Continous Preview
+* - Buffered Sensor Mode Continuous Preview
 */
sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR);
buffered_sensor = (pipe->stream->config.mode == 
IA_CSS_INPUT_MODE_BUFFERED_SENSOR);
@@ -4715,7 +4715,7 @@ ia_css_dequeue_psys_event(struct ia_css_event *event)
event->timer_subcode = payload[2];
}
/* It's a non timer event. So clear first half of the timer 
event data.
-   * If the second part of the TIMER event is not recieved, we 
discard
+   * If the second part of the TIMER event is not received, we 
discard
* the first half of the timer data and process the non timer 
event without
* affecting the flow. So the non timer event falls through
* the code. */
@@ -7610,7 +7610,7 @@ create_host_yuvpp_pipeline(struct ia_css_pipe *pipe)
 * except for the following:
 * - Direct Sensor Mode Online Capture
 * - Direct Sensor Mode Continuous Capture
-* - Buffered Sensor Mode Continous Capture
+* - Buffered Sensor Mode Continuous Capture
 */
sensor = pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR;
buffered_sensor = pipe->stream->config.mode == 
IA_CSS_INPUT_MODE_BUFFERED_SENSOR;
@@ -7950,7 +7950,7 @@ create_host_regular_capture_pipeline(struct ia_css_pipe 
*pipe)
 * - Direct Sensor Mode Online Capture
 * - Direct Sensor Mode Online Capture
 * - Direct Sensor Mode Continuous Capture
-* - Buffered Sensor Mode Continous Capture
+* - Buffered Sensor Mode Continuous Capture
 */
sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR);
buffered_sensor = (pipe->stream->config.mode == 
IA_CSS_INPUT_MODE_BUFFERED_SENSOR);
@@ -8915,7 +8915,7 @@ ia_css_pipe_create(const struct ia_css_pipe_config 
*config,
err = ia_css_pipe_create_extra(config, NULL, pipe);
 
if(err == IA_CSS_SUCCESS) {
-   IA_CSS_LOG("pipe created successfuly = %p", *pipe);
+   IA_CSS_LOG("pipe created successfully = %p", *pipe);
}
 
IA_CSS_LEAVE_ERR_PRIVATE(err);
-- 
2.7.4

Re: [RFC PATCH] media-device: add index field to media_v2_pad

[Hans Verkuil]

On 02/05/2018 01:39 PM, Mauro Carvalho Chehab wrote:
> Em Sun, 4 Feb 2018 14:53:31 +0100
> Hans Verkuil  escreveu:
> 
>> Userspace has no way of knowing the pad index for the entity that
>> owns the pad with the MEDIA_IOC_G_TOPOLOGY ioctl. However, various
>> v4l-subdev ioctls need to pass this as an argument.
> 
> While I'm OK on adding a pad index, it still misses a way for Kernelspace
> to inform the kind of signal it is expected for the cases where an entity
> provides multiple PAD inputs or outputs with different meanings, e. g.
> for cases like TV tuner, where different PAD outputs have different
> signals and should be connected to different entities, based on the PAD
> type.
> 
> In other words, we need also either a:
>   - pad name;
>   - pad type;
>   - pad signal.

As mentioned, I agree but it is unrelated to this issue.

> 
>>
>> Add this missing information.
>>
>> Signed-off-by: Hans Verkuil 
>> ---
>> RFC, so no documentation yet. This works fine, but how would applications
>> know that media_v2_pad has been extended with a new index field? Currently
>> this is 0, which is a valid index.
>>
>> If no one is using this API (or only for DVB devices) then we can do that.
>> The other alternative is to add a new pad flag MEDIA_PAD_FL_HAS_INDEX.

Any comment on this?

Regards,

Hans

>> ---
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index e79f72b8b858..16964d3dfb1e 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -318,6 +320,7 @@ static long media_device_get_topology(struct 
>> media_device *mdev,
>>  kpad.id = pad->graph_obj.id;
>>  kpad.entity_id = pad->entity->graph_obj.id;
>>  kpad.flags = pad->flags;
>> +kpad.index = pad->index;
>>
>>  if (copy_to_user(upad, , sizeof(kpad)))
>>  ret = -EFAULT;
>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>> index b9b9446095e9..c3e7a668e122 100644
>> --- a/include/uapi/linux/media.h
>> +++ b/include/uapi/linux/media.h
>> @@ -375,7 +375,8 @@ struct media_v2_pad {
>>  __u32 id;
>>  __u32 entity_id;
>>  __u32 flags;
>> -__u32 reserved[5];
>> +__u16 index;
>> +__u16 reserved[9];
>>  } __attribute__ ((packed));
>>
>>  struct media_v2_link {
> 
> 
> 
> Thanks,
> Mauro
> 

Re: [RFC PATCH] media-device: add index field to media_v2_pad

[Mauro Carvalho Chehab]

Em Sun, 4 Feb 2018 14:53:31 +0100
Hans Verkuil  escreveu:

> Userspace has no way of knowing the pad index for the entity that
> owns the pad with the MEDIA_IOC_G_TOPOLOGY ioctl. However, various
> v4l-subdev ioctls need to pass this as an argument.

While I'm OK on adding a pad index, it still misses a way for Kernelspace
to inform the kind of signal it is expected for the cases where an entity
provides multiple PAD inputs or outputs with different meanings, e. g.
for cases like TV tuner, where different PAD outputs have different
signals and should be connected to different entities, based on the PAD
type.

In other words, we need also either a:
- pad name;
- pad type;
- pad signal.

> 
> Add this missing information.
> 
> Signed-off-by: Hans Verkuil 
> ---
> RFC, so no documentation yet. This works fine, but how would applications
> know that media_v2_pad has been extended with a new index field? Currently
> this is 0, which is a valid index.
> 
> If no one is using this API (or only for DVB devices) then we can do that.
> The other alternative is to add a new pad flag MEDIA_PAD_FL_HAS_INDEX.
> ---
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index e79f72b8b858..16964d3dfb1e 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -318,6 +320,7 @@ static long media_device_get_topology(struct media_device 
> *mdev,
>   kpad.id = pad->graph_obj.id;
>   kpad.entity_id = pad->entity->graph_obj.id;
>   kpad.flags = pad->flags;
> + kpad.index = pad->index;
> 
>   if (copy_to_user(upad, , sizeof(kpad)))
>   ret = -EFAULT;
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index b9b9446095e9..c3e7a668e122 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -375,7 +375,8 @@ struct media_v2_pad {
>   __u32 id;
>   __u32 entity_id;
>   __u32 flags;
> - __u32 reserved[5];
> + __u16 index;
> + __u16 reserved[9];
>  } __attribute__ ((packed));
> 
>  struct media_v2_link {



Thanks,
Mauro

Re: media_device.c question: can this workaround be removed?

[Hans Verkuil]

On 02/05/2018 12:59 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Feb 05, 2018 at 11:26:47AM +0100, Hans Verkuil wrote:
>> The function media_device_enum_entities() has this workaround:
>>
>> /*
>>  * Workaround for a bug at media-ctl <= v1.10 that makes it to
>>  * do the wrong thing if the entity function doesn't belong to
>>  * either MEDIA_ENT_F_OLD_BASE or MEDIA_ENT_F_OLD_SUBDEV_BASE
>>  * Ranges.
>>  *
>>  * Non-subdevices are expected to be at the MEDIA_ENT_F_OLD_BASE,
>>  * or, otherwise, will be silently ignored by media-ctl when
>>  * printing the graphviz diagram. So, map them into the devnode
>>  * old range.
>>  */
>> if (ent->function < MEDIA_ENT_F_OLD_BASE ||
>> ent->function > MEDIA_ENT_F_TUNER) {
>> if (is_media_entity_v4l2_subdev(ent))
>> entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
>> else if (ent->function != MEDIA_ENT_F_IO_V4L)
>> entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
>> }
>>
>> But this means that the entity type returned by ENUM_ENTITIES just overwrites
>> perfectly fine types by bogus values and thus the returned value differs
>> from that returned by G_TOPOLOGY.
>>
>> Can we please, please remove this workaround? I have no idea why a workaround
>> for media-ctl of all things ever made it in the kernel.
> 
> The entity types were replaced by entity functions back in 2015 with the
> big Media controller reshuffle. While I agree functions are better for
> describing entities than types (and those types had Linux specific
> interfaces in them), the new function-based API still may support a single
> value, i.e. a single function per device.
> 
> This also created two different name spaces for describing entities: the
> old types used by the MC API and the new functions used by MC v2 API.
> 
> This doesn't go well with the fact that, as you noticed, the pad
> information is not available through MC v2 API. The pad information is
> required by the user space so software continues to use the original MC
> API.
> 
> I don't think there's a way to avoid maintaining two name spaces (types and
> functions) without breaking at least one of the APIs.

The comment specifically claims that this workaround is for media-ctl and
it suggests that it is fixed after v1.10. Is that comment bogus? I can't
really tell which commit fixed media-ctl. Does anyone know?

As far as I can tell the function defines have been chosen in such a way that
they will equally well work with the old name space. There should be no
problem there whatsoever and media-ctl should switch to use the new defines.

We now have a broken ENUM_ENTITIES ioctl (it rudely overwrites VBI/DVB/etc 
types)
and a broken G_TOPOLOGY ioctl (no pad index).

This sucks. Let's fix both so that they at least report consistent information.

> 
>>
>> I'm adding media support to the vivid driver and because of this media-ctl -p
>> gives me this:
>>
>> Device topology
>> - entity 1: vivid-000-vid-cap (1 pad, 0 link)
>> type Node subtype V4L flags 0
>> device node name /dev/video0
>> pad0: Source
>>
>> - entity 5: vivid-000-vid-out (1 pad, 0 link)
>> type Node subtype V4L flags 0
>> device node name /dev/video1
>> pad0: Sink
>>
>> - entity 9: vivid-000-vbi-cap (1 pad, 0 link)
>> type Unknown subtype Unknown flags 0
>> pad0: Source
>>
>> - entity 13: vivid-000-vbi-out (1 pad, 0 link)
>>  type Unknown subtype Unknown flags 0
>> pad0: Sink
>>
>> - entity 17: vivid-000-sdr-cap (1 pad, 0 link)
>>  type Unknown subtype Unknown flags 0
>> pad0: Source
> 
> It may be that there's no corresponding type for certain functions.

'type' can be interpreted as 'function'. All possible legacy type/subtype
combinations map to a unique function. It's how the spec defines this as well.
But it is subverted by this awful workaround.

Regards,

Hans

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Mauro Carvalho Chehab]

Em Sun, 04 Feb 2018 15:20:55 +0200
Laurent Pinchart  escreveu:

> Hi Hans,
> 
> On Sunday, 4 February 2018 15:16:26 EET Hans Verkuil wrote:
> > On 02/04/2018 02:13 PM, Laurent Pinchart wrote:  
> > > On Sunday, 4 February 2018 15:06:42 EET Hans Verkuil wrote:  
> > >> Hi Mauro,
> > >> 
> > >> I'm working on adding proper compliance tests for the MC but I think
> > >> something is missing in the G_TOPOLOGY ioctl w.r.t. pads.
> > >> 
> > >> In several v4l-subdev ioctls you need to pass the pad. There the pad is
> > >> an index for the corresponding entity. I.e. an entity has 3 pads, so the
> > >> pad argument is [0-2].
> > >> 
> > >> The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use
> > >> that in the v4l-subdev ioctls, so how do I translate that to a pad index
> > >> in my application?
> > >> 
> > >> It seems to be a missing feature in the API. I assume this information is
> > >> available in the core, so then I would add a field to struct media_v2_pad
> > >> with the pad index for the entity.
> > >> 
> > >> Next time we add new public API features I want to see compliance tests
> > >> before accepting it. It's much too easy to overlook something, either in
> > >> the design or in a driver or in the documentation, so this is really,
> > >> really needed IMHO.  
> > > 
> > > I agree with you, and would even like to go one step beyond by requiring
> > > an implementation in a real use case, not just in a compliance or test
> > > tool.

We could require an open source real application and some hardware to
allow us to test new APIs before allowing adding them, but I suspect that
this will just stall the development, as, on most companies, one development
team work on writing a new Kernel feature. Once done, a separate team
starts to implement userspace tools. On embedded world, this doesn't even
envolve writing any open source apps.

> > > On the topic of the G_TOPOLOGY API, it's pretty clear it was merged too
> > > hastily. We could try to fix it, but given all the issues we haven't
> > > solved yet, I believe a new version of the API would be better.  
> > 
> > It's actually not too bad as an API speaking as an end-user. It's simple and
> > efficient. But this pad issue is a real problem.  
> 
> We have other issues such as connector support

The thing with connector is unrelated with the API that reports entities.
From API PoV, a connector is just a new entity type. 

The discussions around it were purely related about how to deal with the
case where a single physical connector carries multiple signals on it, 
but require different settings, depending on how this is physically wired.

This is a very common problem with S-Video connectors
(MEDIA_ENT_F_CONN_SVIDEO), as lots of boards are shipped with a cable to 
allow using a S-Video input for composite video.

At proprietary software that comes with those boards, it identifies a
single S-Video connector as if they were two different physical connectors
(e. g. it looks at the final connector after the cabling).

In other words S-Video input physical connectors at the board can be used
on two different scenarios:

1) using S-Video/S-Video cable, using 4 pins, 2 for chrominance, 2 for
   luminance. The end connector is a S-Video plug.

2) using a S-Video to composite video cable, using just 2 pins of the
   board input connector. The end connector is RCA composite plug.

There are even some scenarios (very common on Hauppauge devices) where
a single multiple pin proprietary connector has multiple physical
connectors at their ends for both Audio and Video. Among them, there
is a separate S-Video plug and a separate Composite RCA plug.
On such cables, the Composite connector is usually physically wired
to the S-Video Y signal, just like if it had a S-Video to composite
cable.

Depending if the physical connector is connected using a S-Video/S-Video
or a S-Video/Composite cable, the settings at the driver should be
different (not only enabling input pins but also changing some demod
parameters in order to provide enhanced quality for S-Video).

So, while physically there is just one connector at the board, logically, 
there are two different V4L2 device/subdev settings, depending on the type
of cable/signals connected to it.

Any way, such discussion is completely orthogonal to G_TOPOLOGY.
No matter what API we use, we should have a way to allow userspace
to select between "S-Video" and "composite" on devices that provide
S-Video physical connectors.

As discussed, the main alternatives are:

1) Have one pad for Y signal and one pad for C signal.

If both are linked to a S-Video connector, it is in S-Video mode.
If just Y signal is linked, it is in composite mode.

2) Have one pad for S-Video, one pad for Composite.

If composite pad is linked to a S-Video connector, it is composite;
if s-video pad is linked to a S-Video connector, it is S-Video.

We didn't reach a consensus if either (1) or (2) would be 

Re: [PATCH 2/2] media: dt-bindings: Add bindings for Dongwoon DW9807 voice coil

[Sakari Ailus]

Hi Andy,

On Thu, Feb 01, 2018 at 11:48:12PM +0800, Andy Yeh wrote:
> From: Alan Chiang 
> 
> Dongwoon DW9807 is a voice coil lens driver.
> 
> Also add a vendor prefix for Dongwoon for one did not exist previously.
> 
> Signed-off-by: Andy Yeh 

Could you send DT binding patches to the devicetree list as well, please?

I'm cc'ing the DT list this time.

> ---
>  Documentation/devicetree/bindings/media/i2c/dongwoon,dw9807.txt | 9 +
>  1 file changed, 9 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/media/i2c/dongwoon,dw9807.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9807.txt 
> b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9807.txt
> new file mode 100644
> index 000..0a1a860
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9807.txt
> @@ -0,0 +1,9 @@
> +Dongwoon Anatech DW9807 voice coil lens driver
> +
> +DW9807 is a 10-bit DAC with current sink capability. It is intended for
> +controlling voice coil lenses.
> +
> +Mandatory properties:
> +
> +- compatible: "dongwoon,dw9807"
> +- reg: I2C slave address
> -- 
> 2.7.4
> 

-- 
Sakari Ailus
sakari.ai...@linux.intel.com

Re: media_device.c question: can this workaround be removed?

[Sakari Ailus]

Hi Hans,

On Mon, Feb 05, 2018 at 11:26:47AM +0100, Hans Verkuil wrote:
> The function media_device_enum_entities() has this workaround:
> 
> /*
>  * Workaround for a bug at media-ctl <= v1.10 that makes it to
>  * do the wrong thing if the entity function doesn't belong to
>  * either MEDIA_ENT_F_OLD_BASE or MEDIA_ENT_F_OLD_SUBDEV_BASE
>  * Ranges.
>  *
>  * Non-subdevices are expected to be at the MEDIA_ENT_F_OLD_BASE,
>  * or, otherwise, will be silently ignored by media-ctl when
>  * printing the graphviz diagram. So, map them into the devnode
>  * old range.
>  */
> if (ent->function < MEDIA_ENT_F_OLD_BASE ||
> ent->function > MEDIA_ENT_F_TUNER) {
> if (is_media_entity_v4l2_subdev(ent))
> entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
> else if (ent->function != MEDIA_ENT_F_IO_V4L)
> entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
> }
> 
> But this means that the entity type returned by ENUM_ENTITIES just overwrites
> perfectly fine types by bogus values and thus the returned value differs
> from that returned by G_TOPOLOGY.
> 
> Can we please, please remove this workaround? I have no idea why a workaround
> for media-ctl of all things ever made it in the kernel.

The entity types were replaced by entity functions back in 2015 with the
big Media controller reshuffle. While I agree functions are better for
describing entities than types (and those types had Linux specific
interfaces in them), the new function-based API still may support a single
value, i.e. a single function per device.

This also created two different name spaces for describing entities: the
old types used by the MC API and the new functions used by MC v2 API.

This doesn't go well with the fact that, as you noticed, the pad
information is not available through MC v2 API. The pad information is
required by the user space so software continues to use the original MC
API.

I don't think there's a way to avoid maintaining two name spaces (types and
functions) without breaking at least one of the APIs.

> 
> I'm adding media support to the vivid driver and because of this media-ctl -p
> gives me this:
> 
> Device topology
> - entity 1: vivid-000-vid-cap (1 pad, 0 link)
> type Node subtype V4L flags 0
> device node name /dev/video0
> pad0: Source
> 
> - entity 5: vivid-000-vid-out (1 pad, 0 link)
> type Node subtype V4L flags 0
> device node name /dev/video1
> pad0: Sink
> 
> - entity 9: vivid-000-vbi-cap (1 pad, 0 link)
> type Unknown subtype Unknown flags 0
> pad0: Source
> 
> - entity 13: vivid-000-vbi-out (1 pad, 0 link)
>  type Unknown subtype Unknown flags 0
> pad0: Sink
> 
> - entity 17: vivid-000-sdr-cap (1 pad, 0 link)
>  type Unknown subtype Unknown flags 0
> pad0: Source

It may be that there's no corresponding type for certain functions.

> 
> So VBI and SDR report the 'Unknown' type whereas 'v4l2-ctl -D -d /dev/vbi0' 
> (which
> uses G_TOPOLOGY) gives me:
> 
> Interface Info:
> ID   : 0x030b
> Type : V4L VBI
> Entity Info:
> ID   : 0x0009 (9)
> Name : vivid-000-vbi-cap
> Function : VBI I/O
> Pad 0x010a   : Source
> 
> That's how it should be.
> 
> 
> 
> Never again should we allow new userspace APIs without:
> 
> 1) Proper compliance tests
> 2) Adding support for the new API to v4l2-ctl (or related v4l-utils apps)
> 3) If the new API replaces old defines with new defines (e.g.
>#define MEDIA_ENT_T_DEVNODE_V4L MEDIA_ENT_F_IO_V4L) then everything
>in v4l-utils that uses the old define should be updated to the new
>define.
> 4) If reasonable add support for the new API to at least one of the
>virtual drivers (vivid, vimc, vim2m) so this API can be tested without
>needing specialized hardware.
> 
> The MC API did none of this and how on earth are end-users able to work with
> this if we have horribly inconsistent behavior like this?
> 
> BTW, uapi/linux/media.h is an utter mess. I'll see if I can make a patch to
> make it more understandable. Right now it is extremely hard to tell which
> define is legacy and which isn't.
> 
> 
> 
> Regards,
> 
>   Hans

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Hans Verkuil]

On 02/05/2018 12:15 PM, Mauro Carvalho Chehab wrote:
> Hi Hans,
> 
> Em Sun, 4 Feb 2018 14:06:42 +0100
> Hans Verkuil  escreveu:
> 
>> Hi Mauro,
>>
>> I'm working on adding proper compliance tests for the MC but I think 
>> something
>> is missing in the G_TOPOLOGY ioctl w.r.t. pads.
>>
>> In several v4l-subdev ioctls you need to pass the pad. There the pad is an 
>> index
>> for the corresponding entity. I.e. an entity has 3 pads, so the pad argument 
>> is
>> [0-2].
>>
>> The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use 
>> that
>> in the v4l-subdev ioctls, so how do I translate that to a pad index in my 
>> application?
>>
>> It seems to be a missing feature in the API. I assume this information is 
>> available
>> in the core, so then I would add a field to struct media_v2_pad with the pad 
>> index
>> for the entity.
> 
> No, this was designed this way by purpose, back in 2015, when this was
> discussed at the first MC workshop. It was a consensus on that time that
> parameters like PAD index, PAD name, etc should be implemented via the
> properties API, as proposed by Sakari [1]. We also had a few discussions 
> about that on both IRC and ML.
> 
> [1] See: https://linuxtv.org/news.php?entry=2015-08-17.mchehab
> 
>   3.3 Action items
>   ...
>   media_properties: RFC userspace API: Sakari
> 
> Unfortunately, Sakari never found the time to send us a patch series
> implementing a properties API, even as RFC.
> 
> One of the other missing features is to add a new version for setting
> links (I guess we called it as S_LINKS_V2 at the meetings there).
> The hole idea is to provide a way to setup the entire pipeline using
> a single ioctl, on an atomic way.
> 
> The big problem with pad indexes happen on entities that have PADs with
> different types of signal input or output, like for example a TV tuner.
> 
> On almost all PC consumers hardware supported by the Kernel, the same
> PCI/USB ID may come with different types of hardware. This may also
> happen with embedded TV sets, as, depending on the market is is
> sold, the same product may come with a different tuner.
> 
> A "classic" TV tuner usually has those types of output signals:
> 
>   - analog TV luminance IF;
>   - analog TV chrominance IF [1];
>   - digital TV OFDM IF;
>   - audio IF;
> 
> [1] As both luminance and chrominance should go to the same TV
> demod, in practice, we can group both signals together on a
> single pad.
> 
> More modern tuners also have an audio demod integrated, meaning that
> they also have another PAD:
>   - digital mono or stereo audio.
> 
> At the simplest possible scenario, let's say that we have a TV device
> has those entities (among others), each with a single PAD input:
> 
>   - entity #0: a TV tuner;
>   - entity #1: an audio demod;
>   - entity #2: an analog TV demod;
>   - entity #3: a digital TV demod.
> 
> And the TV tuner has 4 output pads:
> 
>   - pad #0 -> analog TV luminance/chrominance;
>   - pad #1 -> digital TV IF;
>   - pad #2 -> audio IF;
>   - pad #3 -> digital audio.
> 
> There, pad #0 can only be connected to entity #2. You cannot
> connect any other pad to entity #2. The same apply to every other
> TV tuner output PAD.
> 
> In this specific scenario, entity #1 can only be connected to pad #2,
> and pad #3 can't be connected anywhere, as, on this model opted to
> have a separate audio demod, and didn't wired the digital audio output.
> Yet, the subdev has it.
> 
> Another TV set may have different pad numbers - placing them even on a
> different order, and opting to use the audio demod tuner, wiring the
> digital audio output.
> 
> Currently, there's no way for an userspace application to know what pads
> can be linked to each entity, as there's no way to tell userspace what
> kind of signal each pad supports.
> 
> In any case, the Kernel should either detect the tuner model or know
> it (via a different DT entry), but userspace need such information,
> in order to be able to properly set the pipelines.
> 
> So, what we really need here is passing an optional set of properties
> to userspace in order to allow it to do the right thing.

I fail to see the problem. Entities have pads. Pads have both a unique
ID and an index within the entity pad list. I.e. the pad ID and the
(entity ID + pad index) tuple both uniquely identify the pad.

Whether a pad is connected to anything or what type it is is unrelated
to this. Passing the pad index of an unconnected pad to e.g. SUBDEV_S_FMT
will just result in an error. All I need is to be able to obtain the
pad index so I can call SUBDEV_S_FMT at all!

I actually like G_TOPOLOGY, it's nice. But it just does not give all the
information needed.

> Yet, I agree with you: we should not wait anymore for a properties API,
> as it seems unlikely that we'll add support for it anytime soon.
> 
> So, let's add two fields there:
>   - pad index 

RE: [PATCH v5 4/5] media: ov5640: add support of DVP parallel interface

[Fabrizio Castro]

Hello Maxime,

thank you for your feedback.

> > > +/*
> > > + * configure parallel port control lines polarity
> > > + *
> > > + * POLARITY CTRL0
> > > + * - [5]:PCLK polarity (0: active low, 1: active high)
> > > + * - [1]:HREF polarity (0: active low, 1: active high)
> > > + * - [0]:VSYNC polarity (mismatch here between
> > > + *datasheet and hardware, 0 is active high
> > > + *and 1 is active low...)
> >
> > I know that yourself and Maxime have both confirmed that VSYNC
> > polarity is inverted, but I am looking at HSYNC and VSYNC with a
> > logic analyser and I am dumping the values written to
> > OV5640_REG_POLARITY_CTRL00 and to me it looks like that HSYNC is
> > active HIGH when hsync_pol == 0, and VSYNC is active HIGH when
> > vsync_pol == 1.
>
> Which mode are you testing this on?

My testing environment is made of the sensor connected to a SoC with 8 data 
lines, vsync, hsync, and pclk, and of course I am specifying hsync-active, 
vsync-active, and pclk-sample in the device tree on both ends so that they 
configure themselves accordingly to work in DVP mode (V4L2_MBUS_PARALLEL), with 
the correct polarities.
Between the sensor and the SoC there is a noninverting bus transceiver (voltage 
translator), for my experiments I have plugged myself onto the outputs of this 
transceiver only to be compliant with the voltage level of my logic analyser.

>
> The non-active periods are insanely high in most modes (1896 for an
> active horizontal length of 640 in 640x480 for example), especially
> hsync, and it's really easy to invert the two.

I am looking at all the data lines, so that I don't confuse the non-active 
periods with the active periods, and with my setup what I reported is what I 
get. I wonder if this difference comes from the sensor revision at all? 
Unfortunately I can only test the one camera I have got.

Thanks,
Fab

>
> Maxime
>
> --
> Maxime Ripard, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> http://bootlin.com



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, 
Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered 
No. 04586709.

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Hans Verkuil]

On 02/05/2018 12:15 PM, Mauro Carvalho Chehab wrote:
> Hi Hans,
> 
> Em Sun, 4 Feb 2018 14:06:42 +0100
> Hans Verkuil  escreveu:
> 
>> Hi Mauro,
>>
>> I'm working on adding proper compliance tests for the MC but I think 
>> something
>> is missing in the G_TOPOLOGY ioctl w.r.t. pads.
>>
>> In several v4l-subdev ioctls you need to pass the pad. There the pad is an 
>> index
>> for the corresponding entity. I.e. an entity has 3 pads, so the pad argument 
>> is
>> [0-2].
>>
>> The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use 
>> that
>> in the v4l-subdev ioctls, so how do I translate that to a pad index in my 
>> application?
>>
>> It seems to be a missing feature in the API. I assume this information is 
>> available
>> in the core, so then I would add a field to struct media_v2_pad with the pad 
>> index
>> for the entity.
> 
> No, this was designed this way by purpose, back in 2015, when this was
> discussed at the first MC workshop. It was a consensus on that time that
> parameters like PAD index, PAD name, etc should be implemented via the
> properties API, as proposed by Sakari [1]. We also had a few discussions 
> about that on both IRC and ML.

I'll read up on this and reply to this later.



>> Next time we add new public API features I want to see compliance tests 
>> before
>> accepting it. It's much too easy to overlook something, either in the design 
>> or
>> in a driver or in the documentation, so this is really, really needed IMHO.
> 
> We added a test tool for G_TOPOLOGY on that time.
> 
> I doubt that writing test/compliance tools in advance will solve all API
> gaps. The thing is that new features will take some time to be used on
> real apps. Some things are only visible when real apps start using the
> new API features.

This is no excuse whatsoever NOT to write compliance tests. Sure, they will
never find everything, but for sure they find a LOT! Especially all the
little stupid things that can make something unusable for certain use-cases.

And equally important, driver developers can use it to check that they didn't
forget anything.

A media-ctl/v4l2-ctl/whatever utility is no substitute for proper compliance
testing. The media API is complex because video is complex. It is impossible
to review code and catch all the little mistakes, but compliance tests can
go far in verifying driver code and also catching core code regressions.

I have yet to see a new driver that didn't fail at least one v4l2-compliance
test, and I very much doubt that existing subdev/media drivers would do any
different.

It would be very interesting if you would test it as well on DVB devices.
You should be able to run v4l2-compliance on the media device. There may
well be bugs in the tests for DVB devices. But that might also be caused
by incomplete documentation in the spec.

Regards,

Hans

Re: [PATCH 0/2] DW9807 DT binding and driver patches

[Sakari Ailus]

Hi Andy,

On Thu, Feb 01, 2018 at 11:47:46PM +0800, Andy Yeh wrote:
> From: Alan Chiang 
> 
> Hi Sakari and Tomasz,
> 
> The two patches are the DT binding and driver for DW9807 VCM controller.

Thanks for the update.

Next time when you're sending a patchset, could you send the patches
together rather than individually? Also, please prepare them using git
format-patch as a set, not individually.

-- 
Kind regards,

Sakari Ailus
sakari.ai...@linux.intel.com

Re: [RFC PATCH] media.h: reorganize header to make it easier to understand

[Hans Verkuil]

Since this patch is hard to read, here is the 'post-patch' header:

 cut here --
/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
/*
 * Multimedia device API
 *
 * Copyright (C) 2010 Nokia Corporation
 *
 * Contacts: Laurent Pinchart 
 *   Sakari Ailus 
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 2 as
 * published by the Free Software Foundation.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 */

#ifndef __LINUX_MEDIA_H
#define __LINUX_MEDIA_H

#ifndef __KERNEL__
#include 
#endif
#include 
#include 
#include 

struct media_device_info {
char driver[16];
char model[32];
char serial[40];
char bus_info[32];
__u32 media_version;
__u32 hw_revision;
__u32 driver_version;
__u32 reserved[31];
};

/*
 * Base number ranges for entity functions
 *
 * NOTE: Userspace should not rely on these ranges to identify a group
 * of function types, as newer functions can be added with any name within
 * the full u32 range.
 *
 * Some older functions use the MEDIA_ENT_F_OLD_*_BASE range. Do not
 * changes this, this is for backwards compatibility. When adding new
 * functions always use MEDIA_ENT_F_BASE.
 */
#define MEDIA_ENT_F_BASE0x
#define MEDIA_ENT_F_OLD_BASE0x0001
#define MEDIA_ENT_F_OLD_SUBDEV_BASE 0x0002

/*
 * Initial value to be used when a new entity is created
 * Drivers should change it to something useful.
 */
#define MEDIA_ENT_F_UNKNOWN MEDIA_ENT_F_BASE

/*
 * Subdevs are initialized with MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN in order
 * to preserve backward compatibility. Drivers must change to the proper
 * subdev type before registering the entity.
 */
#define MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN MEDIA_ENT_F_OLD_SUBDEV_BASE

/*
 * DVB entity functions
 */
#define MEDIA_ENT_F_DTV_DEMOD   (MEDIA_ENT_F_BASE + 0x1)
#define MEDIA_ENT_F_TS_DEMUX(MEDIA_ENT_F_BASE + 0x2)
#define MEDIA_ENT_F_DTV_CA  (MEDIA_ENT_F_BASE + 0x3)
#define MEDIA_ENT_F_DTV_NET_DECAP   (MEDIA_ENT_F_BASE + 0x4)

/*
 * I/O entity functions
 */
#define MEDIA_ENT_F_IO_V4L  (MEDIA_ENT_F_OLD_BASE + 1)
#define MEDIA_ENT_F_IO_DTV  (MEDIA_ENT_F_BASE + 0x01001)
#define MEDIA_ENT_F_IO_VBI  (MEDIA_ENT_F_BASE + 0x01002)
#define MEDIA_ENT_F_IO_SWRADIO  (MEDIA_ENT_F_BASE + 0x01003)

/*
 * Sensor functions
 */
#define MEDIA_ENT_F_CAM_SENSOR  (MEDIA_ENT_F_OLD_SUBDEV_BASE + 
1)
#define MEDIA_ENT_F_FLASH   (MEDIA_ENT_F_OLD_SUBDEV_BASE + 
2)
#define MEDIA_ENT_F_LENS(MEDIA_ENT_F_OLD_SUBDEV_BASE + 
3)

/*
 * Analog video decoder functions
 */
#define MEDIA_ENT_F_ATV_DECODER (MEDIA_ENT_F_OLD_SUBDEV_BASE + 
4)

/*
 * Digital TV, analog TV, radio and/or software defined radio tuner functions.
 *
 * It is a responsibility of the master/bridge drivers to add connectors
 * and links for MEDIA_ENT_F_TUNER. Please notice that some old tuners
 * may require the usage of separate I2C chips to decode analog TV signals,
 * when the master/bridge chipset doesn't have its own TV standard decoder.
 * On such cases, the IF-PLL staging is mapped via one or two entities:
 * MEDIA_ENT_F_IF_VID_DECODER and/or MEDIA_ENT_F_IF_AUD_DECODER.
 */
#define MEDIA_ENT_F_TUNER   (MEDIA_ENT_F_OLD_SUBDEV_BASE + 
5)

/*
 * Analog TV IF-PLL decoder functions
 *
 * It is a responsibility of the master/bridge drivers to create links
 * for MEDIA_ENT_F_IF_VID_DECODER and MEDIA_ENT_F_IF_AUD_DECODER.
 */
#define MEDIA_ENT_F_IF_VID_DECODER  (MEDIA_ENT_F_BASE + 0x02001)
#define MEDIA_ENT_F_IF_AUD_DECODER  (MEDIA_ENT_F_BASE + 0x02002)

/*
 * Audio entity functions
 */
#define MEDIA_ENT_F_AUDIO_CAPTURE   (MEDIA_ENT_F_BASE + 0x03001)
#define MEDIA_ENT_F_AUDIO_PLAYBACK  (MEDIA_ENT_F_BASE + 0x03002)
#define MEDIA_ENT_F_AUDIO_MIXER (MEDIA_ENT_F_BASE + 0x03003)

/*
 * Processing entity functions
 */
#define MEDIA_ENT_F_PROC_VIDEO_COMPOSER (MEDIA_ENT_F_BASE + 0x4001)
#define MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER  (MEDIA_ENT_F_BASE + 0x4002)
#define MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV   (MEDIA_ENT_F_BASE + 0x4003)
#define MEDIA_ENT_F_PROC_VIDEO_LUT  (MEDIA_ENT_F_BASE + 0x4004)
#define MEDIA_ENT_F_PROC_VIDEO_SCALER   (MEDIA_ENT_F_BASE + 0x4005)
#define MEDIA_ENT_F_PROC_VIDEO_STATISTICS   

[RFC PATCH] media.h: reorganize header to make it easier to understand

[Hans Verkuil]

The media.h public header is very messy. It mixes legacy and 'new' defines
and it is not easy to figure out what should and what shouldn't be used. It
also contains confusing comment that are either out of date or completely
uninteresting for anyone that needs to use this header.

The patch groups all entity functions together, including the 'old' defines
based on the old range base. The reader just wants to know about the available
functions and doesn't care about what range is used.

All legacy defines are moved to the end of the header, so it is easier to
locate them and just ignore them.

The legacy structs in the struct media_entity_desc are put under
#if !defined(__KERNEL__) to prevent the kernel from using them, and this is
also a much more effective signal to the reader that they shouldn't be used
compared to the old method of relying on '#if 1' followed by a comment.

The unused MEDIA_INTF_T_ALSA_* defines are also moved to the end of the header
in the legacy area. They are also dropped from intf_type() in media-entity.c.

All defines are also aligned at the same tab making the header easier to read.

Signed-off-by: Hans Verkuil 
---
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index f7c6d64e6031..3498551e618e 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -64,22 +64,6 @@ static inline const char *intf_type(struct media_interface 
*intf)
return "v4l-swradio";
case MEDIA_INTF_T_V4L_TOUCH:
return "v4l-touch";
-   case MEDIA_INTF_T_ALSA_PCM_CAPTURE:
-   return "alsa-pcm-capture";
-   case MEDIA_INTF_T_ALSA_PCM_PLAYBACK:
-   return "alsa-pcm-playback";
-   case MEDIA_INTF_T_ALSA_CONTROL:
-   return "alsa-control";
-   case MEDIA_INTF_T_ALSA_COMPRESS:
-   return "alsa-compress";
-   case MEDIA_INTF_T_ALSA_RAWMIDI:
-   return "alsa-rawmidi";
-   case MEDIA_INTF_T_ALSA_HWDEP:
-   return "alsa-hwdep";
-   case MEDIA_INTF_T_ALSA_SEQUENCER:
-   return "alsa-sequencer";
-   case MEDIA_INTF_T_ALSA_TIMER:
-   return "alsa-timer";
default:
return "unknown-intf";
}
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index b9b9446095e9..febe28054579 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -15,10 +15,6 @@
  * 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.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */

 #ifndef __LINUX_MEDIA_H
@@ -42,108 +38,65 @@ struct media_device_info {
__u32 reserved[31];
 };

-#define MEDIA_ENT_ID_FLAG_NEXT (1 << 31)
-
-/*
- * Initial value to be used when a new entity is created
- * Drivers should change it to something useful
- */
-#define MEDIA_ENT_F_UNKNOWN0x
-
 /*
  * Base number ranges for entity functions
  *
- * NOTE: those ranges and entity function number are phased just to
- * make it easier to maintain this file. Userspace should not rely on
- * the ranges to identify a group of function types, as newer
- * functions can be added with any name within the full u32 range.
+ * NOTE: Userspace should not rely on these ranges to identify a group
+ * of function types, as newer functions can be added with any name within
+ * the full u32 range.
+ *
+ * Some older functions use the MEDIA_ENT_F_OLD_*_BASE range. Do not
+ * changes this, this is for backwards compatibility. When adding new
+ * functions always use MEDIA_ENT_F_BASE.
  */
-#define MEDIA_ENT_F_BASE   0x
-#define MEDIA_ENT_F_OLD_BASE   0x0001
-#define MEDIA_ENT_F_OLD_SUBDEV_BASE0x0002
+#define MEDIA_ENT_F_BASE   0x
+#define MEDIA_ENT_F_OLD_BASE   0x0001
+#define MEDIA_ENT_F_OLD_SUBDEV_BASE0x0002

 /*
- * DVB entities
+ * Initial value to be used when a new entity is created
+ * Drivers should change it to something useful.
  */
-#define MEDIA_ENT_F_DTV_DEMOD  (MEDIA_ENT_F_BASE + 0x1)
-#define MEDIA_ENT_F_TS_DEMUX   (MEDIA_ENT_F_BASE + 0x2)
-#define MEDIA_ENT_F_DTV_CA (MEDIA_ENT_F_BASE + 0x3)
-#define MEDIA_ENT_F_DTV_NET_DECAP  (MEDIA_ENT_F_BASE + 0x4)
+#define MEDIA_ENT_F_UNKNOWNMEDIA_ENT_F_BASE

 /*
- * I/O entities
+ * Subdevs are initialized with MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN in order
+ * to preserve backward compatibility. Drivers must change to the proper
+ * subdev type before registering the entity.
  */
-#define MEDIA_ENT_F_IO_DTV (MEDIA_ENT_F_BASE + 0x01001)
-#define 

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Mauro Carvalho Chehab]

Hi Hans,

Em Sun, 4 Feb 2018 14:06:42 +0100
Hans Verkuil  escreveu:

> Hi Mauro,
> 
> I'm working on adding proper compliance tests for the MC but I think something
> is missing in the G_TOPOLOGY ioctl w.r.t. pads.
> 
> In several v4l-subdev ioctls you need to pass the pad. There the pad is an 
> index
> for the corresponding entity. I.e. an entity has 3 pads, so the pad argument 
> is
> [0-2].
> 
> The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use that
> in the v4l-subdev ioctls, so how do I translate that to a pad index in my 
> application?
> 
> It seems to be a missing feature in the API. I assume this information is 
> available
> in the core, so then I would add a field to struct media_v2_pad with the pad 
> index
> for the entity.

No, this was designed this way by purpose, back in 2015, when this was
discussed at the first MC workshop. It was a consensus on that time that
parameters like PAD index, PAD name, etc should be implemented via the
properties API, as proposed by Sakari [1]. We also had a few discussions 
about that on both IRC and ML.

[1] See: https://linuxtv.org/news.php?entry=2015-08-17.mchehab

3.3 Action items
...
media_properties: RFC userspace API: Sakari

Unfortunately, Sakari never found the time to send us a patch series
implementing a properties API, even as RFC.

One of the other missing features is to add a new version for setting
links (I guess we called it as S_LINKS_V2 at the meetings there).
The hole idea is to provide a way to setup the entire pipeline using
a single ioctl, on an atomic way.

The big problem with pad indexes happen on entities that have PADs with
different types of signal input or output, like for example a TV tuner.

On almost all PC consumers hardware supported by the Kernel, the same
PCI/USB ID may come with different types of hardware. This may also
happen with embedded TV sets, as, depending on the market is is
sold, the same product may come with a different tuner.

A "classic" TV tuner usually has those types of output signals:

- analog TV luminance IF;
- analog TV chrominance IF [1];
- digital TV OFDM IF;
- audio IF;

[1] As both luminance and chrominance should go to the same TV
demod, in practice, we can group both signals together on a
single pad.

More modern tuners also have an audio demod integrated, meaning that
they also have another PAD:
- digital mono or stereo audio.

At the simplest possible scenario, let's say that we have a TV device
has those entities (among others), each with a single PAD input:

- entity #0: a TV tuner;
- entity #1: an audio demod;
- entity #2: an analog TV demod;
- entity #3: a digital TV demod.

And the TV tuner has 4 output pads:

- pad #0 -> analog TV luminance/chrominance;
- pad #1 -> digital TV IF;
- pad #2 -> audio IF;
- pad #3 -> digital audio.

There, pad #0 can only be connected to entity #2. You cannot
connect any other pad to entity #2. The same apply to every other
TV tuner output PAD.

In this specific scenario, entity #1 can only be connected to pad #2,
and pad #3 can't be connected anywhere, as, on this model opted to
have a separate audio demod, and didn't wired the digital audio output.
Yet, the subdev has it.

Another TV set may have different pad numbers - placing them even on a
different order, and opting to use the audio demod tuner, wiring the
digital audio output.

Currently, there's no way for an userspace application to know what pads
can be linked to each entity, as there's no way to tell userspace what
kind of signal each pad supports.

In any case, the Kernel should either detect the tuner model or know
it (via a different DT entry), but userspace need such information,
in order to be able to properly set the pipelines.

So, what we really need here is passing an optional set of properties
to userspace in order to allow it to do the right thing.

Yet, I agree with you: we should not wait anymore for a properties API,
as it seems unlikely that we'll add support for it anytime soon.

So, let's add two fields there:
- pad index number;
- pad type.

In order to make things easy, I guess the best would be to use a string
for the pad type, and fill it only for entities where it is relevant
(like TV tuners and demods).

> Next time we add new public API features I want to see compliance tests before
> accepting it. It's much too easy to overlook something, either in the design 
> or
> in a driver or in the documentation, so this is really, really needed IMHO.

We added a test tool for G_TOPOLOGY on that time.

I doubt that writing test/compliance tools in advance will solve all API
gaps. The thing is that new features will take some time to be used on
real apps. Some things are only visible when real apps start using the
new API features.


Thanks,
Mauro

media_device.c question: can this workaround be removed?

[Hans Verkuil]

The function media_device_enum_entities() has this workaround:

/*
 * Workaround for a bug at media-ctl <= v1.10 that makes it to
 * do the wrong thing if the entity function doesn't belong to
 * either MEDIA_ENT_F_OLD_BASE or MEDIA_ENT_F_OLD_SUBDEV_BASE
 * Ranges.
 *
 * Non-subdevices are expected to be at the MEDIA_ENT_F_OLD_BASE,
 * or, otherwise, will be silently ignored by media-ctl when
 * printing the graphviz diagram. So, map them into the devnode
 * old range.
 */
if (ent->function < MEDIA_ENT_F_OLD_BASE ||
ent->function > MEDIA_ENT_F_TUNER) {
if (is_media_entity_v4l2_subdev(ent))
entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
else if (ent->function != MEDIA_ENT_F_IO_V4L)
entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
}

But this means that the entity type returned by ENUM_ENTITIES just overwrites
perfectly fine types by bogus values and thus the returned value differs
from that returned by G_TOPOLOGY.

Can we please, please remove this workaround? I have no idea why a workaround
for media-ctl of all things ever made it in the kernel.

I'm adding media support to the vivid driver and because of this media-ctl -p
gives me this:

Device topology
- entity 1: vivid-000-vid-cap (1 pad, 0 link)
type Node subtype V4L flags 0
device node name /dev/video0
pad0: Source

- entity 5: vivid-000-vid-out (1 pad, 0 link)
type Node subtype V4L flags 0
device node name /dev/video1
pad0: Sink

- entity 9: vivid-000-vbi-cap (1 pad, 0 link)
type Unknown subtype Unknown flags 0
pad0: Source

- entity 13: vivid-000-vbi-out (1 pad, 0 link)
 type Unknown subtype Unknown flags 0
pad0: Sink

- entity 17: vivid-000-sdr-cap (1 pad, 0 link)
 type Unknown subtype Unknown flags 0
pad0: Source

So VBI and SDR report the 'Unknown' type whereas 'v4l2-ctl -D -d /dev/vbi0' 
(which
uses G_TOPOLOGY) gives me:

Interface Info:
ID   : 0x030b
Type : V4L VBI
Entity Info:
ID   : 0x0009 (9)
Name : vivid-000-vbi-cap
Function : VBI I/O
Pad 0x010a   : Source

That's how it should be.



Never again should we allow new userspace APIs without:

1) Proper compliance tests
2) Adding support for the new API to v4l2-ctl (or related v4l-utils apps)
3) If the new API replaces old defines with new defines (e.g.
   #define MEDIA_ENT_T_DEVNODE_V4L MEDIA_ENT_F_IO_V4L) then everything
   in v4l-utils that uses the old define should be updated to the new
   define.
4) If reasonable add support for the new API to at least one of the
   virtual drivers (vivid, vimc, vim2m) so this API can be tested without
   needing specialized hardware.

The MC API did none of this and how on earth are end-users able to work with
this if we have horribly inconsistent behavior like this?

BTW, uapi/linux/media.h is an utter mess. I'll see if I can make a patch to
make it more understandable. Right now it is extremely hard to tell which
define is legacy and which isn't.



Regards,

Hans

[PATCH] media.h: fix confusing typo in comment

[Hans Verkuil]

Subdevs are initialized with MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN, not
MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN.

Signed-off-by: Hans Verkuil 
---
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index b9b9446095e9..573da38a21c3 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -131,7 +131,7 @@ struct media_device_info {
  * with the legacy v1 API.The number range is out of range by purpose:
  * several previously reserved numbers got excluded from this range.
  *
- * Subdevs are initialized with MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN,
+ * Subdevs are initialized with MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN,
  * in order to preserve backward compatibility.
  * Drivers must change to the proper subdev type before
  * registering the entity.

Re: Compliance tests for new public API features

[Alexandre Courbot]

Hi Hans,

On Sun, Feb 4, 2018 at 10:30 PM, Hans Verkuil  wrote:
> Hi Gustavo, Alexandre,
>
> As you may have seen I have been extending the v4l2-compliance utility with 
> tests
> for v4l-subdevX and mediaX devices. In the process of doing that I promptly
> found a bunch of bugs. Mostly little things that are easy to fix, but much
> harder to find without proper tests.
>
> You are both working on new API additions and I want to give a heads-up that
> I want to have patches for v4l2-compliance that test the new additions to a
> reasonable extent.
>
> I say 'reasonable' because I suspect that e.g. in-fence support might be hard
> to test. But out-fences can definitely be tested and for in-fences you can
> at minimum test what happens when you give it an invalid file descriptor.
>
> For the request API is it obviously too early to start writing tests, but
> as the API crystallizes you may consider starting to work on it.
>
> I've decided to be more strict about this based on my experiences. I'm more
> than happy to assist and give advice, especially since this is a bit of a late
> notice, particularly for Gustavo.
>
> But every time we skip this step it bites us in the ass later on.
>
> It is also highly recommended to add fence/request support to the vivid and
> vim2m drivers. It makes it much easier to find regressions in the API due to
> future changes since you don't need 'real' hardware for these drivers.
>
> Again my apologies for the late notice, but it is better to make these tests
> now while you are working on the new feature, rather than doing it much later
> and finding all sorts of gremlins in the API.

I completely agree with your reasoning and will consider updating
v4l2-compliance as soon as we have something stable on the user-facing
side (which hopefully should be close by now).

Alex.

Re: [PATCH] [BUGFIX] media: vb2: Fix videobuf2 to map correct area

[Marek Szyprowski]

Hi Masami,

On 2018-02-05 03:30, Masami Hiramatsu wrote:

Fixes vb2_vmalloc_get_userptr() to ioremap correct area.
Since the current code does ioremap the page address, if the offset > 0,
it does not do ioremap the last page and results in kernel panic.

This fixes to pass the page address + offset to ioremap so that ioremap
can map correct area. Also, this uses __pfn_to_phys() to get the physical
address of given PFN.

Signed-off-by: Masami Hiramatsu 
Reported-by: Takao Orito 
---
  drivers/media/v4l2-core/videobuf2-vmalloc.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c 
b/drivers/media/v4l2-core/videobuf2-vmalloc.c
index 3a7c80cd1a17..896f2f378b40 100644
--- a/drivers/media/v4l2-core/videobuf2-vmalloc.c
+++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c
@@ -106,7 +106,7 @@ static void *vb2_vmalloc_get_userptr(struct device *dev, 
unsigned long vaddr,
if (nums[i-1] + 1 != nums[i])
goto fail_map;
buf->vaddr = (__force void *)
-   ioremap_nocache(nums[0] << PAGE_SHIFT, size);
+   ioremap_nocache(__pfn_to_phys(nums[0]) + offset, size);


Thanks for reporting this issue. However the above line doesn't look like
a proper fix. Please note that at the end of that function there is already
"buf->vaddr += offset;".

IMHO the proper fix is to create a larger mapping, which would include the
in-page start offset:

ioremap_nocache(__pfn_to_phys(nums[0]), offset + size);

BTW, thanks for updating "<< PAGE_SHIFT" to better __pfn_to_phys() macro!


} else {
buf->vaddr = vm_map_ram(frame_vector_pages(vec), n_pages, -1,
PAGE_KERNEL);






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