Re: [linux-dvb] [RFC-final] videobuf tree
Em Dom, 2007-10-07 às 14:03 -0700, Trent Piepho escreveu:
On Sun, 7 Oct 2007, Mauro Carvalho Chehab wrote:
I took a look at cx23885 code. It seems that there's a serious error on
the way you're using cx23885_buffer there:
cx23885-dvb.c: return cx23885_buf_prepare(q, port, (struct
cx23885_buffer*)vb, field);
cx23885-dvb.c: cx23885_buf_queue(port, (struct cx23885_buffer*)vb);
cx23885-dvb.c: cx23885_free_buffer(q, (struct cx23885_buffer*)vb);
It seems that you are forcing videobuf_buffer to be cx23885_buffer. This
is not right!
This is what is defined on cx23885.h:
struct cx23885_buffer {
/* common v4l buffer stuff -- must be first */
struct videobuf_buffer vb;
I'm not sure that it is competely wrong. Say one has a cx23885_buffer that
contains a videobuf_buffer. Now suppose you have a pointer to the
videobuf_buffer, and you want to get a pointer to the cx23885_buffer that
contains it. What you should write is:
struct videobuf_buffer *vb = ...;
struct cx23885_buffer *buf = container_of(vb, struct cx23885_buffer, vb);
But since vb is the first field of the cx23885_buffer struct, the container_of
will turn into just '(struct cx23885_buffer *)(vb)
This code in videobuf-dma-sg.c looks odd to me:
/* Allocated area consists on 3 parts:
struct video_buffer
struct driver_buffer (cx88_buffer, saa7134_buf, ...)
struct videobuf_pci_sg_memory
static void *__videobuf_alloc(size_t size)
{
struct videbuf_pci_sg_memory *mem;
struct videobuf_buffer *vb;
vb = kzalloc(size+sizeof(*mem),GFP_KERNEL);
mem = vb-priv = ((char *)vb)+size;
What is 'size', is that the size of the driver buffer? Shouldn't you
be
allocating size + sizeof(*vb) + sizeof(*mem)?
Is there documentation for videobuf anywhere? It doesn't look like
any of
the videobuf functions have descriptions of that they do or what the
parameters are.
There aren't any videobuf doc yet. I intend to write one, when I have
some time. IMO, this is the most complex part of V4L core.
For now, let me give a quick explanation of the basics of videobuf.
---
Videobuf uses a memory struct that looks what c++ compilers do when you
use inheritances.
It is something like:
class videobuf_core {
public:
// some data and code
};
class videobuf_dma_sg: public videobuf_core {
private:
// some data and code
}
class foo_buffer: public videobuf_dma_sg {
public:
// some data
}
The constructor for any class derivated from videobuf_dma_sg is:
void *videobuf_pci_alloc (size_t size);
Where size is the size of foo_buffer.
The other videobuf_dma methods are the external functions defined on
videobuf-dma-sg.h. All of them start with videobuf_dma_foo.
A similar inehitance concept happens with videobuf_queue. You have an
abstract videobuf_queue class, defined on videobuf-core, where almost
all methods are virtual, and, currently, two derivated class, that
implements their methods: a DMA Scatter/Gather one (videobuf-dma-sg) and
a vmalloc one (videobuf-vmalloc).
To use the full functionality of videobuf, you will need the
videobuf_queue, that is responsible for controlling the state machine of
the videobuffers. The videobuf_queue constructor will allocate a
videobuf_buffer inherited class. Also, when you need mmapped buffers,
videobuf core will allocate one additional videobuf_buffer inherited
class by each queue (generally, a driver allocates, by default, 8 queues
for receiving data - this avoids data loss, when the machine is with
high workloads).
It is also possible just to use a videobuf_buffer derivated class. ALSA
saa7134 and cx88 drivers implement this way. They have their own state
machine.
--
Cheers,
Mauro
___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [RFC-final] videobuf tree
For now, let me give a quick explanation of the basics of videobuf.
---
(part 2)
As you know, the original author of videobuf is Gerd. At the changes I
did, I've tried to preserve, as much as possible, the code outside
videobuf without changes(*).
(*) This is also true for the binary code that videobuf-core +
videobuf_dma_sg executes: It is almost the same as the original
videobuf, but separated into two separate modules, with distinct
functions.
a) The first one is about the inherited videobuf_buffer class at each
driver.
The approach used on videobuf is somehow different from what other parts
of the Linux Kernel does. For this to work, the first part of a
videobuf_buffer inherited code should do:
cx23885_buffer {
struct videbuf_buffer;
...
}
Otherwise, the videobuf code will fail.
IMO, it would be better to use container_of as you suggested, but this
would mean rewrite more code at the drivers, and do more tests.
b) The destructor used to free videobuf_buffer memory is just kfree. So,
all memory for the entire class should be allocated with just one
kmalloc. The memory model is something like:
struct derivated_class {
struct cx23885_buffer {
struct videobuf_buffer;
// cx23885 own data
}
// dma s/g own data
};
So, videobuf_pci_malloc do something like:
buf=kmalloc (sizeof(struct derivated_class), GFP_KERNEL);
To free a videobuf, you just need to do:
free (buf);
--
Cheers,
Mauro
___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [RFC-final] videobuf tree
Michael, However, cx23885 is now broken. Upon starting a DVB stream, the following OOPS is generated: I've reviewed cx23885 videobuf stuff. I noticed a problem at the conversions: It is still using the abstract videobuf constructor, instead of the pci DMA S/G one. I've just added a patch fixing this at v4l-dvb tree. Probably, this will fix the issue. Please test. - Cheers, Mauro ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [RFC-final] videobuf tree
Mauro Carvalho Chehab wrote: Michael, However, cx23885 is now broken. Upon starting a DVB stream, the following OOPS is generated: I've reviewed cx23885 videobuf stuff. I noticed a problem at the conversions: It is still using the abstract videobuf constructor, instead of the pci DMA S/G one. I've just added a patch fixing this at v4l-dvb tree. Probably, this will fix the issue. Mauro, This new patch fixed the problem. CX23885 functionality is restored! :-) side note: If we had left a single header, video-buf.h, we could have avoided this problem. When we rename files like this, we run the risk of building against the wrong headers, if errors are not caught corrected quickly enough. Are you open to merging the videobuf-*.h into a single video-buf.h header file, to match the filename in previous kernel versions so that we can avoid this issue from recurring? Either that, or including the current headers into a new video-buf.h would do the same job. What do you think? -Mike ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [RFC-final] videobuf tree
Mauro, This new patch fixed the problem. CX23885 functionality is restored! :-) Good! If you send your reviewed-by, I'll add at the proper changesets touching videobuf. side note: If we had left a single header, video-buf.h, we could have avoided this problem. When we rename files like this, we run the risk of building against the wrong headers, if errors are not caught corrected quickly enough. Are you open to merging the videobuf-*.h into a single video-buf.h header file, to match the filename in previous kernel versions so that we can avoid this issue from recurring? Either that, or including the current headers into a new video-buf.h would do the same job. What do you think? I don't like to create a video-buf.h header. This will make non-pci devices dependent on PCI, or will require some additional logic for checking kernel Kconfig symbols. I also expect that other newer videobuf methods to be created. So, this header will just generate undesirable mess. What we might do is to rename the generic abstract method to another name. This will generate compilation errors, making easier for people to realize what happened. I'm not sure if this is valuable enough, since I don't know any other DMA S/G driver using videobuf being developed for their inclusion at Kernel. Cheers, Mauro ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [RFC-final] videobuf tree
Mauro Carvalho Chehab wrote: Mauro, This new patch fixed the problem. CX23885 functionality is restored! :-) Good! If you send your reviewed-by, I'll add at the proper changesets touching videobuf. 3762b92e232a - V4L/DVB (6287) - Fix DMA Scatter/Gather constructor Reviewed-by: Michael Krufky [EMAIL PROTECTED] side note: If we had left a single header, video-buf.h, we could have avoided this problem. When we rename files like this, we run the risk of building against the wrong headers, if errors are not caught corrected quickly enough. Are you open to merging the videobuf-*.h into a single video-buf.h header file, to match the filename in previous kernel versions so that we can avoid this issue from recurring? Either that, or including the current headers into a new video-buf.h would do the same job. What do you think? I don't like to create a video-buf.h header. This will make non-pci devices dependent on PCI, or will require some additional logic for checking kernel Kconfig symbols. I also expect that other newer videobuf methods to be created. So, this header will just generate undesirable mess. This would not create dependencies of non-pci devices on pci -- a simple header inclusion is optimized by the c compiler -- only needed methods are considered and are actually included in the build. What we might do is to rename the generic abstract method to another name. This will generate compilation errors, making easier for people to realize what happened. If we rename it to video-buf.h, it would cover the issue that I have in mind. I'm not sure if this is valuable enough, since I don't know any other DMA S/G driver using videobuf being developed for their inclusion at Kernel. Maybe an out of tree driver uses it? Maybe there is an in-tree driver that still might have old methods being used but we didnt hit those bugs yet due to incomplete testing methods? Cheers, Mike ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [RFC-final] videobuf tree
I don't like to create a video-buf.h header. This will make non-pci
devices dependent on PCI, or will require some additional logic for
checking kernel Kconfig symbols. I also expect that other newer videobuf
methods to be created. So, this header will just generate undesirable
mess.
This would not create dependencies of non-pci devices on pci -- a
simple header inclusion is optimized by the c compiler -- only needed
methods are considered and are actually included in the build.
Wrong. This has nothing to do with c optimizer. Try this:
Create this as header.h:
struct foo {
struct some_pci_struct foo;
};
Create this as main.c:
#include header.h
int main (void)
{
return 0;
}
You will got the following error:
/tmp/header.h:2: error: field ‘foo’ has incomplete type
If we use your approach, a non-pci videobuf driver will work only at the
architectures where you have a PCI bus (since the pci structs won't
exist on that architecture).
What we might do is to rename the generic abstract method to another
name. This will generate compilation errors, making easier for people to
realize what happened.
If we rename it to video-buf.h, it would cover the issue that I have in mind.
It is much more clear to include videobuf-vmalloc or videobuf-dma-sg,
depending on the proper videobuf module that will be needed by a driver.
Creating a video-buf.h that just includes the two will be unused by the
drivers. So, what's the sense of creating a header file at the kernel
that aren't used inside kernel?
I'm not sure if this is valuable enough, since I don't know any other
DMA S/G driver using videobuf being developed for their inclusion at
Kernel.
Maybe an out of tree driver uses it?
Although there's no intention to make life harder for out-of-tree
drivers, they shouldn't be considered on changes made at kernel
internals. The proper place for a kernel driver is in-kernel, not
outside.
Anyway, a compilation with a driver including video-buf.h currently will
generate an error, indicating that something has changed at v4l
infrastructure. By creating a header like you're proposing won't fix
this.
Maybe there is an in-tree driver that still might have old methods being
used but we didnt hit those bugs yet due to incomplete testing methods?
The only in-kernel drivers that use videobuf infrastructure are:
cx88, saa7134, bttv, saa7146 and, now, cx23885.
After the patch, all of they are already converted.
grep videobuf_queue_pci_init *.c
bttv-driver.c: videobuf_queue_pci_init(fh-cap, bttv_video_qops,
bttv-driver.c: videobuf_queue_pci_init(fh-vbi, bttv_vbi_qops,
cx23885-dvb.c: videobuf_queue_pci_init(port-dvb.dvbq, dvb_qops,
dev-pci, port-slock,
cx88-blackbird.c: videobuf_queue_pci_init(fh-mpegq,
blackbird_qops,
cx88-dvb.c: videobuf_queue_pci_init(dev-dvb.dvbq, dvb_qops,
cx88-video.c: videobuf_queue_pci_init(fh-vidq, cx8800_video_qops,
cx88-video.c: videobuf_queue_pci_init(fh-vbiq, cx8800_vbi_qops,
saa7134-dvb.c: videobuf_queue_pci_init(dev-dvb.dvbq,
saa7134_ts_qops,
saa7134-empress.c: videobuf_queue_pci_init(dev-empress_tsq,
saa7134_ts_qops,
saa7134-video.c:videobuf_queue_pci_init(fh-cap, video_qops,
saa7134-video.c:videobuf_queue_pci_init(fh-vbi,
saa7134_vbi_qops,
saa7146_vbi.c: videobuf_queue_pci_init(fh-vbi_q, vbi_qops,
saa7146_video.c:videobuf_queue_pci_init(fh-video_q,
video_qops,
videobuf-dma-sg.c:void videobuf_queue_pci_init(struct videobuf_queue* q,
There's no other driver using the abstract videobuf_queue_init.
A final point for your consideration:
videobuf_queue_init is the only function that had its meaning changed
without changing its parameters (currently, it is just an abstract
method. In the past, this were the function that were used to initialize
a videobuf queue).
The changes you're proposing won't solve the target you've addressed in
the first place, since it will still compile without warning, if you
forget to rename it to videobuf_queue_pci_init.
The proper change is simply doing something like:
s/videobuf_queue_init/videobuf_queue_abstract_init/
After this, on all places where videobuf stuff is used without
conversion, an error will be generated.
After my considerations, if you still think this is important, I'll
accept a patch renaming the old videobuf_queue_init to a new name.
--
Cheers,
Mauro
___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [RFC-final] videobuf tree
Mauro Carvalho Chehab wrote:
I don't like to create a video-buf.h header. This will make non-pci
devices dependent on PCI, or will require some additional logic for
checking kernel Kconfig symbols. I also expect that other newer videobuf
methods to be created. So, this header will just generate undesirable
mess.
This would not create dependencies of non-pci devices on pci -- a
simple header inclusion is optimized by the c compiler -- only needed
methods are considered and are actually included in the build.
Wrong. This has nothing to do with c optimizer. Try this:
Create this as header.h:
struct foo {
struct some_pci_struct foo;
};
Create this as main.c:
#include header.h
int main (void)
{
return 0;
}
You will got the following error:
/tmp/header.h:2: error: field ‘foo’ has incomplete type
If we use your approach, a non-pci videobuf driver will work only at the
architectures where you have a PCI bus (since the pci structs won't
exist on that architecture).
OK, I wasn't thinking clearly when I had written that at first... We
would have to use some #ifdef logic within video-buf.h in order to avoid
this, but that's ugly, and more trouble than it's worth.
You've proven your point.
What we might do is to rename the generic abstract method to another
name. This will generate compilation errors, making easier for people to
realize what happened.
If we rename it to video-buf.h, it would cover the issue that I have in
mind.
It is much more clear to include videobuf-vmalloc or videobuf-dma-sg,
depending on the proper videobuf module that will be needed by a driver.
Creating a video-buf.h that just includes the two will be unused by the
drivers. So, what's the sense of creating a header file at the kernel
that aren't used inside kernel?
It would just be nice to have errors when a module calls a function
defined in the old header, rather than the new one.
This is a concern, but a minor one, as this is only an issue during the
transitional kernels. If we've fixed all instances, then there should
be nothing to worry about.
In general, I just don't like modules building against the wrong headers
and not reporting any errors... I guess we'll just have to live with it
this way for now.
I'm not sure if this is valuable enough, since I don't know any other
DMA S/G driver using videobuf being developed for their inclusion at
Kernel.
Maybe an out of tree driver uses it?
Although there's no intention to make life harder for out-of-tree
drivers, they shouldn't be considered on changes made at kernel
internals. The proper place for a kernel driver is in-kernel, not
outside.
Anyway, a compilation with a driver including video-buf.h currently will
generate an error, indicating that something has changed at v4l
infrastructure. By creating a header like you're proposing won't fix
this.
Maybe there is an in-tree driver that still might have old methods
being used but we didnt hit those bugs yet due to incomplete testing
methods?
The only in-kernel drivers that use videobuf infrastructure are:
cx88, saa7134, bttv, saa7146 and, now, cx23885.
After the patch, all of they are already converted.
grep videobuf_queue_pci_init *.c
bttv-driver.c: videobuf_queue_pci_init(fh-cap, bttv_video_qops,
bttv-driver.c: videobuf_queue_pci_init(fh-vbi, bttv_vbi_qops,
cx23885-dvb.c: videobuf_queue_pci_init(port-dvb.dvbq, dvb_qops,
dev-pci, port-slock,
cx88-blackbird.c: videobuf_queue_pci_init(fh-mpegq,
blackbird_qops,
cx88-dvb.c: videobuf_queue_pci_init(dev-dvb.dvbq, dvb_qops,
cx88-video.c: videobuf_queue_pci_init(fh-vidq, cx8800_video_qops,
cx88-video.c: videobuf_queue_pci_init(fh-vbiq, cx8800_vbi_qops,
saa7134-dvb.c: videobuf_queue_pci_init(dev-dvb.dvbq,
saa7134_ts_qops,
saa7134-empress.c: videobuf_queue_pci_init(dev-empress_tsq,
saa7134_ts_qops,
saa7134-video.c:videobuf_queue_pci_init(fh-cap, video_qops,
saa7134-video.c:videobuf_queue_pci_init(fh-vbi,
saa7134_vbi_qops,
saa7146_vbi.c: videobuf_queue_pci_init(fh-vbi_q, vbi_qops,
saa7146_video.c:videobuf_queue_pci_init(fh-video_q,
video_qops,
videobuf-dma-sg.c:void videobuf_queue_pci_init(struct videobuf_queue* q,
There's no other driver using the abstract videobuf_queue_init.
OK, we should be fine, then.
A final point for your consideration:
videobuf_queue_init is the only function that had its meaning changed
without changing its parameters (currently, it is just an abstract
method. In the past, this were the function that were used to initialize
a videobuf queue).
The changes you're proposing won't solve the target you've addressed in
the first place, since it will still compile without warning, if you
forget to rename it to videobuf_queue_pci_init.
The proper change is simply doing something like:
Re: [linux-dvb] [RFC-final] videobuf tree
Mauro Carvalho Chehab wrote: Hi Michael, Please try the enclosed patch. It is just a hack. Please, post the dmesg, working or not. Mauro, Your patch touches code that apparently is not being executed in this case. I've enclosed dmesg anyway (see attached) Regards, Mike [0.00] Linux version 2.6.20-16-generic ([EMAIL PROTECTED]) (gcc version 4.1.2 (Ubuntu 4.1.2-0ubuntu4)) #2 SMP Thu Aug 30 23:16:15 UTC 2007 (Ubuntu 2.6.20-16.31-generic) [0.00] Command line: root=UUID=600107c6-8f24-4dde-8730-24743aa335ec ro quiet splash [0.00] BIOS-provided physical RAM map: [0.00] BIOS-e820: - 0009ac00 (usable) [0.00] BIOS-e820: 000f - 0010 (reserved) [0.00] BIOS-e820: 0010 - 3f651c00 (usable) [0.00] BIOS-e820: 3f651c00 - 3f653c00 (ACPI NVS) [0.00] BIOS-e820: 3f655c00 - 3f657c00 (ACPI data) [0.00] BIOS-e820: 3f657c00 - 4000 (reserved) [0.00] BIOS-e820: e000 - f000 (reserved) [0.00] BIOS-e820: fec0 - fed00400 (reserved) [0.00] BIOS-e820: fed2 - feda (reserved) [0.00] BIOS-e820: fee0 - fef0 (reserved) [0.00] BIOS-e820: ffb0 - 0001 (reserved) [0.00] Entering add_active_range(0, 0, 154) 0 entries of 3200 used [0.00] Entering add_active_range(0, 256, 259665) 1 entries of 3200 used [0.00] end_pfn_map = 1048576 [0.00] DMI 2.3 present. [0.00] ACPI: RSDP (v002 DELL ) @ 0x000febf0 [0.00] ACPI: XSDT (v001 DELLB8K 0x0014 ASL 0x0061) @ 0x000fd085 [0.00] ACPI: FADT (v003 DELLB8K 0x0014 ASL 0x0061) @ 0x000fd19d [0.00] ACPI: SSDT (v001 DELLst_ex 0x1000 INTL 0x20050624) @ 0xfffdafc7 [0.00] ACPI: MADT (v001 DELLB8K 0x0014 ASL 0x0061) @ 0x000fd291 [0.00] ACPI: BOOT (v001 DELLB8K 0x0014 ASL 0x0061) @ 0x000fd323 [0.00] ACPI: MCFG (v001 DELLB8K 0x0014 ASL 0x0061) @ 0x000fd34b [0.00] ACPI: HPET (v001 DELLB8K 0x0014 ASL 0x0061) @ 0x000fd389 [0.00] ACPI: SLIC (v001 DELLB8K 0x0014 ASL 0x0061) @ 0x000fd3c1 [0.00] ACPI: DSDT (v001 DELLdt_ex 0x1000 INTL 0x20050624) @ 0x [0.00] No NUMA configuration found [0.00] Faking a node at -3f651000 [0.00] Entering add_active_range(0, 0, 154) 0 entries of 3200 used [0.00] Entering add_active_range(0, 256, 259665) 1 entries of 3200 used [0.00] Bootmem setup node 0 -3f651000 [0.00] Zone PFN ranges: [0.00] DMA 0 - 4096 [0.00] DMA324096 - 1048576 [0.00] Normal1048576 - 1048576 [0.00] early_node_map[2] active PFN ranges [0.00] 0:0 - 154 [0.00] 0: 256 - 259665 [0.00] On node 0 totalpages: 259563 [0.00] DMA zone: 56 pages used for memmap [0.00] DMA zone: 1091 pages reserved [0.00] DMA zone: 2847 pages, LIFO batch:0 [0.00] DMA32 zone: 3494 pages used for memmap [0.00] DMA32 zone: 252075 pages, LIFO batch:31 [0.00] Normal zone: 0 pages used for memmap [0.00] ACPI: PM-Timer IO Port: 0x808 [0.00] ACPI: Local APIC address 0xfee0 [0.00] ACPI: LAPIC (acpi_id[0x01] lapic_id[0x00] enabled) [0.00] Processor #0 (Bootup-CPU) [0.00] ACPI: LAPIC (acpi_id[0x02] lapic_id[0x01] enabled) [0.00] Processor #1 [0.00] ACPI: LAPIC (acpi_id[0x03] lapic_id[0x05] disabled) [0.00] ACPI: LAPIC (acpi_id[0x04] lapic_id[0x07] disabled) [0.00] ACPI: LAPIC (acpi_id[0x05] lapic_id[0x00] disabled) [0.00] ACPI: LAPIC (acpi_id[0x06] lapic_id[0x01] disabled) [0.00] ACPI: LAPIC (acpi_id[0x07] lapic_id[0x02] disabled) [0.00] ACPI: LAPIC (acpi_id[0x08] lapic_id[0x03] disabled) [0.00] ACPI: LAPIC_NMI (acpi_id[0xff] high level lint[0x1]) [0.00] ACPI: IOAPIC (id[0x08] address[0xfec0] gsi_base[0]) [0.00] IOAPIC[0]: apic_id 8, address 0xfec0, GSI 0-23 [0.00] ACPI: INT_SRC_OVR (bus 0 bus_irq 0 global_irq 2 dfl dfl) [0.00] ACPI: INT_SRC_OVR (bus 0 bus_irq 9 global_irq 9 high level) [0.00] ACPI: IRQ0 used by override. [0.00] ACPI: IRQ2 used by override. [0.00] ACPI: IRQ9 used by override. [0.00] Setting APIC routing to physical flat [0.00] ACPI: HPET id: 0x8086a201 base: 0xfed0 [0.00] Using ACPI (MADT) for SMP configuration information [0.00] Nosave
Re: [linux-dvb] [RFC-final] videobuf tree
Hi Michael,
Em Dom, 2007-10-07 às 02:30 -0400, Michael Krufky escreveu:
Mauro Carvalho Chehab wrote:
Hi Michael,
Please try the enclosed patch. It is just a hack.
Please, post the dmesg, working or not.
Mauro,
Your patch touches code that apparently is not being executed in this case.
I've enclosed dmesg anyway (see attached)
I took a look at cx23885 code. It seems that there's a serious error on
the way you're using cx23885_buffer there:
cx23885-dvb.c: return cx23885_buf_prepare(q, port, (struct
cx23885_buffer*)vb, field);
cx23885-dvb.c: cx23885_buf_queue(port, (struct cx23885_buffer*)vb);
cx23885-dvb.c: cx23885_free_buffer(q, (struct cx23885_buffer*)vb);
It seems that you are forcing videobuf_buffer to be cx23885_buffer. This
is not right!
This is what is defined on cx23885.h:
struct cx23885_buffer {
/* common v4l buffer stuff -- must be first */
struct videobuf_buffer vb;
/* cx23885 specific */
unsigned int bpl;
struct btcx_riscmemrisc;
struct cx23885_fmt *fmt;
u32count;
};
You should notice that cx23885_buffer size is bigger than
videobuf_buffer. If you just force one to be equal to the other, you'll
use more memory than the allocated one!
Also, videobuf code will also add some extra bytes at the alloced memory
for its own consuption.
The new videobuf code has some magic to protect about this bad usage,
otherwise, you may risk to corrupt other memory areas with dma
transfers. I lost part of my reiserfs btrees once, with a bad code like
this.
What it should be done, is to use the allocated area provided by
videobuf_queue_pci_init.
I dunno how this ever worked!
Cheers,
Mauro
___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [RFC-final] videobuf tree
On Sun, 7 Oct 2007, Mauro Carvalho Chehab wrote:
I took a look at cx23885 code. It seems that there's a serious error on
the way you're using cx23885_buffer there:
cx23885-dvb.c: return cx23885_buf_prepare(q, port, (struct
cx23885_buffer*)vb, field);
cx23885-dvb.c: cx23885_buf_queue(port, (struct cx23885_buffer*)vb);
cx23885-dvb.c: cx23885_free_buffer(q, (struct cx23885_buffer*)vb);
It seems that you are forcing videobuf_buffer to be cx23885_buffer. This
is not right!
This is what is defined on cx23885.h:
struct cx23885_buffer {
/* common v4l buffer stuff -- must be first */
struct videobuf_buffer vb;
I'm not sure that it is competely wrong. Say one has a cx23885_buffer that
contains a videobuf_buffer. Now suppose you have a pointer to the
videobuf_buffer, and you want to get a pointer to the cx23885_buffer that
contains it. What you should write is:
struct videobuf_buffer *vb = ...;
struct cx23885_buffer *buf = container_of(vb, struct cx23885_buffer, vb);
But since vb is the first field of the cx23885_buffer struct, the container_of
will turn into just '(struct cx23885_buffer *)(vb)'
This code in videobuf-dma-sg.c looks odd to me:
/* Allocated area consists on 3 parts:
struct video_buffer
struct driver_buffer (cx88_buffer, saa7134_buf, ...)
struct videobuf_pci_sg_memory
static void *__videobuf_alloc(size_t size)
{
struct videbuf_pci_sg_memory *mem;
struct videobuf_buffer *vb;
vb = kzalloc(size+sizeof(*mem),GFP_KERNEL);
mem = vb-priv = ((char *)vb)+size;
What is 'size', is that the size of the driver buffer? Shouldn't you be
allocating size + sizeof(*vb) + sizeof(*mem)?
Is there documentation for videobuf anywhere? It doesn't look like any of
the videobuf functions have descriptions of that they do or what the
parameters are.
___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [RFC-final] videobuf tree
Am Sonntag, den 07.10.2007, 14:03 -0700 schrieb Trent Piepho:
On Sun, 7 Oct 2007, Mauro Carvalho Chehab wrote:
I took a look at cx23885 code. It seems that there's a serious error on
the way you're using cx23885_buffer there:
cx23885-dvb.c: return cx23885_buf_prepare(q, port, (struct
cx23885_buffer*)vb, field);
cx23885-dvb.c: cx23885_buf_queue(port, (struct cx23885_buffer*)vb);
cx23885-dvb.c: cx23885_free_buffer(q, (struct cx23885_buffer*)vb);
It seems that you are forcing videobuf_buffer to be cx23885_buffer. This
is not right!
This is what is defined on cx23885.h:
struct cx23885_buffer {
/* common v4l buffer stuff -- must be first */
struct videobuf_buffer vb;
I'm not sure that it is competely wrong. Say one has a cx23885_buffer that
contains a videobuf_buffer. Now suppose you have a pointer to the
videobuf_buffer, and you want to get a pointer to the cx23885_buffer that
contains it. What you should write is:
struct videobuf_buffer *vb = ...;
struct cx23885_buffer *buf = container_of(vb, struct cx23885_buffer, vb);
But since vb is the first field of the cx23885_buffer struct, the container_of
will turn into just '(struct cx23885_buffer *)(vb)'
This code in videobuf-dma-sg.c looks odd to me:
/* Allocated area consists on 3 parts:
struct video_buffer
struct driver_buffer (cx88_buffer, saa7134_buf, ...)
struct videobuf_pci_sg_memory
static void *__videobuf_alloc(size_t size)
{
struct videbuf_pci_sg_memory *mem;
struct videobuf_buffer *vb;
vb = kzalloc(size+sizeof(*mem),GFP_KERNEL);
mem = vb-priv = ((char *)vb)+size;
What is 'size', is that the size of the driver buffer? Shouldn't you be
allocating size + sizeof(*vb) + sizeof(*mem)?
Is there documentation for videobuf anywhere? It doesn't look like any of
the videobuf functions have descriptions of that they do or what the
parameters are.
As far as I know, there is no further documentation, except the little
what is on the list.
It is work done by Gerd, and we almost always had fun.
Hermann
___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [RFC-final] videobuf tree
On 10/3/07, Michael Krufky [EMAIL PROTECTED] wrote: On 10/3/07, Ricardo Cerqueira [EMAIL PROTECTED] wrote: I've tested this with a blackbird board (HVR-1300), both with the MPEG encoder and analog. - MPEG is working fine, even after merging in Mike Krufky's and my own blackbird patches for audio. No drops I can see. - Raw analog video is OK - Analog audio through cx88-alsa (which uses videobuf) is also fine. - Playing raw and MPEG simultaneosly works (as long as raw is started first, something that's also happening with the old videobuf) Summarizing: there's no visible performance or functional difference between the new and the old videobuf implementations on the hardware I have available to test. Reviewed-by: Ricardo Cerqueira [EMAIL PROTECTED] Ah, this is great news! I look forward to testing the new tree using cx88-blackbird, cx88-dvb and cx23885 over the upcoming weekend. I'll report whether I have the same results as soon as I can. I've tested the master branch under the following conditions: 1) cx88 raw analog video 2) cx88-blackbird encoded mpeg stream 3) cx88-dvb mpeg TS I'm pleased to report that the above three tests worked out successfully. However, cx23885 is now broken. Upon starting a DVB stream, the following OOPS is generated: [ 280.562598] Unable to handle kernel NULL pointer dereference at RIP: [ 280.562609] [881a0931] :videobuf_core:videobuf_mmap_setup+0x21/0xf0 [ 280.562637] PGD 204fc067 PUD 20504067 PMD 0 [ 280.562642] Oops: [1] SMP [ 280.562646] CPU 1 [ 280.562648] Modules linked in: binfmt_misc rfcomm l2cap bluetooth ppdev i915 drm cpufreq_userspace cpufreq_stats cpufreq_ondemand freq_table cpufreq_conse rvative cpufreq_powersave tc1100_wmi sony_acpi dev_acpi pcc_acpi sbs button ac asus_acpi backlight dock i2c_ec container battery video nls_utf8 ntfs nls_iso8 859_1 nls_cp437 vfat fat ipv6 parport_pc lp parport fuse mt2131 s5h1409 cx88_blackbird cx2341x rtc_isl1208 ir_kbd_i2c snd_seq_dummy snd_seq_oss dvb_pll lgdt3 30x snd_hda_intel snd_seq_midi tuner snd_hda_codec cx88_dvb cx88_vp3054_i2c tea5767 tda8290 tuner_simple mt20xx cx23885 cx88_alsa snd_pcm_oss snd_mixer_oss v ideobuf_dvb snd_pcm dvb_core snd_rawmidi snd_seq_midi_event snd_seq snd_timer snd_seq_device snd cx8800 cx8802 cx88xx soundcore pcspkr psmouse serio_raw ir_c ommon compat_ioctl32 i2c_algo_bit tveeprom i2c_core iTCO_wdt iTCO_vendor_support videodev v4l2_common v4l1_compat videobuf_dma_sg videobuf_core btcx_risc shp chp pci_hotplug snd_page_alloc intel_agp af_packet tsdev evdev ext3 jbd mbcache sg sd_mod sr_mod cdrom usbhid hid ehci_hcd ahci libata scsi_mod uhci_hcd e100 0 usbcore thermal processor fan fbcon tileblit font bitblit softcursor vesafb cfbcopyarea cfbimgblt cfbfillrect capability commoncap [ 280.562748] Pid: 8369, comm: cx23885[0] dvb Not tainted 2.6.20-16-generic #2 [ 280.562752] RIP: 0010:[881a0931] [881a0931] :videobuf_core:videobuf_mmap_setup+0x21/0xf0 [ 280.562764] RSP: 0018:81001bc21e40 EFLAGS: 00010282 [ 280.562767] RAX: RBX: 810033bc6020 RCX: 0002 [ 280.562770] RDX: 6000 RSI: 0020 RDI: 810033bc6020 [ 280.562774] RBP: 810033bc6010 R08: 81001bc2 R09: [ 280.562778] R10: 0013 R11: 80231700 R12: 81001b37db98 [ 280.562780] R13: 6000 R14: 0020 R15: 0002 [ 280.562784] FS: () GS:8100011e0ec0() knlGS: [ 280.562788] CS: 0010 DS: 0018 ES: 0018 CR0: 8005003b [ 280.562790] CR2: CR3: 2057e000 CR4: 06e0 [ 280.562794] Process cx23885[0] dvb (pid: 8369, threadinfo 81001bc2, task 81001f152100) [ 280.562797] Stack: 006f 80265bbb 81001bc21f00 810033bc6020 [ 280.562805] 810033bc6010 81001b37db98 810033bc6178 810033bc6020 [ 280.562812] 810033bc6220 881a0a7b 00011bc21ed0 8028b434 [ 280.562818] Call Trace: [ 280.562831] [80265bbb] thread_return+0x0/0xf5 [ 280.562878] [881a0a7b] :videobuf_core:videobuf_read_start+0x7b/0x150 [ 280.562889] [8028b434] __wake_up_common+0x44/0x80 [ 280.562923] [882943f6] :videobuf_dvb:videobuf_dvb_thread+0x46/0x170 [ 280.562951] [882943b0] :videobuf_dvb:videobuf_dvb_thread+0x0/0x170 [ 280.562957] [802a3170] keventd_create_kthread+0x0/0x90 [ 280.562968] [80233fa9] kthread+0xd9/0x120 [ 280.563006] [80261ec8] child_rip+0xa/0x12 [ 280.563020] [802a3170] keventd_create_kthread+0x0/0x90 [ 280.563082] [80233ed0] kthread+0x0/0x120 [ 280.563091] [80261ebe] child_rip+0x0/0x12 [ 280.563112] [ 280.563114] [ 280.563114] Code: 8b 30 81 fe 03 10 26 12 74 17 ba 03 10 26 12 48 c7 c7 a0 1c [ 280.563129] RIP [881a0931]
Re: [linux-dvb] [RFC-final] videobuf tree
On Sat, 6 Oct 2007, Michael Krufky wrote: I've tested the master branch under the following conditions: 1) cx88 raw analog video 2) cx88-blackbird encoded mpeg stream 3) cx88-dvb mpeg TS I'm pleased to report that the above three tests worked out successfully. However, cx23885 is now broken. Upon starting a DVB stream, the following OOPS is generated: Did you get this recent patch? changeset: 6284:7dba1f554c4a user:Trent Piepho [EMAIL PROTECTED] date:Thu Oct 04 01:28:45 2007 -0700 summary: cx23885: Update to new videobuf code You can compile cx23885 with no warnings without the patch, because the kernel still provides the old video_buf_dvb include files. ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [RFC-final] videobuf tree
Trent Piepho wrote: On Sat, 6 Oct 2007, Michael Krufky wrote: I've tested the master branch under the following conditions: 1) cx88 raw analog video 2) cx88-blackbird encoded mpeg stream 3) cx88-dvb mpeg TS I'm pleased to report that the above three tests worked out successfully. However, cx23885 is now broken. Upon starting a DVB stream, the following OOPS is generated: Did you get this recent patch? changeset: 6284:7dba1f554c4a user:Trent Piepho [EMAIL PROTECTED] date:Thu Oct 04 01:28:45 2007 -0700 summary: cx23885: Update to new videobuf code You can compile cx23885 with no warnings without the patch, because the kernel still provides the old video_buf_dvb include files. Yes... I cloned today's master branch, including your changeset cited above. I was sure to do 'make rminstall' in an older tree, to remove all traces of the older video_buf module before installing the new modules. Regards, Mike ___ linux-dvb mailing list linux-dvb@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [RFC-final] videobuf tree
Yes... I cloned today's master branch, including your changeset cited above.
I was sure to do 'make rminstall' in an older tree, to remove all traces of
the older video_buf module before installing the new modules.
I suspect that there's a race condition related with the way we do mmap. I
got a similar issue with MSI TV @nyware AD NB (saa7134) and a dual core
AMD 64 notebook. The same device on a single core notebook works like a
charm. I had this trouble with both old and new videobuf.
I also noticed, on tm6000 driver, that, on my tests with newer kernels,
the calling order for file .mmap handler and videobuf_iolock has changed.
I did a workaround at videobuf-vmalloc for this case:
/* It seems that some kernel versions need to do remap *after*
the mmap() call
*/
if (mem-vma) {
I suspect that this bug happens on certain workloads, and depends on HZ
value.
Maybe adding a wait_event_timeout() will solve. I'll do some tests here.
--
Cheers,
Mauro
___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
Re: [linux-dvb] [RFC-final] videobuf tree
Hi Michael,
Please try the enclosed patch. It is just a hack.
Please, post the dmesg, working or not.
Cheers,
Mauro.
diff -r 62d749961694 linux/drivers/media/video/videobuf-core.c
--- a/linux/drivers/media/video/videobuf-core.c Fri Aug 24 02:22:15 2007 +0200
+++ b/linux/drivers/media/video/videobuf-core.c Sun Oct 07 00:48:50 2007 -0300
@@ -92,10 +92,25 @@ int videobuf_iolock(struct videobuf_queu
int videobuf_iolock(struct videobuf_queue* q, struct videobuf_buffer *vb,
struct v4l2_framebuffer *fbuf)
{
+ int rc;
+
+printk(%s: init\n,__FUNCTION__);
+
MAGIC_CHECK(vb-magic,MAGIC_BUFFER);
MAGIC_CHECK(q-int_ops-magic,MAGIC_QTYPE_OPS);
- return CALL(q,iolock,q,vb,fbuf);
+ schedule();
+
+// mutex_lock(q-lock);
+
+printk(%s: calling actual handler\n,__FUNCTION__);
+
+ rc = CALL(q,iolock,q,vb,fbuf);
+// mutex_unlock(q-lock);
+
+printk(%s: return\n,__FUNCTION__);
+
+ return rc;
}
/* - */
@@ -915,12 +930,18 @@ int videobuf_mmap_mapper(struct videobuf
{
int retval;
- MAGIC_CHECK(q-int_ops-magic,MAGIC_QTYPE_OPS);
+printk(%s: init\n,__FUNCTION__);
mutex_lock(q-lock);
+
+ MAGIC_CHECK(q-int_ops-magic,MAGIC_QTYPE_OPS);
+printk(%s: calling actual handler\n,__FUNCTION__);
+
retval=CALL(q,mmap_mapper,q,vma);
+
mutex_unlock(q-lock);
+printk(%s: returning\n,__FUNCTION__);
return retval;
}
___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
