[PATCH] media: v4l2-subdev.h: allow V4L2_FRMIVAL_TYPE_CONTINUOUS & _STEPWISE
add max_interval and step_interval to struct v4l2_subdev_frame_interval_enum. When filled correctly by the sensor driver, those fields must be used as follows by the intermediate level : struct v4l2_frmivalenum *fival; struct v4l2_subdev_frame_interval_enum fie; if (fie.max_interval.numerator == 0) { fival->type = V4L2_FRMIVAL_TYPE_DISCRETE; fival->discrete = fie.interval; } else if (fie.step_interval.numerator == 0) { fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS; fival->stepwise.min = fie.interval; fival->stepwise.max = fie.max_interval; } else { fival->type = V4L2_FRMIVAL_TYPE_STEPWISE; fival->stepwise.min = fie.interval; fival->stepwise.max = fie.max_interval; fival->stepwise.step = fie.step_interval; } Signed-off-by: Philippe De Muyter --- .../uapi/v4l/vidioc-subdev-enum-frame-interval.rst | 39 +- include/uapi/linux/v4l2-subdev.h | 4 ++- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst index 1bfe386..acc516e 100644 --- a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst +++ b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst @@ -51,6 +51,37 @@ EINVAL error code if one of the input fields is invalid. All frame intervals are enumerable by beginning at index zero and incrementing by one until ``EINVAL`` is returned. +If the sub-device can work only at the fixed set of frame intervals, +driver must enumerate them with increasing indexes, by only filling +the ``interval`` field. If the sub-device can work with a continuous +range of frame intervals, driver must only return success for index 0 +and fill ``interval`` with the minimum interval, ``max_interval`` with +the maximum interval, and ``step_interval`` with 0 or the step between +the possible intervals. + +Callers are expected to use the returned information as follows : + +.. code-block:: c + +struct v4l2_frmivalenum * fival; +struct v4l2_subdev_frame_interval_enum fie; + +if (fie.max_interval.numerator == 0) { +fival->type = V4L2_FRMIVAL_TYPE_DISCRETE; +fival->discrete = fie.interval; +} else if (fie.step_interval.numerator == 0) { +fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS; +fival->stepwise.min = fie.interval; +fival->stepwise.max = fie.max_interval; +} else { +fival->type = V4L2_FRMIVAL_TYPE_STEPWISE; +fival->stepwise.min = fie.interval; +fival->stepwise.max = fie.max_interval; +fival->stepwise.step = fie.step_interval; +} + +.. code-block:: c + Available frame intervals may depend on the current 'try' formats at other pads of the sub-device, as well as on the current active links. See :ref:`VIDIOC_SUBDEV_G_FMT` for more @@ -92,8 +123,14 @@ multiple pads of the same sub-device is not defined. - ``which`` - Frame intervals to be enumerated, from enum :ref:`v4l2_subdev_format_whence `. +* - struct :c:type:`v4l2_fract` + - ``max_interval`` + - Maximum period, in seconds, between consecutive video frames, or 0. +* - struct :c:type:`v4l2_fract` + - ``step_interval`` + - Frame interval step size, in seconds, or 0. * - __u32 - - ``reserved``\ [8] + - ``reserved``\ [4] - Reserved for future extensions. Applications and drivers must set the array to zero. diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h index 03970ce..c944644 100644 --- a/include/uapi/linux/v4l2-subdev.h +++ b/include/uapi/linux/v4l2-subdev.h @@ -128,7 +128,9 @@ struct v4l2_subdev_frame_interval_enum { __u32 height; struct v4l2_fract interval; __u32 which; - __u32 reserved[8]; + struct v4l2_fract max_interval; + struct v4l2_fract step_interval; + __u32 reserved[4]; }; /** -- 1.8.4
[PATCH 0/2] HEVC/H.265 stateless support for V4L2 and Cedrus
This introduces the required bits for supporting HEVC/H.265 both in the V4L2 framework and the Cedrus VPU driver that concerns Allwinner devices. A specific pixel format is introduced for the HEVC slice format and controls are provided to pass the bitstream metadata to the decoder. Some bitstream extensions are knowingly not supported at this point. Since this is the first proposal for stateless HEVC/H.265 support in V4L2, reviews and comments about the controls definitions are particularly welcome. On the Cedrus side, the H.265 implementation covers frame pictures with both uni-directional and bi-direction prediction modes (P/B slices). Field pictures (interleaved), scaling lists and 10-bit output are not supported at this point. This series is based upon the following series: * Cedrus driver for the Allwinner Video Engine, using media requests * media: cedrus: Add H264 decoding support Cheers! Paul Kocialkowski (2): media: v4l: Add definitions for the HEVC slice format and controls media: cedrus: Add HEVC/H.265 decoding support .../media/uapi/v4l/extended-controls.rst | 416 ++ .../media/uapi/v4l/pixfmt-compressed.rst | 15 + .../media/uapi/v4l/vidioc-queryctrl.rst | 18 + .../media/videodev2.h.rst.exceptions | 3 + drivers/media/v4l2-core/v4l2-ctrls.c | 26 + drivers/media/v4l2-core/v4l2-ioctl.c | 1 + drivers/staging/media/sunxi/cedrus/Makefile | 2 +- drivers/staging/media/sunxi/cedrus/cedrus.c | 19 + drivers/staging/media/sunxi/cedrus/cedrus.h | 20 +- .../staging/media/sunxi/cedrus/cedrus_dec.c | 9 + .../staging/media/sunxi/cedrus/cedrus_h265.c | 540 ++ .../staging/media/sunxi/cedrus/cedrus_hw.c| 4 + .../staging/media/sunxi/cedrus/cedrus_regs.h | 290 ++ .../staging/media/sunxi/cedrus/cedrus_video.c | 13 + include/media/v4l2-ctrls.h| 6 + include/uapi/linux/v4l2-controls.h| 155 + include/uapi/linux/videodev2.h| 7 + 17 files changed, 1542 insertions(+), 2 deletions(-) create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_h265.c -- 2.18.0
cron job: media_tree daily build: ERRORS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Tue Aug 28 09:08:16 CEST 2018 media-tree git hash:da2048b7348a0be92f706ac019e022139e29495e media_build git hash: baf45935ffad914f33faf751ad9f4d0dd276c021 v4l-utils git hash: e37fbf50a28c1a1cfe9e00a60542bc14192a87ba edid-decode git hash: b2da1516df3cc2756bfe8d1fa06d7bf2562ba1f4 gcc version:i686-linux-gcc (GCC) 8.1.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.18.0-marune linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-arm-stm32: OK linux-git-arm64: OK linux-git-i686: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK Check COMPILE_TEST: OK linux-2.6.36.4-i686: OK linux-2.6.36.4-x86_64: OK linux-2.6.37.6-i686: OK linux-2.6.37.6-x86_64: OK linux-2.6.38.8-i686: OK linux-2.6.38.8-x86_64: OK linux-2.6.39.4-i686: OK linux-2.6.39.4-x86_64: OK linux-3.0.101-i686: OK linux-3.0.101-x86_64: OK linux-3.1.10-i686: OK linux-3.1.10-x86_64: OK linux-3.2.102-i686: OK linux-3.2.102-x86_64: OK linux-3.3.8-i686: OK linux-3.3.8-x86_64: OK linux-3.4.113-i686: OK linux-3.4.113-x86_64: OK linux-3.5.7-i686: OK linux-3.5.7-x86_64: OK linux-3.6.11-i686: OK linux-3.6.11-x86_64: OK linux-3.7.10-i686: OK linux-3.7.10-x86_64: OK linux-3.8.13-i686: OK linux-3.8.13-x86_64: OK linux-3.9.11-i686: OK linux-3.9.11-x86_64: OK linux-3.10.108-i686: OK linux-3.10.108-x86_64: OK linux-3.11.10-i686: OK linux-3.11.10-x86_64: OK linux-3.12.74-i686: OK linux-3.12.74-x86_64: OK linux-3.13.11-i686: OK linux-3.13.11-x86_64: OK linux-3.14.79-i686: OK linux-3.14.79-x86_64: OK linux-3.15.10-i686: OK linux-3.15.10-x86_64: OK linux-3.16.57-i686: OK linux-3.16.57-x86_64: OK linux-3.17.8-i686: OK linux-3.17.8-x86_64: OK linux-3.18.119-i686: OK linux-3.18.119-x86_64: OK linux-3.19.8-i686: OK linux-3.19.8-x86_64: OK linux-4.0.9-i686: OK linux-4.0.9-x86_64: OK linux-4.1.52-i686: OK linux-4.1.52-x86_64: OK linux-4.2.8-i686: OK linux-4.2.8-x86_64: OK linux-4.3.6-i686: OK linux-4.3.6-x86_64: OK linux-4.4.152-i686: OK linux-4.4.152-x86_64: OK linux-4.5.7-i686: OK linux-4.5.7-x86_64: OK linux-4.6.7-i686: OK linux-4.6.7-x86_64: ERRORS linux-4.7.10-i686: OK linux-4.7.10-x86_64: ERRORS linux-4.8.17-i686: OK linux-4.8.17-x86_64: ERRORS linux-4.9.124-i686: OK linux-4.9.124-x86_64: OK linux-4.10.17-i686: OK linux-4.10.17-x86_64: ERRORS linux-4.11.12-i686: OK linux-4.11.12-x86_64: ERRORS linux-4.12.14-i686: OK linux-4.12.14-x86_64: ERRORS linux-4.13.16-i686: OK linux-4.13.16-x86_64: ERRORS linux-4.14.67-i686: OK linux-4.14.67-x86_64: OK linux-4.15.18-i686: OK linux-4.15.18-x86_64: ERRORS 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.5-i686: OK linux-4.18.5-x86_64: OK linux-4.19-rc1-i686: ERRORS linux-4.19-rc1-x86_64: ERRORS apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
Re: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI
On Tue, Aug 14, 2018 at 5:50 AM Mauro Carvalho Chehab wrote: > > Em Mon, 13 Aug 2018 15:42:34 +0200 > Hans Verkuil escreveu: > > > On 15/06/18 05:29, Yong Zhi wrote: > > > These meta formats are used on Intel IPU3 ImgU video queues > > > to carry 3A statistics and ISP pipeline parameters. > > > > > > V4L2_META_FMT_IPU3_3A > > > V4L2_META_FMT_IPU3_PARAMS > > > > > > Signed-off-by: Yong Zhi > > > Signed-off-by: Chao C Li > > > Signed-off-by: Rajmohan Mani > > > --- > > > Documentation/media/uapi/v4l/meta-formats.rst |1 + > > > .../media/uapi/v4l/pixfmt-meta-intel-ipu3.rst | 174 ++ > > > include/uapi/linux/intel-ipu3.h| 2816 > > > > > > 3 files changed, 2991 insertions(+) > > > create mode 100644 > > > Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst > > > create mode 100644 include/uapi/linux/intel-ipu3.h > > > > > > diff --git a/Documentation/media/uapi/v4l/meta-formats.rst > > > b/Documentation/media/uapi/v4l/meta-formats.rst > > > index 0c4e1ec..b887fca 100644 > > > --- a/Documentation/media/uapi/v4l/meta-formats.rst > > > +++ b/Documentation/media/uapi/v4l/meta-formats.rst > > > @@ -12,6 +12,7 @@ These formats are used for the :ref:`metadata` > > > interface only. > > > .. toctree:: > > > :maxdepth: 1 > > > > > > +pixfmt-meta-intel-ipu3 > > > pixfmt-meta-uvc > > > pixfmt-meta-vsp1-hgo > > > pixfmt-meta-vsp1-hgt > > > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst > > > b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst > > > new file mode 100644 > > > index 000..5c050e6 > > > --- /dev/null > > > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst > > > @@ -0,0 +1,174 @@ > > > +.. -*- coding: utf-8; mode: rst -*- > > > + > > > +.. _intel-ipu3: > > > + > > > +** > > > +V4L2_META_FMT_IPU3_PARAMS ('ip3p'), V4L2_META_FMT_IPU3_3A ('ip3s') > > > +** > > > + > > > +.. c:type:: ipu3_uapi_stats_3a > > > + > > > +3A statistics > > > += > > > + > > > +For IPU3 ImgU, the 3A statistics accelerators collect different > > > statistics over > > > +an input bayer frame. Those statistics, defined in data struct > > > +:c:type:`ipu3_uapi_stats_3a`, are meta output obtained from "ipu3-imgu > > > 3a stat" > > > +video node, which are then passed to user space for statistics analysis > > > +using :c:type:`v4l2_meta_format` interface. > > > + > > > +The statistics collected are AWB (Auto-white balance) RGBS cells, AWB > > > filter > > Just like you did with AWB, AF and AE, please place the full name in > parenthesis > for RGBS and AWB. > > > > +response, AF (Auto-focus) filter response, and AE (Auto-exposure) > > > histogram. > > > + > > > +struct :c:type:`ipu3_uapi_4a_config` saves configurable parameters for > > > all above. > > > + > > > + > > > +.. code-block:: c > > > + > > > + > > > + struct ipu3_uapi_stats_3a { > > > + IPU3_ALIGN struct ipu3_uapi_awb_raw_buffer awb_raw_buffer; > > > > IPU3_ALIGN? What's that? > > > > OK, after reading the header I see what it does, but I think you should > > drop it in the documentation since it doesn't help the reader. > > Yeah, that IPU3_ALIGN is confusing. > > Yet, instead of just dropping, I would replace it by a comment > to explain that the struct is 32-bytes aligned. > > On a separate (but related) comment, you're declaring it as: > > #define IPU3_ALIGN > __attribute__((aligned(IPU3_UAPI_ISP_WORD_BYTES))) > > This is a gcc-specific dialect. Better to use, instead, __aligned(x) > which is defined as: > > #define __aligned(x)__attribute__((aligned(x))) > Note that this is an uapi/ header. Is the __aligned() macro okay to use in uapi headers? I couldn't find any header there using it and we had problems with our user space compiling with it. By the way, I wonder if this is the right approach for controlling the layout of ABI structs. I don't see many headers using any alignment in uapi/ in general. Perhaps explicit padding bytes would be more appropriate? They are also less tricky when one structure needs to be embedded inside two or more different structures with different alignments, which can't be done easily if you specify __aligned() on the child struct. Best regards, Tomasz
Re: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI
Hi Tomasz, On Tue, Aug 28, 2018 at 05:56:37PM +0900, Tomasz Figa wrote: > On Tue, Aug 14, 2018 at 5:50 AM Mauro Carvalho Chehab > wrote: > > > > Em Mon, 13 Aug 2018 15:42:34 +0200 > > Hans Verkuil escreveu: > > > > > On 15/06/18 05:29, Yong Zhi wrote: > > > > These meta formats are used on Intel IPU3 ImgU video queues > > > > to carry 3A statistics and ISP pipeline parameters. > > > > > > > > V4L2_META_FMT_IPU3_3A > > > > V4L2_META_FMT_IPU3_PARAMS > > > > > > > > Signed-off-by: Yong Zhi > > > > Signed-off-by: Chao C Li > > > > Signed-off-by: Rajmohan Mani > > > > --- > > > > Documentation/media/uapi/v4l/meta-formats.rst |1 + > > > > .../media/uapi/v4l/pixfmt-meta-intel-ipu3.rst | 174 ++ > > > > include/uapi/linux/intel-ipu3.h| 2816 > > > > > > > > 3 files changed, 2991 insertions(+) > > > > create mode 100644 > > > > Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst > > > > create mode 100644 include/uapi/linux/intel-ipu3.h > > > > > > > > diff --git a/Documentation/media/uapi/v4l/meta-formats.rst > > > > b/Documentation/media/uapi/v4l/meta-formats.rst > > > > index 0c4e1ec..b887fca 100644 > > > > --- a/Documentation/media/uapi/v4l/meta-formats.rst > > > > +++ b/Documentation/media/uapi/v4l/meta-formats.rst > > > > @@ -12,6 +12,7 @@ These formats are used for the :ref:`metadata` > > > > interface only. > > > > .. toctree:: > > > > :maxdepth: 1 > > > > > > > > +pixfmt-meta-intel-ipu3 > > > > pixfmt-meta-uvc > > > > pixfmt-meta-vsp1-hgo > > > > pixfmt-meta-vsp1-hgt > > > > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst > > > > b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst > > > > new file mode 100644 > > > > index 000..5c050e6 > > > > --- /dev/null > > > > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst > > > > @@ -0,0 +1,174 @@ > > > > +.. -*- coding: utf-8; mode: rst -*- > > > > + > > > > +.. _intel-ipu3: > > > > + > > > > +** > > > > +V4L2_META_FMT_IPU3_PARAMS ('ip3p'), V4L2_META_FMT_IPU3_3A ('ip3s') > > > > +** > > > > + > > > > +.. c:type:: ipu3_uapi_stats_3a > > > > + > > > > +3A statistics > > > > += > > > > + > > > > +For IPU3 ImgU, the 3A statistics accelerators collect different > > > > statistics over > > > > +an input bayer frame. Those statistics, defined in data struct > > > > +:c:type:`ipu3_uapi_stats_3a`, are meta output obtained from "ipu3-imgu > > > > 3a stat" > > > > +video node, which are then passed to user space for statistics analysis > > > > +using :c:type:`v4l2_meta_format` interface. > > > > + > > > > +The statistics collected are AWB (Auto-white balance) RGBS cells, AWB > > > > filter > > > > Just like you did with AWB, AF and AE, please place the full name in > > parenthesis > > for RGBS and AWB. > > > > > > +response, AF (Auto-focus) filter response, and AE (Auto-exposure) > > > > histogram. > > > > + > > > > +struct :c:type:`ipu3_uapi_4a_config` saves configurable parameters for > > > > all above. > > > > + > > > > + > > > > +.. code-block:: c > > > > + > > > > + > > > > + struct ipu3_uapi_stats_3a { > > > > + IPU3_ALIGN struct ipu3_uapi_awb_raw_buffer awb_raw_buffer; > > > > > > IPU3_ALIGN? What's that? > > > > > > OK, after reading the header I see what it does, but I think you should > > > drop it in the documentation since it doesn't help the reader. > > > > Yeah, that IPU3_ALIGN is confusing. > > > > Yet, instead of just dropping, I would replace it by a comment > > to explain that the struct is 32-bytes aligned. > > > > On a separate (but related) comment, you're declaring it as: > > > > #define IPU3_ALIGN > > __attribute__((aligned(IPU3_UAPI_ISP_WORD_BYTES))) > > > > This is a gcc-specific dialect. Better to use, instead, __aligned(x) > > which is defined as: > > > > #define __aligned(x)__attribute__((aligned(x))) > > > > Note that this is an uapi/ header. Is the __aligned() macro okay to > use in uapi headers? I couldn't find any header there using it and we > had problems with our user space compiling with it. > > By the way, I wonder if this is the right approach for controlling the > layout of ABI structs. I don't see many headers using any alignment in > uapi/ in general. Perhaps explicit padding bytes would be more > appropriate? They are also less tricky when one structure needs to be > embedded inside two or more different structures with different > alignments, which can't be done easily if you specify __aligned() on > the child struct. One of the reasons there are not so many are probably what you just elaborated above. That said, there are a few points to note here: - the alignment is generally the same here as it's due to DMA word size AFAIK, - the device can be only found in an Intel SoC which limits the architectures wher
Re: [PATCH] media: v4l2-subdev.h: allow V4L2_FRMIVAL_TYPE_CONTINUOUS & _STEPWISE
Hi Philippe, On 28/08/18 09:55, Philippe De Muyter wrote: > add max_interval and step_interval to struct > v4l2_subdev_frame_interval_enum. Yeah, I never understood why this wasn't supported when this API was designed. Clearly an oversight. > > When filled correctly by the sensor driver, those fields must be > used as follows by the intermediate level : > > struct v4l2_frmivalenum *fival; > struct v4l2_subdev_frame_interval_enum fie; > > if (fie.max_interval.numerator == 0) { > fival->type = V4L2_FRMIVAL_TYPE_DISCRETE; > fival->discrete = fie.interval; > } else if (fie.step_interval.numerator == 0) { > fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS; > fival->stepwise.min = fie.interval; > fival->stepwise.max = fie.max_interval; > } else { > fival->type = V4L2_FRMIVAL_TYPE_STEPWISE; > fival->stepwise.min = fie.interval; > fival->stepwise.max = fie.max_interval; > fival->stepwise.step = fie.step_interval; > } This is a bit too magical for my tastes. I'd add a type field: #define V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE 0 #define V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS 1 #define V4L2_SUBDEV_FRMIVAL_TYPE_STEPWISE 2 Older applications that do not know about the type field will just see a single discrete interval containing the minimum interval. I guess that's OK as they will keep working. While at it: it would be really nice if you can also add stepwise support to VIDIOC_SUBDEV_ENUM_FRAME_SIZE. I think the only thing you need to do there is to add two new fields: step_width and step_height. If 0, then that just means a step size of 1. Add some helper functions to translate between v4l2_subdev_frame_size/interval_enum and v4l2_frmsize/ivalenum and this becomes much cleaner. Regards, Hans > > Signed-off-by: Philippe De Muyter > --- > .../uapi/v4l/vidioc-subdev-enum-frame-interval.rst | 39 > +- > include/uapi/linux/v4l2-subdev.h | 4 ++- > 2 files changed, 41 insertions(+), 2 deletions(-) > > diff --git > a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst > b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst > index 1bfe386..acc516e 100644 > --- a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst > +++ b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst > @@ -51,6 +51,37 @@ EINVAL error code if one of the input fields is invalid. > All frame > intervals are enumerable by beginning at index zero and incrementing by > one until ``EINVAL`` is returned. > > +If the sub-device can work only at the fixed set of frame intervals, > +driver must enumerate them with increasing indexes, by only filling > +the ``interval`` field. If the sub-device can work with a continuous > +range of frame intervals, driver must only return success for index 0 > +and fill ``interval`` with the minimum interval, ``max_interval`` with > +the maximum interval, and ``step_interval`` with 0 or the step between > +the possible intervals. > + > +Callers are expected to use the returned information as follows : > + > +.. code-block:: c > + > +struct v4l2_frmivalenum * fival; > +struct v4l2_subdev_frame_interval_enum fie; > + > +if (fie.max_interval.numerator == 0) { > +fival->type = V4L2_FRMIVAL_TYPE_DISCRETE; > +fival->discrete = fie.interval; > +} else if (fie.step_interval.numerator == 0) { > +fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS; > +fival->stepwise.min = fie.interval; > +fival->stepwise.max = fie.max_interval; > +} else { > +fival->type = V4L2_FRMIVAL_TYPE_STEPWISE; > +fival->stepwise.min = fie.interval; > +fival->stepwise.max = fie.max_interval; > +fival->stepwise.step = fie.step_interval; > +} > + > +.. code-block:: c > + > Available frame intervals may depend on the current 'try' formats at > other pads of the sub-device, as well as on the current active links. > See :ref:`VIDIOC_SUBDEV_G_FMT` for more > @@ -92,8 +123,14 @@ multiple pads of the same sub-device is not defined. >- ``which`` >- Frame intervals to be enumerated, from enum > :ref:`v4l2_subdev_format_whence `. > +* - struct :c:type:`v4l2_fract` > + - ``max_interval`` > + - Maximum period, in seconds, between consecutive video frames, or 0. > +* - struct :c:type:`v4l2_fract` > + - ``step_interval`` > + - Frame interval step size, in seconds, or 0. > * - __u32 > - - ``reserved``\ [8] > + - ``reserved``\ [4] >- Reserved for future extensions. Applications and drivers must set > the array to zero. > > diff --git a/include/uapi/linux/v4l2-subdev.h > b/include/uapi/linux/v4l2-subdev.h > index
Re: [PATCH] media: v4l2-subdev.h: allow V4L2_FRMIVAL_TYPE_CONTINUOUS & _STEPWISE
Hi Hans, On Tue, Aug 28, 2018 at 12:03:25PM +0200, Hans Verkuil wrote: > Hi Philippe, > > On 28/08/18 09:55, Philippe De Muyter wrote: > > add max_interval and step_interval to struct > > v4l2_subdev_frame_interval_enum. > > Yeah, I never understood why this wasn't supported when this API was designed. > Clearly an oversight. > > > > > When filled correctly by the sensor driver, those fields must be > > used as follows by the intermediate level : > > > > struct v4l2_frmivalenum *fival; > > struct v4l2_subdev_frame_interval_enum fie; > > > > if (fie.max_interval.numerator == 0) { > > fival->type = V4L2_FRMIVAL_TYPE_DISCRETE; > > fival->discrete = fie.interval; > > } else if (fie.step_interval.numerator == 0) { > > fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS; > > fival->stepwise.min = fie.interval; > > fival->stepwise.max = fie.max_interval; > > } else { > > fival->type = V4L2_FRMIVAL_TYPE_STEPWISE; > > fival->stepwise.min = fie.interval; > > fival->stepwise.max = fie.max_interval; > > fival->stepwise.step = fie.step_interval; > > } > > This is a bit too magical for my tastes. I'd add a type field: > > #define V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE 0 > #define V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS 1 > #define V4L2_SUBDEV_FRMIVAL_TYPE_STEPWISE 2 Like that ? struct v4l2_subdev_frame_interval_enum { __u32 index; __u32 pad; __u32 code; __u32 width; __u32 height; struct v4l2_fract interval; __u32 which; __u32 type; struct v4l2_fract max_interval; struct v4l2_fract step_interval; __u32 reserved[3]; }; > > Older applications that do not know about the type field will just > see a single discrete interval containing the minimum interval. > I guess that's OK as they will keep working. That's actually what also happens with the above implementation, because max_interval.numerator and step_interval.numerator were previously reserved and thus 0, and if this code is moved to a helper function, that does not matter if it's a little bit magical :). Both implementations are equal to me, but the proposed one uses less space from the 'reserved' field. > > While at it: it would be really nice if you can also add stepwise > support to VIDIOC_SUBDEV_ENUM_FRAME_SIZE. I think the only thing > you need to do there is to add two new fields: step_width and step_height. > If 0, then that just means a step size of 1. I'll look at that if I find enough interest and test opportunity for it, but those things are unrelated except that they are missing :) > > Add some helper functions to translate between > v4l2_subdev_frame_size/interval_enum > and v4l2_frmsize/ivalenum and this becomes much cleaner. OK > > Regards, > > Hans > > > > > Signed-off-by: Philippe De Muyter > > --- > > .../uapi/v4l/vidioc-subdev-enum-frame-interval.rst | 39 > > +- > > include/uapi/linux/v4l2-subdev.h | 4 ++- > > 2 files changed, 41 insertions(+), 2 deletions(-) > > > > diff --git > > a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst > > b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst > > index 1bfe386..acc516e 100644 > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst > > +++ b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst > > @@ -51,6 +51,37 @@ EINVAL error code if one of the input fields is invalid. > > All frame > > intervals are enumerable by beginning at index zero and incrementing by > > one until ``EINVAL`` is returned. > > > > +If the sub-device can work only at the fixed set of frame intervals, > > +driver must enumerate them with increasing indexes, by only filling > > +the ``interval`` field. If the sub-device can work with a continuous > > +range of frame intervals, driver must only return success for index 0 > > +and fill ``interval`` with the minimum interval, ``max_interval`` with > > +the maximum interval, and ``step_interval`` with 0 or the step between > > +the possible intervals. > > + > > +Callers are expected to use the returned information as follows : > > + > > +.. code-block:: c > > + > > +struct v4l2_frmivalenum * fival; > > +struct v4l2_subdev_frame_interval_enum fie; > > + > > +if (fie.max_interval.numerator == 0) { > > +fival->type = V4L2_FRMIVAL_TYPE_DISCRETE; > > +fival->discrete = fie.interval; > > +} else if (fie.step_interval.numerator == 0) { > > +fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS; > > +fival->stepwise.min = fie.interval; > > +fival->stepwise.max = fie.max_interval; > > +
Re: [PATCH] media: v4l2-subdev.h: allow V4L2_FRMIVAL_TYPE_CONTINUOUS & _STEPWISE
On 28/08/18 12:26, Philippe De Muyter wrote: > Hi Hans, > > On Tue, Aug 28, 2018 at 12:03:25PM +0200, Hans Verkuil wrote: >> Hi Philippe, >> >> On 28/08/18 09:55, Philippe De Muyter wrote: >>> add max_interval and step_interval to struct >>> v4l2_subdev_frame_interval_enum. >> >> Yeah, I never understood why this wasn't supported when this API was >> designed. >> Clearly an oversight. >> >>> >>> When filled correctly by the sensor driver, those fields must be >>> used as follows by the intermediate level : >>> >>> struct v4l2_frmivalenum *fival; >>> struct v4l2_subdev_frame_interval_enum fie; >>> >>> if (fie.max_interval.numerator == 0) { >>> fival->type = V4L2_FRMIVAL_TYPE_DISCRETE; >>> fival->discrete = fie.interval; >>> } else if (fie.step_interval.numerator == 0) { >>> fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS; >>> fival->stepwise.min = fie.interval; >>> fival->stepwise.max = fie.max_interval; >>> } else { >>> fival->type = V4L2_FRMIVAL_TYPE_STEPWISE; >>> fival->stepwise.min = fie.interval; >>> fival->stepwise.max = fie.max_interval; >>> fival->stepwise.step = fie.step_interval; >>> } >> >> This is a bit too magical for my tastes. I'd add a type field: >> >> #define V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE 0 >> #define V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS 1 >> #define V4L2_SUBDEV_FRMIVAL_TYPE_STEPWISE 2 > > Like that ? > > struct v4l2_subdev_frame_interval_enum { > __u32 index; > __u32 pad; > __u32 code; > __u32 width; > __u32 height; > struct v4l2_fract interval; > __u32 which; > __u32 type; > struct v4l2_fract max_interval; > struct v4l2_fract step_interval; > __u32 reserved[3]; > }; Yes. > > >> >> Older applications that do not know about the type field will just >> see a single discrete interval containing the minimum interval. >> I guess that's OK as they will keep working. > > That's actually what also happens with the above implementation, because > max_interval.numerator and step_interval.numerator were previously > reserved and thus 0, and if this code is moved to a helper function, > that does not matter if it's a little bit magical :). It's not just the kernel, but userspace can also enumerate directly from a v4l-subdevX device node, and they can't use any kernel helper functions. Let's keep this API clean. > Both implementations are equal to me, but the proposed one uses less > space from the 'reserved' field. That's OK. Regards, Hans > >> >> While at it: it would be really nice if you can also add stepwise >> support to VIDIOC_SUBDEV_ENUM_FRAME_SIZE. I think the only thing >> you need to do there is to add two new fields: step_width and step_height. >> If 0, then that just means a step size of 1. > > I'll look at that if I find enough interest and test opportunity for it, > but those things are unrelated except that they are missing :) > >> >> Add some helper functions to translate between >> v4l2_subdev_frame_size/interval_enum >> and v4l2_frmsize/ivalenum and this becomes much cleaner. > > OK > >> >> Regards, >> >> Hans >> >>> >>> Signed-off-by: Philippe De Muyter >>> --- >>> .../uapi/v4l/vidioc-subdev-enum-frame-interval.rst | 39 >>> +- >>> include/uapi/linux/v4l2-subdev.h | 4 ++- >>> 2 files changed, 41 insertions(+), 2 deletions(-) >>> >>> diff --git >>> a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst >>> b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst >>> index 1bfe386..acc516e 100644 >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst >>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst >>> @@ -51,6 +51,37 @@ EINVAL error code if one of the input fields is invalid. >>> All frame >>> intervals are enumerable by beginning at index zero and incrementing by >>> one until ``EINVAL`` is returned. >>> >>> +If the sub-device can work only at the fixed set of frame intervals, >>> +driver must enumerate them with increasing indexes, by only filling >>> +the ``interval`` field. If the sub-device can work with a continuous >>> +range of frame intervals, driver must only return success for index 0 >>> +and fill ``interval`` with the minimum interval, ``max_interval`` with >>> +the maximum interval, and ``step_interval`` with 0 or the step between >>> +the possible intervals. >>> + >>> +Callers are expected to use the returned information as follows : >>> + >>> +.. code-block:: c >>> + >>> +struct v4l2_frmivalenum * fival; >>> +struct v4l2_subdev_frame_interval_enum fie; >>> + >>> +if (fie.max_interval.numerator == 0) { >>> +fival->type
Re: [PATCH] media: v4l2-subdev.h: allow V4L2_FRMIVAL_TYPE_CONTINUOUS & _STEPWISE
Hi Hans, On Tue, Aug 28, 2018 at 12:29:21PM +0200, Hans Verkuil wrote: > On 28/08/18 12:26, Philippe De Muyter wrote: > > Hi Hans, > > > > On Tue, Aug 28, 2018 at 12:03:25PM +0200, Hans Verkuil wrote: > >> This is a bit too magical for my tastes. I'd add a type field: > >> > >> #define V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE 0 > >> #define V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS 1 > >> #define V4L2_SUBDEV_FRMIVAL_TYPE_STEPWISE 2 Should I put that in an enum like 'enum v4l2_subdev_format_whence' or use simple define's ? Philippe -- Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles
Re: [PATCH] media: v4l2-subdev.h: allow V4L2_FRMIVAL_TYPE_CONTINUOUS & _STEPWISE
On 28/08/18 12:40, Philippe De Muyter wrote: > Hi Hans, > > On Tue, Aug 28, 2018 at 12:29:21PM +0200, Hans Verkuil wrote: >> On 28/08/18 12:26, Philippe De Muyter wrote: >>> Hi Hans, >>> >>> On Tue, Aug 28, 2018 at 12:03:25PM +0200, Hans Verkuil wrote: This is a bit too magical for my tastes. I'd add a type field: #define V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE 0 #define V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS 1 #define V4L2_SUBDEV_FRMIVAL_TYPE_STEPWISE 2 > > Should I put that in an enum like 'enum v4l2_subdev_format_whence' > or use simple define's ? Use an enum for consistency with the existing framesize/ival APIs. Regards, Hans
Re: [PATCH 4/5] videodev2.h: add new capabilities for buffer types
On 24/08/18 16:36, Tomasz Figa wrote: > Hi Hans, > > On Fri, Aug 24, 2018 at 5:22 PM Hans Verkuil wrote: >> >> From: Hans Verkuil >> >> VIDIOC_REQBUFS and VIDIOC_CREATE_BUFFERS will return capabilities >> telling userspace what the given buffer type is capable of. >> > > Please see my comments below. > >> Signed-off-by: Hans Verkuil >> --- >> .../media/uapi/v4l/vidioc-create-bufs.rst | 10 +- >> .../media/uapi/v4l/vidioc-reqbufs.rst | 36 ++- >> include/uapi/linux/videodev2.h| 13 +-- >> 3 files changed, 55 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst >> b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst >> index a39e18d69511..fd34d3f236c9 100644 >> --- a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst >> +++ b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst >> @@ -102,7 +102,15 @@ than the number requested. >>- ``format`` >>- Filled in by the application, preserved by the driver. >> * - __u32 >> - - ``reserved``\ [8] >> + - ``capabilities`` >> + - Set by the driver. If 0, then the driver doesn't support >> +capabilities. In that case all you know is that the driver is >> + guaranteed to support ``V4L2_MEMORY_MMAP`` and *might* support >> + other :c:type:`v4l2_memory` types. It will not support any others >> + capabilities. See :ref:`here ` for a list of >> the >> + capabilities. > > Perhaps it would make sense to document how the application is > expected to query for these capabilities? Right now, the application > is expected to fill in the "memory" field in this struct (and reqbufs > counterpart), but it sounds a bit strange that one needs to know what > "memory" value to write there to query what set of "memory" values is > supported. In theory, MMAP is expected to be always supported, but it > sounds strange anyway. Also, is there a way to call REQBUFS without > altering the buffer allocation? No, this is only possible with CREATE_BUFS. But it is reasonable to call REQBUFS with a count of 0, since you want to start with a clean slate anyway. The only option I see would be to introduce a new memory type (e.g. V4L2_MEMORY_CAPS) to just return the capabilities. But that's more ugly then just requiring that you use MMAP when calling this. I'm inclined to just document that you need to set memory to MMAP if you want to get the capabilities since MMAP is always guaranteed to exist. Regards, Hans
Re: [PATCH] media: ov5640: fix mode change regression
Hi Hugues, thanks for the patch On Thu, Aug 16, 2018 at 11:46:53AM +0200, Hugues Fruchet wrote: > fixes: 6949d864776e ("media: ov5640: do not change mode if format or frame > interval is unchanged"). > > Symptom was fuzzy image because of JPEG default format > not being changed according to new format selected, fix this. > Init sequence initialises format to YUV422 UYVY but > sensor->fmt initial value was set to JPEG, fix this. > > Signed-off-by: Hugues Fruchet > --- > drivers/media/i2c/ov5640.c | 21 - > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 071f4bc..2ddd86d 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -223,6 +223,7 @@ struct ov5640_dev { > int power_count; > > struct v4l2_mbus_framefmt fmt; > + bool pending_fmt_change; The foundamental issue here is that 'struct ov5640_mode_info' and associated functions do not take the image format into account... That would be the real fix, but I understand it requires changing and re-testing a lot of stuff :( But what if instead of adding more flags, don't we use bitfields in a single "pending_changes" field? As when, and if, framerate will be made more 'dynamic' and we remove the static 15/30FPS configuration from ov5640_mode_info, we will have the same problem we have today with format with framerate too... Something like: struct ov5640_dev { ... - bool pending_mode_change; + #define MODE_CHANGE BIT(0) + #define FMT_CHANGE BIT(1) + u8 pending; ... } > > const struct ov5640_mode_info *current_mode; > enum ov5640_frame_rate current_fr; > @@ -255,7 +256,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct > v4l2_ctrl *ctrl) > * should be identified and removed to speed register load time > * over i2c. > */ > - > +/* YUV422 UYVY VGA@30fps */ > static const struct reg_value ov5640_init_setting_30fps_VGA[] = { > {0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0}, > {0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0}, > @@ -1968,9 +1969,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, > > if (new_mode != sensor->current_mode) { > sensor->current_mode = new_mode; > - sensor->fmt = *mbus_fmt; > sensor->pending_mode_change = true; > } > + if (mbus_fmt->code != sensor->fmt.code) { > + sensor->fmt = *mbus_fmt; > + sensor->pending_fmt_change = true; > + } That would make this simpler sensor->current_mode = new_mode; sensor->fmt = *mbus_fmt; if (new_mode != sensor->current_mode) sensor->pending |= MODE_CHANGE; if (mbus_fmt->code != sensor->fmt.code) { sensor->pending |= FMT_CHANGE; > out: > mutex_unlock(&sensor->lock); > return ret; > @@ -2544,10 +2548,13 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, > int enable) > ret = ov5640_set_mode(sensor, sensor->current_mode); > if (ret) > goto out; > + } > > + if (enable && sensor->pending_fmt_change) { > ret = ov5640_set_framefmt(sensor, &sensor->fmt); > if (ret) > goto out; > + sensor->pending_fmt_change = false; > } > And that would be accordingly: if (sensor->pending & MODE_CHANGE) { ret = ov5640_set_mode(sensor, sensor->current_mode); } if (sensor->pending & FMT_CHANGE) { ret = ov5640_set_framefmt(sensor, &sensor->fmt); ... } What do you (and others) think? Thanks j > if (sensor->ep.bus_type == V4L2_MBUS_CSI2) > @@ -2642,9 +2649,14 @@ static int ov5640_probe(struct i2c_client *client, > return -ENOMEM; > > sensor->i2c_client = client; > + > + /* > + * default init sequence initialize sensor to > + * YUV422 UYVY VGA@30fps > + */ > fmt = &sensor->fmt; > - fmt->code = ov5640_formats[0].code; > - fmt->colorspace = ov5640_formats[0].colorspace; > + fmt->code = MEDIA_BUS_FMT_UYVY8_2X8; > + fmt->colorspace = V4L2_COLORSPACE_SRGB; > fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace); > fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; > fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace); > @@ -2656,7 +2668,6 @@ static int ov5640_probe(struct i2c_client *client, > sensor->current_fr = OV5640_30_FPS; > sensor->current_mode = > &ov5640_mode_data[OV5640_30_FPS][OV5640_MODE_VGA_640_480]; > - sensor->pending_mode_change = true; >
Re: [PATCH] media: ov5640: fix mode change regression
On Tue, Aug 28, 2018 at 02:57:11PM +0200, jacopo mondi wrote: > Hi Hugues, >thanks for the patch > > On Thu, Aug 16, 2018 at 11:46:53AM +0200, Hugues Fruchet wrote: > > fixes: 6949d864776e ("media: ov5640: do not change mode if format or frame > > interval is unchanged"). > > > > Symptom was fuzzy image because of JPEG default format > > not being changed according to new format selected, fix this. > > Init sequence initialises format to YUV422 UYVY but > > sensor->fmt initial value was set to JPEG, fix this. > > > > Signed-off-by: Hugues Fruchet > > --- > > drivers/media/i2c/ov5640.c | 21 - > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > index 071f4bc..2ddd86d 100644 > > --- a/drivers/media/i2c/ov5640.c > > +++ b/drivers/media/i2c/ov5640.c > > @@ -223,6 +223,7 @@ struct ov5640_dev { > > int power_count; > > > > struct v4l2_mbus_framefmt fmt; > > + bool pending_fmt_change; > > The foundamental issue here is that 'struct ov5640_mode_info' and > associated functions do not take the image format into account... > That would be the real fix, but I understand it requires changing and > re-testing a lot of stuff :( > > But what if instead of adding more flags, don't we use bitfields in a single > "pending_changes" field? As when, and if, framerate will be made more > 'dynamic' and we remove the static 15/30FPS configuration from > ov5640_mode_info, we will have the same problem we have today with > format with framerate too... > > Something like: > > struct ov5640_dev { > ... > - bool pending_mode_change; > + #define MODE_CHANGE BIT(0) > + #define FMT_CHANGE BIT(1) > + u8 pending; > ... > } > > > > > const struct ov5640_mode_info *current_mode; > > enum ov5640_frame_rate current_fr; > > @@ -255,7 +256,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct > > v4l2_ctrl *ctrl) > > * should be identified and removed to speed register load time > > * over i2c. > > */ > > - > > +/* YUV422 UYVY VGA@30fps */ > > static const struct reg_value ov5640_init_setting_30fps_VGA[] = { > > {0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0}, > > {0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0}, > > @@ -1968,9 +1969,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, > > > > if (new_mode != sensor->current_mode) { > > sensor->current_mode = new_mode; > > - sensor->fmt = *mbus_fmt; > > sensor->pending_mode_change = true; > > } > > + if (mbus_fmt->code != sensor->fmt.code) { > > + sensor->fmt = *mbus_fmt; > > + sensor->pending_fmt_change = true; > > + } > > That would make this simpler > > sensor->current_mode = new_mode; > sensor->fmt = *mbus_fmt; > > if (new_mode != sensor->current_mode) > sensor->pending |= MODE_CHANGE; > if (mbus_fmt->code != sensor->fmt.code) { > sensor->pending |= FMT_CHANGE; > Yeah, well, this is in wrong order of course :) signature.asc Description: PGP signature
This is it
Do you have photos for cutting out,or adding clipping path? We are here to help you for that also including retouching. Both for product photos and portrait photos. Yours, Jason
This is it
Do you have photos for cutting out,or adding clipping path? We are here to help you for that also including retouching. Both for product photos and portrait photos. Yours, Jason
[PATCHv2 08/10] v4l2-ctrls: improve media_request_(un)lock_for_update
From: Hans Verkuil The request reference count was decreased again once a reference to the request object was taken. Postpone this until we finished using the object. In theory I think it is possible that the request_fd can be closed by the application from another thread. In that case when request_put is called the whole request would be freed. It's highly unlikely, but let's just be safe and fix this potential race condition. Signed-off-by: Hans Verkuil --- drivers/media/v4l2-core/v4l2-ctrls.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index cc266a4a6e88..95d065d54308 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -3657,10 +3657,9 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, } obj = v4l2_ctrls_find_req_obj(hdl, req, set); - /* Reference to the request held through obj */ - media_request_put(req); if (IS_ERR(obj)) { media_request_unlock_for_update(req); + media_request_put(req); return PTR_ERR(obj); } hdl = container_of(obj, struct v4l2_ctrl_handler, @@ -3670,8 +3669,9 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, ret = try_set_ext_ctrls_common(fh, hdl, cs, set); if (obj) { - media_request_unlock_for_update(obj->req); + media_request_unlock_for_update(req); media_request_object_put(obj); + media_request_put(req); } return ret; -- 2.18.0
[PATCHv2 04/10] videodev2.h: add new capabilities for buffer types
From: Hans Verkuil VIDIOC_REQBUFS and VIDIOC_CREATE_BUFFERS will return capabilities telling userspace what the given buffer type is capable of. Signed-off-by: Hans Verkuil --- .../media/uapi/v4l/vidioc-create-bufs.rst | 14 ++- .../media/uapi/v4l/vidioc-reqbufs.rst | 42 ++- include/uapi/linux/videodev2.h| 13 +- 3 files changed, 65 insertions(+), 4 deletions(-) diff --git a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst index a39e18d69511..eadf6f757fbf 100644 --- a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst +++ b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst @@ -102,7 +102,19 @@ than the number requested. - ``format`` - Filled in by the application, preserved by the driver. * - __u32 - - ``reserved``\ [8] + - ``capabilities`` + - Set by the driver. If 0, then the driver doesn't support +capabilities. In that case all you know is that the driver is + guaranteed to support ``V4L2_MEMORY_MMAP`` and *might* support + other :c:type:`v4l2_memory` types. It will not support any others + capabilities. See :ref:`here ` for a list of the + capabilities. + + If you want to just query the capabilities without making any + other changes, then set ``count`` to 0, ``memory`` to + ``V4L2_MEMORY_MMAP`` and ``format.type`` to the buffer type. +* - __u32 + - ``reserved``\ [7] - A place holder for future extensions. Drivers and applications must set the array to zero. diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst index 316f52c8a310..d40c60e8 100644 --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst @@ -88,10 +88,50 @@ any DMA in progress, an implicit ``V4L2_MEMORY_DMABUF`` or ``V4L2_MEMORY_USERPTR``. See :c:type:`v4l2_memory`. * - __u32 - - ``reserved``\ [2] + - ``capabilities`` + - Set by the driver. If 0, then the driver doesn't support +capabilities. In that case all you know is that the driver is + guaranteed to support ``V4L2_MEMORY_MMAP`` and *might* support + other :c:type:`v4l2_memory` types. It will not support any others + capabilities. + + If you want to query the capabilities with a minimum of side-effects, + then this can be called with ``count`` set to 0, ``memory`` set to + ``V4L2_MEMORY_MMAP`` and ``type`` set to the buffer type. This will + free any previously allocated buffers, so this is typically something + that will be done at the start of the application. +* - __u32 + - ``reserved``\ [1] - A place holder for future extensions. Drivers and applications must set the array to zero. +.. tabularcolumns:: |p{6.1cm}|p{2.2cm}|p{8.7cm}| + +.. _v4l2-buf-capabilities: +.. _V4L2-BUF-CAP-SUPPORTS-MMAP: +.. _V4L2-BUF-CAP-SUPPORTS-USERPTR: +.. _V4L2-BUF-CAP-SUPPORTS-DMABUF: +.. _V4L2-BUF-CAP-SUPPORTS-REQUESTS: + +.. cssclass:: longtable + +.. flat-table:: V4L2 Buffer Capabilities Flags +:header-rows: 0 +:stub-columns: 0 +:widths: 3 1 4 + +* - ``V4L2_BUF_CAP_SUPPORTS_MMAP`` + - 0x0001 + - This buffer type supports the ``V4L2_MEMORY_MMAP`` streaming mode. +* - ``V4L2_BUF_CAP_SUPPORTS_USERPTR`` + - 0x0002 + - This buffer type supports the ``V4L2_MEMORY_USERPTR`` streaming mode. +* - ``V4L2_BUF_CAP_SUPPORTS_DMABUF`` + - 0x0004 + - This buffer type supports the ``V4L2_MEMORY_DMABUF`` streaming mode. +* - ``V4L2_BUF_CAP_SUPPORTS_REQUESTS`` + - 0x0008 + - This buffer type supports :ref:`requests `. Return Value diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index 91126b7312f8..490fc9964d97 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -856,9 +856,16 @@ struct v4l2_requestbuffers { __u32 count; __u32 type; /* enum v4l2_buf_type */ __u32 memory; /* enum v4l2_memory */ - __u32 reserved[2]; + __u32 capabilities; + __u32 reserved[1]; }; +/* capabilities for struct v4l2_requestbuffers and v4l2_create_buffers */ +#define V4L2_BUF_CAP_SUPPORTS_MMAP (1 << 0) +#define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1) +#define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2) +#define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3) + /** * struct v4l2_plane - plane info for multi-planar buffers * @bytesused: number of bytes occupied by data in the plane (payload) @@ -2312,6 +2319,7 @@ struct v4l2_dbg_chip_info { * return: number of created buffers * @memory:enum v4l2_memory; buffer memory type * @format:
[PATCHv2 02/10] v4l2-ctrls: return -EACCES if request wasn't completed
From: Hans Verkuil For now (this might be relaxed in the future) we do not allow getting controls from a request that isn't completed. In that case we return -EACCES. Update the documentation accordingly. Signed-off-by: Hans Verkuil Reviewed-by: Tomasz Figa --- .../media/uapi/v4l/vidioc-g-ext-ctrls.rst | 18 +- drivers/media/v4l2-core/v4l2-ctrls.c | 5 ++--- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst index 9c56a9b6e98a..ad8908ce3095 100644 --- a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst +++ b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst @@ -107,13 +107,12 @@ then ``EINVAL`` will be returned. An attempt to call :ref:`VIDIOC_S_EXT_CTRLS ` for a request that has already been queued will result in an ``EBUSY`` error. -If ``request_fd`` is specified and ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` -during a call to :ref:`VIDIOC_G_EXT_CTRLS `, then the -returned values will be the values currently set for the request (or the -hardware value if none is set) if the request has not yet been queued, or the -values of the controls at the time of request completion if it has already -completed. Attempting to get controls while the request has been queued but -not yet completed will result in an ``EBUSY`` error. +If ``request_fd`` is specified and ``which`` is set to +``V4L2_CTRL_WHICH_REQUEST_VAL`` during a call to +:ref:`VIDIOC_G_EXT_CTRLS `, then it will return the +values of the controls at the time of request completion. +If the request is not yet completed, then this will result in an +``EACCES`` error. The driver will only set/get these controls if all control values are correct. This prevents the situation where only some of the controls @@ -405,8 +404,9 @@ ENOSPC and this error code is returned. EACCES -Attempt to try or set a read-only control or to get a write-only -control. +Attempt to try or set a read-only control, or to get a write-only +control, or to get a control from a request that has not yet been +completed. EPERM The ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index a197b60183f5..ccaf3068de6d 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -3301,10 +3301,9 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev, if (IS_ERR(req)) return PTR_ERR(req); - if (req->state != MEDIA_REQUEST_STATE_IDLE && - req->state != MEDIA_REQUEST_STATE_COMPLETE) { + if (req->state != MEDIA_REQUEST_STATE_COMPLETE) { media_request_put(req); - return -EBUSY; + return -EACCES; } obj = v4l2_ctrls_find_req_obj(hdl, req, false); -- 2.18.0
[PATCHv2 05/10] vb2: set reqbufs/create_bufs capabilities
From: Hans Verkuil Set the capabilities field of v4l2_requestbuffers and v4l2_create_buffers. The various mapping modes were easy, but for signaling the request capability a new 'supports_requests' bitfield was added to videobuf2-core.h (and set in vim2m and vivid). Drivers have to set this bitfield for any queue where requests are supported. Signed-off-by: Hans Verkuil Reviewed-by: Tomasz Figa --- .../media/common/videobuf2/videobuf2-v4l2.c | 19 ++- drivers/media/platform/vim2m.c| 1 + drivers/media/platform/vivid/vivid-core.c | 5 + drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 4 +++- drivers/media/v4l2-core/v4l2-ioctl.c | 4 ++-- include/media/videobuf2-core.h| 2 ++ 6 files changed, 31 insertions(+), 4 deletions(-) diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c index a70df16d68f1..2caaabd50532 100644 --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c @@ -384,7 +384,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md return -EPERM; } return 0; - } else if (q->uses_qbuf) { + } else if (q->uses_qbuf || !q->supports_requests) { dprintk(1, "%s: queue does not use requests\n", opname); return -EPERM; } @@ -619,10 +619,24 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b) } EXPORT_SYMBOL(vb2_querybuf); +static void fill_buf_caps(struct vb2_queue *q, u32 *caps) +{ + *caps = 0; + if (q->io_modes & VB2_MMAP) + *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP; + if (q->io_modes & VB2_USERPTR) + *caps |= V4L2_BUF_CAP_SUPPORTS_USERPTR; + if (q->io_modes & VB2_DMABUF) + *caps |= V4L2_BUF_CAP_SUPPORTS_DMABUF; + if (q->supports_requests) + *caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS; +} + int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) { int ret = vb2_verify_memory_type(q, req->memory, req->type); + fill_buf_caps(q, &req->capabilities); return ret ? ret : vb2_core_reqbufs(q, req->memory, &req->count); } EXPORT_SYMBOL_GPL(vb2_reqbufs); @@ -654,6 +668,7 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create) int ret = vb2_verify_memory_type(q, create->memory, f->type); unsigned i; + fill_buf_caps(q, &create->capabilities); create->index = q->num_buffers; if (create->count == 0) return ret != -EBUSY ? ret : 0; @@ -861,6 +876,7 @@ int vb2_ioctl_reqbufs(struct file *file, void *priv, struct video_device *vdev = video_devdata(file); int res = vb2_verify_memory_type(vdev->queue, p->memory, p->type); + fill_buf_caps(vdev->queue, &p->capabilities); if (res) return res; if (vb2_queue_is_busy(vdev, file)) @@ -882,6 +898,7 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv, p->format.type); p->index = vdev->queue->num_buffers; + fill_buf_caps(vdev->queue, &p->capabilities); /* * If count == 0, then just check if memory and type are valid. * Any -EBUSY result from vb2_verify_memory_type can be mapped to 0. diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c index 5423f0dd0821..40fbb1e429af 100644 --- a/drivers/media/platform/vim2m.c +++ b/drivers/media/platform/vim2m.c @@ -855,6 +855,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *ds src_vq->mem_ops = &vb2_vmalloc_memops; src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY; src_vq->lock = &ctx->dev->dev_mutex; + src_vq->supports_requests = true; ret = vb2_queue_init(src_vq); if (ret) diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c index 3f6f5cbe1b60..e7f1394832fe 100644 --- a/drivers/media/platform/vivid/vivid-core.c +++ b/drivers/media/platform/vivid/vivid-core.c @@ -1077,6 +1077,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst) q->min_buffers_needed = 2; q->lock = &dev->mutex; q->dev = dev->v4l2_dev.dev; + q->supports_requests = true; ret = vb2_queue_init(q); if (ret) @@ -1097,6 +1098,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst) q->min_buffers_needed = 2; q->lock = &dev->mutex; q->dev = dev->v4l2_dev.dev; + q->supports_requests = true; ret = vb2_queue_init(q); if (ret) @@ -1117,6 +1119,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst) q->mi
[PATCHv2 07/10] v4l2-ctrls: use media_request_(un)lock_for_access
From: Hans Verkuil When getting control values from a completed request, we have to protect the request against being re-inited why it is being accessed by calling media_request_(un)lock_for_access. Signed-off-by: Hans Verkuil --- drivers/media/v4l2-core/v4l2-ctrls.c | 21 +++-- 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index ccaf3068de6d..cc266a4a6e88 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -3289,11 +3289,10 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev, struct v4l2_ext_controls *cs) { struct media_request_object *obj = NULL; + struct media_request *req = NULL; int ret; if (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL) { - struct media_request *req; - if (!mdev || cs->request_fd < 0) return -EINVAL; @@ -3306,11 +3305,18 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev, return -EACCES; } + ret = media_request_lock_for_access(req); + if (ret) { + media_request_put(req); + return ret; + } + obj = v4l2_ctrls_find_req_obj(hdl, req, false); - /* Reference to the request held through obj */ - media_request_put(req); - if (IS_ERR(obj)) + if (IS_ERR(obj)) { + media_request_unlock_for_access(req); + media_request_put(req); return PTR_ERR(obj); + } hdl = container_of(obj, struct v4l2_ctrl_handler, req_obj); @@ -3318,8 +3324,11 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev, ret = v4l2_g_ext_ctrls_common(hdl, cs); - if (obj) + if (obj) { + media_request_unlock_for_access(req); media_request_object_put(obj); + media_request_put(req); + } return ret; } EXPORT_SYMBOL(v4l2_g_ext_ctrls); -- 2.18.0
[PATCHv2 01/10] media-request: return -EINVAL for invalid request_fds
From: Hans Verkuil Instead of returning -ENOENT when a request_fd was not found (VIDIOC_QBUF and VIDIOC_G/S/TRY_EXT_CTRLS), we now return -EINVAL. This is in line with what we do when invalid dmabuf fds are passed to e.g. VIDIOC_QBUF. Also document that EINVAL is returned for invalid m.fd values, we never documented that. Signed-off-by: Hans Verkuil Reviewed-by: Tomasz Figa --- Documentation/media/uapi/v4l/buffer.rst| 4 ++-- .../media/uapi/v4l/vidioc-g-ext-ctrls.rst | 18 -- Documentation/media/uapi/v4l/vidioc-qbuf.rst | 12 +--- drivers/media/media-request.c | 6 -- 4 files changed, 19 insertions(+), 21 deletions(-) diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst index dd0065a95ea0..35c2fadd10de 100644 --- a/Documentation/media/uapi/v4l/buffer.rst +++ b/Documentation/media/uapi/v4l/buffer.rst @@ -313,8 +313,8 @@ struct v4l2_buffer queued to that request. This is set by the user when calling :ref:`ioctl VIDIOC_QBUF ` and ignored by other ioctls. If the device does not support requests, then ``EPERM`` will be returned. - If requests are supported but an invalid request FD is given, then - ``ENOENT`` will be returned. + If requests are supported but an invalid request file descriptor is + given, then ``EINVAL`` will be returned. diff --git a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst index 771fd1161277..9c56a9b6e98a 100644 --- a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst +++ b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst @@ -101,8 +101,8 @@ then the controls are not applied immediately when calling :ref:`VIDIOC_S_EXT_CTRLS `, but instead are applied by the driver for the buffer associated with the same request. If the device does not support requests, then ``EPERM`` will be returned. -If requests are supported but an invalid request FD is given, then -``ENOENT`` will be returned. +If requests are supported but an invalid request file descriptor is given, +then ``EINVAL`` will be returned. An attempt to call :ref:`VIDIOC_S_EXT_CTRLS ` for a request that has already been queued will result in an ``EBUSY`` error. @@ -301,8 +301,8 @@ still cause this situation. - File descriptor of the request to be used by this operation. Only valid if ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``. If the device does not support requests, then ``EPERM`` will be returned. - If requests are supported but an invalid request FD is given, then - ``ENOENT`` will be returned. + If requests are supported but an invalid request file descriptor is + given, then ``EINVAL`` will be returned. * - __u32 - ``reserved``\ [1] - Reserved for future extensions. @@ -378,11 +378,13 @@ appropriately. The generic error codes are described at the EINVAL The struct :c:type:`v4l2_ext_control` ``id`` is -invalid, the struct :c:type:`v4l2_ext_controls` +invalid, or the struct :c:type:`v4l2_ext_controls` ``which`` is invalid, or the struct :c:type:`v4l2_ext_control` ``value`` was inappropriate (e.g. the given menu index is not supported by the -driver). This error code is also returned by the +driver), or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` +but the given ``request_fd`` was invalid. +This error code is also returned by the :ref:`VIDIOC_S_EXT_CTRLS ` and :ref:`VIDIOC_TRY_EXT_CTRLS ` ioctls if two or more control values are in conflict. @@ -409,7 +411,3 @@ EACCES EPERM The ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the device does not support requests. - -ENOENT -The ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the -the given ``request_fd`` was invalid. diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst index 0e415f2551b2..7bff69c15452 100644 --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst @@ -105,8 +105,8 @@ until the request itself is queued. Also, the driver will apply any settings associated with the request for this buffer. This field will be ignored unless the ``V4L2_BUF_FLAG_REQUEST_FD`` flag is set. If the device does not support requests, then ``EPERM`` will be returned. -If requests are supported but an invalid request FD is given, then -``ENOENT`` will be returned. +If requests are supported but an invalid request file descriptor is given, +then ``EINVAL`` will be returned. .. caution:: It is not allowed to mix queuing requests with queuing buffers directly. @@ -152,7 +152,9 @@ EAGAIN EINVAL The buffer ``type`` is not supported, or the ``index`` is out of bounds, or no buffers have been allocated yet, or the ``userptr`` or -``length`` are invali
[PATCHv2 03/10] buffer.rst: only set V4L2_BUF_FLAG_REQUEST_FD for QBUF
From: Hans Verkuil Document that V4L2_BUF_FLAG_REQUEST_FD should only be used with VIDIOC_QBUF and cleared otherwise. Signed-off-by: Hans Verkuil Reviewed-by: Tomasz Figa --- Documentation/media/uapi/v4l/buffer.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst index 35c2fadd10de..1865cd5b9d3c 100644 --- a/Documentation/media/uapi/v4l/buffer.rst +++ b/Documentation/media/uapi/v4l/buffer.rst @@ -312,6 +312,8 @@ struct v4l2_buffer and flag ``V4L2_BUF_FLAG_REQUEST_FD`` is set, then the buffer will be queued to that request. This is set by the user when calling :ref:`ioctl VIDIOC_QBUF ` and ignored by other ioctls. + Applications should not set ``V4L2_BUF_FLAG_REQUEST_FD`` for any ioctls + other than :ref:`VIDIOC_QBUF `. If the device does not support requests, then ``EPERM`` will be returned. If requests are supported but an invalid request file descriptor is given, then ``EINVAL`` will be returned. -- 2.18.0
[PATCHv2 00/10] Post-v18: Request API updates
From: Hans Verkuil Hi all, This patch series sits on top of my v18 series for the Request API. It makes some final (?) changes as discussed in: https://www.mail-archive.com/linux-media@vger.kernel.org/msg134419.html and: https://www.spinics.net/lists/linux-media/msg138596.html The combined v18 patches + this series is available here: https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=reqv18-1 Updated v4l-utils for this is available here: https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=request Userspace visible changes: - Invalid request_fd values now return -EINVAL instead of -ENOENT. - It is no longer possible to use VIDIOC_G_EXT_CTRLS for requests that are not completed. -EACCES is returned in that case. - Attempting to use requests if requests are not supported by the driver will result in -EACCES instead of -EPERM. Driver visible changes (important for the cedrus driver!): Drivers should set the new vb2_queue 'supports_request' bitfield to 1 if a vb2_queue can support requests. Otherwise the queue cannot be used with requests. This bitfield is also used to fill in the new capabilities field in struct v4l2_requestbuffers and v4l2_create_buffers. Changes since v1: - Updated patch 4/10 to explain how to query the capabilities with REQBUFS/CREATE_BUFS with a minimum of side-effects (requested by Tomasz). - Added patches 6-10: 6: Sakari found a corner case: when accessing a request the request has to be protected from being re-inited. New media_request_(un)lock_for_access helpers are added for this. 7: use these helpers in g_ext_ctrls. 8: make s/try_ext_ctrls more robust by keeping the request references until we're fully done setting/trying the controls. 9: Change two more EPERM's to EACCES. EPERM suggests that you can fix it by changing permissions somehow, but in this case the driver simply doesn't support requests at all. 10: Update the request documentation based on Laurent's comments: https://www.spinics.net/lists/linux-media/msg139152.html To do: split off the V4L2 specifics into a V4L2 specific rst file. But this will take more time and is for later. Regards, Hans Hans Verkuil (10): media-request: return -EINVAL for invalid request_fds v4l2-ctrls: return -EACCES if request wasn't completed buffer.rst: only set V4L2_BUF_FLAG_REQUEST_FD for QBUF videodev2.h: add new capabilities for buffer types vb2: set reqbufs/create_bufs capabilities media-request: add media_request_(un)lock_for_access v4l2-ctrls: use media_request_(un)lock_for_access v4l2-ctrls: improve media_request_(un)lock_for_update media-request: EPERM -> EACCES media-request: update documentation .../uapi/mediactl/media-ioc-request-alloc.rst | 3 +- .../uapi/mediactl/media-request-ioc-queue.rst | 7 +-- .../media/uapi/mediactl/request-api.rst | 51 +++ .../uapi/mediactl/request-func-close.rst | 1 + .../media/uapi/mediactl/request-func-poll.rst | 2 +- Documentation/media/uapi/v4l/buffer.rst | 22 +--- .../media/uapi/v4l/vidioc-create-bufs.rst | 14 - .../media/uapi/v4l/vidioc-g-ext-ctrls.rst | 48 + Documentation/media/uapi/v4l/vidioc-qbuf.rst | 32 +++- .../media/uapi/v4l/vidioc-reqbufs.rst | 42 ++- .../media/common/videobuf2/videobuf2-v4l2.c | 19 ++- drivers/media/media-request.c | 20 ++-- drivers/media/platform/vim2m.c| 1 + drivers/media/platform/vivid/vivid-core.c | 5 ++ drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 4 +- drivers/media/v4l2-core/v4l2-ctrls.c | 32 +++- drivers/media/v4l2-core/v4l2-ioctl.c | 4 +- include/media/media-request.h | 46 + include/media/videobuf2-core.h| 2 + include/uapi/linux/videodev2.h| 13 - 20 files changed, 269 insertions(+), 99 deletions(-) -- 2.18.0
[PATCHv2 06/10] media-request: add media_request_(un)lock_for_access
From: Hans Verkuil Add helper functions to prevent a completed request from being re-inited while it is being accessed. Signed-off-by: Hans Verkuil --- drivers/media/media-request.c | 10 include/media/media-request.h | 46 +++ 2 files changed, 56 insertions(+) diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c index 4cee67e6657e..414197645e09 100644 --- a/drivers/media/media-request.c +++ b/drivers/media/media-request.c @@ -43,6 +43,7 @@ static void media_request_clean(struct media_request *req) /* Just a sanity check. No other code path is allowed to change this. */ WARN_ON(req->state != MEDIA_REQUEST_STATE_CLEANING); WARN_ON(req->updating_count); + WARN_ON(req->access_count); list_for_each_entry_safe(obj, obj_safe, &req->objects, list) { media_request_object_unbind(obj); @@ -50,6 +51,7 @@ static void media_request_clean(struct media_request *req) } req->updating_count = 0; + req->access_count = 0; WARN_ON(req->num_incomplete_objects); req->num_incomplete_objects = 0; wake_up_interruptible_all(&req->poll_wait); @@ -198,6 +200,13 @@ static long media_request_ioctl_reinit(struct media_request *req) spin_unlock_irqrestore(&req->lock, flags); return -EBUSY; } + if (req->access_count) { + dev_dbg(mdev->dev, + "request: %s is being accessed, cannot reinit\n", + req->debug_str); + spin_unlock_irqrestore(&req->lock, flags); + return -EBUSY; + } req->state = MEDIA_REQUEST_STATE_CLEANING; spin_unlock_irqrestore(&req->lock, flags); @@ -313,6 +322,7 @@ int media_request_alloc(struct media_device *mdev, int *alloc_fd) spin_lock_init(&req->lock); init_waitqueue_head(&req->poll_wait); req->updating_count = 0; + req->access_count = 0; *alloc_fd = fd; diff --git a/include/media/media-request.h b/include/media/media-request.h index ac02019c1d77..707c7577f46d 100644 --- a/include/media/media-request.h +++ b/include/media/media-request.h @@ -53,6 +53,7 @@ struct media_request_object; * @debug_str: Prefix for debug messages (process name:fd) * @state: The state of the request * @updating_count: count the number of request updates that are in progress + * @access_count: count the number of request accesses that are in progress * @objects: List of @struct media_request_object request objects * @num_incomplete_objects: The number of incomplete objects in the request * @poll_wait: Wait queue for poll @@ -64,6 +65,7 @@ struct media_request { char debug_str[TASK_COMM_LEN + 11]; enum media_request_state state; unsigned int updating_count; + unsigned int access_count; struct list_head objects; unsigned int num_incomplete_objects; struct wait_queue_head poll_wait; @@ -72,6 +74,50 @@ struct media_request { #ifdef CONFIG_MEDIA_CONTROLLER +/** + * media_request_lock_for_access - Lock the request to access its objects + * + * @req: The media request + * + * Use before accessing a completed request. A reference to the request must + * be held during the access. This usually takes place automatically through + * a file handle. Use @media_request_unlock_for_access when done. + */ +static inline int __must_check +media_request_lock_for_access(struct media_request *req) +{ + unsigned long flags; + int ret = -EBUSY; + + spin_lock_irqsave(&req->lock, flags); + if (req->state == MEDIA_REQUEST_STATE_COMPLETE) { + req->access_count++; + ret = 0; + } + spin_unlock_irqrestore(&req->lock, flags); + + return ret; +} + +/** + * media_request_unlock_for_access - Unlock a request previously locked for + * access + * + * @req: The media request + * + * Unlock a request that has previously been locked using + * @media_request_lock_for_access. + */ +static inline void media_request_unlock_for_access(struct media_request *req) +{ + unsigned long flags; + + spin_lock_irqsave(&req->lock, flags); + if (!WARN_ON(!req->access_count)) + req->access_count--; + spin_unlock_irqrestore(&req->lock, flags); +} + /** * media_request_lock_for_update - Lock the request for updating its objects * -- 2.18.0
[PATCHv2 10/10] media-request: update documentation
From: Hans Verkuil Various clarifications and readability improvements based on Laurent Pinchart's review of the documentation. Signed-off-by: Hans Verkuil --- .../uapi/mediactl/media-ioc-request-alloc.rst | 3 +- .../uapi/mediactl/media-request-ioc-queue.rst | 7 +-- .../media/uapi/mediactl/request-api.rst | 51 +++ .../uapi/mediactl/request-func-close.rst | 1 + .../media/uapi/mediactl/request-func-poll.rst | 2 +- Documentation/media/uapi/v4l/buffer.rst | 14 +++-- .../media/uapi/v4l/vidioc-g-ext-ctrls.rst | 5 +- Documentation/media/uapi/v4l/vidioc-qbuf.rst | 5 +- 8 files changed, 52 insertions(+), 36 deletions(-) diff --git a/Documentation/media/uapi/mediactl/media-ioc-request-alloc.rst b/Documentation/media/uapi/mediactl/media-ioc-request-alloc.rst index 34434e2b3918..0f8b31874002 100644 --- a/Documentation/media/uapi/mediactl/media-ioc-request-alloc.rst +++ b/Documentation/media/uapi/mediactl/media-ioc-request-alloc.rst @@ -52,7 +52,8 @@ for the request to complete. The request will remain allocated until all the file descriptors associated with it are closed by :ref:`close() ` and the driver no -longer uses the request internally. +longer uses the request internally. See also +:ref:`here ` for more information. Return Value diff --git a/Documentation/media/uapi/mediactl/media-request-ioc-queue.rst b/Documentation/media/uapi/mediactl/media-request-ioc-queue.rst index d4f8119e0643..3bf1c2e492eb 100644 --- a/Documentation/media/uapi/mediactl/media-request-ioc-queue.rst +++ b/Documentation/media/uapi/mediactl/media-request-ioc-queue.rst @@ -40,9 +40,6 @@ Other errors can be returned if the contents of the request contained invalid or inconsistent data, see the next section for a list of common error codes. On error both the request and driver state are unchanged. -Typically if you get an error here, then that means that the application -did something wrong and you have to fix the application. - Once a request is queued, then the driver is required to gracefully handle errors that occur when the request is applied to the hardware. The exception is the ``EIO`` error which signals a fatal error that requires @@ -69,8 +66,8 @@ EPERM to use a request. It is not permitted to mix the two APIs. ENOENT The request did not contain any buffers. All requests are required -to have at least one buffer. This can also be returned if required -controls are missing. +to have at least one buffer. This can also be returned if some required +configuration is missing in the request. ENOMEM Out of memory when allocating internal data structures for this request. diff --git a/Documentation/media/uapi/mediactl/request-api.rst b/Documentation/media/uapi/mediactl/request-api.rst index 0b9da58b01e3..1ac42749e564 100644 --- a/Documentation/media/uapi/mediactl/request-api.rst +++ b/Documentation/media/uapi/mediactl/request-api.rst @@ -12,6 +12,9 @@ the same pipeline to reconfigure and collaborate closely on a per-frame basis. Another is support of stateless codecs, which require controls to be applied to specific frames (aka 'per-frame controls') in order to be used efficiently. +While the initial use-case was V4L2, it can be extended to other subsystems +as well, as long as they use the media controller. + Supporting these features without the Request API is not always possible and if it is, it is terribly inefficient: user-space would have to flush all activity on the media pipeline, reconfigure it for the next frame, queue the buffers to @@ -20,19 +23,23 @@ dequeuing before considering the next frame. This defeats the purpose of having buffer queues since in practice only one buffer would be queued at a time. The Request API allows a specific configuration of the pipeline (media -controller topology + controls for each media entity) to be associated with -specific buffers. The parameters are applied by each participating device as -buffers associated to a request flow in. This allows user-space to schedule -several tasks ("requests") with different parameters in advance, knowing that -the parameters will be applied when needed to get the expected result. Control -values at the time of request completion are also available for reading. +controller topology + configuration for each media entity) to be associated with +specific buffers. This allows user-space to schedule several tasks ("requests") +with different configurations in advance, knowing that the configuration will be +applied when needed to get the expected result. Configuration values at the time +of request completion are also available for reading. Usage = -The Request API is used on top of standard media controller and V4L2 calls, -which are augmented with an extra ``request_fd`` parameter. Requests themselves -are allocated from the supporting media controller node. +The Request API extends the Media Controller API and co
[PATCHv2 09/10] media-request: EPERM -> EACCES
From: Hans Verkuil If requests are not supported by the driver, then return EACCES, not EPERM. This is consistent with the error that an invalid request_fd will give, and if requests are not supported, then all request_fd values are invalid. Signed-off-by: Hans Verkuil --- Documentation/media/uapi/v4l/buffer.rst | 2 +- .../media/uapi/v4l/vidioc-g-ext-ctrls.rst | 9 - Documentation/media/uapi/v4l/vidioc-qbuf.rst | 15 +-- drivers/media/media-request.c | 4 ++-- 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst index 1865cd5b9d3c..58a6d7d336e6 100644 --- a/Documentation/media/uapi/v4l/buffer.rst +++ b/Documentation/media/uapi/v4l/buffer.rst @@ -314,7 +314,7 @@ struct v4l2_buffer :ref:`ioctl VIDIOC_QBUF ` and ignored by other ioctls. Applications should not set ``V4L2_BUF_FLAG_REQUEST_FD`` for any ioctls other than :ref:`VIDIOC_QBUF `. - If the device does not support requests, then ``EPERM`` will be returned. + If the device does not support requests, then ``EACCES`` will be returned. If requests are supported but an invalid request file descriptor is given, then ``EINVAL`` will be returned. diff --git a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst index ad8908ce3095..54a999df5aec 100644 --- a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst +++ b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst @@ -100,7 +100,7 @@ file descriptor and ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``, then the controls are not applied immediately when calling :ref:`VIDIOC_S_EXT_CTRLS `, but instead are applied by the driver for the buffer associated with the same request. -If the device does not support requests, then ``EPERM`` will be returned. +If the device does not support requests, then ``EACCES`` will be returned. If requests are supported but an invalid request file descriptor is given, then ``EINVAL`` will be returned. @@ -233,7 +233,7 @@ still cause this situation. these controls have to be retrieved from a request or tried/set for a request. In the latter case the ``request_fd`` field contains the file descriptor of the request that should be used. If the device - does not support requests, then ``EPERM`` will be returned. + does not support requests, then ``EACCES`` will be returned. .. note:: @@ -299,7 +299,7 @@ still cause this situation. - ``request_fd`` - File descriptor of the request to be used by this operation. Only valid if ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``. - If the device does not support requests, then ``EPERM`` will be returned. + If the device does not support requests, then ``EACCES`` will be returned. If requests are supported but an invalid request file descriptor is given, then ``EINVAL`` will be returned. * - __u32 @@ -408,6 +408,5 @@ EACCES control, or to get a control from a request that has not yet been completed. -EPERM -The ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the +Or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the device does not support requests. diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst index 7bff69c15452..a2f4ac0b0ba1 100644 --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst @@ -104,7 +104,7 @@ in use. Setting it means that the buffer will not be passed to the driver until the request itself is queued. Also, the driver will apply any settings associated with the request for this buffer. This field will be ignored unless the ``V4L2_BUF_FLAG_REQUEST_FD`` flag is set. -If the device does not support requests, then ``EPERM`` will be returned. +If the device does not support requests, then ``EACCES`` will be returned. If requests are supported but an invalid request file descriptor is given, then ``EINVAL`` will be returned. @@ -175,9 +175,12 @@ EPIPE codecs if a buffer with the ``V4L2_BUF_FLAG_LAST`` was already dequeued and no new buffers are expected to become available. -EPERM +EACCES The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not -support requests. Or the first buffer was queued via a request, but -the application now tries to queue it directly, or vice versa (it is -not permitted to mix the two APIs). Or an attempt is made to queue a -CAPTURE buffer to a request for a :ref:`memory-to-memory device `. +support requests. + +EPERM +The first buffer was queued via a request, but the application now tries +to queue it directly, or vice versa (it is not permitted to mix the two +APIs). Or an attempt is made t
[PATCH] media: rc: ir-rc6-decoder: enable toggle bit for Kathrein RCU-676 remote
The Kathrein RCU-676 remote uses the 32-bit rc6 protocol and toggles bit 15 (0x8000) on repeated button presses, like MCE remotes. Add it's customer code 0x8046 to the 32-bit rc6 toggle handling code to get proper scancodes and toggle reports. Signed-off-by: Matthias Reichl --- Here's the link to the bugreport and discussion: https://forum.libreelec.tv/thread/13086-get-kathrein-rcu-676-remote-to-work-with-le9/ drivers/media/rc/ir-rc6-decoder.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/media/rc/ir-rc6-decoder.c b/drivers/media/rc/ir-rc6-decoder.c index 68487ce9f79b..d96aed1343e4 100644 --- a/drivers/media/rc/ir-rc6-decoder.c +++ b/drivers/media/rc/ir-rc6-decoder.c @@ -40,6 +40,7 @@ #define RC6_6A_MCE_TOGGLE_MASK 0x8000 /* for the body bits */ #define RC6_6A_LCC_MASK0x /* RC6-6A-32 long customer code mask */ #define RC6_6A_MCE_CC 0x800f /* MCE customer code */ +#define RC6_6A_KATHREIN_CC 0x8046 /* Kathrein RCU-676 customer code */ #ifndef CHAR_BIT #define CHAR_BIT 8 /* Normally in */ #endif @@ -242,13 +243,17 @@ static int ir_rc6_decode(struct rc_dev *dev, struct ir_raw_event ev) toggle = 0; break; case 32: - if ((scancode & RC6_6A_LCC_MASK) == RC6_6A_MCE_CC) { + switch (scancode & RC6_6A_LCC_MASK) { + case RC6_6A_MCE_CC: + case RC6_6A_KATHREIN_CC: protocol = RC_PROTO_RC6_MCE; toggle = !!(scancode & RC6_6A_MCE_TOGGLE_MASK); scancode &= ~RC6_6A_MCE_TOGGLE_MASK; - } else { + break; + default: protocol = RC_PROTO_RC6_6A_32; toggle = 0; + break; } break; default: -- 2.18.0
Re: [PATCH v3 0/2] media: i2c: ov5640: Re-work MIPI startup sequence
On 15 August 2018 at 12:28, Jacopo Mondi wrote: > Hello ov5640 people, >this driver has received a lot of attention recently, and this series aims > to fix the CSI-2 interface startup on i.Mx6Q platforms. > > Please refer to the v2 cover letters for more background informations: > https://www.mail-archive.com/linux-media@vger.kernel.org/msg133420.html > > This two patches alone allows the MIPI interface to startup properly, but in > order to capture good images (good as in 'not completely black') exposure and > gain handling should be fixed too. > Hugues Fruchet has a series in review that fixes that issues: > [PATCH v2 0/5] Fix OV5640 exposure & gain > > And I have re-based it on top of this two fixes here: > git://jmondi.org/linux ov5640/timings_exposure > > Steve Longerbeam tested that branch on his I.MX6q SabreSD board and confirms > he > can now capture frames (I added his Tested-by tag to this patches). I have > verified the same on Engicam iCore I.MX6q and an Intel Atom based board. > > Ideally I would like to have these two fixes merged, and Hugues' ones then > applied on top. Of course, more testing on other platforms using CSI-2 is very > welcome. > > Thanks >j > > v2 -> v3: > - patch [2/2] was originally sent in a different series, compared to v2 it > removes entries from the blob array instead of adding more. > > Jacopo Mondi (2): > media: ov5640: Re-work MIPI startup sequence > media: ov5640: Fix timings setup code > > drivers/media/i2c/ov5640.c | 141 > + > 1 file changed, 92 insertions(+), 49 deletions(-) > > -- > 2.7.4 > Thanks for this work. I've just tested this with a dragonboard-410c (MICPI/CSI) + OV5640 sensor. It works on my side for 1280*720, 1920*1080 and 2592*1944 formats. Tested-by: Loic Poulain
Waiting for
We provide photoshop services to some of the companies from around the world. We have worked on tons of images ever since our team establishment in 2009. Many online retail companies use our services for retouching electronics, jewelry, apparels, furniture etc. by getting the images of their products enhanced. Here are the details of what we provide: Clipping path; Deep etch process Image masking Remove background Portrait retouching Jewelry retouching Fashion retouching Please reply back for further info. We can provide testing for your photos if needed. Thanks, Ruby
Re: [PATCHv2 06/10] media-request: add media_request_(un)lock_for_access
On 28/08/18 15:49, Hans Verkuil wrote: > From: Hans Verkuil > > Add helper functions to prevent a completed request from being > re-inited while it is being accessed. > > Signed-off-by: Hans Verkuil > --- > drivers/media/media-request.c | 10 > include/media/media-request.h | 46 +++ > 2 files changed, 56 insertions(+) > > diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c > index 4cee67e6657e..414197645e09 100644 > --- a/drivers/media/media-request.c > +++ b/drivers/media/media-request.c > @@ -43,6 +43,7 @@ static void media_request_clean(struct media_request *req) > /* Just a sanity check. No other code path is allowed to change this. */ > WARN_ON(req->state != MEDIA_REQUEST_STATE_CLEANING); > WARN_ON(req->updating_count); > + WARN_ON(req->access_count); > > list_for_each_entry_safe(obj, obj_safe, &req->objects, list) { > media_request_object_unbind(obj); > @@ -50,6 +51,7 @@ static void media_request_clean(struct media_request *req) > } > > req->updating_count = 0; > + req->access_count = 0; > WARN_ON(req->num_incomplete_objects); > req->num_incomplete_objects = 0; > wake_up_interruptible_all(&req->poll_wait); > @@ -198,6 +200,13 @@ static long media_request_ioctl_reinit(struct > media_request *req) > spin_unlock_irqrestore(&req->lock, flags); > return -EBUSY; > } > + if (req->access_count) { > + dev_dbg(mdev->dev, > + "request: %s is being accessed, cannot reinit\n", > + req->debug_str); > + spin_unlock_irqrestore(&req->lock, flags); > + return -EBUSY; > + } > req->state = MEDIA_REQUEST_STATE_CLEANING; > spin_unlock_irqrestore(&req->lock, flags); > > @@ -313,6 +322,7 @@ int media_request_alloc(struct media_device *mdev, int > *alloc_fd) > spin_lock_init(&req->lock); > init_waitqueue_head(&req->poll_wait); > req->updating_count = 0; > + req->access_count = 0; > > *alloc_fd = fd; > > diff --git a/include/media/media-request.h b/include/media/media-request.h > index ac02019c1d77..707c7577f46d 100644 > --- a/include/media/media-request.h > +++ b/include/media/media-request.h > @@ -53,6 +53,7 @@ struct media_request_object; > * @debug_str: Prefix for debug messages (process name:fd) > * @state: The state of the request > * @updating_count: count the number of request updates that are in progress > + * @access_count: count the number of request accesses that are in progress > * @objects: List of @struct media_request_object request objects > * @num_incomplete_objects: The number of incomplete objects in the request > * @poll_wait: Wait queue for poll > @@ -64,6 +65,7 @@ struct media_request { > char debug_str[TASK_COMM_LEN + 11]; > enum media_request_state state; > unsigned int updating_count; > + unsigned int access_count; > struct list_head objects; > unsigned int num_incomplete_objects; > struct wait_queue_head poll_wait; > @@ -72,6 +74,50 @@ struct media_request { > > #ifdef CONFIG_MEDIA_CONTROLLER > > +/** > + * media_request_lock_for_access - Lock the request to access its objects > + * > + * @req: The media request > + * > + * Use before accessing a completed request. A reference to the request must > + * be held during the access. This usually takes place automatically through > + * a file handle. Use @media_request_unlock_for_access when done. > + */ > +static inline int __must_check > +media_request_lock_for_access(struct media_request *req) > +{ > + unsigned long flags; > + int ret = -EBUSY; > + > + spin_lock_irqsave(&req->lock, flags); > + if (req->state == MEDIA_REQUEST_STATE_COMPLETE) { > + req->access_count++; > + ret = 0; > + } > + spin_unlock_irqrestore(&req->lock, flags); > + > + return ret; > +} > + > +/** > + * media_request_unlock_for_access - Unlock a request previously locked for > + *access > + * > + * @req: The media request > + * > + * Unlock a request that has previously been locked using > + * @media_request_lock_for_access. > + */ > +static inline void media_request_unlock_for_access(struct media_request *req) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&req->lock, flags); > + if (!WARN_ON(!req->access_count)) > + req->access_count--; > + spin_unlock_irqrestore(&req->lock, flags); > +} > + > /** > * media_request_lock_for_update - Lock the request for updating its objects > * > I also need to add *_for_access() stub functions that are used when the MEDIA_CONTROLLER is not set in the kernel config. I've fixed this in my tree. Regards, Hans
Your reply
We provide photoshop services to some of the companies from around the world. We have worked on tons of images ever since our team establishment in 2009. Many online retail companies use our services for retouching electronics, jewelry, apparels, furniture etc. by getting the images of their products enhanced. Here are the details of what we provide: Clipping path; Deep etch process Image masking Remove background Portrait retouching Jewelry retouching Fashion retouching Please reply back for further info. We can provide testing for your photos if needed. Thanks, Ruby
Re: [PATCH v2 04/23] dt-bindings: media: Specify bus type for MIPI D-PHY, others, explicitly
On Mon, 27 Aug 2018 12:29:41 +0300, Sakari Ailus wrote: > Allow specifying the bus type explicitly for MIPI D-PHY, parallel and > Bt.656 busses. This is useful for devices that can make use of different > bus types. There are CSI-2 transmitters and receivers but the PHY > selection needs to be made between C-PHY and D-PHY; many devices also > support parallel and Bt.656 interfaces but the means to pass that > information to software wasn't there. > > Autodetection (value 0) is removed as an option as the property could be > simply omitted in that case. > > Signed-off-by: Sakari Ailus > --- > Documentation/devicetree/bindings/media/video-interfaces.txt | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Reviewed-by: Rob Herring
Re: [PATCH v2 00/23] V4L2 fwnode rework; support for default configuration
Hi Sakari, On 08/27/2018 02:29 AM, Sakari Ailus wrote: Hello everyone, I've long thought the V4L2 fwnode framework requires some work (it's buggy and it does not adequately serve common needs). This set should address in particular these matters: - Most devices support a particular media bus type but the V4L2 fwnode framework was not able to use such information, but instead tried to guess the bus type with varying levels of success while drivers generally ignored the results. This patchset makes that possible --- setting a bus type enables parsing configuration for only that bus. Failing that check results in returning -ENXIO to be returned. - Support specifying default configuration. If the endpoint has no configuration, the defaults set by the driver (as documented in DT bindings) will prevail. Any available configuration will still be read from the endpoint as one could expect. A common use case for this is e.g. the number of CSI-2 lanes. Few devices support lane mapping, and default 1:1 mapping is provided in absence of a valid default or configuration read OF. - Debugging information is greatly improved. - Recognition of the differences between CSI-2 D-PHY and C-PHY. All currently supported hardware (or at least drivers) is D-PHY only, so this change is still easy. The smiapp driver is converted to use the new functionality. This patchset does not address remaining issues such as supporting setting defaults for e.g. bridge drivers with multiple ports, but with Steve Longerbeam's patchset we're much closer to that goal. I've rebased this set on top of Steve's. Albeit the two deal with the same files, there were only a few trivial conflicts. Note that I've only tested parsing endpoints for the CSI-2 bus (no parallel IF hardware). Jacopo has tested an earlier version of the set with a few changes to the parallel bus handling compared to this one. Comments are welcome. I got around to testing this. The following diff needs to be added to initialize bus_type before calling v4l2_fwnode_endpoint_parse() in imx-media driver, this should probably be squashed with "v4l: fwnode: Initialise the V4L2 fwnode endpoints to zero": diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c index 539159d..ac9d718 100644 --- a/drivers/staging/media/imx/imx-media-csi.c +++ b/drivers/staging/media/imx/imx-media-csi.c @@ -1050,7 +1050,7 @@ static int csi_link_validate(struct v4l2_subdev *sd, struct v4l2_subdev_format *sink_fmt) { struct csi_priv *priv = v4l2_get_subdevdata(sd); - struct v4l2_fwnode_endpoint upstream_ep = {}; + struct v4l2_fwnode_endpoint upstream_ep = { .bus_type = 0 }; bool is_csi2; int ret; @@ -1164,7 +1164,7 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd, struct v4l2_subdev_mbus_code_enum *code) { struct csi_priv *priv = v4l2_get_subdevdata(sd); - struct v4l2_fwnode_endpoint upstream_ep; + struct v4l2_fwnode_endpoint upstream_ep = { .bus_type = 0 }; const struct imx_media_pixfmt *incc; struct v4l2_mbus_framefmt *infmt; int ret = 0; @@ -1403,7 +1403,7 @@ static int csi_set_fmt(struct v4l2_subdev *sd, { struct csi_priv *priv = v4l2_get_subdevdata(sd); struct imx_media_video_dev *vdev = priv->vdev; - struct v4l2_fwnode_endpoint upstream_ep; + struct v4l2_fwnode_endpoint upstream_ep = { .bus_type = 0 }; const struct imx_media_pixfmt *cc; struct v4l2_pix_format vdev_fmt; struct v4l2_mbus_framefmt *fmt; @@ -1542,7 +1542,7 @@ static int csi_set_selection(struct v4l2_subdev *sd, struct v4l2_subdev_selection *sel) { struct csi_priv *priv = v4l2_get_subdevdata(sd); - struct v4l2_fwnode_endpoint upstream_ep; + struct v4l2_fwnode_endpoint upstream_ep = { .bus_type = 0 }; struct v4l2_mbus_framefmt *infmt; struct v4l2_rect *crop, *compose; int pad, ret; After making that change, capture from CSI-2 OV5640 and parallel OV5642 on the imx6q Sabrelite is working fine. Feel free to add my Tested-by on that platform. I've pushed the patches (including Steve's) here: https://git.linuxtv.org/sailus/media_tree.git/log/?h=v4l2-fwnode-next> since v1: - Rebase it all on current media tree master --- there was a conflict in drivers/media/platform/qcom/camss/camss.c in Steve's patch "media: platform: Switch to v4l2_async_notifier_add_subdev"; I hope the resolution was fine. I checked your resolution to camss.c and it was the same resolution I made as well. Thanks, Steve - Default to Bt.656 bus in guessing the bus type if no properties suggesting otherwise are set. In v1 and error was returned, which would have been troublesome for the existing drivers. - Set the bus_type field to zero (i.e. guess) for existing callers of v4l2_fwnode
Re: [PATCH v3 6/7] media: Add controls for JPEG quantization tables
On Mon, 2018-08-27 at 09:47 +0200, Paul Kocialkowski wrote: > Hi, > > On Wed, 2018-08-22 at 13:59 -0300, Ezequiel Garcia wrote: > > From: Shunqian Zheng > > > > Add V4L2_CID_JPEG_LUMA/CHROMA_QUANTIZATION controls to allow userspace > > configure the JPEG quantization tables. > > How about having a single control for quantization? > > In MPEG-2/H.264/H.265, we have a single control exposed as a structure, > which contains the tables for both luma and chroma. In the case of JPEG, > it's not that big a deal, but for advanced video formats, it would be > too much hassle to have one control per table. > > In order to keep the interface consistent, I think it'd be best to merge > both matrices into a single control. > > What do you think? > I think it makes a lot of sense. I don't see the benefit in having luma and chroma separated, and consistency is good. I guess the more consistent solution would be to expose a compound control, similar to the video quantization one. struct v4l2_ctrl_jpeg_quantization { __u8luma_quantization_matrix[64]; __u8chroma_quantization_matrix[64]; }; Thanks! Eze
Re: [PATCH v3 6/7] media: Add controls for JPEG quantization tables
On Wed, Aug 29, 2018 at 11:50 AM Ezequiel Garcia wrote: > > On Mon, 2018-08-27 at 09:47 +0200, Paul Kocialkowski wrote: > > Hi, > > > > On Wed, 2018-08-22 at 13:59 -0300, Ezequiel Garcia wrote: > > > From: Shunqian Zheng > > > > > > Add V4L2_CID_JPEG_LUMA/CHROMA_QUANTIZATION controls to allow userspace > > > configure the JPEG quantization tables. > > > > How about having a single control for quantization? > > > > In MPEG-2/H.264/H.265, we have a single control exposed as a structure, > > which contains the tables for both luma and chroma. In the case of JPEG, > > it's not that big a deal, but for advanced video formats, it would be > > too much hassle to have one control per table. > > > > In order to keep the interface consistent, I think it'd be best to merge > > both matrices into a single control. > > > > What do you think? > > > > I think it makes a lot of sense. I don't see the benefit in having luma > and chroma separated, and consistency is good. > > I guess the more consistent solution would be to expose a compound > control, similar to the video quantization one. > > struct v4l2_ctrl_jpeg_quantization { >__u8luma_quantization_matrix[64]; >__u8chroma_quantization_matrix[64]; > }; Makes sense indeed. It also lets us avoid the hassle of setting .min/.max/.dims and other array control stuff, since everything is already defined by the C struct itself. Best regards, Tomasz
Re: [RFC] Request API and V4L2 capabilities
On Wed, Aug 22, 2018 at 11:10 PM Paul Kocialkowski wrote: > > Hi, > > On Tue, 2018-08-21 at 17:52 +0900, Tomasz Figa wrote: > > Hi Hans, Paul, > > > > On Mon, Aug 6, 2018 at 6:29 PM Paul Kocialkowski > > wrote: > > > > > > On Mon, 2018-08-06 at 11:23 +0200, Hans Verkuil wrote: > > > > On 08/06/2018 11:13 AM, Paul Kocialkowski wrote: > > > > > Hi, > > > > > > > > > > On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote: > > > > > > On 08/06/2018 10:16 AM, Paul Kocialkowski wrote: > > > > > > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote: > > > > > > > > Regarding point 3: I think this should be documented next to > > > > > > > > the pixel format. I.e. > > > > > > > > the MPEG-2 Slice format used by the stateless cedrus codec > > > > > > > > requires the request API > > > > > > > > and that two MPEG-2 controls (slice params and quantization > > > > > > > > matrices) must be present > > > > > > > > in each request. > > > > > > > > > > > > > > > > I am not sure a control flag (e.g. > > > > > > > > V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here. > > > > > > > > It's really implied by the fact that you use a stateless codec. > > > > > > > > It doesn't help > > > > > > > > generic applications like v4l2-ctl or qv4l2 either since in > > > > > > > > order to support > > > > > > > > stateless codecs they will have to know about the details of > > > > > > > > these controls anyway. > > > > > > > > > > > > > > > > So I am inclined to say that it is not necessary to expose this > > > > > > > > information in > > > > > > > > the API, but it has to be documented together with the pixel > > > > > > > > format documentation. > > > > > > > > > > > > > > I think this is affected by considerations about codec > > > > > > > profile/level > > > > > > > support. More specifically, some controls will only be required > > > > > > > for > > > > > > > supporting advanced codec profiles/levels, so they can only be > > > > > > > explicitly marked with appropriate flags by the driver when the > > > > > > > target > > > > > > > profile/level is known. And I don't think it would be sane for > > > > > > > userspace > > > > > > > to explicitly set what profile/level it's aiming at. As a result, > > > > > > > I > > > > > > > don't think we can explicitly mark controls as required or > > > > > > > optional. > > > > I'm not sure this is entirely true. The hardware may need to be > > explicitly told what profile the video is. It may even not be the > > hardware, but the driver itself too, given that the profile may imply > > the CAPTURE pixel format, e.g. for VP9 profiles: > > > > profile 0 > > color depth: 8 bit/sample, chroma subsampling: 4:2:0 > > profile 1 > > color depth: 8 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4 > > profile 2 > > color depth: 10–12 bit, chroma subsampling: 4:2:0 > > profile 3 > > color depth: 10–12 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4 > > > > (reference: https://en.wikipedia.org/wiki/VP9#Profiles) > > I think it would be fair to expect userspace to select the right > destination format (and maybe have the driver error if there's a > mismatch with the meta-data) instead of having the driver somewhat > expose what format should be used. There are many different memory representations of the same physical YUV format, just for YUV 4:2:0: NV12, NV12M, NV21, NV21M, YUV420, YUV420M, YVU420, YVU420M. It depends on hardware and driver which one would be available for given stream to decode. How is the user space expected to know which one is, without querying the driver first? > > But maybe this would be an API violation, since all the enumerated > formats are probably supposed to be selectable? Correct. > > We could also look at it the other way round and consider that selecting > an exposed format is always legit, but that it implies passing a > bitstream that matches it or the driver will error (because of an > invalid bitstream passed, not because of a "wrong" selected format). As per above, it's unlikely that a generic user space can set the right format. It may be able to narrow down the list of exposed formats to those which make sense for the generic information about the stream it has, e.g. VP9 profile 0 -> YUV 4:2:0, but there may still be a constraint on which representation is allowed depending on stream features. > > As far as I understood, the profile/level information is there to > indicate a set of supported features by the decoder, not as an > information used for the decoding process. Each corresponding feature is > enabled or not in the bitstream meta-data and that's all the information > the decoder really needs. > > This is why I think that setting the profile/level explicitly is not > justified by the nature of the process and adding it only for > convenience or marking whether controls are optional doesn't seem > justified at this point, in my opinion. Okay, it's actually a good point. Whether given format can be supported is not entirely dictated by profile/l
Re: [RFC] Request API and V4L2 capabilities
On Thu, Aug 23, 2018 at 5:05 PM Paul Kocialkowski wrote: > > Hi, > > On Wed, 2018-08-22 at 14:33 -0300, Ezequiel Garcia wrote: > > On Wed, 2018-08-22 at 16:10 +0200, Paul Kocialkowski wrote: > > > Hi, > > > > > > On Tue, 2018-08-21 at 17:52 +0900, Tomasz Figa wrote: > > > > Hi Hans, Paul, > > > > > > > > On Mon, Aug 6, 2018 at 6:29 PM Paul Kocialkowski > > > > wrote: > > > > > > > > > > On Mon, 2018-08-06 at 11:23 +0200, Hans Verkuil wrote: > > > > > > On 08/06/2018 11:13 AM, Paul Kocialkowski wrote: > > > > > > > Hi, > > > > > > > > > > > > > > On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote: > > > > > > > > On 08/06/2018 10:16 AM, Paul Kocialkowski wrote: > > > > > > > > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote: > > > > > > > > > > Regarding point 3: I think this should be documented next > > > > > > > > > > to the pixel format. I.e. > > > > > > > > > > the MPEG-2 Slice format used by the stateless cedrus codec > > > > > > > > > > requires the request API > > > > > > > > > > and that two MPEG-2 controls (slice params and quantization > > > > > > > > > > matrices) must be present > > > > > > > > > > in each request. > > > > > > > > > > > > > > > > > > > > I am not sure a control flag (e.g. > > > > > > > > > > V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here. > > > > > > > > > > It's really implied by the fact that you use a stateless > > > > > > > > > > codec. It doesn't help > > > > > > > > > > generic applications like v4l2-ctl or qv4l2 either since in > > > > > > > > > > order to support > > > > > > > > > > stateless codecs they will have to know about the details > > > > > > > > > > of these controls anyway. > > > > > > > > > > > > > > > > > > > > So I am inclined to say that it is not necessary to expose > > > > > > > > > > this information in > > > > > > > > > > the API, but it has to be documented together with the > > > > > > > > > > pixel format documentation. > > > > > > > > > > > > > > > > > > I think this is affected by considerations about codec > > > > > > > > > profile/level > > > > > > > > > support. More specifically, some controls will only be > > > > > > > > > required for > > > > > > > > > supporting advanced codec profiles/levels, so they can only be > > > > > > > > > explicitly marked with appropriate flags by the driver when > > > > > > > > > the target > > > > > > > > > profile/level is known. And I don't think it would be sane > > > > > > > > > for userspace > > > > > > > > > to explicitly set what profile/level it's aiming at. As a > > > > > > > > > result, I > > > > > > > > > don't think we can explicitly mark controls as required or > > > > > > > > > optional. > > > > > > > > I'm not sure this is entirely true. The hardware may need to be > > > > explicitly told what profile the video is. It may even not be the > > > > hardware, but the driver itself too, given that the profile may imply > > > > the CAPTURE pixel format, e.g. for VP9 profiles: > > > > > > > > profile 0 > > > > color depth: 8 bit/sample, chroma subsampling: 4:2:0 > > > > profile 1 > > > > color depth: 8 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4 > > > > profile 2 > > > > color depth: 10–12 bit, chroma subsampling: 4:2:0 > > > > profile 3 > > > > color depth: 10–12 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4 > > > > > > > > (reference: https://en.wikipedia.org/wiki/VP9#Profiles) > > > > > > I think it would be fair to expect userspace to select the right > > > destination format (and maybe have the driver error if there's a > > > mismatch with the meta-data) instead of having the driver somewhat > > > expose what format should be used. > > > > > > But maybe this would be an API violation, since all the enumerated > > > formats are probably supposed to be selectable? > > > > > > We could also look at it the other way round and consider that selecting > > > an exposed format is always legit, but that it implies passing a > > > bitstream that matches it or the driver will error (because of an > > > invalid bitstream passed, not because of a "wrong" selected format). > > > > > > > The API requires the user to negotiate via TRY_FMT/S_FMT. The driver > > usually does not return error on invalid formats, and simply return > > a format it can work with. I think the kernel-user contract has to > > guarantee if the driver accepted a given format, it won't fail to > > encoder or decode. > > Well, the issue here is that in order to correctly enumerate the > formats, the driver needs to be aware of: > 1. in what destination format the bitstream data is decoded to; > 2. what format convesions the VPU can do. > > Step 1 is known by userspace but is only passed to the driver with the > bitstream metadata from controls. This is much too late for trimming the > list of supported formats. That's not true. See my previous reply. The user space only knows the physical representation of samples in the stream, i.e. YUV 4:2:0 or 4:2:2. It doesn't know anything about hardware constraints on the mem
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: Wed Aug 29 05:00:14 CEST 2018 media-tree git hash:5b394b2ddf0347bef56e50c69a58773c94343ff3 media_build git hash: baf45935ffad914f33faf751ad9f4d0dd276c021 v4l-utils git hash: e37fbf50a28c1a1cfe9e00a60542bc14192a87ba edid-decode git hash: b2da1516df3cc2756bfe8d1fa06d7bf2562ba1f4 gcc version:i686-linux-gcc (GCC) 8.1.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.18.0-marune linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-arm-stm32: OK linux-git-arm64: OK linux-git-i686: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK Check COMPILE_TEST: OK linux-2.6.36.4-i686: ERRORS linux-2.6.36.4-x86_64: ERRORS linux-2.6.37.6-i686: ERRORS linux-2.6.37.6-x86_64: ERRORS linux-2.6.38.8-i686: ERRORS linux-2.6.38.8-x86_64: ERRORS linux-2.6.39.4-i686: ERRORS linux-2.6.39.4-x86_64: ERRORS linux-3.0.101-i686: ERRORS linux-3.0.101-x86_64: ERRORS linux-3.1.10-i686: ERRORS linux-3.1.10-x86_64: ERRORS linux-3.2.102-i686: ERRORS linux-3.2.102/arch/x86/include/asm/smp.h:183:17: error: storage class specified for parameter 'disabled_cpus' linux-3.2.102-x86_64: ERRORS linux-3.3.8-i686: ERRORS linux-3.3.8-x86_64: ERRORS linux-3.4.113-i686: ERRORS linux-3.4.113-x86_64: ERRORS linux-3.5.7-i686: ERRORS linux-3.5.7-x86_64: ERRORS linux-3.6.11-i686: ERRORS linux-3.6.11-x86_64: ERRORS linux-3.7.10-i686: ERRORS linux-3.7.10-x86_64: ERRORS linux-3.8.13-i686: ERRORS linux-3.8.13-x86_64: ERRORS linux-3.9.11-i686: ERRORS linux-3.9.11-x86_64: ERRORS linux-3.10.108-i686: ERRORS linux-3.10.108-x86_64: ERRORS linux-3.11.10-i686: ERRORS linux-3.11.10/arch/x86/include/asm/msr.h:131, 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/arch/x86/include/asm/hw_irq.h:49:24: error: storage class specified for parameter 'invalidate_interrupt9' 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.119-i686: ERRORS linux-3.18.119-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.152-i686: ERRORS linux-4.4.152-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.124-i686: ERRORS linux-4.9.124-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: ERRORS linux-4.13.16-x86_64: ERRORS linux-4.14.67-i686: ERRORS linux-4.14.67-x86_64: ERRORS linux-4.15.18-i686: ERRORS linux-4.15.18-x86_64: ERRORS linux-4.16.18-i686: ERRORS linux-4.16.18-x86_64: ERRORS linux-4.17.19-i686: ERRORS linux-4.17.19-x86_64: ERRORS linux-4.18.5-i686: ERRORS linux-4.18.5-x86_64: ERRORS linux-4.19-rc1-i686: ERRORS linux-4.19-rc1-x86_64: ERRORS apps: OK specified for parameter 'kernel_sock_ioctl' specifiers or '...' before 'read_descriptor_t' specified for parameter 'ftrace_graph_init_task' specified for parameter 'ioctl_by_bdev' specifiers or '...' before 'atomic_long_t' specifiers or '...' before 'atomic_long_t' specifiers or '...' before 'atomic64_t' specifiers or '...' before 'atomic64_t' spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Wednesday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Wednesday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
Re: [RFC] Request API and V4L2 capabilities
On Fri, Aug 24, 2018 at 2:33 AM Nicolas Dufresne wrote: > > Le jeudi 23 août 2018 à 10:05 +0200, Paul Kocialkowski a écrit : > > Hi, > > > > On Wed, 2018-08-22 at 14:33 -0300, Ezequiel Garcia wrote: > > > On Wed, 2018-08-22 at 16:10 +0200, Paul Kocialkowski wrote: > > > > Hi, > > > > > > > > On Tue, 2018-08-21 at 17:52 +0900, Tomasz Figa wrote: > > > > > Hi Hans, Paul, > > > > > > > > > > On Mon, Aug 6, 2018 at 6:29 PM Paul Kocialkowski > > > > > wrote: > > > > > > > > > > > > On Mon, 2018-08-06 at 11:23 +0200, Hans Verkuil wrote: > > > > > > > On 08/06/2018 11:13 AM, Paul Kocialkowski wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote: > > > > > > > > > On 08/06/2018 10:16 AM, Paul Kocialkowski wrote: > > > > > > > > > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote: > > > > > > > > > > > Regarding point 3: I think this should be documented next > > > > > > > > > > > to the pixel format. I.e. > > > > > > > > > > > the MPEG-2 Slice format used by the stateless cedrus > > > > > > > > > > > codec requires the request API > > > > > > > > > > > and that two MPEG-2 controls (slice params and > > > > > > > > > > > quantization matrices) must be present > > > > > > > > > > > in each request. > > > > > > > > > > > > > > > > > > > > > > I am not sure a control flag (e.g. > > > > > > > > > > > V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here. > > > > > > > > > > > It's really implied by the fact that you use a stateless > > > > > > > > > > > codec. It doesn't help > > > > > > > > > > > generic applications like v4l2-ctl or qv4l2 either since > > > > > > > > > > > in order to support > > > > > > > > > > > stateless codecs they will have to know about the details > > > > > > > > > > > of these controls anyway. > > > > > > > > > > > > > > > > > > > > > > So I am inclined to say that it is not necessary to > > > > > > > > > > > expose this information in > > > > > > > > > > > the API, but it has to be documented together with the > > > > > > > > > > > pixel format documentation. > > > > > > > > > > > > > > > > > > > > I think this is affected by considerations about codec > > > > > > > > > > profile/level > > > > > > > > > > support. More specifically, some controls will only be > > > > > > > > > > required for > > > > > > > > > > supporting advanced codec profiles/levels, so they can only > > > > > > > > > > be > > > > > > > > > > explicitly marked with appropriate flags by the driver when > > > > > > > > > > the target > > > > > > > > > > profile/level is known. And I don't think it would be sane > > > > > > > > > > for userspace > > > > > > > > > > to explicitly set what profile/level it's aiming at. As a > > > > > > > > > > result, I > > > > > > > > > > don't think we can explicitly mark controls as required or > > > > > > > > > > optional. > > > > > > > > > > I'm not sure this is entirely true. The hardware may need to be > > > > > explicitly told what profile the video is. It may even not be the > > > > > hardware, but the driver itself too, given that the profile may imply > > > > > the CAPTURE pixel format, e.g. for VP9 profiles: > > > > > > > > > > profile 0 > > > > > color depth: 8 bit/sample, chroma subsampling: 4:2:0 > > > > > profile 1 > > > > > color depth: 8 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4 > > > > > profile 2 > > > > > color depth: 10–12 bit, chroma subsampling: 4:2:0 > > > > > profile 3 > > > > > color depth: 10–12 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4 > > > > > > > > > > (reference: https://en.wikipedia.org/wiki/VP9#Profiles) > > > > > > > > I think it would be fair to expect userspace to select the right > > > > destination format (and maybe have the driver error if there's a > > > > mismatch with the meta-data) instead of having the driver somewhat > > > > expose what format should be used. > > > > > > > > But maybe this would be an API violation, since all the enumerated > > > > formats are probably supposed to be selectable? > > > > > > > > We could also look at it the other way round and consider that selecting > > > > an exposed format is always legit, but that it implies passing a > > > > bitstream that matches it or the driver will error (because of an > > > > invalid bitstream passed, not because of a "wrong" selected format). > > > > > > > > > > The API requires the user to negotiate via TRY_FMT/S_FMT. The driver > > > usually does not return error on invalid formats, and simply return > > > a format it can work with. I think the kernel-user contract has to > > > guarantee if the driver accepted a given format, it won't fail to > > > encoder or decode. > > > > Well, the issue here is that in order to correctly enumerate the > > formats, the driver needs to be aware of: > > 1. in what destination format the bitstream data is decoded to; > > This is covered by the state-full specification patch if I remember > correctly. So the driver, if it's a multi-format, will first return all > possible
Re: [RFC] Request API and V4L2 capabilities
On Fri, Aug 24, 2018 at 4:30 PM Hans Verkuil wrote: > > On 08/23/2018 07:37 PM, Nicolas Dufresne wrote: > > Le jeudi 23 août 2018 à 16:31 +0200, Hans Verkuil a écrit : > >>> I propose adding these capabilities: > >>> > >>> #define V4L2_BUF_CAP_HAS_REQUESTS 0x0001 > >>> #define V4L2_BUF_CAP_REQUIRES_REQUESTS0x0002 > >>> #define V4L2_BUF_CAP_HAS_MMAP 0x0100 > >>> #define V4L2_BUF_CAP_HAS_USERPTR 0x0200 > >>> #define V4L2_BUF_CAP_HAS_DMABUF 0x0400 > >> > >> I substituted SUPPORTS for HAS and dropped the REQUIRES_REQUESTS > >> capability. > >> As Tomasz mentioned, technically (at least for stateless codecs) you could > >> handle just one frame at a time without using requests. It's very > >> inefficient, > >> but it would work. > > > > I thought the request was providing a data structure to refer back to > > the frames, so each codec don't have to implement one. Do you mean that > > the framework will implicitly request requests in that mode ? or simply > > that there is no such helper ? > > Yes, that's done through controls as well. > > The idea would be that you set the necessary controls, queue a buffer to > the output queue, dequeue a buffer from the output queue, read back any > new state information and repeat the process. > > That said, I'm not sure if the cedrus driver for example can handle this > at the moment. It is also inefficient and it won't work if codecs require > more than one buffer in the queue for whatever reason. > > Tomasz, Paul, please correct me if I am wrong. > > In any case, I think we can do without this proposed capability since it is > simply a requirement when implementing the pixelformat for the stateless > codec that the Request API will be available and it should be documented > as such in the spec. No correction needed. :) Best regards, Tomasz