Re: [PATCH 04/22] media: Move v4l2_fwnode_parse_link from v4l2 to driver base

[Russell King - ARM Linux admin]

On Wed, Aug 14, 2019 at 04:00:30PM -0700, Steve Longerbeam wrote:
> 
> 
> On 8/14/19 3:04 PM, Russell King - ARM Linux admin wrote:
> > On Wed, Aug 14, 2019 at 12:04:41PM -0700, Steve Longerbeam wrote:
> > > 
> > > On 8/14/19 3:30 AM, Russell King - ARM Linux admin wrote:
> > > > On Tue, Aug 06, 2019 at 09:53:41AM -0700, Steve Longerbeam wrote:
> > > > > The full patchset doesn't seem to be up yet, but see [1] for the cover
> > > > > letter.
> > > > Was the entire series copied to the mailing lists, or just selected
> > > > patches?  I only saw 4, 9, 11 and 13-22 via lakml.
> > > The whole series was posted to the linux-media ML, see [1]. At the time,
> > > none of the linux-media ML archives had the whole series.
> > > 
> > > > In the absence of the other patches, will this solve imx-media binding
> > > > the internal subdevs of sensor devices to the CSI2 interface?
> > > "internal subdevs of sensor devices" ?? That doesn't make any sense.
> > Sorry, but it makes complete sense when you consider that sensor
> > devices may have more than one subdev, but there should be only one
> > that is the "output" to whatever the camera is attached to.  The
> > other subdevs are internal to the sensor.
> 
> Ah, thanks for the clarification. Yes, by "internal subdevs" I understand
> what you mean now. The adv748x and smiapp are examples.
> 
> > 
> > subdevs are not purely the remit of SoC drivers.
> 
> So there is no binding of internal subdevs to the receiver CSI-2. The
> receiver CSI-2 subdev will create media links to the subdev that has an
> externally exposed fwnode endpoint that connects with the CSI-2 sink pad.

Maybe - with 5.2, I get:

- entity 15: imx6-mipi-csi2 (5 pads, 6 links)
 type V4L2 subdev subtype Unknown flags 0
 device node name /dev/v4l-subdev2
pad0: Sink
...
<- "imx219 0-0010":0 []
<- "imx219 pixel 0-0010":0 []

Adding some debug in gives:

[   11.963362] imx-media: imx_media_create_of_links() for imx6-mipi-csi2
[   11.963396] imx-media: create_of_link(): 
/soc/aips-bus@200/iomuxc-gpr@20e/ipu1_csi0_mux
[   11.963422] imx-media: create_of_link(): /soc/ipu@240
[   11.963450] imx-media: create_of_link(): /soc/ipu@280
[   11.963478] imx-media: create_of_link(): 
/soc/aips-bus@200/iomuxc-gpr@20e/ipu2_csi1_mux
[   11.963489] imx-media: imx6-mipi-csi2:4 -> ipu2_csi1_mux:0
[   11.963522] imx-media: create_of_link(): 
/soc/aips-bus@210/i2c@21a/camera@10
[   11.963533] imx-media: imx219 0-0010:0 -> imx6-mipi-csi2:0
[   11.963549] imx-media: imx_media_create_of_links() for imx219 pixel 0-0010
[   11.963577] imx-media: create_of_link(): /soc/aips-bus@210/mipi@21dc000
[   11.963587] imx-media: imx219 pixel 0-0010:0 -> imx6-mipi-csi2:0
[   11.963602] imx-media: imx_media_create_of_links() for imx219 0-0010

Note that it's not created by imx6-mipi-csi2, but by imx-media delving
around in the imx219 subdevs.

>From what I can see, smiapp does the same thing that I do in imx219 -
sets the subdev->dev member to point at the struct device, which then
means that v4l2_device_register_subdev() will associate the same fwnode
with both "imx219 pixel 0-0010" and "imx219 0-0010".

-- 
RMK's Patch system: https://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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 04/22] media: Move v4l2_fwnode_parse_link from v4l2 to driver base

[Russell King - ARM Linux admin]

On Wed, Aug 14, 2019 at 12:04:41PM -0700, Steve Longerbeam wrote:
> 
> 
> On 8/14/19 3:30 AM, Russell King - ARM Linux admin wrote:
> > On Tue, Aug 06, 2019 at 09:53:41AM -0700, Steve Longerbeam wrote:
> > > The full patchset doesn't seem to be up yet, but see [1] for the cover
> > > letter.
> > Was the entire series copied to the mailing lists, or just selected
> > patches?  I only saw 4, 9, 11 and 13-22 via lakml.
> 
> The whole series was posted to the linux-media ML, see [1]. At the time,
> none of the linux-media ML archives had the whole series.
> 
> > In the absence of the other patches, will this solve imx-media binding
> > the internal subdevs of sensor devices to the CSI2 interface?
> 
> "internal subdevs of sensor devices" ?? That doesn't make any sense.

Sorry, but it makes complete sense when you consider that sensor
devices may have more than one subdev, but there should be only one
that is the "output" to whatever the camera is attached to.  The
other subdevs are internal to the sensor.

subdevs are not purely the remit of SoC drivers.

-- 
RMK's Patch system: https://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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 04/22] media: Move v4l2_fwnode_parse_link from v4l2 to driver base

[Russell King - ARM Linux admin]

On Tue, Aug 06, 2019 at 09:53:41AM -0700, Steve Longerbeam wrote:
> The full patchset doesn't seem to be up yet, but see [1] for the cover
> letter.

Was the entire series copied to the mailing lists, or just selected
patches?  I only saw 4, 9, 11 and 13-22 via lakml.

In the absence of the other patches, will this solve imx-media binding
the internal subdevs of sensor devices to the CSI2 interface?

Thanks.

-- 
RMK's Patch system: https://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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[BUG] removing and reinserting imx-media causes kernel to explode

[Russell King - ARM Linux admin]

I just did this:

rmmod imx-media
modprobe imx-media

and was greeted by the below kernel messages.  I don't think this has
been the first issue I found with the iMX media stuff involving a module
unload/reload cycle - may I suggest that this is added to the testing
regime for this code?  Thanks.

imx-media: Removing imx-media
ipu1_vdic: Removing
ipu1_ic_prp: Removing
ipu1_ic_prpenc: Removing
ipu1_ic_prpvf: Removing
ipu2_vdic: Removing
ipu2_ic_prp: Removing
ipu2_ic_prpenc: Removing
ipu2_ic_prpvf: Removing
imx_media: module is from the staging directory, the quality is unknown, you 
have been warned.
ipu2_ic_prpvf: Registered ipu2_ic_prpvf capture as /dev/video2
imx-media: subdev ipu2_ic_prpvf bound
ipu2_ic_prpenc: Registered ipu2_ic_prpenc capture as /dev/video3
imx-media: subdev ipu2_ic_prpenc bound
imx-media: subdev ipu2_ic_prp bound
imx-media: subdev ipu2_vdic bound
ipu1_ic_prpvf: Registered ipu1_ic_prpvf capture as /dev/video4
imx-media: subdev ipu1_ic_prpvf bound
ipu1_ic_prpenc: Registered ipu1_ic_prpenc capture as /dev/video5
imx-media: subdev ipu1_ic_prpenc bound
imx-media: subdev ipu1_ic_prp bound
imx-media: subdev ipu1_vdic bound
kobject (ddca68f0): tried to init an initialized object, something is seriously 
wrong.
CPU: 1 PID: 31521 Comm: modprobe Tainted: G C5.2.0+ #325
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
[] (show_stack) from [] (dump_stack+0x9c/0xd4)
[] (dump_stack) from [] (kobject_init+0x74/0x94)
[] (kobject_init) from [] (device_initialize+0x1c/0xec)
[] (device_initialize) from [] (device_register+0xc/0x18)
[] (device_register) from [] 
(__video_register_device+0x9b4/0x1228)
[] (__video_register_device) from [] 
(imx_media_capture_device_register+0x44/0x1f4 [imx_media_capture])
[] (imx_media_capture_device_register [imx_media_capture]) from 
[] (csi_registered+0x154/0x19c [imx_media_csi])
[] (csi_registered [imx_media_csi]) from [] 
(v4l2_device_register_subdev+0xd0/0x164)
[] (v4l2_device_register_subdev) from [] 
(v4l2_async_match_notify+0x1c/0x130)
[] (v4l2_async_match_notify) from [] 
(v4l2_async_notifier_try_all_subdevs+0x48/0x94)
[] (v4l2_async_notifier_try_all_subdevs) from [] 
(__v4l2_async_notifier_register+0xa8/0x110)
[] (__v4l2_async_notifier_register) from [] 
(v4l2_async_notifier_register+0x3c/0x54)
[] (v4l2_async_notifier_register) from [] 
(imx_media_dev_notifier_register+0x2c/0x70 [imx_media])
[] (imx_media_dev_notifier_register [imx_media]) from [] 
(imx_media_probe+0x3c/0x8c [imx_media])
[] (imx_media_probe [imx_media]) from [] 
(platform_drv_probe+0x48/0x98)
[] (platform_drv_probe) from [] (really_probe+0x1d8/0x2c0)
[] (really_probe) from [] (driver_probe_device+0x5c/0x174)
[] (driver_probe_device) from [] 
(device_driver_attach+0x58/0x60)
[] (device_driver_attach) from [] 
(__driver_attach+0x84/0xc0)
[] (__driver_attach) from [] (bus_for_each_dev+0x58/0x7c)
[] (bus_for_each_dev) from [] (bus_add_driver+0xd0/0x1cc)
[] (bus_add_driver) from [] (driver_register+0x7c/0x110)
[] (driver_register) from [] (do_one_initcall+0x74/0x308)
[] (do_one_initcall) from [] (do_init_module+0x5c/0x1f4)
[] (do_init_module) from [] (load_module+0x19a4/0x2020)
[] (load_module) from [] (sys_finit_module+0x8c/0x98)
[] (sys_finit_module) from [] (ret_fast_syscall+0x0/0x28)
Exception stack(0xdb677fa8 to 0xdb677ff0)
7fa0:   00b04170  0003 007bd84c  00b05cb8
7fc0: 00b04170  1ee84500 017b 0004  00b04eb8 
7fe0: be958178 be958168 007b54bb b6c28712
ipu1_csi0: Registered ipu1_csi0 capture as /dev/video6
imx-media: subdev ipu1_csi0 bound
kobject (dcd780f0): tried to init an initialized object, something is seriously 
wrong.
CPU: 1 PID: 31521 Comm: modprobe Tainted: G C5.2.0+ #325
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
[] (show_stack) from [] (dump_stack+0x9c/0xd4)
[] (dump_stack) from [] (kobject_init+0x74/0x94)
[] (kobject_init) from [] (device_initialize+0x1c/0xec)
[] (device_initialize) from [] (device_register+0xc/0x18)
[] (device_register) from [] 
(__video_register_device+0x9b4/0x1228)
[] (__video_register_device) from [] 
(imx_media_capture_device_register+0x44/0x1f4 [imx_media_capture])
[] (imx_media_capture_device_register [imx_media_capture]) from 
[] (csi_registered+0x154/0x19c [imx_media_csi])
[] (csi_registered [imx_media_csi]) from [] 
(v4l2_device_register_subdev+0xd0/0x164)
[] (v4l2_device_register_subdev) from [] 
(v4l2_async_match_notify+0x1c/0x130)
[] (v4l2_async_match_notify) from [] 
(v4l2_async_notifier_try_all_subdevs+0x48/0x94)
[] (v4l2_async_notifier_try_all_subdevs) from [] 
(__v4l2_async_notifier_register+0xa8/0x110)
[] (__v4l2_async_notifier_register) from [] 
(v4l2_async_notifier_register+0x3c/0x54)
[] (v4l2_async_notifier_register) from [] 
(imx_media_dev_notifier_register+0x2c/0x70 [imx_media])
[] (imx_media_dev_notifier_register 

Re: [PATCH 00/12] treewide: Fix GENMASK misuses

[Russell King - ARM Linux admin]

On Wed, Jul 10, 2019 at 11:17:31AM +0200, Johannes Berg wrote:
> On Tue, 2019-07-09 at 22:04 -0700, Joe Perches wrote:
> > These GENMASK uses are inverted argument order and the
> > actual masks produced are incorrect.  Fix them.
> > 
> > Add checkpatch tests to help avoid more misuses too.
> > 
> > Joe Perches (12):
> >   checkpatch: Add GENMASK tests
> 
> IMHO this doesn't make a lot of sense as a checkpatch test - just throw
> in a BUILD_BUG_ON()?

My personal take on this is that GENMASK() is really not useful, it's
just pure obfuscation and leads to exactly these kinds of mistakes.

Yes, I fully understand the argument that you can just specify the
start and end bits, and it _in theory_ makes the code more readable.

However, the problem is when writing code.  GENMASK(a, b).  Is a the
starting bit or ending bit?  Is b the number of bits?  It's confusing
and causes mistakes resulting in incorrect code.  A BUILD_BUG_ON()
can catch some of the cases, but not all of them.

For example:

GENMASK(6, 2)

would satisify the requirement that a > b, so a BUILD_BUG_ON() will
not trigger, but was the author meaning 0x3c or 0xc0?

Personally, I've decided I am _not_ going to use GENMASK() in my code
because I struggle to get the macro arguments correct - I'm _much_
happier, and it is way more reliable for me to write the mask in hex
notation.

I think this is where use of a ternary operator would come in use.  The
normal way of writing a number of bits tends to be "a:b", so if GENMASK
took something like GENMASK(6:2), then I'd have less issue with it,
because it's argument is then in a familiar notation.

Yes, I'm sure that someone will point out that the GENMASK arguments
are just in the same order, but that doesn't prevent _me_ frequently
getting it wrong - and that's the point.  The macro seems to me to
cause more problems than it solves.

-- 
RMK's Patch system: https://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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] dma-buf: add struct dma_buf_attach_info v2

[Russell King - ARM Linux admin]

On Tue, Apr 30, 2019 at 01:10:02PM +0200, Christian König wrote:
> Add a structure for the parameters of dma_buf_attach, this makes it much 
> easier
> to add new parameters later on.

I don't understand this reasoning.  What are the "new parameters" that
are being proposed, and why do we need to put them into memory to pass
them across this interface?

If the intention is to make it easier to change the interface, passing
parameters in this manner mean that it's easy for the interface to
change and drivers not to notice the changes, since the compiler will
not warn (unless some member of the structure that the driver is using
gets removed, in which case it will error.)

Additions to the structure will go unnoticed by drivers - what if the
caller is expecting some different kind of behaviour, and the driver
ignores that new addition?

This doesn't seem to me like a good idea.

> 
> v2: rebase cleanup and fix all new implementations as well
> 
> Signed-off-by: Christian König 
> ---
>  drivers/dma-buf/dma-buf.c   | 13 +++--
>  drivers/gpu/drm/armada/armada_gem.c |  6 +-
>  drivers/gpu/drm/drm_prime.c |  6 +-
>  drivers/gpu/drm/i915/i915_gem_dmabuf.c  |  6 +-
>  drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c   |  6 +-
>  drivers/gpu/drm/tegra/gem.c |  6 +-
>  drivers/gpu/drm/udl/udl_dmabuf.c|  6 +-
>  .../common/videobuf2/videobuf2-dma-contig.c |  6 +-
>  .../media/common/videobuf2/videobuf2-dma-sg.c   |  6 +-
>  drivers/misc/fastrpc.c  |  6 +-
>  drivers/staging/media/tegra-vde/tegra-vde.c |  6 +-
>  drivers/xen/gntdev-dmabuf.c |  4 
>  include/linux/dma-buf.h | 17 +++--
>  13 files changed, 76 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 3ae6c0c2cc02..e295e76a8c57 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -535,8 +535,9 @@ EXPORT_SYMBOL_GPL(dma_buf_put);
>  /**
>   * dma_buf_attach - Add the device to dma_buf's attachments list; optionally,
>   * calls attach() of dma_buf_ops to allow device-specific attach 
> functionality
> - * @dmabuf:  [in]buffer to attach device to.
> - * @dev: [in]device to be attached.
> + * @info:[in]holds all the attach related information provided
> + *   by the importer. see  dma_buf_attach_info
> + *   for further details.
>   *
>   * Returns struct dma_buf_attachment pointer for this attachment. Attachments
>   * must be cleaned up by calling dma_buf_detach().
> @@ -550,20 +551,20 @@ EXPORT_SYMBOL_GPL(dma_buf_put);
>   * accessible to @dev, and cannot be moved to a more suitable place. This is
>   * indicated with the error code -EBUSY.
>   */
> -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
> -   struct device *dev)
> +struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info 
> *info)
>  {
> + struct dma_buf *dmabuf = info->dmabuf;
>   struct dma_buf_attachment *attach;
>   int ret;
>  
> - if (WARN_ON(!dmabuf || !dev))
> + if (WARN_ON(!dmabuf || !info->dev))
>   return ERR_PTR(-EINVAL);
>  
>   attach = kzalloc(sizeof(*attach), GFP_KERNEL);
>   if (!attach)
>   return ERR_PTR(-ENOMEM);
>  
> - attach->dev = dev;
> + attach->dev = info->dev;
>   attach->dmabuf = dmabuf;
>  
>   mutex_lock(>lock);
> diff --git a/drivers/gpu/drm/armada/armada_gem.c 
> b/drivers/gpu/drm/armada/armada_gem.c
> index 642d0e70d0f8..19c47821032f 100644
> --- a/drivers/gpu/drm/armada/armada_gem.c
> +++ b/drivers/gpu/drm/armada/armada_gem.c
> @@ -501,6 +501,10 @@ armada_gem_prime_export(struct drm_device *dev, struct 
> drm_gem_object *obj,
>  struct drm_gem_object *
>  armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf)
>  {
> + struct dma_buf_attach_info attach_info = {
> + .dev = dev->dev,
> + .dmabuf = buf
> + };
>   struct dma_buf_attachment *attach;
>   struct armada_gem_object *dobj;
>  
> @@ -516,7 +520,7 @@ armada_gem_prime_import(struct drm_device *dev, struct 
> dma_buf *buf)
>   }
>   }
>  
> - attach = dma_buf_attach(buf, dev->dev);
> + attach = dma_buf_attach(_info);
>   if (IS_ERR(attach))
>   return ERR_CAST(attach);
>  
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index dc079efb3b0f..1dd70fc095ee 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -710,6 +710,10 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct 
> drm_device *dev,
>   struct dma_buf *dma_buf,
>   struct device *attach_dev)
>  {
> + struct 

Re: [Outreachy kernel] Re: [PATCH v2] staging: mt7621-dma: Add braces around else branches

[Russell King - ARM Linux]

On Wed, Oct 24, 2018 at 02:06:18PM +0100, Julia Lawall wrote:
> 
> 
> On Wed, 24 Oct 2018, Russell King - ARM Linux wrote:
> 
> > On Wed, Oct 24, 2018 at 12:45:44PM +0200, Matthias Brugger wrote:
> > > Hi Kiberly,
> > >
> > > Thanks for adding all the emails in CC.
> > > I would encourage you for your next patch to distinguish between CC and 
> > > TO.
> > > You should send your patch TO important maintainers in the 
> > > get_maintainers.pl
> > > list (as default, to all of them). If there is someone you really want to 
> > > look
> > > into the patch, then add him/her in TO as well.
> > >
> > > Put the rest (people and mailing lists) in CC. Why? Some people filter 
> > > their
> > > mails so that they can concentrate on the mails they got send directly 
> > > and look
> > > on mails they are in CC with lower priority (maybe not at all, because 
> > > there are
> > > too much?). So it is important to have the maintainers in the TO list and 
> > > not in CC.
> >
> > +1
> >
> > I'm glad that there's someone else in the Linux community that agrees
> > with me on this point, and is willing to speak out about it.
> 
> If it's an important point, perhaps it should be mentioned in
> submitting-patches.rst?  There is a mention of the Cc tag, but no
> indication of who to put in CC.

submitting-patches.rst talks about the Cc tag in the commit, not the To
or Cc in the email client.

In any case, there's a lot of personal issues here: most kernel
developers don't care whether they're in the To or Cc header of an
email, but there are some who do use it as Matthias says - which is
actually the long-standing definition of these headers.

-- 
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: mt7621-dma: Add braces around else branches

[Russell King - ARM Linux]

On Wed, Oct 24, 2018 at 12:45:44PM +0200, Matthias Brugger wrote:
> Hi Kiberly,
> 
> Thanks for adding all the emails in CC.
> I would encourage you for your next patch to distinguish between CC and TO.
> You should send your patch TO important maintainers in the get_maintainers.pl
> list (as default, to all of them). If there is someone you really want to look
> into the patch, then add him/her in TO as well.
> 
> Put the rest (people and mailing lists) in CC. Why? Some people filter their
> mails so that they can concentrate on the mails they got send directly and 
> look
> on mails they are in CC with lower priority (maybe not at all, because there 
> are
> too much?). So it is important to have the maintainers in the TO list and not 
> in CC.

+1

I'm glad that there's someone else in the Linux community that agrees
with me on this point, and is willing to speak out about it.

-- 
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH V4 2/15] KVM/MMU: Add tlb flush with range helper function

[Russell King - ARM Linux]

On Sun, Oct 14, 2018 at 09:21:23PM +0800, Tianyu Lan wrote:
> Sorry to confuse your. I get from CCers from get_maintainer.pl script.

Unfortunately you seem to have made a mistake.  My email address is
'li...@armlinux.org.uk' not 'li...@armlinux.org'.  There is no
'li...@armlinux.org' in MAINTAINERS, yet your emails appear to be
copied to this address.

Consequently, if it was your intention to copy me with the entire
series, that didn't happen.

-- 
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH V4 2/15] KVM/MMU: Add tlb flush with range helper function

[Russell King - ARM Linux]

On Sun, Oct 14, 2018 at 10:16:56AM +0200, Thomas Gleixner wrote:
> On Sun, 14 Oct 2018, Liran Alon wrote:
> > > On 13 Oct 2018, at 17:53, lantianyu1...@gmail.com wrote:
> > > 
> > > From: Lan Tianyu 
> > > 
> > > This patch is to add wrapper functions for tlb_remote_flush_with_range
> > > callback.
> > > 
> > > Signed-off-by: Lan Tianyu 
> > > ---
> > > Change sicne V3:
> > >   Remove code of updating "tlbs_dirty"
> > > Change since V2:
> > >   Fix comment in the kvm_flush_remote_tlbs_with_range()
> > > ---
> > > arch/x86/kvm/mmu.c | 40 
> > > 1 file changed, 40 insertions(+)
> > > 
> > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> > > index c73d9f650de7..ff656d85903a 100644
> > > --- a/arch/x86/kvm/mmu.c
> > > +++ b/arch/x86/kvm/mmu.c
> > > @@ -264,6 +264,46 @@ static void mmu_spte_set(u64 *sptep, u64 spte);
> > > static union kvm_mmu_page_role
> > > kvm_mmu_calc_root_page_role(struct kvm_vcpu *vcpu);
> > > 
> > > +
> > > +static inline bool kvm_available_flush_tlb_with_range(void)
> > > +{
> > > + return kvm_x86_ops->tlb_remote_flush_with_range;
> > > +}
> > 
> > Seems that kvm_available_flush_tlb_with_range() is not used in this patch…
> 
> What's wrong with that? 
> 
> It provides the implementation and later patches make use of it. It's a
> sensible way to split patches into small, self contained entities.

From what I can see of the patches that follow _this_ patch in the
series, this function remains unused.  So, not only is it not used
in this patch, it's not used in this series.

I think the real question that needs asking is - what is the plan
for this function, and when will it be used?

-- 
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH V4 2/15] KVM/MMU: Add tlb flush with range helper function

[Russell King - ARM Linux]

On Sun, Oct 14, 2018 at 10:27:34AM +0100, Russell King - ARM Linux wrote:
> On Sun, Oct 14, 2018 at 10:16:56AM +0200, Thomas Gleixner wrote:
> > On Sun, 14 Oct 2018, Liran Alon wrote:
> > > > On 13 Oct 2018, at 17:53, lantianyu1...@gmail.com wrote:
> > > > 
> > > > From: Lan Tianyu 
> > > > 
> > > > This patch is to add wrapper functions for tlb_remote_flush_with_range
> > > > callback.
> > > > 
> > > > Signed-off-by: Lan Tianyu 
> > > > ---
> > > > Change sicne V3:
> > > >   Remove code of updating "tlbs_dirty"
> > > > Change since V2:
> > > >   Fix comment in the kvm_flush_remote_tlbs_with_range()
> > > > ---
> > > > arch/x86/kvm/mmu.c | 40 
> > > > 1 file changed, 40 insertions(+)
> > > > 
> > > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> > > > index c73d9f650de7..ff656d85903a 100644
> > > > --- a/arch/x86/kvm/mmu.c
> > > > +++ b/arch/x86/kvm/mmu.c
> > > > @@ -264,6 +264,46 @@ static void mmu_spte_set(u64 *sptep, u64 spte);
> > > > static union kvm_mmu_page_role
> > > > kvm_mmu_calc_root_page_role(struct kvm_vcpu *vcpu);
> > > > 
> > > > +
> > > > +static inline bool kvm_available_flush_tlb_with_range(void)
> > > > +{
> > > > +   return kvm_x86_ops->tlb_remote_flush_with_range;
> > > > +}
> > > 
> > > Seems that kvm_available_flush_tlb_with_range() is not used in this patch…
> > 
> > What's wrong with that? 
> > 
> > It provides the implementation and later patches make use of it. It's a
> > sensible way to split patches into small, self contained entities.
> 
> From what I can see of the patches that follow _this_ patch in the
> series, this function remains unused.  So, not only is it not used
> in this patch, it's not used in this series.

Note - I seem to have only received patches 1 through 4, so this is
based on the patches I've received.

-- 
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

and look at this:

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

User Controls

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

Camera Controls

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

Image Source Controls

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

Image Processing Controls

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

What does "a known subdev" mean?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

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

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

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

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

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

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


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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [Bug] VCHIQ functional test broken

[Russell King - ARM Linux]

On Sat, May 13, 2017 at 11:07:28AM +0200, Stefan Wahren wrote:
> In the meantime this issue has been fixed by Phil [1].

Right - definitely a driver bug.  Mapping more memory for DMA than is
actually going to be DMA'd to and expecting data to be preserved is
really horrid.

> Unfortunately i found another issue. If i enable CONFIG_HIGHMEM in
> the kernel config, the data during functional test gets corrupted.
> Phil said it's caused by the usage of get_user_pages() [2].

Without knowing who "Phil" is in that thread, but...

   HIGHMEM is a problem because you can't use get_user_pages on pages in
   HIGHMEM.

is an interesting statement, and without any reasoning or evidence.

I also believe it to be incorrect.  get_user_pages() returns an array
of struct page pointers for the user memory, calling flush_dcache_page()
and flush_anon_page() on them to ensure that any kernel mapping is
coherent with what is in userspace.

As far as returning the array of page pointers, get_user_pages() doesn't
care whether they're lowmem or highmem.

flush_dcache_page() doesn't care either - if it wants to flush the page
and the page is a highmem page, it will temporarily map it before
flushing it.

flush_anon_page() is a no-op for all non-aliasing caches.

get_user_pages() works fine for whatever memory on other platforms and
drivers such as etnaviv, so I think this comes down to the vchip driver
doing things in ways that the kernel interfaces its using don't expect -
exactly like the "lets pass full pages to the DMA API" broken-ness.

I would like to hear the justification for that statement, but without
any justification, I assert that the statement is false.

-- 
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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [Bug] VCHIQ functional test broken

[Russell King - ARM Linux]

On Mon, Apr 24, 2017 at 07:42:27PM +0200, Stefan Wahren wrote:
> > What I can't see is how changing flush_dcache_page() has possibly broken
> > this: it seems to imply that you're getting flush_dcache_page() somehow
> > called inbetween mapping it for DMA, using it for DMA and CPU accesses
> > occuring.  However, the driver doesn't call flush_dcache_page() other
> > than through get_user_pages() - and since dma_map_sg() is used in that
> > path, it should be fine.
> > 
> > So, I don't have much to offer.
> > 
> > However, flush_dcache_page() is probably still a tad heavy - it currently
> > flushes to the point of coherency, but it's really about making sure that
> > the user vs kernel mappings are coherent, not about device coherency.
> > That's the role of the DMA API.
> 
> Currently i don't care too much about performance. More important is to
> fix this regression, because i'm not able to verify the function of this
> driver efficiently.

This is a driver in staging, and the reason its in staging is because it
has problems.  This is just another example of another problem it has...
Yes, the regression is regrettable, but I don't think it's something to
get hung up on.

> I'm thinking about 2 options:
> 
> 1) add a force parameter to flush_dcache_page() which make it possible
>to override the new logic
> 2) create a second version of flush_dcache_page() with the old behavior

Neither, really, because, as I've already explained, flush_dcache_page()
has nothing what so ever to do with DMA coherence - and if a driver
breaks because we change its semantic slightly (but still in a compliant
way) then it's uncovered a latent bug in _that_ driver.

Rather than trying to "fix" flush_dcache_page() to work how this driver
wants it to (which is a totally crazy thing to propose - we can't go
shoe-horning driver specific behaviour into these generic flushing
functions), it would be much better to work out why the driver has
broken.

I see the kernel driver has its own logging (using vchiq_log_info() etc?)
maybe a starting point would be to compare the output from a working
kernel with a non-working kernel to get more of an idea where the problem
actually is?

What I'm willing to do is to temporarily drop the offending patch for
the next merge window to give more time for this driver to be debugged,
but I will want to re-apply it after that merge window closes.

-- 
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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [Bug] VCHIQ functional test broken

[Russell King - ARM Linux]

On Mon, Apr 24, 2017 at 06:12:09PM +0200, Stefan Wahren wrote:
> Am 20.04.2017 um 21:58 schrieb Rabin Vincent:
> > On Thu, Apr 20, 2017 at 11:27:38AM -0700, Eric Anholt wrote:
> >> I'm confused by what you're saying here.  The driver has already been
> >> converted to not use dmac_map_area (commit
> >> cf9caf1929882b66922aee698e99e6c8f357bee5), and uses dma_map_sg instead,
> >> matching the radeon driver you give as a model as far as I can see.
> >> That commit is in v4.11-rc6 from Stefan's regression report.
> > Right.  Sorry.  I must have had an old tag checked out when I looked at
> > the driver earlier.  The DMA API usage in the driver in v4.11-rc6 and
> > current master looks fine, except for one thing:
> >
> > The flush in flush_dcache_page() (from get_user_pages()) was done with a
> > v6_flush_kern_dcache_page() which always did a clean+invalidate while
> > the DMA API only does what is required by the direction, which is only a
> > invalidate for DMA_FROM_DEVICE.  Since the driver calls dma_from_sg() on
> > the entire page, even if userspace sent in an offset into the page,
> > unrelated data in userspace may be thrown away.
> >
> > Does changing the dma API calls to always use DMA_BIDIRECTIONAL make the
> > test pass?
> 
> Unfortunately not (at least not that simple).
> 
> Do we need special DMA mapping attributes here ? Or a dma_sync_sg_for_* ?

I had a look at the driver when you first reported the problem, but I
don't see an obvious problem.

In drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c, I
see it using get_user_pages(), generating a scatterlist, which it then
uses dma_map_sg() with.  It then goes on to populate the DMA coherent
buffer that it allocated with the DMA addresses of these buffers.

The tear-down looks sane too - free_pagelist() looks like it correctly
unmaps the scatterlist, marks the pages dirty if necessary, puts the
pages and frees the DMA coherent memory.

Since you're running on a PIPT cache, the only possible issue is whether
there's a lifetime mismatch between what happens here, and what you're
doing with the pages in userspace.  All the rules wrt the DMA API apply
to these userspace pages, just as these same rules apply in kernel space.
Once you have called dma_map_sg() on them, any CPU access (whether
explicit or speculative) will cause cache lines to be populated, and
possibly marked dirty, which can have the effect of corrupting the data
unless it is unmapped prior to the accesses you care about.

What I can't see is how changing flush_dcache_page() has possibly broken
this: it seems to imply that you're getting flush_dcache_page() somehow
called inbetween mapping it for DMA, using it for DMA and CPU accesses
occuring.  However, the driver doesn't call flush_dcache_page() other
than through get_user_pages() - and since dma_map_sg() is used in that
path, it should be fine.

So, I don't have much to offer.

However, flush_dcache_page() is probably still a tad heavy - it currently
flushes to the point of coherency, but it's really about making sure that
the user vs kernel mappings are coherent, not about device coherency.
That's the role of the DMA API.

-- 
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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [Bug] VCHIQ functional test broken

[Russell King - ARM Linux]

On Thu, Apr 13, 2017 at 07:41:48PM +0200, Stefan Wahren wrote:
> > Stefan Wahren  hat am 11. April 2017 um 20:10 
> > geschrieben:
> > 
> > 
> > Hi,
> > 
> > recently i found that vchiq_test -f doesn't work anymore with current 
> > mainline (4.11-rc6) and linux-next (20170404) on my Raspberry Pi Zero. The 
> > issue is always reproducible, but the error behavior isn't deterministic. 
> > Sometimes vchiq_test hangs and sometimes i get an error message from 
> > vchiq_test like this (but never errors from the kernel):
> > 
> > $ vchiq_test -f 10
> > Functional test - iters:10
> >  iteration 1 
> > vchiq_test: 1502: expected callback reason VCHIQ_MESSAGE_AVAILABLE, got 1
> > vchiq_test: 1524: expected callback reason VCHIQ_BULK_TRANSMIT_DONE, got 5
> > vchiq_test: 863: func_error != 0
> > 
> > I didn't had the time to bisect, but at least 4.10 is safe.
> > 
> 
> Okay, i've bisected this regression to this commit:
> 
> 00a19f3e25c0c40e0ec77f52d4841d23ad269169 is the first bad commit
> commit 00a19f3e25c0c40e0ec77f52d4841d23ad269169
> Author: Rabin Vincent 
> Date:   Tue Nov 8 09:21:19 2016 +0100
> 
> ARM: 8627/1: avoid cache flushing in flush_dcache_page()
> 
> When the data cache is PIPT or VIPT non-aliasing, and cache operations
> are broadcast by the hardware, we can always postpone the flush in
> flush_dcache_page().  A similar change was done for ARM64 in commit
> b5b6c9e9149d ("arm64: Avoid cache flushing in flush_dcache_page()").
> 
> Reviewed-by: Catalin Marinas 
> Signed-off-by: Rabin Vincent 
> Signed-off-by: Russell King 
> 
> It seems that staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm
> relies on the behavior of flush_dcache_page before this patch get
> applied. Any advices to fix this issues are appreciated.

Any ideas why this causes a problem for this driver?  From what I can see,
it doesn't make use of flush_dcache_page().

-- 
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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] [media] imx: csi: retain current field order and colorimetry setting as default

[Russell King - ARM Linux]

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] [media] imx: csi: retain current field order and colorimetry setting as default

[Russell King - ARM Linux]

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] [media] imx: csi: retain current field order and colorimetry setting as default

[Russell King - ARM Linux]

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

[Russell King - ARM Linux]

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

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

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

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

Eg, I see locally:

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

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

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

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

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

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

...
}

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

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

?

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 02/39] [media] dt-bindings: Add bindings for i.MX media driver

[Russell King - ARM Linux]

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

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

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

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

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

The rest 

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 02/39] [media] dt-bindings: Add bindings for i.MX media driver

[Russell King - ARM Linux]

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

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

[Russell King - ARM Linux]

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 02/39] [media] dt-bindings: Add bindings for i.MX media driver

[Russell King - ARM Linux]

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

Not soo good on iMX6:

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt

[Russell King - ARM Linux]

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

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

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



-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt

[Russell King - ARM Linux]

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

Looks fine for the most part.

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

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

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

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt

[Russell King - ARM Linux]

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

Maybe this is the easiest solution:

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

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

;)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt

[Russell King - ARM Linux]

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

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

Thanks for confirming.

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

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

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

New media-ctl:

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

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

Maybe something like the following would be a better idea?

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

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


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt

[Russell King - ARM Linux]

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

Now, there's two possible answers to that:

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

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

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

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

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

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

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

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

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

This causes node->valid_memorytype to be zero.

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt

[Russell King - ARM Linux]

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

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

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

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

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

The same document says:

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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


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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt

[Russell King - ARM Linux]

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

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

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

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

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

https://git.phytec.de/

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

What we have wrt video nodes is (eg):

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

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

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

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

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

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

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

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

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

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

[Russell King - ARM Linux]

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

> Can it be a point of failure?

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

- struct v4l2_subdev_format contains a struct v4l2_mbus_framefmt.

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

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

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

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

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

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

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

There's calls for:

v4l2_subdev_get_try_format
v4l2_subdev_get_try_crop
v4l2_subdev_get_try_compose

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

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

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

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

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

What did you do with:

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: outreachy/moving a driver out of staging

[Russell King - ARM Linux]

On Sun, Mar 19, 2017 at 12:37:47AM -0700, Michael Zoran wrote:
> I just noticed this e-mail.  What exactly is the requirement to get a
> driver or subsystem out of staging?

This is why each driver in staging is supposed to have a TODO file
listing each point that needs to be addressed, and when each point
is addressed, the TODO file should be updated.  The TODO file tells
people what the remaining faults are with the driver.

I see that there's a todo file here:

drivers/staging/vc04_services/interface/vchi/TODO

and the very first thing I looked at was:

  - Figure out an alternative to the dmac_map_area() hack.

$ grep -r dmac_map_area drivers/staging/vc04_services/interface/vchi
drivers/staging/vc04_services/interface/vchi/TODO:  - Figure out an alternative
to the dmac_map_area() hack.

So that one looks like it's resolved, so the TODO file is out of date.
The first step, therefore, is to get the TODO files updated with the
work that has been done, so it's possible to know what work remains.

I notice also that drivers/staging/vc04_services/interface/vchiq_arm
does not have a TODO file, so that needs reviewing and a TODO file
generated.

-- 
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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

That's actually completely incorrect.

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

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

To put it another way, these are possible:

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

Size: Discrete 816x308

Size: Discrete 408x308


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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

It's nothing more than

  gst-launch-1.0 -v v4l2src !  ! xvimagesink

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

Hi Steve,

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

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

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

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

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

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

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

   despite the media pipeline actually being configured for 60fps.

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

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

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

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

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

That sounds way too fragile to me.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

It's about _deterministically_ omitting frames from the capture.

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

This gives a _regular_ pattern of dropped frames.

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

You haven't really addressed my point at all.

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

There's some gotcha's though:

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

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

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

To even suggest that Nokia should be blamed is absurd.

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

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

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

So, I strongly disagree with you.

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

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


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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

[Russell King - ARM Linux]

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

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

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

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

ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size);

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

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

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

ipu_cpmem_set_burstsize(priv->smfc_ch, burst_size);

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

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

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

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

This doesn't help:

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

Your driver basically has no support for bayer formats.

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

Another issue.

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

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

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

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

and here's the same after reboot:

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

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

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

[Russell King - ARM Linux]

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

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


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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

If you think that, you are insulting me.  I've been thinking about this
from the "big picture" point of view.  If you think I'm only thinking
about this from only the bayer point of view, you're wrong.

Given what Mauro has said, I'm convinced that the media controller stuff
is a complete failure for usability, and adding further drivers using it
is a mistake.

I counter your accusation by saying that you are actually so focused on
the media controller way of doing things that you can't see the bigger
picture here.

So, tell me how the user can possibly use iMX6 video capture without
resorting to opening up a terminal and using media-ctl to manually
configure the pipeline.  How is the user going to control the source
device without using media-ctl to find the subdev node, and then using
v4l2-ctl on it.  How is the user supposed to know which /dev/video*
node they should be opening with their capture application?

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

On Sat, Mar 11, 2017 at 08:25:49AM -0300, Mauro Carvalho Chehab wrote:
> This situation is there since 2009. If I remember well, you tried to write
> such generic plugin in the past, but never finished it, apparently because
> it is too complex. Others tried too over the years. 
> 
> The last trial was done by Jacek, trying to cover just the exynos4 driver. 
> Yet, even such limited scope plugin was not good enough, as it was never
> merged upstream. Currently, there's no such plugins upstream.
> 
> If we can't even merge a plugin that solves it for just *one* driver,
> I have no hope that we'll be able to do it for the generic case.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

[Russell King - ARM Linux]

On Sat, Mar 11, 2017 at 11:06:55AM -0800, Steve Longerbeam wrote:
> On 03/11/2017 10:59 AM, Russell King - ARM Linux wrote:
> >On Sat, Mar 11, 2017 at 10:54:55AM -0800, Steve Longerbeam wrote:
> >>
> >>
> >>On 03/11/2017 10:45 AM, Russell King - ARM Linux wrote:
> >>>I really don't think expecting the user to understand and configure
> >>>the pipeline is a sane way forward.  Think about it - should the
> >>>user need to know that, because they have a bayer-only CSI data
> >>>source, that there is only one path possible, and if they try to
> >>>configure a different path, then things will just error out?
> >>>
> >>>For the case of imx219 connected to iMX6, it really is as simple as
> >>>"there is only one possible path" and all the complexity of the media
> >>>interfaces/subdevs is completely unnecessary.  Every other block in
> >>>the graph is just noise.
> >>>
> >>>The fact is that these dot graphs show a complex picture, but reality
> >>>is somewhat different - there's only relatively few paths available
> >>>depending on the connected source and the rest of the paths are
> >>>completely useless.
> >>>
> >>
> >>I totally disagree there. Raw bayer requires passthrough yes, but for
> >>all other media bus formats on a mipi csi-2 bus, and all other media
> >>bus formats on 8-bit parallel buses, the conersion pipelines can be
> >>used for scaling, CSC, rotation, and motion-compensated de-interlacing.
> >
> >... which only makes sense _if_ your source can produce those formats.
> >We don't actually disagree on that.
> >
> >Let me re-state.  If the source can _only_ produce bayer, then there is
> >_only_ _one_ possible path, and all the overhead of the media controller
> >stuff is totally unnecessary.
> >
> >Or, are you going to tell me that the user should have the right to
> >configure paths through the iMX6 hardware that are not permitted by the
> >iMX6 manuals for the data format being produced by the sensor?
> >
> 
> Russell, I'm not following you. The imx6 pipelines allow for many
> different sources, not just the inx219 that only outputs bayer. You
> seem to be saying that those other pipelines should not be present
> because they don't support raw bayer.

What I'm saying is this:

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

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

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

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

... which only makes sense _if_ your source can produce those formats.
We don't actually disagree on that.

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

On Sat, Mar 11, 2017 at 10:14:49AM -0800, Steve Longerbeam wrote:
> On 03/11/2017 03:39 AM, Hans Verkuil wrote:
> >It's fine to use an internal event as long as the end-user doesn't
> >see it. But if you lose vsyncs, then you never capture another frame,
> >right?
> 
> No, that's not correct. By loss of vertical sync, I mean the IPU
> captures portions of two different frames, resulting in a permanent
> "split image", with one frame containing portions of two consecutive
> images. Or, the video rolls continuously, if you remember the old CRT
> television sets of yore, it's the same rolling effect.

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

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

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

[Russell King - ARM Linux]

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

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

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

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

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

The iMX6 manuals are available on the 'net.

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

There are several chapters that cover the capture side:

* MIPI CSI2
* IPU CSI2 gasket
* IPU

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
digraph board {
	rankdir=TB
	n0001 [label="{{ 0 |  1} | ipu1_csi0_mux\n/dev/v4l-subdev0 | { 2}}", shape=Mrecord, style=filled, fillcolor=green]
	n0001:port2 -> n0044:port0
	n0005 [label="{{ 0 |  1} | ipu2_csi1_mux\n/dev/v4l-subdev1 | { 2}}", shape=Mrecord, style=filled, fillcolor=green]
	n0005:port2 -> n0068:port0 [style=dashed]
	n0009 [label="{{ 0 |  1} | ipu1_vdic\n/dev/v4l-subdev2 | { 2}}", shape=Mrecord, style=filled, fillcolor=green]
	n0009:port2 -> n0011:port0 [style=dashed]
	n000d [label="{{ 0 |  1} | ipu2_vdic\n/dev/v4l-subdev3 | { 2}}", shape=Mrecord, style=filled, fillcolor=green]
	n000d:port2 -> n0027:port0 [style=dashed]
	n0011 [label="{{ 0} | ipu1_ic_prp\n/dev/v4l-subdev4 | { 1 |  2}}", shape=Mrecord, style=filled, fillcolor=green]
	n0011:port1 -> n0015:port0 [style=dashed]
	n0011:port2 -> n001e:port0 [style=dashed]
	n0015 [label="{{ 0} | ipu1_ic_prpenc\n/dev/v4l-subdev5 | { 1}}", shape=Mrecord, style=filled, fillcolor=green]
	n0015:port1 -> n0018 [style=dashed]
	n0018 [label="ipu1_ic_prpenc capture\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
	n001e [label="{{ 0} | ipu1_ic_prpvf\n/dev/v4l-subdev6 | { 1}}", shape=Mrecord, style=filled, fillcolor=green]
	n001e:port1 -> n0021 [style=dashed]
	n0021 [label="ipu1_ic_prpvf capture\n/dev/video1", shape=box, style=filled, fillcolor=yellow]
	n0027 [label="{{ 0} | ipu2_ic_prp\n/dev/v4l-subdev7 | { 1 |  2}}", shape=Mrecord, style=filled, fillcolor=green]
	n0027:port1 -> n002b:port0 [style=dashed]
	n0027:port2 -> n0034:port0 [style=dashed]
	n002b [label="{{ 0} | ipu2_ic_prpenc\n/dev/v4l-subdev8 | { 1}}", shape=Mrecord, style=filled, fillcolor=green]
	n002b:port1 -> n002e [style=dashed]
	n002e [label="ipu2_ic_prpenc capture\n/dev/video2", shape=box, style=filled, fillcolor=yellow]
	n0034 [label="{{ 0} | ipu2_ic_prpvf\n/dev/v4l-subdev9 | { 1}}", shape=Mrecord, style=filled, fillcolor=green]
	n0034:port1 -> n0037 [style=dashed]
	n0037 [label="ipu2_ic_prpvf capture\n/dev/video3", shape=box, style=filled, fillcolor=yellow]
	n003d [label="{{ 1} | imx219 0-0010\n/dev/v4l-subdev11 | { 0}}", shape=Mrecord, style=filled, fillcolor=green]
	n003d:port0 -> n0058:port0
	n0040 [label="{{} | imx219 pixel 0-0010\n/dev/v4l-subdev10 | { 0}}", shape=Mrecord, style=filled, fillcolor=green]
	n0040:port0 -> n003d:port1 [style=bold]
	n0044 [label="{{ 0} | ipu1_csi0\n/dev/v4l-subdev12 | { 1 |  2}}", shape=Mrecord, style=filled, fillcolor=green]
	n0044:port2 -> n0048
	n0044:port1 -> n0011:port0 [style=dashed]
	n0044:port1 -> n0009:port0 [style=dashed]
	n0048 [label="ipu1_csi0 capture\n/dev/video4", shape=box, style=filled, fillcolor=yellow]
	n004e [label="{{ 0} | ipu1_csi1\n/dev/v4l-subdev13 | { 1 |  2}}", shape=Mrecord, style=filled, fillcolor=green]
	n004e:port2 -> n0052 [style=dashed]
	n004e:port1 -> n0011:port0 [style=dashed]
	n004e:port1 -> n0009:port0 [style=dashed]
	n0052 [label="ipu1_csi1 capture\n/dev/video5", shape=box, style=filled, fillcolor=yellow]
	n0058 [label="{{ 0} | imx6-mipi-csi2\n/dev/v4l-subdev14 | { 1 |  2 |  3 |  4}}", shape=Mrecord, style=filled, fillcolor=green]
	n0058:port1 -> n0001:port0
	n0058:port2 -> n004e:port0 [style=dashed]
	n0058:port3 -> n005e:port0 [style=dashed]
	n0058:port4 -> n0005:port0 [style=dashed]
	n005e [label="{{ 0} | 

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

[Russell King - ARM Linux]

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

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

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

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

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

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

[Russell King - ARM Linux]

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

Here's the information to back up my claims:

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

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

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

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

[Russell King - ARM Linux]

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

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

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

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

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

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

This causes v4lconvert_supported_dst_fmt_only() to return true.

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

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

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

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

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

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

On Fri, Mar 10, 2017 at 02:22:29PM +0100, Hans Verkuil wrote:
> And nobody of the media core developers has the time to work on the docs,
> utilities and libraries you need to make this all work cleanly and reliably.

Well, talking about docs, and in connection to control inheritence,
this is already documented in at least three separate places:

Documentation/media/uapi/v4l/dev-subdev.rst:

  Controls
  
  ...
  Depending on the driver, those controls might also be exposed through
  one (or several) V4L2 device nodes.

Documentation/media/kapi/v4l2-subdev.rst:

  ``VIDIOC_QUERYCTRL``,
  ``VIDIOC_QUERYMENU``,
  ``VIDIOC_G_CTRL``,
  ``VIDIOC_S_CTRL``,
  ``VIDIOC_G_EXT_CTRLS``,
  ``VIDIOC_S_EXT_CTRLS`` and
  ``VIDIOC_TRY_EXT_CTRLS``:
  
  The controls ioctls are identical to the ones defined in V4L2. They
  behave identically, with the only exception that they deal only with
  controls implemented in the sub-device. Depending on the driver, those
  controls can be also be accessed through one (or several) V4L2 device
  nodes.

Then there's Documentation/media/kapi/v4l2-controls.rst, which gives a
step by step approach to the main video device inheriting controls from
its subdevices, and it says:

  Inheriting Controls
  ---
  
  When a sub-device is registered with a V4L2 driver by calling
  v4l2_device_register_subdev() and the ctrl_handler fields of both v4l2_subdev
  and v4l2_device are set, then the controls of the subdev will become
  automatically available in the V4L2 driver as well. If the subdev driver
  contains controls that already exist in the V4L2 driver, then those will be
  skipped (so a V4L2 driver can always override a subdev control).
  
  What happens here is that v4l2_device_register_subdev() calls
  v4l2_ctrl_add_handler() adding the controls of the subdev to the controls
  of v4l2_device.

So, either the docs are wrong, or the advice being mentioned in emails
about subdev control inheritence is misleading.  Whatever, the two are
currently inconsistent.

As I've already mentioned, from talking about this with Mauro, it seems
Mauro is in agreement with permitting the control inheritence... I wish
Mauro would comment for himself, as I can't quote our private discussion
on the subject.

Right now, my view is that v4l2 is currently being screwed up by people
with different opinions - there is no unified concensus on how any of
this stuff is supposed to work, everyone is pulling in different
directions.  That needs solving _really_ quickly, so I suggest that
v4l2 people urgently talk to each other and thrash out some of the
issues that Steve's patch set has brought up, and settle on a way
forward, rather than what is seemingly happening today - which is
everyone working in isolation of everyone else with their own bias on
how things should be done.

-- 
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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

On Fri, Mar 10, 2017 at 01:54:28PM +0100, Hans Verkuil wrote:
> But there was always meant to be a layer (libv4l plugin) that could be
> used to setup a 'default scenario' that existing applications could use,
> but that was never enforced, sadly.

However, there's other painful issues lurking in userspace, particularly
to do with the v4l libraries.

The idea that the v4l libraries should intercept the format negotiation
between the application and kernel is a particularly painful one - the
default gstreamer build detects the v4l libraries, and links against it.
That much is fine.

However, the problem comes when you're trying to use bayer formats. The
v4l libraries "helpfully" (or rather unhelpfully) intercept the format
negotiation, and decide that they'll invoke v4lconvert to convert the
bayer to RGB for you, whether you want them to do that or not.

v4lconvert may not be the most efficient way to convert, or even what
is desired (eg, you may want to receive the raw bayer image.)  However,
since the v4l libraries/v4lconvert gives you no option but to have its
conversion forced into the pipeline, other options (such as using the
gstreamer neon accelerated de-bayer plugin) isn't an option without
rebuilding gstreamer _without_ linking against the v4l libraries.

At that point, saying "this should be done in a libv4l plugin" becomes
a total nonsense, because if you need to avoid libv4l due to its
stupidities, you don't get the benefit of subdevs, and it yet again
_forces_ people down the route of custom applications.

So, I really don't agree with pushing this into a userspace library
plugin - at least not with the current state there.

_At least_ the debayering in the v4l libraries needs to become optional.

-- 
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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 13/36] [media] v4l2: add a frame timeout event

[Russell King - ARM Linux]

On Thu, Mar 09, 2017 at 06:38:18PM -0800, Steve Longerbeam wrote:
> On 03/05/2017 02:41 PM, Russell King - ARM Linux wrote:
> >I'm not sure that statement is entirely accurate.  With the IMX219
> >camera, I _could_ (with previous iterations of the iMX capture code)
> >stop it streaming, wait a while, and restart it, and everything
> >continues to work.
> 
> Hi Russell, did you see the "EOF timeout" kernel error message when you
> stopped the IMX219 from streaming? Only a "EOF timeout" message
> indicates the unrecoverable case.

I really couldn't tell you anymore - I can't go back and test at all,
because:

(a) your v4 patch set never worked for me
(b) I've now moved forward to v4.11-rc1, which conflicts with your v4
and older patch sets.

In any case, I'm in complete disagreement with many of the points that
Sakari has been bringing up, and I'm finding the direction that things
are progressing to be abhorrent.

I've discussed with Mauro about (eg) the application interface, and
unsurprisingly to me, Mauro immediately said about control inheritence
to the main v4l2 device, contradicting what Sakari has been saying.
The subdev API is supposed to be there to allow for finer control, it's
not a "one or the other" thing.  The controls are still supposed to be
exposed through the main v4l2 subdev device.

Since the v4l2 stuff is becoming abhorrent, and I've also come to
realise that I'm going to have to write an entirely new userspace
application to capture, debayer and encode efficiently, I'm finding
that I've little motivation to take much of a further interest in
iMX6 capture, or indeed continue my reverse engineering efforts of
the IMX219 sensor.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 13/36] [media] v4l2: add a frame timeout event

[Russell King - ARM Linux]

On Sat, Mar 04, 2017 at 04:37:43PM -0800, Steve Longerbeam wrote:
> 
> 
> On 03/04/2017 02:56 AM, Sakari Ailus wrote:
> >That's a bit of a special situation --- still there are alike conditions on
> >existing hardware. You should return the buffers to the user with the ERROR
> >flag set --- or return -EIO from VIDIOC_DQBUF, if the condition will
> >persist:
> 
> On i.MX an EOF timeout is not recoverable without a stream restart, so
> I decided to call vb2_queue_error() when the timeout occurs (instead
> of sending an event). The user will then get -EIO when it attempts to
> queue or dequeue further buffers.

I'm not sure that statement is entirely accurate.  With the IMX219
camera, I _could_ (with previous iterations of the iMX capture code)
stop it streaming, wait a while, and restart it, and everything
continues to work.

Are you sure that the problem you have here is caused by the iMX6
rather than the ADV718x CVBS decoder (your initial description said
it was the decoder.)

If it _is_ the decoder that's going wrong, that doesn't justify
cripping the rest of the driver for one instance of broken hardware
that _might_ be attached to it.

-- 
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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

On Thu, Mar 02, 2017 at 06:02:57PM +0200, Sakari Ailus wrote:
> Hi Steve,
> 
> On Wed, Feb 15, 2017 at 06:19:16PM -0800, Steve Longerbeam wrote:
> > v4l2_pipeline_inherit_controls() will add the v4l2 controls from
> > all subdev entities in a pipeline to a given video device.
> > 
> > Signed-off-by: Steve Longerbeam 
> > ---
> >  drivers/media/v4l2-core/v4l2-mc.c | 48 
> > +++
> >  include/media/v4l2-mc.h   | 25 
> >  2 files changed, 73 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-mc.c 
> > b/drivers/media/v4l2-core/v4l2-mc.c
> > index 303980b..09d4d97 100644
> > --- a/drivers/media/v4l2-core/v4l2-mc.c
> > +++ b/drivers/media/v4l2-core/v4l2-mc.c
> > @@ -22,6 +22,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -238,6 +239,53 @@ int v4l_vb2q_enable_media_source(struct vb2_queue *q)
> >  }
> >  EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_source);
> >  
> > +int __v4l2_pipeline_inherit_controls(struct video_device *vfd,
> > +struct media_entity *start_entity)
> 
> I have a few concerns / questions:
> 
> - What's the purpose of this patch? Why not to access the sub-device node
>   directly?

What tools are in existance _today_ to provide access to these controls
via the sub-device nodes?

v4l-tools doesn't last time I looked - in fact, the only tool in v4l-tools
which is capable of accessing the subdevices is media-ctl, and that only
provides functionality for configuring the pipeline.

So, pointing people at vapourware userspace is really quite rediculous.

The established way to control video capture is through the main video
capture device, not through the sub-devices.  Yes, the controls are
exposed through sub-devices too, but that does not mean that is the
correct way to access them.

The v4l2 documentation (Documentation/media/kapi/v4l2-controls.rst)
even disagrees with your statements.  That talks about control
inheritence from sub-devices to the main video device, and the core
v4l2 code provides _automatic_ support for this - see
v4l2_device_register_subdev():

/* This just returns 0 if either of the two args is NULL */
err = v4l2_ctrl_add_handler(v4l2_dev->ctrl_handler, sd->ctrl_handler, 
NULL);

which merges the subdev's controls into the main device's control
handler.

So, (a) I don't think Steve needs to add this code, and (b) I think
your statements about not inheriting controls goes against the
documentation and API compatibility with _existing_ applications,
and ultimately hurts the user experience, since there's nothing
existing today to support what you're suggesting 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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

On Tue, Feb 21, 2017 at 05:38:34PM +0200, Sakari Ailus wrote:
> Hi Russell,
> 
> On Tue, Feb 21, 2017 at 01:21:32PM +, Russell King - ARM Linux wrote:
> > On Tue, Feb 21, 2017 at 02:37:57PM +0200, Sakari Ailus wrote:
> > > Hi Russell,
> > > 
> > > On Tue, Feb 21, 2017 at 12:13:32AM +, Russell King - ARM Linux wrote:
> > > > On Tue, Feb 21, 2017 at 12:04:10AM +0200, Sakari Ailus wrote:
> > > > > On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:
> > > > > > From: Russell King <rmk+ker...@armlinux.org.uk>
> > > > > > 
> > > > > > Setting and getting frame rates is part of the negotiation mechanism
> > > > > > between subdevs.  The lack of support means that a frame rate at the
> > > > > > sensor can't be negotiated through the subdev path.
> > > > > 
> > > > > Just wondering --- what do you need this for?
> > > > 
> > > > The v4l2 documentation contradicts the media-ctl implementation.
> > > > 
> > > > While v4l2 documentation says:
> > > > 
> > > >   These ioctls are used to get and set the frame interval at specific
> > > >   subdev pads in the image pipeline. The frame interval only makes sense
> > > >   for sub-devices that can control the frame period on their own. This
> > > >   includes, for instance, image sensors and TV tuners. Sub-devices that
> > > >   don't support frame intervals must not implement these ioctls.
> > > > 
> > > > However, when trying to configure the pipeline using media-ctl, eg:
> > > > 
> > > > media-ctl -d /dev/media1 --set-v4l2 '"imx219 pixel 
> > > > 0-0010":0[crop:(0,0)/3264x2464]'
> > > > media-ctl -d /dev/media1 --set-v4l2 '"imx219 
> > > > 0-0010":1[fmt:SRGGB10/3264x2464@1/30]'
> > > > media-ctl -d /dev/media1 --set-v4l2 '"imx219 
> > > > 0-0010":0[fmt:SRGGB8/816x616@1/30]'
> > > > media-ctl -d /dev/media1 --set-v4l2 
> > > > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> > > > Unable to setup formats: Inappropriate ioctl for device (25)
> > > > media-ctl -d /dev/media1 --set-v4l2 
> > > > '"ipu1_csi0_mux":2[fmt:SRGGB8/816x616@1/30]'
> > > > media-ctl -d /dev/media1 --set-v4l2 
> > > > '"ipu1_csi0":2[fmt:SRGGB8/816x616@1/30]'
> > > > 
> > > > The problem there is that the format setting for the csi2 does not get
> > > > propagated forward:
> > > 
> > > The CSI-2 receivers typically do not implement frame interval IOCTLs as 
> > > they
> > > do not control the frame interval. Some sensors or TV tuners typically do,
> > > so they implement these IOCTLs.
> > 
> > No, TV tuners do not.  The frame rate for a TV tuner is set by the
> > broadcaster, not by the tuner.  The tuner can't change that frame rate.
> > The tuner may opt to "skip" fields or frames.  That's no different from
> > what the CSI block in my example below is capable of doing.
> > 
> > Treating a tuner differently from the CSI block is inconsistent and
> > completely wrong.
> 
> I agree tuners in that sense are somewhat similar, and they are not treated
> differently because they are tuners (and not CSI-2 receivers). Neither can
> control the frame rate of the incoming video stream.
> 
> Conceivably a tuner could implement G_FRAME_INTERVAL IOCTL, but based on a
> quick glance none appears to. Neither do CSI-2 receivers. Only sensor
> drivers do currently.

Please look again.  I am being very careful with "CSI" vs "CSI-2" in my
emails, you are conflating the two.

In all my emails so far, "CSI" refers to a block of hardware that is
responsible for receiving an image stream from some kind of source.  It
contains hardware that supports frame skipping.

"CSI-2" refers to a different block of hardware that is responsible for
receiving a serially encoded stream from a MIPI-CSI-2 compliant source
and providing it to the "CSI" block.

I would have thought my diagram that I drew would have made it clear that
they were different blocks of hardware, but I guess in this case, the old
saying "a picture is worth 1000 words" is simply not true.

> Images are transmitted as series of lines, with each line ending in a
> horizontal blanking period, and each frame ending with a similar period of

I'm sorry, are you seriously teaching me to suck rocks?  I am insulted.

I've been involved in TV and video for many years, I don't need you to
tell me how video is transmitted.

Sorry, you've just lost my interest in further discussion.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

On Tue, Feb 21, 2017 at 12:04:10AM +0200, Sakari Ailus wrote:
> On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:
> > From: Russell King 
> > 
> > Setting and getting frame rates is part of the negotiation mechanism
> > between subdevs.  The lack of support means that a frame rate at the
> > sensor can't be negotiated through the subdev path.
> 
> Just wondering --- what do you need this for?

The v4l2 documentation contradicts the media-ctl implementation.

While v4l2 documentation says:

  These ioctls are used to get and set the frame interval at specific
  subdev pads in the image pipeline. The frame interval only makes sense
  for sub-devices that can control the frame period on their own. This
  includes, for instance, image sensors and TV tuners. Sub-devices that
  don't support frame intervals must not implement these ioctls.

However, when trying to configure the pipeline using media-ctl, eg:

media-ctl -d /dev/media1 --set-v4l2 '"imx219 pixel 
0-0010":0[crop:(0,0)/3264x2464]'
media-ctl -d /dev/media1 --set-v4l2 '"imx219 
0-0010":1[fmt:SRGGB10/3264x2464@1/30]'
media-ctl -d /dev/media1 --set-v4l2 '"imx219 0-0010":0[fmt:SRGGB8/816x616@1/30]'
media-ctl -d /dev/media1 --set-v4l2 
'"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
Unable to setup formats: Inappropriate ioctl for device (25)
media-ctl -d /dev/media1 --set-v4l2 '"ipu1_csi0_mux":2[fmt:SRGGB8/816x616@1/30]'
media-ctl -d /dev/media1 --set-v4l2 '"ipu1_csi0":2[fmt:SRGGB8/816x616@1/30]'

The problem there is that the format setting for the csi2 does not get
propagated forward:

$ strace media-ctl -d /dev/media1 --set-v4l2 
'"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
...
open("/dev/v4l-subdev16", O_RDWR)   = 3
ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbec16244) = 0
ioctl(3, VIDIOC_SUBDEV_S_FRAME_INTERVAL, 0xbec162a4) = -1 ENOTTY (Inappropriate
ioctl for device)
fstat64(1, {st_mode=S_IFCHR|0600, st_rdev=makedev(136, 2), ...}) = 0
write(1, "Unable to setup formats: Inappro"..., 61) = 61
Unable to setup formats: Inappropriate ioctl for device (25)
close(3)= 0
exit_group(1)   = ?
+++ exited with 1 +++

because media-ctl exits as soon as it encouters the error while trying
to set the frame rate.

This makes implementing setup of the media pipeline in shell scripts
unnecessarily difficult - as you need to then know whether an entity
is likely not to support the VIDIOC_SUBDEV_S_FRAME_INTERVAL call,
and either avoid specifying a frame rate:

$ strace media-ctl -d /dev/media1 --set-v4l2 
'"imx6-mipi-csi2":1[fmt:SRGGB8/816x616]'
...
open("/dev/v4l-subdev16", O_RDWR)   = 3
ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbeb1a254) = 0
open("/dev/v4l-subdev0", O_RDWR)= 4
ioctl(4, VIDIOC_SUBDEV_S_FMT, 0xbeb1a254) = 0
close(4)= 0
close(3)= 0
exit_group(0)   = ?
+++ exited with 0 +++

or manually setting the format on the sink.

Allowing the S_FRAME_INTERVAL call seems to me to be more in keeping
with the negotiation mechanism that is implemented in subdevs, and
IMHO should be implemented inside the kernel as a pad operation along
with the format negotiation, especially so as frame skipping is
defined as scaling, in just the same way as the frame size is also
scaling:

   -  ``MEDIA_ENT_F_PROC_VIDEO_SCALER``

   -  Video scaler. An entity capable of video scaling must have
  at least one sink pad and one source pad, and scale the
  video frame(s) received on its sink pad(s) to a different
  resolution output on its source pad(s). The range of
  supported scaling ratios is entity-specific and can differ
  between the horizontal and vertical directions (in particular
  scaling can be supported in one direction only). Binning and
  skipping are considered as scaling.

Although, this is vague, as it doesn't define what it means by "skipping",
whether that's skipping pixels (iow, sub-sampling) or whether that's
frame skipping.

Then there's the issue where, if you have this setup:

 camera --> csi2 receiver --> csi --> capture

and the "csi" subdev can skip frames, you need to know (a) at the CSI
sink pad what the frame rate is of the source (b) what the desired
source pad frame rate is, so you can configure the frame skipping.
So, does the csi subdev have to walk back through the media graph
looking for the frame rate?  Does the capture device have to walk back
through the media graph looking for some subdev to tell it what the
frame rate is - the capture device certainly can't go straight to the
sensor to get an answer to that question, because that bypasses the
effect of the CSI frame skipping (which will lower the frame rate.)

IMHO, frame rate is just another format property, just like the
resolution and data format itself, and v4l2 should be treating it no
differently.

In any case, the documentation vs media-ctl create 

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

[Russell King - ARM Linux]

On Sat, Feb 18, 2017 at 09:29:17AM -0800, Steve Longerbeam wrote:
> On 02/18/2017 01:23 AM, Russell King - ARM Linux wrote:
> >On Fri, Feb 17, 2017 at 05:12:44PM -0800, Steve Longerbeam wrote:
> >>Hi Russell,
> >>
> >>I signed-off on this but after more review I'm not sure this is right.
> >>
> >>The CSI-2 receiver really has no control over frame rate. It's output
> >>frame rate is the same as the rate that is delivered to it.
> >>
> >>So this subdev should either not implement these ops, or it should
> >>refer them to the attached source subdev.
> >
> >Where in the V4L2 documentation does it say that is permissible?
> >
> 
> https://www.linuxtv.org/downloads/v4l-dvb-apis-old/vidioc-subdev-g-frame-interval.html
> 
> "The frame interval only makes sense for sub-devices that can control the
> frame period on their own. This includes, for instance, image sensors and TV
> tuners. Sub-devices that don't support frame intervals must not implement
> these ioctls."

That sounds clear - but the TV tuner example seems odd - the frame rate
is determined at transmission time, not reception time.  Yes, it's
possible to skip frames (which would be scaling) but you can't
_control_ the frame rate per se.

> >If you don't implement these, media-ctl fails to propagate _anything_
> >to the next sink pad if you specify a frame rate, because media-ctl
> >throws an error and exits immediately.
> >
> 
> But I agree with you here. I think our only option is to ignore that
> quoted requirement above and propagate [gs]_frame_interval all the way
> to the CSI (which can control the frame rate via frame skipping).

Sounds like something to tackle the media maintainers over - the
documentation vs media-ctl seem to have different ideas on this
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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Russell King - ARM Linux]

On Fri, Feb 17, 2017 at 05:12:44PM -0800, Steve Longerbeam wrote:
> Hi Russell,
> 
> I signed-off on this but after more review I'm not sure this is right.
> 
> The CSI-2 receiver really has no control over frame rate. It's output
> frame rate is the same as the rate that is delivered to it.
> 
> So this subdev should either not implement these ops, or it should
> refer them to the attached source subdev.

Where in the V4L2 documentation does it say that is permissible?

If you don't implement these, media-ctl fails to propagate _anything_
to the next sink pad if you specify a frame rate, because media-ctl
throws an error and exits immediately.

-- 
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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 00/36] i.MX Media Driver

[Russell King - ARM Linux]

On Fri, Feb 17, 2017 at 02:22:14PM +0200, Sakari Ailus wrote:
> Hi Philipp, Steve and Russell,
> 
> On Fri, Feb 17, 2017 at 12:43:38PM +0100, Philipp Zabel wrote:
> > I think with Russell's explanation of how the imx219 sensor operates,
> > we'll have to do something before calling the sensor s_stream, but right
> > now I'm still unsure what exactly.
> 
> Indeed there appears to be no other way to achieve the LP-11 state than
> going through the streaming state for this particular sensor, apart from
> starting streaming.
> 
> Is there a particular reason why you're waiting for the transmitter to
> transfer to LP-11 state? That appears to be the last step which is done in
> the csi2_s_stream() callback.
> 
> What the sensor does next is to start streaming, and the first thing it does
> in that process is to switch to LP-11 state.
> 
> Have you tried what happens if you simply drop the LP-11 check? To me that
> would seem the right thing to do.

The Freescale documentation for iMX6's CSI2 receiver (chapter 40.3.1)
specifies a very specific sequence to be followed to safely bring up the
CSI2 receiver.  Bold text gets used, which implies emphasis on certain
points, which suggests that it's important to follow it.

Presumably, the reason for this is to ensure that a state machine within
the CSI2 receiver is properly synchronised to the incoming data stream,
and while avoiding the sequence may work, it may not be guaranteed to
work every time.

I guess we need someone from NXP to comment.

-- 
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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 23/36] media: imx: Add MIPI CSI-2 Receiver subdev driver

[Russell King - ARM Linux]

On Fri, Feb 17, 2017 at 11:47:59AM +0100, Philipp Zabel wrote:
> On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote:
> > +static void csi2_dphy_init(struct csi2_dev *csi2)
> > +{
> > +   /*
> > +* FIXME: 0x14 is derived from a fixed D-PHY reference
> > +* clock from the HSI_TX PLL, and a fixed target lane max
> > +* bandwidth of 300 Mbps. This value should be derived
> 
> If the table in https://community.nxp.com/docs/DOC-94312 is correct,
> this should be 850 Mbps. Where does this 300 Mbps value come from?

I thought you had some code to compute the correct value, although
I guess we've lost the ability to know how fast the sensor is going
to drive the link.

Note that the IMX219 currently drives the data lanes at 912Mbps almost
exclusively, as I've yet to finish working out how to derive the PLL
parameters.  (I have something that works, but it currently takes on
the order of 100k iterations to derive the parameters.  gcd() doesn't
help you in this instance.)

-- 
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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 00/36] i.MX Media Driver

[Russell King - ARM Linux]

On Fri, Feb 17, 2017 at 11:39:11AM +0100, Philipp Zabel wrote:
> On Thu, 2017-02-16 at 22:57 +0000, Russell King - ARM Linux wrote:
> > On Thu, Feb 16, 2017 at 02:27:41PM -0800, Steve Longerbeam wrote:
> > > 
> > > 
> > > On 02/16/2017 02:20 PM, Russell King - ARM Linux wrote:
> > > >On Wed, Feb 15, 2017 at 06:19:02PM -0800, Steve Longerbeam wrote:
> > > >>In version 4:
> > > >
> > > >With this version, I get:
> > > >
> > > >[28762.892053] imx6-mipi-csi2: LP-11 timeout, phy_state = 0x
> > > >[28762.899409] ipu1_csi0: pipeline_set_stream failed with -110
> > > >
> > > 
> > > Right, in the imx219, on exit from s_power(), the clock and data lanes
> > > must be placed in the LP-11 state. This has been done in the ov5640 and
> > > tc358743 subdevs.
> > 
> > The only way to do that is to enable streaming from the sensor, wait
> > an initialisation time, and then disable streaming, and wait for the
> > current line to finish.  There is _no_ other way to get the sensor to
> > place its clock and data lines into LP-11 state.
> 
> I thought going through LP-11 is part of the D-PHY transmitter
> initialization, during the LP->HS wakeup sequence. But then I have no
> access to MIPI specs.

The D-PHY transmitter initialisation *only* happens as part of the
wake-up from standby to streaming mode.  That is because Sony expect
that you program the sensor, and then when you switch it to streaming
mode, it computes the D-PHY parameters from the PLL, input clock rate
(you have to tell it the clock rate in 1/256 MHz units), number of
lanes, and other parameters.

It is possible to program the D-PHY parameters manually, but that
doesn't change the above sequence in any way (it just avoids the
chip computing the values, it doesn't result in any change of
behaviour on the bus.)

The IMX219 specifications are clear: the clock and data lines are
held low (LP-00 state) after releasing the hardware enable signal.
There's a period of chip initialisation, and then you can access the
I2C bus and configure it.  There's a further period of initialisation
where charge pumps are getting to their operating state.  Then, you
set the streaming bit, and a load more initialisation happens before
the CSI bus enters LP-11 state and the first frame pops out.  When
entering standby, the last frame is completed, and then the CSI bus
enters LP-11 state.

SMIA are slightly different - mostly following what I've said above,
but the clock and data lines are tristated after releasing the
xshutdown signal, and they remain tristated until the clock line
starts toggling before the first frame appears.  There appears to
be no point that the clock line enters LP-11 state before it starts
toggling.  When entering standby, the last frame is completed, and
the CSI bus enters tristate mode (so floating.)  There is no way to
get these sensors into LP-11 state.

-- 
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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 00/36] i.MX Media Driver

[Russell King - ARM Linux]

On Thu, Feb 16, 2017 at 02:27:41PM -0800, Steve Longerbeam wrote:
> 
> 
> On 02/16/2017 02:20 PM, Russell King - ARM Linux wrote:
> >On Wed, Feb 15, 2017 at 06:19:02PM -0800, Steve Longerbeam wrote:
> >>In version 4:
> >
> >With this version, I get:
> >
> >[28762.892053] imx6-mipi-csi2: LP-11 timeout, phy_state = 0x
> >[28762.899409] ipu1_csi0: pipeline_set_stream failed with -110
> >
> 
> Right, in the imx219, on exit from s_power(), the clock and data lanes
> must be placed in the LP-11 state. This has been done in the ov5640 and
> tc358743 subdevs.

The only way to do that is to enable streaming from the sensor, wait
an initialisation time, and then disable streaming, and wait for the
current line to finish.  There is _no_ other way to get the sensor to
place its clock and data lines into LP-11 state.

For that to happen, we need to program the sensor a bit more than we
currently do at power on (to a minimal resolution, and setting up the
PLLs), and introduce another 4ms on top of the 8ms or so that the
runtime resume function already takes.

Looking at the SMIA driver, things are worse, and I suspect that it also
will not work with the current setup - the SMIA spec shows that the CSI
clock and data lines are tristated while the sensor is not streaming,
which means they aren't held at a guaranteed LP-11 state, even if that
driver momentarily enabled streaming.  Hence, Freescale's (or is it
Synopsis') requirement may actually be difficult to satisfy.

However, I regard runtime PM broken with the current imx-capture setup.
At the moment, power is controlled at the sensor by whether the media
links are enabled.  So, if you have an enabled link coming off the
sensor, the sensor will be powered up, whether you're using it or not.

Given that the number of applications out there that know about the
media subdevs is really quite small, this combination makes having
runtime PM in sensor devices completely pointless - they can't sleep
as long as they have an enabled link, which could be persistent over
many days or weeks.

-- 
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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel