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@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
