[PATCH] media: v4l2-subdev.h: allow V4L2_FRMIVAL_TYPE_CONTINUOUS & _STEPWISE

[Philippe De Muyter]

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

[Paul Kocialkowski]

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

[Hans Verkuil]

This message is generated daily by a cron job that builds media_tree for
the kernels and architectures in the list below.

Results of the daily build of media_tree:

date:   Tue 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

[Tomasz Figa]

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

[Sakari Ailus]

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

[Hans Verkuil]

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

[Philippe De Muyter]

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

[Hans Verkuil]

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

[Philippe De Muyter]

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

[Hans Verkuil]

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

[Hans Verkuil]

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

[jacopo mondi]

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

[jacopo mondi]

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

[Jason]

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

[Jason]

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

[Hans Verkuil]

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

[Hans Verkuil]

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

[Hans Verkuil]

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

[Hans Verkuil]

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

[Hans Verkuil]

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

[Hans Verkuil]

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

[Hans Verkuil]

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

[Hans Verkuil]

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

[Hans Verkuil]

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

[Hans Verkuil]

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

[Hans Verkuil]

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

[Matthias Reichl]

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

[Loic Poulain]

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

[Ruby]

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

[Hans Verkuil]

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

[Ruby]

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

[Rob Herring]

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

[Steve Longerbeam]

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

[Ezequiel Garcia]

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

[Tomasz Figa]

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

[Tomasz Figa]

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

[Tomasz Figa]

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

[Hans Verkuil]

This message is generated daily by a cron job that builds media_tree for
the kernels and architectures in the list below.

Results of the daily build of media_tree:

date:   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

[Tomasz Figa]

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

[Tomasz Figa]

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