Re: [PATCH v3] SoC Camera: add driver for OMAP1 camera interface
On Wednesday, 23 March 2011, at 11:00:06, Guennadi Liakhovetski wrote:
> Hi Janusz
>
> You might want to retest ams-delta with the camera on the current
> (next or
>
> git://linuxtv.org/media_tree.git staging/for_v2.6.39
>
> ) kernel - I suspect, you'll need something similar to
>
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructu
> re/30728
Hi Guennadi,
Thanks for bringing this issue to my attention. However, we alread have
something like
.dev= {
...
.coherent_dma_mask = DMA_BIT_MASK(32),
},
defined inside the platform_device structure registered for our OMAP1
camera device, so shouldn't be affected.
Anyway, I have the camera driver review/upgrade task already sitting in
my todo list for a few weeks, and hope to find some spare time to deal
with it soon, so will verify that as well.
Thanks,
Janusz
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" 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] SoC Camera: add driver for OMAP1 camera interface
Hi Janusz You might want to retest ams-delta with the camera on the current (next or git://linuxtv.org/media_tree.git staging/for_v2.6.39 ) kernel - I suspect, you'll need something similar to http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/30728 Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-omap" 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] SoC Camera: add driver for OMAP1 camera interface
Sunday 03 October 2010 04:42:53 Guennadi Liakhovetski napisał(a): > On Sat, 2 Oct 2010, Janusz Krzysztofik wrote: > > Saturday 02 October 2010 08:07:28 Guennadi Liakhovetski napisał(a): > > > Same with this one - let's take it as is and address a couple of > > > clean-ups later. > > > > Guennadi, > > Thanks for taking them both. > > > > BTW, what are your intentions about the last patch from my series still > > left not commented, "SoC Camera: add support for g_parm / s_parm > > operations", http://www.spinics.net/lists/linux-media/msg22887.html ? > > Yes, taking that one too, thanks. I see it right, that I have to apply 3 > of your patches: omap1 camera driver, ov6650 and default .[gs]_fmt for > soc_camera, the rest will go via the OMAP / ARM tree, right? Right. Janusz -- To unsubscribe from this list: send the line "unsubscribe linux-omap" 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] SoC Camera: add driver for OMAP1 camera interface
On Sat, 2 Oct 2010, Janusz Krzysztofik wrote:
> Saturday 02 October 2010 08:07:28 Guennadi Liakhovetski napisał(a):
> > Same with this one - let's take it as is and address a couple of clean-ups
> > later.
>
> Guennadi,
> Thanks for taking them both.
>
> BTW, what are your intentions about the last patch from my series still left
> not commented, "SoC Camera: add support for g_parm / s_parm operations",
> http://www.spinics.net/lists/linux-media/msg22887.html ?
Yes, taking that one too, thanks. I see it right, that I have to apply 3
of your patches: omap1 camera driver, ov6650 and default .[gs]_fmt for
soc_camera, the rest will go via the OMAP / ARM tree, right?
> > On Thu, 30 Sep 2010, Janusz Krzysztofik wrote:
> > > +static void omap1_videobuf_queue(struct videobuf_queue *vq,
> > > + struct videobuf_buffer *vb)
> > > +{
> > > + struct soc_camera_device *icd = vq->priv_data;
> > > + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> > > + struct omap1_cam_dev *pcdev = ici->priv;
> > > + struct omap1_cam_buf *buf;
> > > + u32 mode;
> > > +
> > > + list_add_tail(&vb->queue, &pcdev->capture);
> > > + vb->state = VIDEOBUF_QUEUED;
> > > +
> > > + if (pcdev->active) {
> > > + /*
> > > + * Capture in progress, so don't touch pcdev->ready even if
> > > + * empty. Since the transfer of the DMA programming register set
> > > + * content to the DMA working register set is done automatically
> > > + * by the DMA hardware, this can pretty well happen while we
> > > + * are keeping the lock here. Levae fetching it from the queue
> >
> > "Leave"
>
> Yes, sorry.
>
> > > + * to be done when a next DMA interrupt occures instead.
> > > + */
> > > + return;
> > > + }
> >
> > superfluous braces
>
> I was instructed once to do a reverse in a patch against ASoC subtree (see
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg14863.html), but
> TBH, I can't find a clear enough requirement specified in the
> Documentation/CodingStyle, so I probably change my habits, at least for you
> subtree ;).
Nah, forget it, doesn't matter.
> > > +static void videobuf_done(struct omap1_cam_dev *pcdev,
> > > + enum videobuf_state result)
> > > +{
> > > + struct omap1_cam_buf *buf = pcdev->active;
> > > + struct videobuf_buffer *vb;
> > > + struct device *dev = pcdev->icd->dev.parent;
> > > +
> > > + if (WARN_ON(!buf)) {
> > > + suspend_capture(pcdev);
> > > + disable_capture(pcdev);
> > > + return;
> > > + }
> > > +
> > > + if (result == VIDEOBUF_ERROR)
> > > + suspend_capture(pcdev);
> > > +
> > > + vb = &buf->vb;
> > > + if (waitqueue_active(&vb->done)) {
> > > + if (!pcdev->ready && result != VIDEOBUF_ERROR) {
> > > + /*
> > > + * No next buffer has been entered into the DMA
> > > + * programming register set on time (could be done only
> > > + * while the previous DMA interurpt was processed, not
> > > + * later), so the last DMA block, be it a whole buffer
> > > + * if in CONTIG or its last sgbuf if in SG mode, is
> > > + * about to be reused by the just autoreinitialized DMA
> > > + * engine, and overwritten with next frame data. Best we
> > > + * can do is stopping the capture as soon as possible,
> > > + * hopefully before the next frame start.
> > > + */
> > > + suspend_capture(pcdev);
> > > + }
> >
> > superfluous braces
>
> ditto.
>
> I'll address the issues when ready with my forementioned corner case fixes.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" 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] SoC Camera: add driver for OMAP1 camera interface
Saturday 02 October 2010 08:07:28 Guennadi Liakhovetski napisał(a):
> Same with this one - let's take it as is and address a couple of clean-ups
> later.
Guennadi,
Thanks for taking them both.
BTW, what are your intentions about the last patch from my series still left
not commented, "SoC Camera: add support for g_parm / s_parm operations",
http://www.spinics.net/lists/linux-media/msg22887.html ?
> On Thu, 30 Sep 2010, Janusz Krzysztofik wrote:
> > +static void omap1_videobuf_queue(struct videobuf_queue *vq,
> > + struct videobuf_buffer *vb)
> > +{
> > + struct soc_camera_device *icd = vq->priv_data;
> > + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> > + struct omap1_cam_dev *pcdev = ici->priv;
> > + struct omap1_cam_buf *buf;
> > + u32 mode;
> > +
> > + list_add_tail(&vb->queue, &pcdev->capture);
> > + vb->state = VIDEOBUF_QUEUED;
> > +
> > + if (pcdev->active) {
> > + /*
> > +* Capture in progress, so don't touch pcdev->ready even if
> > +* empty. Since the transfer of the DMA programming register set
> > +* content to the DMA working register set is done automatically
> > +* by the DMA hardware, this can pretty well happen while we
> > +* are keeping the lock here. Levae fetching it from the queue
>
> "Leave"
Yes, sorry.
> > +* to be done when a next DMA interrupt occures instead.
> > +*/
> > + return;
> > + }
>
> superfluous braces
I was instructed once to do a reverse in a patch against ASoC subtree (see
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg14863.html), but
TBH, I can't find a clear enough requirement specified in the
Documentation/CodingStyle, so I probably change my habits, at least for you
subtree ;).
> > +static void videobuf_done(struct omap1_cam_dev *pcdev,
> > + enum videobuf_state result)
> > +{
> > + struct omap1_cam_buf *buf = pcdev->active;
> > + struct videobuf_buffer *vb;
> > + struct device *dev = pcdev->icd->dev.parent;
> > +
> > + if (WARN_ON(!buf)) {
> > + suspend_capture(pcdev);
> > + disable_capture(pcdev);
> > + return;
> > + }
> > +
> > + if (result == VIDEOBUF_ERROR)
> > + suspend_capture(pcdev);
> > +
> > + vb = &buf->vb;
> > + if (waitqueue_active(&vb->done)) {
> > + if (!pcdev->ready && result != VIDEOBUF_ERROR) {
> > + /*
> > +* No next buffer has been entered into the DMA
> > +* programming register set on time (could be done only
> > +* while the previous DMA interurpt was processed, not
> > +* later), so the last DMA block, be it a whole buffer
> > +* if in CONTIG or its last sgbuf if in SG mode, is
> > +* about to be reused by the just autoreinitialized DMA
> > +* engine, and overwritten with next frame data. Best we
> > +* can do is stopping the capture as soon as possible,
> > +* hopefully before the next frame start.
> > +*/
> > + suspend_capture(pcdev);
> > + }
>
> superfluous braces
ditto.
I'll address the issues when ready with my forementioned corner case fixes.
Thanks,
Janusz
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" 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] SoC Camera: add driver for OMAP1 camera interface
Same with this one - let's take it as is and address a couple of clean-ups
later.
On Thu, 30 Sep 2010, Janusz Krzysztofik wrote:
> +static void omap1_videobuf_queue(struct videobuf_queue *vq,
> + struct videobuf_buffer *vb)
> +{
> + struct soc_camera_device *icd = vq->priv_data;
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct omap1_cam_dev *pcdev = ici->priv;
> + struct omap1_cam_buf *buf;
> + u32 mode;
> +
> + list_add_tail(&vb->queue, &pcdev->capture);
> + vb->state = VIDEOBUF_QUEUED;
> +
> + if (pcdev->active) {
> + /*
> + * Capture in progress, so don't touch pcdev->ready even if
> + * empty. Since the transfer of the DMA programming register set
> + * content to the DMA working register set is done automatically
> + * by the DMA hardware, this can pretty well happen while we
> + * are keeping the lock here. Levae fetching it from the queue
"Leave"
> + * to be done when a next DMA interrupt occures instead.
> + */
> + return;
> + }
superfluous braces
> +static void videobuf_done(struct omap1_cam_dev *pcdev,
> + enum videobuf_state result)
> +{
> + struct omap1_cam_buf *buf = pcdev->active;
> + struct videobuf_buffer *vb;
> + struct device *dev = pcdev->icd->dev.parent;
> +
> + if (WARN_ON(!buf)) {
> + suspend_capture(pcdev);
> + disable_capture(pcdev);
> + return;
> + }
> +
> + if (result == VIDEOBUF_ERROR)
> + suspend_capture(pcdev);
> +
> + vb = &buf->vb;
> + if (waitqueue_active(&vb->done)) {
> + if (!pcdev->ready && result != VIDEOBUF_ERROR) {
> + /*
> + * No next buffer has been entered into the DMA
> + * programming register set on time (could be done only
> + * while the previous DMA interurpt was processed, not
> + * later), so the last DMA block, be it a whole buffer
> + * if in CONTIG or its last sgbuf if in SG mode, is
> + * about to be reused by the just autoreinitialized DMA
> + * engine, and overwritten with next frame data. Best we
> + * can do is stopping the capture as soon as possible,
> + * hopefully before the next frame start.
> + */
> + suspend_capture(pcdev);
> + }
superfluous braces
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] SoC Camera: add driver for OMAP1 camera interface
This is a V4L2 driver for TI OMAP1 SoC camera interface. Both videobuf-dma versions are supported, contig and sg, selectable with a module option. The former uses less processing power, but often fails to allocate contignuous buffer memory. The latter is free of this problem, but generates tens of DMA interrupts per frame. If contig memory allocation ever fails, the driver falls back to sg automatically on next open, but still can be switched back to contig manually. Both paths work stable for me, even under heavy load, on my OMAP1510 based Amstrad Delta videophone, that is the oldest, least powerfull OMAP1 implementation. The interface generally works in pass-through mode. Since input data byte endianess can be swapped, it provides up to two v4l2 pixel formats per each of several soc_mbus formats that have their swapped endian counterparts. Boards using this driver can provide it with the following platform data: - if and what freqency clock is expected by an on-board camera sensor, - what is the maximum pixel clock that should be accepted from the sensor, - what is the polarity of the sensor provided pixel clock, - if the interface GPIO line is connected to a sensor reset/powerdown input and what is the input polarity. Created and tested against linux-2.6.36-rc5 on Amstrad Delta. Signed-off-by: Janusz Krzysztofik --- Guennadi, Since 2.6.36-rc6 is out, I've decided to submit the driver, no longer trying to make it still better or waiting for more comments, in hope it's still not too late for it to be merged during the upcomming window. The redefined set_fmt and set_crop operations work for me as expected with the last version (v3) of my ov6650 sensor driver. There are still two SG mode specific corner cases to be corrected, previously not detected because of poor sensor driver functionality: 1) frame size not exceeding one page, resulting in "unexpected end of frame" message and capture restart every frame, and 2) last sgbuf lenght less than bytes_per_line, resulting in unstable picture. I'm going to address those two with fixes. Thanks, Janusz v2->v3 changes: requested, suggested or inspired by Guennadi Liakhovetski (thanks again!): - compare enum variable against one of enum values instead of relaying on the fact that a possible value can be either 0 or not 0, - put an argument inside parenthesis in a macro definition, - CONTIG and SG are not good enough names to be defined in a header under include/..., - still better explain why suspend capture if pcdev->ready is NULL on buffer completion, - ephasize the fact and explain why not fetch new buffer from the queue immediately, rather do it only when a next DMA interrupt occures, - use more correct English wording in a few cases, - explain why not yet call videobuf_done() right after the end of current sglist is detected, but only on next DMA interrupt, - don't split strings, even on print format specifier boudries, - v2 updated subdev_call_with_sense() macro return value was no longer stored in a variable which was still examined next, - don't store 1 in a bool variable, use "true" instead, - allocate register cache dynamically based on platform resource size instead of sizing it staticaly with a predefined OMAP1_CAMERA_IOSIZE macro; move this macro out of the , suggested by Ralph Corderoy (thanks again!): - a few hex print formats still not optimal, other: - correct a few typos still found, - if a block consisting of a single command contains a comment as well, it should be enclosed in braces, - make the driver less noisy by lowering down levels of a fwe more messages, - is_dma_aligned() and dma_align() results can depend on vb_mode istead of always assuming CONTIG mode as a worth case, - redefine set_crop and set_fmt algorithms to better follow the v4l2 model, drop a few no longer used helper functions, create a new one, set_mbus_format(), from the new code common to both operations. v1->v2 changes: requested by Guennadi Liakhovetski (thanks!): - first try contig, and if it fails - fall back to sg; invalidates next two, - invalidated: Kconfig: VIDEO_OMAP1_SG: not need "if", the "depends on" should suffice, - invalidated: include both and headers, - extensively document buffer manipulations, better yet clean it up, - a copyright / licence header was missing form a header file, - need to #include if using BIT() macro, - don't need macros representing frequencies - use numbers directly, - add a few missing "static" qualifiers, - use u32 type for register content handling, - some cached registers were unnecessarily read from the hardware directly, - use true/false constants instead of 0/1 for booleans, - avoid assigning variables inside other constructs, - don't need to test if RAZ_FIFO is cleared, - no need to split "\n" to a new line, don't worry about > 80 characters, - don't increment field_count in case of a VIDEOBUF_ERROR, - adjust mbus format codes to the new names, - m
