[linux-dvb] [PATCH] 1/3: BUG FIX in dvb_ringbuffer_flush

[linux-dvb] [PATCH] 1/3: BUG FIX in dvb_ringbuffer_flush

[Andrea]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

linux-dvb-request@linuxtv.org wrote:
> Date: Sat, 22 Mar 2008 04:56:43 +0100
> From: Oliver Endriss <o.endriss@gmx.de>
> 
  > Nak. At the first glance one might think that this patch is correct.
  > Unfortunately, it introduces a subtle bug.

> So I suggest to leave dvb_ringbuffer_flush() as is and zero the read and
> write pointers only where it is really required...

Thanks for your feedback.
You are right and I have changed the code.

I've added a new function in the ringbuffer that resets the pointers to 0 and clears the error flag.
There might be some more factoring and one could move into that function 2 more lines

	buf->data = NULL;
	buf->size = size;
	dvb_ringbuffer_reset(buf);


Andrea


[-- Attachment #2: ring2.diff --]
[-- Type: text/x-patch, Size: 1661 bytes --]

diff -r 1886a5ea2f84 linux/drivers/media/dvb/dvb-core/dmxdev.c
--- a/linux/drivers/media/dvb/dvb-core/dmxdev.c	Fri Mar 21 08:04:55 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dmxdev.c	Sat Mar 22 10:12:59 2008 +0000
@@ -281,7 +313,7 @@ static int dvb_dmxdev_set_buffer_size(st
 	mem = buf->data;
 	buf->data = NULL;
 	buf->size = size;
-	dvb_ringbuffer_flush(buf);
+	dvb_ringbuffer_reset(buf);
 	spin_unlock_irq(&dmxdevfilter->dev->lock);
 	vfree(mem);
 
diff -r 1886a5ea2f84 linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Fri Mar 21 08:04:55 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Sat Mar 22 10:12:59 2008 +0000
@@ -90,6 +90,11 @@ void dvb_ringbuffer_flush(struct dvb_rin
 	rbuf->error = 0;
 }
 
+void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf)
+{
+	rbuf->pread = rbuf->pwrite = 0;
+	rbuf->error = 0;
+}
 
 
 void dvb_ringbuffer_flush_spinlock_wakeup(struct dvb_ringbuffer *rbuf)
diff -r 1886a5ea2f84 linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h	Fri Mar 21 08:04:55 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h	Sat Mar 22 10:12:59 2008 +0000
@@ -84,6 +84,12 @@ extern ssize_t dvb_ringbuffer_free(struc
 /* return the number of bytes waiting in the buffer */
 extern ssize_t dvb_ringbuffer_avail(struct dvb_ringbuffer *rbuf);
 
+/* 
+** Reset the read and write pointers to zero and flush the buffer
+** This counts as a read and write operation
+*/
+
+extern void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf);
 
 /* read routines & macros */
 /* ---------------------- */


[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] 1/3: BUG FIX in dvb_ringbuffer_flush

[Andrea]

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

Andrea wrote:
> linux-dvb-request@linuxtv.org wrote:
> I've added a new function in the ringbuffer that resets the pointers to 
> 0 and clears the error flag.
> There might be some more factoring and one could move into that function 
> 2 more lines
> 
>     buf->data = NULL;
>     buf->size = size;
>     dvb_ringbuffer_reset(buf);

I've just added a comment to patch 1/3.
I post it here again.

This patch fixes the bug in DMX_SET_BUFFER_SIZE for the demux.
Basically it resets read and write pointers to 0 in case they are beyond the new size of the buffer.

In the next patch (2/3) I rewrite this function to behave the same as the new DMX_SET_BUFFER_SIZE 
for the dvr.
I thought it is a good idea for the 2 very similar ioctl to be implemented in the same way.

Andrea

[-- Attachment #2: patch.1 --]
[-- Type: text/plain, Size: 1715 bytes --]

diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dmxdev.c
--- a/linux/drivers/media/dvb/dvb-core/dmxdev.c	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dmxdev.c	Sun Apr 13 09:50:59 2008 +0100
@@ -281,7 +281,9 @@ static int dvb_dmxdev_set_buffer_size(st
 	mem = buf->data;
 	buf->data = NULL;
 	buf->size = size;
-	dvb_ringbuffer_flush(buf);
+	
+	// reset and not flush in case the buffer shrinks
+	dvb_ringbuffer_reset(buf);
 	spin_unlock_irq(&dmxdevfilter->dev->lock);
 	vfree(mem);
 
diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Sun Apr 13 09:50:59 2008 +0100
@@ -90,6 +90,11 @@ void dvb_ringbuffer_flush(struct dvb_rin
 	rbuf->error = 0;
 }
 
+void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf)
+{
+	rbuf->pread = rbuf->pwrite = 0;
+	rbuf->error = 0;
+}
 
 
 void dvb_ringbuffer_flush_spinlock_wakeup(struct dvb_ringbuffer *rbuf)
diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h	Sun Apr 13 09:50:59 2008 +0100
@@ -84,6 +84,12 @@ extern ssize_t dvb_ringbuffer_free(struc
 /* return the number of bytes waiting in the buffer */
 extern ssize_t dvb_ringbuffer_avail(struct dvb_ringbuffer *rbuf);
 
+/* 
+** Reset the read and write pointers to zero and flush the buffer
+** This counts as a read and write operation
+*/
+
+extern void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf);
 
 /* read routines & macros */
 /* ---------------------- */

[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] 1/3: BUG FIX in dvb_ringbuffer_flush

[Andrea]

[-- Attachment #1: Type: text/plain, Size: 556 bytes --]

Andrea wrote:
> I've just added a comment to patch 1/3.
> I post it here again.
> 
> This patch fixes the bug in DMX_SET_BUFFER_SIZE for the demux.
> Basically it resets read and write pointers to 0 in case they are beyond 
> the new size of the buffer.
> 
> In the next patch (2/3) I rewrite this function to behave the same as 
> the new DMX_SET_BUFFER_SIZE for the dvr.
> I thought it is a good idea for the 2 very similar ioctl to be 
> implemented in the same way.
> 
> Andrea

I've fixed some formatting errors reported by "make checkpatch".

Andrea

[-- Attachment #2: patch.1 --]
[-- Type: text/plain, Size: 1716 bytes --]

diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dmxdev.c
--- a/linux/drivers/media/dvb/dvb-core/dmxdev.c	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dmxdev.c	Sun Apr 13 10:46:37 2008 +0100
@@ -281,7 +281,9 @@ static int dvb_dmxdev_set_buffer_size(st
 	mem = buf->data;
 	buf->data = NULL;
 	buf->size = size;
-	dvb_ringbuffer_flush(buf);
+
+	/* reset and not flush in case the buffer shrinks */
+	dvb_ringbuffer_reset(buf);
 	spin_unlock_irq(&dmxdevfilter->dev->lock);
 	vfree(mem);
 
diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Sun Apr 13 10:46:37 2008 +0100
@@ -90,6 +90,11 @@ void dvb_ringbuffer_flush(struct dvb_rin
 	rbuf->error = 0;
 }
 
+void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf)
+{
+	rbuf->pread = rbuf->pwrite = 0;
+	rbuf->error = 0;
+}
 
 
 void dvb_ringbuffer_flush_spinlock_wakeup(struct dvb_ringbuffer *rbuf)
diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h	Sun Apr 13 10:46:37 2008 +0100
@@ -84,6 +84,12 @@ extern ssize_t dvb_ringbuffer_free(struc
 /* return the number of bytes waiting in the buffer */
 extern ssize_t dvb_ringbuffer_avail(struct dvb_ringbuffer *rbuf);
 
+/*
+** Reset the read and write pointers to zero and flush the buffer
+** This counts as a read and write operation
+*/
+
+extern void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf);
 
 /* read routines & macros */
 /* ---------------------- */

[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] 1/3: BUG FIX in dvb_ringbuffer_flush

[Andrea]

Andrea wrote:
> Andrea wrote:
>> I've just added a comment to patch 1/3.
>> I post it here again.
>>
>> This patch fixes the bug in DMX_SET_BUFFER_SIZE for the demux.
>> Basically it resets read and write pointers to 0 in case they are 
>> beyond the new size of the buffer.
>>
>> In the next patch (2/3) I rewrite this function to behave the same as 
>> the new DMX_SET_BUFFER_SIZE for the dvr.
>> I thought it is a good idea for the 2 very similar ioctl to be 
>> implemented in the same way.
>>
>> Andrea
> 
> I've fixed some formatting errors reported by "make checkpatch".
> 
> Andrea


This patch fixes a bug in DMX_SET_BUFFER_SIZE in case the buffer shrinks.

Signed-off-by: Andrea Odetti <mariofutire@gmail.com>

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] 1/3: BUG FIX in dvb_ringbuffer_flush

[Oliver Endriss]

Andrea wrote:
> Andrea wrote:
> > Andrea wrote:
> >> I've just added a comment to patch 1/3.
> >> I post it here again.
> >>
> >> This patch fixes the bug in DMX_SET_BUFFER_SIZE for the demux.
> >> Basically it resets read and write pointers to 0 in case they are 
> >> beyond the new size of the buffer.
> >>
> >> In the next patch (2/3) I rewrite this function to behave the same as 
> >> the new DMX_SET_BUFFER_SIZE for the dvr.
> >> I thought it is a good idea for the 2 very similar ioctl to be 
> >> implemented in the same way.
> >>
> >> Andrea
> > 
> > I've fixed some formatting errors reported by "make checkpatch".
> > 
> > Andrea
> 
> 
> This patch fixes a bug in DMX_SET_BUFFER_SIZE in case the buffer shrinks.
> 
> Signed-off-by: Andrea Odetti <mariofutire@gmail.com>

Committed to HG. Thanks.

CU
Oliver

-- 
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
----------------------------------------------------------------

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

[linux-dvb] [PATCH] 2/3: implement DMX_SET_BUFFER_SIZE for dvr

[Andrea]

[-- Attachment #1: Type: text/plain, Size: 1503 bytes --]

I've updated this patch following your remarks about PATCH 1/3.

About your feedback, I am not sure I understand what you mean, but I will try:

  > Do not release the lock before the ringbuffer is consistent again.

If you mean that the lock should not be released while buf->data = NULL, this is *not* a problem.
A ringbuffer with NULL data *is* consistent (even though it is useless).
All functions in dmxdev.c check for a NULL pointer before calling read or write on the ringbuffer.
The same happens for the buffer of the demux.

  > We should not free the old buffer before we got a new one.

I like you idea, but I did not want to change the existing logic.

You might have noticed that the new function dvb_dvr_set_buffer_size is very similar to
dvb_dmxdev_set_buffer_size (which exists already): I did not want to introduce a new logic of
resizing the ringbuffer, but I've written a function very very similar.

Maybe in a following patch one could change both of them.

  > As the ring buffer can be written from an ISR, we have to use
    spin_lock_irqsave/spin_unlock_irqrestore here.

Ok, I've changed the patch.
If I use spin_lock_irqsave in dvb_dvr_set_buffer_size (called via IOCTL), do I need to use the same
kind of spin_lock on all other spin_lock on the same lock (e.g. dvb_dmxdev_ts_callback)?




Let me know if I should be more aggressive and change the resize for dvr and demux at once within
this patch. Otherwise if and when this is accepted I will rearrange the code.

Andrea


[-- Attachment #2: size2.diff --]
[-- Type: text/x-patch, Size: 1498 bytes --]

diff -r 1886a5ea2f84 linux/drivers/media/dvb/dvb-core/dmxdev.c
--- a/linux/drivers/media/dvb/dvb-core/dmxdev.c	Fri Mar 21 08:04:55 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dmxdev.c	Sat Mar 22 10:12:58 2008 +0000
@@ -259,6 +259,38 @@ static ssize_t dvb_dvr_read(struct file 
 	return ret;
 }
 
+static int dvb_dvr_set_buffer_size(struct dmxdev *dmxdev,
+				      unsigned long size)
+{
+	struct dvb_ringbuffer *buf = &dmxdev->dvr_buffer;
+	void *mem;
+
+	if (buf->size == size)
+		return 0;
+
+	dprintk("function : %s\n", __FUNCTION__);
+
+	spin_lock(&dmxdev->lock);
+	mem = buf->data;
+	buf->data = NULL;
+	buf->size = size;
+	dvb_ringbuffer_reset(buf);
+	
+	spin_unlock(&dmxdev->lock);
+	vfree(mem);
+
+	if (buf->size) {
+		mem = vmalloc(dmxdev->dvr_buffer.size);
+		if (!mem)
+			return -ENOMEM;
+		spin_lock(&dmxdev->lock);
+		buf->data = mem;
+		spin_unlock(&dmxdev->lock);
+	}
+
+	return 0;
+}
+
 static inline void dvb_dmxdev_filter_state_set(struct dmxdev_filter
 					       *dmxdevfilter, int state)
 {
@@ -1009,6 +1041,7 @@ static int dvb_dvr_do_ioctl(struct inode
 {
 	struct dvb_device *dvbdev = file->private_data;
 	struct dmxdev *dmxdev = dvbdev->priv;
+	unsigned long arg = (unsigned long)parg;
 	int ret;
 
 	if (mutex_lock_interruptible(&dmxdev->mutex))
@@ -1016,8 +1049,7 @@ static int dvb_dvr_do_ioctl(struct inode
 
 	switch (cmd) {
 	case DMX_SET_BUFFER_SIZE:
-		// FIXME: implement
-		ret = 0;
+		ret = dvb_dvr_set_buffer_size(dmxdev, arg);
 		break;
 
 	default:


[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

[linux-dvb] [PATCH] 2/3: implement DMX_SET_BUFFER_SIZE for dvr

[Andrea]

linux-dvb-request@linuxtv.org wrote:

 > What about this fragment:
 > 	...
 > 	if (!size)
 > 		return -EINVAL;
 >
 > 	mem = vmalloc(size);
 > 	if (!mem)
 > 		return -ENOMEM;
 >
 > 	mem2 = buf->data;
 >
 >       spin_lock_irqsave(&dmxdev->lock);
 >       buf->pread = buf->pwrite = 0;
 > 	buf->data = mem;
 > 	buf->size = size;
 > 	spin_unlock_irqrestore(&dmxdev->lock);
 >
 > 	vfree(mem2);
 > 	return 0;

Maybe I can think of one reason while the current code is not implemented this way:

In your version the new buffer is allocated before the old one is released.
In the current implementation the old buffer is released and afterwards the new one allocated.

One could argue that the new implementation has a maximum memory requirement higher than the old one.
It's not much but I am not too familiar with kernel development, so I don't know how important that 
could be.

What do you think?

About the spin_lock_irqsave: currently it is not used anywhere in the code for the demux in dmxdev.c.
I am always a bit scared when I introduce something new, maybe I am missing the current logic.

Cheers

Andrea

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] 2/3: implement DMX_SET_BUFFER_SIZE for dvr

[Oliver Endriss]

Andrea wrote:
> linux-dvb-request@linuxtv.org wrote:
huh?

>  > What about this fragment:
>  > 	...
>  > 	if (!size)
>  > 		return -EINVAL;
>  >
>  > 	mem = vmalloc(size);
>  > 	if (!mem)
>  > 		return -ENOMEM;
>  >
>  > 	mem2 = buf->data;
>  >
>  >       spin_lock_irqsave(&dmxdev->lock);
>  >       buf->pread = buf->pwrite = 0;
>  > 	buf->data = mem;
>  > 	buf->size = size;
>  > 	spin_unlock_irqrestore(&dmxdev->lock);
>  >
>  > 	vfree(mem2);
>  > 	return 0;
> 
> Maybe I can think of one reason while the current code is not implemented this way:
> 
> In your version the new buffer is allocated before the old one is released.

Yes, this is intentional. See below.

> In the current implementation the old buffer is released and afterwards the new one allocated.
> 
> One could argue that the new implementation has a maximum memory requirement higher than the old one.
> It's not much but I am not too familiar with kernel development, so I don't know how important that 
> could be.
> 
> What do you think?

- With your code the demux becomes unusable if the memory allocation
  failes for some reason. This should be avoided. It is better have a
  working demux with a smaller buffer than to have an defunct demux.

- If there is not enough memory for both buffers, your machine has a problem
  anyway, and you should not increase buffer size.

> About the spin_lock_irqsave: currently it is not used anywhere in the code for the demux in dmxdev.c.
> I am always a bit scared when I introduce something new, maybe I am missing the current logic.

I'm sorry, spin_lock_irqsave/spin_unlock_irqrestore was a typo.
We have to use spin_[un]lock_irq because buffer writing _might_ occur
in interrupt context. So the '_irq' is very important!

CU
Oliver

-- 
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
----------------------------------------------------------------

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] 2/3: implement DMX_SET_BUFFER_SIZE for dvr

[Andrea]

[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]

Oliver Endriss wrote:
> - With your code the demux becomes unusable if the memory allocation
>   failes for some reason. This should be avoided. It is better have a
>   working demux with a smaller buffer than to have an defunct demux.
> 
> - If there is not enough memory for both buffers, your machine has a problem
>   anyway, and you should not increase buffer size.
> 
 > ....
> I'm sorry, spin_lock_irqsave/spin_unlock_irqrestore was a typo.
> We have to use spin_[un]lock_irq because buffer writing _might_ occur
> in interrupt context. So the '_irq' is very important!
> 

Ok.

I've changed the second patch to
1) allocate the new buffer before releasing the old one
2) use spin_[un]lock_irq

3) On top of that, I have rearranged the code of DMX_SET_BUFFER_SIZE for the demux so that it does 
the same as the dvr (i.e. allocate the new buffer before releasing the old one). I think it is a 
good idea that 2 very similar functions are implemented in the same way. (if you don't agree, or if 
you think a 3rd separate patch for this point is a better idea, let me know.)

PS: Both patches 1/3 and 2/3 are against a clean v4l-dvb tree. I do not know how to generate 
incremental patch for 2/3.

Let me know what you think about that.

Andrea

[-- Attachment #2: patch.2 --]
[-- Type: text/plain, Size: 3682 bytes --]

diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dmxdev.c
--- a/linux/drivers/media/dvb/dvb-core/dmxdev.c	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dmxdev.c	Sun Apr 13 10:14:54 2008 +0100
@@ -259,6 +259,39 @@ static ssize_t dvb_dvr_read(struct file 
 	return ret;
 }
 
+static int dvb_dvr_set_buffer_size(struct dmxdev *dmxdev,
+				      unsigned long size)
+{
+	struct dvb_ringbuffer *buf = &dmxdev->dvr_buffer;
+	void *newmem;
+	void *oldmem;
+	
+	dprintk("function : %s\n", __FUNCTION__);
+	
+	if (buf->size == size)
+		return 0;
+	if (!size)
+		return -EINVAL;
+	
+	newmem = vmalloc(size);
+	if (!newmem)
+		return -ENOMEM;
+	
+	oldmem = buf->data;
+
+	spin_lock_irq(&dmxdev->lock);
+	buf->data = newmem;
+	buf->size = size;
+	
+	// reset and not flush in case the buffer shrinks
+	dvb_ringbuffer_reset(buf);
+	spin_unlock_irq(&dmxdev->lock);
+
+	vfree(oldmem);
+	
+	return 0;
+}
+
 static inline void dvb_dmxdev_filter_state_set(struct dmxdev_filter
 					       *dmxdevfilter, int state)
 {
@@ -271,28 +304,32 @@ static int dvb_dmxdev_set_buffer_size(st
 				      unsigned long size)
 {
 	struct dvb_ringbuffer *buf = &dmxdevfilter->buffer;
-	void *mem;
+	void *newmem;
+	void *oldmem;
 
 	if (buf->size == size)
 		return 0;
+	if (!size)
+		return -EINVAL;
 	if (dmxdevfilter->state >= DMXDEV_STATE_GO)
 		return -EBUSY;
+
+	newmem = vmalloc(size);
+	if (!newmem)
+		return -ENOMEM;
+
+	oldmem = buf->data;
+
 	spin_lock_irq(&dmxdevfilter->dev->lock);
-	mem = buf->data;
-	buf->data = NULL;
+	buf->data = newmem;
 	buf->size = size;
-	dvb_ringbuffer_flush(buf);
+	
+	// reset and not flush, in case the new buffer is smaller
+	dvb_ringbuffer_reset(buf);
 	spin_unlock_irq(&dmxdevfilter->dev->lock);
-	vfree(mem);
-
-	if (buf->size) {
-		mem = vmalloc(dmxdevfilter->buffer.size);
-		if (!mem)
-			return -ENOMEM;
-		spin_lock_irq(&dmxdevfilter->dev->lock);
-		buf->data = mem;
-		spin_unlock_irq(&dmxdevfilter->dev->lock);
-	}
+
+	vfree(oldmem);
+	
 	return 0;
 }
 
@@ -1009,6 +1046,7 @@ static int dvb_dvr_do_ioctl(struct inode
 {
 	struct dvb_device *dvbdev = file->private_data;
 	struct dmxdev *dmxdev = dvbdev->priv;
+	unsigned long arg = (unsigned long)parg;
 	int ret;
 
 	if (mutex_lock_interruptible(&dmxdev->mutex))
@@ -1016,8 +1054,7 @@ static int dvb_dvr_do_ioctl(struct inode
 
 	switch (cmd) {
 	case DMX_SET_BUFFER_SIZE:
-		// FIXME: implement
-		ret = 0;
+		ret = dvb_dvr_set_buffer_size(dmxdev, arg);
 		break;
 
 	default:
diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Sun Apr 13 10:14:54 2008 +0100
@@ -90,6 +90,11 @@ void dvb_ringbuffer_flush(struct dvb_rin
 	rbuf->error = 0;
 }
 
+void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf)
+{
+	rbuf->pread = rbuf->pwrite = 0;
+	rbuf->error = 0;
+}
 
 
 void dvb_ringbuffer_flush_spinlock_wakeup(struct dvb_ringbuffer *rbuf)
diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h	Sun Apr 13 10:14:54 2008 +0100
@@ -84,6 +84,12 @@ extern ssize_t dvb_ringbuffer_free(struc
 /* return the number of bytes waiting in the buffer */
 extern ssize_t dvb_ringbuffer_avail(struct dvb_ringbuffer *rbuf);
 
+/* 
+** Reset the read and write pointers to zero and flush the buffer
+** This counts as a read and write operation
+*/
+
+extern void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf);
 
 /* read routines & macros */
 /* ---------------------- */

[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] 2/3: implement DMX_SET_BUFFER_SIZE for dvr

[Andrea]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

Andrea wrote:
> Ok.
> 
> I've changed the second patch to
> 1) allocate the new buffer before releasing the old one
> 2) use spin_[un]lock_irq
> 
> 3) On top of that, I have rearranged the code of DMX_SET_BUFFER_SIZE for 
> the demux so that it does the same as the dvr (i.e. allocate the new 
> buffer before releasing the old one). I think it is a good idea that 2 
> very similar functions are implemented in the same way. (if you don't 
> agree, or if you think a 3rd separate patch for this point is a better 
> idea, let me know.)
> 
> PS: Both patches 1/3 and 2/3 are against a clean v4l-dvb tree. I do not 
> know how to generate incremental patch for 2/3.
> 
> Let me know what you think about that.
> 
> Andrea

I've fixed the patch to pass the "make checkpatch" check.

Andrea

[-- Attachment #2: patch.2 --]
[-- Type: text/plain, Size: 3675 bytes --]

diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dmxdev.c
--- a/linux/drivers/media/dvb/dvb-core/dmxdev.c	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dmxdev.c	Sun Apr 13 10:44:54 2008 +0100
@@ -259,6 +259,39 @@ static ssize_t dvb_dvr_read(struct file 
 	return ret;
 }
 
+static int dvb_dvr_set_buffer_size(struct dmxdev *dmxdev,
+				      unsigned long size)
+{
+	struct dvb_ringbuffer *buf = &dmxdev->dvr_buffer;
+	void *newmem;
+	void *oldmem;
+
+	dprintk("function : %s\n", __func__);
+
+	if (buf->size == size)
+		return 0;
+	if (!size)
+		return -EINVAL;
+
+	newmem = vmalloc(size);
+	if (!newmem)
+		return -ENOMEM;
+
+	oldmem = buf->data;
+
+	spin_lock_irq(&dmxdev->lock);
+	buf->data = newmem;
+	buf->size = size;
+
+	/* reset and not flush in case the buffer shrinks */
+	dvb_ringbuffer_reset(buf);
+	spin_unlock_irq(&dmxdev->lock);
+
+	vfree(oldmem);
+
+	return 0;
+}
+
 static inline void dvb_dmxdev_filter_state_set(struct dmxdev_filter
 					       *dmxdevfilter, int state)
 {
@@ -271,28 +304,32 @@ static int dvb_dmxdev_set_buffer_size(st
 				      unsigned long size)
 {
 	struct dvb_ringbuffer *buf = &dmxdevfilter->buffer;
-	void *mem;
+	void *newmem;
+	void *oldmem;
 
 	if (buf->size == size)
 		return 0;
+	if (!size)
+		return -EINVAL;
 	if (dmxdevfilter->state >= DMXDEV_STATE_GO)
 		return -EBUSY;
+
+	newmem = vmalloc(size);
+	if (!newmem)
+		return -ENOMEM;
+
+	oldmem = buf->data;
+
 	spin_lock_irq(&dmxdevfilter->dev->lock);
-	mem = buf->data;
-	buf->data = NULL;
+	buf->data = newmem;
 	buf->size = size;
-	dvb_ringbuffer_flush(buf);
+
+	/* reset and not flush, in case the new buffer is smaller */
+	dvb_ringbuffer_reset(buf);
 	spin_unlock_irq(&dmxdevfilter->dev->lock);
-	vfree(mem);
-
-	if (buf->size) {
-		mem = vmalloc(dmxdevfilter->buffer.size);
-		if (!mem)
-			return -ENOMEM;
-		spin_lock_irq(&dmxdevfilter->dev->lock);
-		buf->data = mem;
-		spin_unlock_irq(&dmxdevfilter->dev->lock);
-	}
+
+	vfree(oldmem);
+
 	return 0;
 }
 
@@ -1009,6 +1046,7 @@ static int dvb_dvr_do_ioctl(struct inode
 {
 	struct dvb_device *dvbdev = file->private_data;
 	struct dmxdev *dmxdev = dvbdev->priv;
+	unsigned long arg = (unsigned long)parg;
 	int ret;
 
 	if (mutex_lock_interruptible(&dmxdev->mutex))
@@ -1016,8 +1054,7 @@ static int dvb_dvr_do_ioctl(struct inode
 
 	switch (cmd) {
 	case DMX_SET_BUFFER_SIZE:
-		// FIXME: implement
-		ret = 0;
+		ret = dvb_dvr_set_buffer_size(dmxdev, arg);
 		break;
 
 	default:
diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Sun Apr 13 10:44:54 2008 +0100
@@ -90,6 +90,11 @@ void dvb_ringbuffer_flush(struct dvb_rin
 	rbuf->error = 0;
 }
 
+void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf)
+{
+	rbuf->pread = rbuf->pwrite = 0;
+	rbuf->error = 0;
+}
 
 
 void dvb_ringbuffer_flush_spinlock_wakeup(struct dvb_ringbuffer *rbuf)
diff -r 54cdcd915a6b linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h
--- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h	Fri Apr 11 08:29:44 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.h	Sun Apr 13 10:44:54 2008 +0100
@@ -84,6 +84,12 @@ extern ssize_t dvb_ringbuffer_free(struc
 /* return the number of bytes waiting in the buffer */
 extern ssize_t dvb_ringbuffer_avail(struct dvb_ringbuffer *rbuf);
 
+/*
+** Reset the read and write pointers to zero and flush the buffer
+** This counts as a read and write operation
+*/
+
+extern void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf);
 
 /* read routines & macros */
 /* ---------------------- */

[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] 2/3: implement DMX_SET_BUFFER_SIZE for dvr

[Oliver Endriss]

Andrea wrote:
> Andrea wrote:
> > Ok.
> > 
> > I've changed the second patch to
> > 1) allocate the new buffer before releasing the old one
> > 2) use spin_[un]lock_irq
> > 
> > 3) On top of that, I have rearranged the code of DMX_SET_BUFFER_SIZE for 
> > the demux so that it does the same as the dvr (i.e. allocate the new 
> > buffer before releasing the old one). I think it is a good idea that 2 
> > very similar functions are implemented in the same way. (if you don't 
> > agree, or if you think a 3rd separate patch for this point is a better 
> > idea, let me know.)
> > 
> > PS: Both patches 1/3 and 2/3 are against a clean v4l-dvb tree. I do not 
> > know how to generate incremental patch for 2/3.
> > 
> > Let me know what you think about that.
> > 
> > Andrea
> 
> I've fixed the patch to pass the "make checkpatch" check.
> 
> Andrea

Thanks. Afaics both patches look ok.

I'll sort out that incremental issue when I commit them.
If nobody complains I will do the check-in next weekend.

Please sign your patches. See README.patches for more information.
You can do this by replying to this thread with a message which contains
your 'Signed-off-by:'. Sorry for that. :-(

CU
Oliver

-- 
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
----------------------------------------------------------------

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] 2/3: implement DMX_SET_BUFFER_SIZE for dvr

[Andrea]

Andrea wrote:
> Andrea wrote:
>> Ok.
>>
>> I've changed the second patch to
>> 1) allocate the new buffer before releasing the old one
>> 2) use spin_[un]lock_irq
>>
>> 3) On top of that, I have rearranged the code of DMX_SET_BUFFER_SIZE 
>> for the demux so that it does the same as the dvr (i.e. allocate the 
>> new buffer before releasing the old one). I think it is a good idea 
>> that 2 very similar functions are implemented in the same way. (if you 
>> don't agree, or if you think a 3rd separate patch for this point is a 
>> better idea, let me know.)
>>
>> PS: Both patches 1/3 and 2/3 are against a clean v4l-dvb tree. I do 
>> not know how to generate incremental patch for 2/3.
>>
>> Let me know what you think about that.
>>
>> Andrea
> 
> I've fixed the patch to pass the "make checkpatch" check.
> 
> Andrea

Implementation of DMX_SET_BUFFER_SIZE for dvr.
Synchronization of the code of DMX_SET_BUFFER_SIZE for demux and dvr.

Signed-off-by: Andrea Odetti <mariofutire@gmail.com>

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] 2/3: implement DMX_SET_BUFFER_SIZE for dvr

[Oliver Endriss]

Andrea wrote:
> Andrea wrote:
> > Andrea wrote:
> >> Ok.
> >>
> >> I've changed the second patch to
> >> 1) allocate the new buffer before releasing the old one
> >> 2) use spin_[un]lock_irq
> >>
> >> 3) On top of that, I have rearranged the code of DMX_SET_BUFFER_SIZE 
> >> for the demux so that it does the same as the dvr (i.e. allocate the 
> >> new buffer before releasing the old one). I think it is a good idea 
> >> that 2 very similar functions are implemented in the same way. (if you 
> >> don't agree, or if you think a 3rd separate patch for this point is a 
> >> better idea, let me know.)
> >>
> >> PS: Both patches 1/3 and 2/3 are against a clean v4l-dvb tree. I do 
> >> not know how to generate incremental patch for 2/3.
> >>
> >> Let me know what you think about that.
> >>
> >> Andrea
> > 
> > I've fixed the patch to pass the "make checkpatch" check.
> > 
> > Andrea
> 
> Implementation of DMX_SET_BUFFER_SIZE for dvr.
> Synchronization of the code of DMX_SET_BUFFER_SIZE for demux and dvr.
> 
> Signed-off-by: Andrea Odetti <mariofutire@gmail.com>

Committed to HG. Thanks.

CU
Oliver

-- 
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
----------------------------------------------------------------

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

[linux-dvb] [PATCH] 2/3: implement DMX_SET_BUFFER_SIZE for dvr

[Andrea]

[-- Attachment #1: Type: text/plain, Size: 1347 bytes --]

This patch implements the ioctl DMX_SET_BUFFER_SIZE for the dvr.

The ioctl used to be supported but not yet implemented.

The way it works is that it replaces the ringbuffer with a new one.
Beforehand it flushes the old buffer.
This means that part of the stream is lost, and reading errors (like overflow) are cleaned.
The flushing is not a problem since this operation usually occurs at beginning before start reading.

Since the dvr is *always* up and running this change has to be done while the buffer is written:

1) On the userspace side, it is as safe as dvb_dvr_read is now:
	- dvb_dvr_set_buffer_size locks the mutex
	- dvb_dvr_read does *not* lock the mutex (the code is there commented out)

So as long as one does not call read simultaneously it works properly.
Maybe the mutex should be enforced in dvb_dvr_read.

2) On the kernel side
The only thing I am not 100% sure about is whether the spin_lock I've used is enough to guarantee
synchronization between the new function dvb_dvr_set_buffer_size (which uses spin_lock_irq) and
dvb_dmxdev_ts_callback and dvb_dmxdev_section_callback (using spin_lock).
But this looks to me the same as all other functions do.


I would like to change documentation about DMX_SET_BUFFER_SIZE, but I could not find the source of
http://www.linuxtv.org/docs/dvbapi/DVB_Demux_Device.html

Andrea






[-- Attachment #2: size.diff --]
[-- Type: text/x-patch, Size: 1497 bytes --]

diff -r 1886a5ea2f84 linux/drivers/media/dvb/dvb-core/dmxdev.c
--- a/linux/drivers/media/dvb/dvb-core/dmxdev.c	Fri Mar 21 08:04:55 2008 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dmxdev.c	Sat Mar 22 00:07:56 2008 +0000
@@ -259,6 +259,37 @@ static ssize_t dvb_dvr_read(struct file 
 	return ret;
 }
 
+static int dvb_dvr_set_buffer_size(struct dmxdev *dmxdev,
+				      unsigned long size)
+{
+	struct dvb_ringbuffer *buf = &dmxdev->dvr_buffer;
+	void *mem;
+
+	if (buf->size == size)
+		return 0;
+
+	dprintk("function : %s\n", __FUNCTION__);
+
+	spin_lock(&dmxdev->lock);
+	mem = buf->data;
+	buf->data = NULL;
+	buf->size = size;
+	dvb_ringbuffer_flush(buf);
+	spin_unlock(&dmxdev->lock);
+	vfree(mem);
+
+	if (buf->size) {
+		mem = vmalloc(dmxdev->dvr_buffer.size);
+		if (!mem)
+			return -ENOMEM;
+		spin_lock(&dmxdev->lock);
+		buf->data = mem;
+		spin_unlock(&dmxdev->lock);
+	}
+
+	return 0;
+}
+
 static inline void dvb_dmxdev_filter_state_set(struct dmxdev_filter
 					       *dmxdevfilter, int state)
 {
@@ -1009,6 +1040,7 @@ static int dvb_dvr_do_ioctl(struct inode
 {
 	struct dvb_device *dvbdev = file->private_data;
 	struct dmxdev *dmxdev = dvbdev->priv;
+	unsigned long arg = (unsigned long)parg;
 	int ret;
 
 	if (mutex_lock_interruptible(&dmxdev->mutex))
@@ -1016,8 +1048,7 @@ static int dvb_dvr_do_ioctl(struct inode
 
 	switch (cmd) {
 	case DMX_SET_BUFFER_SIZE:
-		// FIXME: implement
-		ret = 0;
+		ret = dvb_dvr_set_buffer_size(dmxdev, arg);
 		break;
 
 	default:




[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [PATCH] 2/3: implement DMX_SET_BUFFER_SIZE for dvr

[Oliver Endriss]

Andrea wrote:
> This patch implements the ioctl DMX_SET_BUFFER_SIZE for the dvr.
> 
> The ioctl used to be supported but not yet implemented.
> 
> The way it works is that it replaces the ringbuffer with a new one.
> Beforehand it flushes the old buffer.
> This means that part of the stream is lost, and reading errors (like overflow) are cleaned.
> The flushing is not a problem since this operation usually occurs at beginning before start reading.
> 
> Since the dvr is *always* up and running this change has to be done while the buffer is written:
> 
> 1) On the userspace side, it is as safe as dvb_dvr_read is now:
> 	- dvb_dvr_set_buffer_size locks the mutex
> 	- dvb_dvr_read does *not* lock the mutex (the code is there commented out)
> 
> So as long as one does not call read simultaneously it works properly.
> Maybe the mutex should be enforced in dvb_dvr_read.
> 
> 2) On the kernel side
> The only thing I am not 100% sure about is whether the spin_lock I've used is enough to guarantee
> synchronization between the new function dvb_dvr_set_buffer_size (which uses spin_lock_irq) and
> dvb_dmxdev_ts_callback and dvb_dmxdev_section_callback (using spin_lock).
> But this looks to me the same as all other functions do.

Please see my other mail, too

>       spin_lock(&dmxdev->lock);
>       mem = buf->data;
>       buf->data = NULL;
>       buf->size = size;
>       dvb_ringbuffer_flush(buf);
>       spin_unlock(&dmxdev->lock);
>       vfree(mem);
>
>       if (buf->size) {
>               mem = vmalloc(dmxdev->dvr_buffer.size);
>               if (!mem)
>                       return -ENOMEM;
>               spin_lock(&dmxdev->lock);
>               buf->data = mem;
>               spin_unlock(&dmxdev->lock);
>       }

I see some problems here:
- Do not release the lock before the ringbuffer is consistent again.
- We should not free the old buffer before we got a new one.
- As the ring buffer can be written from an ISR, we have to use
  spin_lock_irqsave/spin_unlock_irqrestore here.

What about this fragment:
	...
	if (!size)
		return -EINVAL;

	mem = vmalloc(size);
	if (!mem)
		return -ENOMEM;

	mem2 = buf->data;

        spin_lock_irqsave(&dmxdev->lock);
        buf->pread = buf->pwrite = 0;
	buf->data = mem;
	buf->size = size;
	spin_unlock_irqrestore(&dmxdev->lock);

	vfree(mem2);
	return 0;

Any comemnts? I hope that someone else also has a look at this because
I don't have much time atm.

CU
Oliver

-- 
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
----------------------------------------------------------------

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb