Mailing List Archive: ivtv: devel

[PATCH] cx18: convert driver to video_ioctl2() (Re: [PATCH] video4linux: Push down the BKL)

Index | Next | Previous | View Threaded

awalls at radix

May 31, 2008, 7:34 PM

Post #1 of 7 (2751 views)
Permalink

[PATCH] cx18: convert driver to video_ioctl2() (Re: [PATCH] video4linux: Push down the BKL)

On Mon, 2008-05-26 at 18:46 +0200, Hans Verkuil wrote:
> On Monday 26 May 2008 18:34:57 Mauro Carvalho Chehab wrote:

> > In the specific case of ivtv and cx18, I think that the better would
> > be to convert it first to video_ioctl2. Then, remove the BKL, with a
> > video_ioctl2_unlocked version.
> >
> > Douglas already did an experimental patch converting ivtv to
> > video_ioctl2 and sent to Hans. It needs testing, since he doesn't
> > have any ivtv board. It should be trivial to port this to cx18, since
> > both drivers have similar structures.
> >
> > Douglas,
> >
> > Could you send this patch to the ML for people to review and for Andy
> > to port it to cx18?
>
> Unless there is an objection, I would prefer to take Douglas' patch and
> merge it into the v4l-dvb ivtv driver myself. There were several things
> in the patch I didn't like, but I need to 'work' with it a bit to see
> how/if it can be done better.
>
> I can work on it tonight and tomorrow. Hopefully it is finished by then.
> I can move the BKL down at the same time for ivtv. It is unlikely that
> I will have time to do cx18 as well as I'm abroad from Wednesday until
> Monday, but I think Andy can do that easily based on the ivtv changes.

I have attached a patch, made against Hans' v4l-dvb-ioctl2 repository,
to convert the cx18 driver to use video_ioctl2(). In the process I
pushed down the priority checks and the debug messages into the
individual functions. I did not remove the serialization lock as I have
not had the time to assess if that would be safe. I "#if 0"'ed out some
sliced VBI code that was being skipped in the original code.

Comments are welcome.

Many thanks to Hans, for without his changeset to ivtv for reference,
this would have taken me much, much longer.

Short term initial tests of this patch with MythTV, v4l-ctl, and v4l-dbg
indicate things are OK. I did notice once strange artifact with MythTV.
When switching from one analog channel to another, for about a second, I
see the two television video fields non-interlaced, stacked one atop of
the other in the frame. Weird, but not a big deal.

Regards,
Andy

Attachments:

cx18-ioctl2.patch.bz2 (9.39 KB)

hverkuil at xs4all

Jun 1, 2008, 3:15 AM

Post #2 of 7 (2636 views)
Permalink

Re: [PATCH] cx18: convert driver to video_ioctl2() (Re: [PATCH] video4linux: Push down the BKL) [In reply to]

On Sunday 01 June 2008 04:34, Andy Walls wrote:
> On Mon, 2008-05-26 at 18:46 +0200, Hans Verkuil wrote:
> > On Monday 26 May 2008 18:34:57 Mauro Carvalho Chehab wrote:
> > > In the specific case of ivtv and cx18, I think that the better
> > > would be to convert it first to video_ioctl2. Then, remove the
> > > BKL, with a video_ioctl2_unlocked version.
> > >
> > > Douglas already did an experimental patch converting ivtv to
> > > video_ioctl2 and sent to Hans. It needs testing, since he doesn't
> > > have any ivtv board. It should be trivial to port this to cx18,
> > > since both drivers have similar structures.
> > >
> > > Douglas,
> > >
> > > Could you send this patch to the ML for people to review and for
> > > Andy to port it to cx18?
> >
> > Unless there is an objection, I would prefer to take Douglas' patch
> > and merge it into the v4l-dvb ivtv driver myself. There were
> > several things in the patch I didn't like, but I need to 'work'
> > with it a bit to see how/if it can be done better.
> >
> > I can work on it tonight and tomorrow. Hopefully it is finished by
> > then. I can move the BKL down at the same time for ivtv. It is
> > unlikely that I will have time to do cx18 as well as I'm abroad
> > from Wednesday until Monday, but I think Andy can do that easily
> > based on the ivtv changes.
>
> I have attached a patch, made against Hans' v4l-dvb-ioctl2
> repository, to convert the cx18 driver to use video_ioctl2(). In the
> process I pushed down the priority checks and the debug messages into
> the individual functions. I did not remove the serialization lock as
> I have not had the time to assess if that would be safe. I "#if
> 0"'ed out some sliced VBI code that was being skipped in the original
> code.
>
> Comments are welcome.
>
> Many thanks to Hans, for without his changeset to ivtv for reference,
> this would have taken me much, much longer.
>
>
> Short term initial tests of this patch with MythTV, v4l-ctl, and
> v4l-dbg indicate things are OK. I did notice once strange artifact
> with MythTV. When switching from one analog channel to another, for
> about a second, I see the two television video fields non-interlaced,
> stacked one atop of the other in the frame. Weird, but not a big
> deal.

Thanks Andy!

I'll take a closer look on Tuesday or Wednesday, but I noticed one
thing: you set unused callbacks to NULL in cx18_set_funcs(), however
these can just be removed as they are NULL by default.

Regards,

Hans

_______________________________________________
ivtv-devel mailing list
ivtv-devel [at] ivtvdriver
http://ivtvdriver.org/mailman/listinfo/ivtv-devel

awalls at radix

Jun 1, 2008, 12:01 PM

Post #3 of 7 (2626 views)
Permalink

Re: [PATCH] cx18: convert driver to video_ioctl2() (Re: [PATCH] video4linux: Push down the BKL) [In reply to]

On Sun, 2008-06-01 at 12:15 +0200, Hans Verkuil wrote:

> Thanks Andy!
>
> I'll take a closer look on Tuesday or Wednesday, but I noticed one
> thing: you set unused callbacks to NULL in cx18_set_funcs(), however
> these can just be removed as they are NULL by default.

Yeah, they can go. I left them in as an aid for double checking that I
didn't forget any callbacks that needed to be implemented.

Regards,
Andy

_______________________________________________
ivtv-devel mailing list
ivtv-devel [at] ivtvdriver
http://ivtvdriver.org/mailman/listinfo/ivtv-devel

mchehab at infradead

Jun 3, 2008, 2:20 PM

Post #4 of 7 (2593 views)
Permalink

Re: [PATCH] cx18: convert driver to video_ioctl2() (Re: [PATCH] video4linux: Push down the BKL) [In reply to]

On Sun, 01 Jun 2008 15:01:59 -0400
Andy Walls <awalls [at] radix> wrote:

> On Sun, 2008-06-01 at 12:15 +0200, Hans Verkuil wrote:
>
>
> > Thanks Andy!
> >
> > I'll take a closer look on Tuesday or Wednesday, but I noticed one
> > thing: you set unused callbacks to NULL in cx18_set_funcs(), however
> > these can just be removed as they are NULL by default.
>
> Yeah, they can go. I left them in as an aid for double checking that I
> didn't forget any callbacks that needed to be implemented.

Please don't do that. All static vars that have a value 0 or NULL shouldn't be
initialized, since this will eat some space inside the module.

Cheers,
Mauro

_______________________________________________
ivtv-devel mailing list
ivtv-devel [at] ivtvdriver
http://ivtvdriver.org/mailman/listinfo/ivtv-devel

alan at redhat

Jun 3, 2008, 3:08 PM

Post #5 of 7 (2586 views)
Permalink

Re: [PATCH] cx18: convert driver to video_ioctl2() (Re: [PATCH] video4linux: Push down the BKL) [In reply to]

On Tue, Jun 03, 2008 at 06:20:52PM -0300, Mauro Carvalho Chehab wrote:
> > Yeah, they can go. I left them in as an aid for double checking that I
> > didn't forget any callbacks that needed to be implemented.
>
> Please don't do that. All static vars that have a value 0 or NULL shouldn't be
> initialized, since this will eat some space inside the module.

The compiler is smarter than that. Besides which 4 bytes will make no difference
whether it is data or bss given 4K disk block sizes 8)

It is coding style not to do it but it isn't a bad idea to leave them in if
they make something explicitly clear IMHO.

_______________________________________________
ivtv-devel mailing list
ivtv-devel [at] ivtvdriver
http://ivtvdriver.org/mailman/listinfo/ivtv-devel

awalls at radix

Jun 3, 2008, 5:44 PM

Post #6 of 7 (2579 views)
Permalink

Re: [PATCH] cx18: convert driver to video_ioctl2() (Re: [PATCH] video4linux: Push down the BKL) [In reply to]

On Tue, 2008-06-03 at 18:08 -0400, Alan Cox wrote:
> On Tue, Jun 03, 2008 at 06:20:52PM -0300, Mauro Carvalho Chehab wrote:
> > > Yeah, they can go. I left them in as an aid for double checking that I
> > > didn't forget any callbacks that needed to be implemented.
> >
> > Please don't do that. All static vars that have a value 0 or NULL shouldn't be
> > initialized, since this will eat some space inside the module.

Mauro,

OK.

Alan,

> The compiler is smarter than that. Besides which 4 bytes will make no difference
> whether it is data or bss given 4K disk block sizes 8)

Objdump reveals, unfortunately the compiler isn't that smart. It does
explicitly create code to store the NULL pointers.

For the record, every pointer store looks something like this (before
relocations) on 64-bit:

1f9: 48 c7 87 c0 04 00 00 movq $0x0,0x4c0(%rdi)
200: 00 00 00 00

11 bytes * 19 NULL pointer stores = 209 bytes + wasted CPU cycles.

Quite honestly, I would have thought the compiler smart enough to do
something like

xor %rax, %rax
mov %rax, 0x123(%rdi)

as most of the NULL pointer stores happened in groups of two or three.
But apparently, it wasn't.

-Andy

> It is coding style not to do it but it isn't a bad idea to leave them in if
> they make something explicitly clear IMHO.

_______________________________________________
ivtv-devel mailing list
ivtv-devel [at] ivtvdriver
http://ivtvdriver.org/mailman/listinfo/ivtv-devel

alan at redhat

Jun 4, 2008, 3:02 AM

Post #7 of 7 (2566 views)
Permalink

Re: [PATCH] cx18: convert driver to video_ioctl2() (Re: [PATCH] video4linux: Push down the BKL) [In reply to]

On Tue, Jun 03, 2008 at 08:44:30PM -0400, Andy Walls wrote:
> For the record, every pointer store looks something like this (before
> relocations) on 64-bit:
>
> 1f9: 48 c7 87 c0 04 00 00 movq $0x0,0x4c0(%rdi)
> 200: 00 00 00 00
>
> 11 bytes * 19 NULL pointer stores = 209 bytes + wasted CPU cycles.

Static objects don't get stored at init.. they are either in the data
segment (older compilers) or BSS (smarter ones)

_______________________________________________
ivtv-devel mailing list
ivtv-devel [at] ivtvdriver
http://ivtvdriver.org/mailman/listinfo/ivtv-devel

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads