Re: [PATCH 0/2 v6] uvcvideo: asynchronous controls
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
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, &timingscap) != 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, Enab
Re: Please help test the new v4l-subdev support in v4l2-compliance
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, &timingscap) != 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 VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not
cron job: media_tree daily build: ERRORS
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
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
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?
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 MEDIA_ENT_F_PROC_VIDEO_SCALER in > >
Re: media_device.c question: can this workaround be removed?
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
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
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
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
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
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
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
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
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(&ir->timer, ir->polling * 100); + missed = hrtimer_forward_now(&ir->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(&ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); ir->timer.function = cx88_ir_work; - hrtimer_start(&ir->timer, ir->polling * 100, + hrtimer_start(&ir->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
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
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
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
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, &ov965x->sd, "Changed frame interval to %u us\n", -fiv->interval.numerator * 100 / fiv->interval.denominator); + v4l2_dbg(1, debug, &ov965x->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
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
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
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
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
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
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
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 unsigned int, v4l2_query_ext_ctrl> >]’: > /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 v4l2_query_ext_ctrl> >, std::less, > std::allocator > > >::const_iterator {aka std::_Rb_tree_const_iterator int, v4l2_query_ext_ctrl> >}’ 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
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 unsigned int, v4l2_query_ext_ctrl> >]’: > /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 v4l2_query_ext_ctrl> >, std::less, > std::allocator > > >::const_iterator {aka std::_Rb_tree_const_iterator int, v4l2_query_ext_ctrl> >}’ 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 int, v4l2_query_ext_ctrl> >; _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 v4l2_query_ext_ctrl> >, std::less, > std::allocator > > >::const_iterator {aka std::_Rb_tree_const_iterator int, v4l2_query_ext_ctrl> >}’ 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 v4l2_query_ext_ctrl> >, std::less, > std::allocator > > >::const_iterator {aka std::_Rb_tree_const_iterator int, v4l2_query_ext_ctrl> >}’ 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 ‘st
Re: Please help test the new v4l-subdev support in v4l2-compliance
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
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, from ctrl-tab.cpp:20: /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:
Re: media_device.c question: can this workaround be removed?
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 > [fmt:RGB888_1X24/640x480 field:n
Re: Please help test the new v4l-subdev support in v4l2-compliance
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
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)’: media-info.cpp:120:39: error: no matching
Re: media_device.c question: can this workaround be removed?
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 gives
Re: [PATCH 5/5] add default control values as module parameters
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
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 v4l2_streamparm *p) >
[PATCH 4/5] add V4L2 control functions
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 = { .vidioc_g_input
[PATCH 3/5] add video control register handlers
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
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
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
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
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
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?
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: vivid-000-sdr-
Re: MEDIA_IOC_G_TOPOLOGY and pad indices
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 > >> (entity ID + pad index) tuple both uniquely identify the pad. > >>
Re: [PATCH v5 4/5] media: ov5640: add support of DVP parallel interface
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
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&D Institute Poland > -- Masami Hiramatsu
Re: MEDIA_IOC_G_TOPOLOGY and pad indices
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 index so I can call SUBDEV_S_FMT at all! >
Re: [RFC PATCH] media-device: add index field to media_v2_pad
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, &kpad, 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
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
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 problem is not at S_FMT. It happens before
[PATCH] drivers: staging: media: atomisp: pci: atomisp2: css2400: fix misspellings
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
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, &kpad, 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
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, &kpad, 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?
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
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 the better alternative. The main reas
Re: [PATCH 2/2] media: dt-bindings: Add bindings for Dongwoon DW9807 voice coil
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?
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
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 number; So should I also
RE: [PATCH v5 4/5] media: ov5640: add support of DVP parallel interface
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
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
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
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 (MEDIA_ENT_F_BASE + 0x4006) /* * Switch and bridge entity functions */
[RFC PATCH] media.h: reorganize header to make it easier to understand
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 MEDIA_ENT_F_IO_VBI (M
Re: MEDIA_IOC_G_TOPOLOGY and pad indices
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?
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
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
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
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&D Institute Poland