cron job: media_tree daily build: ERRORS

[Hans Verkuil]

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

Results of the daily build of media_tree:

date:   Tue Mar 14 05:00:31 CET 2017
media-tree git hash:700ea5e0e0dd70420a04e703ff264cc133834cba
media_build git hash:   bc4c2a205c087c8deff3cd14ed663c4767dd2016
v4l-utils git hash: ca6a0c099399cc51ecfacff7ef938be6ef73b8b3
gcc version:i686-linux-gcc (GCC) 6.2.0
sparse version: v0.5.0-3553-g78b2ea6
smatch version: v0.5.0-3553-g78b2ea6
host hardware:  x86_64
host os:4.9.0-164

linux-git-arm-at91: OK
linux-git-arm-davinci: OK
linux-git-arm-multi: OK
linux-git-arm-pxa: OK
linux-git-blackfin-bf561: OK
linux-git-i686: OK
linux-git-m32r: OK
linux-git-mips: OK
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: OK
linux-2.6.36.4-i686: WARNINGS
linux-2.6.37.6-i686: WARNINGS
linux-2.6.38.8-i686: WARNINGS
linux-2.6.39.4-i686: WARNINGS
linux-3.0.60-i686: WARNINGS
linux-3.1.10-i686: WARNINGS
linux-3.2.37-i686: WARNINGS
linux-3.3.8-i686: WARNINGS
linux-3.4.27-i686: WARNINGS
linux-3.5.7-i686: WARNINGS
linux-3.6.11-i686: WARNINGS
linux-3.7.4-i686: WARNINGS
linux-3.8-i686: WARNINGS
linux-3.9.2-i686: WARNINGS
linux-3.10.1-i686: WARNINGS
linux-3.11.1-i686: ERRORS
linux-3.12.67-i686: ERRORS
linux-3.13.11-i686: WARNINGS
linux-3.14.9-i686: WARNINGS
linux-3.15.2-i686: WARNINGS
linux-3.16.7-i686: WARNINGS
linux-3.17.8-i686: WARNINGS
linux-3.18.7-i686: WARNINGS
linux-3.19-i686: WARNINGS
linux-4.0.9-i686: WARNINGS
linux-4.1.33-i686: WARNINGS
linux-4.2.8-i686: WARNINGS
linux-4.3.6-i686: WARNINGS
linux-4.4.22-i686: WARNINGS
linux-4.5.7-i686: WARNINGS
linux-4.6.7-i686: WARNINGS
linux-4.7.5-i686: WARNINGS
linux-4.8-i686: OK
linux-4.9-i686: OK
linux-4.10.1-i686: OK
linux-4.11-rc1-i686: OK
linux-2.6.36.4-x86_64: WARNINGS
linux-2.6.37.6-x86_64: WARNINGS
linux-2.6.38.8-x86_64: WARNINGS
linux-2.6.39.4-x86_64: WARNINGS
linux-3.0.60-x86_64: WARNINGS
linux-3.1.10-x86_64: WARNINGS
linux-3.2.37-x86_64: WARNINGS
linux-3.3.8-x86_64: WARNINGS
linux-3.4.27-x86_64: WARNINGS
linux-3.5.7-x86_64: WARNINGS
linux-3.6.11-x86_64: WARNINGS
linux-3.7.4-x86_64: WARNINGS
linux-3.8-x86_64: WARNINGS
linux-3.9.2-x86_64: WARNINGS
linux-3.10.1-x86_64: WARNINGS
linux-3.11.1-x86_64: ERRORS
linux-3.12.67-x86_64: ERRORS
linux-3.13.11-x86_64: WARNINGS
linux-3.14.9-x86_64: WARNINGS
linux-3.15.2-x86_64: WARNINGS
linux-3.16.7-x86_64: WARNINGS
linux-3.17.8-x86_64: WARNINGS
linux-3.18.7-x86_64: WARNINGS
linux-3.19-x86_64: WARNINGS
linux-4.0.9-x86_64: WARNINGS
linux-4.1.33-x86_64: WARNINGS
linux-4.2.8-x86_64: WARNINGS
linux-4.3.6-x86_64: WARNINGS
linux-4.4.22-x86_64: WARNINGS
linux-4.5.7-x86_64: WARNINGS
linux-4.6.7-x86_64: WARNINGS
linux-4.7.5-x86_64: WARNINGS
linux-4.8-x86_64: WARNINGS
linux-4.9-x86_64: WARNINGS
linux-4.10.1-x86_64: WARNINGS
linux-4.11-rc1-x86_64: OK
apps: WARNINGS
spec-git: OK
sparse: WARNINGS

Detailed results are available here:

http://www.xs4all.nl/~hverkuil/logs/Tuesday.log

Full logs are available here:

http://www.xs4all.nl/~hverkuil/logs/Tuesday.tar.bz2

The Media Infrastructure API from this daily build is here:

http://www.xs4all.nl/~hverkuil/spec/index.html

Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Mauro Carvalho Chehab]

Hi Sakari,

I started preparing a long argument about it, but gave up in favor of a
simpler one.

Em Mon, 13 Mar 2017 14:46:22 +0200
Sakari Ailus  escreveu:

> Drivers are written to support hardware, not particular use case.  

No, it is just the reverse: drivers and hardware are developed to
support use cases.

Btw, you should remember that the hardware is the full board, not just the
SoC. In practice, the board do limit the use cases: several provide a
single physical CSI connector, allowing just one sensor.

> > This situation is there since 2009. If I remember well, you tried to write
> > such generic plugin in the past, but never finished it, apparently because
> > it is too complex. Others tried too over the years.   
> 
> I'd argue I know better what happened with that attempt than you do. I had a
> prototype of a generic pipeline configuration library but due to various
> reasons I haven't been able to continue working on that since around 2012.

...

> > The last trial was done by Jacek, trying to cover just the exynos4 driver. 
> > Yet, even such limited scope plugin was not good enough, as it was never
> > merged upstream. Currently, there's no such plugins upstream.
> > 
> > If we can't even merge a plugin that solves it for just *one* driver,
> > I have no hope that we'll be able to do it for the generic case.  
> 
> I believe Jacek ceased to work on that plugin in his day job; other than
> that, there are some matters left to be addressed in his latest patchset.

The two above basically summaries the issue: the task of doing a generic
plugin on userspace, even for a single driver is complex enough to
not cover within a reasonable timeline.

>From 2009 to 2012, you were working on it, but didn't finish it.

Apparently, nobody worked on it between 2013-2014 (but I may be wrong, as
I didn't check when the generic plugin interface was added to libv4l).

In the case of Jacek's work, the first patch I was able to find was
written in Oct, 2014:
https://patchwork.kernel.org/patch/5098111/
(not sure what happened with the version 1).

The last e-mail about this subject was issued in Dec, 2016.

In summary, you had this on your task for 3 years for an OMAP3
plugin (where you have a good expertise), and Jacek for 2 years, 
for Exynos 4, where he should also have a good knowledge.

Yet, with all that efforts, no concrete results were achieved, as none
of the plugins got merged.

Even if they were merged, if we keep the same mean time to develop a
libv4l plugin, that would mean that a plugin for i.MX6 could take 2-3
years to be developed.

There's a clear message on it:
- we shouldn't keep pushing for a solution via libv4l.

Thanks,
Mauro

Re: [RFC PATCH 10/13] mm: Introduce first class virtual address spaces

[Till Smejkal]

Hi Vineet,

On Mon, 13 Mar 2017, Vineet Gupta wrote:
> I've not looked at the patches closely (or read the references paper fully 
> yet),
> but at first glance it seems on ARC architecture, we can can potentially
> use/leverage this mechanism to implement the shared TLB entries. Before anyone
> shouts these are not same as the IA64/x86 protection keys which allow TLB 
> entries
> with different protection bits across processes etc. These TLB entries are
> actually *shared* by processes.
> 
> Conceptually there's shared address spaces, independent of processes. e.g. 
> ldso
> code is shared address space #1, libc (code) #2  System can support a 
> limited
> number of shared addr spaces (say 64, enough for typical embedded sys).
> 
> While Normal TLB entries are tagged with ASID (Addr space ID) to keep them 
> unique
> across processes, Shared TLB entries are tagged with Shared address space ID.
> 
> A process MMU context consists of ASID (a single number) and a SASID bitmap 
> (to
> allow "subscription" to multiple Shared spaces. The subscriptions are set up 
> bu
> userspace ld.so which knows about the libs process wants to map.
> 
> The restriction ofcourse is that the spaces are mapped at *same* vaddr is all
> participating processes. I know this goes against whole security, address 
> space
> randomization - but it gives much better real time performance. Why does each
> process need to take a MMU exception for libc code...
> 
> So long story short - it seems there can be multiple uses of this 
> infrastructure !

During the development of this code, we also looked at shared TLB entries, but
the other way around. We wanted to use them to prevent flushing of TLB entries 
of
shared memory regions when switching between multiple ASes. Unfortunately, we 
never
finished this part of the code.

However, we also investigated into a different use-case for first class virtual
address spaces that is related to what you propose if I didn't understand 
something
wrong. The idea is to move shared libraries into their own first class virtual
address space and only load some small trampoline code in the application AS. 
This
trampoline code performs the VAS switch in the libraries AS and execute the 
requested
function there. If we combine this architecture with tagged TLB entries to 
prevent
TLB flushes during the switch operation, it can also reach an acceptable 
performance.
A side effect of moving the shared library into its own AS is that it can not 
be used
by ROP-attacks because it is not accessible in the application's AS.

Till

Re: [RFC PATCH 10/13] mm: Introduce first class virtual address spaces

[Vineet Gupta]

+CC Ingo, tglx

Hi Till,

On 03/13/2017 03:14 PM, Till Smejkal wrote:
> Introduce a different type of address spaces which are first class citizens
> in the OS. That means that the kernel now handles two types of AS, those
> which are closely coupled with a process and those which aren't. While the
> former ones are created and destroyed together with the process by the
> kernel and are the default type of AS in the Linux kernel, the latter ones
> have to be managed explicitly by the user and are the newly introduced
> type.
> 
> Accordingly, a first class AS (also called VAS == virtual address space)
> can exist in the OS independently from any process. A user has to
> explicitly create and destroy them in the system. Processes and VAS can be
> combined by attaching a previously created VAS to a process which basically
> adds an additional AS to the process that the process' threads are able to
> execute in. Hence, VAS allow a process to have different views onto the
> main memory of the system (its original AS and the attached VAS) between
> which its threads can switch arbitrarily during their lifetime.
> 
> The functionality made available through first class virtual address spaces
> can be used in various different ways. One possible way to utilize VAS is
> to compartmentalize a process for security reasons. Another possible usage
> is to improve the performance of data-centric applications by being able to
> manage different sets of data in memory without the need to map or unmap
> them.
> 
> Furthermore, first class virtual address spaces can be attached to
> different processes at the same time if the underlying memory is only
> readable. This mechanism allows sharing of whole address spaces between
> multiple processes that can both execute in them using the contained
> memory.

I've not looked at the patches closely (or read the references paper fully yet),
but at first glance it seems on ARC architecture, we can can potentially
use/leverage this mechanism to implement the shared TLB entries. Before anyone
shouts these are not same as the IA64/x86 protection keys which allow TLB 
entries
with different protection bits across processes etc. These TLB entries are
actually *shared* by processes.

Conceptually there's shared address spaces, independent of processes. e.g. ldso
code is shared address space #1, libc (code) #2  System can support a 
limited
number of shared addr spaces (say 64, enough for typical embedded sys).

While Normal TLB entries are tagged with ASID (Addr space ID) to keep them 
unique
across processes, Shared TLB entries are tagged with Shared address space ID.

A process MMU context consists of ASID (a single number) and a SASID bitmap (to
allow "subscription" to multiple Shared spaces. The subscriptions are set up bu
userspace ld.so which knows about the libs process wants to map.

The restriction ofcourse is that the spaces are mapped at *same* vaddr is all
participating processes. I know this goes against whole security, address space
randomization - but it gives much better real time performance. Why does each
process need to take a MMU exception for libc code...

So long story short - it seems there can be multiple uses of this 
infrastructure !

-Vineet

Re: [PATCH] staging: atomisp: use k{v}zalloc instead of k{v}alloc and memset

[DaeSeok Youn]

2017-03-14 2:54 GMT+09:00 Alan Cox :
>
> On Mon, 2017-03-13 at 19:54 +0900, Daeseok Youn wrote:
> > If the atomisp_kernel_zalloc() has "true" as a second parameter, it
> > tries to allocate zeroing memory from kmalloc(vmalloc) and memset.
> > But using kzalloc is rather than kmalloc followed by memset with 0.
> > (vzalloc is for same reason with kzalloc)
>
> This is true but please don't apply this. There are about five other
> layers of indirection for memory allocators that want removing first so
> that the driver just uses the correct kmalloc/kzalloc/kv* functions in
> the right places.
right. kvmalloc/kvzalloc would be used after preparing those
interfaces in staging tree.
I will try to change all the atomisp_kernel_m{z}alloc() callers to
correct functions to allocate memory.

Thanks.
Regards,
Jake.

>
> Alan
>

Re: [PATCH v5 00/39] i.MX Media Driver

[Steve Longerbeam]

er, I meant I will integrate this patch. And verify/fix
possible breakage for non-bayer passthrough.

Steve


On 03/13/2017 02:30 AM, Russell King - ARM Linux wrote:

On Mon, Mar 13, 2017 at 08:16:25AM +, Russell King - ARM Linux wrote:

On Sun, Mar 12, 2017 at 09:26:41PM -0700, Steve Longerbeam wrote:

On 03/12/2017 01:22 PM, Russell King - ARM Linux wrote:

What I had was this patch for your v3.  I never got to testing your
v4 because of the LP-11 problem.

In v5, you've changed to propagate the ipu_cpmem_set_image() error
code to avoid the resulting corruption, but that leaves the other bits
of this patch unaddressed, along my "media: imx: smfc: add support
for bayer formats" patch.

Your driver basically has no support for bayer formats.

You added the patches to this driver that adds the bayer support,
I don't think there is anything more required of the driver at this
point to support bayer, the remaining work needs to happen in the IPUv3
driver.

There is more work, because the way you've merged my changes to
imx_smfc_setup_channel() into csi_idmac_setup_channel() is wrong with
respect to the burst size.

You always set it to 8 or 16 depending on the width:

burst_size = (image.pix.width & 0xf) ? 8 : 16;

ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size);

and then you have my switch() statement which assigns burst_size.
My _tested_ code removed the above, added the switch, which had
a default case which reflected the above setting:

default:
burst_size = (outfmt->width & 0xf) ? 8 : 16;

and then went on to set the burst size _after_ the switch statement:

ipu_cpmem_set_burstsize(priv->smfc_ch, burst_size);

The effect is unchanged for non-bayer formats.  For bayer formats, the
burst size is determined by the bayer data size.

So, even if it's appropriate to fix ipu_cpmem_set_image(), fixing the
above is still required.

I'm not convinced that fixing ipu_cpmem_set_image() is even the best
solution - it's not as trivial as it looks on the surface:

 ipu_cpmem_set_resolution(ch, image->rect.width, image->rect.height);
 ipu_cpmem_set_stride(ch, pix->bytesperline);

this is fine, it doesn't depend on the format.  However, the next line:

 ipu_cpmem_set_fmt(ch, v4l2_pix_fmt_to_drm_fourcc(pix->pixelformat));

does - v4l2_pix_fmt_to_drm_fourcc() is a locally defined function (it
isn't v4l2 code) that converts a v4l2 pixel format to a DRM fourcc.
DRM knows nothing about bayer formats, there aren't fourcc codes in
DRM for it.  The result is that v4l2_pix_fmt_to_drm_fourcc() returns
-EINVAL cast to a u32, which gets passed unchecked into ipu_cpmem_set_fmt().

ipu_cpmem_set_fmt() won't recognise that, and also returns -EINVAL - and
it's a bug that this is not checked and propagated.  If it is checked and
propagated, then we need this to support bayer formats, and I don't see
DRM people wanting bayer format fourcc codes added without there being
a real DRM driver wanting to use them.

Then there's the business of calculating the top-left offset of the image,
which for bayer always needs to be an even number of pixels - as this
function takes the top-left offset, it ought to respect it, but if it
doesn't meet this criteria, what should it do?  csi_idmac_setup_channel()
always sets them to zero, but that's not really something that
ipu_cpmem_set_image() should assume.

For the time being, I've restored the functionality along the same lines
as I originally had.  This seems to get me working capture, but might
break non-bayer passthrough mode:

diff --git a/drivers/staging/media/imx/imx-media-csi.c 
b/drivers/staging/media/imx/imx-media-csi.c
index fc0036aa84d0..df336971a009 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -314,14 +314,6 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
image.phys0 = phys[0];
image.phys1 = phys[1];
  
-	ret = ipu_cpmem_set_image(priv->idmac_ch, );

-   if (ret)
-   return ret;
-
-   burst_size = (image.pix.width & 0xf) ? 8 : 16;
-
-   ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size);
-
/*
 * Check for conditions that require the IPU to handle the
 * data internally as generic data, aka passthrough mode:
@@ -346,15 +338,29 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
passthrough_bits = 16;
break;
default:
+   burst_size = (image.pix.width & 0xf) ? 8 : 16;
passthrough = (sensor_ep->bus_type != V4L2_MBUS_CSI2 &&
   sensor_ep->bus.parallel.bus_width >= 16);
passthrough_bits = 16;
break;
}
  
-	if (passthrough)

+   if (passthrough) {
+   ipu_cpmem_set_resolution(priv->idmac_ch, image.rect.width,
+image.rect.height);
+   ipu_cpmem_set_stride(priv->idmac_ch, 

Re: [PATCH v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/13/2017 01:16 AM, Russell King - ARM Linux wrote:

On Sun, Mar 12, 2017 at 09:26:41PM -0700, Steve Longerbeam wrote:

On 03/12/2017 01:22 PM, Russell King - ARM Linux wrote:

What I had was this patch for your v3.  I never got to testing your
v4 because of the LP-11 problem.

In v5, you've changed to propagate the ipu_cpmem_set_image() error
code to avoid the resulting corruption, but that leaves the other bits
of this patch unaddressed, along my "media: imx: smfc: add support
for bayer formats" patch.

Your driver basically has no support for bayer formats.

You added the patches to this driver that adds the bayer support,
I don't think there is anything more required of the driver at this
point to support bayer, the remaining work needs to happen in the IPUv3
driver.

There is more work, because the way you've merged my changes to
imx_smfc_setup_channel() into csi_idmac_setup_channel() is wrong with
respect to the burst size.

You always set it to 8 or 16 depending on the width:

burst_size = (image.pix.width & 0xf) ? 8 : 16;

ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size);

and then you have my switch() statement which assigns burst_size.
My _tested_ code removed the above, added the switch, which had
a default case which reflected the above setting:

default:
burst_size = (outfmt->width & 0xf) ? 8 : 16;

and then went on to set the burst size _after_ the switch statement:

ipu_cpmem_set_burstsize(priv->smfc_ch, burst_size);

The effect is unchanged for non-bayer formats.  For bayer formats, the
burst size is determined by the bayer data size.

So, even if it's appropriate to fix ipu_cpmem_set_image(), fixing the
above is still required.


Oops, sorry missed that. I'll fix.



I'm not convinced that fixing ipu_cpmem_set_image() is even the best
solution - it's not as trivial as it looks on the surface:

 ipu_cpmem_set_resolution(ch, image->rect.width, image->rect.height);
 ipu_cpmem_set_stride(ch, pix->bytesperline);

this is fine, it doesn't depend on the format.  However, the next line:

 ipu_cpmem_set_fmt(ch, v4l2_pix_fmt_to_drm_fourcc(pix->pixelformat));

does - v4l2_pix_fmt_to_drm_fourcc() is a locally defined function (it
isn't v4l2 code) that converts a v4l2 pixel format to a DRM fourcc.
DRM knows nothing about bayer formats, there aren't fourcc codes in
DRM for it.


right, yeah that's a problem.


   The result is that v4l2_pix_fmt_to_drm_fourcc() returns
-EINVAL cast to a u32, which gets passed unchecked into ipu_cpmem_set_fmt().


Ugh.



ipu_cpmem_set_fmt() won't recognise that, and also returns -EINVAL - and
it's a bug that this is not checked and propagated.  If it is checked and
propagated, then we need this to support bayer formats, and I don't see
DRM people wanting bayer format fourcc codes added without there being
a real DRM driver wanting to use them.


true.



Then there's the business of calculating the top-left offset of the image,
which for bayer always needs to be an even number of pixels - as this
function takes the top-left offset, it ought to respect it, but if it
doesn't meet this criteria, what should it do?  csi_idmac_setup_channel()
always sets them to zero, but that's not really something that
ipu_cpmem_set_image() should assume.


Well, I will integrate your patch above. Thanks for doing this
work for me.

We do need to address the issues you brought up in ipu_cpmem at
some point.

Steve

Re: [PATCH] docs-rst: Don't use explicit Makefile rules to build SVG and DOT files

[Jonathan Corbet]

On Thu,  9 Mar 2017 15:14:52 -0300
Mauro Carvalho Chehab  wrote:

> Now that we have an extension to handle images, use it.

Applied, finally - thanks.

jon

Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging

[Laura Abbott]

On 03/13/2017 02:29 PM, Rob Clark wrote:
> On Mon, Mar 13, 2017 at 5:09 PM, Laura Abbott  wrote:
>>> Hm, we might want to expose all the heaps as individual
>>> /dev/ion_$heapname nodes? Should we do this from the start, since
>>> we're massively revamping the uapi anyway (imo not needed, current
>>> state seems to work too)?
>>> -Daniel
>>>
>>
>> I thought about that. One advantage with separate /dev/ion_$heap
>> is that we don't have to worry about a limit of 32 possible
>> heaps per system (32-bit heap id allocation field). But dealing
>> with an ioctl seems easier than names. Userspace might be less
>> likely to hardcode random id numbers vs. names as well.
> 
> 
> other advantage, I think, is selinux (brought up elsewhere on this
> thread).. heaps at known fixed PAs are useful for certain sorts of
> attacks so being able to restrict access more easily seems like a good
> thing
> 
> BR,
> -R
> 

Some other kind of filtering (BPF/LSM/???) might work as well
(http://kernsec.org/files/lss2015/vanderstoep.pdf ?)

The fixed PA issue is a larger problem. We're never going to
be able to get away from "this heap must exist at address X"
problems but the location of CMA in general should be
randomized. I haven't actually come up with a good proposal
to this though.

I'd like for Ion to be a framework for memory allocation and
not security exploits. Hopefully this isn't a pipe dream.

Thanks,
Laura

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Steve Longerbeam]

On 03/13/2017 10:10 AM, Hans Verkuil wrote:

On 03/13/2017 06:06 PM, Steve Longerbeam wrote:



On 03/13/2017 03:53 AM, Hans Verkuil wrote:

On 03/13/2017 11:45 AM, Russell King - ARM Linux wrote:

On Mon, Mar 13, 2017 at 11:02:34AM +0100, Hans Verkuil wrote:

On 03/11/2017 07:14 PM, Steve Longerbeam wrote:

The event must be user visible, otherwise the user has no indication
the error, and can't correct it by stream restart.


In that case the driver can detect this and call vb2_queue_error. It's
what it is there for.

The event doesn't help you since only this driver has this issue. So nobody
will watch this event, unless it is sw specifically written for this SoC.

Much better to call vb2_queue_error to signal a fatal error (which this
apparently is) since there are more drivers that do this, and vivid supports
triggering this condition as well.


So today, I can fiddle around with the IMX219 registers to help gain
an understanding of how this sensor works.  Several of the registers
(such as the PLL setup [*]) require me to disable streaming on the
sensor while changing them.

This is something I've done many times while testing various ideas,
and is my primary way of figuring out and testing such things.

Whenever I resume streaming (provided I've let the sensor stop
streaming at a frame boundary) it resumes as if nothing happened.  If I
stop the sensor mid-frame, then I get the rolling issue that Steve
reports, but once the top of the frame becomes aligned with the top of
the capture, everything then becomes stable again as if nothing happened.

The side effect of what you're proposing is that when I disable streaming
at the sensor by poking at its registers, rather than the capture just
stopping, an error is going to be delivered to gstreamer, and gstreamer
is going to exit, taking the entire capture process down.

This severely restricts the ability to be able to develop and test
sensor drivers.

So, I strongly disagree with you.

Loss of capture frames is not necessarily a fatal error - as I have been
saying repeatedly.  In Steve's case, there's some unknown interaction
between the source and iMX6 hardware that is causing the instability,
but that is simply not true of other sources, and I oppose any idea that
we should cripple the iMX6 side of the capture based upon just one
hardware combination where this is a problem.

Steve suggested that the problem could be in the iMX6 CSI block - and I
note comparing Steve's code with the code in FSL's repository that there
are some changes that are missing in Steve's code to do with the CCIR656
sync code setup, particularly for >8 bit.  The progressive CCIR656 8-bit
setup looks pretty similar though - but I think what needs to be asked
is whether the same problem is visible using the FSL/NXP vendor kernel.


* - the PLL setup is something that requires research at the moment.
Sony's official position (even to their customers) is that they do not
supply the necessary information, instead they expect customers to tell
them the capture settings they want, and Sony will throw the values into
a spreadsheet, and they'll supply the register settings back to the
customer.  Hence, the only way to proceed with a generic driver for
this sensor is to experiment, and experimenting requires the ability to
pause the stream at the sensor while making changes.  Take this away,
and we're stuck with the tables-of-register-settings-for-set-of-fixed-
capture-settings approach.  I've made a lot of progress away from this
which is all down to the flexibility afforded by _not_ killing the
capture process.



In other words: Steve should either find a proper fix for this, or only
call vb2_queue_error in this specific case. Sending an event that nobody
will know how to handle or what to do with is pretty pointless IMHO.

Let's just give him time to try and figure out the real issue here.



This is a long-standing issue, I've traveled to Hildesheim working with
our customer to try and get to the bottom of it. I can go into a lot of
details from those trips, we probed the bt.656 bus with a logic analyzer
and I can share those results with anyone who asks. But the results of
those investigations indicate the CSI is not handling the SAV/EAV sync
codes correctly - if there is a shift in the line position at which
those codes occur, the CSI/IPU does not abort the frame capture DMA
and start from the new sync code position, it just continues to capture
lines until the programmed number of lines are transferred, hence you
get these split images. Freescale also informed us of a mechanism in the
IPU that will add lines if it detects these short frames, until the
programmed number of lines are reached. Apparently that is what creates
the rolling effect, but this rolling can last for up to a full minute,
which is completely unacceptable, it must be corrected as soon as
possible.

So the only thing we could come up with was to monitor frame intervals,
this is purely empirical, but we 

Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging

[Laura Abbott]

On 03/13/2017 06:21 AM, Mark Brown wrote:
> On Mon, Mar 13, 2017 at 10:54:33AM +, Brian Starkey wrote:
>> On Sun, Mar 12, 2017 at 02:34:14PM +0100, Benjamin Gaignard wrote:
> 
>>> Another point is how can we put secure rules (like selinux policy) on
>>> heaps since all the allocations
>>> go to the same device (/dev/ion) ? For example, until now, in Android
>>> we have to give the same
>>> access rights to all the process that use ION.
>>> It will become problem when we will add secure heaps because we won't
>>> be able to distinguish secure
>>> processes to standard ones or set specific policy per heaps.
>>> Maybe I'm wrong here but I have never see selinux policy checking an
>>> ioctl field but if that
>>> exist it could be a solution.
> 
>> I might be thinking of a different type of "secure", but...
> 
>> Should the security of secure heaps be enforced by OS-level
>> permissions? I don't know about other architectures, but at least on
>> arm/arm64 this is enforced in hardware; it doesn't matter who has
>> access to the ion heap, because only secure devices (or the CPU
>> running a secure process) is physically able to access the memory
>> backing the buffer.
> 3
>> In fact, in the use-cases I know of, the process asking for the ion
>> allocation is not a secure process, and so we wouldn't *want* to
>> restrict the secure heap to be allocated from only by secure
>> processes.
> 
> I think there's an orthogonal level of OS level security that can be
> applied here - it's reasonable for it to want to say things like "only
> processes that are supposed to be implementing functionality X should be
> able to try to allocate memory set aside for that functionality".  This
> mitigates against escallation attacks and so on, it's not really
> directly related to secure memory as such though.
> 

Ion also makes it pretty trivial to allocate large amounts of kernel
memory and possibly DoS the system. I'd like to have as little
policy in Ion as possible but more important would be a general
security review and people shouting "bad idea ahead".

Thanks,
Laura

[no subject]

[Martin Herrman]

unsubscribe linux-media

Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates

[Russell King - ARM Linux]

On Mon, Mar 13, 2017 at 11:03:50PM +0200, Sakari Ailus wrote:
> Hi Steve,
> 
> On Mon, Mar 13, 2017 at 11:06:22AM -0700, Steve Longerbeam wrote:
> > I'm kinda in the middle on this topic. I agree with Sakari that
> > frame rate can fluctuate, but that should only be temporary. If
> > the frame rate permanently shifts from what a subdev reports via
> > g_frame_interval, then that is a system problem. So I agree with
> > Phillip and Russell that a link validation of frame interval still
> > makes sense.
> > 
> > But I also have to agree with Sakari that a subdev that has no
> > control over frame rate has no business implementing those ops.
> > 
> > And then I agree with Russell that for subdevs that do have control
> > over frame rate, they would have to walk the graph to find the frame
> > rate source.
> > 
> > So we're stuck in a broken situation: either the subdevs have to walk
> > the graph to find the source of frame rate, or s_frame_interval
> > would have to be mandatory and validated between pads, same as set_fmt.
> 
> It's not broken; what we are missing though is documentation on how to
> control devices that can change the frame rate i.e. presumably drop frames
> occasionally.

It's not about "presumably drop frames occasionally."  The definition
of "occasional" is "occurring or appearing at irregular or infrequent
intervals".  Another word which describes what you're saying would be
"randomly drop frames" which would be a quite absurd "feature" to
include in hardware.

It's about _deterministically_ omitting frames from the capture.

The hardware provides two controls:
1. the size of a group of frames - between 1 and 5 frames
2. select which frames within the group are dropped using a bitmask

This gives a _regular_ pattern of dropped frames.

The rate scaling is given by: hweight(bitmask) / group size.

So, for example, if you're receiving a 50fps TV broadcast, and want to
capture at 25fps, you can set the group size to 2, and set the frame
drop to binary "01" or "10" - if it's interlaced, this would have the
effect of selecting one field, or skipping every other frame if
progressive.

That's not "we'll occasionally drop some frames", that's frame rate
scaling.  Just like you can scale the size of an image by omitting
every other pixel and every other line, this hardware allows omitting
every other frame or more.

So, to configure this feature, CSI needs to know two bits of information:

1. The _source_ frame rate.
2. The desired _sink_ frame rate.

>From that, it can compute how many frames to drop, and size of the group.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging

[Rob Clark]

On Mon, Mar 13, 2017 at 5:09 PM, Laura Abbott  wrote:
>> Hm, we might want to expose all the heaps as individual
>> /dev/ion_$heapname nodes? Should we do this from the start, since
>> we're massively revamping the uapi anyway (imo not needed, current
>> state seems to work too)?
>> -Daniel
>>
>
> I thought about that. One advantage with separate /dev/ion_$heap
> is that we don't have to worry about a limit of 32 possible
> heaps per system (32-bit heap id allocation field). But dealing
> with an ioctl seems easier than names. Userspace might be less
> likely to hardcode random id numbers vs. names as well.


other advantage, I think, is selinux (brought up elsewhere on this
thread).. heaps at known fixed PAs are useful for certain sorts of
attacks so being able to restrict access more easily seems like a good
thing

BR,
-R

Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging

[Laura Abbott]

On 03/13/2017 03:54 AM, Brian Starkey wrote:
> On Sun, Mar 12, 2017 at 02:34:14PM +0100, Benjamin Gaignard wrote:
>> 2017-03-09 18:38 GMT+01:00 Laura Abbott :
>>> On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:
 2017-03-06 17:04 GMT+01:00 Daniel Vetter :
> On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote:
>> On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote:
>>
>>> No one gave a thing about android in upstream, so Greg KH just dumped it
>>> all into staging/android/. We've discussed ION a bunch of times, 
>>> recorded
>>> anything we'd like to fix in staging/android/TODO, and Laura's patch
>>> series here addresses a big chunk of that.
>>
>>> This is pretty much the same approach we (gpu folks) used to de-stage 
>>> the
>>> syncpt stuff.
>>
>> Well, there's also the fact that quite a few people have issues with the
>> design (like Laurent).  It seems like a lot of them have either got more
>> comfortable with it over time, or at least not managed to come up with
>> any better ideas in the meantime.
>
> See the TODO, it has everything a really big group (look at the patch for
> the full Cc: list) figured needs to be improved at LPC 2015. We don't just
> merge stuff because merging stuff is fun :-)
>
> Laurent was even in that group ...
> -Daniel

 For me those patches are going in the right direction.

 I still have few questions:
 - since alignment management has been remove from ion-core, should it
 be also removed from ioctl structure ?
>>>
>>> Yes, I think I'm going to go with the suggestion to fixup the ABI
>>> so we don't need the compat layer and as part of that I'm also
>>> dropping the align argument.
>>>
 - can you we ride off ion_handle (at least in userland) and only
 export a dma-buf descriptor ?
>>>
>>> Yes, I think this is the right direction given we're breaking
>>> everything anyway. I was debating trying to keep the two but
>>> moving to only dma bufs is probably cleaner. The only reason
>>> I could see for keeping the handles is running out of file
>>> descriptors for dma-bufs but that seems unlikely.

 In the future how can we add new heaps ?
 Some platforms have very specific memory allocation
 requirements (just have a look in the number of gem custom allocator in 
 drm)
 Do you plan to add heap type/mask for each ?
>>>
>>> Yes, that was my thinking.
>>
>> My concern is about the policy to adding heaps, will you accept
>> "customs" heap per
>> platforms ? per devices ? or only generic ones ?
>> If you are too strict, we will have lot of out-of-tree heaps and if
>> you accept of of them
>> it will be a nightmare to maintain
>>
> 
> Are you concerned about actual heaps (e.g. a carveout at 0x8000 vs
> a carveout at 0x6000) or heap types?
> 
> For heap types, I think the policy can be strict - if it's generally
> useful then it should live in-tree in ion. Otherwise, it would be
> out-of-tree. I'd expect most "custom" heaps to be parameterisable to
> the point of being generally useful.
> 

I'm willing to be reasonably permissive in what lives in tree. A good
example would be something like a heap for the OMAP tiler which had
weird hardware requirements. The associated devices that go with the
heap should be well supported upstream though.

> For actual heap instances, I would expect them to be communicated via
> reserved-memory regions or something similar, and so the maintenance
> burden is pretty low.
> 

Yes. After the next round of review for this series I'm going to
start thinking about properties for chunk and carveout heaps if nobody
proposes something first.

> The existing query ioctl can allow heap IDs to get assigned
> dynamically at runtime, so there's no need to reserve "bit 6" for
> "CUSTOM_ACME_HEAP_1"
> 
>> Another point is how can we put secure rules (like selinux policy) on
>> heaps since all the allocations
>> go to the same device (/dev/ion) ? For example, until now, in Android
>> we have to give the same
>> access rights to all the process that use ION.
>> It will become problem when we will add secure heaps because we won't
>> be able to distinguish secure
>> processes to standard ones or set specific policy per heaps.
>> Maybe I'm wrong here but I have never see selinux policy checking an
>> ioctl field but if that
>> exist it could be a solution.
>>
> 
> I might be thinking of a different type of "secure", but...
> 
> Should the security of secure heaps be enforced by OS-level
> permissions? I don't know about other architectures, but at least on
> arm/arm64 this is enforced in hardware; it doesn't matter who has
> access to the ion heap, because only secure devices (or the CPU
> running a secure process) is physically able to access the memory
> backing the buffer.
> 
> In fact, in the use-cases I know of, the process asking for the ion
> 

Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates

[Russell King - ARM Linux]

On Mon, Mar 13, 2017 at 10:56:46PM +0200, Sakari Ailus wrote:
> Hi Russell,
> 
> On Mon, Mar 13, 2017 at 01:27:02PM +, Russell King - ARM Linux wrote:
> > On Mon, Mar 13, 2017 at 03:16:48PM +0200, Sakari Ailus wrote:
> > > The vast majority of existing drivers do not implement them nor the user
> > > space expects having to set them. Making that mandatory would break 
> > > existing
> > > user space.
> > > 
> > > In addition, that does not belong to link validation either: link 
> > > validation
> > > should only include static properties of the link that are required for
> > > correct hardware operation. Frame rate is not such property: hardware that
> > > supports the MC interface generally does not recognise such concept (with
> > > the exception of some sensors). Additionally, it is dynamic: the frame 
> > > rate
> > > can change during streaming, making its validation at streamon time 
> > > useless.
> > 
> > So how do we configure the CSI, which can do frame skipping?
> > 
> > With what you're proposing, it means it's possible to configure the
> > camera sensor source pad to do 50fps.  Configure the CSI sink pad to
> > an arbitary value, such as 30fps, and configure the CSI source pad to
> > 15fps.
> > 
> > What you actually get out of the CSI is 25fps, which bears very little
> > with the actual values used on the CSI source pad.
> > 
> > You could say "CSI should ask the camera sensor" - well, that's fine
> > if it's immediately downstream, but otherwise we'd need to go walking
> > down the graph to find something that resembles its source - there may
> > be mux and CSI2 interface subdev blocks in that path.  Or we just accept
> > that frame rates are completely arbitary and bear no useful meaning what
> > so ever.
> 
> The user is responsible for configuring the pipeline. It is thus not
> unreasonable to as the user to configure the frame rate as well if there are
> device in the pipeline that can affect the frame rate. The way I proposed to
> implement it is compliant with the existing API and entirely deterministic,
> contrary to what you're saying.

You haven't really addressed my point at all.

What you seem to be saying is that you're quite happy for the situation
(which is a total misconfiguration) to exist.  Given the vapourware of
userspace (which I don't see changing in any kind of reasonable timeline)
I think this is completely absurd.

I'll state clearly now: everything that we've discussed so far, I'm
finding very hard to take anything you've said seriously.  I think we
have very different and incompatible point of views about what is
acceptable from a user point of view, so much so that we're never going
to agree on any point.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging

[Laura Abbott]

On 03/12/2017 12:05 PM, Daniel Vetter wrote:
> On Sun, Mar 12, 2017 at 2:34 PM, Benjamin Gaignard
>  wrote:
>> 2017-03-09 18:38 GMT+01:00 Laura Abbott :
>>> On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:
 2017-03-06 17:04 GMT+01:00 Daniel Vetter :
> On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote:
>> On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote:
>>
>>> No one gave a thing about android in upstream, so Greg KH just dumped it
>>> all into staging/android/. We've discussed ION a bunch of times, 
>>> recorded
>>> anything we'd like to fix in staging/android/TODO, and Laura's patch
>>> series here addresses a big chunk of that.
>>
>>> This is pretty much the same approach we (gpu folks) used to de-stage 
>>> the
>>> syncpt stuff.
>>
>> Well, there's also the fact that quite a few people have issues with the
>> design (like Laurent).  It seems like a lot of them have either got more
>> comfortable with it over time, or at least not managed to come up with
>> any better ideas in the meantime.
>
> See the TODO, it has everything a really big group (look at the patch for
> the full Cc: list) figured needs to be improved at LPC 2015. We don't just
> merge stuff because merging stuff is fun :-)
>
> Laurent was even in that group ...
> -Daniel

 For me those patches are going in the right direction.

 I still have few questions:
 - since alignment management has been remove from ion-core, should it
 be also removed from ioctl structure ?
>>>
>>> Yes, I think I'm going to go with the suggestion to fixup the ABI
>>> so we don't need the compat layer and as part of that I'm also
>>> dropping the align argument.
>>>
 - can you we ride off ion_handle (at least in userland) and only
 export a dma-buf descriptor ?
>>>
>>> Yes, I think this is the right direction given we're breaking
>>> everything anyway. I was debating trying to keep the two but
>>> moving to only dma bufs is probably cleaner. The only reason
>>> I could see for keeping the handles is running out of file
>>> descriptors for dma-bufs but that seems unlikely.

 In the future how can we add new heaps ?
 Some platforms have very specific memory allocation
 requirements (just have a look in the number of gem custom allocator in 
 drm)
 Do you plan to add heap type/mask for each ?
>>>
>>> Yes, that was my thinking.
>>
>> My concern is about the policy to adding heaps, will you accept
>> "customs" heap per
>> platforms ? per devices ? or only generic ones ?
>> If you are too strict, we will have lot of out-of-tree heaps and if
>> you accept of of them
>> it will be a nightmare to maintain
> 
> I think ion should expose any heap that's also directly accessible to
> devices using dma_alloc(_coherent). That should leave very few things
> left, like your SMA heap.
> 
>> Another point is how can we put secure rules (like selinux policy) on
>> heaps since all the allocations
>> go to the same device (/dev/ion) ? For example, until now, in Android
>> we have to give the same
>> access rights to all the process that use ION.
>> It will become problem when we will add secure heaps because we won't
>> be able to distinguish secure
>> processes to standard ones or set specific policy per heaps.
>> Maybe I'm wrong here but I have never see selinux policy checking an
>> ioctl field but if that
>> exist it could be a solution.
> 
> Hm, we might want to expose all the heaps as individual
> /dev/ion_$heapname nodes? Should we do this from the start, since
> we're massively revamping the uapi anyway (imo not needed, current
> state seems to work too)?
> -Daniel
> 

I thought about that. One advantage with separate /dev/ion_$heap
is that we don't have to worry about a limit of 32 possible
heaps per system (32-bit heap id allocation field). But dealing
with an ioctl seems easier than names. Userspace might be less
likely to hardcode random id numbers vs. names as well.

Thanks,
Laura

Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates

[Sakari Ailus]

Hi Steve,

On Mon, Mar 13, 2017 at 11:06:22AM -0700, Steve Longerbeam wrote:
> 
> 
> On 03/13/2017 06:55 AM, Philipp Zabel wrote:
> >On Mon, 2017-03-13 at 13:27 +, Russell King - ARM Linux wrote:
> >>On Mon, Mar 13, 2017 at 03:16:48PM +0200, Sakari Ailus wrote:
> >>>The vast majority of existing drivers do not implement them nor the user
> >>>space expects having to set them. Making that mandatory would break 
> >>>existing
> >>>user space.
> >>>
> >>>In addition, that does not belong to link validation either: link 
> >>>validation
> >>>should only include static properties of the link that are required for
> >>>correct hardware operation. Frame rate is not such property: hardware that
> >>>supports the MC interface generally does not recognise such concept (with
> >>>the exception of some sensors). Additionally, it is dynamic: the frame rate
> >>>can change during streaming, making its validation at streamon time 
> >>>useless.
> >>
> >>So how do we configure the CSI, which can do frame skipping?
> >>
> >>With what you're proposing, it means it's possible to configure the
> >>camera sensor source pad to do 50fps.  Configure the CSI sink pad to
> >>an arbitary value, such as 30fps, and configure the CSI source pad to
> >>15fps.
> >>
> >>What you actually get out of the CSI is 25fps, which bears very little
> >>with the actual values used on the CSI source pad.
> >>
> >>You could say "CSI should ask the camera sensor" - well, that's fine
> >>if it's immediately downstream, but otherwise we'd need to go walking
> >>down the graph to find something that resembles its source - there may
> >>be mux and CSI2 interface subdev blocks in that path.  Or we just accept
> >>that frame rates are completely arbitary and bear no useful meaning what
> >>so ever.
> >
> >Which would include the frame interval returned by VIDIOC_G_PARM on the
> >connected video device, as that gets its information from the CSI output
> >pad's frame interval.
> >
> 
> I'm kinda in the middle on this topic. I agree with Sakari that
> frame rate can fluctuate, but that should only be temporary. If
> the frame rate permanently shifts from what a subdev reports via
> g_frame_interval, then that is a system problem. So I agree with
> Phillip and Russell that a link validation of frame interval still
> makes sense.
> 
> But I also have to agree with Sakari that a subdev that has no
> control over frame rate has no business implementing those ops.
> 
> And then I agree with Russell that for subdevs that do have control
> over frame rate, they would have to walk the graph to find the frame
> rate source.
> 
> So we're stuck in a broken situation: either the subdevs have to walk
> the graph to find the source of frame rate, or s_frame_interval
> would have to be mandatory and validated between pads, same as set_fmt.

It's not broken; what we are missing though is documentation on how to
control devices that can change the frame rate i.e. presumably drop frames
occasionally.

If you're doing something that hasn't been done before, it may be that new
documentation needs to be written to accomodate that use case. As we have an
existing interface (VIDIOC_SUBDEV_[GS]_FRAME_INTERVAL) it does make sense
to use that. What is not possible, though, is to mandate its use in link
validation everywhere.

If you had a hardware limitation that would require that the frame rate is
constant, then we'd need to handle that in link validation for that
particular piece of hardware. But there really is no case for doing that for
everything else.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates

[Sakari Ailus]

Hi Russell,

On Mon, Mar 13, 2017 at 01:27:02PM +, Russell King - ARM Linux wrote:
> On Mon, Mar 13, 2017 at 03:16:48PM +0200, Sakari Ailus wrote:
> > The vast majority of existing drivers do not implement them nor the user
> > space expects having to set them. Making that mandatory would break existing
> > user space.
> > 
> > In addition, that does not belong to link validation either: link validation
> > should only include static properties of the link that are required for
> > correct hardware operation. Frame rate is not such property: hardware that
> > supports the MC interface generally does not recognise such concept (with
> > the exception of some sensors). Additionally, it is dynamic: the frame rate
> > can change during streaming, making its validation at streamon time useless.
> 
> So how do we configure the CSI, which can do frame skipping?
> 
> With what you're proposing, it means it's possible to configure the
> camera sensor source pad to do 50fps.  Configure the CSI sink pad to
> an arbitary value, such as 30fps, and configure the CSI source pad to
> 15fps.
> 
> What you actually get out of the CSI is 25fps, which bears very little
> with the actual values used on the CSI source pad.
> 
> You could say "CSI should ask the camera sensor" - well, that's fine
> if it's immediately downstream, but otherwise we'd need to go walking
> down the graph to find something that resembles its source - there may
> be mux and CSI2 interface subdev blocks in that path.  Or we just accept
> that frame rates are completely arbitary and bear no useful meaning what
> so ever.

The user is responsible for configuring the pipeline. It is thus not
unreasonable to as the user to configure the frame rate as well if there are
device in the pipeline that can affect the frame rate. The way I proposed to
implement it is compliant with the existing API and entirely deterministic,
contrary to what you're saying.

It's not the job of the CSI sub-device to figure it out.

S_PARM and G_PARM function as interface on the V4L2 API to access the frame
rate on plain V4L2 devices. The i.MX6 IPU is not a plain V4L2 device.
Essentially the V4L2 device node you have there is an interface to a rather
plain DMA engine, no more.

There have been plans to add device profiles to the documentation but that
work has not yet been done.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

[PATCH] media: platform: rcar_imr: add IMR-LSX3 support

[Sergei Shtylyov]

Add support for the image renderer light SRAM extended 3 (IMR-LSX3) found
only in the R-Car V2H (R8A7792) SoC.  It differs  from IMR-LX4 in that it
supports only planar video formats but can use the video capture data for
the textures.

Signed-off-by: Sergei Shtylyov 

---
This patch  is against the 'media_tree.git' repo's 'master' branch plus the
latest version of the  Renesas IMR driver...

 Documentation/devicetree/bindings/media/rcar_imr.txt |   11 +-
 drivers/media/platform/rcar_imr.c|   93 ---
 2 files changed, 90 insertions(+), 14 deletions(-)

Index: media_tree/Documentation/devicetree/bindings/media/rcar_imr.txt
===
--- media_tree.orig/Documentation/devicetree/bindings/media/rcar_imr.txt
+++ media_tree/Documentation/devicetree/bindings/media/rcar_imr.txt
@@ -8,9 +8,14 @@ and drawing with respect to any shape th
 
 Required properties:
 
-- compatible: "renesas,-imr-lx4", "renesas,imr-lx4" as a fallback for
-  the image renderer light extended 4 (IMR-LX4) found in the R-Car gen3 SoCs,
-  where the examples with  are:
+- compatible:
+  "renesas,-imr-lsx3", "renesas,imr-lsx3" as a fallback for the image
+  renderer light SRAM extended 3 (IMR-LSX3) found in the R-Car gen2 SoCs, where
+  the examples with  are:
+  - "renesas,r8a7792-imr-lsx3" for R-Car V2H;
+  "renesas,-imr-lx4", "renesas,imr-lx4" as a fallback for the image
+  renderer light extended 4 (IMR-LX4) found in the R-Car gen3 SoCs, where the
+  examples with  are:
   - "renesas,r8a7795-imr-lx4" for R-Car H3,
   - "renesas,r8a7796-imr-lx4" for R-Car M3-W.
 - reg: offset and length of the register block;
Index: media_tree/drivers/media/platform/rcar_imr.c
===
--- media_tree.orig/drivers/media/platform/rcar_imr.c
+++ media_tree/drivers/media/platform/rcar_imr.c
@@ -1,5 +1,5 @@
 /*
- * rcar_imr.c -- R-Car IMR-LX4 Driver
+ * rcar_imr.c -- R-Car IMR-LSX3/LX4 Driver
  *
  * Copyright (C) 2015-2017 Cogent Embedded, Inc. 
  *
@@ -14,7 +14,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -81,8 +81,21 @@ struct imr_format_info {
u32 flags;
 };
 
+enum imr_gen {
+   IMR_LSX3,
+   IMR_LX4,
+};
+
+/* IMR type specific data */
+struct imr_type {
+   enum imr_gengen;
+   const struct imr_format_info *formats;
+   unsigned intnum_formats;
+};
+
 /* per-device data */
 struct imr_device {
+   const struct imr_type   *type;
struct device   *dev;
struct clk  *clock;
void __iomem*mmio;
@@ -180,6 +193,7 @@ struct imr_ctx {
 #define IMR_IMR_IEMBIT(1)
 #define IMR_IMR_INMBIT(2)
 
+#define IMR_CMRCR_TXTM BIT(0)  /* IMR-LSX3 only */
 #define IMR_CMRCR_LUCE BIT(1)
 #define IMR_CMRCR_CLCE BIT(2)
 #define IMR_CMRCR_DUV_SHIFT3
@@ -282,6 +296,34 @@ static u32 __imr_flags_common(u32 iflags
return iflags & oflags & IMR_F_PLANES_MASK;
 }
 
+static const struct imr_format_info imr_lsx3_formats[] = {
+   {
+   .name   = "YUV 4:2:2 semiplanar (NV16)",
+   .fourcc = V4L2_PIX_FMT_NV16,
+   .flags  = IMR_F_Y8 | IMR_F_UV8 | IMR_F_PLANAR,
+   },
+   {
+   .name   = "Greyscale 8-bit",
+   .fourcc = V4L2_PIX_FMT_GREY,
+   .flags  = IMR_F_Y8 | IMR_F_PLANAR,
+   },
+   {
+   .name   = "Greyscale 10-bit",
+   .fourcc = V4L2_PIX_FMT_Y10,
+   .flags  = IMR_F_Y8 | IMR_F_Y10 | IMR_F_PLANAR,
+   },
+   {
+   .name   = "Greyscale 12-bit",
+   .fourcc = V4L2_PIX_FMT_Y12,
+   .flags  = IMR_F_Y8 | IMR_F_Y10 | IMR_F_Y12 | IMR_F_PLANAR,
+   },
+   {
+   .name   = "Chrominance UV 8-bit",
+   .fourcc = V4L2_PIX_FMT_UV8,
+   .flags  = IMR_F_UV8 | IMR_F_PLANAR,
+   },
+};
+
 static const struct imr_format_info imr_lx4_formats[] = {
{
.name   = "YUV 4:2:2 semiplanar (NV16)",
@@ -335,6 +377,18 @@ static const struct imr_format_info imr_
},
 };
 
+static const struct imr_type imr_lsx3 = {
+   .gen= IMR_LSX3,
+   .formats= imr_lsx3_formats,
+   .num_formats= ARRAY_SIZE(imr_lsx3_formats),
+};
+
+static const struct imr_type imr_lx4 = {
+   .gen= IMR_LX4,
+   .formats= imr_lx4_formats,
+   .num_formats= ARRAY_SIZE(imr_lx4_formats),
+};
+
 /* mesh configuration constructor */
 static struct imr_cfg *imr_cfg_create(struct imr_ctx *ctx,
  u32 dl_size, u32 dl_start)
@@ -767,7 +821,8 @@ static void imr_dl_program_setup(struct
 "setup %ux%u -> %ux%u mapping (type=%x)\n", w, h, W, 

Re: [PATCH RESEND 1/1] media: platform: Renesas IMR driver

[Sergei Shtylyov]

Hello!

On 02/22/2017 10:05 PM, Sergei Shtylyov wrote:


From: Konstantin Kozhevnikov 

The image renderer light extended 4 (IMR-LX4) or the distortion correction
engine is a drawing processor with a simple  instruction system capable of
referencing data on an external memory as 2D texture data and performing
texture mapping and drawing with respect to any shape that is split into
triangular objects.

This V4L2 memory-to-memory device driver only supports image renderer found
in the R-Car gen3 SoCs; the R-Car gen2 support  can be added later...

[Sergei: merged 2 original patches, added the patch description, removed
unrelated parts,  added the binding document, ported the driver to the
modern kernel, renamed the UAPI header file and the guard  macros to match
the driver name, extended the copyrights, fixed up Kconfig prompt/depends/
help, made use of the BIT()/GENMASK() macros, sorted #include's, removed
leading  dots and fixed grammar in the comments, fixed up indentation to
use tabs where possible, renamed IMR_DLSR to IMR_DLPR to match the manual,
separated the register offset/bit #define's, removed *inline* from .c file,
fixed lines over 80 columns, removed useless parens, operators, casts,
braces, variables, #include's, (commented out) statements, and even
function, inserted empty line after desclaration, removed extra empty
lines, reordered some local variable desclarations, removed calls to
4l2_err() on kmalloc() failure, fixed the error returned by imr_default(),
avoided code duplication in the IRQ handler, used '__packed' for the UAPI
structures, enclosed the macro parameters in parens, exchanged the values
of IMR_MAP_AUTO[SD]G macros.]

Signed-off-by: Konstantin Kozhevnikov

Signed-off-by: Sergei Shtylyov 

---
This patch is against the 'media_tree.git' repo's 'master' branch.

 Documentation/devicetree/bindings/media/rcar_imr.txt |   23
 drivers/media/platform/Kconfig   |   13
 drivers/media/platform/Makefile  |1
 drivers/media/platform/rcar_imr.c| 1923
+++
 include/uapi/linux/rcar_imr.h|   94
 5 files changed, 2054 insertions(+)

Index: media_tree/Documentation/devicetree/bindings/media/rcar_imr.txt
===
--- /dev/null
+++ media_tree/Documentation/devicetree/bindings/media/rcar_imr.txt
@@ -0,0 +1,23 @@
+Renesas R-Car Image Renderer (Distortion Correction Engine)
+---

[...]

+
+Required properties:
+- compatible: must be "renesas,imr-lx4" for the image renderer light
extended 4
+  (IMR-LX4)  found in the R-Car gen3 SoCs;


Needs an SoC specific compatible string too.


   Strings, to be precise -- there are several SoCs but the IMR-LX4 core seems
the same among them. Well, if you say so...


The description is above, so you just need to list the compatible
strings.


   There's (most probably) gonna be other versions of the IMR core supported,
(this core can be forund in gen2 SoCs too)...


   Seriously, I strongly doubt that we need the SoC specific compatibles in 
this case -- they don't add any value and seem to only clutter the bindings 
(more so with adding support for the other variants of the IMR core). The 
manuals don't seem to have any real differences between the SoCs for any given 
variant of the IMR core...


MBR, Sergei

[RFC 01/10] [media] vb2: add explicit fence user API

[Gustavo Padovan]

From: Gustavo Padovan 

Turn the reserved2 field into fence_fd that we will use to send
an in-fence to the kernel return an out-fence from the kernel to
userspace.

Two new flags were added, V4L2_BUF_FLAG_IN_FENCE and
V4L2_BUF_FLAG_OUT_FENCE. They should be used when setting in-fence and
out-fence, respectively.

Signed-off-by: Gustavo Padovan 
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 4 ++--
 drivers/media/v4l2-core/videobuf2-v4l2.c  | 2 +-
 include/uapi/linux/videodev2.h| 6 --
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c 
b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index eac9565..0a522cb 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -348,7 +348,7 @@ struct v4l2_buffer32 {
__s32   fd;
} m;
__u32   length;
-   __u32   reserved2;
+   __s32   fence_fd;
__u32   reserved;
 };
 
@@ -511,7 +511,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct 
v4l2_buffer32 __user
put_user(kp->timestamp.tv_usec, >timestamp.tv_usec) ||
copy_to_user(>timecode, >timecode, sizeof(struct 
v4l2_timecode)) ||
put_user(kp->sequence, >sequence) ||
-   put_user(kp->reserved2, >reserved2) ||
+   put_user(kp->fence_fd, >fence_fd) ||
put_user(kp->reserved, >reserved) ||
put_user(kp->length, >length))
return -EFAULT;
diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c 
b/drivers/media/v4l2-core/videobuf2-v4l2.c
index 3529849..d23c1bf 100644
--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
@@ -203,7 +203,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void 
*pb)
b->timestamp = ns_to_timeval(vb->timestamp);
b->timecode = vbuf->timecode;
b->sequence = vbuf->sequence;
-   b->reserved2 = 0;
+   b->fence_fd = -1;
b->reserved = 0;
 
if (q->is_multiplanar) {
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 45184a2..3b6cfa6 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -911,7 +911,7 @@ struct v4l2_buffer {
__s32   fd;
} m;
__u32   length;
-   __u32   reserved2;
+   __s32   fence_fd;
__u32   reserved;
 };
 
@@ -946,8 +946,10 @@ struct v4l2_buffer {
 #define V4L2_BUF_FLAG_TSTAMP_SRC_MASK  0x0007
 #define V4L2_BUF_FLAG_TSTAMP_SRC_EOF   0x
 #define V4L2_BUF_FLAG_TSTAMP_SRC_SOE   0x0001
+#define V4L2_BUF_FLAG_IN_FENCE 0x0010
+#define V4L2_BUF_FLAG_OUT_FENCE0x0020
 /* mem2mem encoder/decoder */
-#define V4L2_BUF_FLAG_LAST 0x0010
+#define V4L2_BUF_FLAG_LAST 0x0040
 
 /**
  * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
-- 
2.9.3

[RFC 03/10] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

From: Gustavo Padovan 

Receive in-fence from userspace and support for waiting on them
before queueing the buffer for the driver.

Signed-off-by: Gustavo Padovan 
---
 drivers/media/Kconfig|  1 +
 drivers/media/v4l2-core/videobuf2-core.c | 24 
 drivers/media/v4l2-core/videobuf2-v4l2.c | 14 +-
 include/media/videobuf2-core.h   |  7 ++-
 4 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
index 3512316..7c5a0e0 100644
--- a/drivers/media/Kconfig
+++ b/drivers/media/Kconfig
@@ -5,6 +5,7 @@
 menuconfig MEDIA_SUPPORT
tristate "Multimedia support"
depends on HAS_IOMEM
+   select SYNC_FILE
help
  If you want to use Webcams, Video grabber devices and/or TV devices
  enable this option and other options below.
diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
b/drivers/media/v4l2-core/videobuf2-core.c
index 0e30fcd..e0e7109 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1400,7 +1400,18 @@ static int __vb2_core_qbuf(struct vb2_buffer *vb, struct 
vb2_queue *q)
return 0;
 }
 
-int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
+static void vb2_qbuf_fence_cb(struct dma_fence *f, struct dma_fence_cb *cb)
+{
+   struct vb2_buffer *vb = container_of(cb, struct vb2_buffer, fence_cb);
+
+   dma_fence_put(vb->in_fence);
+   vb->in_fence = NULL;
+
+   __vb2_core_qbuf(vb, vb->vb2_queue);
+}
+
+int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
+ struct dma_fence *fence)
 {
struct vb2_buffer *vb;
int ret;
@@ -1432,6 +1443,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
index, void *pb)
if (pb)
call_void_bufop(q, fill_user_buffer, vb, pb);
 
+   vb->in_fence = fence;
+   if (fence && !dma_fence_add_callback(fence, >fence_cb,
+vb2_qbuf_fence_cb))
+   return 0;
+
return __vb2_core_qbuf(vb, q);
 }
 EXPORT_SYMBOL_GPL(vb2_core_qbuf);
@@ -2246,7 +2262,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int 
read)
 * Queue all buffers.
 */
for (i = 0; i < q->num_buffers; i++) {
-   ret = vb2_core_qbuf(q, i, NULL);
+   ret = vb2_core_qbuf(q, i, NULL, NULL);
if (ret)
goto err_reqbufs;
fileio->bufs[i].queued = 1;
@@ -2425,7 +2441,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, 
char __user *data, size_
 
if (copy_timestamp)
b->timestamp = ktime_get_ns();
-   ret = vb2_core_qbuf(q, index, NULL);
+   ret = vb2_core_qbuf(q, index, NULL, NULL);
dprintk(5, "vb2_dbuf result: %d\n", ret);
if (ret)
return ret;
@@ -2528,7 +2544,7 @@ static int vb2_thread(void *data)
if (copy_timestamp)
vb->timestamp = ktime_get_ns();;
if (!threadio->stop)
-   ret = vb2_core_qbuf(q, vb->index, NULL);
+   ret = vb2_core_qbuf(q, vb->index, NULL, NULL);
call_void_qop(q, wait_prepare, q);
if (ret || threadio->stop)
break;
diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c 
b/drivers/media/v4l2-core/videobuf2-v4l2.c
index d23c1bf..c164aa0 100644
--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -557,6 +558,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
 
 int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 {
+   struct dma_fence *fence = NULL;
int ret;
 
if (vb2_fileio_is_active(q)) {
@@ -565,7 +567,17 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
}
 
ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
-   return ret ? ret : vb2_core_qbuf(q, b->index, b);
+
+   if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
+   if (b->memory != VB2_MEMORY_DMABUF)
+   return -EINVAL;
+
+   fence = sync_file_get_fence(b->fence_fd);
+   if (!fence)
+   return -EINVAL;
+   }
+
+   return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
 }
 EXPORT_SYMBOL_GPL(vb2_qbuf);
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index ac5898a..fe2de99 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -16,6 +16,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define VB2_MAX_FRAME  (32)
 #define VB2_MAX_PLANES (8)
@@ -259,6 +260,9 @@ 

[RFC 02/10] [media] vb2: split out queueing from vb_core_qbuf()

[Gustavo Padovan]

From: Gustavo Padovan 

In order to support explicit synchronization we need to divide
vb2_core_qbuf() in two parts one, to be executed before the fence
signals and another one after that, to do the actual queueing of
the buffer.

Signed-off-by: Gustavo Padovan 
---
 drivers/media/v4l2-core/videobuf2-core.c | 65 ++--
 1 file changed, 36 insertions(+), 29 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
b/drivers/media/v4l2-core/videobuf2-core.c
index 94afbbf9..0e30fcd 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1363,29 +1363,10 @@ static int vb2_start_streaming(struct vb2_queue *q)
return ret;
 }
 
-int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
+static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
 {
-   struct vb2_buffer *vb;
int ret;
 
-   vb = q->bufs[index];
-
-   switch (vb->state) {
-   case VB2_BUF_STATE_DEQUEUED:
-   ret = __buf_prepare(vb, pb);
-   if (ret)
-   return ret;
-   break;
-   case VB2_BUF_STATE_PREPARED:
-   break;
-   case VB2_BUF_STATE_PREPARING:
-   dprintk(1, "buffer still being prepared\n");
-   return -EINVAL;
-   default:
-   dprintk(1, "invalid buffer state %d\n", vb->state);
-   return -EINVAL;
-   }
-
/*
 * Add to the queued buffers list, a buffer will stay on it until
 * dequeued in dqbuf.
@@ -1395,11 +1376,6 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
index, void *pb)
q->waiting_for_buffers = false;
vb->state = VB2_BUF_STATE_QUEUED;
 
-   if (pb)
-   call_void_bufop(q, copy_timestamp, vb, pb);
-
-   trace_vb2_qbuf(q, vb);
-
/*
 * If already streaming, give the buffer to driver for processing.
 * If not, the buffer will be given to driver on next streamon.
@@ -1407,10 +1383,6 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
index, void *pb)
if (q->start_streaming_called)
__enqueue_in_driver(vb);
 
-   /* Fill buffer information for the userspace */
-   if (pb)
-   call_void_bufop(q, fill_user_buffer, vb, pb);
-
/*
 * If streamon has been called, and we haven't yet called
 * start_streaming() since not enough buffers were queued, and
@@ -1427,6 +1399,41 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
index, void *pb)
dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
return 0;
 }
+
+int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
+{
+   struct vb2_buffer *vb;
+   int ret;
+
+   vb = q->bufs[index];
+
+   switch (vb->state) {
+   case VB2_BUF_STATE_DEQUEUED:
+   ret = __buf_prepare(vb, pb);
+   if (ret)
+   return ret;
+   break;
+   case VB2_BUF_STATE_PREPARED:
+   break;
+   case VB2_BUF_STATE_PREPARING:
+   dprintk(1, "buffer still being prepared\n");
+   return -EINVAL;
+   default:
+   dprintk(1, "invalid buffer state %d\n", vb->state);
+   return -EINVAL;
+   }
+
+   if (pb)
+   call_void_bufop(q, copy_timestamp, vb, pb);
+
+   trace_vb2_qbuf(q, vb);
+
+   /* Fill buffer information for the userspace */
+   if (pb)
+   call_void_bufop(q, fill_user_buffer, vb, pb);
+
+   return __vb2_core_qbuf(vb, q);
+}
 EXPORT_SYMBOL_GPL(vb2_core_qbuf);
 
 /**
-- 
2.9.3

[RFC 10/10] [media] vb2: add out-fence support to QBUF

[Gustavo Padovan]

From: Gustavo Padovan 

If V4L2_BUF_FLAG_OUT_FENCE flag is present on the QBUF call we create
an out_fence for the buffer and return it to userspace on the fence_fd
field.

The fence is signaled on buffer_done(), when the job on the buffer is
finished.

TODO: clean up on __vb2_queue_cancel()

Signed-off-by: Gustavo Padovan 
---
 drivers/media/v4l2-core/videobuf2-core.c |  6 ++
 drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
b/drivers/media/v4l2-core/videobuf2-core.c
index 54b1404..ca8abcc 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -356,6 +356,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum 
vb2_memory memory,
vb->planes[plane].length = plane_sizes[plane];
vb->planes[plane].min_length = plane_sizes[plane];
}
+   vb->out_fence_fd = -1;
q->bufs[vb->index] = vb;
 
/* Allocate video buffer memory for the MMAP type */
@@ -940,6 +941,11 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
vb2_buffer_state state)
__enqueue_in_driver(vb);
return;
default:
+   dma_fence_signal(vb->out_fence);
+   dma_fence_put(vb->out_fence);
+   vb->out_fence = NULL;
+   vb->out_fence_fd = -1;
+
/* Inform any processes that may be waiting for buffers */
wake_up(>done_wq);
break;
diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c 
b/drivers/media/v4l2-core/videobuf2-v4l2.c
index c164aa0..1b43d9f 100644
--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
@@ -204,9 +204,14 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void 
*pb)
b->timestamp = ns_to_timeval(vb->timestamp);
b->timecode = vbuf->timecode;
b->sequence = vbuf->sequence;
-   b->fence_fd = -1;
+   b->fence_fd = vb->out_fence_fd;
b->reserved = 0;
 
+   if (vb->sync_file) {
+   fd_install(vb->out_fence_fd, vb->sync_file->file);
+   vb->sync_file = NULL;
+   }
+
if (q->is_multiplanar) {
/*
 * Fill in plane-related data if userspace provided an array
@@ -577,6 +582,14 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
return -EINVAL;
}
 
+   if (b->flags & V4L2_BUF_FLAG_OUT_FENCE) {
+   ret = vb2_setup_out_fence(q, b->index);
+   if (ret) {
+   dma_fence_put(fence);
+   return ret;
+   }
+   }
+
return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
 }
 EXPORT_SYMBOL_GPL(vb2_qbuf);
-- 
2.9.3

[RFC 04/10] [media] uvc: enable subscriptions to other events

[Gustavo Padovan]

From: Gustavo Padovan 

Call v4l2_ctrl_subscribe_event to subscribe to more events supported by
v4l.

Signed-off-by: Gustavo Padovan 
---
 drivers/media/usb/uvc/uvc_v4l2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 3e7e283..dfa0ccd 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1240,7 +1240,7 @@ static int uvc_ioctl_subscribe_event(struct v4l2_fh *fh,
case V4L2_EVENT_CTRL:
return v4l2_event_subscribe(fh, sub, 0, _ctrl_sub_ev_ops);
default:
-   return -EINVAL;
+   return v4l2_ctrl_subscribe_event(fh, sub);
}
 }
 
-- 
2.9.3

[RFC 07/10] [media] v4l: add support to BUF_QUEUED event

[Gustavo Padovan]

From: Gustavo Padovan 

Implement the needed pieces to let userspace subscribe for
V4L2_EVENT_BUF_QUEUED events. Videobuf2 will queue the event for the
DQEVENT ioctl.

Signed-off-by: Gustavo Padovan 
---
 drivers/media/v4l2-core/v4l2-ctrls.c |  6 +-
 drivers/media/v4l2-core/videobuf2-core.c | 15 +++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
b/drivers/media/v4l2-core/v4l2-ctrls.c
index b9e08e3..1be554b 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -3413,8 +3413,12 @@ EXPORT_SYMBOL(v4l2_ctrl_log_status);
 int v4l2_ctrl_subscribe_event(struct v4l2_fh *fh,
const struct v4l2_event_subscription *sub)
 {
-   if (sub->type == V4L2_EVENT_CTRL)
+   switch (sub->type) {
+   case V4L2_EVENT_CTRL:
return v4l2_event_subscribe(fh, sub, 0, _ctrl_sub_ev_ops);
+   case V4L2_EVENT_BUF_QUEUED:
+   return v4l2_event_subscribe(fh, sub, 0, NULL);
+   }
return -EINVAL;
 }
 EXPORT_SYMBOL(v4l2_ctrl_subscribe_event);
diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
b/drivers/media/v4l2-core/videobuf2-core.c
index e0e7109..d9cb777 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -25,6 +25,7 @@
 #include 
 
 #include 
+#include 
 #include 
 
 #include 
@@ -1221,6 +1222,18 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const 
void *pb)
return ret;
 }
 
+static void vb2_buffer_queued_event(struct vb2_buffer *vb)
+{
+   struct video_device *vdev = to_video_device(vb->vb2_queue->dev);
+   struct v4l2_event event;
+
+   memset(, 0, sizeof(event));
+   event.type = V4L2_EVENT_BUF_QUEUED;
+   event.u.buf_queued.index = vb->index;
+
+   v4l2_event_queue(vdev, );
+}
+
 /**
  * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
  */
@@ -1239,6 +1252,8 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
 
call_void_vb_qop(vb, buf_queue, vb);
+
+   vb2_buffer_queued_event(vb);
 }
 
 static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
-- 
2.9.3

[RFC 08/10] [media] vb2: add videobuf2 dma-buf fence helpers

[Gustavo Padovan]

From: Javier Martinez Canillas 

Add a videobuf2-fence.h header file that contains different helpers
for DMA buffer sharing explicit fence support in videobuf2.

Signed-off-by: Javier Martinez Canillas 
Signed-off-by: Gustavo Padovan 
---
 include/media/videobuf2-fence.h | 49 +
 1 file changed, 49 insertions(+)
 create mode 100644 include/media/videobuf2-fence.h

diff --git a/include/media/videobuf2-fence.h b/include/media/videobuf2-fence.h
new file mode 100644
index 000..ed5612c
--- /dev/null
+++ b/include/media/videobuf2-fence.h
@@ -0,0 +1,49 @@
+/*
+ * videobuf2-fence.h - DMA buffer sharing fence helpers for videobuf 2
+ *
+ * Copyright (C) 2016 Samsung Electronics
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ */
+
+#include 
+#include 
+
+static DEFINE_SPINLOCK(vb2_fence_lock);
+
+static inline const char *vb2_fence_get_driver_name(struct dma_fence *fence)
+{
+   return "vb2_fence";
+}
+
+static inline const char *vb2_fence_get_timeline_name(struct dma_fence *fence)
+{
+   return "vb2_fence_timeline";
+}
+
+static inline bool vb2_fence_enable_signaling(struct dma_fence *fence)
+{
+   return true;
+}
+
+static const struct dma_fence_ops vb2_fence_ops = {
+   .get_driver_name = vb2_fence_get_driver_name,
+   .get_timeline_name = vb2_fence_get_timeline_name,
+   .enable_signaling = vb2_fence_enable_signaling,
+   .wait = dma_fence_default_wait,
+};
+
+static inline struct dma_fence *vb2_fence_alloc(void)
+{
+   struct dma_fence *vb2_fence = kzalloc(sizeof(*vb2_fence), GFP_KERNEL);
+
+   if (!vb2_fence)
+   return NULL;
+
+   dma_fence_init(vb2_fence, _fence_ops, _fence_lock,
+  dma_fence_context_alloc(1), 1);
+
+   return vb2_fence;
+}
-- 
2.9.3

[RFC 09/10] [media] vb2: add infrastructure to support out-fences

[Gustavo Padovan]

From: Gustavo Padovan 

Add vb2_setup_out_fence() and the needed members to struct vb2_buffer.

Signed-off-by: Gustavo Padovan 
---
 drivers/media/v4l2-core/videobuf2-core.c | 31 +++
 include/media/videobuf2-core.h   |  5 +
 2 files changed, 36 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
b/drivers/media/v4l2-core/videobuf2-core.c
index d9cb777..54b1404 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -23,8 +23,11 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 #include 
+#include 
 #include 
 #include 
 
@@ -1315,6 +1318,34 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned 
int index, void *pb)
 }
 EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
 
+int vb2_setup_out_fence(struct vb2_queue *q, unsigned int index)
+{
+   struct vb2_buffer *vb = q->bufs[index];
+
+   vb->out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
+   if (vb->out_fence_fd < 0)
+   return vb->out_fence_fd;
+
+   vb->out_fence = vb2_fence_alloc();
+   if (!vb->out_fence)
+   goto err;
+
+   vb->sync_file = sync_file_create(vb->out_fence);
+   if (!vb->sync_file) {
+   dma_fence_put(vb->out_fence);
+   vb->out_fence = NULL;
+   goto err;
+   }
+
+   return 0;
+
+err:
+   put_unused_fd(vb->out_fence_fd);
+   vb->out_fence_fd = -1;
+   return -ENOMEM;
+}
+EXPORT_SYMBOL_GPL(vb2_setup_out_fence);
+
 /**
  * vb2_start_streaming() - Attempt to start streaming.
  * @q: videobuf2 queue
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index fe2de99..efdc390 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -263,6 +263,10 @@ struct vb2_buffer {
 
struct dma_fence*in_fence;
struct dma_fence_cb fence_cb;
+
+   struct dma_fence*out_fence;
+   struct sync_file*sync_file;
+   int out_fence_fd;
 #ifdef CONFIG_VIDEO_ADV_DEBUG
/*
 * Counters for how often these buffer-related ops are
@@ -710,6 +714,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum 
vb2_memory memory,
  */
 int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
 
+int vb2_setup_out_fence(struct vb2_queue *q, unsigned int index);
 /**
  * vb2_core_qbuf() - Queue a buffer from userspace
  *
-- 
2.9.3

[RFC 05/10] [media] vivid: assign the specific device to the vb2_queue->dev

[Gustavo Padovan]

From: Gustavo Padovan 

Instead of assign the global v4l2 device assigned the specific device,
this was causing trouble when using using V4L2 events with vivid
devices. The queue device should be the same one we opened in userspace.

Signed-off-by: Gustavo Padovan 
---
 drivers/media/platform/vivid/vivid-core.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/vivid/vivid-core.c 
b/drivers/media/platform/vivid/vivid-core.c
index ef344b9..8843170 100644
--- a/drivers/media/platform/vivid/vivid-core.c
+++ b/drivers/media/platform/vivid/vivid-core.c
@@ -1070,7 +1070,7 @@ static int vivid_create_instance(struct platform_device 
*pdev, int inst)
q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
q->min_buffers_needed = 2;
q->lock = >mutex;
-   q->dev = dev->v4l2_dev.dev;
+   q->dev = >vid_cap_dev.dev;
 
ret = vb2_queue_init(q);
if (ret)
@@ -1090,7 +1090,7 @@ static int vivid_create_instance(struct platform_device 
*pdev, int inst)
q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
q->min_buffers_needed = 2;
q->lock = >mutex;
-   q->dev = dev->v4l2_dev.dev;
+   q->dev = >vid_out_dev.dev;
 
ret = vb2_queue_init(q);
if (ret)
@@ -1110,7 +1110,7 @@ static int vivid_create_instance(struct platform_device 
*pdev, int inst)
q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
q->min_buffers_needed = 2;
q->lock = >mutex;
-   q->dev = dev->v4l2_dev.dev;
+   q->dev = >vbi_cap_dev.dev;
 
ret = vb2_queue_init(q);
if (ret)
@@ -1130,7 +1130,7 @@ static int vivid_create_instance(struct platform_device 
*pdev, int inst)
q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
q->min_buffers_needed = 2;
q->lock = >mutex;
-   q->dev = dev->v4l2_dev.dev;
+   q->dev = >vbi_out_dev.dev;
 
ret = vb2_queue_init(q);
if (ret)
@@ -1149,7 +1149,7 @@ static int vivid_create_instance(struct platform_device 
*pdev, int inst)
q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
q->min_buffers_needed = 8;
q->lock = >mutex;
-   q->dev = dev->v4l2_dev.dev;
+   q->dev = >sdr_cap_dev.dev;
 
ret = vb2_queue_init(q);
if (ret)
-- 
2.9.3

[RFC 06/10] [media] v4l: add V4L2_EVENT_BUF_QUEUED event

[Gustavo Padovan]

From: Gustavo Padovan 

Add a new event the userspace can subscribe to receive notifications
about when a buffer was enqueued onto the driver. The event provides
the index of the enqueued buffer.

Signed-off-by: Gustavo Padovan 
---
 include/uapi/linux/videodev2.h | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 3b6cfa6..5b44fd0 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2133,6 +2133,7 @@ struct v4l2_streamparm {
 #define V4L2_EVENT_FRAME_SYNC  4
 #define V4L2_EVENT_SOURCE_CHANGE   5
 #define V4L2_EVENT_MOTION_DET  6
+#define V4L2_EVENT_BUF_QUEUED  7
 #define V4L2_EVENT_PRIVATE_START   0x0800
 
 /* Payload for V4L2_EVENT_VSYNC */
@@ -2185,6 +2186,10 @@ struct v4l2_event_motion_det {
__u32 region_mask;
 };
 
+struct v4l2_event_buf_queued {
+   __u32 index;
+};
+
 struct v4l2_event {
__u32   type;
union {
@@ -2193,6 +2198,7 @@ struct v4l2_event {
struct v4l2_event_frame_syncframe_sync;
struct v4l2_event_src_changesrc_change;
struct v4l2_event_motion_detmotion_det;
+   struct v4l2_event_buf_queuedbuf_queued;
__u8data[64];
} u;
__u32   pending;
-- 
2.9.3

[RFC 00/10] V4L2 explicit synchronization support

[Gustavo Padovan]

From: Gustavo Padovan 

Hi,

This RFC adds support for Explicit Synchronization of shared buffers in V4L2.
It uses the Sync File Framework[1] as vector to communicate the fences
between kernel and userspace.

I'm sending this to start the discussion on the best approach to implement
Explicit Synchronization, please check the TODO/OPEN section below.

Explicit Synchronization allows us to control the synchronization of
shared buffers from userspace by passing fences to the kernel and/or 
receiving them from the the kernel.

Fences passed to the kernel are named in-fences and the kernel should wait
them to signal before using the buffer. On the other side, the kernel creates
out-fences for every buffer it receives from userspace. This fence is sent back
to userspace and it will signal when the capture, for example, has finished.

Signalling an out-fence in V4L2 would mean that the job on the buffer is done
and the buffer can be used by other drivers.

Current RFC implementation
--

The current implementation is not intended to be more than a PoC to start
the discussion on how Explicit Synchronization should be supported in V4L2.

The first patch proposes an userspace API for fences, then on patch 2
we prepare to the addition of in-fences in patch 3, by introducing the
infrastructure on vb2 to wait on an in-fence signal before queueing the buffer
in the driver.

Patch 4 fix uvc v4l2 event handling and patch 5 configure q->dev for vivid
drivers to enable to subscribe and dequeue events on it.

Patches 6-7 enables support to notify BUF_QUEUED events, i.e., let userspace
know that particular buffer was enqueued in the driver. This is needed,
because we return the out-fence fd as an out argument in QBUF, but at the time
it returns we don't know to which buffer the fence will be attached thus
the BUF_QUEUED event tells which buffer is associated to the fence received in
QBUF by userspace.

Patches 8 and 9 add more fence infrastructure to support out-fences and finally
patch 10 adds support to out-fences.

TODO/OPEN:
--

* For this first implementation we will keep the ordering of the buffers queued
in videobuf2, that means we will only enqueue buffer whose fence was signalled
if that buffer is the first one in the queue. Otherwise it has to wait until it
is the first one. This is not implmented yet. Later we could create a flag to
allow unordered queing in the drivers from vb2 if needed.

* Should we have out-fences per-buffer or per-plane? or both? In this RFC, for
simplicity they are per-buffer, but Mauro and Javier raised the option of
doing per-plane fences. That could benefit mem2mem and V4L2 <-> GPU operation
at least on cases when we have Capture hw that releases the Y frame before the
other frames for example. When using V4L2 per-plane out-fences to communicate
with KMS they would need to be merged together as currently the DRM Plane
interface only supports one fence per DRM Plane.

In-fences should be per-buffer as the DRM only has per-buffer fences, but
in case of mem2mem operations per-plane fences might be useful?

So should we have both ways, per-plane and per-buffer, or just one of them
for now?

* other open topics are how to deal with hw-fences and Request API.

Comments are welcome!

Regards,

Gustavo

---
Gustavo Padovan (9):
  [media] vb2: add explicit fence user API
  [media] vb2: split out queueing from vb_core_qbuf()
  [media] vb2: add in-fence support to QBUF
  [media] uvc: enable subscriptions to other events
  [media] vivid: assign the specific device to the vb2_queue->dev
  [media] v4l: add V4L2_EVENT_BUF_QUEUED event
  [media] v4l: add support to BUF_QUEUED event
  [media] vb2: add infrastructure to support out-fences
  [media] vb2: add out-fence support to QBUF

Javier Martinez Canillas (1):
  [media] vb2: add videobuf2 dma-buf fence helpers

 drivers/media/Kconfig |   1 +
 drivers/media/platform/vivid/vivid-core.c |  10 +-
 drivers/media/usb/uvc/uvc_v4l2.c  |   2 +-
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c |   4 +-
 drivers/media/v4l2-core/v4l2-ctrls.c  |   6 +-
 drivers/media/v4l2-core/videobuf2-core.c  | 139 --
 drivers/media/v4l2-core/videobuf2-v4l2.c  |  29 +-
 include/media/videobuf2-core.h|  12 ++-
 include/media/videobuf2-fence.h   |  49 +
 include/uapi/linux/videodev2.h|  12 ++-
 10 files changed, 218 insertions(+), 46 deletions(-)
 create mode 100644 include/media/videobuf2-fence.h

-- 
2.9.3

Re: [PATCH] [media] v4l2-dv-timings: Introduce v4l2_calc_fps()

[Jose Abreu]

Hi Hans,


On 09-03-2017 15:40, Hans Verkuil wrote:
> On 09/03/17 16:15, Jose Abreu wrote:
>> Hi Hans,
>>
>>
>> Thanks for the review!
>>
>>
>> On 09-03-2017 12:29, Hans Verkuil wrote:
>>> On 07/03/17 17:48, Jose Abreu wrote:
 HDMI Receivers receive video modes which, according to
 CEA specification, can have different frames per second
 (fps) values.

 This patch introduces a helper function in the media core
 which can calculate the expected video mode fps given the
 pixel clock value and the horizontal/vertical values. HDMI
 video receiver drivers are expected to use this helper so
 that they can correctly fill the v4l2_streamparm structure
 which is requested by vidioc_g_parm callback.

 We could also use a lookup table for this but it wouldn't
 correctly handle 60Hz vs 59.94Hz situations as this all
 depends on the pixel clock value.

 Signed-off-by: Jose Abreu 
 Cc: Carlos Palminha 
 Cc: Mauro Carvalho Chehab 
 Cc: Hans Verkuil 
 Cc: Charles-Antoine Couret 
 Cc: linux-media@vger.kernel.org
 Cc: linux-ker...@vger.kernel.org
 ---
  drivers/media/v4l2-core/v4l2-dv-timings.c | 29 
 +
  include/media/v4l2-dv-timings.h   |  8 
  2 files changed, 37 insertions(+)

 diff --git a/drivers/media/v4l2-core/v4l2-dv-timings.c 
 b/drivers/media/v4l2-core/v4l2-dv-timings.c
 index 5c8c49d..19946c6 100644
 --- a/drivers/media/v4l2-core/v4l2-dv-timings.c
 +++ b/drivers/media/v4l2-core/v4l2-dv-timings.c
 @@ -814,3 +814,32 @@ struct v4l2_fract v4l2_calc_aspect_ratio(u8 
 hor_landscape, u8 vert_portrait)
return aspect;
  }
  EXPORT_SYMBOL_GPL(v4l2_calc_aspect_ratio);
 +
 +struct v4l2_fract v4l2_calc_fps(const struct v4l2_dv_timings *t)
 +{
 +  const struct v4l2_bt_timings *bt = >bt;
 +  struct v4l2_fract fps_fract = { 1, 1 };
 +  unsigned long n, d;
 +  unsigned long mask = GENMASK(BITS_PER_LONG - 1, 0);
>>> This is wrong since v4l2_fract uses u32, and LONG can be 64 bits.
>> Yes, its wrong. I will remove the variable and just use fps, 100
>> instead of mask, mask.
>>
 +  u32 htot, vtot, fps;
 +  u64 pclk;
 +
 +  if (t->type != V4L2_DV_BT_656_1120)
 +  return fps_fract;
 +
 +  htot = V4L2_DV_BT_FRAME_WIDTH(bt);
 +  vtot = V4L2_DV_BT_FRAME_HEIGHT(bt);
 +  pclk = bt->pixelclock;
 +  if (bt->interlaced)
 +  htot /= 2;
>>> This can be dropped. This is the timeperframe, not timeperfield. So for 
>>> interleaved
>>> formats the time is that of two fields (aka one frame).
>> Ok, but then there is something not correct in
>> v4l2_dv_timings_presets structure field values because I get
>> wrong results in double clocked modes. I checked the definition
>> and the modes that are double clocked are defined with half the
>> clock, i.e., V4L2_DV_BT_CEA_720X480I59_94 is defined with a pixel
>> clock of 13.5MHz but in CEA spec this mode is defined with pixel
>> clock of 27MHz.
> It's defined in the CEA spec as 1440x480 which is the double clocked
> version of 720x480.
>
> The presets are defined without any pixel repeating. In fact, no driver
> that is in the kernel today supports pixel repeating. Mostly because there was
> never any need since almost nobody uses resolutions that require this.
>
> If you decide to add support for this, then it would not surprise me if
> some of the core dv-timings support needs to be adjusted.
>
> To be honest, I never spent time digging into the pixel repeating details,
> so I am not an expert on this at all.
>
 +
 +  fps = (htot * vtot) > 0 ? div_u64((100 * pclk), (htot * vtot)) : 0;
 +
 +  rational_best_approximation(fps, 100, mask, mask, , );
>>> I think you can just use fps, 100 instead of mask, mask.
>>>
>>> What is returned if fps == 0?
>> I will add a check for this.
>>
>>> I don't have a problem as such with this function, but just be aware that 
>>> the
>>> pixelclock is never precise: there are HDMI receivers that are unable to 
>>> report
>>> the pixelclock with enough precision to even detect if it is 60 vs 59.94 Hz.
>>>
>>> And even for those that can, it is often not reliable.
>> My initial intention for this function was that it should be used
>> with v4l2_find_dv_timings_cea861_vic, when possible. That is,
>> HDMI receivers have access to AVI infoframe contents. Then they
>> should get the vic, call v4l2_find_dv_timings_cea861_vic, get
>> timings and then call v4l2_calc_fps to get fps. If no AVI
>> infoframe is available then it should resort to pixel clock and
>> H/V measures as last resort.
> Right, but there are no separate VIC codes for 60 vs 59.94 Hz. Any vertical
> refresh rate that can be divided by 6 can also support these slightly lower
> refresh rates. 

[PATCH] staging: atomisp: fix missing break in switch statement

[Colin King]

From: Colin Ian King 

I believe there is a missing break in the switch statement for
case V4L2_CID_FOCUS_STATUS as the current fall-through looks
suspect to me.

Detected by CoverityScan, CID#1416580 ("Missing break in switch")

Signed-off-by: Colin Ian King 
---
 drivers/staging/media/atomisp/i2c/ov5693/ov5693.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.c 
b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.c
index e3d4d0e..ac75982 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.c
+++ b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.c
@@ -1132,7 +1132,7 @@ static int ov5693_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
break;
case V4L2_CID_FOCUS_STATUS:
ret = ov5693_q_focus_status(>sd, >val);
-
+   break;
case V4L2_CID_BIN_FACTOR_HORZ:
ret = ov5693_g_bin_factor_x(>sd, >val);
break;
-- 
2.10.2

Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates

[Steve Longerbeam]

On 03/13/2017 06:55 AM, Philipp Zabel wrote:

On Mon, 2017-03-13 at 13:27 +, Russell King - ARM Linux wrote:

On Mon, Mar 13, 2017 at 03:16:48PM +0200, Sakari Ailus wrote:

The vast majority of existing drivers do not implement them nor the user
space expects having to set them. Making that mandatory would break existing
user space.

In addition, that does not belong to link validation either: link validation
should only include static properties of the link that are required for
correct hardware operation. Frame rate is not such property: hardware that
supports the MC interface generally does not recognise such concept (with
the exception of some sensors). Additionally, it is dynamic: the frame rate
can change during streaming, making its validation at streamon time useless.


So how do we configure the CSI, which can do frame skipping?

With what you're proposing, it means it's possible to configure the
camera sensor source pad to do 50fps.  Configure the CSI sink pad to
an arbitary value, such as 30fps, and configure the CSI source pad to
15fps.

What you actually get out of the CSI is 25fps, which bears very little
with the actual values used on the CSI source pad.

You could say "CSI should ask the camera sensor" - well, that's fine
if it's immediately downstream, but otherwise we'd need to go walking
down the graph to find something that resembles its source - there may
be mux and CSI2 interface subdev blocks in that path.  Or we just accept
that frame rates are completely arbitary and bear no useful meaning what
so ever.


Which would include the frame interval returned by VIDIOC_G_PARM on the
connected video device, as that gets its information from the CSI output
pad's frame interval.



I'm kinda in the middle on this topic. I agree with Sakari that
frame rate can fluctuate, but that should only be temporary. If
the frame rate permanently shifts from what a subdev reports via
g_frame_interval, then that is a system problem. So I agree with
Phillip and Russell that a link validation of frame interval still
makes sense.

But I also have to agree with Sakari that a subdev that has no
control over frame rate has no business implementing those ops.

And then I agree with Russell that for subdevs that do have control
over frame rate, they would have to walk the graph to find the frame
rate source.

So we're stuck in a broken situation: either the subdevs have to walk
the graph to find the source of frame rate, or s_frame_interval
would have to be mandatory and validated between pads, same as set_fmt.

Steve

[PATCH] staging/atomisp: remove redundant null check on frame

[Colin King]

From: Colin Ian King 

There is no need to perform a null check on frame as there is an earlier
null check check and return hence making the null check redundant.
Remove it.

Detected by CoverityScan, CID#1416563 ("Logically Dead Code")

Signed-off-by: Colin Ian King 
---
 .../media/atomisp/pci/atomisp2/css2400/runtime/frame/src/frame.c   | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/frame/src/frame.c 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/frame/src/frame.c
index 25e9d88..604bde6 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/frame/src/frame.c
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/frame/src/frame.c
@@ -159,8 +159,7 @@ enum ia_css_err ia_css_frame_allocate(struct ia_css_frame 
**frame,
 
 #ifndef ISP2401
ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
- "ia_css_frame_allocate() leave: frame=%p\n",
- frame ? *frame : (void *)-1);
+ "ia_css_frame_allocate() leave: frame=%p\n", *frame);
 #else
if ((*frame != NULL) && err == IA_CSS_SUCCESS)
ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
-- 
2.10.2

Re: [PATCH] staging: atomisp: use k{v}zalloc instead of k{v}alloc and memset

[Alan Cox]

On Mon, 2017-03-13 at 19:54 +0900, Daeseok Youn wrote:
> If the atomisp_kernel_zalloc() has "true" as a second parameter, it
> tries to allocate zeroing memory from kmalloc(vmalloc) and memset.
> But using kzalloc is rather than kmalloc followed by memset with 0.
> (vzalloc is for same reason with kzalloc)

This is true but please don't apply this. There are about five other
layers of indirection for memory allocators that want removing first so
that the driver just uses the correct kmalloc/kzalloc/kv* functions in
the right places.

Alan

Re: [PATCH] staging: atomisp: potential underflow in atomisp_get_metadata_by_type()

[Alan Cox]

On Mon, 2017-03-13 at 15:34 +0300, Dan Carpenter wrote:
> md_type is an enum.  On my tests, GCC treats it as unsigned but
> according to the C standard it's an implementation dependant thing so
> we
> should check for negatives.

Can do but the kernel assumes GNU C everywhere else.

Acked-by: Alan Cox 

Alan

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Hans Verkuil]

On 03/13/2017 06:06 PM, Steve Longerbeam wrote:
> 
> 
> On 03/13/2017 03:53 AM, Hans Verkuil wrote:
>> On 03/13/2017 11:45 AM, Russell King - ARM Linux wrote:
>>> On Mon, Mar 13, 2017 at 11:02:34AM +0100, Hans Verkuil wrote:
 On 03/11/2017 07:14 PM, Steve Longerbeam wrote:
> The event must be user visible, otherwise the user has no indication
> the error, and can't correct it by stream restart.

 In that case the driver can detect this and call vb2_queue_error. It's
 what it is there for.

 The event doesn't help you since only this driver has this issue. So nobody
 will watch this event, unless it is sw specifically written for this SoC.

 Much better to call vb2_queue_error to signal a fatal error (which this
 apparently is) since there are more drivers that do this, and vivid 
 supports
 triggering this condition as well.
>>>
>>> So today, I can fiddle around with the IMX219 registers to help gain
>>> an understanding of how this sensor works.  Several of the registers
>>> (such as the PLL setup [*]) require me to disable streaming on the
>>> sensor while changing them.
>>>
>>> This is something I've done many times while testing various ideas,
>>> and is my primary way of figuring out and testing such things.
>>>
>>> Whenever I resume streaming (provided I've let the sensor stop
>>> streaming at a frame boundary) it resumes as if nothing happened.  If I
>>> stop the sensor mid-frame, then I get the rolling issue that Steve
>>> reports, but once the top of the frame becomes aligned with the top of
>>> the capture, everything then becomes stable again as if nothing happened.
>>>
>>> The side effect of what you're proposing is that when I disable streaming
>>> at the sensor by poking at its registers, rather than the capture just
>>> stopping, an error is going to be delivered to gstreamer, and gstreamer
>>> is going to exit, taking the entire capture process down.
>>>
>>> This severely restricts the ability to be able to develop and test
>>> sensor drivers.
>>>
>>> So, I strongly disagree with you.
>>>
>>> Loss of capture frames is not necessarily a fatal error - as I have been
>>> saying repeatedly.  In Steve's case, there's some unknown interaction
>>> between the source and iMX6 hardware that is causing the instability,
>>> but that is simply not true of other sources, and I oppose any idea that
>>> we should cripple the iMX6 side of the capture based upon just one
>>> hardware combination where this is a problem.
>>>
>>> Steve suggested that the problem could be in the iMX6 CSI block - and I
>>> note comparing Steve's code with the code in FSL's repository that there
>>> are some changes that are missing in Steve's code to do with the CCIR656
>>> sync code setup, particularly for >8 bit.  The progressive CCIR656 8-bit
>>> setup looks pretty similar though - but I think what needs to be asked
>>> is whether the same problem is visible using the FSL/NXP vendor kernel.
>>>
>>>
>>> * - the PLL setup is something that requires research at the moment.
>>> Sony's official position (even to their customers) is that they do not
>>> supply the necessary information, instead they expect customers to tell
>>> them the capture settings they want, and Sony will throw the values into
>>> a spreadsheet, and they'll supply the register settings back to the
>>> customer.  Hence, the only way to proceed with a generic driver for
>>> this sensor is to experiment, and experimenting requires the ability to
>>> pause the stream at the sensor while making changes.  Take this away,
>>> and we're stuck with the tables-of-register-settings-for-set-of-fixed-
>>> capture-settings approach.  I've made a lot of progress away from this
>>> which is all down to the flexibility afforded by _not_ killing the
>>> capture process.
>>>
>>
>> In other words: Steve should either find a proper fix for this, or only
>> call vb2_queue_error in this specific case. Sending an event that nobody
>> will know how to handle or what to do with is pretty pointless IMHO.
>>
>> Let's just give him time to try and figure out the real issue here.
> 
> 
> This is a long-standing issue, I've traveled to Hildesheim working with
> our customer to try and get to the bottom of it. I can go into a lot of
> details from those trips, we probed the bt.656 bus with a logic analyzer
> and I can share those results with anyone who asks. But the results of
> those investigations indicate the CSI is not handling the SAV/EAV sync
> codes correctly - if there is a shift in the line position at which
> those codes occur, the CSI/IPU does not abort the frame capture DMA
> and start from the new sync code position, it just continues to capture
> lines until the programmed number of lines are transferred, hence you
> get these split images. Freescale also informed us of a mechanism in the
> IPU that will add lines if it detects these short frames, until the
> programmed number of lines are reached. Apparently that is 

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Steve Longerbeam]

On 03/13/2017 03:53 AM, Hans Verkuil wrote:

On 03/13/2017 11:45 AM, Russell King - ARM Linux wrote:

On Mon, Mar 13, 2017 at 11:02:34AM +0100, Hans Verkuil wrote:

On 03/11/2017 07:14 PM, Steve Longerbeam wrote:

The event must be user visible, otherwise the user has no indication
the error, and can't correct it by stream restart.


In that case the driver can detect this and call vb2_queue_error. It's
what it is there for.

The event doesn't help you since only this driver has this issue. So nobody
will watch this event, unless it is sw specifically written for this SoC.

Much better to call vb2_queue_error to signal a fatal error (which this
apparently is) since there are more drivers that do this, and vivid supports
triggering this condition as well.


So today, I can fiddle around with the IMX219 registers to help gain
an understanding of how this sensor works.  Several of the registers
(such as the PLL setup [*]) require me to disable streaming on the
sensor while changing them.

This is something I've done many times while testing various ideas,
and is my primary way of figuring out and testing such things.

Whenever I resume streaming (provided I've let the sensor stop
streaming at a frame boundary) it resumes as if nothing happened.  If I
stop the sensor mid-frame, then I get the rolling issue that Steve
reports, but once the top of the frame becomes aligned with the top of
the capture, everything then becomes stable again as if nothing happened.

The side effect of what you're proposing is that when I disable streaming
at the sensor by poking at its registers, rather than the capture just
stopping, an error is going to be delivered to gstreamer, and gstreamer
is going to exit, taking the entire capture process down.

This severely restricts the ability to be able to develop and test
sensor drivers.

So, I strongly disagree with you.

Loss of capture frames is not necessarily a fatal error - as I have been
saying repeatedly.  In Steve's case, there's some unknown interaction
between the source and iMX6 hardware that is causing the instability,
but that is simply not true of other sources, and I oppose any idea that
we should cripple the iMX6 side of the capture based upon just one
hardware combination where this is a problem.

Steve suggested that the problem could be in the iMX6 CSI block - and I
note comparing Steve's code with the code in FSL's repository that there
are some changes that are missing in Steve's code to do with the CCIR656
sync code setup, particularly for >8 bit.  The progressive CCIR656 8-bit
setup looks pretty similar though - but I think what needs to be asked
is whether the same problem is visible using the FSL/NXP vendor kernel.


* - the PLL setup is something that requires research at the moment.
Sony's official position (even to their customers) is that they do not
supply the necessary information, instead they expect customers to tell
them the capture settings they want, and Sony will throw the values into
a spreadsheet, and they'll supply the register settings back to the
customer.  Hence, the only way to proceed with a generic driver for
this sensor is to experiment, and experimenting requires the ability to
pause the stream at the sensor while making changes.  Take this away,
and we're stuck with the tables-of-register-settings-for-set-of-fixed-
capture-settings approach.  I've made a lot of progress away from this
which is all down to the flexibility afforded by _not_ killing the
capture process.



In other words: Steve should either find a proper fix for this, or only
call vb2_queue_error in this specific case. Sending an event that nobody
will know how to handle or what to do with is pretty pointless IMHO.

Let's just give him time to try and figure out the real issue here.



This is a long-standing issue, I've traveled to Hildesheim working with
our customer to try and get to the bottom of it. I can go into a lot of
details from those trips, we probed the bt.656 bus with a logic analyzer
and I can share those results with anyone who asks. But the results of
those investigations indicate the CSI is not handling the SAV/EAV sync
codes correctly - if there is a shift in the line position at which
those codes occur, the CSI/IPU does not abort the frame capture DMA
and start from the new sync code position, it just continues to capture
lines until the programmed number of lines are transferred, hence you
get these split images. Freescale also informed us of a mechanism in the
IPU that will add lines if it detects these short frames, until the
programmed number of lines are reached. Apparently that is what creates 
the rolling effect, but this rolling can last for up to a full minute,

which is completely unacceptable, it must be corrected as soon as
possible.

So the only thing we could come up with was to monitor frame intervals,
this is purely empirical, but we observed that frame intervals drop
by approx. one line time (~60 usec) when these short frames are

[PATCH v7 5/9] media: venus: vdec: add video decoder files

[Stanimir Varbanov]

This consists of video decoder implementation plus decoder
controls.

Signed-off-by: Stanimir Varbanov 
---
 drivers/media/platform/qcom/venus/vdec.c   | 1091 
 drivers/media/platform/qcom/venus/vdec.h   |   23 +
 drivers/media/platform/qcom/venus/vdec_ctrls.c |  149 
 3 files changed, 1263 insertions(+)
 create mode 100644 drivers/media/platform/qcom/venus/vdec.c
 create mode 100644 drivers/media/platform/qcom/venus/vdec.h
 create mode 100644 drivers/media/platform/qcom/venus/vdec_ctrls.c

diff --git a/drivers/media/platform/qcom/venus/vdec.c 
b/drivers/media/platform/qcom/venus/vdec.c
new file mode 100644
index ..ec5203f2ba81
--- /dev/null
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -0,0 +1,1091 @@
+/*
+ * Copyright (c) 2012-2016, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2017 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "hfi_venus_io.h"
+#include "core.h"
+#include "helpers.h"
+#include "vdec.h"
+
+static u32 get_framesize_uncompressed(unsigned int plane, u32 width, u32 
height)
+{
+   u32 y_stride, uv_stride, y_plane;
+   u32 y_sclines, uv_sclines, uv_plane;
+   u32 size;
+
+   y_stride = ALIGN(width, 128);
+   uv_stride = ALIGN(width, 128);
+   y_sclines = ALIGN(height, 32);
+   uv_sclines = ALIGN(((height + 1) >> 1), 16);
+
+   y_plane = y_stride * y_sclines;
+   uv_plane = uv_stride * uv_sclines + SZ_4K;
+   size = y_plane + uv_plane + SZ_8K;
+
+   return ALIGN(size, SZ_4K);
+}
+
+static u32 get_framesize_compressed(unsigned int width, unsigned int height)
+{
+   return ((width * height * 3 / 2) / 2) + 128;
+}
+
+static const struct venus_format vdec_formats[] = {
+   {
+   .pixfmt = V4L2_PIX_FMT_NV12,
+   .num_planes = 1,
+   .type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
+   }, {
+   .pixfmt = V4L2_PIX_FMT_MPEG4,
+   .num_planes = 1,
+   .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+   }, {
+   .pixfmt = V4L2_PIX_FMT_MPEG2,
+   .num_planes = 1,
+   .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+   }, {
+   .pixfmt = V4L2_PIX_FMT_H263,
+   .num_planes = 1,
+   .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+   }, {
+   .pixfmt = V4L2_PIX_FMT_VC1_ANNEX_G,
+   .num_planes = 1,
+   .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+   }, {
+   .pixfmt = V4L2_PIX_FMT_VC1_ANNEX_L,
+   .num_planes = 1,
+   .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+   }, {
+   .pixfmt = V4L2_PIX_FMT_H264,
+   .num_planes = 1,
+   .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+   }, {
+   .pixfmt = V4L2_PIX_FMT_VP8,
+   .num_planes = 1,
+   .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+   }, {
+   .pixfmt = V4L2_PIX_FMT_XVID,
+   .num_planes = 1,
+   .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+   },
+};
+
+static const struct venus_format *find_format(u32 pixfmt, u32 type)
+{
+   const struct venus_format *fmt = vdec_formats;
+   unsigned int size = ARRAY_SIZE(vdec_formats);
+   unsigned int i;
+
+   for (i = 0; i < size; i++) {
+   if (fmt[i].pixfmt == pixfmt)
+   break;
+   }
+
+   if (i == size || fmt[i].type != type)
+   return NULL;
+
+   return [i];
+}
+
+static const struct venus_format *find_format_by_index(int index, u32 type)
+{
+   const struct venus_format *fmt = vdec_formats;
+   unsigned int size = ARRAY_SIZE(vdec_formats);
+   int i, k = 0;
+
+   if (index < 0 || index > size)
+   return NULL;
+
+   for (i = 0; i < size; i++) {
+   if (fmt[i].type != type)
+   continue;
+   if (k == index)
+   break;
+   k++;
+   }
+
+   if (i == size)
+   return NULL;
+
+   return [i];
+}
+
+static const struct venus_format *
+vdec_try_fmt_common(struct venus_inst *inst, struct v4l2_format *f)
+{
+   struct v4l2_pix_format_mplane *pixmp = >fmt.pix_mp;
+   struct v4l2_plane_pix_format *pfmt = pixmp->plane_fmt;
+   const struct venus_format *fmt;
+   unsigned int p;
+
+   

[PATCH v7 6/9] media: venus: venc: add video encoder files

[Stanimir Varbanov]

This adds encoder part of the driver plus encoder controls.

Signed-off-by: Stanimir Varbanov 
---
 drivers/media/platform/qcom/venus/venc.c   | 1231 
 drivers/media/platform/qcom/venus/venc.h   |   23 +
 drivers/media/platform/qcom/venus/venc_ctrls.c |  258 +
 3 files changed, 1512 insertions(+)
 create mode 100644 drivers/media/platform/qcom/venus/venc.c
 create mode 100644 drivers/media/platform/qcom/venus/venc.h
 create mode 100644 drivers/media/platform/qcom/venus/venc_ctrls.c

diff --git a/drivers/media/platform/qcom/venus/venc.c 
b/drivers/media/platform/qcom/venus/venc.c
new file mode 100644
index ..dddfc3554953
--- /dev/null
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -0,0 +1,1231 @@
+/*
+ * Copyright (c) 2012-2016, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2017 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "hfi_venus_io.h"
+#include "core.h"
+#include "helpers.h"
+#include "venc.h"
+
+#define NUM_B_FRAMES_MAX   4
+
+static u32 get_framesize_uncompressed(unsigned int plane, u32 width, u32 
height)
+{
+   u32 y_stride, uv_stride, y_plane;
+   u32 y_sclines, uv_sclines, uv_plane;
+   u32 size;
+
+   y_stride = ALIGN(width, 128);
+   uv_stride = ALIGN(width, 128);
+   y_sclines = ALIGN(height, 32);
+   uv_sclines = ALIGN(((height + 1) >> 1), 16);
+
+   y_plane = y_stride * y_sclines;
+   uv_plane = uv_stride * uv_sclines + SZ_4K;
+   size = y_plane + uv_plane + SZ_8K;
+   size = ALIGN(size, SZ_4K);
+
+   return size;
+}
+
+static u32 get_framesize_compressed(u32 width, u32 height)
+{
+   u32 sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2 / 2;
+
+   return ALIGN(sz, SZ_4K);
+}
+
+static const struct venus_format venc_formats[] = {
+   {
+   .pixfmt = V4L2_PIX_FMT_NV12,
+   .num_planes = 1,
+   .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+   }, {
+   .pixfmt = V4L2_PIX_FMT_MPEG4,
+   .num_planes = 1,
+   .type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
+   }, {
+   .pixfmt = V4L2_PIX_FMT_H263,
+   .num_planes = 1,
+   .type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
+   }, {
+   .pixfmt = V4L2_PIX_FMT_H264,
+   .num_planes = 1,
+   .type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
+   }, {
+   .pixfmt = V4L2_PIX_FMT_VP8,
+   .num_planes = 1,
+   .type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
+   },
+};
+
+static const struct venus_format *find_format(u32 pixfmt, int type)
+{
+   const struct venus_format *fmt = venc_formats;
+   unsigned int size = ARRAY_SIZE(venc_formats);
+   unsigned int i;
+
+   for (i = 0; i < size; i++) {
+   if (fmt[i].pixfmt == pixfmt)
+   break;
+   }
+
+   if (i == size || fmt[i].type != type)
+   return NULL;
+
+   return [i];
+}
+
+static const struct venus_format *find_format_by_index(int index, int type)
+{
+   const struct venus_format *fmt = venc_formats;
+   unsigned int size = ARRAY_SIZE(venc_formats);
+   int i, k = 0;
+
+   if (index < 0 || index > size)
+   return NULL;
+
+   for (i = 0; i < size; i++) {
+   if (fmt[i].type != type)
+   continue;
+   if (k == index)
+   break;
+   k++;
+   }
+
+   if (i == size)
+   return NULL;
+
+   return [i];
+}
+
+static int venc_v4l2_to_hfi(int id, int value)
+{
+   switch (id) {
+   case V4L2_CID_MPEG_VIDEO_MPEG4_LEVEL:
+   switch (value) {
+   case V4L2_MPEG_VIDEO_MPEG4_LEVEL_0:
+   default:
+   return HFI_MPEG4_LEVEL_0;
+   case V4L2_MPEG_VIDEO_MPEG4_LEVEL_0B:
+   return HFI_MPEG4_LEVEL_0b;
+   case V4L2_MPEG_VIDEO_MPEG4_LEVEL_1:
+   return HFI_MPEG4_LEVEL_1;
+   case V4L2_MPEG_VIDEO_MPEG4_LEVEL_2:
+   return HFI_MPEG4_LEVEL_2;
+   case V4L2_MPEG_VIDEO_MPEG4_LEVEL_3:
+   return HFI_MPEG4_LEVEL_3;
+   case V4L2_MPEG_VIDEO_MPEG4_LEVEL_4:
+   return HFI_MPEG4_LEVEL_4;
+   case 

[PATCH v7 7/9] media: venus: hfi: add Host Firmware Interface (HFI)

[Stanimir Varbanov]

This is the implementation of HFI. It is charged with the
responsibility to comunicate with the firmware through an
interface commands and messages.

 - hfi.c has interface functions used by the core, decoder
and encoder parts to comunicate with the firmware. For example
there are functions for session and core initialisation.

 - hfi_cmds has packetization operations which preparing
packets to be send from host to firmware.

 - hfi_msgs takes care of messages sent from firmware to the
host.

Signed-off-by: Stanimir Varbanov 
---
 drivers/media/platform/qcom/venus/hfi.c|  520 ++
 drivers/media/platform/qcom/venus/hfi.h|  174 
 drivers/media/platform/qcom/venus/hfi_cmds.c   | 1256 
 drivers/media/platform/qcom/venus/hfi_cmds.h   |  304 ++
 drivers/media/platform/qcom/venus/hfi_helper.h | 1050 
 drivers/media/platform/qcom/venus/hfi_msgs.c   | 1058 
 drivers/media/platform/qcom/venus/hfi_msgs.h   |  283 ++
 7 files changed, 4645 insertions(+)
 create mode 100644 drivers/media/platform/qcom/venus/hfi.c
 create mode 100644 drivers/media/platform/qcom/venus/hfi.h
 create mode 100644 drivers/media/platform/qcom/venus/hfi_cmds.c
 create mode 100644 drivers/media/platform/qcom/venus/hfi_cmds.h
 create mode 100644 drivers/media/platform/qcom/venus/hfi_helper.h
 create mode 100644 drivers/media/platform/qcom/venus/hfi_msgs.c
 create mode 100644 drivers/media/platform/qcom/venus/hfi_msgs.h

diff --git a/drivers/media/platform/qcom/venus/hfi.c 
b/drivers/media/platform/qcom/venus/hfi.c
new file mode 100644
index ..fc2ae08c4679
--- /dev/null
+++ b/drivers/media/platform/qcom/venus/hfi.c
@@ -0,0 +1,520 @@
+/*
+ * Copyright (c) 2012-2016, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2017 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "core.h"
+#include "hfi.h"
+#include "hfi_cmds.h"
+#include "hfi_venus.h"
+
+#define TIMEOUTmsecs_to_jiffies(1000)
+
+static u32 to_codec_type(u32 pixfmt)
+{
+   switch (pixfmt) {
+   case V4L2_PIX_FMT_H264:
+   case V4L2_PIX_FMT_H264_NO_SC:
+   return HFI_VIDEO_CODEC_H264;
+   case V4L2_PIX_FMT_H263:
+   return HFI_VIDEO_CODEC_H263;
+   case V4L2_PIX_FMT_MPEG1:
+   return HFI_VIDEO_CODEC_MPEG1;
+   case V4L2_PIX_FMT_MPEG2:
+   return HFI_VIDEO_CODEC_MPEG2;
+   case V4L2_PIX_FMT_MPEG4:
+   return HFI_VIDEO_CODEC_MPEG4;
+   case V4L2_PIX_FMT_VC1_ANNEX_G:
+   case V4L2_PIX_FMT_VC1_ANNEX_L:
+   return HFI_VIDEO_CODEC_VC1;
+   case V4L2_PIX_FMT_VP8:
+   return HFI_VIDEO_CODEC_VP8;
+   case V4L2_PIX_FMT_XVID:
+   return HFI_VIDEO_CODEC_DIVX;
+   default:
+   return 0;
+   }
+}
+
+int hfi_core_init(struct venus_core *core)
+{
+   int ret = 0;
+
+   mutex_lock(>lock);
+
+   if (core->state >= CORE_INIT)
+   goto unlock;
+
+   reinit_completion(>done);
+
+   ret = core->ops->core_init(core);
+   if (ret)
+   goto unlock;
+
+   ret = wait_for_completion_timeout(>done, TIMEOUT);
+   if (!ret) {
+   ret = -ETIMEDOUT;
+   goto unlock;
+   }
+
+   ret = 0;
+
+   if (core->error != HFI_ERR_NONE) {
+   ret = -EIO;
+   goto unlock;
+   }
+
+   core->state = CORE_INIT;
+unlock:
+   mutex_unlock(>lock);
+   return ret;
+}
+
+static int core_deinit_wait_atomic_t(atomic_t *p)
+{
+   schedule();
+   return 0;
+}
+
+int hfi_core_deinit(struct venus_core *core, bool blocking)
+{
+   int ret = 0, empty;
+
+   mutex_lock(>lock);
+
+   if (core->state == CORE_UNINIT)
+   goto unlock;
+
+   empty = list_empty(>instances);
+
+   if (!empty && blocking == false) {
+   ret = -EBUSY;
+   goto unlock;
+   }
+
+   if (!empty) {
+   mutex_unlock(>lock);
+   wait_on_atomic_t(>insts_count, core_deinit_wait_atomic_t,
+TASK_UNINTERRUPTIBLE);
+   mutex_lock(>lock);
+   }
+
+   ret = core->ops->core_deinit(core);
+
+   if (!ret)
+   core->state = CORE_UNINIT;
+
+unlock:
+   mutex_unlock(>lock);
+   return ret;
+}
+
+int hfi_core_suspend(struct venus_core *core)
+{
+   if (core->state != 

[PATCH v7 4/9] media: venus: adding core part and helper functions

[Stanimir Varbanov]

 * core.c has implemented the platform dirver methods, file
operations and v4l2 registration.

 * helpers.c has implemented common helper functions for:
   - buffer management

   - vb2_ops and functions for format propagation,

   - functions for allocating and freeing buffers for
   internal usage. The buffer parameters describing internal
   buffers depends on current format, resolution and codec.

   - functions for calculation of current load of the
   hardware. Depending on the count of instances and
   resolutions it selects the best clock rate for the video
   core.

 * firmware loader

Signed-off-by: Stanimir Varbanov 
---
 drivers/media/platform/qcom/venus/core.c | 386 
 drivers/media/platform/qcom/venus/core.h | 306 +
 drivers/media/platform/qcom/venus/firmware.c | 107 +
 drivers/media/platform/qcom/venus/firmware.h |  22 +
 drivers/media/platform/qcom/venus/helpers.c  | 632 +++
 drivers/media/platform/qcom/venus/helpers.h  |  41 ++
 6 files changed, 1494 insertions(+)
 create mode 100644 drivers/media/platform/qcom/venus/core.c
 create mode 100644 drivers/media/platform/qcom/venus/core.h
 create mode 100644 drivers/media/platform/qcom/venus/firmware.c
 create mode 100644 drivers/media/platform/qcom/venus/firmware.h
 create mode 100644 drivers/media/platform/qcom/venus/helpers.c
 create mode 100644 drivers/media/platform/qcom/venus/helpers.h

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
new file mode 100644
index ..557b6ec4cc48
--- /dev/null
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -0,0 +1,386 @@
+/*
+ * Copyright (c) 2012-2016, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2017 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "core.h"
+#include "vdec.h"
+#include "venc.h"
+#include "firmware.h"
+
+static const struct hfi_core_ops venus_core_ops;
+
+static void venus_sys_error_handler(struct work_struct *work)
+{
+   struct venus_core *core =
+   container_of(work, struct venus_core, work.work);
+   int ret;
+
+   dev_warn(core->dev, "system error occurred, starting recovery!\n");
+
+   pm_runtime_get_sync(core->dev);
+
+   hfi_core_deinit(core, true);
+
+   hfi_destroy(core);
+
+   mutex_lock(>lock);
+
+   venus_shutdown(>dev_fw);
+
+   pm_runtime_put_sync(core->dev);
+
+   ret = hfi_create(core, _core_ops);
+
+   pm_runtime_get_sync(core->dev);
+
+   ret = venus_boot(core->dev, >dev_fw);
+
+   ret = hfi_core_resume(core, true);
+
+   enable_irq(core->irq);
+
+   mutex_unlock(>lock);
+
+   ret = hfi_core_init(core);
+   if (ret)
+   dev_err(core->dev, "hfi_core_init (%d)\n", ret);
+
+   pm_runtime_put_sync(core->dev);
+
+   core->sys_error = false;
+}
+
+static void venus_event_notify(struct venus_core *core, u32 event)
+{
+   struct venus_inst *inst;
+
+   switch (event) {
+   case EVT_SYS_WATCHDOG_TIMEOUT:
+   case EVT_SYS_ERROR:
+   break;
+   default:
+   return;
+   }
+
+   mutex_lock(>lock);
+   core->sys_error = true;
+   list_for_each_entry(inst, >instances, list)
+   inst->ops->event_notify(inst, EVT_SESSION_ERROR, NULL);
+   mutex_unlock(>lock);
+
+   disable_irq_nosync(core->irq);
+
+   /*
+* Sleep for 5 sec to ensure venus has completed any pending cache
+* operations. Without this sleep, we see device reset when firmware is
+* unloaded after a system error.
+*/
+   schedule_delayed_work(>work, msecs_to_jiffies(100));
+}
+
+static const struct hfi_core_ops venus_core_ops = {
+   .event_notify = venus_event_notify,
+};
+
+static int venus_clks_get(struct venus_core *core)
+{
+   const struct venus_resources *res = core->res;
+   struct device *dev = core->dev;
+   unsigned int i;
+
+   for (i = 0; i < res->clks_num; i++) {
+   core->clks[i] = devm_clk_get(dev, res->clks[i]);
+   if (IS_ERR(core->clks[i]))
+   return PTR_ERR(core->clks[i]);
+   }
+
+   return 0;
+}
+
+static int venus_clks_enable(struct venus_core *core)
+{
+   const struct venus_resources *res = core->res;
+   unsigned int i;
+   int ret;
+
+   

[PATCH v7 9/9] media: venus: enable building of Venus video driver

[Stanimir Varbanov]

This adds Venus driver Makefile and changes v4l2 platform
Makefile/Kconfig in order to enable building of the driver.

Signed-off-by: Stanimir Varbanov 
---
 drivers/media/platform/Kconfig | 14 ++
 drivers/media/platform/Makefile|  2 ++
 drivers/media/platform/qcom/venus/Makefile | 11 +++
 3 files changed, 27 insertions(+)
 create mode 100644 drivers/media/platform/qcom/venus/Makefile

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 53f6f12bff0d..8a6c3d664307 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -447,6 +447,20 @@ config VIDEO_TI_VPE_DEBUG
---help---
  Enable debug messages on VPE driver.
 
+config VIDEO_QCOM_VENUS
+   tristate "Qualcomm Venus V4L2 encoder/decoder driver"
+   depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA
+   depends on ARCH_QCOM && OF
+   depends on IOMMU_DMA
+   select QCOM_MDT_LOADER
+   select VIDEOBUF2_DMA_SG
+   select V4L2_MEM2MEM_DEV
+   ---help---
+ This is a V4L2 driver for Qualcomm Venus video accelerator
+ hardware. It accelerates encoding and decoding operations
+ on various Qualcomm SoCs.
+ To compile this driver as a module choose m here.
+
 endif # V4L_MEM2MEM_DRIVERS
 
 # TI VIDEO PORT Helper Modules
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 8959f6e6692a..bd5cae68db8a 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -73,3 +73,5 @@ obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC)   += mtk-vcodec/
 obj-$(CONFIG_VIDEO_MEDIATEK_MDP)   += mtk-mdp/
 
 obj-$(CONFIG_VIDEO_MEDIATEK_JPEG)  += mtk-jpeg/
+
+obj-$(CONFIG_VIDEO_QCOM_VENUS) += qcom/venus/
diff --git a/drivers/media/platform/qcom/venus/Makefile 
b/drivers/media/platform/qcom/venus/Makefile
new file mode 100644
index ..0fe9afb83697
--- /dev/null
+++ b/drivers/media/platform/qcom/venus/Makefile
@@ -0,0 +1,11 @@
+# Makefile for Qualcomm Venus driver
+
+venus-core-objs += core.o helpers.o firmware.o \
+  hfi_venus.o hfi_msgs.o hfi_cmds.o hfi.o
+
+venus-dec-objs += vdec.o vdec_ctrls.o
+venus-enc-objs += venc.o venc_ctrls.o
+
+obj-$(CONFIG_VIDEO_QCOM_VENUS) += venus-core.o
+obj-$(CONFIG_VIDEO_QCOM_VENUS) += venus-dec.o
+obj-$(CONFIG_VIDEO_QCOM_VENUS) += venus-enc.o
-- 
2.7.4

[PATCH v7 8/9] media: venus: hfi: add Venus HFI files

[Stanimir Varbanov]

Here is the implementation of Venus video accelerator low-level
functionality. It contanins code which setup the registers and
startup uthe processor, allocate and manipulates with the shared
memory used for sending commands and receiving messages.

Signed-off-by: Stanimir Varbanov 
---
 drivers/media/platform/qcom/venus/hfi_venus.c| 1570 ++
 drivers/media/platform/qcom/venus/hfi_venus.h|   23 +
 drivers/media/platform/qcom/venus/hfi_venus_io.h |  113 ++
 3 files changed, 1706 insertions(+)
 create mode 100644 drivers/media/platform/qcom/venus/hfi_venus.c
 create mode 100644 drivers/media/platform/qcom/venus/hfi_venus.h
 create mode 100644 drivers/media/platform/qcom/venus/hfi_venus_io.h

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c 
b/drivers/media/platform/qcom/venus/hfi_venus.c
new file mode 100644
index ..74533359d12f
--- /dev/null
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -0,0 +1,1570 @@
+/*
+ * Copyright (c) 2012-2016, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2017 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "core.h"
+#include "hfi_cmds.h"
+#include "hfi_msgs.h"
+#include "hfi_venus.h"
+#include "hfi_venus_io.h"
+
+#define HFI_MASK_QHDR_TX_TYPE  0xff00
+#define HFI_MASK_QHDR_RX_TYPE  0x00ff
+#define HFI_MASK_QHDR_PRI_TYPE 0xff00
+#define HFI_MASK_QHDR_ID_TYPE  0x00ff
+
+#define HFI_HOST_TO_CTRL_CMD_Q 0
+#define HFI_CTRL_TO_HOST_MSG_Q 1
+#define HFI_CTRL_TO_HOST_DBG_Q 2
+#define HFI_MASK_QHDR_STATUS   0x00ff
+
+#define IFACEQ_NUM 3
+#define IFACEQ_CMD_IDX 0
+#define IFACEQ_MSG_IDX 1
+#define IFACEQ_DBG_IDX 2
+#define IFACEQ_MAX_BUF_COUNT   50
+#define IFACEQ_MAX_PARALLEL_CLNTS  16
+#define IFACEQ_DFLT_QHDR   0x0101
+
+#define POLL_INTERVAL_US   50
+
+#define IFACEQ_MAX_PKT_SIZE1024
+#define IFACEQ_MED_PKT_SIZE768
+#define IFACEQ_MIN_PKT_SIZE8
+#define IFACEQ_VAR_SMALL_PKT_SIZE  100
+#define IFACEQ_VAR_LARGE_PKT_SIZE  512
+#define IFACEQ_VAR_HUGE_PKT_SIZE   (1024 * 12)
+
+enum tzbsp_video_state {
+   TZBSP_VIDEO_STATE_SUSPEND = 0,
+   TZBSP_VIDEO_STATE_RESUME
+};
+
+struct hfi_queue_table_header {
+   u32 version;
+   u32 size;
+   u32 qhdr0_offset;
+   u32 qhdr_size;
+   u32 num_q;
+   u32 num_active_q;
+};
+
+struct hfi_queue_header {
+   u32 status;
+   u32 start_addr;
+   u32 type;
+   u32 q_size;
+   u32 pkt_size;
+   u32 pkt_drop_cnt;
+   u32 rx_wm;
+   u32 tx_wm;
+   u32 rx_req;
+   u32 tx_req;
+   u32 rx_irq_status;
+   u32 tx_irq_status;
+   u32 read_idx;
+   u32 write_idx;
+};
+
+#define IFACEQ_TABLE_SIZE  \
+   (sizeof(struct hfi_queue_table_header) +\
+sizeof(struct hfi_queue_header) * IFACEQ_NUM)
+
+#define IFACEQ_QUEUE_SIZE  (IFACEQ_MAX_PKT_SIZE *  \
+   IFACEQ_MAX_BUF_COUNT * IFACEQ_MAX_PARALLEL_CLNTS)
+
+#define IFACEQ_GET_QHDR_START_ADDR(ptr, i) \
+   (void *)(((ptr) + sizeof(struct hfi_queue_table_header)) +  \
+   ((i) * sizeof(struct hfi_queue_header)))
+
+#define QDSS_SIZE  SZ_4K
+#define SFR_SIZE   SZ_4K
+#define QUEUE_SIZE \
+   (IFACEQ_TABLE_SIZE + (IFACEQ_QUEUE_SIZE * IFACEQ_NUM))
+
+#define ALIGNED_QDSS_SIZE  ALIGN(QDSS_SIZE, SZ_4K)
+#define ALIGNED_SFR_SIZE   ALIGN(SFR_SIZE, SZ_4K)
+#define ALIGNED_QUEUE_SIZE ALIGN(QUEUE_SIZE, SZ_4K)
+#define SHARED_QSIZE   ALIGN(ALIGNED_SFR_SIZE + ALIGNED_QUEUE_SIZE + \
+ ALIGNED_QDSS_SIZE, SZ_1M)
+
+struct mem_desc {
+   dma_addr_t da;  /* device address */
+   void *kva;  /* kernel virtual address */
+   u32 size;
+   unsigned long attrs;
+};
+
+struct iface_queue {
+   struct hfi_queue_header *qhdr;
+   struct mem_desc qmem;
+};
+
+enum venus_state {
+   VENUS_STATE_DEINIT = 1,
+   VENUS_STATE_INIT,
+};
+
+struct venus_hfi_device {
+   struct venus_core *core;
+   u32 irq_status;
+   u32 last_packet_type;
+   bool power_enabled;
+   bool suspended;
+   enum venus_state state;
+   struct mutex lock;
+   struct completion pwr_collapse_prep;
+   

[PATCH v7 1/9] media: v4l2-mem2mem: extend m2m APIs for more accurate buffer management

[Stanimir Varbanov]

this add functions for:
  - remove buffers from src/dst queue by index
  - remove exact buffer from src/dst queue

also extends m2m API to iterate over a list of src/dst buffers
in safely and non-safely manner.

Signed-off-by: Stanimir Varbanov 
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 37 ++
 include/media/v4l2-mem2mem.h   | 92 ++
 2 files changed, 129 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c 
b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 6bc27e7b2a33..f62e68aa04c4 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -126,6 +126,43 @@ void *v4l2_m2m_buf_remove(struct v4l2_m2m_queue_ctx *q_ctx)
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_buf_remove);
 
+void v4l2_m2m_buf_remove_by_buf(struct v4l2_m2m_queue_ctx *q_ctx,
+   struct vb2_v4l2_buffer *vbuf)
+{
+   struct v4l2_m2m_buffer *b;
+   unsigned long flags;
+
+   spin_lock_irqsave(_ctx->rdy_spinlock, flags);
+   b = container_of(vbuf, struct v4l2_m2m_buffer, vb);
+   list_del(>list);
+   q_ctx->num_rdy--;
+   spin_unlock_irqrestore(_ctx->rdy_spinlock, flags);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_buf_remove_by_buf);
+
+struct vb2_v4l2_buffer *
+v4l2_m2m_buf_remove_by_idx(struct v4l2_m2m_queue_ctx *q_ctx, unsigned int idx)
+
+{
+   struct v4l2_m2m_buffer *b, *tmp;
+   struct vb2_v4l2_buffer *ret = NULL;
+   unsigned long flags;
+
+   spin_lock_irqsave(_ctx->rdy_spinlock, flags);
+   list_for_each_entry_safe(b, tmp, _ctx->rdy_queue, list) {
+   if (b->vb.vb2_buf.index == idx) {
+   list_del(>list);
+   q_ctx->num_rdy--;
+   ret = >vb;
+   break;
+   }
+   }
+   spin_unlock_irqrestore(_ctx->rdy_spinlock, flags);
+
+   return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_buf_remove_by_idx);
+
 /*
  * Scheduling handlers
  */
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index 3ccd01bd245e..e157d5c9b224 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -437,6 +437,47 @@ static inline void *v4l2_m2m_next_dst_buf(struct 
v4l2_m2m_ctx *m2m_ctx)
 }
 
 /**
+ * v4l2_m2m_for_each_dst_buf() - iterate over a list of destination ready
+ * buffers
+ *
+ * @m2m_ctx: m2m context assigned to the instance given by struct _m2m_ctx
+ * @b: current buffer of type struct v4l2_m2m_buffer
+ */
+#define v4l2_m2m_for_each_dst_buf(m2m_ctx, b)  \
+   list_for_each_entry(b, _ctx->cap_q_ctx.rdy_queue, list)
+
+/**
+ * v4l2_m2m_for_each_src_buf() - iterate over a list of source ready buffers
+ *
+ * @m2m_ctx: m2m context assigned to the instance given by struct _m2m_ctx
+ * @b: current buffer of type struct v4l2_m2m_buffer
+ */
+#define v4l2_m2m_for_each_src_buf(m2m_ctx, b)  \
+   list_for_each_entry(b, _ctx->out_q_ctx.rdy_queue, list)
+
+/**
+ * v4l2_m2m_for_each_dst_buf_safe() - iterate over a list of destination ready
+ * buffers safely
+ *
+ * @m2m_ctx: m2m context assigned to the instance given by struct _m2m_ctx
+ * @b: current buffer of type struct v4l2_m2m_buffer
+ * @n: used as temporary storage
+ */
+#define v4l2_m2m_for_each_dst_buf_safe(m2m_ctx, b, n)  \
+   list_for_each_entry_safe(b, n, _ctx->cap_q_ctx.rdy_queue, list)
+
+/**
+ * v4l2_m2m_for_each_src_buf_safe() - iterate over a list of source ready
+ * buffers safely
+ *
+ * @m2m_ctx: m2m context assigned to the instance given by struct _m2m_ctx
+ * @b: current buffer of type struct v4l2_m2m_buffer
+ * @n: used as temporary storage
+ */
+#define v4l2_m2m_for_each_src_buf_safe(m2m_ctx, b, n)  \
+   list_for_each_entry_safe(b, n, _ctx->out_q_ctx.rdy_queue, list)
+
+/**
  * v4l2_m2m_get_src_vq() - return vb2_queue for source buffers
  *
  * @m2m_ctx: m2m context assigned to the instance given by struct _m2m_ctx
@@ -488,6 +529,57 @@ static inline void *v4l2_m2m_dst_buf_remove(struct 
v4l2_m2m_ctx *m2m_ctx)
return v4l2_m2m_buf_remove(_ctx->cap_q_ctx);
 }
 
+/**
+ * v4l2_m2m_buf_remove_by_buf() - take off exact buffer from the list of ready
+ * buffers
+ *
+ * @q_ctx: pointer to struct @v4l2_m2m_queue_ctx
+ * @vbuf: the buffer to be removed
+ */
+void v4l2_m2m_buf_remove_by_buf(struct v4l2_m2m_queue_ctx *q_ctx,
+   struct vb2_v4l2_buffer *vbuf);
+
+/**
+ * v4l2_m2m_src_buf_remove_by_buf() - take off exact source buffer from the 
list
+ * of ready buffers
+ *
+ * @m2m_ctx: m2m context assigned to the instance given by struct _m2m_ctx
+ * @vbuf: the buffer to be removed
+ */
+static inline void v4l2_m2m_src_buf_remove_by_buf(struct v4l2_m2m_ctx *m2m_ctx,
+ struct vb2_v4l2_buffer *vbuf)
+{
+   v4l2_m2m_buf_remove_by_buf(_ctx->out_q_ctx, vbuf);
+}
+
+/**
+ * v4l2_m2m_dst_buf_remove_by_buf() - take off exact destination buffer from 
the
+ * list of ready 

[PATCH v7 0/9] Qualcomm video decoder/encoder driver

[Stanimir Varbanov]

Hi all,

Here is seventh version of the patch-set - no functional changes in
v4l2 APIs.

The changes since v6 are.
  * changes in DT binding document - moved memory-region DT property
in video-codec node - see 2/9.
  * improved recovery mechanism. 
  * fixed various issues found during testing.

Build dependencies:
  - qcom_scm_set_remote_state is merged in Linux 4.11-rc1
  - qcom mdt_loader is merged in Linux 4.11-rc1

regards,
Stan
  
Stanimir Varbanov (9):
  media: v4l2-mem2mem: extend m2m APIs for more accurate buffer
management
  doc: DT: venus: binding document for Qualcomm video driver
  MAINTAINERS: Add Qualcomm Venus video accelerator driver
  media: venus: adding core part and helper functions
  media: venus: vdec: add video decoder files
  media: venus: venc: add video encoder files
  media: venus: hfi: add Host Firmware Interface (HFI)
  media: venus: hfi: add Venus HFI files
  media: venus: enable building of Venus video driver

 .../devicetree/bindings/media/qcom,venus.txt   |  107 ++
 MAINTAINERS|8 +
 drivers/media/platform/Kconfig |   14 +
 drivers/media/platform/Makefile|2 +
 drivers/media/platform/qcom/venus/Makefile |   11 +
 drivers/media/platform/qcom/venus/core.c   |  386 +
 drivers/media/platform/qcom/venus/core.h   |  306 
 drivers/media/platform/qcom/venus/firmware.c   |  107 ++
 drivers/media/platform/qcom/venus/firmware.h   |   22 +
 drivers/media/platform/qcom/venus/helpers.c|  632 
 drivers/media/platform/qcom/venus/helpers.h|   41 +
 drivers/media/platform/qcom/venus/hfi.c|  520 +++
 drivers/media/platform/qcom/venus/hfi.h|  174 +++
 drivers/media/platform/qcom/venus/hfi_cmds.c   | 1256 
 drivers/media/platform/qcom/venus/hfi_cmds.h   |  304 
 drivers/media/platform/qcom/venus/hfi_helper.h | 1050 +
 drivers/media/platform/qcom/venus/hfi_msgs.c   | 1058 +
 drivers/media/platform/qcom/venus/hfi_msgs.h   |  283 
 drivers/media/platform/qcom/venus/hfi_venus.c  | 1570 
 drivers/media/platform/qcom/venus/hfi_venus.h  |   23 +
 drivers/media/platform/qcom/venus/hfi_venus_io.h   |  113 ++
 drivers/media/platform/qcom/venus/vdec.c   | 1091 ++
 drivers/media/platform/qcom/venus/vdec.h   |   23 +
 drivers/media/platform/qcom/venus/vdec_ctrls.c |  149 ++
 drivers/media/platform/qcom/venus/venc.c   | 1231 +++
 drivers/media/platform/qcom/venus/venc.h   |   23 +
 drivers/media/platform/qcom/venus/venc_ctrls.c |  258 
 drivers/media/v4l2-core/v4l2-mem2mem.c |   37 +
 include/media/v4l2-mem2mem.h   |   92 ++
 29 files changed, 10891 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/qcom,venus.txt
 create mode 100644 drivers/media/platform/qcom/venus/Makefile
 create mode 100644 drivers/media/platform/qcom/venus/core.c
 create mode 100644 drivers/media/platform/qcom/venus/core.h
 create mode 100644 drivers/media/platform/qcom/venus/firmware.c
 create mode 100644 drivers/media/platform/qcom/venus/firmware.h
 create mode 100644 drivers/media/platform/qcom/venus/helpers.c
 create mode 100644 drivers/media/platform/qcom/venus/helpers.h
 create mode 100644 drivers/media/platform/qcom/venus/hfi.c
 create mode 100644 drivers/media/platform/qcom/venus/hfi.h
 create mode 100644 drivers/media/platform/qcom/venus/hfi_cmds.c
 create mode 100644 drivers/media/platform/qcom/venus/hfi_cmds.h
 create mode 100644 drivers/media/platform/qcom/venus/hfi_helper.h
 create mode 100644 drivers/media/platform/qcom/venus/hfi_msgs.c
 create mode 100644 drivers/media/platform/qcom/venus/hfi_msgs.h
 create mode 100644 drivers/media/platform/qcom/venus/hfi_venus.c
 create mode 100644 drivers/media/platform/qcom/venus/hfi_venus.h
 create mode 100644 drivers/media/platform/qcom/venus/hfi_venus_io.h
 create mode 100644 drivers/media/platform/qcom/venus/vdec.c
 create mode 100644 drivers/media/platform/qcom/venus/vdec.h
 create mode 100644 drivers/media/platform/qcom/venus/vdec_ctrls.c
 create mode 100644 drivers/media/platform/qcom/venus/venc.c
 create mode 100644 drivers/media/platform/qcom/venus/venc.h
 create mode 100644 drivers/media/platform/qcom/venus/venc_ctrls.c

-- 
2.7.4

[PATCH v7 3/9] MAINTAINERS: Add Qualcomm Venus video accelerator driver

[Stanimir Varbanov]

Add an entry for Venus video encoder/decoder accelerator driver.

Signed-off-by: Stanimir Varbanov 
---
 MAINTAINERS | 8 
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 83a42ef1d1a7..cce2537d4d00 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10400,6 +10400,14 @@ T: git 
git://git.kernel.org/pub/scm/linux/kernel/git/rkuo/linux-hexagon-kernel.g
 S: Supported
 F: arch/hexagon/
 
+QUALCOMM VENUS VIDEO ACCELERATOR DRIVER
+M: Stanimir Varbanov 
+L: linux-media@vger.kernel.org
+L: linux-arm-...@vger.kernel.org
+T: git git://linuxtv.org/media_tree.git
+S: Maintained
+F: drivers/media/platform/qcom/venus/
+
 QUALCOMM WCN36XX WIRELESS DRIVER
 M: Eugene Krasnikov 
 L: wcn3...@lists.infradead.org
-- 
2.7.4

[PATCH v7 2/9] doc: DT: venus: binding document for Qualcomm video driver

[Stanimir Varbanov]

Add binding document for Venus video encoder/decoder driver

Cc: Rob Herring 
Cc: devicet...@vger.kernel.org
Acked-by: Rob Herring 
Signed-off-by: Stanimir Varbanov 
---
 .../devicetree/bindings/media/qcom,venus.txt   | 107 +
 1 file changed, 107 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/qcom,venus.txt

diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt 
b/Documentation/devicetree/bindings/media/qcom,venus.txt
new file mode 100644
index ..2693449daf73
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/qcom,venus.txt
@@ -0,0 +1,107 @@
+* Qualcomm Venus video encoder/decoder accelerators
+
+- compatible:
+   Usage: required
+   Value type: 
+   Definition: Value should contain one of:
+   - "qcom,msm8916-venus"
+   - "qcom,msm8996-venus"
+- reg:
+   Usage: required
+   Value type: 
+   Definition: Register base address and length of the register map.
+- interrupts:
+   Usage: required
+   Value type: 
+   Definition: Should contain interrupt line number.
+- clocks:
+   Usage: required
+   Value type: 
+   Definition: A List of phandle and clock specifier pairs as listed
+   in clock-names property.
+- clock-names:
+   Usage: required for msm8916
+   Value type: 
+   Definition: Should contain the following entries:
+   - "core"Core video accelerator clock
+   - "iface"   Video accelerator AHB clock
+   - "bus" Video accelerator AXI clock
+- clock-names:
+   Usage: required for msm8996
+   Value type: 
+   Definition: Should contain the following entries:
+   - "core"Core video accelerator clock
+   - "iface"   Video accelerator AHB clock
+   - "bus" Video accelerator AXI clock
+   - "mbus"Video MAXI clock
+- power-domains:
+   Usage: required
+   Value type: 
+   Definition: A phandle and power domain specifier pairs to the
+   power domain which is responsible for collapsing
+   and restoring power to the peripheral.
+- iommus:
+   Usage: required
+   Value type: 
+   Definition: A list of phandle and IOMMU specifier pairs.
+- memory-region:
+   Usage: required
+   Value type: 
+   Definition: reference to the reserved-memory for the firmware
+   memory region.
+
+* Subnodes
+The Venus video-codec node must contain two subnodes representing
+video-decoder and video-encoder.
+
+Every of video-encoder or video-decoder subnode should have:
+
+- compatible:
+   Usage: required
+   Value type: 
+   Definition: Value should contain "venus-decoder" or "venus-encoder"
+- clocks:
+   Usage: required for msm8996
+   Value type: 
+   Definition: A List of phandle and clock specifier pairs as listed
+   in clock-names property.
+- clock-names:
+   Usage: required for msm8996
+   Value type: 
+   Definition: Should contain the following entries:
+   - "core"Subcore video accelerator clock
+
+- power-domains:
+   Usage: required for msm8996
+   Value type: 
+   Definition: A phandle and power domain specifier pairs to the
+   power domain which is responsible for collapsing
+   and restoring power to the subcore.
+
+* An Example
+   video-codec@1d0 {
+   compatible = "qcom,msm8916-venus";
+   reg = <0x01d0 0xff000>;
+   interrupts = ;
+   clocks = < GCC_VENUS0_VCODEC0_CLK>,
+< GCC_VENUS0_AHB_CLK>,
+< GCC_VENUS0_AXI_CLK>;
+   clock-names = "core", "iface", "bus";
+   power-domains = < VENUS_GDSC>;
+   iommus = <_iommu 5>;
+   memory-region = <_mem>;
+
+   video-decoder {
+   compatible = "venus-decoder";
+   clocks = < VIDEO_SUBCORE0_CLK>;
+   clock-names = "core";
+   power-domains = < VENUS_CORE0_GDSC>;
+   };
+
+   video-encoder {
+   compatible = "venus-encoder";
+   clocks = < VIDEO_SUBCORE1_CLK>;
+   clock-names = "core";
+   power-domains = < VENUS_CORE1_GDSC>;
+   };
+   };
-- 
2.7.4

Re: [PATCH] staging: atomisp: use k{v}zalloc instead of k{v}alloc and memset

[DaeSeok Youn]

2017-03-13 23:07 GMT+09:00 DaeSeok Youn :
> 2017-03-13 20:51 GMT+09:00 Dan Carpenter :
>> On Mon, Mar 13, 2017 at 07:54:21PM +0900, Daeseok Youn wrote:
>>> If the atomisp_kernel_zalloc() has "true" as a second parameter, it
>>> tries to allocate zeroing memory from kmalloc(vmalloc) and memset.
>>> But using kzalloc is rather than kmalloc followed by memset with 0.
>>> (vzalloc is for same reason with kzalloc)
>>>
>>> And also atomisp_kernel_malloc() can be used with
>>> atomisp_kernel_zalloc(, false);
>>>
>>
>> We should just change all the callers to kvmalloc() and kvzmalloc().
> ok. I will try to change all the callers to kvmalloc() and kvzalloc().

The kvmalloc() and kvzalloc() are not ready to use in staging-testing
branch on staging tree.
If the kvmalloc and kvzalloc are available to use, I will replace
atomisp_kernel_malloc() and atomisp_kernel_zalloc() with kvmalloc()
and kvzalloc().

Thanks.
Regards,
Daeseok Youn.


>
> Thanks.
> Regards,
> Daeseok Youn
>>
>> regards,
>> dan carpenter
>>

Re: [PATCH 12/13] [media] tuners/tda18212: add flag for retrying tuner init on failure

[Antti Palosaari]

On 03/07/2017 08:57 PM, Daniel Scheller wrote:

From: Daniel Scheller 

Taken from tda18212dd, first read after cold reset sometimes fails on some
cards, trying twice shall do the trick. This is the case with the STV0367
demods soldered on the CineCTv6 bridge boards and older DuoFlex CT modules.

All other users (configs) of the tda18212 are updated as well to be sure
they won't be affected at all by this change.


That sounds like a i2c adapter problem and fix should be there, no hack 
on a tuner driver.


Antti



Signed-off-by: Daniel Scheller 
---
 drivers/media/platform/sti/c8sectpfe/c8sectpfe-dvb.c | 1 +
 drivers/media/tuners/tda18212.c  | 5 +
 drivers/media/tuners/tda18212.h  | 7 +++
 drivers/media/usb/dvb-usb-v2/anysee.c| 2 ++
 drivers/media/usb/em28xx/em28xx-dvb.c| 1 +
 5 files changed, 16 insertions(+)

diff --git a/drivers/media/platform/sti/c8sectpfe/c8sectpfe-dvb.c 
b/drivers/media/platform/sti/c8sectpfe/c8sectpfe-dvb.c
index 2c0015b..03688ee 100644
--- a/drivers/media/platform/sti/c8sectpfe/c8sectpfe-dvb.c
+++ b/drivers/media/platform/sti/c8sectpfe/c8sectpfe-dvb.c
@@ -111,6 +111,7 @@ static struct tda18212_config tda18212_conf = {
.if_dvbt_7 = 4150,
.if_dvbt_8 = 4500,
.if_dvbc = 5000,
+   .init_flags = 0,
 };

 int c8sectpfe_frontend_attach(struct dvb_frontend **fe,
diff --git a/drivers/media/tuners/tda18212.c b/drivers/media/tuners/tda18212.c
index 7b80683..2488537 100644
--- a/drivers/media/tuners/tda18212.c
+++ b/drivers/media/tuners/tda18212.c
@@ -220,6 +220,11 @@ static int tda18212_probe(struct i2c_client *client,
fe->ops.i2c_gate_ctrl(fe, 1); /* open I2C-gate */

ret = regmap_read(dev->regmap, 0x00, _id);
+
+   /* retry probe if desired */
+   if (ret && (cfg->init_flags & TDA18212_INIT_RETRY))
+   ret = regmap_read(dev->regmap, 0x00, _id);
+
dev_dbg(>client->dev, "chip_id=%02x\n", chip_id);

if (fe->ops.i2c_gate_ctrl)
diff --git a/drivers/media/tuners/tda18212.h b/drivers/media/tuners/tda18212.h
index 6391daf..717aa2c 100644
--- a/drivers/media/tuners/tda18212.h
+++ b/drivers/media/tuners/tda18212.h
@@ -23,6 +23,8 @@

 #include "dvb_frontend.h"

+#define TDA18212_INIT_RETRY(1 << 0)
+
 struct tda18212_config {
u16 if_dvbt_6;
u16 if_dvbt_7;
@@ -36,6 +38,11 @@ struct tda18212_config {
u16 if_atsc_qam;

/*
+* flags for tuner init control
+*/
+   u32 init_flags;
+
+   /*
 * pointer to DVB frontend
 */
struct dvb_frontend *fe;
diff --git a/drivers/media/usb/dvb-usb-v2/anysee.c 
b/drivers/media/usb/dvb-usb-v2/anysee.c
index 6795c0c..c35b66e 100644
--- a/drivers/media/usb/dvb-usb-v2/anysee.c
+++ b/drivers/media/usb/dvb-usb-v2/anysee.c
@@ -332,6 +332,7 @@ static struct tda18212_config anysee_tda18212_config = {
.if_dvbt_7 = 4150,
.if_dvbt_8 = 4150,
.if_dvbc = 5000,
+   .init_flags = 0,
 };

 static struct tda18212_config anysee_tda18212_config2 = {
@@ -342,6 +343,7 @@ static struct tda18212_config anysee_tda18212_config2 = {
.if_dvbt2_7 = 4000,
.if_dvbt2_8 = 4000,
.if_dvbc = 5000,
+   .init_flags = 0,
 };

 static struct cx24116_config anysee_cx24116_config = {
diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c 
b/drivers/media/usb/em28xx/em28xx-dvb.c
index 82edd37..143efb0 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -380,6 +380,7 @@ static struct tda18271_config kworld_ub435q_v2_config = {
 static struct tda18212_config kworld_ub435q_v3_config = {
.if_atsc_vsb= 3600,
.if_atsc_qam= 3600,
+   .init_flags = 0,
 };

 static struct zl10353_config em28xx_zl10353_xc3028_no_i2c_gate = {



--
http://palosaari.fi/

Re: [PATCH] staging: atomisp: use k{v}zalloc instead of k{v}alloc and memset

[DaeSeok Youn]

2017-03-13 20:51 GMT+09:00 Dan Carpenter :
> On Mon, Mar 13, 2017 at 07:54:21PM +0900, Daeseok Youn wrote:
>> If the atomisp_kernel_zalloc() has "true" as a second parameter, it
>> tries to allocate zeroing memory from kmalloc(vmalloc) and memset.
>> But using kzalloc is rather than kmalloc followed by memset with 0.
>> (vzalloc is for same reason with kzalloc)
>>
>> And also atomisp_kernel_malloc() can be used with
>> atomisp_kernel_zalloc(, false);
>>
>
> We should just change all the callers to kvmalloc() and kvzmalloc().
ok. I will try to change all the callers to kvmalloc() and kvzalloc().

Thanks.
Regards,
Daeseok Youn
>
> regards,
> dan carpenter
>

Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates

[Philipp Zabel]

On Mon, 2017-03-13 at 13:27 +, Russell King - ARM Linux wrote:
> On Mon, Mar 13, 2017 at 03:16:48PM +0200, Sakari Ailus wrote:
> > The vast majority of existing drivers do not implement them nor the user
> > space expects having to set them. Making that mandatory would break existing
> > user space.
> > 
> > In addition, that does not belong to link validation either: link validation
> > should only include static properties of the link that are required for
> > correct hardware operation. Frame rate is not such property: hardware that
> > supports the MC interface generally does not recognise such concept (with
> > the exception of some sensors). Additionally, it is dynamic: the frame rate
> > can change during streaming, making its validation at streamon time useless.
> 
> So how do we configure the CSI, which can do frame skipping?
> 
> With what you're proposing, it means it's possible to configure the
> camera sensor source pad to do 50fps.  Configure the CSI sink pad to
> an arbitary value, such as 30fps, and configure the CSI source pad to
> 15fps.
> 
> What you actually get out of the CSI is 25fps, which bears very little
> with the actual values used on the CSI source pad.
> 
> You could say "CSI should ask the camera sensor" - well, that's fine
> if it's immediately downstream, but otherwise we'd need to go walking
> down the graph to find something that resembles its source - there may
> be mux and CSI2 interface subdev blocks in that path.  Or we just accept
> that frame rates are completely arbitary and bear no useful meaning what
> so ever.

Which would include the frame interval returned by VIDIOC_G_PARM on the
connected video device, as that gets its information from the CSI output
pad's frame interval.

regards
Philipp

Re: [PATCHv5 07/16] atmel-isi: remove dependency of the soc-camera framework

[Nicolas Ferre]

Le 13/03/2017 à 12:43, Hans Verkuil a écrit :
> On 03/12/2017 05:44 PM, Guennadi Liakhovetski wrote:
>> Hi Hans,
>>
>> Thanks for the patch. Why hasn't this patch been CCed to Josh Wu? Is he 
>> still at Atmel? Adding to CC to check.
> 
> To the best of my knowledge Josh no longer works for Atmel/Microchip, and 
> Songjun
> took over.

Yes absolutely, Josh Wu is no longer with us and Songjun and Ludovic
took over the maintenance.
But we have full confidence in experts like you guys and we thank you so
much Hans for having handled this move for atmel-isi.

>> A general comment: this patch doesn't only remove soc-camera dependencies, 
>> it also changes the code and the behaviour. I would prefer to break that 
>> down in multiple patches, or remove this driver completely and add a new 
>> one. I'll provide some comments, but of course I cannot make an extensive 
>> review of a 1200-line of change patch without knowing the hardware and the 
>> set ups well enough.
>>
>> On Sat, 11 Mar 2017, Hans Verkuil wrote:
>>
>>> From: Hans Verkuil 
>>>
>>> This patch converts the atmel-isi driver from a soc-camera driver to a 
>>> driver
>>> that is stand-alone.
>>>
>>> Signed-off-by: Hans Verkuil 
>>> ---
>>>  drivers/media/platform/soc_camera/Kconfig |3 +-
>>>  drivers/media/platform/soc_camera/atmel-isi.c | 1209 
>>> +++--
>>>  2 files changed, 714 insertions(+), 498 deletions(-)

[..]

>>> +static struct isi_format isi_formats[] = {
>>
>> This isn't a const array, you're modifying it during initialisation. Are 
>> we sure there aren't going to be any SoCs with more than one ISI?
> 
> When that happens, this should be changed at that time. I think it is unlikely
> since as I understand it ISI has been replaced by ISC (atmel-isc).

Yes, ISC has replaced ISI for all Atmel/Microchip MPUs onwards. We may
have several of them, but very likely never more than one ISI.

Regards,
-- 
Nicolas Ferre

[PATCH 1/2] cx88: convert struct cx88_core.refcount from atomic_t to refcount_t

[Sakari Ailus]

From: Elena Reshetova 

refcount_t is better suitable for counting references than atomic_t.

Signed-off-by: Elena Reshetova 
Signed-off-by: Hans Liljestrand 
Signed-off-by: Kees Cook 
Signed-off-by: David Windsor 
Signed-off-by: Sakari Ailus 
---
 drivers/media/pci/cx88/cx88-cards.c | 2 +-
 drivers/media/pci/cx88/cx88-core.c  | 4 ++--
 drivers/media/pci/cx88/cx88.h   | 3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/media/pci/cx88/cx88-cards.c 
b/drivers/media/pci/cx88/cx88-cards.c
index 61e1803..73cc7a6 100644
--- a/drivers/media/pci/cx88/cx88-cards.c
+++ b/drivers/media/pci/cx88/cx88-cards.c
@@ -3670,7 +3670,7 @@ struct cx88_core *cx88_core_create(struct pci_dev *pci, 
int nr)
if (!core)
return NULL;
 
-   atomic_inc(>refcount);
+   refcount_set(>refcount, 1);
core->pci_bus  = pci->bus->number;
core->pci_slot = PCI_SLOT(pci->devfn);
core->pci_irqmask = PCI_INT_RISC_RD_BERRINT | PCI_INT_RISC_WR_BERRINT |
diff --git a/drivers/media/pci/cx88/cx88-core.c 
b/drivers/media/pci/cx88/cx88-core.c
index 973a9cd4..8bfa5b7 100644
--- a/drivers/media/pci/cx88/cx88-core.c
+++ b/drivers/media/pci/cx88/cx88-core.c
@@ -1052,7 +1052,7 @@ struct cx88_core *cx88_core_get(struct pci_dev *pci)
mutex_unlock();
return NULL;
}
-   atomic_inc(>refcount);
+   refcount_inc(>refcount);
mutex_unlock();
return core;
}
@@ -1073,7 +1073,7 @@ void cx88_core_put(struct cx88_core *core, struct pci_dev 
*pci)
release_mem_region(pci_resource_start(pci, 0),
   pci_resource_len(pci, 0));
 
-   if (!atomic_dec_and_test(>refcount))
+   if (!refcount_dec_and_test(>refcount))
return;
 
mutex_lock();
diff --git a/drivers/media/pci/cx88/cx88.h b/drivers/media/pci/cx88/cx88.h
index 115414c..6777926 100644
--- a/drivers/media/pci/cx88/cx88.h
+++ b/drivers/media/pci/cx88/cx88.h
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -339,7 +340,7 @@ struct cx8802_dev;
 
 struct cx88_core {
struct list_head   devlist;
-   atomic_t   refcount;
+   refcount_t refcount;
 
/* board name */
intnr;
-- 
2.7.4

[PATCH 2/2] vb2: convert vb2_vmarea_handler refcount from atomic_t to refcount_t

[Sakari Ailus]

From: Elena Reshetova 

Use refcount_t to manage the refcount to the memory type specific buffer
videobuf2 buffer implementations. refcount_t is better suitable for the
purpose than atomic_t.

Signed-off-by: Elena Reshetova 
Signed-off-by: Hans Liljestrand 
Signed-off-by: Kees Cook 
Signed-off-by: David Windsor 
Signed-off-by: Sakari Ailus 
---
 drivers/media/v4l2-core/videobuf2-dma-contig.c | 11 ++-
 drivers/media/v4l2-core/videobuf2-dma-sg.c | 11 ++-
 drivers/media/v4l2-core/videobuf2-memops.c |  6 +++---
 drivers/media/v4l2-core/videobuf2-vmalloc.c| 11 ++-
 include/media/videobuf2-memops.h   |  3 ++-
 5 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c 
b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index fb6a177..d29a07f 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -12,6 +12,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -34,7 +35,7 @@ struct vb2_dc_buf {
 
/* MMAP related */
struct vb2_vmarea_handler   handler;
-   atomic_trefcount;
+   refcount_t  refcount;
struct sg_table *sgt_base;
 
/* DMABUF related */
@@ -86,7 +87,7 @@ static unsigned int vb2_dc_num_users(void *buf_priv)
 {
struct vb2_dc_buf *buf = buf_priv;
 
-   return atomic_read(>refcount);
+   return refcount_read(>refcount);
 }
 
 static void vb2_dc_prepare(void *buf_priv)
@@ -122,7 +123,7 @@ static void vb2_dc_put(void *buf_priv)
 {
struct vb2_dc_buf *buf = buf_priv;
 
-   if (!atomic_dec_and_test(>refcount))
+   if (!refcount_dec_and_test(>refcount))
return;
 
if (buf->sgt_base) {
@@ -170,7 +171,7 @@ static void *vb2_dc_alloc(struct device *dev, unsigned long 
attrs,
buf->handler.put = vb2_dc_put;
buf->handler.arg = buf;
 
-   atomic_inc(>refcount);
+   refcount_set(>refcount, 1);
 
return buf;
 }
@@ -407,7 +408,7 @@ static struct dma_buf *vb2_dc_get_dmabuf(void *buf_priv, 
unsigned long flags)
return NULL;
 
/* dmabuf keeps reference to vb2 buffer */
-   atomic_inc(>refcount);
+   refcount_inc(>refcount);
 
return dbuf;
 }
diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c 
b/drivers/media/v4l2-core/videobuf2-dma-sg.c
index ecff8f4..29fde1a 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
@@ -12,6 +12,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -46,7 +47,7 @@ struct vb2_dma_sg_buf {
struct sg_table *dma_sgt;
size_t  size;
unsigned intnum_pages;
-   atomic_trefcount;
+   refcount_t  refcount;
struct vb2_vmarea_handler   handler;
 
struct dma_buf_attachment   *db_attach;
@@ -150,7 +151,7 @@ static void *vb2_dma_sg_alloc(struct device *dev, unsigned 
long dma_attrs,
buf->handler.put = vb2_dma_sg_put;
buf->handler.arg = buf;
 
-   atomic_inc(>refcount);
+   refcount_set(>refcount, 1);
 
dprintk(1, "%s: Allocated buffer of %d pages\n",
__func__, buf->num_pages);
@@ -176,7 +177,7 @@ static void vb2_dma_sg_put(void *buf_priv)
struct sg_table *sgt = >sg_table;
int i = buf->num_pages;
 
-   if (atomic_dec_and_test(>refcount)) {
+   if (refcount_dec_and_test(>refcount)) {
dprintk(1, "%s: Freeing buffer of %d pages\n", __func__,
buf->num_pages);
dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
@@ -320,7 +321,7 @@ static unsigned int vb2_dma_sg_num_users(void *buf_priv)
 {
struct vb2_dma_sg_buf *buf = buf_priv;
 
-   return atomic_read(>refcount);
+   return refcount_read(>refcount);
 }
 
 static int vb2_dma_sg_mmap(void *buf_priv, struct vm_area_struct *vma)
@@ -530,7 +531,7 @@ static struct dma_buf *vb2_dma_sg_get_dmabuf(void 
*buf_priv, unsigned long flags
return NULL;
 
/* dmabuf keeps reference to vb2 buffer */
-   atomic_inc(>refcount);
+   refcount_inc(>refcount);
 
return dbuf;
 }
diff --git a/drivers/media/v4l2-core/videobuf2-memops.c 
b/drivers/media/v4l2-core/videobuf2-memops.c
index 1cd322e..4bb8424 100644
--- a/drivers/media/v4l2-core/videobuf2-memops.c
+++ b/drivers/media/v4l2-core/videobuf2-memops.c
@@ -96,10 +96,10 @@ static void vb2_common_vm_open(struct vm_area_struct *vma)
struct vb2_vmarea_handler *h = vma->vm_private_data;
 
pr_debug("%s: %p, refcount: %d, vma: %08lx-%08lx\n",
-  

[PATCH 0/2] V4L2 driver refcount_t conversion

[Sakari Ailus]

Hi folks,

These two patches convert a few V4L2 driver from using atomic_t for
refcounts to the newly added refcount_t.

The previous version of the patches were posted by Elena to the list and
the review comments given to those patches have been addressed. In
particular,

- One of the patches was dropped (atomic_t not used as refcount),

- Merged four last VB2 patches as the in-between state would not compile /
  would compile with warnings and

- Indentation was fixed in the first patch.

-- 
Kind regards,
Sakari

Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates

[Russell King - ARM Linux]

On Mon, Mar 13, 2017 at 03:16:48PM +0200, Sakari Ailus wrote:
> The vast majority of existing drivers do not implement them nor the user
> space expects having to set them. Making that mandatory would break existing
> user space.
> 
> In addition, that does not belong to link validation either: link validation
> should only include static properties of the link that are required for
> correct hardware operation. Frame rate is not such property: hardware that
> supports the MC interface generally does not recognise such concept (with
> the exception of some sensors). Additionally, it is dynamic: the frame rate
> can change during streaming, making its validation at streamon time useless.

So how do we configure the CSI, which can do frame skipping?

With what you're proposing, it means it's possible to configure the
camera sensor source pad to do 50fps.  Configure the CSI sink pad to
an arbitary value, such as 30fps, and configure the CSI source pad to
15fps.

What you actually get out of the CSI is 25fps, which bears very little
with the actual values used on the CSI source pad.

You could say "CSI should ask the camera sensor" - well, that's fine
if it's immediately downstream, but otherwise we'd need to go walking
down the graph to find something that resembles its source - there may
be mux and CSI2 interface subdev blocks in that path.  Or we just accept
that frame rates are completely arbitary and bear no useful meaning what
so ever.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging

[Mark Brown]

On Mon, Mar 13, 2017 at 10:54:33AM +, Brian Starkey wrote:
> On Sun, Mar 12, 2017 at 02:34:14PM +0100, Benjamin Gaignard wrote:

> > Another point is how can we put secure rules (like selinux policy) on
> > heaps since all the allocations
> > go to the same device (/dev/ion) ? For example, until now, in Android
> > we have to give the same
> > access rights to all the process that use ION.
> > It will become problem when we will add secure heaps because we won't
> > be able to distinguish secure
> > processes to standard ones or set specific policy per heaps.
> > Maybe I'm wrong here but I have never see selinux policy checking an
> > ioctl field but if that
> > exist it could be a solution.

> I might be thinking of a different type of "secure", but...

> Should the security of secure heaps be enforced by OS-level
> permissions? I don't know about other architectures, but at least on
> arm/arm64 this is enforced in hardware; it doesn't matter who has
> access to the ion heap, because only secure devices (or the CPU
> running a secure process) is physically able to access the memory
> backing the buffer.

> In fact, in the use-cases I know of, the process asking for the ion
> allocation is not a secure process, and so we wouldn't *want* to
> restrict the secure heap to be allocated from only by secure
> processes.

I think there's an orthogonal level of OS level security that can be
applied here - it's reasonable for it to want to say things like "only
processes that are supposed to be implementing functionality X should be
able to try to allocate memory set aside for that functionality".  This
mitigates against escallation attacks and so on, it's not really
directly related to secure memory as such though.


signature.asc
Description: PGP signature

Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates

[Sakari Ailus]

Hi Philipp,

On Tue, Feb 21, 2017 at 09:50:23AM +0100, Philipp Zabel wrote:
...
> > > Then there's the issue where, if you have this setup:
> > >
> > >  camera --> csi2 receiver --> csi --> capture
> > >
> > > and the "csi" subdev can skip frames, you need to know (a) at the CSI
> > > sink pad what the frame rate is of the source (b) what the desired
> > > source pad frame rate is, so you can configure the frame skipping.
> > > So, does the csi subdev have to walk back through the media graph
> > > looking for the frame rate?  Does the capture device have to walk back
> > > through the media graph looking for some subdev to tell it what the
> > > frame rate is - the capture device certainly can't go straight to the
> > > sensor to get an answer to that question, because that bypasses the
> > > effect of the CSI frame skipping (which will lower the frame rate.)
> > >
> > > IMHO, frame rate is just another format property, just like the
> > > resolution and data format itself, and v4l2 should be treating it no
> > > differently.
> > >
> > 
> > I agree, frame rate, if indicated/specified by both sides of a link,
> > should match. So maybe this should be part of v4l2 link validation.
> > 
> > This might be a good time to propose the following patch.
> 
> I agree with Steve and Russell. I don't see why the (nominal) frame
> interval should be handled differently than resolution, data format, and
> colorspace information. I think it should just be propagated in the same
> way, and there is no reason to have two connected pads set to a
> different interval. That would make implementing the g/s_frame_interval
> subdev calls mandatory.

The vast majority of existing drivers do not implement them nor the user
space expects having to set them. Making that mandatory would break existing
user space.

In addition, that does not belong to link validation either: link validation
should only include static properties of the link that are required for
correct hardware operation. Frame rate is not such property: hardware that
supports the MC interface generally does not recognise such concept (with
the exception of some sensors). Additionally, it is dynamic: the frame rate
can change during streaming, making its validation at streamon time useless.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

[PATCH 4/6] [media] cx231xx-audio: fix init error path

[Johan Hovold]

Make sure to release the snd_card also on a late allocation error.

Fixes: e0d3bafd0258 ("V4L/DVB (10954): Add cx231xx USB driver")
Cc: stable  # 2.6.30
Cc: Sri Deevi 
Signed-off-by: Johan Hovold 
---
 drivers/media/usb/cx231xx/cx231xx-audio.c | 25 ++---
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/media/usb/cx231xx/cx231xx-audio.c 
b/drivers/media/usb/cx231xx/cx231xx-audio.c
index cf80842dfa08..f3729d6eb46a 100644
--- a/drivers/media/usb/cx231xx/cx231xx-audio.c
+++ b/drivers/media/usb/cx231xx/cx231xx-audio.c
@@ -670,10 +670,8 @@ static int cx231xx_audio_init(struct cx231xx *dev)
 
spin_lock_init(>slock);
err = snd_pcm_new(card, "Cx231xx Audio", 0, 0, 1, );
-   if (err < 0) {
-   snd_card_free(card);
-   return err;
-   }
+   if (err < 0)
+   goto err_free_card;
 
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE,
_cx231xx_pcm_capture);
@@ -687,10 +685,9 @@ static int cx231xx_audio_init(struct cx231xx *dev)
INIT_WORK(>wq_trigger, audio_trigger);
 
err = snd_card_register(card);
-   if (err < 0) {
-   snd_card_free(card);
-   return err;
-   }
+   if (err < 0)
+   goto err_free_card;
+
adev->sndcard = card;
adev->udev = dev->udev;
 
@@ -709,9 +706,10 @@ static int cx231xx_audio_init(struct cx231xx *dev)
"audio EndPoint Addr 0x%x, Alternate settings: %i\n",
adev->end_point_addr, adev->num_alt);
adev->alt_max_pkt_size = kmalloc(32 * adev->num_alt, GFP_KERNEL);
-
-   if (adev->alt_max_pkt_size == NULL)
-   return -ENOMEM;
+   if (!adev->alt_max_pkt_size) {
+   err = -ENOMEM;
+   goto err_free_card;
+   }
 
for (i = 0; i < adev->num_alt; i++) {
u16 tmp =
@@ -725,6 +723,11 @@ static int cx231xx_audio_init(struct cx231xx *dev)
}
 
return 0;
+
+err_free_card:
+   snd_card_free(card);
+
+   return err;
 }
 
 static int cx231xx_audio_fini(struct cx231xx *dev)
-- 
2.12.0

[PATCH 2/6] [media] usbvision: fix NULL-deref at probe

[Johan Hovold]

Make sure to check the number of endpoints to avoid dereferencing a
NULL-pointer or accessing memory beyond the endpoint array should a
malicious device lack the expected endpoints.

Fixes: 2a9f8b5d25be ("V4L/DVB (5206): Usbvision: set alternate interface
modification")
Cc: stable  # 2.6.21
Cc: Thierry MERLE 
Signed-off-by: Johan Hovold 
---
 drivers/media/usb/usbvision/usbvision-video.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/usbvision/usbvision-video.c 
b/drivers/media/usb/usbvision/usbvision-video.c
index f5c635a67d74..f9c3325aa4d4 100644
--- a/drivers/media/usb/usbvision/usbvision-video.c
+++ b/drivers/media/usb/usbvision/usbvision-video.c
@@ -1501,7 +1501,14 @@ static int usbvision_probe(struct usb_interface *intf,
}
 
for (i = 0; i < usbvision->num_alt; i++) {
-   u16 tmp = le16_to_cpu(uif->altsetting[i].endpoint[1].desc.
+   u16 tmp;
+
+   if (uif->altsetting[i].desc.bNumEndpoints < 2) {
+   ret = -ENODEV;
+   goto err_pkt;
+   }
+
+   tmp = le16_to_cpu(uif->altsetting[i].endpoint[1].desc.
  wMaxPacketSize);
usbvision->alt_max_pkt_size[i] =
(tmp & 0x07ff) * (((tmp & 0x1800) >> 11) + 1);
-- 
2.12.0

[PATCH 6/6] [media] gspca: konica: add missing endpoint sanity check

[Johan Hovold]

Make sure to check the number of endpoints to avoid accessing memory
beyond the endpoint array should a device lack the expected endpoints.

Note that, as far as I can tell, the gspca framework has already made
sure there is at least one endpoint in the current alternate setting so
there should be no risk for a NULL-pointer dereference here.

Fixes: b517af722860 ("V4L/DVB: gspca_konica: New gspca subdriver for
konica chipset using cams")
Cc: stable  # 2.6.37
Cc: Hans de Goede 
Signed-off-by: Johan Hovold 
---
 drivers/media/usb/gspca/konica.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/usb/gspca/konica.c b/drivers/media/usb/gspca/konica.c
index 71f273377f83..31b2117e8f1d 100644
--- a/drivers/media/usb/gspca/konica.c
+++ b/drivers/media/usb/gspca/konica.c
@@ -184,6 +184,9 @@ static int sd_start(struct gspca_dev *gspca_dev)
return -EIO;
}
 
+   if (alt->desc.bNumEndpoints < 2)
+   return -ENODEV;
+
packet_size = le16_to_cpu(alt->endpoint[0].desc.wMaxPacketSize);
 
n = gspca_dev->cam.cam_mode[gspca_dev->curr_mode].priv;
-- 
2.12.0

[PATCH 5/6] [media] cx231xx-audio: fix NULL-deref at probe

[Johan Hovold]

Make sure to check the number of endpoints to avoid dereferencing a
NULL-pointer or accessing memory beyond the endpoint array should a
malicious device lack the expected endpoints.

Fixes: e0d3bafd0258 ("V4L/DVB (10954): Add cx231xx USB driver")
Cc: stable  # 2.6.30
Cc: Sri Deevi 
Signed-off-by: Johan Hovold 
---
 drivers/media/usb/cx231xx/cx231xx-audio.c | 17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/cx231xx/cx231xx-audio.c 
b/drivers/media/usb/cx231xx/cx231xx-audio.c
index f3729d6eb46a..a050d125934c 100644
--- a/drivers/media/usb/cx231xx/cx231xx-audio.c
+++ b/drivers/media/usb/cx231xx/cx231xx-audio.c
@@ -697,6 +697,11 @@ static int cx231xx_audio_init(struct cx231xx *dev)
hs_config_info[0].interface_info.
audio_index + 1];
 
+   if (uif->altsetting[0].desc.bNumEndpoints < isoc_pipe + 1) {
+   err = -ENODEV;
+   goto err_free_card;
+   }
+
adev->end_point_addr =
uif->altsetting[0].endpoint[isoc_pipe].desc.
bEndpointAddress;
@@ -712,8 +717,14 @@ static int cx231xx_audio_init(struct cx231xx *dev)
}
 
for (i = 0; i < adev->num_alt; i++) {
-   u16 tmp =
-   le16_to_cpu(uif->altsetting[i].endpoint[isoc_pipe].desc.
+   u16 tmp;
+
+   if (uif->altsetting[i].desc.bNumEndpoints < isoc_pipe + 1) {
+   err = -ENODEV;
+   goto err_free_pkt_size;
+   }
+
+   tmp = le16_to_cpu(uif->altsetting[i].endpoint[isoc_pipe].desc.
wMaxPacketSize);
adev->alt_max_pkt_size[i] =
(tmp & 0x07ff) * (((tmp & 0x1800) >> 11) + 1);
@@ -724,6 +735,8 @@ static int cx231xx_audio_init(struct cx231xx *dev)
 
return 0;
 
+err_free_pkt_size:
+   kfree(adev->alt_max_pkt_size);
 err_free_card:
snd_card_free(card);
 
-- 
2.12.0

[PATCH 3/6] [media] cx231xx-cards: fix NULL-deref at probe

[Johan Hovold]

Make sure to check the number of endpoints to avoid dereferencing a
NULL-pointer or accessing memory beyond the endpoint array should a
malicious device lack the expected endpoints.

Fixes: e0d3bafd0258 ("V4L/DVB (10954): Add cx231xx USB driver")
Cc: stable  # 2.6.30
Cc: Sri Deevi 
Signed-off-by: Johan Hovold 
---
 drivers/media/usb/cx231xx/cx231xx-cards.c | 45 +++
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/cx231xx/cx231xx-cards.c 
b/drivers/media/usb/cx231xx/cx231xx-cards.c
index f730fdbc9156..f850267a0095 100644
--- a/drivers/media/usb/cx231xx/cx231xx-cards.c
+++ b/drivers/media/usb/cx231xx/cx231xx-cards.c
@@ -1426,6 +1426,9 @@ static int cx231xx_init_v4l2(struct cx231xx *dev,
 
uif = udev->actconfig->interface[idx];
 
+   if (uif->altsetting[0].desc.bNumEndpoints < isoc_pipe + 1)
+   return -ENODEV;
+
dev->video_mode.end_point_addr = 
uif->altsetting[0].endpoint[isoc_pipe].desc.bEndpointAddress;
dev->video_mode.num_alt = uif->num_altsetting;
 
@@ -1439,7 +1442,12 @@ static int cx231xx_init_v4l2(struct cx231xx *dev,
return -ENOMEM;
 
for (i = 0; i < dev->video_mode.num_alt; i++) {
-   u16 tmp = 
le16_to_cpu(uif->altsetting[i].endpoint[isoc_pipe].desc.wMaxPacketSize);
+   u16 tmp;
+
+   if (uif->altsetting[i].desc.bNumEndpoints < isoc_pipe + 1)
+   return -ENODEV;
+
+   tmp = 
le16_to_cpu(uif->altsetting[i].endpoint[isoc_pipe].desc.wMaxPacketSize);
dev->video_mode.alt_max_pkt_size[i] = (tmp & 0x07ff) * (((tmp & 
0x1800) >> 11) + 1);
dev_dbg(dev->dev,
"Alternate setting %i, max size= %i\n", i,
@@ -1456,6 +1464,9 @@ static int cx231xx_init_v4l2(struct cx231xx *dev,
}
uif = udev->actconfig->interface[idx];
 
+   if (uif->altsetting[0].desc.bNumEndpoints < isoc_pipe + 1)
+   return -ENODEV;
+
dev->vbi_mode.end_point_addr =
uif->altsetting[0].endpoint[isoc_pipe].desc.
bEndpointAddress;
@@ -1472,8 +1483,12 @@ static int cx231xx_init_v4l2(struct cx231xx *dev,
return -ENOMEM;
 
for (i = 0; i < dev->vbi_mode.num_alt; i++) {
-   u16 tmp =
-   le16_to_cpu(uif->altsetting[i].endpoint[isoc_pipe].
+   u16 tmp;
+
+   if (uif->altsetting[i].desc.bNumEndpoints < isoc_pipe + 1)
+   return -ENODEV;
+
+   tmp = le16_to_cpu(uif->altsetting[i].endpoint[isoc_pipe].
desc.wMaxPacketSize);
dev->vbi_mode.alt_max_pkt_size[i] =
(tmp & 0x07ff) * (((tmp & 0x1800) >> 11) + 1);
@@ -1493,6 +1508,9 @@ static int cx231xx_init_v4l2(struct cx231xx *dev,
}
uif = udev->actconfig->interface[idx];
 
+   if (uif->altsetting[0].desc.bNumEndpoints < isoc_pipe + 1)
+   return -ENODEV;
+
dev->sliced_cc_mode.end_point_addr =
uif->altsetting[0].endpoint[isoc_pipe].desc.
bEndpointAddress;
@@ -1507,7 +1525,12 @@ static int cx231xx_init_v4l2(struct cx231xx *dev,
return -ENOMEM;
 
for (i = 0; i < dev->sliced_cc_mode.num_alt; i++) {
-   u16 tmp = le16_to_cpu(uif->altsetting[i].endpoint[isoc_pipe].
+   u16 tmp;
+
+   if (uif->altsetting[i].desc.bNumEndpoints < isoc_pipe + 1)
+   return -ENODEV;
+
+   tmp = le16_to_cpu(uif->altsetting[i].endpoint[isoc_pipe].
desc.wMaxPacketSize);
dev->sliced_cc_mode.alt_max_pkt_size[i] =
(tmp & 0x07ff) * (((tmp & 0x1800) >> 11) + 1);
@@ -1676,6 +1699,11 @@ static int cx231xx_usb_probe(struct usb_interface 
*interface,
}
uif = udev->actconfig->interface[idx];
 
+   if (uif->altsetting[0].desc.bNumEndpoints < isoc_pipe + 1) {
+   retval = -ENODEV;
+   goto err_video_alt;
+   }
+
dev->ts1_mode.end_point_addr =
uif->altsetting[0].endpoint[isoc_pipe].
desc.bEndpointAddress;
@@ -1693,7 +1721,14 @@ static int cx231xx_usb_probe(struct usb_interface 
*interface,
}
 
for (i = 0; i < dev->ts1_mode.num_alt; i++) {
-   u16 tmp = le16_to_cpu(uif->altsetting[i].
+   u16 tmp;
+
+   if (uif->altsetting[i].desc.bNumEndpoints < isoc_pipe + 
1) {
+   retval = -ENODEV;
+   goto err_video_alt;
+   }
+
+   tmp = le16_to_cpu(uif->altsetting[i].

[PATCH 1/6] [media] dib0700: fix NULL-deref at probe

[Johan Hovold]

Make sure to check the number of endpoints to avoid dereferencing a
NULL-pointer should a malicious device lack endpoints.

Fixes: c4018fa2e4c0 ("[media] dib0700: fix RC support on Hauppauge
Nova-TD")
Cc: stable  # 3.16
Cc: Mauro Carvalho Chehab 
Signed-off-by: Johan Hovold 
---
 drivers/media/usb/dvb-usb/dib0700_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c 
b/drivers/media/usb/dvb-usb/dib0700_core.c
index dd5edd3a17ee..08acdd32e412 100644
--- a/drivers/media/usb/dvb-usb/dib0700_core.c
+++ b/drivers/media/usb/dvb-usb/dib0700_core.c
@@ -809,6 +809,9 @@ int dib0700_rc_setup(struct dvb_usb_device *d, struct 
usb_interface *intf)
 
/* Starting in firmware 1.20, the RC info is provided on a bulk pipe */
 
+   if (intf->altsetting[0].desc.bNumEndpoints < rc_ep + 1)
+   return -ENODEV;
+
purb = usb_alloc_urb(0, GFP_KERNEL);
if (purb == NULL)
return -ENOMEM;
-- 
2.12.0

[PATCH 0/6] [media] fix missing endpoint sanity checks

[Johan Hovold]

This series fixes a number of NULL-pointer dereferences (and related
issues) due to missing endpoint sanity checks that can be triggered by a
malicious USB device.

Johan


Johan Hovold (6):
  [media] dib0700: fix NULL-deref at probe
  [media] usbvision: fix NULL-deref at probe
  [media] cx231xx-cards: fix NULL-deref at probe
  [media] cx231xx-audio: fix init error path
  [media] cx231xx-audio: fix NULL-deref at probe
  [media] gspca: konica: add missing endpoint sanity check

 drivers/media/usb/cx231xx/cx231xx-audio.c | 42 +
 drivers/media/usb/cx231xx/cx231xx-cards.c | 45 ---
 drivers/media/usb/dvb-usb/dib0700_core.c  |  3 ++
 drivers/media/usb/gspca/konica.c  |  3 ++
 drivers/media/usb/usbvision/usbvision-video.c |  9 +-
 5 files changed, 83 insertions(+), 19 deletions(-)

-- 
2.12.0

Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Sakari Ailus]

Hi Mauro,

On Sat, Mar 11, 2017 at 08:25:49AM -0300, Mauro Carvalho Chehab wrote:
> Em Sat, 11 Mar 2017 00:37:14 +0200
> Sakari Ailus  escreveu:
> 
> > Hi Mauro (and others),
> > 
> > On Fri, Mar 10, 2017 at 12:53:42PM -0300, Mauro Carvalho Chehab wrote:
> > > Em Fri, 10 Mar 2017 15:20:48 +0100
> > > Hans Verkuil  escreveu:
> > >   
> > > >   
> > > > > As I've already mentioned, from talking about this with Mauro, it 
> > > > > seems
> > > > > Mauro is in agreement with permitting the control inheritence... I 
> > > > > wish
> > > > > Mauro would comment for himself, as I can't quote our private 
> > > > > discussion
> > > > > on the subject.
> > > > 
> > > > I can't comment either, not having seen his mail and reasoning.  
> > > 
> > > The rationale is that we should support the simplest use cases first.
> > > 
> > > In the case of the first MC-based driver (and several subsequent
> > > ones), the simplest use case required MC, as it was meant to suport
> > > a custom-made sophisticated application that required fine control
> > > on each component of the pipeline and to allow their advanced
> > > proprietary AAA userspace-based algorithms to work.  
> > 
> > The first MC based driver (omap3isp) supports what the hardware can do, it
> > does not support applications as such.
> 
> All media drivers support a subset of what the hardware can do. The
> question is if such subset covers the use cases or not.

Can you name a feature in the OMAP 3 ISP that is not and can not be
supported using the current driver model (MC + V4L2 sub-device + V4L2) that
could be even remotely useful?

> 
> The current MC-based drivers (except for uvc) took a patch to offer a
> more advanced API, to allow direct control to each IP module, as it was
> said, by the time we merged the OMAP3 driver, that, for the N9/N900 camera
> to work, it was mandatory to access the pipeline's individual components.
> 
> Such approach require that some userspace software will have knowledge
> about some hardware details, in order to setup pipelines and send controls
> to the right components. That makes really hard to have a generic user
> friendly application to use such devices.

The effect you described above is true, but I disagree with the cause. The
cause is the hardware is more complex and variable than what has been
supported previously, and providing a generic interface for accessing such
hardware will require more complex interface.

The hardware we have today and the user cases we have today are more --- not
less --- complex and nuanced than when the Media controller was merged back
in 2010. Arguably there is thus more need for the functionality it provides,
not less.

> 
> Non-MC based drivers control the hardware via a portable interface with
> doesn't require any knowledge about the hardware specifics, as either the
> Kernel or some firmware at the device will set any needed pipelines.
> 
> In the case of V4L2 controls, when there's no subdev API, the main
> driver (e. g. the driver that creates the /dev/video nodes) sends a
> multicast message to all bound I2C drivers. The driver(s) that need 
> them handle it. When the same control may be implemented on different
> drivers, the main driver sends a unicast message to just one driver[1].
> 
> [1] There are several non-MC drivers that have multiple ways to
> control some things, like doing scaling or adjust volume levels at
> either the bridge driver or at a subdriver.
> 
> There's nothing wrong with this approach: it works, it is simpler,
> it is generic. So, if it covers most use cases, why not allowing it
> for usecases where a finer control is not a requirement?

Drivers are written to support hardware, not particular use case. Use case
specific knowledge should be present only in applications, not in drivers.
Doing it otherwise will lead to use case specific drivers and more driver
code to maintain for any particular piece of hardware.

An individual could possibly choose the right driver for his / her use case,
but this approach could hardly work for Linux distribution kernels.

The plain V4L2 interface is generic within its own scope: hardware can be
supported within the hardware model assumed by the interface. However, on
some devices this will end up being a small subset of what the hardware can
do. Besides that, when writing the driver, you need to decide at detail
level what kind of subset that might be.

That's not something anyone writing a driver should need to confront.

> 
> > Adding support to drivers for different "operation modes" --- this is
> > essentially what is being asked for --- is not an approach which could serve
> > either purpose (some functionality with simple interface vs. fully support
> > what the hardware can do, with interfaces allowing that) adequately in the
> > short or the long run.
> 
> Why not?

Let's suppose that the omap3isp driver provided an "operation mode" for more
simple applications.

Would 

Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Russell King - ARM Linux]

On Mon, Mar 13, 2017 at 08:42:15AM -0300, Mauro Carvalho Chehab wrote:
> I don't have myself any hardware with i.MX6. Yet, I believe that
> a low cost board like SolidRun Hummingboard - with comes with a 
> CSI interface compatible with RPi camera modules - will likely
> attract users who need to run generic applications on their
> devices.

As you've previously mentioned about camorama, I've installed it (I
run Ubuntu 16.04 with "gnome-flashback-metacity" on the HB) and I'm
able to use camorama to view the IMX219 camera sensor.

There's some gotcha's though:

* you need to start it on the command line, manually specifying
  which /dev/video device to use, as it always wants to use
  /dev/video0.  With the CODA mem2mem driver loaded, this may not
  be a camera device:

$ v4l2-ctl -d 0 --all
Driver Info (not using libv4l2):
Driver name   : coda
Card type : CODA960
Bus info  : platform:coda
Driver version: 4.11.0

* camorama seems to use the v4lconvert library, and looking at the
  resulting image quality, is rather pixelated - my guess is that
  v4lconvert is using a basic algorithm to de-bayer the data.  It
  also appears to only manage 7fps at best.  The gstreamer neon
  debayer plugin appears to be faster and higher quality.

* it provides five controls - brightness/contrast/color/hue/white
  balance, each of which are not supported by the hardware (IMX219
  supports gain and analogue gain only.)  These controls appear to
  have no effect on the resulting image.

However, using qv4l2 (with the segfault bug in
GeneralTab::updateFrameSize() fixed - m_frameSize, m_frameWidth and
m_frameHeight can be NULL) provides access to all controls.  This
can happen if GeneralTab::inputSection() is not called.

The USB uvcvideo camera achieves around 24fps with functional controls
in camorama (mainly because it provides those exact controls to
userspace.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[bug report] staging/atomisp: Add support for the Intel IPU v2

[Dan Carpenter]

Hello Alan Cox,

The patch a49d25364dfb: "staging/atomisp: Add support for the Intel
IPU v2" from Feb 17, 2017, leads to the following static checker
warning:


drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c:676 
gmin_get_config_var()
warn: impossible condition '(*out_len > (~0)) => (0-u64max > u64max)'

drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
   620  /* Retrieves a device-specific configuration variable.  The dev
   621   * argument should be a device with an ACPI companion, as all
   622   * configuration is based on firmware ID. */
   623  int gmin_get_config_var(struct device *dev, const char *var, char *out, 
size_t *out_len)

out_len is a size_t which is always a ulong.

   624  {
   625  char var8[CFG_VAR_NAME_MAX];
   626  efi_char16_t var16[CFG_VAR_NAME_MAX];
   627  struct efivar_entry *ev;
   628  u32 efiattr_dummy;
   629  int i, j, ret;
   630  unsigned long efilen;
   631  
   632  if (dev && ACPI_COMPANION(dev))
   633  dev = _COMPANION(dev)->dev;
   634  
   635  if (dev)
   636  ret = snprintf(var8, sizeof(var8), "%s_%s", 
dev_name(dev), var);
   637  else
   638  ret = snprintf(var8, sizeof(var8), "gmin_%s", var);
   639  
   640  if (ret < 0 || ret >= sizeof(var8)-1)
   641  return -EINVAL;
   642  
   643  /* First check a hard-coded list of board-specific variables.
   644   * Some device firmwares lack the ability to set EFI variables 
at
   645   * runtime. */
   646  for (i = 0; i < ARRAY_SIZE(hard_vars); i++) {
   647  if (dmi_match(DMI_BOARD_NAME, 
hard_vars[i].dmi_board_name)) {
   648  for (j = 0; hard_vars[i].vars[j].name; j++) {
   649  size_t vl;
   650  const struct gmin_cfg_var *gv;
   651  
   652  gv = _vars[i].vars[j];
   653  vl = strlen(gv->val);
   654  
   655  if (strcmp(var8, gv->name))
   656  continue;
   657  if (vl > *out_len-1)
   658  return -ENOSPC;
   659  
   660  memcpy(out, gv->val, min(*out_len, 
vl+1));
   661  out[*out_len-1] = 0;
   662  *out_len = vl;
   663  
   664  return 0;
   665  }
   666  }
   667  }
   668  
   669  /* Our variable names are ASCII by construction, but EFI names
   670   * are wide chars.  Convert and zero-pad. */
   671  memset(var16, 0, sizeof(var16));
   672  for (i=0; var8[i] && i < sizeof(var8); i++)
   673  var16[i] = var8[i];
   674  
   675  /* To avoid owerflows when calling the efivar API */
   676  if (*out_len > ULONG_MAX)

This is impossible.  Was UINT_MAX intended?

   677  return -EINVAL;
   678  

regards,
dan carpenter

[PATCH] staging: atomisp: potential underflow in atomisp_get_metadata_by_type()

[Dan Carpenter]

md_type is an enum.  On my tests, GCC treats it as unsigned but
according to the C standard it's an implementation dependant thing so we
should check for negatives.

Fixes: a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
Signed-off-by: Dan Carpenter 

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index d9a5c24633cb..0d77ebc5c865 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -3005,7 +3005,7 @@ int atomisp_get_metadata_by_type(struct 
atomisp_sub_device *asd, int flag,
return 0;
 
md_type = md->type;
-   if (md_type >= ATOMISP_METADATA_TYPE_NUM)
+   if (md_type < 0 || md_type >= ATOMISP_METADATA_TYPE_NUM)
return -EINVAL;
 
/* This is done in the atomisp_buf_done() */

Conditional sys/sysmacros.h inclusion

[Ingo Feinerer]

Hi,

please find attached a diff that makes the inclusion of the sys/sysmacros.h
header file conditional. I noticed it on OpenBSD which has no sys/sysmacros.h,
so compilation fails there.

Best regards,
Ingo

diff --git a/configure.ac b/configure.ac
index f3269728a..ae58da377 100644
--- a/configure.ac
+++ b/configure.ac
@@ -146,6 +146,7 @@ if test "x$gl_cv_func_ioctl_posix_signature" = xyes; then
 fi
 
 AC_CHECK_FUNCS([__secure_getenv secure_getenv])
+AC_HEADER_MAJOR
 
 # Check host os
 case "$host_os" in
diff --git a/lib/libv4lconvert/control/libv4lcontrol.c 
b/lib/libv4lconvert/control/libv4lcontrol.c
index 59f28b137..1e784eda8 100644
--- a/lib/libv4lconvert/control/libv4lcontrol.c
+++ b/lib/libv4lconvert/control/libv4lcontrol.c
@@ -20,7 +20,9 @@
  */
 
 #include 
+#if defined(MAJOR_IN_SYSMACROS)
 #include 
+#endif
 #include 
 #include 
 #include 

Re: [PATCH] staging: atomisp: use k{v}zalloc instead of k{v}alloc and memset

[Dan Carpenter]

On Mon, Mar 13, 2017 at 07:54:21PM +0900, Daeseok Youn wrote:
> If the atomisp_kernel_zalloc() has "true" as a second parameter, it
> tries to allocate zeroing memory from kmalloc(vmalloc) and memset.
> But using kzalloc is rather than kmalloc followed by memset with 0.
> (vzalloc is for same reason with kzalloc)
> 
> And also atomisp_kernel_malloc() can be used with
> atomisp_kernel_zalloc(, false);
> 

We should just change all the callers to kvmalloc() and kvzmalloc().

regards,
dan carpenter

Re: [PATCH v3 4/6] drm: bridge: dw-hdmi: Switch to V4L bus format and encodings

[Jose Abreu]

Hi Neil,


On 09-03-2017 14:27, Jose Abreu wrote:
> Hi Neil,
>
>
> On 08-03-2017 12:12, Neil Armstrong wrote:
>> Hi Jose,
>>
>> It seems here that we only have the RGB444<->YUV444 8bit tables, from the 
>> Amlogic
>> source I have the following for 10bit, 12bit and 16bit for itu601 :
>>
>> static const u16 csc_coeff_rgb_out_eitu601_10b[3][4] = {
>>  { 0x2000, 0x6926, 0x74fd, 0x043b },
>>  { 0x2000, 0x2cdd, 0x, 0x7a65 },
>>  { 0x2000, 0x, 0x38b4, 0x78ea }
>> };
>>
>> static const u16 csc_coeff_rgb_out_eitu601_12b_16b[3][4] = {
>>  { 0x2000, 0x6926, 0x74fd, 0x10ee },
>>  { 0x2000, 0x2cdd, 0x, 0x6992 },
>>  { 0x2000, 0x, 0x38b4, 0x63a6 }
>> };
> These two do not match anything I have here.
>
>> static const u16 csc_coeff_rgb_in_eitu601_10b[3][4] = {
>>  { 0x2591, 0x1322, 0x074b, 0x },
>>  { 0x6535, 0x2000, 0x7acc, 0x0800 },
>>  { 0x6acd, 0x7534, 0x2000, 0x0800 }
>> };
> This is more or less correct, I have small offsets. Note that I
> even have offsets with the values that are in dw-hdmi driver,
> which can be caused because I'm seeing the latest document that
> contain these values. I guess they were updated.
>
>> static const u16 csc_coeff_rgb_in_eitu601_12b_16b[3][4] = {
>>  { 0x2591, 0x1322, 0x074b, 0x },
>>  { 0x6535, 0x2000, 0x7acc, 0x2000 },
>>  { 0x6acd, 0x7534, 0x2000, 0x2000 }
>> };
> The same for this.
>
>> But I miss the itu709 values.
>>
>> Could you confirm these values and maybe provide the itu709 values ?
> I will have to check if I can provide you the values. I will get
> back to you.

Sorry but looks like I won't be able to provide you the correct
values :/ If you are working for a Synopsys customer you can
contact our CAE (If so I can guide you to the correct CAE).

Anyway, unless you can test the values you have I suggest you
don't use them, they do not match what I have here and I can't
test them because right now I don't have a setup (I'm assuming
that your CSC block within the controller was not modified).

Best regards,
Jose Miguel Abreu

Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Mauro Carvalho Chehab]

Em Mon, 13 Mar 2017 10:58:42 +
Russell King - ARM Linux  escreveu:

> On Mon, Mar 13, 2017 at 11:44:50AM +0100, Hans Verkuil wrote:
> > On 03/12/2017 06:56 PM, Steve Longerbeam wrote:  
> > > In summary, I do like the media framework, it's a good abstraction of
> > > hardware pipelines. It does require a lot of system level knowledge to
> > > configure, but as I said that is a matter of good documentation.  
> > 
> > And the reason we went into this direction is that the end-users that use
> > these SoCs with complex pipelines actually *need* this functionality. Which
> > is also part of the reason why work on improved userspace support gets
> > little attention: they don't need to have a plugin that allows generic V4L2
> > applications to work (at least with simple scenarios).  
> 
> If you stop inheriting controls from the capture sensor to the v4l2
> capture device, then this breaks - generic v4l2 applications are not
> going to be able to show the controls, because they're not visible at
> the v4l2 capture device anymore.  They're only visible through the
> subdev interfaces, which these generic applications know nothing about.

True. That's why IMHO, the best is to do control inheritance when
there are use cases for generic applications and is possible for
the driver to do it (e. g. when the pipeline is not too complex
to prevent it to work).

As Hans said, for the drivers currently upstreamed at drivers/media,
there are currently very little interest on running generic apps 
there, as they're meant to be used inside embedded hardware using
specialized applications.

I don't have myself any hardware with i.MX6. Yet, I believe that
a low cost board like SolidRun Hummingboard - with comes with a 
CSI interface compatible with RPi camera modules - will likely
attract users who need to run generic applications on their
devices.

So, I believe that it makes sense for i.MX6 driver to inherit
controls from video devnode.

Thanks,
Mauro

Re: [PATCHv5 07/16] atmel-isi: remove dependency of the soc-camera framework

[Hans Verkuil]

On 03/12/2017 05:44 PM, Guennadi Liakhovetski wrote:
> Hi Hans,
> 
> Thanks for the patch. Why hasn't this patch been CCed to Josh Wu? Is he 
> still at Atmel? Adding to CC to check.

To the best of my knowledge Josh no longer works for Atmel/Microchip, and 
Songjun
took over.

> 
> A general comment: this patch doesn't only remove soc-camera dependencies, 
> it also changes the code and the behaviour. I would prefer to break that 
> down in multiple patches, or remove this driver completely and add a new 
> one. I'll provide some comments, but of course I cannot make an extensive 
> review of a 1200-line of change patch without knowing the hardware and the 
> set ups well enough.
> 
> On Sat, 11 Mar 2017, Hans Verkuil wrote:
> 
>> From: Hans Verkuil 
>>
>> This patch converts the atmel-isi driver from a soc-camera driver to a driver
>> that is stand-alone.
>>
>> Signed-off-by: Hans Verkuil 
>> ---
>>  drivers/media/platform/soc_camera/Kconfig |3 +-
>>  drivers/media/platform/soc_camera/atmel-isi.c | 1209 
>> +++--
>>  2 files changed, 714 insertions(+), 498 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/Kconfig 
>> b/drivers/media/platform/soc_camera/Kconfig
>> index 86d74788544f..a37ec91b026e 100644
>> --- a/drivers/media/platform/soc_camera/Kconfig
>> +++ b/drivers/media/platform/soc_camera/Kconfig
>> @@ -29,9 +29,8 @@ config VIDEO_SH_MOBILE_CEU
>>  
>>  config VIDEO_ATMEL_ISI
>>  tristate "ATMEL Image Sensor Interface (ISI) support"
>> -depends on VIDEO_DEV && SOC_CAMERA
>> +depends on VIDEO_V4L2 && OF && HAS_DMA
>>  depends on ARCH_AT91 || COMPILE_TEST
>> -depends on HAS_DMA
>>  select VIDEOBUF2_DMA_CONTIG
>>  ---help---
>>This module makes the ATMEL Image Sensor Interface available
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c 
>> b/drivers/media/platform/soc_camera/atmel-isi.c
>> index 46de657c3e6d..a6d60c2e207d 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -22,18 +22,22 @@
>>  #include 
>>  #include 
>>  #include 
>> -
>> -#include 
>> -#include 
>> +#include 
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #include "atmel-isi.h"
>>  
>> -#define MAX_BUFFER_NUM  32
>>  #define MAX_SUPPORT_WIDTH   2048
>>  #define MAX_SUPPORT_HEIGHT  2048
>> -#define VID_LIMIT_BYTES (16 * 1024 * 1024)
>>  #define MIN_FRAME_RATE  15
>>  #define FRAME_INTERVAL_MILLI_SEC(1000 / MIN_FRAME_RATE)
>>  
>> @@ -65,9 +69,37 @@ struct frame_buffer {
>>  struct list_head list;
>>  };
>>  
>> +struct isi_graph_entity {
>> +struct device_node *node;
>> +
>> +struct v4l2_async_subdev asd;
>> +struct v4l2_subdev *subdev;
>> +};
>> +
>> +/*
>> + * struct isi_format - ISI media bus format information
>> + * @fourcc: Fourcc code for this format
>> + * @mbus_code:  V4L2 media bus format code.
>> + * @bpp:Bytes per pixel (when stored in memory)
>> + * @swap:   Byte swap configuration value
>> + * @support:Indicates format supported by subdev
>> + * @skip:   Skip duplicate format supported by subdev
>> + */
>> +struct isi_format {
>> +u32 fourcc;
>> +u32 mbus_code;
>> +u8  bpp;
>> +u32 swap;
>> +
>> +boolsupport;
>> +boolskip;
>> +};
>> +
>> +
>>  struct atmel_isi {
>>  /* Protects the access of variables shared with the ISR */
>> -spinlock_t  lock;
>> +spinlock_t  irqlock;
>> +struct device   *dev;
>>  void __iomem*regs;
>>  
>>  int sequence;
>> @@ -76,7 +108,7 @@ struct atmel_isi {
>>  struct fbd  *p_fb_descriptors;
>>  dma_addr_t  fb_descriptors_phys;
>>  struct  list_head dma_desc_head;
>> -struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
>> +struct isi_dma_desc dma_desc[VIDEO_MAX_FRAME];
>>  boolenable_preview_path;
>>  
>>  struct completion   complete;
>> @@ -90,9 +122,22 @@ struct atmel_isi {
>>  struct list_headvideo_buffer_list;
>>  struct frame_buffer *active;
>>  
>> -struct soc_camera_host  soc_host;
>> +struct v4l2_device  v4l2_dev;
>> +struct video_device *vdev;
>> +struct v4l2_async_notifier  notifier;
>> +struct isi_graph_entity entity;
>> +struct v4l2_format  fmt;
>> +
>> +struct isi_format   **user_formats;
>> +unsigned intnum_user_formats;
>> +const struct isi_format

Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Hans Verkuil]

On 03/13/2017 11:58 AM, Russell King - ARM Linux wrote:
> On Mon, Mar 13, 2017 at 11:44:50AM +0100, Hans Verkuil wrote:
>> On 03/12/2017 06:56 PM, Steve Longerbeam wrote:
>>> In summary, I do like the media framework, it's a good abstraction of
>>> hardware pipelines. It does require a lot of system level knowledge to
>>> configure, but as I said that is a matter of good documentation.
>>
>> And the reason we went into this direction is that the end-users that use
>> these SoCs with complex pipelines actually *need* this functionality. Which
>> is also part of the reason why work on improved userspace support gets
>> little attention: they don't need to have a plugin that allows generic V4L2
>> applications to work (at least with simple scenarios).
> 
> If you stop inheriting controls from the capture sensor to the v4l2
> capture device, then this breaks - generic v4l2 applications are not
> going to be able to show the controls, because they're not visible at
> the v4l2 capture device anymore.  They're only visible through the
> subdev interfaces, which these generic applications know nothing about.
> 
>> If you want to blame anyone for this, blame Nokia who set fire to
>> their linux-based phones and thus to the funding for this work.
> 
> No, I think that's completely unfair to Nokia.  If the MC approach is
> the way you want to go, you should be thanking Nokia for the amount of
> effort that they have put in to it, and recognising that it was rather
> unfortunate that the market had changed, which meant that they weren't
> able to continue.
> 
> No one has any right to require any of us to finish what we start
> coding up in open source, unless there is a contractual obligation in
> place.  That goes for Nokia too.
> 
> Nokia's decision had ramifications far and wide (resulting in knock on
> effects in TI and further afield), so don't think for a moment I wasn't
> affected by what happened in Nokia.  Even so, it was a decision for
> Nokia to make, they had the right to make it, and we have no right to
> attribute "blame" to Nokia for having made that decision.
> 
> To even suggest that Nokia should be blamed is absurd.
> 
> Open source gives rights to everyone.  It gives rights to contribute
> and use, but it also gives rights to walk away without notice (remember
> the "as is" and "no warranty" clauses?)

Sorry, unfortunate choice of words. While it lasted they did great work.
But the reason why MC development stopped for quite some time (esp. the
work on userspace software) was because the funding from Nokia dried up.

Regards,

Hans

Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Russell King - ARM Linux]

On Mon, Mar 13, 2017 at 11:44:50AM +0100, Hans Verkuil wrote:
> On 03/12/2017 06:56 PM, Steve Longerbeam wrote:
> > In summary, I do like the media framework, it's a good abstraction of
> > hardware pipelines. It does require a lot of system level knowledge to
> > configure, but as I said that is a matter of good documentation.
> 
> And the reason we went into this direction is that the end-users that use
> these SoCs with complex pipelines actually *need* this functionality. Which
> is also part of the reason why work on improved userspace support gets
> little attention: they don't need to have a plugin that allows generic V4L2
> applications to work (at least with simple scenarios).

If you stop inheriting controls from the capture sensor to the v4l2
capture device, then this breaks - generic v4l2 applications are not
going to be able to show the controls, because they're not visible at
the v4l2 capture device anymore.  They're only visible through the
subdev interfaces, which these generic applications know nothing about.

> If you want to blame anyone for this, blame Nokia who set fire to
> their linux-based phones and thus to the funding for this work.

No, I think that's completely unfair to Nokia.  If the MC approach is
the way you want to go, you should be thanking Nokia for the amount of
effort that they have put in to it, and recognising that it was rather
unfortunate that the market had changed, which meant that they weren't
able to continue.

No one has any right to require any of us to finish what we start
coding up in open source, unless there is a contractual obligation in
place.  That goes for Nokia too.

Nokia's decision had ramifications far and wide (resulting in knock on
effects in TI and further afield), so don't think for a moment I wasn't
affected by what happened in Nokia.  Even so, it was a decision for
Nokia to make, they had the right to make it, and we have no right to
attribute "blame" to Nokia for having made that decision.

To even suggest that Nokia should be blamed is absurd.

Open source gives rights to everyone.  It gives rights to contribute
and use, but it also gives rights to walk away without notice (remember
the "as is" and "no warranty" clauses?)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] staging: atomisp: use k{v}zalloc instead of k{v}alloc and memset

[Daeseok Youn]

If the atomisp_kernel_zalloc() has "true" as a second parameter, it
tries to allocate zeroing memory from kmalloc(vmalloc) and memset.
But using kzalloc is rather than kmalloc followed by memset with 0.
(vzalloc is for same reason with kzalloc)

And also atomisp_kernel_malloc() can be used with
atomisp_kernel_zalloc(, false);

Signed-off-by: Daeseok Youn 
---
I think kvmalloc() or kvzalloc() can be used to allocate memory if there is
no reason to use vmalloc() when the requested bytes is over PAGE_SIZE.

 .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 25 --
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index d9a5c24..44b2244 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -86,32 +86,35 @@
 };
 
 /*
- * atomisp_kernel_malloc: chooses whether kmalloc() or vmalloc() is preferable.
+ * atomisp_kernel_malloc:
+ * allocating memory from atomisp_kernel_zalloc() without zeroing memory.
  *
  * It is also a wrap functions to pass into css framework.
  */
 void *atomisp_kernel_malloc(size_t bytes)
 {
-   /* vmalloc() is preferable if allocating more than 1 page */
-   if (bytes > PAGE_SIZE)
-   return vmalloc(bytes);
-
-   return kmalloc(bytes, GFP_KERNEL);
+   return atomisp_kernel_zalloc(bytes, false);
 }
 
 /*
- * atomisp_kernel_zalloc: chooses whether set 0 to the allocated memory.
+ * atomisp_kernel_zalloc: chooses whether set 0 to the allocated memory
+ * with k{z,m}alloc or v{z,m}alloc
  *
  * It is also a wrap functions to pass into css framework.
  */
 void *atomisp_kernel_zalloc(size_t bytes, bool zero_mem)
 {
-   void *ptr = atomisp_kernel_malloc(bytes);
+   /* vmalloc() is preferable if allocating more than 1 page */
+   if (bytes > PAGE_SIZE) {
+   if (zero_mem)
+   return vzalloc(bytes);
+   return vmalloc(bytes);
+   }
 
-   if (ptr && zero_mem)
-   memset(ptr, 0, bytes);
+   if (zero_mem)
+   return kzalloc(bytes, GFP_KERNEL);
 
-   return ptr;
+   return kmalloc(bytes, GFP_KERNEL);
 }
 
 /*
-- 
1.9.1

Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging

[Brian Starkey]

On Sun, Mar 12, 2017 at 02:34:14PM +0100, Benjamin Gaignard wrote:

2017-03-09 18:38 GMT+01:00 Laura Abbott :

On 03/09/2017 02:00 AM, Benjamin Gaignard wrote:

2017-03-06 17:04 GMT+01:00 Daniel Vetter :

On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote:

On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote:


No one gave a thing about android in upstream, so Greg KH just dumped it
all into staging/android/. We've discussed ION a bunch of times, recorded
anything we'd like to fix in staging/android/TODO, and Laura's patch
series here addresses a big chunk of that.



This is pretty much the same approach we (gpu folks) used to de-stage the
syncpt stuff.


Well, there's also the fact that quite a few people have issues with the
design (like Laurent).  It seems like a lot of them have either got more
comfortable with it over time, or at least not managed to come up with
any better ideas in the meantime.


See the TODO, it has everything a really big group (look at the patch for
the full Cc: list) figured needs to be improved at LPC 2015. We don't just
merge stuff because merging stuff is fun :-)

Laurent was even in that group ...
-Daniel


For me those patches are going in the right direction.

I still have few questions:
- since alignment management has been remove from ion-core, should it
be also removed from ioctl structure ?


Yes, I think I'm going to go with the suggestion to fixup the ABI
so we don't need the compat layer and as part of that I'm also
dropping the align argument.


- can you we ride off ion_handle (at least in userland) and only
export a dma-buf descriptor ?


Yes, I think this is the right direction given we're breaking
everything anyway. I was debating trying to keep the two but
moving to only dma bufs is probably cleaner. The only reason
I could see for keeping the handles is running out of file
descriptors for dma-bufs but that seems unlikely.


In the future how can we add new heaps ?
Some platforms have very specific memory allocation
requirements (just have a look in the number of gem custom allocator in drm)
Do you plan to add heap type/mask for each ?


Yes, that was my thinking.


My concern is about the policy to adding heaps, will you accept
"customs" heap per
platforms ? per devices ? or only generic ones ?
If you are too strict, we will have lot of out-of-tree heaps and if
you accept of of them
it will be a nightmare to maintain



Are you concerned about actual heaps (e.g. a carveout at 0x8000 vs
a carveout at 0x6000) or heap types?

For heap types, I think the policy can be strict - if it's generally
useful then it should live in-tree in ion. Otherwise, it would be
out-of-tree. I'd expect most "custom" heaps to be parameterisable to
the point of being generally useful.

For actual heap instances, I would expect them to be communicated via
reserved-memory regions or something similar, and so the maintenance
burden is pretty low.

The existing query ioctl can allow heap IDs to get assigned
dynamically at runtime, so there's no need to reserve "bit 6" for
"CUSTOM_ACME_HEAP_1"


Another point is how can we put secure rules (like selinux policy) on
heaps since all the allocations
go to the same device (/dev/ion) ? For example, until now, in Android
we have to give the same
access rights to all the process that use ION.
It will become problem when we will add secure heaps because we won't
be able to distinguish secure
processes to standard ones or set specific policy per heaps.
Maybe I'm wrong here but I have never see selinux policy checking an
ioctl field but if that
exist it could be a solution.



I might be thinking of a different type of "secure", but...

Should the security of secure heaps be enforced by OS-level
permissions? I don't know about other architectures, but at least on
arm/arm64 this is enforced in hardware; it doesn't matter who has
access to the ion heap, because only secure devices (or the CPU
running a secure process) is physically able to access the memory
backing the buffer.

In fact, in the use-cases I know of, the process asking for the ion
allocation is not a secure process, and so we wouldn't *want* to
restrict the secure heap to be allocated from only by secure
processes.

-Brian





Benjamin



Thanks,
Laura

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Hans Verkuil]

On 03/13/2017 11:45 AM, Russell King - ARM Linux wrote:
> On Mon, Mar 13, 2017 at 11:02:34AM +0100, Hans Verkuil wrote:
>> On 03/11/2017 07:14 PM, Steve Longerbeam wrote:
>>> The event must be user visible, otherwise the user has no indication
>>> the error, and can't correct it by stream restart.
>>
>> In that case the driver can detect this and call vb2_queue_error. It's
>> what it is there for.
>>
>> The event doesn't help you since only this driver has this issue. So nobody
>> will watch this event, unless it is sw specifically written for this SoC.
>>
>> Much better to call vb2_queue_error to signal a fatal error (which this
>> apparently is) since there are more drivers that do this, and vivid supports
>> triggering this condition as well.
> 
> So today, I can fiddle around with the IMX219 registers to help gain
> an understanding of how this sensor works.  Several of the registers
> (such as the PLL setup [*]) require me to disable streaming on the
> sensor while changing them.
> 
> This is something I've done many times while testing various ideas,
> and is my primary way of figuring out and testing such things.
> 
> Whenever I resume streaming (provided I've let the sensor stop
> streaming at a frame boundary) it resumes as if nothing happened.  If I
> stop the sensor mid-frame, then I get the rolling issue that Steve
> reports, but once the top of the frame becomes aligned with the top of
> the capture, everything then becomes stable again as if nothing happened.
> 
> The side effect of what you're proposing is that when I disable streaming
> at the sensor by poking at its registers, rather than the capture just
> stopping, an error is going to be delivered to gstreamer, and gstreamer
> is going to exit, taking the entire capture process down.
> 
> This severely restricts the ability to be able to develop and test
> sensor drivers.
> 
> So, I strongly disagree with you.
> 
> Loss of capture frames is not necessarily a fatal error - as I have been
> saying repeatedly.  In Steve's case, there's some unknown interaction
> between the source and iMX6 hardware that is causing the instability,
> but that is simply not true of other sources, and I oppose any idea that
> we should cripple the iMX6 side of the capture based upon just one
> hardware combination where this is a problem.
> 
> Steve suggested that the problem could be in the iMX6 CSI block - and I
> note comparing Steve's code with the code in FSL's repository that there
> are some changes that are missing in Steve's code to do with the CCIR656
> sync code setup, particularly for >8 bit.  The progressive CCIR656 8-bit
> setup looks pretty similar though - but I think what needs to be asked
> is whether the same problem is visible using the FSL/NXP vendor kernel.
> 
> 
> * - the PLL setup is something that requires research at the moment.
> Sony's official position (even to their customers) is that they do not
> supply the necessary information, instead they expect customers to tell
> them the capture settings they want, and Sony will throw the values into
> a spreadsheet, and they'll supply the register settings back to the
> customer.  Hence, the only way to proceed with a generic driver for
> this sensor is to experiment, and experimenting requires the ability to
> pause the stream at the sensor while making changes.  Take this away,
> and we're stuck with the tables-of-register-settings-for-set-of-fixed-
> capture-settings approach.  I've made a lot of progress away from this
> which is all down to the flexibility afforded by _not_ killing the
> capture process.
> 

In other words: Steve should either find a proper fix for this, or only
call vb2_queue_error in this specific case. Sending an event that nobody
will know how to handle or what to do with is pretty pointless IMHO.

Let's just give him time to try and figure out the real issue here.

Regards,

Hans

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Russell King - ARM Linux]

On Mon, Mar 13, 2017 at 11:02:34AM +0100, Hans Verkuil wrote:
> On 03/11/2017 07:14 PM, Steve Longerbeam wrote:
> > The event must be user visible, otherwise the user has no indication
> > the error, and can't correct it by stream restart.
> 
> In that case the driver can detect this and call vb2_queue_error. It's
> what it is there for.
> 
> The event doesn't help you since only this driver has this issue. So nobody
> will watch this event, unless it is sw specifically written for this SoC.
> 
> Much better to call vb2_queue_error to signal a fatal error (which this
> apparently is) since there are more drivers that do this, and vivid supports
> triggering this condition as well.

So today, I can fiddle around with the IMX219 registers to help gain
an understanding of how this sensor works.  Several of the registers
(such as the PLL setup [*]) require me to disable streaming on the
sensor while changing them.

This is something I've done many times while testing various ideas,
and is my primary way of figuring out and testing such things.

Whenever I resume streaming (provided I've let the sensor stop
streaming at a frame boundary) it resumes as if nothing happened.  If I
stop the sensor mid-frame, then I get the rolling issue that Steve
reports, but once the top of the frame becomes aligned with the top of
the capture, everything then becomes stable again as if nothing happened.

The side effect of what you're proposing is that when I disable streaming
at the sensor by poking at its registers, rather than the capture just
stopping, an error is going to be delivered to gstreamer, and gstreamer
is going to exit, taking the entire capture process down.

This severely restricts the ability to be able to develop and test
sensor drivers.

So, I strongly disagree with you.

Loss of capture frames is not necessarily a fatal error - as I have been
saying repeatedly.  In Steve's case, there's some unknown interaction
between the source and iMX6 hardware that is causing the instability,
but that is simply not true of other sources, and I oppose any idea that
we should cripple the iMX6 side of the capture based upon just one
hardware combination where this is a problem.

Steve suggested that the problem could be in the iMX6 CSI block - and I
note comparing Steve's code with the code in FSL's repository that there
are some changes that are missing in Steve's code to do with the CCIR656
sync code setup, particularly for >8 bit.  The progressive CCIR656 8-bit
setup looks pretty similar though - but I think what needs to be asked
is whether the same problem is visible using the FSL/NXP vendor kernel.


* - the PLL setup is something that requires research at the moment.
Sony's official position (even to their customers) is that they do not
supply the necessary information, instead they expect customers to tell
them the capture settings they want, and Sony will throw the values into
a spreadsheet, and they'll supply the register settings back to the
customer.  Hence, the only way to proceed with a generic driver for
this sensor is to experiment, and experimenting requires the ability to
pause the stream at the sensor while making changes.  Take this away,
and we're stuck with the tables-of-register-settings-for-set-of-fixed-
capture-settings approach.  I've made a lot of progress away from this
which is all down to the flexibility afforded by _not_ killing the
capture process.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

[Hans Verkuil]

On 03/12/2017 06:56 PM, Steve Longerbeam wrote:
> 
> 
> On 03/11/2017 11:37 PM, Russell King - ARM Linux wrote:
>> On Sat, Mar 11, 2017 at 07:31:18PM -0800, Steve Longerbeam wrote:
>>>
>>>
>>> On 03/11/2017 10:59 AM, Russell King - ARM Linux wrote:
 On Sat, Mar 11, 2017 at 10:54:55AM -0800, Steve Longerbeam wrote:
>
>
> On 03/11/2017 10:45 AM, Russell King - ARM Linux wrote:
>> I really don't think expecting the user to understand and configure
>> the pipeline is a sane way forward.  Think about it - should the
>> user need to know that, because they have a bayer-only CSI data
>> source, that there is only one path possible, and if they try to
>> configure a different path, then things will just error out?
>>
>> For the case of imx219 connected to iMX6, it really is as simple as
>> "there is only one possible path" and all the complexity of the media
>> interfaces/subdevs is completely unnecessary.  Every other block in
>> the graph is just noise.
>>
>> The fact is that these dot graphs show a complex picture, but reality
>> is somewhat different - there's only relatively few paths available
>> depending on the connected source and the rest of the paths are
>> completely useless.
>>
>
> I totally disagree there. Raw bayer requires passthrough yes, but for
> all other media bus formats on a mipi csi-2 bus, and all other media
> bus formats on 8-bit parallel buses, the conersion pipelines can be
> used for scaling, CSC, rotation, and motion-compensated de-interlacing.

 ... which only makes sense _if_ your source can produce those formats.
 We don't actually disagree on that.
>>>
>>> ...and there are lots of those sources! You should try getting out of
>>> your imx219 shell some time, and have a look around! :)
>>
>> If you think that, you are insulting me.  I've been thinking about this
>> from the "big picture" point of view.  If you think I'm only thinking
>> about this from only the bayer point of view, you're wrong.
> 
> No insult there, you have my utmost respect Russel. Me gives you the
> Ali-G "respec!" :)
> 
> It was just a light-hearted attempt at suggesting you might be too
> entangled with the imx219 (or short on hardware access, which I can
> certainly understand).
> 
> 
>>
>> Given what Mauro has said, I'm convinced that the media controller stuff
>> is a complete failure for usability, and adding further drivers using it
>> is a mistake.
>>
> 
> I do agree with you that MC places a lot of burden on the user to
> attain a lot of knowledge of the system's architecture. That's really
> why I included that control inheritance patch, to ease the burden
> somewhat.
> 
> On the other hand, I also think this just requires that MC drivers have
> very good user documentation.
> 
> And my other point is, I think most people who have a need to work with
> the media framework on a particular platform will likely already be
> quite familiar with that platform.
> 
>> I counter your accusation by saying that you are actually so focused on
>> the media controller way of doing things that you can't see the bigger
>> picture here.
>>
> 
> Yeah I've been too mired in the details of this driver.
> 
> 
>> So, tell me how the user can possibly use iMX6 video capture without
>> resorting to opening up a terminal and using media-ctl to manually
>> configure the pipeline.  How is the user going to control the source
>> device without using media-ctl to find the subdev node, and then using
>> v4l2-ctl on it.  How is the user supposed to know which /dev/video*
>> node they should be opening with their capture application?
> 
> The media graph for imx6 is fairly self-explanatory in my opinion.
> Yes that graph has to be generated, but just with a simple 'media-ctl
> --print-dot', I don't see how that is difficult for the user.
> 
> The graph makes it quite clear which subdev node belongs to which
> entity.
> 
> As for which /dev/videoX node to use, I hope I made it fairly clear
> in the user doc what functions each node performs. But I will review
> the doc again and make sure it's been made explicitly clear.
> 
> 
>>
>> If you can actually respond to the points that I've been raising about
>> end user usability, then we can have a discussion.
> 
> Right, I haven't added my input to the middle-ware discussions (libv4l,
> v4lconvert, and the auto-pipeline-configuration library work). I can
> only say at this point that v4lconvert does indeed sound broken w.r.t
> bayer formats from your description. But it also sounds like an isolated
> problem and it just needs a patch to allow passing bayer through without
> software conversion.
> 
> I wish I had the IMX219 to help you debug these bayer issues. I don't
> have any bayer sources.
> 
> In summary, I do like the media framework, it's a good abstraction of
> hardware pipelines. It does require a lot of system level knowledge to
> configure, but as I said that is a 

Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue

[Hans Verkuil]

On 03/13/2017 10:32 AM, Wu, Songjun wrote:
> 
> 
> On 3/13/2017 17:25, Hans Verkuil wrote:
>> On 03/13/2017 06:53 AM, Wu, Songjun wrote:
>>>
>>>
>>> On 3/9/2017 18:57, Hans Verkuil wrote:
 Hi Songjun,

 On 08/03/17 03:25, Wu, Songjun wrote:
> Hi Colin,
>
> Thank you for your comment.
> It is a bug, will be fixed in the next patch.

 Do you mean that you will provide a new patch for this? Is there anything
 wrong with this patch? It seems reasonable to me.

>>> Hi Hans,
>>>
>>> I see this patch is merged in git://linuxtv.org/media_tree.git.
>>> So I do not need submit isc-pipeline-v3 patch, just submit the patches,
>>> based on the current master branch?
>>
>> Huh? Where do you see that this patch is merged? I don't see it in the 
>> media_tree master
>> branch.
>>
> Hi Hans,
> 
> I see this patch on the master branch in media_tree.
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/atmel/atmel-isc.c

???

That's a link to the source, not a patch.

And that source still does <= HIST_ENTRIES.

Still confused,

Hans

> 
>> Regards,
>>
>>  Hans
>>
>>>
 Regards,

Hans

>
> On 3/7/2017 22:30, Colin King wrote:
>> From: Colin Ian King 
>>
>> The are only HIST_ENTRIES worth of entries in  hist_entry however the
>> for-loop is iterating one too many times leasing to a read access off
>> the end off the array ctrls->hist_entry.  Fix this by iterating by
>> the correct number of times.
>>
>> Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")
>>
>> Signed-off-by: Colin Ian King 
>> ---
>>  drivers/media/platform/atmel/atmel-isc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/atmel/atmel-isc.c 
>> b/drivers/media/platform/atmel/atmel-isc.c
>> index b380a7d..7dacf8c 100644
>> --- a/drivers/media/platform/atmel/atmel-isc.c
>> +++ b/drivers/media/platform/atmel/atmel-isc.c
>> @@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
>>  regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);
>>
>>  *hist_count = 0;
>> -for (i = 0; i <= HIST_ENTRIES; i++)
>> +for (i = 0; i < HIST_ENTRIES; i++)
>>  *hist_count += i * (*hist_entry++);
>>  }
>>
>>

>>

Re: [PATCH] [media] vcodev: mediatek: add missing include in JPEG decoder driver

[Hans Verkuil]

On 03/12/2017 09:13 PM, Jérémy Lefaure wrote:
> The driver uses kzalloc and kfree functions. So it should include
> linux/slab.h. This header file is implicitly included by v4l2-common.h
> if CONFIG_SPI is enabled. But when it is disabled, slab.h is not
> included. In this case, the driver does not compile:
> 
> drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c: In function ‘mtk_jpeg_open’:
> drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c:1017:8: error: implicit
> declaration of function ‘kzalloc’
> [-Werror=implicit-function-declaration]
>   ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> ^~~
> drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c:1017:6: warning:
> assignment makes pointer from integer without a cast [-Wint-conversion]
>   ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>   ^
> drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c:1047:2: error: implicit
> declaration of function ‘kfree’ [-Werror=implicit-function-declaration]
>   kfree(ctx);
>   ^
> 
> This patch adds the missing include to fix this issue.

Thanks for the patch, but someone else made the same fix already and it is
queued up for 4.12.

Regards,

Hans

> 
> Signed-off-by: Jérémy Lefaure 
> ---
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c 
> b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> index b10183f7942b..f9bd58ce7d32 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> 

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Hans Verkuil]

On 03/11/2017 07:14 PM, Steve Longerbeam wrote:
> 
> 
> On 03/11/2017 03:39 AM, Hans Verkuil wrote:
>> On 10/03/17 19:37, Steve Longerbeam wrote:
>>> Hi Hans,
>>>
>>> On 03/10/2017 04:03 AM, Hans Verkuil wrote:
 On 10/03/17 05:52, Steve Longerbeam wrote:
> Add a new FRAME_INTERVAL_ERROR event to signal that a video capture or
> output device has measured an interval between the reception or transmit
> completion of two consecutive frames of video that is outside the nominal
> frame interval by some tolerance value.

 Reading back what was said on this I agree with Sakari that this doesn't
 belong here.

 Userspace can detect this just as easily (if not easier) with a timeout.

>>>
>>>
>>> Unfortunately measuring frame intervals from userland is not accurate
>>> enough for i.MX6.
>>>
>>> The issue here is that the IPUv3, specifically the CSI unit, can
>>> permanently lose vertical sync if there are truncated frames sent
>>> on the bt.656 bus. We have seen a single missing line of video cause
>>> loss of vertical sync. The only way to correct this is to shutdown
>>> the IPU capture hardware and restart, which can be accomplished
>>> simply by restarting streaming from userland.
>>>
>>> There are no other indicators from the sensor about these short
>>> frame events (believe me, we've exhausted all avenues with the ADV718x).
>>> And the IPUv3 DMA engine has no status indicators for short frames
>>> either. So the only way to detect them is by measuring frame intervals.
>>>
>>> The intervals have to be able to resolve a single line of missing video.
>>> With a PAL video source that requires better than 58 usec accuracy.
>>>
>>> There is too much uncertainty to resolve this at user level. The
>>> driver is able to resolve this by measuring intervals between hardware
>>> interrupts as long as interrupt latency is reasonably low, and we
>>> have another method using the i.MX6 hardware input capture support
>>> that can measure these intervals very accurately with no errors
>>> introduced by interrupt latency.
>>>
>>> I made this event a private event to imx-media driver in a previous
>>> iteration, so I can return it to a private event, but this can't be
>>> done at user level.
>>
>> It's fine to use an internal event as long as the end-user doesn't
>> see it. But if you lose vsyncs, then you never capture another frame,
>> right?
> 
> No, that's not correct. By loss of vertical sync, I mean the IPU
> captures portions of two different frames, resulting in a permanent
> "split image", with one frame containing portions of two consecutive
> images. Or, the video rolls continuously, if you remember the old CRT
> television sets of yore, it's the same rolling effect.
> 
> 
>> So userspace can detect that (i.e. no new frames arrive) and
>> it can timeout on that. Or you detect it in the driver and restart there,
>> or call vb2_queue_error().
>>
> 
> There is no timeout, the frames keep coming, but they are split images
> or rolling.

Ah, OK. That wasn't clear to me from the description.

> 
>> Anything really as long as this event isn't user-visible :-)
> 
> The event must be user visible, otherwise the user has no indication
> the error, and can't correct it by stream restart.

In that case the driver can detect this and call vb2_queue_error. It's
what it is there for.

The event doesn't help you since only this driver has this issue. So nobody
will watch this event, unless it is sw specifically written for this SoC.

Much better to call vb2_queue_error to signal a fatal error (which this
apparently is) since there are more drivers that do this, and vivid supports
triggering this condition as well.

Regards,

Hans

Re: [PATCH v2 12/23] MAINTAINERS: Add file patterns for media device tree bindings

[Mauro Carvalho Chehab]

Em Mon, 13 Mar 2017 08:43:52 +0100
Geert Uytterhoeven  escreveu:

> Hi Mauro,
> 
> On Mon, Mar 13, 2017 at 2:02 AM, Mauro Carvalho Chehab
>  wrote:
> > Em Sun, 12 Mar 2017 14:16:56 +0100
> > Geert Uytterhoeven  escreveu:
> >  
> >> Submitters of device tree binding documentation may forget to CC
> >> the subsystem maintainer if this is missing.
> >>
> >> Signed-off-by: Geert Uytterhoeven 
> >> Reviewed-by: Javier Martinez Canillas 
> >> Cc: Mauro Carvalho Chehab   
> >
> > As the To: is devicetree, I'm assuming that this patch will be
> > applied there, so:
> >
> > Acked-by: Mauro Carvalho Chehab   
> 
> Thanks!
> 
> > I may also merge via my tree, if that would be better. Just let me
> > know in such case.  
> 
> Please apply to your tree, cfr.
> 
> | Please apply this patch directly if you want to be involved in device
> | tree binding documentation for your subsystem.

I saw that text, but, as you send it to devicetree ML, c/c me and media,
I was in doubt if "your subsystem" would be DT subsystem or media.

I'll apply it on my tree.

Regards,
Mauro

Re: [PATCH v5 21/39] UAPI: Add media UAPI Kbuild file

[Hans Verkuil]

On 03/11/2017 07:20 PM, Steve Longerbeam wrote:
> 
> 
> On 03/11/2017 05:49 AM, Sakari Ailus wrote:
>> Hi Steve,
>>
>> On Thu, Mar 09, 2017 at 08:53:01PM -0800, Steve Longerbeam wrote:
>>> Add an empty UAPI Kbuild file for media UAPI headers.
>>>
>>> Signed-off-by: Steve Longerbeam 
>>
>> The existing V4L2 UAPI headers are under include/uapi/linux. Could you use
>> that directory instead?
>>
>> I actually wouldn't really object doing this but it should have been done in
>> 2002 or so when the first V4L2 header was added. Now the benefit is
>> questionable.
>>
> 
> Agreed, I think the current headers should be moved to uapi/media
> eventually, but for now I'll go ahead and put under uapi/linux.

No, while in staging it shouldn't be exported.

Put it in include/linux and move it to uapi when this driver is mainlined.

I don't think we can move headers from uapi/linux to uapi/media, I'm sure it's
too late for that.

Regards,

Hans

Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue

[Wu, Songjun]

On 3/13/2017 17:25, Hans Verkuil wrote:

On 03/13/2017 06:53 AM, Wu, Songjun wrote:



On 3/9/2017 18:57, Hans Verkuil wrote:

Hi Songjun,

On 08/03/17 03:25, Wu, Songjun wrote:

Hi Colin,

Thank you for your comment.
It is a bug, will be fixed in the next patch.


Do you mean that you will provide a new patch for this? Is there anything
wrong with this patch? It seems reasonable to me.


Hi Hans,

I see this patch is merged in git://linuxtv.org/media_tree.git.
So I do not need submit isc-pipeline-v3 patch, just submit the patches,
based on the current master branch?


Huh? Where do you see that this patch is merged? I don't see it in the 
media_tree master
branch.


Hi Hans,

I see this patch on the master branch in media_tree.
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/atmel/atmel-isc.c


Regards,

Hans




Regards,

Hans



On 3/7/2017 22:30, Colin King wrote:

From: Colin Ian King 

The are only HIST_ENTRIES worth of entries in  hist_entry however the
for-loop is iterating one too many times leasing to a read access off
the end off the array ctrls->hist_entry.  Fix this by iterating by
the correct number of times.

Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")

Signed-off-by: Colin Ian King 
---
 drivers/media/platform/atmel/atmel-isc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index b380a7d..7dacf8c 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
 regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);

 *hist_count = 0;
-for (i = 0; i <= HIST_ENTRIES; i++)
+for (i = 0; i < HIST_ENTRIES; i++)
 *hist_count += i * (*hist_entry++);
 }

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Mon, Mar 13, 2017 at 08:16:25AM +, Russell King - ARM Linux wrote:
> On Sun, Mar 12, 2017 at 09:26:41PM -0700, Steve Longerbeam wrote:
> > On 03/12/2017 01:22 PM, Russell King - ARM Linux wrote:
> > >What I had was this patch for your v3.  I never got to testing your
> > >v4 because of the LP-11 problem.
> > >
> > >In v5, you've changed to propagate the ipu_cpmem_set_image() error
> > >code to avoid the resulting corruption, but that leaves the other bits
> > >of this patch unaddressed, along my "media: imx: smfc: add support
> > >for bayer formats" patch.
> > >
> > >Your driver basically has no support for bayer formats.
> > 
> > You added the patches to this driver that adds the bayer support,
> > I don't think there is anything more required of the driver at this
> > point to support bayer, the remaining work needs to happen in the IPUv3
> > driver.
> 
> There is more work, because the way you've merged my changes to
> imx_smfc_setup_channel() into csi_idmac_setup_channel() is wrong with
> respect to the burst size.
> 
> You always set it to 8 or 16 depending on the width:
> 
>   burst_size = (image.pix.width & 0xf) ? 8 : 16;
> 
>   ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size);
> 
> and then you have my switch() statement which assigns burst_size.
> My _tested_ code removed the above, added the switch, which had
> a default case which reflected the above setting:
> 
>   default:
>   burst_size = (outfmt->width & 0xf) ? 8 : 16;
> 
> and then went on to set the burst size _after_ the switch statement:
> 
>   ipu_cpmem_set_burstsize(priv->smfc_ch, burst_size);
> 
> The effect is unchanged for non-bayer formats.  For bayer formats, the
> burst size is determined by the bayer data size.
> 
> So, even if it's appropriate to fix ipu_cpmem_set_image(), fixing the
> above is still required.
> 
> I'm not convinced that fixing ipu_cpmem_set_image() is even the best
> solution - it's not as trivial as it looks on the surface:
> 
> ipu_cpmem_set_resolution(ch, image->rect.width, image->rect.height);
> ipu_cpmem_set_stride(ch, pix->bytesperline);
> 
> this is fine, it doesn't depend on the format.  However, the next line:
> 
> ipu_cpmem_set_fmt(ch, v4l2_pix_fmt_to_drm_fourcc(pix->pixelformat));
> 
> does - v4l2_pix_fmt_to_drm_fourcc() is a locally defined function (it
> isn't v4l2 code) that converts a v4l2 pixel format to a DRM fourcc.
> DRM knows nothing about bayer formats, there aren't fourcc codes in
> DRM for it.  The result is that v4l2_pix_fmt_to_drm_fourcc() returns
> -EINVAL cast to a u32, which gets passed unchecked into ipu_cpmem_set_fmt().
> 
> ipu_cpmem_set_fmt() won't recognise that, and also returns -EINVAL - and
> it's a bug that this is not checked and propagated.  If it is checked and
> propagated, then we need this to support bayer formats, and I don't see
> DRM people wanting bayer format fourcc codes added without there being
> a real DRM driver wanting to use them.
> 
> Then there's the business of calculating the top-left offset of the image,
> which for bayer always needs to be an even number of pixels - as this
> function takes the top-left offset, it ought to respect it, but if it
> doesn't meet this criteria, what should it do?  csi_idmac_setup_channel()
> always sets them to zero, but that's not really something that
> ipu_cpmem_set_image() should assume.

For the time being, I've restored the functionality along the same lines
as I originally had.  This seems to get me working capture, but might
break non-bayer passthrough mode:

diff --git a/drivers/staging/media/imx/imx-media-csi.c 
b/drivers/staging/media/imx/imx-media-csi.c
index fc0036aa84d0..df336971a009 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -314,14 +314,6 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
image.phys0 = phys[0];
image.phys1 = phys[1];
 
-   ret = ipu_cpmem_set_image(priv->idmac_ch, );
-   if (ret)
-   return ret;
-
-   burst_size = (image.pix.width & 0xf) ? 8 : 16;
-
-   ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size);
-
/*
 * Check for conditions that require the IPU to handle the
 * data internally as generic data, aka passthrough mode:
@@ -346,15 +338,29 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
passthrough_bits = 16;
break;
default:
+   burst_size = (image.pix.width & 0xf) ? 8 : 16;
passthrough = (sensor_ep->bus_type != V4L2_MBUS_CSI2 &&
   sensor_ep->bus.parallel.bus_width >= 16);
passthrough_bits = 16;
break;
}
 
-   if (passthrough)
+   if (passthrough) {
+   ipu_cpmem_set_resolution(priv->idmac_ch, image.rect.width,
+image.rect.height);
+   

Re: [PATCH] v4l: soc-camera: Remove videobuf1 support

[Guennadi Liakhovetski]

On Mon, 13 Mar 2017, Hans Verkuil wrote:

> On 03/12/2017 01:06 PM, Guennadi Liakhovetski wrote:
> > Hi Laurent,
> > 
> > Thanks for the patch. I just checked in the current media/master, there
> > are still 2 users of vb1: sh_mobile_ceu_camera.c and atmel-isi.c. I
> > understand, that they are about to be removed either completely or out of
> > soc-camera, maybe patches for that have already beed submitted, but they
> > haven't been committed yet. Shall we wait until then with this patch?
> > Would be easier to handle dependencies, there isn't any hurry with it,
> > right?
> 
> 
> 
> Both drivers use vb2.

Uhm, ok, I stand corrected. I just grepped for symbols, that get deleted 
by the patch and my grep was too generous.

Thanks
Guennadi

> I've already added this patch to a pull request of mine.
> 
> Regards,
> 
>   Hans
> 

Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue

[Hans Verkuil]

On 03/13/2017 06:53 AM, Wu, Songjun wrote:
> 
> 
> On 3/9/2017 18:57, Hans Verkuil wrote:
>> Hi Songjun,
>>
>> On 08/03/17 03:25, Wu, Songjun wrote:
>>> Hi Colin,
>>>
>>> Thank you for your comment.
>>> It is a bug, will be fixed in the next patch.
>>
>> Do you mean that you will provide a new patch for this? Is there anything
>> wrong with this patch? It seems reasonable to me.
>>
> Hi Hans,
> 
> I see this patch is merged in git://linuxtv.org/media_tree.git.
> So I do not need submit isc-pipeline-v3 patch, just submit the patches, 
> based on the current master branch?

Huh? Where do you see that this patch is merged? I don't see it in the 
media_tree master
branch.

Regards,

Hans

> 
>> Regards,
>>
>>  Hans
>>
>>>
>>> On 3/7/2017 22:30, Colin King wrote:
 From: Colin Ian King 

 The are only HIST_ENTRIES worth of entries in  hist_entry however the
 for-loop is iterating one too many times leasing to a read access off
 the end off the array ctrls->hist_entry.  Fix this by iterating by
 the correct number of times.

 Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")

 Signed-off-by: Colin Ian King 
 ---
  drivers/media/platform/atmel/atmel-isc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/drivers/media/platform/atmel/atmel-isc.c 
 b/drivers/media/platform/atmel/atmel-isc.c
 index b380a7d..7dacf8c 100644
 --- a/drivers/media/platform/atmel/atmel-isc.c
 +++ b/drivers/media/platform/atmel/atmel-isc.c
 @@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
  regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);

  *hist_count = 0;
 -for (i = 0; i <= HIST_ENTRIES; i++)
 +for (i = 0; i < HIST_ENTRIES; i++)
  *hist_count += i * (*hist_entry++);
  }


>>

Re: [PATCH] v4l: soc-camera: Remove videobuf1 support

[Hans Verkuil]

On 03/12/2017 01:06 PM, Guennadi Liakhovetski wrote:

Hi Laurent,

Thanks for the patch. I just checked in the current media/master, there
are still 2 users of vb1: sh_mobile_ceu_camera.c and atmel-isi.c. I
understand, that they are about to be removed either completely or out of
soc-camera, maybe patches for that have already beed submitted, but they
haven't been committed yet. Shall we wait until then with this patch?
Would be easier to handle dependencies, there isn't any hurry with it,
right?




Both drivers use vb2.

I've already added this patch to a pull request of mine.

Regards,

Hans

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 12, 2017 at 09:26:41PM -0700, Steve Longerbeam wrote:
> On 03/12/2017 01:22 PM, Russell King - ARM Linux wrote:
> >What I had was this patch for your v3.  I never got to testing your
> >v4 because of the LP-11 problem.
> >
> >In v5, you've changed to propagate the ipu_cpmem_set_image() error
> >code to avoid the resulting corruption, but that leaves the other bits
> >of this patch unaddressed, along my "media: imx: smfc: add support
> >for bayer formats" patch.
> >
> >Your driver basically has no support for bayer formats.
> 
> You added the patches to this driver that adds the bayer support,
> I don't think there is anything more required of the driver at this
> point to support bayer, the remaining work needs to happen in the IPUv3
> driver.

There is more work, because the way you've merged my changes to
imx_smfc_setup_channel() into csi_idmac_setup_channel() is wrong with
respect to the burst size.

You always set it to 8 or 16 depending on the width:

burst_size = (image.pix.width & 0xf) ? 8 : 16;

ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size);

and then you have my switch() statement which assigns burst_size.
My _tested_ code removed the above, added the switch, which had
a default case which reflected the above setting:

default:
burst_size = (outfmt->width & 0xf) ? 8 : 16;

and then went on to set the burst size _after_ the switch statement:

ipu_cpmem_set_burstsize(priv->smfc_ch, burst_size);

The effect is unchanged for non-bayer formats.  For bayer formats, the
burst size is determined by the bayer data size.

So, even if it's appropriate to fix ipu_cpmem_set_image(), fixing the
above is still required.

I'm not convinced that fixing ipu_cpmem_set_image() is even the best
solution - it's not as trivial as it looks on the surface:

ipu_cpmem_set_resolution(ch, image->rect.width, image->rect.height);
ipu_cpmem_set_stride(ch, pix->bytesperline);

this is fine, it doesn't depend on the format.  However, the next line:

ipu_cpmem_set_fmt(ch, v4l2_pix_fmt_to_drm_fourcc(pix->pixelformat));

does - v4l2_pix_fmt_to_drm_fourcc() is a locally defined function (it
isn't v4l2 code) that converts a v4l2 pixel format to a DRM fourcc.
DRM knows nothing about bayer formats, there aren't fourcc codes in
DRM for it.  The result is that v4l2_pix_fmt_to_drm_fourcc() returns
-EINVAL cast to a u32, which gets passed unchecked into ipu_cpmem_set_fmt().

ipu_cpmem_set_fmt() won't recognise that, and also returns -EINVAL - and
it's a bug that this is not checked and propagated.  If it is checked and
propagated, then we need this to support bayer formats, and I don't see
DRM people wanting bayer format fourcc codes added without there being
a real DRM driver wanting to use them.

Then there's the business of calculating the top-left offset of the image,
which for bayer always needs to be an even number of pixels - as this
function takes the top-left offset, it ought to respect it, but if it
doesn't meet this criteria, what should it do?  csi_idmac_setup_channel()
always sets them to zero, but that's not really something that
ipu_cpmem_set_image() should assume.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 12/23] MAINTAINERS: Add file patterns for media device tree bindings

[Geert Uytterhoeven]

Hi Mauro,

On Mon, Mar 13, 2017 at 2:02 AM, Mauro Carvalho Chehab
 wrote:
> Em Sun, 12 Mar 2017 14:16:56 +0100
> Geert Uytterhoeven  escreveu:
>
>> Submitters of device tree binding documentation may forget to CC
>> the subsystem maintainer if this is missing.
>>
>> Signed-off-by: Geert Uytterhoeven 
>> Reviewed-by: Javier Martinez Canillas 
>> Cc: Mauro Carvalho Chehab 
>
> As the To: is devicetree, I'm assuming that this patch will be
> applied there, so:
>
> Acked-by: Mauro Carvalho Chehab 

Thanks!

> I may also merge via my tree, if that would be better. Just let me
> know in such case.

Please apply to your tree, cfr.

| Please apply this patch directly if you want to be involved in device
| tree binding documentation for your subsystem.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds