Re: [RFP] Which V4L2 ioctls could be replaced by better versions?
Hi Nicolas, On Sat, Oct 27, 2018 at 6:38 PM Nicolas Dufresne wrote: > > Le lundi 22 octobre 2018 à 12:37 +0900, Tomasz Figa a écrit : > > Hi Philipp, > > > > On Mon, Oct 22, 2018 at 1:28 AM Philipp Zabel wrote: > > > > > > On Wed, Oct 03, 2018 at 05:24:39PM +0900, Tomasz Figa wrote: > > > [...] > > > > > Yes, but that would fall in a complete redesign I guess. The buffer > > > > > allocation scheme is very inflexible. You can't have buffers of two > > > > > dimensions allocated at the same time for the same queue. Worst, you > > > > > cannot leave even 1 buffer as your scannout buffer while reallocating > > > > > new buffers, this is not permitted by the framework (in software). As > > > > > a > > > > > side effect, there is no way to optimize the resolution changes, you > > > > > even have to copy your scannout buffer on the CPU, to free it in order > > > > > to proceed. Resolution changes are thus painfully slow, by design. > > > > > > [...] > > > > Also, I fail to understand the scanout issue. If one exports a vb2 > > > > buffer to a DMA-buf and import it to the scanout engine, it can keep > > > > scanning out from it as long as it want, because the DMA-buf will hold > > > > a reference on the buffer, even if it's removed from the vb2 queue. > > > > > > REQBUFS 0 fails if the vb2 buffer is still in use, including from dmabuf > > > attachments: vb2_buffer_in_use checks the num_users memop. The refcount > > > returned by num_users shared between the vmarea handler and dmabuf ops, > > > so any dmabuf attachment counts towards in_use. > > > > Ah, right. I've managed to completely forget about it, since we have a > > downstream patch that we attempted to upstream earlier [1], but didn't > > have a chance to follow up on the comments and there wasn't much > > interest in it in general. > > > > [1] https://lore.kernel.org/patchwork/patch/607853/ > > > > Perhaps it would be worth reviving? > > In this patch we should consider a way to tell userspace that this has > been opt in, otherwise existing userspace will have to remain using > sub-optimal copy based reclaiming in order to ensure that renegotiation > can work on older kernel tool. At worst someone could probably do trial > and error (reqbufs(1)/mmap/reqbufs(0)) but on CMA with large buffers > this introduces extra startup time. Would such REQBUFS dance be really needed? Couldn't one simply try reqbufs(0) when it's really needed and if it fails then do the copy, otherwise just proceed normally? Best regards, Tomasz
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: Thu Nov 8 05:00:11 CET 2018 media-tree git hash:fbe57dde7126d1b2712ab5ea93fb9d15f89de708 media_build git hash: 0c8bb27f3aaa682b9548b656f77505c3d1f11e71 v4l-utils git hash: d0a218fff81dd7930d3bd4bb1423e24d6fce4034 edid-decode git hash: 5eeb151a748788666534d6ea3da07f90400d24c2 gcc version:i686-linux-gcc (GCC) 8.2.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.18.0-2-amd64 linux-git-arm-at91: WARNINGS linux-git-arm-davinci: WARNINGS linux-git-arm-multi: WARNINGS linux-git-arm-pxa: WARNINGS linux-git-arm-stm32: WARNINGS linux-git-arm64: WARNINGS linux-git-i686: WARNINGS linux-git-mips: OK linux-git-powerpc64: WARNINGS linux-git-sh: OK linux-git-x86_64: WARNINGS Check COMPILE_TEST: OK linux-3.10.108-i686: ERRORS linux-3.10.108-x86_64: ERRORS linux-3.11.10-i686: ERRORS linux-3.11.10-x86_64: ERRORS linux-3.12.74-i686: ERRORS linux-3.12.74-x86_64: ERRORS linux-3.13.11-i686: ERRORS linux-3.13.11-x86_64: ERRORS linux-3.14.79-i686: ERRORS linux-3.14.79-x86_64: ERRORS linux-3.15.10-i686: ERRORS linux-3.15.10-x86_64: ERRORS linux-3.16.57-i686: ERRORS linux-3.16.57-x86_64: ERRORS linux-3.17.8-i686: ERRORS linux-3.17.8-x86_64: ERRORS linux-3.18.123-i686: ERRORS linux-3.18.123-x86_64: ERRORS linux-3.19.8-i686: ERRORS linux-3.19.8-x86_64: ERRORS linux-4.0.9-i686: ERRORS linux-4.0.9-x86_64: ERRORS linux-4.1.52-i686: ERRORS linux-4.1.52-x86_64: ERRORS linux-4.2.8-i686: ERRORS linux-4.2.8-x86_64: ERRORS linux-4.3.6-i686: ERRORS linux-4.3.6-x86_64: ERRORS linux-4.4.159-i686: ERRORS linux-4.4.159-x86_64: ERRORS linux-4.5.7-i686: ERRORS linux-4.5.7-x86_64: ERRORS linux-4.6.7-i686: ERRORS linux-4.6.7-x86_64: ERRORS linux-4.7.10-i686: ERRORS linux-4.7.10-x86_64: ERRORS linux-4.8.17-i686: ERRORS linux-4.8.17-x86_64: ERRORS linux-4.9.131-i686: ERRORS linux-4.9.131-x86_64: ERRORS linux-4.10.17-i686: ERRORS linux-4.10.17-x86_64: ERRORS linux-4.11.12-i686: ERRORS linux-4.11.12-x86_64: ERRORS linux-4.12.14-i686: ERRORS linux-4.12.14-x86_64: ERRORS linux-4.13.16-i686: OK linux-4.13.16-x86_64: OK linux-4.14.74-i686: OK linux-4.14.74-x86_64: OK linux-4.15.18-i686: OK linux-4.15.18-x86_64: OK linux-4.16.18-i686: OK linux-4.16.18-x86_64: OK linux-4.17.19-i686: OK linux-4.17.19-x86_64: OK linux-4.18.12-i686: OK linux-4.18.12-x86_64: OK linux-4.19.1-i686: OK linux-4.19.1-x86_64: OK linux-4.20-rc1-i686: OK linux-4.20-rc1-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Logs weren't copied as they are too large (1688 kB) The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
media: ov8856: Add support for OV8856 sensor
This patch adds driver for Omnivision's ov8856 sensor,
the driver supports following features:
- manual exposure/gain(analog and digital) control support
- two link frequencies
- VBLANK/HBLANK support
- test pattern support
- media controller support
- runtime pm support
- supported resolutions
+ 3280x2464 at 30FPS
+ 1640x1232 at 30FPS
Signed-off-by: Ben Kao
---
drivers/media/i2c/Kconfig | 12 +
drivers/media/i2c/Makefile |1 +
drivers/media/i2c/ov8856.c | 1296
3 files changed, 1309 insertions(+)
create mode 100644 drivers/media/i2c/ov8856.c
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 704af21..4fdcb92 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -806,6 +806,18 @@ config VIDEO_OV7740
This is a Video4Linux2 sensor driver for the OmniVision
OV7740 VGA camera sensor.
+config VIDEO_OV8856
+ tristate "OmniVision OV8856 sensor support"
+ depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+ depends on MEDIA_CAMERA_SUPPORT
+ select V4L2_FWNODE
+ help
+ This is a Video4Linux2 sensor driver for the OmniVision
+ OV8856 camera sensor.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ov8856.
+
config VIDEO_OV9650
tristate "OmniVision OV9650/OV9652 sensor support"
depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 260d4d9..47accb0 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -78,6 +78,7 @@ obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
obj-$(CONFIG_VIDEO_OV772X) += ov772x.o
obj-$(CONFIG_VIDEO_OV7740) += ov7740.o
+obj-$(CONFIG_VIDEO_OV8856) += ov8856.o
obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
obj-$(CONFIG_VIDEO_OV13858) += ov13858.o
obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
new file mode 100644
index 000..feae739
--- /dev/null
+++ b/drivers/media/i2c/ov8856.c
@@ -0,0 +1,1296 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Intel Corporation.
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#ifndef V4L2_CID_DIGITAL_GAIN
+#define V4L2_CID_DIGITAL_GAIN V4L2_CID_GAIN
+#endif
+
+#define OV8856_REG_VALUE_08BIT 1
+#define OV8856_REG_VALUE_16BIT 2
+#define OV8856_REG_VALUE_24BIT 3
+
+#define OV8856_LINK_FREQ_360MHZ36000ULL
+#define OV8856_LINK_FREQ_180MHZ18000ULL
+#define OV8856_SCLK14400ULL
+#define OV8856_MCLK1920
+#define OV8856_DATA_LANES 4
+
+#define OV8856_REG_CHIP_ID 0x300a
+#define OV8856_CHIP_ID 0x00885a
+
+#define OV8856_REG_MODE_SELECT 0x0100
+#define OV8856_MODE_STANDBY0x00
+#define OV8856_MODE_STREAMING 0x01
+
+/* vertical-timings from sensor */
+#define OV8856_REG_VTS 0x380e
+#define OV8856_VTS_MAX 0x7fff
+
+/* horizontal-timings from sensor */
+#define OV8856_REG_HTS 0x380c
+
+/* Exposure controls from sensor */
+#define OV8856_REG_EXPOSURE0x3500
+#defineOV8856_EXPOSURE_MIN 6
+#define OV8856_EXPOSURE_MAX_MARGIN 6
+#defineOV8856_EXPOSURE_STEP1
+
+/* Analog gain controls from sensor */
+#define OV8856_REG_ANALOG_GAIN 0x3508
+#defineOV8856_ANAL_GAIN_MIN128
+#defineOV8856_ANAL_GAIN_MAX2047
+#defineOV8856_ANAL_GAIN_STEP 1
+
+/* Digital gain controls from sensor */
+#define OV8856_REG_MWB_R_GAIN 0x5019
+#define OV8856_REG_MWB_G_GAIN 0x501b
+#define OV8856_REG_MWB_B_GAIN 0x501d
+#define OV8856_DGTL_GAIN_MIN 0
+#define OV8856_DGTL_GAIN_MAX 4095
+#define OV8856_DGTL_GAIN_STEP 1
+#define OV8856_DGTL_GAIN_DEFAULT 1024
+
+/* Test Pattern Control */
+#define OV8856_REG_TEST_PATTERN0x5e00
+#define OV8856_TEST_PATTERN_ENABLE BIT(7)
+#define OV8856_TEST_PATTERN_BAR_SHIFT 2
+
+#define to_ov8856(_sd) container_of(_sd, struct ov8856, sd)
+
+enum {
+ OV8856_LINK_FREQ_720MBPS,
+ OV8856_LINK_FREQ_360MBPS,
+};
+
+struct ov8856_reg {
+ u16 address;
+ u8 val;
+};
+
+struct ov8856_reg_list {
+ u32 num_of_regs;
+ const struct ov8856_reg *regs;
+};
+
+struct ov8856_link_freq_config {
+ const struct ov8856_reg_list reg_list;
+};
+
+struct ov8856_mode {
+ /* Frame width in pixels */
+ u32 width;
+
+ /* Frame height in pixels */
+ u32 height;
+
+ /* Horizontal timining size */
+ u32 hts;
+
+ /* Default vertical timining size */
+ u32 vts_def;
+
+ /* Min vertical timining size
RE: [PATCH] media: imx258: remove test pattern map from driver
Hi Sakari
Thanks for your feedback.
I will update to the next patch.
B.R.,
Jason
-Original Message-
From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
Sent: Wednesday, November 7, 2018 6:32 PM
To: Chen, JasonX Z
Cc: linux-media@vger.kernel.org; Yeh, Andy ;
tf...@chromium.org
Subject: Re: [PATCH] media: imx258: remove test pattern map from driver
Hi Jason,
Thanks for the patch.
On Wed, Nov 07, 2018 at 03:22:23PM +0800, jasonx.z.c...@intel.com wrote:
> From: "Chen, JasonX Z"
>
> Test Pattern mode be picked at HAL instead of driver.
> do a FLIP when userspace use test pattern mode.
> add entity_ops for validating imx258 link.
>
> Signed-off-by: Chen, JasonX Z
> ---
> drivers/media/i2c/imx258.c | 28
> 1 file changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 31a1e22..71f9875 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -62,11 +62,6 @@
>
> /* Test Pattern Control */
> #define IMX258_REG_TEST_PATTERN 0x0600
> -#define IMX258_TEST_PATTERN_DISABLE 0
> -#define IMX258_TEST_PATTERN_SOLID_COLOR 1
> -#define IMX258_TEST_PATTERN_COLOR_BARS 2
> -#define IMX258_TEST_PATTERN_GREY_COLOR 3
> -#define IMX258_TEST_PATTERN_PN9 4
>
> /* Orientation */
> #define REG_MIRROR_FLIP_CONTROL 0x0101
> @@ -504,20 +499,12 @@ struct imx258_mode {
>
> static const char * const imx258_test_pattern_menu[] = {
> "Disabled",
> - "Color Bars",
> "Solid Color",
> + "Color Bars",
> "Grey Color Bars",
> "PN9"
> };
>
> -static const int imx258_test_pattern_val[] = {
> - IMX258_TEST_PATTERN_DISABLE,
> - IMX258_TEST_PATTERN_COLOR_BARS,
> - IMX258_TEST_PATTERN_SOLID_COLOR,
> - IMX258_TEST_PATTERN_GREY_COLOR,
> - IMX258_TEST_PATTERN_PN9,
> -};
> -
> /* Configurations for supported link frequencies */
> #define IMX258_LINK_FREQ_634MHZ 63360ULL
> #define IMX258_LINK_FREQ_320MHZ 32000ULL
> @@ -752,7 +739,6 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> container_of(ctrl->handler, struct imx258, ctrl_handler);
> struct i2c_client *client = v4l2_get_subdevdata(>sd);
> int ret = 0;
> -
This seems like an unrelated change, and it's a common (and usually good)
practice to add an empty line after variable declarations.
> /*
>* Applying V4L2 control value only happens
>* when power is up for streaming
> @@ -778,13 +764,10 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> case V4L2_CID_TEST_PATTERN:
> ret = imx258_write_reg(imx258, IMX258_REG_TEST_PATTERN,
> IMX258_REG_VALUE_16BIT,
> - imx258_test_pattern_val[ctrl->val]);
> -
> + ctrl->val);
> ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
> IMX258_REG_VALUE_08BIT,
> - ctrl->val == imx258_test_pattern_val
> - [IMX258_TEST_PATTERN_DISABLE] ?
> - REG_CONFIG_MIRROR_FLIP :
> + !ctrl->val?REG_CONFIG_MIRROR_FLIP :
> REG_CONFIG_FLIP_TEST_PATTERN);
> break;
> default:
> @@ -1105,6 +1088,10 @@ static int imx258_identify_module(struct imx258
> *imx258)
> .pad = _pad_ops,
> };
>
> +static const struct media_entity_operations imx258_subdev_entity_ops = {
> + .link_validate = v4l2_subdev_link_validate, };
> +
This seems unrelated as well. Do you need the link validation for something?
As far as I understand, the driver exposes a single source pad; therefore the
link_validate op will never be called.
> static const struct v4l2_subdev_internal_ops imx258_internal_ops = {
> .open = imx258_open,
> };
> @@ -1250,6 +1237,7 @@ static int imx258_probe(struct i2c_client
> *client)
>
> /* Initialize subdev */
> imx258->sd.internal_ops = _internal_ops;
> + imx258->sd.entity.ops = _subdev_entity_ops;
Ditto.
> imx258->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> imx258->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
[PATCH] media: imx258: remove test pattern map from driver
From: "Chen, JasonX Z"
change bayer order when using test pattern mode.
remove test pattern mapping method
Signed-off-by: Chen, JasonX Z
---
drivers/media/i2c/imx258.c | 23 ---
1 file changed, 4 insertions(+), 19 deletions(-)
diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index 31a1e22..5a72b4a 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -62,11 +62,6 @@
/* Test Pattern Control */
#define IMX258_REG_TEST_PATTERN0x0600
-#define IMX258_TEST_PATTERN_DISABLE0
-#define IMX258_TEST_PATTERN_SOLID_COLOR1
-#define IMX258_TEST_PATTERN_COLOR_BARS 2
-#define IMX258_TEST_PATTERN_GREY_COLOR 3
-#define IMX258_TEST_PATTERN_PN94
/* Orientation */
#define REG_MIRROR_FLIP_CONTROL0x0101
@@ -504,20 +499,12 @@ struct imx258_mode {
static const char * const imx258_test_pattern_menu[] = {
"Disabled",
- "Color Bars",
"Solid Color",
+ "Color Bars",
"Grey Color Bars",
"PN9"
};
-static const int imx258_test_pattern_val[] = {
- IMX258_TEST_PATTERN_DISABLE,
- IMX258_TEST_PATTERN_COLOR_BARS,
- IMX258_TEST_PATTERN_SOLID_COLOR,
- IMX258_TEST_PATTERN_GREY_COLOR,
- IMX258_TEST_PATTERN_PN9,
-};
-
/* Configurations for supported link frequencies */
#define IMX258_LINK_FREQ_634MHZ63360ULL
#define IMX258_LINK_FREQ_320MHZ32000ULL
@@ -757,6 +744,7 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
* Applying V4L2 control value only happens
* when power is up for streaming
*/
+
if (pm_runtime_get_if_in_use(>dev) == 0)
return 0;
@@ -778,13 +766,10 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
case V4L2_CID_TEST_PATTERN:
ret = imx258_write_reg(imx258, IMX258_REG_TEST_PATTERN,
IMX258_REG_VALUE_16BIT,
- imx258_test_pattern_val[ctrl->val]);
-
+ ctrl->val);
ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
IMX258_REG_VALUE_08BIT,
- ctrl->val == imx258_test_pattern_val
- [IMX258_TEST_PATTERN_DISABLE] ?
- REG_CONFIG_MIRROR_FLIP :
+ !ctrl->val ? REG_CONFIG_MIRROR_FLIP :
REG_CONFIG_FLIP_TEST_PATTERN);
break;
default:
--
1.9.1
Re: [PATCH] media: xilinx-video: fix bad of_node_put() on endpoint error
Hi Akinobu,
Thanks for the patch.
On Sun, 2018-11-04 at 05:11:10 -0800, Akinobu Mita wrote:
> The fwnode_graph_get_next_endpoint() returns an 'endpoint' node pointer
> with refcount incremented, and refcount of the passed as a previous
> 'endpoint' node is decremented.
>
> So when iterating over all nodes using fwnode_graph_get_next_endpoint(),
> we don't need to call fwnode_handle_put() for each node except for error
> exit paths. Otherwise we get "OF: ERROR: Bad of_node_put() on ..."
> messages.
>
> Fixes: d079f94c9046 ("media: platform: Switch to
> v4l2_async_notifier_add_subdev")
> Cc: Steve Longerbeam
> Cc: Hyun Kwon
> Cc: Laurent Pinchart
> Cc: Mauro Carvalho Chehab
> Signed-off-by: Akinobu Mita
This looks good to me,
Reviewed-by: Hyun Kwon
Thanks,
-hyun
> ---
> drivers/media/platform/xilinx/xilinx-vipp.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c
> b/drivers/media/platform/xilinx/xilinx-vipp.c
> index 574614d..26b13fd 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -377,8 +377,6 @@ static int xvip_graph_parse_one(struct
> xvip_composite_device *xdev,
> goto err_notifier_cleanup;
> }
>
> - fwnode_handle_put(ep);
> -
> /* Skip entities that we have already processed. */
> if (remote == of_fwnode_handle(xdev->dev->of_node) ||
> xvip_graph_find_entity(xdev, remote)) {
> --
> 2.7.4
>
Re: [RFC] Create test script(s?) for regression testing
On 11/07/2018 12:10 PM, Mauro Carvalho Chehab wrote: > Em Wed, 07 Nov 2018 12:06:55 +0200 > Laurent Pinchart escreveu: > >> Hi Hans, >> >> On Wednesday, 7 November 2018 10:05:12 EET Hans Verkuil wrote: >>> On 11/06/2018 08:58 PM, Laurent Pinchart wrote: On Tuesday, 6 November 2018 15:56:34 EET Hans Verkuil wrote: > On 11/06/18 14:12, Laurent Pinchart wrote: >> On Tuesday, 6 November 2018 13:36:55 EET Sakari Ailus wrote: >>> On Tue, Nov 06, 2018 at 09:37:07AM +0100, Hans Verkuil wrote: Hi all, After the media summit (heavy on test discussions) and the V4L2 event regression we just found it is clear we need to do a better job with testing. All the pieces are in place, so what is needed is to combine it and create a script that anyone of us as core developers can run to check for regressions. The same script can be run as part of the kernelci regression testing. >>> >>> I'd say that *some* pieces are in place. Of course, the more there is, >>> the better. >>> >>> The more there are tests, the more important it would be they're >>> automated, preferrably without the developer having to run them on his/ >>> her own machine. >> >> From my experience with testing, it's important to have both a core set >> of tests (a.k.a. smoke tests) that can easily be run on developers' >> machines, and extended tests that can be offloaded to a shared testing >> infrastructure (but possibly also run locally if desired). > > That was my idea as well for the longer term. First step is to do the > basic smoke tests (i.e. run compliance tests, do some (limited) streaming > test). > > There are more extensive (and longer running) tests that can be done, but > that's something to look at later. > We have four virtual drivers: vivid, vim2m, vimc and vicodec. The last one is IMHO not quite good enough yet for testing: it is not fully compliant to the upcoming stateful codec spec. Work for that is planned as part of an Outreachy project. My idea is to create a script that is maintained as part of v4l-utils that loads the drivers and runs v4l2-compliance and possibly other tests against the virtual drivers. > > (adding Shuah) > > IMO, the best would be to have something like that as part of Kernel > self test, as this could give a broader covering than just Kernel CI. > I agree with the broader coverage benefit that comes with adding tests to kselftest. It makes it easier for making changes to tests/tools coupled with kernel/driver changes. Common TAP13 reporting can be taken advantage of without doing any additional work in the tests if author chooses to do so. Tests can be added such that they don't get run by default if there is a reason do so and Kernel CI and other rings can invoke it as a special case if necessary. There are very clear advantages to making these tests part of the kernel source tree. We can discuss at the Kernel Summit next week if you are interested. thanks, -- Shuah
Re: [PATCH v5 8/9] media: uvcvideo: Rename uvc_{un,}init_video()
Hi Kieran,
On Wednesday, 7 November 2018 16:30:46 EET Kieran Bingham wrote:
> On 06/11/2018 23:13, Laurent Pinchart wrote:
> > On Tuesday, 6 November 2018 23:27:19 EET Kieran Bingham wrote:
> >> From: Kieran Bingham
> >>
> >> We have both uvc_init_video() and uvc_video_init() calls which can be
> >> quite confusing to determine the process for each. Now that video
> >> uvc_video_enable() has been renamed to uvc_video_start_streaming(),
> >> adapt these calls to suit the new flow.
> >>
> >> Rename uvc_init_video() to uvc_video_start() and uvc_uninit_video() to
> >> uvc_video_stop().
> >
> > I agree that these functions are badly named and should be renamed. We are
> > however entering the nitpicking territory :-) The two functions do more
> > that starting and stopping, they also allocate and free URBs and the
> > associated buffers. It could also be argued that they don't actually
> > start and stop anything, as beyond URB management, they just queue the
> > URBs initially and kill them. I thus wonder if we could come up with
> > better names.
>
> Well the act of killing (poisoning now) the URBs will certainly stop the
> stream, but I guess submitting the URBs isn't necessarily the key act to
> starting the stream.
>
> I believe that needs the interface to be set correctly, and the buffers
> to be available?
>
> Although - I've just double-checked uvc_{video_start,init_video}() and
> that is indeed what it does?
>
> - start stats
> - Initialise endpoints
>- Perform allocations
> - Submit URBs
>
> Am I missing something? Is there another step that is pivotal to
> starting the USB packet/urb stream flow after this point ?
>
>
> Is it not true that the USB stack will start processing data at
> submitting URB completion callbacks after the end of uvc_video_start();
> and will no longer process data at the end of uvc_video_stop() (and thus
> no more completion callbacks)?
>
> (That's a real question to verify my interpretation)
>
> To me - these functions feel like the real 'start' and 'stop' components
> of the data stream - hence my choice in naming.
The other part of the start operation is committing the streaming parameters
(see uvc_video_start_streaming()). For the stop operation it's issuing a
SET_INTERFACE or CLEAR_FEATURE(HALT) request (see uvc_video_stop_streaming()).
> Is your concern that you would like the functions to be more descriptive
> over their other actions such as? :
>
> uvc_video_initialise_start()
> uvc_video_allocate_init_start()
>
> Or something else? (I don't think those two are good names though)
Probably something else :-) A possibly equally bad proposal would be
uvc_video_start_transfer() and uvc_video_stop_transfer().
--
Regards,
Laurent Pinchart
Re: [RFC] Create test script(s?) for regression testing
Hi Mauro, On Wednesday, 7 November 2018 21:53:20 EET Mauro Carvalho Chehab wrote: > Em Wed, 07 Nov 2018 21:35:32 +0200 Laurent Pinchart escreveu: > > On Wednesday, 7 November 2018 21:10:35 EET Mauro Carvalho Chehab wrote: [snip] > >> I'm with Hans on that matter: better to start with an absolute minimum > >> of dependencies (like just: make, autotools, c, c++, bash), > > > > On a site note, for a new project, we might want to move away from > > autotools. cmake and meson are possible alternatives that are way less > > painful. > > Each toolset has advantages or disadvantages. We all know how > autotools can be painful. > > One bad thing with cmake is that they deprecate stuff. A long-live project > usually require several" backward" compat stuff at cmake files in order > to cope with different behaviors that change as cmake evolves. I don't know how much of a problem that would be. My experience with cmake is good so far, but I haven't used it in a large scale project with 10+ years of contributions :-) > I never used mason myself. It's the build system we picked for libcamera, I expect to provide feedback in the not too distant future. [snip] -- Regards, Laurent Pinchart
Re: [RFC] Create test script(s?) for regression testing
Em Wed, 07 Nov 2018 21:35:32 +0200 Laurent Pinchart escreveu: > Hi Mauro, > > On Wednesday, 7 November 2018 21:10:35 EET Mauro Carvalho Chehab wrote: > > Em Wed, 07 Nov 2018 12:06:55 +0200 Laurent Pinchart escreveu: > > > On Wednesday, 7 November 2018 10:05:12 EET Hans Verkuil wrote: > > >> On 11/06/2018 08:58 PM, Laurent Pinchart wrote: > > >>> On Tuesday, 6 November 2018 15:56:34 EET Hans Verkuil wrote: > > On 11/06/18 14:12, Laurent Pinchart wrote: > > > On Tuesday, 6 November 2018 13:36:55 EET Sakari Ailus wrote: > > >> On Tue, Nov 06, 2018 at 09:37:07AM +0100, Hans Verkuil wrote: > > >>> Hi all, > > >>> > > >>> After the media summit (heavy on test discussions) and the V4L2 > > >>> event regression we just found it is clear we need to do a better > > >>> job with testing. > > >>> > > >>> All the pieces are in place, so what is needed is to combine it > > >>> and create a script that anyone of us as core developers can run to > > >>> check for regressions. The same script can be run as part of the > > >>> kernelci regression testing. > > >> > > >> I'd say that *some* pieces are in place. Of course, the more there > > >> is, the better. > > >> > > >> The more there are tests, the more important it would be they're > > >> automated, preferrably without the developer having to run them on > > >> his/her own machine. > > > > > > From my experience with testing, it's important to have both a core > > > set of tests (a.k.a. smoke tests) that can easily be run on > > > developers' machines, and extended tests that can be offloaded to a > > > shared testing infrastructure (but possibly also run locally if > > > desired). > > > > That was my idea as well for the longer term. First step is to do the > > basic smoke tests (i.e. run compliance tests, do some (limited) > > streaming test). > > > > There are more extensive (and longer running) tests that can be done, > > but that's something to look at later. > > > > >>> We have four virtual drivers: vivid, vim2m, vimc and vicodec. The > > >>> last one is IMHO not quite good enough yet for testing: it is not > > >>> fully compliant to the upcoming stateful codec spec. Work for that > > >>> is planned as part of an Outreachy project. > > >>> > > >>> My idea is to create a script that is maintained as part of > > >>> v4l-utils that loads the drivers and runs v4l2-compliance and > > >>> possibly other tests against the virtual drivers. > > > > (adding Shuah) > > > > IMO, the best would be to have something like that as part of Kernel > > self test, as this could give a broader covering than just Kernel CI. > > > > Yeah, I know that one of the concerns is that the *-compliance stuff > > we have are written in C++ and it is easier to maintain then at > > v4l-utils, but maybe it would be acceptable at kselftest to have a > > test bench there with would download the sources from a git tree > > and then build just the v4l2-compliance stuff, e. g. having a Kernel > > self test target that would do something like: > > > > git clone --depth 1 git://linuxtv.org/v4l-utils.git tests && \ > > cd tests && ./autogen.sh && make tests && ./run_tests.sh > > Let me make sure I understand this properly. Are you proposing to add to > kselftest, which is part of the Linux kernel, and as such benefits from the > level of trust of Linus' tree, and which is run by a very large number of > machines from developer workstations to automated large-scale test > infrastructure, a provision to execute locally code that is downloaded at > runtime from the internet, with all the security issues this implies ? No, I'm not proposing to make it unsafe. The above is just a rogue example to explain an idea. The actual implementation should take security into account. It could, for example, use things like downloading a signed tarball and run it inside a container, use some git tree hosted at kernel.org, etc. > > > (the actual selftest target would likely be different, as it > > should take into account make O=) > > > > If this would be acceptable upstream, then we'll need to stick with the > > output format defined by Kernel Self Test[1]. > > > > [1] I guess it uses the TAP13 format: > > https://testanything.org/tap-version-13-specification.html > > > > >> How about spending a little time to pick a suitable framework for > > >> running the tests? It could be useful to get more informative > > >> reports than just pass / fail. > > > > > > We should keep in mind that other tests will be added later, and the > > > test framework should make that easy. > > > > Since we want to be able to run this on kernelci.org, I think it > > makes sense to let the kernelci folks (Hi Ezequiel!) decide this. > > >>> > > >>> KernelCI
Re: [PATCH] keytable: fix BPF protocol compilation on mips
On 11/7/18 4:36 PM, Sean Young wrote: clang -idirafter /usr/local/include -idirafter +/usr/lib/llvm-6.0/lib/clang/6.0.1/include -idirafter +/usr/include/mips64el-linux-gnuabi64 -idirafter /usr/include +-I../../../include -target bpf -O2 -c grundig.c In file included from grundig.c:5: In file included from ../../../include/linux/lirc.h:10: In file included from /usr/include/linux/types.h:9: In file included from /usr/include/linux/posix_types.h:36: In file included from +/usr/include/mips64el-linux-gnuabi64/asm/posix_types.h:13: /usr/include/mips64el-linux-gnuabi64/asm/sgidefs.h:19:2: error: Use a Linux +compiler or give up. #error Use a Linux compiler or give up. This requires __linux__ to be defined. Thanks! I uploaded a new Debian package. Let's see how it goes: https://buildd.debian.org/status/package.php?p=v4l-utils I'll pick up your other patches soon. -Gregor
Re: [RFC] Create test script(s?) for regression testing
On Wed, 2018-11-07 at 09:05 +0100, Hans Verkuil wrote: > On 11/06/2018 08:58 PM, Laurent Pinchart wrote: > > Hi Hans, > > > > On Tuesday, 6 November 2018 15:56:34 EET Hans Verkuil wrote: > > > On 11/06/18 14:12, Laurent Pinchart wrote: > > > > On Tuesday, 6 November 2018 13:36:55 EET Sakari Ailus wrote: > > > > > On Tue, Nov 06, 2018 at 09:37:07AM +0100, Hans Verkuil wrote: > > > > > > Hi all, > > > > > > > > > > > > After the media summit (heavy on test discussions) and the V4L2 > > > > > > event > > > > > > regression we just found it is clear we need to do a better job with > > > > > > testing. > > > > > > > > > > > > All the pieces are in place, so what is needed is to combine it and > > > > > > create a script that anyone of us as core developers can run to > > > > > > check > > > > > > for regressions. The same script can be run as part of the kernelci > > > > > > regression testing. > > > > > > > > > > I'd say that *some* pieces are in place. Of course, the more there is, > > > > > the better. > > > > > > > > > > The more there are tests, the more important it would be they're > > > > > automated, preferrably without the developer having to run them on > > > > > his/ > > > > > her own machine. > > > > > > > > From my experience with testing, it's important to have both a core set > > > > of > > > > tests (a.k.a. smoke tests) that can easily be run on developers' > > > > machines, > > > > and extended tests that can be offloaded to a shared testing > > > > infrastructure (but possibly also run locally if desired). > > > > > > That was my idea as well for the longer term. First step is to do the > > > basic > > > smoke tests (i.e. run compliance tests, do some (limited) streaming test). > > > > > > There are more extensive (and longer running) tests that can be done, but > > > that's something to look at later. > > > > > > > > > We have four virtual drivers: vivid, vim2m, vimc and vicodec. The > > > > > > last > > > > > > one is IMHO not quite good enough yet for testing: it is not fully > > > > > > compliant to the upcoming stateful codec spec. Work for that is > > > > > > planned > > > > > > as part of an Outreachy project. > > > > > > > > > > > > My idea is to create a script that is maintained as part of > > > > > > v4l-utils > > > > > > that loads the drivers and runs v4l2-compliance and possibly other > > > > > > tests > > > > > > against the virtual drivers. > > > > > > > > > > How about spending a little time to pick a suitable framework for > > > > > running > > > > > the tests? It could be useful to get more informative reports than > > > > > just > > > > > pass / fail. > > > > > > > > We should keep in mind that other tests will be added later, and the > > > > test > > > > framework should make that easy. > > > > > > Since we want to be able to run this on kernelci.org, I think it makes > > > sense > > > to let the kernelci folks (Hi Ezequiel!) decide this. > > > > KernelCI isn't the only test infrastructure out there, so let's not forget > > about the other ones. > > True, but they are putting time and money into this, so they get to choose as > far as I am concerned :-) > Well, we are also resource-constrained, so my idea for the first iteration is to pick the simplest yet useful setup. We plan to leverage existing tests only. Currently xxx-compliance tools are what cover more. I believe that CI and tests should be independent components. > If others are interested and willing to put up time and money, they should let > themselves be known. > > I'm not going to work on such an integration, although I happily accept > patches. > > > > As a developer all I need is a script to run smoke tests so I can catch > > > most > > > regressions (you never catch all). > > > > > > I'm happy to work with them to make any changes to compliance tools and > > > scripts so they fit better into their test framework. > > > > > > The one key requirement to all this is that you should be able to run > > > these > > > tests without dependencies to all sorts of external packages/libraries. > > > > v4l-utils already has a set of dependencies, but those are largely > > manageable. > > For v4l2-compliance we'll install libv4l, which depends on libjpeg. > > That's already too much. You can manually build v4l2-compliance with no > dependencies > whatsoever, but we're missing a Makefile target for that. It's been useful for > embedded systems with poor cross-compile environments. > > It is really very useful to be able to compile those core utilities with no > external libraries other than glibc. You obviously will loose some > functionality > when you compile it that way. > > These utilities are not like a typical application. I really don't care how > many > libraries are linked in by e.g. qv4l2, xawtv, etc. But for v4l2-ctl, > v4l2-compliance, > cec-ctl/follower/compliance (and probably a few others as well) you want a > minimum > of dependencies so you can run
Re: [RFC] Create test script(s?) for regression testing
Hi Mauro, On Wednesday, 7 November 2018 21:10:35 EET Mauro Carvalho Chehab wrote: > Em Wed, 07 Nov 2018 12:06:55 +0200 Laurent Pinchart escreveu: > > On Wednesday, 7 November 2018 10:05:12 EET Hans Verkuil wrote: > >> On 11/06/2018 08:58 PM, Laurent Pinchart wrote: > >>> On Tuesday, 6 November 2018 15:56:34 EET Hans Verkuil wrote: > On 11/06/18 14:12, Laurent Pinchart wrote: > > On Tuesday, 6 November 2018 13:36:55 EET Sakari Ailus wrote: > >> On Tue, Nov 06, 2018 at 09:37:07AM +0100, Hans Verkuil wrote: > >>> Hi all, > >>> > >>> After the media summit (heavy on test discussions) and the V4L2 > >>> event regression we just found it is clear we need to do a better > >>> job with testing. > >>> > >>> All the pieces are in place, so what is needed is to combine it > >>> and create a script that anyone of us as core developers can run to > >>> check for regressions. The same script can be run as part of the > >>> kernelci regression testing. > >> > >> I'd say that *some* pieces are in place. Of course, the more there > >> is, the better. > >> > >> The more there are tests, the more important it would be they're > >> automated, preferrably without the developer having to run them on > >> his/her own machine. > > > > From my experience with testing, it's important to have both a core > > set of tests (a.k.a. smoke tests) that can easily be run on > > developers' machines, and extended tests that can be offloaded to a > > shared testing infrastructure (but possibly also run locally if > > desired). > > That was my idea as well for the longer term. First step is to do the > basic smoke tests (i.e. run compliance tests, do some (limited) > streaming test). > > There are more extensive (and longer running) tests that can be done, > but that's something to look at later. > > >>> We have four virtual drivers: vivid, vim2m, vimc and vicodec. The > >>> last one is IMHO not quite good enough yet for testing: it is not > >>> fully compliant to the upcoming stateful codec spec. Work for that > >>> is planned as part of an Outreachy project. > >>> > >>> My idea is to create a script that is maintained as part of > >>> v4l-utils that loads the drivers and runs v4l2-compliance and > >>> possibly other tests against the virtual drivers. > > (adding Shuah) > > IMO, the best would be to have something like that as part of Kernel > self test, as this could give a broader covering than just Kernel CI. > > Yeah, I know that one of the concerns is that the *-compliance stuff > we have are written in C++ and it is easier to maintain then at > v4l-utils, but maybe it would be acceptable at kselftest to have a > test bench there with would download the sources from a git tree > and then build just the v4l2-compliance stuff, e. g. having a Kernel > self test target that would do something like: > > git clone --depth 1 git://linuxtv.org/v4l-utils.git tests && \ > cd tests && ./autogen.sh && make tests && ./run_tests.sh Let me make sure I understand this properly. Are you proposing to add to kselftest, which is part of the Linux kernel, and as such benefits from the level of trust of Linus' tree, and which is run by a very large number of machines from developer workstations to automated large-scale test infrastructure, a provision to execute locally code that is downloaded at runtime from the internet, with all the security issues this implies ? > (the actual selftest target would likely be different, as it > should take into account make O=) > > If this would be acceptable upstream, then we'll need to stick with the > output format defined by Kernel Self Test[1]. > > [1] I guess it uses the TAP13 format: > https://testanything.org/tap-version-13-specification.html > > >> How about spending a little time to pick a suitable framework for > >> running the tests? It could be useful to get more informative > >> reports than just pass / fail. > > > > We should keep in mind that other tests will be added later, and the > > test framework should make that easy. > > Since we want to be able to run this on kernelci.org, I think it > makes sense to let the kernelci folks (Hi Ezequiel!) decide this. > >>> > >>> KernelCI isn't the only test infrastructure out there, so let's not > >>> forget about the other ones. > >> > >> True, but they are putting time and money into this, so they get to > >> choose as far as I am concerned :-) > > Surely, but no matter who is paying, if one wants to merge things upstream, > he/she has to stick with upstream ruleset. > > That's said, we should try to not make life harder than it should be for > it, but some things should be standardized, if we want future contributions > there. At very minimal, from my side, I'd like it to be as much compatible > with
Re: [RFC] Create test script(s?) for regression testing
Em Wed, 07 Nov 2018 12:06:55 +0200 Laurent Pinchart escreveu: > Hi Hans, > > On Wednesday, 7 November 2018 10:05:12 EET Hans Verkuil wrote: > > On 11/06/2018 08:58 PM, Laurent Pinchart wrote: > > > On Tuesday, 6 November 2018 15:56:34 EET Hans Verkuil wrote: > > >> On 11/06/18 14:12, Laurent Pinchart wrote: > > >>> On Tuesday, 6 November 2018 13:36:55 EET Sakari Ailus wrote: > > On Tue, Nov 06, 2018 at 09:37:07AM +0100, Hans Verkuil wrote: > > > Hi all, > > > > > > After the media summit (heavy on test discussions) and the V4L2 event > > > regression we just found it is clear we need to do a better job with > > > testing. > > > > > > All the pieces are in place, so what is needed is to combine it and > > > create a script that anyone of us as core developers can run to check > > > for regressions. The same script can be run as part of the kernelci > > > regression testing. > > > > I'd say that *some* pieces are in place. Of course, the more there is, > > the better. > > > > The more there are tests, the more important it would be they're > > automated, preferrably without the developer having to run them on his/ > > her own machine. > > >>> > > >>> From my experience with testing, it's important to have both a core set > > >>> of tests (a.k.a. smoke tests) that can easily be run on developers' > > >>> machines, and extended tests that can be offloaded to a shared testing > > >>> infrastructure (but possibly also run locally if desired). > > >> > > >> That was my idea as well for the longer term. First step is to do the > > >> basic smoke tests (i.e. run compliance tests, do some (limited) streaming > > >> test). > > >> > > >> There are more extensive (and longer running) tests that can be done, but > > >> that's something to look at later. > > >> > > > We have four virtual drivers: vivid, vim2m, vimc and vicodec. The last > > > one is IMHO not quite good enough yet for testing: it is not fully > > > compliant to the upcoming stateful codec spec. Work for that is > > > planned as part of an Outreachy project. > > > > > > My idea is to create a script that is maintained as part of v4l-utils > > > that loads the drivers and runs v4l2-compliance and possibly other > > > tests against the virtual drivers. (adding Shuah) IMO, the best would be to have something like that as part of Kernel self test, as this could give a broader covering than just Kernel CI. Yeah, I know that one of the concerns is that the *-compliance stuff we have are written in C++ and it is easier to maintain then at v4l-utils, but maybe it would be acceptable at kselftest to have a test bench there with would download the sources from a git tree and then build just the v4l2-compliance stuff, e. g. having a Kernel self test target that would do something like: git clone --depth 1 git://linuxtv.org/v4l-utils.git tests && \ cd tests && ./autogen.sh && make tests && ./run_tests.sh (the actual selftest target would likely be different, as it should take into account make O=) If this would be acceptable upstream, then we'll need to stick with the output format defined by Kernel Self Test[1]. [1] I guess it uses the TAP13 format: https://testanything.org/tap-version-13-specification.html > > > > How about spending a little time to pick a suitable framework for > > running the tests? It could be useful to get more informative reports > > than just pass / fail. > > >>> > > >>> We should keep in mind that other tests will be added later, and the > > >>> test framework should make that easy. > > >> > > >> Since we want to be able to run this on kernelci.org, I think it makes > > >> sense to let the kernelci folks (Hi Ezequiel!) decide this. > > > > > > KernelCI isn't the only test infrastructure out there, so let's not forget > > > about the other ones. > > > > True, but they are putting time and money into this, so they get to choose > > as far as I am concerned :-) Surely, but no matter who is paying, if one wants to merge things upstream, he/she has to stick with upstream ruleset. That's said, we should try to not make life harder than it should be for it, but some things should be standardized, if we want future contributions there. At very minimal, from my side, I'd like it to be as much compatible with Kernel selftest infrastructure as possible. I would try to avoid placing KernelCI-specific stuff (like adding LAVA code) inside v4l-utils tree. With regards to that, one alternative would be to split KernelCI specific code on a different tree and use "git subtree". > It's still our responsibility to give V4L2 a good test framework, and to > drive > it in the right direction. We don't accept V4L2 API extensions blindly just > because a company happens to put time and money into it (there may have been > one exception, but it's not
[PATCH] keytable: fix BPF protocol compilation on mips
clang -idirafter /usr/local/include -idirafter
+/usr/lib/llvm-6.0/lib/clang/6.0.1/include -idirafter
+/usr/include/mips64el-linux-gnuabi64 -idirafter /usr/include
+-I../../../include -target bpf -O2 -c grundig.c
> In file included from grundig.c:5:
> In file included from ../../../include/linux/lirc.h:10:
> In file included from /usr/include/linux/types.h:9:
> In file included from /usr/include/linux/posix_types.h:36:
> In file included from
+/usr/include/mips64el-linux-gnuabi64/asm/posix_types.h:13:
> /usr/include/mips64el-linux-gnuabi64/asm/sgidefs.h:19:2: error: Use a Linux
+compiler or give up.
> #error Use a Linux compiler or give up.
This requires __linux__ to be defined.
Signed-off-by: Sean Young
---
utils/keytable/bpf_protocols/Makefile.am | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/utils/keytable/bpf_protocols/Makefile.am
b/utils/keytable/bpf_protocols/Makefile.am
index 8887b897..ba79742c 100644
--- a/utils/keytable/bpf_protocols/Makefile.am
+++ b/utils/keytable/bpf_protocols/Makefile.am
@@ -8,7 +8,7 @@ CLANG_SYS_INCLUDES := $(shell $(CLANG) -v -E - &1 \
| sed -n '/<...> search starts here:/,/End of search list./{ s|
\(/.*\)|-idirafter \1|p }')
%.o: %.c bpf_helpers.h
- $(CLANG) $(CLANG_SYS_INCLUDES) -I$(top_srcdir)/include -target bpf -O2
-c $<
+ $(CLANG) $(CLANG_SYS_INCLUDES) -D__linux__ -I$(top_srcdir)/include
-target bpf -O2 -c $<
PROTOCOLS = grundig.o pulse_distance.o pulse_length.o rc_mm.o manchester.o
--
2.17.2
Re: [PATCH 1/1] v4l: uAPI doc: Simplify NATIVE_SIZE selection target documentation
On 11/07/18 16:04, Sakari Ailus wrote: > The NATIVE_SIZE target is documented for mem2mem devices but no driver has > ever apparently used it. It may be never needed; remove it for now. > > Suggested-by: Hans Verkuil > Signed-off-by: Sakari Ailus Acked-by: Hans Verkuil Thanks, Hans > --- > Documentation/media/uapi/v4l/v4l2-selection-targets.rst | 7 +-- > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/Documentation/media/uapi/v4l/v4l2-selection-targets.rst > b/Documentation/media/uapi/v4l/v4l2-selection-targets.rst > index 87433ec76c6b..bee31611947e 100644 > --- a/Documentation/media/uapi/v4l/v4l2-selection-targets.rst > +++ b/Documentation/media/uapi/v4l/v4l2-selection-targets.rst > @@ -42,12 +42,7 @@ of the two interfaces they are used. > * - ``V4L2_SEL_TGT_NATIVE_SIZE`` >- 0x0003 >- The native size of the device, e.g. a sensor's pixel array. > - ``left`` and ``top`` fields are zero for this target. Setting the > - native size will generally only make sense for memory to memory > - devices where the software can create a canvas of a given size in > - which for example a video frame can be composed. In that case > - V4L2_SEL_TGT_NATIVE_SIZE can be used to configure the size of > - that canvas. > + ``left`` and ``top`` fields are zero for this target. >- Yes >- Yes > * - ``V4L2_SEL_TGT_COMPOSE`` >
[PATCH 1/1] v4l: uAPI doc: Simplify NATIVE_SIZE selection target documentation
The NATIVE_SIZE target is documented for mem2mem devices but no driver has ever apparently used it. It may be never needed; remove it for now. Suggested-by: Hans Verkuil Signed-off-by: Sakari Ailus --- Documentation/media/uapi/v4l/v4l2-selection-targets.rst | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/Documentation/media/uapi/v4l/v4l2-selection-targets.rst b/Documentation/media/uapi/v4l/v4l2-selection-targets.rst index 87433ec76c6b..bee31611947e 100644 --- a/Documentation/media/uapi/v4l/v4l2-selection-targets.rst +++ b/Documentation/media/uapi/v4l/v4l2-selection-targets.rst @@ -42,12 +42,7 @@ of the two interfaces they are used. * - ``V4L2_SEL_TGT_NATIVE_SIZE`` - 0x0003 - The native size of the device, e.g. a sensor's pixel array. - ``left`` and ``top`` fields are zero for this target. Setting the - native size will generally only make sense for memory to memory - devices where the software can create a canvas of a given size in - which for example a video frame can be composed. In that case - V4L2_SEL_TGT_NATIVE_SIZE can be used to configure the size of - that canvas. + ``left`` and ``top`` fields are zero for this target. - Yes - Yes * - ``V4L2_SEL_TGT_COMPOSE`` -- 2.11.0
Re: [PATCH v5 8/9] media: uvcvideo: Rename uvc_{un,}init_video()
Hi Laurent,
On 06/11/2018 23:13, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> On Tuesday, 6 November 2018 23:27:19 EET Kieran Bingham wrote:
>> From: Kieran Bingham
>>
>> We have both uvc_init_video() and uvc_video_init() calls which can be
>> quite confusing to determine the process for each. Now that video
>> uvc_video_enable() has been renamed to uvc_video_start_streaming(),
>> adapt these calls to suit the new flow.
>>
>> Rename uvc_init_video() to uvc_video_start() and uvc_uninit_video() to
>> uvc_video_stop().
>
> I agree that these functions are badly named and should be renamed. We are
> however entering the nitpicking territory :-) The two functions do more that
> starting and stopping, they also allocate and free URBs and the associated
> buffers. It could also be argued that they don't actually start and stop
> anything, as beyond URB management, they just queue the URBs initially and
> kill them. I thus wonder if we could come up with better names.
Well the act of killing (poisoning now) the URBs will certainly stop the
stream, but I guess submitting the URBs isn't necessarily the key act to
starting the stream.
I believe that needs the interface to be set correctly, and the buffers
to be available?
Although - I've just double-checked uvc_{video_start,init_video}() and
that is indeed what it does?
- start stats
- Initialise endpoints
- Perform allocations
- Submit URBs
Am I missing something? Is there another step that is pivotal to
starting the USB packet/urb stream flow after this point ?
Is it not true that the USB stack will start processing data at
submitting URB completion callbacks after the end of uvc_video_start();
and will no longer process data at the end of uvc_video_stop() (and thus
no more completion callbacks)?
(That's a real question to verify my interpretation)
To me - these functions feel like the real 'start' and 'stop' components
of the data stream - hence my choice in naming.
Is your concern that you would like the functions to be more descriptive
over their other actions such as? :
uvc_video_initialise_start()
uvc_video_allocate_init_start()
Or something else? (I don't think those two are good names though)
--
Kieran
>> Signed-off-by: Kieran Bingham
>> ---
>> drivers/media/usb/uvc/uvc_video.c | 18 +-
>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_video.c
>> b/drivers/media/usb/uvc/uvc_video.c index 0d35e933856a..020022e6ade4 100644
>> --- a/drivers/media/usb/uvc/uvc_video.c
>> +++ b/drivers/media/usb/uvc/uvc_video.c
>> @@ -1641,7 +1641,7 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming
>> *stream, /*
>> * Uninitialize isochronous/bulk URBs and free transfer buffers.
>> */
>> -static void uvc_uninit_video(struct uvc_streaming *stream, int
>> free_buffers) +static void uvc_video_stop(struct uvc_streaming *stream, int
>> free_buffers) {
>> struct uvc_urb *uvc_urb;
>>
>> @@ -1718,7 +1718,7 @@ static int uvc_init_video_isoc(struct uvc_streaming
>> *stream,
>>
>> urb = usb_alloc_urb(npackets, gfp_flags);
>> if (urb == NULL) {
>> -uvc_uninit_video(stream, 1);
>> +uvc_video_stop(stream, 1);
>> return -ENOMEM;
>> }
>>
>> @@ -1786,7 +1786,7 @@ static int uvc_init_video_bulk(struct uvc_streaming
>> *stream,
>>
>> urb = usb_alloc_urb(0, gfp_flags);
>> if (urb == NULL) {
>> -uvc_uninit_video(stream, 1);
>> +uvc_video_stop(stream, 1);
>> return -ENOMEM;
>> }
>>
>> @@ -1806,7 +1806,7 @@ static int uvc_init_video_bulk(struct uvc_streaming
>> *stream, /*
>> * Initialize isochronous/bulk URBs and allocate transfer buffers.
>> */
>> -static int uvc_init_video(struct uvc_streaming *stream, gfp_t gfp_flags)
>> +static int uvc_video_start(struct uvc_streaming *stream, gfp_t gfp_flags)
>> {
>> struct usb_interface *intf = stream->intf;
>> struct usb_host_endpoint *ep;
>> @@ -1894,7 +1894,7 @@ static int uvc_init_video(struct uvc_streaming
>> *stream, gfp_t gfp_flags) if (ret < 0) {
>> uvc_printk(KERN_ERR, "Failed to submit URB %u "
>> "(%d).\n", i, ret);
>> -uvc_uninit_video(stream, 1);
>> +uvc_video_stop(stream, 1);
>> return ret;
>> }
>> }
>> @@ -1925,7 +1925,7 @@ int uvc_video_suspend(struct uvc_streaming *stream)
>> return 0;
>>
>> stream->frozen = 1;
>> -uvc_uninit_video(stream, 0);
>> +uvc_video_stop(stream, 0);
>> usb_set_interface(stream->dev->udev, stream->intfnum, 0);
>> return 0;
>> }
>> @@ -1961,7 +1961,7 @@ int uvc_video_resume(struct uvc_streaming *stream, int
>> reset) if (ret < 0)
>> return ret;
>>
>> -return uvc_init_video(stream,
[PATCH] vim2m: use cancel_delayed_work_sync instead of flush_schedule_work
The use of flush_schedule_work() made no sense and caused a syzkaller error.
Replace with the correct cancel_delayed_work_sync().
Signed-off-by: Hans Verkuil
Reported-by: syzbot+69780d144754b8071...@syzkaller.appspotmail.com
---
diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index d82db738f174..f938a2c54314 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -805,10 +805,11 @@ static int vim2m_start_streaming(struct vb2_queue *q,
unsigned count)
static void vim2m_stop_streaming(struct vb2_queue *q)
{
struct vim2m_ctx *ctx = vb2_get_drv_priv(q);
+ struct vim2m_dev *dev = ctx->dev;
struct vb2_v4l2_buffer *vbuf;
unsigned long flags;
- flush_scheduled_work();
+ cancel_delayed_work_sync(>work_run);
for (;;) {
if (V4L2_TYPE_IS_OUTPUT(q->type))
vbuf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
Re: [PATCH v5 9/9] media: uvcvideo: Utilise for_each_uvc_urb iterator
Hi Laurent,
On 06/11/2018 23:21, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> On Tuesday, 6 November 2018 23:27:20 EET Kieran Bingham wrote:
>> From: Kieran Bingham
>>
>> A new iterator is available for processing UVC URB structures. This
>> simplifies the processing of the internal stream data.
>>
>> Convert the manual loop iterators to the new helper, adding an index
>> helper to keep the existing debug print.
>>
>> Signed-off-by: Kieran Bingham
>>
>> ---
>> This patch converts to using the iterator which for most hunks makes
>> sense. The only one with uncertainty is in uvc_alloc_urb_buffers() where
>> the loop index is used to determine if all the buffers were successfully
>> allocated.
>>
>> As the loop index is not incremented by the loops, we can obtain the
>> buffer index - but then we are offset and out-by-one.
>>
>> Adjusting this in the code is fine - but at that point it feels like
>> it's not adding much value. I've left that hunk in for this patch - but
>> that part could be reverted if desired - unless anyone has a better
>> rework of the buffer check?
>>
>> drivers/media/usb/uvc/uvc_video.c | 51
>> drivers/media/usb/uvc/uvcvideo.h | 3 ++-
>> 2 files changed, 29 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_video.c
>> b/drivers/media/usb/uvc/uvc_video.c index 020022e6ade4..f6e5db7ea50e 100644
>> --- a/drivers/media/usb/uvc/uvc_video.c
>> +++ b/drivers/media/usb/uvc/uvc_video.c
>> @@ -1556,20 +1556,19 @@ static void uvc_video_complete(struct urb *urb)
>> */
>> static void uvc_free_urb_buffers(struct uvc_streaming *stream)
>> {
>> -unsigned int i;
>> +struct uvc_urb *uvc_urb;
>>
>> -for (i = 0; i < UVC_URBS; ++i) {
>> -struct uvc_urb *uvc_urb = >uvc_urb[i];
>> +for_each_uvc_urb(uvc_urb, stream) {
>> +if (!uvc_urb->buffer)
>> +continue;
>>
>> -if (uvc_urb->buffer) {
>> #ifndef CONFIG_DMA_NONCOHERENT
>> -usb_free_coherent(stream->dev->udev, stream->urb_size,
>> -uvc_urb->buffer, uvc_urb->dma);
>> +usb_free_coherent(stream->dev->udev, stream->urb_size,
>> + uvc_urb->buffer, uvc_urb->dma);
>> #else
>> -kfree(uvc_urb->buffer);
>> +kfree(uvc_urb->buffer);
>> #endif
>> -uvc_urb->buffer = NULL;
>> -}
>> +uvc_urb->buffer = NULL;
>> }
>>
>> stream->urb_size = 0;
>> @@ -1589,8 +1588,9 @@ static void uvc_free_urb_buffers(struct uvc_streaming
>> *stream) static int uvc_alloc_urb_buffers(struct uvc_streaming *stream,
>> unsigned int size, unsigned int psize, gfp_t gfp_flags)
>> {
>> +struct uvc_urb *uvc_urb;
>> unsigned int npackets;
>> -unsigned int i;
>> +unsigned int i = 0;
>>
>> /* Buffers are already allocated, bail out. */
>> if (stream->urb_size)
>> @@ -1605,8 +1605,12 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming
>> *stream,
>>
>> /* Retry allocations until one succeed. */
>> for (; npackets > 1; npackets /= 2) {
>> -for (i = 0; i < UVC_URBS; ++i) {
>> -struct uvc_urb *uvc_urb = >uvc_urb[i];
>> +for_each_uvc_urb(uvc_urb, stream) {
>> +/*
>> + * Track how many URBs we allocate, adding one to the
>> + * index to account for our zero index.
>> + */
>> +i = uvc_urb_index(uvc_urb) + 1;
>
> That's a bit ugly indeed, I think we could keep the existing loop;
I do agree, as stated I left it in for 'completeness' of the patch. But
I don't think it adds value here.
Will remove.
>> stream->urb_size = psize * npackets;
>> #ifndef CONFIG_DMA_NONCOHERENT
>> @@ -1700,7 +1704,8 @@ static int uvc_init_video_isoc(struct uvc_streaming
>> *stream, struct usb_host_endpoint *ep, gfp_t gfp_flags)
>> {
>> struct urb *urb;
>> -unsigned int npackets, i, j;
>> +struct uvc_urb *uvc_urb;
>> +unsigned int npackets, j;
>
> j without i seems weird, could you rename it ?
Sure. Done
>
>> u16 psize;
>> u32 size;
>>
>> @@ -1713,9 +1718,7 @@ static int uvc_init_video_isoc(struct uvc_streaming
>> *stream,
>>
>> size = npackets * psize;
>>
>> -for (i = 0; i < UVC_URBS; ++i) {
>> -struct uvc_urb *uvc_urb = >uvc_urb[i];
>> -
>> +for_each_uvc_urb(uvc_urb, stream) {
>> urb = usb_alloc_urb(npackets, gfp_flags);
>> if (urb == NULL) {
>> uvc_video_stop(stream, 1);
>> @@ -1757,7 +1760,8 @@ static int uvc_init_video_bulk(struct uvc_streaming
>> *stream, struct usb_host_endpoint *ep, gfp_t gfp_flags)
>> {
>> struct urb *urb;
>> -unsigned int npackets, pipe, i;
>> +struct uvc_urb *uvc_urb;
>> +unsigned int npackets, pipe;
>> u16 psize;
>> u32
Re: [PATCH v4l-utils] keytable: fix compilation warning
On Wed, Nov 07, 2018 at 12:02:14PM +, Sean Young wrote: > keytable.c: In function ‘parse_opt’: > keytable.c:835:7: warning: ‘param’ may be used uninitialized in this function > [-Wuninitialized] > > Signed-off-by: Sean Young Acked-by: Sakari Ailus -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH v5 6/9] media: uvcvideo: Move decode processing to process context
On 06/11/2018 22:58, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> On Tuesday, 6 November 2018 23:27:17 EET Kieran Bingham wrote:
>> From: Kieran Bingham
>>
>> Newer high definition cameras, and cameras with multiple lenses such as
>> the range of stereo-vision cameras now available have ever increasing
>> data rates.
>>
>> The inclusion of a variable length packet header in URB packets mean
>> that we must memcpy the frame data out to our destination 'manually'.
>> This can result in data rates of up to 2 gigabits per second being
>> processed.
>>
>> To improve efficiency, and maximise throughput, handle the URB decode
>> processing through a work queue to move it from interrupt context, and
>> allow multiple processors to work on URBs in parallel.
>>
>> Signed-off-by: Kieran Bingham
>
> Reviewed-by: Laurent Pinchart
>
> I wonder if we shouldn't, as a future improvement, only queue async work when
> the quantity of data to be copied is above a certain threshold.
Possibly - lets keep it in mind for when we get back to looking at
Keiichi's patch and any cache management for further performance
improvements
--
Kieran
>
>> ---
>> v2:
>> - Lock full critical section of usb_submit_urb()
>>
>> v3:
>> - Fix race on submitting uvc_video_decode_data_work() to work queue.
>> - Rename uvc_decode_op -> uvc_copy_op (Generic to encode/decode)
>> - Rename decodes -> copy_operations
>> - Don't queue work if there is no async task
>> - obtain copy op structure directly in uvc_video_decode_data()
>> - uvc_video_decode_data_work() -> uvc_video_copy_data_work()
>>
>> v4:
>> - Provide for_each_uvc_urb()
>> - Simplify fix for shutdown race to flush queue before freeing URBs
>> - Rebase to v4.16-rc4 (linux-media/master) adjusting for metadata
>>conflicts.
>>
>> v5:
>> - Rebase to media/v4.20-2
>> - Use GFP_KERNEL allocation in uvc_video_copy_data_work()
>> - Fix function documentation for uvc_video_copy_data_work()
>> - Add periods to the end of sentences
>> - Rename 'decode' variable to 'op' in uvc_video_decode_data()
>> - Move uvc_urb->async_operations initialisation to before use
>> - Move async workqueue to match uvc_streaming lifetime instead of
>>streamon/streamoff
>>
>> drivers/media/usb/uvc/uvc_driver.c | 2 +-
>> drivers/media/usb/uvc/uvc_video.c | 110 +++---
>> drivers/media/usb/uvc/uvcvideo.h | 28 -
>> 3 files changed, 116 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_driver.c
>> b/drivers/media/usb/uvc/uvc_driver.c index bc369a0934a3..e61a6d26e812
>> 100644
>> --- a/drivers/media/usb/uvc/uvc_driver.c
>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>> @@ -1883,6 +1883,8 @@ static void uvc_unregister_video(struct uvc_device
>> *dev) video_unregister_device(>vdev);
>> video_unregister_device(>meta.vdev);
>>
>> +destroy_workqueue(stream->async_wq);
>> +
>> uvc_debugfs_cleanup_stream(stream);
>> }
>> }
>> diff --git a/drivers/media/usb/uvc/uvc_video.c
>> b/drivers/media/usb/uvc/uvc_video.c index 7a7779e1b466..ce9e40444507 100644
>> --- a/drivers/media/usb/uvc/uvc_video.c
>> +++ b/drivers/media/usb/uvc/uvc_video.c
>> @@ -1094,21 +1094,54 @@ static int uvc_video_decode_start(struct
>> uvc_streaming *stream, return data[0];
>> }
>>
>> -static void uvc_video_decode_data(struct uvc_streaming *stream,
>> +/*
>> + * uvc_video_decode_data_work: Asynchronous memcpy processing
>> + *
>> + * Copy URB data to video buffers in process context, releasing buffer
>> + * references and requeuing the URB when done.
>> + */
>> +static void uvc_video_copy_data_work(struct work_struct *work)
>> +{
>> +struct uvc_urb *uvc_urb = container_of(work, struct uvc_urb, work);
>> +unsigned int i;
>> +int ret;
>> +
>> +for (i = 0; i < uvc_urb->async_operations; i++) {
>> +struct uvc_copy_op *op = _urb->copy_operations[i];
>> +
>> +memcpy(op->dst, op->src, op->len);
>> +
>> +/* Release reference taken on this buffer. */
>> +uvc_queue_buffer_release(op->buf);
>> +}
>> +
>> +ret = usb_submit_urb(uvc_urb->urb, GFP_KERNEL);
>> +if (ret < 0)
>> +uvc_printk(KERN_ERR, "Failed to resubmit video URB (%d).\n",
>> + ret);
>> +}
>> +
>> +static void uvc_video_decode_data(struct uvc_urb *uvc_urb,
>> struct uvc_buffer *buf, const u8 *data, int len)
>> {
>> -unsigned int maxlen, nbytes;
>> -void *mem;
>> +unsigned int active_op = uvc_urb->async_operations;
>> +struct uvc_copy_op *op = _urb->copy_operations[active_op];
>> +unsigned int maxlen;
>>
>> if (len <= 0)
>> return;
>>
>> -/* Copy the video data to the buffer. */
>> maxlen = buf->length - buf->bytesused;
>> -mem = buf->mem + buf->bytesused;
>> -nbytes = min((unsigned int)len, maxlen);
>> -memcpy(mem, data, nbytes);
>> -buf->bytesused += nbytes;
>> +
>> +
Re: [PATCH v5 7/9] media: uvcvideo: Split uvc_video_enable into two
Hi Laurent,
On 06/11/2018 23:08, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> On Tuesday, 6 November 2018 23:27:18 EET Kieran Bingham wrote:
>> From: Kieran Bingham
>>
>> uvc_video_enable() is used both to start and stop the video stream
>> object, however the single function entry point shares no code between
>> the two operations.
>>
>> Split the function into two distinct calls, and rename to
>> uvc_video_start_streaming() and uvc_video_stop_streaming() as
>> appropriate.
>>
>> Signed-off-by: Kieran Bingham
>> ---
>> drivers/media/usb/uvc/uvc_queue.c | 4 +-
>> drivers/media/usb/uvc/uvc_video.c | 56 +++-
>> drivers/media/usb/uvc/uvcvideo.h | 3 +-
>> 3 files changed, 31 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_queue.c
>> b/drivers/media/usb/uvc/uvc_queue.c index cd8c03341de0..682698ec1118 100644
>> --- a/drivers/media/usb/uvc/uvc_queue.c
>> +++ b/drivers/media/usb/uvc/uvc_queue.c
>> @@ -176,7 +176,7 @@ static int uvc_start_streaming(struct vb2_queue *vq,
>> unsigned int count)
>>
>> queue->buf_used = 0;
>>
>> -ret = uvc_video_enable(stream, 1);
>> +ret = uvc_video_start_streaming(stream);
>> if (ret == 0)
>> return 0;
>>
>> @@ -194,7 +194,7 @@ static void uvc_stop_streaming(struct vb2_queue *vq)
>> lockdep_assert_irqs_enabled();
>>
>> if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
>> -uvc_video_enable(uvc_queue_to_stream(queue), 0);
>> +uvc_video_stop_streaming(uvc_queue_to_stream(queue));
>>
>> spin_lock_irq(>irqlock);
>> uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
>> diff --git a/drivers/media/usb/uvc/uvc_video.c
>> b/drivers/media/usb/uvc/uvc_video.c index ce9e40444507..0d35e933856a 100644
>> --- a/drivers/media/usb/uvc/uvc_video.c
>> +++ b/drivers/media/usb/uvc/uvc_video.c
>> @@ -2082,38 +2082,10 @@ int uvc_video_init(struct uvc_streaming *stream)
>> return 0;
>> }
>>
>> -/*
>> - * Enable or disable the video stream.
>> - */
>> -int uvc_video_enable(struct uvc_streaming *stream, int enable)
>> +int uvc_video_start_streaming(struct uvc_streaming *stream)
>> {
>> int ret;
>>
>> -if (!enable) {
>> -uvc_uninit_video(stream, 1);
>> -if (stream->intf->num_altsetting > 1) {
>> -usb_set_interface(stream->dev->udev,
>> - stream->intfnum, 0);
>> -} else {
>> -/* UVC doesn't specify how to inform a bulk-based device
>> - * when the video stream is stopped. Windows sends a
>> - * CLEAR_FEATURE(HALT) request to the video streaming
>> - * bulk endpoint, mimic the same behaviour.
>> - */
>> -unsigned int epnum = stream->header.bEndpointAddress
>> - & USB_ENDPOINT_NUMBER_MASK;
>> -unsigned int dir = stream->header.bEndpointAddress
>> - & USB_ENDPOINT_DIR_MASK;
>> -unsigned int pipe;
>> -
>> -pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
>> -usb_clear_halt(stream->dev->udev, pipe);
>> -}
>> -
>> -uvc_video_clock_cleanup(stream);
>> -return 0;
>> -}
>> -
>> ret = uvc_video_clock_init(stream);
>> if (ret < 0)
>> return ret;
>> @@ -2136,3 +2108,29 @@ int uvc_video_enable(struct uvc_streaming *stream,
>> int enable)
>>
>> return ret;
>> }
>> +
>> +int uvc_video_stop_streaming(struct uvc_streaming *stream)
>> +{
>> +uvc_uninit_video(stream, 1);
>> +if (stream->intf->num_altsetting > 1) {
>> +usb_set_interface(stream->dev->udev,
>> + stream->intfnum, 0);
>
> This now holds on a single line.
Ah yes.
>
>> +} else {
>> +/* UVC doesn't specify how to inform a bulk-based device
>
> Let's fix the checkpatch.pl warning here.
Oh ? I didn't get any checkpatch warnings. Do I need to add some
parameters to my checkpatch now?
>
>> + * when the video stream is stopped. Windows sends a
>> + * CLEAR_FEATURE(HALT) request to the video streaming
>> + * bulk endpoint, mimic the same behaviour.
>> + */
>> +unsigned int epnum = stream->header.bEndpointAddress
>> + & USB_ENDPOINT_NUMBER_MASK;
>> +unsigned int dir = stream->header.bEndpointAddress
>> + & USB_ENDPOINT_DIR_MASK;
>> +unsigned int pipe;
>> +
>> +pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
>> +usb_clear_halt(stream->dev->udev, pipe);
>> +}
>> +
>> +uvc_video_clock_cleanup(stream);
>> +return 0;
>
> As this always return 0 you can make it a void function.
Certainly.
>
> Apart from that,
>
Re: [PATCH v4l-utils] Add missing linux/bpf_common.h
Hi Peter, On Tue, Nov 06, 2018 at 10:43:58PM +0100, Peter Seiderer wrote: > On Tue, 6 Nov 2018 10:38:56 +, Sean Young wrote: > > > On Mon, Nov 05, 2018 at 09:30:47PM +0100, Peter Seiderer wrote: > > > Copy from [1], needed by bpf.h. > > > > > > [1] > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/include/uapi/linux/bpf_common.h?h=v4.19 > > > > So bpf.h does include this file, but we don't use anything from it in > > v4l-utils. > > > > Maybe alternative fix is to remove the include (or not if your want > the headers to be in sync with the kernel ones, but then they should > be complete enough to be used for compile)? > > > This include file is for the original BPF, which has been around for a > > long time. So why is this include file missing, i.e. what problem are you > > trying to solve? > > A buildroot autobuild failure (see [1] for details) with older toolchains > not providing this header... > > > > > Lastely, the file should be included in the sync-with-kernel target so > > it does not get out of sync -- should it really be necessary to add the > > file. > > O.k, can do it on next patch iteration... > > Regards, > Peter > > [1] http://lists.busybox.net/pipermail/buildroot/2018-November/234840.html So here libelf was not detected, hence ir-keytable should have been built without BPF support, but it is still including bpf.h despite it not being used. I've just sent a patch for better support for building without BPF, see here: https://patchwork.linuxtv.org/patch/52841/ Would you mind seeing if that works for you? Thanks, Sean
[PATCH v4l-utils] keytable: fix compilation warning
keytable.c: In function ‘parse_opt’:
keytable.c:835:7: warning: ‘param’ may be used uninitialized in this function
[-Wuninitialized]
Signed-off-by: Sean Young
---
utils/keytable/keytable.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/utils/keytable/keytable.c b/utils/keytable/keytable.c
index 6fc22358..e15440de 100644
--- a/utils/keytable/keytable.c
+++ b/utils/keytable/keytable.c
@@ -832,7 +832,7 @@ static error_t parse_opt(int k, char *arg, struct
argp_state *state)
do {
struct bpf_parameter *param;
- if (!param) {
+ if (!p) {
argp_error(state, _("Missing parameter name:
%s"), arg);
break;
}
--
2.17.2
[PATCH v2 v4l-utils] configure: build without BPF support in ir-keytable
It currently does not build on mips and some platforms do not have
BPF support yet (risc-v, for example).
Signed-off-by: Sean Young
---
configure.ac | 17 +
utils/keytable/Makefile.am | 7 ---
utils/keytable/keytable.c | 5 -
3 files changed, 21 insertions(+), 8 deletions(-)
diff --git a/configure.ac b/configure.ac
index 387f8539..5cc34c24 100644
--- a/configure.ac
+++ b/configure.ac
@@ -173,16 +173,12 @@ AM_CONDITIONAL([HAVE_X11], [test x$x11_pkgconfig = xyes])
PKG_CHECK_MODULES([LIBELF], [libelf], [libelf_pkgconfig=yes],
[libelf_pkgconfig=no])
AC_SUBST([LIBELF_CFLAGS])
AC_SUBST([LIBELF_LIBS])
-AM_CONDITIONAL([HAVE_LIBELF], [test x$libelf_pkgconfig = xyes])
if test "x$libelf_pkgconfig" = "xyes"; then
AC_CHECK_PROG([CLANG], clang, clang)
- AC_DEFINE([HAVE_LIBELF], [1], [libelf library is present])
else
AC_MSG_WARN(libelf library not available)
fi
-AM_CONDITIONAL([BPF_PROTOCOLS], [test x$CLANG = xclang])
-
AS_IF([test "x$x11_pkgconfig" = xyes],
[PKG_CHECK_MODULES(GL, [gl], [gl_pkgconfig=yes], [gl_pkgconfig=no])],
[gl_pkgconfig=no])
AC_SUBST([GL_CFLAGS])
@@ -453,6 +449,14 @@ AC_ARG_ENABLE(gconv,
esac]
)
+AC_ARG_ENABLE(bpf,
+ AS_HELP_STRING([--disable-bpf], [disable IR BPF decoders]),
+ [case "${enableval}" in
+yes | no ) ;;
+*) AC_MSG_ERROR(bad value ${enableval} for --enable-bpf) ;;
+ esac]
+)
+
PKG_CHECK_MODULES([SDL2], [sdl2 SDL2_image], [sdl_pc=yes], [sdl_pc=no])
AM_CONDITIONAL([HAVE_SDL], [test x$sdl_pc = xyes])
@@ -475,6 +479,7 @@ AM_CONDITIONAL([WITH_GCONV],[test x$enable_gconv =
xyes -a x$enable_shar
AM_CONDITIONAL([WITH_V4L2_CTL_LIBV4L], [test x${enable_v4l2_ctl_libv4l} !=
xno])
AM_CONDITIONAL([WITH_V4L2_CTL_STREAM_TO], [test x${enable_v4l2_ctl_stream_to}
!= xno])
AM_CONDITIONAL([WITH_V4L2_COMPLIANCE_LIBV4L], [test
x${enable_v4l2_compliance_libv4l} != xno])
+AM_CONDITIONAL([WITH_BPF], [test x$enable_bpf != xno -a
x$libelf_pkgconfig = xyes -a x$CLANG = xclang])
# append -static to libtool compile and link command to enforce static libs
AS_IF([test x$enable_libdvbv5 = xno], [AC_SUBST([ENFORCE_LIBDVBV5_STATIC],
["-static"])])
@@ -505,6 +510,9 @@ AM_COND_IF([WITH_V4L_WRAPPERS], [USE_V4L_WRAPPERS="yes"],
[USE_V4L_WRAPPERS="no"
AM_COND_IF([WITH_GCONV], [USE_GCONV="yes"], [USE_GCONV="no"])
AM_COND_IF([WITH_V4L2_CTL_LIBV4L], [USE_V4L2_CTL_LIBV4L="yes"],
[USE_V4L2_CTL_LIBV4L="no"])
AM_COND_IF([WITH_V4L2_COMPLIANCE_LIBV4L], [USE_V4L2_COMPLIANCE_LIBV4L="yes"],
[USE_V4L2_COMPLIANCE_LIBV4L="no"])
+AM_COND_IF([WITH_BPF], [USE_BPF="yes"
+AC_DEFINE([HAVE_BPF], [1], [BPF IR decoder
support enabled])],
+ [USE_BPF="no"])
AS_IF([test "x$alsa_pkgconfig" = "xtrue"], [USE_ALSA="yes"], [USE_ALSA="no"])
AC_OUTPUT
@@ -549,4 +557,5 @@ compile time options summary
qvidcap: $USE_QVIDCAP
v4l2-ctl uses libv4l : $USE_V4L2_CTL_LIBV4L
v4l2-compliance uses libv4l: $USE_V4L2_COMPLIANCE_LIBV4L
+BPF IR Decoders: : $USE_BPF
EOF
diff --git a/utils/keytable/Makefile.am b/utils/keytable/Makefile.am
index 90e4c8c8..ddbab0f7 100644
--- a/utils/keytable/Makefile.am
+++ b/utils/keytable/Makefile.am
@@ -6,14 +6,15 @@ udevrules_DATA = 70-infrared.rules
ir_keytable_SOURCES = keytable.c parse.h ir-encode.c ir-encode.h toml.c toml.h
-if HAVE_LIBELF
+if WITH_BPF
ir_keytable_SOURCES += bpf.c bpf_load.c bpf.h bpf_load.h
endif
ir_keytable_LDADD = @LIBINTL@
-ir_keytable_LDFLAGS = $(ARGP_LIBS) $(LIBELF_LIBS)
+ir_keytable_LDFLAGS = $(ARGP_LIBS)
-if BPF_PROTOCOLS
+if WITH_BPF
+ir_keytable_LDFLAGS += $(LIBELF_LIBS)
SUBDIRS = bpf_protocols
endif
diff --git a/utils/keytable/keytable.c b/utils/keytable/keytable.c
index a7ed436b..6fc22358 100644
--- a/utils/keytable/keytable.c
+++ b/utils/keytable/keytable.c
@@ -34,8 +34,11 @@
#include "ir-encode.h"
#include "parse.h"
#include "toml.h"
+
+#ifdef HAVE_BPF
#include "bpf.h"
#include "bpf_load.h"
+#endif
#ifdef ENABLE_NLS
# define _(string) gettext(string)
@@ -1847,7 +1850,7 @@ static void device_info(int fd, char *prepend)
perror ("EVIOCGID");
}
-#ifdef HAVE_LIBELF
+#ifdef HAVE_BPF
#define MAX_PROGS 64
static void attach_bpf(const char *lirc_name, const char *bpf_prog, struct
toml_table_t *toml)
{
--
2.17.2
Re: [PATCH 1/3] media: imx: add capture compose rectangle
Hi Philipp,
On Tue, Nov 06, 2018 at 03:44:07PM +0100, Philipp Zabel wrote:
> Hi Sakari,
>
> On Tue, 2018-11-06 at 16:01 +0200, Sakari Ailus wrote:
> [...]
> > @@ -290,6 +294,35 @@ static int capture_s_std(struct file *file, void *fh,
> > v4l2_std_id std)
> > > return v4l2_subdev_call(priv->src_sd, video, s_std, std);
> > > }
> > >
> > > +static int capture_g_selection(struct file *file, void *fh,
> > > +struct v4l2_selection *s)
> > > +{
> > > + struct capture_priv *priv = video_drvdata(file);
> > > +
> > > + switch (s->target) {
> > > + case V4L2_SEL_TGT_CROP:
> > > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > > + case V4L2_SEL_TGT_NATIVE_SIZE:
> >
> > The NATIVE_SIZE is for devices such as sensors. It doesn't make sense here.
>
> Should this be documented in Documentation/media/uapi/v4l/v4l2-
> selection-targets.rst ? There it only mentions when to make it
> writeable.
This seems to have originated from the documentation before the ReST
conversion and I had hard time to figure out where the current text (apart
from sensor pixel array) came from. There is also no driver using it in
that meaning, and I doubt if the use is not already been covered by the
compose rectangle.
This indeed requires some follow-up, but that's out of scope of your set.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v7 05/12] media: dt-bindings: add bindings for i.MX7 media driver
On Fri, Aug 10, 2018 at 03:20:38PM +0100, Rui Miguel Silva wrote: > Add bindings documentation for i.MX7 media drivers. > The imx7 MIPI CSI2 and imx7 CMOS Sensor Interface. > > Reviewed-by: Rob Herring > Signed-off-by: Rui Miguel Silva Acked-by: Sakari Ailus - -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH] media: imx258: remove test pattern map from driver
Hi Jason,
Thanks for the patch.
On Wed, Nov 07, 2018 at 03:22:23PM +0800, jasonx.z.c...@intel.com wrote:
> From: "Chen, JasonX Z"
>
> Test Pattern mode be picked at HAL instead of driver.
> do a FLIP when userspace use test pattern mode.
> add entity_ops for validating imx258 link.
>
> Signed-off-by: Chen, JasonX Z
> ---
> drivers/media/i2c/imx258.c | 28
> 1 file changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 31a1e22..71f9875 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -62,11 +62,6 @@
>
> /* Test Pattern Control */
> #define IMX258_REG_TEST_PATTERN 0x0600
> -#define IMX258_TEST_PATTERN_DISABLE 0
> -#define IMX258_TEST_PATTERN_SOLID_COLOR 1
> -#define IMX258_TEST_PATTERN_COLOR_BARS 2
> -#define IMX258_TEST_PATTERN_GREY_COLOR 3
> -#define IMX258_TEST_PATTERN_PN9 4
>
> /* Orientation */
> #define REG_MIRROR_FLIP_CONTROL 0x0101
> @@ -504,20 +499,12 @@ struct imx258_mode {
>
> static const char * const imx258_test_pattern_menu[] = {
> "Disabled",
> - "Color Bars",
> "Solid Color",
> + "Color Bars",
> "Grey Color Bars",
> "PN9"
> };
>
> -static const int imx258_test_pattern_val[] = {
> - IMX258_TEST_PATTERN_DISABLE,
> - IMX258_TEST_PATTERN_COLOR_BARS,
> - IMX258_TEST_PATTERN_SOLID_COLOR,
> - IMX258_TEST_PATTERN_GREY_COLOR,
> - IMX258_TEST_PATTERN_PN9,
> -};
> -
> /* Configurations for supported link frequencies */
> #define IMX258_LINK_FREQ_634MHZ 63360ULL
> #define IMX258_LINK_FREQ_320MHZ 32000ULL
> @@ -752,7 +739,6 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> container_of(ctrl->handler, struct imx258, ctrl_handler);
> struct i2c_client *client = v4l2_get_subdevdata(>sd);
> int ret = 0;
> -
This seems like an unrelated change, and it's a common (and usually good)
practice to add an empty line after variable declarations.
> /*
>* Applying V4L2 control value only happens
>* when power is up for streaming
> @@ -778,13 +764,10 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> case V4L2_CID_TEST_PATTERN:
> ret = imx258_write_reg(imx258, IMX258_REG_TEST_PATTERN,
> IMX258_REG_VALUE_16BIT,
> - imx258_test_pattern_val[ctrl->val]);
> -
> + ctrl->val);
> ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
> IMX258_REG_VALUE_08BIT,
> - ctrl->val == imx258_test_pattern_val
> - [IMX258_TEST_PATTERN_DISABLE] ?
> - REG_CONFIG_MIRROR_FLIP :
> + !ctrl->val?REG_CONFIG_MIRROR_FLIP :
> REG_CONFIG_FLIP_TEST_PATTERN);
> break;
> default:
> @@ -1105,6 +1088,10 @@ static int imx258_identify_module(struct imx258
> *imx258)
> .pad = _pad_ops,
> };
>
> +static const struct media_entity_operations imx258_subdev_entity_ops = {
> + .link_validate = v4l2_subdev_link_validate,
> +};
> +
This seems unrelated as well. Do you need the link validation for something?
As far as I understand, the driver exposes a single source pad; therefore
the link_validate op will never be called.
> static const struct v4l2_subdev_internal_ops imx258_internal_ops = {
> .open = imx258_open,
> };
> @@ -1250,6 +1237,7 @@ static int imx258_probe(struct i2c_client *client)
>
> /* Initialize subdev */
> imx258->sd.internal_ops = _internal_ops;
> + imx258->sd.entity.ops = _subdev_entity_ops;
Ditto.
> imx258->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> imx258->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
[GIT PULL FOR v4.21] Various fixes
Just one note: the "cec: keep track of outstanding transmits" is CC-ed to stable for v4.18 and up, but I prefer to wait until v4.21 before merging it to give it more test time. It is not something that happens in normal usage, so delaying this isn't a problem. Regards, Hans The following changes since commit fbe57dde7126d1b2712ab5ea93fb9d15f89de708: media: ov7740: constify structures stored in fields of v4l2_subdev_ops structure (2018-11-06 07:17:02 -0500) are available in the Git repository at: git://linuxtv.org/hverkuil/media_tree.git tags/br-v4.21a for you to fetch changes up to b6f3defe272a97ea65f4352cdc9c0b943164a75e: vivid: fill in media_device bus_info (2018-11-07 11:15:12 +0100) Tag branch Hans Verkuil (7): adv7604: add CEC support for adv7611/adv7612 cec: report Vendor ID after initialization cec: add debug_phys_addr module option cec: keep track of outstanding transmits vivid: fix error handling of kthread_run vivid: set min width/height to a value > 0 vivid: fill in media_device bus_info Julia Lawall (4): media: vicodec: constify v4l2_ctrl_ops structure media: rockchip/rga: constify v4l2_m2m_ops structure media: vimc: constify structures stored in fields of v4l2_subdev_ops structure media: rockchip/rga: constify video_device structure Sean Young (1): media: v4l uapi docs: few minor corrections and typos Documentation/media/uapi/v4l/app-pri.rst | 2 +- Documentation/media/uapi/v4l/audio.rst | 2 +- Documentation/media/uapi/v4l/dev-capture.rst | 2 +- Documentation/media/uapi/v4l/dev-teletext.rst| 2 +- Documentation/media/uapi/v4l/format.rst | 2 +- Documentation/media/uapi/v4l/mmap.rst| 22 ++--- Documentation/media/uapi/v4l/open.rst| 2 +- Documentation/media/uapi/v4l/tuner.rst | 4 ++-- Documentation/media/uapi/v4l/userp.rst | 8 Documentation/media/uapi/v4l/video.rst | 4 ++-- drivers/media/cec/cec-adap.c | 34 +++- drivers/media/cec/cec-core.c | 6 ++ drivers/media/i2c/adv7604.c | 63 ++-- drivers/media/platform/rockchip/rga/rga.c| 4 ++-- drivers/media/platform/vicodec/vicodec-core.c| 2 +- drivers/media/platform/vimc/vimc-sensor.c| 2 +- drivers/media/platform/vivid/vivid-core.c| 2 ++ drivers/media/platform/vivid/vivid-kthread-cap.c | 5 - drivers/media/platform/vivid/vivid-kthread-out.c | 5 - drivers/media/platform/vivid/vivid-vid-common.c | 2 +- include/media/cec.h | 1 + 21 files changed, 125 insertions(+), 51 deletions(-
Re: [PATCH] [v4l-utils] libv4l2subdev: Add MEDIA_BUS_FMT_FIXED to mbus_formats[]
On Tue, Nov 06, 2018 at 09:12:56AM -0800, Yong Zhi wrote:
> Also add V4L2_COLORSPACE_RAW to the colorspaces[].
>
> Signed-off-by: Yong Zhi
> ---
> utils/media-ctl/libv4l2subdev.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c
> index a989efb..46668eb 100644
> --- a/utils/media-ctl/libv4l2subdev.c
> +++ b/utils/media-ctl/libv4l2subdev.c
> @@ -855,6 +855,7 @@ static const struct {
> enum v4l2_mbus_pixelcode code;
> } mbus_formats[] = {
> #include "media-bus-format-names.h"
> + { "FIXED", MEDIA_BUS_FMT_FIXED},
> { "Y8", MEDIA_BUS_FMT_Y8_1X8},
> { "Y10", MEDIA_BUS_FMT_Y10_1X10 },
> { "Y12", MEDIA_BUS_FMT_Y12_1X12 },
> @@ -965,7 +966,9 @@ static struct {
> { "srgb", V4L2_COLORSPACE_SRGB },
> { "oprgb", V4L2_COLORSPACE_OPRGB },
> { "bt2020", V4L2_COLORSPACE_BT2020 },
> + { "raw", V4L2_COLORSPACE_RAW },
> { "dcip3", V4L2_COLORSPACE_DCI_P3 },
> +
> };
>
> const char *v4l2_subdev_colorspace_to_string(enum v4l2_colorspace colorspace)
The diff became:
diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c
index 46668eb5..0d0afbe7 100644
--- a/utils/media-ctl/libv4l2subdev.c
+++ b/utils/media-ctl/libv4l2subdev.c
@@ -855,8 +855,8 @@ static const struct {
enum v4l2_mbus_pixelcode code;
} mbus_formats[] = {
#include "media-bus-format-names.h"
- { "FIXED", MEDIA_BUS_FMT_FIXED},
- { "Y8", MEDIA_BUS_FMT_Y8_1X8},
+ { "FIXED", MEDIA_BUS_FMT_FIXED },
+ { "Y8", MEDIA_BUS_FMT_Y8_1X8 },
{ "Y10", MEDIA_BUS_FMT_Y10_1X10 },
{ "Y12", MEDIA_BUS_FMT_Y12_1X12 },
{ "YUYV", MEDIA_BUS_FMT_YUYV8_1X16 },
@@ -968,7 +968,6 @@ static struct {
{ "bt2020", V4L2_COLORSPACE_BT2020 },
{ "raw", V4L2_COLORSPACE_RAW },
{ "dcip3", V4L2_COLORSPACE_DCI_P3 },
-
};
const char *v4l2_subdev_colorspace_to_string(enum v4l2_colorspace colorspace)
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [RFC] Create test script(s?) for regression testing
Hi Hans, On Wednesday, 7 November 2018 10:05:12 EET Hans Verkuil wrote: > On 11/06/2018 08:58 PM, Laurent Pinchart wrote: > > On Tuesday, 6 November 2018 15:56:34 EET Hans Verkuil wrote: > >> On 11/06/18 14:12, Laurent Pinchart wrote: > >>> On Tuesday, 6 November 2018 13:36:55 EET Sakari Ailus wrote: > On Tue, Nov 06, 2018 at 09:37:07AM +0100, Hans Verkuil wrote: > > Hi all, > > > > After the media summit (heavy on test discussions) and the V4L2 event > > regression we just found it is clear we need to do a better job with > > testing. > > > > All the pieces are in place, so what is needed is to combine it and > > create a script that anyone of us as core developers can run to check > > for regressions. The same script can be run as part of the kernelci > > regression testing. > > I'd say that *some* pieces are in place. Of course, the more there is, > the better. > > The more there are tests, the more important it would be they're > automated, preferrably without the developer having to run them on his/ > her own machine. > >>> > >>> From my experience with testing, it's important to have both a core set > >>> of tests (a.k.a. smoke tests) that can easily be run on developers' > >>> machines, and extended tests that can be offloaded to a shared testing > >>> infrastructure (but possibly also run locally if desired). > >> > >> That was my idea as well for the longer term. First step is to do the > >> basic smoke tests (i.e. run compliance tests, do some (limited) streaming > >> test). > >> > >> There are more extensive (and longer running) tests that can be done, but > >> that's something to look at later. > >> > > We have four virtual drivers: vivid, vim2m, vimc and vicodec. The last > > one is IMHO not quite good enough yet for testing: it is not fully > > compliant to the upcoming stateful codec spec. Work for that is > > planned as part of an Outreachy project. > > > > My idea is to create a script that is maintained as part of v4l-utils > > that loads the drivers and runs v4l2-compliance and possibly other > > tests against the virtual drivers. > > How about spending a little time to pick a suitable framework for > running the tests? It could be useful to get more informative reports > than just pass / fail. > >>> > >>> We should keep in mind that other tests will be added later, and the > >>> test framework should make that easy. > >> > >> Since we want to be able to run this on kernelci.org, I think it makes > >> sense to let the kernelci folks (Hi Ezequiel!) decide this. > > > > KernelCI isn't the only test infrastructure out there, so let's not forget > > about the other ones. > > True, but they are putting time and money into this, so they get to choose > as far as I am concerned :-) It's still our responsibility to give V4L2 a good test framework, and to drive it in the right direction. We don't accept V4L2 API extensions blindly just because a company happens to put time and money into it (there may have been one exception, but it's not the rule), we instead review all proposals carefully. The same should be true with tests. > If others are interested and willing to put up time and money, they should > let themselves be known. > > I'm not going to work on such an integration, although I happily accept > patches. > > >> As a developer all I need is a script to run smoke tests so I can catch > >> most regressions (you never catch all). > >> > >> I'm happy to work with them to make any changes to compliance tools and > >> scripts so they fit better into their test framework. > >> > >> The one key requirement to all this is that you should be able to run > >> these tests without dependencies to all sorts of external packages/ > >> libraries. > > > > v4l-utils already has a set of dependencies, but those are largely > > manageable. For v4l2-compliance we'll install libv4l, which depends on > > libjpeg. > > That's already too much. You can manually build v4l2-compliance with no > dependencies whatsoever, but we're missing a Makefile target for that. It's > been useful for embedded systems with poor cross-compile environments. I don't think depending on libv4l and libjpeg would be a big issue. On the other hand, given what v4l2-compliance do, one could also argue that it should not use libv4l at all and go straight for the kernel API. This boils down to the question of whether we consider libv4l as part of the official V4L2 stack, or if we want to officially deprecate it given that it hasn't really lived to the promises it made. > It is really very useful to be able to compile those core utilities with no > external libraries other than glibc. You obviously will loose some > functionality when you compile it that way. > > These utilities are not like a typical application. I really don't care how > many libraries are linked in by e.g.
Re: [PATCH v7 00/12] media: staging/imx7: add i.MX7 media driver
Hi Hans, On Wed 07 Nov 2018 at 09:58, Hans Verkuil wrote: Hi Rui, On 08/10/18 16:20, Rui Miguel Silva wrote: Hi, This series introduces the Media driver to work with the i.MX7 SoC. it uses the already existing imx media core drivers but since the i.MX7, contrary to i.MX5/6, do not have an IPU and because of that some changes in the imx media core are made along this series to make it support that case. This patches adds CSI and MIPI-CSI2 drivers for i.MX7, along with several configurations changes for this to work as a capture subsystem. Some bugs are also fixed along the line. And necessary documentation. For a more detailed view of the capture paths, pads links in the i.MX7 please take a look at the documentation in PATCH 14. The system used to test and develop this was the Warp7 board with an OV2680 sensor, which output format is 10-bit bayer. So, only MIPI interface was tested, a scenario with an parallel input would nice to have. Bellow goes an example of the output of the pads and links and the output of v4l2-compliance testing. The v4l-utils version used is: v4l2-compliance SHA : 90905c2e4b17d7595256f3824e2d30d19b0df1a1 from Aug 6th The Media Driver fail some tests but this failures are coming from code out of scope of this series (imx-capture), and some from the sensor OV2680 but that I think not related with the sensor driver but with the testing and core. The csi and mipi-csi entities pass all compliance tests. Can you rebase and repost? Please re-run v4l2-compliance as well with the latest compliance code. Yup, I may take one week or so, other things in hand, but will try to send it sooner than later. We should be able to merge this for 4.21 (finally!). Yeah!! Thanks. --- Cheers, Rui
Re: [PATCH v7 00/12] media: staging/imx7: add i.MX7 media driver
Hi Rui, On 08/10/18 16:20, Rui Miguel Silva wrote: > Hi, > This series introduces the Media driver to work with the i.MX7 SoC. it uses > the > already existing imx media core drivers but since the i.MX7, contrary to > i.MX5/6, do not have an IPU and because of that some changes in the imx media > core are made along this series to make it support that case. > > This patches adds CSI and MIPI-CSI2 drivers for i.MX7, along with several > configurations changes for this to work as a capture subsystem. Some bugs are > also fixed along the line. And necessary documentation. > > For a more detailed view of the capture paths, pads links in the i.MX7 please > take a look at the documentation in PATCH 14. > > The system used to test and develop this was the Warp7 board with an OV2680 > sensor, which output format is 10-bit bayer. So, only MIPI interface was > tested, a scenario with an parallel input would nice to have. > > > Bellow goes an example of the output of the pads and links and the output of > v4l2-compliance testing. > > The v4l-utils version used is: > v4l2-compliance SHA : 90905c2e4b17d7595256f3824e2d30d19b0df1a1 from Aug 6th > > The Media Driver fail some tests but this failures are coming from code out of > scope of this series (imx-capture), and some from the sensor OV2680 > but that I think not related with the sensor driver but with the testing and > core. > > The csi and mipi-csi entities pass all compliance tests. Can you rebase and repost? Please re-run v4l2-compliance as well with the latest compliance code. We should be able to merge this for 4.21 (finally!). Thanks! Hans > > Cheers, > Rui > > > v6->v7: > Myself: > - Clock patches removed from this version since they were already merged > - Rebuild and test with the latest v4l2-compliance > - Add patch to video-mux regarding bayer formats > - remove reference to dependent patch serie (was already merged) > > Sakari Ailus: > - add port and endpoint explanantions > - fix some wording should -> shall > > v5->v6: > Rob Herring: > - rename power-domain node name from: pgc-power-domain to power-domain > - change mux-control-cells to 0 > - remove bus-width from mipi bindings and dts > - remove err... regarding clock names line > - remove clk-settle from example > - split mipi-csi2 and csi bindings per file > - add OF graph description to CSI > > Philipp Zabel: > - rework group IDs and rename them with an _IPU_ prefix, this allowed to > remove >the ipu_present flag need. > > v4->v5: > Sakari Ailus: > - fix remove of the capture entries in dts bindings in the right patch > > Stephen Boyd: > - Send all series to clk list > > v3->v4: > Philipp Zabel: > - refactor initialization code from media device probe to be possible to used >from other modules > - Remove index of csi from all accurrencs (dts, code, documentation) > - Remove need for capture node for imx7 > - fix pinctrl for ov2680 > - add reviewed tag to add multiplexer controls patch > > Fabio Estevam: > - remove always on from new regulator > > Randy Dunlap: > - several text editing fixes in documentation > > Myself: > - rebase on top of v4 of Steve series > - change CSI probe to initialize imx media device > - remove csi mux parallel endpoint from mux to avoid warning message > > v2->v3: > Philipp Zabel: > - use of_match_device in imx-media-dev instead of of_device_match > - fix number of data lanes from 4 to 2 > - change the clock definitions and use of mipi > - move hs-settle from endpoint > > Rob Herring: > - fix phy-supply description > - add vendor properties > - fix examples indentations > > Stephen Boyd: patch 3/14 > - fix double sign-off > - add fixes tag > > Dong Aisheng: patch 3/14 > - fix double sign-off > - add Acked-by tag > > Shawn Guo: > patch 4/14 > - remove line breakage in parent redifiniton > - added Acked-by tag > > - dropped CMA area increase and add more verbose information in case of >dma allocation failure > patch 9/14 > - remove extra line between cells and reg masks > > Myself: > - rework on frame end in csi > - add rxcount in csi driver > - add power supplies to ov2680 node and fix gpio polarity > > v1->v2: > Dan Carpenter: > - fix return paths and codes; > - fix clk_frequency validation and return code; > - handle the csi remove (release resources that was missing) > - revert the logic arround the ipu_present flag > > Philipp Zabel: > - drop patch that changed the rgb formats and address the pixel/bus format in >mipi_csis code. > > MySelf: > - add patch that add ov2680 node to the warp7 dts, so the all data path is >complete. > - add linux-clk mailing list to the clock patches cc: > > > v4l2-compliance SHA: 90905c2e4b17d7595256f3824e2d30d19b0df1a1, 32 bits > > Compliance test for device /dev/media0: > > Media Driver Info: > Driver name : imx7-csi > Model: imx-media > Serial : > Bus info
Re: [PATCH] [v4l-utils] libv4l2subdev: Add MEDIA_BUS_FMT_FIXED to mbus_formats[]
Hi Yong,
On Tue, Nov 06, 2018 at 09:12:56AM -0800, Yong Zhi wrote:
> Also add V4L2_COLORSPACE_RAW to the colorspaces[].
>
> Signed-off-by: Yong Zhi
> ---
> utils/media-ctl/libv4l2subdev.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c
> index a989efb..46668eb 100644
> --- a/utils/media-ctl/libv4l2subdev.c
> +++ b/utils/media-ctl/libv4l2subdev.c
> @@ -855,6 +855,7 @@ static const struct {
> enum v4l2_mbus_pixelcode code;
> } mbus_formats[] = {
> #include "media-bus-format-names.h"
> + { "FIXED", MEDIA_BUS_FMT_FIXED},
> { "Y8", MEDIA_BUS_FMT_Y8_1X8},
> { "Y10", MEDIA_BUS_FMT_Y10_1X10 },
> { "Y12", MEDIA_BUS_FMT_Y12_1X12 },
> @@ -965,7 +966,9 @@ static struct {
> { "srgb", V4L2_COLORSPACE_SRGB },
> { "oprgb", V4L2_COLORSPACE_OPRGB },
> { "bt2020", V4L2_COLORSPACE_BT2020 },
> + { "raw", V4L2_COLORSPACE_RAW },
> { "dcip3", V4L2_COLORSPACE_DCI_P3 },
> +
Extra newlne.
I can remove it as well while applying the patch.
> };
>
> const char *v4l2_subdev_colorspace_to_string(enum v4l2_colorspace colorspace)
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [RFC] Create test script(s?) for regression testing
On 11/06/2018 08:58 PM, Laurent Pinchart wrote: > Hi Hans, > > On Tuesday, 6 November 2018 15:56:34 EET Hans Verkuil wrote: >> On 11/06/18 14:12, Laurent Pinchart wrote: >>> On Tuesday, 6 November 2018 13:36:55 EET Sakari Ailus wrote: On Tue, Nov 06, 2018 at 09:37:07AM +0100, Hans Verkuil wrote: > Hi all, > > After the media summit (heavy on test discussions) and the V4L2 event > regression we just found it is clear we need to do a better job with > testing. > > All the pieces are in place, so what is needed is to combine it and > create a script that anyone of us as core developers can run to check > for regressions. The same script can be run as part of the kernelci > regression testing. I'd say that *some* pieces are in place. Of course, the more there is, the better. The more there are tests, the more important it would be they're automated, preferrably without the developer having to run them on his/ her own machine. >>> >>> From my experience with testing, it's important to have both a core set of >>> tests (a.k.a. smoke tests) that can easily be run on developers' machines, >>> and extended tests that can be offloaded to a shared testing >>> infrastructure (but possibly also run locally if desired). >> >> That was my idea as well for the longer term. First step is to do the basic >> smoke tests (i.e. run compliance tests, do some (limited) streaming test). >> >> There are more extensive (and longer running) tests that can be done, but >> that's something to look at later. >> > We have four virtual drivers: vivid, vim2m, vimc and vicodec. The last > one is IMHO not quite good enough yet for testing: it is not fully > compliant to the upcoming stateful codec spec. Work for that is planned > as part of an Outreachy project. > > My idea is to create a script that is maintained as part of v4l-utils > that loads the drivers and runs v4l2-compliance and possibly other tests > against the virtual drivers. How about spending a little time to pick a suitable framework for running the tests? It could be useful to get more informative reports than just pass / fail. >>> >>> We should keep in mind that other tests will be added later, and the test >>> framework should make that easy. >> >> Since we want to be able to run this on kernelci.org, I think it makes sense >> to let the kernelci folks (Hi Ezequiel!) decide this. > > KernelCI isn't the only test infrastructure out there, so let's not forget > about the other ones. True, but they are putting time and money into this, so they get to choose as far as I am concerned :-) If others are interested and willing to put up time and money, they should let themselves be known. I'm not going to work on such an integration, although I happily accept patches. > >> As a developer all I need is a script to run smoke tests so I can catch most >> regressions (you never catch all). >> >> I'm happy to work with them to make any changes to compliance tools and >> scripts so they fit better into their test framework. >> >> The one key requirement to all this is that you should be able to run these >> tests without dependencies to all sorts of external packages/libraries. > > v4l-utils already has a set of dependencies, but those are largely > manageable. > For v4l2-compliance we'll install libv4l, which depends on libjpeg. That's already too much. You can manually build v4l2-compliance with no dependencies whatsoever, but we're missing a Makefile target for that. It's been useful for embedded systems with poor cross-compile environments. It is really very useful to be able to compile those core utilities with no external libraries other than glibc. You obviously will loose some functionality when you compile it that way. These utilities are not like a typical application. I really don't care how many libraries are linked in by e.g. qv4l2, xawtv, etc. But for v4l2-ctl, v4l2-compliance, cec-ctl/follower/compliance (and probably a few others as well) you want a minimum of dependencies so you can run them everywhere, even with the crappiest toolchains or cross-compile environments. > >>> Regarding the test output, many formats exist (see >>> https://testanything.org/ and >>> https://chromium.googlesource.com/chromium/src/+/master/docs/testing/ >>> json_test_results_format.md for instance), we should pick one of the >>> leading industry standards (what those standards are still needs to be >>> researched :-)). >>> Do note that for different hardware the tests would be likely different as well although there are classes of devices for which the exact same tests would be applicable. >>> >>> See http://git.ideasonboard.com/renesas/vsp-tests.git for an example of >>> device-specific tests. I think some of that could be generalized. >>> > It should be simple to use and require very little in the way of >
