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:
>
> 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.
>
This wouldn't be a bad idea.
> 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.
>
I think that you've proven that it is NOT important... although, it
still might be a good idea. I'm not sure -- I was only making a suggestion.
Cheers,
Mike
_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
