Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue
On 3/9/2017 18:57, Hans Verkuil wrote:
Hi Songjun,
On 08/03/17 03:25, Wu, Songjun wrote:
Hi Colin,
Thank you for your comment.
It is a bug, will be fixed in the next patch.
Do you mean that you will provide a new patch for this? Is there anything
wrong with this patch? It seems reasonable to me.
Hi Hans,
I see this patch is merged in git://linuxtv.org/media_tree.git.
So I do not need submit isc-pipeline-v3 patch, just submit the patches,
based on the current master branch?
Regards,
Hans
On 3/7/2017 22:30, Colin King wrote:
From: Colin Ian King
The are only HIST_ENTRIES worth of entries in hist_entry however the
for-loop is iterating one too many times leasing to a read access off
the end off the array ctrls->hist_entry. Fix this by iterating by
the correct number of times.
Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")
Signed-off-by: Colin Ian King
---
drivers/media/platform/atmel/atmel-isc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
index b380a7d..7dacf8c 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);
*hist_count = 0;
-for (i = 0; i <= HIST_ENTRIES; i++)
+for (i = 0; i < HIST_ENTRIES; i++)
*hist_count += i * (*hist_entry++);
}
cron job: media_tree daily build: ERRORS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Mon Mar 13 05:00:14 CET 2017 media-tree git hash:700ea5e0e0dd70420a04e703ff264cc133834cba media_build git hash: bc4c2a205c087c8deff3cd14ed663c4767dd2016 v4l-utils git hash: ca6a0c099399cc51ecfacff7ef938be6ef73b8b3 gcc version:i686-linux-gcc (GCC) 6.2.0 sparse version: v0.5.0-3553-g78b2ea6 smatch version: v0.5.0-3553-g78b2ea6 host hardware: x86_64 host os:4.9.0-164 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-blackfin-bf561: OK linux-git-i686: OK linux-git-m32r: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK linux-2.6.36.4-i686: WARNINGS linux-2.6.37.6-i686: WARNINGS linux-2.6.38.8-i686: WARNINGS linux-2.6.39.4-i686: WARNINGS linux-3.0.60-i686: WARNINGS linux-3.1.10-i686: WARNINGS linux-3.2.37-i686: WARNINGS linux-3.3.8-i686: WARNINGS linux-3.4.27-i686: WARNINGS linux-3.5.7-i686: WARNINGS linux-3.6.11-i686: WARNINGS linux-3.7.4-i686: WARNINGS linux-3.8-i686: WARNINGS linux-3.9.2-i686: WARNINGS linux-3.10.1-i686: WARNINGS linux-3.11.1-i686: ERRORS linux-3.12.67-i686: ERRORS linux-3.13.11-i686: WARNINGS linux-3.14.9-i686: WARNINGS linux-3.15.2-i686: WARNINGS linux-3.16.7-i686: WARNINGS linux-3.17.8-i686: WARNINGS linux-3.18.7-i686: WARNINGS linux-3.19-i686: WARNINGS linux-4.0.9-i686: WARNINGS linux-4.1.33-i686: WARNINGS linux-4.2.8-i686: WARNINGS linux-4.3.6-i686: WARNINGS linux-4.4.22-i686: WARNINGS linux-4.5.7-i686: WARNINGS linux-4.6.7-i686: WARNINGS linux-4.7.5-i686: WARNINGS linux-4.8-i686: OK linux-4.9-i686: OK linux-4.10.1-i686: OK linux-4.11-rc1-i686: OK linux-2.6.36.4-x86_64: WARNINGS linux-2.6.37.6-x86_64: WARNINGS linux-2.6.38.8-x86_64: WARNINGS linux-2.6.39.4-x86_64: WARNINGS linux-3.0.60-x86_64: WARNINGS linux-3.1.10-x86_64: WARNINGS linux-3.2.37-x86_64: WARNINGS linux-3.3.8-x86_64: WARNINGS linux-3.4.27-x86_64: WARNINGS linux-3.5.7-x86_64: WARNINGS linux-3.6.11-x86_64: WARNINGS linux-3.7.4-x86_64: WARNINGS linux-3.8-x86_64: WARNINGS linux-3.9.2-x86_64: WARNINGS linux-3.10.1-x86_64: WARNINGS linux-3.11.1-x86_64: ERRORS linux-3.12.67-x86_64: ERRORS linux-3.13.11-x86_64: WARNINGS linux-3.14.9-x86_64: WARNINGS linux-3.15.2-x86_64: WARNINGS linux-3.16.7-x86_64: WARNINGS linux-3.17.8-x86_64: WARNINGS linux-3.18.7-x86_64: WARNINGS linux-3.19-x86_64: WARNINGS linux-4.0.9-x86_64: WARNINGS linux-4.1.33-x86_64: WARNINGS linux-4.2.8-x86_64: WARNINGS linux-4.3.6-x86_64: WARNINGS linux-4.4.22-x86_64: WARNINGS linux-4.5.7-x86_64: WARNINGS linux-4.6.7-x86_64: WARNINGS linux-4.7.5-x86_64: WARNINGS linux-4.8-x86_64: WARNINGS linux-4.9-x86_64: WARNINGS linux-4.10.1-x86_64: WARNINGS linux-4.11-rc1-x86_64: OK apps: WARNINGS spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Monday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Monday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
Re: [PATCH v5 00/39] i.MX Media Driver
On 03/12/2017 01:22 PM, Russell King - ARM Linux wrote:
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.
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.
I'll see if I have time to write that patch to IPUv3, but it's simple,
in fact what you wrote below can be translate directly into
ipu_cpmem_set_image(). There's a few other places bayer needs to be
treated in IPUv3, but it should be obvious by grepping for the
reference to pixel formats.
Steve
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, &image);
-
-
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, &image);
+
burst_size = (outfmt->width & 0xf) ? 8 : 16;
/*
Re: [PATCH] [media] vcodev: mediatek: add missing include in JPEG decoder driver
On Sun, 2017-03-12 at 16:13 -0400, Jérémy Lefaure wrote: > The driver uses kzalloc and kfree functions. So it should include > linux/slab.h. This header file is implicitly included by v4l2-common.h > if CONFIG_SPI is enabled. But when it is disabled, slab.h is not > included. In this case, the driver does not compile: > > drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c: In function ‘mtk_jpeg_open’: > drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c:1017:8: error: implicit > declaration of function ‘kzalloc’ > [-Werror=implicit-function-declaration] > ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); > ^~~ > drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c:1017:6: warning: > assignment makes pointer from integer without a cast [-Wint-conversion] > ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); > ^ > drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c:1047:2: error: implicit > declaration of function ‘kfree’ [-Werror=implicit-function-declaration] > kfree(ctx); > ^ > > This patch adds the missing include to fix this issue. > > Signed-off-by: Jérémy Lefaure > --- > drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 1 + > 1 file changed, 1 insertion(+) Acked-by: Rick Chang
Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue
On 3/9/2017 19:50, Colin Ian King wrote:
On 09/03/17 11:49, walter harms wrote:
Am 09.03.2017 11:57, schrieb Hans Verkuil:
Hi Songjun,
On 08/03/17 03:25, Wu, Songjun wrote:
Hi Colin,
Thank you for your comment.
It is a bug, will be fixed in the next patch.
Do you mean that you will provide a new patch for this? Is there anything
wrong with this patch? It seems reasonable to me.
Regards,
Hans
perhaps he will make it a bit more readable, like:
*hist_count += i * (*hist_entry++);
*hist_count += hist_entry[i]*i;
As long as it gets fixed somehow, then I'm happy.
You suggestion is very good, I will modify it like this.
Thank you.
Colin
re,
wh
On 3/7/2017 22:30, Colin King wrote:
From: Colin Ian King
The are only HIST_ENTRIES worth of entries in hist_entry however the
for-loop is iterating one too many times leasing to a read access off
the end off the array ctrls->hist_entry. Fix this by iterating by
the correct number of times.
Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")
Signed-off-by: Colin Ian King
---
drivers/media/platform/atmel/atmel-isc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
index b380a7d..7dacf8c 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);
*hist_count = 0;
-for (i = 0; i <= HIST_ENTRIES; i++)
+for (i = 0; i < HIST_ENTRIES; i++)
*hist_count += i * (*hist_entry++);
}
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 12/23] MAINTAINERS: Add file patterns for media device tree bindings
Em Sun, 12 Mar 2017 14:16:56 +0100 Geert Uytterhoeven escreveu: > Submitters of device tree binding documentation may forget to CC > the subsystem maintainer if this is missing. > > Signed-off-by: Geert Uytterhoeven > Reviewed-by: Javier Martinez Canillas > Cc: Mauro Carvalho Chehab As the To: is devicetree, I'm assuming that this patch will be applied there, so: Acked-by: Mauro Carvalho Chehab I may also merge via my tree, if that would be better. Just let me know in such case. > Cc: linux-media@vger.kernel.org > --- > Please apply this patch directly if you want to be involved in device > tree binding documentation for your subsystem. > > v2: > - Add Reviewed-by. > > Impact on next-20170310: > > -Rob Herring (maintainer:OPEN FIRMWARE AND FLATTENED > DEVICE TREE BINDINGS,commit_signer:24/39=62%) > +Mauro Carvalho Chehab (maintainer:MEDIA INPUT > INFRASTRUCTURE (V4L/DVB)) > +Rob Herring (maintainer:OPEN FIRMWARE AND FLATTENED > DEVICE TREE BINDINGS) > Mark Rutland (maintainer:OPEN FIRMWARE AND FLATTENED > DEVICE TREE BINDINGS) > -Mauro Carvalho Chehab (commit_signer:34/39=87%) > -Hans Verkuil (commit_signer:13/39=33%) > -Laurent Pinchart > (commit_signer:11/39=28%,authored:3/39=8%) > -Marek Szyprowski > (commit_signer:4/39=10%,authored:4/39=10%) > -Kieran Bingham (authored:3/39=8%) > -Martin Blumenstingl (authored:2/39=5%) > -Eric Engestrom (authored:2/39=5%) > +linux-media@vger.kernel.org (open list:MEDIA INPUT INFRASTRUCTURE (V4L/DVB)) > devicet...@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE > TREE BINDINGS) > linux-ker...@vger.kernel.org (open list) > --- > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 2692055d221e2bb2..3e108e31636d4db2 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -8085,6 +8085,7 @@ W: https://linuxtv.org > Q: http://patchwork.kernel.org/project/linux-media/list/ > T: git git://linuxtv.org/media_tree.git > S: Maintained > +F: Documentation/devicetree/bindings/media/ > F: Documentation/media/ > F: drivers/media/ > F: drivers/staging/media/ -- Thanks, Mauro
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline
Em Sun, 12 Mar 2017 22:29:04 +0100 Pavel Machek escreveu: > Mid-layer is difficult... there are _hundreds_ of possible > pipeline setups. If it should live in kernel or in userspace is a > question... but I don't think having it in kernel helps in any way. Mid-layer is difficult, because we either need to feed some library with knowledge for all kernel drivers or we need to improve the MC API to provide more details. For example, several drivers used to expose entities via the generic MEDIA_ENT_T_DEVNODE to represent entities of different types. See, for example, entities 1, 5 and 7 (and others) at: https://mchehab.fedorapeople.org/mc-next-gen/igepv2_omap3isp.png A device-specific code could either be hardcoding the entity number or checking for the entity strings to add some logic to setup controls on those "unknown" entities, a generic app won't be able to do anything with them, as it doesn't know what function(s) such entity provide. Also, on some devices, like the analog TV decoder at: https://mchehab.fedorapeople.org/mc-next-gen/au0828_test/mc_nextgen_test-output.png May have pads with different signals on their output. In such case, pads 1 and 2 provide video, while pad 3 provides audio using a different type of output. The application needs to know such kind of things in order to be able to properly setup the pipeline [1]. [1] this specific device has a fixed pipeline, but I'm aware of SoC with flexible pipelines that have this kind of issue (nobody upstreamed the V4L part of those devices yet). Thanks, Mauro
Re: [PATCH v5 00/39] i.MX Media Driver
Em Sun, 12 Mar 2017 21:13:24 +
Russell King - ARM Linux escreveu:
> 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.
That's a driver problem. v4l_id gets information to build the persistent
name from the result of VIDIOC_QUERYCAP.
In the case of Exynos gsc driver, for example, the information is here:
static int gsc_m2m_querycap(struct file *file, void *fh,
struct v4l2_capability *cap)
{
struct gsc_ctx *ctx = fh_to_ctx(fh);
struct gsc_dev *gsc = ctx->gsc_dev;
strlcpy(cap->driver, GSC_MODULE_NAME, sizeof(cap->driver));
strlcpy(cap->card, GSC_MODULE_NAME " gscaler", sizeof(cap->card));
snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
dev_name(&gsc->pdev->dev));
cap->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_M2M_MPLANE |
V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_VIDEO_OUTPUT_MPLANE;
cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
return 0;
}
See that the bus_info there is filled with:
snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
dev_name(&gsc->pdev->dev));
>From the output you printed, it seems that the i.MX6 is just doing:
snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:");
for some devices.
If you change the i.MX6 driver to do the same, you'll likely be able to
have unique names there too.
Regards,
Mauro
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline
Em Sun, 12 Mar 2017 10:56:53 -0700 Steve Longerbeam escreveu: > On 03/11/2017 11:37 PM, Russell King - ARM Linux wrote: > > On Sat, Mar 11, 2017 at 07:31:18PM -0800, Steve Longerbeam wrote: > > 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 never said that. The thing is that the V4L2 API was designed in 1999, when the video hardware were a way simpler: just one DMA engine on a PCI device, one video/audio input switch and a few video entries. On those days, setting up a pipeline on such devices is simple, and can be done via VIDIOC_*_INPUT ioctls. Nowadays hardware used on SoC devices are a way more complex. SoC devices comes with several DMA engines for buffer transfers, plus video transform blocks whose pipeline can be set dynamically. The MC API is a need to allow setting a complex pipeline, as VIDIOC_*_INPUT cannot work with such complexity. The subdev API solves a different issue. On a "traditional" device, we usually have a pipeline like: ==> ==> /dev/video0 Where controls something at the device (like bright and/or resolution) if you change something at the /dev/video0 node, it is clear that the block should handle it. On complex devices, with a pipeline like: ==> ==> ==> ==> /dev/video0 If you send a command to adjust the something at /dev/video0, it is not clear for the device driver to do it at processing0 or at processing1. Ok, the driver can decide it, but this can be sub-optimal. Yet, several drivers do that. For example, with em28xx-based drivers several parameters can be adjusted either at the em28xx driver or at the video decoder driver (saa711x). There's a logic inside the driver that decides it. The pipeline there is fixed, though, so it is easy to hardcode a logic for that. So, I've no doubt that both MC and subdev APIs are needed when full hardware control is required. I don't know about how much flexibility the i.MX6 hardware gives, nor if all such flexibility is needed for most use case applications. If I were to code a driver for such hardware, though, I would try to provide a subset of the functionality that would work without the subdev API, allowing it to work with standard V4L applications. That doesn't sound hard to do, as the driver may limit the pipelines to a subset that would make sense, in order to make easier for the driver to take the right decision about to where send a control to setup some parameter. > I do agree with you that MC places a lot of burden on the user to > attain a lot of knowledge of the system's architecture. Setting up the pipeline is not the hard part. One could write a script to do that. > That's really why I included that control inheritance patch, to > ease the burden somewhat. IMHO, that makes sense, as, once some script sets the pipeline, any V4L2 application can work, if you forward the controls to the right I2C devices. > On the other hand, I also think this just requires that MC drivers have > very good user documentation. No, it is not a matter of just documentation. It is a matter of having to rewrite applications for each device, as the information exposed by MC are not enough for an application to do what's needed. For a generic application to work properly with MC, we need to have to add more stuff to MC, in order to allow applications to know more about the features of each subdevice and to things like discovering what kind of signal is present on each PAD. We're calling it as "properties API"[1]. [1] we discussed about that at the ML and at the MC workshop: https://linuxtv.org/news.php?entry=2015-08-17.mchehab Unfortunately, nobody sent any patches implementing it so far :-( > And my other point is, I think most people who have a need to work with > the media framework on a particular platform will likely already be > quite familiar with that platform. I disagree. The most popular platform device currently is Raspberry PI. I doubt that almost all owners of RPi + camera module know anything about MC. They just use Raspberry's official driver with just provides the V4L2 interface. I have a strong opinion that, for hardware like RPi, just the V4L2 API is enough for more than 90% of the cases. > The media graph for imx6 is fairly self-explanatory in my opinion. > Yes that graph has to be generated, but just with a simple 'media-ctl > --print-dot', I don't see how that is difficult for the user. Again, IMHO, the problem is not how to setup the pipeline, but, instead, the need to forward controls to the subdevices. To use a camera, the user needs to set up a set of controls for the image to make sense (bright, contrast, focus, etc). If the driver doesn't forward those controls to the subdevs, an application like "camorama" won't actually work for real, as the user won't be able to adjust those parameters via GUI. Thanks, Mauro
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline
On Sat 2017-03-11 23:14:56, Russell King - ARM Linux wrote: > 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. Yeah. We need a mid-layer between legacy applications and MC devices. Such layer does not exist in userspace or in kernel. > 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? We have docs. Users can write shell scripts. Still, mid-layer would be nice. > 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 seriously > doubt that we will ever see any kind of solution to this if we > continue to progress with "we'll sort it in userspace some day." Mid-layer is difficult... there are _hundreds_ of possible pipeline setups. If it should live in kernel or in userspace is a question... but I don't think having it in kernel helps in any way. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v5 00/39] i.MX Media Driver
On Sun, Mar 12, 2017 at 05:59:28PM -0300, Mauro Carvalho Chehab wrote:
> Yet, udev/systemd has some rules that provide an unique name for V4L
> devices at /lib/udev/rules.d/60-persistent-v4l.rules. Basically, it
> runs a small application (v4l_id) with creates a persistent symling
> using rules like this:
>
> KERNEL=="video*", ENV{ID_SERIAL}=="?*",
> SYMLINK+="v4l/by-id/$env{ID_BUS}-$env{ID_SERIAL}-video-index$attr{index}"
>
> Those names are stored at /dev/v4l/by-path.
This doesn't help:
$ ls -Al /dev/v4l/by-id/
total 0
lrwxrwxrwx 1 root root 13 Mar 12 19:54
usb-Sonix_Technology_Co.__Ltd._USB_2.0_Camera-video-index0 -> ../../video10
$ ls -Al /dev/v4l/by-path/
total 0
lrwxrwxrwx 1 root root 12 Mar 12 19:54 platform-204.vpu-video-index0 ->
../../video0
lrwxrwxrwx 1 root root 12 Mar 12 19:54 platform-204.vpu-video-index1 ->
../../video1
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index0
-> ../../video2
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index1
-> ../../video3
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index2
-> ../../video4
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index3
-> ../../video5
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index4
-> ../../video6
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index5
-> ../../video7
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index6
-> ../../video8
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index7
-> ../../video9
lrwxrwxrwx 1 root root 13 Mar 12 19:54
platform-ci_hdrc.0-usb-0:1:1.0-video-index0 -> ../../video10
The problem is the "platform-capture-subsystem-video-index" entries.
These themselves change order. For instance, I now have:
- entity 72: ipu1_csi0 capture (1 pad, 1 link)
type Node subtype V4L flags 0
device node name /dev/video6
which means it's platform-capture-subsystem-video-index4. Before, it
was platform-capture-subsystem-video-index2.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
Re: [PATCH v5 00/39] i.MX Media Driver
On Sun, Mar 12, 2017 at 08:40:37PM +, Russell King - ARM Linux wrote: > On Sun, Mar 12, 2017 at 01:36:32PM -0700, Steve Longerbeam wrote: > > But hold on, if my logic is correct, then why did the CSI power-off > > get reached in your case, multiple times? Yes I think there is a bug, > > link_notify() is not checking if the link has already been disabled. > > I will fix this. But I'm surprised media core's link_notify handling > > doesn't do this. > > Well, I think there's something incredibly fishy going on here. I > turned that dev_dbg() at the top of the function into a dev_info(), > and I get: > > root@hbi2ex:~# dmesg |grep -A2 imx-ipuv3-csi > [ 53.370949] imx-ipuv3-csi imx-ipuv3-csi.0: power OFF > [ 53.371015] [ cut here ] > [ 53.371075] WARNING: CPU: 0 PID: 1515 at > drivers/staging/media/imx/imx-media-csi.c:806 csi_s_power+0xb8/0xd0 > [imx_media_csi] > -- > [ 53.372624] imx-ipuv3-csi imx-ipuv3-csi.0: power OFF > [ 53.372637] [ cut here ] > [ 53.372663] WARNING: CPU: 0 PID: 1515 at > drivers/staging/media/imx/imx-media-csi.c:806 csi_s_power+0xb8/0xd0 > [imx_media_csi] > > There isn't a power on event being generated before these two power > off events. I don't see a power on event even when I attempt to > start streaming either (which fails due to the lack of bayer > support.) Found it - my imx219 driver returns '1' from its s_power function when powering up, which triggers a bug in your code - when imx_media_set_power() fails to power up, you call imx_media_set_power() telling it to power everything off - including devices that are already powered off. This is really bad news - s_power() may be called via other paths, such as when the subdev is opened. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v5 00/39] i.MX Media Driver
Em Sun, 12 Mar 2017 19:47:00 +
Russell King - ARM Linux escreveu:
> 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.)
>
Didn't have time yet to read/comment the other e-mails in this thread.
Yet, as this is a simple issue, let me answer it first.
With regards to /dev/video?, the device number depends on the probing
order, with can be random on SoC drivers, due to the way OF works.
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.
For example, on Exynos, we have:
$ ls -lctra /dev/v4l/by-path/
total 0
lrwxrwxrwx 1 root root 12 Mar 11 07:19
platform-13e1.video-scaler-video-index0 -> ../../video7
lrwxrwxrwx 1 root root 12 Mar 11 07:19
platform-13e0.video-scaler-video-index0 -> ../../video6
lrwxrwxrwx 1 root root 12 Mar 11 07:19 platform-11f6.jpeg-video-index0 ->
../../video4
lrwxrwxrwx 1 root root 12 Mar 11 07:19 platform-11f5.jpeg-video-index1 ->
../../video3
lrwxrwxrwx 1 root root 12 Mar 11 07:19 platform-11f5.jpeg-video-index0 ->
../../video2
drwxr-xr-x 3 root root 60 Mar 11 07:19 ..
lrwxrwxrwx 1 root root 12 Mar 11 07:19 platform-11f6.jpeg-video-index1 ->
../../video5
lrwxrwxrwx 1 root root 12 Mar 11 07:19 platform-1100.codec-video-index1 ->
../../video1
lrwxrwxrwx 1 root root 12 Mar 11 07:19 platform-1100.codec-video-index0 ->
../../video0
No matter what driver gets probed first, the above names should not
change.
So, if you want to write a script, the best is to use the /dev/v4l/by-path.
Unfortunately, gstreamer has some issues with that, as some of their plugins
don't seem to allow passing the name of the devnode, but just the number of
/dev/video?.
So, you need some script to convert from /dev/v4l/by-path/foo to
/dev/video?.
What I'm using on Exynos scripts is this logic:
NEEDED1=platform-13e0.video-scaler-video-index0
DEV1=$(ls -l /dev/v4l/by-path/$NEEDED1|perl -ne ' print $1 if
(m,/video(\d+),)')
Then, if I need to talk with this mem2mem driver using the v4l2video
convert plugin, I can launch gst with something like:
gst-launch-1.0 videotestsrc ! v4l2video${DEV1}convert ! fakesink
Thanks,
Mauro
Thanks,
Mauro
Re: [PATCH v5 00/39] i.MX Media Driver
On Sun, Mar 12, 2017 at 01:36:32PM -0700, Steve Longerbeam wrote: > But hold on, if my logic is correct, then why did the CSI power-off > get reached in your case, multiple times? Yes I think there is a bug, > link_notify() is not checking if the link has already been disabled. > I will fix this. But I'm surprised media core's link_notify handling > doesn't do this. Well, I think there's something incredibly fishy going on here. I turned that dev_dbg() at the top of the function into a dev_info(), and I get: root@hbi2ex:~# dmesg |grep -A2 imx-ipuv3-csi [ 53.370949] imx-ipuv3-csi imx-ipuv3-csi.0: power OFF [ 53.371015] [ cut here ] [ 53.371075] WARNING: CPU: 0 PID: 1515 at drivers/staging/media/imx/imx-media-csi.c:806 csi_s_power+0xb8/0xd0 [imx_media_csi] -- [ 53.372624] imx-ipuv3-csi imx-ipuv3-csi.0: power OFF [ 53.372637] [ cut here ] [ 53.372663] WARNING: CPU: 0 PID: 1515 at drivers/staging/media/imx/imx-media-csi.c:806 csi_s_power+0xb8/0xd0 [imx_media_csi] There isn't a power on event being generated before these two power off events. I don't see a power on event even when I attempt to start streaming either (which fails due to the lack of bayer support.) -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v5 00/39] i.MX Media Driver
On 03/12/2017 01:36 PM, Steve Longerbeam wrote:
On 03/12/2017 01:16 PM, Steve Longerbeam wrote:
On 03/12/2017 12:44 PM, Steve Longerbeam wrote:
On 03/12/2017 12:29 PM, Russell King - ARM Linux wrote:
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?
Yes, the CSI will be powered off even if it still has an enabled link.
But one of its other links has been disabled, meaning the pipeline as
a whole is disabled. So I think it makes sense to power down the CSI,
the pipeline isn't usable at that point.
And remember that the CSI does not allow both output pads to be enabled
at the same time. If that were so then indeed there would be a problem,
because it would mean there is another active pipeline that requires the
CSI being powered on, but that's not the case.
I think this is consistent with the other entities as well, but I will
double check.
At first I thought this could be a problem for one entity, the csi-2
receiver.
It can enable all four of its output pads at once (if the input stream
contains all 4 virtual channels, the csi-2 receiver must support
demuxing all of them onto all 4 of its output pads).
But after more review, this should not be an issue. If a csi-2 sink
(a CSI or a CSI mux) link is disabled, the csi-2 receiver is no longer
reachable from that sink, so attempts to disable the csi-2 via that
path again is not possible. The other potential problem is disabling
from the csi-2's own sink pad, but in that case the csi-2 no longer
has a source, so again it makes sense to power off the csi-2 even
if it has enabled output pads.
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.
but it does:
int __media_entity_setup_link(struct media_link *link, u32 flags)
{
...
if (link->flags == flags)
return 0;
...
}
What the heck. Anyway, I'll track this down.
Steve
Steve
Re: [PATCH v5 00/39] i.MX Media Driver
On 03/12/2017 01:16 PM, Steve Longerbeam wrote: On 03/12/2017 12:44 PM, Steve Longerbeam wrote: On 03/12/2017 12:29 PM, Russell King - ARM Linux wrote: 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? Yes, the CSI will be powered off even if it still has an enabled link. But one of its other links has been disabled, meaning the pipeline as a whole is disabled. So I think it makes sense to power down the CSI, the pipeline isn't usable at that point. And remember that the CSI does not allow both output pads to be enabled at the same time. If that were so then indeed there would be a problem, because it would mean there is another active pipeline that requires the CSI being powered on, but that's not the case. I think this is consistent with the other entities as well, but I will double check. At first I thought this could be a problem for one entity, the csi-2 receiver. It can enable all four of its output pads at once (if the input stream contains all 4 virtual channels, the csi-2 receiver must support demuxing all of them onto all 4 of its output pads). But after more review, this should not be an issue. If a csi-2 sink (a CSI or a CSI mux) link is disabled, the csi-2 receiver is no longer reachable from that sink, so attempts to disable the csi-2 via that path again is not possible. The other potential problem is disabling from the csi-2's own sink pad, but in that case the csi-2 no longer has a source, so again it makes sense to power off the csi-2 even if it has enabled output pads. 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. Steve
Re: [PATCH v5 00/39] i.MX Media Driver
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, &image);
-
-
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, &image);
+
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.
[PATCH] [media] vcodev: mediatek: add missing include in JPEG decoder driver
The driver uses kzalloc and kfree functions. So it should include linux/slab.h. This header file is implicitly included by v4l2-common.h if CONFIG_SPI is enabled. But when it is disabled, slab.h is not included. In this case, the driver does not compile: drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c: In function ‘mtk_jpeg_open’: drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c:1017:8: error: implicit declaration of function ‘kzalloc’ [-Werror=implicit-function-declaration] ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); ^~~ drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c:1017:6: warning: assignment makes pointer from integer without a cast [-Wint-conversion] ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); ^ drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c:1047:2: error: implicit declaration of function ‘kfree’ [-Werror=implicit-function-declaration] kfree(ctx); ^ This patch adds the missing include to fix this issue. Signed-off-by: Jérémy Lefaure --- drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c index b10183f7942b..f9bd58ce7d32 100644 --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include -- 2.12.0
Re: [PATCH v5 00/39] i.MX Media Driver
On 03/12/2017 12:44 PM, Steve Longerbeam wrote: On 03/12/2017 12:29 PM, Russell King - ARM Linux wrote: 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? Yes, the CSI will be powered off even if it still has an enabled link. But one of its other links has been disabled, meaning the pipeline as a whole is disabled. So I think it makes sense to power down the CSI, the pipeline isn't usable at that point. And remember that the CSI does not allow both output pads to be enabled at the same time. If that were so then indeed there would be a problem, because it would mean there is another active pipeline that requires the CSI being powered on, but that's not the case. I think this is consistent with the other entities as well, but I will double check. At first I thought this could be a problem for one entity, the csi-2 receiver. It can enable all four of its output pads at once (if the input stream contains all 4 virtual channels, the csi-2 receiver must support demuxing all of them onto all 4 of its output pads). But after more review, this should not be an issue. If a csi-2 sink (a CSI or a CSI mux) link is disabled, the csi-2 receiver is no longer reachable from that sink, so attempts to disable the csi-2 via that path again is not possible. The other potential problem is disabling from the csi-2's own sink pad, but in that case the csi-2 no longer has a source, so again it makes sense to power off the csi-2 even if it has enabled output pads. Steve
Re: [PATCH v5 00/39] i.MX Media Driver
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. Steve 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]
Re: [PATCH v5 00/39] i.MX Media Driver
On 03/12/2017 12:47 PM, Russell King - ARM Linux wrote: 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. Yes, that's still an issue I haven't had the chance to get to yet. It could be as simple as passing a fixed device node # to video_register_device(), but something tells me it won't be that easy. But I'll get to this in next version. Steve 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.)
Re: [PATCH v5 00/39] i.MX Media Driver
On Sat, Mar 11, 2017 at 04:30:53PM -0800, Steve Longerbeam wrote: > If it's too difficult to get the imx219 csi-2 transmitter into the > LP-11 state on power on, perhaps the csi-2 receiver can be a little > more lenient on the transmitter and make the LP-11 timeout a warning > instead of error-out. > > Can you try the attached change on top of the version 5 patchset? > > If that doesn't work then you're just going to have to fix the bug > in imx219. That patch gets me past that hurdle, only to reveal that there's another issue: imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200 imx219 0-0010: VT: pixclk 13920Hz line 80742Hz frame 30.0Hz imx219 0-0010: VT: line period 12385ns imx219 0-0010: OP: pixclk 3850Hz, 2 lanes, 308Mbps peak each imx219 0-0010: OP: 3288 bits/line/lane act=10675ns lp/idle=1710ns ipu1_csi0: csi_idmac_setup failed: -22 ipu1_csi0: pipeline start failed with -22 [ cut here ] WARNING: CPU: 0 PID: 1860 at /home/rmk/git/linux-rmk/drivers/media/v4l2-core/videobuf2-core.c:1340 vb2_start_streaming+0x124/0x1b4 [videobuf2_core] -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v5 00/39] i.MX Media Driver
Another issue. The "reboot and the /dev/video* devices come up in a completely different order" problem seems to exist with this version. The dot graph I supplied previously had "ipu1_csi0 capture" on /dev/video4. I've just rebooted, and now I find it's on /dev/video2 instead. Here's the extract from the .dot file of the old listing: n0018 [label="ipu1_ic_prpenc capture\n/dev/video0", shape=box, style=filled, fillcolor=yellow] n0021 [label="ipu1_ic_prpvf capture\n/dev/video1", shape=box, style=filled, fillcolor=yellow] n002e [label="ipu2_ic_prpenc capture\n/dev/video2", shape=box, style=filled, fillcolor=yellow] n0037 [label="ipu2_ic_prpvf capture\n/dev/video3", shape=box, style=filled, fillcolor=yellow] n0048 [label="ipu1_csi0 capture\n/dev/video4", shape=box, style=filled, fillcolor=yellow] n0052 [label="ipu1_csi1 capture\n/dev/video5", shape=box, style=filled, fillcolor=yellow] n0062 [label="ipu2_csi0 capture\n/dev/video6", shape=box, style=filled, fillcolor=yellow] n006c [label="ipu2_csi1 capture\n/dev/video7", shape=box, style=filled, fillcolor=yellow] and here's the same after reboot: n0014 [label="ipu1_csi0 capture\n/dev/video2", shape=box, style=filled, fillcolor=yellow] n001e [label="ipu1_csi1 capture\n/dev/video3", shape=box, style=filled, fillcolor=yellow] n0028 [label="ipu2_csi0 capture\n/dev/video4", shape=box, style=filled, fillcolor=yellow] n0035 [label="ipu1_ic_prpenc capture\n/dev/video5", shape=box, style=filled, fillcolor=yellow] n003e [label="ipu1_ic_prpvf capture\n/dev/video6", shape=box, style=filled, fillcolor=yellow] n004c [label="ipu2_csi1 capture\n/dev/video7", shape=box, style=filled, fillcolor=yellow] n0059 [label="ipu2_ic_prpenc capture\n/dev/video8", shape=box, style=filled, fillcolor=yellow] n0062 [label="ipu2_ic_prpvf capture\n/dev/video9", shape=box, style=filled, fillcolor=yellow] (/dev/video0 and /dev/video1 are taken up by CODA, since I updated the names of the firmware files, and now CODA initialises... seems the back-compat filenames don't work, but that's not a problem with imx6 capture.) -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v5 00/39] i.MX Media Driver
On 03/12/2017 12:29 PM, Russell King - ARM Linux wrote: 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? Yes, the CSI will be powered off even if it still has an enabled link. But one of its other links has been disabled, meaning the pipeline as a whole is disabled. So I think it makes sense to power down the CSI, the pipeline isn't usable at that point. And remember that the CSI does not allow both output pads to be enabled at the same time. If that were so then indeed there would be a problem, because it would mean there is another active pipeline that requires the CSI being powered on, but that's not the case. I think this is consistent with the other entities as well, but I will double check. Steve
Re: [PATCH v5 00/39] i.MX Media Driver
On Sun, Mar 12, 2017 at 12:21:45PM -0700, Steve Longerbeam wrote: > There's actually nothing preventing userland from disabling a link > multiple times, and imx_media_link_notify() complies, and so > csi_s_power(OFF) gets called multiple times, and so that WARN_ON() > in there is silly, I borrowed this from other MC driver examples, > but it makes no sense to me, I'll remove it and prevent the power > count from going negative. Hmm. So what happens if one of the CSI's links is enabled, and we disable a different link from the CSI several times? Doesn't that mean the power count will go to zero despite there being an enabled link? -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v5 00/39] i.MX Media Driver
On 03/12/2017 10:51 AM, Russell King - ARM Linux wrote: 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. 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. Steve [ 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_p
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging
On Sun, Mar 12, 2017 at 2:34 PM, Benjamin Gaignard wrote: > 2017-03-09 18:38 GMT+01:00 Laura Abbott : >> On 03/09/2017 02:00 AM, Benjamin Gaignard wrote: >>> 2017-03-06 17:04 GMT+01:00 Daniel Vetter : On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote: > On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote: > >> No one gave a thing about android in upstream, so Greg KH just dumped it >> all into staging/android/. We've discussed ION a bunch of times, recorded >> anything we'd like to fix in staging/android/TODO, and Laura's patch >> series here addresses a big chunk of that. > >> This is pretty much the same approach we (gpu folks) used to de-stage the >> syncpt stuff. > > Well, there's also the fact that quite a few people have issues with the > design (like Laurent). It seems like a lot of them have either got more > comfortable with it over time, or at least not managed to come up with > any better ideas in the meantime. See the TODO, it has everything a really big group (look at the patch for the full Cc: list) figured needs to be improved at LPC 2015. We don't just merge stuff because merging stuff is fun :-) Laurent was even in that group ... -Daniel >>> >>> For me those patches are going in the right direction. >>> >>> I still have few questions: >>> - since alignment management has been remove from ion-core, should it >>> be also removed from ioctl structure ? >> >> Yes, I think I'm going to go with the suggestion to fixup the ABI >> so we don't need the compat layer and as part of that I'm also >> dropping the align argument. >> >>> - can you we ride off ion_handle (at least in userland) and only >>> export a dma-buf descriptor ? >> >> Yes, I think this is the right direction given we're breaking >> everything anyway. I was debating trying to keep the two but >> moving to only dma bufs is probably cleaner. The only reason >> I could see for keeping the handles is running out of file >> descriptors for dma-bufs but that seems unlikely. >>> >>> In the future how can we add new heaps ? >>> Some platforms have very specific memory allocation >>> requirements (just have a look in the number of gem custom allocator in drm) >>> Do you plan to add heap type/mask for each ? >> >> Yes, that was my thinking. > > My concern is about the policy to adding heaps, will you accept > "customs" heap per > platforms ? per devices ? or only generic ones ? > If you are too strict, we will have lot of out-of-tree heaps and if > you accept of of them > it will be a nightmare to maintain I think ion should expose any heap that's also directly accessible to devices using dma_alloc(_coherent). That should leave very few things left, like your SMA heap. > Another point is how can we put secure rules (like selinux policy) on > heaps since all the allocations > go to the same device (/dev/ion) ? For example, until now, in Android > we have to give the same > access rights to all the process that use ION. > It will become problem when we will add secure heaps because we won't > be able to distinguish secure > processes to standard ones or set specific policy per heaps. > Maybe I'm wrong here but I have never see selinux policy checking an > ioctl field but if that > exist it could be a solution. Hm, we might want to expose all the heaps as individual /dev/ion_$heapname nodes? Should we do this from the start, since we're massively revamping the uapi anyway (imo not needed, current state seems to work too)? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline
On Sun 2017-03-12 07:37:45, Russell King - ARM Linux wrote: > On Sat, Mar 11, 2017 at 07:31:18PM -0800, Steve Longerbeam wrote: > > > > > > On 03/11/2017 10:59 AM, Russell King - ARM Linux wrote: > > >On Sat, Mar 11, 2017 at 10:54:55AM -0800, Steve Longerbeam wrote: > > >> > > >> > > >>On 03/11/2017 10:45 AM, Russell King - ARM Linux wrote: > > >>>I really don't think expecting the user to understand and configure > > >>>the pipeline is a sane way forward. Think about it - should the > > >>>user need to know that, because they have a bayer-only CSI data > > >>>source, that there is only one path possible, and if they try to > > >>>configure a different path, then things will just error out? > > >>> > > >>>For the case of imx219 connected to iMX6, it really is as simple as > > >>>"there is only one possible path" and all the complexity of the media > > >>>interfaces/subdevs is completely unnecessary. Every other block in > > >>>the graph is just noise. > > >>> > > >>>The fact is that these dot graphs show a complex picture, but reality > > >>>is somewhat different - there's only relatively few paths available > > >>>depending on the connected source and the rest of the paths are > > >>>completely useless. > > >>> > > >> > > >>I totally disagree there. Raw bayer requires passthrough yes, but for > > >>all other media bus formats on a mipi csi-2 bus, and all other media > > >>bus formats on 8-bit parallel buses, the conersion pipelines can be > > >>used for scaling, CSC, rotation, and motion-compensated de-interlacing. > > > > > >... which only makes sense _if_ your source can produce those formats. > > >We don't actually disagree on that. > > > > ...and there are lots of those sources! You should try getting out of > > your imx219 shell some time, and have a look around! :) > > If you think that, you are insulting me. I've been thinking about this > from the "big picture" point of view. If you think I'm only thinking > about this from only the bayer point of view, you're wrong. Can you stop that insults nonsense? > 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. Hmm. But you did not present any alternative. Seems some hardware is simply complex. So either we don't add complex drivers (_that_ would be a mistake), or some userspace solution will need to be done. Shell-script running media-ctl does not seem that hard. > 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? Complex hardware sometimes requires userspace configuration. Running a shell script on startup does not seem that hard. And maybe we could do some kind of default setup in kernel, but that does not really solve the problem. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline
On 03/11/2017 11:37 PM, Russell King - ARM Linux wrote: On Sat, Mar 11, 2017 at 07:31:18PM -0800, Steve Longerbeam wrote: On 03/11/2017 10:59 AM, Russell King - ARM Linux wrote: On Sat, Mar 11, 2017 at 10:54:55AM -0800, Steve Longerbeam wrote: On 03/11/2017 10:45 AM, Russell King - ARM Linux wrote: I really don't think expecting the user to understand and configure the pipeline is a sane way forward. Think about it - should the user need to know that, because they have a bayer-only CSI data source, that there is only one path possible, and if they try to configure a different path, then things will just error out? For the case of imx219 connected to iMX6, it really is as simple as "there is only one possible path" and all the complexity of the media interfaces/subdevs is completely unnecessary. Every other block in the graph is just noise. The fact is that these dot graphs show a complex picture, but reality is somewhat different - there's only relatively few paths available depending on the connected source and the rest of the paths are completely useless. I totally disagree there. Raw bayer requires passthrough yes, but for all other media bus formats on a mipi csi-2 bus, and all other media bus formats on 8-bit parallel buses, the conersion pipelines can be used for scaling, CSC, rotation, and motion-compensated de-interlacing. ... which only makes sense _if_ your source can produce those formats. We don't actually disagree on that. ...and there are lots of those sources! You should try getting out of your imx219 shell some time, and have a look around! :) If you think that, you are insulting me. I've been thinking about this from the "big picture" point of view. If you think I'm only thinking about this from only the bayer point of view, you're wrong. No insult there, you have my utmost respect Russel. Me gives you the Ali-G "respec!" :) It was just a light-hearted attempt at suggesting you might be too entangled with the imx219 (or short on hardware access, which I can certainly understand). Given what Mauro has said, I'm convinced that the media controller stuff is a complete failure for usability, and adding further drivers using it is a mistake. I do agree with you that MC places a lot of burden on the user to attain a lot of knowledge of the system's architecture. That's really why I included that control inheritance patch, to ease the burden somewhat. On the other hand, I also think this just requires that MC drivers have very good user documentation. And my other point is, I think most people who have a need to work with the media framework on a particular platform will likely already be quite familiar with that platform. I counter your accusation by saying that you are actually so focused on the media controller way of doing things that you can't see the bigger picture here. Yeah I've been too mired in the details of this driver. So, tell me how the user can possibly use iMX6 video capture without resorting to opening up a terminal and using media-ctl to manually configure the pipeline. How is the user going to control the source device without using media-ctl to find the subdev node, and then using v4l2-ctl on it. How is the user supposed to know which /dev/video* node they should be opening with their capture application? The media graph for imx6 is fairly self-explanatory in my opinion. Yes that graph has to be generated, but just with a simple 'media-ctl --print-dot', I don't see how that is difficult for the user. The graph makes it quite clear which subdev node belongs to which entity. As for which /dev/videoX node to use, I hope I made it fairly clear in the user doc what functions each node performs. But I will review the doc again and make sure it's been made explicitly clear. If you can actually respond to the points that I've been raising about end user usability, then we can have a discussion. Right, I haven't added my input to the middle-ware discussions (libv4l, v4lconvert, and the auto-pipeline-configuration library work). I can only say at this point that v4lconvert does indeed sound broken w.r.t bayer formats from your description. But it also sounds like an isolated problem and it just needs a patch to allow passing bayer through without software conversion. I wish I had the IMX219 to help you debug these bayer issues. I don't have any bayer sources. In summary, I do like the media framework, it's a good abstraction of hardware pipelines. It does require a lot of system level knowledge to configure, but as I said that is a matter of good documentation. Steve
Re: [PATCH v5 00/39] i.MX Media Driver
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 r7:ede00
Re: [PATCH v5 00/39] i.MX Media Driver
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.
[PATCH v2] staging: media: Remove unused function atomisp_set_stop_timeout()
The function atomisp_set_stop_timeout on being called, simply returns
back. The function hasn't been mentioned in the TODO and doesn't have
FIXME code around. Hence, atomisp_set_stop_timeout and its calls have been
removed.
This was done using Coccinelle.
@@
identifier f;
@@
void f(...) {
-return;
}
Signed-off-by: simran singhal
---
v2:
-cc the patch to more developers
drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 1 -
drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h | 1 -
drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c | 5 -
3 files changed, 7 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index d9a5c24..9720756 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -1692,7 +1692,6 @@ void atomisp_wdt_work(struct work_struct *work)
}
}
#endif
- atomisp_set_stop_timeout(ATOMISP_CSS_STOP_TIMEOUT_US);
dev_err(isp->dev, "timeout recovery handling done\n");
atomic_set(&isp->wdt_work_queued, 0);
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h
index e6b0cce..fb8b8fa 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h
@@ -660,7 +660,6 @@ int atomisp_css_set_acc_parameters(struct atomisp_acc_fw
*acc_fw);
int atomisp_css_isr_thread(struct atomisp_device *isp,
bool *frame_done_found,
bool *css_pipe_done);
-void atomisp_set_stop_timeout(unsigned int timeout);
bool atomisp_css_valid_sof(struct atomisp_device *isp);
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
index 6697d72..cfa0ad4 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
@@ -4699,11 +4699,6 @@ int atomisp_css_isr_thread(struct atomisp_device *isp,
return 0;
}
-void atomisp_set_stop_timeout(unsigned int timeout)
-{
- return;
-}
-
bool atomisp_css_valid_sof(struct atomisp_device *isp)
{
unsigned int i, j;
--
2.7.4
[GIT PULL] soc-camera for 4.12
Hi Mauro, The following changes since commit 700ea5e0e0dd70420a04e703ff264cc133834cba: Merge tag 'v4.11-rc1' into patchwork (2017-03-06 06:49:34 -0300) are available in the git repository at: git://linuxtv.org/gliakhovetski/v4l-dvb.git for-4.12-1 for you to fetch changes up to c259da29a447dbb5737c5c85e99c039263df94cc: soc-camera: fix rectangle adjustment in cropping (2017-03-12 12:53:25 +0100) Bhumika Goyal (1): media: i2c: soc_camera: constify v4l2_subdev_* structures Geliang Tang (1): sh_mobile_ceu_camera: use module_platform_driver Janusz Krzysztofik (1): media: i2c/soc_camera: fix ov6650 sensor getting wrong clock Koji Matsuoka (1): soc-camera: fix rectangle adjustment in cropping drivers/media/i2c/soc_camera/imx074.c| 6 +++--- drivers/media/i2c/soc_camera/mt9m001.c | 6 +++--- drivers/media/i2c/soc_camera/mt9t031.c | 6 +++--- drivers/media/i2c/soc_camera/mt9t112.c | 6 +++--- drivers/media/i2c/soc_camera/mt9v022.c | 6 +++--- drivers/media/i2c/soc_camera/ov2640.c| 6 +++--- drivers/media/i2c/soc_camera/ov5642.c| 6 +++--- drivers/media/i2c/soc_camera/ov6650.c| 8 drivers/media/i2c/soc_camera/ov772x.c| 6 +++--- drivers/media/i2c/soc_camera/ov9640.c| 6 +++--- drivers/media/i2c/soc_camera/ov9740.c| 6 +++--- drivers/media/i2c/soc_camera/rj54n1cb0c.c| 6 +++--- drivers/media/i2c/soc_camera/tw9910.c| 6 +++--- drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c | 13 + drivers/media/platform/soc_camera/soc_scale_crop.c | 11 ++- 15 files changed, 47 insertions(+), 57 deletions(-) Thanks Guennadi
Re: [PATCHv5 07/16] atmel-isi: remove dependency of the soc-camera framework
Hi Hans,
Thanks for the patch. Why hasn't this patch been CCed to Josh Wu? Is he
still at Atmel? Adding to CC to check.
A general comment: this patch doesn't only remove soc-camera dependencies,
it also changes the code and the behaviour. I would prefer to break that
down in multiple patches, or remove this driver completely and add a new
one. I'll provide some comments, but of course I cannot make an extensive
review of a 1200-line of change patch without knowing the hardware and the
set ups well enough.
On Sat, 11 Mar 2017, Hans Verkuil wrote:
> From: Hans Verkuil
>
> This patch converts the atmel-isi driver from a soc-camera driver to a driver
> that is stand-alone.
>
> Signed-off-by: Hans Verkuil
> ---
> drivers/media/platform/soc_camera/Kconfig |3 +-
> drivers/media/platform/soc_camera/atmel-isi.c | 1209
> +++--
> 2 files changed, 714 insertions(+), 498 deletions(-)
>
> diff --git a/drivers/media/platform/soc_camera/Kconfig
> b/drivers/media/platform/soc_camera/Kconfig
> index 86d74788544f..a37ec91b026e 100644
> --- a/drivers/media/platform/soc_camera/Kconfig
> +++ b/drivers/media/platform/soc_camera/Kconfig
> @@ -29,9 +29,8 @@ config VIDEO_SH_MOBILE_CEU
>
> config VIDEO_ATMEL_ISI
> tristate "ATMEL Image Sensor Interface (ISI) support"
> - depends on VIDEO_DEV && SOC_CAMERA
> + depends on VIDEO_V4L2 && OF && HAS_DMA
> depends on ARCH_AT91 || COMPILE_TEST
> - depends on HAS_DMA
> select VIDEOBUF2_DMA_CONTIG
> ---help---
> This module makes the ATMEL Image Sensor Interface available
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
> b/drivers/media/platform/soc_camera/atmel-isi.c
> index 46de657c3e6d..a6d60c2e207d 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -22,18 +22,22 @@
> #include
> #include
> #include
> -
> -#include
> -#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> #include
> #include
> +#include
>
> #include "atmel-isi.h"
>
> -#define MAX_BUFFER_NUM 32
> #define MAX_SUPPORT_WIDTH2048
> #define MAX_SUPPORT_HEIGHT 2048
> -#define VID_LIMIT_BYTES (16 * 1024 * 1024)
> #define MIN_FRAME_RATE 15
> #define FRAME_INTERVAL_MILLI_SEC (1000 / MIN_FRAME_RATE)
>
> @@ -65,9 +69,37 @@ struct frame_buffer {
> struct list_head list;
> };
>
> +struct isi_graph_entity {
> + struct device_node *node;
> +
> + struct v4l2_async_subdev asd;
> + struct v4l2_subdev *subdev;
> +};
> +
> +/*
> + * struct isi_format - ISI media bus format information
> + * @fourcc: Fourcc code for this format
> + * @mbus_code: V4L2 media bus format code.
> + * @bpp: Bytes per pixel (when stored in memory)
> + * @swap:Byte swap configuration value
> + * @support: Indicates format supported by subdev
> + * @skip:Skip duplicate format supported by subdev
> + */
> +struct isi_format {
> + u32 fourcc;
> + u32 mbus_code;
> + u8 bpp;
> + u32 swap;
> +
> + boolsupport;
> + boolskip;
> +};
> +
> +
> struct atmel_isi {
> /* Protects the access of variables shared with the ISR */
> - spinlock_t lock;
> + spinlock_t irqlock;
> + struct device *dev;
> void __iomem*regs;
>
> int sequence;
> @@ -76,7 +108,7 @@ struct atmel_isi {
> struct fbd *p_fb_descriptors;
> dma_addr_t fb_descriptors_phys;
> struct list_head dma_desc_head;
> - struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
> + struct isi_dma_desc dma_desc[VIDEO_MAX_FRAME];
> boolenable_preview_path;
>
> struct completion complete;
> @@ -90,9 +122,22 @@ struct atmel_isi {
> struct list_headvideo_buffer_list;
> struct frame_buffer *active;
>
> - struct soc_camera_host soc_host;
> + struct v4l2_device v4l2_dev;
> + struct video_device *vdev;
> + struct v4l2_async_notifier notifier;
> + struct isi_graph_entity entity;
> + struct v4l2_format fmt;
> +
> + struct isi_format **user_formats;
> + unsigned intnum_user_formats;
> + const struct isi_format *current_fmt;
> +
> + struct mutexlock;
> + struct vb2_queuequeue;
> };
>
> +#define notifier_to_isi(n) container_of(n, struct atmel_isi, notifier)
> +
> static void isi_writel(struct atmel_isi *isi, u32 reg, u32 val)
> {
> writel(val, isi
Re: [Outreachy kernel] Re: [PATCH v1] staging: media: Remove unused function atomisp_set_stop_timeout()
On Sun, 12 Mar 2017, SIMRAN SINGHAL wrote:
> On Sun, Mar 12, 2017 at 7:24 PM, Greg KH wrote:
> > On Fri, Mar 10, 2017 at 07:05:05PM +0530, simran singhal wrote:
> >> The function atomisp_set_stop_timeout on being called, simply returns
> >> back. The function hasn't been mentioned in the TODO and doesn't have
> >> FIXME code around. Hence, atomisp_set_stop_timeout and its calls have been
> >> removed.
> >>
> >> This was done using Coccinelle.
> >>
> >> @@
> >> identifier f;
> >> @@
> >>
> >> void f(...) {
> >>
> >> -return;
> >>
> >> }
> >>
> >> Signed-off-by: simran singhal
> >> ---
> >> v1:
> >>-Change Subject to include name of function
> >>-change commit message to include the coccinelle script
> >
> > You should also cc: the developers doing all of the current work on this
> > driver, Alan Cox, to get their comment if this really is something that
> > can be removed or not.
> >
> > thanks,
> >
> Greg I have cc'd all the developers which script get_maintainer.pl showed:
>
> $ git show HEAD | perl scripts/get_maintainer.pl --separator ,
> --nokeywords --nogit --nogit-fallback --norolestats
Sometimes people do a lot of work on something without being the
maintainer. You can see who has done this using git log. Dropping some
of the "no" arguments might give you the same information, but in practice
the results tend to be an overapproximation...
julia
>
> Mauro Carvalho Chehab ,Greg Kroah-Hartman
> ,
> linux-media@vger.kernel.org,de...@driverdev.osuosl.org,linux-ker...@vger.kernel.org
>
> > greg k-h
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/CALrZqyOdKmSF10Ba60_00OzzRMnKAt7XwjksmuQfGEKvBY-avg%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
Re: [PATCH v1] staging: media: Remove unused function atomisp_set_stop_timeout()
On Sun, Mar 12, 2017 at 7:24 PM, Greg KH wrote:
> On Fri, Mar 10, 2017 at 07:05:05PM +0530, simran singhal wrote:
>> The function atomisp_set_stop_timeout on being called, simply returns
>> back. The function hasn't been mentioned in the TODO and doesn't have
>> FIXME code around. Hence, atomisp_set_stop_timeout and its calls have been
>> removed.
>>
>> This was done using Coccinelle.
>>
>> @@
>> identifier f;
>> @@
>>
>> void f(...) {
>>
>> -return;
>>
>> }
>>
>> Signed-off-by: simran singhal
>> ---
>> v1:
>>-Change Subject to include name of function
>>-change commit message to include the coccinelle script
>
> You should also cc: the developers doing all of the current work on this
> driver, Alan Cox, to get their comment if this really is something that
> can be removed or not.
>
> thanks,
>
Greg I have cc'd all the developers which script get_maintainer.pl showed:
$ git show HEAD | perl scripts/get_maintainer.pl --separator ,
--nokeywords --nogit --nogit-fallback --norolestats
Mauro Carvalho Chehab ,Greg Kroah-Hartman
,
linux-media@vger.kernel.org,de...@driverdev.osuosl.org,linux-ker...@vger.kernel.org
> greg k-h
Re: [PATCH] staging: atomisp: clean up return logic, remove redunant code
On Sun, 12 Mar 2017, walter harms wrote:
>
>
> Am 11.03.2017 20:32, schrieb Colin King:
> > From: Colin Ian King
> >
> > There is no need to check if ret is non-zero, remove this
> > redundant check and just return the error status from the call
> > to mt9m114_write_reg_array.
> >
> > Detected by CoverityScan, CID#1416577 ("Identical code for
> > different branches")
> >
> > Signed-off-by: Colin Ian King
> > ---
> > drivers/staging/media/atomisp/i2c/mt9m114.c | 6 +-
> > 1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/i2c/mt9m114.c
> > b/drivers/staging/media/atomisp/i2c/mt9m114.c
> > index 8762124..a555aec 100644
> > --- a/drivers/staging/media/atomisp/i2c/mt9m114.c
> > +++ b/drivers/staging/media/atomisp/i2c/mt9m114.c
> > @@ -444,12 +444,8 @@ static int mt9m114_set_suspend(struct v4l2_subdev *sd)
> > static int mt9m114_init_common(struct v4l2_subdev *sd)
> > {
> > struct i2c_client *client = v4l2_get_subdevdata(sd);
> > - int ret;
> >
> > - ret = mt9m114_write_reg_array(client, mt9m114_common, PRE_POLLING);
> > - if (ret)
> > - return ret;
> > - return ret;
> > + return mt9m114_write_reg_array(client, mt9m114_common, PRE_POLLING);
> > }
>
>
> any use for "client" ?
I guess the code would be on two lines in any case. It looks like a nice
decomposition as is.
julia
Re: [PATCH] staging: atomisp: clean up return logic, remove redunant code
Am 11.03.2017 20:32, schrieb Colin King:
> From: Colin Ian King
>
> There is no need to check if ret is non-zero, remove this
> redundant check and just return the error status from the call
> to mt9m114_write_reg_array.
>
> Detected by CoverityScan, CID#1416577 ("Identical code for
> different branches")
>
> Signed-off-by: Colin Ian King
> ---
> drivers/staging/media/atomisp/i2c/mt9m114.c | 6 +-
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/i2c/mt9m114.c
> b/drivers/staging/media/atomisp/i2c/mt9m114.c
> index 8762124..a555aec 100644
> --- a/drivers/staging/media/atomisp/i2c/mt9m114.c
> +++ b/drivers/staging/media/atomisp/i2c/mt9m114.c
> @@ -444,12 +444,8 @@ static int mt9m114_set_suspend(struct v4l2_subdev *sd)
> static int mt9m114_init_common(struct v4l2_subdev *sd)
> {
> struct i2c_client *client = v4l2_get_subdevdata(sd);
> - int ret;
>
> - ret = mt9m114_write_reg_array(client, mt9m114_common, PRE_POLLING);
> - if (ret)
> - return ret;
> - return ret;
> + return mt9m114_write_reg_array(client, mt9m114_common, PRE_POLLING);
> }
any use for "client" ?
re,
wh
>
> static int power_ctrl(struct v4l2_subdev *sd, bool flag)
Re: [PATCH v1 1/7] staging: gc2235: Remove unnecessary typecast of c90 int constant
On Fri, Mar 10, 2017 at 04:20:23AM +0530, simran singhal wrote: > This patch removes unnecessary typecast of c90 int constant. > > WARNING: Unnecessary typecast of c90 int constant > > Signed-off-by: simran singhal > --- > drivers/staging/media/atomisp/i2c/gc2235.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) This series doesn't apply to my tree at all, please rebase and resend. thanks, greg k-h
Re: [PATCH v1] staging: media: Remove unused function atomisp_set_stop_timeout()
On Fri, Mar 10, 2017 at 07:05:05PM +0530, simran singhal wrote:
> The function atomisp_set_stop_timeout on being called, simply returns
> back. The function hasn't been mentioned in the TODO and doesn't have
> FIXME code around. Hence, atomisp_set_stop_timeout and its calls have been
> removed.
>
> This was done using Coccinelle.
>
> @@
> identifier f;
> @@
>
> void f(...) {
>
> -return;
>
> }
>
> Signed-off-by: simran singhal
> ---
> v1:
>-Change Subject to include name of function
>-change commit message to include the coccinelle script
You should also cc: the developers doing all of the current work on this
driver, Alan Cox, to get their comment if this really is something that
can be removed or not.
thanks,
greg k-h
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging
2017-03-09 18:38 GMT+01:00 Laura Abbott : > On 03/09/2017 02:00 AM, Benjamin Gaignard wrote: >> 2017-03-06 17:04 GMT+01:00 Daniel Vetter : >>> On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote: On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote: > No one gave a thing about android in upstream, so Greg KH just dumped it > all into staging/android/. We've discussed ION a bunch of times, recorded > anything we'd like to fix in staging/android/TODO, and Laura's patch > series here addresses a big chunk of that. > This is pretty much the same approach we (gpu folks) used to de-stage the > syncpt stuff. Well, there's also the fact that quite a few people have issues with the design (like Laurent). It seems like a lot of them have either got more comfortable with it over time, or at least not managed to come up with any better ideas in the meantime. >>> >>> See the TODO, it has everything a really big group (look at the patch for >>> the full Cc: list) figured needs to be improved at LPC 2015. We don't just >>> merge stuff because merging stuff is fun :-) >>> >>> Laurent was even in that group ... >>> -Daniel >> >> For me those patches are going in the right direction. >> >> I still have few questions: >> - since alignment management has been remove from ion-core, should it >> be also removed from ioctl structure ? > > Yes, I think I'm going to go with the suggestion to fixup the ABI > so we don't need the compat layer and as part of that I'm also > dropping the align argument. > >> - can you we ride off ion_handle (at least in userland) and only >> export a dma-buf descriptor ? > > Yes, I think this is the right direction given we're breaking > everything anyway. I was debating trying to keep the two but > moving to only dma bufs is probably cleaner. The only reason > I could see for keeping the handles is running out of file > descriptors for dma-bufs but that seems unlikely. >> >> In the future how can we add new heaps ? >> Some platforms have very specific memory allocation >> requirements (just have a look in the number of gem custom allocator in drm) >> Do you plan to add heap type/mask for each ? > > Yes, that was my thinking. My concern is about the policy to adding heaps, will you accept "customs" heap per platforms ? per devices ? or only generic ones ? If you are too strict, we will have lot of out-of-tree heaps and if you accept of of them it will be a nightmare to maintain Another point is how can we put secure rules (like selinux policy) on heaps since all the allocations go to the same device (/dev/ion) ? For example, until now, in Android we have to give the same access rights to all the process that use ION. It will become problem when we will add secure heaps because we won't be able to distinguish secure processes to standard ones or set specific policy per heaps. Maybe I'm wrong here but I have never see selinux policy checking an ioctl field but if that exist it could be a solution. > >> >> Benjamin >> > > Thanks, > Laura >
[PATCH v2 12/23] MAINTAINERS: Add file patterns for media device tree bindings
Submitters of device tree binding documentation may forget to CC the subsystem maintainer if this is missing. Signed-off-by: Geert Uytterhoeven Reviewed-by: Javier Martinez Canillas Cc: Mauro Carvalho Chehab Cc: linux-media@vger.kernel.org --- Please apply this patch directly if you want to be involved in device tree binding documentation for your subsystem. v2: - Add Reviewed-by. Impact on next-20170310: -Rob Herring (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,commit_signer:24/39=62%) +Mauro Carvalho Chehab (maintainer:MEDIA INPUT INFRASTRUCTURE (V4L/DVB)) +Rob Herring (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Mark Rutland (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) -Mauro Carvalho Chehab (commit_signer:34/39=87%) -Hans Verkuil (commit_signer:13/39=33%) -Laurent Pinchart (commit_signer:11/39=28%,authored:3/39=8%) -Marek Szyprowski (commit_signer:4/39=10%,authored:4/39=10%) -Kieran Bingham (authored:3/39=8%) -Martin Blumenstingl (authored:2/39=5%) -Eric Engestrom (authored:2/39=5%) +linux-media@vger.kernel.org (open list:MEDIA INPUT INFRASTRUCTURE (V4L/DVB)) devicet...@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) linux-ker...@vger.kernel.org (open list) --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 2692055d221e2bb2..3e108e31636d4db2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8085,6 +8085,7 @@ W:https://linuxtv.org Q: http://patchwork.kernel.org/project/linux-media/list/ T: git git://linuxtv.org/media_tree.git S: Maintained +F: Documentation/devicetree/bindings/media/ F: Documentation/media/ F: drivers/media/ F: drivers/staging/media/ -- 2.7.4
Re: [PATCHv5 09/16] ov2640: fix colorspace handling
On Sat, 11 Mar 2017, Hans Verkuil wrote:
> From: Hans Verkuil
>
> The colorspace is independent of whether YUV or RGB is sent to the SoC.
> Fix this.
>
> Signed-off-by: Hans Verkuil
I'm not sure why the first hunk is needed and how it is related :-) But it
doesn't break anything. I understand, this patch should better go in as a
part of a series, so
Acked-by: Guennadi Liakhovetski
Thanks
Guennadi
> ---
> drivers/media/i2c/soc_camera/ov2640.c | 23 +++
> 1 file changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
> b/drivers/media/i2c/soc_camera/ov2640.c
> index 56de18263359..b9a0069f5b33 100644
> --- a/drivers/media/i2c/soc_camera/ov2640.c
> +++ b/drivers/media/i2c/soc_camera/ov2640.c
> @@ -794,10 +794,11 @@ static int ov2640_set_params(struct i2c_client *client,
> u32 *width, u32 *height,
> dev_dbg(&client->dev, "%s: Selected cfmt YUYV (YUV422)",
> __func__);
> selected_cfmt_regs = ov2640_yuyv_regs;
> break;
> - default:
> case MEDIA_BUS_FMT_UYVY8_2X8:
> + default:
> dev_dbg(&client->dev, "%s: Selected cfmt UYVY", __func__);
> selected_cfmt_regs = ov2640_uyvy_regs;
> + break;
> }
>
> /* reset hardware */
> @@ -865,17 +866,7 @@ static int ov2640_get_fmt(struct v4l2_subdev *sd,
> mf->width = priv->win->width;
> mf->height = priv->win->height;
> mf->code= priv->cfmt_code;
> -
> - switch (mf->code) {
> - case MEDIA_BUS_FMT_RGB565_2X8_BE:
> - case MEDIA_BUS_FMT_RGB565_2X8_LE:
> - mf->colorspace = V4L2_COLORSPACE_SRGB;
> - break;
> - default:
> - case MEDIA_BUS_FMT_YUYV8_2X8:
> - case MEDIA_BUS_FMT_UYVY8_2X8:
> - mf->colorspace = V4L2_COLORSPACE_JPEG;
> - }
> + mf->colorspace = V4L2_COLORSPACE_SRGB;
> mf->field = V4L2_FIELD_NONE;
>
> return 0;
> @@ -897,17 +888,17 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
> ov2640_select_win(&mf->width, &mf->height);
>
> mf->field = V4L2_FIELD_NONE;
> + mf->colorspace = V4L2_COLORSPACE_SRGB;
>
> switch (mf->code) {
> case MEDIA_BUS_FMT_RGB565_2X8_BE:
> case MEDIA_BUS_FMT_RGB565_2X8_LE:
> - mf->colorspace = V4L2_COLORSPACE_SRGB;
> + case MEDIA_BUS_FMT_YUYV8_2X8:
> + case MEDIA_BUS_FMT_UYVY8_2X8:
> break;
> default:
> mf->code = MEDIA_BUS_FMT_UYVY8_2X8;
> - case MEDIA_BUS_FMT_YUYV8_2X8:
> - case MEDIA_BUS_FMT_UYVY8_2X8:
> - mf->colorspace = V4L2_COLORSPACE_JPEG;
> + break;
> }
>
> if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> --
> 2.11.0
>
Re: [PATCH] v4l: soc-camera: Remove videobuf1 support
Hi Laurent,
Thanks for the patch. I just checked in the current media/master, there
are still 2 users of vb1: sh_mobile_ceu_camera.c and atmel-isi.c. I
understand, that they are about to be removed either completely or out of
soc-camera, maybe patches for that have already beed submitted, but they
haven't been committed yet. Shall we wait until then with this patch?
Would be easier to handle dependencies, there isn't any hurry with it,
right?
Thanks
Guennaddi
On Wed, 8 Mar 2017, Laurent Pinchart wrote:
> All remaining soc-camera drivers use videobuf2, drop support for
> videobuf1.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/media/platform/soc_camera/soc_camera.c | 103
> +
> include/media/soc_camera.h | 14 +---
> 2 files changed, 20 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c
> b/drivers/media/platform/soc_camera/soc_camera.c
> index edd1c1de4e33..3c9421f4d8e3 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -37,18 +37,12 @@
> #include
> #include
> #include
> -#include
> #include
>
> /* Default to VGA resolution */
> #define DEFAULT_WIDTH640
> #define DEFAULT_HEIGHT 480
>
> -#define is_streaming(ici, icd) \
> - (((ici)->ops->init_videobuf) ? \
> - (icd)->vb_vidq.streaming : \
> - vb2_is_streaming(&(icd)->vb2_vidq))
> -
> #define MAP_MAX_NUM 32
> static DECLARE_BITMAP(device_map, MAP_MAX_NUM);
> static LIST_HEAD(hosts);
> @@ -367,23 +361,13 @@ static int soc_camera_reqbufs(struct file *file, void
> *priv,
> {
> int ret;
> struct soc_camera_device *icd = file->private_data;
> - struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>
> WARN_ON(priv != file->private_data);
>
> if (icd->streamer && icd->streamer != file)
> return -EBUSY;
>
> - if (ici->ops->init_videobuf) {
> - ret = videobuf_reqbufs(&icd->vb_vidq, p);
> - if (ret < 0)
> - return ret;
> -
> - ret = ici->ops->reqbufs(icd, p);
> - } else {
> - ret = vb2_reqbufs(&icd->vb2_vidq, p);
> - }
> -
> + ret = vb2_reqbufs(&icd->vb2_vidq, p);
> if (!ret)
> icd->streamer = p->count ? file : NULL;
> return ret;
> @@ -393,61 +377,44 @@ static int soc_camera_querybuf(struct file *file, void
> *priv,
> struct v4l2_buffer *p)
> {
> struct soc_camera_device *icd = file->private_data;
> - struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>
> WARN_ON(priv != file->private_data);
>
> - if (ici->ops->init_videobuf)
> - return videobuf_querybuf(&icd->vb_vidq, p);
> - else
> - return vb2_querybuf(&icd->vb2_vidq, p);
> + return vb2_querybuf(&icd->vb2_vidq, p);
> }
>
> static int soc_camera_qbuf(struct file *file, void *priv,
> struct v4l2_buffer *p)
> {
> struct soc_camera_device *icd = file->private_data;
> - struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>
> WARN_ON(priv != file->private_data);
>
> if (icd->streamer != file)
> return -EBUSY;
>
> - if (ici->ops->init_videobuf)
> - return videobuf_qbuf(&icd->vb_vidq, p);
> - else
> - return vb2_qbuf(&icd->vb2_vidq, p);
> + return vb2_qbuf(&icd->vb2_vidq, p);
> }
>
> static int soc_camera_dqbuf(struct file *file, void *priv,
> struct v4l2_buffer *p)
> {
> struct soc_camera_device *icd = file->private_data;
> - struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>
> WARN_ON(priv != file->private_data);
>
> if (icd->streamer != file)
> return -EBUSY;
>
> - if (ici->ops->init_videobuf)
> - return videobuf_dqbuf(&icd->vb_vidq, p, file->f_flags &
> O_NONBLOCK);
> - else
> - return vb2_dqbuf(&icd->vb2_vidq, p, file->f_flags & O_NONBLOCK);
> + return vb2_dqbuf(&icd->vb2_vidq, p, file->f_flags & O_NONBLOCK);
> }
>
> static int soc_camera_create_bufs(struct file *file, void *priv,
> struct v4l2_create_buffers *create)
> {
> struct soc_camera_device *icd = file->private_data;
> - struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> int ret;
>
> - /* videobuf2 only */
> - if (ici->ops->init_videobuf)
> - return -ENOTTY;
> -
> if (icd->streamer && icd->streamer != file)
> return -EBUSY;
>
> @@ -461,24 +428,14 @@ static int soc_camera_prepare_buf(struct file *file,
> void *priv,
> struct v4l2_buffer *b)
> {
> struct soc_camera_device *icd = file->private_data;
> - struct soc_camera_ho
