Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-13 Thread Sakari Ailus
Hi Jacopo,

On Wed, Nov 14, 2018 at 01:25:11AM +0100, jacopo mondi wrote:
> On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
> > Hi,
> >
> > This series adds support for the Intel IPU3 (Image Processing Unit)
> > ImgU which is essentially a modern memory-to-memory ISP. It implements
> > raw Bayer to YUV image format conversion as well as a large number of
> > other pixel processing algorithms for improving the image quality.
> >
> > Meta data formats are defined for image statistics (3A, i.e. automatic
> > white balance, exposure and focus, histogram and local area contrast
> > enhancement) as well as for the pixel processing algorithm parameters.
> > The documentation for these formats is currently not included in the
> > patchset but will be added in a future version of this set.
> >
> > The algorithm parameters need to be considered specific to a given frame
> > and typically a large number of these parameters change on frame to frame
> > basis. Additionally, the parameters are highly structured (and not a flat
> > space of independent configuration primitives). They also reflect the
> > data structures used by the firmware and the hardware. On top of that,
> > the algorithms require highly specialized user space to make meaningful
> > use of them. For these reasons it has been chosen video buffers to pass
> > the parameters to the device.
> >
> > On individual patches:
> >
> > The heart of ImgU is the CSS, or Camera Subsystem, which contains the
> > image processors and HW accelerators.
> >
> > The 3A statistics and other firmware parameter computation related
> > functions are implemented in patch 11.
> >
> > All IPU3 pipeline default settings can be found in patch 10.
> >
> 
> Seems to me that patch 10 didn't make it to the mailing list, am I
> wrong?
> 
> I'm pointing it out as the same happened on your v6.

Thanks for pointing this out. I've uploaded the entire set here:

https://git.linuxtv.org/sailus/media_tree.git/log/?h=ipu3-v7>

including the 10th patch:

https://git.linuxtv.org/sailus/media_tree.git/commit/?h=ipu3-v7=41e2f0d114dbc195efed079202d22748ddedbe83>

It's too big to get through the list server. :-(

Luckily, it's mostly tables so there's not much to review there. Default
settings, effectively.

-- 
Regards,

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


Re: [PATCH 1/9] videodev2.h: add tag support

2018-11-13 Thread Hans Verkuil
On 11/13/2018 10:42 AM, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> Add support for 'tags' to struct v4l2_buffer. These can be used
> by m2m devices so userspace can set a tag for an output buffer and
> this value will then be copied to the capture buffer(s).
> 
> This tag can be used to refer to capture buffers, something that
> is needed by stateless HW codecs.
> 
> The new V4L2_BUF_CAP_SUPPORTS_TAGS capability indicates whether
> or not tags are supported.
> 
> Signed-off-by: Hans Verkuil 
> ---
>  include/uapi/linux/videodev2.h | 38 +-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index c8e8ff810190..ec1fef2a9445 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -879,6 +879,7 @@ struct v4l2_requestbuffers {
>  #define V4L2_BUF_CAP_SUPPORTS_USERPTR(1 << 1)
>  #define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2)
>  #define V4L2_BUF_CAP_SUPPORTS_REQUESTS   (1 << 3)
> +#define V4L2_BUF_CAP_SUPPORTS_TAGS   (1 << 4)
>  
>  /**
>   * struct v4l2_plane - plane info for multi-planar buffers
> @@ -912,6 +913,11 @@ struct v4l2_plane {
>   __u32   reserved[11];
>  };
>  
> +struct v4l2_buffer_tag {
> + __u32 low;
> + __u32 high;
> +};
> +
>  /**
>   * struct v4l2_buffer - video buffer info
>   * @index:   id number of the buffer
> @@ -950,7 +956,10 @@ struct v4l2_buffer {
>   __u32   flags;
>   __u32   field;
>   struct timeval  timestamp;
> - struct v4l2_timecodetimecode;
> + union {
> + struct v4l2_timecodetimecode;
> + struct v4l2_buffer_tag  tag;
> + };
>   __u32   sequence;
>  
>   /* memory location */
> @@ -988,6 +997,8 @@ struct v4l2_buffer {
>  #define V4L2_BUF_FLAG_IN_REQUEST 0x0080
>  /* timecode field is valid */
>  #define V4L2_BUF_FLAG_TIMECODE   0x0100
> +/* tag field is valid */
> +#define V4L2_BUF_FLAG_TAG0x0200
>  /* Buffer is prepared for queuing */
>  #define V4L2_BUF_FLAG_PREPARED   0x0400
>  /* Cache handling flags */
> @@ -1007,6 +1018,31 @@ struct v4l2_buffer {
>  /* request_fd is valid */
>  #define V4L2_BUF_FLAG_REQUEST_FD 0x0080
>  
> +static inline void v4l2_buffer_set_tag(struct v4l2_buffer *buf, __u64 tag)
> +{
> + buf->tag.high = tag >> 32;
> + buf->tag.low = tag & 0xULL;
> + buf->flags |= V4L2_BUF_FLAG_TAG;
> +}
> +
> +static inline void v4l2_buffer_set_tag_ptr(struct v4l2_buffer *buf,
> +const void *tag)
> +{
> + v4l2_buffer_set_tag(buf, (uintptr_t)tag);
> +}
> +
> +static inline __u64 v4l2_buffer_get_tag(const struct v4l2_buffer *buf)
> +{
> + if (!(buf->flags & V4L2_BUF_FLAG_TAG))
> + return 0;
> + return (((__u64)buf->tag.high) << 32) | (__u64)buf->tag.low;
> +}
> +
> +static inline void *v4l2_buffer_get_tag_ptr(const struct v4l2_buffer *buf)
> +{
> + return (void *)(uintptr_t)v4l2_buffer_get_tag(buf);
> +}
> +

I'm reconsidering my decision to use a u64 for the tag. It is too fiddly due
to the fact that I have to use a struct v4l2_buffer_tag. I think I'll just
use a u32.

I'll post a new version where I make this change.

Regards,

Hans

>  /**
>   * struct v4l2_exportbuffer - export of video buffer as DMABUF file 
> descriptor
>   *
> 



Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-13 Thread Bing Bu Cao



On 11/14/2018 05:58 AM, Sakari Ailus wrote:
> On Tue, Nov 13, 2018 at 07:04:01PM +0800, Bing Bu Cao wrote:
>>
>> On 11/13/2018 06:31 PM, Sakari Ailus wrote:
>>> Hi Bing Bu,
>>>
>>> On Mon, Nov 12, 2018 at 12:31:16PM +0800, Bing Bu Cao wrote:
 On 11/09/2018 06:09 PM, Sakari Ailus wrote:
> Hi Bing Bu,
>
> On Wed, Nov 07, 2018 at 12:16:47PM +0800, Bing Bu Cao wrote:
>> On 11/01/2018 08:03 PM, Sakari Ailus wrote:
>>> Hi Yong,
>>>
>>> Thanks for the update!
>>>
>>> On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
 Hi,

 This series adds support for the Intel IPU3 (Image Processing Unit)
 ImgU which is essentially a modern memory-to-memory ISP. It implements
 raw Bayer to YUV image format conversion as well as a large number of
 other pixel processing algorithms for improving the image quality.

 Meta data formats are defined for image statistics (3A, i.e. automatic
 white balance, exposure and focus, histogram and local area contrast
 enhancement) as well as for the pixel processing algorithm parameters.
 The documentation for these formats is currently not included in the
 patchset but will be added in a future version of this set.

 The algorithm parameters need to be considered specific to a given 
 frame
 and typically a large number of these parameters change on frame to 
 frame
 basis. Additionally, the parameters are highly structured (and not a 
 flat
 space of independent configuration primitives). They also reflect the
 data structures used by the firmware and the hardware. On top of that,
 the algorithms require highly specialized user space to make meaningful
 use of them. For these reasons it has been chosen video buffers to pass
 the parameters to the device.

 On individual patches:

 The heart of ImgU is the CSS, or Camera Subsystem, which contains the
 image processors and HW accelerators.

 The 3A statistics and other firmware parameter computation related
 functions are implemented in patch 11.

 All IPU3 pipeline default settings can be found in patch 10.

 To access DDR via ImgU's own memory space, IPU3 is also equipped with
 its own MMU unit, the driver is implemented in patch 6.

 Patch 7 uses above driver for DMA mapping operation.

 The communication between IPU3 firmware and driver is implemented with 
 circular
 queues in patch 8.

 Patch 9 provide some utility functions and manage IPU3 fw download and
 install.

 The firmware which is called ipu3-fw.bin can be downloaded from:

 git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
 (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)

 Firmware ABI is defined in patches 4 and 5.

 Patches 12 and 13 are of the same file, the former contains all h/w 
 programming
 related code, the latter implements interface functions for access fw 
 & hw
 capabilities.

 Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:

 https://patchwork.kernel.org/patch/9976295/>
>>> I've pushed the latest set here:
>>>
>>> https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output>
>>>
>>> You can just say the entire set depends on those going forward; the
>>> documentation is needed, too.
>>>
 Patch 15 represents the top level that glues all of the other 
 components together,
 passing arguments between the components.

 Patch 16 is a recent effort to extend v6 for advanced camera features 
 like
 Continuous View Finder (CVF) and Snapshot During Video(SDV) support.

 Link to user space implementation:

 git clone 
 https://chromium.googlesource.com/chromiumos/platform/arc-camera

 ImgU media topology print:

 # media-ctl -d /dev/media0 -p
 Media controller API version 4.19.0

 Media device information
 
 driver  ipu3-imgu
 model   ipu3-imgu
 serial  
 bus infoPCI::00:05.0
 hw revision 0x80862015
 driver version  4.19.0

 Device topology
 - entity 1: ipu3-imgu 0 (5 pads, 5 links)
 type V4L2 subdev subtype Unknown flags 0
 device node name /dev/v4l-subdev0
pad0: Sink
[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
>>> This doesn't seem right. Which formats can be 

cron job: media_tree daily build: WARNINGS

2018-11-13 Thread 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 Nov 14 05:00:11 CET 2018
media-tree git hash:fbe57dde7126d1b2712ab5ea93fb9d15f89de708
media_build git hash:   a8aef9cea0a4a2f3ea86c0b37bd6a1378018c0c1
v4l-utils git hash: 98b4c9f276a18535b5691e5f350f59ffbf5a9aa5
edid-decode git hash:   5eeb151a748788666534d6ea3da07f90400d24c2
gcc version:i686-linux-gcc (GCC) 8.2.0
sparse version: 0.5.2
smatch version: 0.5.1
host hardware:  x86_64
host os:4.18.0-2-amd64

linux-git-arm-at91: WARNINGS
linux-git-arm-davinci: WARNINGS
linux-git-arm-multi: WARNINGS
linux-git-arm-pxa: WARNINGS
linux-git-arm-stm32: WARNINGS
linux-git-arm64: WARNINGS
linux-git-i686: WARNINGS
linux-git-mips: OK
linux-git-powerpc64: WARNINGS
linux-git-sh: WARNINGS
linux-git-x86_64: WARNINGS
Check COMPILE_TEST: 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.123-i686: OK
linux-3.18.123-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.159-i686: OK
linux-4.4.159-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: OK
linux-4.7.10-i686: OK
linux-4.7.10-x86_64: OK
linux-4.8.17-i686: OK
linux-4.8.17-x86_64: OK
linux-4.9.131-i686: OK
linux-4.9.131-x86_64: OK
linux-4.10.17-i686: OK
linux-4.10.17-x86_64: OK
linux-4.11.12-i686: OK
linux-4.11.12-x86_64: OK
linux-4.12.14-i686: OK
linux-4.12.14-x86_64: OK
linux-4.13.16-i686: OK
linux-4.13.16-x86_64: OK
linux-4.14.74-i686: OK
linux-4.14.74-x86_64: OK
linux-4.15.18-i686: OK
linux-4.15.18-x86_64: OK
linux-4.16.18-i686: OK
linux-4.16.18-x86_64: OK
linux-4.17.19-i686: OK
linux-4.17.19-x86_64: OK
linux-4.18.12-i686: OK
linux-4.18.12-x86_64: OK
linux-4.19.1-i686: OK
linux-4.19.1-x86_64: OK
linux-4.20-rc1-i686: OK
linux-4.20-rc1-x86_64: OK
apps: OK
spec-git: OK
sparse: WARNINGS

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: [PATCH] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers

2018-11-13 Thread Tomasz Figa
On Wed, Nov 14, 2018 at 8:51 AM Nicolas Dufresne  wrote:
>
> Le mercredi 14 novembre 2018 à 00:27 +0200, Sakari Ailus a écrit :
> > Hi Philipp,
> >
> > On Tue, Nov 13, 2018 at 04:06:21PM +0100, Philipp Zabel wrote:
> > > From: John Sheu 
> > >
> > > Videobuf2 presently does not allow VIDIOC_REQBUFS to destroy outstanding
> > > buffers if the queue is of type V4L2_MEMORY_MMAP, and if the buffers are
> > > considered "in use".  This is different behavior than for other memory
> > > types and prevents us from deallocating buffers in following two cases:
> > >
> > > 1) There are outstanding mmap()ed views on the buffer. However even if
> > >we put the buffer in reqbufs(0), there will be remaining references,
> > >due to vma .open/close() adjusting vb2 buffer refcount appropriately.
> > >This means that the buffer will be in fact freed only when the last
> > >mmap()ed view is unmapped.
> > >
> > > 2) Buffer has been exported as a DMABUF. Refcount of the vb2 buffer
> > >is managed properly by VB2 DMABUF ops, i.e. incremented on DMABUF
> > >get and decremented on DMABUF release. This means that the buffer
> > >will be alive until all importers release it.
> > >
> > > Considering both cases above, there does not seem to be any need to
> > > prevent reqbufs(0) operation, because buffer lifetime is already
> > > properly managed by both mmap() and DMABUF code paths. Let's remove it
> > > and allow userspace freeing the queue (and potentially allocating a new
> > > one) even though old buffers might be still in processing.
> > >
> > > To let userspace know that the kernel now supports orphaning buffers
> > > that are still in use, add a new V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS
> > > to be set by reqbufs and create_bufs.
> > >
> > > Signed-off-by: John Sheu 
> > > Reviewed-by: Pawel Osciak 
> > > Reviewed-by: Tomasz Figa 
> > > Signed-off-by: Tomasz Figa 
> > > [p.za...@pengutronix.de: moved __vb2_queue_cancel out of the mmap_lock
> > >  and added V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS]
> > > Signed-off-by: Philipp Zabel 
> >
> > This lets the user to allocate lots of mmap'ed buffers that are pinned in
> > physical memory. Considering that we don't really have a proper mechanism
> > to limit that anyway,
>
> It's currently limited to 32 buffers. It's not worst then DRM dumb
> buffers which will let you allocate as much as you want.
>

32 buffers for one mem2mem instance. One can open many of those and
allocate more anyway.

I think it's a part of the global problem of DMA memory not being
accounted to the process allocating...

Best regards,
Tomasz

> >
> > Acked-by: Sakari Ailus 
> >
> > That said, the patch must be accompanied by the documentation change in
> > Documentation/media/uapi/v4l/vidioc-reqbufs.rst .
> >
> > I wonder what Hans thinks.
> >
> > > ---
> > >  .../media/common/videobuf2/videobuf2-core.c   | 26 +--
> > >  .../media/common/videobuf2/videobuf2-v4l2.c   |  2 +-
> > >  include/uapi/linux/videodev2.h|  1 +
> > >  3 files changed, 3 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
> > > b/drivers/media/common/videobuf2/videobuf2-core.c
> > > index 975ff5669f72..608459450c1e 100644
> > > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > > @@ -553,20 +553,6 @@ bool vb2_buffer_in_use(struct vb2_queue *q, struct 
> > > vb2_buffer *vb)
> > >  }
> > >  EXPORT_SYMBOL(vb2_buffer_in_use);
> > >
> > > -/*
> > > - * __buffers_in_use() - return true if any buffers on the queue are in 
> > > use and
> > > - * the queue cannot be freed (by the means of REQBUFS(0)) call
> > > - */
> > > -static bool __buffers_in_use(struct vb2_queue *q)
> > > -{
> > > -   unsigned int buffer;
> > > -   for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> > > -   if (vb2_buffer_in_use(q, q->bufs[buffer]))
> > > -   return true;
> > > -   }
> > > -   return false;
> > > -}
> > > -
> > >  void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
> > >  {
> > > call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
> > > @@ -674,23 +660,13 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum 
> > > vb2_memory memory,
> > >
> > > if (*count == 0 || q->num_buffers != 0 ||
> > > (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
> > > -   /*
> > > -* We already have buffers allocated, so first check if they
> > > -* are not in use and can be freed.
> > > -*/
> > > -   mutex_lock(>mmap_lock);
> > > -   if (q->memory == VB2_MEMORY_MMAP && __buffers_in_use(q)) {
> > > -   mutex_unlock(>mmap_lock);
> > > -   dprintk(1, "memory in use, cannot free\n");
> > > -   return -EBUSY;
> > > -   }
> > > -
> > > /*
> > >  * Call queue_cancel to clean up any buffers in the

Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-13 Thread jacopo mondi
On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
> Hi,
>
> This series adds support for the Intel IPU3 (Image Processing Unit)
> ImgU which is essentially a modern memory-to-memory ISP. It implements
> raw Bayer to YUV image format conversion as well as a large number of
> other pixel processing algorithms for improving the image quality.
>
> Meta data formats are defined for image statistics (3A, i.e. automatic
> white balance, exposure and focus, histogram and local area contrast
> enhancement) as well as for the pixel processing algorithm parameters.
> The documentation for these formats is currently not included in the
> patchset but will be added in a future version of this set.
>
> The algorithm parameters need to be considered specific to a given frame
> and typically a large number of these parameters change on frame to frame
> basis. Additionally, the parameters are highly structured (and not a flat
> space of independent configuration primitives). They also reflect the
> data structures used by the firmware and the hardware. On top of that,
> the algorithms require highly specialized user space to make meaningful
> use of them. For these reasons it has been chosen video buffers to pass
> the parameters to the device.
>
> On individual patches:
>
> The heart of ImgU is the CSS, or Camera Subsystem, which contains the
> image processors and HW accelerators.
>
> The 3A statistics and other firmware parameter computation related
> functions are implemented in patch 11.
>
> All IPU3 pipeline default settings can be found in patch 10.
>

Seems to me that patch 10 didn't make it to the mailing list, am I
wrong?

I'm pointing it out as the same happened on your v6.

Thanks
   j

> To access DDR via ImgU's own memory space, IPU3 is also equipped with
> its own MMU unit, the driver is implemented in patch 6.
>
> Patch 7 uses above driver for DMA mapping operation.
>
> The communication between IPU3 firmware and driver is implemented with 
> circular
> queues in patch 8.
>
> Patch 9 provide some utility functions and manage IPU3 fw download and
> install.
>
> The firmware which is called ipu3-fw.bin can be downloaded from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
> (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)
>
> Firmware ABI is defined in patches 4 and 5.
>
> Patches 12 and 13 are of the same file, the former contains all h/w 
> programming
> related code, the latter implements interface functions for access fw & hw
> capabilities.
>
> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:
>
> https://patchwork.kernel.org/patch/9976295/>
>
> Patch 15 represents the top level that glues all of the other components 
> together,
> passing arguments between the components.
>
> Patch 16 is a recent effort to extend v6 for advanced camera features like
> Continuous View Finder (CVF) and Snapshot During Video(SDV) support.
>
> Link to user space implementation:
>
> git clone https://chromium.googlesource.com/chromiumos/platform/arc-camera
>
> ImgU media topology print:
>
> # media-ctl -d /dev/media0 -p
> Media controller API version 4.19.0
>
> Media device information
> 
> driver  ipu3-imgu
> model   ipu3-imgu
> serial
> bus infoPCI::00:05.0
> hw revision 0x80862015
> driver version  4.19.0
>
> Device topology
> - entity 1: ipu3-imgu 0 (5 pads, 5 links)
> type V4L2 subdev subtype Unknown flags 0
> device node name /dev/v4l-subdev0
>   pad0: Sink
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
>crop:(0,0)/1920x1080
>compose:(0,0)/1920x1080]
>   <- "ipu3-imgu 0 input":0 []
>   pad1: Sink
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
>   <- "ipu3-imgu 0 parameters":0 []
>   pad2: Source
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
>   -> "ipu3-imgu 0 output":0 []
>   pad3: Source
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
>   -> "ipu3-imgu 0 viewfinder":0 []
>   pad4: Source
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
>   -> "ipu3-imgu 0 3a stat":0 []
>
> - entity 7: ipu3-imgu 1 (5 pads, 5 links)
> type V4L2 subdev subtype Unknown flags 0
> device node name /dev/v4l-subdev1
>   pad0: Sink
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
>crop:(0,0)/1920x1080
>compose:(0,0)/1920x1080]
>   <- "ipu3-imgu 1 input":0 []
>   pad1: Sink
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
>   <- "ipu3-imgu 1 parameters":0 []
>   pad2: Source
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
>   -> "ipu3-imgu 1 output":0 []
>   pad3: Source
>   

Re: [PATCH 0/7] media: i2c: small enhancements for camera sensor drivers

2018-11-13 Thread jacopo mondi
Hello Mita-san,
   thanks for the patches

On Tue, Nov 13, 2018 at 01:00:47AM +0900, Akinobu Mita wrote:
> This patchset addds relatively small enhancements (log_status ioctl, event
> interface, V4L2_CID_TEST_PATTERN control, and V4L2_CID_COLORFX control) for
> mt9m111, ov2640, ov5640, ov7670, and ov772x drivers.  I have these devices
> so these patches are tested with real devices.
>

For the ov772x part:
Acked-by: Jacopo Mondi 

Thanks
   j

> Akinobu Mita (7):
>   media: mt9m111: support log_status ioctl and event interface
>   media: mt9m111: add V4L2_CID_COLORFX control
>   media: ov2640: add V4L2_CID_TEST_PATTERN control
>   media: ov2640: support log_status ioctl and event interface
>   media: ov5640: support log_status ioctl and event interface
>   media: ov7670: support log_status ioctl and event interface
>   media: ov772x: support log_status ioctl and event interface
>
>  drivers/media/i2c/mt9m111.c | 44 ++--
>  drivers/media/i2c/ov2640.c  | 21 +++--
>  drivers/media/i2c/ov5640.c  |  7 ++-
>  drivers/media/i2c/ov7670.c  |  6 +-
>  drivers/media/i2c/ov772x.c  |  7 ++-
>  5 files changed, 78 insertions(+), 7 deletions(-)
>
> Cc: Steve Longerbeam 
> Cc: Jonathan Corbet 
> Cc: Jacopo Mondi 
> Cc: Sakari Ailus 
> Cc: Mauro Carvalho Chehab 
> --
> 2.7.4
>


signature.asc
Description: PGP signature


Re: [PATCH] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers

2018-11-13 Thread Nicolas Dufresne
Le mercredi 14 novembre 2018 à 00:27 +0200, Sakari Ailus a écrit :
> Hi Philipp,
> 
> On Tue, Nov 13, 2018 at 04:06:21PM +0100, Philipp Zabel wrote:
> > From: John Sheu 
> > 
> > Videobuf2 presently does not allow VIDIOC_REQBUFS to destroy outstanding
> > buffers if the queue is of type V4L2_MEMORY_MMAP, and if the buffers are
> > considered "in use".  This is different behavior than for other memory
> > types and prevents us from deallocating buffers in following two cases:
> > 
> > 1) There are outstanding mmap()ed views on the buffer. However even if
> >we put the buffer in reqbufs(0), there will be remaining references,
> >due to vma .open/close() adjusting vb2 buffer refcount appropriately.
> >This means that the buffer will be in fact freed only when the last
> >mmap()ed view is unmapped.
> > 
> > 2) Buffer has been exported as a DMABUF. Refcount of the vb2 buffer
> >is managed properly by VB2 DMABUF ops, i.e. incremented on DMABUF
> >get and decremented on DMABUF release. This means that the buffer
> >will be alive until all importers release it.
> > 
> > Considering both cases above, there does not seem to be any need to
> > prevent reqbufs(0) operation, because buffer lifetime is already
> > properly managed by both mmap() and DMABUF code paths. Let's remove it
> > and allow userspace freeing the queue (and potentially allocating a new
> > one) even though old buffers might be still in processing.
> > 
> > To let userspace know that the kernel now supports orphaning buffers
> > that are still in use, add a new V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS
> > to be set by reqbufs and create_bufs.
> > 
> > Signed-off-by: John Sheu 
> > Reviewed-by: Pawel Osciak 
> > Reviewed-by: Tomasz Figa 
> > Signed-off-by: Tomasz Figa 
> > [p.za...@pengutronix.de: moved __vb2_queue_cancel out of the mmap_lock
> >  and added V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS]
> > Signed-off-by: Philipp Zabel 
> 
> This lets the user to allocate lots of mmap'ed buffers that are pinned in
> physical memory. Considering that we don't really have a proper mechanism
> to limit that anyway,

It's currently limited to 32 buffers. It's not worst then DRM dumb
buffers which will let you allocate as much as you want.

> 
> Acked-by: Sakari Ailus 
> 
> That said, the patch must be accompanied by the documentation change in
> Documentation/media/uapi/v4l/vidioc-reqbufs.rst .
> 
> I wonder what Hans thinks.
> 
> > ---
> >  .../media/common/videobuf2/videobuf2-core.c   | 26 +--
> >  .../media/common/videobuf2/videobuf2-v4l2.c   |  2 +-
> >  include/uapi/linux/videodev2.h|  1 +
> >  3 files changed, 3 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
> > b/drivers/media/common/videobuf2/videobuf2-core.c
> > index 975ff5669f72..608459450c1e 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -553,20 +553,6 @@ bool vb2_buffer_in_use(struct vb2_queue *q, struct 
> > vb2_buffer *vb)
> >  }
> >  EXPORT_SYMBOL(vb2_buffer_in_use);
> >  
> > -/*
> > - * __buffers_in_use() - return true if any buffers on the queue are in use 
> > and
> > - * the queue cannot be freed (by the means of REQBUFS(0)) call
> > - */
> > -static bool __buffers_in_use(struct vb2_queue *q)
> > -{
> > -   unsigned int buffer;
> > -   for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> > -   if (vb2_buffer_in_use(q, q->bufs[buffer]))
> > -   return true;
> > -   }
> > -   return false;
> > -}
> > -
> >  void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
> >  {
> > call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
> > @@ -674,23 +660,13 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum 
> > vb2_memory memory,
> >  
> > if (*count == 0 || q->num_buffers != 0 ||
> > (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
> > -   /*
> > -* We already have buffers allocated, so first check if they
> > -* are not in use and can be freed.
> > -*/
> > -   mutex_lock(>mmap_lock);
> > -   if (q->memory == VB2_MEMORY_MMAP && __buffers_in_use(q)) {
> > -   mutex_unlock(>mmap_lock);
> > -   dprintk(1, "memory in use, cannot free\n");
> > -   return -EBUSY;
> > -   }
> > -
> > /*
> >  * Call queue_cancel to clean up any buffers in the
> >  * QUEUED state which is possible if buffers were prepared or
> >  * queued without ever calling STREAMON.
> >  */
> > __vb2_queue_cancel(q);
> > +   mutex_lock(>mmap_lock);
> > ret = __vb2_queue_free(q, q->num_buffers);
> > mutex_unlock(>mmap_lock);
> > if (ret)
> > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c 
> > 

Re: [PATCH 4/4] SoC camera: Tidy the header

2018-11-13 Thread Sakari Ailus
Hi Hans, Mauro,

On Wed, Oct 31, 2018 at 07:54:21AM -0300, Mauro Carvalho Chehab wrote:
> Em Wed, 31 Oct 2018 11:00:22 +0100
> Hans Verkuil  escreveu:
> 
> > On 10/31/2018 10:40 AM, Mauro Carvalho Chehab wrote:
> > > Em Wed, 31 Oct 2018 11:29:45 +0200
> > > Sakari Ailus  escreveu:
> > >   
> > >> Hi Mauro,
> > >>
> > >> On Tue, Oct 30, 2018 at 09:06:18AM -0300, Mauro Carvalho Chehab wrote:  
> > >>> Em Tue, 30 Oct 2018 01:00:29 +0200
> > >>> Sakari Ailus  escreveu:
> > >>> 
> >  Clean up the SoC camera framework header. It only exists now to keep 
> >  board
> >  code compiling. The header can be removed once the board code 
> >  dependencies
> >  to it has been removed.
> > 
> >  Signed-off-by: Sakari Ailus 
> >  ---
> >   include/media/soc_camera.h | 335 
> >  -
> >   1 file changed, 335 deletions(-)
> > 
> >  diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> >  index b7e42a1b0910..14d19da6052a 100644
> >  --- a/include/media/soc_camera.h
> >  +++ b/include/media/soc_camera.h
> >  @@ -22,172 +22,6 @@
> >   #include 
> >   #include 
> > >>>
> > >>> That doesn't make any sense. soc_camera.h should have the same fate
> > >>> as the entire soc_camera infrastructure: either be removed or moved
> > >>> to staging, and everything else that doesn't have the same fate
> > >>> should get rid of this header.
> > >>
> > >> We can't just remove this; there is board code that depends on it.
> > >>
> > >> The intent is to remove the board code as well but presuming that the 
> > >> board
> > >> code would be merged through a different tree, it'd be less hassle to 
> > >> wait
> > >> a bit; hence the patch removing any unnecessary stuff here.  
> > > 
> > > Then we need *first* to remove board code, wait for those changes to be
> > > applied and *then* remove soc_camera (and not the opposite).  
> > 
> > Please note that the camera support for all the remaining boards using
> > soc_camera has been dead for years. The soc_camera drivers that they 
> > depended
> > on have been removed a long time ago.
> > 
> > So all they depend on are the header. We can safely remove soc_camera 
> > without
> > loss of functionality (and thus prevent others from basing new drivers on
> > soc_camera), while we work to update the board files so we can remove this 
> > last
> > header.
> > 
> > I have modified some board files here:
> > 
> > https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=rm-soc-camera=d7ae2fcf6e447022f0780bb86a2b63d5c7cf4d35
> 
> Good! the arch-specific mach-* changes should likely be on separate
> patches, though.
> 
> > Only arch/arm/mach-imx/mach-imx27_visstrim_m10.c hasn't been fixed yet in 
> > that
> > patch (ENOTIME).
> 
> So, the code we don't manage seems to be just 3 archs, right (mach-omap1,
> mach-pxa and mach-imx)?
> 
> Btw, the soc_camera dependent code at mach-imx27_visstrim_m10.c is:
> 
>   static struct soc_camera_link iclink_tvp5150 = {
>   .bus_id = 0,
>   .board_info = _i2c_camera,
>   .i2c_adapter_id = 0,
>   .power = visstrim_camera_power,
>   .reset = visstrim_camera_reset,
>   };
> 
> ...
> platform_device_register_resndata(NULL, "soc-camera-pdrv", 0, NULL, 0,
>   _tvp5150, 
> sizeof(iclink_tvp5150));
> 
> 
> As tvp5150 is not actually a soc_camera driver, I suspect that
> the conversion here would be to make it to use the tvp5150 driver
> directly, instead of doing it via soc_camera.

Using board files? The IMX27 SoC has plenty of dts files around which
suggests that it's just this particular board that has not been converted.
I don't think this is a good excuse to keep the board file stuff around.

If someone is willing to add dts files for the board, all the better. But
I'm not going to do that.

> 
> > The problem is just lack of time to clean this up and figure out who should
> > take these board patches.
> 
> No need to rush it.
> 
> > So I think it is a nice solution to just replace the header with a dummy 
> > version
> > so the board files still compile, and then we can delete the dead soc_camera
> > driver. It's probably easier as well to push through the board file changes 
> > once
> > soc_camera is removed since you can just point out that the framework it 
> > depended
> > on is gone.
> 
> I still think that reverting the order and rushing the removal is not the
> way to go.

I think no-one can blame us for rushing the removal of this: it's been
broken for years.

I propose removing the dead code with the least effort while minimising
collateral damage as well as additional maintenance work done to support
the to-be-removed code.

As far as I can see, that involves:

- Removing the SoC camera framework (as my set does; there'll be v2 to fix
  a few minor matters),

- Removing the board files (I can 

Re: [PATCH] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers

2018-11-13 Thread Sakari Ailus
Hi Philipp,

On Tue, Nov 13, 2018 at 04:06:21PM +0100, Philipp Zabel wrote:
> From: John Sheu 
> 
> Videobuf2 presently does not allow VIDIOC_REQBUFS to destroy outstanding
> buffers if the queue is of type V4L2_MEMORY_MMAP, and if the buffers are
> considered "in use".  This is different behavior than for other memory
> types and prevents us from deallocating buffers in following two cases:
> 
> 1) There are outstanding mmap()ed views on the buffer. However even if
>we put the buffer in reqbufs(0), there will be remaining references,
>due to vma .open/close() adjusting vb2 buffer refcount appropriately.
>This means that the buffer will be in fact freed only when the last
>mmap()ed view is unmapped.
> 
> 2) Buffer has been exported as a DMABUF. Refcount of the vb2 buffer
>is managed properly by VB2 DMABUF ops, i.e. incremented on DMABUF
>get and decremented on DMABUF release. This means that the buffer
>will be alive until all importers release it.
> 
> Considering both cases above, there does not seem to be any need to
> prevent reqbufs(0) operation, because buffer lifetime is already
> properly managed by both mmap() and DMABUF code paths. Let's remove it
> and allow userspace freeing the queue (and potentially allocating a new
> one) even though old buffers might be still in processing.
> 
> To let userspace know that the kernel now supports orphaning buffers
> that are still in use, add a new V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS
> to be set by reqbufs and create_bufs.
> 
> Signed-off-by: John Sheu 
> Reviewed-by: Pawel Osciak 
> Reviewed-by: Tomasz Figa 
> Signed-off-by: Tomasz Figa 
> [p.za...@pengutronix.de: moved __vb2_queue_cancel out of the mmap_lock
>  and added V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS]
> Signed-off-by: Philipp Zabel 

This lets the user to allocate lots of mmap'ed buffers that are pinned in
physical memory. Considering that we don't really have a proper mechanism
to limit that anyway,

Acked-by: Sakari Ailus 

That said, the patch must be accompanied by the documentation change in
Documentation/media/uapi/v4l/vidioc-reqbufs.rst .

I wonder what Hans thinks.

> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 26 +--
>  .../media/common/videobuf2/videobuf2-v4l2.c   |  2 +-
>  include/uapi/linux/videodev2.h|  1 +
>  3 files changed, 3 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
> b/drivers/media/common/videobuf2/videobuf2-core.c
> index 975ff5669f72..608459450c1e 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -553,20 +553,6 @@ bool vb2_buffer_in_use(struct vb2_queue *q, struct 
> vb2_buffer *vb)
>  }
>  EXPORT_SYMBOL(vb2_buffer_in_use);
>  
> -/*
> - * __buffers_in_use() - return true if any buffers on the queue are in use 
> and
> - * the queue cannot be freed (by the means of REQBUFS(0)) call
> - */
> -static bool __buffers_in_use(struct vb2_queue *q)
> -{
> - unsigned int buffer;
> - for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> - if (vb2_buffer_in_use(q, q->bufs[buffer]))
> - return true;
> - }
> - return false;
> -}
> -
>  void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
>  {
>   call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
> @@ -674,23 +660,13 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum 
> vb2_memory memory,
>  
>   if (*count == 0 || q->num_buffers != 0 ||
>   (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
> - /*
> -  * We already have buffers allocated, so first check if they
> -  * are not in use and can be freed.
> -  */
> - mutex_lock(>mmap_lock);
> - if (q->memory == VB2_MEMORY_MMAP && __buffers_in_use(q)) {
> - mutex_unlock(>mmap_lock);
> - dprintk(1, "memory in use, cannot free\n");
> - return -EBUSY;
> - }
> -
>   /*
>* Call queue_cancel to clean up any buffers in the
>* QUEUED state which is possible if buffers were prepared or
>* queued without ever calling STREAMON.
>*/
>   __vb2_queue_cancel(q);
> + mutex_lock(>mmap_lock);
>   ret = __vb2_queue_free(q, q->num_buffers);
>   mutex_unlock(>mmap_lock);
>   if (ret)
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c 
> b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index a17033ab2c22..f02d452ceeb9 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -624,7 +624,7 @@ EXPORT_SYMBOL(vb2_querybuf);
>  
>  static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
>  {
> - *caps = 0;
> + *caps = V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
>  

Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-13 Thread Sakari Ailus
On Tue, Nov 13, 2018 at 07:04:01PM +0800, Bing Bu Cao wrote:
> 
> 
> On 11/13/2018 06:31 PM, Sakari Ailus wrote:
> > Hi Bing Bu,
> >
> > On Mon, Nov 12, 2018 at 12:31:16PM +0800, Bing Bu Cao wrote:
> >>
> >> On 11/09/2018 06:09 PM, Sakari Ailus wrote:
> >>> Hi Bing Bu,
> >>>
> >>> On Wed, Nov 07, 2018 at 12:16:47PM +0800, Bing Bu Cao wrote:
>  On 11/01/2018 08:03 PM, Sakari Ailus wrote:
> > Hi Yong,
> >
> > Thanks for the update!
> >
> > On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
> >> Hi,
> >>
> >> This series adds support for the Intel IPU3 (Image Processing Unit)
> >> ImgU which is essentially a modern memory-to-memory ISP. It implements
> >> raw Bayer to YUV image format conversion as well as a large number of
> >> other pixel processing algorithms for improving the image quality.
> >>
> >> Meta data formats are defined for image statistics (3A, i.e. automatic
> >> white balance, exposure and focus, histogram and local area contrast
> >> enhancement) as well as for the pixel processing algorithm parameters.
> >> The documentation for these formats is currently not included in the
> >> patchset but will be added in a future version of this set.
> >>
> >> The algorithm parameters need to be considered specific to a given 
> >> frame
> >> and typically a large number of these parameters change on frame to 
> >> frame
> >> basis. Additionally, the parameters are highly structured (and not a 
> >> flat
> >> space of independent configuration primitives). They also reflect the
> >> data structures used by the firmware and the hardware. On top of that,
> >> the algorithms require highly specialized user space to make meaningful
> >> use of them. For these reasons it has been chosen video buffers to pass
> >> the parameters to the device.
> >>
> >> On individual patches:
> >>
> >> The heart of ImgU is the CSS, or Camera Subsystem, which contains the
> >> image processors and HW accelerators.
> >>
> >> The 3A statistics and other firmware parameter computation related
> >> functions are implemented in patch 11.
> >>
> >> All IPU3 pipeline default settings can be found in patch 10.
> >>
> >> To access DDR via ImgU's own memory space, IPU3 is also equipped with
> >> its own MMU unit, the driver is implemented in patch 6.
> >>
> >> Patch 7 uses above driver for DMA mapping operation.
> >>
> >> The communication between IPU3 firmware and driver is implemented with 
> >> circular
> >> queues in patch 8.
> >>
> >> Patch 9 provide some utility functions and manage IPU3 fw download and
> >> install.
> >>
> >> The firmware which is called ipu3-fw.bin can be downloaded from:
> >>
> >> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
> >> (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)
> >>
> >> Firmware ABI is defined in patches 4 and 5.
> >>
> >> Patches 12 and 13 are of the same file, the former contains all h/w 
> >> programming
> >> related code, the latter implements interface functions for access fw 
> >> & hw
> >> capabilities.
> >>
> >> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:
> >>
> >> https://patchwork.kernel.org/patch/9976295/>
> > I've pushed the latest set here:
> >
> > https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output>
> >
> > You can just say the entire set depends on those going forward; the
> > documentation is needed, too.
> >
> >> Patch 15 represents the top level that glues all of the other 
> >> components together,
> >> passing arguments between the components.
> >>
> >> Patch 16 is a recent effort to extend v6 for advanced camera features 
> >> like
> >> Continuous View Finder (CVF) and Snapshot During Video(SDV) support.
> >>
> >> Link to user space implementation:
> >>
> >> git clone 
> >> https://chromium.googlesource.com/chromiumos/platform/arc-camera
> >>
> >> ImgU media topology print:
> >>
> >> # media-ctl -d /dev/media0 -p
> >> Media controller API version 4.19.0
> >>
> >> Media device information
> >> 
> >> driver  ipu3-imgu
> >> model   ipu3-imgu
> >> serial  
> >> bus infoPCI::00:05.0
> >> hw revision 0x80862015
> >> driver version  4.19.0
> >>
> >> Device topology
> >> - entity 1: ipu3-imgu 0 (5 pads, 5 links)
> >> type V4L2 subdev subtype Unknown flags 0
> >> device node name /dev/v4l-subdev0
> >>pad0: Sink
> >>[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
> > This doesn't seem right. Which formats can be enumerated from the pad?
> >>> Looking at the 

Re: [PATCH 3/7] media: ov2640: add V4L2_CID_TEST_PATTERN control

2018-11-13 Thread Sakari Ailus
On Tue, Nov 13, 2018 at 10:55:46PM +0900, Akinobu Mita wrote:
> 2018年11月13日(火) 19:37 Sakari Ailus :
> >
> > On Tue, Nov 13, 2018 at 01:00:50AM +0900, Akinobu Mita wrote:
> > > The ov2640 has the test pattern generator features.  This makes use of
> > > it through V4L2_CID_TEST_PATTERN control.
> > >
> > > Cc: Sakari Ailus 
> > > Cc: Mauro Carvalho Chehab 
> > > Signed-off-by: Akinobu Mita 
> > > ---
> > >  drivers/media/i2c/ov2640.c | 14 +-
> > >  1 file changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> > > index 20a8853..4992d77 100644
> > > --- a/drivers/media/i2c/ov2640.c
> > > +++ b/drivers/media/i2c/ov2640.c
> > > @@ -705,6 +705,11 @@ static int ov2640_reset(struct i2c_client *client)
> > >   return ret;
> > >  }
> > >
> > > +static const char * const ov2640_test_pattern_menu[] = {
> > > + "Disabled",
> > > + "Color bar",
> >
> > s/b/B/
> >
> > The smiapp driver uses "Eight Vertical Colour Bars", not sure if that'd fit
> > here or not. FYI.
> 
> This test pattern shows eight vertical color bars with blending actual
> sensor image, although the datasheet tells just 'Color bar'.
> 
> So either is fine for me.

I'll use what the smiapp driver does.

Thanks.

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


Re: [PATCH v2] Input: Add missing event codes for common IR remote buttons

2018-11-13 Thread VDR User
> On Sat, 2018-11-03 at 07:55 -0700, Derek Kelly wrote:
> > The following patch adds event codes for common buttons found on
> > various
> > provider and universal remote controls. They represent functions not
> > covered by existing event codes. Once added, rc_keymaps can be
> > updated
> > accordingly where applicable.
>
> Would be great to have more than "those are used", such as knowing how
> they are labeled, both with text and/or icons, and an explanation as to
> why a particular existing key isn't usable.

Hi Bastien,

Text & icons may vary from remote to remote but the purpose/function
of those buttons is basically the same. As explained, the defines in
this patch represent functions not already addressed by other defines.
See below for more detail. The one thing I will add that I probably
should've mentioned is that these defines focus on media/htpc/stb. If
you're not aware, Linux has become a common choice for these types of
systems thanks to the popularity of software like Plex, Kodi, Mythtv,
VDR, etc. Lastly, all these represent *common* functions in this area.
Please keep this in mind as you read further.

> > +/* Remote control buttons found across provider & universal remotes */
> > +#define KEY_LIVE_TV  0x2e8   /* Jump to live tv viewing */
>
> KEY_TV?

KEY_TV selects TV as a *input source* the same as KEY_VCR, KEY_SAT,
KEY_CD, etc. whereas KEY_LIVE_TV jumps directly to live tv as opposed
to local/networked media playback, dvr playback, etc.

> > +#define KEY_OPTIONS  0x2e9   /* Jump to options */
>
> KEY_OPTION?

Software vs. media playback options.

> > +#define KEY_INTERACTIVE  0x2ea   /* Jump to 
> > interactive system/menu/item */
> > +#define KEY_MIC_INPUT0x2eb   /* Trigger MIC 
> > input/listen mode */
>
> KEY_MICMUTE?

This button doesn't mute the mic, in fact it does the opposite. The
mic is off until you press this button, thus triggering MIC
input/listen mode and allowing the user to speak his commands. It
automatically shuts off after X seconds of silence.

> > +#define KEY_SCREEN_INPUT 0x2ec   /* Open on-screen input 
> > system */
>
> KEY_SWITCHVIDEOMODE?

KEY_SWITCHVIDEOMODE is used for "Cycle between available video outputs
(Monitor/LCD/TV-out/etc) ". This is poorly labeled in my opinion and
should've been called KEY_SWITCHVIDEOOUTPUT or something similar.
"Video mode" typically refers to something entirely different - how
video is presented on the display, not what physical display you're
using. KEY_SCREEN_INPUT is used to bring up things like an on-screen
keyboard or other on-onscreen user input method.

> > +#define KEY_SYSTEM_MENU  0x2ed   /* Open systems 
> > menu/display */
>
> KEY_MENU?

Systems menus as pertains to DVB. KEY_MENU is generic and having only
one `menu` option is problematic when you have different types of
menus which aren't accessible from each other.

> > +#define KEY_SERVICES 0x2ee   /* Access services */
> > +#define KEY_DISPLAY_FORMAT   0x2ef   /* Cycle display formats */
>
> KEY_CONTEXT_MENU?

KEY_DISPLAY_FORMAT doesn't open any menus and is used to cycle through
how video is displayed on-screen to the user; full, zoomed,
letterboxed, stretched, etc. KEY_CONTEXT_MENU would be for something
like bringing up a playback menu where you'd set things like
upscaling, deinterlacing, audio mixdown/mixup, etc.

> > +#define KEY_PIP  0x2f0   /* Toggle 
> > Picture-in-Picture on/off */
> > +#define KEY_PIP_SWAP 0x2f1   /* Swap contents between main 
> > view and PIP window */
> > +#define KEY_PIP_POSITION 0x2f2   /* Cycle PIP window position 
> > */
> > +
> >  /* We avoid low common keys in module aliases so they don't get huge. */
> >  #define KEY_MIN_INTERESTING  KEY_MUTE
> >  #define KEY_MAX  0x2ff
>

Hopefully that makes things more clear. This patch helps users map
common (media/htpc/stb) remote control buttons directly to their real
functions as opposed to mapping them to some random unrelated & unused
event, which can be both confusing and problematic on systems where
both remote controls and say bluetooth keyboards are used.

Best regards,
Derek


Re: [PATCH] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers

2018-11-13 Thread Hans Verkuil
Hi Philipp,

On 11/13/18 16:06, Philipp Zabel wrote:
> From: John Sheu 
> 
> Videobuf2 presently does not allow VIDIOC_REQBUFS to destroy outstanding
> buffers if the queue is of type V4L2_MEMORY_MMAP, and if the buffers are
> considered "in use".  This is different behavior than for other memory
> types and prevents us from deallocating buffers in following two cases:
> 
> 1) There are outstanding mmap()ed views on the buffer. However even if
>we put the buffer in reqbufs(0), there will be remaining references,
>due to vma .open/close() adjusting vb2 buffer refcount appropriately.
>This means that the buffer will be in fact freed only when the last
>mmap()ed view is unmapped.
> 
> 2) Buffer has been exported as a DMABUF. Refcount of the vb2 buffer
>is managed properly by VB2 DMABUF ops, i.e. incremented on DMABUF
>get and decremented on DMABUF release. This means that the buffer
>will be alive until all importers release it.
> 
> Considering both cases above, there does not seem to be any need to
> prevent reqbufs(0) operation, because buffer lifetime is already
> properly managed by both mmap() and DMABUF code paths. Let's remove it
> and allow userspace freeing the queue (and potentially allocating a new
> one) even though old buffers might be still in processing.
> 
> To let userspace know that the kernel now supports orphaning buffers
> that are still in use, add a new V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS
> to be set by reqbufs and create_bufs.

Looks good, but I have some questions:

1) does v4l2-compliance together with vivid (easiest to test) still work?
   I don't think I have a proper test for this in v4l2-compliance, but
   I'm not 100% certain. If it fails with this patch, then please provide
   a fix for v4l2-compliance as well.

2) I would like to see a new test in v4l2-compliance for this: i.e. if
   the capability is set, then check that you can call REQBUFS(0) before
   unmapping all buffers. Ditto with dmabuffers.

I said during the media summit that I wanted to be more strict about
requiring compliance tests before adding new features, so you're the
unlucky victim of that :-)

Look for munmap_bufs in v4l2-test-buffers.cpp (the MMAP case). The dmabuf
tests are a bit trickier since I noticed that I never actually close
the dmabuf fds in v4l2-compliance. This will fix that:

diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp 
b/utils/v4l2-compliance/v4l2-test-buffers.cpp
index c59a56d9..03639301 100644
--- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
+++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
@@ -1201,6 +1201,7 @@ int testDmaBuf(struct node *expbuf_node, struct node 
*node, unsigned frame_count
fail_on_test(captureBufs(node, q, m2m_q, frame_count, true));
fail_on_test(node->streamoff(q.g_type()));
fail_on_test(node->streamoff(q.g_type()));
+   exp_q.close_exported_fds();
}
return 0;
 }

What is also missing in testDmaBuf is calling q.reqbufs(node, 0) to free all
buffers, and I never munmap the buffers by calling q.munmap_bufs(node);

In other words, clearly I never wrote proper tests for the behavior of 
mmap()/dmabuf
and REQBUFS(0).

Regards,

Hans

> 
> Signed-off-by: John Sheu 
> Reviewed-by: Pawel Osciak 
> Reviewed-by: Tomasz Figa 
> Signed-off-by: Tomasz Figa 
> [p.za...@pengutronix.de: moved __vb2_queue_cancel out of the mmap_lock
>  and added V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS]
> Signed-off-by: Philipp Zabel 
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 26 +--
>  .../media/common/videobuf2/videobuf2-v4l2.c   |  2 +-
>  include/uapi/linux/videodev2.h|  1 +
>  3 files changed, 3 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
> b/drivers/media/common/videobuf2/videobuf2-core.c
> index 975ff5669f72..608459450c1e 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -553,20 +553,6 @@ bool vb2_buffer_in_use(struct vb2_queue *q, struct 
> vb2_buffer *vb)
>  }
>  EXPORT_SYMBOL(vb2_buffer_in_use);
>  
> -/*
> - * __buffers_in_use() - return true if any buffers on the queue are in use 
> and
> - * the queue cannot be freed (by the means of REQBUFS(0)) call
> - */
> -static bool __buffers_in_use(struct vb2_queue *q)
> -{
> - unsigned int buffer;
> - for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> - if (vb2_buffer_in_use(q, q->bufs[buffer]))
> - return true;
> - }
> - return false;
> -}
> -
>  void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
>  {
>   call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
> @@ -674,23 +660,13 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum 
> vb2_memory memory,
>  
>   if (*count == 0 || q->num_buffers != 0 ||
>   (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
> 

Re: [PATCH] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers

2018-11-13 Thread Philipp Zabel
Sorry, that should have said [PATCH v2].

regards
Philipp


[PATCH 1/2] vb2: add waiting_in_dqbuf flag

2018-11-13 Thread Hans Verkuil
Calling VIDIOC_DQBUF can release the core serialization lock pointed to
by vb2_queue->lock if it has to wait for a new buffer to arrive.

However, if userspace dup()ped the video device filehandle, then it is
possible to read or call DQBUF from two filehandles at the same time.

It is also possible to call REQBUFS from one filehandle while the other
is waiting for a buffer. This will remove all the buffers and reallocate
new ones. Removing all the buffers isn't the problem here (that's already
handled correctly by DQBUF), but the reallocating part is: DQBUF isn't
aware that the buffers have changed.

This is fixed by setting a flag whenever the lock is released while waiting
for a buffer to arrive. And checking the flag where needed so we can return
-EBUSY.

Signed-off-by: Hans Verkuil 
---
 .../media/common/videobuf2/videobuf2-core.c| 18 ++
 include/media/videobuf2-core.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
b/drivers/media/common/videobuf2/videobuf2-core.c
index 03954c13024c..138223af701f 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -672,6 +672,11 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory 
memory,
return -EBUSY;
}
 
+   if (q->waiting_in_dqbuf && *count) {
+   dprintk(1, "another dup()ped fd is waiting for a buffer\n");
+   return -EBUSY;
+   }
+
if (*count == 0 || q->num_buffers != 0 ||
(q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
/*
@@ -1624,6 +1629,11 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, 
int nonblocking)
for (;;) {
int ret;
 
+   if (q->waiting_in_dqbuf) {
+   dprintk(1, "another dup()ped fd is waiting for a 
buffer\n");
+   return -EBUSY;
+   }
+
if (!q->streaming) {
dprintk(1, "streaming off, will not wait for 
buffers\n");
return -EINVAL;
@@ -1651,6 +1661,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, 
int nonblocking)
return -EAGAIN;
}
 
+   q->waiting_in_dqbuf = 1;
/*
 * We are streaming and blocking, wait for another buffer to
 * become ready or for streamoff. Driver's lock is released to
@@ -1671,6 +1682,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, 
int nonblocking)
 * the locks or return an error if one occurred.
 */
call_void_qop(q, wait_finish, q);
+   q->waiting_in_dqbuf = 0;
if (ret) {
dprintk(1, "sleep was interrupted\n");
return ret;
@@ -2547,6 +2559,12 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, 
char __user *data, size_
if (!data)
return -EINVAL;
 
+   if (q->waiting_in_dqbuf) {
+   dprintk(3, "another dup()ped fd is %s\n",
+   read ? "reading" : "writing");
+   return -EBUSY;
+   }
+
/*
 * Initialize emulator on first call.
 */
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index e86981d615ae..613f22910174 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -584,6 +584,7 @@ struct vb2_queue {
unsigned intstart_streaming_called:1;
unsigned interror:1;
unsigned intwaiting_for_buffers:1;
+   unsigned intwaiting_in_dqbuf:1;
unsigned intis_multiplanar:1;
unsigned intis_output:1;
unsigned intcopy_timestamp:1;
-- 
2.19.1



[PATCH 0/2] vb2: fix syzkaller race conditions

2018-11-13 Thread Hans Verkuil
From: Hans Verkuil 

These two patches fix syzkaller race conditions. The basic problem is
the same for both: in some specific cases an ioctl can release the
core serialization mutex, thus allowing another thread to access the
same vb2 queue through a dup()ped filehandle.

This can happen in VIDIOC_DQBUF and read()/write() (this calls dqbuf
internally, so it is really the same problem): if no new buffer is
available the DQBUF ioctl will block and wait for a new
buffer to arrive. Before it waits it releases the serialization lock,
and afterwards it reacquires it. This is intentional, since you do not
want to block other ioctls while waiting for a buffer.

But this means that you need to flag that you are waiting for a buffer
and check the flag in the appropriate places.

The same can happen in the stop_streaming operation: the driver may
have to release the serialization lock while waiting for streaming to
stop (vivid does this). The same approach is used to prevent new
read()s/write()s or QBUF ioctls while it is in stop_streaming.

These flags are always checked/set with the serialization mutex locked.

Regards,

Hans

Hans Verkuil (2):
  vb2: add waiting_in_dqbuf flag
  vb2: don't allow queueing buffers when canceling queue

 .../media/common/videobuf2/videobuf2-core.c   | 32 ++-
 include/media/videobuf2-core.h|  2 ++
 2 files changed, 33 insertions(+), 1 deletion(-)

-- 
2.19.1



[PATCH 2/2] vb2: don't allow queueing buffers when canceling queue

2018-11-13 Thread Hans Verkuil
Calling the stop_streaming op can release the core serialization lock
pointed to by vb2_queue->lock if it has to wait for buffers to finish.
An example of that behavior is the vivid driver.

However, if userspace dup()ped the video device filehandle, then it is
possible to stop streaming on one filehandle and call read/write or
VIDIOC_QBUF from the other.

This is fixed by setting a flag whenever stop_streaming is called and
checking the flag where needed so we can return -EBUSY.

Signed-off-by: Hans Verkuil 
Reported-by: syzbot+736c3aae4af7b50d9...@syzkaller.appspotmail.com
---
 drivers/media/common/videobuf2/videobuf2-core.c | 14 +-
 include/media/videobuf2-core.h  |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
b/drivers/media/common/videobuf2/videobuf2-core.c
index 138223af701f..560577321fe7 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1503,6 +1503,10 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
index, void *pb,
dprintk(1, "fatal error occurred on queue\n");
return -EIO;
}
+   if (q->in_stop_streaming) {
+   dprintk(1, "stop_streaming is called\n");
+   return -EBUSY;
+   }
 
vb = q->bufs[index];
 
@@ -1834,8 +1838,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 * Tell driver to stop all transactions and release all queued
 * buffers.
 */
-   if (q->start_streaming_called)
+   if (q->start_streaming_called) {
+   q->in_stop_streaming = 1;
call_void_qop(q, stop_streaming, q);
+   q->in_stop_streaming = 0;
+   }
 
/*
 * If you see this warning, then the driver isn't cleaning up properly
@@ -2565,6 +2572,11 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, 
char __user *data, size_
return -EBUSY;
}
 
+   if (q->in_stop_streaming) {
+   dprintk(3, "stop_streaming is called\n");
+   return -EBUSY;
+   }
+
/*
 * Initialize emulator on first call.
 */
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 613f22910174..5a3d3ada5940 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -585,6 +585,7 @@ struct vb2_queue {
unsigned interror:1;
unsigned intwaiting_for_buffers:1;
unsigned intwaiting_in_dqbuf:1;
+   unsigned intin_stop_streaming:1;
unsigned intis_multiplanar:1;
unsigned intis_output:1;
unsigned intcopy_timestamp:1;
-- 
2.19.1



[PATCH] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers

2018-11-13 Thread Philipp Zabel
From: John Sheu 

Videobuf2 presently does not allow VIDIOC_REQBUFS to destroy outstanding
buffers if the queue is of type V4L2_MEMORY_MMAP, and if the buffers are
considered "in use".  This is different behavior than for other memory
types and prevents us from deallocating buffers in following two cases:

1) There are outstanding mmap()ed views on the buffer. However even if
   we put the buffer in reqbufs(0), there will be remaining references,
   due to vma .open/close() adjusting vb2 buffer refcount appropriately.
   This means that the buffer will be in fact freed only when the last
   mmap()ed view is unmapped.

2) Buffer has been exported as a DMABUF. Refcount of the vb2 buffer
   is managed properly by VB2 DMABUF ops, i.e. incremented on DMABUF
   get and decremented on DMABUF release. This means that the buffer
   will be alive until all importers release it.

Considering both cases above, there does not seem to be any need to
prevent reqbufs(0) operation, because buffer lifetime is already
properly managed by both mmap() and DMABUF code paths. Let's remove it
and allow userspace freeing the queue (and potentially allocating a new
one) even though old buffers might be still in processing.

To let userspace know that the kernel now supports orphaning buffers
that are still in use, add a new V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS
to be set by reqbufs and create_bufs.

Signed-off-by: John Sheu 
Reviewed-by: Pawel Osciak 
Reviewed-by: Tomasz Figa 
Signed-off-by: Tomasz Figa 
[p.za...@pengutronix.de: moved __vb2_queue_cancel out of the mmap_lock
 and added V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS]
Signed-off-by: Philipp Zabel 
---
 .../media/common/videobuf2/videobuf2-core.c   | 26 +--
 .../media/common/videobuf2/videobuf2-v4l2.c   |  2 +-
 include/uapi/linux/videodev2.h|  1 +
 3 files changed, 3 insertions(+), 26 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
b/drivers/media/common/videobuf2/videobuf2-core.c
index 975ff5669f72..608459450c1e 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -553,20 +553,6 @@ bool vb2_buffer_in_use(struct vb2_queue *q, struct 
vb2_buffer *vb)
 }
 EXPORT_SYMBOL(vb2_buffer_in_use);
 
-/*
- * __buffers_in_use() - return true if any buffers on the queue are in use and
- * the queue cannot be freed (by the means of REQBUFS(0)) call
- */
-static bool __buffers_in_use(struct vb2_queue *q)
-{
-   unsigned int buffer;
-   for (buffer = 0; buffer < q->num_buffers; ++buffer) {
-   if (vb2_buffer_in_use(q, q->bufs[buffer]))
-   return true;
-   }
-   return false;
-}
-
 void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
 {
call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
@@ -674,23 +660,13 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory 
memory,
 
if (*count == 0 || q->num_buffers != 0 ||
(q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
-   /*
-* We already have buffers allocated, so first check if they
-* are not in use and can be freed.
-*/
-   mutex_lock(>mmap_lock);
-   if (q->memory == VB2_MEMORY_MMAP && __buffers_in_use(q)) {
-   mutex_unlock(>mmap_lock);
-   dprintk(1, "memory in use, cannot free\n");
-   return -EBUSY;
-   }
-
/*
 * Call queue_cancel to clean up any buffers in the
 * QUEUED state which is possible if buffers were prepared or
 * queued without ever calling STREAMON.
 */
__vb2_queue_cancel(q);
+   mutex_lock(>mmap_lock);
ret = __vb2_queue_free(q, q->num_buffers);
mutex_unlock(>mmap_lock);
if (ret)
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c 
b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index a17033ab2c22..f02d452ceeb9 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -624,7 +624,7 @@ EXPORT_SYMBOL(vb2_querybuf);
 
 static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
 {
-   *caps = 0;
+   *caps = V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
if (q->io_modes & VB2_MMAP)
*caps |= V4L2_BUF_CAP_SUPPORTS_MMAP;
if (q->io_modes & VB2_USERPTR)
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index c8e8ff810190..2a223835214c 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -879,6 +879,7 @@ struct v4l2_requestbuffers {
 #define V4L2_BUF_CAP_SUPPORTS_USERPTR  (1 << 1)
 #define V4L2_BUF_CAP_SUPPORTS_DMABUF   (1 << 2)
 #define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
+#define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4)
 
 /**
  * 

[PATCH] vb2: vb2_mmap: move lock up

2018-11-13 Thread Hans Verkuil
If a filehandle is dup()ped, then it is possible to close it from one fd
and call mmap from the other. This creates a race condition in vb2_mmap
where it is using queue data that __vb2_queue_free (called from close())
is in the process of releasing.

By moving up the mutex_lock(mmap_lock) in vb2_mmap this race is avoided
since __vb2_queue_free is called with the same mutex locked. So vb2_mmap
now reads consistent buffer data.

Signed-off-by: Hans Verkuil 
Reported-by: syzbot+be93025dd45dccd89...@syzkaller.appspotmail.com
---
 drivers/media/common/videobuf2/videobuf2-core.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
b/drivers/media/common/videobuf2/videobuf2-core.c
index c49c67473408..03954c13024c 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -2120,9 +2120,13 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct 
*vma)
return -EINVAL;
}
}
+
+   mutex_lock(>mmap_lock);
+
if (vb2_fileio_is_active(q)) {
dprintk(1, "mmap: file io in progress\n");
-   return -EBUSY;
+   ret = -EBUSY;
+   goto unlock;
}

/*
@@ -2130,7 +2134,7 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct 
*vma)
 */
ret = __find_plane_by_offset(q, off, , );
if (ret)
-   return ret;
+   goto unlock;

vb = q->bufs[buffer];

@@ -2146,8 +2150,9 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct 
*vma)
return -EINVAL;
}

-   mutex_lock(>mmap_lock);
ret = call_memop(vb, mmap, vb->planes[plane].mem_priv, vma);
+
+unlock:
mutex_unlock(>mmap_lock);
if (ret)
return ret;
-- 
2.19.1



Re: [PATCH 3/7] media: ov2640: add V4L2_CID_TEST_PATTERN control

2018-11-13 Thread Akinobu Mita
2018年11月13日(火) 19:37 Sakari Ailus :
>
> On Tue, Nov 13, 2018 at 01:00:50AM +0900, Akinobu Mita wrote:
> > The ov2640 has the test pattern generator features.  This makes use of
> > it through V4L2_CID_TEST_PATTERN control.
> >
> > Cc: Sakari Ailus 
> > Cc: Mauro Carvalho Chehab 
> > Signed-off-by: Akinobu Mita 
> > ---
> >  drivers/media/i2c/ov2640.c | 14 +-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> > index 20a8853..4992d77 100644
> > --- a/drivers/media/i2c/ov2640.c
> > +++ b/drivers/media/i2c/ov2640.c
> > @@ -705,6 +705,11 @@ static int ov2640_reset(struct i2c_client *client)
> >   return ret;
> >  }
> >
> > +static const char * const ov2640_test_pattern_menu[] = {
> > + "Disabled",
> > + "Color bar",
>
> s/b/B/
>
> The smiapp driver uses "Eight Vertical Colour Bars", not sure if that'd fit
> here or not. FYI.

This test pattern shows eight vertical color bars with blending actual
sensor image, although the datasheet tells just 'Color bar'.

So either is fine for me.


Re: [PATCH v2] Input: Add missing event codes for common IR remote buttons

2018-11-13 Thread Bastien Nocera
On Sat, 2018-11-03 at 07:55 -0700, Derek Kelly wrote:
> The following patch adds event codes for common buttons found on
> various
> provider and universal remote controls. They represent functions not
> covered by existing event codes. Once added, rc_keymaps can be
> updated
> accordingly where applicable.

Would be great to have more than "those are used", such as knowing how
they are labeled, both with text and/or icons, and an explanation as to
why a particular existing key isn't usable.

> v2 changes:
> Renamed KEY_SYSTEM to KEY_SYSTEM_MENU to avoid conflict with powerpc
> KEY_SYSTEM define.
> 
> Signed-off-by: Derek Kelly 
> ---
>  include/uapi/linux/input-event-codes.h | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/uapi/linux/input-event-codes.h
> b/include/uapi/linux/input-event-codes.h
> index 53fbae27b280..a15fd3c944d2 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -689,6 +689,19 @@
>  #define BTN_TRIGGER_HAPPY39  0x2e6
>  #define BTN_TRIGGER_HAPPY40  0x2e7
>  
> +/* Remote control buttons found across provider & universal remotes */
> +#define KEY_LIVE_TV  0x2e8   /* Jump to live tv viewing */

KEY_TV?

> +#define KEY_OPTIONS  0x2e9   /* Jump to options */

KEY_OPTION?

> +#define KEY_INTERACTIVE  0x2ea   /* Jump to interactive 
> system/menu/item */
> +#define KEY_MIC_INPUT0x2eb   /* Trigger MIC 
> input/listen mode */

KEY_MICMUTE?

> +#define KEY_SCREEN_INPUT 0x2ec   /* Open on-screen input system 
> */

KEY_SWITCHVIDEOMODE?

> +#define KEY_SYSTEM_MENU  0x2ed   /* Open systems 
> menu/display */

KEY_MENU?

> +#define KEY_SERVICES 0x2ee   /* Access services */
> +#define KEY_DISPLAY_FORMAT   0x2ef   /* Cycle display formats */

KEY_CONTEXT_MENU?

> +#define KEY_PIP  0x2f0   /* Toggle 
> Picture-in-Picture on/off */
> +#define KEY_PIP_SWAP 0x2f1   /* Swap contents between main 
> view and PIP window */
> +#define KEY_PIP_POSITION 0x2f2   /* Cycle PIP window position */
> +
>  /* We avoid low common keys in module aliases so they don't get huge. */
>  #define KEY_MIN_INTERESTING  KEY_MUTE
>  #define KEY_MAX  0x2ff



[PATCH v5 10/11] media: ov5640: Add 60 fps support

2018-11-13 Thread Maxime Ripard
Now that we have everything in place to compute the clock rate at runtime,
we can enable the 60fps framerate for the mode we tested it with.

Signed-off-by: Maxime Ripard 
---
 drivers/media/i2c/ov5640.c | 20 
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index dd6a07a8a4e5..7fa508f61dc6 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -110,6 +110,7 @@ enum ov5640_mode_id {
 enum ov5640_frame_rate {
OV5640_15_FPS = 0,
OV5640_30_FPS,
+   OV5640_60_FPS,
OV5640_NUM_FRAMERATES,
 };
 
@@ -138,6 +139,7 @@ MODULE_PARM_DESC(virtual_channel,
 static const int ov5640_framerates[] = {
[OV5640_15_FPS] = 15,
[OV5640_30_FPS] = 30,
+   [OV5640_60_FPS] = 60,
 };
 
 /* regulator supplies */
@@ -1562,6 +1564,11 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum 
ov5640_frame_rate fr,
(!nearest && (mode->hact != width || mode->vact != height)))
return NULL;
 
+   /* Only 640x480 can operate at 60fps (for now) */
+   if (fr == OV5640_60_FPS &&
+   !(mode->hact == 640 && mode->vact == 480))
+   return NULL;
+
return mode;
 }
 
@@ -2057,12 +2064,13 @@ static int ov5640_try_frame_interval(struct ov5640_dev 
*sensor,
int i;
 
minfps = ov5640_framerates[OV5640_15_FPS];
-   maxfps = ov5640_framerates[OV5640_30_FPS];
+   maxfps = ov5640_framerates[OV5640_60_FPS];
 
if (fi->numerator == 0) {
fi->denominator = maxfps;
fi->numerator = 1;
-   return OV5640_30_FPS;
+   rate = OV5640_60_FPS;
+   goto find_mode;
}
 
fps = clamp_val(DIV_ROUND_CLOSEST(fi->denominator, fi->numerator),
@@ -2081,6 +2089,7 @@ static int ov5640_try_frame_interval(struct ov5640_dev 
*sensor,
fi->numerator = 1;
fi->denominator = best_fps;
 
+find_mode:
mode = ov5640_find_mode(sensor, rate, width, height, false);
return mode ? rate : -EINVAL;
 }
@@ -2700,8 +2709,11 @@ static int ov5640_s_frame_interval(struct v4l2_subdev 
*sd,
 
frame_rate = ov5640_try_frame_interval(sensor, >interval,
   mode->hact, mode->vact);
-   if (frame_rate < 0)
-   frame_rate = OV5640_15_FPS;
+   if (frame_rate < 0) {
+   /* Always return a valid frame interval value */
+   fi->interval = sensor->frame_interval;
+   goto out;
+   }
 
mode = ov5640_find_mode(sensor, frame_rate, mode->hact,
mode->vact, true);
-- 
2.19.1



[PATCH v5 05/11] media: ov5640: Compute the clock rate at runtime

2018-11-13 Thread Maxime Ripard
The clock rate, while hardcoded until now, is actually a function of the
resolution, framerate and bytes per pixel. Now that we have an algorithm to
adjust our clock rate, we can select it dynamically when we change the
mode.

This changes a bit the clock rate being used, with the following effect:

+--+--+--+--+-+-++---+
| Hact | Vact | Htot | Vtot | FPS | Hardcoded clock | Computed clock | 
Deviation |
+--+--+--+--+-+-++---+
|  640 |  480 | 1896 | 1080 |  15 |5600 |   61430400 | 8.84 %   
 |
|  640 |  480 | 1896 | 1080 |  30 |   11200 |  122860800 | 8.84 %   
 |
| 1024 |  768 | 1896 | 1080 |  15 |5600 |   61430400 | 8.84 %   
 |
| 1024 |  768 | 1896 | 1080 |  30 |   11200 |  122860800 | 8.84 %   
 |
|  320 |  240 | 1896 |  984 |  15 |5600 |   55969920 | 0.05 %   
 |
|  320 |  240 | 1896 |  984 |  30 |   11200 |  111939840 | 0.05 %   
 |
|  176 |  144 | 1896 |  984 |  15 |5600 |   55969920 | 0.05 %   
 |
|  176 |  144 | 1896 |  984 |  30 |   11200 |  111939840 | 0.05 %   
 |
|  720 |  480 | 1896 |  984 |  15 |5600 |   55969920 | 0.05 %   
 |
|  720 |  480 | 1896 |  984 |  30 |   11200 |  111939840 | 0.05 %   
 |
|  720 |  576 | 1896 |  984 |  15 |5600 |   55969920 | 0.05 %   
 |
|  720 |  576 | 1896 |  984 |  30 |   11200 |  111939840 | 0.05 %   
 |
| 1280 |  720 | 1892 |  740 |  15 |4200 |   42002400 | 0.01 %   
 |
| 1280 |  720 | 1892 |  740 |  30 |8400 |   84004800 | 0.01 %   
 |
| 1920 | 1080 | 2500 | 1120 |  15 |8400 |   8400 | 0.00 %   
 |
| 1920 | 1080 | 2500 | 1120 |  30 |   16800 |  16800 | 0.00 %   
 |
| 2592 | 1944 | 2844 | 1944 |  15 |8400 |  165862080 | 49.36 %  
 |
+--+--+--+--+-+-++---+

Only the 640x480, 1024x768 and 2592x1944 modes are significantly affected
by the new formula.

In this case, 640x480 and 1024x768 are actually fixed by this change.
Indeed, the sensor was sending data at, for example, 27.33fps instead of
30fps. This is -9%, which is roughly what we're seeing in the array.
Testing these modes with the new clock setup actually fix that error, and
data are now sent at around 30fps.

2592x1944, on the other hand, is probably due to the fact that this mode
can only be used using MIPI-CSI2, in a two lane mode, and never really
tested with a DVP bus.

Signed-off-by: Maxime Ripard 
---
 drivers/media/i2c/ov5640.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 25613ecf83c5..bcfb2b25a450 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1992,7 +1992,8 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
 * All the formats we support have 16 bits per pixel, seems to require
 * the same rate than YUV, so we can just use 16 bpp all the time.
 */
-   rate = mode->pixel_clock * 16;
+   rate = mode->vtot * mode->htot * 16;
+   rate *= ov5640_framerates[sensor->current_fr];
if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
rate = rate / sensor->ep.bus.mipi_csi2.num_data_lanes;
ret = ov5640_set_mipi_pclk(sensor, rate);
-- 
2.19.1



[PATCH v5 06/11] media: ov5640: Remove pixel clock rates

2018-11-13 Thread Maxime Ripard
The pixel clock rates were introduced to report the initially static clock
rate.

Since this is now handled dynamically, we can remove them entirely.

Signed-off-by: Maxime Ripard 
---
 drivers/media/i2c/ov5640.c | 21 +
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index bcfb2b25a450..e96063c9e352 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -172,7 +172,6 @@ struct ov5640_mode_info {
u32 htot;
u32 vact;
u32 vtot;
-   u32 pixel_clock;
const struct reg_value *reg_data;
u32 reg_data_size;
 };
@@ -696,7 +695,6 @@ static const struct reg_value 
ov5640_setting_15fps_QSXGA_2592_1944[] = {
 /* power-on sensor init reg table */
 static const struct ov5640_mode_info ov5640_mode_init_data = {
0, SUBSAMPLING, 640, 1896, 480, 984,
-   5600,
ov5640_init_setting_30fps_VGA,
ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
 };
@@ -706,91 +704,74 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] 
= {
{
{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 176, 1896, 144, 984,
-2800,
 ov5640_setting_15fps_QCIF_176_144,
 ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 320, 1896, 240, 984,
-2800,
 ov5640_setting_15fps_QVGA_320_240,
 ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 640, 1896, 480, 1080,
-2800,
 ov5640_setting_15fps_VGA_640_480,
 ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 720, 1896, 480, 984,
-2800,
 ov5640_setting_15fps_NTSC_720_480,
 ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 720, 1896, 576, 984,
-2800,
 ov5640_setting_15fps_PAL_720_576,
 ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 1024, 1896, 768, 1080,
-2800,
 ov5640_setting_15fps_XGA_1024_768,
 ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 1280, 1892, 720, 740,
-2100,
 ov5640_setting_15fps_720P_1280_720,
 ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
{OV5640_MODE_1080P_1920_1080, SCALING,
 1920, 2500, 1080, 1120,
-4200,
 ov5640_setting_15fps_1080P_1920_1080,
 ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
{OV5640_MODE_QSXGA_2592_1944, SCALING,
 2592, 2844, 1944, 1968,
-8400,
 ov5640_setting_15fps_QSXGA_2592_1944,
 ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
}, {
{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 176, 1896, 144, 984,
-5600,
 ov5640_setting_30fps_QCIF_176_144,
 ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 320, 1896, 240, 984,
-5600,
 ov5640_setting_30fps_QVGA_320_240,
 ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 640, 1896, 480, 1080,
-5600,
 ov5640_setting_30fps_VGA_640_480,
 ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 720, 1896, 480, 984,
-5600,
 ov5640_setting_30fps_NTSC_720_480,
 ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 720, 1896, 576, 984,
-5600,
 ov5640_setting_30fps_PAL_720_576,
 ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576)},
{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 1024, 1896, 768, 1080,
-5600,
 ov5640_setting_30fps_XGA_1024_768,
 ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768)},
{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 1280, 1892, 720, 740,
-4200,
 ov5640_setting_30fps_720P_1280_720,
 ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
{OV5640_MODE_1080P_1920_1080, 

[PATCH v5 04/11] media: ov5640: Remove redundant register setup

2018-11-13 Thread Maxime Ripard
The MIPI divider is also cleared as part of the clock setup sequence, so we
can remove that code.

Signed-off-by: Maxime Ripard 
---
 drivers/media/i2c/ov5640.c | 10 --
 1 file changed, 10 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 1b295d07aa15..25613ecf83c5 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1403,16 +1403,6 @@ static int ov5640_set_stream_dvp(struct ov5640_dev 
*sensor, bool on)
 */
 
if (on) {
-   /*
-* reset MIPI PCLK/SERCLK divider
-*
-* SC PLL CONTRL1 0
-* - [3..0]:MIPI PCLK/SERCLK divider
-*/
-   ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1, 0x0f, 0);
-   if (ret)
-   return ret;
-
/*
 * configure parallel port control lines polarity
 *
-- 
2.19.1



[PATCH v5 03/11] media: ov5640: Remove redundant defines

2018-11-13 Thread Maxime Ripard
The OV5640_SCLK2X_ROOT_DIVIDER_DEFAULT and OV5640_SCLK_ROOT_DIVIDER_DEFAULT
defines represent exactly the same setup, and are at the same value, than
the more consistent with the rest of the driver OV5640_SCLK2X_ROOT_DIV and
OV5640_SCLK_ROOT_DIV.

Remove them.

Signed-off-by: Maxime Ripard 
---
 drivers/media/i2c/ov5640.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 584e01ea765b..1b295d07aa15 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -94,9 +94,6 @@
 #define OV5640_REG_SDE_CTRL5   0x5585
 #define OV5640_REG_AVG_READOUT 0x56a1
 
-#define OV5640_SCLK2X_ROOT_DIVIDER_DEFAULT 1
-#define OV5640_SCLK_ROOT_DIVIDER_DEFAULT   2
-
 enum ov5640_mode_id {
OV5640_MODE_QCIF_176_144 = 0,
OV5640_MODE_QVGA_320_240,
@@ -2086,8 +2083,8 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
sensor->last_mode = _mode_init_data;
 
ret = ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x3f,
-(ilog2(OV5640_SCLK2X_ROOT_DIVIDER_DEFAULT) << 2) |
-ilog2(OV5640_SCLK_ROOT_DIVIDER_DEFAULT));
+(ilog2(OV5640_SCLK2X_ROOT_DIV) << 2) |
+ilog2(OV5640_SCLK_ROOT_DIV));
if (ret)
return ret;
 
-- 
2.19.1



[PATCH v5 09/11] media: ov5640: Make the FPS clamping / rounding more extendable

2018-11-13 Thread Maxime Ripard
The current code uses an algorithm to clamp the FPS values and round them
to the closest supported one that isn't really allows to be extended to
more than two values.

Rework it a bit to make it much easier to extend the amount of FPS options
we support.

Signed-off-by: Maxime Ripard 
---
 drivers/media/i2c/ov5640.c | 27 +++
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index fc2e03193da6..dd6a07a8a4e5 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -2053,7 +2053,8 @@ static int ov5640_try_frame_interval(struct ov5640_dev 
*sensor,
 {
const struct ov5640_mode_info *mode;
enum ov5640_frame_rate rate = OV5640_30_FPS;
-   u32 minfps, maxfps, fps;
+   int minfps, maxfps, best_fps, fps;
+   int i;
 
minfps = ov5640_framerates[OV5640_15_FPS];
maxfps = ov5640_framerates[OV5640_30_FPS];
@@ -2064,19 +2065,21 @@ static int ov5640_try_frame_interval(struct ov5640_dev 
*sensor,
return OV5640_30_FPS;
}
 
-   fps = DIV_ROUND_CLOSEST(fi->denominator, fi->numerator);
+   fps = clamp_val(DIV_ROUND_CLOSEST(fi->denominator, fi->numerator),
+   minfps, maxfps);
+
+   best_fps = minfps;
+   for (i = 0; i < ARRAY_SIZE(ov5640_framerates); i++) {
+   int curr_fps = ov5640_framerates[i];
+
+   if (abs(curr_fps - fps) < abs(best_fps - fps)) {
+   best_fps = curr_fps;
+   rate = i;
+   }
+   }
 
fi->numerator = 1;
-   if (fps > maxfps)
-   fi->denominator = maxfps;
-   else if (fps < minfps)
-   fi->denominator = minfps;
-   else if (2 * fps >= 2 * minfps + (maxfps - minfps))
-   fi->denominator = maxfps;
-   else
-   fi->denominator = minfps;
-
-   rate = (fi->denominator == minfps) ? OV5640_15_FPS : OV5640_30_FPS;
+   fi->denominator = best_fps;
 
mode = ov5640_find_mode(sensor, rate, width, height, false);
return mode ? rate : -EINVAL;
-- 
2.19.1



[PATCH v5 07/11] media: ov5640: Enhance FPS handling

2018-11-13 Thread Maxime Ripard
Now that we have moved the clock generation logic out of the bytes array,
these arrays are identical between the 15fps and 30fps variants.

Remove the duplicate entries, and convert the code accordingly.

Signed-off-by: Maxime Ripard 
---
 drivers/media/i2c/ov5640.c | 306 +++--
 1 file changed, 51 insertions(+), 255 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index e96063c9e352..be047dd7fbfc 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -340,64 +340,7 @@ static const struct reg_value 
ov5640_init_setting_30fps_VGA[] = {
{0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300},
 };
 
-static const struct reg_value ov5640_setting_30fps_VGA_640_480[] = {
-   {0x3c07, 0x08, 0, 0},
-   {0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-   {0x3814, 0x31, 0, 0},
-   {0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-   {0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
-   {0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-   {0x3810, 0x00, 0, 0},
-   {0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
-   {0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-   {0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
-   {0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x0e, 0, 0},
-   {0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
-   {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-   {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
-   {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-   {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x3503, 0x00, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_15fps_VGA_640_480[] = {
-   {0x3c07, 0x08, 0, 0},
-   {0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-   {0x3814, 0x31, 0, 0},
-   {0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-   {0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
-   {0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-   {0x3810, 0x00, 0, 0},
-   {0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
-   {0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-   {0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
-   {0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
-   {0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
-   {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-   {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
-   {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-   {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_30fps_XGA_1024_768[] = {
-   {0x3c07, 0x08, 0, 0},
-   {0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-   {0x3814, 0x31, 0, 0},
-   {0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-   {0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
-   {0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-   {0x3810, 0x00, 0, 0},
-   {0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
-   {0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-   {0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
-   {0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x0e, 0, 0},
-   {0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
-   {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-   {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
-   {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-   {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x3503, 0x00, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_15fps_XGA_1024_768[] = {
+static const struct reg_value ov5640_setting_VGA_640_480[] = {
{0x3c07, 0x08, 0, 0},
{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
{0x3814, 0x31, 0, 0},
@@ -416,7 +359,7 @@ static const struct reg_value 
ov5640_setting_15fps_XGA_1024_768[] = {
{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_30fps_QVGA_320_240[] = {
+static const struct reg_value ov5640_setting_XGA_1024_768[] = {
{0x3c07, 0x08, 0, 0},
{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
{0x3814, 0x31, 0, 0},
@@ -435,7 +378,7 @@ static const struct reg_value 
ov5640_setting_30fps_QVGA_320_240[] = {
{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_15fps_QVGA_320_240[] = {
+static const 

[PATCH v5 02/11] media: ov5640: Remove the clocks registers initialization

2018-11-13 Thread Maxime Ripard
Part of the hardcoded initialization sequence is to set up the proper clock
dividers. However, this is now done dynamically through proper code and as
such, the static one is now redundant.

Let's remove it.

Signed-off-by: Maxime Ripard 
---
 drivers/media/i2c/ov5640.c | 46 ++
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 8476f85bb8e7..584e01ea765b 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -262,8 +262,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct 
v4l2_ctrl *ctrl)
 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},
-   {0x3034, 0x18, 0, 0}, {0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0},
-   {0x3037, 0x13, 0, 0}, {0x3630, 0x36, 0, 0},
+   {0x3630, 0x36, 0, 0},
{0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
{0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
{0x3715, 0x78, 0, 0}, {0x3717, 0x01, 0, 0}, {0x370b, 0x60, 0, 0},
@@ -346,7 +345,7 @@ static const struct reg_value 
ov5640_init_setting_30fps_VGA[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_VGA_640_480[] = {
-   {0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+   {0x3c07, 0x08, 0, 0},
{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
{0x3814, 0x31, 0, 0},
{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -365,7 +364,7 @@ static const struct reg_value 
ov5640_setting_30fps_VGA_640_480[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_VGA_640_480[] = {
-   {0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+   {0x3c07, 0x08, 0, 0},
{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
{0x3814, 0x31, 0, 0},
{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -384,7 +383,7 @@ static const struct reg_value 
ov5640_setting_15fps_VGA_640_480[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_XGA_1024_768[] = {
-   {0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+   {0x3c07, 0x08, 0, 0},
{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
{0x3814, 0x31, 0, 0},
{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -400,11 +399,10 @@ static const struct reg_value 
ov5640_setting_30fps_XGA_1024_768[] = {
{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x3503, 0x00, 0, 0},
-   {0x3035, 0x12, 0, 0},
 };
 
 static const struct reg_value ov5640_setting_15fps_XGA_1024_768[] = {
-   {0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+   {0x3c07, 0x08, 0, 0},
{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
{0x3814, 0x31, 0, 0},
{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -423,7 +421,7 @@ static const struct reg_value 
ov5640_setting_15fps_XGA_1024_768[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_QVGA_320_240[] = {
-   {0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+   {0x3c07, 0x08, 0, 0},
{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
{0x3814, 0x31, 0, 0},
{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -442,7 +440,7 @@ static const struct reg_value 
ov5640_setting_30fps_QVGA_320_240[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_QVGA_320_240[] = {
-   {0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+   {0x3c07, 0x08, 0, 0},
{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
{0x3814, 0x31, 0, 0},
{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -461,7 +459,7 @@ static const struct reg_value 
ov5640_setting_15fps_QVGA_320_240[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_QCIF_176_144[] = {
-   {0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+   {0x3c07, 0x08, 0, 0},
{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
{0x3814, 0x31, 0, 0},
{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -480,7 +478,7 @@ static const struct reg_value 
ov5640_setting_30fps_QCIF_176_144[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_QCIF_176_144[] = {
-   {0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+   {0x3c07, 0x08, 0, 0},
{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
{0x3814, 0x31, 0, 0},
  

[PATCH v5 08/11] media: ov5640: Make the return rate type more explicit

2018-11-13 Thread Maxime Ripard
In the ov5640_try_frame_interval function, the ret variable actually holds
the frame rate index to use, which is represented by the enum
ov5640_frame_rate in the driver.

Make it more obvious.

Signed-off-by: Maxime Ripard 
---
 drivers/media/i2c/ov5640.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index be047dd7fbfc..fc2e03193da6 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -2052,8 +2052,8 @@ static int ov5640_try_frame_interval(struct ov5640_dev 
*sensor,
 u32 width, u32 height)
 {
const struct ov5640_mode_info *mode;
+   enum ov5640_frame_rate rate = OV5640_30_FPS;
u32 minfps, maxfps, fps;
-   int ret;
 
minfps = ov5640_framerates[OV5640_15_FPS];
maxfps = ov5640_framerates[OV5640_30_FPS];
@@ -2076,10 +2076,10 @@ static int ov5640_try_frame_interval(struct ov5640_dev 
*sensor,
else
fi->denominator = minfps;
 
-   ret = (fi->denominator == minfps) ? OV5640_15_FPS : OV5640_30_FPS;
+   rate = (fi->denominator == minfps) ? OV5640_15_FPS : OV5640_30_FPS;
 
-   mode = ov5640_find_mode(sensor, ret, width, height, false);
-   return mode ? ret : -EINVAL;
+   mode = ov5640_find_mode(sensor, rate, width, height, false);
+   return mode ? rate : -EINVAL;
 }
 
 static int ov5640_get_fmt(struct v4l2_subdev *sd,
-- 
2.19.1



[PATCH v5 01/11] media: ov5640: Adjust the clock based on the expected rate

2018-11-13 Thread Maxime Ripard
The clock structure for the PCLK is quite obscure in the documentation, and
was hardcoded through the bytes array of each and every mode.

This is troublesome, since we cannot adjust it at runtime based on other
parameters (such as the number of bytes per pixel), and we can't support
either framerates that have not been used by the various vendors, since we
don't have the needed initialization sequence.

We can however understand how the clock tree works, and then implement some
functions to derive the various parameters from a given rate. And now that
those parameters are calculated at runtime, we can remove them from the
initialization sequence.

The modes also gained a new parameter which is the clock that they are
running at, from the register writes they were doing, so for now the switch
to the new algorithm should be transparent.

Signed-off-by: Maxime Ripard 
---
 drivers/media/i2c/ov5640.c | 366 -
 1 file changed, 365 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index eaefdb58653b..8476f85bb8e7 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -175,6 +175,7 @@ struct ov5640_mode_info {
u32 htot;
u32 vact;
u32 vtot;
+   u32 pixel_clock;
const struct reg_value *reg_data;
u32 reg_data_size;
 };
@@ -700,6 +701,7 @@ static const struct reg_value 
ov5640_setting_15fps_QSXGA_2592_1944[] = {
 /* power-on sensor init reg table */
 static const struct ov5640_mode_info ov5640_mode_init_data = {
0, SUBSAMPLING, 640, 1896, 480, 984,
+   5600,
ov5640_init_setting_30fps_VGA,
ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
 };
@@ -709,74 +711,91 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] 
= {
{
{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 176, 1896, 144, 984,
+2800,
 ov5640_setting_15fps_QCIF_176_144,
 ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 320, 1896, 240, 984,
+2800,
 ov5640_setting_15fps_QVGA_320_240,
 ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 640, 1896, 480, 1080,
+2800,
 ov5640_setting_15fps_VGA_640_480,
 ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 720, 1896, 480, 984,
+2800,
 ov5640_setting_15fps_NTSC_720_480,
 ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 720, 1896, 576, 984,
+2800,
 ov5640_setting_15fps_PAL_720_576,
 ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 1024, 1896, 768, 1080,
+2800,
 ov5640_setting_15fps_XGA_1024_768,
 ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 1280, 1892, 720, 740,
+2100,
 ov5640_setting_15fps_720P_1280_720,
 ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
{OV5640_MODE_1080P_1920_1080, SCALING,
 1920, 2500, 1080, 1120,
+4200,
 ov5640_setting_15fps_1080P_1920_1080,
 ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
{OV5640_MODE_QSXGA_2592_1944, SCALING,
 2592, 2844, 1944, 1968,
+8400,
 ov5640_setting_15fps_QSXGA_2592_1944,
 ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
}, {
{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 176, 1896, 144, 984,
+5600,
 ov5640_setting_30fps_QCIF_176_144,
 ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 320, 1896, 240, 984,
+5600,
 ov5640_setting_30fps_QVGA_320_240,
 ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 640, 1896, 480, 1080,
+5600,
 ov5640_setting_30fps_VGA_640_480,
 ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 720, 1896, 480, 984,
+5600,
 ov5640_setting_30fps_NTSC_720_480,
 ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
{OV5640_MODE_PAL_720_576, 

[PATCH v5 00/11] media: ov5640: Misc cleanup and improvements

2018-11-13 Thread Maxime Ripard
Hi,

Here is a "small" series that mostly cleans up the ov5640 driver code,
slowly getting rid of the big data array for more understandable code
(hopefully).

The biggest addition would be the clock rate computation at runtime,
instead of relying on those arrays to setup the clock tree
properly. As a side effect, it fixes the framerate that was off by
around 10% on the smaller resolutions, and we now support 60fps.

This also introduces a bunch of new features.

Let me know what you think,
Maxime

Changes from v4:
  - Squashed Jacopo patches fixing the MIPI-CSI case
  - Prefer clock rates superior to the ideal clock rate, even if it
means having a less precise one.
  - Fix the JPEG case according to Hugues suggestions
  - Rebased on 4.20

Changes from v3:
  - Rebased on current Sakari tree
  - Fixed an error when changing only the framerate

Changes from v2:
  - Rebased on latest Sakari PR
  - Fixed the issues reported by Hugues: improper FPS returned for
formats, improper rounding of the FPS, some with his suggestions,
some by simplifying the logic.
  - Expanded the clock tree comments based on the feedback from Samuel
Bobrowicz and Loic Poulain
  - Merged some of the changes made by Samuel Bobrowicz to fix the
MIPI rate computation, fix the call sites of the
ov5640_set_timings function, the auto-exposure calculation call,
etc.
  - Split the patches into smaller ones in order to make it more
readable (hopefully)

Changes from v1:
  - Integrated Hugues' suggestions to fix v4l2-compliance
  - Fixed the bus width with JPEG
  - Dropped the clock rate calculation loops for something simpler as
suggested by Sakari
  - Cache the exposure value instead of using the control value
  - Rebased on top of 4.17

Maxime Ripard (11):
  media: ov5640: Adjust the clock based on the expected rate
  media: ov5640: Remove the clocks registers initialization
  media: ov5640: Remove redundant defines
  media: ov5640: Remove redundant register setup
  media: ov5640: Compute the clock rate at runtime
  media: ov5640: Remove pixel clock rates
  media: ov5640: Enhance FPS handling
  media: ov5640: Make the return rate type more explicit
  media: ov5640: Make the FPS clamping / rounding more extendable
  media: ov5640: Add 60 fps support
  media: ov5640: Remove duplicate auto-exposure setup

 drivers/media/i2c/ov5640.c | 748 ++---
 1 file changed, 445 insertions(+), 303 deletions(-)

-- 
2.19.1



[PATCH v5 11/11] media: ov5640: Remove duplicate auto-exposure setup

2018-11-13 Thread Maxime Ripard
The autoexposure setup in the 1080p init array is redundant with the
default value of the sensor.

Remove it.

Signed-off-by: Maxime Ripard 
---
 drivers/media/i2c/ov5640.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 7fa508f61dc6..0bb5f78571fe 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -504,7 +504,7 @@ static const struct reg_value 
ov5640_setting_1080P_1920_1080[] = {
{0x3a0e, 0x03, 0, 0}, {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x04, 0, 0},
{0x3a15, 0x60, 0, 0}, {0x4713, 0x02, 0, 0}, {0x4407, 0x04, 0, 0},
{0x460b, 0x37, 0, 0}, {0x460c, 0x20, 0, 0}, {0x3824, 0x04, 0, 0},
-   {0x4005, 0x1a, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3503, 0, 0, 0},
+   {0x4005, 0x1a, 0, 0}, {0x3008, 0x02, 0, 0},
 };
 
 static const struct reg_value ov5640_setting_QSXGA_2592_1944[] = {
-- 
2.19.1



Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-13 Thread Bing Bu Cao



On 11/13/2018 06:31 PM, Sakari Ailus wrote:
> Hi Bing Bu,
>
> On Mon, Nov 12, 2018 at 12:31:16PM +0800, Bing Bu Cao wrote:
>>
>> On 11/09/2018 06:09 PM, Sakari Ailus wrote:
>>> Hi Bing Bu,
>>>
>>> On Wed, Nov 07, 2018 at 12:16:47PM +0800, Bing Bu Cao wrote:
 On 11/01/2018 08:03 PM, Sakari Ailus wrote:
> Hi Yong,
>
> Thanks for the update!
>
> On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
>> Hi,
>>
>> This series adds support for the Intel IPU3 (Image Processing Unit)
>> ImgU which is essentially a modern memory-to-memory ISP. It implements
>> raw Bayer to YUV image format conversion as well as a large number of
>> other pixel processing algorithms for improving the image quality.
>>
>> Meta data formats are defined for image statistics (3A, i.e. automatic
>> white balance, exposure and focus, histogram and local area contrast
>> enhancement) as well as for the pixel processing algorithm parameters.
>> The documentation for these formats is currently not included in the
>> patchset but will be added in a future version of this set.
>>
>> The algorithm parameters need to be considered specific to a given frame
>> and typically a large number of these parameters change on frame to frame
>> basis. Additionally, the parameters are highly structured (and not a flat
>> space of independent configuration primitives). They also reflect the
>> data structures used by the firmware and the hardware. On top of that,
>> the algorithms require highly specialized user space to make meaningful
>> use of them. For these reasons it has been chosen video buffers to pass
>> the parameters to the device.
>>
>> On individual patches:
>>
>> The heart of ImgU is the CSS, or Camera Subsystem, which contains the
>> image processors and HW accelerators.
>>
>> The 3A statistics and other firmware parameter computation related
>> functions are implemented in patch 11.
>>
>> All IPU3 pipeline default settings can be found in patch 10.
>>
>> To access DDR via ImgU's own memory space, IPU3 is also equipped with
>> its own MMU unit, the driver is implemented in patch 6.
>>
>> Patch 7 uses above driver for DMA mapping operation.
>>
>> The communication between IPU3 firmware and driver is implemented with 
>> circular
>> queues in patch 8.
>>
>> Patch 9 provide some utility functions and manage IPU3 fw download and
>> install.
>>
>> The firmware which is called ipu3-fw.bin can be downloaded from:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
>> (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)
>>
>> Firmware ABI is defined in patches 4 and 5.
>>
>> Patches 12 and 13 are of the same file, the former contains all h/w 
>> programming
>> related code, the latter implements interface functions for access fw & 
>> hw
>> capabilities.
>>
>> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:
>>
>> https://patchwork.kernel.org/patch/9976295/>
> I've pushed the latest set here:
>
> https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output>
>
> You can just say the entire set depends on those going forward; the
> documentation is needed, too.
>
>> Patch 15 represents the top level that glues all of the other components 
>> together,
>> passing arguments between the components.
>>
>> Patch 16 is a recent effort to extend v6 for advanced camera features 
>> like
>> Continuous View Finder (CVF) and Snapshot During Video(SDV) support.
>>
>> Link to user space implementation:
>>
>> git clone 
>> https://chromium.googlesource.com/chromiumos/platform/arc-camera
>>
>> ImgU media topology print:
>>
>> # media-ctl -d /dev/media0 -p
>> Media controller API version 4.19.0
>>
>> Media device information
>> 
>> driver  ipu3-imgu
>> model   ipu3-imgu
>> serial  
>> bus infoPCI::00:05.0
>> hw revision 0x80862015
>> driver version  4.19.0
>>
>> Device topology
>> - entity 1: ipu3-imgu 0 (5 pads, 5 links)
>> type V4L2 subdev subtype Unknown flags 0
>> device node name /dev/v4l-subdev0
>>  pad0: Sink
>>  [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
> This doesn't seem right. Which formats can be enumerated from the pad?
>>> Looking at the code, the OUTPUT video nodes have 10-bit GRBG (or a variant)
>>> format whereas the CAPTURE video nodes always have NV12. Can you confirm?
>> Hi, Sakari,
>> Yes, I think the pad_fmt should also be changed.
>> Yong, could you add some extra code for this and test? like:
>>
>> static int ipu3_v4l2_node_setup(struct imgu_device *imgu, 

Re: [PATCH 3/7] media: ov2640: add V4L2_CID_TEST_PATTERN control

2018-11-13 Thread Sakari Ailus
On Tue, Nov 13, 2018 at 01:00:50AM +0900, Akinobu Mita wrote:
> The ov2640 has the test pattern generator features.  This makes use of
> it through V4L2_CID_TEST_PATTERN control.
> 
> Cc: Sakari Ailus 
> Cc: Mauro Carvalho Chehab 
> Signed-off-by: Akinobu Mita 
> ---
>  drivers/media/i2c/ov2640.c | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> index 20a8853..4992d77 100644
> --- a/drivers/media/i2c/ov2640.c
> +++ b/drivers/media/i2c/ov2640.c
> @@ -705,6 +705,11 @@ static int ov2640_reset(struct i2c_client *client)
>   return ret;
>  }
>  
> +static const char * const ov2640_test_pattern_menu[] = {
> + "Disabled",
> + "Color bar",

s/b/B/

The smiapp driver uses "Eight Vertical Colour Bars", not sure if that'd fit
here or not. FYI.

> +};
> +
>  /*
>   * functions
>   */
> @@ -740,6 +745,9 @@ static int ov2640_s_ctrl(struct v4l2_ctrl *ctrl)
>   case V4L2_CID_HFLIP:
>   val = ctrl->val ? REG04_HFLIP_IMG : 0x00;
>   return ov2640_mask_set(client, REG04, REG04_HFLIP_IMG, val);
> + case V4L2_CID_TEST_PATTERN:
> + val = ctrl->val ? COM7_COLOR_BAR_TEST : 0x00;
> + return ov2640_mask_set(client, COM7, COM7_COLOR_BAR_TEST, val);
>   }
>  
>   return -EINVAL;
> @@ -1184,12 +1192,16 @@ static int ov2640_probe(struct i2c_client *client,
>   v4l2_i2c_subdev_init(>subdev, client, _subdev_ops);
>   priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>   mutex_init(>lock);
> - v4l2_ctrl_handler_init(>hdl, 2);
> + v4l2_ctrl_handler_init(>hdl, 3);
>   priv->hdl.lock = >lock;
>   v4l2_ctrl_new_std(>hdl, _ctrl_ops,
>   V4L2_CID_VFLIP, 0, 1, 1, 0);
>   v4l2_ctrl_new_std(>hdl, _ctrl_ops,
>   V4L2_CID_HFLIP, 0, 1, 1, 0);
> + v4l2_ctrl_new_std_menu_items(>hdl, _ctrl_ops,
> + V4L2_CID_TEST_PATTERN,
> + ARRAY_SIZE(ov2640_test_pattern_menu) - 1, 0, 0,
> + ov2640_test_pattern_menu);
>   priv->subdev.ctrl_handler = >hdl;
>   if (priv->hdl.error) {
>   ret = priv->hdl.error;
> -- 
> 2.7.4
> 

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


Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-13 Thread Sakari Ailus
Hi Bing Bu,

On Mon, Nov 12, 2018 at 12:31:16PM +0800, Bing Bu Cao wrote:
> 
> 
> On 11/09/2018 06:09 PM, Sakari Ailus wrote:
> > Hi Bing Bu,
> >
> > On Wed, Nov 07, 2018 at 12:16:47PM +0800, Bing Bu Cao wrote:
> >> On 11/01/2018 08:03 PM, Sakari Ailus wrote:
> >>> Hi Yong,
> >>>
> >>> Thanks for the update!
> >>>
> >>> On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
>  Hi,
> 
>  This series adds support for the Intel IPU3 (Image Processing Unit)
>  ImgU which is essentially a modern memory-to-memory ISP. It implements
>  raw Bayer to YUV image format conversion as well as a large number of
>  other pixel processing algorithms for improving the image quality.
> 
>  Meta data formats are defined for image statistics (3A, i.e. automatic
>  white balance, exposure and focus, histogram and local area contrast
>  enhancement) as well as for the pixel processing algorithm parameters.
>  The documentation for these formats is currently not included in the
>  patchset but will be added in a future version of this set.
> 
>  The algorithm parameters need to be considered specific to a given frame
>  and typically a large number of these parameters change on frame to frame
>  basis. Additionally, the parameters are highly structured (and not a flat
>  space of independent configuration primitives). They also reflect the
>  data structures used by the firmware and the hardware. On top of that,
>  the algorithms require highly specialized user space to make meaningful
>  use of them. For these reasons it has been chosen video buffers to pass
>  the parameters to the device.
> 
>  On individual patches:
> 
>  The heart of ImgU is the CSS, or Camera Subsystem, which contains the
>  image processors and HW accelerators.
> 
>  The 3A statistics and other firmware parameter computation related
>  functions are implemented in patch 11.
> 
>  All IPU3 pipeline default settings can be found in patch 10.
> 
>  To access DDR via ImgU's own memory space, IPU3 is also equipped with
>  its own MMU unit, the driver is implemented in patch 6.
> 
>  Patch 7 uses above driver for DMA mapping operation.
> 
>  The communication between IPU3 firmware and driver is implemented with 
>  circular
>  queues in patch 8.
> 
>  Patch 9 provide some utility functions and manage IPU3 fw download and
>  install.
> 
>  The firmware which is called ipu3-fw.bin can be downloaded from:
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
>  (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)
> 
>  Firmware ABI is defined in patches 4 and 5.
> 
>  Patches 12 and 13 are of the same file, the former contains all h/w 
>  programming
>  related code, the latter implements interface functions for access fw & 
>  hw
>  capabilities.
> 
>  Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:
> 
>  https://patchwork.kernel.org/patch/9976295/>
> >>> I've pushed the latest set here:
> >>>
> >>> https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output>
> >>>
> >>> You can just say the entire set depends on those going forward; the
> >>> documentation is needed, too.
> >>>
>  Patch 15 represents the top level that glues all of the other components 
>  together,
>  passing arguments between the components.
> 
>  Patch 16 is a recent effort to extend v6 for advanced camera features 
>  like
>  Continuous View Finder (CVF) and Snapshot During Video(SDV) support.
> 
>  Link to user space implementation:
> 
>  git clone 
>  https://chromium.googlesource.com/chromiumos/platform/arc-camera
> 
>  ImgU media topology print:
> 
>  # media-ctl -d /dev/media0 -p
>  Media controller API version 4.19.0
> 
>  Media device information
>  
>  driver  ipu3-imgu
>  model   ipu3-imgu
>  serial  
>  bus infoPCI::00:05.0
>  hw revision 0x80862015
>  driver version  4.19.0
> 
>  Device topology
>  - entity 1: ipu3-imgu 0 (5 pads, 5 links)
>  type V4L2 subdev subtype Unknown flags 0
>  device node name /dev/v4l-subdev0
>   pad0: Sink
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
> >>> This doesn't seem right. Which formats can be enumerated from the pad?
> > Looking at the code, the OUTPUT video nodes have 10-bit GRBG (or a variant)
> > format whereas the CAPTURE video nodes always have NV12. Can you confirm?
> Hi, Sakari,
> Yes, I think the pad_fmt should also be changed.
> Yong, could you add some extra code for this and test? like:
> 
> static int ipu3_v4l2_node_setup(struct imgu_device *imgu, unsigned int pipe,
> ...
>     

[PATCH 8/9] vicodec: add tag support

2018-11-13 Thread Hans Verkuil
From: Hans Verkuil 

Copy tags in vicodec.

Signed-off-by: Hans Verkuil 
---
 drivers/media/platform/vicodec/vicodec-core.c | 14 +++---
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/vicodec/vicodec-core.c 
b/drivers/media/platform/vicodec/vicodec-core.c
index b292cff26c86..72245183b077 100644
--- a/drivers/media/platform/vicodec/vicodec-core.c
+++ b/drivers/media/platform/vicodec/vicodec-core.c
@@ -190,18 +190,8 @@ static int device_process(struct vicodec_ctx *ctx,
}
 
out_vb->sequence = q_cap->sequence++;
-   out_vb->vb2_buf.timestamp = in_vb->vb2_buf.timestamp;
-
-   if (in_vb->flags & V4L2_BUF_FLAG_TIMECODE)
-   out_vb->timecode = in_vb->timecode;
-   out_vb->field = in_vb->field;
out_vb->flags &= ~V4L2_BUF_FLAG_LAST;
-   out_vb->flags |= in_vb->flags &
-   (V4L2_BUF_FLAG_TIMECODE |
-V4L2_BUF_FLAG_KEYFRAME |
-V4L2_BUF_FLAG_PFRAME |
-V4L2_BUF_FLAG_BFRAME |
-V4L2_BUF_FLAG_TSTAMP_SRC_MASK);
+   v4l2_m2m_buf_copy_data(in_vb, out_vb, !ctx->is_enc);
 
return 0;
 }
@@ -1066,6 +1056,7 @@ static int queue_init(void *priv, struct vb2_queue 
*src_vq,
src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
src_vq->lock = ctx->is_enc ? >dev->enc_mutex :
>dev->dec_mutex;
+   src_vq->supports_tags = true;
 
ret = vb2_queue_init(src_vq);
if (ret)
@@ -1081,6 +1072,7 @@ static int queue_init(void *priv, struct vb2_queue 
*src_vq,
dst_vq->mem_ops = _vmalloc_memops;
dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
dst_vq->lock = src_vq->lock;
+   dst_vq->supports_tags = true;
 
return vb2_queue_init(dst_vq);
 }
-- 
2.19.1



[PATCH 2/9] vb2: add tag support

2018-11-13 Thread Hans Verkuil
From: Hans Verkuil 

Add support for tags to vb2. Besides just storing and setting
the tag this patch also adds the vb2_find_tag() function that
can be used to find a buffer with the given tag.

This function will only look at DEQUEUED and DONE buffers, i.e.
buffers that are already processed.

Signed-off-by: Hans Verkuil 
---
 .../media/common/videobuf2/videobuf2-v4l2.c   | 43 ---
 include/media/videobuf2-v4l2.h| 21 -
 2 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c 
b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index a17033ab2c22..36781d2367a9 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -50,7 +50,8 @@ module_param(debug, int, 0644);
 V4L2_BUF_FLAG_TIMESTAMP_MASK)
 /* Output buffer flags that should be passed on to the driver */
 #define V4L2_BUFFER_OUT_FLAGS  (V4L2_BUF_FLAG_PFRAME | V4L2_BUF_FLAG_BFRAME | \
-V4L2_BUF_FLAG_KEYFRAME | 
V4L2_BUF_FLAG_TIMECODE)
+V4L2_BUF_FLAG_KEYFRAME | \
+V4L2_BUF_FLAG_TIMECODE | V4L2_BUF_FLAG_TAG)
 
 /*
  * __verify_planes_array() - verify that the planes array passed in struct
@@ -144,8 +145,11 @@ static void __copy_timestamp(struct vb2_buffer *vb, const 
void *pb)
 */
if (q->copy_timestamp)
vb->timestamp = timeval_to_ns(>timestamp);
-   vbuf->flags |= b->flags & V4L2_BUF_FLAG_TIMECODE;
-   if (b->flags & V4L2_BUF_FLAG_TIMECODE)
+   vbuf->flags |= b->flags &
+   (V4L2_BUF_FLAG_TIMECODE | V4L2_BUF_FLAG_TAG);
+   if (b->flags & V4L2_BUF_FLAG_TAG)
+   vbuf->tag = v4l2_buffer_get_tag(b);
+   else if (b->flags & V4L2_BUF_FLAG_TIMECODE)
vbuf->timecode = b->timecode;
}
 };
@@ -195,6 +199,7 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, 
struct v4l2_buffer *b
}
vbuf->sequence = 0;
vbuf->request_fd = -1;
+   vbuf->tag = 0;
 
if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
switch (b->memory) {
@@ -314,13 +319,19 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer 
*vb, struct v4l2_buffer *b
}
 
if (V4L2_TYPE_IS_OUTPUT(b->type)) {
+   if ((b->flags & V4L2_BUF_FLAG_TIMECODE) &&
+   (b->flags & V4L2_BUF_FLAG_TAG)) {
+   dprintk(1, "buffer flag TIMECODE cannot be combined 
with flag TAG\n");
+   return -EINVAL;
+   }
+
/*
 * For output buffers mask out the timecode flag:
 * this will be handled later in vb2_qbuf().
 * The 'field' is valid metadata for this output buffer
 * and so that needs to be copied here.
 */
-   vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE;
+   vbuf->flags &= ~(V4L2_BUF_FLAG_TIMECODE | V4L2_BUF_FLAG_TAG);
vbuf->field = b->field;
} else {
/* Zero any output buffer flags as this is a capture buffer */
@@ -461,7 +472,10 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void 
*pb)
b->flags = vbuf->flags;
b->field = vbuf->field;
b->timestamp = ns_to_timeval(vb->timestamp);
-   b->timecode = vbuf->timecode;
+   if (b->flags & V4L2_BUF_FLAG_TAG)
+   v4l2_buffer_set_tag(b, vbuf->tag);
+   else
+   b->timecode = vbuf->timecode;
b->sequence = vbuf->sequence;
b->reserved2 = 0;
b->request_fd = 0;
@@ -587,6 +601,25 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
.copy_timestamp = __copy_timestamp,
 };
 
+int vb2_find_tag(const struct vb2_queue *q, u64 tag,
+unsigned int start_idx)
+{
+   unsigned int i;
+
+   for (i = start_idx; i < q->num_buffers; i++) {
+   struct vb2_buffer *vb = q->bufs[i];
+   struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+
+   if ((vb->state == VB2_BUF_STATE_DEQUEUED ||
+vb->state == VB2_BUF_STATE_DONE) &&
+   (vbuf->flags & V4L2_BUF_FLAG_TAG) &&
+   vbuf->tag == tag)
+   return i;
+   }
+   return -1;
+}
+EXPORT_SYMBOL_GPL(vb2_find_tag);
+
 /*
  * vb2_querybuf() - query video buffer information
  * @q: videobuf queue
diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
index 727855463838..60ff7348a2b6 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -31,8 +31,9 @@
  * @field: field order of the image in the buffer, as defined by
  *  v4l2_field.
  * @timecode:  frame timecode.
+ * @tag:   user specified buffer tag 

[PATCH 4/9] buffer.rst: document the new buffer tag feature.

2018-11-13 Thread Hans Verkuil
Document V4L2_BUF_FLAG_TAG and struct v4l2_buffer_tag.

Signed-off-by: Hans Verkuil 
---
 Documentation/media/uapi/v4l/buffer.rst   | 61 ---
 .../media/uapi/v4l/vidioc-reqbufs.rst |  4 ++
 2 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/Documentation/media/uapi/v4l/buffer.rst 
b/Documentation/media/uapi/v4l/buffer.rst
index 2e266d32470a..8160631bb9d9 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -220,17 +220,29 @@ struct v4l2_buffer
use ``V4L2_BUF_FLAG_TIMESTAMP_COPY`` the application has to fill
in the timestamp which will be copied by the driver to the capture
stream.
-* - struct :c:type:`v4l2_timecode`
+* - union
+* -
+  - struct :c:type:`v4l2_timecode`
   - ``timecode``
-  -
-  - When ``type`` is ``V4L2_BUF_TYPE_VIDEO_CAPTURE`` and the
-   ``V4L2_BUF_FLAG_TIMECODE`` flag is set in ``flags``, this
+  - When the ``V4L2_BUF_FLAG_TIMECODE`` flag is set in ``flags``, this
structure contains a frame timecode. In
:c:type:`V4L2_FIELD_ALTERNATE ` mode the top and
bottom field contain the same timecode. Timecodes are intended to
help video editing and are typically recorded on video tapes, but
also embedded in compressed formats like MPEG. This field is
independent of the ``timestamp`` and ``sequence`` fields.
+* -
+  - struct :c:type:`v4l2_buffer_tag`
+  - ``tag``
+  - When the ``V4L2_BUF_FLAG_TAG`` flag is set in ``flags``, this
+   structure contains a user-specified 64-bit tag value. It can be
+   set with the helper functions :c:func:`v4l2_buffer_set_tag()` or
+   :c:func:`v4l2_buffer_set_tag_ptr()`, and it can be retrieved with
+   the helper functions :c:func:`v4l2_buffer_get_tag()` or
+:c:func:`v4l2_buffer_get_tag_ptr()`.
+
+   It is used by stateless codecs where this tag can be used to
+   refer to buffers that contain reference frames.
 * - __u32
   - ``sequence``
   -
@@ -567,6 +579,14 @@ Buffer Flags
when the ``VIDIOC_DQBUF`` ioctl is called. Applications can set
this bit and the corresponding ``timecode`` structure when
``type`` refers to an output stream.
+* .. _`V4L2-BUF-FLAG-TAG`:
+
+  - ``V4L2_BUF_FLAG_TAG``
+  - 0x0200
+  - The ``tag`` field is valid. Applications can set
+   this bit and the corresponding ``tag`` structure. If tags are
+   supported then the ``V4L2_BUF_CAP_SUPPORTS_TAGS`` capability
+   is also set.
 * .. _`V4L2-BUF-FLAG-PREPARED`:
 
   - ``V4L2_BUF_FLAG_PREPARED``
@@ -704,10 +724,10 @@ enum v4l2_memory
 Timecodes
 =
 
-The struct :c:type:`v4l2_timecode` structure is designed to hold a
-:ref:`smpte12m` or similar timecode. (struct
-struct :c:type:`timeval` timestamps are stored in struct
-:c:type:`v4l2_buffer` field ``timestamp``.)
+The :c:type:`v4l2_buffer_tag` structure is designed to hold a
+:ref:`smpte12m` or similar timecode.
+(struct :c:type:`timeval` timestamps are stored in the struct
+:c:type:`v4l2_buffer` ``timestamp`` field.)
 
 
 .. c:type:: v4l2_timecode
@@ -807,3 +827,28 @@ Timecode Flags
 * - ``V4L2_TC_USERBITS_8BITCHARS``
   - 0x0008
   - 8-bit ISO characters.
+
+Tags
+
+
+The :c:type:`v4l2_buffer_tag` structure is designed to hold a
+64 bit tag.
+
+.. c:type:: v4l2_buffer_tag
+
+struct v4l2_buffer_tag
+--
+
+.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
+
+.. flat-table::
+:header-rows:  0
+:stub-columns: 0
+:widths:   1 1 2
+
+* - __u32
+  - ``low``
+  - Low 32 bits of the 64 bit tag value.
+* - __u32
+  - ``high``
+  - High 32 bits of the 64 bit tag value.
diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst 
b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
index d40c60e8..5090a62f324c 100644
--- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
+++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
@@ -112,6 +112,7 @@ any DMA in progress, an implicit
 .. _V4L2-BUF-CAP-SUPPORTS-USERPTR:
 .. _V4L2-BUF-CAP-SUPPORTS-DMABUF:
 .. _V4L2-BUF-CAP-SUPPORTS-REQUESTS:
+.. _V4L2-BUF-CAP-SUPPORTS-TAGS:
 
 .. cssclass:: longtable
 
@@ -132,6 +133,9 @@ any DMA in progress, an implicit
 * - ``V4L2_BUF_CAP_SUPPORTS_REQUESTS``
   - 0x0008
   - This buffer type supports :ref:`requests `.
+* - ``V4L2_BUF_CAP_SUPPORTS_TAGS``
+  - 0x0010
+  - This buffer type supports ``V4L2_BUF_FLAG_TAG``.
 
 Return Value
 
-- 
2.19.1



[PATCH 5/9] v4l2-mem2mem: add v4l2_m2m_buf_copy_data helper function

2018-11-13 Thread Hans Verkuil
Memory-to-memory devices should copy various parts of
struct v4l2_buffer from the output buffer to the capture buffer.

Add a helper function that does that to simplify the driver code.

Signed-off-by: Hans Verkuil 
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 23 +++
 include/media/v4l2-mem2mem.h   | 21 +
 2 files changed, 44 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c 
b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 1ed2465972ac..bfd5321fda1c 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -953,6 +953,29 @@ void v4l2_m2m_buf_queue(struct v4l2_m2m_ctx *m2m_ctx,
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_buf_queue);
 
+void v4l2_m2m_buf_copy_data(const struct vb2_v4l2_buffer *out_vb,
+   struct vb2_v4l2_buffer *cap_vb,
+   bool copy_frame_flags)
+{
+   u32 mask = V4L2_BUF_FLAG_TIMECODE | V4L2_BUF_FLAG_TAG |
+  V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
+
+   if (copy_frame_flags)
+   mask |= V4L2_BUF_FLAG_KEYFRAME | V4L2_BUF_FLAG_PFRAME |
+   V4L2_BUF_FLAG_BFRAME;
+
+   cap_vb->vb2_buf.timestamp = out_vb->vb2_buf.timestamp;
+
+   if (out_vb->flags & V4L2_BUF_FLAG_TAG)
+   cap_vb->tag = out_vb->tag;
+   if (out_vb->flags & V4L2_BUF_FLAG_TIMECODE)
+   cap_vb->timecode = out_vb->timecode;
+   cap_vb->field = out_vb->field;
+   cap_vb->flags &= ~mask;
+   cap_vb->flags |= out_vb->flags & mask;
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_buf_copy_data);
+
 void v4l2_m2m_request_queue(struct media_request *req)
 {
struct media_request_object *obj, *obj_safe;
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index 5467264771ec..35716af7ccbc 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -622,6 +622,27 @@ v4l2_m2m_dst_buf_remove_by_idx(struct v4l2_m2m_ctx 
*m2m_ctx, unsigned int idx)
return v4l2_m2m_buf_remove_by_idx(_ctx->cap_q_ctx, idx);
 }
 
+/**
+ * v4l2_m2m_buf_copy_data() - copy buffer data from the output buffer to the
+ * capture buffer
+ *
+ * @out_vb: the output buffer that is the source of the data.
+ * @cap_vb: the capture buffer that will receive the data.
+ * @copy_frame_flags: copy the KEY/B/PFRAME flags as well.
+ *
+ * This helper function copies the timestamp, timecode (if the TIMECODE
+ * buffer flag was set), tag (if the TAG buffer flag was set), field
+ * and the TIMECODE, TAG, KEYFRAME, BFRAME, PFRAME and TSTAMP_SRC_MASK
+ * flags from @out_vb to @cap_vb.
+ *
+ * If @copy_frame_flags is false, then the KEYFRAME, BFRAME and PFRAME
+ * flags are not copied. This is typically needed for encoders that
+ * set this bits explicitly.
+ */
+void v4l2_m2m_buf_copy_data(const struct vb2_v4l2_buffer *out_vb,
+   struct vb2_v4l2_buffer *cap_vbi,
+   bool copy_frame_flags);
+
 /* v4l2 request helper */
 
 void v4l2_m2m_request_queue(struct media_request *req);
-- 
2.19.1



[PATCH 0/9] vb2/cedrus: add tag support

2018-11-13 Thread Hans Verkuil
As was discussed here (among other places):

https://lkml.org/lkml/2018/10/19/440

using capture queue buffer indices to refer to reference frames is
not a good idea. A better idea is to use a 'tag' where the
application can assign a u64 tag to an output buffer, which is then 
copied to the capture buffer(s) derived from the output buffer.

A u64 is chosen since this allows userspace to also use pointers to
internal structures as 'tag'.

The first three patches add core tag support, the next patch document
the tag support, then a new helper function is added to v4l2-mem2mem.c
to easily copy data from a source to a destination buffer that drivers
can use.

Next a new supports_tags vb2_queue flag is added to indicate that
the driver supports tags. Ideally this should not be necessary, but
that would require that all m2m drivers are converted to using the
new helper function introduced in the previous patch. That takes more
time then I have now since we want to get this in for 4.20.

Finally the vim2m, vicodec and cedrus drivers are converted to support
tags.

I also removed the 'pad' fields from the mpeg2 control structs (it
should never been added in the first place) and aligned the structs
to a u32 boundary (u64 for the tag values).

Note that this might change further (Paul suggested using bitfields).

Also note that the cedrus code doesn't set the sequence counter, that's
something that should still be added before this driver can be moved
out of staging.

Note: if no buffer is found for a certain tag, then the dma address
is just set to 0. That happened before as well with invalid buffer
indices. This should be checked in the driver!

The previous RFC series was tested successfully with the cedrus driver.

Regards,

Hans

Main changes since the RFC:

- Added new buffer capability flag
- Added m2m helper to copy data between buffers
- Added documentation
- Added tag logging in v4l2-ioctl.c

Hans Verkuil (9):
  videodev2.h: add tag support
  vb2: add tag support
  v4l2-ioctl.c: log v4l2_buffer tag
  buffer.rst: document the new buffer tag feature.
  v4l2-mem2mem: add v4l2_m2m_buf_copy_data helper function
  vb2: add new supports_tags queue flag
  vim2m: add tag support
  vicodec: add tag support
  cedrus: add tag support

 Documentation/media/uapi/v4l/buffer.rst   | 61 ---
 .../media/uapi/v4l/vidioc-reqbufs.rst |  4 ++
 .../media/common/videobuf2/videobuf2-v4l2.c   | 45 --
 drivers/media/platform/vicodec/vicodec-core.c | 14 +
 drivers/media/platform/vim2m.c| 14 +
 drivers/media/v4l2-core/v4l2-ctrls.c  |  9 ---
 drivers/media/v4l2-core/v4l2-ioctl.c  |  9 ++-
 drivers/media/v4l2-core/v4l2-mem2mem.c| 23 +++
 drivers/staging/media/sunxi/cedrus/cedrus.h   |  9 ++-
 .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 +
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 +++
 .../staging/media/sunxi/cedrus/cedrus_video.c |  2 +
 include/media/v4l2-mem2mem.h  | 21 +++
 include/media/videobuf2-core.h|  2 +
 include/media/videobuf2-v4l2.h| 21 ++-
 include/uapi/linux/v4l2-controls.h| 14 ++---
 include/uapi/linux/videodev2.h| 38 +++-
 17 files changed, 236 insertions(+), 73 deletions(-)

-- 
2.19.1



[PATCH 7/9] vim2m: add tag support

2018-11-13 Thread Hans Verkuil
From: Hans Verkuil 

Copy tags in vim2m.

Signed-off-by: Hans Verkuil 
---
 drivers/media/platform/vim2m.c | 14 +++---
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index d82db738f174..9d1222f489b8 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -241,17 +241,7 @@ static int device_process(struct vim2m_ctx *ctx,
out_vb->sequence =
get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE)->sequence++;
in_vb->sequence = q_data->sequence++;
-   out_vb->vb2_buf.timestamp = in_vb->vb2_buf.timestamp;
-
-   if (in_vb->flags & V4L2_BUF_FLAG_TIMECODE)
-   out_vb->timecode = in_vb->timecode;
-   out_vb->field = in_vb->field;
-   out_vb->flags = in_vb->flags &
-   (V4L2_BUF_FLAG_TIMECODE |
-V4L2_BUF_FLAG_KEYFRAME |
-V4L2_BUF_FLAG_PFRAME |
-V4L2_BUF_FLAG_BFRAME |
-V4L2_BUF_FLAG_TSTAMP_SRC_MASK);
+   v4l2_m2m_buf_copy_data(out_vb, in_vb, true);
 
switch (ctx->mode) {
case MEM2MEM_HFLIP | MEM2MEM_VFLIP:
@@ -856,6 +846,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, 
struct vb2_queue *ds
src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
src_vq->lock = >dev->dev_mutex;
src_vq->supports_requests = true;
+   src_vq->supports_tags = true;
 
ret = vb2_queue_init(src_vq);
if (ret)
@@ -869,6 +860,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, 
struct vb2_queue *ds
dst_vq->mem_ops = _vmalloc_memops;
dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
dst_vq->lock = >dev->dev_mutex;
+   dst_vq->supports_tags = true;
 
return vb2_queue_init(dst_vq);
 }
-- 
2.19.1



[PATCH 9/9] cedrus: add tag support

2018-11-13 Thread Hans Verkuil
Replace old reference frame indices by new tag method.

Signed-off-by: Hans Verkuil 
---
 drivers/media/v4l2-core/v4l2-ctrls.c  |  9 
 drivers/staging/media/sunxi/cedrus/cedrus.h   |  9 +---
 .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 ++
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 ---
 .../staging/media/sunxi/cedrus/cedrus_video.c |  2 ++
 include/uapi/linux/v4l2-controls.h| 14 +
 6 files changed, 24 insertions(+), 33 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 5f2b033a7a42..b854cceb19dc 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1660,15 +1660,6 @@ static int std_validate(const struct v4l2_ctrl *ctrl, 
u32 idx,
return -EINVAL;
}
 
-   if (p_mpeg2_slice_params->backward_ref_index >= VIDEO_MAX_FRAME 
||
-   p_mpeg2_slice_params->forward_ref_index >= VIDEO_MAX_FRAME)
-   return -EINVAL;
-
-   if (p_mpeg2_slice_params->pad ||
-   p_mpeg2_slice_params->picture.pad ||
-   p_mpeg2_slice_params->sequence.pad)
-   return -EINVAL;
-
return 0;
 
case V4L2_CTRL_TYPE_MPEG2_QUANTIZATION:
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h 
b/drivers/staging/media/sunxi/cedrus/cedrus.h
index 3f61248c57ac..781676b55a1b 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -142,11 +142,14 @@ static inline dma_addr_t cedrus_buf_addr(struct 
vb2_buffer *buf,
 }
 
 static inline dma_addr_t cedrus_dst_buf_addr(struct cedrus_ctx *ctx,
-unsigned int index,
-unsigned int plane)
+int index, unsigned int plane)
 {
-   struct vb2_buffer *buf = ctx->dst_bufs[index];
+   struct vb2_buffer *buf;
 
+   if (index < 0)
+   return 0;
+
+   buf = ctx->dst_bufs[index];
return buf ? cedrus_buf_addr(buf, >dst_fmt, plane) : 0;
 }
 
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c 
b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
index e40180a33951..097d36e24e27 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
@@ -53,6 +53,8 @@ void cedrus_device_run(void *priv)
break;
}
 
+   v4l2_m2m_buf_copy_data(run.src, run.dst);
+
dev->dec_ops[ctx->current_codec]->setup(ctx, );
 
spin_unlock_irqrestore(>dev->irq_lock, flags);
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c 
b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
index 9abd39cae38c..fdde9a099153 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
@@ -82,7 +82,10 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, 
struct cedrus_run *run)
dma_addr_t fwd_luma_addr, fwd_chroma_addr;
dma_addr_t bwd_luma_addr, bwd_chroma_addr;
struct cedrus_dev *dev = ctx->dev;
+   struct vb2_queue *cap_q = >fh.m2m_ctx->cap_q_ctx.q;
const u8 *matrix;
+   int forward_idx;
+   int backward_idx;
unsigned int i;
u32 reg;
 
@@ -156,23 +159,17 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, 
struct cedrus_run *run)
cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg);
 
/* Forward and backward prediction reference buffers. */
+   forward_idx = vb2_find_tag(cap_q, slice_params->forward_ref_tag, 0);
 
-   fwd_luma_addr = cedrus_dst_buf_addr(ctx,
-   slice_params->forward_ref_index,
-   0);
-   fwd_chroma_addr = cedrus_dst_buf_addr(ctx,
- slice_params->forward_ref_index,
- 1);
+   fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
+   fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
 
cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr);
cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR, fwd_chroma_addr);
 
-   bwd_luma_addr = cedrus_dst_buf_addr(ctx,
-   slice_params->backward_ref_index,
-   0);
-   bwd_chroma_addr = cedrus_dst_buf_addr(ctx,
- slice_params->backward_ref_index,
- 1);
+   backward_idx = vb2_find_tag(cap_q, slice_params->backward_ref_tag, 0);
+   bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
+   bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
 
cedrus_write(dev, 

[PATCH 3/9] v4l2-ioctl.c: log v4l2_buffer tag

2018-11-13 Thread Hans Verkuil
When debugging is on, log the new tag field of struct v4l2_buffer
as well.

Signed-off-by: Hans Verkuil 
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
b/drivers/media/v4l2-core/v4l2-ioctl.c
index c63746968fa3..81b9b3ccd7c3 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -498,9 +498,12 @@ static void v4l_print_buffer(const void *arg, bool 
write_only)
p->bytesused, p->m.userptr, p->length);
}
 
-   printk(KERN_DEBUG "timecode=%02d:%02d:%02d type=%d, flags=0x%08x, 
frames=%d, userbits=0x%08x\n",
-   tc->hours, tc->minutes, tc->seconds,
-   tc->type, tc->flags, tc->frames, *(__u32 
*)tc->userbits);
+   if (p->flags & V4L2_BUF_FLAG_TAG)
+   printk(KERN_DEBUG "tag=%llx\n", v4l2_buffer_get_tag(p));
+   else if (p->flags & V4L2_BUF_FLAG_TIMECODE)
+   printk(KERN_DEBUG "timecode=%02d:%02d:%02d type=%d, 
flags=0x%08x, frames=%d, userbits=0x%08x\n",
+  tc->hours, tc->minutes, tc->seconds,
+  tc->type, tc->flags, tc->frames, *(__u32 *)tc->userbits);
 }
 
 static void v4l_print_exportbuffer(const void *arg, bool write_only)
-- 
2.19.1



[PATCH 1/9] videodev2.h: add tag support

2018-11-13 Thread Hans Verkuil
From: Hans Verkuil 

Add support for 'tags' to struct v4l2_buffer. These can be used
by m2m devices so userspace can set a tag for an output buffer and
this value will then be copied to the capture buffer(s).

This tag can be used to refer to capture buffers, something that
is needed by stateless HW codecs.

The new V4L2_BUF_CAP_SUPPORTS_TAGS capability indicates whether
or not tags are supported.

Signed-off-by: Hans Verkuil 
---
 include/uapi/linux/videodev2.h | 38 +-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index c8e8ff810190..ec1fef2a9445 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -879,6 +879,7 @@ struct v4l2_requestbuffers {
 #define V4L2_BUF_CAP_SUPPORTS_USERPTR  (1 << 1)
 #define V4L2_BUF_CAP_SUPPORTS_DMABUF   (1 << 2)
 #define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
+#define V4L2_BUF_CAP_SUPPORTS_TAGS (1 << 4)
 
 /**
  * struct v4l2_plane - plane info for multi-planar buffers
@@ -912,6 +913,11 @@ struct v4l2_plane {
__u32   reserved[11];
 };
 
+struct v4l2_buffer_tag {
+   __u32 low;
+   __u32 high;
+};
+
 /**
  * struct v4l2_buffer - video buffer info
  * @index: id number of the buffer
@@ -950,7 +956,10 @@ struct v4l2_buffer {
__u32   flags;
__u32   field;
struct timeval  timestamp;
-   struct v4l2_timecodetimecode;
+   union {
+   struct v4l2_timecodetimecode;
+   struct v4l2_buffer_tag  tag;
+   };
__u32   sequence;
 
/* memory location */
@@ -988,6 +997,8 @@ struct v4l2_buffer {
 #define V4L2_BUF_FLAG_IN_REQUEST   0x0080
 /* timecode field is valid */
 #define V4L2_BUF_FLAG_TIMECODE 0x0100
+/* tag field is valid */
+#define V4L2_BUF_FLAG_TAG  0x0200
 /* Buffer is prepared for queuing */
 #define V4L2_BUF_FLAG_PREPARED 0x0400
 /* Cache handling flags */
@@ -1007,6 +1018,31 @@ struct v4l2_buffer {
 /* request_fd is valid */
 #define V4L2_BUF_FLAG_REQUEST_FD   0x0080
 
+static inline void v4l2_buffer_set_tag(struct v4l2_buffer *buf, __u64 tag)
+{
+   buf->tag.high = tag >> 32;
+   buf->tag.low = tag & 0xULL;
+   buf->flags |= V4L2_BUF_FLAG_TAG;
+}
+
+static inline void v4l2_buffer_set_tag_ptr(struct v4l2_buffer *buf,
+  const void *tag)
+{
+   v4l2_buffer_set_tag(buf, (uintptr_t)tag);
+}
+
+static inline __u64 v4l2_buffer_get_tag(const struct v4l2_buffer *buf)
+{
+   if (!(buf->flags & V4L2_BUF_FLAG_TAG))
+   return 0;
+   return (((__u64)buf->tag.high) << 32) | (__u64)buf->tag.low;
+}
+
+static inline void *v4l2_buffer_get_tag_ptr(const struct v4l2_buffer *buf)
+{
+   return (void *)(uintptr_t)v4l2_buffer_get_tag(buf);
+}
+
 /**
  * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
  *
-- 
2.19.1



[PATCH 6/9] vb2: add new supports_tags queue flag

2018-11-13 Thread Hans Verkuil
Add new flag to indicate that buffer tags are supported.

Signed-off-by: Hans Verkuil 
---
 drivers/media/common/videobuf2/videobuf2-v4l2.c | 2 ++
 include/media/videobuf2-core.h  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c 
b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 36781d2367a9..cb46e37b2629 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -666,6 +666,8 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
*caps |= V4L2_BUF_CAP_SUPPORTS_DMABUF;
if (q->supports_requests)
*caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
+   if (q->supports_tags)
+   *caps |= V4L2_BUF_CAP_SUPPORTS_TAGS;
 }
 
 int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index e86981d615ae..81f2dbfd0094 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -473,6 +473,7 @@ struct vb2_buf_ops {
  *  has not been called. This is a vb1 idiom that has been adopted
  *  also by vb2.
  * @supports_requests: this queue supports the Request API.
+ * @supports_tags: this queue supports tags in struct v4l2_buffer.
  * @uses_qbuf: qbuf was used directly for this queue. Set to 1 the first
  * time this is called. Set to 0 when the queue is canceled.
  * If this is 1, then you cannot queue buffers from a request.
@@ -547,6 +548,7 @@ struct vb2_queue {
unsignedallow_zero_bytesused:1;
unsigned   quirk_poll_must_check_waiting_for_buffers:1;
unsignedsupports_requests:1;
+   unsignedsupports_tags:1;
unsigneduses_qbuf:1;
unsigneduses_requests:1;
 
-- 
2.19.1



Re: [PATCH v5 0/5] Make sure .device_run is always called in non-atomic context

2018-11-13 Thread Paul Kocialkowski
Hi,

On Mon, 2018-11-12 at 18:05 -0300, Ezequiel Garcia wrote:
> On Mon, 12 Nov 2018 at 13:52, Paul Kocialkowski
>  wrote:
> > Hi,
> > 
> > On Sun, 2018-11-11 at 18:26 -0300, Ezequiel Garcia wrote:
> > > On Thu, 2018-10-18 at 15:02 -0300, Ezequiel Garcia wrote:
> > > > This series goal is to avoid drivers from having ad-hoc code
> > > > to call .device_run in non-atomic context. Currently, .device_run
> > > > can be called via v4l2_m2m_job_finish(), not only running
> > > > in interrupt context, but also creating a nasty re-entrant
> > > > path into mem2mem drivers.
> > > > 
> > > > The proposed solution is to add a per-device worker that is scheduled
> > > > by v4l2_m2m_job_finish, which replaces drivers having a threaded 
> > > > interrupt
> > > > or similar.
> > > > 
> > > > This change allows v4l2_m2m_job_finish() to be called in interrupt
> > > > context, separating .device_run and v4l2_m2m_job_finish() contexts.
> > > > 
> > > > It's worth mentioning that v4l2_m2m_cancel_job() doesn't need
> > > > to flush or cancel the new worker, because the job_spinlock
> > > > synchronizes both and also because the core prevents simultaneous
> > > > jobs. Either v4l2_m2m_cancel_job() will wait for the worker, or the
> > > > worker will be unable to run a new job.
> > > > 
> > > > Patches apply on top of Request API and the Cedrus VPU
> > > > driver.
> > > > 
> > > > Tested with cedrus driver using v4l2-request-test and
> > > > vicodec driver using gstreamer.
> > > > 
> > > > Ezequiel Garcia (4):
> > > >   mem2mem: Require capture and output mutexes to match
> > > >   v4l2-ioctl.c: Simplify locking for m2m devices
> > > >   v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish
> > > >   media: cedrus: Get rid of interrupt bottom-half
> > > > 
> > > > Sakari Ailus (1):
> > > >   v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule
> > > > 
> > > >  drivers/media/v4l2-core/v4l2-ioctl.c  | 47 +
> > > >  drivers/media/v4l2-core/v4l2-mem2mem.c| 66 ---
> > > >  .../staging/media/sunxi/cedrus/cedrus_hw.c| 26 ++--
> > > >  3 files changed, 51 insertions(+), 88 deletions(-)
> > > > 
> > > 
> > > Hans, Maxime:
> > > 
> > > Any feedback for this?
> > 
> > I just tested the whole series with the cedrus driver and everything
> > looks good!
> > 
> 
> Good! So this means we can add a Tested-by for the entire series?

Definitely, I just sent the tested-by line on the cover letter!

> > Removing the interrupt bottom-half in favor of a workqueue in the core
> > seems like a good way to simplify m2m driver development by avoiding
> > per-driver workqueues or threaded irqs.
> > 
> 
> Thanks for the test!

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com


signature.asc
Description: This is a digitally signed message part


Re: [PATCH v5 0/5] Make sure .device_run is always called in non-atomic context

2018-11-13 Thread Paul Kocialkowski
Hi,

On Thu, 2018-10-18 at 15:02 -0300, Ezequiel Garcia wrote:
> This series goal is to avoid drivers from having ad-hoc code
> to call .device_run in non-atomic context. Currently, .device_run
> can be called via v4l2_m2m_job_finish(), not only running
> in interrupt context, but also creating a nasty re-entrant
> path into mem2mem drivers.
> 
> The proposed solution is to add a per-device worker that is scheduled
> by v4l2_m2m_job_finish, which replaces drivers having a threaded interrupt
> or similar.
> 
> This change allows v4l2_m2m_job_finish() to be called in interrupt
> context, separating .device_run and v4l2_m2m_job_finish() contexts.
> 
> It's worth mentioning that v4l2_m2m_cancel_job() doesn't need
> to flush or cancel the new worker, because the job_spinlock
> synchronizes both and also because the core prevents simultaneous
> jobs. Either v4l2_m2m_cancel_job() will wait for the worker, or the
> worker will be unable to run a new job.
> 
> Patches apply on top of Request API and the Cedrus VPU
> driver.
> 
> Tested with cedrus driver using v4l2-request-test and
> vicodec driver using gstreamer.

For the entire series, tested with the cedrus driver:
Tested-by: Paul Kocialkowski 

Cheers,

Paul

> Ezequiel Garcia (4):
>   mem2mem: Require capture and output mutexes to match
>   v4l2-ioctl.c: Simplify locking for m2m devices
>   v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish
>   media: cedrus: Get rid of interrupt bottom-half
> 
> Sakari Ailus (1):
>   v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule
> 
>  drivers/media/v4l2-core/v4l2-ioctl.c  | 47 +
>  drivers/media/v4l2-core/v4l2-mem2mem.c| 66 ---
>  .../staging/media/sunxi/cedrus/cedrus_hw.c| 26 ++--
>  3 files changed, 51 insertions(+), 88 deletions(-)
> 
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com


signature.asc
Description: This is a digitally signed message part