Re: noveau vs arm dma ops

[Russell King - ARM Linux]

(While there's a rain shower...)

On Thu, Apr 26, 2018 at 02:09:42AM -0700, Christoph Hellwig wrote:
> synopsis:
> 
> drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c:pdevinfo.dma_mask 
>   = DMA_BIT_MASK(32);
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c:  pdevinfo.dma_mask = 
> DMA_BIT_MASK(32);

This is for the AHB audio driver, and is a correct default on two counts:

1. It is historically usual to initialise DMA masks to 32-bit, and leave
   it to the device drivers to negotiate via the DMA mask functions if
   they wish to use higher orders of bits.

2. The AHB audio hardware in the HDMI block only supports 32-bit addresses.

What I've missed from the AHB audio driver is calling the DMA mask
functions... oops.  Patch below.

> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c:  pdevinfo.dma_mask = 
> DMA_BIT_MASK(32);

This is for the I2S audio driver, and I suspect is wrong - I doubt
that the I2S sub-device itself does any DMA what so ever.

8<===
From: Russell King 
Subject: drm: bridge: dw-hdmi: Negotiate dma mask with DMA API

DMA drivers are supposed to negotiate the DMA mask with the DMA API,
but this was missing from the AHB audio driver.  Add the necessary
call.

Signed-off-by: Russell King 
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c 
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
index cf3f0caf9c63..16c45b6cd6af 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
@@ -539,6 +539,10 @@ static int snd_dw_hdmi_probe(struct platform_device *pdev)
unsigned revision;
int ret;
 
+   ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
+   if (ret)
+   return ret;
+
writeb_relaxed(HDMI_IH_MUTE_AHBDMAAUD_STAT0_ALL,
   data->base + HDMI_IH_MUTE_AHBDMAAUD_STAT0);
revision = readb_relaxed(data->base + HDMI_REVISION_ID);


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: noveau vs arm dma ops

[Russell King - ARM Linux]

On Wed, Apr 25, 2018 at 11:35:13PM +0200, Daniel Vetter wrote:
> On arm that doesn't work. The iommu api seems like a good fit, except
> the dma-api tends to get in the way a bit (drm/msm apparently has
> similar problems like tegra), and if you need contiguous memory
> dma_alloc_coherent is the only way to get at contiguous memory. There
> was a huge discussion years ago about that, and direct cma access was
> shot down because it would have exposed too much of the caching
> attribute mangling required (most arm platforms need wc-pages to not
> be in the kernel's linear map apparently).

I think you completely misunderstand ARM from what you've written above,
and this worries me greatly about giving DRM the level of control that
is being asked for.

Modern ARMs have a PIPT cache or a non-aliasing VIPT cache, and cache
attributes are stored in the page tables.  These caches are inherently
non-aliasing when there are multiple mappings (which is a great step
forward compared to the previous aliasing caches.)

As the cache attributes are stored in the page tables, this in theory
allows different virtual mappings of the same physical memory to have
different cache attributes.  However, there's a problem, and that's
called speculative prefetching.

Let's say you have one mapping which is cacheable, and another that is
marked as write combining.  If a cache line is speculatively prefetched
through the cacheable mapping of this memory, and then you read the
same physical location through the write combining mapping, it is
possible that you could read cached data.

So, it is generally accepted that all mappings of any particular
physical bit of memory should have the same cache attributes to avoid
unpredictable behaviour.

This presents a problem with what is generally called "lowmem" where
the memory is mapped in kernel virtual space with cacheable
attributes.  It can also happen with highmem if the memory is
kmapped.

This is why, on ARM, you can't use something like get_free_pages() to
grab some pages from the system, pass it to the GPU, map it into
userspace as write-combining, etc.  It _might_ work for some CPUs,
but ARM CPUs vary in how much prefetching they do, and what may work
for one particular CPU is in no way guaranteed to work for another
ARM CPU.

The official line from architecture folk is to assume that the caches
infinitely speculate, are of infinite size, and can writeback *dirty*
data at any moment.

The way to stop things like speculative prefetches to particular
physical memory is to, quite "simply", not have any cacheable
mappings of that physical memory anywhere in the system.

Now, cache flushes on ARM tend to be fairly expensive for GPU buffers.
If you have, say, an 8MB buffer (for a 1080p frame) and you need to
do a cache operation on that buffer, you'll be iterating over it
32 or maybe 64 bytes at a time "just in case" there's a cache line
present.  Referring to my previous email, where I detailed the
potential need for _two_ flushes, one before the GPU operation and
one after, and this becomes _really_ expensive.  At that point, you're
probably way better off using write-combine memory where you don't
need to spend CPU cycles performing cache flushing - potentially
across all CPUs in the system if cache operations aren't broadcasted.

This isn't a simple matter of "just provide some APIs for cache
operations" - there's much more that needs to be understood by
all parties here, especially when we have GPU drivers that can be
used with quite different CPUs.

It may well be that for some combinations of CPUs and workloads, it's
better to use write-combine memory without cache flushing, but for
other CPUs that tradeoff (for the same workload) could well be
different.

Older ARMs get more interesting, because they have aliasing caches.
That means the CPU cache aliases across different virtual space
mappings in some way, which complicates (a) the mapping of memory
and (b) handling the cache operations on it.

It's too late for me to go into that tonight, and I probably won't
be reading mail for the next week and a half, sorry.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: noveau vs arm dma ops

[Russell King - ARM Linux]

On Wed, Apr 25, 2018 at 08:33:12AM -0700, Christoph Hellwig wrote:
> On Wed, Apr 25, 2018 at 12:04:29PM +0200, Daniel Vetter wrote:
> > - dma api hides the cache flushing requirements from us. GPUs love
> >   non-snooped access, and worse give userspace control over that. We want
> >   a strict separation between mapping stuff and flushing stuff. With the
> >   IOMMU api we mostly have the former, but for the later arch maintainers
> >   regularly tells they won't allow that. So we have drm_clflush.c.
> 
> The problem is that a cache flushing API entirely separate is hard. That
> being said if you look at my generic dma-noncoherent API series it tries
> to move that way.  So far it is in early stages and apparently rather
> buggy unfortunately.

And if folk want a cacheable mapping with explicit cache flushing, the
cache flushing must not be defined in terms of "this is what CPU seems
to need" but from the point of view of a CPU with infinite prefetching,
infinite caching and infinite capacity to perform writebacks of dirty
cache lines at unexpected moments when the memory is mapped in a
cacheable mapping.

(The reason for that is you're operating in a non-CPU specific space,
so you can't make any guarantees as to how much caching or prefetching
will occur by the CPU - different CPUs will do different amounts.)

So, for example, the sequence:

GPU writes to memory
CPU reads from cacheable memory

if the memory was previously dirty (iow, CPU has written), you need to
flush the dirty cache lines _before_ the GPU writes happen, but you
don't know whether the CPU has speculatively prefetched, so you need
to flush any prefetched cache lines before reading from the cacheable
memory _after_ the GPU has finished writing.

Also note that "flush" there can be "clean the cache", "clean and
invalidate the cache" or "invalidate the cache" as appropriate - some
CPUs are able to perform those three operations, and the appropriate
one depends on not only where in the above sequence it's being used,
but also on what the operations are.

So, the above sequence could be:

CPU invalidates cache for memory
(due to possible dirty cache lines)
GPU writes to memory
CPU invalidates cache for memory
(to get rid of any speculatively prefetched
 lines)
CPU reads from cacheable memory

Yes, in the above case, _two_ cache operations are required to ensure
correct behaviour.  However, if you know for certain that the memory was
previously clean, then the first cache operation can be skipped.

What I'm pointing out is there's much more than just "I want to flush
the cache" here, which is currently what DRM seems to assume at the
moment with the code in drm_cache.c.

If we can agree a set of interfaces that allows _proper_ use of these
facilities, one which can be used appropriately, then there shouldn't
be a problem.  The DMA API does that via it's ideas about who owns a
particular buffer (because of the above problem) and that's something
which would need to be carried over to such a cache flushing API (it
should be pretty obvious that having a GPU read or write memory while
the cache for that memory is being cleaned will lead to unexpected
results.)

Also note that things get even more interesting in a SMP environment
if cache operations aren't broadcasted across the SMP cluster (which
means cache operations have to be IPI'd to other CPUs.)

The next issue, which I've brought up before, is that exposing cache
flushing to userspace on architectures where it isn't already exposed
comes.  As has been shown by Google Project Zero, this risks exposing
those architectures to Spectre and Meltdown exploits where they weren't
at such a risk before.  (I've pretty much shown here that you _do_
need to control which cache lines get flushed to make these exploits
work, and flushing the cache by reading lots of data in liu of having
the ability to explicitly flush bits of cache makes it very difficult
to impossible for them to work.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: noveau vs arm dma ops

[Russell King - ARM Linux]

On Wed, Apr 25, 2018 at 01:54:39AM -0700, Christoph Hellwig wrote:
> [discussion about this patch, which should have been cced to the iommu
>  and linux-arm-kernel lists, but wasn't:
>  https://www.spinics.net/lists/dri-devel/msg173630.html]
> 
> On Wed, Apr 25, 2018 at 09:41:51AM +0200, Thierry Reding wrote:
> > > API from the iommu/dma-mapping code.  Drivers have no business poking
> > > into these details.
> > 
> > The interfaces that the above patch uses are all EXPORT_SYMBOL_GPL,
> > which is rather misleading if they are not meant to be used by drivers
> > directly.

EXPORT_SYMBOL* means nothing as far as whether a driver should
be able to use the symbol or not - it merely means that the symbol is
made available to a module.  Modules cover much more than just device
drivers - we have library modules, filesystem modules, helper modules
to name a few non-driver classes of modules.

We also have symbols that are exported as part of the architecture
implementation detail of a public interface.  For example, the
public interface "copy_from_user" is implemented as an inline
function (actually several layers of inline functions) eventually
calling into arm_copy_from_user().  arm_copy_from_user() is exported,
but drivers (in fact no module) is allowed to make direct reference
to arm_copy_from_user() - it'd fail when software PAN is enabled.

The whole idea that "if a symbol is exported, it's fine for a driver
to use it" is a complete work of fiction, always has been, and always
will be.

We've had this with the architecture implementation details of the
DMA API before, and with the architecture implementation details of
the CPU cache flushing.  There's only so much commentry, or __
prefixes you can add to a symbol before things get rediculous, and
none of it stops people creating this abuse.  The only thing that
seems to prevent it is to make life hard for folk wanting to use
the symbols (eg, hiding the symbol prototype in a private header,
etc.)

Never, ever go under the covers of an interface.  If the interface
doesn't do what you want, _discuss_ it, don't just think "oh, that
architecture private facility looks like what I need, I'll use that
directly."

If you ever are on the side of trying to maintain those implementation
details that are abused in this way, you'll soon understand why this
behaviour by driver authors is soo annoying, and the maintainability
problems it creates.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v3 0/7] TDA998x CEC support

[Russell King - ARM Linux]

On Tue, Apr 24, 2018 at 11:29:42AM +0200, Hans Verkuil wrote:
> On 04/09/18 14:15, Russell King - ARM Linux wrote:
> > Hi,
> > 
> > This patch series adds CEC support to the DRM TDA998x driver.  The
> > TDA998x family of devices integrate a TDA9950 CEC at a separate I2C
> > address from the HDMI encoder.
> > 
> > Implementation of the CEC part is separate to allow independent CEC
> > implementations, or independent HDMI implementations (since the
> > TDA9950 may be a separate device.)
> 
> Reviewed, looks good.

Thanks!  If we need to rework the calibration GPIO stuff for BB, we can
do that in a later patch.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v3 5/7] drm/i2c: tda9950: add CEC driver

[Russell King - ARM Linux]

On Fri, Apr 20, 2018 at 05:48:12PM +0200, Hans Verkuil wrote:
> On 04/20/2018 05:31 PM, Russell King - ARM Linux wrote:
> > Hi Hans,
> > 
> > Any comments?
> 
> I have been traveling and haven't had time to look at this. Next week will
> be busy as well, but I expect to be able to look at it the week after that.

Well, that doesn't work because I won't be reading mail that week,
and I'll probably simply ignore the excessive backlog when I do
start reading mail again.

> I remember from the previous series that I couldn't test it with my BeagleBone
> Black board (the calibration gpio had to switch from in to out but it wasn't 
> allowed
> since it had an associated irq). That's still a problem?
> 
> I didn't see any changes in that area when I did a quick scan.

Correct, and unless you wish me to do the work for you (in which case
you can pay me) nothing is going to change on that front!  Seriously,
please do not expect me to add support for platforms I don't have
access to.  I'm just a volunteer for this, probably the same as you.

I don't think we ended up with an answer for that problem.  I don't
see that dropping the requested interrupt, using the GPIO, and then
re-requesting the interrupt is an option - how do we handle a failure
to re-request the interrupt?  Do we just ignore the error, or let DRM
stop working properly?

In any case, I don't have a working HDMI CEC-compliant setup anymore,
(no TV, just a HDMI monitor now) so I would rather _not_ change the
driver from its known-to-be-working state.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v3 5/7] drm/i2c: tda9950: add CEC driver

[Russell King - ARM Linux]

Hi Hans,

Any comments?

Thanks.

On Mon, Apr 09, 2018 at 01:16:32PM +0100, Russell King wrote:
> Add a CEC driver for the TDA9950, which is a stand-alone I2C CEC device,
> but is also integrated into HDMI transceivers such as the TDA9989 and
> TDA19989.
> 
> The TDA9950 contains a command processor which handles retransmissions
> and the low level bus protocol.  The driver just has to read and write
> the messages, and handle error conditions.
> 
> Signed-off-by: Russell King 
> ---
>  drivers/gpu/drm/i2c/Kconfig   |   5 +
>  drivers/gpu/drm/i2c/Makefile  |   1 +
>  drivers/gpu/drm/i2c/tda9950.c | 509 
> ++
>  include/linux/platform_data/tda9950.h |  16 ++
>  4 files changed, 531 insertions(+)
>  create mode 100644 drivers/gpu/drm/i2c/tda9950.c
>  create mode 100644 include/linux/platform_data/tda9950.h
> 
> diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
> index a6c92beb410a..3a232f5ff0a1 100644
> --- a/drivers/gpu/drm/i2c/Kconfig
> +++ b/drivers/gpu/drm/i2c/Kconfig
> @@ -26,4 +26,9 @@ config DRM_I2C_NXP_TDA998X
>   help
> Support for NXP Semiconductors TDA998X HDMI encoders.
>  
> +config DRM_I2C_NXP_TDA9950
> + tristate "NXP Semiconductors TDA9950/TDA998X HDMI CEC"
> + select CEC_NOTIFIER
> + select CEC_CORE
> +
>  endmenu
> diff --git a/drivers/gpu/drm/i2c/Makefile b/drivers/gpu/drm/i2c/Makefile
> index b20100c18ffb..a962f6f08568 100644
> --- a/drivers/gpu/drm/i2c/Makefile
> +++ b/drivers/gpu/drm/i2c/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_DRM_I2C_SIL164) += sil164.o
>  
>  tda998x-y := tda998x_drv.o
>  obj-$(CONFIG_DRM_I2C_NXP_TDA998X) += tda998x.o
> +obj-$(CONFIG_DRM_I2C_NXP_TDA9950) += tda9950.o
> diff --git a/drivers/gpu/drm/i2c/tda9950.c b/drivers/gpu/drm/i2c/tda9950.c
> new file mode 100644
> index ..3f7396caad48
> --- /dev/null
> +++ b/drivers/gpu/drm/i2c/tda9950.c
> @@ -0,0 +1,509 @@
> +/*
> + *  TDA9950 Consumer Electronics Control driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * The NXP TDA9950 implements the HDMI Consumer Electronics Control
> + * interface.  The host interface is similar to a mailbox: the data
> + * registers starting at REG_CDR0 are written to send a command to the
> + * internal CPU, and replies are read from these registers.
> + *
> + * As the data registers represent a mailbox, they must be accessed
> + * as a single I2C transaction.  See the TDA9950 data sheet for details.
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +enum {
> + REG_CSR = 0x00,
> + CSR_BUSY = BIT(7),
> + CSR_INT  = BIT(6),
> + CSR_ERR  = BIT(5),
> +
> + REG_CER = 0x01,
> +
> + REG_CVR = 0x02,
> +
> + REG_CCR = 0x03,
> + CCR_RESET = BIT(7),
> + CCR_ON= BIT(6),
> +
> + REG_ACKH = 0x04,
> + REG_ACKL = 0x05,
> +
> + REG_CCONR = 0x06,
> + CCONR_ENABLE_ERROR = BIT(4),
> + CCONR_RETRY_MASK = 7,
> +
> + REG_CDR0 = 0x07,
> +
> + CDR1_REQ = 0x00,
> + CDR1_CNF = 0x01,
> + CDR1_IND = 0x81,
> + CDR1_ERR = 0x82,
> + CDR1_IER = 0x83,
> +
> + CDR2_CNF_SUCCESS= 0x00,
> + CDR2_CNF_OFF_STATE  = 0x80,
> + CDR2_CNF_BAD_REQ= 0x81,
> + CDR2_CNF_CEC_ACCESS = 0x82,
> + CDR2_CNF_ARB_ERROR  = 0x83,
> + CDR2_CNF_BAD_TIMING = 0x84,
> + CDR2_CNF_NACK_ADDR  = 0x85,
> + CDR2_CNF_NACK_DATA  = 0x86,
> +};
> +
> +struct tda9950_priv {
> + struct i2c_client *client;
> + struct device *hdmi;
> + struct cec_adapter *adap;
> + struct tda9950_glue *glue;
> + u16 addresses;
> + struct cec_msg rx_msg;
> + struct cec_notifier *notify;
> + bool open;
> +};
> +
> +static int tda9950_write_range(struct i2c_client *client, u8 addr, u8 *p, 
> int cnt)
> +{
> + struct i2c_msg msg;
> + u8 buf[cnt + 1];
> + int ret;
> +
> + buf[0] = addr;
> + memcpy(buf + 1, p, cnt);
> +
> + msg.addr = client->addr;
> + msg.flags = 0;
> + msg.len = cnt + 1;
> + msg.buf = buf;
> +
> + dev_dbg(>dev, "wr 0x%02x: %*ph\n", addr, cnt, p);
> +
> + ret = i2c_transfer(client->adapter, , 1);
> + if (ret < 0)
> + dev_err(>dev, "Error %d writing to cec:0x%x\n", ret, 
> addr);
> + return ret < 0 ? ret : 0;
> +}
> +
> +static void tda9950_write(struct i2c_client *client, u8 addr, u8 val)
> +{
> + tda9950_write_range(client, addr, , 1);
> +}
> +
> +static int tda9950_read_range(struct i2c_client *client, u8 addr, u8 *p, int 
> cnt)
> +{
> + struct i2c_msg msg[2];
> + int ret;
> +
> + msg[0].addr = client->addr;
> + msg[0].flags = 0;
> + msg[0].len = 1;
> + msg[0].buf = 
> + msg[1].addr = client->addr;
> + msg[1].flags = I2C_M_RD;
> + 

[PATCH v3 0/7] TDA998x CEC support

[Russell King - ARM Linux]

Hi,

This patch series adds CEC support to the DRM TDA998x driver.  The
TDA998x family of devices integrate a TDA9950 CEC at a separate I2C
address from the HDMI encoder.

Implementation of the CEC part is separate to allow independent CEC
implementations, or independent HDMI implementations (since the
TDA9950 may be a separate device.)

 .../devicetree/bindings/display/bridge/tda998x.txt |   3 +
 drivers/gpu/drm/i2c/Kconfig|   6 +
 drivers/gpu/drm/i2c/Makefile   |   1 +
 drivers/gpu/drm/i2c/tda9950.c  | 509 +
 drivers/gpu/drm/i2c/tda998x_drv.c  | 242 --
 include/linux/platform_data/tda9950.h  |  16 +
 6 files changed, 750 insertions(+), 27 deletions(-)
 create mode 100644 drivers/gpu/drm/i2c/tda9950.c
 create mode 100644 include/linux/platform_data/tda9950.h

v3: addressed most of Hans comments in v2
v2: updated DT property.
 
-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v2 00/19] prevent bounds-check bypass via speculative execution

[Russell King - ARM Linux]

Do you think that the appropriate patches could be copied to the
appropriate people please?

On Thu, Jan 11, 2018 at 04:46:24PM -0800, Dan Williams wrote:
> Changes since v1 [1]:
> * fixup the ifence definition to use alternative_2 per recent AMD
>   changes in tip/x86/pti (Tom)
> 
> * drop 'nospec_ptr' (Linus, Mark)
> 
> * rename 'nospec_array_ptr' to 'array_ptr' (Alexei)
> 
> * rename 'nospec_barrier' to 'ifence' (Peter, Ingo)
> 
> * clean up occasions of 'variable assignment in if()' (Sergei, Stephen)
> 
> * make 'array_ptr' use a mask instead of an architectural ifence by
>   default (Linus, Alexei)
> 
> * provide a command line and compile-time opt-in to the ifence
>   mechanism, if an architecture provides 'ifence_array_ptr'.
> 
> * provide an optimized mask generation helper, 'array_ptr_mask', for
>   x86 (Linus)
> 
> * move 'get_user' hardening from '__range_not_ok' to '__uaccess_begin'
>   (Linus)
> 
> * drop "Thermal/int340x: prevent bounds-check..." since userspace does
>   not have arbitrary control over the 'trip' index (Srinivas)
> 
> * update the changelog of "net: mpls: prevent bounds-check..." and keep
>   it in the series to continue the debate about Spectre hygiene patches.
>   (Eric).
> 
> * record a reviewed-by from Laurent on "[media] uvcvideo: prevent
>   bounds-check..."
> 
> * update the cover letter
> 
> [1]: https://lwn.net/Articles/743376/
> 
> ---
> 
> Quoting Mark's original RFC:
> 
> "Recently, Google Project Zero discovered several classes of attack
> against speculative execution. One of these, known as variant-1, allows
> explicit bounds checks to be bypassed under speculation, providing an
> arbitrary read gadget. Further details can be found on the GPZ blog [2]
> and the Documentation patch in this series."
> 
> This series incorporates Mark Rutland's latest ARM changes and adds
> the x86 specific implementation of 'ifence_array_ptr'. That ifence
> based approach is provided as an opt-in fallback, but the default
> mitigation, '__array_ptr', uses a 'mask' approach that removes
> conditional branches instructions, and otherwise aims to redirect
> speculation to use a NULL pointer rather than a user controlled value.
> 
> The mask is generated by the following from Alexei, and Linus:
> 
> mask = ~(long)(_i | (_s - 1 - _i)) >> (BITS_PER_LONG - 1);
> 
> ...and Linus provided an optimized mask generation helper for x86:
> 
> asm ("cmpq %1,%2; sbbq %0,%0;"
>   :"=r" (mask)
>   :"r"(sz),"r" (idx)
>   :"cc");
> 
> The 'array_ptr' mechanism can be switched between 'mask' and 'ifence'
> via the spectre_v1={mask,ifence} command line option, and the
> compile-time default is set by selecting either CONFIG_SPECTRE1_MASK or
> CONFIG_SPECTRE1_IFENCE.
> 
> The 'array_ptr' infrastructure is the primary focus this patch set. The
> individual patches that perform 'array_ptr' conversions are a point in
> time (i.e. earlier kernel, early analysis tooling, x86 only etc...)
> start at finding some of these gadgets.
> 
> Another consideration for reviewing these patches is the 'hygiene'
> argument. When a patch refers to hygiene it is concerned with stopping
> speculation on an unconstrained or insufficiently constrained pointer
> value under userspace control. That by itself is not sufficient for
> attack (per current understanding) [3], but it is a necessary
> pre-condition.  So 'hygiene' refers to cleaning up those suspect
> pointers regardless of whether they are usable as a gadget.
> 
> These patches are also be available via the 'nospec-v2' git branch
> here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux nospec-v2
> 
> Note that the BPF fix for Spectre variant1 is merged in the bpf.git
> tree [4], and is not included in this branch.
> 
> [2]: 
> https://googleprojectzero.blogspot.co.uk/2018/01/reading-privileged-memory-with-side.html
> [3]: https://spectreattack.com/spectre.pdf
> [4]: 
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=b2157399cc98
> 
> ---
> 
> Dan Williams (16):
>   x86: implement ifence()
>   x86: implement ifence_array_ptr() and array_ptr_mask()
>   asm-generic/barrier: mask speculative execution flows
>   x86: introduce __uaccess_begin_nospec and ASM_IFENCE
>   x86: use __uaccess_begin_nospec and ASM_IFENCE in get_user paths
>   ipv6: prevent bounds-check bypass via speculative execution
>   ipv4: prevent bounds-check bypass via speculative execution
>   vfs, fdtable: prevent bounds-check bypass via speculative execution
>   userns: prevent bounds-check bypass via speculative execution
>   udf: prevent bounds-check bypass via speculative execution
>   [media] uvcvideo: prevent bounds-check bypass via speculative execution
>   carl9170: prevent bounds-check bypass via speculative execution
>   p54: prevent bounds-check bypass via speculative execution
>   qla2xxx: prevent bounds-check bypass via speculative execution
>   

Re: [PATCH 1/3] drm: bridge: synopsys/dw-hdmi: Enable cec clock

[Russell King - ARM Linux]

On Sat, Oct 14, 2017 at 12:53:35AM +0200, Pierre-Hugues Husson wrote:
> @@ -2382,6 +2383,18 @@ __dw_hdmi_probe(struct platform_device *pdev,
>   goto err_isfr;
>   }
>  
> + hdmi->cec_clk = devm_clk_get(hdmi->dev, "cec");
> + if (IS_ERR(hdmi->cec_clk)) {
> + hdmi->cec_clk = NULL;

What if devm_clk_get() returns EPROBE_DEFER?  Does that really mean the
clock does not exist?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH][MEDIA]i.MX6 CSI: Fix MIPI camera operation in RAW/Bayer mode

[Russell King - ARM Linux]

On Tue, Oct 17, 2017 at 03:26:19PM +0200, Krzysztof Hałasa wrote:
> Fabio Estevam  writes:
> 
> >> --- a/drivers/staging/media/imx/imx-media-csi.c
> >> +++ b/drivers/staging/media/imx/imx-media-csi.c
> >> @@ -340,7 +340,7 @@ static int csi_idmac_setup_channel(struct csi_priv 
> >> *priv)
> >> case V4L2_PIX_FMT_SGBRG8:
> >> case V4L2_PIX_FMT_SGRBG8:
> >> case V4L2_PIX_FMT_SRGGB8:
> >> -   burst_size = 8;
> >> +   burst_size = 16;
> >> passthrough = true;
> >> passthrough_bits = 8;
> >> break;
> >
> > Russell has sent the same fix and Philipp made a comment about the
> > possibility of using 32-byte or 64-byte bursts:
> > http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2017-October/111960.html
> 
> Great.
> 
> Sometimes I wonder how many people are working on exactly the same
> stuff.

I do wish the patch was merged (which fixes a real problem) rather than
hanging around for optimisation questions.  We can always increase it
in the future if it's deemed that a larger burst size is beneficial.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH] media: staging/imx: do not return error in link_notify for unknown sources

[Russell King - ARM Linux]

On Thu, Oct 12, 2017 at 12:06:33AM +0100, Russell King - ARM Linux wrote:
> Now, if you mean "known" to be equivalent with "recognised by
> imx-media" then, as I've pointed out several times already, that
> statement is FALSE.  I'm not sure how many times I'm going to have to
> state this fact.  Let me re-iterate again.  On my imx219 driver, I
> have two subdevs.  Both subdevs have controls attached.  The pixel
> subdev is not "recognised" by imx-media, and without a modification
> like my or your patch, it fails.  However, with the modification,
> this "unrecognised" subdev _STILL_ has it's controls visible through
> imx-media.

If you refuse to believe what I'm saying, then read through:

http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=csi-v6=e3f847cd37b007d55b76282414bfcf13abb8fc9a

and look at this:

# for f in 2 3 4 5 6 7 8 9; do v4l2-ctl -L -d /dev/video$f; done
# ./cam/cam-setup.sh
# v4l2-ctl -L -d /dev/video6

User Controls

   gain (int): min=256 max=4095 step=1 default=256 
value=256
 fim_enable (bool)   : default=0 value=0
fim_num_average (int): min=1 max=64 step=1 default=8 
value=8  fim_tolerance_min (int): min=2 max=200 step=1 
default=50 value=50
  fim_tolerance_max (int): min=0 max=500 step=1 default=0 
value=0
   fim_num_skip (int): min=0 max=256 step=1 default=2 
value=2
 fim_input_capture_edge (int): min=0 max=3 step=1 default=0 value=0
  fim_input_capture_channel (int): min=0 max=1 step=1 default=0 value=0

Camera Controls

 exposure_time_absolute (int): min=1 max=399 step=1 default=399 
value=166

Image Source Controls

  vertical_blanking (int): min=32 max=64814 step=1 default=2509
value=2509 flags=read-only
horizontal_blanking (int): min=2168 max=31472 step=1 
default=2168 value=2168 flags=read-only
  analogue_gain (int): min=1000 max=8000 step=1 
default=1000 value=1000
red_pixel_value (int): min=0 max=1023 step=1 default=0 
value=0 flags=inactive
  green_red_pixel_value (int): min=0 max=1023 step=1 default=0 
value=0 flags=inactive
   blue_pixel_value (int): min=0 max=1023 step=1 default=0 
value=0 flags=inactive
 green_blue_pixel_value (int): min=0 max=1023 step=1 default=0 
value=0 flags=inactive

Image Processing Controls

 pixel_rate (int64)  : min=16000 max=27840 step=1 
default=27840 value=27840 flags=read-only
   test_pattern (menu)   : min=0 max=9 default=0 value=0
0: Disabled
1: Solid Color
2: 100% Color Bars
3: Fade to Grey Color Bar
4: PN9
5: 16 Split Color Bar
6: 16 Split Inverted Color Bar
7: Column Counter
8: Inverted Column Counter
9: PN31

Now, the pixel array subdev has these controls:
gain
vertical_blanking
horizontal_blanking
analogue_gain
pixel_rate
exposure_time_absolute

The root subdev (which is the one which connects to your code) has
these controls:
red_pixel_value
green_red_pixel_value
blue_pixel_value
green_blue_pixel_value
test_pattern

As you can see from the above output, _all_ controls from _both_ subdevs
are listed.

Now, before you spot your old patch adding v4l2_pipeline_inherit_controls()
and try to blame that for this, nothing in my kernel calls that function,
so that patch can be dropped, and so it's not responsible for this.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH] media: staging/imx: do not return error in link_notify for unknown sources

[Russell King - ARM Linux]

On Wed, Oct 11, 2017 at 03:14:26PM -0700, Steve Longerbeam wrote:
> 
> 
> On 10/11/2017 02:49 PM, Russell King - ARM Linux wrote:
> >On Tue, Oct 03, 2017 at 12:09:13PM -0700, Steve Longerbeam wrote:
> >>imx_media_link_notify() should not return error if the source subdevice
> >>is not recognized by imx-media, that isn't an error. If the subdev has
> >>controls they will be inherited starting from a known subdev.
> >What does "a known subdev" mean?
> 
> It refers to the previous sentence, "not recognized by imx-media". A
> subdev that was not registered via async registration and so not in
> imx-media's async subdev list. I could elaborate in the commit message
> but it seems fairly obvious to me.

I don't think it's obvious, and I suspect you won't think it's obvious
in years to come (I talk from experience of some commentry I've added
in the past.)

Now, isn't it true that for a subdev to be part of a media device, it
has to be registered, and if it's part of a media device that is made
up of lots of different drivers, it has to use the async registration
code?  So, is it not also true that any subdev that is part of a
media device, it will be "known"?

Under what circumstances could a subdev be part of a media device but
not be "known" ?

Now, if you mean "known" to be equivalent with "recognised by
imx-media" then, as I've pointed out several times already, that
statement is FALSE.  I'm not sure how many times I'm going to have to
state this fact.  Let me re-iterate again.  On my imx219 driver, I
have two subdevs.  Both subdevs have controls attached.  The pixel
subdev is not "recognised" by imx-media, and without a modification
like my or your patch, it fails.  However, with the modification,
this "unrecognised" subdev _STILL_ has it's controls visible through
imx-media.

Hence, I believe your comment in the code and your commit message
are misleading and wrong.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH] media: staging/imx: do not return error in link_notify for unknown sources

[Russell King - ARM Linux]

On Tue, Oct 03, 2017 at 12:09:13PM -0700, Steve Longerbeam wrote:
> imx_media_link_notify() should not return error if the source subdevice
> is not recognized by imx-media, that isn't an error. If the subdev has
> controls they will be inherited starting from a known subdev.

What does "a known subdev" mean?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH RFC] media: staging/imx: fix complete handler

[Russell King - ARM Linux]

On Mon, Oct 02, 2017 at 05:59:30PM -0700, Steve Longerbeam wrote:
> 
> 
> On 10/01/2017 04:36 PM, Russell King - ARM Linux wrote:
> >On Sun, Oct 01, 2017 at 01:16:53PM -0700, Steve Longerbeam wrote:
> >>Right, imx_media_add_vdev_to_pa() has followed a link to an
> >>entity that imx is not aware of.
> >>
> >>The only effect of this patch (besides allowing the driver to load
> >>with smiapp cameras), is that no controls from the unknown entity
> >>will be inherited to the capture device nodes. That's not a big deal
> >>since the controls presumably can still be accessed from the subdev
> >>node.
> >smiapp is just one example I used to illustrate the problem.  The imx
> >media implementation must not claim subdevs exclusively for itself if
> >it's going to be useful, it has to cater for subdevs existing for
> >other hardware attached to it.
> >
> >As you know, the camera that interests me is my IMX219 camera, and it's
> >regressed with your driver because of your insistence that you have sole
> >ownership over subdevs in the imx media stack
> 
> If by "sole ownership", you mean expecting async registration of subdevs
> and setting up the media graph between them, imx-media will only do that
> if those devices and the connections between them are described in the
> device tree. If they are not, i.e. the subdevs and media pads and links are
> created internally by the driver, then imx-media doesn't interfere with
> that.

By "sole ownership" I mean that _at the moment_ imx-media believes
that it has sole right to make use of all subdevs with the exception
of one external subdev, and expects every subdev to have an imx media
subdev structure associated with it.

That's clearly true, because as soon as a multi-subdev device is
attempted to be connected to imx-media, imx-media falls apart because
it's unable to find its private imx media subdev structure for the
additional subdevs.

> >  - I'm having to carry more
> >and more hacks to workaround things that end up broken.  The imx-media
> >stack needs to start playing better with the needs of others, which it
> >can only do by allowing subdevs to be used by others.
> 
> Well, for example imx-media will chain s_stream until reaches your
> IMX219 driver. It's then up to your driver to pass s_stream to the
> subdevs that it owns.

Of course it is.  It's your responsibility to pass appropriate stuff
down the chain as far as you know how to, which is basically up to
the first external subdev facing imx-media.  What happens beyond there
is up to the external drivers.

> >   One way to
> >achieve that change that results in something that works is the patch
> >that I've posted - and tested.
> 
>  Can you change the error message to be more descriptive, something
> like "any controls for unknown subdev %s will not be inherited\n" and maybe
> convert to a warn. After that I will ack it.

No, that's plainly untrue as I said below:

> >It also results in all controls (which are spread over the IMX219's two
> >subdevs) to be visible via the v4l2 video interface - I have all the
> >controls attached to the pixel array subdev as well as the controls
> >attached to the scaling subdev.

Given that I said this, and I can prove that it does happen, I've no
idea why your reply seemed to totally ignore this paragraph.

So I refuse to add a warning message that is incorrect.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH 1/1] v4l: async: Fix notifier complete callback error handling

[Russell King - ARM Linux]

On Mon, Oct 02, 2017 at 02:28:46PM +0300, Sakari Ailus wrote:
> On Mon, Oct 02, 2017 at 01:59:54PM +0300, Sakari Ailus wrote:
> > The notifier complete callback may return an error. This error code was
> > simply returned to the caller but never handled properly.
> > 
> > Move calling the complete callback function to the caller from
> > v4l2_async_test_notify and undo the work that was done either in async
> > sub-device or async notifier registration.
> > 
> > Reported-by: Russell King 
> > Signed-off-by: Sakari Ailus 
> 
> Oh, I forgot to metion this patch depends on another patch here, part of
> the fwnode parsing patchset:
> 
> 

Any chance of sending me that patch so I can test this patch?  I'd
rather not manually de-html-ise the above patch.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH RFC] media: staging/imx: fix complete handler

[Russell King - ARM Linux]

On Sun, Oct 01, 2017 at 01:16:53PM -0700, Steve Longerbeam wrote:
> Right, imx_media_add_vdev_to_pa() has followed a link to an
> entity that imx is not aware of.
> 
> The only effect of this patch (besides allowing the driver to load
> with smiapp cameras), is that no controls from the unknown entity
> will be inherited to the capture device nodes. That's not a big deal
> since the controls presumably can still be accessed from the subdev
> node.

smiapp is just one example I used to illustrate the problem.  The imx
media implementation must not claim subdevs exclusively for itself if
it's going to be useful, it has to cater for subdevs existing for
other hardware attached to it.

As you know, the camera that interests me is my IMX219 camera, and it's
regressed with your driver because of your insistence that you have sole
ownership over subdevs in the imx media stack - I'm having to carry more
and more hacks to workaround things that end up broken.  The imx-media
stack needs to start playing better with the needs of others, which it
can only do by allowing subdevs to be used by others.  One way to
achieve that change that results in something that works is the patch
that I've posted - and tested.

The reason that the IMX219 driver users subdevs is because it's capable
of image cropping and binning on the camera module - which helps greatly
if you want to achieve higher FPS for high speed capture [*].

It also results in all controls (which are spread over the IMX219's two
subdevs) to be visible via the v4l2 video interface - I have all the
controls attached to the pixel array subdev as well as the controls
attached to the scaling subdev.

> However, I still have some concerns about supporting smiapp cameras
> in imx-media driver, and that is regarding pad indexes. The smiapp device
> that exposes a source pad to the "outside world", which is either the binner
> or the scaler entity, has a pad index of 1. But unless the device tree port
> for the smiapp device is given a reg value of 1 for that port, imx-media
> will assume it is pad 0, not 1.

For IMX219, the source pad on the scaler (which is connected to the CSI
input pad) is pad 0 - always has been.  So I guess this problem is hidden
because of that choice.  Maybe that's a problem for someone who has a
SMIAPP camera to address.

Right now, my patch stack to get the imx219 on v4.14-rc1 working is:

media: staging/imx: fix complete handler
[media] v4l: async: don't bomb out on ->complete failure
media: imx-csi: fix burst size
media: imx: debug power on
ARM: dts: imx6qdl-hummingboard: add IMX219 camera
media: i2c: imx219 camera driver
media: imx: add frame intervals back in
fix lp-11 timeout

The frame interval patch is there because I just don't agree with the
position of the v4l2 folk, and keeping it means I don't have to screw
up my camera configuration scripts with special handling.  The
"fix lp-11 timeout" changes the LP-11 timeout to be a warning rather
than a failure - and contary to what FSL/NXP say, it works every time
on the iMX6 devices without needing to go through their workaround.


* - This is the whole reason I bought the IMX219, and have written the
IMX219 driver.  I want to use it for high speed capture of an arrow
leaving a recurve bow.  Why?  Everyone archer shoots subtly differently,
and I want to see what's happening to the arrows that are leaving my
bow.  However, for that to be achievable, I (a) need a working capture
implementation for imx6, and (b) I need to be able to quickly convert
bayer to an image, and (c) I need to either encode it on the fly, or
write the raw images to SSD.

(a) is thwarted by the breakage I keep stumbling over with the capture
code.

(b) I have using the GC320 GPU and a gstreamer plugin, trivially
converting the bayer data to grayscale.

(c) I've yet to achieve - encoding may be supported by the CODA v4l
driver, but nothing in userspace appears to support it, there's no
gstreamer v4l plugin for encoding, only one for decoding.  I also
suspect at either the 16G I have free on the SSD will get eaten up
rapidly without encoding, or the SSD may not keep up with the data
rate.

Right now, all my testing is around displaying on X:

DISPLAY=:0 gst-launch-1.0 -v v4l2src device=/dev/video9 io-mode=4 ! bayer2rgbgc 
! clockoverlay halignment=2 valignment=1 ! xvimagesink

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH v3 0/2] [media] videobuf2-dc: Add support for cacheable MMAP

[Russell King - ARM Linux]

On Thu, Jul 13, 2017 at 01:13:28PM +0200, Marek Szyprowski wrote:
> Hi Christoph,
> 
> On 2017-07-05 19:33, Christoph Hellwig wrote:
> >On Mon, Jul 03, 2017 at 11:27:32AM +0200, Marek Szyprowski wrote:
> >>The main question here if we want to merge incomplete solution or not. As
> >>for now, there is no support in ARM/ARM64 for NON_CONSISTENT attribute.
> >>Also none of the v4l2 drivers use it. Sadly support for NON_CONSISTENT
> >>attribute is not fully implemented nor even defined in mainline.
> >>
> >DMA_ATTR_NON_CONSISTENT is the way to get the dma_alloc_noncoherent
> >semantics through the dma_alloc_attr API, and as such I think it is
> >pretty well defined, although the documentation in
> >Documentation/DMA-attributes.txt is really bad and we need to improve
> >it, by merging it with the dma_alloc_noncoherent description in
> >Documentation/DMA-API.txt. My series to remove dma_alloc_noncoherent
> >updates the latter to mention DMA_ATTR_NON_CONSISTENT, but
> >we should probably merge Documentation/DMA-API.txt,
> >Documentation/DMA-attributes.txt and Documentation/DMA-API-HOWTO.txt
> >into a single coherent document.
> 
> Right. I started conversion of dma_alloc_noncoherent to NON_CONSISTENT
> DMA attribute, but later I got stuck at the details of cache
> synchronization.
> 
> >>I know that it works fine for some vendor kernel trees, but supporting it in
> >>mainline was a bit controversial. There is no proper way to sync cache for
> >>such
> >>buffers. Calling dma_sync_sg worked so far, but it has to be first agreed as
> >>a proper DMA API.
> >As documented in Documentation/DMA-API.txt the proper way to sync
> >noncoherent/nonconsistent regions is to call dma_cache_sync.  It seems
> >like it generally is the same as dma_sync_range/sg so if we could
> >eventually merge these APIs that should reduce the confusion further.
> 
> Original dma_alloc_noncoherent utilized dma_cache_sync() function, which had
> some flaws, which prevented me to continue that task and introduce it to ARM
> architecture. The dma_alloc_noncoherent() and dma_cache_sync() API lacks
> buffer ownership and imprecisely defines how and when the caches has to be
> synchronized. dma_cache_sync() also lacks DMA address argument, what also
> complicates potential lightweight implementation.

My conclusion of the dma_alloc_noncoherent() and dma_cache_sync() API
when it was introduced is that it's basically a completely broken
interface, and I've never seen any point to it.  Maybe some of that is
because it's badly documented - which in turn makes it badly designed
(because there's no specification detailing what it's supposed to be
doing.)

I'd like to see that thing die...

-- 
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] autogain support for bayer10 format (was Re: [patch] propagating controls in libv4l2)

[Russell King - ARM Linux]

On Wed, Apr 26, 2017 at 06:43:54PM +0300, Ivaylo Dimitrov wrote:
> >+static int get_luminosity_bayer10(uint16_t *buf, const struct v4l2_format 
> >*fmt)
> >+{
> >+long long avg_lum = 0;
> >+int x, y;
> >+
> >+buf += fmt->fmt.pix.height * fmt->fmt.pix.bytesperline / 4 +
> >+fmt->fmt.pix.width / 4;
> >+
> >+for (y = 0; y < fmt->fmt.pix.height / 2; y++) {
> >+for (x = 0; x < fmt->fmt.pix.width / 2; x++)
> 
> That would take some time :). AIUI, we have NEON support in ARM kernels
> (CONFIG_KERNEL_MODE_NEON), I wonder if it makes sense (me) to convert the
> above loop to NEON-optimized when it comes to it? Are there any drawbacks in
> using NEON code in kernel?

Using neon without the VFP state saved and restored corrupts userspace's
FP state.  So, you have to save the entire VFP state to use neon in kernel
mode.  There are helper functions for this: kernel_neon_begin() and
kernel_neon_end().

You can't build C code with the compiler believing that neon is available
as the compiler could emit neon instructions in unprotected kernel code.

Note that kernel_neon_begin() is only allowed to be called outside
interrupt context and with preemption disabled.

Given that, do we really want to be walking over multi-megabytes of image
data in the kernel with preemption disabled - it sounds like a recipe for
a very sluggish system.  I think this should (and can only sensibly be
done) in 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.

Re: [PATCH] arm: dma: fix sharing of coherent DMA memory without struct page

[Russell King - ARM Linux]

On Sun, Apr 16, 2017 at 07:10:21PM -0600, Shuah Khan wrote:
> On 04/14/2017 03:46 AM, Russell King - ARM Linux wrote:
> > On Fri, Apr 14, 2017 at 09:56:07AM +0200, Marek Szyprowski wrote:
> >>>> This would be however quite large task, especially taking into account
> >>>> all current users of DMA-buf framework...
> >>> Yeah it will be a large task.
> >>
> >> Maybe once scatterlist are switched to pfns, changing dmabuf internal
> >> memory representation to pfn array might be much easier.
> > 
> > Switching to a PFN array won't work either as we have no cross-arch
> > way to translate PFNs to a DMA address and vice versa.  Yes, we have
> > them in ARM, but they are an _implementation detail_ of ARM's
> > DMA API support, they are not for use by drivers.
> > 
> > So, the very first problem that needs solving is this:
> > 
> >   How do we go from a coherent DMA allocation for device X to a set
> >   of DMA addresses for device Y.
> > 
> > Essentially, we need a way of remapping the DMA buffer for use with
> > another device, and returning a DMA address suitable for that device.
> > This could well mean that we need to deal with setting up an IOMMU
> > mapping.  My guess is that this needs to happen at the DMA coherent
> > API level - the DMA coherent API needs to be augmented with support
> > for this.  I'll call this "DMA coherent remap".
> > 
> > We then need to think about how to pass this through the dma-buf API.
> > dma_map_sg() is done by the exporter, who should know what kind of
> > memory is being exported.  The exporter can avoid calling dma_map_sg()
> > if it knows in advance that it is exporting DMA coherent memory.
> > Instead, the exporter can simply create a scatterlist with the DMA
> > address and DMA length prepopulated with the results of the DMA
> > coherent remap operation above.
> 
> The only way to conclusively say that it is coming from coherent area
> is at the time it is getting allocated in dma_alloc_from_coherent().
> Since dma_alloc_attrs() will go on to find memory from other areas if
> dma_alloc_from_coherent() doesn't allocate memory.

Sorry, I disagree.

The only thing that matters is "did this memory come from
dma_alloc_coherent()".  It doesn't matter where dma_alloc_coherent()
ultimately got the memory from, it's memory from the coherent allocator
interface, and it should not be passed back into the streaming APIs.
It is, after all, DMA _coherent_ memory, passing it into the streaming
APIs which is for DMA _noncoherent_ memory is insane - the streaming APIs
can bring extra expensive cache flushes, which are not required for
DMA _coherent_ memory.

The exporter should know where it got the memory from.  It's really not
sane for anyone except the _original_ allocator to be exporting memory
through a DMA buffer - only the original allocator knows the properties
of that memory, and how to map it, whether that be for DMA, kmap or
mmap.

If a dmabuf is imported into a driver and then re-exported, the original
dmabuf should be what is re-exported, not some creation of the driver -
the re-exporting driver can't know what the properties of the memory
backing the dmabuf are, so anything else is just insane.

> How about adding get_sgtable, map_sg, unmap_sg to dma_buf_ops. The right
> ops need to be installed based on buffer type. As I mentioned before, we
> don't know which memory we got until dma_alloc_from_coherent() finds
> memory in dev->mem area. So how about using the dma_check_dev_coherent()
> to determine which ops we need. These could be set based on buffer type.
> vb2_dc_get_dmabuf() can do that.

Given my statement above, I don't believe any of that is necessary.  All
memory allocated from dma_alloc_coherent() is DMA coherent.  So, if
memory was obtained from dma_alloc_coherent() or similar, then it must
not be passed to the streaming DMA API.  It doesn't matter where it
ultimately came from.

-- 
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] arm: dma: fix sharing of coherent DMA memory without struct page

[Russell King - ARM Linux]

On Fri, Apr 14, 2017 at 09:56:07AM +0200, Marek Szyprowski wrote:
> >>This would be however quite large task, especially taking into account
> >>all current users of DMA-buf framework...
> >Yeah it will be a large task.
> 
> Maybe once scatterlist are switched to pfns, changing dmabuf internal
> memory representation to pfn array might be much easier.

Switching to a PFN array won't work either as we have no cross-arch
way to translate PFNs to a DMA address and vice versa.  Yes, we have
them in ARM, but they are an _implementation detail_ of ARM's
DMA API support, they are not for use by drivers.

So, the very first problem that needs solving is this:

  How do we go from a coherent DMA allocation for device X to a set
  of DMA addresses for device Y.

Essentially, we need a way of remapping the DMA buffer for use with
another device, and returning a DMA address suitable for that device.
This could well mean that we need to deal with setting up an IOMMU
mapping.  My guess is that this needs to happen at the DMA coherent
API level - the DMA coherent API needs to be augmented with support
for this.  I'll call this "DMA coherent remap".

We then need to think about how to pass this through the dma-buf API.
dma_map_sg() is done by the exporter, who should know what kind of
memory is being exported.  The exporter can avoid calling dma_map_sg()
if it knows in advance that it is exporting DMA coherent memory.
Instead, the exporter can simply create a scatterlist with the DMA
address and DMA length prepopulated with the results of the DMA
coherent remap operation above.

What the scatterlist can't carry in this case is a set of valid
struct page pointers, and an importer must not walk the scatterlist
expecting to get at the virtual address parameters or struct page
pointers.

On the mmap() side of things, remember that DMA coherent allocations
may require special mapping into userspace, and which can only be
mapped by the DMA coherent mmap support.  kmap etc will also need to
be different.  So it probably makes sense for DMA coherent dma-buf
exports to use a completely separate set of dma_buf_ops from the
streaming version.

I think this is the easiest approach to solving the problem without
needing massive driver changes all over the kernel.

-- 
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] [media] imx: csi: retain current field order and colorimetry setting as default

[Russell King - ARM Linux]

On Thu, Apr 06, 2017 at 05:01:52PM +0200, Philipp Zabel wrote:
> On Thu, 2017-04-06 at 15:05 +0100, Russell King - ARM Linux wrote:
> > On Thu, Apr 06, 2017 at 03:55:29PM +0200, Philipp Zabel wrote:
> > > +
> > > + /* Retain current field setting as default */
> > > + if (sdformat->format.field == V4L2_FIELD_ANY)
> > > + sdformat->format.field = fmt->field;
> > > +
> > > + /* Retain current colorspace setting as default */
> > > + if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) {
> > > + sdformat->format.colorspace = fmt->colorspace;
> > > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT)
> > > + sdformat->format.xfer_func = fmt->xfer_func;
> > > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT)
> > > + sdformat->format.ycbcr_enc = fmt->ycbcr_enc;
> > > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT)
> > > + sdformat->format.quantization = fmt->quantization;
> > > + } else {
> > > + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) {
> > > + sdformat->format.xfer_func =
> > > + V4L2_MAP_XFER_FUNC_DEFAULT(
> > > + sdformat->format.colorspace);
> > > + }
> > > + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) {
> > > + sdformat->format.ycbcr_enc =
> > > + V4L2_MAP_YCBCR_ENC_DEFAULT(
> > > + sdformat->format.colorspace);
> > > + }
> > > + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) 
> > > {
> > > + sdformat->format.quantization =
> > > + V4L2_MAP_QUANTIZATION_DEFAULT(
> > > + cc->cs != IPUV3_COLORSPACE_YUV,
> > > + sdformat->format.colorspace,
> > > + sdformat->format.ycbcr_enc);
> > > + }
> > > + }
> > 
> > Would it make sense for this to be a helper function?
> 
> Quite possible, the next subdev that has to set frame_interval on both
> pads manually because its upstream source pad doesn't suport
> frame_interval might want to do the same.

Hmm.  I'm not sure I agree with this approach.  If a subdev hardware
does not support any modification of the colourspace or field, then
it should not be modifyable at the source pad - it should retain the
propagated settings from the sink pad.

I thought I had already sent a patch doing exactly that.

-- 
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] [media] imx: csi: retain current field order and colorimetry setting as default

[Russell King - ARM Linux]

On Thu, Apr 06, 2017 at 04:20:21PM +0200, Hans Verkuil wrote:
> On 04/06/2017 03:55 PM, Philipp Zabel wrote:
> > If the the field order is set to ANY in set_fmt, choose the currently
> > set field order. If the colorspace is set to DEFAULT, choose the current
> > colorspace.  If any of xfer_func, ycbcr_enc or quantization are set to
> > DEFAULT, either choose the current setting, or the default setting for the
> > new colorspace, if non-DEFAULT colorspace was given.
> > 
> > This allows to let field order and colorimetry settings be propagated
> > from upstream by calling media-ctl on the upstream entity source pad,
> > and then call media-ctl on the sink pad to manually set the input frame
> > interval, without changing the already set field order and colorimetry
> > information.
> > 
> > Signed-off-by: Philipp Zabel 
> > ---
> > This is based on imx-media-staging-md-v14, and it is supposed to allow
> > configuring the pipeline with media-ctl like this:
> > 
> > 1) media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY8_1X16/1920x1080]"
> > 2) media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY8_1X16/1920x108]"
> > 3) media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY8_1X16/1920x1080]"
> > 4) media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/60]"
> > 5) media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/1920x1080@1/30]"
> > 
> > Without having step 4) overwrite the colorspace and field order set on
> > 'ipu1_csi0':0 by the propagation in step 3).
> > ---
> >  drivers/staging/media/imx/imx-media-csi.c | 34 
> > +++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/drivers/staging/media/imx/imx-media-csi.c 
> > b/drivers/staging/media/imx/imx-media-csi.c
> > index 64dc454f6b371..d94ce1de2bf05 100644
> > --- a/drivers/staging/media/imx/imx-media-csi.c
> > +++ b/drivers/staging/media/imx/imx-media-csi.c
> > @@ -1325,6 +1325,40 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
> > csi_try_fmt(priv, sensor, cfg, sdformat, crop, compose, );
> >  
> > fmt = __csi_get_fmt(priv, cfg, sdformat->pad, sdformat->which);
> > +
> > +   /* Retain current field setting as default */
> > +   if (sdformat->format.field == V4L2_FIELD_ANY)
> > +   sdformat->format.field = fmt->field;
> 
> sdformat->format.field should never be FIELD_ANY. If it is, then that's a
> subdev bug and I'm pretty sure FIELD_NONE was intended.

Please explain further - sdformat->format.field is the field passed _in_
from userspace, and from what I can see, userspace is free to pass in
any value through the ioctl as check_format() does nothing to validate
that the various format.* fields are sane.

This patch is detecting that the user is requesting FIELD_ANY, and
fixing it up.  Surely that's the right thing to do, and is way more
preferable than just accepting the FIELD_ANY from userspace and
storing that value?

-- 
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] [media] imx: csi: retain current field order and colorimetry setting as default

[Russell King - ARM Linux]

On Thu, Apr 06, 2017 at 03:55:29PM +0200, Philipp Zabel wrote:
> +
> + /* Retain current field setting as default */
> + if (sdformat->format.field == V4L2_FIELD_ANY)
> + sdformat->format.field = fmt->field;
> +
> + /* Retain current colorspace setting as default */
> + if (sdformat->format.colorspace == V4L2_COLORSPACE_DEFAULT) {
> + sdformat->format.colorspace = fmt->colorspace;
> + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT)
> + sdformat->format.xfer_func = fmt->xfer_func;
> + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT)
> + sdformat->format.ycbcr_enc = fmt->ycbcr_enc;
> + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT)
> + sdformat->format.quantization = fmt->quantization;
> + } else {
> + if (sdformat->format.xfer_func == V4L2_XFER_FUNC_DEFAULT) {
> + sdformat->format.xfer_func =
> + V4L2_MAP_XFER_FUNC_DEFAULT(
> + sdformat->format.colorspace);
> + }
> + if (sdformat->format.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) {
> + sdformat->format.ycbcr_enc =
> + V4L2_MAP_YCBCR_ENC_DEFAULT(
> + sdformat->format.colorspace);
> + }
> + if (sdformat->format.quantization == V4L2_QUANTIZATION_DEFAULT) 
> {
> + sdformat->format.quantization =
> + V4L2_MAP_QUANTIZATION_DEFAULT(
> + cc->cs != IPUV3_COLORSPACE_YUV,
> + sdformat->format.colorspace,
> + sdformat->format.ycbcr_enc);
> + }
> + }

Would it make sense for this to be a helper function?

-- 
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] arm: dma: fix sharing of coherent DMA memory without struct page

[Russell King - ARM Linux]

On Wed, Apr 05, 2017 at 10:02:42AM -0600, Shuah Khan wrote:
> When coherent DMA memory without struct page is shared, importer
> fails to find the page and runs into kernel page fault when it
> tries to dmabuf_ops_attach/map_sg/map_page the invalid page found
> in the sg_table. Please see www.spinics.net/lists/stable/msg164204.html
> for more information on this problem.
> 
> This solution allows coherent DMA memory without struct page to be
> shared by providing a way for the exporter to tag the DMA buffer as
> a special buffer without struct page association and passing the
> information in sg_table to the importer. This information is used
> in attach/map_sg to avoid cleaning D-cache and mapping.
> 
> The details of the change are:
> 
> Framework:
> - Add a new dma_attrs field to struct scatterlist.
> - Add a new DMA_ATTR_DEV_COHERENT_NOPAGE attribute to clearly identify
>   Coherent memory without struct page.
> - Add a new dma_check_dev_coherent() interface to check if memory is
>   the device coherent area. There is no way to tell where the memory
>   returned by dma_alloc_attrs() came from.
> 
> Exporter logic:
> - Add logic to vb2_dc_alloc() to call dma_check_dev_coherent() and set
>   DMA_ATTR_DEV_COHERENT_NOPAGE based the results of the check. This is
>   done in the exporter context.
> - Add logic to arm_dma_get_sgtable() to identify memory without struct
>   page using DMA_ATTR_DEV_COHERENT_NOPAGE attribute. If this attr is
>   set, arm_dma_get_sgtable() will set page as the cpu_addr and update
>   dma_address and dma_attrs fields in struct scatterlist for this sgl.
>   This is done in exporter context when buffer is exported. With this

This sentence appears to just end...

I'm not convinced that coherent allocations should be setting the "page"
of a scatterlist to anything that isn't a real struct page or NULL.  It
is, after all, an error to look up the virtual address etc of the
scatterlist entry or kmap it when it isn't backed by a struct page.

I'm actually already passing non-page backed memory through the DMA API
in armada-drm, although not entirely correctly, and etnaviv handles it
fine:

} else if (dobj->linear) {
/* Single contiguous physical region - no struct page */
if (sg_alloc_table(sgt, 1, GFP_KERNEL))
goto free_sgt;
sg_dma_address(sgt->sgl) = dobj->dev_addr;
sg_dma_len(sgt->sgl) = dobj->obj.size;

This is not quite correct, as it assumes (which is safe for it currently)
that the DMA address is the same on all devices.  On Dove, which is where
this is used, that is the case, but it's not true elsewhere.  Also note
that I'm avoid calling dma_map_sg() and dma_unmap_sg() - there's no iommus
to be considered.

I'd suggest that this follows the same pattern - setting the DMA address
(more appropriately for generic code) and the DMA length, while leaving
the virtual address members NULL/0.  However, there's also the
complication of setting up any IOMMUs that would be necessary.  I haven't
looked at that, or how it could work.

I also think this should be documented in the dmabuf API that it can
pass such scatterlists that are DMA-parameter only.

Lastly, I'd recommend that anything using this does _not_ provide
functional kmap/kmap_atomic support for these - kmap and kmap_atomic
are both out because there's no struct page anyway (and their use would
probably oops the kernel in this scenario.)  I avoided mmap support in
armada drm, but if there's a pressing reason and real use case for the
importer to mmap() the buffers in userspace, it's something I could be
convinced of.

What I'm quite certain of is that we do _not_ want to be passing
coherent memory allocations into the streaming DMA API, not even with
a special attribute.  The DMA API is about gaining coherent memory
(shared ownership of the buffer), or mapping system memory to a
specified device (which can imply unique ownership.)  Trying to mix
the two together muddies the separation that we have there, and makes
it harder to explain.  As can be seen from this patch, we'd end up
needing to add this special DMA_ATTR_DEV_COHERENT_NOPAGE everywhere,
which is added complexity on top of stuff that is not required for
this circumstance.

I can see why you're doing it, to avoid having to duplicate more of
the generic code in drm_prime, but I don't think plasting over this
problem in arch code by adding this special flag is a particularly
good way forward.

-- 
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] [media] imx: assume MEDIA_ENT_F_ATV_DECODER entities output video on pad 1

[Russell King - ARM Linux]

On Tue, Apr 04, 2017 at 05:44:05PM -0700, Steve Longerbeam wrote:
> 
> 
> On 04/04/2017 05:40 PM, Steve Longerbeam wrote:
> >
> >
> >On 04/04/2017 04:10 PM, Russell King - ARM Linux wrote:
> >>On Thu, Mar 30, 2017 at 07:25:49PM +0200, Philipp Zabel wrote:
> >>>The TVP5150 DT bindings specify a single output port (port 0) that
> >>>corresponds to the video output pad (pad 1, DEMOD_PAD_VID_OUT).
> >>>
> >>>Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
> >>>---
> >>>I'm trying to get this to work with a TVP5150 analog TV decoder, and the
> >>>first problem is that this device doesn't have pad 0 as its single
> >>>output pad. Instead, as a MEDIA_ENT_F_ATV_DECODER entity, it has for
> >>>pads (input, video out, vbi out, audio out), and video out is pad 1,
> >>>whereas the device tree only defines a single port (0).
> >>
> >>Looking at the patch, it's highlighted another review point with
> >>Steve's driver.
> >>
> >>>diff --git a/drivers/staging/media/imx/imx-media-dev.c
> >>>b/drivers/staging/media/imx/imx-media-dev.c
> >>>index 17e2386a3ca3a..c52d6ca797965 100644
> >>>--- a/drivers/staging/media/imx/imx-media-dev.c
> >>>+++ b/drivers/staging/media/imx/imx-media-dev.c
> >>>@@ -267,6 +267,15 @@ static int imx_media_create_link(struct
> >>>imx_media_dev *imxmd,
> >>> source_pad = link->local_pad;
> >>> sink_pad = link->remote_pad;
> >>>
> >>>+/*
> >>>+ * If the source subdev is an analog video decoder with a single
> >>>source
> >>>+ * port, assume that this port 0 corresponds to the
> >>>DEMOD_PAD_VID_OUT
> >>>+ * entity pad.
> >>>+ */
> >>>+if (source->entity.function == MEDIA_ENT_F_ATV_DECODER &&
> >>>+local_sd->num_sink_pads == 0 && local_sd->num_src_pads == 1)
> >>>+source_pad = DEMOD_PAD_VID_OUT;
> >>>+
> >>> v4l2_info(>v4l2_dev, "%s: %s:%d -> %s:%d\n", __func__,
> >>>   source->name, source_pad, sink->name, sink_pad);
> >>
> >>What is "local" and what is "remote" here?  It seems that, in the case of
> >>a link being created with the sensor(etc), it's all back to front.
> >>
> >>Eg, I see locally:
> >>
> >>imx-media: imx_media_create_link: imx219 0-0010:0 -> imx6-mipi-csi2:0
> >>
> >>So here, "source" is the imx219 (the sensor), and sink is
> >>"imx6-mipi-csi2"
> >>(part of the iMX6 capture.)  However, this makes "local_sd" the subdev of
> >>the sensor, and "remote_sd" the subdev of the CSI2 interface - which is
> >>totally back to front - this code is part of the iMX6 capture system,
> >>so "local" implies that it should be part of that, and the "remote" thing
> >>would be the sensor.
> >>
> >>Hence, this seems completely confused.  I'd suggest that:
> >>
> >>(a) the "pad->pad.flags & MEDIA_PAD_FL_SINK" test in
> >>imx_media_create_link()
> >>is moved into imx_media_create_links(), and placed here instead:
> >>
> >>for (j = 0; j < num_pads; j++) {
> >>pad = _sd->pad[j];
> >>
> >>if (pad->pad.flags & MEDIA_PAD_FL_SINK)
> >>continue;
> >>
> >>...
> >>}
> >>
> >>as the pad isn't going to spontaneously change this flag while we
> >>consider each individual link.
> >
> >
> >Sure, I can do that. It would avoid iterating unnecessarily through the
> >pad's links if the pad is a sink.
> >
> >
> >> However, maybe the test should be:
> >>
> >>if (!(pad->pad.flags & MEDIA_PAD_FL_SOURCE))
> >>
> >>?
> >>
> >
> >maybe that is more intuitive.
> >
> >
> >>(b) the terms "local" and "remote" in imx_media_create_link() are
> >>replaced with "source" and "sink" respectively, since this will
> >>now be called with a guaranteed source pad.
> >
> >Agreed. I'll change the arg and local var names.
> >
> >>
> >>As for Philipp's solution, I'm not sure what the correct solution for
> >>something like this is.

Re: [RFC] [media] imx: assume MEDIA_ENT_F_ATV_DECODER entities output video on pad 1

[Russell King - ARM Linux]

On Thu, Mar 30, 2017 at 07:25:49PM +0200, Philipp Zabel wrote:
> The TVP5150 DT bindings specify a single output port (port 0) that
> corresponds to the video output pad (pad 1, DEMOD_PAD_VID_OUT).
> 
> Signed-off-by: Philipp Zabel 
> ---
> I'm trying to get this to work with a TVP5150 analog TV decoder, and the
> first problem is that this device doesn't have pad 0 as its single
> output pad. Instead, as a MEDIA_ENT_F_ATV_DECODER entity, it has for
> pads (input, video out, vbi out, audio out), and video out is pad 1,
> whereas the device tree only defines a single port (0).

Looking at the patch, it's highlighted another review point with
Steve's driver.

> diff --git a/drivers/staging/media/imx/imx-media-dev.c 
> b/drivers/staging/media/imx/imx-media-dev.c
> index 17e2386a3ca3a..c52d6ca797965 100644
> --- a/drivers/staging/media/imx/imx-media-dev.c
> +++ b/drivers/staging/media/imx/imx-media-dev.c
> @@ -267,6 +267,15 @@ static int imx_media_create_link(struct imx_media_dev 
> *imxmd,
>   source_pad = link->local_pad;
>   sink_pad = link->remote_pad;
>  
> + /*
> +  * If the source subdev is an analog video decoder with a single source
> +  * port, assume that this port 0 corresponds to the DEMOD_PAD_VID_OUT
> +  * entity pad.
> +  */
> + if (source->entity.function == MEDIA_ENT_F_ATV_DECODER &&
> + local_sd->num_sink_pads == 0 && local_sd->num_src_pads == 1)
> + source_pad = DEMOD_PAD_VID_OUT;
> +
>   v4l2_info(>v4l2_dev, "%s: %s:%d -> %s:%d\n", __func__,
> source->name, source_pad, sink->name, sink_pad);

What is "local" and what is "remote" here?  It seems that, in the case of
a link being created with the sensor(etc), it's all back to front.

Eg, I see locally:

imx-media: imx_media_create_link: imx219 0-0010:0 -> imx6-mipi-csi2:0

So here, "source" is the imx219 (the sensor), and sink is "imx6-mipi-csi2"
(part of the iMX6 capture.)  However, this makes "local_sd" the subdev of
the sensor, and "remote_sd" the subdev of the CSI2 interface - which is
totally back to front - this code is part of the iMX6 capture system,
so "local" implies that it should be part of that, and the "remote" thing
would be the sensor.

Hence, this seems completely confused.  I'd suggest that:

(a) the "pad->pad.flags & MEDIA_PAD_FL_SINK" test in imx_media_create_link()
is moved into imx_media_create_links(), and placed here instead:

for (j = 0; j < num_pads; j++) {
pad = _sd->pad[j];

if (pad->pad.flags & MEDIA_PAD_FL_SINK)
continue;

...
}

as the pad isn't going to spontaneously change this flag while we
consider each individual link.  However, maybe the test should be:

if (!(pad->pad.flags & MEDIA_PAD_FL_SOURCE))

?

(b) the terms "local" and "remote" in imx_media_create_link() are
replaced with "source" and "sink" respectively, since this will
now be called with a guaranteed source pad.

As for Philipp's solution, I'm not sure what the correct solution for
something like this is.  It depends how you view "hardware interface"
as defined by video-interfaces.txt, and whether the pads on the TVP5150
represent the hardware interfaces.  If we take the view that the pads
do represent hardware interfaces, then using the reg= property on the
port node will solve this problem.

If not, it would mean that we would have to have the iMX capture code
scan the pads on the source device, looking for outputs - but that
runs into a problem - if the source device has multiple outputs, does
the reg= property specify the output pad index or the pad number, and
what if one binding for a device specifies it one way and another
device's binding specifies it a different way.

There's lots of scope for making things really painful here, and
ending up with needing sensor-specific code in capture drivers to
work around different decisions on this.

I think someone needs to nail this down, if it's not already too late.

-- 
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 2/2] [media] cec: Handle RC capability more elegantly

[Russell King - ARM Linux]

On Tue, Apr 04, 2017 at 05:10:05PM +0100, Lee Jones wrote:
> @@ -237,7 +241,6 @@ struct cec_adapter *cec_allocate_adapter(const struct 
> cec_adap_ops *ops,
>   if (!(caps & CEC_CAP_RC))
>   return adap;
>  
> -#if IS_REACHABLE(CONFIG_RC_CORE)
>   /* Prepare the RC input device */
>   adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE);
>   if (!adap->rc) {

The above, coupled with patch 1:

+#ifdef CONFIG_RC_CORE
 struct rc_dev *rc_allocate_device(enum rc_driver_type);
+#else
+static inline struct rc_dev *rc_allocate_device(int unused)
+{
+   return ERR_PTR(-EOPNOTSUPP);
+}
+#endif

is really not nice.  You claim that this is how stuff is done elsewhere
in the kernel, but no, it isn't.  Look at debugfs.

You're right that debugfs returns an error pointer when it's not
configured.  However, the debugfs dentry is only ever passed back into
the debugfs APIs, it is never dereferenced by the caller.

That is not the case here.  The effect if your change is that the
following dereferences will oops the kernel.  This is unacceptable for
a feature that is deconfigured.

-- 
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] [media] cec: Handle RC capability more elegantly

[Russell King - ARM Linux]

On Tue, Apr 04, 2017 at 04:19:39PM +0100, Lee Jones wrote:
> On Tue, 04 Apr 2017, Hans Verkuil wrote:
> 
> > On 04/04/2017 04:43 PM, Lee Jones wrote:
> > > If a user specifies the use of RC as a capability, they should
> > > really be enabling RC Core code.  If they do not we WARN() them
> > > of this and disable the capability for them.
> > > 
> > > Once we know RC Core code has not been enabled, we can update
> > > the user's capabilities and use them as a term of reference for
> > > other RC-only calls.  This is preferable to having ugly #ifery
> > > scattered throughout C code.
> > > 
> > > Most of the functions are actually safe to call, since they
> > > sensibly check for a NULL RC pointer before they attempt to
> > > deference it.
> > > 
> > > Signed-off-by: Lee Jones 
> > > ---
> > >  drivers/media/cec/cec-core.c | 19 +++
> > >  1 file changed, 7 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> > > index cfe414a..51be8d6 100644
> > > --- a/drivers/media/cec/cec-core.c
> > > +++ b/drivers/media/cec/cec-core.c
> > > @@ -208,9 +208,13 @@ struct cec_adapter *cec_allocate_adapter(const 
> > > struct cec_adap_ops *ops,
> > >   return ERR_PTR(-EINVAL);
> > >   if (WARN_ON(!available_las || available_las > CEC_MAX_LOG_ADDRS))
> > >   return ERR_PTR(-EINVAL);
> > > + if (WARN_ON(caps & CEC_CAP_RC && !IS_REACHABLE(CONFIG_RC_CORE)))
> > > + caps &= ~CEC_CAP_RC;
> > 
> > Don't use WARN_ON, this is not an error of any kind.
> 
> Right, this is not an error.
> 
> That's why we are warning the user instead of bombing out.

Please print warning using pr_warn() or dev_warn().  Using WARN_ON()
because something is not configured is _really_ not nice behaviour.
Consider how useful a stack trace is to the user for this situation -
it's completely meaningless.

A message that prompts the user to enable RC_CORE would make more sense,
and be much more informative to the user.  Maybe something like this:

+   if (caps & CEC_CAP_RC && !IS_REACHABLE(CONFIG_RC_CORE)) {
+   pr_warn("CEC: driver %pf requests RC, please enable 
CONFIG_RC_CORE\n",
+   __builtin_return_address(0));
+   caps &= ~CEC_CAP_RC;
+   }

It could be much more informative by using dev_warn() if we had the
'struct device' passed in to this function, and then we wouldn't need
to use __builtin_return_address().

-- 
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 v6 02/39] [media] dt-bindings: Add bindings for i.MX media driver

[Russell King - ARM Linux]

On Mon, Apr 03, 2017 at 09:07:43AM -0500, Rob Herring wrote:
> On Tue, Mar 28, 2017 at 05:35:52PM -0700, Steve Longerbeam wrote:
> > I assume if there's another binding doc in progress, it means
> > someone is working on another Synopsys DW CSI-2 subdevice driver.
> 
> Yes. see http://patchwork.ozlabs.org/patch/736177/
> 
> > Unfortunately I don't have the time to contribute and switch to
> > this other subdevice, and do test/debug.
> 
> >From a DT perspective, I'm not asking that you share the subdevice 
> driver, only the binding. Simply put, there's 1 h/w block here, so there 
> should only be 1 binding. The binding is an ABI, so you can't just merge 
> it and change it later.

I think it would be nice to have some kind of standard base binding
for CSI2 interfaces, but beyond the standard compatible/reg/interrupts
and graph properties, I'm not sure what it would look like.

As far as those properties go, the iMX6 version does better than the
DW version, because we specify the full graph, whereas the DW version
only specifies the downstream link.  Once that's done, there's some
properties (like those specifying the output configuration) which
probably ought to be moved to the graph links instead, once they exist.

So, if anything, I think it's the DW version needs to be augmented with
fuller information, and some of the properties moved.

Also, as I've mentioned in my other reply, while they may both appear
to be called "Synopsys DW CSI-2" devices, they appear to be quite
different from the hardware perspective.

The rest 

-- 
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 v6 02/39] [media] dt-bindings: Add bindings for i.MX media driver

[Russell King - ARM Linux]

On Mon, Apr 03, 2017 at 09:11:35AM -0500, Rob Herring wrote:
> On Wed, Mar 29, 2017 at 09:39:05AM +0100, Russell King - ARM Linux wrote:
> > On Tue, Mar 28, 2017 at 07:21:34PM -0500, Rob Herring wrote:
> > > On Mon, Mar 27, 2017 at 7:40 PM, Steve Longerbeam <slongerb...@gmail.com> 
> > > wrote:
> > > > Add bindings documentation for the i.MX media driver.
> > > >
> > > > Signed-off-by: Steve Longerbeam <steve_longerb...@mentor.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/media/imx.txt | 74 
> > > > +
> > > >  1 file changed, 74 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/media/imx.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/imx.txt 
> > > > b/Documentation/devicetree/bindings/media/imx.txt
> > > > new file mode 100644
> > > > index 000..3059c06
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/imx.txt
> > > > @@ -0,0 +1,74 @@
> > > > +Freescale i.MX Media Video Device
> > > > +=
> > > > +
> > > > +Video Media Controller node
> > > > +---
> > > > +
> > > > +This is the media controller node for video capture support. It is a
> > > > +virtual device that lists the camera serial interface nodes that the
> > > > +media device will control.
> > > > +
> > > > +Required properties:
> > > > +- compatible : "fsl,imx-capture-subsystem";
> > > > +- ports  : Should contain a list of phandles pointing to camera
> > > > +   sensor interface ports of IPU devices
> > > > +
> > > > +example:
> > > > +
> > > > +capture-subsystem {
> > > > +   compatible = "fsl,imx-capture-subsystem";
> > > > +   ports = <_csi0>, <_csi1>;
> > > > +};
> > > > +
> > > > +fim child node
> > > > +--
> > > > +
> > > > +This is an optional child node of the ipu_csi port nodes. If present 
> > > > and
> > > > +available, it enables the Frame Interval Monitor. Its properties can be
> > > > +used to modify the method in which the FIM measures frame intervals.
> > > > +Refer to Documentation/media/v4l-drivers/imx.rst for more info on the
> > > > +Frame Interval Monitor.
> > > > +
> > > > +Optional properties:
> > > > +- fsl,input-capture-channel: an input capture channel and channel 
> > > > flags,
> > > > +specified as . The channel 
> > > > number
> > > > +must be 0 or 1. The flags can be
> > > > +IRQ_TYPE_EDGE_RISING, 
> > > > IRQ_TYPE_EDGE_FALLING, or
> > > > +IRQ_TYPE_EDGE_BOTH, and specify which input
> > > > +capture signal edge will trigger the input
> > > > +capture event. If an input capture channel 
> > > > is
> > > > +specified, the FIM will use this method to
> > > > +measure frame intervals instead of via the 
> > > > EOF
> > > > +interrupt. The input capture method is much
> > > > +preferred over EOF as it is not subject to
> > > > +interrupt latency errors. However it 
> > > > requires
> > > > +routing the VSYNC or FIELD output signals 
> > > > of
> > > > +the camera sensor to one of the i.MX input
> > > > +capture pads (SD1_DAT0, SD1_DAT1), which 
> > > > also
> > > > +gives up support for SD1.
> > > > +
> > > > +
> > > > +mipi_csi2 node
> > > > +--
> > > > +
> > > > +This is the device node for the MIPI CSI-2 Receiver, required for MIPI
> > > > +CSI-2 sensors.
> > > > +
> > > > +Required properties:
> > > > +- compatible   : "fsl,imx6-mipi-csi2", "snps,dw-mipi-csi2";
> > > 
> > > As I mentioned in v5, there

Re: [PATCHv6 01/10] media: add CEC notifier support

[Russell King - ARM Linux]

On Sat, Apr 01, 2017 at 11:22:03AM +0200, Hans Verkuil wrote:
> On 31/03/17 22:46, Russell King - ARM Linux wrote:
> > On Fri, Mar 31, 2017 at 02:20:27PM +0200, Hans Verkuil wrote:
> >> +struct cec_notifier *cec_notifier_get(struct device *dev)
> >> +{
> >> +  struct cec_notifier *n;
> >> +
> >> +  mutex_lock(_notifiers_lock);
> >> +  list_for_each_entry(n, _notifiers, head) {
> >> +  if (n->dev == dev) {
> >> +  mutex_unlock(_notifiers_lock);
> >> +  kref_get(>kref);
> > 
> > Isn't this racy?  What stops one thread trying to get the notifier
> > while another thread puts the notifier?
> > 
> 
> Both get and put take the global cec_notifiers_lock mutex.

No, that doesn't help:

Thread 0Thread 1
mutex_lock()
list_for_each_entry()
if()
mutex_unlock()
mutex_lock()
kref_put()
list_del()
kfree()
mutex_unlock()
kref_get()

So, it's possible that kref_get() can be called on kfree'd memory.

-- 
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: [PATCHv6 01/10] media: add CEC notifier support

[Russell King - ARM Linux]

On Fri, Mar 31, 2017 at 02:20:27PM +0200, Hans Verkuil wrote:
> +struct cec_notifier *cec_notifier_get(struct device *dev)
> +{
> + struct cec_notifier *n;
> +
> + mutex_lock(_notifiers_lock);
> + list_for_each_entry(n, _notifiers, head) {
> + if (n->dev == dev) {
> + mutex_unlock(_notifiers_lock);
> + kref_get(>kref);

Isn't this racy?  What stops one thread trying to get the notifier
while another thread puts the notifier?

-- 
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: [PATCHv6 00/10] video/exynos/sti/cec: add CEC notifier & use in drivers

[Russell King - ARM Linux]

On Fri, Mar 31, 2017 at 03:39:20PM +0100, Russell King - ARM Linux wrote:
> On Fri, Mar 31, 2017 at 02:20:26PM +0200, Hans Verkuil wrote:
> > Comments are welcome. I'd like to get this in for the 4.12 kernel as
> > this is a missing piece needed to integrate CEC drivers.
> 
> First two patches seem fine, and work with dw-hdmi.
> 
> I'll hold dw-hdmi off until after 4.11 - I currently have this stuff
> merged against 4.10, and there's some conflicts with 4.11.
> 
> I also wanted to say that tda998x/tda9950 works, and send you those
> patches, but while trying to test them this afternoon in a tree with
> some of the DRM code that was merged during the last merge window on
> a v4.10 based tree (which I need because of etnaviv), the kernel
> oopses in DRM for god-knows-why.  If/when I get this sorted (don't
> know when) I'll send that stuff as a follow-up to your series.

... and that's looking impossible - the next problem after that seems
to be that the rootfs drive for the box has failed, so I've currently
no way to test tda998x stuff until I get a new drive, filesystem and
so forth rebuilt.

-- 
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: [PATCHv6 00/10] video/exynos/sti/cec: add CEC notifier & use in drivers

[Russell King - ARM Linux]

On Fri, Mar 31, 2017 at 02:20:26PM +0200, Hans Verkuil wrote:
> Comments are welcome. I'd like to get this in for the 4.12 kernel as
> this is a missing piece needed to integrate CEC drivers.

First two patches seem fine, and work with dw-hdmi.

I'll hold dw-hdmi off until after 4.11 - I currently have this stuff
merged against 4.10, and there's some conflicts with 4.11.

I also wanted to say that tda998x/tda9950 works, and send you those
patches, but while trying to test them this afternoon in a tree with
some of the DRM code that was merged during the last merge window on
a v4.10 based tree (which I need because of etnaviv), the kernel
oopses in DRM for god-knows-why.  If/when I get this sorted (don't
know when) I'll send that stuff as a follow-up to your series.

-- 
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: [PATCHv5 04/11] exynos_hdmi: add CEC notifier support

[Russell King - ARM Linux]

On Wed, Mar 29, 2017 at 04:15:36PM +0200, Hans Verkuil wrote:
> + cec_notifier_set_phys_addr(hdata->notifier,
> +cec_get_edid_phys_addr(edid));

This pattern causes problems - can we have the notifier taking the EDID
please, and stubs in cec-notifier.h to stub that out?

Maybe something like cec_notifier_set_phys_addr_from_edid(edid) ?

Having converted the tda998x code over to your new notifier, the 0-day
builder reported this tonight:

>> ERROR: "cec_get_edid_phys_addr" [drivers/gpu/drm/i2c/tda998x.ko] undefined!

which is caused exactly by this problem.  I can add #ifdefs into the
tda998x driver, but as you're already stubbing out
cec_notifier_set_phys_addr() in cec-notifier.h, it would be stupid to
have to resort to #ifdefs in driver code to solve this issue.

Thanks.

-- 
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 v6 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Thu, Mar 30, 2017 at 09:12:29AM -0700, Steve Longerbeam wrote:
> 
> 
> On 03/30/2017 04:02 AM, Russell King - ARM Linux wrote:
> >This fails at step 1.  The removal of the frame interval support now
> >means my setup script fails when trying to set the frame interval on
> >the camera:
> >
> >Enumerating pads and links
> >Setting up format SRGGB8_1X8 816x616 on pad imx219 0-0010/0
> >Format set: SRGGB8_1X8 816x616
> >Setting up frame interval 1/25 on pad imx219 0-0010/0
> >Frame interval set: 1/25
> >Setting up format SRGGB8_1X8 816x616 on pad imx6-mipi-csi2/0
> >Format set: SRGGB8_1X8 816x616
> >Setting up frame interval 1/25 on pad imx6-mipi-csi2/0
> >Unable to set frame interval: Inappropriate ioctl for device (-25)Unable to 
> >setup formats: Inappropriate ioctl for device (25)
> >
> >This is because media-ctl tries to propagate it from the imx219 source
> >pad to the csi2 sink pad, and the csi2 now fails that ioctl.
> 
> I assume you're using Philipp's frame interval patches to media-ctl.
> Can you make the frame interval propagation optional in those patches?
> I.e. don't error-out with a failure code if the ioctl returns ENOTTY.

Damn, you're right.  There's way too much stuff to try and keep track
of with this stuff.

-- 
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 v6 00/39] i.MX Media Driver

[Russell King - ARM Linux]

This fails at step 1.  The removal of the frame interval support now
means my setup script fails when trying to set the frame interval on
the camera:

Enumerating pads and links
Setting up format SRGGB8_1X8 816x616 on pad imx219 0-0010/0
Format set: SRGGB8_1X8 816x616
Setting up frame interval 1/25 on pad imx219 0-0010/0
Frame interval set: 1/25
Setting up format SRGGB8_1X8 816x616 on pad imx6-mipi-csi2/0
Format set: SRGGB8_1X8 816x616
Setting up frame interval 1/25 on pad imx6-mipi-csi2/0
Unable to set frame interval: Inappropriate ioctl for device (-25)Unable to 
setup formats: Inappropriate ioctl for device (25)

This is because media-ctl tries to propagate it from the imx219 source
pad to the csi2 sink pad, and the csi2 now fails that ioctl.

This makes media-ctl return a failure code, which means that it's not
possible for a script to determine whether the failure was due to the
camera setup or something else.  So, we have to assume that the
whole command failed.

This is completely broken, and I'm even more convinced that those
arguing for this behaviour really have not thought it through well
enough before demanding that this code was removed.

As far as I'm concerned, the end result is completely broken and
unusable.  I'm going to be merging the frame interval support back
into my test tree, because that's the only sane thing to do.

If v4l2 people want to object to having frame interval support present
for all subdevs, then _they_ need to make sure that the rest of their
software conforms to what they're telling people to do.

-- 
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 v6 02/39] [media] dt-bindings: Add bindings for i.MX media driver

[Russell King - ARM Linux]

On Tue, Mar 28, 2017 at 07:21:34PM -0500, Rob Herring wrote:
> On Mon, Mar 27, 2017 at 7:40 PM, Steve Longerbeam  
> wrote:
> > Add bindings documentation for the i.MX media driver.
> >
> > Signed-off-by: Steve Longerbeam 
> > ---
> >  Documentation/devicetree/bindings/media/imx.txt | 74 
> > +
> >  1 file changed, 74 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/imx.txt
> >
> > diff --git a/Documentation/devicetree/bindings/media/imx.txt 
> > b/Documentation/devicetree/bindings/media/imx.txt
> > new file mode 100644
> > index 000..3059c06
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/imx.txt
> > @@ -0,0 +1,74 @@
> > +Freescale i.MX Media Video Device
> > +=
> > +
> > +Video Media Controller node
> > +---
> > +
> > +This is the media controller node for video capture support. It is a
> > +virtual device that lists the camera serial interface nodes that the
> > +media device will control.
> > +
> > +Required properties:
> > +- compatible : "fsl,imx-capture-subsystem";
> > +- ports  : Should contain a list of phandles pointing to camera
> > +   sensor interface ports of IPU devices
> > +
> > +example:
> > +
> > +capture-subsystem {
> > +   compatible = "fsl,imx-capture-subsystem";
> > +   ports = <_csi0>, <_csi1>;
> > +};
> > +
> > +fim child node
> > +--
> > +
> > +This is an optional child node of the ipu_csi port nodes. If present and
> > +available, it enables the Frame Interval Monitor. Its properties can be
> > +used to modify the method in which the FIM measures frame intervals.
> > +Refer to Documentation/media/v4l-drivers/imx.rst for more info on the
> > +Frame Interval Monitor.
> > +
> > +Optional properties:
> > +- fsl,input-capture-channel: an input capture channel and channel flags,
> > +specified as . The channel number
> > +must be 0 or 1. The flags can be
> > +IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING, or
> > +IRQ_TYPE_EDGE_BOTH, and specify which input
> > +capture signal edge will trigger the input
> > +capture event. If an input capture channel is
> > +specified, the FIM will use this method to
> > +measure frame intervals instead of via the EOF
> > +interrupt. The input capture method is much
> > +preferred over EOF as it is not subject to
> > +interrupt latency errors. However it requires
> > +routing the VSYNC or FIELD output signals of
> > +the camera sensor to one of the i.MX input
> > +capture pads (SD1_DAT0, SD1_DAT1), which also
> > +gives up support for SD1.
> > +
> > +
> > +mipi_csi2 node
> > +--
> > +
> > +This is the device node for the MIPI CSI-2 Receiver, required for MIPI
> > +CSI-2 sensors.
> > +
> > +Required properties:
> > +- compatible   : "fsl,imx6-mipi-csi2", "snps,dw-mipi-csi2";
> 
> As I mentioned in v5, there's a DW CSI2 binding in progress. This
> needs to be based on that.

Maybe someone can provide some kind of reference to it, and it's
associated driver?

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Tue, Mar 21, 2017 at 11:59:02AM +0100, Hans Verkuil wrote:
> Ah, good to hear that -s works with MC. I was not sure about that.
> Thanks for the feedback!

Not soo good on iMX6:

$ v4l2-compliance -d /dev/video10 -s --expbuf-device=/dev/video0
...
Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
fail: v4l2-test-input-output.cpp(420): G_INPUT not supported 
for a capture device
test VIDIOC_G/S/ENUMINPUT: FAIL
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 0 Audio Inputs: 0 Tuners: 0
...
Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
test VIDIOC_G/S/TRY_EXT_CTRLS: OK
fail: v4l2-test-controls.cpp(782): subscribe event for control 
'User Controls' failed
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL
...
Streaming ioctls:
test read/write: OK (Not Supported)
fail: v4l2-test-buffers.cpp(297): g_field() == V4L2_FIELD_ANY
fail: v4l2-test-buffers.cpp(703): buf.check(q, last_seq)
fail: v4l2-test-buffers.cpp(973): captureBufs(node, q, m2m_q, 
frame_count, false)
test MMAP: FAIL
test USERPTR: OK (Not Supported)
fail: v4l2-test-buffers.cpp(1188): can_stream && ret != EINVAL
test DMABUF: FAIL

(/dev/video0 being CODA).  CODA itself seems to have failures:

Format ioctls:
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
warn: v4l2-test-formats.cpp(1187): S_PARM is supported for 
buftype 2, but not ENUM_FRAMEINTERVALS
warn: v4l2-test-formats.cpp(1194): S_PARM is supported but 
doesn't report V4L2_CAP_TIMEPERFRAME
test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
fail: v4l2-test-formats.cpp(774): fmt_out.g_colorspace() != col
...
Streaming ioctls:
test read/write: OK (Not Supported)
fail: v4l2-test-buffers.cpp(956): q.create_bufs(node, 1, ) 
!= EINVAL
test MMAP: FAIL
test USERPTR: OK (Not Supported)
test DMABUF: Cannot test, specify --expbuf-device

-- 
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 v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 06:23:24PM +0100, Philipp Zabel wrote:
> @@ -1173,15 +1196,8 @@ static void csi_try_fmt(struct csi_priv *priv,
>   incc = imx_media_find_mbus_format(infmt->code,
> CS_SEL_ANY, true);
>  
> - if (sdformat->format.width < priv->crop.width * 3 / 4)
> - sdformat->format.width = priv->crop.width / 2;
> - else
> - sdformat->format.width = priv->crop.width;
> -
> - if (sdformat->format.height < priv->crop.height * 3 / 4)
> - sdformat->format.height = priv->crop.height / 2;
> - else
> - sdformat->format.height = priv->crop.height;
> + sdformat->format.width = compose->width;
> + sdformat->format.height = compose->height;
>  
>   if (incc->bayer) {
>   sdformat->format.code = infmt->code;

We need to do more in here, because right now setting the source pads
overwrites the colorimetry etc information.  Maybe something like the
below?  I'm wondering if it would be a saner approach to copy the
sink format and update the parameters that can be changed, rather than
trying to list all the parameters that shouldn't be changed.  What if
the format structure gains a new member?

diff --git a/drivers/staging/media/imx/imx-media-csi.c 
b/drivers/staging/media/imx/imx-media-csi.c
index 1492b92e1970..756204ced53c 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1221,6 +1221,12 @@ static void csi_try_fmt(struct csi_priv *priv,
sdformat->format.field =  (infmt->height == 480) ?
V4L2_FIELD_SEQ_TB : V4L2_FIELD_SEQ_BT;
}
+
+   /* copy settings we can't change */
+   sdformat->format.colorspace = infmt->colorspace;
+   sdformat->format.ycbcr_enc = infmt->ycbcr_enc;
+   sdformat->format.quantization = infmt->quantization;
+   sdformat->format.xfer_func = infmt->xfer_func;
break;
case CSI_SINK_PAD:
v4l_bound_align_image(>format.width, MIN_W, MAX_W,



-- 
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 v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 06:23:24PM +0100, Philipp Zabel wrote:
> --8<--
> >From 2830aebc404bdfc9d7fc1ec94e5282d0b668e8f6 Mon Sep 17 00:00:00 2001
> From: Philipp Zabel 
> Date: Mon, 20 Mar 2017 17:10:21 +0100
> Subject: [PATCH] media: imx: csi: add sink selection rectangles
> 
> Move the crop rectangle to the sink pad and add a sink compose rectangle
> to configure scaling. Also propagate rectangles from sink pad to crop
> rectangle, to compose rectangle, and to the source pads both in ACTIVE
> and TRY variants of set_fmt/selection, and initialize the default crop
> and compose rectangles.

Looks fine for the most part.

> - /*
> -  * Modifying the crop rectangle always changes the format on the source
> -  * pad. If the KEEP_CONFIG flag is set, just return the current crop
> -  * rectangle.
> -  */
> - if (sel->flags & V4L2_SEL_FLAG_KEEP_CONFIG) {
> - sel->r = priv->crop;
> - if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> - cfg->try_crop = sel->r;
> + infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sel->which);
> + crop = __csi_get_crop(priv, cfg, sel->which);
> + compose = __csi_get_compose(priv, cfg, sel->which);
> +
> + switch (sel->target) {
> + case V4L2_SEL_TGT_CROP:
> + /*
> +  * Modifying the crop rectangle always changes the format on
> +  * the source pads. If the KEEP_CONFIG flag is set, just return
> +  * the current crop rectangle.
> +  */
> + if (sel->flags & V4L2_SEL_FLAG_KEEP_CONFIG) {
> + sel->r = priv->crop;

My understanding of KEEP_CONFIG is that the only thing we're not
allowed to do is to propagate the change downstream.

Since downstream of the crop is the compose, that means the only
restriction here is that the width and height of the crop window must
be either equal to the compose width/height, or double the compose
width/height.  (Anything else would necessitate the compose window
changing.)

However, the crop window can move position within the crop bounds,
provided it's entirely contained within those crop bounds.

The problem is that this becomes rather more complex it deal with
(as I'm finding out in my imx219 camera driver) and I'm thinking
that some of this complexity should probably be in a helper in
generic v4l2 code.

I don't know whether this applies (I hope it doesn't) but there's a
pile of guidelines in Documentation/media/uapi/v4l/vidioc-g-selection.rst
which describe how a crop/compose rectangle should be adjusted.  As
I say, I hope they don't apply, because if they do, we would _really_
need helpers for this stuff, as I don't think having each driver
implement all these rules would be too successful!

> + if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> + *crop = sel->r;
> + goto out;
> + }
> +
> + csi_try_crop(priv, >r, cfg, infmt, sensor);
> +
> + *crop = sel->r;
> +
> + /* Reset scaling to 1:1 */
> + compose->width = crop->width;
> + compose->height = crop->height;
> + break;
> + case V4L2_SEL_TGT_COMPOSE:
> + /*
> +  * Modifying the compose rectangle always changes the format on
> +  * the source pads. If the KEEP_CONFIG flag is set, just return
> +  * the current compose rectangle.
> +  */
> + if (sel->flags & V4L2_SEL_FLAG_KEEP_CONFIG) {
> + sel->r = priv->compose;

I think, with my understanding of how the KEEP_CONFIG flag works, this
should be:
sel->r = *compose;

because if we change the compose rectangle width/height, we would need
to propagate this to the source pad, and the KEEP_CONFIG description
says we're not allowed to do that.

-- 
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 v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 06:40:21PM +0100, Philipp Zabel wrote:
> On Mon, 2017-03-20 at 14:17 +0000, Russell King - ARM Linux wrote:
> > I have tripped over a bug in media-ctl when specifying both a crop and
> > compose rectangle - the --help output suggests that "," should be used
> > to separate them.  media-ctl rejects that, telling me the character at
> > the "," should be "]".  Replacing the "," with " " allows media-ctl to
> > accept it and set both rectangles, so it sounds like a parser bug - I've
> > not looked into this any further yet.
> 
> I can confirm this. I don't see any place in
> v4l2_subdev_parse_pad_format that handles the "," separator. There's
> just whitespace skipping between the v4l2-properties.

Maybe this is the easiest solution:

 utils/media-ctl/options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils/media-ctl/options.c b/utils/media-ctl/options.c
index 83ca1ca..8b97874 100644
--- a/utils/media-ctl/options.c
+++ b/utils/media-ctl/options.c
@@ -65,7 +65,7 @@ static void usage(const char *argv0)
printf("\tentity  = entity-number | ( '\"' entity-name '\"' ) 
;\n");
printf("\n");
printf("\tv4l2= pad '[' v4l2-properties ']' ;\n");
-   printf("\tv4l2-properties = v4l2-property { ',' v4l2-property } ;\n");
+   printf("\tv4l2-properties = v4l2-property { ' '* v4l2-property } ;\n");
printf("\tv4l2-property   = v4l2-mbusfmt | v4l2-crop | 
v4l2-interval\n");
printf("\t| v4l2-compose | v4l2-interval ;\n");
printf("\tv4l2-mbusfmt= 'fmt:' fcc '/' size ; { 'field:' v4l2-field 
; } { 'colorspace:' v4l2-colorspace ; }\n");

;)

-- 
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 v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 02:17:05PM +, Russell King - ARM Linux wrote:
> On Mon, Mar 20, 2017 at 03:00:51PM +0100, Philipp Zabel wrote:
> > On Mon, 2017-03-20 at 12:08 +, Russell King - ARM Linux wrote:
> > > The same document says:
> > > 
> > >   Scaling support is optional. When supported by a subdev, the crop
> > >   rectangle on the subdev's sink pad is scaled to the size configured
> > >   using the
> > >   :ref:`VIDIOC_SUBDEV_S_SELECTION ` IOCTL
> > >   using ``V4L2_SEL_TGT_COMPOSE`` selection target on the same pad. If the
> > >   subdev supports scaling but not composing, the top and left values are
> > >   not used and must always be set to zero.
> > 
> > Right, this sentence does imply that when scaling is supported, there
> > must be a sink compose rectangle, even when composing is not.
> > 
> > I have previously set up scaling like this:
> > 
> > media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> > media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
> > 
> > Does this mean, it should work like this instead?
> > 
> > media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> > media-ctl --set-v4l2 
> > "'ipu1_csi0':0[fmt:UYVY2X8/1920x1080@1/60,compose:(0,0)/960x540]"
> > media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
> > 
> > I suppose setting the source pad format should not be allowed to modify
> > the sink compose rectangle.
> 
> That is what I believe having read these documents several times, but
> we need v4l2 people to confirm.
> 
> Note that setting the format on 'ipu1_csi0':0 should already be done by
> the previous media-ctl command, so it should be possible to simplify
> that to:
> 
> media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> media-ctl --set-v4l2 "'ipu1_csi0':0[compose:(0,0)/960x540]"
> media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"

There is a slightly puzzling bit in the documentation.  If we consider
the CSI, and the sink compose rectangle size has to always match the
the source output pad format size (since in hardware they are one of
the same), then:

  Inside subdevs, the order of image processing steps will always be from
  the sink pad towards the source pad. This is also reflected in the order
  in which the configuration must be performed by the user: the changes
  made will be propagated to any subsequent stages. If this behaviour is
  not desired, the user must set ``V4L2_SEL_FLAG_KEEP_CONFIG`` flag. This
 
  flag causes no propagation of the changes are allowed in any
  
  circumstances. This may also cause the accessed rectangle to be adjusted
  
  by the driver, depending on the properties of the underlying hardware.
  ^^

presumably, this means if we get a request to change the source compose
rectangle with V4L2_SEL_FLAG_KEEP_CONFIG set, we need to force its size
to be the current output format size.  I don't think we can do anything
else - because the above makes it very clear that any following stages
shall not be updated.  The last sentence appears to give permission to
do that.

This also has implications when changing the sink crop - the sink crop
(eg) must not be smaller than the sink compose, as we don't support
scaling up in CSI.

It seems to me that V4L2_SEL_FLAG_KEEP_CONFIG in practice changes the
way validation of the request works.  So, rather than validating the
request against the upstream rectangle and propagating downstream, it
needs to be validated against both the upstream and downstream
rectangles instead.

It seems there's many subtleties to this...

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 05:29:07PM +0100, Philipp Zabel wrote:
> According to the documentation [1], you are doing the right thing:
> 
> The struct v4l2_subdev_frame_interval pad references a non-existing
> pad, or the pad doesn’t support frame intervals.
> 
> But v4l2_subdev_call returns -ENOIOCTLCMD if the g_frame_interval op is
> not implemented at all, which is turned into -ENOTTY by video_usercopy.
> 
> [1] 
> https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-subdev-g-frame-interval.html#return-value

Thanks for confirming.

> > Maybe something like the following would be a better idea?
> > 
> >  utils/media-ctl/media-ctl.c | 10 +-
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c
> > index f61963a..a50a559 100644
> > --- a/utils/media-ctl/media-ctl.c
> > +++ b/utils/media-ctl/media-ctl.c
> > @@ -81,22 +81,22 @@ static void v4l2_subdev_print_format(struct 
> > media_entity *entity,
> > struct v4l2_mbus_framefmt format;
> > struct v4l2_fract interval = { 0, 0 };
> > struct v4l2_rect rect;
> > -   int ret;
> > +   int ret, err_fi;
> >  
> > ret = v4l2_subdev_get_format(entity, , pad, which);
> > if (ret != 0)
> > return;
> >  
> > -   ret = v4l2_subdev_get_frame_interval(entity, , pad);
> > -   if (ret != 0 && ret != -ENOTTY)
> > -   return;
> > +   err_fi = v4l2_subdev_get_frame_interval(entity, , pad);
> 
> Not supporting frame intervals doesn't warrant a visible error message,
> I think -EINVAL should also be ignored above, if the spec is to be
> believed.
> 
> >  
> > printf("\t\t[fmt:%s/%ux%u",
> >v4l2_subdev_pixelcode_to_string(format.code),
> >format.width, format.height);
> >  
> > -   if (interval.numerator || interval.denominator)
> > +   if (err_fi == 0 && (interval.numerator || interval.denominator))
> > printf("@%u/%u", interval.numerator, interval.denominator);
> > +   else if (err_fi != -ENOTTY)
> > +   printf("@", strerror(-err_fi));
> 
> Or here.

I don't mind which - I could change this to:

else if (err_fi != -ENOTTY && err_fi != -EINVAL)

Or an alternative would be to print an error (ignoring ENOTTY and EINVAL)
to stderr at the "v4l2_subdev_get_frame_interval" callsite and continue
on (ensuring that interval is zeroed).

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

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 12:39:38PM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 20 Mar 2017 14:24:25 +0100
> Hans Verkuil  escreveu:
> > I don't think this control inheritance patch will magically prevent you
> > from needed a plugin.
> 
> Yeah, it is not just control inheritance. The driver needs to work
> without subdev API, e. g. mbus settings should also be done via the
> video devnode.
> 
> Btw, Sakari made a good point on IRC: what happens if some app 
> try to change the pipeline or subdev settings while another
> application is using the device? The driver should block such 
> changes, maybe using the V4L2 priority mechanism.

My understanding is that there are already mechanisms in place to
prevent that, but it's driver dependent - certainly several of the
imx driver subdevs check whether they have an active stream, and
refuse (eg) all set_fmt calls with -EBUSY if that is so.

(That statement raises another question in my mind: if the subdev is
streaming, should it refuse all set_fmt, even for the TRY stuff?)

> In parallel, someone has to fix libv4l for it to be default on
> applications like gstreamer, e. g. adding support for DMABUF
> and fixing other issues that are preventing it to be used by
> default.

Hmm, not sure what you mean there - I've used dmabuf with gstreamer's
v4l2src linked to libv4l2, importing the buffers into etnaviv using
a custom plugin.  There are distros around (ubuntu) where the v4l2
plugin is built against libv4l2.

> My understanding here is that there are developers wanting/needing
> to have standard V4L2 apps support for (some) i.MX6 devices. Those are
> the ones that may/will allocate some time for it to happen.

Quite - but we need to first know what is acceptable to the v4l2
community before we waste a lot of effort coding something up that
may not be suitable.  Like everyone else, there's only a limited
amount of effort available, so using it wisely is a very good idea.

Up until recently, it seemed that the only solution was to solve the
userspace side of the media controller API via v4l2 plugins and the
like.

Much of my time that I have available to look at the imx6 capture
stuff at the moment is taken up by triping over UAPI issues with the
current code (such as the ones about CSI scaling, colorimetry, etc)
and trying to get concensus on what the right solution to fix those
issues actually is, and at the moment I don't have spare time to
start addressing any kind of v4l2 plugin for user controls nor any
other solution.

Eg, I spent much of this last weekend sorting out my IMX219 camera
sensor driver for my new understanding about how scaling is supposed
to work, the frame enumeration issue (which I've posted patches for)
and the CSI scaling issue (which I've some half-baked patch for at the
moment, but probably by the time I've finished sorting that, Philipp
or Steve will already have a solution.)

That said, my new understanding of the scaling impacts the four patches
I posted, and probably makes the frame size enumeration in CSI (due to
its scaling) rather obsolete.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 02:20:16PM +0100, Philipp Zabel wrote:
> To set and read colorimetry information:
> https://patchwork.linuxtv.org/patch/39350/

Thanks, I've applied all four of your patches, but there's a side effect
from that.  Old media-ctl (modified by me):

- entity 53: imx219 0-0010 (2 pads, 2 links)
 type V4L2 subdev subtype Unknown flags 0
 device node name /dev/v4l-subdev9
pad0: Source
[fmt:SRGGB8/816x616 field:none
 frame_interval:1/25]
-> "imx6-mipi-csi2":0 [ENABLED]
pad1: Sink
[fmt:SRGGB10/3280x2464 field:none
 crop.bounds:(0,0)/3280x2464
 crop:(0,0)/3264x2464
 compose.bounds:(0,0)/3264x2464
 compose:(0,0)/816x616]
<- "imx219 pixel 0-0010":0 [ENABLED,IMMUTABLE]

New media-ctl:

- entity 53: imx219 0-0010 (2 pads, 2 links)
 type V4L2 subdev subtype Unknown flags 0
 device node name /dev/v4l-subdev9
pad0: Source
[fmt:SRGGB8_1X8/816x616@1/25 field:none colorspace:srgb 
xfer:srgb]
-> "imx6-mipi-csi2":0 [ENABLED]
pad1: Sink
<- "imx219 pixel 0-0010":0 [ENABLED,IMMUTABLE]

It looks like we successfully retrieve the frame interval for pad 0
and print it, but when we try to retrieve the frame interval for pad 1,
we get EINVAL (because that's what I'm returning, but I'm wondering if
that's the correct thing to do...) and that prevents _all_ format
information being output.

Maybe something like the following would be a better idea?

 utils/media-ctl/media-ctl.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c
index f61963a..a50a559 100644
--- a/utils/media-ctl/media-ctl.c
+++ b/utils/media-ctl/media-ctl.c
@@ -81,22 +81,22 @@ static void v4l2_subdev_print_format(struct media_entity 
*entity,
struct v4l2_mbus_framefmt format;
struct v4l2_fract interval = { 0, 0 };
struct v4l2_rect rect;
-   int ret;
+   int ret, err_fi;
 
ret = v4l2_subdev_get_format(entity, , pad, which);
if (ret != 0)
return;
 
-   ret = v4l2_subdev_get_frame_interval(entity, , pad);
-   if (ret != 0 && ret != -ENOTTY)
-   return;
+   err_fi = v4l2_subdev_get_frame_interval(entity, , pad);
 
printf("\t\t[fmt:%s/%ux%u",
   v4l2_subdev_pixelcode_to_string(format.code),
   format.width, format.height);
 
-   if (interval.numerator || interval.denominator)
+   if (err_fi == 0 && (interval.numerator || interval.denominator))
printf("@%u/%u", interval.numerator, interval.denominator);
+   else if (err_fi != -ENOTTY)
+   printf("@", strerror(-err_fi));
 
if (format.field)
printf(" field:%s", v4l2_subdev_field_to_string(format.field));


-- 
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: [PATCHv2 1/4] video: add hotplug detect notifier support

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 02:26:16PM +, Russell King - ARM Linux wrote:
> On Mon, Jan 02, 2017 at 03:19:04PM +0100, Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verk...@cisco.com>
> > 
> > Add support for video hotplug detect and EDID/ELD notifiers, which is used
> > to convey information from video drivers to their CEC and audio 
> > counterparts.
> > 
> > Based on an earlier version from Russell King:
> > 
> > https://patchwork.kernel.org/patch/9277043/
> > 
> > The hpd_notifier is a reference counted object containing the HPD/EDID/ELD 
> > state
> > of a video device.
> 
> I thought we had decided to drop the ELD bits?

Ignore that - mailer wrapped around to the first message in my mailbox!

-- 
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: [PATCHv2 1/4] video: add hotplug detect notifier support

[Russell King - ARM Linux]

On Mon, Jan 02, 2017 at 03:19:04PM +0100, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> Add support for video hotplug detect and EDID/ELD notifiers, which is used
> to convey information from video drivers to their CEC and audio counterparts.
> 
> Based on an earlier version from Russell King:
> 
> https://patchwork.kernel.org/patch/9277043/
> 
> The hpd_notifier is a reference counted object containing the HPD/EDID/ELD 
> state
> of a video device.

I thought we had decided to drop the ELD bits?

-- 
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 v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 03:00:51PM +0100, Philipp Zabel wrote:
> On Mon, 2017-03-20 at 12:08 +0000, Russell King - ARM Linux wrote:
> > The same document says:
> > 
> >   Scaling support is optional. When supported by a subdev, the crop
> >   rectangle on the subdev's sink pad is scaled to the size configured
> >   using the
> >   :ref:`VIDIOC_SUBDEV_S_SELECTION ` IOCTL
> >   using ``V4L2_SEL_TGT_COMPOSE`` selection target on the same pad. If the
> >   subdev supports scaling but not composing, the top and left values are
> >   not used and must always be set to zero.
> 
> Right, this sentence does imply that when scaling is supported, there
> must be a sink compose rectangle, even when composing is not.
> 
> I have previously set up scaling like this:
> 
> media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
> 
> Does this mean, it should work like this instead?
> 
> media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> media-ctl --set-v4l2 
> "'ipu1_csi0':0[fmt:UYVY2X8/1920x1080@1/60,compose:(0,0)/960x540]"
> media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
> 
> I suppose setting the source pad format should not be allowed to modify
> the sink compose rectangle.

That is what I believe having read these documents several times, but
we need v4l2 people to confirm.

Note that setting the format on 'ipu1_csi0':0 should already be done by
the previous media-ctl command, so it should be possible to simplify
that to:

media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
media-ctl --set-v4l2 "'ipu1_csi0':0[compose:(0,0)/960x540]"
media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"

I have tripped over a bug in media-ctl when specifying both a crop and
compose rectangle - the --help output suggests that "," should be used
to separate them.  media-ctl rejects that, telling me the character at
the "," should be "]".  Replacing the "," with " " allows media-ctl to
accept it and set both rectangles, so it sounds like a parser bug - I've
not looked into this any further yet.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 02:57:03PM +0100, Hans Verkuil wrote:
> On 03/20/2017 02:29 PM, Russell King - ARM Linux wrote:
> > It's what I have - remember, not everyone is happy to constantly replace
> > their distro packages with random new stuff.
> 
> This is a compliance test, which is continuously developed in tandem with
> new kernel versions. If you are working with an upstream kernel, then you
> should also use the corresponding v4l2-compliance test. What's the point
> of using an old one?
> 
> I will not support driver developers that use an old version of the
> compliance test, that's a waste of my time.

The reason that people may _not_ wish to constantly update v4l-utils
is that it changes the libraries installed on their systems.

So, the solution to that is not to complain about developers not using
the latest version, but instead to de-couple it from the rest of the
package, and provide it as a separate, stand-alone package that doesn't
come with all the extra baggage.

Now, there's two possible answers to that:

1. it depends on the libv4l2 version.  If that's so, then you are
   insisting that people constantly move to the latest libv4l2 because
   of API changes, and those API changes may upset applications they're
   using.  So this isn't really on.

2. it doesn't depend on libv4l2 version, in which case there's no reason
   for it to be packaged with v4l-utils.

The reality is that v4l2-compliance links with libv4l2, so I'm not sure
which it is.  What I am sure of is that I don't want to upgrade libv4l2
on an ad-hoc basis, potentially causing issues with applications.

> >> To test actual streaming you need to provide the -s option.
> >>
> >> Note: v4l2-compliance has been developed for 'regular' video devices,
> >> not MC devices. It may or may not work with the -s option.
> > 
> > Right, and it exists to verify that the establised v4l2 API is correctly
> > implemented.  If the v4l2 API is being offered to user applications,
> > then it must be conformant, otherwise it's not offering the v4l2 API.
> > (That's very much a definition statement in itself.)
> > 
> > So, are we really going to say MC devices do not offer the v4l2 API to
> > userspace, but something that might work?  We've already seen today
> > one user say that they're not going to use mainline because of the
> > crud surrounding MC.
> > 
> 
> Actually, my understanding was that he was stuck on the old kernel code.

Err, sorry, I really don't follow.  Who is "he"?

_I_ was the one who reported the EXPBUF problem.  Your comment makes no
sense.

> In the case of v4l2-compliance, I never had the time to make it work with
> MC devices. Same for that matter of certain memory to memory devices.
> 
> Just like MC devices these too behave differently. They are partially
> supported in v4l2-compliance, but not fully.

It seems you saying that the API provided by /dev/video* for a MC device
breaks the v4l2-compliance tests?

_No one_ has mentioned using v4l2-compliance on the subdevs.

> Complaining about this really won't help. We know it's a problem and unless
> someone (me perhaps?) manages to get paid to work on this it's unlikely to
> change for now.

Like the above comment, your comment makes no sense.  I'm not complaining,
I'm trying to find out the details.

Yes, MC stuff sucks big time right now, the documentation is poor, there's
a lack of understanding on all sides of the issues (which can be seen by
the different opinions that people hold.)  The only way to resolve these
differences is via discussion, and if you're going to start thinking that
everyone is complaining, then there's not going to be any forward progress.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 02:01:58PM +0100, Hans Verkuil wrote:
> On 03/19/2017 06:54 PM, Steve Longerbeam wrote:
> > 
> > 
> > On 03/19/2017 03:38 AM, Russell King - ARM Linux wrote:
> >> What did you do with:
> >>
> >> ioctl(3, VIDIOC_REQBUFS, {count=0, type=0 /* V4L2_BUF_TYPE_??? */, 
> >> memory=0 /* V4L2_MEMORY_??? */}) = -1 EINVAL (Invalid argument)
> >>  test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> >> ioctl(3, VIDIOC_EXPBUF, 0xbef405bc) = -1 EINVAL (Invalid argument)
> >>  fail: v4l2-test-buffers.cpp(571): q.has_expbuf(node)
> 
> This is really a knock-on effect from an earlier issue where the compliance 
> test
> didn't detect support for MEMORY_MMAP.

So why does it succeed when I fix the compliance errors with VIDIOC_G_FMT?
With that fixed, I now get:

Format ioctls:
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
test Cropping: OK (Not Supported)
test Composing: OK (Not Supported)
test Scaling: OK (Not Supported)

Buffer ioctls:
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK

The reason is, if you look at the code, VIDIOC_G_FMT populates a list
of possible buffer formats "node->valid_buftypes".  If the VIDIOC_G_FMT
test fails, then node->valid_buftypes is zero.

This causes testReqBufs() to only check for the all-zeroed VIDIOC_REQBUFS
and declare it conformant, without creating any buffers (it can't, it
doesn't know which formats are supported.)

This causes node->valid_memorytype to be zero.

We then go on to testExpBuf(), and valid_memorytype zero, claiming (falsely)
that MMAP is not supported.  The reality is that it _is_ supported, but
it's just the non-compliant VICIOC_G_FMT call (due to the colorspace
issue) causes the sequence of tests to fail.

> Always build from the master repo. 1.10 is pretty old.

It's what I have - remember, not everyone is happy to constantly replace
their distro packages with random new stuff.

> >> In any case, it doesn't look like the buffer management is being
> >> tested at all by v4l2-compliance - we know that gstreamer works, so
> >> buffers _can_ be allocated, and I've also used dmabufs with gstreamer,
> >> so I also know that VIDIOC_EXPBUF works there.
> 
> To test actual streaming you need to provide the -s option.
> 
> Note: v4l2-compliance has been developed for 'regular' video devices,
> not MC devices. It may or may not work with the -s option.

Right, and it exists to verify that the establised v4l2 API is correctly
implemented.  If the v4l2 API is being offered to user applications,
then it must be conformant, otherwise it's not offering the v4l2 API.
(That's very much a definition statement in itself.)

So, are we really going to say MC devices do not offer the v4l2 API to
userspace, but something that might work?  We've already seen today
one user say that they're not going to use mainline because of the
crud surrounding MC.

-- 
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 v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 12:55:26PM +0100, Philipp Zabel wrote:
> The above paragraph suggests we skip any rectangles that are not
> supported. In our case that would be 3. and 4., since the CSI can't
> compose into a larger frame. I hadn't realised that the crop selection
> currently happens on the source pad.

I'd recommend viewing the documentation in its post-processed version,
because then you get the examples as pictures, and they say that a
picture is worth 1000 words.  See

  https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/dev-subdev.html

There is almost an exact example of what we're trying to do - it's
figure 4.6.  Here, we have a sink pad with a cropping rectangle on
the input, which is then scaled to a composition rectangle (there's
no bounds rectangle, and it's specified that in such a case the
top,left of the composition rectangle will always be 0,0 - see quote
below).

Where it differs is that the example also supports source cropping
for two source pads.  We don't support that.

The same document says:

  Scaling support is optional. When supported by a subdev, the crop
  rectangle on the subdev's sink pad is scaled to the size configured
  using the
  :ref:`VIDIOC_SUBDEV_S_SELECTION ` IOCTL
  using ``V4L2_SEL_TGT_COMPOSE`` selection target on the same pad. If the
  subdev supports scaling but not composing, the top and left values are
  not used and must always be set to zero.

which in itself describes _exactly_ our hardware here as far as the
cropping and scaling the hardware supports.

> Except when composing is not supported. If the sink compose and source
> crop rectangles are not supported, the source pad format takes their
> place in determining the scaling output resolution. At least that's how
> I read the documentation.

This isn't how I read it.  Scaling is the relationship between the size
of the sink crop and sink compose rectangle.  Composition requires that
there be a composition bounds rectangle to define the composition space,
and the top,left of the composition rectangle be adjustable to place
the composition rectangle within that space.

The above quoted paragraph from the documentation backs up my view in
its final sentence - it doesn't say "if the subdev supports scaling
but not composing, there is no composition rectangle" but says that
there _is_ one but its top,left coordinates are always zero.

-- 
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 4/4] media: imx-media-capture: add frame sizes/interval enumeration

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 10:23:30AM +0100, Philippe De Muyter wrote:
> So existing gstreamer applications using /dev/video* to control framerate,
> and even gain and exposure won't work anymore :( ?
> 
> I had hoped to keep compatibility, with added robustness and functionality.
> 
> I seems like I'll stay with my NXP/Freescale old and imperfect kernel.

Thank you for saying this, this supports my views which I've already
stated about what influences which kernel people will use.

-- 
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 4/4] media: imx-media-capture: add frame sizes/interval enumeration

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 09:55:12AM +0100, Philippe De Muyter wrote:
> Hi Russel,
> 
> On Sun, Mar 19, 2017 at 10:49:08AM +, Russell King wrote:
> > Add support for enumerating frame sizes and frame intervals from the
> > first subdev via the V4L2 interfaces.
> > 
> > Signed-off-by: Russell King 
> > ---
> >  drivers/staging/media/imx/imx-media-capture.c | 62 
> > +++
> >  1 file changed, 62 insertions(+)
> > 
> ...
> > +static int capture_enum_frameintervals(struct file *file, void *fh,
> > +  struct v4l2_frmivalenum *fival)
> > +{
> > +   struct capture_priv *priv = video_drvdata(file);
> > +   const struct imx_media_pixfmt *cc;
> > +   struct v4l2_subdev_frame_interval_enum fie = {
> > +   .index = fival->index,
> > +   .pad = priv->src_sd_pad,
> > +   .width = fival->width,
> > +   .height = fival->height,
> > +   .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +   };
> > +   int ret;
> > +
> > +   cc = imx_media_find_format(fival->pixel_format, CS_SEL_ANY, true);
> > +   if (!cc)
> > +   return -EINVAL;
> > +
> > +   fie.code = cc->codes[0];
> > +
> > +   ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval, NULL, 
> > );
> > +   if (ret)
> > +   return ret;
> > +
> > +   fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> > +   fival->discrete = fie.interval;
> 
> For some parallel sensors (mine is a E2V ev76c560) "any" frame interval is 
> possible,
> and hence type should be V4L2_FRMIVAL_TYPE_CONTINUOUS.

For my sensor, any frame interval is also possible, but that isn't the
point here.

/dev/video* only talks to the CSI source pad, not it's sink pad.  The
sink pad gets configured with the sensor frame rate via the media
controller API.  /dev/video* itself has no control over the sensor
frame rate.

The media controller stuff completely changes the way the established
/dev/video* functionality works - the ability to select arbitary frame
sizes and frame rates supported by the ultimate sensor is gone.  All
that needs to be setup through the media controller pipeline, one
subdev at a time.

-- 
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 4/4] media: imx-media-capture: add frame sizes/interval enumeration

[Russell King - ARM Linux]

On Sun, Mar 19, 2017 at 03:21:37PM -0700, Steve Longerbeam wrote:
> 
> 
> On 03/19/2017 03:49 AM, Russell King wrote:
> >Add support for enumerating frame sizes and frame intervals from the
> >first subdev via the V4L2 interfaces.
> >
> >Signed-off-by: Russell King 
> >---
> >  drivers/staging/media/imx/imx-media-capture.c | 62 
> > +++
> >  1 file changed, 62 insertions(+)
> >
> >diff --git a/drivers/staging/media/imx/imx-media-capture.c 
> >b/drivers/staging/media/imx/imx-media-capture.c
> >index cdeb2cd8b1d7..bc99d9310e36 100644
> >--- a/drivers/staging/media/imx/imx-media-capture.c
> >+++ b/drivers/staging/media/imx/imx-media-capture.c
> >@@ -82,6 +82,65 @@ static int vidioc_querycap(struct file *file, void *fh,
> > return 0;
> >  }
> >+static int capture_enum_framesizes(struct file *file, void *fh,
> >+   struct v4l2_frmsizeenum *fsize)
> >+{
> >+struct capture_priv *priv = video_drvdata(file);
> >+const struct imx_media_pixfmt *cc;
> >+struct v4l2_subdev_frame_size_enum fse = {
> >+.index = fsize->index,
> >+.pad = priv->src_sd_pad,
> >+.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >+};
> >+int ret;
> >+
> >+cc = imx_media_find_format(fsize->pixel_format, CS_SEL_ANY, true);
> >+if (!cc)
> >+return -EINVAL;
> >+
> >+fse.code = cc->codes[0];
> >+
> >+ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_size, NULL, );
> >+if (ret)
> >+return ret;
> >+
> >+fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> >+fsize->discrete.width = fse.min_width;
> >+fsize->discrete.height = fse.max_height;
> >+
> >+return 0;
> >+}
> 
> 
> The PRP ENC/VF subdevices will return a continuous range of
> supported frame sizes at their source pad, so this should be
> modified to:
> 
> ...
> if (fse.min_width == fse.max_width &&
> fse.min_height == fse.max_height) {
> fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> fsize->discrete.width = fse.min_width;
> fsize->discrete.height = fse.min_height;
> } else {
> fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
> fsize->stepwise.min_width = fse.min_width;
> fsize->stepwise.max_width = fse.max_width;
> fsize->stepwise.min_height = fse.min_height;
> fsize->stepwise.max_height = fse.max_height;
> fsize->stepwise.step_width = 1;
> fsize->stepwise.step_height = 1;
> }
> ...

Fine by me - I don't have any experience of those subdevices as they're
unusable for me.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 19, 2017 at 11:37:15AM -0700, Steve Longerbeam wrote:
> On 03/19/2017 05:14 AM, Russell King - ARM Linux wrote:
> >Right now, CSI doesn't do that - it only looks at the width, height,
> >code, and field.
> 
> Correct, there is currently no propagation of the colorimetry
> parameters (colorspace, ycbcr_enc, quantization, and xfer_func).
> For the most part, those are just ignored ATM. Philipp Zabel did
> do some work earlier to start propagating those, but that's still
> TODO.


> 
> >
> >I think we've got other bugs though that haven't been picked up by any
> >review - csi_try_fmt() adjusts the format using the _current_
> >configuration of the sink pad, even when using V4L2_SUBDEV_FORMAT_TRY.
> >This seems wrong according to the docs: the purpose of the try
> >mechanism is to be able to setup the _entire_ pipeline using the TRY
> >mechanism to work out whether the configuration works, before then
> >setting for real.  If we're validating the TRY formats against the
> >live configuration, then we're not doing that.
> 
> I don't believe that is correct. csi_try_fmt() for the source pads calls
> __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which) to get
> the sink format, and for the TRY trial-run from csi_set_fmt(),
> sdformat->which will be set to TRY, so the returned sink format
> is the TRY format.

Look at csi_try_fmt() - it validates the source pad against
priv->crop, which is the actively live cropping rectangle, not the
one which has been configured for the TRY trial-run.

Also, as I mention elsewhere, I believe the way we're doing scaling
is completely wrong...

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 19, 2017 at 10:54:22AM -0700, Steve Longerbeam wrote:
> 
> 
> On 03/19/2017 03:38 AM, Russell King - ARM Linux wrote:
> >On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> >>Right, imx-media-capture.c (the "standard" v4l2 user interface module)
> >>is not implementing VIDIOC_ENUM_FRAMESIZES. It should, but it can only
> >>return the single frame size that the pipeline has configured (the mbus
> >>format of the attached source pad).
> >I now have a set of patches that enumerate the frame sizes and intervals
> >from the source pad of the first subdev (since you're setting the formats
> >etc there from the capture device, it seems sensible to return what it
> >can support.)  This means my patch set doesn't add to non-CSI subdevs.
> >
> >>Can you share your gstreamer pipeline? For now, until
> >>VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
> >>does not attempt to specify a frame rate. I use the attached
> >>script for testing, which works for me.
> >Note that I'm not specifying a frame rate on gstreamer - I'm setting
> >the pipeline up for 60fps, but gstreamer in its wisdom is unable to
> >enumerate the frame sizes, and therefore is unable to enumerate the
> >frame intervals (frame intervals depend on frame sizes), so it
> >falls back to the "tvnorms" which are basically 25/1 and 3/1001.
> >
> >It sees 60fps via G_PARM, and then decides to set 3/1001 via S_PARM.
> >So, we end up with most of the pipeline operating at 60fps, with CSI
> >doing frame skipping to reduce the frame rate to 30fps.
> >
> >gstreamer doesn't complain, doesn't issue any warnings, the only way
> >you can spot this is to enable debugging and look through the copious
> >debug log, or use -v and check the pad capabilities.
> >
> >Testing using gstreamer, and only using "does it produce video" is a
> >good simple test, but it's just that - it's a simple test.  It doesn't
> >tell you that what you're seeing is what you intended to see (such as
> >video at the frame rate you expected) without more work.
> >
> >>Thanks, I've fixed most of v4l2-compliance issues, but this is not
> >>done yet. Is that something you can help with?
> >What did you do with:
> >
> >ioctl(3, VIDIOC_REQBUFS, {count=0, type=0 /* V4L2_BUF_TYPE_??? */, memory=0 
> >/* V4L2_MEMORY_??? */}) = -1 EINVAL (Invalid argument)
> > test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> >ioctl(3, VIDIOC_EXPBUF, 0xbef405bc) = -1 EINVAL (Invalid argument)
> > fail: v4l2-test-buffers.cpp(571): q.has_expbuf(node)
> > test VIDIOC_EXPBUF: FAIL
> >
> >To me, this looks like a bug in v4l2-compliance (I'm using 1.10.0).
> >I'm not sure what buffer VIDIOC_EXPBUF is expected to export, since
> >afaics no buffers have been allocated, so of course it's going to fail.
> >Either that, or the v4l2 core vb2 code is non-compliant with v4l2's
> >interface requirements.
> >
> >In any case, it doesn't look like the buffer management is being
> >tested at all by v4l2-compliance - we know that gstreamer works, so
> >buffers _can_ be allocated, and I've also used dmabufs with gstreamer,
> >so I also know that VIDIOC_EXPBUF works there.
> >
> 
> I wouldn't be surprised if you hit on a bug in v4l2-compliance. I stopped
> with v4l2-compliance
> at a different test failure that also didn't make sense to me:

It isn't - the problem is that the results are misleading.  The
VIDIOC_REQBUFS depends on the GET_FMT test succeeding, so it knows
which buffer formats are valid.

Since the GET_FMT test fails due to the colorspace issue, it decides
that it can't trust the format, so it ends up with no formats to test.
This causes the "VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF" test to pass,
but then it moves on to testing "VIDIOC_EXPBUF" with no available
buffers, which then fails.

Fixing GET_FMT (which I've done locally) to return proper colorspace
information results in GET_FMT passing, and also solves the EXPBUF
problem too.

-- 
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 v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt

[Russell King - ARM Linux]

On Thu, Mar 09, 2017 at 08:53:18PM -0800, Steve Longerbeam wrote:
> From: Philipp Zabel 
> 
> The csi_try_crop call in set_fmt should compare the cropping rectangle
> to the currently set input format, not to the previous input format.

Are we really sure that the cropping support is implemented correctly?

I came across this while looking at what we're doing with the
V4L2_SEL_FLAG_KEEP_CONFIG flag.

Documentation/media/uapi/v4l/dev-subdev.rst defines the behaviour of
the user API, and "Order of configuration and format propagation" says:

  The coordinates to a step always refer to the actual size of the
  previous step. The exception to this rule is the source compose
  rectangle, which refers to the sink compose bounds rectangle --- if it
  is supported by the hardware.
  
  1. Sink pad format. The user configures the sink pad format. This format
 defines the parameters of the image the entity receives through the
 pad for further processing.
  
  2. Sink pad actual crop selection. The sink pad crop defines the crop
 performed to the sink pad format.
  
  3. Sink pad actual compose selection. The size of the sink pad compose
 rectangle defines the scaling ratio compared to the size of the sink
 pad crop rectangle. The location of the compose rectangle specifies
 the location of the actual sink compose rectangle in the sink compose
 bounds rectangle.
  
  4. Source pad actual crop selection. Crop on the source pad defines crop
 performed to the image in the sink compose bounds rectangle.
  
  5. Source pad format. The source pad format defines the output pixel
 format of the subdev, as well as the other parameters with the
 exception of the image width and height. Width and height are defined
 by the size of the source pad actual crop selection.
  
  Accessing any of the above rectangles not supported by the subdev will
  return ``EINVAL``. Any rectangle referring to a previous unsupported
  rectangle coordinates will instead refer to the previous supported
  rectangle. For example, if sink crop is not supported, the compose
  selection will refer to the sink pad format dimensions instead.

Note step 3 above: scaling is defined by the ratio of the _sink_ crop
rectangle to the _sink_ compose rectangle.

So, lets say that the camera produces a 1280x720 image, and the sink
pad format is configured with 1280x720.  That's step 1.

The sink crop operates within that rectangle, cropping it to an area.
Let's say we're only interested in its centre, so we'd chose 640x360
with the top-left as 320,180.  This is step 2.

Then, if we want to down-scale by a factor of two, we'd set the sink
compose selection to 320x180.

This seems to be at odds with how the scaling is done in CSI at
present: the selection implementations all reject attempts to
configure the sink pad, instead only supporting crop rectangles on
the source, and we use the source crop rectangle to define the
down-scaling.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 19, 2017 at 05:00:08PM +0200, Vladimir Zapolskiy wrote:
> On 03/19/2017 04:22 PM, Russell King - ARM Linux wrote:
> > On Sun, Mar 19, 2017 at 02:21:10PM +, Russell King - ARM Linux wrote:
> >> There's a good reason why I dumped a full debug log using GST_DEBUG=*:9,
> >> analysed it for the cause of the failure, and tried several different
> >> pipelines, including the standard bayer2rgb plugin.
> >>
> >> Please don't blame this on random stuff after analysis of the logs _and_
> >> reading the appropriate plugin code has shown where the problem is.  I
> >> know gstreamer can be very complex, but it's very possible to analyse
> >> the cause of problems and pin them down with detailed logs in conjunction
> >> with the source code.
> > 
> > Oh, and the proof of correct analysis is that fixing the kernel capture
> > driver to enumerate the frame sizes and intervals fixes the issue, even
> > with bayer2rgbneon being used.
> > 
> > Therefore, there is _no way_ what so ever that it could be caused by that
> > plugin.
> > 
> 
> Hey, no blaming of the unknown to me bayer2rgbneon element from my side,
> I've just asked an innocent question, thanks for reply. I failed to find
> the source code of the plugin, I was interested to compare its performance
> and features with mine in-house NEON powered RGGB/BGGR to RGB24 GStreamer
> conversion element, which is written years ago. My question was offtopic.

If you wanted to know where to get it from, you should've asked that.
You can find all the bits here:

https://git.phytec.de/

You need bayer2rgb-neon and gst-bayer2rgb-neon, and it requires some
fixes to the configure script and Makefiles get it to build if you
don't have gengenopt available.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 19, 2017 at 10:33:25AM -0400, Nicolas Dufresne wrote:
> Le dimanche 19 mars 2017 à 00:54 +0000, Russell King - ARM Linux a
> écrit :
> > > 
> > > In practice, I have the impression there is a fair reason why
> > > framerate
> > > enumeration isn't implemented (considering there is only 1 valid
> > > rate).
> > 
> > That's actually completely incorrect.
> > 
> > With the capture device interfacing directly with CSI, it's possible
> > _today_ to select:
> > 
> > * the CSI sink pad's resolution
> > * the CSI sink pad's resolution with the width and/or height halved
> > * the CSI sink pad's frame rate
> > * the CSI sink pad's frame rate divided by the frame drop factor
> > 
> > To put it another way, these are possible:
> > 
> > # v4l2-ctl -d /dev/video10 --list-formats-ext
> > ioctl: VIDIOC_ENUM_FMT
> >     Index   : 0
> >     Type    : Video Capture
> >     Pixel Format: 'RGGB'
> >     Name    : 8-bit Bayer RGRG/GBGB
> >     Size: Discrete 816x616
> >     Interval: Discrete 0.040s (25.000 fps)
> >     Interval: Discrete 0.048s (20.833 fps)
> >     Interval: Discrete 0.050s (20.000 fps)
> >     Interval: Discrete 0.053s (18.750 fps)
> >     Interval: Discrete 0.060s (16.667 fps)
> >     Interval: Discrete 0.067s (15.000 fps)
> >     Interval: Discrete 0.080s (12.500 fps)
> >     Interval: Discrete 0.100s (10.000 fps)
> >     Interval: Discrete 0.120s (8.333 fps)
> >     Interval: Discrete 0.160s (6.250 fps)
> >     Interval: Discrete 0.200s (5.000 fps)
> >     Interval: Discrete 0.240s (4.167 fps)
> >     Size: Discrete 408x616
> > 
> >     Size: Discrete 816x308
> > 
> >     Size: Discrete 408x308
> > 
> > 
> > These don't become possible as a result of implementing the enums,
> > they're all already requestable through /dev/video10.
> 
> Ok that wasn't clear. So basically video9 is a front-end to video10,
> and it does not proxy the enumerations.

No.  We've sent .dot graphs which show the structure of the imx capture
driver.

What we have wrt video nodes is (eg):

sensor --->  csi2 > mux ---> csi +--> csi capture
subdev  subdevsubdevsubdev   |   /dev/video10
 |
 +-\
 |  \
 +--> vdic ---> ic_prpenc ---> ic_prpenc
 subdev subdev capture

... etc ... for full details, see the .dot diagrams that have been
sent (sorry I can't recall where they are in the threads.)

> I understand this is what you
> are now fixing. And this has to be fixed, because I can image cases
> where the front-end could support only a subset of the sub-dev. So
> having userspace enumerate on another device (and having to find this
> device by walking the tree) is unlikely to work in all scenarios.

The capture blocks (imx-media-capture) all talk to their immediate
upstream subdev and configure its source pad according to the formats,
frame size and frame interval requested by the capture application.
The subdev source pad decides whether the request is valid, and allows
it, modifies it or rejects it as appropriate.

Without working enumeration support, there's no way for an application
to find out what possible settings there are, and, as I've already
explained, the CSI subdev is capable itself of two things:

* Scaling down the image by a factor of two independently in the
  horizontal and vertical directions
* Deterministically dropping frames received from its upstream
  element, thereby reducing the frame rate.

> p.s. This is why caps negotiation is annoyingly complex in GStreamer,
> specially that there is no shortcut, you connect pads, and they figure-
> out what format they will use between each other.

Right, so when you specify video/x-raw,...,framerate=60/1 it introduces
a new element which has one source and sink pad, which only supports
the specification given.  If the neighbour's pad doesn't support it,
gstreamer fails because the caps negotiation fails.

So, if v4l2src believes (via the tvnorms, because it's lacking any
other information) that the capture device can only do 25fps and
30fps, then trying to set 60fps _even if S_PARM may accept it_ will
cause gstreamer to fail - because v4l2src can only advertise that
it supports a source of 25fps and 30fps.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 19, 2017 at 02:21:10PM +, Russell King - ARM Linux wrote:
> There's a good reason why I dumped a full debug log using GST_DEBUG=*:9,
> analysed it for the cause of the failure, and tried several different
> pipelines, including the standard bayer2rgb plugin.
> 
> Please don't blame this on random stuff after analysis of the logs _and_
> reading the appropriate plugin code has shown where the problem is.  I
> know gstreamer can be very complex, but it's very possible to analyse
> the cause of problems and pin them down with detailed logs in conjunction
> with the source code.

Oh, and the proof of correct analysis is that fixing the kernel capture
driver to enumerate the frame sizes and intervals fixes the issue, even
with bayer2rgbneon being used.

Therefore, there is _no way_ what so ever that it could be caused by that
plugin.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 19, 2017 at 03:57:56PM +0200, Vladimir Zapolskiy wrote:
> Hi Russell,
> 
> On 03/18/2017 10:43 PM, Russell King - ARM Linux wrote:
> > On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> >> Can you share your gstreamer pipeline? For now, until
> >> VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
> >> does not attempt to specify a frame rate. I use the attached
> >> script for testing, which works for me.
> > 
> > It's nothing more than
> > 
> >   gst-launch-1.0 -v v4l2src !  ! xvimagesink
> > 
> > in my case, the conversions are bayer2rgbneon.  However, this only shows
> > you the frame rate negotiated on the pads (which is actually good enough
> > to show the issue.)
> 
> I'm sorry for potential offtopic, but is bayer2rgbneon element found in
> any officially supported by GStreamer plugin?

No it isn't.  Google is wonderful, please make use of planetary search
facilities.

> Can it be a point of failure?

There's a good reason why I dumped a full debug log using GST_DEBUG=*:9,
analysed it for the cause of the failure, and tried several different
pipelines, including the standard bayer2rgb plugin.

Please don't blame this on random stuff after analysis of the logs _and_
reading the appropriate plugin code has shown where the problem is.  I
know gstreamer can be very complex, but it's very possible to analyse
the cause of problems and pin them down with detailed logs in conjunction
with the source code.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> On 03/18/2017 12:22 PM, Russell King - ARM Linux wrote:
> >0:00:01.955927879 20954  0x15ffe90 INFOv4l2 
> >gstv4l2object.c:3811:gst_v4l2_object_get_caps: probed caps: 
> >video/x-bayer, format=(string)rggb, framerate=(fraction)3/1001, 
> >width=(int)816, height=(int)616, pixel-aspect-ratio=(fraction)1/1; 
> >video/x-raw, format=(string)I420, framerate=(fraction)3/1001, 
> >width=(int)816, height=(int)616, interlace-mode=(string)progressive, 
> >pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)YV12, 
> >framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
> >interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; 
> >video/x-raw, format=(string)BGR, framerate=(fraction)3/1001, 
> >width=(int)816, height=(int)616, interlace-mode=(string)progressive, 
> >pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)RGB, 
> >framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
> >interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1
> >
> >despite the media pipeline actually being configured for 60fps.
> >
> >Forcing it by adjusting the pipeline only results in gstreamer
> >failing, because it believes that v4l2 is unable to operate at
> >60fps.
> >
> >Also note the complaints from v4l2src about the non-compliance...
> 
> Thanks, I've fixed most of v4l2-compliance issues, but this is not
> done yet. Is that something you can help with?

I've looked at this, and IMHO it's yet another media control API mess.

- media-ctl itself allows setting the format on subdev pads via
  struct v4l2_subdev_format.

- struct v4l2_subdev_format contains a struct v4l2_mbus_framefmt.

- struct v4l2_mbus_framefmt contains:
  * @width:  frame width
  * @height: frame height
  * @code:   data format code (from enum v4l2_mbus_pixelcode)
  * @field:  used interlacing type (from enum v4l2_field)
  * @colorspace: colorspace of the data (from enum v4l2_colorspace)
  * @ycbcr_enc:  YCbCr encoding of the data (from enum v4l2_ycbcr_encoding)
  * @quantization: quantization of the data (from enum v4l2_quantization)
  * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func)

- media-ctl sets width, height, code and field, but nothing else.

We're already agreed that the fields that media-ctl are part of the
format negotiation between the ultimate source, flowing down to the
capture device.  However, there's no support in media-ctl to deal
with these other fields - so media-ctl in itself is only half-
implemented.

>From what I can tell, _we_ are doing the right thing in imx-media-capture.

However, I think part of the problem is the set_fmt implementation.
When a source pad is configured via set_fmt(), any fields that can
not be altered (eg, because the subdev doesn't support colorspace
conversion) need to be preserved from the subdev's sink pad.

Right now, CSI doesn't do that - it only looks at the width, height,
code, and field.

I think we've got other bugs though that haven't been picked up by any
review - csi_try_fmt() adjusts the format using the _current_
configuration of the sink pad, even when using V4L2_SUBDEV_FORMAT_TRY.
This seems wrong according to the docs: the purpose of the try
mechanism is to be able to setup the _entire_ pipeline using the TRY
mechanism to work out whether the configuration works, before then
setting for real.  If we're validating the TRY formats against the
live configuration, then we're not doing that.

There's calls for:

v4l2_subdev_get_try_format
v4l2_subdev_get_try_crop
v4l2_subdev_get_try_compose

to get the try configuration - we hardly make use of all of these.  I
would suggest that we change the approach to implementing the various
subdevs such that:

1) like __csi_get_fmt(), we have accessors that gets a pointer to the
   correct state for the TRY/live settings.

2) everywhere we're asked to get or set parameters that can be TRY/live,
   we use these accessors to retrieve a pointer to the correct state to
   not only read, but also modify.

3) when we're evaluating parameters against another pad, we use these
   accessors to obtain the other pad's configuration, rather than poking
   about in the state saved in the subdev's priv-> (which is irrelevant
   for the TRY variant.)

4) ensure that all parameters which the subdev itself does not support
   modification of are correctly propagated from the sink pad to all
   source pads, and are unable to be modified via the source pad.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> Right, imx-media-capture.c (the "standard" v4l2 user interface module)
> is not implementing VIDIOC_ENUM_FRAMESIZES. It should, but it can only
> return the single frame size that the pipeline has configured (the mbus
> format of the attached source pad).

I now have a set of patches that enumerate the frame sizes and intervals
from the source pad of the first subdev (since you're setting the formats
etc there from the capture device, it seems sensible to return what it
can support.)  This means my patch set doesn't add to non-CSI subdevs.

> Can you share your gstreamer pipeline? For now, until
> VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
> does not attempt to specify a frame rate. I use the attached
> script for testing, which works for me.

Note that I'm not specifying a frame rate on gstreamer - I'm setting
the pipeline up for 60fps, but gstreamer in its wisdom is unable to
enumerate the frame sizes, and therefore is unable to enumerate the
frame intervals (frame intervals depend on frame sizes), so it
falls back to the "tvnorms" which are basically 25/1 and 3/1001.

It sees 60fps via G_PARM, and then decides to set 3/1001 via S_PARM.
So, we end up with most of the pipeline operating at 60fps, with CSI
doing frame skipping to reduce the frame rate to 30fps.

gstreamer doesn't complain, doesn't issue any warnings, the only way
you can spot this is to enable debugging and look through the copious
debug log, or use -v and check the pad capabilities.

Testing using gstreamer, and only using "does it produce video" is a
good simple test, but it's just that - it's a simple test.  It doesn't
tell you that what you're seeing is what you intended to see (such as
video at the frame rate you expected) without more work.

> Thanks, I've fixed most of v4l2-compliance issues, but this is not
> done yet. Is that something you can help with?

What did you do with:

ioctl(3, VIDIOC_REQBUFS, {count=0, type=0 /* V4L2_BUF_TYPE_??? */, memory=0 /* 
V4L2_MEMORY_??? */}) = -1 EINVAL (Invalid argument)
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
ioctl(3, VIDIOC_EXPBUF, 0xbef405bc) = -1 EINVAL (Invalid argument)
fail: v4l2-test-buffers.cpp(571): q.has_expbuf(node)
test VIDIOC_EXPBUF: FAIL

To me, this looks like a bug in v4l2-compliance (I'm using 1.10.0).
I'm not sure what buffer VIDIOC_EXPBUF is expected to export, since
afaics no buffers have been allocated, so of course it's going to fail.
Either that, or the v4l2 core vb2 code is non-compliant with v4l2's
interface requirements.

In any case, it doesn't look like the buffer management is being
tested at all by v4l2-compliance - we know that gstreamer works, so
buffers _can_ be allocated, and I've also used dmabufs with gstreamer,
so I also know that VIDIOC_EXPBUF works there.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sat, Mar 18, 2017 at 08:41:14PM -0400, Nicolas Dufresne wrote:
> Along with the norm fallback, GStreamer could could also consider the
> currently set framerate as returned by VIDIOC_G_PARM. At the same time,
> implementing that enumeration shall be straightforward, and will make a
> large amount of existing userspace work.

Since, according to v4l2-compliance, providing the enumeration ioctls
appears to be optional:

1) should v4l2-compliance be checking whether other frame sizes/frame
   intervals are possible, and failing if the enumeration ioctls are
   not supported?

2) would it also make sense to allow gstreamer's v4l2src to try setting
   a these parameters, and only fail if it's unable to set it?  IOW, if
   I use:

gst-launch-1.0 v4l2src device=/dev/video10 ! \
video/x-bayer,format=RGGB,framerate=20/1 ! ...

   where G_PARM says its currently configured for 25fps, but a S_PARM
   with 20fps would actually succeed.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sat, Mar 18, 2017 at 08:41:14PM -0400, Nicolas Dufresne wrote:
> Le samedi 18 mars 2017 à 20:43 +0000, Russell King - ARM Linux a
> écrit :
> > On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> > > Can you share your gstreamer pipeline? For now, until
> > > VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
> > > does not attempt to specify a frame rate. I use the attached
> > > script for testing, which works for me.
> > 
> > It's nothing more than
> > 
> >   gst-launch-1.0 -v v4l2src !  ! xvimagesink
> > 
> > in my case, the conversions are bayer2rgbneon.  However, this only
> > shows
> > you the frame rate negotiated on the pads (which is actually good
> > enough
> > to show the issue.)
> > 
> > How I stumbled across though this was when I was trying to encode:
> > 
> >  gst-launch-1.0 v4l2src device=/dev/video9 ! bayer2rgbneon ! \
> > videoconvert ! x264enc speed-preset=1 ! avimux ! \
> > filesink location=test.avi
> > 
> > I noticed that vlc would always say it was playing the resulting AVI
> > at 30fps.
> 
> In practice, I have the impression there is a fair reason why framerate
> enumeration isn't implemented (considering there is only 1 valid rate).

That's actually completely incorrect.

With the capture device interfacing directly with CSI, it's possible
_today_ to select:

* the CSI sink pad's resolution
* the CSI sink pad's resolution with the width and/or height halved
* the CSI sink pad's frame rate
* the CSI sink pad's frame rate divided by the frame drop factor

To put it another way, these are possible:

# v4l2-ctl -d /dev/video10 --list-formats-ext
ioctl: VIDIOC_ENUM_FMT
Index   : 0
Type: Video Capture
Pixel Format: 'RGGB'
Name: 8-bit Bayer RGRG/GBGB
Size: Discrete 816x616
Interval: Discrete 0.040s (25.000 fps)
Interval: Discrete 0.048s (20.833 fps)
Interval: Discrete 0.050s (20.000 fps)
Interval: Discrete 0.053s (18.750 fps)
Interval: Discrete 0.060s (16.667 fps)
Interval: Discrete 0.067s (15.000 fps)
Interval: Discrete 0.080s (12.500 fps)
Interval: Discrete 0.100s (10.000 fps)
Interval: Discrete 0.120s (8.333 fps)
Interval: Discrete 0.160s (6.250 fps)
Interval: Discrete 0.200s (5.000 fps)
Interval: Discrete 0.240s (4.167 fps)
Size: Discrete 408x616

Size: Discrete 816x308

Size: Discrete 408x308


These don't become possible as a result of implementing the enums,
they're all already requestable through /dev/video10.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> Can you share your gstreamer pipeline? For now, until
> VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
> does not attempt to specify a frame rate. I use the attached
> script for testing, which works for me.

It's nothing more than

  gst-launch-1.0 -v v4l2src !  ! xvimagesink

in my case, the conversions are bayer2rgbneon.  However, this only shows
you the frame rate negotiated on the pads (which is actually good enough
to show the issue.)

How I stumbled across though this was when I was trying to encode:

 gst-launch-1.0 v4l2src device=/dev/video9 ! bayer2rgbneon ! \
videoconvert ! x264enc speed-preset=1 ! avimux ! \
filesink location=test.avi

I noticed that vlc would always say it was playing the resulting AVI
at 30fps.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

Hi Steve,

I've just been trying to get gstreamer to capture and h264 encode
video from my camera at various frame rates, and what I've discovered
does not look good.

1) when setting frame rates, media-ctl _always_ calls
   VIDIOC_SUBDEV_S_FRAME_INTERVAL with pad=0.

2) media-ctl never retrieves the frame interval information, so there's
   no way to read it back with standard tools, and no indication that
   this is going on...

3) gstreamer v4l2src is getting upset, because it can't enumerate the
   frame sizes (VIDIOC_ENUM_FRAMESIZES fails), which causes it to
   fallback to using the "tvnorms" to decide about frame rates.  This
   makes it impossible to use frame rates higher than 3/1001, and
   causes the pipeline validation to fail.

0:00:01.937465845 20954  0x15ffe90 DEBUG   v4l2 
gstv4l2object.c:2474:gst_v4l2_object_probe_caps_for_format: 
Enumerating frame sizes for RGGB
0:00:01.937588518 20954  0x15ffe90 DEBUG   v4l2 
gstv4l2object.c:2601:gst_v4l2_object_probe_caps_for_format: Failed to 
enumerate frame sizes for pixelformat RGGB (Inappropriate ioctl for device)
0:00:01.937879535 20954  0x15ffe90 LOG v4l2 
gstv4l2object.c:2708:gst_v4l2_object_get_nearest_size: getting 
nearest size to 1x1 with format RGGB
0:00:01.937990874 20954  0x15ffe90 LOG v4l2 
gstv4l2object.c:2724:gst_v4l2_object_get_nearest_size: got nearest 
size 816x616
0:00:01.938250889 20954  0x15ffe90 ERROR   v4l2 
gstv4l2object.c:1873:gst_v4l2_object_get_interlace_mode: Driver bug detected - 
check driver with v4l2-compliance from http://git.linuxtv.org/v4l-utils.git
0:00:01.938326893 20954  0x15ffe90 LOG v4l2 
gstv4l2object.c:2708:gst_v4l2_object_get_nearest_size: getting 
nearest size to 32768x32768 with format RGGB
0:00:01.938431566 20954  0x15ffe90 LOG v4l2 
gstv4l2object.c:2724:gst_v4l2_object_get_nearest_size: got nearest 
size 816x616
0:00:01.939776641 20954  0x15ffe90 ERROR   v4l2 
gstv4l2object.c:1873:gst_v4l2_object_get_interlace_mode: Driver bug detected - 
check driver with v4l2-compliance from http://git.linuxtv.org/v4l-utils.git
0:00:01.940110660 20954  0x15ffe90 DEBUG   v4l2 
gstv4l2object.c:1955:gst_v4l2_object_get_colorspace: Unknown enum 
v4l2_colorspace 0

   This triggers the "/* Since we can't get framerate directly, try to
   use the current norm */" code in v4l2object.c, which causes it to
   select one of the 3/1001 norms:

0:00:01.955927879 20954  0x15ffe90 INFOv4l2 
gstv4l2object.c:3811:gst_v4l2_object_get_caps: probed caps: 
video/x-bayer, format=(string)rggb, framerate=(fraction)3/1001, 
width=(int)816, height=(int)616, pixel-aspect-ratio=(fraction)1/1; video/x-raw, 
format=(string)I420, framerate=(fraction)3/1001, width=(int)816, 
height=(int)616, interlace-mode=(string)progressive, 
pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)YV12, 
framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; 
video/x-raw, format=(string)BGR, framerate=(fraction)3/1001, 
width=(int)816, height=(int)616, interlace-mode=(string)progressive, 
pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)RGB, 
framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1

   despite the media pipeline actually being configured for 60fps.

   Forcing it by adjusting the pipeline only results in gstreamer
   failing, because it believes that v4l2 is unable to operate at
   60fps.

   Also note the complaints from v4l2src about the non-compliance...

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

[Russell King - ARM Linux]

On Fri, Mar 17, 2017 at 02:51:10PM +0100, Philipp Zabel wrote:
> On Fri, 2017-03-17 at 10:24 -0300, Mauro Carvalho Chehab wrote:
> [...]
> > The big question, waiting for an answer on the last 8 years is
> > who would do that? Such person would need to have several different
> > hardware from different vendors, in order to ensure that it has
> > a generic solution.
> > 
> > It is a way more feasible that the Kernel developers that already 
> > have a certain hardware on their hands to add support inside the
> > driver to forward the controls through the pipeline and to setup
> > a "default" pipeline that would cover the common use cases at
> > driver's probe.
> 
> Actually, would setting pipeline via libv4l2 plugin and letting drivers
> provide a sane enabled default pipeline configuration be mutually
> exclusive? Not sure about the control forwarding, but at least a simple
> link setup and format forwarding would also be possible in the kernel
> without hindering userspace from doing it themselves later.

I think this is the exact same problem as controls in ALSA.

When ALSA started off in life, the requirement was that all controls
shall default to minimum, and the user is expected to adjust controls
after the system is running.

After OSS, this gave quite a marked change in system behaviour, and
led to a lot of "why doesn't my sound work anymore" problems, because
people then had to figure out which combination of controls had to be
set to get sound out of their systems.

Now it seems to be much better, where install Linux on a platform, and
you have a working sound system (assuming that the drivers are all there
which is generally the case for x86.)

However, it's still possible to adjust all the controls from userspace.
All that's changed is the defaults.

Why am I mentioning this - because from what I understand Mauro saying,
it's no different from this situation.  Userspace will still have the
power to disable all links and setup its own.  The difference is that
there will be a default configuration that the kernel sets up at boot
time that will be functional, rather than the current default
configuration where the system is completely non-functional until
manually configured.

However, at the end of the day, I don't care _where_ the usability
problems are solved, only that there is some kind of solution.  It's not
the _where_ that's the real issue here, but the _how_, and discussion of
the _how_ is completely missing.

So, let's try kicking off a discussion about _how_ to do things.

_How_ do we setup a media controller system so that we end up with a
usable configuration - let's start with the obvious bit... which links
should be enabled.

I think the first pre-requisit is that we stop exposing capture devices
that can never be functional for the hardware that's present on the board,
so that there isn't this plentora of useless /dev/video* nodes and useless
subdevices.

One possible solution to finding a default path may be "find the shortest
path between the capture device and the sensor and enable intervening
links".

Then we need to try configuring that path with format/resolution
information.

However, what if something in the shortest path can't handle the format
that the sensor produces?  I think at that point, we'd need to drop that
subdev out of the path resolution, re-run the "find the shortest path"
algorithm, and try again.

Repeat until success or no path between the capture and sensor exists.

This works fine if you have just one sensor visible from a capture device,
but not if there's more than one (which I suspect is the case with the
Sabrelite board with its two cameras and video receiver.)  That breaks
the "find the shortest path" algorithm.

So, maybe it's a lot better to just let the board people provide via DT
a default setup for the connectivity of the modules somehow - certainly
one big step forward would be to disable in DT parts of the capture
system that can never be used (remembering that boards like the RPi /
Hummingboard may end up using DT overlays to describe this for different
cameras, so the capture setup may change after initial boot.)

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

[Russell King - ARM Linux]

On Fri, Mar 17, 2017 at 01:02:07PM +0100, Philipp Zabel wrote:
> I think most of the simple, fixed pipeline use cases could be handled by
> libv4l2, by allowing to pass a v4l2 subdevice path to v4l2_open. If that
> function internally would set up the media links to the
> nearest /dev/video interface, propagate format, resolution and frame
> intervals if necessary, and return an fd to the video device, there'd be
> no additional complexity for the users beyond selecting the v4l2_subdev
> instead of the video device.

... which would then require gstreamer to be modified too. The gstreamer
v4l2 plugin looks for /dev/video* or /dev/v4l2/video* devices and monitors
these for changes, so gstreamer applications know which capture devices
are available:

  const gchar *paths[] = { "/dev", "/dev/v4l2", NULL };
  const gchar *names[] = { "video", NULL };

  /* Add some depedency, so the dynamic features get updated upon changes in
   * /dev/video* */
  gst_plugin_add_dependency (plugin,
  NULL, paths, names, GST_PLUGIN_DEPENDENCY_FLAG_FILE_NAME_IS_PREFIX);

I haven't checked yet whether sys/v4l2/gstv4l2deviceprovider.c knows
anything about the v4l2 subdevs.

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

[Russell King - ARM Linux]

On Tue, Mar 14, 2017 at 08:55:36AM +0100, Hans Verkuil wrote:
> We're all very driver-development-driven, and userspace gets very little
> attention in general. So before just throwing in the towel we should take
> a good look at the reasons why there has been little or no development: is
> it because of fundamental design defects, or because nobody paid attention
> to it?
> 
> I strongly suspect it is the latter.
> 
> In addition, I suspect end-users of these complex devices don't really care
> about a plugin: they want full control and won't typically use generic
> applications. If they would need support for that, we'd have seen much more
> interest. The main reason for having a plugin is to simplify testing and
> if this is going to be used on cheap hobbyist devkits.

I think you're looking at it with a programmers hat on, not a users hat.

Are you really telling me that requiring users to 'su' to root, and then
use media-ctl to manually configure the capture device is what most
users "want" ?

Hasn't the way technology has moved towards graphical interfaces,
particularly smart phones, taught us that the vast majority of users
want is intuitive, easy to use interfaces, and not the command line
with reams of documentation?

Why are smart phones soo popular - it's partly because they're flashy,
but also because of the wealth of apps, and apps which follow the
philosophy of "do one job, do it well" (otherwise they get bad reviews.)

> An additional complication is simply that it is hard to find fully supported
> MC hardware. omap3 boards are hard to find these days, renesas boards are not
> easy to get, freescale isn't the most popular either. Allwinner, mediatek,
> amlogic, broadcom and qualcomm all have closed source implementations or no
> implementation at all.

Right, and that in itself tells us something - the problem that we're
trying to solve is not one that commonly exists in the real world.

Yes, the hardware we have in front of us may be very complex, but if
there's very few systems out there which are capable of making use of
all that complexity, then we're trying to solve a problem that isn't
the common case - and if it's going to take years to solve it (it
already has taken years) then it's the wrong problem to be solved.

I bet most of the problem can be eliminated if, rather than exposing
all this complexity, we instead expose a simpler capture system where
the board designer gets to "wire up" the capture system.

I'll go back to my Bayer example, because that's the simplest.  As
I've already said many times in these threads, there is only one
possible path through the iMX6 device that such a source can be used
with - it's a fixed path.  The actual path depends on the CSI2
virtual channel that the camera has been _configured_ to use, but
apart from that, it's effectively a well known set of blocks.  Such
a configuration could be placed in DT.

For RGB connected to a single parallel CSI, things get a little more
complex - capture through the CSI or through two other capture devices
for de-interlacing or other features.  However, I'm not convinced that
exposing multiple /dev/video* devices for different features for the
same video source is a sane approach - I think that's a huge usability
problem.  (The user is expected to select the capture device on iMX6
depending on the features they want, and if they want to change features,
they're expected to shut down their application and start it up on a
different capture device.)  For the most part on iMX6, there's one
path down to the CSI block, and then there's optional routing through
the rest of the IPU depending on what features you want (such as
de-interlacing.)

The complex case is a CSI2 connected camera which produces multiple
streams through differing virtual channels - and that's IMHO the only
case where we need multiple different /dev/video* capture devices to
be present.

-- 
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 v5 15/39] [media] v4l2: add a frame interval error event

[Russell King - ARM Linux]

On Tue, Mar 14, 2017 at 12:21:31PM -0400, Nicolas Dufresne wrote:
> My main concern here based on what I'm reading, is that this driver is
> not even able to notice immediately that a produced frame was corrupted
> (because it's out of sync). From usability perspective, this is really
> bad. Can't the driver derive a clock from some irq and calculate for
> each frame if the timing was correct ? And if not mark the buffer with
> V4L2_BUF_FLAG_ERROR ?

One of the issues of measuring timing with IRQs is the fact that the
IRQ subsystem only allows one IRQ to run at a time.  If an IRQ takes
a relatively long time to process, then it throws the timing of other
IRQs out.

If you're going to decide that a buffer should be marked in error on
the basis of an interrupt arriving late, this can trigger spuriously.

It wasn't that long ago that USB HID was regularly eating something
like 20ms of interrupt time... that's been solved, but that doesn't
mean all cases are solved - there are still interrupt handlers in the
kernel that are on the order of milliseconds to complete.

Given the quality I observe of some USB serial devices (eg, running at
115200 baud, but feeling like they deliver characters to userspace at
9600 baud) I wouldn't be surprised if some USB serial drivers eat a lot
of IRQ time... and if so, all it'll take is to plug such a device in
to disrupt capture.

That sounds way too fragile to me.

-- 
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 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: [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: [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: [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.

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.

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 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 &&

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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 12, 2017 at 05:59:28PM -0300, Mauro Carvalho Chehab wrote:
> Yet, udev/systemd has some rules that provide an unique name for V4L
> devices at /lib/udev/rules.d/60-persistent-v4l.rules. Basically, it
> runs a small application (v4l_id) with creates a persistent symling
> using rules like this:
> 
>   KERNEL=="video*", ENV{ID_SERIAL}=="?*", 
> SYMLINK+="v4l/by-id/$env{ID_BUS}-$env{ID_SERIAL}-video-index$attr{index}"
> 
> Those names are stored at /dev/v4l/by-path.

This doesn't help:

$ ls -Al /dev/v4l/by-id/
total 0
lrwxrwxrwx 1 root root 13 Mar 12 19:54 
usb-Sonix_Technology_Co.__Ltd._USB_2.0_Camera-video-index0 -> ../../video10
$ ls -Al /dev/v4l/by-path/
total 0
lrwxrwxrwx 1 root root 12 Mar 12 19:54 platform-204.vpu-video-index0 -> 
../../video0
lrwxrwxrwx 1 root root 12 Mar 12 19:54 platform-204.vpu-video-index1 -> 
../../video1
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index0 
-> ../../video2
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index1 
-> ../../video3
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index2 
-> ../../video4
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index3 
-> ../../video5
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index4 
-> ../../video6
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index5 
-> ../../video7
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index6 
-> ../../video8
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index7 
-> ../../video9
lrwxrwxrwx 1 root root 13 Mar 12 19:54 
platform-ci_hdrc.0-usb-0:1:1.0-video-index0 -> ../../video10

The problem is the "platform-capture-subsystem-video-index" entries.
These themselves change order.  For instance, I now have:

- entity 72: ipu1_csi0 capture (1 pad, 1 link)
 type Node subtype V4L flags 0
 device node name /dev/video6

which means it's platform-capture-subsystem-video-index4.  Before, it
was platform-capture-subsystem-video-index2.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 12, 2017 at 08:40:37PM +, Russell King - ARM Linux wrote:
> On Sun, Mar 12, 2017 at 01:36:32PM -0700, Steve Longerbeam wrote:
> > But hold on, if my logic is correct, then why did the CSI power-off
> > get reached in your case, multiple times? Yes I think there is a bug,
> > link_notify() is not checking if the link has already been disabled.
> > I will fix this. But I'm surprised media core's link_notify handling
> > doesn't do this.
> 
> Well, I think there's something incredibly fishy going on here.  I
> turned that dev_dbg() at the top of the function into a dev_info(),
> and I get:
> 
> root@hbi2ex:~# dmesg |grep -A2 imx-ipuv3-csi
> [   53.370949] imx-ipuv3-csi imx-ipuv3-csi.0: power OFF
> [   53.371015] [ cut here ]
> [   53.371075] WARNING: CPU: 0 PID: 1515 at 
> drivers/staging/media/imx/imx-media-csi.c:806 csi_s_power+0xb8/0xd0 
> [imx_media_csi]
> --
> [   53.372624] imx-ipuv3-csi imx-ipuv3-csi.0: power OFF
> [   53.372637] [ cut here ]
> [   53.372663] WARNING: CPU: 0 PID: 1515 at 
> drivers/staging/media/imx/imx-media-csi.c:806 csi_s_power+0xb8/0xd0 
> [imx_media_csi]
> 
> There isn't a power on event being generated before these two power
> off events.  I don't see a power on event even when I attempt to
> start streaming either (which fails due to the lack of bayer
> support.)

Found it - my imx219 driver returns '1' from its s_power function when
powering up, which triggers a bug in your code - when imx_media_set_power()
fails to power up, you call imx_media_set_power() telling it to power
everything off - including devices that are already powered off.

This is really bad news - s_power() may be called via other paths,
such as when the subdev is opened.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 12, 2017 at 01:36:32PM -0700, Steve Longerbeam wrote:
> But hold on, if my logic is correct, then why did the CSI power-off
> get reached in your case, multiple times? Yes I think there is a bug,
> link_notify() is not checking if the link has already been disabled.
> I will fix this. But I'm surprised media core's link_notify handling
> doesn't do this.

Well, I think there's something incredibly fishy going on here.  I
turned that dev_dbg() at the top of the function into a dev_info(),
and I get:

root@hbi2ex:~# dmesg |grep -A2 imx-ipuv3-csi
[   53.370949] imx-ipuv3-csi imx-ipuv3-csi.0: power OFF
[   53.371015] [ cut here ]
[   53.371075] WARNING: CPU: 0 PID: 1515 at 
drivers/staging/media/imx/imx-media-csi.c:806 csi_s_power+0xb8/0xd0 
[imx_media_csi]
--
[   53.372624] imx-ipuv3-csi imx-ipuv3-csi.0: power OFF
[   53.372637] [ cut here ]
[   53.372663] WARNING: CPU: 0 PID: 1515 at 
drivers/staging/media/imx/imx-media-csi.c:806 csi_s_power+0xb8/0xd0 
[imx_media_csi]

There isn't a power on event being generated before these two power
off events.  I don't see a power on event even when I attempt to
start streaming either (which fails due to the lack of bayer
support.)

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 12, 2017 at 01:05:06PM -0700, Steve Longerbeam wrote:
> 
> 
> On 03/12/2017 12:57 PM, Russell King - ARM Linux wrote:
> >On Sat, Mar 11, 2017 at 04:30:53PM -0800, Steve Longerbeam wrote:
> >>If it's too difficult to get the imx219 csi-2 transmitter into the
> >>LP-11 state on power on, perhaps the csi-2 receiver can be a little
> >>more lenient on the transmitter and make the LP-11 timeout a warning
> >>instead of error-out.
> >>
> >>Can you try the attached change on top of the version 5 patchset?
> >>
> >>If that doesn't work then you're just going to have to fix the bug
> >>in imx219.
> >
> >That patch gets me past that hurdle, only to reveal that there's another
> >issue:
> 
> Yeah, ipu_cpmem_set_image() failed because it doesn't recognize the
> bayer formats. Wait, didn't we fix this already? I've lost track.
> Ah, right, we were going to move this support into the IPUv3 driver,
> but in the meantime I think you had some patches to get around this.

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.

diff --git a/drivers/staging/media/imx/imx-smfc.c 
b/drivers/staging/media/imx/imx-smfc.c
index 313732201a52..4351c0365cf4 100644
--- a/drivers/staging/media/imx/imx-smfc.c
+++ b/drivers/staging/media/imx/imx-smfc.c
@@ -234,11 +234,6 @@ static void imx_smfc_setup_channel(struct imx_smfc_priv 
*priv)
buf1 = imx_media_dma_buf_get_next_queued(priv->out_ring);
priv->next = buf1;
 
-   image.phys0 = buf0->phys;
-   image.phys1 = buf1->phys;
-   ipu_cpmem_set_image(priv->smfc_ch, );
-
-
switch (image.pix.pixelformat) {
case V4L2_PIX_FMT_SBGGR8:
case V4L2_PIX_FMT_SGBRG8:
@@ -247,6 +242,10 @@ static void imx_smfc_setup_channel(struct imx_smfc_priv 
*priv)
burst_size = 8;
passthrough = true;
passthrough_bits = 8;
+   ipu_cpmem_set_resolution(priv->smfc_ch, image.rect.width, 
image.rect.height);
+   ipu_cpmem_set_stride(priv->smfc_ch, image.pix.bytesperline);
+   ipu_cpmem_set_buffer(priv->smfc_ch, 0, buf0->phys);
+   ipu_cpmem_set_buffer(priv->smfc_ch, 1, buf1->phys);
break;
 
case V4L2_PIX_FMT_SBGGR16:
@@ -256,9 +255,17 @@ static void imx_smfc_setup_channel(struct imx_smfc_priv 
*priv)
burst_size = 4;
passthrough = true;
passthrough_bits = 16;
+   ipu_cpmem_set_resolution(priv->smfc_ch, image.rect.width, 
image.rect.height);
+   ipu_cpmem_set_stride(priv->smfc_ch, image.pix.bytesperline);
+   ipu_cpmem_set_buffer(priv->smfc_ch, 0, buf0->phys);
+   ipu_cpmem_set_buffer(priv->smfc_ch, 1, buf1->phys);
break;
 
default:
+   image.phys0 = buf0->phys;
+   image.phys1 = buf1->phys;
+   ipu_cpmem_set_image(priv->smfc_ch, );
+
burst_size = (outfmt->width & 0xf) ? 8 : 16;
 
/*

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sat, Mar 11, 2017 at 04:30:53PM -0800, Steve Longerbeam wrote:
> If it's too difficult to get the imx219 csi-2 transmitter into the
> LP-11 state on power on, perhaps the csi-2 receiver can be a little
> more lenient on the transmitter and make the LP-11 timeout a warning
> instead of error-out.
> 
> Can you try the attached change on top of the version 5 patchset?
> 
> If that doesn't work then you're just going to have to fix the bug
> in imx219.

That patch gets me past that hurdle, only to reveal that there's another
issue:

imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
imx219 0-0010: VT: pixclk 13920Hz line 80742Hz frame 30.0Hz
imx219 0-0010: VT: line period 12385ns
imx219 0-0010: OP: pixclk 3850Hz, 2 lanes, 308Mbps peak each
imx219 0-0010: OP: 3288 bits/line/lane act=10675ns lp/idle=1710ns
ipu1_csi0: csi_idmac_setup failed: -22
ipu1_csi0: pipeline start failed with -22
[ cut here ]
WARNING: CPU: 0 PID: 1860 at 
/home/rmk/git/linux-rmk/drivers/media/v4l2-core/videobuf2-core.c:1340 
vb2_start_streaming+0x124/0x1b4 [videobuf2_core]

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

Another issue.

The "reboot and the /dev/video* devices come up in a completely
different order" problem seems to exist with this version.

The dot graph I supplied previously had "ipu1_csi0 capture" on
/dev/video4.  I've just rebooted, and now I find it's on
/dev/video2 instead.

Here's the extract from the .dot file of the old listing:

n0018 [label="ipu1_ic_prpenc capture\n/dev/video0", shape=box, 
style=filled, fillcolor=yellow]
n0021 [label="ipu1_ic_prpvf capture\n/dev/video1", shape=box, 
style=filled, fillcolor=yellow]
n002e [label="ipu2_ic_prpenc capture\n/dev/video2", shape=box, 
style=filled, fillcolor=yellow]
n0037 [label="ipu2_ic_prpvf capture\n/dev/video3", shape=box, 
style=filled, fillcolor=yellow]
n0048 [label="ipu1_csi0 capture\n/dev/video4", shape=box, 
style=filled, fillcolor=yellow]
n0052 [label="ipu1_csi1 capture\n/dev/video5", shape=box, 
style=filled, fillcolor=yellow]
n0062 [label="ipu2_csi0 capture\n/dev/video6", shape=box, 
style=filled, fillcolor=yellow]
n006c [label="ipu2_csi1 capture\n/dev/video7", shape=box, 
style=filled, fillcolor=yellow]

and here's the same after reboot:

n0014 [label="ipu1_csi0 capture\n/dev/video2", shape=box, 
style=filled, fillcolor=yellow]
n001e [label="ipu1_csi1 capture\n/dev/video3", shape=box, 
style=filled, fillcolor=yellow]
n0028 [label="ipu2_csi0 capture\n/dev/video4", shape=box, 
style=filled, fillcolor=yellow]
n0035 [label="ipu1_ic_prpenc capture\n/dev/video5", shape=box, 
style=filled, fillcolor=yellow]
n003e [label="ipu1_ic_prpvf capture\n/dev/video6", shape=box, 
style=filled, fillcolor=yellow]
n004c [label="ipu2_csi1 capture\n/dev/video7", shape=box, 
style=filled, fillcolor=yellow]
n0059 [label="ipu2_ic_prpenc capture\n/dev/video8", shape=box, 
style=filled, fillcolor=yellow]
n0062 [label="ipu2_ic_prpvf capture\n/dev/video9", shape=box, 
style=filled, fillcolor=yellow]

(/dev/video0 and /dev/video1 are taken up by CODA, since I updated the
names of the firmware files, and now CODA initialises... seems the
back-compat filenames don't work, but that's not a problem with imx6
capture.)

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 12, 2017 at 12:21:45PM -0700, Steve Longerbeam wrote:
> There's actually nothing preventing userland from disabling a link
> multiple times, and imx_media_link_notify() complies, and so
> csi_s_power(OFF) gets called multiple times, and so that WARN_ON()
> in there is silly, I borrowed this from other MC driver examples,
> but it makes no sense to me, I'll remove it and prevent the power
> count from going negative.

Hmm.  So what happens if one of the CSI's links is enabled, and we
disable a different link from the CSI several times?  Doesn't that
mean the power count will go to zero despite there being an enabled
link?

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

I've just looked at my test system's dmesg, and spotted this in the log.
It's been a while since these popped out of the kernel, so I don't know
what caused them (other than the obvious, a media-ctl command.)

My script which sets this up only enables links, and then configures the
formats etc, and doesn't disable them, so I don't see why the power
count should be going negative.

[ cut here ]
WARNING: CPU: 1 PID: 1889 at drivers/staging/media/imx/imx-media-csi.c:806 
csi_s_power+0x9c/0xa8 [imx_media_csi]
Modules linked in: caam_jr uvcvideo snd_soc_imx_sgtl5000 snd_soc_fsl_asoc_card 
snd_soc_imx_spdif imx_media_csi(C) imx6_mipi_csi2(C) snd_soc_imx_audmux 
snd_soc_sgtl5000 imx219 imx_media_ic(C) imx_media_capture(C) imx_media_vdic(C) 
caam video_multiplexer imx_sdma coda v4l2_mem2mem videobuf2_v4l2 imx2_wdt 
imx_vdoa videobuf2_dma_contig videobuf2_core videobuf2_vmalloc videobuf2_memops 
snd_soc_fsl_ssi
imx_thermal snd_soc_fsl_spdif imx_pcm_dma imx_media(C) imx_media_common(C) nfsd
rc_pinnacle_pctv_hd dw_hdmi_ahb_audio dw_hdmi_cec etnaviv
CPU: 1 PID: 1889 Comm: media-ctl Tainted: G C  4.11.0-rc1+ #2125
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
Backtrace:
[] (dump_backtrace) from [] (show_stack+0x18/0x1c)
 r6:600e0013 r5: r4: r3:
[] (show_stack) from [] (dump_stack+0xa4/0xdc)
[] (dump_stack) from [] (__warn+0xdc/0x108)
 r6:bf124014 r5: r4: r3:c09ea4a8
[] (__warn) from [] (warn_slowpath_null+0x28/0x30)
 r10:ede00010 r8:ede4a348 r7:d039501c r6:d0395140 r5: r4:d0395010
[] (warn_slowpath_null) from [] (csi_s_power+0x9c/0xa8 
[imx_media_csi])
[] (csi_s_power [imx_media_csi]) from [] 
(imx_media_set_power+0x3c/0x108 [imx_media_common])
 r7:d039501c r6: r5: r4:000c
[] (imx_media_set_power [imx_media_common]) from [] 
(imx_media_pipeline_set_power+0x38/0x40 [imx_media_common])
 r10:0001 r9:0001 r8:ede4a348 r7:ede00010 r6:ede4a348 r5:d039501c
 r4:0001
[] (imx_media_pipeline_set_power [imx_media_common]) from 
[] (imx_media_link_notify+0xf0/0x144 [imx_media])
 r7:ede00010 r6:ed59f900 r5: r4:d039501c
[] (imx_media_link_notify [imx_media]) from [] 
(__media_entity_setup_link+0x110/0x1d8)
 r10:c0347c03 r9:d7eb3dc8 r8:befe92b0 r7:ede00010 r6: r5:0001
 r4:ed59f900 r3:bf052058
[] (__media_entity_setup_link) from [] 
(media_device_setup_link+0x84/0x90)
 r7:ede00010 r6:ede00010 r5:ef3fd810 r4:d7eb3dc8
[] (media_device_setup_link) from [] 
(media_device_ioctl+0xa4/0x148)
 r6: r5:d7eb3dc8 r4:c077b014 r3:c04f9b2c
[] (media_device_ioctl) from [] (media_ioctl+0x38/0x4c)
 r10:ed5eca68 r9:d7eb2000 r8:befe92b0 r7:0003 r6:0003 r5:e82ca280
 r4:c0190304
[] (media_ioctl) from [] (do_vfs_ioctl+0x98/0x9a0)
[] (do_vfs_ioctl) from [] (SyS_ioctl+0x3c/0x60)
 r10: r9:d7eb2000 r8:befe92b0 r7:0003 r6:c0347c03 r5:e82ca280
 r4:e82ca280
[] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x1c)
 r8:c000ff04 r7:0036 r6:000261d0 r5:0001 r4:009162e8 r3:0001
---[ end trace 4fdd40e5adfc4485 ]---
[ cut here ]
WARNING: CPU: 1 PID: 1889 at drivers/staging/media/imx/imx-media-csi.c:806 
csi_s_power+0x9c/0xa8 [imx_media_csi]
Modules linked in: caam_jr uvcvideo snd_soc_imx_sgtl5000 snd_soc_fsl_asoc_card 
snd_soc_imx_spdif imx_media_csi(C) imx6_mipi_csi2(C) snd_soc_imx_audmux 
snd_soc_sgtl5000 imx219 imx_media_ic(C) imx_media_capture(C) imx_media_vdic(C) 
caam video_multiplexer imx_sdma coda v4l2_mem2mem videobuf2_v4l2 imx2_wdt 
imx_vdoa videobuf2_dma_contig videobuf2_core videobuf2_vmalloc videobuf2_memops 
snd_soc_fsl_ssi
imx_thermal snd_soc_fsl_spdif imx_pcm_dma imx_media(C) imx_media_common(C) nfsd
rc_pinnacle_pctv_hd dw_hdmi_ahb_audio dw_hdmi_cec etnaviv
CPU: 1 PID: 1889 Comm: media-ctl Tainted: GWC  4.11.0-rc1+ #2125
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
Backtrace:
[] (dump_backtrace) from [] (show_stack+0x18/0x1c)
 r6:600e0013 r5: r4: r3:
[] (show_stack) from [] (dump_stack+0xa4/0xdc)
[] (dump_stack) from [] (__warn+0xdc/0x108)
 r6:bf124014 r5: r4: r3:c09ea4a8
[] (__warn) from [] (warn_slowpath_null+0x28/0x30)
 r10:ede00010 r8:ede4a348 r7:ee000800 r6:d0395140 r5: r4:d0395010
[] (warn_slowpath_null) from [] (csi_s_power+0x9c/0xa8 
[imx_media_csi])
[] (csi_s_power [imx_media_csi]) from [] 
(imx_media_set_power+0x3c/0x108 [imx_media_common])
 r7:ee000800 r6: r5: r4:000c
[] (imx_media_set_power [imx_media_common]) from [] 
(imx_media_pipeline_set_power+0x38/0x40 [imx_media_common])
 r10:0001 r9:0001 r8:ede4a348 r7:ede00010 r6:ede4a348 r5:ee000800
 r4:0001
[] (imx_media_pipeline_set_power [imx_media_common]) from 
[] (imx_media_link_notify+0xf0/0x144 [imx_media])
 r7:ede00010 r6:d0320480 r5:ee000800 r4:ee000800
[] (imx_media_link_notify [imx_media]) from [] 
(__media_entity_setup_link+0x110/0x1d8)
 r10:c0347c03 r9:d7eb3dc8 r8:befe92b0 

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

[Russell King - ARM Linux]

On Fri, Mar 10, 2017 at 03:20:34PM -0800, Steve Longerbeam wrote:
> 
> 
> On 03/10/2017 12:13 PM, Russell King - ARM Linux wrote:
> >Version 5 gives me no v4l2 controls exposed through the video device
> >interface.
> >
> >Just like with version 4, version 5 is completely useless with IMX219:
> >
> >imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
> >ipu1_csi0: pipeline start failed with -110
> >imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
> >ipu1_csi0: pipeline start failed with -110
> >imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
> >ipu1_csi0: pipeline start failed with -110
> >
> >So, like v4, I can't do any further testing.
> >
> 
> Is the imx219 placing the csi-2 bus in LP-11 state on exit
> from s_power(ON)?
> 
> I realize that probably means bringing the chip up to a
> completely operational state and then setting it to stream
> OFF in the s_power() op.
> 
> The same had to be done for the OV5640.

What do you suggest - setting it to the highest CSI2 bus speed that
it supports?  That's likely to be over the maximum data rate specified
for iMX6Q if it's wired up using four lanes.


Also, as I've already said, I think that powering on the sensor just
because it's got an enabled media-controller link is a silly idea.

Right now, the only way of using the imx6 capture stuff is to manually
configure it with media-ctl, which means that happens either at boot
due to a custom boot script, or when you first use it (by manually
running a script.)

This results in the sensor staying powered from that point onwards,
wasting power unnecessarily.

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

[Russell King - ARM Linux]

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.

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 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.

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?

If you can actually respond to the points that I've been raising about
end user usability, then we can have a discussion.

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

[Russell King - ARM Linux]

On Sat, Mar 11, 2017 at 08:25:49AM -0300, Mauro Carvalho Chehab wrote:
> 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. 
> 
> 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.

This is what really worries me right now about the current proposal for
iMX6.  What's being proposed is to make the driver exclusively MC-based.

What that means is that existing applications are _not_ going to work
until we have some answer for libv4l2, and from what you've said above,
it seems that this has been attempted multiple times over the last _8_
years, and each time it's failed.

When thinking about it, it's quite obvious why merely trying to push
the problem into userspace fails:

  If we assert that the kernel does not have sufficient information to
  make decisions about how to route and control the hardware, then under
  what scenario does a userspace library have sufficient information to
  make those decisions?

So, merely moving the problem into userspace doesn't solve anything.

Loading the problem onto the user in the hope that the user knows
enough to properly configure it also doesn't work - who is going to
educate the user about the various quirks of the hardware they're
dealing with?

I don't think pushing it into platform specific libv4l2 plugins works
either - as you say above, even just trying to develop a plugin for
exynos4 seems to have failed, so what makes us think that developing
a plugin for iMX6 is going to succeed?  Actually, that's exactly where
the problem lies.

Is "iMX6 plugin" even right?  That only deals with the complexity of
one part of the system - what about the source device, which as we
have already seen can be a tuner or a camera with its own multiple
sub-devices.  What if there's a video improvement chip in the chain
as well - how is a "generic" iMX6 plugin supposed to know how to deal
with that?

It seems to me that what's required is not an "iMX6 plugin" but a
separate plugin for each platform - or worse.  Consider boards like
the Raspberry Pi, where users can attach a variety of cameras.  I
don't think this approach scales.  (This is relevant: the iMX6 board
I have here has a RPi compatible connector for a MIPI CSI2 camera.
In fact, the IMX219 module I'm using _is_ a RPi camera, it's the RPi
NoIR Camera V2.)

The iMX6 problem is way larger than just "which subdev do I need to
configure for control X" - if you look at the dot graphs both Steve
and myself have supplied, you'll notice that there are eight (yes,
8) video capture devices.  Let's say that we can solve the subdev
problem in libv4l2.  There's another problem lurking here - libv4l2
is /dev/video* based.  How does it know which /dev/video* device to
open?

We don't open by sensor, we open by /dev/video*.  In my case, there
is only one correct /dev/video* node for the attached sensor, the
other seven are totally irrelevant.  For other situations, there may
be the choice of three functional /dev/video* nodes.

Right now, for my case, there isn't the information exported from the
kernel to know which is the correct one, since that requires knowing
which virtual channel the data is going to be sent over the CSI2
interface.  That information is not present in DT, or anywhere.  It
only comes from system knowledge - in my case, I know that the IMX219
is currently being configured to use virtual channel 0.  SMIA cameras
are also configurable.  Then there's CSI2 cameras that can produce
different formats via different virtual channels (eg, JPEG compressed
image on one channel while streaming a RGB image via the other channel.)

Whether you can use one or three in _this_ scenario depends on the
source format - again, another bit of implementation specific
information that userspace would need to know.  Kernel space should
know that, and it's discoverable by testing which paths accept the
source format - but that doesn't tell you ahead of time which
/dev/video* node to open.

So, the problem space we have here is absolutely huge, and merely
having a plugin that activates when you open a /dev/video* node
really doesn't solve it.

All in all, I really don't think "lets hope someone writes a v4l2
plugin to solve it" is ever going to be successful.  I don't even
see that there will ever be a userspace application that is anything
more than a representation of the dot graphs that users can use to
manually configure the capture system with system knowledge.

I think everyone needs to take a step back and think long and hard
about this from the system usability perspective - I 

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

[Russell King - ARM Linux]

On Sat, Mar 11, 2017 at 11:06:55AM -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.
> >
> >Let me re-state.  If the source can _only_ produce bayer, then there is
> >_only_ _one_ possible path, and all the overhead of the media controller
> >stuff is totally unnecessary.
> >
> >Or, are you going to tell me that the user should have the right to
> >configure paths through the iMX6 hardware that are not permitted by the
> >iMX6 manuals for the data format being produced by the sensor?
> >
> 
> Russell, I'm not following you. The imx6 pipelines allow for many
> different sources, not just the inx219 that only outputs bayer. You
> seem to be saying that those other pipelines should not be present
> because they don't support raw bayer.

What I'm saying is this:

_If_ you have a sensor connected that can _only_ produce bayer, _then_
there is only _one_ possible path through the imx6 pipelines that is
legal.  Offering other paths from the source is noise, because every
other path can't be used with a bayer source.

_If_ you have a sensor connected which can produce RGB or YUV formats,
_then_ other paths are available, and pipeline needs to be configured
to select the appropriate path with the desired features.

So, in the case of a bayer source, offering the user the chance to
manually configure the _single_ allowable route through the tree is
needless complexity.  Forcing the user to have to use the subdev
interfaces to configure the camera is needless complexity.  Such a
source can only ever be used with one single /dev/video* node.

Moreover, this requires user education, and this brings me on to much
larger concerns.  We seem to be saying "this is too complicated, the
user can work it out!"

We've been here with VGA devices.  Remember the old days when you had
to put mode lines into the Xorg.conf, or go through a lengthy setup
process to get X running?  It wasn't very user-friendly.  We seem to
be making the same mistake here.

Usability comes first and foremost - throwing complex problems at
users is not a solution.

Now, given that this media control API has been around for several
years, and the userspace side of the story has not really improved
(according to Mauro, several attempts have been made, every single
attempt so far has failed, even for specific hardware) it seems to me
that using the media control API is a very poor choice for the very
simple reason that _no one_ knows how to configure a system using it.
Hans thoughts of getting some funding to look at this aspect is a
good idea, but I really wonder, given the history so far, how long
this will take - and whether it _ever_ will get solved.

If it doesn't get solved, then we're stuck with quite a big problem.

So, I suggest that we don't merge any further media-controller based
kernel code _until_ we have the userspace side sorted out.  Merging
the kernel side drivers when we don't even know that the userspace
API is functionally usable in userspace beyond test programs is
utterly absurd - what if it turns out that no one can write v4l
plugins that sort out the issues that have been highlighted throughout
these discussions.

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

[Russell King - ARM Linux]

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.

Let me re-state.  If the source can _only_ produce bayer, then there is
_only_ _one_ possible path, and all the overhead of the media controller
stuff is totally unnecessary.

Or, are you going to tell me that the user should have the right to
configure paths through the iMX6 hardware that are not permitted by the
iMX6 manuals for the data format being produced by the sensor?

-- 
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 v5 15/39] [media] v4l2: add a frame interval error event

[Russell King - ARM Linux]

On Sat, Mar 11, 2017 at 10:14:49AM -0800, Steve Longerbeam wrote:
> On 03/11/2017 03:39 AM, Hans Verkuil wrote:
> >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.

I have seen that rolling effect, but the iMX6 regains correct sync
within one complete "roll" just fine here with IMX219.  However, it
has always recovered.

So, I don't think there's a problem with the iMX6 part of the
processing, and so I don't think we should cripple the iMX6 capture
drivers for this problem.

It seems to me that the problem is with the source.

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

[Russell King - ARM Linux]

On Sat, Mar 11, 2017 at 10:08:23AM -0800, Steve Longerbeam wrote:
> On 03/11/2017 07:32 AM, Sakari Ailus wrote:
> >Hi Mauro and Hans,
> >
> >On Sat, Mar 11, 2017 at 10:14:08AM -0300, Mauro Carvalho Chehab wrote:
> >>Em Sat, 11 Mar 2017 12:32:43 +0100
> >>Hans Verkuil  escreveu:
> >>
> >>>On 10/03/17 16:09, Mauro Carvalho Chehab wrote:
> Em Fri, 10 Mar 2017 13:54:28 +0100
> Hans Verkuil  escreveu:
> 
> >>Devices that have complex pipeline that do essentially require using the
> >>Media controller interface to configure them are out of that scope.
> >>
> >
> >Way too much of how the MC devices should be used is in the minds of 
> >developers.
> >There is a major lack for good detailed documentation, utilities, 
> >compliance
> >test (really needed!) and libv4l plugins.
> 
> Unfortunately, we merged an incomplete MC support at the Kernel. We knew
> all the problems with MC-based drivers and V4L2 applications by the time
> it was developed, and we requested Nokia developers (with was sponsoring 
> MC
> develoment, on that time) to work on a solution to allow standard V4L2
> applications to work with MC based boards.
> 
> Unfortunately, we took the decision to merge MC without that, because
> Nokia was giving up on Linux development, and we didn't want to lose the
> 2 years of discussions and work around it, as Nokia employers were leaving
> the company. Also, on that time, there was already some patches floating
> around adding backward support via libv4l. Unfortunately, those patches
> were never finished.
> 
> The net result is that MC was merged with some huge gaps, including
> the lack of a proper solution for a generic V4L2 program to work
> with V4L2 devices that use the subdev API.
> 
> That was not that bad by then, as MC was used only on cell phones
> that run custom-made applications.
> 
> The reallity changed, as now, we have lots of low cost SoC based
> boards, used for all sort of purposes. So, we need a quick solution
> for it.
> 
> In other words, while that would be acceptable support special apps
> on really embedded systems, it is *not OK* for general purpose SoC
> harware[1].
> 
> [1] I'm calling "general purpose SoC harware" those ARM boards
> like Raspberry Pi that are shipped to the mass and used by a wide
> range of hobbyists and other people that just wants to run Linux on
> ARM. It is possible to buy such boards for a very cheap price,
> making them to be used not only on special projects, where a custom
> made application could be interesting, but also for a lot of
> users that just want to run Linux on a low cost ARM board, while
> keeping using standard V4L2 apps, like "camorama".
> 
> That's perhaps one of the reasons why it took a long time for us to
> start receiving drivers upstream for such hardware: it is quite
> intimidating and not logical to require developers to implement
> on their drivers 2 complex APIs (MC, subdev) for those
> hardware that most users won't care. From user's perspective,
> being able to support generic applications like "camorama" and
> "zbar" is all they want.
> 
> In summary, I'm pretty sure we need to support standard V4L2
> applications on boards like Raspberry Pi and those low-cost
> SoC-based boards that are shipped to end users.
> 
> >Anyway, regarding this specific patch and for this MC-aware driver: no, 
> >you
> >shouldn't inherit controls from subdevs. It defeats the purpose.
> 
> Sorry, but I don't agree with that. The subdev API is an optional API
> (and even the MC API can be optional).
> 
> I see the rationale for using MC and subdev APIs on cell phones,
> ISV and other embedded hardware, as it will allow fine-tuning
> the driver's support to allow providing the required quality for
> certain custom-made applications. but on general SoC hardware,
> supporting standard V4L2 applications is a need.
> 
> Ok, perhaps supporting both subdev API and V4L2 API at the same
> time doesn't make much sense. We could disable one in favor of the
> other, either at compilation time or at runtime.
> >>>
> >>>Right. If the subdev API is disabled, then you have to inherit the subdev
> >>>controls in the bridge driver (how else would you be able to access them?).
> >>>And that's the usual case.
> >>>
> >>>If you do have the subdev API enabled, AND you use the MC, then the
> >>>intention clearly is to give userspace full control and inheriting controls
> >>>no longer makes any sense (and is highly confusing IMHO).
> >>
> >>I tend to agree with that.
> >
> >I agree as well.
> >
> >This is in line with how existing drivers behave, too.
> 
> Well, sounds like there is consensus on this topic. I guess I'll
> go 

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

[Russell King - ARM Linux]

On Sat, Mar 11, 2017 at 05:32:29PM +0200, Sakari Ailus wrote:
> My understanding of the i.MX6 case is the hardware is configurable enough
> to warrant the use of the Media controller API. Some patches indicate
> there are choices to be made in data routing.

The iMX6 does have configurable data routing, but in some scenarios
(eg, when receiving bayer data) there's only one possible routing.

> Steve: could you enlighten us on the topic, by e.g. doing media-ctl
> --print-dot and sending the results to the list? What kind of different IP
> blocks are there and what do they do? A pointer to hardware documentation
> wouldn't hurt either (if it's available).

Attached for the imx219 camera.  Note that although the CSI2 block has
four outputs, each output is dedicated to a CSI virtual channel, so
they can not be arbitarily assigned without configuring the sensor.

Since the imx219 only produces bayer, the graph is also showing the
_only_ possible routing for the imx219 configured for CSI virtual
channel 0.

The iMX6 manuals are available on the 'net.

https://community.nxp.com/docs/DOC-101840

There are several chapters that cover the capture side:

* MIPI CSI2
* IPU CSI2 gasket
* IPU

The IPU not only performs capture, but also display as well.

-- 
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.
digraph board {
	rankdir=TB
	n0001 [label="{{ 0 |  1} | ipu1_csi0_mux\n/dev/v4l-subdev0 | { 2}}", shape=Mrecord, style=filled, fillcolor=green]
	n0001:port2 -> n0044:port0
	n0005 [label="{{ 0 |  1} | ipu2_csi1_mux\n/dev/v4l-subdev1 | { 2}}", shape=Mrecord, style=filled, fillcolor=green]
	n0005:port2 -> n0068:port0 [style=dashed]
	n0009 [label="{{ 0 |  1} | ipu1_vdic\n/dev/v4l-subdev2 | { 2}}", shape=Mrecord, style=filled, fillcolor=green]
	n0009:port2 -> n0011:port0 [style=dashed]
	n000d [label="{{ 0 |  1} | ipu2_vdic\n/dev/v4l-subdev3 | { 2}}", shape=Mrecord, style=filled, fillcolor=green]
	n000d:port2 -> n0027:port0 [style=dashed]
	n0011 [label="{{ 0} | ipu1_ic_prp\n/dev/v4l-subdev4 | { 1 |  2}}", shape=Mrecord, style=filled, fillcolor=green]
	n0011:port1 -> n0015:port0 [style=dashed]
	n0011:port2 -> n001e:port0 [style=dashed]
	n0015 [label="{{ 0} | ipu1_ic_prpenc\n/dev/v4l-subdev5 | { 1}}", shape=Mrecord, style=filled, fillcolor=green]
	n0015:port1 -> n0018 [style=dashed]
	n0018 [label="ipu1_ic_prpenc capture\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
	n001e [label="{{ 0} | ipu1_ic_prpvf\n/dev/v4l-subdev6 | { 1}}", shape=Mrecord, style=filled, fillcolor=green]
	n001e:port1 -> n0021 [style=dashed]
	n0021 [label="ipu1_ic_prpvf capture\n/dev/video1", shape=box, style=filled, fillcolor=yellow]
	n0027 [label="{{ 0} | ipu2_ic_prp\n/dev/v4l-subdev7 | { 1 |  2}}", shape=Mrecord, style=filled, fillcolor=green]
	n0027:port1 -> n002b:port0 [style=dashed]
	n0027:port2 -> n0034:port0 [style=dashed]
	n002b [label="{{ 0} | ipu2_ic_prpenc\n/dev/v4l-subdev8 | { 1}}", shape=Mrecord, style=filled, fillcolor=green]
	n002b:port1 -> n002e [style=dashed]
	n002e [label="ipu2_ic_prpenc capture\n/dev/video2", shape=box, style=filled, fillcolor=yellow]
	n0034 [label="{{ 0} | ipu2_ic_prpvf\n/dev/v4l-subdev9 | { 1}}", shape=Mrecord, style=filled, fillcolor=green]
	n0034:port1 -> n0037 [style=dashed]
	n0037 [label="ipu2_ic_prpvf capture\n/dev/video3", shape=box, style=filled, fillcolor=yellow]
	n003d [label="{{ 1} | imx219 0-0010\n/dev/v4l-subdev11 | { 0}}", shape=Mrecord, style=filled, fillcolor=green]
	n003d:port0 -> n0058:port0
	n0040 [label="{{} | imx219 pixel 0-0010\n/dev/v4l-subdev10 | { 0}}", shape=Mrecord, style=filled, fillcolor=green]
	n0040:port0 -> n003d:port1 [style=bold]
	n0044 [label="{{ 0} | ipu1_csi0\n/dev/v4l-subdev12 | { 1 |  2}}", shape=Mrecord, style=filled, fillcolor=green]
	n0044:port2 -> n0048
	n0044:port1 -> n0011:port0 [style=dashed]
	n0044:port1 -> n0009:port0 [style=dashed]
	n0048 [label="ipu1_csi0 capture\n/dev/video4", shape=box, style=filled, fillcolor=yellow]
	n004e [label="{{ 0} | ipu1_csi1\n/dev/v4l-subdev13 | { 1 |  2}}", shape=Mrecord, style=filled, fillcolor=green]
	n004e:port2 -> n0052 [style=dashed]
	n004e:port1 -> n0011:port0 [style=dashed]
	n004e:port1 -> n0009:port0 [style=dashed]
	n0052 [label="ipu1_csi1 capture\n/dev/video5", shape=box, style=filled, fillcolor=yellow]
	n0058 [label="{{ 0} | imx6-mipi-csi2\n/dev/v4l-subdev14 | { 1 |  2 |  3 |  4}}", shape=Mrecord, style=filled, fillcolor=green]
	n0058:port1 -> n0001:port0
	n0058:port2 -> n004e:port0 [style=dashed]
	n0058:port3 -> n005e:port0 [style=dashed]
	n0058:port4 -> n0005:port0 [style=dashed]
	n005e [label="{{ 0} | 

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

[Russell King - ARM Linux]

Version 5 gives me no v4l2 controls exposed through the video device
interface.

Just like with version 4, version 5 is completely useless with IMX219:

imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
ipu1_csi0: pipeline start failed with -110
imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
ipu1_csi0: pipeline start failed with -110
imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
ipu1_csi0: pipeline start failed with -110

So, like v4, I can't do any further testing.

On Thu, Mar 09, 2017 at 08:52:40PM -0800, Steve Longerbeam wrote:
> In version 5:
> 
> - ov5640: renamed "pwdn-gpios" to "powerdown-gpios"
> 
> - ov5640: add mutex lock around the subdev op entry points.
> 
> - ov5640: don't attempt to program the new mode in ov5640_set_fmt().
>   Instead set a new flag, pending_mode_change, and program the new
>   mode at s_stream() if flag is set.
> 
> - ov5640: implement [gs]_frame_interval. As part of that, create
>   ov5640_try_frame_interval(), which is used by both [gs]_frame_interval
>   and [gs]_parm.
> 
> - ov5640: don't attempt to set controls in ov5640_s_ctrl(), or at
>   mode change, do it instead after first power-up.
> 
> - video-multiplexer: include link_validate in media_entity_operations.
> 
> - video-multiplexer: enforce that output pad frame interval must match
>   input pad frame interval in vidsw_s_frame_interval().
> 
> - video-multiplexer: initialize frame interval to a default 30 fps.
> 
> - mipi csi-2: renamed "cfg" clock name property to "ref". This is the
>   27 MHz mipi csi-2 PLL reference clock.
> 
> - mipi csi-2: create a hsfreq_map[] table based on
>   https://community.nxp.com/docs/DOC-94312. Use it to select
>   a hsfreqrange_sel value when programming the D-PHY, based on
>   a max Mbps per lane. This is computed from the source subdev
>   via V4L2_CID_LINK_FREQ control, and if the subdev doesn't implement
>   that control, use a default hard-coded max Mbps per lane.
> 
> - added required ports property description to imx-media binding doc.
> 
> - removed event V4L2_EVENT_FRAME_TIMEOUT. On a frame timeout, which
>   is always unrecoverable, call vb2_queue_error() instead.
> 
> - export the remaining custom events to V4L2_EVENT_FRAME_INTERVAL_ERROR
>   and V4L2_EVENT_NEW_FRAME_BEFORE_EOF.
> 
> - vdic: use V4L2_CID_DEINTERLACING_MODE for motion compensation control
>   instead of a custom control.
> 
> - add v4l2_subdev_link_validate_frame_interval(). Call this in the
>   link_validate imx-media subdev callbacks and video-multiplexer.
> 
> - fix subdev event registration: implementation of subscribe_event()
>   and unsubscribe_event() subdev ops were missing.
> 
> - all calls from the pipeline to the sensor subdev have been removed.
>   Only the CSI subdev still refers to a sensor, and only to retrieve
>   its media bus config, which is necessary to setup the CSI interface.
> 
> - add mutex locks around the imx-media subdev op entry points.
> 
> - completed the propagation of all pad format parameters from sink
>   pads to source pads within every imx-media subdev.
> 
> - implement [gs]_frame_interval in all the imx-media subdevs.
> 
> - imx-ic-prpencvf: there isn't necessarily a CSI subdev in the pipeline
>   in the future, so make sure this is optional when calling the CSI's
>   FIM.
> 
> - the source pads that attach to capture device nodes now require the
>   IPU internal pixel codes. The capture device translates these to
>   v4l2 fourcc memory formats.
> 
> - fix control inheritance to the capture device. When the pipeline
>   was modified, the inherited controls were not being refreshed.
>   v4l2_pipeline_inherit_controls() is now called only in imx-media
>   link_notify() callback when a pipelink link is disabled or modified.
>   imx_media_find_pipeline_video_device() is created to locate the
>   capture device in the pipeline.
> 
> - fix a possible race when propagating formats to the capture device.
>   The subdevs and capture device use different mutex locks when setting
>   formats. imx_media_capture_device_set_format() is created which acquires
>   the capture device mutex when updating the capture device format.
> 
> - verify all subdevs were bound in the async completion callback.
>  
> 
> Philipp Zabel (7):
>   [media] dt-bindings: Add bindings for video-multiplexer device
>   ARM: dts: imx6qdl: Add mipi_ipu1/2 multiplexers, mipi_csi, and their
> connections
>   add mux and video interface bridge entity functions
>   platform: add video-multiplexer subdevice driver
>   media: imx: csi: fix crop rectangle changes in set_fmt
>   media: imx: csi: add frame skipping support
>   media: imx: csi: fix crop rectangle reset in sink set_fmt
> 
> Russell King (4):
>   media: imx: add support for bayer formats
>   media: imx: csi: add support for bayer formats
>   media: imx: mipi-csi2: enable setting and getting of frame rates
>   media: imx: csi/fim: add support for frame intervals
> 
> Steve Longerbeam (28):
>   [media] dt-bindings: Add bindings 

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

[Russell King - ARM Linux]

On Fri, Mar 10, 2017 at 03:57:09PM +, Russell King - ARM Linux wrote:
> Enabling debug output in gstreamer's v4l2src plugin confirms that
> the kernel's bayer format are totally hidden from gstreamer when
> linked with libv4l2, but are present when it isn't linked with
> libv4l2.

Here's the information to back up my claims:

root@hbi2ex:~# v4l2-ctl -d 6 --list-formats-ext
ioctl: VIDIOC_ENUM_FMT
Index   : 0
Type: Video Capture
Pixel Format: 'RGGB'
Name: 8-bit Bayer RGRG/GBGB

root@hbi2ex:~# DISPLAY=:0 GST_DEBUG_NO_COLOR=1 GST_DEBUG=v4l2:9 gst-launch-1.0 
v4l2src device=/dev/video6 ! bayer2rgbneon ! xvimagesink > gst-v4l2-1.log 2>&1
root@hbi2ex:~# cut -b65- gst-v4l2-1.log|less
v4l2_calls.c:519:gst_v4l2_open: Trying to open device /dev/video6
v4l2_calls.c:69:gst_v4l2_get_capabilities: getting capabilities
v4l2_calls.c:77:gst_v4l2_get_capabilities: driver:  
'imx-media-camif'
v4l2_calls.c:78:gst_v4l2_get_capabilities: card:
'imx-media-camif'
v4l2_calls.c:79:gst_v4l2_get_capabilities: bus_info:''
v4l2_calls.c:80:gst_v4l2_get_capabilities: version: 00040a00
v4l2_calls.c:81:gst_v4l2_get_capabilities: capabilites: 8521
...
v4l2_calls.c:258:gst_v4l2_fill_lists:   controls+menus
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 
v4l2_calls.c:319:gst_v4l2_fill_lists: starting control class 'User 
Controls'
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 00980001
v4l2_calls.c:389:gst_v4l2_fill_lists: Adding ControlID 
white_balance_automatic (98090c)
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 0098090c
v4l2_calls.c:389:gst_v4l2_fill_lists: Adding ControlID gamma (980910)
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 00980910
v4l2_calls.c:389:gst_v4l2_fill_lists: Adding ControlID gain (980913)
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 00980913
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 00980914
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 00980915
v4l2_calls.c:319:gst_v4l2_fill_lists: starting control class 'Camera 
Controls'
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 009a0001
v4l2_calls.c:382:gst_v4l2_fill_lists: ControlID 
exposure_time_absolute (9a0902) unhandled, FIXME
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 009a0902
v4l2_calls.c:319:gst_v4l2_fill_lists: starting control class 'Image 
Source Controls'
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 009e0001
v4l2_calls.c:382:gst_v4l2_fill_lists: ControlID vertical_blanking 
(9e0901) unhandled, FIXME
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 009e0901
v4l2_calls.c:382:gst_v4l2_fill_lists: ControlID horizontal_blanking 
(9e0902) unhandled, FIXME
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 009e0902
v4l2_calls.c:382:gst_v4l2_fill_lists: ControlID analogue_gain 
(9e0903) unhandled, FIXME
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 009e0903
v4l2_calls.c:382:gst_v4l2_fill_lists: ControlID red_pixel_value 
(9e0904) unhandled, FIXME
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 009e0904
v4l2_calls.c:382:gst_v4l2_fill_lists: ControlID green_red_pixel_value 
(9e0905) unhandled, FIXME
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 009e0905
v4l2_calls.c:382:gst_v4l2_fill_lists: ControlID blue_pixel_value 
(9e0906) unhandled, FIXME
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 009e0906
v4l2_calls.c:382:gst_v4l2_fill_lists: ControlID 
green_blue_pixel_value (9e0907) unhandled, FIXME
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 009e0907
v4l2_calls.c:319:gst_v4l2_fill_lists: starting control class 'Image 
Processing Controls'
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 009f0001
v4l2_calls.c:340:gst_v4l2_fill_lists: Control type for 'Pixel Rate' 
not suppored for extra controls.
v4l2_calls.c:382:gst_v4l2_fill_lists: ControlID Pixel Rate (9f0902) 
unhandled, FIXME
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 009f0902
v4l2_calls.c:382:gst_v4l2_fill_lists: ControlID test_pattern (9f0903) 
unhandled, FIXME
v4l2_calls.c:278:gst_v4l2_fill_lists: checking control 009f0903
v4l2_calls.c:284:gst_v4l2_fill_lists: controls finished
v4l2_calls.c:451:gst_v4l2_fill_lists: done
v4l2_calls.c:587:gst_v4l2_open: Opened device 'imx-media-camif' 
(/dev/video6) successfully
gstv4l2object.c:804:gst_v4l2_set_defaults: tv_norm=0x0, norm=(nil)
v4l2_calls.c:734:gst_v4l2_get_norm: getting norm
v4l2_calls.c:1021:gst_v4l2_get_input: trying to get input
v4l2_calls.c:1031:gst_v4l2_get_input: input: 0

gstv4l2object.c:1106:gst_v4l2_object_fill_format_list: getting src 
format enumerations
gstv4l2object.c:1124:gst_v4l2_object_fill_format_list: index:   0
gstv4l2object.c:1125:gst_v4l2_object_fill_format_list: type:1
gstv4l2object.c:1126:gst_v4l2_object_fill_format_list: flags:   
0002
gstv4l2object.c:1128:gst_v4l2_object_fill_format_list: description: 
'RGB3'
gstv4l2object.c:1130:gst_v4l2_o

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

[Russell King - ARM Linux]

On Fri, Mar 10, 2017 at 12:26:34PM -0300, Mauro Carvalho Chehab wrote:
> Hi Russell,
> 
> Em Fri, 10 Mar 2017 13:07:33 +
> Russell King - ARM Linux <li...@armlinux.org.uk> escreveu:
> 
> > The idea that the v4l libraries should intercept the format negotiation
> > between the application and kernel is a particularly painful one - the
> > default gstreamer build detects the v4l libraries, and links against it.
> > That much is fine.
> > 
> > However, the problem comes when you're trying to use bayer formats. The
> > v4l libraries "helpfully" (or rather unhelpfully) intercept the format
> > negotiation, and decide that they'll invoke v4lconvert to convert the
> > bayer to RGB for you, whether you want them to do that or not.
> > 
> > v4lconvert may not be the most efficient way to convert, or even what
> > is desired (eg, you may want to receive the raw bayer image.)  However,
> > since the v4l libraries/v4lconvert gives you no option but to have its
> > conversion forced into the pipeline, other options (such as using the
> > gstreamer neon accelerated de-bayer plugin) isn't an option 
> 
> That's not true. There is an special flag, used only by libv4l2
> emulated formats, that indicates when a video format is handled
> via v4lconvert:

I'm afraid that my statement comes from trying to use gstreamer with
libv4l2 and _not_ being able to use the 8-bit bayer formats there at
all - they are simply not passed across to the application through
libv4l2/v4lconvert.

Instead, the formats that are passed across are the emulated formats.
As I said above, that forces applications to use only the v4lconvert
formats, the raw formats are not available.

So, the presence or absence of the V4L2_FMT_FLAG_EMULATED is quite
meaningless if you can't even enumerate the non-converted formats.

The problem comes from the "always needs conversion" stuff in
v4lconvert coupled with the way this subdev stuff works - since it
requires manual configuration of all the pads within the kernel
media pipeline, the kernel ends up only advertising _one_ format
to userspace - in my case, that's RGGB8.

When v4lconvert_create_with_dev_ops() enumerates the formats from
the kernel, it gets only RGGB8.  That causes always_needs_conversion
in there to remain true, so the special v4l control which enables/
disables conversion gets created with a default value of "true".
The RGGB8 bit is also set in data->supported_src_formats.

This causes v4lconvert_supported_dst_fmt_only() to return true.

What this all means is that v4lconvert_enum_fmt() will _not_ return
any of the kernel formats, only the faked formats.

Ergo, the RGGB8 format from the kernel is completely hidden from the
application, and only the emulated format is made available.  As I
said above, this forces v4lconvert's debayering on the application,
whether you want it or not.

In the gstreamer case, it knows nothing about this special control,
which means that trying to use this gstreamer pipeline:

$ gst-launch-1.0 v4l2src device=/dev/video6 ! bayer2rgbneon ! xvimagesink

is completely impossible without first rebuilding gstreamer _without_
libv4l support.  Build gstreamer without libv4l support, and the above
works.

Enabling debug output in gstreamer's v4l2src plugin confirms that
the kernel's bayer format are totally hidden from gstreamer when
linked with libv4l2, but are present when it isn't linked with
libv4l2.

-- 
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.