Pushdown bkl from v4l ioctls
in [Kernel]
|
Prev: linux-next: manual merge of the tty tree with the net tree
Next: [git pull] spi & OF fixes and documentation cleanup.
From: Laurent Pinchart on 30 Apr 2010 13:50 Hi Hans, On Thursday 29 April 2010 08:44:29 Hans Verkuil wrote: > On Thursday 29 April 2010 05:42:39 Frederic Weisbecker wrote: > > Hi, > > > > Linus suggested to rename struct v4l2_file_operations::ioctl > > into bkl_ioctl to eventually get something greppable and make > > its background explicit. > > > > While at it I thought it could be a good idea to just pushdown > > the bkl to every v4l drivers that have an .ioctl, so that we > > actually remove struct v4l2_file_operations::ioctl for good. > > > > It passed make allyesconfig on sparc. > > Please tell me what you think. > > I much prefer to keep the bkl inside the v4l2 core. One reason is that I > think that we can replace the bkl in the core with a mutex. Still not > ideal of course, so the next step will be to implement proper locking in > each driver. For this some additional v4l infrastructure work needs to be > done. I couldn't proceed with that until the v4l events API patches went > in, and that happened yesterday. > > So from my point of view the timeline is this: > > 1) I do the infrastructure work this weekend. This will make it much easier > to convert drivers to do proper locking. And it will also simplify > v4l2_priority handling, so I'm killing two birds with one stone :-) > > 2) Wait until Arnd's patch gets merged that pushes the bkl down to > v4l2-dev.c > > 3) Investigate what needs to be done to replace the bkl with a v4l2-dev.c > global mutex. Those drivers that call the bkl themselves should probably be > converted to do proper locking, but there are only about 14 drivers that do > this. The other 60 or so drivers should work fine if a v4l2-dev global lock > is used. At this point the bkl is effectively removed from the v4l > subsystem. > > 4) Work on the remaining 60 drivers to do proper locking and get rid of the > v4l2-dev global lock. This is probably less work than it sounds. > > Since your patch moves everything down to the driver level it will actually > make this work harder rather than easier. And it touches almost all drivers > as well. Every driver will need to be carefully checked to make sure the BKL can be replaced by a v4l2-dev global mutex. Why would it be more difficult to do so if the BKL is pushed down to the drivers ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 30 Apr 2010 16:40 On Thursday 29 April 2010 09:10:42 Laurent Pinchart wrote: > On Thursday 29 April 2010 08:44:29 Hans Verkuil wrote: > > > > 3) Investigate what needs to be done to replace the bkl with a v4l2-dev.c > > global mutex. Those drivers that call the bkl themselves should probably be > > converted to do proper locking, but there are only about 14 drivers that do > > this. The other 60 or so drivers should work fine if a v4l2-dev global lock > > is used. At this point the bkl is effectively removed from the v4l > > subsystem. > > > > 4) Work on the remaining 60 drivers to do proper locking and get rid of the > > v4l2-dev global lock. This is probably less work than it sounds. > > > > Since your patch moves everything down to the driver level it will actually > > make this work harder rather than easier. And it touches almost all drivers > > as well. > > Every driver will need to be carefully checked to make sure the BKL can be > replaced by a v4l2-dev global mutex. Why would it be more difficult to do so > if the BKL is pushed down to the drivers ? Note that you can completely skip the step of a v4l2-dev global mutex with Frederic's patch. This is the only use of the BKL in the common v4l2 code as far as I can tell, so instead of introducing yet another global lock, you can go straight to stage 4 and look at each driver separately, possibly introducing a per driver lock. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Hans Verkuil on 30 Apr 2010 21:10 On Thursday 29 April 2010 05:42:39 Frederic Weisbecker wrote: > Hi, > > Linus suggested to rename struct v4l2_file_operations::ioctl > into bkl_ioctl to eventually get something greppable and make > its background explicit. > > While at it I thought it could be a good idea to just pushdown > the bkl to every v4l drivers that have an .ioctl, so that we > actually remove struct v4l2_file_operations::ioctl for good. > > It passed make allyesconfig on sparc. > Please tell me what you think. I much prefer to keep the bkl inside the v4l2 core. One reason is that I think that we can replace the bkl in the core with a mutex. Still not ideal of course, so the next step will be to implement proper locking in each driver. For this some additional v4l infrastructure work needs to be done. I couldn't proceed with that until the v4l events API patches went in, and that happened yesterday. So from my point of view the timeline is this: 1) I do the infrastructure work this weekend. This will make it much easier to convert drivers to do proper locking. And it will also simplify v4l2_priority handling, so I'm killing two birds with one stone :-) 2) Wait until Arnd's patch gets merged that pushes the bkl down to v4l2-dev.c 3) Investigate what needs to be done to replace the bkl with a v4l2-dev.c global mutex. Those drivers that call the bkl themselves should probably be converted to do proper locking, but there are only about 14 drivers that do this. The other 60 or so drivers should work fine if a v4l2-dev global lock is used. At this point the bkl is effectively removed from the v4l subsystem. 4) Work on the remaining 60 drivers to do proper locking and get rid of the v4l2-dev global lock. This is probably less work than it sounds. Since your patch moves everything down to the driver level it will actually make this work harder rather than easier. And it touches almost all drivers as well. Regards, Hans > > Thanks. > > Frederic Weisbecker (5): > v4l: Pushdown bkl into video_ioctl2 > v4l: Use video_ioctl2_unlocked from drivers that don't want the bkl > v4l: Change users of video_ioctl2 to use unlocked_ioctl > v4l: Pushdown bkl to drivers that implement their own ioctl > v4l: Remove struct v4l2_file_operations::ioctl > > drivers/media/common/saa7146_fops.c | 2 +- > drivers/media/radio/dsbr100.c | 2 +- > drivers/media/radio/radio-aimslab.c | 2 +- > drivers/media/radio/radio-aztech.c | 2 +- > drivers/media/radio/radio-cadet.c | 2 +- > drivers/media/radio/radio-gemtek-pci.c | 2 +- > drivers/media/radio/radio-gemtek.c | 2 +- > drivers/media/radio/radio-maestro.c | 2 +- > drivers/media/radio/radio-maxiradio.c | 2 +- > drivers/media/radio/radio-miropcm20.c | 2 +- > drivers/media/radio/radio-mr800.c | 2 +- > drivers/media/radio/radio-rtrack2.c | 2 +- > drivers/media/radio/radio-sf16fmi.c | 2 +- > drivers/media/radio/radio-sf16fmr2.c | 2 +- > drivers/media/radio/radio-si4713.c | 2 +- > drivers/media/radio/radio-tea5764.c | 2 +- > drivers/media/radio/radio-terratec.c | 2 +- > drivers/media/radio/radio-timb.c | 2 +- > drivers/media/radio/radio-trust.c | 2 +- > drivers/media/radio/radio-typhoon.c | 2 +- > drivers/media/radio/radio-zoltrix.c | 2 +- > drivers/media/radio/si470x/radio-si470x-common.c | 2 +- > drivers/media/video/arv.c | 2 +- > drivers/media/video/au0828/au0828-video.c | 14 ++++---- > drivers/media/video/bt8xx/bttv-driver.c | 26 +++++++------- > drivers/media/video/bw-qcam.c | 11 +++++- > drivers/media/video/c-qcam.c | 11 +++++- > drivers/media/video/cafe_ccic.c | 14 ++++---- > drivers/media/video/cpia.c | 11 +++++- > drivers/media/video/cpia2/cpia2_v4l.c | 11 +++++- > drivers/media/video/cx18/cx18-streams.c | 12 +++--- > drivers/media/video/cx231xx/cx231xx-video.c | 4 +- > drivers/media/video/cx23885/cx23885-417.c | 2 +- > drivers/media/video/cx23885/cx23885-video.c | 4 +- > drivers/media/video/cx88/cx88-blackbird.c | 2 +- > drivers/media/video/cx88/cx88-video.c | 4 +- > drivers/media/video/davinci/vpfe_capture.c | 2 +- > drivers/media/video/davinci/vpif_capture.c | 2 +- > drivers/media/video/davinci/vpif_display.c | 2 +- > drivers/media/video/em28xx/em28xx-video.c | 4 +- > drivers/media/video/et61x251/et61x251_core.c | 27 +++++++++++---- > drivers/media/video/gspca/gspca.c | 2 +- > drivers/media/video/hdpvr/hdpvr-video.c | 2 +- > drivers/media/video/meye.c | 2 +- > drivers/media/video/omap24xxcam.c | 10 +++--- > drivers/media/video/ov511.c | 15 +++++--- > drivers/media/video/pms.c | 2 +- > drivers/media/video/pvrusb2/pvrusb2-v4l2.c | 20 +++++++---- > drivers/media/video/pwc/pwc-if.c | 19 ++++++---- > drivers/media/video/s2255drv.c | 12 +++--- > drivers/media/video/saa5246a.c | 11 ++++-- > drivers/media/video/saa5249.c | 6 +++- > drivers/media/video/saa7134/saa7134-empress.c | 14 ++++---- > drivers/media/video/saa7134/saa7134-video.c | 26 +++++++------- > drivers/media/video/se401.c | 20 +++++++---- > drivers/media/video/sn9c102/sn9c102_core.c | 27 +++++++++++---- > drivers/media/video/soc_camera.c | 2 +- > drivers/media/video/stk-webcam.c | 14 ++++---- > drivers/media/video/stradis.c | 26 +++++++++++---- > drivers/media/video/stv680.c | 20 +++++++---- > drivers/media/video/tlg2300/pd-radio.c | 8 ++-- > drivers/media/video/tlg2300/pd-video.c | 2 +- > drivers/media/video/usbvideo/usbvideo.c | 21 ++++++++---- > drivers/media/video/usbvideo/vicam.c | 14 +++++++- > drivers/media/video/usbvision/usbvision-video.c | 4 +- > drivers/media/video/uvc/uvc_v4l2.c | 11 +++++- > drivers/media/video/v4l2-dev.c | 38 ++------------------- > drivers/media/video/v4l2-ioctl.c | 17 ++++++++- > drivers/media/video/vivi.c | 2 +- > drivers/media/video/w9966.c | 2 +- > drivers/media/video/w9968cf.c | 25 +++++++++++--- > drivers/media/video/zc0301/zc0301_core.c | 27 +++++++++++---- > drivers/media/video/zoran/zoran_driver.c | 16 ++++---- > drivers/media/video/zr364xx.c | 14 ++++---- > drivers/staging/cx25821/cx25821-audups11.c | 18 ++++++---- > drivers/staging/cx25821/cx25821-video0.c | 14 ++++---- > drivers/staging/cx25821/cx25821-video1.c | 14 ++++---- > drivers/staging/cx25821/cx25821-video2.c | 14 ++++---- > drivers/staging/cx25821/cx25821-video3.c | 14 ++++---- > drivers/staging/cx25821/cx25821-video4.c | 14 ++++---- > drivers/staging/cx25821/cx25821-video5.c | 14 ++++---- > drivers/staging/cx25821/cx25821-video6.c | 14 ++++---- > drivers/staging/cx25821/cx25821-video7.c | 14 ++++---- > drivers/staging/cx25821/cx25821-videoioctl.c | 27 +++++++++++---- > drivers/staging/cx25821/cx25821-vidups10.c | 19 +++++++---- > drivers/staging/cx25821/cx25821-vidups9.c | 18 ++++++---- > drivers/staging/dream/camera/msm_v4l2.c | 27 +++++++++++---- > drivers/staging/go7007/go7007-v4l2.c | 2 +- > drivers/staging/tm6000/tm6000-video.c | 2 +- > include/media/v4l2-dev.h | 1 - > include/media/v4l2-ioctl.h | 2 + > sound/i2c/other/tea575x-tuner.c | 2 +- > 92 files changed, 530 insertions(+), 360 deletions(-) > -- Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Hans Verkuil on 1 May 2010 06:00 On Thursday 29 April 2010 09:10:42 Laurent Pinchart wrote: > Hi Hans, > > On Thursday 29 April 2010 08:44:29 Hans Verkuil wrote: > > On Thursday 29 April 2010 05:42:39 Frederic Weisbecker wrote: > > > Hi, > > > > > > Linus suggested to rename struct v4l2_file_operations::ioctl > > > into bkl_ioctl to eventually get something greppable and make > > > its background explicit. > > > > > > While at it I thought it could be a good idea to just pushdown > > > the bkl to every v4l drivers that have an .ioctl, so that we > > > actually remove struct v4l2_file_operations::ioctl for good. > > > > > > It passed make allyesconfig on sparc. > > > Please tell me what you think. > > > > I much prefer to keep the bkl inside the v4l2 core. One reason is that I > > think that we can replace the bkl in the core with a mutex. Still not > > ideal of course, so the next step will be to implement proper locking in > > each driver. For this some additional v4l infrastructure work needs to be > > done. I couldn't proceed with that until the v4l events API patches went > > in, and that happened yesterday. > > > > So from my point of view the timeline is this: > > > > 1) I do the infrastructure work this weekend. This will make it much easier > > to convert drivers to do proper locking. And it will also simplify > > v4l2_priority handling, so I'm killing two birds with one stone :-) > > > > 2) Wait until Arnd's patch gets merged that pushes the bkl down to > > v4l2-dev.c > > > > 3) Investigate what needs to be done to replace the bkl with a v4l2-dev.c > > global mutex. Those drivers that call the bkl themselves should probably be > > converted to do proper locking, but there are only about 14 drivers that do > > this. The other 60 or so drivers should work fine if a v4l2-dev global lock > > is used. At this point the bkl is effectively removed from the v4l > > subsystem. > > > > 4) Work on the remaining 60 drivers to do proper locking and get rid of the > > v4l2-dev global lock. This is probably less work than it sounds. > > > > Since your patch moves everything down to the driver level it will actually > > make this work harder rather than easier. And it touches almost all drivers > > as well. > > Every driver will need to be carefully checked to make sure the BKL can be > replaced by a v4l2-dev global mutex. Why would it be more difficult to do so > if the BKL is pushed down to the drivers ? The main reason is really that pushing the bkl into the v4l core makes it easier to review. I noticed for example that this patch series forgot to change the video_ioctl2 call in ivtv-ioctl.c to video_ioctl2_unlocked. And there may be other places as well that were missed. Having so many drivers changed also means a lot of careful reviewing. But I will not block this change. However, I do think it would be better to create a video_ioctl2_bkl rather than add a video_ioctl2_unlocked. The current video_ioctl2 function *is* already unlocked. So you are subtle changing the behavior of video_ioctl2. Not a good idea IMHO. And yes, grepping for video_ioctl2_bkl is also easy to do and makes it more obvious that the BKL is used in drivers that call this. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 1 May 2010 06:50
On Saturday 01 May 2010 11:55:37 Hans Verkuil wrote: > However, I do think it would be better to > create a video_ioctl2_bkl rather than add a video_ioctl2_unlocked. The current > video_ioctl2 function is already unlocked. So you are subtle changing the > behavior of video_ioctl2. Not a good idea IMHO. And yes, grepping for > video_ioctl2_bkl is also easy to do and makes it more obvious that the BKL is > used in drivers that call this. Yes, that makes sense. It also allows working towards a goal of 'removing video_ioctl2_bkl', which is easier to understand than 'converting video_ioctl2 users to video_ioctl2_unlocked and later renaming that'. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |