Re: [PATCH 12/13] [media] omap3isp: Support for deferred probing when requesting DMA channel
Hi Laurent,
On 11/09/2015 09:50 PM, Laurent Pinchart wrote:
> Hi Peter,
>
> Thank you for the patch.
>
> What happened to this patch series ? It looks like
> dma_request_slave_channel_compat_reason() isn't in mainline, so I can't apply
> this patch.
>
> I'll mark this patch as deferred in patchwork, please resubmit it if you
> resubmit the series
The original series - containing this patch - generated a bit of discussion
and it seams that I will need to do bigger change in the dmaengine API
compared to this.
I think this patch can be dropped as the dmaengine changes will not go in as
they were.
(and by the look of it the issue you're trying to fix
> still exists, so it would be nice if you could get it eventually fixed).
Yes, the issue still valid for the OMAP/DaVinci driver the series was touching.
I will try to send a new series in the coming weeks.
Thanks,
Péter
>
> On Tuesday 26 May 2015 16:26:07 Peter Ujfalusi wrote:
>> Switch to use ma_request_slave_channel_compat_reason() to request the DMA
>> channel. Only fall back to pio mode if the error code returned is not
>> -EPROBE_DEFER, otherwise return from the probe with the -EPROBE_DEFER.
>>
>> Signed-off-by: Peter Ujfalusi
>> CC: Laurent Pinchart
>> CC: Mauro Carvalho Chehab
>> ---
>> drivers/media/platform/omap3isp/isphist.c | 12 +---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/isphist.c
>> b/drivers/media/platform/omap3isp/isphist.c index
>> 7138b043a4aa..e690ca13af0e 100644
>> --- a/drivers/media/platform/omap3isp/isphist.c
>> +++ b/drivers/media/platform/omap3isp/isphist.c
>> @@ -499,14 +499,20 @@ int omap3isp_hist_init(struct isp_device *isp)
>> if (res)
>> sig = res->start;
>>
>> -hist->dma_ch = dma_request_slave_channel_compat(mask,
>> +hist->dma_ch = dma_request_slave_channel_compat_reason(mask,
>> omap_dma_filter_fn, &sig, isp->dev, "hist");
>> -if (!hist->dma_ch)
>> +if (IS_ERR(hist->dma_ch)) {
>> +ret = PTR_ERR(hist->dma_ch);
>> +if (ret == -EPROBE_DEFER)
>> +return ret;
>> +
>> +hist->dma_ch = NULL;
>> dev_warn(isp->dev,
>> "hist: DMA channel request failed, using
>> PIO\n");
>> -else
>> +} else {
>> dev_dbg(isp->dev, "hist: using DMA channel %s\n",
>> dma_chan_name(hist->dma_ch));
>> +}
>> }
>>
>> hist->ops = &hist_ops;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/19] v4l: omap3isp: fix handling platform_get_irq result
On 11/09/2015 09:16 PM, Laurent Pinchart wrote:
> Hi Andrzej,
>
> Thank you for the patch.
>
> On Thursday 24 September 2015 16:00:12 Andrzej Hajda wrote:
>> The function can return negative value.
>>
>> The problem has been detected using proposed semantic patch
>> scripts/coccinelle/tests/assign_signed_to_unsigned.cocci [1].
>>
>> [1]: http://permalink.gmane.org/gmane.linux.kernel/2046107
>>
>> Signed-off-by: Andrzej Hajda
>> ---
>> Hi,
>>
>> To avoid problems with too many mail recipients I have sent whole
>> patchset only to LKML. Anyway patches have no dependencies.
>>
>> Regards
>> Andrzej
>> ---
>> drivers/media/platform/omap3isp/isp.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/isp.c
>> b/drivers/media/platform/omap3isp/isp.c index 56e683b..df9d2c2 100644
>> --- a/drivers/media/platform/omap3isp/isp.c
>> +++ b/drivers/media/platform/omap3isp/isp.c
>> @@ -2442,12 +2442,13 @@ static int isp_probe(struct platform_device *pdev)
>> }
>>
>> /* Interrupt */
>> -isp->irq_num = platform_get_irq(pdev, 0);
>> -if (isp->irq_num <= 0) {
>> +ret = platform_get_irq(pdev, 0);
>> +if (ret <= 0) {
> Looking at platform_get_irq() all error values are negative. You could just
> test for ret < 0 here, and remove the ret = -ENODEV assignment below to keep
> the error code returned by platform_get_irq().
>
> If you're fine with that modification there's no need to resubmit, just let
> me
> know and I'll fix it when applying it to my tree.
I left it as before, as it was not related to the patch. Additionally I have
lurked little bit inside platform_get_irq and it looks little bit scary to me:
platform_get_irq returns value of of_irq_get if ret >= 0,
of_irq_get calls of_irq_parse_one which can return 0,
in such case irq_create_of_mapping value is returned which is unsigned int
and evaluates to 0 in case of failures.
I am not sure if above scenario can ever occur, but the code looks so messy to
me,
that I gave up :)
Anyway if you are sure about your change I am OK with it also :)
Regards
Andrzej
>
>> dev_err(isp->dev, "No IRQ resource\n");
>> ret = -ENODEV;
>> goto error_iommu;
>> }
>> +isp->irq_num = ret;
>>
>> if (devm_request_irq(isp->dev, isp->irq_num, isp_isr, IRQF_SHARED,
>> "OMAP3 ISP", isp)) {
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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: Tue Nov 10 04:00:19 CET 2015 git branch: test git hash: 79f5b6ae960d380c829fb67d5dadcd1d025d2775 gcc version:i686-linux-gcc (GCC) 5.1.0 sparse version: v0.5.0 smatch version: host hardware: x86_64 host os:4.2.0-164 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-exynos: OK linux-git-arm-mx: OK linux-git-arm-omap: OK linux-git-arm-omap1: 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.32.27-i686: OK linux-2.6.33.7-i686: OK linux-2.6.34.7-i686: OK linux-2.6.35.9-i686: OK linux-2.6.36.4-i686: OK linux-2.6.37.6-i686: OK linux-2.6.38.8-i686: OK linux-2.6.39.4-i686: OK linux-3.0.60-i686: OK linux-3.1.10-i686: OK linux-3.2.37-i686: OK linux-3.3.8-i686: OK linux-3.4.27-i686: OK linux-3.5.7-i686: OK linux-3.6.11-i686: OK linux-3.7.4-i686: OK linux-3.8-i686: OK linux-3.9.2-i686: OK linux-3.10.1-i686: OK linux-3.11.1-i686: OK linux-3.12.23-i686: OK linux-3.13.11-i686: OK linux-3.14.9-i686: OK linux-3.15.2-i686: OK linux-3.16.7-i686: OK linux-3.17.8-i686: OK linux-3.18.7-i686: OK linux-3.19-i686: OK linux-4.0-i686: OK linux-4.1.1-i686: OK linux-4.2-i686: OK linux-4.3-i686: OK linux-2.6.32.27-x86_64: OK linux-2.6.33.7-x86_64: OK linux-2.6.34.7-x86_64: OK linux-2.6.35.9-x86_64: OK linux-2.6.36.4-x86_64: OK linux-2.6.37.6-x86_64: OK linux-2.6.38.8-x86_64: OK linux-2.6.39.4-x86_64: OK linux-3.0.60-x86_64: OK linux-3.1.10-x86_64: OK linux-3.2.37-x86_64: OK linux-3.3.8-x86_64: OK linux-3.4.27-x86_64: OK linux-3.5.7-x86_64: OK linux-3.6.11-x86_64: OK linux-3.7.4-x86_64: OK linux-3.8-x86_64: OK linux-3.9.2-x86_64: OK linux-3.10.1-x86_64: OK linux-3.11.1-x86_64: OK linux-3.12.23-x86_64: OK linux-3.13.11-x86_64: OK linux-3.14.9-x86_64: OK linux-3.15.2-x86_64: OK linux-3.16.7-x86_64: OK linux-3.17.8-x86_64: OK linux-3.18.7-x86_64: OK linux-3.19-x86_64: OK linux-4.0-x86_64: OK linux-4.1.1-x86_64: OK linux-4.2-x86_64: OK linux-4.3-x86_64: ERRORS apps: OK spec-git: OK sparse: ERRORS smatch: OK Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] v4l: Clarify RGB666 pixel format definition
Hi Sylwester, Reviving a "slightly" old discussion. On Wednesday 29 October 2014 13:45:46 Laurent Pinchart wrote: > On Tuesday 09 September 2014 16:45:17 Sylwester Nawrocki wrote: > > On 09/09/14 15:18, Laurent Pinchart wrote: > >> On Tuesday 22 July 2014 00:44:34 Hans Verkuil wrote: > >>> On 07/22/2014 12:30 AM, Laurent Pinchart wrote: > On Monday 21 July 2014 23:43:16 Hans Verkuil wrote: > > On 07/21/2014 10:39 PM, Laurent Pinchart wrote: > >> The RGB666 pixel format doesn't include an alpha channel. Document > >> it as such. > >> > >> Signed-off-by: Laurent Pinchart > >> --- > >> > >> .../DocBook/media/v4l/pixfmt-packed-rgb.xml | 20 > >> --- > >> > >> 1 file changed, 6 insertions(+), 14 deletions(-) > >> > >> diff --git a/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml > >> b/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml index > >> 32feac9..c47692a 100644 > >> --- a/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml > >> +++ b/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml > >> @@ -330,20 +330,12 @@ colorspace > >> V4L2_COLORSPACE_SRGB. > >> > >> > >>r1 > >>r0 > >> > >> - > >> - > >> - > >> - > >> - > >> - > >> - > >> - > >> - > >> - > >> - > >> - > >> - > >> - > >> + - > >> + - > >> + - > >> + - > >> + - > >> + - > > > > Just to clarify: BGR666 is a three byte format, not a four byte > > format? [snip] > > My apologies, I didn't notice this earlier. > > > > In case of S5P/Exynos FIMC the format is: > > > > Byte 0 Byte 1 Byte 2 Byte 3 > > > > BBGG RR-- > > > > i.e. 4 byte per pixel, with 14-bit padding (don't care bits). > > > > As far as S3C6410 CAMIF is concerned it's hard to say. I primarily > > developed the s3c-camif driver for S3C2440 SoC, which doesn't support > > BGR666 format. I merged some patches from others adding s3c6410 support, > > before sending upstream. > > > > Nevertheless, looking at the S3C CAMIF datasheet the RGB666 format seems > > identical with the FIMC one. See [1], chapter "20.7.4 MEMORY STORING > > METHOD". This would make sense, since the S5P/Exynos FIMC is basically > > a significantly evolved S3C CAMIF AFAICT. > > Looking at the figure, I would understand RGB666 as follows > > Bits 31 24|23 16|158|7 0 > | -- R5 R4|R3 R2 R1 R0 G5 G4 G3 G2|G1 G0 B5 B4 B3 B2 B1 B0 > > Assuming little endian memory format, that would thus be > > Byte 0 Byte 1 Byte 2 Byte 3 > > GGBB --RR > > If the memory format was big endian it would instead be > > --RR GGBB > > > [1] http://www.arm9board.net/download/OK6410/docs/S3C6410X.pdf Did I understand something incorrectly ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] media: omap4iss: csi2: Fix IRQ handling when stopping module
When stopping the CSI2 receiver the s_stream handler will wait for the
IRQ handler to notice the stop request. The receiver, automatically
disabled by the hardware after each frame, is then not reenabled by the
IRQ handler as it returns immediately.
As the IRQ handler check is performed before handling the context IRQ,
the context IRQ source isn't cleared, and the CSI2 IRQ is then fired
again immediately. The IRQ handler then fails to notice that the module
is being stopped, processes the IRQ normally and reenables the CSI2
hardware.
The problem goes unnoticed at stream stop time, but depending on the IRQ
and s_stream scheduling timings, the CSI2 receiver can end up being
hanged and will not produce any interrupt the next time it gets enabled,
despite being soft-reset then.
Fix this by checking for module stop after clearing the context IRQ
source.
Signed-off-by: Laurent Pinchart
---
drivers/staging/media/omap4iss/iss_csi2.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/media/omap4iss/iss_csi2.c
b/drivers/staging/media/omap4iss/iss_csi2.c
index bc83f8246101..b6b5c12b9c9d 100644
--- a/drivers/staging/media/omap4iss/iss_csi2.c
+++ b/drivers/staging/media/omap4iss/iss_csi2.c
@@ -674,6 +674,9 @@ static void csi2_isr_ctx(struct iss_csi2_device *csi2,
status = iss_reg_read(csi2->iss, csi2->regs1, CSI2_CTX_IRQSTATUS(n));
iss_reg_write(csi2->iss, csi2->regs1, CSI2_CTX_IRQSTATUS(n), status);
+ if (omap4iss_module_sync_is_stopping(&csi2->wait, &csi2->stopping))
+ return;
+
/* Propagate frame number */
if (status & CSI2_CTX_IRQ_FS) {
struct iss_pipeline *pipe =
@@ -776,9 +779,6 @@ void omap4iss_csi2_isr(struct iss_csi2_device *csi2)
pipe->error = true;
}
- if (omap4iss_module_sync_is_stopping(&csi2->wait, &csi2->stopping))
- return;
-
/* Successful cases */
if (csi2_irqstatus & CSI2_IRQ_CONTEXT0)
csi2_isr_ctx(csi2, &csi2->contexts[0]);
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] media: omap4iss: Make module stop timeout print a warning message
Module stop timeouts are serious enough that they deserve a proper
warning message, not a debug message that will go unnoticed.
Signed-off-by: Laurent Pinchart
---
drivers/staging/media/omap4iss/iss.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/omap4iss/iss.c
b/drivers/staging/media/omap4iss/iss.c
index 0b03cb7c59d5..b7e82eed8aad 100644
--- a/drivers/staging/media/omap4iss/iss.c
+++ b/drivers/staging/media/omap4iss/iss.c
@@ -601,8 +601,8 @@ static int iss_pipeline_disable(struct iss_pipeline *pipe,
subdev = media_entity_to_v4l2_subdev(entity);
ret = v4l2_subdev_call(subdev, video, s_stream, 0);
if (ret < 0) {
- dev_dbg(iss->dev, "%s: module stop timeout.\n",
- subdev->name);
+ dev_warn(iss->dev, "%s: module stop timeout.\n",
+subdev->name);
/* If the entity failed to stopped, assume it has
* crashed. Mark it as such, the ISS will be reset when
* applications will release it.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] omap4iss: Fix overlapping luma/chroma planes
From: Nate Weibley
The chroma data base address for NV12 formatted data should begin offset
rows*bytes_per_row from the base address for luminance data. We were OBO
causing a stripe of green pixels at the bottom of the frame.
Signed-off-by: Nate Weibley
Signed-off-by: Laurent Pinchart
---
drivers/staging/media/omap4iss/iss_resizer.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Nate, I've split your original patch
(https://patchwork.linuxtv.org/patch/21816/) in two as the second part still
needs investigation. I'll push this patch to mainline for v4.5.
diff --git a/drivers/staging/media/omap4iss/iss_resizer.c
b/drivers/staging/media/omap4iss/iss_resizer.c
index 5030cf3cd34c..b7f9f622385c 100644
--- a/drivers/staging/media/omap4iss/iss_resizer.c
+++ b/drivers/staging/media/omap4iss/iss_resizer.c
@@ -158,8 +158,8 @@ static void resizer_set_outaddr(struct iss_resizer_device
*resizer, u32 addr)
/* Program UV buffer address... Hardcoded to be contiguous! */
if ((informat->code == MEDIA_BUS_FMT_UYVY8_1X16) &&
(outformat->code == MEDIA_BUS_FMT_YUYV8_1_5X8)) {
- u32 c_addr = addr + (resizer->video_out.bpl_value *
-(outformat->height - 1));
+ u32 c_addr = addr + resizer->video_out.bpl_value
+ * outformat->height;
/* Ensure Y_BAD_L[6:0] = C_BAD_L[6:0]*/
if ((c_addr ^ addr) & 0x7f) {
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL FOR v4.5] Davinci staging fixes
Hi Mauro, I've collected the pending Davinci staging fixes from patchwork and prepared a branch for you. Prabhakar, is that fine with you ? Do you still maintain the driver ? If so, do you expect patches to be picked up when you ack them, or can you collect them in a branch somewhere and send a pull request ? The following changes since commit 79f5b6ae960d380c829fb67d5dadcd1d025d2775: [media] c8sectpfe: Remove select on CONFIG_FW_LOADER_USER_HELPER_FALLBACK (2015-10-20 16:02:41 -0200) are available in the git repository at: git://linuxtv.org/pinchartl/media.git davinci for you to fetch changes up to b542f513822fd11460ef781742d6c0446b40eeb8: staging: media: davinci_vpfe: fix ipipe_mode type (2015-11-09 23:07:53 +0200) Andrzej Hajda (1): staging: media: davinci_vpfe: fix ipipe_mode type Arnd Bergmann (1): staging/davinci/vpfe/dm365: add missing dependencies Julia Lawall (1): drivers/staging/media/davinci_vpfe/vpfe_mc_capture.c: use correct structure type name in sizeof Junsu Shin (1): staging: media: davinci_vpfe: Fix over 80 characters coding style issue Nicholas Mc Guire (1): staging: media: davinci_vpfe: drop condition with no effect drivers/staging/media/davinci_vpfe/Kconfig | 2 ++ drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 5 +++-- drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c | 2 +- drivers/staging/media/davinci_vpfe/dm365_resizer.c | 7 +-- drivers/staging/media/davinci_vpfe/vpfe_mc_capture.c | 2 +- 5 files changed, 8 insertions(+), 10 deletions(-) -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] [media] uvcvideo: Remain runtime-suspended at sleeps
Hi Tomeu, On Monday 20 April 2015 09:11:36 Tomeu Vizoso wrote: > On 17 April 2015 at 19:32, Alan Stern wrote: > > On Fri, 17 Apr 2015, Tomeu Vizoso wrote: > >> When the system goes to sleep and afterwards resumes, a significant > >> amount of time is spent suspending and resuming devices that were > >> already runtime-suspended. > >> > >> By setting the power.force_direct_complete flag, the PM core will ignore > >> the state of descendant devices and the device will be let in > >> runtime-suspend. > >> > >> Signed-off-by: Tomeu Vizoso > >> --- > >> > >> drivers/media/usb/uvc/uvc_driver.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/media/usb/uvc/uvc_driver.c > >> b/drivers/media/usb/uvc/uvc_driver.c index 5970dd6..ae75a70 100644 > >> --- a/drivers/media/usb/uvc/uvc_driver.c > >> +++ b/drivers/media/usb/uvc/uvc_driver.c > >> @@ -1945,6 +1945,8 @@ static int uvc_probe(struct usb_interface *intf, > >> > >> "supported.\n", ret); > >> > >> } > >> > >> + intf->dev.parent->power.force_direct_complete = true; > > > > This seems wrong. The uvc driver is bound to intf, not to intf's > > parent. So it would be okay for the driver to set > > intf->dev.power.force_direct_complete, but it's wrong to set > > intf->dev.parent->power.force_direct_complete. > > Agreed. Do you plan to resubmit this patch series with the above fix ? I know you've had a hard time trying to find an approach that could get accepted, but please rest assured that your work on the uvcvideo driver is appreciated. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 25/38] staging: media: davinci_vpfe: fix ipipe_mode type
Hi Andrzej, Thank you for the patch. On Monday 21 September 2015 15:33:57 Andrzej Hajda wrote: > The variable can take negative values. > > The problem has been detected using proposed semantic patch > scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1]. > > [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576 > > Signed-off-by: Andrzej Hajda Acked-by: Laurent Pinchart and applied to my tree. > --- > drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c > b/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c index > 2a3a56b..b1d5e23 100644 > --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c > +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c > @@ -254,7 +254,7 @@ int config_ipipe_hw(struct vpfe_ipipe_device *ipipe) > void __iomem *ipipe_base = ipipe->base_addr; > struct v4l2_mbus_framefmt *outformat; > u32 color_pat; > - u32 ipipe_mode; > + int ipipe_mode; > u32 data_path; > > /* enable clock to IPIPE */ -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Y2038] [PATCH v2 9/9] [media] omap3isp: support 64-bit version of omap3isp_stat_data
Hi Arnd, On Monday 09 November 2015 21:30:41 Arnd Bergmann wrote: > On Monday 09 November 2015 22:09:26 Laurent Pinchart wrote: > > On Thursday 17 September 2015 23:19:40 Arnd Bergmann wrote: > > > C libraries with 64-bit time_t use an incompatible format for > > > struct omap3isp_stat_data. This changes the kernel code to > > > support either version, by moving over the normal handling > > > to the 64-bit variant, and adding compatiblity code to handle > > > the old binary format with the existing ioctl command code. > > > > > > Fortunately, the command code includes the size of the structure, > > > so the difference gets handled automatically. > > > > We plan to design a new interface to handle statistics in V4L2. That API > > should support proper 64-bit timestamps out of the box, and will be > > implemented by the OMAP3 ISP driver. Userspace should then move to it. I > > wonder if it's worth it to fix the existing VIDIOC_OMAP3ISP_STAT_REQ ioctl > > given that I expect it to have a handful of users at most. > > We still need to do something to the driver. The alternative would > be to make the existing ioctl command optional at kernel compile-time > so we can still build the driver once we remove the 'struct timeval' > definition. That patch would add slightly less complexity here > but also lose functionality. > > As my patch here depends on the struct v4l2_timeval I introduced in > an earlier patch of the series, we will have to change it anyways, > but I'd prefer to keep the basic idea. Let's get back to this one > after the v4l_buffer replacement work is done. Fine with me. I'll mark the patch as "Obsoleted" in patchwork in the meantime. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] /media/davinci_vpfe/dm365_resizer.c:Bug containing more than 80 characters in a line is fixed.
Hi Ravinder,
Thank you for the patch.
I'm afraid the patch doesn't apply on top of the mainline kernel.
On Wednesday 19 August 2015 21:14:56 Ravinder Atla wrote:
> Signed-off-by: Ravinder Atla
> ---
> drivers/staging/media/davinci_vpfe/dm365_resizer.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/media/davinci_vpfe/dm365_resizer.c
> b/drivers/staging/media/davinci_vpfe/dm365_resizer.c index 6218230..273aea3
> 100644
> --- a/drivers/staging/media/davinci_vpfe/dm365_resizer.c
> +++ b/drivers/staging/media/davinci_vpfe/dm365_resizer.c
> @@ -1393,8 +1393,8 @@ resizer_try_format(struct v4l2_subdev *sd, struct
> v4l2_subdev_pad_config *cfg, * return -EINVAL or zero on success
> */
> static int resizer_set_format(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_format *
> - fmt)
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *fmt)
> {
> struct vpfe_resizer_device *resizer = v4l2_get_subdevdata(sd);
> struct v4l2_mbus_framefmt *format;
> @@ -1454,8 +1454,8 @@ static int resizer_set_format(struct v4l2_subdev *sd,
> * return -EINVAL or zero on success
> */
> static int resizer_get_format(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_format *
> - fmt)
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *fmt)
> {
> struct v4l2_mbus_framefmt *format;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" 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] Staging: media: davinci_vpfe: Fix over 80 characters coding style issue
Hello Junsu,
Thank you for the patch.
On Monday 10 August 2015 16:40:59 Junsu Shin wrote:
> This is a patch to the dm365_ipipe.c that fixes over 80 characters warning
> detected.
It's a bit ironic to submit a patch fixing a 80 characters limit issue and
having a commit message that is larger than 72 characters per line :-)
Anyway, I've wrapped the commit message to 72 columns and applied the patch to
my tree.
> Signed-off-by: Junsu Shin
> ---
> drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c index 1bbb90c..a474adf
> 100644
> --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> @@ -1536,8 +1536,9 @@ ipipe_get_format(struct v4l2_subdev *sd, struct
> v4l2_subdev_pad_config *cfg, * @fse: pointer to v4l2_subdev_frame_size_enum
> structure.
> */
> static int
> -ipipe_enum_frame_size(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config
> *cfg, - struct v4l2_subdev_frame_size_enum *fse)
> +ipipe_enum_frame_size(struct v4l2_subdev *sd,
> +struct v4l2_subdev_pad_config *cfg,
> +struct v4l2_subdev_frame_size_enum *fse)
> {
> struct vpfe_ipipe_device *ipipe = v4l2_get_subdevdata(sd);
> struct v4l2_mbus_framefmt format;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] drivers/staging/media/davinci_vpfe/vpfe_mc_capture.c: use correct structure type name in sizeof
Hi Julia,
Thank you for the patch.
On Tuesday 29 July 2014 17:16:43 Julia Lawall wrote:
> From: Julia Lawall
>
> Correct typo in the name of the type given to sizeof. Because it is the
> size of a pointer that is wanted, the typo has no impact on compilation or
> execution.
>
> This problem was found using Coccinelle (http://coccinelle.lip6.fr/). The
> semantic patch used can be found in message 0 of this patch series.
>
> Signed-off-by: Julia Lawall
>
> ---
> drivers/staging/media/davinci_vpfe/vpfe_mc_capture.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/davinci_vpfe/vpfe_mc_capture.c
> b/drivers/staging/media/davinci_vpfe/vpfe_mc_capture.c index
> cda8388..255590f 100644
> --- a/drivers/staging/media/davinci_vpfe/vpfe_mc_capture.c
> +++ b/drivers/staging/media/davinci_vpfe/vpfe_mc_capture.c
> @@ -227,7 +227,7 @@ static int vpfe_enable_clock(struct vpfe_device
> *vpfe_dev) return 0;
>
> vpfe_dev->clks = kzalloc(vpfe_cfg->num_clocks *
> -sizeof(struct clock *), GFP_KERNEL);
> +sizeof(struct clk *), GFP_KERNEL);
I'd use sizeof(*vpfe_dev->clks) to avoid such issues.
Apart from that,
Acked-by: Laurent Pinchart
I've applied the patch to my tree with the above change, there's no need to
resubmit if you agree with the proposal.
> if (vpfe_dev->clks == NULL) {
> v4l2_err(vpfe_dev->pdev->driver, "Memory allocation failed\n");
> return -ENOMEM;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Y2038] [PATCH v2 9/9] [media] omap3isp: support 64-bit version of omap3isp_stat_data
On Monday 09 November 2015 22:09:26 Laurent Pinchart wrote: > Hi Arnd, > > Thank you for the patch. > > On Thursday 17 September 2015 23:19:40 Arnd Bergmann wrote: > > C libraries with 64-bit time_t use an incompatible format for > > struct omap3isp_stat_data. This changes the kernel code to > > support either version, by moving over the normal handling > > to the 64-bit variant, and adding compatiblity code to handle > > the old binary format with the existing ioctl command code. > > > > Fortunately, the command code includes the size of the structure, > > so the difference gets handled automatically. > > We plan to design a new interface to handle statistics in V4L2. That API > should support proper 64-bit timestamps out of the box, and will be > implemented by the OMAP3 ISP driver. Userspace should then move to it. I > wonder if it's worth it to fix the existing VIDIOC_OMAP3ISP_STAT_REQ ioctl > given that I expect it to have a handful of users at most. We still need to do something to the driver. The alternative would be to make the existing ioctl command optional at kernel compile-time so we can still build the driver once we remove the 'struct timeval' definition. That patch would add slightly less complexity here but also lose functionality. As my patch here depends on the struct v4l2_timeval I introduced in an earlier patch of the series, we will have to change it anyways, but I'd prefer to keep the basic idea. Let's get back to this one after the v4l_buffer replacement work is done. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/19] v4l: omap3isp: fix handling platform_get_irq result
Hi Andrzej,
Thank you for the patch.
On Thursday 24 September 2015 16:00:12 Andrzej Hajda wrote:
> The function can return negative value.
>
> The problem has been detected using proposed semantic patch
> scripts/coccinelle/tests/assign_signed_to_unsigned.cocci [1].
>
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2046107
>
> Signed-off-by: Andrzej Hajda
> ---
> Hi,
>
> To avoid problems with too many mail recipients I have sent whole
> patchset only to LKML. Anyway patches have no dependencies.
>
> Regards
> Andrzej
> ---
> drivers/media/platform/omap3isp/isp.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 56e683b..df9d2c2 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2442,12 +2442,13 @@ static int isp_probe(struct platform_device *pdev)
> }
>
> /* Interrupt */
> - isp->irq_num = platform_get_irq(pdev, 0);
> - if (isp->irq_num <= 0) {
> + ret = platform_get_irq(pdev, 0);
> + if (ret <= 0) {
Looking at platform_get_irq() all error values are negative. You could just
test for ret < 0 here, and remove the ret = -ENODEV assignment below to keep
the error code returned by platform_get_irq().
If you're fine with that modification there's no need to resubmit, just let me
know and I'll fix it when applying it to my tree.
> dev_err(isp->dev, "No IRQ resource\n");
> ret = -ENODEV;
> goto error_iommu;
> }
> + isp->irq_num = ret;
>
> if (devm_request_irq(isp->dev, isp->irq_num, isp_isr, IRQF_SHARED,
>"OMAP3 ISP", isp)) {
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" 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 9/9] [media] omap3isp: support 64-bit version of omap3isp_stat_data
Hi Arnd,
Thank you for the patch.
On Thursday 17 September 2015 23:19:40 Arnd Bergmann wrote:
> C libraries with 64-bit time_t use an incompatible format for
> struct omap3isp_stat_data. This changes the kernel code to
> support either version, by moving over the normal handling
> to the 64-bit variant, and adding compatiblity code to handle
> the old binary format with the existing ioctl command code.
>
> Fortunately, the command code includes the size of the structure,
> so the difference gets handled automatically.
We plan to design a new interface to handle statistics in V4L2. That API
should support proper 64-bit timestamps out of the box, and will be
implemented by the OMAP3 ISP driver. Userspace should then move to it. I
wonder if it's worth it to fix the existing VIDIOC_OMAP3ISP_STAT_REQ ioctl
given that I expect it to have a handful of users at most.
> Signed-off-by: Arnd Bergmann
> ---
> drivers/media/platform/omap3isp/isph3a_aewb.c | 2 ++
> drivers/media/platform/omap3isp/isph3a_af.c | 2 ++
> drivers/media/platform/omap3isp/isphist.c | 2 ++
> drivers/media/platform/omap3isp/ispstat.c | 18 --
> drivers/media/platform/omap3isp/ispstat.h | 2 ++
> include/uapi/linux/omap3isp.h | 19 +++
> 6 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c
> b/drivers/media/platform/omap3isp/isph3a_aewb.c index
> ccaf92f39236..2d567725608f 100644
> --- a/drivers/media/platform/omap3isp/isph3a_aewb.c
> +++ b/drivers/media/platform/omap3isp/isph3a_aewb.c
> @@ -250,6 +250,8 @@ static long h3a_aewb_ioctl(struct v4l2_subdev *sd,
> unsigned int cmd, void *arg) return omap3isp_stat_config(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_REQ:
> return omap3isp_stat_request_statistics(stat, arg);
> + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> + return omap3isp_stat_request_statistics_time32(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_EN: {
> unsigned long *en = arg;
> return omap3isp_stat_enable(stat, !!*en);
> diff --git a/drivers/media/platform/omap3isp/isph3a_af.c
> b/drivers/media/platform/omap3isp/isph3a_af.c index
> 92937f7eecef..2ac02371318b 100644
> --- a/drivers/media/platform/omap3isp/isph3a_af.c
> +++ b/drivers/media/platform/omap3isp/isph3a_af.c
> @@ -314,6 +314,8 @@ static long h3a_af_ioctl(struct v4l2_subdev *sd,
> unsigned int cmd, void *arg) return omap3isp_stat_config(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_REQ:
> return omap3isp_stat_request_statistics(stat, arg);
> + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> + return omap3isp_stat_request_statistics_time32(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_EN: {
> int *en = arg;
> return omap3isp_stat_enable(stat, !!*en);
> diff --git a/drivers/media/platform/omap3isp/isphist.c
> b/drivers/media/platform/omap3isp/isphist.c index
> 7138b043a4aa..669b97b079ee 100644
> --- a/drivers/media/platform/omap3isp/isphist.c
> +++ b/drivers/media/platform/omap3isp/isphist.c
> @@ -436,6 +436,8 @@ static long hist_ioctl(struct v4l2_subdev *sd, unsigned
> int cmd, void *arg) return omap3isp_stat_config(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_REQ:
> return omap3isp_stat_request_statistics(stat, arg);
> + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> + return omap3isp_stat_request_statistics_time32(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_EN: {
> int *en = arg;
> return omap3isp_stat_enable(stat, !!*en);
> diff --git a/drivers/media/platform/omap3isp/ispstat.c
> b/drivers/media/platform/omap3isp/ispstat.c index
> fa96e330c563..3d70332422b5 100644
> --- a/drivers/media/platform/omap3isp/ispstat.c
> +++ b/drivers/media/platform/omap3isp/ispstat.c
> @@ -496,8 +496,7 @@ int omap3isp_stat_request_statistics(struct ispstat
> *stat, return PTR_ERR(buf);
> }
>
> - data->ts.tv_sec = buf->ts.tv_sec;
> - data->ts.tv_usec = buf->ts.tv_usec;
> + data->ts = buf->ts;
> data->config_counter = buf->config_counter;
> data->frame_number = buf->frame_number;
> data->buf_size = buf->buf_size;
> @@ -509,6 +508,21 @@ int omap3isp_stat_request_statistics(struct ispstat
> *stat, return 0;
> }
>
> +int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
> + struct omap3isp_stat_data_time32 *data)
> +{
> + struct omap3isp_stat_data data64;
> + int ret;
> +
> + ret = omap3isp_stat_request_statistics(stat, &data64);
> +
> + data->ts.tv_sec = data64.ts.tv_sec;
> + data->ts.tv_usec = data64.ts.tv_usec;
> + memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts));
> +
> + return ret;
> +}
> +
> /*
> * omap3isp_stat_config - Receives new statistic engine configuration.
> * @new_conf: Pointer to config structure.
> diff --git a/drivers/media/platform/omap3isp/ispstat.h
Re: [PATCH 12/13] [media] omap3isp: Support for deferred probing when requesting DMA channel
Hi Peter,
Thank you for the patch.
What happened to this patch series ? It looks like
dma_request_slave_channel_compat_reason() isn't in mainline, so I can't apply
this patch.
I'll mark this patch as deferred in patchwork, please resubmit it if you
resubmit the series (and by the look of it the issue you're trying to fix
still exists, so it would be nice if you could get it eventually fixed).
On Tuesday 26 May 2015 16:26:07 Peter Ujfalusi wrote:
> Switch to use ma_request_slave_channel_compat_reason() to request the DMA
> channel. Only fall back to pio mode if the error code returned is not
> -EPROBE_DEFER, otherwise return from the probe with the -EPROBE_DEFER.
>
> Signed-off-by: Peter Ujfalusi
> CC: Laurent Pinchart
> CC: Mauro Carvalho Chehab
> ---
> drivers/media/platform/omap3isp/isphist.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/isphist.c
> b/drivers/media/platform/omap3isp/isphist.c index
> 7138b043a4aa..e690ca13af0e 100644
> --- a/drivers/media/platform/omap3isp/isphist.c
> +++ b/drivers/media/platform/omap3isp/isphist.c
> @@ -499,14 +499,20 @@ int omap3isp_hist_init(struct isp_device *isp)
> if (res)
> sig = res->start;
>
> - hist->dma_ch = dma_request_slave_channel_compat(mask,
> + hist->dma_ch = dma_request_slave_channel_compat_reason(mask,
> omap_dma_filter_fn, &sig, isp->dev, "hist");
> - if (!hist->dma_ch)
> + if (IS_ERR(hist->dma_ch)) {
> + ret = PTR_ERR(hist->dma_ch);
> + if (ret == -EPROBE_DEFER)
> + return ret;
> +
> + hist->dma_ch = NULL;
> dev_warn(isp->dev,
>"hist: DMA channel request failed, using
> PIO\n");
> - else
> + } else {
> dev_dbg(isp->dev, "hist: using DMA channel %s\n",
> dma_chan_name(hist->dma_ch));
> + }
> }
>
> hist->ops = &hist_ops;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] media: omap3isp: use vb2_buffer_state enum for vb2 buffer state
Hi Prabhakar,
Thank you for the patch.
On Wednesday 25 February 2015 15:58:14 Lad Prabhakar wrote:
> From: "Lad, Prabhakar"
>
> use the vb2_buffer_state enum for assigning the state
> of the vb2 buffer, along side making isp_pipeline_state
> state variable local to the block.
> This fixes the following sparse warning as well:
> drivers/media/platform/omap3isp/ispvideo.c:497:35: warning: mixing different
> enum types drivers/media/platform/omap3isp/ispvideo.c:497:35: int enum
> isp_pipeline_state versus
> drivers/media/platform/omap3isp/ispvideo.c:497:35: int enum
> vb2_buffer_state
>
> Signed-off-by: Lad, Prabhakar
Reviewed-by: Laurent Pinchart
and applied to my tree.
> ---
> drivers/media/platform/omap3isp/ispvideo.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> b/drivers/media/platform/omap3isp/ispvideo.c index 3fe9047..8cd3a57 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -449,7 +449,7 @@ static const struct vb2_ops isp_video_queue_ops = {
> struct isp_buffer *omap3isp_video_buffer_next(struct isp_video *video)
> {
> struct isp_pipeline *pipe = to_isp_pipeline(&video->video.entity);
> - enum isp_pipeline_state state;
> + enum vb2_buffer_state vb_state;
> struct isp_buffer *buf;
> unsigned long flags;
> struct timespec ts;
> @@ -488,17 +488,19 @@ struct isp_buffer *omap3isp_video_buffer_next(struct
> isp_video *video)
>
> /* Report pipeline errors to userspace on the capture device side. */
> if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE && pipe->error) {
> - state = VB2_BUF_STATE_ERROR;
> + vb_state = VB2_BUF_STATE_ERROR;
> pipe->error = false;
> } else {
> - state = VB2_BUF_STATE_DONE;
> + vb_state = VB2_BUF_STATE_DONE;
> }
>
> - vb2_buffer_done(&buf->vb, state);
> + vb2_buffer_done(&buf->vb, vb_state);
>
> spin_lock_irqsave(&video->irqlock, flags);
>
> if (list_empty(&video->dmaqueue)) {
> + enum isp_pipeline_state state;
> +
> spin_unlock_irqrestore(&video->irqlock, flags);
>
> if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] omap3isp: Return buffers back to videobuf2 if pipeline streamon fails
Hi Sakari,
Thank you for the patch.
On Friday 19 September 2014 00:57:49 Sakari Ailus wrote:
> When the video buffer queue was stopped before the stream source was started
> in omap3isp_streamon(), the buffers were not returned back to videobuf2.
>
> Signed-off-by: Sakari Ailus
> ---
> drivers/media/platform/omap3isp/isp.c |4 ++--
> drivers/media/platform/omap3isp/ispvideo.c | 16 ++--
> drivers/media/platform/omap3isp/ispvideo.h |3 ++-
> 3 files changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 72265e5..2aa0a8e 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1062,9 +1062,9 @@ int omap3isp_pipeline_set_stream(struct isp_pipeline
> *pipe, void omap3isp_pipeline_cancel_stream(struct isp_pipeline *pipe)
> {
> if (pipe->input)
> - omap3isp_video_cancel_stream(pipe->input);
> + omap3isp_video_cancel_stream(pipe->input, VB2_BUF_STATE_ERROR);
> if (pipe->output)
> - omap3isp_video_cancel_stream(pipe->output);
> + omap3isp_video_cancel_stream(pipe->output, VB2_BUF_STATE_ERROR);
> }
>
> /*
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> b/drivers/media/platform/omap3isp/ispvideo.c index b233c8e..73c0194 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -443,8 +443,10 @@ static int isp_video_start_streaming(struct vb2_queue
> *queue,
>
> ret = omap3isp_pipeline_set_stream(pipe,
> ISP_PIPELINE_STREAM_CONTINUOUS);
> - if (ret < 0)
> + if (ret < 0) {
> + omap3isp_video_cancel_stream(video, VB2_BUF_STATE_QUEUED);
This will set video->error to true. As the flag is only reset to false when
stopping the stream in isp_video_streamoff we'll have a problem here.
One option would be to split the buffer handling part of
omap3isp_video_cancel_stream() to a separate function
omap3isp_video_return_buffers() and call that one when stream start fails.
> return ret;
> + }
>
> spin_lock_irqsave(&video->irqlock, flags);
> if (list_empty(&video->dmaqueue))
> @@ -566,10 +568,12 @@ struct isp_buffer *omap3isp_video_buffer_next(struct
> isp_video *video) * omap3isp_video_cancel_stream - Cancel stream on a video
> node
> * @video: ISP video object
You're missing documentation of the state parameter here.
> *
> - * Cancelling a stream mark all buffers on the video node as erroneous and
> makes
> - * sure no new buffer can be queued.
> + * Cancelling a stream mark all buffers on the video node as erroneous
> + * and makes sure no new buffer can be queued. Buffers are returned
> + * back to videobuf2 in the given state.
The buffer isn't marked as erroneous when the state is set to
VB2_BUF_STATE_QUEUED. How about
"Cancelling a stream returns all buffers queued on the video node to videobuf2
in the given state and makes sure no new buffer can be queued. The buffer
state should be VB2_BUF_STATE_QUEUED when the stream is cancelled due to an
error when starting the stream, or VB2_BUF_STATE_ERROR otherwise."
I've pushed a new version of the patch that fixes all this to
git://linuxtv.org/pinchartl/media.git omap3isp/next
(http://git.linuxtv.org/cgit.cgi/pinchartl/media.git/commit/?h=omap3isp/next&id=da80a526014da7a2c07af9978aaafc47d816e103)
If you like it there's no need to resubmit.
> */
> -void omap3isp_video_cancel_stream(struct isp_video *video)
> +void omap3isp_video_cancel_stream(struct isp_video *video,
> + enum vb2_buffer_state state)
> {
> unsigned long flags;
>
> @@ -581,7 +585,7 @@ void omap3isp_video_cancel_stream(struct isp_video
> *video) buf = list_first_entry(&video->dmaqueue,
> struct isp_buffer, irqlist);
> list_del(&buf->irqlist);
> - vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
> + vb2_buffer_done(&buf->vb, state);
> }
>
> video->error = true;
> @@ -1166,7 +1170,7 @@ isp_video_streamoff(struct file *file, void *fh, enum
> v4l2_buf_type type)
>
> /* Stop the stream. */
> omap3isp_pipeline_set_stream(pipe, ISP_PIPELINE_STREAM_STOPPED);
> - omap3isp_video_cancel_stream(video);
> + omap3isp_video_cancel_stream(video, VB2_BUF_STATE_ERROR);
>
> mutex_lock(&video->queue_lock);
> vb2_streamoff(&vfh->queue, type);
> diff --git a/drivers/media/platform/omap3isp/ispvideo.h
> b/drivers/media/platform/omap3isp/ispvideo.h index 0b7efed..7e4732a 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.h
> +++ b/drivers/media/platform/omap3isp/ispvideo.h
> @@ -201,7 +201,8 @@ int omap3isp_video_register(struct isp_video *video,
> struct v4l2_device *vdev);
> void omap3isp_video_unregister(struct isp_video *video);
Re: [PATCH 2/3] omap3isp: Move starting the sensor from streamon IOCTL handler to VB2 QOP
Hi Sakari,
Thank you for the patch.
On Friday 19 September 2014 00:57:48 Sakari Ailus wrote:
> Move the starting of the sensor from the VIDIOC_STREAMON handler to the
> videobuf2 queue op start_streaming. This avoids failing starting the stream
> after vb2_streamon() has already finished.
>
> Signed-off-by: Sakari Ailus
Reviewed-by: Laurent Pinchart
and applied to my tree.
> ---
> drivers/media/platform/omap3isp/ispvideo.c | 49 +++-
> 1 file changed, 30 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> b/drivers/media/platform/omap3isp/ispvideo.c index bc38c88..b233c8e 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -425,10 +425,40 @@ static void isp_video_buffer_queue(struct vb2_buffer
> *buf) }
> }
>
> +static int isp_video_start_streaming(struct vb2_queue *queue,
> + unsigned int count)
> +{
> + struct isp_video_fh *vfh = vb2_get_drv_priv(queue);
> + struct isp_video *video = vfh->video;
> + struct isp_pipeline *pipe = to_isp_pipeline(&video->video.entity);
> + unsigned long flags;
> + int ret;
> +
> + /* In sensor-to-memory mode, the stream can be started synchronously
> + * to the stream on command. In memory-to-memory mode, it will be
> + * started when buffers are queued on both the input and output.
> + */
> + if (pipe->input)
> + return 0;
> +
> + ret = omap3isp_pipeline_set_stream(pipe,
> +ISP_PIPELINE_STREAM_CONTINUOUS);
> + if (ret < 0)
> + return ret;
> +
> + spin_lock_irqsave(&video->irqlock, flags);
> + if (list_empty(&video->dmaqueue))
> + video->dmaqueue_flags |= ISP_VIDEO_DMAQUEUE_UNDERRUN;
> + spin_unlock_irqrestore(&video->irqlock, flags);
> +
> + return 0;
> +}
> +
> static const struct vb2_ops isp_video_queue_ops = {
> .queue_setup = isp_video_queue_setup,
> .buf_prepare = isp_video_buffer_prepare,
> .buf_queue = isp_video_buffer_queue,
> + .start_streaming = isp_video_start_streaming,
> };
>
> /*
> @@ -1077,28 +1107,9 @@ isp_video_streamon(struct file *file, void *fh, enum
> v4l2_buf_type type) if (ret < 0)
> goto err_check_format;
>
> - /* In sensor-to-memory mode, the stream can be started synchronously
> - * to the stream on command. In memory-to-memory mode, it will be
> - * started when buffers are queued on both the input and output.
> - */
> - if (pipe->input == NULL) {
> - ret = omap3isp_pipeline_set_stream(pipe,
> - ISP_PIPELINE_STREAM_CONTINUOUS);
> - if (ret < 0)
> - goto err_set_stream;
> - spin_lock_irqsave(&video->irqlock, flags);
> - if (list_empty(&video->dmaqueue))
> - video->dmaqueue_flags |= ISP_VIDEO_DMAQUEUE_UNDERRUN;
> - spin_unlock_irqrestore(&video->irqlock, flags);
> - }
> -
> mutex_unlock(&video->stream_lock);
> return 0;
>
> -err_set_stream:
> - mutex_lock(&video->queue_lock);
> - vb2_streamoff(&vfh->queue, type);
> - mutex_unlock(&video->queue_lock);
> err_check_format:
> media_entity_pipeline_stop(&video->video.entity);
> err_pipeline_start:
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] ARM: OMAP2+: Remove legacy OMAP3 ISP instantiation
Hi Tony, On Monday 12 October 2015 10:05:59 Tony Lindgren wrote: > * Tony Lindgren [150914 09:37]: > > * Suman Anna [150914 09:33]: > >> On 09/03/2015 05:34 PM, Suman Anna wrote: > >>> On 07/16/2015 07:58 AM, Tony Lindgren wrote: > * Laurent Pinchart [150716 05:57]: > The OMAP3 ISP is now fully supported in DT, remove its instantiation > > from C code. > > Please feel to queue this along with the second patch in this series, > this should not cause any merge conflicts: > > Acked-by: Tony Lindgren > >>> > >>> Just wondering if you have already queued this, I see the v4l changes > >>> in linux-next, but not this patch. Also, can you confirm if this > >>> series is making it into 4.3? > >> > >> This patch is not in 4.3-rc1, can you pick this up for the next merge > >> window? I am gonna send out some additional cleanup patches (removing > >> legacy mailbox and iommu pieces) on top on this patch. The second patch > >> in this series did make it. > > > > OK tagging this one for next, will apply once I'm done with fixes. > > Applying into omap-for-v4.4/cleanup finally thanks. Is see that the patch is in your master branch but not your for-next branch. Will it make it to v4.4-rc1 ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add interface protocol 1 for Surface Pro 3 cameras
Hi Zvi,
Thank you for the patch.
On Monday 24 August 2015 15:57:42 Zvi Effron wrote:
> The cameras on the Surface Pro 3 report interface protocol of 1.
> The generic USB video class doesn't work for them.
> This adds entries for the front and rear camera.
This doesn't need to be restricted to the Surface Pro 3 cameras as
bInterfaceProtocol 1 is (or at least should be) used by all UVC 1.5 devices.
I've just posted "[PATCH] uvcvideo: Enable UVC 1.5 device detection" to the
linux-media mailing list. Could you check whether it fixes your problem ?
> Signed-off-by: Zvi Effron
> ---
> drivers/media/usb/uvc/uvc_driver.c | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 4b5b3e8..d2fdbc1 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2142,6 +2142,22 @@ static struct usb_device_id uvc_ids[] = {
> .bInterfaceSubClass = 1,
> .bInterfaceProtocol = 0,
> .driver_info = UVC_QUIRK_PROBE_MINMAX },
> + /* Microsoft Surface Pro 3 LifeCam Front */
> + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> + | USB_DEVICE_ID_MATCH_INT_INFO,
> + .idVendor = 0x045e,
> + .idProduct= 0x07be,
> + .bInterfaceClass = USB_CLASS_VIDEO,
> + .bInterfaceSubClass = 1,
> + .bInterfaceProtocol = 1 },
> + /* Microsoft Surface Pro 3 LifeCam Rear */
> + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> + | USB_DEVICE_ID_MATCH_INT_INFO,
> + .idVendor = 0x045e,
> + .idProduct= 0x07bf,
> + .bInterfaceClass = USB_CLASS_VIDEO,
> + .bInterfaceSubClass = 1,
> + .bInterfaceProtocol = 1 },
> /* Logitech Quickcam Fusion */
> { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
>
> | USB_DEVICE_ID_MATCH_INT_INFO,
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] uvcvideo: Enable UVC 1.5 device detection
UVC 1.5 devices report a bInterfaceProtocol value set to 1 in their
interface descriptors. The uvcvideo driver only matches on
bInterfaceProtocol 0, preventing those devices from being detected.
More changes to the driver are needed for full UVC 1.5 compatibility.
However, at least the UVC 1.5 Microsoft Surface Pro 3 cameras have been
reported to work out of the box with the driver with an updated match
table.
Enable UVC 1.5 support in the match table to support the devices that
can work with the current driver implementation. Devices that can't will
fail, but that's hardly a regression as they're currently not detected
at all anyway.
Signed-off-by: Laurent Pinchart
---
drivers/media/usb/uvc/uvc_driver.c | 3 ++-
include/uapi/linux/usb/video.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/uvc/uvc_driver.c
b/drivers/media/usb/uvc/uvc_driver.c
index f8e7793e2056..a978f7d9b81d 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2597,7 +2597,8 @@ static struct usb_device_id uvc_ids[] = {
.bInterfaceProtocol = 0,
.driver_info = UVC_QUIRK_FORCE_Y8 },
/* Generic USB Video Class */
- { USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, 0) },
+ { USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) },
+ { USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) },
{}
};
diff --git a/include/uapi/linux/usb/video.h b/include/uapi/linux/usb/video.h
index 3b3b95e01f71..69ab695fad2e 100644
--- a/include/uapi/linux/usb/video.h
+++ b/include/uapi/linux/usb/video.h
@@ -28,6 +28,7 @@
/* A.3. Video Interface Protocol Codes */
#define UVC_PC_PROTOCOL_UNDEFINED 0x00
+#define UVC_PC_PROTOCOL_15 0x01
/* A.5. Video Class-Specific VC Interface Descriptor Subtypes */
#define UVC_VC_DESCRIPTOR_UNDEFINED0x00
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: uvc: add support for the Microsoft Surface Pro 3 Cameras
Hi Denis, On Thursday 11 June 2015 13:13:30 Dennis Chen wrote: > > Could you please send me the output of 'lsusb -v -d 045e:07be' and > > 'lsusb -v - > > d 045e:07bf' (running as root if possible) ? > > Bus 001 Device 004: ID 045e:07bf Microsoft Corp. > Device Descriptor: > bLength18 > bDescriptorType 1 > bcdUSB 2.00 > bDeviceClass 239 Miscellaneous Device > bDeviceSubClass 2 ? > bDeviceProtocol 1 Interface Association > bMaxPacketSize064 > idVendor 0x045e Microsoft Corp. > idProduct 0x07bf > bcdDevice 21.52 > iManufacturer 1 QCM > iProduct2 Microsoft LifeCam Rear > iSerial 0 > bNumConfigurations 1 [snip] > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber0 > bAlternateSetting 0 > bNumEndpoints 1 > bInterfaceClass14 Video > bInterfaceSubClass 1 Video Control > bInterfaceProtocol 1 > iInterface 2 Microsoft LifeCam Rear [snip] I see where the problem comes from now. I had missed it before, but your device sets the bInterfaceProtocol value to 1 as it's UVC 1.5 compliant, as opposed to value 0 for UVC 1.1. The uvcvideo driver doesn't support UVC 1.5 yet. It looks like your camera supports the UVC 1.1 protocol as well, but that's not true of all UVC devices in general. I expect that enabling detection of UVC 1.5 support in the driver will result in issues with UVC 1.5 devices, but on the other hand those devices are currently not supported at all. I'll thus submit a patch to enable UVC 1.5 device detection, and we'll see how that goes. I'll CC you and would appreciate if you could test the patch. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/2] uvc: Add a quirk flag for cameras that do not produce correct timestamps
Hi Sakari,
On Wednesday 05 November 2014 18:12:33 Sakari Ailus wrote:
> The UVC devices do produce hardware timestamps according to the spec, but
> not all cameras implement it or implement it correctly. Add a quirk flag for
> such devices, and use monotonic timestamp from the end of the frame
> instead.
>
> Signed-off-by: Sakari Ailus
I don't think this is the right fix, as the problem seems to come from the
driver, not from the camera. I've disabled hardware timestamps by default for
now which should work around the problem until I find time to conduct a full
investigation and fix it.
> ---
> drivers/media/usb/uvc/uvc_queue.c | 6 --
> drivers/media/usb/uvc/uvc_video.c | 14 +-
> drivers/media/usb/uvc/uvcvideo.h | 4 +++-
> 3 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_queue.c
> b/drivers/media/usb/uvc/uvc_queue.c index 6e92d20..3f6432f 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -141,7 +141,7 @@ static struct vb2_ops uvc_queue_qops = {
> };
>
> int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
> - int drop_corrupted)
> +bool drop_corrupted, bool tstamp_eof)
> {
> int ret;
>
> @@ -152,7 +152,9 @@ int uvc_queue_init(struct uvc_video_queue *queue, enum
> v4l2_buf_type type, queue->queue.ops = &uvc_queue_qops;
> queue->queue.mem_ops = &vb2_vmalloc_memops;
> queue->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC
> - | V4L2_BUF_FLAG_TSTAMP_SRC_SOE;
> + | (tstamp_eof ? V4L2_BUF_FLAG_TSTAMP_SRC_EOF
> +: V4L2_BUF_FLAG_TSTAMP_SRC_SOE);
> +
> ret = vb2_queue_init(&queue->queue);
> if (ret)
> return ret;
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index df81b9c..f599112 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -382,6 +382,9 @@ uvc_video_clock_decode(struct uvc_streaming *stream,
> struct uvc_buffer *buf, u16 host_sof;
> u16 dev_sof;
>
> + if (stream->dev->quirks & UVC_QUIRK_BAD_TIMESTAMP)
> + return;
> +
> switch (data[1] & (UVC_STREAM_PTS | UVC_STREAM_SCR)) {
> case UVC_STREAM_PTS | UVC_STREAM_SCR:
> header_size = 12;
> @@ -490,6 +493,9 @@ static int uvc_video_clock_init(struct uvc_streaming
> *stream) {
> struct uvc_clock *clock = &stream->clock;
>
> + if (stream->dev->quirks & UVC_QUIRK_BAD_TIMESTAMP)
> + return 0;
> +
> spin_lock_init(&clock->lock);
> clock->size = 32;
>
> @@ -615,6 +621,11 @@ void uvc_video_clock_update(struct uvc_streaming
> *stream, u32 rem;
> u64 y;
>
> + if (stream->dev->quirks & UVC_QUIRK_BAD_TIMESTAMP) {
> + v4l2_get_timestamp(&v4l2_buf->timestamp);
> + return;
> + }
> +
> spin_lock_irqsave(&clock->lock, flags);
>
> if (clock->count < clock->size)
> @@ -1779,7 +1790,8 @@ int uvc_video_init(struct uvc_streaming *stream)
> atomic_set(&stream->active, 0);
>
> /* Initialize the video buffers queue. */
> - ret = uvc_queue_init(&stream->queue, stream->type, !uvc_no_drop_param);
> + ret = uvc_queue_init(&stream->queue, stream->type, !uvc_no_drop_param,
> + stream->dev->quirks & UVC_QUIRK_BAD_TIMESTAMP);
> if (ret)
> return ret;
>
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 864ada7..89a638c 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -148,6 +148,7 @@
> #define UVC_QUIRK_PROBE_DEF 0x0100
> #define UVC_QUIRK_RESTRICT_FRAME_RATE0x0200
> #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x0400
> +#define UVC_QUIRK_BAD_TIMESTAMP 0x0800
>
> /* Format flags */
> #define UVC_FMT_FLAG_COMPRESSED 0x0001
> @@ -622,7 +623,8 @@ extern struct uvc_entity *uvc_entity_by_id(struct
> uvc_device *dev, int id);
>
> /* Video buffers queue management. */
> extern int uvc_queue_init(struct uvc_video_queue *queue,
> - enum v4l2_buf_type type, int drop_corrupted);
> + enum v4l2_buf_type type, bool drop_corrupted,
> + bool tstamp_eof);
> extern int uvc_alloc_buffers(struct uvc_video_queue *queue,
> struct v4l2_requestbuffers *rb);
> extern void uvc_free_buffers(struct uvc_video_queue *queue);
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] [media] mt9v032: Add reset and standby gpios
Hi Markus, On Monday 09 November 2015 16:33:03 Markus Pargmann wrote: > On Monday 09 November 2015 14:28:56 Laurent Pinchart wrote: > > On Friday 06 November 2015 14:13:43 Markus Pargmann wrote: > >> Add optional reset and standby gpios. The reset gpio is used to reset > >> the chip in power_on(). > >> > >> The standby gpio is not used currently. It is just unset, so the chip is > >> not in standby. > > > > We could use a gpio hog for this, but given that the standby signal should > > eventually get used, and given that specifying it in DT is a good hardware > > description, that looks good to me. > > > >> Signed-off-by: Markus Pargmann > >> Reviewed-by: Philipp Zabel > >> --- > >> > >> .../devicetree/bindings/media/i2c/mt9v032.txt | 2 ++ > >> drivers/media/i2c/mt9v032.c| 23 +++ > >> 2 files changed, 25 insertions(+) [snip] > > If you're fine with these changes there's no need to resubmit the patch, I > > can fix it when applying it to my tree. > > Thanks, I am fine with all your changes. But as there will be a v2 for the > other two patches I could as well send an updated version if you wish. As you wish, both options are fine with me. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] [media] mt9v032: Add reset and standby gpios
Hi,
On Monday 09 November 2015 14:28:56 Laurent Pinchart wrote:
> Hi Markus,
>
> Thank you for the patch.
>
> On Friday 06 November 2015 14:13:43 Markus Pargmann wrote:
> > Add optional reset and standby gpios. The reset gpio is used to reset
> > the chip in power_on().
> >
> > The standby gpio is not used currently. It is just unset, so the chip is
> > not in standby.
>
> We could use a gpio hog for this, but given that the standby signal should
> eventually get used, and given that specifying it in DT is a good hardware
> description, that looks good to me.
>
> > Signed-off-by: Markus Pargmann
> > Reviewed-by: Philipp Zabel
> > ---
> > .../devicetree/bindings/media/i2c/mt9v032.txt | 2 ++
> > drivers/media/i2c/mt9v032.c| 23 +++
> > 2 files changed, 25 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt
> > b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt index
> > 202565313e82..100f0ae43269 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt
> > @@ -20,6 +20,8 @@ Optional Properties:
> >
> > - link-frequencies: List of allowed link frequencies in Hz. Each frequency
> > is expressed as a 64-bit big-endian integer.
> > +- reset-gpios: GPIO handle which is connected to the reset pin of the chip.
> > +- standby-gpios: GPIO handle which is connected to the standby pin of the
> > chip.
> >
> > For further reading on port node refer to
> > Documentation/devicetree/bindings/media/video-interfaces.txt.
> > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> > index a68ce94ee097..4aefde9634f5 100644
> > --- a/drivers/media/i2c/mt9v032.c
> > +++ b/drivers/media/i2c/mt9v032.c
> > @@ -24,6 +24,7 @@
> > #include
> > #include
> > #include
> > +#include
>
> module.h escaped my vigilance, but let's try to keep headers alphabetically
> sorted.
> >
> > #include
> > #include
> > @@ -251,6 +252,8 @@ struct mt9v032 {
> >
> > struct regmap *regmap;
> > struct clk *clk;
> > + struct gpio_desc *reset_gpio;
> > + struct gpio_desc *standby_gpio;
> >
> > struct mt9v032_platform_data *pdata;
> > const struct mt9v032_model_info *model;
> > @@ -312,16 +315,26 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
> > struct regmap *map = mt9v032->regmap;
> > int ret;
> >
> > + gpiod_set_value_cansleep(mt9v032->reset_gpio, 1);
> > +
> > ret = clk_set_rate(mt9v032->clk, mt9v032->sysclk);
> > if (ret < 0)
> > return ret;
> >
> > + /* system clock has to be enabled before releasing the reset */
>
> Nitpicking, the driver capitalizes the first letter of comments.
>
> > ret = clk_prepare_enable(mt9v032->clk);
> > if (ret)
> > return ret;
> >
> > udelay(1);
> >
> > + gpiod_set_value_cansleep(mt9v032->reset_gpio, 0);
> > +
> > + /*
> > +* After releasing reset, it can take up to 1us until the chip is done
> > +*/
> > + udelay(1);
> > +
>
> The delay isn't necessary if there's no reset GPIO. How about
>
> if (mt9v032->reset_gpio) {
> gpiod_set_value_cansleep(mt9v032->reset_gpio, 0);
>
> /* After releasing reset, it can take up to 1us until the
>* chip is done.
>*/
> udelay(1);
> }
>
> And, according to the datasheet, the delay is 10 SYSCLK periods. 1µs should
> be
> safe as the minimum SYSCLK frequency is 13 MHz. I'd still mention it in a
> comment, maybe as
>
> /* After releasing reset we need to wait 10 clock cycles
>* before accessing the sensor over I2C. As the minimum SYSCLK
>* frequency is 13MHz, waiting 1µs will be enough in the worst
>* case.
>*/
> udelay(1);
>
> If you're fine with these changes there's no need to resubmit the patch, I
> can
> fix it when applying it to my tree.
Thanks, I am fine with all your changes. But as there will be a v2 for the
other two patches I could as well send an updated version if you wish.
Thanks,
Markus
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
signature.asc
Description: This is a digitally signed message part.
Re: [RFC/PATCH] media: omap3isp: Set maximum DMA segment size
Hi Laurent,
On 09/11/15 14:18, Laurent Pinchart wrote:
Hello everybody,
Ping ?
Apologies, I did start writing a response a while ago, but it ended up
getting subsumed into the bigger DMA API discussion thread.
On Tuesday 13 October 2015 16:18:36 Laurent Pinchart wrote:
The maximum DMA segment size controls the IOMMU mapping granularity. Its
default value is 64kB, resulting in potentially non-contiguous IOMMU
mappings. Configure it to 4GB to ensure that buffers get mapped
contiguously.
Signed-off-by: Laurent Pinchart
---
drivers/media/platform/omap3isp/isp.c | 4
drivers/media/platform/omap3isp/isp.h | 1 +
2 files changed, 5 insertions(+)
I'm posting this as an RFC because I'm not happy with the patch, even if it
gets the job done.
On ARM the maximum DMA segment size is used when creating IOMMU mappings. As
a large number of devices require contiguous memory buffers (this is a very
common requirement for video-related embedded devices) the default 64kB
value doesn't work.
Per the initial patch (6b7b65105522), dma_parms was intended to expose
hardware limitations of scatter-gather-capable devices, to prevent DMA
API implementations from merging segments beyond a device's limits. Thus
the way 32-bit ARM (and seemingly noone else) is taking it as something
to apply to non-scatter-gather-capable devices in order to force the DMA
API implementation to merge segments seems very questionable.
There's nothing in the streaming DMA API which gives any guarantee of a
contiguous mapping, so it's the incorrect assumption that it does which
needs fixing. Whether we rework the callers or the API itself is the
open question there, I think.
I haven't investigated the history behind this API in details but I have a
feeling something is not quite right. We force most drivers to explicitly
set the maximum segment size from a default that seems valid for specific
use cases only. Furthermore, as the DMA parameters are not stored directly
in struct device this require allocation of external memory for which we
have no proper management rule, making automatic handling of the DMA
parameters in frameworks or helper functions cumbersome (for a discussion
on this topic see http://www.spinics.net/lists/linux-media/msg92467.html
and http://lists.infradead.org/pipermail/linux-arm-kernel/2014->
November/305913.html).
Is it time to fix this mess ?
I agree that it would certainly be preferable to tackle the underlying
issue instead of adding more point hacks to further entrench
non-portable code into the kernel. In terms of modifying the API, the
most reasonable idea which comes to mind would be a DMA attribute, and
on closer inspection, I see that DMA_ATTR_FORCE_CONTIGUOUS is already a
thing - perhaps we should weigh up whether coherent and streaming DMA
could overload the same attribute with subtly different meanings, or
whether we'd want e.g. DMA_ATTR_FORCE_PHYS_CONTIGUOUS and
DMA_ATTR_FORCE_DMA_CONTIGUOUS to coexist.
Robin.
diff --git a/drivers/media/platform/omap3isp/isp.c
b/drivers/media/platform/omap3isp/isp.c index 17430a6ed85a..ebf7dc76e94d
100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2444,6 +2444,10 @@ static int isp_probe(struct platform_device *pdev)
if (ret)
goto error;
+ isp->dev->dma_parms = &isp->dma_parms;
+ dma_set_max_seg_size(isp->dev, DMA_BIT_MASK(32));
+ dma_set_seg_boundary(isp->dev, 0x);
Whatever happens, 002edb6f6f2a should now make this line redundant :D
+
platform_set_drvdata(pdev, isp);
/* Regulators */
diff --git a/drivers/media/platform/omap3isp/isp.h
b/drivers/media/platform/omap3isp/isp.h index e579943175c4..4b2231cf01be
100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -193,6 +193,7 @@ struct isp_device {
u32 syscon_offset;
u32 phy_type;
+ struct device_dma_parameters dma_parms;
struct dma_iommu_mapping *mapping;
/* ISP Obj */
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] [media] mt9v032: Add V4L2 controls for AEC and AGC
Hi,
On Monday 09 November 2015 15:22:06 Laurent Pinchart wrote:
> Hi Markus,
>
> Thank you for the patch.
>
> On Friday 06 November 2015 14:13:45 Markus Pargmann wrote:
> > This patch adds V4L2 controls for Auto Exposure Control and Auto Gain
> > Control settings. These settings include low pass filter, update
> > frequency of these settings and the update interval for those units.
> >
> > Signed-off-by: Markus Pargmann
> > ---
> > drivers/media/i2c/mt9v032.c | 153 +
> > 1 file changed, 153 insertions(+)
> >
> > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> > index 943c3f39ea73..978ae8cbb0cc 100644
> > --- a/drivers/media/i2c/mt9v032.c
> > +++ b/drivers/media/i2c/mt9v032.c
> > @@ -133,9 +133,16 @@
> > #defineMT9V032_TEST_PATTERN_GRAY_DIAGONAL (3 << 11)
> > #defineMT9V032_TEST_PATTERN_ENABLE (1 << 13)
> > #defineMT9V032_TEST_PATTERN_FLIP (1 << 14)
> > +#define MT9V032_AEC_LPF0xa8
> > +#define MT9V032_AGC_LPF0xaa
> > +#define MT9V032_DESIRED_BIN0xa5
>
> To better match the datasheet, could you call this MT9V032_AEGC_DESIRED_BIN ?
> Same comment for the related control name.
Ok, fixed for next version.
>
> > +#define MT9V032_AEC_UPDATE_INTERVAL0xa6
> > +#define MT9V032_AGC_UPDATE_INTERVAL0xa9
>
> Simalarly I'd call these two registers MT9V032_AEC_UPDATE_FREQUENCY and
> MT9V032_AGC_UPDATE_FREQUENCY as that's how they're named in the datasheet (at
> least the version I have). It makes sense to keep using interval in the
> control names though, as that's how they operate.
Yes they are called differently, fixed.
>
> Could you please keep the registers sorted numerically ?
Yes sorry.
>
> > #define MT9V032_AEC_AGC_ENABLE 0xaf
> > #defineMT9V032_AEC_ENABLE (1 << 0)
> > #defineMT9V032_AGC_ENABLE (1 << 1)
> > +#define MT9V024_AEC_MAX_SHUTTER_WIDTH 0xad
>
> As other registers specific to the MT9V024 and MT9V034 use the MT9V034
> prefix,
> could you do so here as well ?
Yes, fixed.
>
> Would it make sense to add the minimum shutter width too ?
Yes perhaps, I personally just needed the extra exposure to get the image
brighter. However I don't have any information about the minimum register. For
mt9v032 this seems to be hardwired to 1 and for mt9v024 this is just mentioned
in text without further information.
>
> > +#define MT9V032_AEC_MAX_SHUTTER_WIDTH 0xbd
> > #define MT9V032_THERMAL_INFO 0xc1
> >
> > enum mt9v032_model {
> > @@ -162,6 +169,7 @@ struct mt9v032_model_data {
> > unsigned int min_shutter;
> > unsigned int max_shutter;
> > unsigned int pclk_reg;
> > + unsigned int aec_max_shutter_reg;
> > };
> >
> > struct mt9v032_model_info {
> > @@ -185,6 +193,7 @@ static const struct mt9v032_model_data
> > mt9v032_model_data[] = { .min_shutter = MT9V032_TOTAL_SHUTTER_WIDTH_MIN,
> > .max_shutter = MT9V032_TOTAL_SHUTTER_WIDTH_MAX,
> > .pclk_reg = MT9V032_PIXEL_CLOCK,
> > + .aec_max_shutter_reg = MT9V032_AEC_MAX_SHUTTER_WIDTH,
> > }, {
> > /* MT9V024, MT9V034 */
> > .min_row_time = 690,
> > @@ -194,6 +203,7 @@ static const struct mt9v032_model_data
> > mt9v032_model_data[] = { .min_shutter = MT9V034_TOTAL_SHUTTER_WIDTH_MIN,
> > .max_shutter = MT9V034_TOTAL_SHUTTER_WIDTH_MAX,
> > .pclk_reg = MT9V034_PIXEL_CLOCK,
> > + .aec_max_shutter_reg = MT9V024_AEC_MAX_SHUTTER_WIDTH,
> > },
> > };
> >
> > @@ -265,6 +275,12 @@ struct mt9v032 {
> > struct {
> > struct v4l2_ctrl *test_pattern;
> > struct v4l2_ctrl *test_pattern_color;
> > + struct v4l2_ctrl *desired_bin;
> > + struct v4l2_ctrl *aec_lpf;
> > + struct v4l2_ctrl *agc_lpf;
> > + struct v4l2_ctrl *aec_update_interval;
> > + struct v4l2_ctrl *agc_update_interval;
> > + struct v4l2_ctrl *aec_max_shutter_width;
>
> You don't need to store all those controls in the mt9v032 structure as you
> don't use the pointers anywhere. The reason why the test_pattern and
> test_pattern_color controls are stored there is that they both affect the
> same
> register and are thus grouped into a control cluster.
Thanks, indeed, removed.
>
> > };
> > };
> >
> > @@ -643,6 +659,33 @@ static int mt9v032_set_selection(struct v4l2_subdev
> > *subdev, */
> >
> > #define V4L2_CID_TEST_PATTERN_COLOR(V4L2_CID_USER_BASE | 0x1001)
> > +/*
> > + * Value between 1 and 64 to set the desired bin. This is effectively a
> > measure
> > + * of how bright the image is supposed to be. Both AGC and AEC try to r
Re: [PATCH] [media] mt9v032: Remove duplicate test for I2C_FUNC_SMBUS_WORD_DATA functionality
Hi Axel,
Thank you for the patch.
On Sunday 10 August 2014 17:41:49 Axel Lin wrote:
> Since commit b42261078a91 ("regmap: i2c: fallback to SMBus if the adapter
> does not support standard I2C"), regmap-i2c will check the
> I2C_FUNC_SMBUS_[BYTE|WORD]_DATA functionality based on the regmap_config
> setting if the adapter does not support standard I2C.
>
> So remove the I2C_FUNC_SMBUS_WORD_DATA functionality check in the driver
> code.
>
> Signed-off-by: Axel Lin
Reviewed-by: Laurent Pinchart
and applied to my tree.
> ---
> drivers/media/i2c/mt9v032.c | 7 ---
> 1 file changed, 7 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> index d044bce..f9e4bf7 100644
> --- a/drivers/media/i2c/mt9v032.c
> +++ b/drivers/media/i2c/mt9v032.c
> @@ -879,13 +879,6 @@ static int mt9v032_probe(struct i2c_client *client,
> unsigned int i;
> int ret;
>
> - if (!i2c_check_functionality(client->adapter,
> - I2C_FUNC_SMBUS_WORD_DATA)) {
> - dev_warn(&client->adapter->dev,
> - "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
> - return -EIO;
> - }
> -
> mt9v032 = devm_kzalloc(&client->dev, sizeof(*mt9v032), GFP_KERNEL);
> if (!mt9v032)
> return -ENOMEM;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] smiapp: Remove useless rval assignment in smiapp_get_pdata()
Hi Sakari,
Thank you for the patch.
On Saturday 11 April 2015 01:42:47 Sakari Ailus wrote:
> The value is not used after the assignment.
>
> Signed-off-by: Sakari Ailus
Acked-by: Laurent Pinchart
and applied to my tree.
> ---
> drivers/media/i2c/smiapp/smiapp-core.c |4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
> b/drivers/media/i2c/smiapp/smiapp-core.c index 636ebd6..4b1e112 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -3032,10 +3032,8 @@ static struct smiapp_platform_data
> *smiapp_get_pdata(struct device *dev) pdata->op_sys_clock = devm_kcalloc(
> dev, bus_cfg->nr_of_link_frequencies + 1 /* guardian */,
> sizeof(*pdata->op_sys_clock), GFP_KERNEL);
> - if (!pdata->op_sys_clock) {
> - rval = -ENOMEM;
> + if (!pdata->op_sys_clock)
> goto out_err;
> - }
>
> for (i = 0; i < bus_cfg->nr_of_link_frequencies; i++) {
> pdata->op_sys_clock[i] = bus_cfg->link_frequencies[i];
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] mt9t001: constify v4l2_subdev_internal_ops structure
Hi Julia,
Thank you for the patch.
On Sunday 11 October 2015 13:57:13 Julia Lawall wrote:
> This v4l2_subdev_internal_ops structure is never modified. All other
> v4l2_subdev_internal_ops structures are declared as const.
>
> Done with the help of Coccinelle.
>
> Signed-off-by: Julia Lawall
Acked-by: Laurent Pinchart
and applied to my tree.
> ---
> drivers/media/i2c/mt9t001.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/mt9t001.c b/drivers/media/i2c/mt9t001.c
> index 8ae99f7..4383a5d 100644
> --- a/drivers/media/i2c/mt9t001.c
> +++ b/drivers/media/i2c/mt9t001.c
> @@ -834,7 +834,7 @@ static struct v4l2_subdev_ops mt9t001_subdev_ops = {
> .pad = &mt9t001_subdev_pad_ops,
> };
>
> -static struct v4l2_subdev_internal_ops mt9t001_subdev_internal_ops = {
> +static const struct v4l2_subdev_internal_ops mt9t001_subdev_internal_ops =
> { .registered = mt9t001_registered,
> .open = mt9t001_open,
> .close = mt9t001_close,
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] media: omap3isp: Set maximum DMA segment size
Hello everybody,
Ping ?
On Tuesday 13 October 2015 16:18:36 Laurent Pinchart wrote:
> The maximum DMA segment size controls the IOMMU mapping granularity. Its
> default value is 64kB, resulting in potentially non-contiguous IOMMU
> mappings. Configure it to 4GB to ensure that buffers get mapped
> contiguously.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/media/platform/omap3isp/isp.c | 4
> drivers/media/platform/omap3isp/isp.h | 1 +
> 2 files changed, 5 insertions(+)
>
> I'm posting this as an RFC because I'm not happy with the patch, even if it
> gets the job done.
>
> On ARM the maximum DMA segment size is used when creating IOMMU mappings. As
> a large number of devices require contiguous memory buffers (this is a very
> common requirement for video-related embedded devices) the default 64kB
> value doesn't work.
>
> I haven't investigated the history behind this API in details but I have a
> feeling something is not quite right. We force most drivers to explicitly
> set the maximum segment size from a default that seems valid for specific
> use cases only. Furthermore, as the DMA parameters are not stored directly
> in struct device this require allocation of external memory for which we
> have no proper management rule, making automatic handling of the DMA
> parameters in frameworks or helper functions cumbersome (for a discussion
> on this topic see http://www.spinics.net/lists/linux-media/msg92467.html
> and http://lists.infradead.org/pipermail/linux-arm-kernel/2014->
> November/305913.html).
>
> Is it time to fix this mess ?
>
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 17430a6ed85a..ebf7dc76e94d
> 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2444,6 +2444,10 @@ static int isp_probe(struct platform_device *pdev)
> if (ret)
> goto error;
>
> + isp->dev->dma_parms = &isp->dma_parms;
> + dma_set_max_seg_size(isp->dev, DMA_BIT_MASK(32));
> + dma_set_seg_boundary(isp->dev, 0x);
> +
> platform_set_drvdata(pdev, isp);
>
> /* Regulators */
> diff --git a/drivers/media/platform/omap3isp/isp.h
> b/drivers/media/platform/omap3isp/isp.h index e579943175c4..4b2231cf01be
> 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -193,6 +193,7 @@ struct isp_device {
> u32 syscon_offset;
> u32 phy_type;
>
> + struct device_dma_parameters dma_parms;
> struct dma_iommu_mapping *mapping;
>
> /* ISP Obj */
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] [media] mt9v032: Do not unset master_mode
Hi Laurent, On Monday 09 November 2015 14:46:42 Laurent Pinchart wrote: > Hi Markus, > > Thank you for the patch. > > On Friday 06 November 2015 14:13:44 Markus Pargmann wrote: > > The power_on function of the driver resets the chip and sets the > > CHIP_CONTROL register to 0. This switches the operating mode to slave. > > The s_stream function sets the correct mode. But this caused problems on > > a board where the camera chip is operated as master. The camera started > > after a random amount of time streaming an image, I observed between 10 > > and 300 seconds. > > > > The STRFM_OUT and STLN_OUT pins are not connected on this board which > > may cause some issues in slave mode. I could not find any documentation > > about this. > > > > Keeping the chip in master mode after the reset helped to fix this > > issue for me. > > > > Signed-off-by: Markus Pargmann > > --- > > drivers/media/i2c/mt9v032.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c > > index 4aefde9634f5..943c3f39ea73 100644 > > --- a/drivers/media/i2c/mt9v032.c > > +++ b/drivers/media/i2c/mt9v032.c > > @@ -344,7 +344,8 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032) > > if (ret < 0) > > return ret; > > > > - return regmap_write(map, MT9V032_CHIP_CONTROL, 0); > > + return regmap_write(map, MT9V032_CHIP_CONTROL, > > + MT9V032_CHIP_CONTROL_MASTER_MODE); > > This makes sense, but shouldn't you also fix the mt9v032_s_stream() function > then ? It clears the MT9V032_CHIP_CONTROL_MASTER_MODE bit when turning the > stream off. Oh yes, thanks. Will fix it for the next version. Best Regards, Markus -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | signature.asc Description: This is a digitally signed message part.
[v4l-utils PATCH 3/4] libv4l2subdev: Add a function to list library supported pixel codes
Also mark which format definitions are compat definitions for the
pre-existing codes. This way we don't end up listing the same formats
twice.
Signed-off-by: Sakari Ailus
---
utils/media-ctl/libv4l2subdev.c | 63 +++--
utils/media-ctl/v4l2subdev.h| 11 +++
2 files changed, 47 insertions(+), 27 deletions(-)
diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c
index 607e943..7e0da46 100644
--- a/utils/media-ctl/libv4l2subdev.c
+++ b/utils/media-ctl/libv4l2subdev.c
@@ -718,35 +718,36 @@ int v4l2_subdev_parse_setup_formats(struct media_device
*media, const char *p)
static struct {
const char *name;
enum v4l2_mbus_pixelcode code;
+ bool compat;
} mbus_formats[] = {
#include "media-bus-formats.h"
- { "Y8", MEDIA_BUS_FMT_Y8_1X8},
- { "Y10", MEDIA_BUS_FMT_Y10_1X10 },
- { "Y12", MEDIA_BUS_FMT_Y12_1X12 },
- { "YUYV", MEDIA_BUS_FMT_YUYV8_1X16 },
- { "YUYV1_5X8", MEDIA_BUS_FMT_YUYV8_1_5X8 },
- { "YUYV2X8", MEDIA_BUS_FMT_YUYV8_2X8 },
- { "UYVY", MEDIA_BUS_FMT_UYVY8_1X16 },
- { "UYVY1_5X8", MEDIA_BUS_FMT_UYVY8_1_5X8 },
- { "UYVY2X8", MEDIA_BUS_FMT_UYVY8_2X8 },
- { "SBGGR8", MEDIA_BUS_FMT_SBGGR8_1X8 },
- { "SGBRG8", MEDIA_BUS_FMT_SGBRG8_1X8 },
- { "SGRBG8", MEDIA_BUS_FMT_SGRBG8_1X8 },
- { "SRGGB8", MEDIA_BUS_FMT_SRGGB8_1X8 },
- { "SBGGR10", MEDIA_BUS_FMT_SBGGR10_1X10 },
- { "SGBRG10", MEDIA_BUS_FMT_SGBRG10_1X10 },
- { "SGRBG10", MEDIA_BUS_FMT_SGRBG10_1X10 },
- { "SRGGB10", MEDIA_BUS_FMT_SRGGB10_1X10 },
- { "SBGGR10_DPCM8", MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8 },
- { "SGBRG10_DPCM8", MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8 },
- { "SGRBG10_DPCM8", MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8 },
- { "SRGGB10_DPCM8", MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8 },
- { "SBGGR12", MEDIA_BUS_FMT_SBGGR12_1X12 },
- { "SGBRG12", MEDIA_BUS_FMT_SGBRG12_1X12 },
- { "SGRBG12", MEDIA_BUS_FMT_SGRBG12_1X12 },
- { "SRGGB12", MEDIA_BUS_FMT_SRGGB12_1X12 },
- { "AYUV32", MEDIA_BUS_FMT_AYUV8_1X32 },
- { "ARGB32", MEDIA_BUS_FMT_ARGB_1X32 },
+ { "Y8", MEDIA_BUS_FMT_Y8_1X8, true },
+ { "Y10", MEDIA_BUS_FMT_Y10_1X10, true },
+ { "Y12", MEDIA_BUS_FMT_Y12_1X12, true },
+ { "YUYV", MEDIA_BUS_FMT_YUYV8_1X16, true },
+ { "YUYV1_5X8", MEDIA_BUS_FMT_YUYV8_1_5X8, true },
+ { "YUYV2X8", MEDIA_BUS_FMT_YUYV8_2X8, true },
+ { "UYVY", MEDIA_BUS_FMT_UYVY8_1X16, true },
+ { "UYVY1_5X8", MEDIA_BUS_FMT_UYVY8_1_5X8, true },
+ { "UYVY2X8", MEDIA_BUS_FMT_UYVY8_2X8, true },
+ { "SBGGR8", MEDIA_BUS_FMT_SBGGR8_1X8, true },
+ { "SGBRG8", MEDIA_BUS_FMT_SGBRG8_1X8, true },
+ { "SGRBG8", MEDIA_BUS_FMT_SGRBG8_1X8, true },
+ { "SRGGB8", MEDIA_BUS_FMT_SRGGB8_1X8, true },
+ { "SBGGR10", MEDIA_BUS_FMT_SBGGR10_1X10, true },
+ { "SGBRG10", MEDIA_BUS_FMT_SGBRG10_1X10, true },
+ { "SGRBG10", MEDIA_BUS_FMT_SGRBG10_1X10, true },
+ { "SRGGB10", MEDIA_BUS_FMT_SRGGB10_1X10, true },
+ { "SBGGR10_DPCM8", MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8, true },
+ { "SGBRG10_DPCM8", MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8, true },
+ { "SGRBG10_DPCM8", MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8, true },
+ { "SRGGB10_DPCM8", MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8, true },
+ { "SBGGR12", MEDIA_BUS_FMT_SBGGR12_1X12, true },
+ { "SGBRG12", MEDIA_BUS_FMT_SGBRG12_1X12, true },
+ { "SGRBG12", MEDIA_BUS_FMT_SGRBG12_1X12, true },
+ { "SRGGB12", MEDIA_BUS_FMT_SRGGB12_1X12, true },
+ { "AYUV32", MEDIA_BUS_FMT_AYUV8_1X32, true },
+ { "ARGB32", MEDIA_BUS_FMT_ARGB_1X32, true },
};
const char *v4l2_subdev_pixelcode_to_string(enum v4l2_mbus_pixelcode code)
@@ -820,3 +821,11 @@ enum v4l2_field v4l2_subdev_string_to_field(const char
*string,
return fields[i].field;
}
+
+enum v4l2_mbus_pixelcode v4l2_subdev_pixelcode_list(unsigned int i)
+{
+ if (i >= ARRAY_SIZE(mbus_formats) || mbus_formats[i].compat)
+ return (enum v4l2_mbus_pixelcode)-1;
+
+ return mbus_formats[i].code;
+}
diff --git a/utils/media-ctl/v4l2subdev.h b/utils/media-ctl/v4l2subdev.h
index 104e420..ef8ef95 100644
--- a/utils/media-ctl/v4l2subdev.h
+++ b/utils/media-ctl/v4l2subdev.h
@@ -279,4 +279,15 @@ const char *v4l2_subdev_field_to_string(enum v4l2_field
field);
enum v4l2_field v4l2_subdev_string_to_field(const char *string,
unsigned int length);
+/**
+ * @brief Enumerate library supported media bus pixel codes.
+ * @param i - index starting from zero
+ *
+ * Enumerate pixel codes supported by libv4l2subdev starting from
+ * index 0.
+ *
+ * @return media bus pixelcode on success, -1 on failure.
+ */
+enum v4l2_mbus_pixelcode v4l2_subdev_pixelcode_list(unsigned int i);
+
#endif
--
2.1.0.231.g7484e3b
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord..
[v4l-utils PATCH 2/4] libv4l2subdev: Use generated format definitions in libv4l2subdev
Instead of manually adding each and every new media bus pixel code to
libv4l2subdev, generate the list automatically. The pre-existing formats
that do not match the list are not modified so that existing users are
unaffected by this change, with the exception of converting codes to
strings, which will use the new definitions.
Signed-off-by: Sakari Ailus
---
utils/media-ctl/.gitignore | 1 +
utils/media-ctl/Makefile.am | 8
utils/media-ctl/libv4l2subdev.c | 1 +
3 files changed, 10 insertions(+)
diff --git a/utils/media-ctl/.gitignore b/utils/media-ctl/.gitignore
index 95b6a57..8c7d576 100644
--- a/utils/media-ctl/.gitignore
+++ b/utils/media-ctl/.gitignore
@@ -1 +1,2 @@
media-ctl
+media-bus-formats.h
diff --git a/utils/media-ctl/Makefile.am b/utils/media-ctl/Makefile.am
index a3931fb..a1a9225 100644
--- a/utils/media-ctl/Makefile.am
+++ b/utils/media-ctl/Makefile.am
@@ -4,6 +4,14 @@ libmediactl_la_SOURCES = libmediactl.c mediactl-priv.h
libmediactl_la_CFLAGS = -static $(LIBUDEV_CFLAGS)
libmediactl_la_LDFLAGS = -static $(LIBUDEV_LIBS)
+media-bus-formats.h: ../../include/linux/media-bus-format.h
+ sed -e '/#define MEDIA_BUS_FMT/ ! d; s/.*FMT_//; /FIXED/ d; s/\t.*//;
s/.*/{ \"&\", MEDIA_BUS_FMT_& },/;' \
+ < $< > $@
+
+BUILT_SOURCES = media-bus-formats.h
+CLEANFILES = media-bus-formats.h
+
+nodist_libv4l2subdev_la_SOURCES = media-bus-formats.h
libv4l2subdev_la_SOURCES = libv4l2subdev.c
libv4l2subdev_la_LIBADD = libmediactl.la
libv4l2subdev_la_CFLAGS = -static
diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c
index b33d3fd..607e943 100644
--- a/utils/media-ctl/libv4l2subdev.c
+++ b/utils/media-ctl/libv4l2subdev.c
@@ -719,6 +719,7 @@ static struct {
const char *name;
enum v4l2_mbus_pixelcode code;
} mbus_formats[] = {
+#include "media-bus-formats.h"
{ "Y8", MEDIA_BUS_FMT_Y8_1X8},
{ "Y10", MEDIA_BUS_FMT_Y10_1X10 },
{ "Y12", MEDIA_BUS_FMT_Y12_1X12 },
--
2.1.0.231.g7484e3b
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[v4l-utils PATCH 0/4] List supported formats in libv4l2subdev
Hi,
This set moves libv4l2subdev to use media bus format definitions instead
of the old V4L2 mbus formats. In addition, support is added to all
existing formats, as the format list is generated instead of manually
adding formats to the list. The media-ctl test program is amended by the
list of the formats which media-ctl itself supports.
These patches go on the top of the previous set I posted ("[v4l-utils
PATCH 0/2] Add field support to libv4l2subdev").
--
Kind regards,
Sakari
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[v4l-utils PATCH 4/4] media-ctl: List supported media bus formats
Add a new topic option for -h to allow listing supported media bus codes
in conversion functions. This is useful in figuring out which media bus
codes are actually supported by the library. The numeric values of the
codes are listed as well.
Signed-off-by: Sakari Ailus
---
utils/media-ctl/options.c | 42 ++
1 file changed, 38 insertions(+), 4 deletions(-)
diff --git a/utils/media-ctl/options.c b/utils/media-ctl/options.c
index 75d7ad7..e0f1ff5 100644
--- a/utils/media-ctl/options.c
+++ b/utils/media-ctl/options.c
@@ -22,7 +22,9 @@
#include
#include
#include
+#include
#include
+#include
#include
@@ -45,7 +47,8 @@ static void usage(const char *argv0)
printf("-V, --set-v4l2 v4l2 Comma-separated list of formats to
setup\n");
printf("--get-v4l2 pad Print the active format on a given
pad\n");
printf("--set-dv padConfigure DV timings on a given pad\n");
- printf("-h, --help Show verbose help and exit\n");
+ printf("-h, --help[=topic] Show verbose help and exit\n");
+ printf("topics: mbus-fmt: List supported media
bus pixel codes\n");
printf("-i, --interactive Modify links interactively\n");
printf("-l, --links links Comma-separated list of link
descriptors to setup\n");
printf("-p, --print-topologyPrint the device topology\n");
@@ -100,7 +103,7 @@ static struct option opts[] = {
{"get-format", 1, 0, OPT_GET_FORMAT},
{"get-v4l2", 1, 0, OPT_GET_FORMAT},
{"set-dv", 1, 0, OPT_SET_DV},
- {"help", 0, 0, 'h'},
+ {"help", 2, 0, 'h'},
{"interactive", 0, 0, 'i'},
{"links", 1, 0, 'l'},
{"print-dot", 0, 0, OPT_PRINT_DOT},
@@ -109,6 +112,27 @@ static struct option opts[] = {
{"verbose", 0, 0, 'v'},
};
+void list_mbus_formats(void)
+{
+ unsigned int i;
+
+ printf("Supported media bus pixel codes\n");
+
+ for (i = 0; ; i++) {
+ unsigned int code = v4l2_subdev_pixelcode_list(i);
+ const char *str = v4l2_subdev_pixelcode_to_string(code);
+ int spaces = 30 - (int)strlen(str);
+
+ if (code == -1)
+ break;
+
+ if (spaces < 0)
+ spaces = 0;
+
+ printf("\t%s %*c (0x%8.8x)\n", str, spaces, ' ', code);
+ }
+}
+
int parse_cmdline(int argc, char **argv)
{
int opt;
@@ -119,7 +143,8 @@ int parse_cmdline(int argc, char **argv)
}
/* parse options */
- while ((opt = getopt_long(argc, argv, "d:e:f:hil:prvV:", opts, NULL))
!= -1) {
+ while ((opt = getopt_long(argc, argv, "d:e:f:h::il:prvV:",
+ opts, NULL)) != -1) {
switch (opt) {
case 'd':
media_opts.devname = optarg;
@@ -141,7 +166,16 @@ int parse_cmdline(int argc, char **argv)
break;
case 'h':
- usage(argv[0]);
+ if (optarg) {
+ if (!strcmp(optarg, "mbus-fmt"))
+ list_mbus_formats();
+ else
+ fprintf(stderr,
+ "Unknown topic \"%s\"\n",
+ optarg);
+ } else {
+ usage(argv[0]);
+ }
exit(0);
case 'i':
--
2.1.0.231.g7484e3b
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[v4l-utils PATCH 1/4] libv4l2subdev: Switch to media bus formats
No longer use the old V4L2 based MBUS_FMT definitions. Instead, use the
newer media bus format definitions.
Signed-off-by: Sakari Ailus
---
utils/media-ctl/libv4l2subdev.c | 54 -
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c
index 949eeff..b33d3fd 100644
--- a/utils/media-ctl/libv4l2subdev.c
+++ b/utils/media-ctl/libv4l2subdev.c
@@ -719,33 +719,33 @@ static struct {
const char *name;
enum v4l2_mbus_pixelcode code;
} mbus_formats[] = {
- { "Y8", V4L2_MBUS_FMT_Y8_1X8},
- { "Y10", V4L2_MBUS_FMT_Y10_1X10 },
- { "Y12", V4L2_MBUS_FMT_Y12_1X12 },
- { "YUYV", V4L2_MBUS_FMT_YUYV8_1X16 },
- { "YUYV1_5X8", V4L2_MBUS_FMT_YUYV8_1_5X8 },
- { "YUYV2X8", V4L2_MBUS_FMT_YUYV8_2X8 },
- { "UYVY", V4L2_MBUS_FMT_UYVY8_1X16 },
- { "UYVY1_5X8", V4L2_MBUS_FMT_UYVY8_1_5X8 },
- { "UYVY2X8", V4L2_MBUS_FMT_UYVY8_2X8 },
- { "SBGGR8", V4L2_MBUS_FMT_SBGGR8_1X8 },
- { "SGBRG8", V4L2_MBUS_FMT_SGBRG8_1X8 },
- { "SGRBG8", V4L2_MBUS_FMT_SGRBG8_1X8 },
- { "SRGGB8", V4L2_MBUS_FMT_SRGGB8_1X8 },
- { "SBGGR10", V4L2_MBUS_FMT_SBGGR10_1X10 },
- { "SGBRG10", V4L2_MBUS_FMT_SGBRG10_1X10 },
- { "SGRBG10", V4L2_MBUS_FMT_SGRBG10_1X10 },
- { "SRGGB10", V4L2_MBUS_FMT_SRGGB10_1X10 },
- { "SBGGR10_DPCM8", V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8 },
- { "SGBRG10_DPCM8", V4L2_MBUS_FMT_SGBRG10_DPCM8_1X8 },
- { "SGRBG10_DPCM8", V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8 },
- { "SRGGB10_DPCM8", V4L2_MBUS_FMT_SRGGB10_DPCM8_1X8 },
- { "SBGGR12", V4L2_MBUS_FMT_SBGGR12_1X12 },
- { "SGBRG12", V4L2_MBUS_FMT_SGBRG12_1X12 },
- { "SGRBG12", V4L2_MBUS_FMT_SGRBG12_1X12 },
- { "SRGGB12", V4L2_MBUS_FMT_SRGGB12_1X12 },
- { "AYUV32", V4L2_MBUS_FMT_AYUV8_1X32 },
- { "ARGB32", V4L2_MBUS_FMT_ARGB_1X32 },
+ { "Y8", MEDIA_BUS_FMT_Y8_1X8},
+ { "Y10", MEDIA_BUS_FMT_Y10_1X10 },
+ { "Y12", MEDIA_BUS_FMT_Y12_1X12 },
+ { "YUYV", MEDIA_BUS_FMT_YUYV8_1X16 },
+ { "YUYV1_5X8", MEDIA_BUS_FMT_YUYV8_1_5X8 },
+ { "YUYV2X8", MEDIA_BUS_FMT_YUYV8_2X8 },
+ { "UYVY", MEDIA_BUS_FMT_UYVY8_1X16 },
+ { "UYVY1_5X8", MEDIA_BUS_FMT_UYVY8_1_5X8 },
+ { "UYVY2X8", MEDIA_BUS_FMT_UYVY8_2X8 },
+ { "SBGGR8", MEDIA_BUS_FMT_SBGGR8_1X8 },
+ { "SGBRG8", MEDIA_BUS_FMT_SGBRG8_1X8 },
+ { "SGRBG8", MEDIA_BUS_FMT_SGRBG8_1X8 },
+ { "SRGGB8", MEDIA_BUS_FMT_SRGGB8_1X8 },
+ { "SBGGR10", MEDIA_BUS_FMT_SBGGR10_1X10 },
+ { "SGBRG10", MEDIA_BUS_FMT_SGBRG10_1X10 },
+ { "SGRBG10", MEDIA_BUS_FMT_SGRBG10_1X10 },
+ { "SRGGB10", MEDIA_BUS_FMT_SRGGB10_1X10 },
+ { "SBGGR10_DPCM8", MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8 },
+ { "SGBRG10_DPCM8", MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8 },
+ { "SGRBG10_DPCM8", MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8 },
+ { "SRGGB10_DPCM8", MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8 },
+ { "SBGGR12", MEDIA_BUS_FMT_SBGGR12_1X12 },
+ { "SGBRG12", MEDIA_BUS_FMT_SGBRG12_1X12 },
+ { "SGRBG12", MEDIA_BUS_FMT_SGRBG12_1X12 },
+ { "SRGGB12", MEDIA_BUS_FMT_SRGGB12_1X12 },
+ { "AYUV32", MEDIA_BUS_FMT_AYUV8_1X32 },
+ { "ARGB32", MEDIA_BUS_FMT_ARGB_1X32 },
};
const char *v4l2_subdev_pixelcode_to_string(enum v4l2_mbus_pixelcode code)
--
2.1.0.231.g7484e3b
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] [media] mt9v032: Add V4L2 controls for AEC and AGC
Hi Markus,
Thank you for the patch.
On Friday 06 November 2015 14:13:45 Markus Pargmann wrote:
> This patch adds V4L2 controls for Auto Exposure Control and Auto Gain
> Control settings. These settings include low pass filter, update
> frequency of these settings and the update interval for those units.
>
> Signed-off-by: Markus Pargmann
> ---
> drivers/media/i2c/mt9v032.c | 153 +
> 1 file changed, 153 insertions(+)
>
> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> index 943c3f39ea73..978ae8cbb0cc 100644
> --- a/drivers/media/i2c/mt9v032.c
> +++ b/drivers/media/i2c/mt9v032.c
> @@ -133,9 +133,16 @@
> #define MT9V032_TEST_PATTERN_GRAY_DIAGONAL (3 << 11)
> #define MT9V032_TEST_PATTERN_ENABLE (1 << 13)
> #define MT9V032_TEST_PATTERN_FLIP (1 << 14)
> +#define MT9V032_AEC_LPF 0xa8
> +#define MT9V032_AGC_LPF 0xaa
> +#define MT9V032_DESIRED_BIN 0xa5
To better match the datasheet, could you call this MT9V032_AEGC_DESIRED_BIN ?
Same comment for the related control name.
> +#define MT9V032_AEC_UPDATE_INTERVAL 0xa6
> +#define MT9V032_AGC_UPDATE_INTERVAL 0xa9
Simalarly I'd call these two registers MT9V032_AEC_UPDATE_FREQUENCY and
MT9V032_AGC_UPDATE_FREQUENCY as that's how they're named in the datasheet (at
least the version I have). It makes sense to keep using interval in the
control names though, as that's how they operate.
Could you please keep the registers sorted numerically ?
> #define MT9V032_AEC_AGC_ENABLE 0xaf
> #define MT9V032_AEC_ENABLE (1 << 0)
> #define MT9V032_AGC_ENABLE (1 << 1)
> +#define MT9V024_AEC_MAX_SHUTTER_WIDTH0xad
As other registers specific to the MT9V024 and MT9V034 use the MT9V034 prefix,
could you do so here as well ?
Would it make sense to add the minimum shutter width too ?
> +#define MT9V032_AEC_MAX_SHUTTER_WIDTH0xbd
> #define MT9V032_THERMAL_INFO 0xc1
>
> enum mt9v032_model {
> @@ -162,6 +169,7 @@ struct mt9v032_model_data {
> unsigned int min_shutter;
> unsigned int max_shutter;
> unsigned int pclk_reg;
> + unsigned int aec_max_shutter_reg;
> };
>
> struct mt9v032_model_info {
> @@ -185,6 +193,7 @@ static const struct mt9v032_model_data
> mt9v032_model_data[] = { .min_shutter = MT9V032_TOTAL_SHUTTER_WIDTH_MIN,
> .max_shutter = MT9V032_TOTAL_SHUTTER_WIDTH_MAX,
> .pclk_reg = MT9V032_PIXEL_CLOCK,
> + .aec_max_shutter_reg = MT9V032_AEC_MAX_SHUTTER_WIDTH,
> }, {
> /* MT9V024, MT9V034 */
> .min_row_time = 690,
> @@ -194,6 +203,7 @@ static const struct mt9v032_model_data
> mt9v032_model_data[] = { .min_shutter = MT9V034_TOTAL_SHUTTER_WIDTH_MIN,
> .max_shutter = MT9V034_TOTAL_SHUTTER_WIDTH_MAX,
> .pclk_reg = MT9V034_PIXEL_CLOCK,
> + .aec_max_shutter_reg = MT9V024_AEC_MAX_SHUTTER_WIDTH,
> },
> };
>
> @@ -265,6 +275,12 @@ struct mt9v032 {
> struct {
> struct v4l2_ctrl *test_pattern;
> struct v4l2_ctrl *test_pattern_color;
> + struct v4l2_ctrl *desired_bin;
> + struct v4l2_ctrl *aec_lpf;
> + struct v4l2_ctrl *agc_lpf;
> + struct v4l2_ctrl *aec_update_interval;
> + struct v4l2_ctrl *agc_update_interval;
> + struct v4l2_ctrl *aec_max_shutter_width;
You don't need to store all those controls in the mt9v032 structure as you
don't use the pointers anywhere. The reason why the test_pattern and
test_pattern_color controls are stored there is that they both affect the same
register and are thus grouped into a control cluster.
> };
> };
>
> @@ -643,6 +659,33 @@ static int mt9v032_set_selection(struct v4l2_subdev
> *subdev, */
>
> #define V4L2_CID_TEST_PATTERN_COLOR (V4L2_CID_USER_BASE | 0x1001)
> +/*
> + * Value between 1 and 64 to set the desired bin. This is effectively a
> measure
> + * of how bright the image is supposed to be. Both AGC and AEC try to reach
> + * this.
> + */
Do you know what the value represents exactly ? Does it have a linear
relationship with the overall image luminance ? Is it related to image binning
at all ?
> +#define V4L2_CID_DESIRED_BIN (V4L2_CID_USER_BASE | 0x1002)
> +/*
> + * LPF is the low pass filter capability of the chip. Both AEC and AGC have
> + * this setting. This limits the speed in which AGC/AEC adjust their
> settings.
> + * Possible values are 0-2. 0 means no LPF. For 1 and 2 this equation is
> used:
> + * if |(Calculated new exp - current exp)| > (current exp / 4)
> + * next exp = Calculated new exp
> + * else
>
Re: [PATCH v6 1/3] iommu: Implement common IOMMU ops for DMA mapping
On 04/11/15 05:12, Tomasz Figa wrote: On Wed, Nov 4, 2015 at 2:41 AM, Robin Murphy wrote: Hi Tomasz, On 02/11/15 13:43, Tomasz Figa wrote: I'd like to know what is the boundary mask and what hardware imposes requirements like this. The cost here is not only over-allocating a little, but making many, many buffers contiguously mappable on the CPU, unmappable contiguously in IOMMU, which just defeats the purpose of having an IOMMU, which I believe should be there for simple IP blocks taking one DMA address to be able to view the buffer the same way as the CPU. The expectation with dma_map_sg() is that you're either going to be iterating over the buffer segments, handing off each address to the device to process one by one; My understanding of a scatterlist was that it represents a buffer as a whole, by joining together its physically discontinuous segments. It can, but there are also cases where a single scatterlist is used to batch up multiple I/O requests - see the stuff in block/blk-merge.c as described in section 2.2 of Documentation/biodoc.txt, and AFAICS anyone could quite happily use the dmaengine API, and possibly others, in the same way. Ultimately a scatterlist is no more specific than "a list of blocks of physical memory that each want giving a DMA address". I don't see how single segments (layout of which is completely up to the allocator; often just single pages) would be usable for hardware that needs to do some work more serious than just writing a byte stream continuously to subsequent buffers. In case of such simple devices you don't even need an IOMMU (for means other than protection and/or getting over address space limitations). However, IMHO the most important use case of an IOMMU is to make buffers, which are contiguous in CPU virtual address space (VA), contiguous in device's address space (IOVA). Your implementation of dma_map_sg() effectively breaks this ability, so I'm not really following why it's located under drivers/iommu and supposed to be used with IOMMU-enabled platforms... or you have a scatter-gather-capable device, in which case you hand off the whole list at once. No need for mapping ability of the IOMMU here as well (except for working around address space issues, as I mentioned above). Ok, now I'm starting to wonder if you're wilfully choosing to miss the point. Look at 64-bit systems of any architecture, and those address space issues are pretty much the primary consideration for including an IOMMU in the first place (behind virtualisation, which we can forget about here). Take the Juno board on my desk - most of the peripherals cannot address 75% of the RAM, and CPU bounce buffers are both not overly efficient and a limited resource (try using dmatest with sufficiently large buffers to stress/measure memory bandwidth and watch it take down the kernel, and that's without any other SWIOTLB contention). The only one that really cares at all about contiguous buffers is the HDLCD, but that's perfectly happy when it calls dma_alloc_coherent() via drm_fb_cma_helper and pulls a contiguous 8MB framebuffer out of thin air, without even knowing that CMA itself is disabled and it couldn't natively address 75% of the memory that might be backing that buffer. That last point also illustrates that the thing for providing DMA-contiguous buffers is indeed very good at providing DMA-contiguous buffers when backed by an IOMMU. It's in the latter case where you have to make sure the list doesn't exceed the hardware limitations of that device. I believe the original concern was disk controllers (the introduction of dma_parms seems to originate from the linux-scsi list), but most scatter-gather engines are going to have some limit on how much they can handle per entry (IMO the dmaengine drivers are the easiest example to look at). Segment boundaries are a little more arcane, but my assumption is that they relate to the kind of devices whose addressing is not flat but relative to some separate segment register (The "64-bit" mode of USB EHCI is one concrete example I can think of) - since you cannot realistically change the segment register while the device is in the middle of accessing a single buffer entry, that entry must not fall across a segment boundary or at some point the device's accesses are going to overflow the offset address bits and wrap around to bogus addresses at the bottom of the segment. The two requirements above sound like something really specific to scatter-gather-capable hardware, which as I pointed above, barely need an IOMMU (at least its mapping capabilities). We are talking here about very IOMMU-specific code, though... Now, while I see that on some systems there might be IOMMU used for improving protection and working around addressing issues with SG-capable hardware, the code shouldn't be breaking the majority of systems with IOMMU used as the only possible way to make physically discontinuous appear (IO-virtu
Re: [PATCH 2/3] [media] mt9v032: Do not unset master_mode
Hi Markus, Thank you for the patch. On Friday 06 November 2015 14:13:44 Markus Pargmann wrote: > The power_on function of the driver resets the chip and sets the > CHIP_CONTROL register to 0. This switches the operating mode to slave. > The s_stream function sets the correct mode. But this caused problems on > a board where the camera chip is operated as master. The camera started > after a random amount of time streaming an image, I observed between 10 > and 300 seconds. > > The STRFM_OUT and STLN_OUT pins are not connected on this board which > may cause some issues in slave mode. I could not find any documentation > about this. > > Keeping the chip in master mode after the reset helped to fix this > issue for me. > > Signed-off-by: Markus Pargmann > --- > drivers/media/i2c/mt9v032.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c > index 4aefde9634f5..943c3f39ea73 100644 > --- a/drivers/media/i2c/mt9v032.c > +++ b/drivers/media/i2c/mt9v032.c > @@ -344,7 +344,8 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032) > if (ret < 0) > return ret; > > - return regmap_write(map, MT9V032_CHIP_CONTROL, 0); > + return regmap_write(map, MT9V032_CHIP_CONTROL, > + MT9V032_CHIP_CONTROL_MASTER_MODE); This makes sense, but shouldn't you also fix the mt9v032_s_stream() function then ? It clears the MT9V032_CHIP_CONTROL_MASTER_MODE bit when turning the stream off. > } > > static void mt9v032_power_off(struct mt9v032 *mt9v032) -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] [media] mt9v032: Add reset and standby gpios
Hi Markus,
Thank you for the patch.
On Friday 06 November 2015 14:13:43 Markus Pargmann wrote:
> Add optional reset and standby gpios. The reset gpio is used to reset
> the chip in power_on().
>
> The standby gpio is not used currently. It is just unset, so the chip is
> not in standby.
We could use a gpio hog for this, but given that the standby signal should
eventually get used, and given that specifying it in DT is a good hardware
description, that looks good to me.
> Signed-off-by: Markus Pargmann
> Reviewed-by: Philipp Zabel
> ---
> .../devicetree/bindings/media/i2c/mt9v032.txt | 2 ++
> drivers/media/i2c/mt9v032.c| 23 +++
> 2 files changed, 25 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt
> b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt index
> 202565313e82..100f0ae43269 100644
> --- a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt
> @@ -20,6 +20,8 @@ Optional Properties:
>
> - link-frequencies: List of allowed link frequencies in Hz. Each frequency
> is expressed as a 64-bit big-endian integer.
> +- reset-gpios: GPIO handle which is connected to the reset pin of the chip.
> +- standby-gpios: GPIO handle which is connected to the standby pin of the
> chip.
>
> For further reading on port node refer to
> Documentation/devicetree/bindings/media/video-interfaces.txt.
> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> index a68ce94ee097..4aefde9634f5 100644
> --- a/drivers/media/i2c/mt9v032.c
> +++ b/drivers/media/i2c/mt9v032.c
> @@ -24,6 +24,7 @@
> #include
> #include
> #include
> +#include
module.h escaped my vigilance, but let's try to keep headers alphabetically
sorted.
>
> #include
> #include
> @@ -251,6 +252,8 @@ struct mt9v032 {
>
> struct regmap *regmap;
> struct clk *clk;
> + struct gpio_desc *reset_gpio;
> + struct gpio_desc *standby_gpio;
>
> struct mt9v032_platform_data *pdata;
> const struct mt9v032_model_info *model;
> @@ -312,16 +315,26 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
> struct regmap *map = mt9v032->regmap;
> int ret;
>
> + gpiod_set_value_cansleep(mt9v032->reset_gpio, 1);
> +
> ret = clk_set_rate(mt9v032->clk, mt9v032->sysclk);
> if (ret < 0)
> return ret;
>
> + /* system clock has to be enabled before releasing the reset */
Nitpicking, the driver capitalizes the first letter of comments.
> ret = clk_prepare_enable(mt9v032->clk);
> if (ret)
> return ret;
>
> udelay(1);
>
> + gpiod_set_value_cansleep(mt9v032->reset_gpio, 0);
> +
> + /*
> + * After releasing reset, it can take up to 1us until the chip is done
> + */
> + udelay(1);
> +
The delay isn't necessary if there's no reset GPIO. How about
if (mt9v032->reset_gpio) {
gpiod_set_value_cansleep(mt9v032->reset_gpio, 0);
/* After releasing reset, it can take up to 1us until the
* chip is done.
*/
udelay(1);
}
And, according to the datasheet, the delay is 10 SYSCLK periods. 1µs should be
safe as the minimum SYSCLK frequency is 13 MHz. I'd still mention it in a
comment, maybe as
/* After releasing reset we need to wait 10 clock cycles
* before accessing the sensor over I2C. As the minimum SYSCLK
* frequency is 13MHz, waiting 1µs will be enough in the worst
* case.
*/
udelay(1);
If you're fine with these changes there's no need to resubmit the patch, I can
fix it when applying it to my tree.
> /* Reset the chip and stop data read out */
> ret = regmap_write(map, MT9V032_RESET, 1);
> if (ret < 0)
> @@ -954,6 +967,16 @@ static int mt9v032_probe(struct i2c_client *client,
> if (IS_ERR(mt9v032->clk))
> return PTR_ERR(mt9v032->clk);
>
> + mt9v032->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(mt9v032->reset_gpio))
> + return PTR_ERR(mt9v032->reset_gpio);
> +
> + mt9v032->standby_gpio = devm_gpiod_get_optional(&client->dev, "standby",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(mt9v032->standby_gpio))
> + return PTR_ERR(mt9v032->standby_gpio);
> +
> mutex_init(&mt9v032->power_lock);
> mt9v032->pdata = pdata;
> mt9v032->model = (const void *)did->driver_data;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: DVBSky T330 DVB-C regression Linux 4.1.12 to 4.3
Hi Stephan, I had a look at recent changes to si2168 and dvb-usb-dvbsky drivers that are the ones most relevant here. si2157 is probably not the issue here as your demod does lock. si2168: 2015-08-11[media] dvb-frontends: Drop owner assignment from i2c_driver 2015-06-09[media] dvb: Get rid of typedev usage for enums 2015-06-05[media] si2168: Implement own I2C adapter locking dvb-usb-dvbsky: 2015-10-03[media] Add Terratec H7 Revision 4 to DVBSky driver 2015-06-10[media] TS2020: Calculate tuner gain correctly 2015-06-09[media] dvb: Get rid of typedev usage for enums All these changes seem rather innocent. I've got the same tuner (sold as a TechnoTrend device) and I could try the 4.3 kernel to confirm it's still ok for DVB-T/T2 broadcasts that I've got available here, but it'll be a while as I'm travelling at the moment. Hopefully someone else can confirm before that. Cheers, -olli On 8 November 2015 at 02:37, Stephan Eisvogel wrote: > Hi Antti, hi Olli, > > I'm a Raspberry Pi 2 + TVHeadend + DVBSky T330 clone owner/user and am > observing > a regression from Linux 4.1.12 to 4.3. TVheadend is delivering this error: > > Nov 04 19:32:17 RPI2 tvheadend[714]: mpegts: 450MHz in Kabel Deutschland - > scan no data, failed > > Basically EPG and TV is full dead. I saw the > > "9cd700e m88ds3103: use own update_bits() implementation" > "a8e2219 m88ds3103: use regmap for I2C register access" > > bits that went into 4.3 recently. Anything like that applicable to the > Si2168/Si2157 USB varieties? > > Device: > > Bus 001 Device 005: ID 0572:0320 Conexant Systems (Rockwell), Inc. DVBSky > T330 DVB-T2/C tuner > > Relevant dmesg: > > [2.063240] usb 1-1.2: new high-speed USB device number 5 using dwc_otg > [2.157399] usb 1-1.2: New USB device found, idVendor=0572, > idProduct=0320 > [2.157422] usb 1-1.2: New USB device strings: Mfr=1, Product=2, > SerialNumber=3 > [2.157433] usb 1-1.2: Product: DVB-T2/C USB-Stick > [2.157443] usb 1-1.2: Manufacturer: Bestunar Inc > [2.157453] usb 1-1.2: SerialNumber: 20140126 > [6.273255] usb 1-1.2: dvb_usb_v2: found a 'DVBSky T330' in warm state > [6.273832] usb 1-1.2: dvb_usb_v2: will pass the complete MPEG2 transport > stream to the software demuxer > [6.273921] DVB: registering new adapter (DVBSky T330) > [6.275286] usb 1-1.2: dvb_usb_v2: MAC address: 00:cc:10:a5:33:0c > [6.474110] i2c i2c-3: Added multiplexed i2c bus 4 > [6.474138] si2168 3-0064: Silicon Labs Si2168 successfully attached > [6.680772] si2157 4-0060: Silicon Labs Si2147/2148/2157/2158 > successfully attached > [6.680835] usb 1-1.2: DVB: registering adapter 0 frontend 0 (Silicon > Labs Si2168)... > [6.933236] Registered IR keymap rc-dvbsky > [6.933705] input: DVBSky T330 as > /devices/platform/soc/3f98.usb/usb1/1-1/1-1.2/rc/rc0/input4 > [6.933972] rc0: DVBSky T330 as > /devices/platform/soc/3f98.usb/usb1/1-1/1-1.2/rc/rc0 > [6.933998] usb 1-1.2: dvb_usb_v2: schedule remote query interval to 300 > msecs > [6.934013] usb 1-1.2: dvb_usb_v2: 'DVBSky T330' successfully initialized > and connected > [6.934146] usbcore: registered new interface driver dvb_usb_dvbsky > [ 15.066829] si2168 3-0064: found a 'Silicon Labs Si2168-B40' > [ 15.086772] si2168 3-0064: downloading firmware from file > 'dvb-demod-si2168-b40-01.fw' > [ 16.568679] si2168 3-0064: firmware version: 4.0.19 > [ 16.579069] si2157 4-0060: found a 'Silicon Labs Si2158-A20' > [ 16.599232] si2157 4-0060: downloading firmware from file > 'dvb-tuner-si2158-a20-01.fw' > [ 17.581060] si2157 4-0060: firmware version: 2.1.6 > [ 17.581145] usb 1-1.2: DVB: adapter 0 frontend 0 frequency 0 out of range > (5500..86200) > > > Thanks for any insight! > > Best regards from Nuremberg/Germany, > Stephan > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
