* [PATCH v2 0/4] media: pxa_camera conversion to dmaengine @ 2015-07-05 18:27 Robert Jarzmik 2015-07-05 18:27 ` [PATCH v2 1/4] media: pxa_camera: fix the buffer free path Robert Jarzmik ` (3 more replies) 0 siblings, 4 replies; 14+ messages in thread From: Robert Jarzmik @ 2015-07-05 18:27 UTC (permalink / raw) To: Guennadi Liakhovetski, Mauro Carvalho Chehab, Jiri Kosina Cc: linux-media, linux-kernel, Robert Jarzmik Hi Guennadi, This is the next round. Most of your comments are addressed or answered, the one big thing left apart is the videobuf_sg_cut() implementation and complexity. If you have a better idea, I'm all ears. One thing that changed since v1 is that pxa_dma driver was accepted into dmaengine tree and merged, as well as the dma buffer "reusability" stuff. This begins round 2 of dmaengine conversion of pxa_camera, happy review. Cheers. -- Robert Robert Jarzmik (4): media: pxa_camera: fix the buffer free path media: pxa_camera: move interrupt to tasklet media: pxa_camera: trivial move of dma irq functions media: pxa_camera: conversion to dmaengine drivers/media/platform/soc_camera/pxa_camera.c | 522 +++++++++++++------------ 1 file changed, 267 insertions(+), 255 deletions(-) -- 2.1.4 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/4] media: pxa_camera: fix the buffer free path 2015-07-05 18:27 [PATCH v2 0/4] media: pxa_camera conversion to dmaengine Robert Jarzmik @ 2015-07-05 18:27 ` Robert Jarzmik 2015-07-12 13:58 ` Guennadi Liakhovetski 2015-07-05 18:27 ` [PATCH v2 2/4] media: pxa_camera: move interrupt to tasklet Robert Jarzmik ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Robert Jarzmik @ 2015-07-05 18:27 UTC (permalink / raw) To: Guennadi Liakhovetski, Mauro Carvalho Chehab, Jiri Kosina Cc: linux-media, linux-kernel, Robert Jarzmik, Robert Jarzmik From: Robert Jarzmik <robert.jarzmik@intel.com> Fix the error path where the video buffer wasn't allocated nor mapped. In this case, in the driver free path don't try to unmap memory which was not mapped in the first place. Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> --- drivers/media/platform/soc_camera/pxa_camera.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c index 8d6e343..3ca33f0 100644 --- a/drivers/media/platform/soc_camera/pxa_camera.c +++ b/drivers/media/platform/soc_camera/pxa_camera.c @@ -272,8 +272,8 @@ static void free_buffer(struct videobuf_queue *vq, struct pxa_buffer *buf) * longer in STATE_QUEUED or STATE_ACTIVE */ videobuf_waiton(vq, &buf->vb, 0, 0); - videobuf_dma_unmap(vq->dev, dma); - videobuf_dma_free(dma); + if (buf->vb.state == VIDEOBUF_NEEDS_INIT) + return; for (i = 0; i < ARRAY_SIZE(buf->dmas); i++) { if (buf->dmas[i].sg_cpu) @@ -283,6 +283,8 @@ static void free_buffer(struct videobuf_queue *vq, struct pxa_buffer *buf) buf->dmas[i].sg_dma); buf->dmas[i].sg_cpu = NULL; } + videobuf_dma_unmap(vq->dev, dma); + videobuf_dma_free(dma); buf->vb.state = VIDEOBUF_NEEDS_INIT; } -- 2.1.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/4] media: pxa_camera: fix the buffer free path 2015-07-05 18:27 ` [PATCH v2 1/4] media: pxa_camera: fix the buffer free path Robert Jarzmik @ 2015-07-12 13:58 ` Guennadi Liakhovetski 2015-07-12 14:35 ` Robert Jarzmik 0 siblings, 1 reply; 14+ messages in thread From: Guennadi Liakhovetski @ 2015-07-12 13:58 UTC (permalink / raw) To: Robert Jarzmik Cc: Mauro Carvalho Chehab, Jiri Kosina, linux-media, linux-kernel, Robert Jarzmik Hi Robert, On Sun, 5 Jul 2015, Robert Jarzmik wrote: > From: Robert Jarzmik <robert.jarzmik@intel.com> > > Fix the error path where the video buffer wasn't allocated nor > mapped. In this case, in the driver free path don't try to unmap memory > which was not mapped in the first place. Have I missed your reply to my comments to v1 of this patch? This one seems to be its exact copy? Thanks Guennadi > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > --- > drivers/media/platform/soc_camera/pxa_camera.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c > index 8d6e343..3ca33f0 100644 > --- a/drivers/media/platform/soc_camera/pxa_camera.c > +++ b/drivers/media/platform/soc_camera/pxa_camera.c > @@ -272,8 +272,8 @@ static void free_buffer(struct videobuf_queue *vq, struct pxa_buffer *buf) > * longer in STATE_QUEUED or STATE_ACTIVE > */ > videobuf_waiton(vq, &buf->vb, 0, 0); > - videobuf_dma_unmap(vq->dev, dma); > - videobuf_dma_free(dma); > + if (buf->vb.state == VIDEOBUF_NEEDS_INIT) > + return; > > for (i = 0; i < ARRAY_SIZE(buf->dmas); i++) { > if (buf->dmas[i].sg_cpu) > @@ -283,6 +283,8 @@ static void free_buffer(struct videobuf_queue *vq, struct pxa_buffer *buf) > buf->dmas[i].sg_dma); > buf->dmas[i].sg_cpu = NULL; > } > + videobuf_dma_unmap(vq->dev, dma); > + videobuf_dma_free(dma); > > buf->vb.state = VIDEOBUF_NEEDS_INIT; > } > -- > 2.1.4 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/4] media: pxa_camera: fix the buffer free path 2015-07-12 13:58 ` Guennadi Liakhovetski @ 2015-07-12 14:35 ` Robert Jarzmik 0 siblings, 0 replies; 14+ messages in thread From: Robert Jarzmik @ 2015-07-12 14:35 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Mauro Carvalho Chehab, Jiri Kosina, linux-media, linux-kernel, Robert Jarzmik Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: > Hi Robert, > > On Sun, 5 Jul 2015, Robert Jarzmik wrote: > >> From: Robert Jarzmik <robert.jarzmik@intel.com> >> >> Fix the error path where the video buffer wasn't allocated nor >> mapped. In this case, in the driver free path don't try to unmap memory >> which was not mapped in the first place. > > Have I missed your reply to my comments to v1 of this patch? This one > seems to be its exact copy? Yeah, that's because I don't have any ... reply from you on v1. At least I don't remember it and in [1] I don't see it. Cheers. -- Robert [1] http://www.spinics.net/lists/linux-media/msg88021.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/4] media: pxa_camera: move interrupt to tasklet 2015-07-05 18:27 [PATCH v2 0/4] media: pxa_camera conversion to dmaengine Robert Jarzmik 2015-07-05 18:27 ` [PATCH v2 1/4] media: pxa_camera: fix the buffer free path Robert Jarzmik @ 2015-07-05 18:27 ` Robert Jarzmik 2015-07-05 18:27 ` [PATCH v2 3/4] media: pxa_camera: trivial move of dma irq functions Robert Jarzmik 2015-07-05 18:27 ` [PATCH v2 4/4] media: pxa_camera: conversion to dmaengine Robert Jarzmik 3 siblings, 0 replies; 14+ messages in thread From: Robert Jarzmik @ 2015-07-05 18:27 UTC (permalink / raw) To: Guennadi Liakhovetski, Mauro Carvalho Chehab, Jiri Kosina Cc: linux-media, linux-kernel, Robert Jarzmik From: Robert Jarzmik <robert.jarzmik@intel.com> In preparation for dmaengine conversion, move the camera interrupt handling into a tasklet. This won't change the global flow, as this interrupt is only used to detect the end of frame and activate DMA fifos handling. Signed-off-by: Robert Jarzmik <robert.jarzmik@intel.com> --- drivers/media/platform/soc_camera/pxa_camera.c | 44 +++++++++++++++++--------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c index 3ca33f0..c0c0f0f 100644 --- a/drivers/media/platform/soc_camera/pxa_camera.c +++ b/drivers/media/platform/soc_camera/pxa_camera.c @@ -223,6 +223,7 @@ struct pxa_camera_dev { struct pxa_buffer *active; struct pxa_dma_desc *sg_tail[3]; + struct tasklet_struct task_eof; u32 save_cicr[5]; }; @@ -605,6 +606,7 @@ static void pxa_camera_start_capture(struct pxa_camera_dev *pcdev) unsigned long cicr0; dev_dbg(pcdev->soc_host.v4l2_dev.dev, "%s\n", __func__); + __raw_writel(__raw_readl(pcdev->base + CISR), pcdev->base + CISR); /* Enable End-Of-Frame Interrupt */ cicr0 = __raw_readl(pcdev->base + CICR0) | CICR0_ENB; cicr0 &= ~CICR0_EOFM; @@ -922,13 +924,35 @@ static void pxa_camera_deactivate(struct pxa_camera_dev *pcdev) clk_disable_unprepare(pcdev->clk); } -static irqreturn_t pxa_camera_irq(int irq, void *data) +static void pxa_camera_eof(unsigned long arg) { - struct pxa_camera_dev *pcdev = data; - unsigned long status, cifr, cicr0; + struct pxa_camera_dev *pcdev = (struct pxa_camera_dev *)arg; + unsigned long cifr; struct pxa_buffer *buf; struct videobuf_buffer *vb; + dev_dbg(pcdev->soc_host.v4l2_dev.dev, + "Camera interrupt status 0x%x\n", + __raw_readl(pcdev->base + CISR)); + + /* Reset the FIFOs */ + cifr = __raw_readl(pcdev->base + CIFR) | CIFR_RESET_F; + __raw_writel(cifr, pcdev->base + CIFR); + + pcdev->active = list_first_entry(&pcdev->capture, + struct pxa_buffer, vb.queue); + vb = &pcdev->active->vb; + buf = container_of(vb, struct pxa_buffer, vb); + pxa_videobuf_set_actdma(pcdev, buf); + + pxa_dma_start_channels(pcdev); +} + +static irqreturn_t pxa_camera_irq(int irq, void *data) +{ + struct pxa_camera_dev *pcdev = data; + unsigned long status, cicr0; + status = __raw_readl(pcdev->base + CISR); dev_dbg(pcdev->soc_host.v4l2_dev.dev, "Camera interrupt status 0x%lx\n", status); @@ -939,20 +963,9 @@ static irqreturn_t pxa_camera_irq(int irq, void *data) __raw_writel(status, pcdev->base + CISR); if (status & CISR_EOF) { - /* Reset the FIFOs */ - cifr = __raw_readl(pcdev->base + CIFR) | CIFR_RESET_F; - __raw_writel(cifr, pcdev->base + CIFR); - - pcdev->active = list_first_entry(&pcdev->capture, - struct pxa_buffer, vb.queue); - vb = &pcdev->active->vb; - buf = container_of(vb, struct pxa_buffer, vb); - pxa_videobuf_set_actdma(pcdev, buf); - - pxa_dma_start_channels(pcdev); - cicr0 = __raw_readl(pcdev->base + CICR0) | CICR0_EOFM; __raw_writel(cicr0, pcdev->base + CICR0); + tasklet_schedule(&pcdev->task_eof); } return IRQ_HANDLED; @@ -1834,6 +1847,7 @@ static int pxa_camera_probe(struct platform_device *pdev) pcdev->soc_host.priv = pcdev; pcdev->soc_host.v4l2_dev.dev = &pdev->dev; pcdev->soc_host.nr = pdev->id; + tasklet_init(&pcdev->task_eof, pxa_camera_eof, (unsigned long)pcdev); err = soc_camera_host_register(&pcdev->soc_host); if (err) -- 2.1.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 3/4] media: pxa_camera: trivial move of dma irq functions 2015-07-05 18:27 [PATCH v2 0/4] media: pxa_camera conversion to dmaengine Robert Jarzmik 2015-07-05 18:27 ` [PATCH v2 1/4] media: pxa_camera: fix the buffer free path Robert Jarzmik 2015-07-05 18:27 ` [PATCH v2 2/4] media: pxa_camera: move interrupt to tasklet Robert Jarzmik @ 2015-07-05 18:27 ` Robert Jarzmik 2015-07-12 14:06 ` Guennadi Liakhovetski 2015-07-05 18:27 ` [PATCH v2 4/4] media: pxa_camera: conversion to dmaengine Robert Jarzmik 3 siblings, 1 reply; 14+ messages in thread From: Robert Jarzmik @ 2015-07-05 18:27 UTC (permalink / raw) To: Guennadi Liakhovetski, Mauro Carvalho Chehab, Jiri Kosina Cc: linux-media, linux-kernel, Robert Jarzmik From: Robert Jarzmik <robert.jarzmik@intel.com> This moves the dma irq handling functions up in the source file, so that they are available before DMA preparation functions. It prepares the conversion to DMA engine, where the descriptors are populated with these functions as callbacks. Signed-off-by: Robert Jarzmik <robert.jarzmik@intel.com> --- Since v1: fixed prototypes change --- drivers/media/platform/soc_camera/pxa_camera.c | 40 ++++++++++++++------------ 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c index c0c0f0f..1ab4f9d 100644 --- a/drivers/media/platform/soc_camera/pxa_camera.c +++ b/drivers/media/platform/soc_camera/pxa_camera.c @@ -311,6 +311,28 @@ static int calculate_dma_sglen(struct scatterlist *sglist, int sglen, BUG_ON(size != 0); return i + 1; +static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, + enum pxa_camera_active_dma act_dma); + +static void pxa_camera_dma_irq_y(int channel, void *data) +{ + struct pxa_camera_dev *pcdev = data; + + pxa_camera_dma_irq(pcdev, DMA_Y); +} + +static void pxa_camera_dma_irq_u(int channel, void *data) +{ + struct pxa_camera_dev *pcdev = data; + + pxa_camera_dma_irq(pcdev, DMA_U); +} + +static void pxa_camera_dma_irq_v(int channel, void *data) +{ + struct pxa_camera_dev *pcdev = data; + + pxa_camera_dma_irq(pcdev, DMA_V); } /** @@ -810,24 +832,6 @@ out: spin_unlock_irqrestore(&pcdev->lock, flags); } -static void pxa_camera_dma_irq_y(int channel, void *data) -{ - struct pxa_camera_dev *pcdev = data; - pxa_camera_dma_irq(channel, pcdev, DMA_Y); -} - -static void pxa_camera_dma_irq_u(int channel, void *data) -{ - struct pxa_camera_dev *pcdev = data; - pxa_camera_dma_irq(channel, pcdev, DMA_U); -} - -static void pxa_camera_dma_irq_v(int channel, void *data) -{ - struct pxa_camera_dev *pcdev = data; - pxa_camera_dma_irq(channel, pcdev, DMA_V); -} - static struct videobuf_queue_ops pxa_videobuf_ops = { .buf_setup = pxa_videobuf_setup, .buf_prepare = pxa_videobuf_prepare, -- 2.1.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/4] media: pxa_camera: trivial move of dma irq functions 2015-07-05 18:27 ` [PATCH v2 3/4] media: pxa_camera: trivial move of dma irq functions Robert Jarzmik @ 2015-07-12 14:06 ` Guennadi Liakhovetski 2015-07-12 14:32 ` Robert Jarzmik 0 siblings, 1 reply; 14+ messages in thread From: Guennadi Liakhovetski @ 2015-07-12 14:06 UTC (permalink / raw) To: Robert Jarzmik Cc: Mauro Carvalho Chehab, Jiri Kosina, linux-media, linux-kernel, Robert Jarzmik On Sun, 5 Jul 2015, Robert Jarzmik wrote: > From: Robert Jarzmik <robert.jarzmik@intel.com> > > This moves the dma irq handling functions up in the source file, so that > they are available before DMA preparation functions. It prepares the > conversion to DMA engine, where the descriptors are populated with these > functions as callbacks. > > Signed-off-by: Robert Jarzmik <robert.jarzmik@intel.com> > --- > Since v1: fixed prototypes change > --- > drivers/media/platform/soc_camera/pxa_camera.c | 40 ++++++++++++++------------ > 1 file changed, 22 insertions(+), 18 deletions(-) > > diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c > index c0c0f0f..1ab4f9d 100644 > --- a/drivers/media/platform/soc_camera/pxa_camera.c > +++ b/drivers/media/platform/soc_camera/pxa_camera.c > @@ -311,6 +311,28 @@ static int calculate_dma_sglen(struct scatterlist *sglist, int sglen, > > BUG_ON(size != 0); > return i + 1; > +static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, > + enum pxa_camera_active_dma act_dma); Yes, functions look ok now as the sense - they are just moved up with no modifications, but the patch itself looks as broken to me as it originally was... Please, look 2 lines up - where you add your lines. Thanks Guennadi > + > +static void pxa_camera_dma_irq_y(int channel, void *data) > +{ > + struct pxa_camera_dev *pcdev = data; > + > + pxa_camera_dma_irq(pcdev, DMA_Y); > +} > + > +static void pxa_camera_dma_irq_u(int channel, void *data) > +{ > + struct pxa_camera_dev *pcdev = data; > + > + pxa_camera_dma_irq(pcdev, DMA_U); > +} > + > +static void pxa_camera_dma_irq_v(int channel, void *data) > +{ > + struct pxa_camera_dev *pcdev = data; > + > + pxa_camera_dma_irq(pcdev, DMA_V); > } > > /** > @@ -810,24 +832,6 @@ out: > spin_unlock_irqrestore(&pcdev->lock, flags); > } > > -static void pxa_camera_dma_irq_y(int channel, void *data) > -{ > - struct pxa_camera_dev *pcdev = data; > - pxa_camera_dma_irq(channel, pcdev, DMA_Y); > -} > - > -static void pxa_camera_dma_irq_u(int channel, void *data) > -{ > - struct pxa_camera_dev *pcdev = data; > - pxa_camera_dma_irq(channel, pcdev, DMA_U); > -} > - > -static void pxa_camera_dma_irq_v(int channel, void *data) > -{ > - struct pxa_camera_dev *pcdev = data; > - pxa_camera_dma_irq(channel, pcdev, DMA_V); > -} > - > static struct videobuf_queue_ops pxa_videobuf_ops = { > .buf_setup = pxa_videobuf_setup, > .buf_prepare = pxa_videobuf_prepare, > -- > 2.1.4 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/4] media: pxa_camera: trivial move of dma irq functions 2015-07-12 14:06 ` Guennadi Liakhovetski @ 2015-07-12 14:32 ` Robert Jarzmik 0 siblings, 0 replies; 14+ messages in thread From: Robert Jarzmik @ 2015-07-12 14:32 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Mauro Carvalho Chehab, Jiri Kosina, linux-media, linux-kernel, Robert Jarzmik Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: >> diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c >> index c0c0f0f..1ab4f9d 100644 >> --- a/drivers/media/platform/soc_camera/pxa_camera.c >> +++ b/drivers/media/platform/soc_camera/pxa_camera.c >> @@ -311,6 +311,28 @@ static int calculate_dma_sglen(struct scatterlist *sglist, int sglen, >> >> BUG_ON(size != 0); >> return i + 1; >> +static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, >> + enum pxa_camera_active_dma act_dma); > > Yes, functions look ok now as the sense - they are just moved up with no > modifications, but the patch itself looks as broken to me as it originally > was... Please, look 2 lines up - where you add your lines. Indeed. I don't know how that could happen. I would have sworn they were in the correct place, then I rebased / solved conflicts, and ... messed up. That would be for the v3, and I'll inspect the mails before sending. Cheers. -- Robert ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 4/4] media: pxa_camera: conversion to dmaengine 2015-07-05 18:27 [PATCH v2 0/4] media: pxa_camera conversion to dmaengine Robert Jarzmik ` (2 preceding siblings ...) 2015-07-05 18:27 ` [PATCH v2 3/4] media: pxa_camera: trivial move of dma irq functions Robert Jarzmik @ 2015-07-05 18:27 ` Robert Jarzmik 2015-07-12 17:05 ` Guennadi Liakhovetski 3 siblings, 1 reply; 14+ messages in thread From: Robert Jarzmik @ 2015-07-05 18:27 UTC (permalink / raw) To: Guennadi Liakhovetski, Mauro Carvalho Chehab, Jiri Kosina Cc: linux-media, linux-kernel, Robert Jarzmik Convert pxa_camera to dmaengine. This removes all DMA registers manipulation in favor of the more generic dmaengine API. The functional level should be the same as before. The biggest change is in the videobuf_sg_splice() function, which splits a videobuf-dma into several scatterlists for 3 planes captures (Y, U, V). Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> --- Since v1: Guennadi's fixes dma tasklet functions prototypes change (trivial move) --- drivers/media/platform/soc_camera/pxa_camera.c | 438 ++++++++++++------------- 1 file changed, 215 insertions(+), 223 deletions(-) diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c index 1ab4f9d..76b2b7b 100644 --- a/drivers/media/platform/soc_camera/pxa_camera.c +++ b/drivers/media/platform/soc_camera/pxa_camera.c @@ -28,6 +28,9 @@ #include <linux/clk.h> #include <linux/sched.h> #include <linux/slab.h> +#include <linux/dmaengine.h> +#include <linux/dma-mapping.h> +#include <linux/dma/pxa-dma.h> #include <media/v4l2-common.h> #include <media/v4l2-dev.h> @@ -38,7 +41,6 @@ #include <linux/videodev2.h> -#include <mach/dma.h> #include <linux/platform_data/camera-pxa.h> #define PXA_CAM_VERSION "0.0.6" @@ -175,21 +177,16 @@ enum pxa_camera_active_dma { DMA_V = 0x4, }; -/* descriptor needed for the PXA DMA engine */ -struct pxa_cam_dma { - dma_addr_t sg_dma; - struct pxa_dma_desc *sg_cpu; - size_t sg_size; - int sglen; -}; - /* buffer for one video frame */ struct pxa_buffer { /* common v4l buffer stuff -- must be first */ struct videobuf_buffer vb; u32 code; /* our descriptor lists for Y, U and V channels */ - struct pxa_cam_dma dmas[3]; + struct dma_async_tx_descriptor *descs[3]; + dma_cookie_t cookie[3]; + struct scatterlist *sg[3]; + int sg_len[3]; int inwork; enum pxa_camera_active_dma active_dma; }; @@ -207,7 +204,7 @@ struct pxa_camera_dev { void __iomem *base; int channels; - unsigned int dma_chans[3]; + struct dma_chan *dma_chans[3]; struct pxacamera_platform_data *pdata; struct resource *res; @@ -222,7 +219,6 @@ struct pxa_camera_dev { spinlock_t lock; struct pxa_buffer *active; - struct pxa_dma_desc *sg_tail[3]; struct tasklet_struct task_eof; u32 save_cicr[5]; @@ -259,7 +255,6 @@ static int pxa_videobuf_setup(struct videobuf_queue *vq, unsigned int *count, static void free_buffer(struct videobuf_queue *vq, struct pxa_buffer *buf) { struct soc_camera_device *icd = vq->priv_data; - struct soc_camera_host *ici = to_soc_camera_host(icd->parent); struct videobuf_dmabuf *dma = videobuf_to_dma(&buf->vb); int i; @@ -276,59 +271,99 @@ static void free_buffer(struct videobuf_queue *vq, struct pxa_buffer *buf) if (buf->vb.state == VIDEOBUF_NEEDS_INIT) return; - for (i = 0; i < ARRAY_SIZE(buf->dmas); i++) { - if (buf->dmas[i].sg_cpu) - dma_free_coherent(ici->v4l2_dev.dev, - buf->dmas[i].sg_size, - buf->dmas[i].sg_cpu, - buf->dmas[i].sg_dma); - buf->dmas[i].sg_cpu = NULL; + for (i = 0; i < 3 && buf->descs[i]; i++) { + async_tx_ack(buf->descs[i]); + kfree(buf->sg[i]); + buf->descs[i] = NULL; + buf->sg[i] = NULL; + buf->sg_len[i] = 0; } videobuf_dma_unmap(vq->dev, dma); videobuf_dma_free(dma); buf->vb.state = VIDEOBUF_NEEDS_INIT; + + dev_dbg(icd->parent, "%s end (vb=0x%p) 0x%08lx %d\n", __func__, + &buf->vb, buf->vb.baddr, buf->vb.bsize); } -static int calculate_dma_sglen(struct scatterlist *sglist, int sglen, - int sg_first_ofs, int size) +static struct scatterlist *videobuf_sg_cut(struct scatterlist *sglist, + int sglen, int offset, int size, + int *new_sg_len) { - int i, offset, dma_len, xfer_len; - struct scatterlist *sg; + struct scatterlist *sg0, *sg, *sg_first = NULL; + int i, dma_len, dropped_xfer_len, dropped_remain, remain; + int nfirst = -1, nfirst_offset = 0, xfer_len; - offset = sg_first_ofs; + *new_sg_len = 0; + dropped_remain = offset; + remain = size; for_each_sg(sglist, sg, sglen, i) { dma_len = sg_dma_len(sg); - /* PXA27x Developer's Manual 27.4.4.1: round up to 8 bytes */ - xfer_len = roundup(min(dma_len - offset, size), 8); + dropped_xfer_len = roundup(min(dma_len, dropped_remain), 8); + if (dropped_remain) + dropped_remain -= dropped_xfer_len; + xfer_len = dma_len - dropped_xfer_len; + + if (nfirst < 0 && xfer_len > 0) { + sg_first = sg; + nfirst = i; + nfirst_offset = dropped_xfer_len; + } + if (xfer_len > 0) { + (*new_sg_len)++; + remain -= xfer_len; + } + if (remain <= 0) + break; + } + WARN_ON(nfirst >= sglen); - size = max(0, size - xfer_len); - offset = 0; - if (size == 0) + sg0 = kmalloc_array(*new_sg_len, sizeof(struct scatterlist), + GFP_KERNEL); + if (!sg0) + return NULL; + + remain = size; + for_each_sg(sg_first, sg, *new_sg_len, i) { + dma_len = sg_dma_len(sg); + sg0[i] = *sg; + + sg0[i].offset = nfirst_offset; + nfirst_offset = 0; + + xfer_len = min_t(int, remain, dma_len - sg0[i].offset); + xfer_len = roundup(xfer_len, 8); + sg_dma_len(&sg0[i]) = xfer_len; + + remain -= xfer_len; + if (remain <= 0) { + sg_mark_end(&sg0[i]); break; + } } - BUG_ON(size != 0); - return i + 1; + return sg0; +} static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, enum pxa_camera_active_dma act_dma); -static void pxa_camera_dma_irq_y(int channel, void *data) +static void pxa_camera_dma_irq_y(void *data) { struct pxa_camera_dev *pcdev = data; pxa_camera_dma_irq(pcdev, DMA_Y); } -static void pxa_camera_dma_irq_u(int channel, void *data) +static void pxa_camera_dma_irq_u(void *data) { struct pxa_camera_dev *pcdev = data; pxa_camera_dma_irq(pcdev, DMA_U); } -static void pxa_camera_dma_irq_v(int channel, void *data) +static void pxa_camera_dma_irq_v(void *data) { struct pxa_camera_dev *pcdev = data; @@ -343,93 +378,59 @@ static void pxa_camera_dma_irq_v(int channel, void *data) * @channel: dma channel (0 => 'Y', 1 => 'U', 2 => 'V') * @cibr: camera Receive Buffer Register * @size: bytes to transfer - * @sg_first: first element of sg_list - * @sg_first_ofs: offset in first element of sg_list + * @offset: offset in videobuffer of the first byte to transfer * * Prepares the pxa dma descriptors to transfer one camera channel. - * Beware sg_first and sg_first_ofs are both input and output parameters. * - * Returns 0 or -ENOMEM if no coherent memory is available + * Returns 0 if success or -ENOMEM if no memory is available */ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev, struct pxa_buffer *buf, struct videobuf_dmabuf *dma, int channel, - int cibr, int size, - struct scatterlist **sg_first, int *sg_first_ofs) + int cibr, int size, int offset) { - struct pxa_cam_dma *pxa_dma = &buf->dmas[channel]; - struct device *dev = pcdev->soc_host.v4l2_dev.dev; + struct dma_chan *dma_chan = pcdev->dma_chans[channel]; struct scatterlist *sg; - int i, offset, sglen; - int dma_len = 0, xfer_len = 0; - - if (pxa_dma->sg_cpu) - dma_free_coherent(dev, pxa_dma->sg_size, - pxa_dma->sg_cpu, pxa_dma->sg_dma); - - sglen = calculate_dma_sglen(*sg_first, dma->sglen, - *sg_first_ofs, size); - - pxa_dma->sg_size = (sglen + 1) * sizeof(struct pxa_dma_desc); - pxa_dma->sg_cpu = dma_alloc_coherent(dev, pxa_dma->sg_size, - &pxa_dma->sg_dma, GFP_KERNEL); - if (!pxa_dma->sg_cpu) - return -ENOMEM; - - pxa_dma->sglen = sglen; - offset = *sg_first_ofs; - - dev_dbg(dev, "DMA: sg_first=%p, sglen=%d, ofs=%d, dma.desc=%x\n", - *sg_first, sglen, *sg_first_ofs, pxa_dma->sg_dma); - - - for_each_sg(*sg_first, sg, sglen, i) { - dma_len = sg_dma_len(sg); - - /* PXA27x Developer's Manual 27.4.4.1: round up to 8 bytes */ - xfer_len = roundup(min(dma_len - offset, size), 8); - - size = max(0, size - xfer_len); - - pxa_dma->sg_cpu[i].dsadr = pcdev->res->start + cibr; - pxa_dma->sg_cpu[i].dtadr = sg_dma_address(sg) + offset; - pxa_dma->sg_cpu[i].dcmd = - DCMD_FLOWSRC | DCMD_BURST8 | DCMD_INCTRGADDR | xfer_len; -#ifdef DEBUG - if (!i) - pxa_dma->sg_cpu[i].dcmd |= DCMD_STARTIRQEN; -#endif - pxa_dma->sg_cpu[i].ddadr = - pxa_dma->sg_dma + (i + 1) * sizeof(struct pxa_dma_desc); - - dev_vdbg(dev, "DMA: desc.%08x->@phys=0x%08x, len=%d\n", - pxa_dma->sg_dma + i * sizeof(struct pxa_dma_desc), - sg_dma_address(sg) + offset, xfer_len); - offset = 0; - - if (size == 0) - break; + int sglen; + struct dma_async_tx_descriptor *tx; + + sg = videobuf_sg_cut(dma->sglist, dma->sglen, offset, size, &sglen); + if (!sg) + goto fail; + + tx = dmaengine_prep_slave_sg(dma_chan, sg, sglen, DMA_DEV_TO_MEM, + DMA_PREP_INTERRUPT | DMA_CTRL_ACK); + if (!tx) { + dev_err(pcdev->soc_host.v4l2_dev.dev, + "dmaengine_prep_slave_sg failed\n"); + goto fail; } - pxa_dma->sg_cpu[sglen].ddadr = DDADR_STOP; - pxa_dma->sg_cpu[sglen].dcmd = DCMD_FLOWSRC | DCMD_BURST8 | DCMD_ENDIRQEN; - - /* - * Handle 1 special case : - * - in 3 planes (YUV422P format), we might finish with xfer_len equal - * to dma_len (end on PAGE boundary). In this case, the sg element - * for next plane should be the next after the last used to store the - * last scatter gather RAM page - */ - if (xfer_len >= dma_len) { - *sg_first_ofs = xfer_len - dma_len; - *sg_first = sg_next(sg); - } else { - *sg_first_ofs = xfer_len; - *sg_first = sg; + tx->callback_param = pcdev; + switch (channel) { + case 0: + tx->callback = pxa_camera_dma_irq_y; + break; + case 1: + tx->callback = pxa_camera_dma_irq_u; + break; + case 2: + tx->callback = pxa_camera_dma_irq_v; + break; } + buf->descs[channel] = tx; + buf->sg[channel] = sg; + buf->sg_len[channel] = sglen; return 0; +fail: + kfree(sg); + + dev_dbg(pcdev->soc_host.v4l2_dev.dev, + "%s (vb=0x%p) dma_tx=%p\n", + __func__, &buf->vb, tx); + + return -ENOMEM; } static void pxa_videobuf_set_actdma(struct pxa_camera_dev *pcdev, @@ -498,9 +499,7 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq, if (vb->state == VIDEOBUF_NEEDS_INIT) { int size = vb->size; - int next_ofs = 0; struct videobuf_dmabuf *dma = videobuf_to_dma(vb); - struct scatterlist *sg; ret = videobuf_iolock(vq, vb, NULL); if (ret) @@ -513,11 +512,9 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq, size_y = size; } - sg = dma->sglist; - /* init DMA for Y channel */ - ret = pxa_init_dma_channel(pcdev, buf, dma, 0, CIBR0, size_y, - &sg, &next_ofs); + ret = pxa_init_dma_channel(pcdev, buf, dma, 0, CIBR0, + size_y, 0); if (ret) { dev_err(dev, "DMA initialization for Y/RGB failed\n"); goto fail; @@ -526,19 +523,19 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq, /* init DMA for U channel */ if (size_u) ret = pxa_init_dma_channel(pcdev, buf, dma, 1, CIBR1, - size_u, &sg, &next_ofs); + size_u, size_y); if (ret) { dev_err(dev, "DMA initialization for U failed\n"); - goto fail_u; + goto fail; } /* init DMA for V channel */ if (size_v) ret = pxa_init_dma_channel(pcdev, buf, dma, 2, CIBR2, - size_v, &sg, &next_ofs); + size_v, size_y + size_u); if (ret) { dev_err(dev, "DMA initialization for V failed\n"); - goto fail_v; + goto fail; } vb->state = VIDEOBUF_PREPARED; @@ -549,12 +546,6 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq, return 0; -fail_v: - dma_free_coherent(dev, buf->dmas[1].sg_size, - buf->dmas[1].sg_cpu, buf->dmas[1].sg_dma); -fail_u: - dma_free_coherent(dev, buf->dmas[0].sg_size, - buf->dmas[0].sg_cpu, buf->dmas[0].sg_dma); fail: free_buffer(vq, buf); out: @@ -578,10 +569,8 @@ static void pxa_dma_start_channels(struct pxa_camera_dev *pcdev) for (i = 0; i < pcdev->channels; i++) { dev_dbg(pcdev->soc_host.v4l2_dev.dev, - "%s (channel=%d) ddadr=%08x\n", __func__, - i, active->dmas[i].sg_dma); - DDADR(pcdev->dma_chans[i]) = active->dmas[i].sg_dma; - DCSR(pcdev->dma_chans[i]) = DCSR_RUN; + "%s (channel=%d)\n", __func__, i); + dma_async_issue_pending(pcdev->dma_chans[i]); } } @@ -592,7 +581,7 @@ static void pxa_dma_stop_channels(struct pxa_camera_dev *pcdev) for (i = 0; i < pcdev->channels; i++) { dev_dbg(pcdev->soc_host.v4l2_dev.dev, "%s (channel=%d)\n", __func__, i); - DCSR(pcdev->dma_chans[i]) = 0; + dmaengine_terminate_all(pcdev->dma_chans[i]); } } @@ -600,18 +589,12 @@ static void pxa_dma_add_tail_buf(struct pxa_camera_dev *pcdev, struct pxa_buffer *buf) { int i; - struct pxa_dma_desc *buf_last_desc; for (i = 0; i < pcdev->channels; i++) { - buf_last_desc = buf->dmas[i].sg_cpu + buf->dmas[i].sglen; - buf_last_desc->ddadr = DDADR_STOP; - - if (pcdev->sg_tail[i]) - /* Link the new buffer to the old tail */ - pcdev->sg_tail[i]->ddadr = buf->dmas[i].sg_dma; - - /* Update the channel tail */ - pcdev->sg_tail[i] = buf_last_desc; + buf->cookie[i] = dmaengine_submit(buf->descs[i]); + dev_dbg(pcdev->soc_host.v4l2_dev.dev, + "%s (channel=%d) : submit vb=%p cookie=%d\n", + __func__, i, buf, buf->descs[i]->cookie); } } @@ -703,8 +686,6 @@ static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev, struct videobuf_buffer *vb, struct pxa_buffer *buf) { - int i; - /* _init is used to debug races, see comment in pxa_camera_reqbufs() */ list_del_init(&vb->queue); vb->state = VIDEOBUF_DONE; @@ -716,8 +697,6 @@ static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev, if (list_empty(&pcdev->capture)) { pxa_camera_stop_capture(pcdev); - for (i = 0; i < pcdev->channels; i++) - pcdev->sg_tail[i] = NULL; return; } @@ -741,50 +720,41 @@ static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev, * * Context: should only be called within the dma irq handler */ -static void pxa_camera_check_link_miss(struct pxa_camera_dev *pcdev) +static void pxa_camera_check_link_miss(struct pxa_camera_dev *pcdev, + dma_cookie_t last_submitted, + dma_cookie_t last_issued) { - int i, is_dma_stopped = 1; + int is_dma_stopped; - for (i = 0; i < pcdev->channels; i++) - if (DDADR(pcdev->dma_chans[i]) != DDADR_STOP) - is_dma_stopped = 0; + is_dma_stopped = (last_submitted != last_issued); dev_dbg(pcdev->soc_host.v4l2_dev.dev, - "%s : top queued buffer=%p, dma_stopped=%d\n", + "%s : top queued buffer=%p, is_dma_stopped=%d\n", __func__, pcdev->active, is_dma_stopped); if (pcdev->active && is_dma_stopped) pxa_camera_start_capture(pcdev); } -static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev, +static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, enum pxa_camera_active_dma act_dma) { struct device *dev = pcdev->soc_host.v4l2_dev.dev; - struct pxa_buffer *buf; + struct pxa_buffer *buf, *last_buf; unsigned long flags; - u32 status, camera_status, overrun; + u32 camera_status, overrun; + int chan; struct videobuf_buffer *vb; + enum dma_status last_status; + dma_cookie_t last_issued; spin_lock_irqsave(&pcdev->lock, flags); - status = DCSR(channel); - DCSR(channel) = status; - camera_status = __raw_readl(pcdev->base + CISR); + dev_dbg(dev, "camera dma irq, cisr=0x%x dma=%d\n", + camera_status, act_dma); overrun = CISR_IFO_0; if (pcdev->channels == 3) overrun |= CISR_IFO_1 | CISR_IFO_2; - if (status & DCSR_BUSERR) { - dev_err(dev, "DMA Bus Error IRQ!\n"); - goto out; - } - - if (!(status & (DCSR_ENDINTR | DCSR_STARTINTR))) { - dev_err(dev, "Unknown DMA IRQ source, status: 0x%08x\n", - status); - goto out; - } - /* * pcdev->active should not be NULL in DMA irq handler. * @@ -804,28 +774,39 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev, buf = container_of(vb, struct pxa_buffer, vb); WARN_ON(buf->inwork || list_empty(&vb->queue)); - dev_dbg(dev, "%s channel=%d %s%s(vb=0x%p) dma.desc=%x\n", - __func__, channel, status & DCSR_STARTINTR ? "SOF " : "", - status & DCSR_ENDINTR ? "EOF " : "", vb, DDADR(channel)); - - if (status & DCSR_ENDINTR) { - /* - * It's normal if the last frame creates an overrun, as there - * are no more DMA descriptors to fetch from QCI fifos - */ - if (camera_status & overrun && - !list_is_last(pcdev->capture.next, &pcdev->capture)) { - dev_dbg(dev, "FIFO overrun! CISR: %x\n", - camera_status); - pxa_camera_stop_capture(pcdev); - pxa_camera_start_capture(pcdev); - goto out; - } - buf->active_dma &= ~act_dma; - if (!buf->active_dma) { - pxa_camera_wakeup(pcdev, vb, buf); - pxa_camera_check_link_miss(pcdev); - } + /* + * It's normal if the last frame creates an overrun, as there + * are no more DMA descriptors to fetch from QCI fifos + */ + switch (act_dma) { + case DMA_U: + chan = 1; + break; + case DMA_V: + chan = 2; + break; + default: + chan = 0; + break; + } + last_buf = list_entry(pcdev->capture.prev, + struct pxa_buffer, vb.queue); + last_status = dma_async_is_tx_complete(pcdev->dma_chans[chan], + last_buf->cookie[chan], + NULL, &last_issued); + if (camera_status & overrun && + last_status != DMA_COMPLETE) { + dev_dbg(dev, "FIFO overrun! CISR: %x\n", + camera_status); + pxa_camera_stop_capture(pcdev); + pxa_camera_start_capture(pcdev); + goto out; + } + buf->active_dma &= ~act_dma; + if (!buf->active_dma) { + pxa_camera_wakeup(pcdev, vb, buf); + pxa_camera_check_link_miss(pcdev, last_buf->cookie[chan], + last_issued); } out: @@ -1012,10 +993,7 @@ static void pxa_camera_clock_stop(struct soc_camera_host *ici) __raw_writel(0x3ff, pcdev->base + CICR0); /* Stop DMA engine */ - DCSR(pcdev->dma_chans[0]) = 0; - DCSR(pcdev->dma_chans[1]) = 0; - DCSR(pcdev->dma_chans[2]) = 0; - + pxa_dma_stop_channels(pcdev); pxa_camera_deactivate(pcdev); } @@ -1629,10 +1607,6 @@ static int pxa_camera_resume(struct device *dev) struct pxa_camera_dev *pcdev = ici->priv; int i = 0, ret = 0; - DRCMR(68) = pcdev->dma_chans[0] | DRCMR_MAPVLD; - DRCMR(69) = pcdev->dma_chans[1] | DRCMR_MAPVLD; - DRCMR(70) = pcdev->dma_chans[2] | DRCMR_MAPVLD; - __raw_writel(pcdev->save_cicr[i++] & ~CICR0_ENB, pcdev->base + CICR0); __raw_writel(pcdev->save_cicr[i++], pcdev->base + CICR1); __raw_writel(pcdev->save_cicr[i++], pcdev->base + CICR2); @@ -1738,8 +1712,11 @@ static int pxa_camera_probe(struct platform_device *pdev) struct pxa_camera_dev *pcdev; struct resource *res; void __iomem *base; + struct dma_slave_config config; + dma_cap_mask_t mask; + struct pxad_param params; int irq; - int err = 0; + int err = 0, i; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); irq = platform_get_irq(pdev, 0); @@ -1807,36 +1784,51 @@ static int pxa_camera_probe(struct platform_device *pdev) pcdev->base = base; /* request dma */ - err = pxa_request_dma("CI_Y", DMA_PRIO_HIGH, - pxa_camera_dma_irq_y, pcdev); - if (err < 0) { + dma_cap_zero(mask); + dma_cap_set(DMA_SLAVE, mask); + dma_cap_set(DMA_PRIVATE, mask); + + params.prio = 0; + params.drcmr = 68; + pcdev->dma_chans[0] = + dma_request_slave_channel_compat(mask, pxad_filter_fn, + ¶ms, &pdev->dev, "CI_Y"); + if (!pcdev->dma_chans[0]) { dev_err(&pdev->dev, "Can't request DMA for Y\n"); - return err; + return -ENODEV; } - pcdev->dma_chans[0] = err; - dev_dbg(&pdev->dev, "got DMA channel %d\n", pcdev->dma_chans[0]); - err = pxa_request_dma("CI_U", DMA_PRIO_HIGH, - pxa_camera_dma_irq_u, pcdev); - if (err < 0) { - dev_err(&pdev->dev, "Can't request DMA for U\n"); + params.drcmr = 69; + pcdev->dma_chans[1] = + dma_request_slave_channel_compat(mask, pxad_filter_fn, + ¶ms, &pdev->dev, "CI_U"); + if (!pcdev->dma_chans[1]) { + dev_err(&pdev->dev, "Can't request DMA for Y\n"); goto exit_free_dma_y; } - pcdev->dma_chans[1] = err; - dev_dbg(&pdev->dev, "got DMA channel (U) %d\n", pcdev->dma_chans[1]); - err = pxa_request_dma("CI_V", DMA_PRIO_HIGH, - pxa_camera_dma_irq_v, pcdev); - if (err < 0) { + params.drcmr = 70; + pcdev->dma_chans[2] = + dma_request_slave_channel_compat(mask, pxad_filter_fn, + ¶ms, &pdev->dev, "CI_V"); + if (!pcdev->dma_chans[2]) { dev_err(&pdev->dev, "Can't request DMA for V\n"); goto exit_free_dma_u; } - pcdev->dma_chans[2] = err; - dev_dbg(&pdev->dev, "got DMA channel (V) %d\n", pcdev->dma_chans[2]); - DRCMR(68) = pcdev->dma_chans[0] | DRCMR_MAPVLD; - DRCMR(69) = pcdev->dma_chans[1] | DRCMR_MAPVLD; - DRCMR(70) = pcdev->dma_chans[2] | DRCMR_MAPVLD; + memset(&config, 0, sizeof(config)); + config.src_addr_width = 0; + config.src_maxburst = 8; + config.direction = DMA_DEV_TO_MEM; + for (i = 0; i < 3; i++) { + config.src_addr = pcdev->res->start + CIBR0 + i * 8; + err = dmaengine_slave_config(pcdev->dma_chans[i], &config); + if (err < 0) { + dev_err(&pdev->dev, "dma slave config failed: %d\n", + err); + goto exit_free_dma; + } + } /* request irq */ err = devm_request_irq(&pdev->dev, pcdev->irq, pxa_camera_irq, 0, @@ -1860,11 +1852,11 @@ static int pxa_camera_probe(struct platform_device *pdev) return 0; exit_free_dma: - pxa_free_dma(pcdev->dma_chans[2]); + dma_release_channel(pcdev->dma_chans[2]); exit_free_dma_u: - pxa_free_dma(pcdev->dma_chans[1]); + dma_release_channel(pcdev->dma_chans[1]); exit_free_dma_y: - pxa_free_dma(pcdev->dma_chans[0]); + dma_release_channel(pcdev->dma_chans[0]); return err; } @@ -1874,9 +1866,9 @@ static int pxa_camera_remove(struct platform_device *pdev) struct pxa_camera_dev *pcdev = container_of(soc_host, struct pxa_camera_dev, soc_host); - pxa_free_dma(pcdev->dma_chans[0]); - pxa_free_dma(pcdev->dma_chans[1]); - pxa_free_dma(pcdev->dma_chans[2]); + dma_release_channel(pcdev->dma_chans[0]); + dma_release_channel(pcdev->dma_chans[1]); + dma_release_channel(pcdev->dma_chans[2]); soc_camera_host_unregister(soc_host); -- 2.1.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/4] media: pxa_camera: conversion to dmaengine 2015-07-05 18:27 ` [PATCH v2 4/4] media: pxa_camera: conversion to dmaengine Robert Jarzmik @ 2015-07-12 17:05 ` Guennadi Liakhovetski 2015-07-12 17:33 ` Robert Jarzmik 0 siblings, 1 reply; 14+ messages in thread From: Guennadi Liakhovetski @ 2015-07-12 17:05 UTC (permalink / raw) To: Robert Jarzmik Cc: Mauro Carvalho Chehab, Jiri Kosina, linux-media, linux-kernel Hi Robert, On Sun, 5 Jul 2015, Robert Jarzmik wrote: > Convert pxa_camera to dmaengine. This removes all DMA registers > manipulation in favor of the more generic dmaengine API. > > The functional level should be the same as before. The biggest change is > in the videobuf_sg_splice() function, which splits a videobuf-dma into > several scatterlists for 3 planes captures (Y, U, V). > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > --- > Since v1: Guennadi's fixes > dma tasklet functions prototypes change (trivial move) > --- > drivers/media/platform/soc_camera/pxa_camera.c | 438 ++++++++++++------------- > 1 file changed, 215 insertions(+), 223 deletions(-) > > diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c > index 1ab4f9d..76b2b7b 100644 > --- a/drivers/media/platform/soc_camera/pxa_camera.c > +++ b/drivers/media/platform/soc_camera/pxa_camera.c [snip] > @@ -498,9 +499,7 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq, > > if (vb->state == VIDEOBUF_NEEDS_INIT) { > int size = vb->size; > - int next_ofs = 0; > struct videobuf_dmabuf *dma = videobuf_to_dma(vb); > - struct scatterlist *sg; > > ret = videobuf_iolock(vq, vb, NULL); > if (ret) > @@ -513,11 +512,9 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq, > size_y = size; > } > > - sg = dma->sglist; > - > /* init DMA for Y channel */ How about taking the loop over the sg list out of pxa_init_dma_channel() to avoid having to iterate it from the beginning each time? Then you would be able to split it into channels inside that global loop? Would that work? Of course you might need to rearrange functions to avoid too deep code nesting. Thanks Guennadi > - ret = pxa_init_dma_channel(pcdev, buf, dma, 0, CIBR0, size_y, > - &sg, &next_ofs); > + ret = pxa_init_dma_channel(pcdev, buf, dma, 0, CIBR0, > + size_y, 0); > if (ret) { > dev_err(dev, "DMA initialization for Y/RGB failed\n"); > goto fail; > @@ -526,19 +523,19 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq, > /* init DMA for U channel */ > if (size_u) > ret = pxa_init_dma_channel(pcdev, buf, dma, 1, CIBR1, > - size_u, &sg, &next_ofs); > + size_u, size_y); > if (ret) { > dev_err(dev, "DMA initialization for U failed\n"); > - goto fail_u; > + goto fail; > } > > /* init DMA for V channel */ > if (size_v) > ret = pxa_init_dma_channel(pcdev, buf, dma, 2, CIBR2, > - size_v, &sg, &next_ofs); > + size_v, size_y + size_u); > if (ret) { > dev_err(dev, "DMA initialization for V failed\n"); > - goto fail_v; > + goto fail; > } > > vb->state = VIDEOBUF_PREPARED; > @@ -549,12 +546,6 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq, > > return 0; > > -fail_v: > - dma_free_coherent(dev, buf->dmas[1].sg_size, > - buf->dmas[1].sg_cpu, buf->dmas[1].sg_dma); > -fail_u: > - dma_free_coherent(dev, buf->dmas[0].sg_size, > - buf->dmas[0].sg_cpu, buf->dmas[0].sg_dma); > fail: > free_buffer(vq, buf); > out: ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/4] media: pxa_camera: conversion to dmaengine 2015-07-12 17:05 ` Guennadi Liakhovetski @ 2015-07-12 17:33 ` Robert Jarzmik 2015-07-18 23:00 ` Robert Jarzmik 0 siblings, 1 reply; 14+ messages in thread From: Robert Jarzmik @ 2015-07-12 17:33 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Mauro Carvalho Chehab, Jiri Kosina, linux-media, linux-kernel Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: >> /* init DMA for Y channel */ > > How about taking the loop over the sg list out of pxa_init_dma_channel() > to avoid having to iterate it from the beginning each time? Then you would > be able to split it into channels inside that global loop? Would that > work? Of course you might need to rearrange functions to avoid too deep > code nesting. Ok, will try that. The more I think of it, the more it looks to me like a generic thing : take an sglist, and an array of sizes, and split the sglist into several sglists, each of the defined size in the array. Or more code-like speaking : - sglist_split(struct scatterlist *sg_int, size_t *sizes, int nb_sizes, struct scatterlist **sg_out) - and sg_out is an array of nb_sizes (struct scatterlist *sg) So I will try that out. Maybe if that works out for pxa_camera, Jens or Russell would accept that into lib/scatterlist.c. Cheers. -- Robert ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/4] media: pxa_camera: conversion to dmaengine 2015-07-12 17:33 ` Robert Jarzmik @ 2015-07-18 23:00 ` Robert Jarzmik 2015-07-26 11:36 ` Guennadi Liakhovetski 0 siblings, 1 reply; 14+ messages in thread From: Robert Jarzmik @ 2015-07-18 23:00 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Mauro Carvalho Chehab, Jiri Kosina, linux-media, linux-kernel Robert Jarzmik <robert.jarzmik@free.fr> writes: > Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: > >>> /* init DMA for Y channel */ >> >> How about taking the loop over the sg list out of pxa_init_dma_channel() >> to avoid having to iterate it from the beginning each time? Then you would >> be able to split it into channels inside that global loop? Would that >> work? Of course you might need to rearrange functions to avoid too deep >> code nesting. > > Ok, will try that. > The more I think of it, the more it looks to me like a generic thing : take an > sglist, and an array of sizes, and split the sglist into several sglists, each > of the defined size in the array. > > Or more code-like speaking : > - sglist_split(struct scatterlist *sg_int, size_t *sizes, int nb_sizes, > struct scatterlist **sg_out) > - and sg_out is an array of nb_sizes (struct scatterlist *sg) > > So I will try that out. Maybe if that works out for pxa_camera, Jens or Russell > would accept that into lib/scatterlist.c. Ok, I made the code ... and I hate it. It's in [1], which is an incremental patch over patch 4/4. If that's what you had in mind, tell me. Cheers. -- Robert [1] The despised patch ---<8--- commit 43bbb9a4e3ac Author: Robert Jarzmik <robert.jarzmik@free.fr> Date: Tue Jul 14 20:17:51 2015 +0200 tmp: pxa_camera: working on sg_split Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c index 26a66b9ff570..83efd284e976 100644 --- a/drivers/media/platform/soc_camera/pxa_camera.c +++ b/drivers/media/platform/soc_camera/pxa_camera.c @@ -287,64 +287,110 @@ static void free_buffer(struct videobuf_queue *vq, struct pxa_buffer *buf) &buf->vb, buf->vb.baddr, buf->vb.bsize); } -static struct scatterlist *videobuf_sg_cut(struct scatterlist *sglist, - int sglen, int offset, int size, - int *new_sg_len) + +struct sg_splitter { + struct scatterlist *in_sg0; + int nents; + off_t skip_sg0; + size_t len_last_sg; + struct scatterlist *out_sg; +}; + +static struct sg_splitter * +sg_calculate_split(struct scatterlist *in, off_t skip, + const size_t *sizes, int nb_splits, gfp_t gfp_mask) { - struct scatterlist *sg0, *sg, *sg_first = NULL; - int i, dma_len, dropped_xfer_len, dropped_remain, remain; - int nfirst = -1, nfirst_offset = 0, xfer_len; - - *new_sg_len = 0; - dropped_remain = offset; - remain = size; - for_each_sg(sglist, sg, sglen, i) { - dma_len = sg_dma_len(sg); - /* PXA27x Developer's Manual 27.4.4.1: round up to 8 bytes */ - dropped_xfer_len = roundup(min(dma_len, dropped_remain), 8); - if (dropped_remain) - dropped_remain -= dropped_xfer_len; - xfer_len = dma_len - dropped_xfer_len; - - if (nfirst < 0 && xfer_len > 0) { - sg_first = sg; - nfirst = i; - nfirst_offset = dropped_xfer_len; + int i, nents; + size_t size, len; + struct sg_splitter *splitters, *curr; + struct scatterlist *sg; + + splitters = kcalloc(nb_splits, sizeof(*splitters), gfp_mask); + if (!splitters) + return NULL; + + nents = 0; + size = *sizes; + curr = splitters; + for_each_sg(in, sg, sg_nents(in), i) { + if (skip > sg_dma_len(sg)) { + skip -= sg_dma_len(sg); + continue; + } + len = min_t(size_t, size, sg_dma_len(sg) - skip); + if (!curr->in_sg0) { + curr->in_sg0 = sg; + curr->skip_sg0 = sg_dma_len(sg) - len; } - if (xfer_len > 0) { - (*new_sg_len)++; - remain -= xfer_len; + size -= len; + nents++; + if (!size) { + curr->nents = nents; + curr->len_last_sg = len; + nents = 0; + size = *(++sizes); + + if (!--nb_splits) + break; + + if (len < curr->len_last_sg) { + (splitters + 1)->in_sg0 = sg; + (splitters + 1)->skip_sg0 = 0; + } + curr++; } - if (remain <= 0) - break; } - WARN_ON(nfirst >= sglen); - sg0 = kmalloc_array(*new_sg_len, sizeof(struct scatterlist), - GFP_KERNEL); - if (!sg0) - return NULL; + return splitters; +} - remain = size; - for_each_sg(sg_first, sg, *new_sg_len, i) { - dma_len = sg_dma_len(sg); - sg0[i] = *sg; +static int sg_split(struct scatterlist *in, const int nb_splits, + const size_t *split_sizes, struct scatterlist **out, + gfp_t gfp_mask) +{ + int i, j; + struct scatterlist *in_sg, *out_sg; + struct sg_splitter *splitters, *split; - sg0[i].offset = nfirst_offset; - nfirst_offset = 0; + splitters = sg_calculate_split(in, 0, split_sizes, nb_splits, gfp_mask); + if (!splitters) + return -ENOMEM; - xfer_len = min_t(int, remain, dma_len - sg0[i].offset); - xfer_len = roundup(xfer_len, 8); - sg_dma_len(&sg0[i]) = xfer_len; + for (i = 0; i < nb_splits; i++) { + (splitters + i)->out_sg = + kmalloc_array((splitters + i)->nents, + sizeof(struct scatterlist), gfp_mask); + if (!(splitters + i)->out_sg) + goto err; + } - remain -= xfer_len; - if (remain <= 0) { - sg_mark_end(&sg0[i]); - break; + for (i = 0; i < nb_splits; i++) { + split = splitters + i; + in_sg = split->in_sg0; + out_sg = split->out_sg; + out[i] = out_sg; + for (j = 0; j < split->nents; j++) { + out_sg[j] = *in_sg; + if (!j) { + out_sg[j].offset = split->skip_sg0; + sg_dma_len(&out_sg[j]) -= split->skip_sg0; + } else { + out_sg[j].offset = 0; + } + in_sg = sg_next(in_sg); } + sg_dma_len(out_sg + split->nents - 1) = split->len_last_sg; + sg_mark_end(out_sg + split->nents - 1); } - return sg0; + kfree(splitters); + return 0; + +err: + for (i = 0; i < nb_splits; i++) + kfree((splitters + i)->out_sg); + kfree(splitters); + return -ENOMEM; } static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, @@ -391,14 +437,11 @@ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev, int cibr, int size, int offset) { struct dma_chan *dma_chan = pcdev->dma_chans[channel]; - struct scatterlist *sg; + struct scatterlist *sg = buf->sg[channel]; int sglen; struct dma_async_tx_descriptor *tx; - sg = videobuf_sg_cut(dma->sglist, dma->sglen, offset, size, &sglen); - if (!sg) - goto fail; - + sglen = sg_nents(sg); tx = dmaengine_prep_slave_sg(dma_chan, sg, sglen, DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT | DMA_CTRL_ACK); if (!tx) { @@ -421,7 +464,6 @@ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev, } buf->descs[channel] = tx; - buf->sg[channel] = sg; buf->sg_len[channel] = sglen; return 0; fail: @@ -458,6 +500,7 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq, struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb); int ret; int size_y, size_u = 0, size_v = 0; + size_t sizes[3]; dev_dbg(dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, vb, vb->baddr, vb->bsize); @@ -513,6 +556,16 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq, size_y = size; } + sizes[0] = size_y; + sizes[1] = size_u; + sizes[2] = size_v; + ret = sg_split(dma->sglist, pcdev->channels, sizes, buf->sg, + GFP_KERNEL); + if (ret) { + dev_err(dev, "sg_split failed: %d\n", ret); + goto fail; + } + /* init DMA for Y channel */ ret = pxa_init_dma_channel(pcdev, buf, dma, 0, CIBR0, size_y, 0); ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/4] media: pxa_camera: conversion to dmaengine 2015-07-18 23:00 ` Robert Jarzmik @ 2015-07-26 11:36 ` Guennadi Liakhovetski 2015-07-26 12:33 ` Robert Jarzmik 0 siblings, 1 reply; 14+ messages in thread From: Guennadi Liakhovetski @ 2015-07-26 11:36 UTC (permalink / raw) To: Robert Jarzmik Cc: Mauro Carvalho Chehab, Jiri Kosina, linux-media, linux-kernel Hi Robert, On Sun, 19 Jul 2015, Robert Jarzmik wrote: > Robert Jarzmik <robert.jarzmik@free.fr> writes: > > > Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: > > > >>> /* init DMA for Y channel */ > >> > >> How about taking the loop over the sg list out of pxa_init_dma_channel() > >> to avoid having to iterate it from the beginning each time? Then you would > >> be able to split it into channels inside that global loop? Would that > >> work? Of course you might need to rearrange functions to avoid too deep > >> code nesting. > > > > Ok, will try that. > > The more I think of it, the more it looks to me like a generic thing : take an > > sglist, and an array of sizes, and split the sglist into several sglists, each > > of the defined size in the array. > > > > Or more code-like speaking : > > - sglist_split(struct scatterlist *sg_int, size_t *sizes, int nb_sizes, > > struct scatterlist **sg_out) > > - and sg_out is an array of nb_sizes (struct scatterlist *sg) > > > > So I will try that out. Maybe if that works out for pxa_camera, Jens or Russell > > would accept that into lib/scatterlist.c. > Ok, I made the code ... and I hate it. > It's in [1], which is an incremental patch over patch 4/4. It would've been easier to review, if it were on top of the mainline, resp. your patches 1-3 from this series. > If that's what you had in mind, tell me. In principle, yes, it doesn't look all that horrible to me. You first split the global SG list into up to 3 per-channel ones, then you initialise your channels, what's wrong with that? Just have to add some polish to it here and there... This is a preliminary review, I'll do a proper one, once you fix these and send me anew version, not based on top of patch 4/4. > Cheers. > > -- > Robert > > [1] The despised patch > ---<8--- > commit 43bbb9a4e3ac > Author: Robert Jarzmik <robert.jarzmik@free.fr> > Date: Tue Jul 14 20:17:51 2015 +0200 > > tmp: pxa_camera: working on sg_split > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > > diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c > index 26a66b9ff570..83efd284e976 100644 > --- a/drivers/media/platform/soc_camera/pxa_camera.c > +++ b/drivers/media/platform/soc_camera/pxa_camera.c > @@ -287,64 +287,110 @@ static void free_buffer(struct videobuf_queue *vq, struct pxa_buffer *buf) > &buf->vb, buf->vb.baddr, buf->vb.bsize); > } > > -static struct scatterlist *videobuf_sg_cut(struct scatterlist *sglist, > - int sglen, int offset, int size, > - int *new_sg_len) > + > +struct sg_splitter { > + struct scatterlist *in_sg0; > + int nents; > + off_t skip_sg0; > + size_t len_last_sg; > + struct scatterlist *out_sg; > +}; > + > +static struct sg_splitter * > +sg_calculate_split(struct scatterlist *in, off_t skip, You don't need "skip," you only call this function once with skip == 0. Besides I usually prefer all the keywords before the function name, the function name, the opening parenthesis and at least the first argument on the same line. So far pxa_camera.c follows this, let's keep it this way. It makes grepping for functions easier. And no, I don't care about 80 chars... > + const size_t *sizes, int nb_splits, gfp_t gfp_mask) > { > - struct scatterlist *sg0, *sg, *sg_first = NULL; > - int i, dma_len, dropped_xfer_len, dropped_remain, remain; > - int nfirst = -1, nfirst_offset = 0, xfer_len; > - > - *new_sg_len = 0; > - dropped_remain = offset; > - remain = size; > - for_each_sg(sglist, sg, sglen, i) { > - dma_len = sg_dma_len(sg); > - /* PXA27x Developer's Manual 27.4.4.1: round up to 8 bytes */ > - dropped_xfer_len = roundup(min(dma_len, dropped_remain), 8); > - if (dropped_remain) > - dropped_remain -= dropped_xfer_len; > - xfer_len = dma_len - dropped_xfer_len; > - > - if (nfirst < 0 && xfer_len > 0) { > - sg_first = sg; > - nfirst = i; > - nfirst_offset = dropped_xfer_len; > + int i, nents; > + size_t size, len; > + struct sg_splitter *splitters, *curr; > + struct scatterlist *sg; > + > + splitters = kcalloc(nb_splits, sizeof(*splitters), gfp_mask); This is an array of at most 3 elements, 20 bytes each. I'd just allocate it on stack in the calling function and avoid this kcalloc(). Then you can make this function return the total number of sg elements, which is actually at most original number of elements + 2, right? Then you can use that total number to allocate all new sg elements in one go to reduce the number of allocations. > + if (!splitters) > + return NULL; > + > + nents = 0; I would put these in initialisation: + int i, nents = 0; + size_t size = sizes[0], len; etc. > + size = *sizes; > + curr = splitters; > + for_each_sg(in, sg, sg_nents(in), i) { > + if (skip > sg_dma_len(sg)) { > + skip -= sg_dma_len(sg); > + continue; > + } > + len = min_t(size_t, size, sg_dma_len(sg) - skip); > + if (!curr->in_sg0) { > + curr->in_sg0 = sg; > + curr->skip_sg0 = sg_dma_len(sg) - len; > } > - if (xfer_len > 0) { > - (*new_sg_len)++; > - remain -= xfer_len; > + size -= len; > + nents++; > + if (!size) { > + curr->nents = nents; > + curr->len_last_sg = len; > + nents = 0; > + size = *(++sizes); > + > + if (!--nb_splits) > + break; This break won't be needed, because: > + > + if (len < curr->len_last_sg) { How is this possible? You just did + curr->len_last_sg = len; > + (splitters + 1)->in_sg0 = sg; In general I like pointer arithmetics and use it always when I need a _pointer_, but in such cases I'd normally just write splitters[1].in_sg0, don't you think that would look better? Ditto everywhere below. > + (splitters + 1)->skip_sg0 = 0; > + } > + curr++; > } > - if (remain <= 0) > - break; > } > - WARN_ON(nfirst >= sglen); > > - sg0 = kmalloc_array(*new_sg_len, sizeof(struct scatterlist), > - GFP_KERNEL); > - if (!sg0) > - return NULL; > + return splitters; > +} > > - remain = size; > - for_each_sg(sg_first, sg, *new_sg_len, i) { > - dma_len = sg_dma_len(sg); > - sg0[i] = *sg; > +static int sg_split(struct scatterlist *in, const int nb_splits, > + const size_t *split_sizes, struct scatterlist **out, > + gfp_t gfp_mask) > +{ > + int i, j; > + struct scatterlist *in_sg, *out_sg; > + struct sg_splitter *splitters, *split; > > - sg0[i].offset = nfirst_offset; > - nfirst_offset = 0; > + splitters = sg_calculate_split(in, 0, split_sizes, nb_splits, gfp_mask); > + if (!splitters) > + return -ENOMEM; > > - xfer_len = min_t(int, remain, dma_len - sg0[i].offset); > - xfer_len = roundup(xfer_len, 8); > - sg_dma_len(&sg0[i]) = xfer_len; > + for (i = 0; i < nb_splits; i++) { > + (splitters + i)->out_sg = > + kmalloc_array((splitters + i)->nents, > + sizeof(struct scatterlist), gfp_mask); > + if (!(splitters + i)->out_sg) > + goto err; > + } > > - remain -= xfer_len; > - if (remain <= 0) { > - sg_mark_end(&sg0[i]); > - break; > + for (i = 0; i < nb_splits; i++) { Maybe + for (i = 0, split = splitters; i < nb_splits; i++, split++) { > + split = splitters + i; > + in_sg = split->in_sg0; > + out_sg = split->out_sg; > + out[i] = out_sg; > + for (j = 0; j < split->nents; j++) { + for (j = 0; j < split->nents; j++, out_sg++) { > + out_sg[j] = *in_sg; + *out_sg = *in_sg; > + if (!j) { > + out_sg[j].offset = split->skip_sg0; > + sg_dma_len(&out_sg[j]) -= split->skip_sg0; + out_sg->offset = split->skip_sg0; + sg_dma_len(out_sg) -= split->skip_sg0; etc. > + } else { > + out_sg[j].offset = 0; > + } > + in_sg = sg_next(in_sg); > } > + sg_dma_len(out_sg + split->nents - 1) = split->len_last_sg; > + sg_mark_end(out_sg + split->nents - 1); > } > > - return sg0; > + kfree(splitters); > + return 0; > + > +err: > + for (i = 0; i < nb_splits; i++) > + kfree((splitters + i)->out_sg); > + kfree(splitters); > + return -ENOMEM; > } > > static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev, > @@ -391,14 +437,11 @@ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev, > int cibr, int size, int offset) > { > struct dma_chan *dma_chan = pcdev->dma_chans[channel]; > - struct scatterlist *sg; > + struct scatterlist *sg = buf->sg[channel]; > int sglen; > struct dma_async_tx_descriptor *tx; > > - sg = videobuf_sg_cut(dma->sglist, dma->sglen, offset, size, &sglen); > - if (!sg) > - goto fail; > - > + sglen = sg_nents(sg); > tx = dmaengine_prep_slave_sg(dma_chan, sg, sglen, DMA_DEV_TO_MEM, > DMA_PREP_INTERRUPT | DMA_CTRL_ACK); > if (!tx) { > @@ -421,7 +464,6 @@ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev, > } > > buf->descs[channel] = tx; > - buf->sg[channel] = sg; > buf->sg_len[channel] = sglen; > return 0; > fail: > @@ -458,6 +500,7 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq, > struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb); > int ret; > int size_y, size_u = 0, size_v = 0; > + size_t sizes[3]; > > dev_dbg(dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, > vb, vb->baddr, vb->bsize); > @@ -513,6 +556,16 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq, > size_y = size; > } > > + sizes[0] = size_y; > + sizes[1] = size_u; > + sizes[2] = size_v; > + ret = sg_split(dma->sglist, pcdev->channels, sizes, buf->sg, > + GFP_KERNEL); > + if (ret) { In most places pxa_camera.c checks for (ret < 0), but no longer in all anyway. Thanks Guennadi > + dev_err(dev, "sg_split failed: %d\n", ret); > + goto fail; > + } > + > /* init DMA for Y channel */ > ret = pxa_init_dma_channel(pcdev, buf, dma, 0, CIBR0, > size_y, 0); > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/4] media: pxa_camera: conversion to dmaengine 2015-07-26 11:36 ` Guennadi Liakhovetski @ 2015-07-26 12:33 ` Robert Jarzmik 0 siblings, 0 replies; 14+ messages in thread From: Robert Jarzmik @ 2015-07-26 12:33 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Mauro Carvalho Chehab, Jiri Kosina, linux-media, linux-kernel Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: > Hi Robert, > > On Sun, 19 Jul 2015, Robert Jarzmik wrote: > In principle, yes, it doesn't look all that horrible to me. You first > split the global SG list into up to 3 per-channel ones, then you > initialise your channels, what's wrong with that? Just have to add some > polish to it here and there... This is a preliminary review, I'll do a > proper one, once you fix these and send me anew version, not based on top > of patch 4/4. Ok. >> +struct sg_splitter { >> + struct scatterlist *in_sg0; >> + int nents; >> + off_t skip_sg0; >> + size_t len_last_sg; >> + struct scatterlist *out_sg; >> +}; >> + >> +static struct sg_splitter * >> +sg_calculate_split(struct scatterlist *in, off_t skip, > > You don't need "skip," you only call this function once with skip == 0. In the specific pxa_camera case, that's true. But I made this code for the broader case, where : - skip != 0 is possible - the sum of sizes is smaller that total sg span My goal is after this submission to try to move this code to the more generic lib/sglist.c, hence the genericity. > Besides I usually prefer all the keywords before the function name, the > function name, the opening parenthesis and at least the first argument on > the same line. So far pxa_camera.c follows this, let's keep it this way. > It makes grepping for functions easier. And no, I don't care about 80 > chars... As you wish. >> + int i, nents; >> + size_t size, len; >> + struct sg_splitter *splitters, *curr; >> + struct scatterlist *sg; >> + >> + splitters = kcalloc(nb_splits, sizeof(*splitters), gfp_mask); > > This is an array of at most 3 elements, 20 bytes each. I'd just allocate > it on stack in the calling function and avoid this kcalloc(). Then you can > make this function return the total number of sg elements, which is > actually at most original number of elements + 2, right? Then you can use > that total number to allocate all new sg elements in one go to reduce the > number of allocations. You think pxa_camera specific, I think sglist generic. I think I should try to submit the generic sglist code first, if I get turned down, specialize the code for pxa_camera, don't you think ? > I would put these in initialisation: > > + int i, nents = 0; > + size_t size = sizes[0], len; Ok. >> + size = *sizes; >> + curr = splitters; >> + for_each_sg(in, sg, sg_nents(in), i) { >> + if (skip > sg_dma_len(sg)) { >> + skip -= sg_dma_len(sg); >> + continue; >> + } >> + len = min_t(size_t, size, sg_dma_len(sg) - skip); >> + if (!curr->in_sg0) { >> + curr->in_sg0 = sg; >> + curr->skip_sg0 = sg_dma_len(sg) - len; >> } >> - if (xfer_len > 0) { >> - (*new_sg_len)++; >> - remain -= xfer_len; >> + size -= len; >> + nents++; >> + if (!size) { >> + curr->nents = nents; >> + curr->len_last_sg = len; >> + nents = 0; >> + size = *(++sizes); >> + >> + if (!--nb_splits) >> + break; > > This break won't be needed, because: It is needed (in the generic case) if I choose to split in 3 4k pages an sg of 4 pages. In that case, without this break, the for_each_sg() loop will continue, and curr will be out of bounds. > >> + >> + if (len < curr->len_last_sg) { > > How is this possible? You just did > > + curr->len_last_sg = len; Ah yes, it's indeed wrong. This test was to take care of the case when an sg is split as : - partly as the last sg entry of splitters[n]->in_sg0 - partly as the first sg entry of splitter[n+1]->in_sg0 I must rework that condition to : - if (len < dma_sg_len(sg)) > In general I like pointer arithmetics and use it always when I need a > _pointer_, but in such cases I'd normally just write splitters[1].in_sg0, > don't you think that would look better? Ditto everywhere below. Ok. > >> + (splitters + 1)->skip_sg0 = 0; >> + } >> + curr++; >> } >> - if (remain <= 0) >> - break; >> } >> - WARN_ON(nfirst >= sglen); >> >> - sg0 = kmalloc_array(*new_sg_len, sizeof(struct scatterlist), >> - GFP_KERNEL); >> - if (!sg0) >> - return NULL; >> + return splitters; >> +} >> >> - remain = size; >> - for_each_sg(sg_first, sg, *new_sg_len, i) { >> - dma_len = sg_dma_len(sg); >> - sg0[i] = *sg; >> + for (i = 0; i < nb_splits; i++) { > > Maybe > > + for (i = 0, split = splitters; i < nb_splits; i++, split++) { Yes. > >> + split = splitters + i; >> + in_sg = split->in_sg0; >> + out_sg = split->out_sg; >> + out[i] = out_sg; >> + for (j = 0; j < split->nents; j++) { > > + for (j = 0; j < split->nents; j++, out_sg++) { Yes. >> @@ -458,6 +500,7 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq, >> struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb); >> int ret; >> int size_y, size_u = 0, size_v = 0; >> + size_t sizes[3]; >> >> dev_dbg(dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, >> vb, vb->baddr, vb->bsize); >> @@ -513,6 +556,16 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq, >> size_y = size; >> } >> >> + sizes[0] = size_y; >> + sizes[1] = size_u; >> + sizes[2] = size_v; >> + ret = sg_split(dma->sglist, pcdev->channels, sizes, buf->sg, >> + GFP_KERNEL); >> + if (ret) { > > In most places pxa_camera.c checks for (ret < 0), but no longer in all > anyway. Oh you're right, and I like things to remain homogenous, so I'll fix that. Cheers. -- Robert ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-07-26 12:37 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-07-05 18:27 [PATCH v2 0/4] media: pxa_camera conversion to dmaengine Robert Jarzmik 2015-07-05 18:27 ` [PATCH v2 1/4] media: pxa_camera: fix the buffer free path Robert Jarzmik 2015-07-12 13:58 ` Guennadi Liakhovetski 2015-07-12 14:35 ` Robert Jarzmik 2015-07-05 18:27 ` [PATCH v2 2/4] media: pxa_camera: move interrupt to tasklet Robert Jarzmik 2015-07-05 18:27 ` [PATCH v2 3/4] media: pxa_camera: trivial move of dma irq functions Robert Jarzmik 2015-07-12 14:06 ` Guennadi Liakhovetski 2015-07-12 14:32 ` Robert Jarzmik 2015-07-05 18:27 ` [PATCH v2 4/4] media: pxa_camera: conversion to dmaengine Robert Jarzmik 2015-07-12 17:05 ` Guennadi Liakhovetski 2015-07-12 17:33 ` Robert Jarzmik 2015-07-18 23:00 ` Robert Jarzmik 2015-07-26 11:36 ` Guennadi Liakhovetski 2015-07-26 12:33 ` Robert Jarzmik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).