Re: [PATCH 0/5] Pushdown bkl from v4l ioctls
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-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] Pushdown bkl from v4l ioctls
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-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] Pushdown bkl from v4l ioctls
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 I did look at this a long time ago - it doesn't really work becaue the mutex you propose then has to be dropped and taken in the sleeping parts of each ioctl to avoid app problems and in some cases threaded apps deadlocking. I think Arnd is right on his approach to this, having tried the other way. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] Pushdown bkl from v4l ioctls
On Sat, May 01, 2010 at 11:55:37AM +0200, Hans Verkuil wrote: 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. Indeed, that's because I did it in a half automated way and my script didn't took the direct calls to video_ioctl2() into account, so I had to check them manually and probably missed a few, I will fix this one and double check. 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. Totally agreed, will respin with this rename. Thanks. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] Pushdown bkl from v4l ioctls
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-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] Pushdown bkl from v4l ioctls
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-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] Pushdown bkl from v4l ioctls
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
[PATCH 0/5] Pushdown bkl from v4l ioctls
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. 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
