Re: [PATCH v5 7/9] videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range
On Wed, Jan 02, 2019 at 04:23:15PM +0530, Souptick Joarder wrote:
> On Mon, Dec 24, 2018 at 6:53 PM Souptick Joarder wrote:
> >
> > Convert to use vm_insert_range to map range of kernel memory
> > to user vma.
> >
> > Signed-off-by: Souptick Joarder
> > Reviewed-by: Matthew Wilcox
> > Acked-by: Marek Szyprowski
> > Acked-by: Mauro Carvalho Chehab
> > ---
> > drivers/media/common/videobuf2/videobuf2-dma-sg.c | 23
> > +++
> > 1 file changed, 7 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > index 015e737..898adef 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > @@ -328,28 +328,19 @@ static unsigned int vb2_dma_sg_num_users(void
> > *buf_priv)
> > static int vb2_dma_sg_mmap(void *buf_priv, struct vm_area_struct *vma)
> > {
> > struct vb2_dma_sg_buf *buf = buf_priv;
> > - unsigned long uaddr = vma->vm_start;
> > - unsigned long usize = vma->vm_end - vma->vm_start;
> > - int i = 0;
> > + unsigned long page_count = vma_pages(vma);
> > + int err;
> >
> > if (!buf) {
> > printk(KERN_ERR "No memory to map\n");
> > return -EINVAL;
> > }
> >
> > - do {
> > - int ret;
> > -
> > - ret = vm_insert_page(vma, uaddr, buf->pages[i++]);
> > - if (ret) {
> > - printk(KERN_ERR "Remapping memory, error: %d\n",
> > ret);
> > - return ret;
> > - }
> > -
> > - uaddr += PAGE_SIZE;
> > - usize -= PAGE_SIZE;
> > - } while (usize > 0);
> > -
> > + err = vm_insert_range(vma, vma->vm_start, buf->pages, page_count);
> > + if (err) {
> > + printk(KERN_ERR "Remapping memory, error: %d\n", err);
> > + return err;
> > + }
> >
>
> Looking into the original code -
> drivers/media/common/videobuf2/videobuf2-dma-sg.c
>
> Inside vb2_dma_sg_alloc(),
>...
>buf->num_pages = size >> PAGE_SHIFT;
>buf->dma_sgt = &buf->sg_table;
>
>buf->pages = kvmalloc_array(buf->num_pages, sizeof(struct page *),
>GFP_KERNEL |
> __GFP_ZERO);
>...
>
> buf->pages has index upto *buf->num_pages*.
>
> now inside vb2_dma_sg_mmap(),
>
>unsigned long usize = vma->vm_end - vma->vm_start;
>int i = 0;
>...
>do {
> int ret;
>
> ret = vm_insert_page(vma, uaddr, buf->pages[i++]);
> if (ret) {
>printk(KERN_ERR "Remapping memory, error:
> %d\n", ret);
>return ret;
> }
>
> uaddr += PAGE_SIZE;
> usize -= PAGE_SIZE;
>} while (usize > 0);
>...
> is it possible for any value of *i > (buf->num_pages)*,
> buf->pages[i] is going to overrun the page boundary ?
Yes it is, and you've found an array-overrun condition that is
triggerable from userspace - potentially non-root userspace too.
Depending on what it can cause to be mapped without oopsing the
kernel, it could be very serious. At best, it'll oops the kernel.
At worst, it could expose pages of memory that userspace should
not have access to.
This is why I've been saying that we need a helper that takes the
_object_ and the user request, and does all the checking internally,
so these kinds of checks do not get overlooked.
A good API is one that helpers authors avoid bugs.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH v5 0/9] Use vm_insert_range
Having discussed with Matthew offlist, I think we've come to the
following conclusion - there's a number of drivers that buggily
ignore vm_pgoff.
So, what I proposed is:
static int __vm_insert_range(struct vm_struct *vma, struct page *pages,
size_t num, unsigned long offset)
{
unsigned long count = vma_pages(vma);
unsigned long uaddr = vma->vm_start;
int ret;
/* Fail if the user requested offset is beyond the end of the object */
if (offset > num)
return -ENXIO;
/* Fail if the user requested size exceeds available object size */
if (count > num - offset)
return -ENXIO;
/* Never exceed the number of pages that the user requested */
for (i = 0; i < count; i++) {
ret = vm_insert_page(vma, uaddr, pages[offset + i]);
if (ret < 0)
return ret;
uaddr += PAGE_SIZE;
}
return 0;
}
/*
* Maps an object consisting of `num' `pages', catering for the user's
* requested vm_pgoff
*/
int vm_insert_range(struct vm_struct *vma, struct page *pages, size_t num)
{
return __vm_insert_range(vma, pages, num, vma->vm_pgoff);
}
/*
* Maps a set of pages, always starting at page[0]
*/
int vm_insert_range_buggy(struct vm_struct *vma, struct page *pages, size_t num)
{
return __vm_insert_range(vma, pages, num, 0);
}
With this, drivers such as iommu/dma-iommu.c can be converted thusly:
int iommu_dma_mmap(struct page **pages, size_t size, struct vm_area_struct
*vma+)
{
- unsigned long uaddr = vma->vm_start;
- unsigned int i, count = PAGE_ALIGN(size) >> PAGE_SHIFT;
- int ret = -ENXIO;
-
- for (i = vma->vm_pgoff; i < count && uaddr < vma->vm_end; i++) {
- ret = vm_insert_page(vma, uaddr, pages[i]);
- if (ret)
- break;
- uaddr += PAGE_SIZE;
- }
- return ret;
+ return vm_insert_range(vma, pages, PAGE_ALIGN(size) >> PAGE_SHIFT);
}
and drivers such as firewire/core-iso.c:
int fw_iso_buffer_map_vma(struct fw_iso_buffer *buffer,
struct vm_area_struct *vma)
{
- unsigned long uaddr;
- int i, err;
-
- uaddr = vma->vm_start;
- for (i = 0; i < buffer->page_count; i++) {
- err = vm_insert_page(vma, uaddr, buffer->pages[i]);
- if (err)
- return err;
-
- uaddr += PAGE_SIZE;
- }
-
- return 0;
+ return vm_insert_range_buggy(vma, buffer->pages, buffer->page_count);
}
and this gives us something to grep for to find these buggy drivers.
Now, this may not look exactly equivalent, but if you look at
fw_device_op_mmap(), buffer->page_count is basically vma_pages(vma)
at this point, which means this should be equivalent.
We _could_ then at a later date "fix" these drivers to behave according
to the normal vm_pgoff offsetting simply by removing the _buggy suffix
on the function name... and if that causes regressions, it gives us an
easy way to revert (as long as vm_insert_range_buggy() remains
available.)
In the case of firewire/core-iso.c, it currently ignores the mmap offset
entirely, so making the above suggested change would be tantamount to
causing it to return -ENXIO for any non-zero mmap offset.
IMHO, this approach is way simpler, and easier to get it correct at
each call site, rather than the current approach which seems to be
error-prone.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Re: noveau vs arm dma ops
(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
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
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
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
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
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
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(&client->dev, "wr 0x%02x: %*ph\n", addr, cnt, p);
> +
> + ret = i2c_transfer(client->adapter, &msg, 1);
> + if (ret < 0)
> + dev_err(&client->dev, "Error %d writing to cec:0x%x\n", ret,
> addr);
> + return r
[PATCH v3 1/7] drm/i2c: tda998x: move mutex/waitqueue/timer/work init early
Move the mutex, waitqueue, timer and detect work initialisation early in the driver's initialisation, rather than being after we've registered the CEC device. Acked-by: Hans Verkuil Signed-off-by: Russell King --- drivers/gpu/drm/i2c/tda998x_drv.c | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index cd3f0873bbdd..83407159e957 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1475,7 +1475,11 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) u32 video; int rev_lo, rev_hi, ret; - mutex_init(&priv->audio_mutex); /* Protect access from audio thread */ + mutex_init(&priv->mutex); /* protect the page access */ + mutex_init(&priv->audio_mutex); /* protect access from audio thread */ + init_waitqueue_head(&priv->edid_delay_waitq); + timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0); + INIT_WORK(&priv->detect_work, tda998x_detect_work); priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3); priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1); @@ -1489,11 +1493,6 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) if (!priv->cec) return -ENODEV; - mutex_init(&priv->mutex); /* protect the page access */ - init_waitqueue_head(&priv->edid_delay_waitq); - timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0); - INIT_WORK(&priv->detect_work, tda998x_detect_work); - /* wake up the device: */ cec_write(priv, REG_CEC_ENAMODS, CEC_ENAMODS_EN_RXSENS | CEC_ENAMODS_EN_HDMI); -- 2.7.4
[PATCH v3 5/7] drm/i2c: tda9950: add CEC driver
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(&client->dev, "wr 0x%02x: %*ph\n", addr, cnt, p);
+
+ ret = i2c_transfer(client->adapter, &msg, 1);
+ if (ret < 0)
+ dev_err(&client->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, &val, 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 = &addr;
+ msg[1].addr = client->addr;
+ msg[1].flags = I2C_M_RD;
+ msg[1].len = cnt;
+ msg[1].buf = p;
+
+ ret = i2c_transfer(client->adapter, msg, 2);
+ if (ret < 0)
+ dev_err(&client->dev, "Error %d reading from cec:0x%x\n", ret,
[PATCH v3 7/7] dt-bindings: tda998x: add the calibration gpio
Add the optional calibration gpio for integrated TDA9950 CEC support. This GPIO corresponds with the interrupt from the TDA998x, as the calibration requires driving the interrupt pin low. Reviewed-by: Rob Herring Signed-off-by: Russell King --- Documentation/devicetree/bindings/display/bridge/tda998x.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/devicetree/bindings/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt index 24cc2466185a..1a4eaca40d94 100644 --- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt +++ b/Documentation/devicetree/bindings/display/bridge/tda998x.txt @@ -27,6 +27,9 @@ Required properties; in question is used. The implementation allows one or two DAIs. If two DAIs are defined, they must be of different type. + - nxp,calib-gpios: calibration GPIO, which must correspond with the + gpio used for the TDA998x interrupt pin. + [1] Documentation/sound/alsa/soc/DAI.txt [2] include/dt-bindings/display/tda998x.h -- 2.7.4
[PATCH v3 4/7] drm/i2c: tda998x: always disable and clear interrupts at probe
Always disable and clear interrupts at probe time to ensure that the
TDA998x is in a sane state. This ensures that the interrupt line,
which is also the CEC clock calibration signal, is always deasserted.
Acked-by: Hans Verkuil
Signed-off-by: Russell King
---
drivers/gpu/drm/i2c/tda998x_drv.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 7f2762fab5c9..16e0439cad44 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1546,6 +1546,15 @@ static int tda998x_create(struct i2c_client *client,
struct tda998x_priv *priv)
cec_write(priv, REG_CEC_FRO_IM_CLK_CTRL,
CEC_FRO_IM_CLK_CTRL_GHOST_DIS |
CEC_FRO_IM_CLK_CTRL_IMCLK_SEL);
+ /* ensure interrupts are disabled */
+ cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
+
+ /* clear pending interrupts */
+ cec_read(priv, REG_CEC_RXSHPDINT);
+ reg_read(priv, REG_INT_FLAGS_0);
+ reg_read(priv, REG_INT_FLAGS_1);
+ reg_read(priv, REG_INT_FLAGS_2);
+
/* initialize the optional IRQ */
if (client->irq) {
unsigned long irq_flags;
@@ -1553,11 +1562,6 @@ static int tda998x_create(struct i2c_client *client,
struct tda998x_priv *priv)
/* init read EDID waitqueue and HDP work */
init_waitqueue_head(&priv->wq_edid);
- /* clear pending interrupts */
- reg_read(priv, REG_INT_FLAGS_0);
- reg_read(priv, REG_INT_FLAGS_1);
- reg_read(priv, REG_INT_FLAGS_2);
-
irq_flags =
irqd_get_trigger_type(irq_get_irq_data(client->irq));
irq_flags |= IRQF_SHARED | IRQF_ONESHOT;
--
2.7.4
[PATCH v3 6/7] drm/i2c: tda998x: add CEC support
The TDA998x is a HDMI transmitter with a TDA9950 CEC engine integrated
onto the same die. Add support for the TDA9950 CEC engine to the
TDA998x driver.
Signed-off-by: Russell King
---
drivers/gpu/drm/i2c/Kconfig | 1 +
drivers/gpu/drm/i2c/tda998x_drv.c | 195 --
2 files changed, 187 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
index 3a232f5ff0a1..65d3acb61c03 100644
--- a/drivers/gpu/drm/i2c/Kconfig
+++ b/drivers/gpu/drm/i2c/Kconfig
@@ -22,6 +22,7 @@ config DRM_I2C_SIL164
config DRM_I2C_NXP_TDA998X
tristate "NXP Semiconductors TDA998X HDMI encoder"
default m if DRM_TILCDC
+ select CEC_CORE if CEC_NOTIFIER
select SND_SOC_HDMI_CODEC if SND_SOC
help
Support for NXP Semiconductors TDA998X HDMI encoders.
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 16e0439cad44..eb9916bd84a4 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -16,8 +16,10 @@
*/
#include
+#include
#include
#include
+#include
#include
#include
#include
@@ -29,6 +31,8 @@
#include
#include
+#include
+
#define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
struct tda998x_audio_port {
@@ -55,6 +59,7 @@ struct tda998x_priv {
struct platform_device *audio_pdev;
struct mutex audio_mutex;
+ struct mutex edid_mutex;
wait_queue_head_t wq_edid;
volatile int wq_edid_wait;
@@ -67,6 +72,9 @@ struct tda998x_priv {
struct drm_connector connector;
struct tda998x_audio_port audio_port[2];
+ struct tda9950_glue cec_glue;
+ struct gpio_desc *calib;
+ struct cec_notifier *cec_notify;
};
#define conn_to_tda998x_priv(x) \
@@ -345,6 +353,12 @@ struct tda998x_priv {
#define REG_CEC_INTSTATUS0xee/* read */
# define CEC_INTSTATUS_CEC (1 << 0)
# define CEC_INTSTATUS_HDMI (1 << 1)
+#define REG_CEC_CAL_XOSC_CTRL10xf2
+# define CEC_CAL_XOSC_CTRL1_ENA_CALBIT(0)
+#define REG_CEC_DES_FREQ2 0xf5
+# define CEC_DES_FREQ2_DIS_AUTOCAL BIT(7)
+#define REG_CEC_CLK 0xf6
+# define CEC_CLK_FRO 0x11
#define REG_CEC_FRO_IM_CLK_CTRL 0xfb/* read/write */
# define CEC_FRO_IM_CLK_CTRL_GHOST_DIS (1 << 7)
# define CEC_FRO_IM_CLK_CTRL_ENA_OTP (1 << 6)
@@ -359,6 +373,7 @@ struct tda998x_priv {
# define CEC_RXSHPDLEV_HPD(1 << 1)
#define REG_CEC_ENAMODS 0xff/* read/write */
+# define CEC_ENAMODS_EN_CEC_CLK (1 << 7)
# define CEC_ENAMODS_DIS_FRO (1 << 6)
# define CEC_ENAMODS_DIS_CCLK (1 << 5)
# define CEC_ENAMODS_EN_RXSENS(1 << 2)
@@ -417,6 +432,114 @@ cec_read(struct tda998x_priv *priv, u8 addr)
return val;
}
+static void cec_enamods(struct tda998x_priv *priv, u8 mods, bool enable)
+{
+ int val = cec_read(priv, REG_CEC_ENAMODS);
+
+ if (val < 0)
+ return;
+
+ if (enable)
+ val |= mods;
+ else
+ val &= ~mods;
+
+ cec_write(priv, REG_CEC_ENAMODS, val);
+}
+
+static void tda998x_cec_set_calibration(struct tda998x_priv *priv, bool enable)
+{
+ if (enable) {
+ u8 val;
+
+ cec_write(priv, 0xf3, 0xc0);
+ cec_write(priv, 0xf4, 0xd4);
+
+ /* Enable automatic calibration mode */
+ val = cec_read(priv, REG_CEC_DES_FREQ2);
+ val &= ~CEC_DES_FREQ2_DIS_AUTOCAL;
+ cec_write(priv, REG_CEC_DES_FREQ2, val);
+
+ /* Enable free running oscillator */
+ cec_write(priv, REG_CEC_CLK, CEC_CLK_FRO);
+ cec_enamods(priv, CEC_ENAMODS_DIS_FRO, false);
+
+ cec_write(priv, REG_CEC_CAL_XOSC_CTRL1,
+ CEC_CAL_XOSC_CTRL1_ENA_CAL);
+ } else {
+ cec_write(priv, REG_CEC_CAL_XOSC_CTRL1, 0);
+ }
+}
+
+/*
+ * Calibration for the internal oscillator: we need to set calibration mode,
+ * and then pulse the IRQ line low for a 10ms ± 1% period.
+ */
+static void tda998x_cec_calibration(struct tda998x_priv *priv)
+{
+ struct gpio_desc *calib = priv->calib;
+
+ mutex_lock(&priv->edid_mutex);
+ if (priv->hdmi->irq > 0)
+ disable_irq(priv->hdmi->irq);
+ gpiod_direction_output(calib, 1);
+ tda998x_cec_set_calibration(priv, true);
+
+ local_irq_disable();
+ gpiod_set_value(calib, 0);
+ mdelay(10);
+ gpiod_set_value(calib, 1);
+ local_irq_enable();
+
+ tda998x_cec_set_calibration(priv, false);
+ gpiod_direction_input(calib);
+ if (priv->hdmi->irq > 0)
+ enable_irq(priv->hdmi->irq);
+ mutex_unlock(&priv->edid_mut
[PATCH v3 3/7] drm/i2c: tda998x: move CEC device initialisation later
We no longer use the CEC client to access the CEC part itself, so we can
move this later in the initialisation sequence.
Acked-by: Hans Verkuil
Signed-off-by: Russell King
---
drivers/gpu/drm/i2c/tda998x_drv.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 2a99930f1bda..7f2762fab5c9 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1489,9 +1489,6 @@ static int tda998x_create(struct i2c_client *client,
struct tda998x_priv *priv)
priv->cec_addr = 0x34 + (client->addr & 0x03);
priv->current_page = 0xff;
priv->hdmi = client;
- priv->cec = i2c_new_dummy(client->adapter, priv->cec_addr);
- if (!priv->cec)
- return -ENODEV;
/* wake up the device: */
cec_write(priv, REG_CEC_ENAMODS,
@@ -1578,6 +1575,12 @@ static int tda998x_create(struct i2c_client *client,
struct tda998x_priv *priv)
cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD);
}
+ priv->cec = i2c_new_dummy(client->adapter, priv->cec_addr);
+ if (!priv->cec) {
+ ret = -ENODEV;
+ goto fail;
+ }
+
/* enable EDID read irq: */
reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
@@ -1594,14 +1597,14 @@ static int tda998x_create(struct i2c_client *client,
struct tda998x_priv *priv)
ret = tda998x_get_audio_ports(priv, np);
if (ret)
- goto err_audio;
+ goto fail;
if (priv->audio_port[0].format != AFMT_UNUSED)
tda998x_audio_codec_init(priv, &client->dev);
return 0;
-err_audio:
+fail:
if (client->irq)
free_irq(client->irq, priv);
err_irq:
--
2.7.4
[PATCH v3 2/7] drm/i2c: tda998x: fix error cleanup paths
If tda998x_get_audio_ports() fails, and we requested the interrupt, we
fail to free the interrupt before returning failure. Rework the failure
cleanup code and exit paths so that we always clean up properly after an
error, and always propagate the error code.
Acked-by: Hans Verkuil
Signed-off-by: Russell King
---
drivers/gpu/drm/i2c/tda998x_drv.c | 31 ++-
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 83407159e957..2a99930f1bda 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1501,10 +1501,15 @@ static int tda998x_create(struct i2c_client *client,
struct tda998x_priv *priv)
/* read version: */
rev_lo = reg_read(priv, REG_VERSION_LSB);
+ if (rev_lo < 0) {
+ dev_err(&client->dev, "failed to read version: %d\n", rev_lo);
+ return rev_lo;
+ }
+
rev_hi = reg_read(priv, REG_VERSION_MSB);
- if (rev_lo < 0 || rev_hi < 0) {
- ret = rev_lo < 0 ? rev_lo : rev_hi;
- goto fail;
+ if (rev_hi < 0) {
+ dev_err(&client->dev, "failed to read version: %d\n", rev_hi);
+ return rev_hi;
}
priv->rev = rev_lo | rev_hi << 8;
@@ -1528,7 +1533,7 @@ static int tda998x_create(struct i2c_client *client,
struct tda998x_priv *priv)
default:
dev_err(&client->dev, "found unsupported device: %04x\n",
priv->rev);
- goto fail;
+ return -ENXIO;
}
/* after reset, enable DDC: */
@@ -1566,7 +1571,7 @@ static int tda998x_create(struct i2c_client *client,
struct tda998x_priv *priv)
dev_err(&client->dev,
"failed to request IRQ#%u: %d\n",
client->irq, ret);
- goto fail;
+ goto err_irq;
}
/* enable HPD irq */
@@ -1589,19 +1594,19 @@ static int tda998x_create(struct i2c_client *client,
struct tda998x_priv *priv)
ret = tda998x_get_audio_ports(priv, np);
if (ret)
- goto fail;
+ goto err_audio;
if (priv->audio_port[0].format != AFMT_UNUSED)
tda998x_audio_codec_init(priv, &client->dev);
return 0;
-fail:
- /* if encoder_init fails, the encoder slave is never registered,
-* so cleanup here:
-*/
- if (priv->cec)
- i2c_unregister_device(priv->cec);
- return -ENXIO;
+
+err_audio:
+ if (client->irq)
+ free_irq(client->irq, priv);
+err_irq:
+ i2c_unregister_device(priv->cec);
+ return ret;
}
static void tda998x_encoder_prepare(struct drm_encoder *encoder)
--
2.7.4
[PATCH v3 0/7] TDA998x CEC support
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
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
> cw1
Re: [PATCH 1/3] drm: bridge: synopsys/dw-hdmi: Enable cec clock
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
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
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&id=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
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
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
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
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: > > http://www.spinics.net/lists/linux-media/msg122689.html> 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
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
[PATCH] imx-csi: fix burst size
Setting a burst size of "8" doesn't work for IMX219 with 8-bit bayer, but a burst size of "16" does. Fix this. Signed-off-by: Russell King --- drivers/staging/media/imx/imx-media-csi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c index 6d856118c223..e27bcdb63973 100644 --- a/drivers/staging/media/imx/imx-media-csi.c +++ b/drivers/staging/media/imx/imx-media-csi.c @@ -341,7 +341,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; -- 2.7.4
[PATCH RFC] media: staging/imx: fix complete handler
The complete handler walks all entities, expecting to find an imx
subdevice for each and every entity.
However, camera drivers such as smiapp can themselves contain multiple
entities, for which there will not be an imx subdevice. This causes
imx_media_find_subdev_by_sd() to fail, making the imx capture system
unusable with such cameras.
Work around this by killing the error entirely, thereby allowing
the imx capture to be used with such cameras.
Signed-off-by: Russell King
---
Not the best solution, but the only one I can think of to fix the
regression that happened sometime between a previous revision of
Steve's patch set and the version that got merged.
drivers/staging/media/imx/imx-media-dev.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-dev.c
b/drivers/staging/media/imx/imx-media-dev.c
index d96f4512224f..6ba59939dd7a 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -345,8 +345,11 @@ static int imx_media_add_vdev_to_pad(struct imx_media_dev
*imxmd,
sd = media_entity_to_v4l2_subdev(entity);
imxsd = imx_media_find_subdev_by_sd(imxmd, sd);
- if (IS_ERR(imxsd))
- return PTR_ERR(imxsd);
+ if (IS_ERR(imxsd)) {
+ v4l2_err(&imxmd->v4l2_dev, "failed to find subdev for entity
%s, sd %p err %ld\n",
+entity->name, sd, PTR_ERR(imxsd));
+ return 0;
+ }
imxpad = &imxsd->pad[srcpad->index];
vdev_idx = imxpad->num_vdevs;
--
2.7.4
[PATCH RFC] [media] v4l: async: don't bomb out on ->complete failure
When a subdev is being registered via v4l2_async_register_subdev(), we
test to see if we have all the components, and if so, we call the
->complete() handler.
However, the error handling is broken - if notifier->complete() returns
an error, we propagate the error out of v4l2_async_register_subdev(),
but leaving the subdev bound and on the notifier's "done" list.
Drivers calling v4l2_async_register_subdev() assume that this means
that v4l2_async_register_subdev() failed, which causes probe failure
and the memory backing the subdev to be freed.
At this point, we now have a subdev on the notifier's done list which
has been freed. If we then try to remove the notifier via
v4l2_async_notifier_unregister(), we walk the notifier's done list,
cleaning up the subdevs - and hit the freed subdev, causing a kernel
oops such as:
Unable to handle kernel paging request at virtual address 6e6f6994
pgd = d039c000
[6e6f6994] *pgd=
Internal error: Oops: 5 [#1] SMP ARM
Modules linked in: mux_mmio caam_jr snd_soc_imx_sgtl5000 snd_soc_fsl_asoc_card
snd_soc_imx_spdif imx_media_csi(C) ci_hdrc_imx snd_soc_imx_audmux
imx6_mipi_csi2(C) imx219 snd_soc_sgtl5000 ci_hdrc usbmisc_imx udc_core
caam video_mux mux_core imx_media_ic(C) imx_sdma imx_media_vdic(C)
imx_media_capture(C) imx2_wdt snd_soc_fsl_ssi snd_soc_fsl_spdif
imx_pcm_dma coda v4l2_mem2mem videobuf2_v4l2 imx_vdoa videobuf2_dma_contig
videobuf2_core imx_thermal videobuf2_vmalloc videobuf2_memops imx_media(C-)
imx_media_common(C) v4l2_fwnode nfsd rc_pinnacle_pctv_hd dw_hdmi_ahb_audio
dw_hdmi_cec etnaviv
CPU: 1 PID: 8039 Comm: rmmod Tainted: G C 4.14.0-rc1+ #2194
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
task: ee522700 task.stack: e5f68000
PC is at __lock_acquire+0x98/0x13f4
LR is at lock_acquire+0xd8/0x250
pc : []lr : []psr: 200e0093
sp : e5f69d28 ip : e5f68000 fp : e5f69da4
r10: r9 : r8 : 6e6f6994
r7 : r6 : c0aa4f64 r5 : ee522700 r4 : c13f4abc
r3 : r2 : 0001 r1 : r0 : 6e6f6994
Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none
Control: 10c5387d Table: 2039c04a DAC: 0051
Process rmmod (pid: 8039, stack limit = 0xe5f68210)
Backtrace:
[] (__lock_acquire) from [] (lock_acquire+0xd8/0x250)
[] (lock_acquire) from [] (_raw_spin_lock+0x34/0x44)
[] (_raw_spin_lock) from []
(v4l2_device_unregister_subdev+0x2c/0xb0)
[] (v4l2_device_unregister_subdev) from []
(v4l2_async_cleanup+0x14/0x40)
[] (v4l2_async_cleanup) from []
(v4l2_async_notifier_unregister+0xa8/0x1ec)
[] (v4l2_async_notifier_unregister) from []
(imx_media_remove+0x2c/0x54 [imx_media])
[] (imx_media_remove [imx_media]) from []
(platform_drv_remove+0x2c/0x44)
[] (platform_drv_remove) from []
(device_release_driver_internal+0x158/0x1f8)
[] (device_release_driver_internal) from []
(driver_detach+0x40/0x74)
[] (driver_detach) from [] (bus_remove_driver+0x54/0x98)
[] (bus_remove_driver) from [] (driver_unregister+0x30/0x50)
[] (driver_unregister) from []
(platform_driver_unregister+0x14/0x18)
[] (platform_driver_unregister) from []
(imx_media_pdrv_exit+0x14/0x1c [imx_media])
[] (imx_media_pdrv_exit [imx_media]) from []
(SyS_delete_module+0x170/0x1c0)
[] (SyS_delete_module) from [] (ret_fast_syscall+0x0/0x1c)
Code: 1a0e e3a0 e24bd028 e89daff0 (e598)
---[ end trace 97732329ac63e5ae ]---
Avoid this by reporting an error to the kernel message log about the
failure, rather than silently propagating the error from ->complete()
and causing this latent use-after-free oops.
Signed-off-by: Russell King
---
drivers/media/v4l2-core/v4l2-async.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-async.c
b/drivers/media/v4l2-core/v4l2-async.c
index d741a8e0fdac..8d221f4a8a8d 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -122,8 +122,12 @@ static int v4l2_async_test_notify(struct
v4l2_async_notifier *notifier,
/* Move from the global subdevice list to notifier's done */
list_move(&sd->async_list, ¬ifier->done);
- if (list_empty(¬ifier->waiting) && notifier->complete)
- return notifier->complete(notifier);
+ if (list_empty(¬ifier->waiting) && notifier->complete) {
+ int ret = notifier->complete(notifier);
+ if (ret)
+ dev_err(notifier->v4l2_dev->dev,
+ "complete notifier failed: %d\n", ret);
+ }
return 0;
}
--
2.7.4
Re: [PATCH v3 0/2] [media] videobuf2-dc: Add support for cacheable MMAP
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)
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
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
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
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
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, &cc); > > > > 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
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
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
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
> >>>---
> >>>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(&imxmd->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 = &local_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 soluti
Re: [RFC] [media] imx: assume MEDIA_ENT_F_ATV_DECODER entities output video on pad 1
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(&imxmd->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 = &local_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
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
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
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
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
> > > 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 = <&ipu1_csi0>, <&ipu1_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
&g
Re: [PATCHv6 01/10] media: add CEC notifier support
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(&cec_notifiers_lock);
> >> + list_for_each_entry(n, &cec_notifiers, head) {
> >> + if (n->dev == dev) {
> >> + mutex_unlock(&cec_notifiers_lock);
> >> + kref_get(&n->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
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(&cec_notifiers_lock);
> + list_for_each_entry(n, &cec_notifiers, head) {
> + if (n->dev == dev) {
> + mutex_unlock(&cec_notifiers_lock);
> + kref_get(&n->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
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
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.
[PATCH] dma-buf: align debugfs output
Align the heading with the values output from debugfs.
Signed-off-by: Russell King
---
drivers/dma-buf/dma-buf.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index ebaf1923ad6b..f72aaacbe023 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -1072,7 +1072,8 @@ static int dma_buf_debug_show(struct seq_file *s, void
*unused)
return ret;
seq_puts(s, "\nDma-buf Objects:\n");
- seq_puts(s, "size\tflags\tmode\tcount\texp_name\n");
+ seq_printf(s, "%-8s\t%-8s\t%-8s\t%-8s\texp_name\n",
+ "size", "flags", "mode", "count");
list_for_each_entry(buf_obj, &db_list.head, list_node) {
ret = mutex_lock_interruptible(&buf_obj->lock);
--
2.7.4
[PATCH] dma-buf: fence debugging
Add debugfs output to report shared and exclusive fences on a dma_buf
object. This produces output such as:
Dma-buf Objects:
sizeflags modecount exp_name
0829440000050005drm
Exclusive fence: etnaviv 134000.gpu signalled
Attached Devices:
gpu-subsystem
Total 1 devices attached
Total 1 objects, 8294400 bytes
Signed-off-by: Russell King
---
drivers/dma-buf/dma-buf.c | 34 +-
1 file changed, 33 insertions(+), 1 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 0007b792827b..f72aaacbe023 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -1059,7 +1059,11 @@ static int dma_buf_debug_show(struct seq_file *s, void
*unused)
int ret;
struct dma_buf *buf_obj;
struct dma_buf_attachment *attach_obj;
- int count = 0, attach_count;
+ struct reservation_object *robj;
+ struct reservation_object_list *fobj;
+ struct dma_fence *fence;
+ unsigned seq;
+ int count = 0, attach_count, shared_count, i;
size_t size = 0;
ret = mutex_lock_interruptible(&db_list.lock);
@@ -1085,6 +1090,34 @@ static int dma_buf_debug_show(struct seq_file *s, void
*unused)
file_count(buf_obj->file),
buf_obj->exp_name);
+ robj = buf_obj->resv;
+ while (true) {
+ seq = read_seqcount_begin(&robj->seq);
+ rcu_read_lock();
+ fobj = rcu_dereference(robj->fence);
+ shared_count = fobj ? fobj->shared_count : 0;
+ fence = rcu_dereference(robj->fence_excl);
+ if (!read_seqcount_retry(&robj->seq, seq))
+ break;
+ rcu_read_unlock();
+ }
+
+ if (fence)
+ seq_printf(s, "\tExclusive fence: %s %s %ssignalled\n",
+ fence->ops->get_driver_name(fence),
+ fence->ops->get_timeline_name(fence),
+ dma_fence_is_signaled(fence) ? "" : "un");
+ for (i = 0; i < shared_count; i++) {
+ fence = rcu_dereference(fobj->shared[i]);
+ if (!dma_fence_get_rcu(fence))
+ continue;
+ seq_printf(s, "\tShared fence: %s %s %ssignalled\n",
+ fence->ops->get_driver_name(fence),
+ fence->ops->get_timeline_name(fence),
+ dma_fence_is_signaled(fence) ? "" : "un");
+ }
+ rcu_read_unlock();
+
seq_puts(s, "\tAttached Devices:\n");
attach_count = 0;
--
2.7.4
Re: [PATCHv5 04/11] exynos_hdmi: add CEC notifier support
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
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
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
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 = <&ipu1_csi0>, <&ipu1_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
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, &fmt) != 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
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(&sdformat->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
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, &sel->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
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
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
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, &format, pad, which);
> > if (ret != 0)
> > return;
> >
> > - ret = v4l2_subdev_get_frame_interval(entity, &interval, pad);
> > - if (ret != 0 && ret != -ENOTTY)
> > - return;
> > + err_fi = v4l2_subdev_get_frame_interval(entity, &interval, 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
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
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, &format, pad, which);
if (ret != 0)
return;
- ret = v4l2_subdev_get_frame_interval(entity, &interval, pad);
- if (ret != 0 && ret != -ENOTTY)
- return;
+ err_fi = v4l2_subdev_get_frame_interval(entity, &interval, 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
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 > > > > 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
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
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
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
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
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
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
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,
> > &fie);
> > + 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
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, &fse);
> >+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
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
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
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
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
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
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
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
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.
[PATCH 3/4] media: imx-csi: add frame size/interval enumeration
Add frame size and frame interval enumeration to CSI.
CSI can scale the image independently horizontally and vertically by a
factor of two, which enumerates to four different frame sizes.
CSI can also drop frames, resulting in frame rate reduction, so
enumerate the resulting possible output frame rates.
Signed-off-by: Russell King
---
drivers/staging/media/imx/imx-media-csi.c | 51 +++
1 file changed, 51 insertions(+)
diff --git a/drivers/staging/media/imx/imx-media-csi.c
b/drivers/staging/media/imx/imx-media-csi.c
index 65346e789dd6..d19659d7ddc2 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1073,6 +1073,55 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd,
return ret;
}
+static int csi_enum_frame_size(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_size_enum *fse)
+{
+ struct csi_priv *priv = v4l2_get_subdevdata(sd);
+ struct v4l2_mbus_framefmt *fmt;
+ int ret = 0;
+
+ if (fse->pad >= CSI_NUM_PADS ||
+ fse->index > (fse->pad == CSI_SINK_PAD ? 0 : 3))
+ return -EINVAL;
+
+ mutex_lock(&priv->lock);
+ fmt = __csi_get_fmt(priv, cfg, fse->pad, fse->which);
+ fse->min_width = fse->max_width = fse->index & 1 ?
+ fmt->width >> 1 : fmt->width;
+ fse->min_height = fse->max_height = fse->index & 2 ?
+ fmt->height >> 1 : fmt->height;
+ mutex_unlock(&priv->lock);
+
+ return ret;
+}
+
+static int csi_enum_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_interval_enum *fie)
+{
+ struct csi_priv *priv = v4l2_get_subdevdata(sd);
+ struct v4l2_mbus_framefmt *fmt;
+ int ret = 0;
+
+ if (fie->pad >= CSI_NUM_PADS ||
+ fie->index >= (fie->pad == CSI_SINK_PAD ? 1 : ARRAY_SIZE(csi_skip)))
+ return -EINVAL;
+
+ mutex_lock(&priv->lock);
+ fmt = __csi_get_fmt(priv, cfg, fie->pad, fie->which);
+ if ((fie->width == fmt->width || fie->width == fmt->width / 2) &&
+ (fie->height == fmt->height || fie->height == fmt->height / 2)) {
+ fie->interval = priv->frame_interval;
+ csi_apply_skip_interval(&csi_skip[fie->index], &fie->interval);
+ } else {
+ ret = -EINVAL;
+ }
+ mutex_unlock(&priv->lock);
+
+ return ret;
+}
+
static int csi_get_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *sdformat)
@@ -1473,6 +1522,8 @@ static struct v4l2_subdev_video_ops csi_video_ops = {
static struct v4l2_subdev_pad_ops csi_pad_ops = {
.enum_mbus_code = csi_enum_mbus_code,
+ .enum_frame_size = csi_enum_frame_size,
+ .enum_frame_interval = csi_enum_frame_interval,
.get_fmt = csi_get_fmt,
.set_fmt = csi_set_fmt,
.get_selection = csi_get_selection,
--
2.7.4
[PATCH 4/4] media: imx-media-capture: add frame sizes/interval enumeration
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, &fse);
+ if (ret)
+ return ret;
+
+ fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
+ fsize->discrete.width = fse.min_width;
+ fsize->discrete.height = fse.max_height;
+
+ return 0;
+}
+
+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,
&fie);
+ if (ret)
+ return ret;
+
+ fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
+ fival->discrete = fie.interval;
+
+ return 0;
+}
+
static int capture_enum_fmt_vid_cap(struct file *file, void *fh,
struct v4l2_fmtdesc *f)
{
@@ -270,6 +329,9 @@ static int capture_s_parm(struct file *file, void *fh,
static const struct v4l2_ioctl_ops capture_ioctl_ops = {
.vidioc_querycap= vidioc_querycap,
+ .vidioc_enum_framesizes = capture_enum_framesizes,
+ .vidioc_enum_frameintervals = capture_enum_frameintervals,
+
.vidioc_enum_fmt_vid_cap= capture_enum_fmt_vid_cap,
.vidioc_g_fmt_vid_cap = capture_g_fmt_vid_cap,
.vidioc_try_fmt_vid_cap = capture_try_fmt_vid_cap,
--
2.7.4
[PATCH 2/4] media: imx: allow bayer pixel formats to be looked up
Allow imx_media_find_format() to look up bayer formats, which is
required to support frame size and interval enumeration.
Signed-off-by: Russell King
---
drivers/staging/media/imx/imx-media-capture.c | 11 ++-
drivers/staging/media/imx/imx-media-utils.c | 6 +++---
drivers/staging/media/imx/imx-media.h | 2 +-
3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-capture.c
b/drivers/staging/media/imx/imx-media-capture.c
index ee914396080f..cdeb2cd8b1d7 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -164,10 +164,10 @@ static int capture_try_fmt_vid_cap(struct file *file,
void *fh,
CS_SEL_YUV : CS_SEL_RGB;
fourcc = f->fmt.pix.pixelformat;
- cc = imx_media_find_format(fourcc, cs_sel);
+ cc = imx_media_find_format(fourcc, cs_sel, false);
if (!cc) {
imx_media_enum_format(&fourcc, 0, cs_sel);
- cc = imx_media_find_format(fourcc, cs_sel);
+ cc = imx_media_find_format(fourcc, cs_sel, false);
}
}
@@ -193,7 +193,7 @@ static int capture_s_fmt_vid_cap(struct file *file, void
*fh,
priv->vdev.fmt.fmt.pix = f->fmt.pix;
priv->vdev.cc = imx_media_find_format(f->fmt.pix.pixelformat,
- CS_SEL_ANY);
+ CS_SEL_ANY, false);
return 0;
}
@@ -505,7 +505,8 @@ void imx_media_capture_device_set_format(struct
imx_media_video_dev *vdev,
mutex_lock(&priv->mutex);
priv->vdev.fmt.fmt.pix = *pix;
- priv->vdev.cc = imx_media_find_format(pix->pixelformat, CS_SEL_ANY);
+ priv->vdev.cc = imx_media_find_format(pix->pixelformat, CS_SEL_ANY,
+ false);
mutex_unlock(&priv->mutex);
}
EXPORT_SYMBOL_GPL(imx_media_capture_device_set_format);
@@ -614,7 +615,7 @@ int imx_media_capture_device_register(struct
imx_media_video_dev *vdev)
imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt.fmt.pix,
&fmt_src.format, NULL);
vdev->cc = imx_media_find_format(vdev->fmt.fmt.pix.pixelformat,
-CS_SEL_ANY);
+CS_SEL_ANY, false);
v4l2_info(sd, "Registered %s as /dev/%s\n", vfd->name,
video_device_node_name(vfd));
diff --git a/drivers/staging/media/imx/imx-media-utils.c
b/drivers/staging/media/imx/imx-media-utils.c
index 6eb7e3c5279e..d048e4a080d0 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -329,9 +329,9 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
}
const struct imx_media_pixfmt *
-imx_media_find_format(u32 fourcc, enum codespace_sel cs_sel)
+imx_media_find_format(u32 fourcc, enum codespace_sel cs_sel, bool allow_bayer)
{
- return find_format(fourcc, 0, cs_sel, true, false);
+ return find_format(fourcc, 0, cs_sel, true, allow_bayer);
}
EXPORT_SYMBOL_GPL(imx_media_find_format);
@@ -524,7 +524,7 @@ int imx_media_ipu_image_to_mbus_fmt(struct
v4l2_mbus_framefmt *mbus,
{
const struct imx_media_pixfmt *fmt;
- fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY);
+ fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY, false);
if (!fmt)
return -EINVAL;
diff --git a/drivers/staging/media/imx/imx-media.h
b/drivers/staging/media/imx/imx-media.h
index 234242271a13..d8c9536bf1f8 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -178,7 +178,7 @@ enum codespace_sel {
};
const struct imx_media_pixfmt *
-imx_media_find_format(u32 fourcc, enum codespace_sel cs_sel);
+imx_media_find_format(u32 fourcc, enum codespace_sel cs_sel, bool allow_bayer);
int imx_media_enum_format(u32 *fourcc, u32 index, enum codespace_sel cs_sel);
const struct imx_media_pixfmt *
imx_media_find_mbus_format(u32 code, enum codespace_sel cs_sel,
--
2.7.4
[PATCH 1/4] media: imx-media-csi: fix v4l2-compliance check
v4l2-compliance was failing with:
fail: v4l2-test-formats.cpp(1076): cap->timeperframe.numerator
== 0 || cap->timeperframe.denominator == 0
test VIDIOC_G/S_PARM: FAIL
Fix this.
Signed-off-by: Russell King
---
drivers/staging/media/imx/imx-media-csi.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/media/imx/imx-media-csi.c
b/drivers/staging/media/imx/imx-media-csi.c
index 0336891069dc..65346e789dd6 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -680,8 +680,10 @@ static const struct csi_skip_desc
*csi_find_best_skip(struct v4l2_fract *in,
/* Default to 1:1 ratio */
if (out->numerator == 0 || out->denominator == 0 ||
- in->numerator == 0 || in->denominator == 0)
+ in->numerator == 0 || in->denominator == 0) {
+ *out = *in;
return best_skip;
+ }
want_us = div_u64((u64)USEC_PER_SEC * out->numerator, out->denominator);
--
2.7.4
Re: [PATCH v5 00/39] i.MX Media Driver
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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, &image); - 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; passthr
Re: [PATCH v5 00/39] i.MX Media Driver
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
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.
