diff mbox

[v5,1/4] media: pxa_camera: fix the buffer free path

Message ID 1441539733-19201-1-git-send-email-robert.jarzmik@free.fr (mailing list archive)
State New, archived
Headers show

Commit Message

Robert Jarzmik Sept. 6, 2015, 11:42 a.m. UTC
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>
---
Since v3: take into account the 2 paths possibilities to free_buffer()
---
 drivers/media/platform/soc_camera/pxa_camera.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

Comments

Robert Jarzmik Oct. 24, 2015, 9:59 a.m. UTC | #1
Robert Jarzmik <robert.jarzmik@free.fr> writes:

> 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>
> ---
> Since v3: take into account the 2 paths possibilities to free_buffer()
Okay Guennadi, it's been enough time.
Could you you have another look at this serie please ?

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski Oct. 27, 2015, 10:07 p.m. UTC | #2
Hi Robert,

Didn't you tell me, that your dmaengine patch got rejected and therefore 
these your patches were on hold?

Thanks
Guennadi

On Sat, 24 Oct 2015, Robert Jarzmik wrote:

> Robert Jarzmik <robert.jarzmik@free.fr> writes:
> 
> > 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>
> > ---
> > Since v3: take into account the 2 paths possibilities to free_buffer()
> Okay Guennadi, it's been enough time.
> Could you you have another look at this serie please ?
> 
> Cheers.
> 
> --
> Robert
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Robert Jarzmik Oct. 27, 2015, 10:15 p.m. UTC | #3
Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> Hi Robert,
>
> Didn't you tell me, that your dmaengine patch got rejected and therefore 
> these your patches were on hold?
They were reverted, and then revamped into DMA_CTRL_REUSE, upstreamed and
merged, as in the commit 272420214d26 ("dmaengine: Add DMA_CTRL_REUSE"). I'd

Of course a pending fix is still underway
(http://www.serverphorums.com/read.php?12,1318680). But that shouldn't stop us
from reviewing to get ready to merge.

I want this serie to be ready, so that as soon as Vinod merges the fix, I can
ping you to trigger the merge into your tree, without doing (and waiting)
additional review cycles.

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski Oct. 29, 2015, 4:01 p.m. UTC | #4
Hi Robert,

On Tue, 27 Oct 2015, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > Hi Robert,
> >
> > Didn't you tell me, that your dmaengine patch got rejected and therefore 
> > these your patches were on hold?
> They were reverted, and then revamped into DMA_CTRL_REUSE, upstreamed and
> merged, as in the commit 272420214d26 ("dmaengine: Add DMA_CTRL_REUSE"). I'd
> 
> Of course a pending fix is still underway
> (http://www.serverphorums.com/read.php?12,1318680). But that shouldn't stop us
> from reviewing to get ready to merge.
> 
> I want this serie to be ready, so that as soon as Vinod merges the fix, I can
> ping you to trigger the merge into your tree, without doing (and waiting)
> additional review cycles.

Thanks, understand now. As we discussed before, correct me if I am wrong, 
this is your hobby project. PXA270 is a legacy platform, nobody except you 
is interested in this work. I have nothing against hobby projects and I 
want to support them as much as I can, but I hope you'll understand, that 
I don't have too much free time, so I cannot handle such projects with a 
high priority. I understand your desire to process these patches ASAP, 
however, I'd like to try to minimise my work too. So, I can propose the 
following: let us wait, until your PXA dmaengine patches are _indeed_ in 
the mainline. Then you test your camera patches on top of that tree again, 
perform any eventually necessary updates and either let me know, that 
either your last version is ok and I can now review it, or submit a new 
version, that _works_ on top of then current tree.

Thanks
Guennadi

> Cheers.
> 
> --
> Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Robert Jarzmik Feb. 8, 2016, 8:25 p.m. UTC | #5
Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> Hi Robert,
>
> On Tue, 27 Oct 2015, Robert Jarzmik wrote:
>
>> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
>> 
>> > Hi Robert,
>> >
>> > Didn't you tell me, that your dmaengine patch got rejected and therefore 
>> > these your patches were on hold?
>> They were reverted, and then revamped into DMA_CTRL_REUSE, upstreamed and
>> merged, as in the commit 272420214d26 ("dmaengine: Add DMA_CTRL_REUSE"). I'd
>> 
>> Of course a pending fix is still underway
>> (http://www.serverphorums.com/read.php?12,1318680). But that shouldn't stop us
>> from reviewing to get ready to merge.
>> 
>> I want this serie to be ready, so that as soon as Vinod merges the fix, I can
>> ping you to trigger the merge into your tree, without doing (and waiting)
>> additional review cycles.
>
> Thanks, understand now. As we discussed before, correct me if I am wrong, 
> this is your hobby project. PXA270 is a legacy platform, nobody except you 
> is interested in this work. I have nothing against hobby projects and I 
> want to support them as much as I can, but I hope you'll understand, that 
> I don't have too much free time, so I cannot handle such projects with a 
> high priority. I understand your desire to process these patches ASAP, 
> however, I'd like to try to minimise my work too. So, I can propose the 
> following: let us wait, until your PXA dmaengine patches are _indeed_ in 
> the mainline. Then you test your camera patches on top of that tree again, 
> perform any eventually necessary updates and either let me know, that 
> either your last version is ok and I can now review it, or submit a new 
> version, that _works_ on top of then current tree.

Okay Guennadi, I retested this version on top of v4.5-rc2, still good to
go. There is a minor conflict in the includes since this submission, and I can
repost a v6 which solves it.

So please tell me how I should proceed, either repost a rebased v6 or take v5 or
anything else ...

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski Feb. 21, 2016, 1:01 p.m. UTC | #6
On Mon, 8 Feb 2016, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > Hi Robert,
> >
> > On Tue, 27 Oct 2015, Robert Jarzmik wrote:
> >
> >> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> >> 
> >> > Hi Robert,
> >> >
> >> > Didn't you tell me, that your dmaengine patch got rejected and therefore 
> >> > these your patches were on hold?
> >> They were reverted, and then revamped into DMA_CTRL_REUSE, upstreamed and
> >> merged, as in the commit 272420214d26 ("dmaengine: Add DMA_CTRL_REUSE"). I'd
> >> 
> >> Of course a pending fix is still underway
> >> (http://www.serverphorums.com/read.php?12,1318680). But that shouldn't stop us
> >> from reviewing to get ready to merge.
> >> 
> >> I want this serie to be ready, so that as soon as Vinod merges the fix, I can
> >> ping you to trigger the merge into your tree, without doing (and waiting)
> >> additional review cycles.
> >
> > Thanks, understand now. As we discussed before, correct me if I am wrong, 
> > this is your hobby project. PXA270 is a legacy platform, nobody except you 
> > is interested in this work. I have nothing against hobby projects and I 
> > want to support them as much as I can, but I hope you'll understand, that 
> > I don't have too much free time, so I cannot handle such projects with a 
> > high priority. I understand your desire to process these patches ASAP, 
> > however, I'd like to try to minimise my work too. So, I can propose the 
> > following: let us wait, until your PXA dmaengine patches are _indeed_ in 
> > the mainline. Then you test your camera patches on top of that tree again, 
> > perform any eventually necessary updates and either let me know, that 
> > either your last version is ok and I can now review it, or submit a new 
> > version, that _works_ on top of then current tree.
> 
> Okay Guennadi, I retested this version on top of v4.5-rc2, still good to
> go. There is a minor conflict in the includes since this submission, and I can
> repost a v6 which solves it.

How did you test it with that patchg #3?? What's a minor conflict? If a 
patch doesn't apply at all or applies with a fuzz, yes, please fix. If 
it's just a few lines off, no need to fix that. But you'll do a v6 anyway, 
I assume.

Thanks
Guennadi

> So please tell me how I should proceed, either repost a rebased v6 or take v5 or
> anything else ...
> 
> Cheers.
> 
> --
> Robert
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Robert Jarzmik Feb. 21, 2016, 2:53 p.m. UTC | #7
Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

>> Okay Guennadi, I retested this version on top of v4.5-rc2, still good to
>> go. There is a minor conflict in the includes since this submission, and I can
>> repost a v6 which solves it.
>
> How did you test it with that patchg #3??
I rebased my patches on top of v4.5-rc2. To be exact, I rebased the tree I had
with these last patches on top of v4.5-rc2. I'll recheck, it's been some time
...

> What's a minor conflict?
A conflict on a context line :
#include <mach/dma.h>
#include <linux/platform_data/media/camera-pxa.h>

I think linux/platform_data/media/camera-pxa.h changed from my last submssion,
hence the conflict.

> If a patch doesn't apply at all or applies with a fuzz, yes, please fix. If
> it's just a few lines off, no need to fix that. But you'll do a v6 anyway, I
> assume.
But of course, let us have a v6 which cleanly applies on v4.5-rc2, and restested
once more. I'll try to have it done this evening.

Cheers.
Guennadi Liakhovetski Feb. 21, 2016, 4:50 p.m. UTC | #8
On Sun, 21 Feb 2016, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> >> Okay Guennadi, I retested this version on top of v4.5-rc2, still good to
> >> go. There is a minor conflict in the includes since this submission, and I can
> >> repost a v6 which solves it.
> >
> > How did you test it with that patchg #3??
> I rebased my patches on top of v4.5-rc2. To be exact, I rebased the tree I had
> with these last patches on top of v4.5-rc2. I'll recheck, it's been some time
> ...
> 
> > What's a minor conflict?
> A conflict on a context line :
> #include <mach/dma.h>
> #include <linux/platform_data/media/camera-pxa.h>
> 
> I think linux/platform_data/media/camera-pxa.h changed from my last submssion,
> hence the conflict.
> 
> > If a patch doesn't apply at all or applies with a fuzz, yes, please fix. If
> > it's just a few lines off, no need to fix that. But you'll do a v6 anyway, I
> > assume.
> But of course, let us have a v6 which cleanly applies on v4.5-rc2, and restested
> once more. I'll try to have it done this evening.

Please, have a look at 
http://git.linuxtv.org/gliakhovetski/v4l-dvb.git/log/?h=for-4.6-2
If all is good there, no need for a v6

Thanks
Guennadi

> 
> Cheers.
> 
> -- 
> Robert
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Robert Jarzmik Feb. 22, 2016, 9:22 p.m. UTC | #9
Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> On Sun, 21 Feb 2016, Robert Jarzmik wrote:
> Please, have a look at 
> http://git.linuxtv.org/gliakhovetski/v4l-dvb.git/log/?h=for-4.6-2
> If all is good there, no need for a v6

Thanks for fixing the *_dma_irq() mess, and sorry for that.

And I just tested your branch, and it's working perfectly fine.

Cheers.
diff mbox

Patch

diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c
index fcb942de0c7f..d4e887841372 100644
--- a/drivers/media/platform/soc_camera/pxa_camera.c
+++ b/drivers/media/platform/soc_camera/pxa_camera.c
@@ -272,8 +272,6 @@  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);
 
 	for (i = 0; i < ARRAY_SIZE(buf->dmas); i++) {
 		if (buf->dmas[i].sg_cpu)
@@ -283,6 +281,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;
 }
@@ -479,7 +479,7 @@  static int pxa_videobuf_prepare(struct videobuf_queue *vq,
 
 		ret = videobuf_iolock(vq, vb, NULL);
 		if (ret)
-			goto fail;
+			goto out;
 
 		if (pcdev->channels == 3) {
 			size_y = size / 2;
@@ -504,7 +504,7 @@  static int pxa_videobuf_prepare(struct videobuf_queue *vq,
 						   size_u, &sg, &next_ofs);
 		if (ret) {
 			dev_err(dev, "DMA initialization for U failed\n");
-			goto fail_u;
+			goto fail;
 		}
 
 		/* init DMA for V channel */
@@ -513,7 +513,7 @@  static int pxa_videobuf_prepare(struct videobuf_queue *vq,
 						   size_v, &sg, &next_ofs);
 		if (ret) {
 			dev_err(dev, "DMA initialization for V failed\n");
-			goto fail_v;
+			goto fail;
 		}
 
 		vb->state = VIDEOBUF_PREPARED;
@@ -524,12 +524,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: