Re: [PATCH 3/4] SoC camera: Remove the framework and the drivers
On 10/30/2018 10:17 PM, jacopo mondi wrote: > On Tue, Oct 30, 2018 at 05:35:23PM -0300, Mauro Carvalho Chehab wrote: >> Em Tue, 30 Oct 2018 21:28:57 +0100 >> jacopo mondi escreveu: >> >>> Hi Mauro, >>> >>> On Tue, Oct 30, 2018 at 09:14:09AM -0300, Mauro Carvalho Chehab wrote: Em Tue, 30 Oct 2018 01:21:34 +0200 Sakari Ailus escreveu: > The SoC camera framework has been obsolete for some time and it is no > longer functional. A few drivers have been converted to the V4L2 > sub-device API but for the rest the conversion has not taken place yet. > > In order to keep the tree clean and to avoid keep maintaining > non-functional and obsolete code, remove the SoC camera framework as well > as the drivers that depend on it. > > Signed-off-by: Sakari Ailus > --- > Resending, this time with git format-patch -D . > > MAINTAINERS|8 - > drivers/media/i2c/Kconfig |8 - > drivers/media/i2c/Makefile |1 - > drivers/media/i2c/soc_camera/Kconfig | 66 - > drivers/media/i2c/soc_camera/Makefile | 10 - > drivers/media/i2c/soc_camera/ov9640.h | 208 -- > drivers/media/i2c/soc_camera/soc_mt9m001.c | 757 --- > drivers/media/i2c/soc_camera/soc_mt9t112.c | 1157 --- > drivers/media/i2c/soc_camera/soc_mt9v022.c | 1012 - > drivers/media/i2c/soc_camera/soc_ov5642.c | 1087 -- > drivers/media/i2c/soc_camera/soc_ov772x.c | 1123 -- > drivers/media/i2c/soc_camera/soc_ov9640.c | 738 --- > drivers/media/i2c/soc_camera/soc_ov9740.c | 996 - > drivers/media/i2c/soc_camera/soc_rj54n1cb0c.c | 1415 - > drivers/media/i2c/soc_camera/soc_tw9910.c | 999 - I don't see why we should remove those. I mean, Jacopo is actually converting those drivers to not depend on soc_camera, and it is a way better to review those patches with the old code in place. >>> >>> I have converted a few drivers used by some SH boards where I dropped >>> dependencies on soc_camera, not to remove camera support from those. For >>> others I don't have cameras to test with, nor I know about boards in >>> mainline using them. >>> >>> From my side, driver conversion is done. >>> So, at least while Jacopo is keep doing this work, I would keep at Kernel tree, as it helps to see a diff when the driver changes when getting rid of soc_camera dependencies. So, IMO, the best would be to move those to /staging, eventually depending on BROKEN. >>> >>> However, somebody with a (rather old) development setup using those camera >>> sensor may wants to see if mainline supports them. We actually had a >>> few patches coming lately (for ov. I understand Sakari's argument that those >>> could be retrieved from git history, but a few people will notice imo. >>> I also understand the additional maintainership burden of keeping them >>> around, so I'm fine with either ways ;) >>> >>> This is a list of the current situation in mainline, to have a better >>> idea: >>> >>> $for i in `seq 1 9`; do CAM=$(head -n $i /tmp/soc_cams | tail -n 1); echo >>> $CAM; find drivers/media/ -name $CAM; done >>> t9m001.c >>> drivers/media/i2c/soc_camera/mt9m001.c >>> mt9t112.c >>> drivers/media/i2c/mt9t112.c >>> drivers/media/i2c/soc_camera/mt9t112.c >>> mt9v022.c >>> drivers/media/i2c/soc_camera/mt9v022.c >>> ov5642.c >>> drivers/media/i2c/soc_camera/ov5642.c >>> ov772x.c >>> drivers/media/i2c/ov772x.c >>> drivers/media/i2c/soc_camera/ov772x.c >>> ov9640.c >>> drivers/media/i2c/soc_camera/ov9640.c >>> ov9740.c >>> drivers/media/i2c/soc_camera/ov9740.c >>> rj54n1cb0c.c >>> drivers/media/i2c/rj54n1cb0c.c >>> drivers/media/i2c/soc_camera/rj54n1cb0c.c >>> tw9910.c >>> drivers/media/i2c/tw9910.c >>> drivers/media/i2c/soc_camera/tw9910.c >>> >>> So it seems to me only the following sensor do not have a >>> non-soc_camera driver at the moment: >>> >>> mt9m001.c >>> mt9v022.c >>> ov5642.c >>> ov9640.c >>> ov9740.c > > For a few of them (mt9m001, ov5642) there are cheap modules available > online. The others ones have public documentation. I know they are old > and dusty, supporting only parallel video interface. > >> >> Ok. So, what about keeping just those 5 drivers at staging? If, after an >> year, people won't do conversions, we can just drop them. >> > > Let's see what Sakari and Hans think. Again, I'm fine with both ways > ;) My preference is to just remove them. But moving them to staging under CONFIG_BROKEN for a year is OK with me, but frankly I don't see the point. Regards, Hans
Re: [PATCH 4/4] SoC camera: Tidy the header
Hi Mauro, On Tue, Oct 30, 2018 at 09:06:18AM -0300, Mauro Carvalho Chehab wrote: > Em Tue, 30 Oct 2018 01:00:29 +0200 > Sakari Ailus escreveu: > > > Clean up the SoC camera framework header. It only exists now to keep board > > code compiling. The header can be removed once the board code dependencies > > to it has been removed. > > > > Signed-off-by: Sakari Ailus > > --- > > include/media/soc_camera.h | 335 > > - > > 1 file changed, 335 deletions(-) > > > > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h > > index b7e42a1b0910..14d19da6052a 100644 > > --- a/include/media/soc_camera.h > > +++ b/include/media/soc_camera.h > > @@ -22,172 +22,6 @@ > > #include > > #include > > That doesn't make any sense. soc_camera.h should have the same fate > as the entire soc_camera infrastructure: either be removed or moved > to staging, and everything else that doesn't have the same fate > should get rid of this header. We can't just remove this; there is board code that depends on it. The intent is to remove the board code as well but presuming that the board code would be merged through a different tree, it'd be less hassle to wait a bit; hence the patch removing any unnecessary stuff here. -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH 3/4] SoC camera: Remove the framework and the drivers
Hi Mauro, On Tue, Oct 30, 2018 at 05:35:23PM -0300, Mauro Carvalho Chehab wrote: > Em Tue, 30 Oct 2018 21:28:57 +0100 > jacopo mondi escreveu: > > > Hi Mauro, > > > > On Tue, Oct 30, 2018 at 09:14:09AM -0300, Mauro Carvalho Chehab wrote: > > > Em Tue, 30 Oct 2018 01:21:34 +0200 > > > Sakari Ailus escreveu: > > > > > > > The SoC camera framework has been obsolete for some time and it is no > > > > longer functional. A few drivers have been converted to the V4L2 > > > > sub-device API but for the rest the conversion has not taken place yet. > > > > > > > > In order to keep the tree clean and to avoid keep maintaining > > > > non-functional and obsolete code, remove the SoC camera framework as > > > > well > > > > as the drivers that depend on it. > > > > > > > > Signed-off-by: Sakari Ailus > > > > --- > > > > Resending, this time with git format-patch -D . > > > > > > > > MAINTAINERS|8 - > > > > drivers/media/i2c/Kconfig |8 - > > > > drivers/media/i2c/Makefile |1 - > > > > drivers/media/i2c/soc_camera/Kconfig | 66 - > > > > drivers/media/i2c/soc_camera/Makefile | 10 - > > > > drivers/media/i2c/soc_camera/ov9640.h | 208 -- > > > > drivers/media/i2c/soc_camera/soc_mt9m001.c | 757 --- > > > > drivers/media/i2c/soc_camera/soc_mt9t112.c | 1157 --- > > > > drivers/media/i2c/soc_camera/soc_mt9v022.c | 1012 - > > > > drivers/media/i2c/soc_camera/soc_ov5642.c | 1087 -- > > > > drivers/media/i2c/soc_camera/soc_ov772x.c | 1123 -- > > > > drivers/media/i2c/soc_camera/soc_ov9640.c | 738 --- > > > > drivers/media/i2c/soc_camera/soc_ov9740.c | 996 - > > > > drivers/media/i2c/soc_camera/soc_rj54n1cb0c.c | 1415 - > > > > drivers/media/i2c/soc_camera/soc_tw9910.c | 999 - > > > > > > I don't see why we should remove those. I mean, Jacopo is > > > actually converting those drivers to not depend on soc_camera, > > > and it is a way better to review those patches with the old > > > code in place. > > > > I have converted a few drivers used by some SH boards where I dropped > > dependencies on soc_camera, not to remove camera support from those. For > > others I don't have cameras to test with, nor I know about boards in > > mainline using them. > > > > From my side, driver conversion is done. > > > > > > > > So, at least while Jacopo is keep doing this work, I would keep > > > at Kernel tree, as it helps to see a diff when the driver changes > > > when getting rid of soc_camera dependencies. > > > > > > So, IMO, the best would be to move those to /staging, eventually > > > depending on BROKEN. > > > > However, somebody with a (rather old) development setup using those camera > > sensor may wants to see if mainline supports them. We actually had a > > few patches coming lately (for ov. I understand Sakari's argument that those > > could be retrieved from git history, but a few people will notice imo. > > I also understand the additional maintainership burden of keeping them > > around, so I'm fine with either ways ;) > > > > This is a list of the current situation in mainline, to have a better > > idea: > > > > $for i in `seq 1 9`; do CAM=$(head -n $i /tmp/soc_cams | tail -n 1); echo > > $CAM; find drivers/media/ -name $CAM; done > > t9m001.c > > drivers/media/i2c/soc_camera/mt9m001.c > > mt9t112.c > > drivers/media/i2c/mt9t112.c > > drivers/media/i2c/soc_camera/mt9t112.c > > mt9v022.c > > drivers/media/i2c/soc_camera/mt9v022.c > > ov5642.c > > drivers/media/i2c/soc_camera/ov5642.c > > ov772x.c > > drivers/media/i2c/ov772x.c > > drivers/media/i2c/soc_camera/ov772x.c > > ov9640.c > > drivers/media/i2c/soc_camera/ov9640.c > > ov9740.c > > drivers/media/i2c/soc_camera/ov9740.c > > rj54n1cb0c.c > > drivers/media/i2c/rj54n1cb0c.c > > drivers/media/i2c/soc_camera/rj54n1cb0c.c > > tw9910.c > > drivers/media/i2c/tw9910.c > > drivers/media/i2c/soc_camera/tw9910.c > > > > So it seems to me only the following sensor do not have a > > non-soc_camera driver at the moment: > > > > mt9m001.c > > mt9v022.c > > ov5642.c > > ov9640.c > > ov9740.c > > Ok. So, what about keeping just those 5 drivers at staging? If, after an > year, people won't do conversions, we can just drop them. They've been there for years without anyone converting them. Do note that the conversion can be still done once the code is removed. We did the same for a big bunch of sensor drivers that came with the atomisp2 driver. I don't see a difference here. -- Kind regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH 4/4] SoC camera: Tidy the header
Em Wed, 31 Oct 2018 11:29:45 +0200 Sakari Ailus escreveu: > Hi Mauro, > > On Tue, Oct 30, 2018 at 09:06:18AM -0300, Mauro Carvalho Chehab wrote: > > Em Tue, 30 Oct 2018 01:00:29 +0200 > > Sakari Ailus escreveu: > > > > > Clean up the SoC camera framework header. It only exists now to keep board > > > code compiling. The header can be removed once the board code dependencies > > > to it has been removed. > > > > > > Signed-off-by: Sakari Ailus > > > --- > > > include/media/soc_camera.h | 335 > > > - > > > 1 file changed, 335 deletions(-) > > > > > > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h > > > index b7e42a1b0910..14d19da6052a 100644 > > > --- a/include/media/soc_camera.h > > > +++ b/include/media/soc_camera.h > > > @@ -22,172 +22,6 @@ > > > #include > > > #include > > > > That doesn't make any sense. soc_camera.h should have the same fate > > as the entire soc_camera infrastructure: either be removed or moved > > to staging, and everything else that doesn't have the same fate > > should get rid of this header. > > We can't just remove this; there is board code that depends on it. > > The intent is to remove the board code as well but presuming that the board > code would be merged through a different tree, it'd be less hassle to wait > a bit; hence the patch removing any unnecessary stuff here. Then we need *first* to remove board code, wait for those changes to be applied and *then* remove soc_camera (and not the opposite). Thanks, Mauro
Re: [PATCH 4/4] SoC camera: Tidy the header
On Wed, Oct 31, 2018 at 06:40:30AM -0300, Mauro Carvalho Chehab wrote: > Em Wed, 31 Oct 2018 11:29:45 +0200 > Sakari Ailus escreveu: > > > Hi Mauro, > > > > On Tue, Oct 30, 2018 at 09:06:18AM -0300, Mauro Carvalho Chehab wrote: > > > Em Tue, 30 Oct 2018 01:00:29 +0200 > > > Sakari Ailus escreveu: > > > > > > > Clean up the SoC camera framework header. It only exists now to keep > > > > board > > > > code compiling. The header can be removed once the board code > > > > dependencies > > > > to it has been removed. > > > > > > > > Signed-off-by: Sakari Ailus > > > > --- > > > > include/media/soc_camera.h | 335 > > > > - > > > > 1 file changed, 335 deletions(-) > > > > > > > > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h > > > > index b7e42a1b0910..14d19da6052a 100644 > > > > --- a/include/media/soc_camera.h > > > > +++ b/include/media/soc_camera.h > > > > @@ -22,172 +22,6 @@ > > > > #include > > > > #include > > > > > > That doesn't make any sense. soc_camera.h should have the same fate > > > as the entire soc_camera infrastructure: either be removed or moved > > > to staging, and everything else that doesn't have the same fate > > > should get rid of this header. > > > > We can't just remove this; there is board code that depends on it. > > > > The intent is to remove the board code as well but presuming that the board > > code would be merged through a different tree, it'd be less hassle to wait > > a bit; hence the patch removing any unnecessary stuff here. > > Then we need *first* to remove board code, wait for those changes to be > applied and *then* remove soc_camera (and not the opposite). Any reasoning behind that? It's dead code and there is no dependency between the SoC camera framework (apart from the header) and the board code. -- Sakari Ailus sakari.ai...@linux.intel.com
Re: VIVID/VIMC and media fuzzing
On 10/30/2018 03:02 PM, Dmitry Vyukov wrote: > Hello Helen and linux-media, > > I've attended your talk "Shifting Media App Development into High > Gear" on OSS Summit last week and approached you with some questions > if/how this can be used for kernel testing. Thanks, turn out to be a > very useful talk! > > I am working on syzkaller/syzbot, continuous kernel fuzzing system: > https://github.com/google/syzkaller > https://github.com/google/syzkaller/blob/master/docs/syzbot.md > https://syzkaller.appspot.com > > After simply enabling CONFIG_VIDEO_VIMC, CONFIG_VIDEO_VIM2M, > CONFIG_VIDEO_VIVID, CONFIG_VIDEO_VICODEC syzbot has found 8 bugs in > media subsystem in just 24 hours: > > KASAN: use-after-free Read in vb2_mmap > https://groups.google.com/forum/#!msg/syzkaller-bugs/XGGH69jMWQ0/S8vfxgEmCgAJ > > KASAN: use-after-free Write in __vb2_cleanup_fileio > https://groups.google.com/forum/#!msg/syzkaller-bugs/qKKhsZVPo3o/P6AB2of2CQAJ > > WARNING in __vb2_queue_cancel > https://groups.google.com/forum/#!msg/syzkaller-bugs/S29GU_NtfPY/ZvAz8UDtCQAJ > > divide error in vivid_vid_cap_s_dv_timings > https://groups.google.com/forum/#!msg/syzkaller-bugs/GwF5zGBCfyg/wnuWmW_sCQAJ Should be fixed by https://patchwork.linuxtv.org/patch/52641/ > > KASAN: use-after-free Read in wake_up_if_idle > https://groups.google.com/forum/#!msg/syzkaller-bugs/aBWb_yV1kiI/sWQO63fkCQAJ > > KASAN: use-after-free Read in __vb2_perform_fileio > https://groups.google.com/forum/#!msg/syzkaller-bugs/MdFCZHz0LUQ/qSK_bFbcCQAJ > > INFO: task hung in vivid_stop_generating_vid_cap > https://groups.google.com/forum/#!msg/syzkaller-bugs/F_KFW6PVyTA/wTBeHLfTCQAJ > > KASAN: null-ptr-deref Write in kthread_stop > https://groups.google.com/forum/#!msg/syzkaller-bugs/u0AGnYvSlf4/fUiyfA_TCQAJ These last two should be fixed by https://patchwork.linuxtv.org/patch/52640/ Haven't figured out the others yet (hope to get back to that next week). > > Based on this I think if we put more effort into media fuzzing, it > will be able to find dozens more. Yeah, this is good stuff. Thank you for setting this up. > > syzkaller needs descriptions of kernel interfaces to efficiently cover > a subsystem. For example, see: > https://github.com/google/syzkaller/blob/master/sys/linux/uinput.txt > Hopefully you can read it without much explanation, it basically > states that there is that node in /dev and here are ioctls and other > syscalls that are relevant for this device and here are types of > arguments and layout of involved data structures. > > Turned we actually have such descriptions for /dev/video* and > /dev/v4l-subdev*: > https://github.com/google/syzkaller/blob/master/sys/linux/video4linux.txt > But we don't have anything for /dev/media*, fuzzer merely knows that > it can open the device: > https://github.com/google/syzkaller/blob/12b38f22c18c6109a5cc1c0238d015eef121b9b7/sys/linux/sys.txt#L479 > and then it will just blindly execute completely random workload on > it, e.g. most likely it won't be able to come up with a proper complex > structure layout for some ioctls. And I am actually not completely > sure about completeness and coverage of video4linux.txt descriptions > too as they were contributed by somebody interested in android > testing. A quick look suggests that it is based on the 4.9 videodev2.h, which ain't too bad. There are some differences between the 4.20 videodev2.h and the 4.9, but not too many. > > I wonder if somebody knowledgeable in /dev/media interface be willing > to contribute additional descriptions? We'll have to wait for 4.20-rc1 to be released since there are important additions to the media API. I can probably come up with something, I'm just not sure when I get around to it. Ping me in three weeks time if you haven't heard from me. > > We also have code coverage reports with the coverage fuzzer achieved > so far. Here in the Cover column: > https://syzkaller.appspot.com/#managers > e.g. this one (but note this is a ~80MB html file): > https://storage.googleapis.com/syzkaller/cover/ci-upstream-kasan-gce-root.html > This can be used to assess e.g. v4l coverage. But I don't know what's > coverable in general from syscalls and what's coverable via the stub > drivers in particular. So some expertise from media developers would > be helpful too. The four virtual drivers should give pretty decent coverage of the core code. Are you able to test with a 32-bit syzkaller application on a 64-bit kernel as well? That way the compat32 code is tested. > > Do I understand it correctly that when a process opens /dev/video* or > /dev/media* it gets a private instance of the device? In particular, > if several processes test this in parallel, will they collide? Or they > will stress separate objects? It actually depends on the driver. M2M devices will give you a private instance whenever you open it. Others do not, but you can call most ioctls in parallel. But after calling REQBUFS or CREATE_BUFS the filehandle that ca
Re: [PATCH 2/4] tw9910: No SoC camera dependency
On Tue, Oct 30, 2018 at 01:03:18PM +0100, Hans Verkuil wrote: > On 10/30/2018 12:00 AM, Sakari Ailus wrote: > > The tw9910 driver does not depend on SoC camera framework. Don't include > > the header, but instead include media/v4l2-async.h which the driver really > > needs. > > You might want to make a note of the fact that soc_camera.h includes > v4l2-async.h, so removing soc_camera.h requires adding v4l2-async.h. > > I couldn't understand how it compiled before without the v4l2-async.h > header until I saw that soc_camera.h includes it. Yes. How about this: Also include i2c/v4l2-async.h in drivers/media/i2c/tw9910.c as it depends on the header which used to be included through media/soc_camera.h. -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH 2/4] tw9910: No SoC camera dependency
On 10/31/2018 10:49 AM, Sakari Ailus wrote: > On Tue, Oct 30, 2018 at 01:03:18PM +0100, Hans Verkuil wrote: >> On 10/30/2018 12:00 AM, Sakari Ailus wrote: >>> The tw9910 driver does not depend on SoC camera framework. Don't include >>> the header, but instead include media/v4l2-async.h which the driver really >>> needs. >> >> You might want to make a note of the fact that soc_camera.h includes >> v4l2-async.h, so removing soc_camera.h requires adding v4l2-async.h. >> >> I couldn't understand how it compiled before without the v4l2-async.h >> header until I saw that soc_camera.h includes it. > > Yes. How about this: > > Also include i2c/v4l2-async.h in drivers/media/i2c/tw9910.c as it depends > on the header which used to be included through media/soc_camera.h. > Looks good. With that change: Acked-by: Hans Verkuil for the whole series. Regards, Hans
Re: [PATCH 4/4] SoC camera: Tidy the header
On Wed, Oct 31, 2018 at 06:40:30AM -0300, Mauro Carvalho Chehab wrote: > Em Wed, 31 Oct 2018 11:29:45 +0200 > Sakari Ailus escreveu: > > > Hi Mauro, > > > > On Tue, Oct 30, 2018 at 09:06:18AM -0300, Mauro Carvalho Chehab wrote: > > > Em Tue, 30 Oct 2018 01:00:29 +0200 > > > Sakari Ailus escreveu: > > > > > > > Clean up the SoC camera framework header. It only exists now to keep > > > > board > > > > code compiling. The header can be removed once the board code > > > > dependencies > > > > to it has been removed. > > > > > > > > Signed-off-by: Sakari Ailus > > > > --- > > > > include/media/soc_camera.h | 335 > > > > - > > > > 1 file changed, 335 deletions(-) > > > > > > > > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h > > > > index b7e42a1b0910..14d19da6052a 100644 > > > > --- a/include/media/soc_camera.h > > > > +++ b/include/media/soc_camera.h > > > > @@ -22,172 +22,6 @@ > > > > #include > > > > #include > > > > > > That doesn't make any sense. soc_camera.h should have the same fate > > > as the entire soc_camera infrastructure: either be removed or moved > > > to staging, and everything else that doesn't have the same fate > > > should get rid of this header. > > > > We can't just remove this; there is board code that depends on it. > > > > The intent is to remove the board code as well but presuming that the board > > code would be merged through a different tree, it'd be less hassle to wait > > a bit; hence the patch removing any unnecessary stuff here. > > Then we need *first* to remove board code, wait for those changes to be > applied and *then* remove soc_camera (and not the opposite). I'll post patches to remove the affeced board code soon, too. I have no objections to merge it through the media tree either, if the maintainers agree. -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH 4/4] SoC camera: Tidy the header
On 10/31/2018 10:40 AM, Mauro Carvalho Chehab wrote: > Em Wed, 31 Oct 2018 11:29:45 +0200 > Sakari Ailus escreveu: > >> Hi Mauro, >> >> On Tue, Oct 30, 2018 at 09:06:18AM -0300, Mauro Carvalho Chehab wrote: >>> Em Tue, 30 Oct 2018 01:00:29 +0200 >>> Sakari Ailus escreveu: >>> Clean up the SoC camera framework header. It only exists now to keep board code compiling. The header can be removed once the board code dependencies to it has been removed. Signed-off-by: Sakari Ailus --- include/media/soc_camera.h | 335 - 1 file changed, 335 deletions(-) diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h index b7e42a1b0910..14d19da6052a 100644 --- a/include/media/soc_camera.h +++ b/include/media/soc_camera.h @@ -22,172 +22,6 @@ #include #include >>> >>> That doesn't make any sense. soc_camera.h should have the same fate >>> as the entire soc_camera infrastructure: either be removed or moved >>> to staging, and everything else that doesn't have the same fate >>> should get rid of this header. >> >> We can't just remove this; there is board code that depends on it. >> >> The intent is to remove the board code as well but presuming that the board >> code would be merged through a different tree, it'd be less hassle to wait >> a bit; hence the patch removing any unnecessary stuff here. > > Then we need *first* to remove board code, wait for those changes to be > applied and *then* remove soc_camera (and not the opposite). Please note that the camera support for all the remaining boards using soc_camera has been dead for years. The soc_camera drivers that they depended on have been removed a long time ago. So all they depend on are the header. We can safely remove soc_camera without loss of functionality (and thus prevent others from basing new drivers on soc_camera), while we work to update the board files so we can remove this last header. I have modified some board files here: https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=rm-soc-camera&id=d7ae2fcf6e447022f0780bb86a2b63d5c7cf4d35 Only arch/arm/mach-imx/mach-imx27_visstrim_m10.c hasn't been fixed yet in that patch (ENOTIME). The problem is just lack of time to clean this up and figure out who should take these board patches. So I think it is a nice solution to just replace the header with a dummy version so the board files still compile, and then we can delete the dead soc_camera driver. It's probably easier as well to push through the board file changes once soc_camera is removed since you can just point out that the framework it depended on is gone. Regards, Hans
Re: [PATCH 3/4] SoC camera: Remove the framework and the drivers
Em Wed, 31 Oct 2018 11:32:02 +0200 Sakari Ailus escreveu: > Hi Mauro, > > On Tue, Oct 30, 2018 at 05:35:23PM -0300, Mauro Carvalho Chehab wrote: > > Em Tue, 30 Oct 2018 21:28:57 +0100 > > jacopo mondi escreveu: > > > > > Hi Mauro, > > > > > > On Tue, Oct 30, 2018 at 09:14:09AM -0300, Mauro Carvalho Chehab wrote: > > > > Em Tue, 30 Oct 2018 01:21:34 +0200 > > > > Sakari Ailus escreveu: > > > > > > > > > The SoC camera framework has been obsolete for some time and it is no > > > > > longer functional. A few drivers have been converted to the V4L2 > > > > > sub-device API but for the rest the conversion has not taken place > > > > > yet. > > > > > > > > > > In order to keep the tree clean and to avoid keep maintaining > > > > > non-functional and obsolete code, remove the SoC camera framework as > > > > > well > > > > > as the drivers that depend on it. > > > > > > > > > > Signed-off-by: Sakari Ailus > > > > > --- > > > > > Resending, this time with git format-patch -D . > > > > > > > > > > MAINTAINERS|8 - > > > > > drivers/media/i2c/Kconfig |8 - > > > > > drivers/media/i2c/Makefile |1 - > > > > > drivers/media/i2c/soc_camera/Kconfig | 66 - > > > > > drivers/media/i2c/soc_camera/Makefile | 10 - > > > > > drivers/media/i2c/soc_camera/ov9640.h | 208 -- > > > > > drivers/media/i2c/soc_camera/soc_mt9m001.c | 757 --- > > > > > drivers/media/i2c/soc_camera/soc_mt9t112.c | 1157 --- > > > > > drivers/media/i2c/soc_camera/soc_mt9v022.c | 1012 - > > > > > drivers/media/i2c/soc_camera/soc_ov5642.c | 1087 -- > > > > > drivers/media/i2c/soc_camera/soc_ov772x.c | 1123 -- > > > > > drivers/media/i2c/soc_camera/soc_ov9640.c | 738 --- > > > > > drivers/media/i2c/soc_camera/soc_ov9740.c | 996 - > > > > > drivers/media/i2c/soc_camera/soc_rj54n1cb0c.c | 1415 > > > > > - > > > > > drivers/media/i2c/soc_camera/soc_tw9910.c | 999 - > > > > > > > > > > > > > I don't see why we should remove those. I mean, Jacopo is > > > > actually converting those drivers to not depend on soc_camera, > > > > and it is a way better to review those patches with the old > > > > code in place. > > > > > > I have converted a few drivers used by some SH boards where I dropped > > > dependencies on soc_camera, not to remove camera support from those. For > > > others I don't have cameras to test with, nor I know about boards in > > > mainline using them. > > > > > > From my side, driver conversion is done. > > > > > > > > > > > So, at least while Jacopo is keep doing this work, I would keep > > > > at Kernel tree, as it helps to see a diff when the driver changes > > > > when getting rid of soc_camera dependencies. > > > > > > > > So, IMO, the best would be to move those to /staging, eventually > > > > depending on BROKEN. > > > > > > However, somebody with a (rather old) development setup using those camera > > > sensor may wants to see if mainline supports them. We actually had a > > > few patches coming lately (for ov. I understand Sakari's argument that > > > those > > > could be retrieved from git history, but a few people will notice imo. > > > I also understand the additional maintainership burden of keeping them > > > around, so I'm fine with either ways ;) > > > > > > This is a list of the current situation in mainline, to have a better > > > idea: > > > > > > $for i in `seq 1 9`; do CAM=$(head -n $i /tmp/soc_cams | tail -n 1); echo > > > $CAM; find drivers/media/ -name $CAM; done > > > t9m001.c > > > drivers/media/i2c/soc_camera/mt9m001.c > > > mt9t112.c > > > drivers/media/i2c/mt9t112.c > > > drivers/media/i2c/soc_camera/mt9t112.c > > > mt9v022.c > > > drivers/media/i2c/soc_camera/mt9v022.c > > > ov5642.c > > > drivers/media/i2c/soc_camera/ov5642.c > > > ov772x.c > > > drivers/media/i2c/ov772x.c > > > drivers/media/i2c/soc_camera/ov772x.c > > > ov9640.c > > > drivers/media/i2c/soc_camera/ov9640.c > > > ov9740.c > > > drivers/media/i2c/soc_camera/ov9740.c > > > rj54n1cb0c.c > > > drivers/media/i2c/rj54n1cb0c.c > > > drivers/media/i2c/soc_camera/rj54n1cb0c.c > > > tw9910.c > > > drivers/media/i2c/tw9910.c > > > drivers/media/i2c/soc_camera/tw9910.c > > > > > > So it seems to me only the following sensor do not have a > > > non-soc_camera driver at the moment: > > > > > > mt9m001.c > > > mt9v022.c > > > ov5642.c > > > ov9640.c > > > ov9740.c > > > > Ok. So, what about keeping just those 5 drivers at staging? If, after an > > year, people won't do conversions, we can just drop them. > > They've been there for years without anyone converting them. Do note that > the conversion can be still done once the code is removed. Well, people converted a lot of drivers already. See above. It is just that i
Re: VIVID/VIMC and media fuzzing
CC-ing Sean Young: please see question at the end. On 10/31/2018 10:46 AM, Hans Verkuil wrote: > On 10/30/2018 03:02 PM, Dmitry Vyukov wrote: >> Hello Helen and linux-media, >> >> I've attended your talk "Shifting Media App Development into High >> Gear" on OSS Summit last week and approached you with some questions >> if/how this can be used for kernel testing. Thanks, turn out to be a >> very useful talk! >> >> I am working on syzkaller/syzbot, continuous kernel fuzzing system: >> https://github.com/google/syzkaller >> https://github.com/google/syzkaller/blob/master/docs/syzbot.md >> https://syzkaller.appspot.com >> >> After simply enabling CONFIG_VIDEO_VIMC, CONFIG_VIDEO_VIM2M, >> CONFIG_VIDEO_VIVID, CONFIG_VIDEO_VICODEC syzbot has found 8 bugs in >> media subsystem in just 24 hours: >> >> KASAN: use-after-free Read in vb2_mmap >> https://groups.google.com/forum/#!msg/syzkaller-bugs/XGGH69jMWQ0/S8vfxgEmCgAJ >> >> KASAN: use-after-free Write in __vb2_cleanup_fileio >> https://groups.google.com/forum/#!msg/syzkaller-bugs/qKKhsZVPo3o/P6AB2of2CQAJ >> >> WARNING in __vb2_queue_cancel >> https://groups.google.com/forum/#!msg/syzkaller-bugs/S29GU_NtfPY/ZvAz8UDtCQAJ >> >> divide error in vivid_vid_cap_s_dv_timings >> https://groups.google.com/forum/#!msg/syzkaller-bugs/GwF5zGBCfyg/wnuWmW_sCQAJ > > Should be fixed by https://patchwork.linuxtv.org/patch/52641/ > >> >> KASAN: use-after-free Read in wake_up_if_idle >> https://groups.google.com/forum/#!msg/syzkaller-bugs/aBWb_yV1kiI/sWQO63fkCQAJ >> >> KASAN: use-after-free Read in __vb2_perform_fileio >> https://groups.google.com/forum/#!msg/syzkaller-bugs/MdFCZHz0LUQ/qSK_bFbcCQAJ >> >> INFO: task hung in vivid_stop_generating_vid_cap >> https://groups.google.com/forum/#!msg/syzkaller-bugs/F_KFW6PVyTA/wTBeHLfTCQAJ >> >> KASAN: null-ptr-deref Write in kthread_stop >> https://groups.google.com/forum/#!msg/syzkaller-bugs/u0AGnYvSlf4/fUiyfA_TCQAJ > > These last two should be fixed by https://patchwork.linuxtv.org/patch/52640/ > > Haven't figured out the others yet (hope to get back to that next week). > >> >> Based on this I think if we put more effort into media fuzzing, it >> will be able to find dozens more. > > Yeah, this is good stuff. Thank you for setting this up. > >> >> syzkaller needs descriptions of kernel interfaces to efficiently cover >> a subsystem. For example, see: >> https://github.com/google/syzkaller/blob/master/sys/linux/uinput.txt >> Hopefully you can read it without much explanation, it basically >> states that there is that node in /dev and here are ioctls and other >> syscalls that are relevant for this device and here are types of >> arguments and layout of involved data structures. >> >> Turned we actually have such descriptions for /dev/video* and >> /dev/v4l-subdev*: >> https://github.com/google/syzkaller/blob/master/sys/linux/video4linux.txt >> But we don't have anything for /dev/media*, fuzzer merely knows that >> it can open the device: >> https://github.com/google/syzkaller/blob/12b38f22c18c6109a5cc1c0238d015eef121b9b7/sys/linux/sys.txt#L479 >> and then it will just blindly execute completely random workload on >> it, e.g. most likely it won't be able to come up with a proper complex >> structure layout for some ioctls. And I am actually not completely >> sure about completeness and coverage of video4linux.txt descriptions >> too as they were contributed by somebody interested in android >> testing. > > A quick look suggests that it is based on the 4.9 videodev2.h, which ain't > too bad. There are some differences between the 4.20 videodev2.h and the > 4.9, but not too many. > >> >> I wonder if somebody knowledgeable in /dev/media interface be willing >> to contribute additional descriptions? > > We'll have to wait for 4.20-rc1 to be released since there are important > additions to the media API. I can probably come up with something, I'm > just not sure when I get around to it. Ping me in three weeks time if you > haven't heard from me. While adding support for the media API I should also add support for the CEC API, since vivid can emulate a CEC device as well. It would be really neat to have similar support for DVB and RC as well, but we don't have virtual drivers for those. Although CEC can expose an RC input device if enabled by the kernel config. Sean, would that be a good starting point for fuzzing the RC API? Or do you think we really need proper RC emulation in vivid as we discussed last week? Regards, Hans
Re: [PATCH 4/4] SoC camera: Tidy the header
Em Wed, 31 Oct 2018 11:00:22 +0100
Hans Verkuil escreveu:
> On 10/31/2018 10:40 AM, Mauro Carvalho Chehab wrote:
> > Em Wed, 31 Oct 2018 11:29:45 +0200
> > Sakari Ailus escreveu:
> >
> >> Hi Mauro,
> >>
> >> On Tue, Oct 30, 2018 at 09:06:18AM -0300, Mauro Carvalho Chehab wrote:
> >>> Em Tue, 30 Oct 2018 01:00:29 +0200
> >>> Sakari Ailus escreveu:
> >>>
> Clean up the SoC camera framework header. It only exists now to keep
> board
> code compiling. The header can be removed once the board code
> dependencies
> to it has been removed.
>
> Signed-off-by: Sakari Ailus
> ---
> include/media/soc_camera.h | 335
> -
> 1 file changed, 335 deletions(-)
>
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index b7e42a1b0910..14d19da6052a 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -22,172 +22,6 @@
> #include
> #include
> >>>
> >>> That doesn't make any sense. soc_camera.h should have the same fate
> >>> as the entire soc_camera infrastructure: either be removed or moved
> >>> to staging, and everything else that doesn't have the same fate
> >>> should get rid of this header.
> >>
> >> We can't just remove this; there is board code that depends on it.
> >>
> >> The intent is to remove the board code as well but presuming that the board
> >> code would be merged through a different tree, it'd be less hassle to wait
> >> a bit; hence the patch removing any unnecessary stuff here.
> >
> > Then we need *first* to remove board code, wait for those changes to be
> > applied and *then* remove soc_camera (and not the opposite).
>
> Please note that the camera support for all the remaining boards using
> soc_camera has been dead for years. The soc_camera drivers that they depended
> on have been removed a long time ago.
>
> So all they depend on are the header. We can safely remove soc_camera without
> loss of functionality (and thus prevent others from basing new drivers on
> soc_camera), while we work to update the board files so we can remove this
> last
> header.
>
> I have modified some board files here:
>
> https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=rm-soc-camera&id=d7ae2fcf6e447022f0780bb86a2b63d5c7cf4d35
Good! the arch-specific mach-* changes should likely be on separate
patches, though.
> Only arch/arm/mach-imx/mach-imx27_visstrim_m10.c hasn't been fixed yet in that
> patch (ENOTIME).
So, the code we don't manage seems to be just 3 archs, right (mach-omap1,
mach-pxa and mach-imx)?
Btw, the soc_camera dependent code at mach-imx27_visstrim_m10.c is:
static struct soc_camera_link iclink_tvp5150 = {
.bus_id = 0,
.board_info = &visstrim_i2c_camera,
.i2c_adapter_id = 0,
.power = visstrim_camera_power,
.reset = visstrim_camera_reset,
};
...
platform_device_register_resndata(NULL, "soc-camera-pdrv", 0, NULL, 0,
&iclink_tvp5150, sizeof(iclink_tvp5150));
As tvp5150 is not actually a soc_camera driver, I suspect that
the conversion here would be to make it to use the tvp5150 driver
directly, instead of doing it via soc_camera.
> The problem is just lack of time to clean this up and figure out who should
> take these board patches.
No need to rush it.
> So I think it is a nice solution to just replace the header with a dummy
> version
> so the board files still compile, and then we can delete the dead soc_camera
> driver. It's probably easier as well to push through the board file changes
> once
> soc_camera is removed since you can just point out that the framework it
> depended
> on is gone.
I still think that reverting the order and rushing the removal is not the
way to go.
For example, on that specific imx.27 board above mentioned, it may
still be working with the tvp5150 driver[1] and replacing the soc_camera.h
by a dummy version would just break for no good reason.
[1] I don't have such board to test, nor I checked if what's left from
soc_camera still allows the tvp5150 driver to register.
Thanks,
Mauro
Re: VIVID/VIMC and media fuzzing
On Wed, Oct 31, 2018 at 11:05:10AM +0100, Hans Verkuil wrote: > CC-ing Sean Young: please see question at the end. > > On 10/31/2018 10:46 AM, Hans Verkuil wrote: > > On 10/30/2018 03:02 PM, Dmitry Vyukov wrote: > >> Hello Helen and linux-media, > >> > >> I've attended your talk "Shifting Media App Development into High > >> Gear" on OSS Summit last week and approached you with some questions > >> if/how this can be used for kernel testing. Thanks, turn out to be a > >> very useful talk! > >> > >> I am working on syzkaller/syzbot, continuous kernel fuzzing system: > >> https://github.com/google/syzkaller > >> https://github.com/google/syzkaller/blob/master/docs/syzbot.md > >> https://syzkaller.appspot.com > >> > >> After simply enabling CONFIG_VIDEO_VIMC, CONFIG_VIDEO_VIM2M, > >> CONFIG_VIDEO_VIVID, CONFIG_VIDEO_VICODEC syzbot has found 8 bugs in > >> media subsystem in just 24 hours: > >> > >> KASAN: use-after-free Read in vb2_mmap > >> https://groups.google.com/forum/#!msg/syzkaller-bugs/XGGH69jMWQ0/S8vfxgEmCgAJ > >> > >> KASAN: use-after-free Write in __vb2_cleanup_fileio > >> https://groups.google.com/forum/#!msg/syzkaller-bugs/qKKhsZVPo3o/P6AB2of2CQAJ > >> > >> WARNING in __vb2_queue_cancel > >> https://groups.google.com/forum/#!msg/syzkaller-bugs/S29GU_NtfPY/ZvAz8UDtCQAJ > >> > >> divide error in vivid_vid_cap_s_dv_timings > >> https://groups.google.com/forum/#!msg/syzkaller-bugs/GwF5zGBCfyg/wnuWmW_sCQAJ > > > > Should be fixed by https://patchwork.linuxtv.org/patch/52641/ > > > >> > >> KASAN: use-after-free Read in wake_up_if_idle > >> https://groups.google.com/forum/#!msg/syzkaller-bugs/aBWb_yV1kiI/sWQO63fkCQAJ > >> > >> KASAN: use-after-free Read in __vb2_perform_fileio > >> https://groups.google.com/forum/#!msg/syzkaller-bugs/MdFCZHz0LUQ/qSK_bFbcCQAJ > >> > >> INFO: task hung in vivid_stop_generating_vid_cap > >> https://groups.google.com/forum/#!msg/syzkaller-bugs/F_KFW6PVyTA/wTBeHLfTCQAJ > >> > >> KASAN: null-ptr-deref Write in kthread_stop > >> https://groups.google.com/forum/#!msg/syzkaller-bugs/u0AGnYvSlf4/fUiyfA_TCQAJ > > > > These last two should be fixed by https://patchwork.linuxtv.org/patch/52640/ > > > > Haven't figured out the others yet (hope to get back to that next week). > > > >> > >> Based on this I think if we put more effort into media fuzzing, it > >> will be able to find dozens more. > > > > Yeah, this is good stuff. Thank you for setting this up. > > > >> > >> syzkaller needs descriptions of kernel interfaces to efficiently cover > >> a subsystem. For example, see: > >> https://github.com/google/syzkaller/blob/master/sys/linux/uinput.txt > >> Hopefully you can read it without much explanation, it basically > >> states that there is that node in /dev and here are ioctls and other > >> syscalls that are relevant for this device and here are types of > >> arguments and layout of involved data structures. > >> > >> Turned we actually have such descriptions for /dev/video* and > >> /dev/v4l-subdev*: > >> https://github.com/google/syzkaller/blob/master/sys/linux/video4linux.txt > >> But we don't have anything for /dev/media*, fuzzer merely knows that > >> it can open the device: > >> https://github.com/google/syzkaller/blob/12b38f22c18c6109a5cc1c0238d015eef121b9b7/sys/linux/sys.txt#L479 > >> and then it will just blindly execute completely random workload on > >> it, e.g. most likely it won't be able to come up with a proper complex > >> structure layout for some ioctls. And I am actually not completely > >> sure about completeness and coverage of video4linux.txt descriptions > >> too as they were contributed by somebody interested in android > >> testing. > > > > A quick look suggests that it is based on the 4.9 videodev2.h, which ain't > > too bad. There are some differences between the 4.20 videodev2.h and the > > 4.9, but not too many. > > > >> > >> I wonder if somebody knowledgeable in /dev/media interface be willing > >> to contribute additional descriptions? > > > > We'll have to wait for 4.20-rc1 to be released since there are important > > additions to the media API. I can probably come up with something, I'm > > just not sure when I get around to it. Ping me in three weeks time if you > > haven't heard from me. > > While adding support for the media API I should also add support for the CEC > API, since vivid can emulate a CEC device as well. > > It would be really neat to have similar support for DVB and RC as well, but we > don't have virtual drivers for those. Although CEC can expose an RC input > device > if enabled by the kernel config. I've been looking at this (adding DVB and RC) as we discussed. I like this idea, as it also helps me with getting started with DVB. > Sean, would that be a good starting point for fuzzing the RC API? Or do you > think > we really need proper RC emulation in vivid as we discussed last week? So if we have an RC device which his not CEC, then we can do much more testing with raw IR for example. I'll make a start this
Re: VIVID/VIMC and media fuzzing
Hi Dmitry, On 10/31/18 7:46 AM, Hans Verkuil wrote: > On 10/30/2018 03:02 PM, Dmitry Vyukov wrote: >> Hello Helen and linux-media, >> >> I've attended your talk "Shifting Media App Development into High >> Gear" on OSS Summit last week and approached you with some questions >> if/how this can be used for kernel testing. Thanks, turn out to be a >> very useful talk! Great, I am glad it was useful :) >> >> I am working on syzkaller/syzbot, continuous kernel fuzzing system: >> https://github.com/google/syzkaller >> https://github.com/google/syzkaller/blob/master/docs/syzbot.md >> https://syzkaller.appspot.com >> >> After simply enabling CONFIG_VIDEO_VIMC, CONFIG_VIDEO_VIM2M, >> CONFIG_VIDEO_VIVID, CONFIG_VIDEO_VICODEC syzbot has found 8 bugs in >> media subsystem in just 24 hours: >> >> KASAN: use-after-free Read in vb2_mmap >> https://groups.google.com/forum/#!msg/syzkaller-bugs/XGGH69jMWQ0/S8vfxgEmCgAJ >> >> KASAN: use-after-free Write in __vb2_cleanup_fileio >> https://groups.google.com/forum/#!msg/syzkaller-bugs/qKKhsZVPo3o/P6AB2of2CQAJ >> >> WARNING in __vb2_queue_cancel >> https://groups.google.com/forum/#!msg/syzkaller-bugs/S29GU_NtfPY/ZvAz8UDtCQAJ >> >> divide error in vivid_vid_cap_s_dv_timings >> https://groups.google.com/forum/#!msg/syzkaller-bugs/GwF5zGBCfyg/wnuWmW_sCQAJ > > Should be fixed by https://patchwork.linuxtv.org/patch/52641/ > >> >> KASAN: use-after-free Read in wake_up_if_idle >> https://groups.google.com/forum/#!msg/syzkaller-bugs/aBWb_yV1kiI/sWQO63fkCQAJ >> >> KASAN: use-after-free Read in __vb2_perform_fileio >> https://groups.google.com/forum/#!msg/syzkaller-bugs/MdFCZHz0LUQ/qSK_bFbcCQAJ >> >> INFO: task hung in vivid_stop_generating_vid_cap >> https://groups.google.com/forum/#!msg/syzkaller-bugs/F_KFW6PVyTA/wTBeHLfTCQAJ >> >> KASAN: null-ptr-deref Write in kthread_stop >> https://groups.google.com/forum/#!msg/syzkaller-bugs/u0AGnYvSlf4/fUiyfA_TCQAJ > > These last two should be fixed by https://patchwork.linuxtv.org/patch/52640/ > > Haven't figured out the others yet (hope to get back to that next week). > >> >> Based on this I think if we put more effort into media fuzzing, it >> will be able to find dozens more. > > Yeah, this is good stuff. Thank you for setting this up. Agreed, Dmitry thank you for doing this. > >> >> syzkaller needs descriptions of kernel interfaces to efficiently cover >> a subsystem. For example, see: >> https://github.com/google/syzkaller/blob/master/sys/linux/uinput.txt >> Hopefully you can read it without much explanation, it basically >> states that there is that node in /dev and here are ioctls and other >> syscalls that are relevant for this device and here are types of >> arguments and layout of involved data structures. >> >> Turned we actually have such descriptions for /dev/video* and >> /dev/v4l-subdev*: >> https://github.com/google/syzkaller/blob/master/sys/linux/video4linux.txt >> But we don't have anything for /dev/media*, fuzzer merely knows that >> it can open the device: >> https://github.com/google/syzkaller/blob/12b38f22c18c6109a5cc1c0238d015eef121b9b7/sys/linux/sys.txt#L479 >> and then it will just blindly execute completely random workload on >> it, e.g. most likely it won't be able to come up with a proper complex >> structure layout for some ioctls. And I am actually not completely >> sure about completeness and coverage of video4linux.txt descriptions >> too as they were contributed by somebody interested in android >> testing. > > A quick look suggests that it is based on the 4.9 videodev2.h, which ain't > too bad. There are some differences between the 4.20 videodev2.h and the > 4.9, but not too many. > >> >> I wonder if somebody knowledgeable in /dev/media interface be willing >> to contribute additional descriptions? > > We'll have to wait for 4.20-rc1 to be released since there are important > additions to the media API. I can probably come up with something, I'm > just not sure when I get around to it. Ping me in three weeks time if you > haven't heard from me. > >> >> We also have code coverage reports with the coverage fuzzer achieved >> so far. Here in the Cover column: >> https://syzkaller.appspot.com/#managers >> e.g. this one (but note this is a ~80MB html file): >> https://storage.googleapis.com/syzkaller/cover/ci-upstream-kasan-gce-root.html >> This can be used to assess e.g. v4l coverage. But I don't know what's >> coverable in general from syscalls and what's coverable via the stub >> drivers in particular. So some expertise from media developers would >> be helpful too. > > The four virtual drivers should give pretty decent coverage of the core > code. Are you able to test with a 32-bit syzkaller application on a 64-bit > kernel as well? That way the compat32 code is tested. > >> >> Do I understand it correctly that when a process opens /dev/video* or >> /dev/media* it gets a private instance of the device? In particular, >> if several processes test this in parallel, will they collide? Or they >>
Re: i.MX6: can't capture on MIPI-CSI2 with DS90UB954
Hi Jean-Michel,
We've done some work with another FPD-Link de-serializer (ds90ux940) and
IIRC we had some trouble figuring out how to coax the lanes into LP-11
state. But on the ds90ux940 it can be done by setting bit 7 in the CSI
Enable Port registers (offsets 0x13 and 0x14). But the "imx6-mipi-csi2:
clock lane timeout" message is something else and indicates the
de-serializer is not activating the clock lane during its s_stream(ON)
subdev call.
Hope that helps,
Steve
On 10/30/18 9:41 AM, Jean-Michel Hautbois wrote:
Hi there,
I am using the i.MX6D from Digi (connect core 6 sbc) with a mailine
kernel (well, 4.14 right now) and have an issue with mipi-csi2
capture.
First I will give brief explanation of my setup, and then I will
detail the issue.
I have a camera sensor (OV2732, but could be any other sensor)
connected on a DS90UB953 FPD-Link III serializer.
Then a coax cable propagates the signal to a DS90UB954 FPD-Link III
deserializer.
The DS90UB954 has the ability to work in a pattern generation mode,
and I will use it for the rest of the discussion.
It is an I²C device, and I have written a basic driver (for the moment
;)) in order to make it visible on the imx6-mipi-csi2 bus as a camera
sensor.
I can give an access to the driver if necessary.
I then program the MC pipeline :
media-ctl -l "'ds90ub954 2-0034':0 -> 'imx6-mipi-csi2':0[1]" -v
media-ctl -l "'imx6-mipi-csi2':1 -> 'ipu1_csi0_mux':0[1]" -v
media-ctl -l "'ipu1_csi0_mux':2 -> 'ipu1_csi0':0[1]" -v
media-ctl -l "'ipu1_csi0':2 -> 'ipu1_csi0 capture':0[1]"
media-ctl -V "'ds90ub954 2-0034':0 [fmt:RGB888_1X24/1280x720 field:none]"
media-ctl -V "'imx6-mipi-csi2':1 [fmt:RGB888_1X24/1280x720 field:none]"
media-ctl -V "'ipu1_csi0_mux':2 [fmt:RGB888_1X24/1280x720 field:none]"
media-ctl -V "'ipu1_csi0':2 [fmt:RGB888_1X24/1280x720 field:none]"
Everything works fine, all nodes are correctly configured, and the
DS90UB954 is normaly sending data on 2 lanes, with VC-ID=0.
The pattern is 1280x720@30 RGB888.
Then, I start a Gstreamer pipeline (I tried with v4l2-ctl and have the
same issue though) :
GST_DEBUG="v4l2:5" gst-launch-1.0 v4l2src device=/dev/video4 !
video/x-raw, width=1280, height=720, format=RGB ! fakesink
And... well, I had to use this patch
https://lkml.org/lkml/2017/3/11/270 in order to go further, but I am
finishing into :
[ 164.077302] imx-ipuv3-csi imx-ipuv3-csi.0: stream ON
[ 164.097955] imx-ipuv3-csi imx-ipuv3-csi.0: FI=3 usec
[ 165.129424] ipu1_csi0: EOF timeout
[ 165.142395] imx-ipuv3-csi imx-ipuv3-csi.0: stream OFF
[ 166.169299] ipu1_csi0: wait last EOF timeout
Sounds like a recurrent issue on this ML :).
I can think of several things which could explain this, but I tried a
lot and am a bit stuck.
The clock is set to 800MHz on DS90UB954 side.
=> Should CSI2_PHY_TST_CTRL1 be 0x32 ? 0x12 ? or 0x4a (ie 400MHz) ?
I think I have tried all but still the same issue.
Maybe this could be a hint, when booting, the first stream-on leads to:
imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200 => just a warning now
imx6-mipi-csi2: clock lane timeout, phy_state = 0x0230
The next one leads to the EOF timeout.
Here is the dts part BTW :
&i2c3 {
status = "okay";
ds90ub954: camera@34 {
compatible = "ti,ds90ub954";
status = "okay";
reg = <0x34>;
clocks = <&clks IMX6QDL_CLK_CKO>;
clock-names = "xclk";
port {
#address-cells = <1>;
#size-cells = <0>;
ds90ub954_out0: endpoint {
remote-endpoint = <&mipi_csi2_in>;
clock-lanes = <0>;
data-lanes = <1 2>;
};
};
};
};
&mipi_csi {
status = "okay";
port@0 {
reg = <0>;
mipi_csi2_in: endpoint {
remote-endpoint = <&ds90ub954_out0>;
clock-lanes = <0>;
data-lanes = <1 2>;
};
};
};
If ayone has a suggestion...
Thanks a lot !
Regards,
JM
Geschäfts- / Projektkredit 1,5%
-- Schönen Tag. Wir hoffen, Sie gut zu treffen. Benötigen Sie einen dringenden Kredit für ein Geschäft oder ein Projekt? Wir bieten Kredite zu 1,5% und wir können Ihre Transaktion innerhalb von 3 bis 5 Arbeitstagen abschließen. Wenn Sie ernsthaft an einem Kredit interessiert sind, empfehlen wir Ihnen, unten die Einzelheiten zur Bearbeitung Ihrer Transaktion anzugeben. Vollständiger Name:.. Darlehensbetrag: .. Darlehensdauer: .. Darlehen Zweck:.. Telefon:.. Wir erwarten Ihre Darlehensdaten wie oben beschrieben für die Abwicklung Ihrer Transaktion. Freundliche Grüße. Wilson Rog. Buchhalter / Berater
Re: VIVID/VIMC and media fuzzing
Hi Dmitry, On Wed, Oct 31, 2018 at 6:46 PM Hans Verkuil wrote: > > On 10/30/2018 03:02 PM, Dmitry Vyukov wrote: [snip] > > > > Do I understand it correctly that when a process opens /dev/video* or > > /dev/media* it gets a private instance of the device? In particular, > > if several processes test this in parallel, will they collide? Or they > > will stress separate objects? > > It actually depends on the driver. M2M devices will give you a private > instance whenever you open it. Others do not, but you can call most ioctls > in parallel. But after calling REQBUFS or CREATE_BUFS the filehandle that > called those ioctls becomes owner of the device until the buffers are > released. So other filehandles cannot do any streaming operations (EBUSY > will be returned). FWIW, you can query whether the device is M2M or not by VIDIOC_QUERYCAP [1]. The capabilities field would have V4L2_CAP_VIDEO_M2M or V4L2_CAP_VIDEO_M2M_MPLANE set for M2M devices. [1] https://www.kernel.org/doc/html/latest/media/uapi/v4l/vidioc-querycap.html Best regards, Tomasz
[PATCH 0/2] media: video-i2c: add Melexis MLX90640 thermal camera support
Add initial support for Melexis line of thermal cameras. This is the first part of processing pipeline in which the real processing is done in userspace using the V4L2 camera data. Dependency patchset series: https://patchwork.kernel.org/cover/10650541/ Matt Ranostay (2): media: video-i2c: check if chip struct has set_power function media: video-i2c: add Melexis MLX90640 thermal camera support drivers/media/i2c/Kconfig | 1 + drivers/media/i2c/video-i2c.c | 131 -- 2 files changed, 126 insertions(+), 6 deletions(-) -- 2.17.1
[PATCH 1/2] media: video-i2c: check if chip struct has set_power function
Not all future supported video chips will always have power management
support, and so it is important to check before calling set_power() is
defined.
Signed-off-by: Matt Ranostay
---
drivers/media/i2c/video-i2c.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index cb5db5bdab12..6d3b6df0b634 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -736,9 +736,11 @@ static int video_i2c_probe(struct i2c_client *client,
video_set_drvdata(&data->vdev, data);
i2c_set_clientdata(client, data);
- ret = data->chip->set_power(data, true);
- if (ret)
- goto error_unregister_device;
+ if (data->chip->set_power) {
+ ret = data->chip->set_power(data, true);
+ if (ret)
+ goto error_unregister_device;
+ }
pm_runtime_get_noresume(&client->dev);
pm_runtime_set_active(&client->dev);
@@ -767,7 +769,8 @@ static int video_i2c_probe(struct i2c_client *client,
pm_runtime_disable(&client->dev);
pm_runtime_set_suspended(&client->dev);
pm_runtime_put_noidle(&client->dev);
- data->chip->set_power(data, false);
+ if (data->chip->set_power)
+ data->chip->set_power(data, false);
error_unregister_device:
v4l2_device_unregister(v4l2_dev);
@@ -791,7 +794,9 @@ static int video_i2c_remove(struct i2c_client *client)
pm_runtime_disable(&client->dev);
pm_runtime_set_suspended(&client->dev);
pm_runtime_put_noidle(&client->dev);
- data->chip->set_power(data, false);
+
+ if (data->chip->set_power)
+ data->chip->set_power(data, false);
video_unregister_device(&data->vdev);
@@ -804,6 +809,9 @@ static int video_i2c_pm_runtime_suspend(struct device *dev)
{
struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
+ if (!data->chip->set_power)
+ return 0;
+
return data->chip->set_power(data, false);
}
@@ -811,6 +819,9 @@ static int video_i2c_pm_runtime_resume(struct device *dev)
{
struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
+ if (!data->chip->set_power)
+ return 0;
+
return data->chip->set_power(data, true);
}
--
2.17.1
[PATCH 2/2] media: video-i2c: add Melexis MLX90640 thermal camera support
Add initial support for MLX90640 thermal cameras which output an 32x24
greyscale pixel image along with 2 rows of coefficent data.
Because of this the data outputed is really 32x26 and needs the two rows
removed after using the coefficent information to generate processed
images in userspace.
Signed-off-by: Matt Ranostay
---
drivers/media/i2c/Kconfig | 1 +
drivers/media/i2c/video-i2c.c | 110 +-
2 files changed, 110 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 704af210e270..4bfb2c66d192 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -1085,6 +1085,7 @@ config VIDEO_I2C
Enable the I2C transport video support which supports the
following:
* Panasonic AMG88xx Grid-Eye Sensors
+ * Melexis MLX90640 Thermal Cameras
To compile this driver as a module, choose M here: the
module will be called video-i2c
diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 6d3b6df0b634..38ade8cb7656 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -6,6 +6,7 @@
*
* Supported:
* - Panasonic AMG88xx Grid-Eye Sensors
+ * - Melexis MLX90640 Thermal Cameras
*/
#include
@@ -18,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -66,12 +68,26 @@ static const struct v4l2_frmsize_discrete amg88xx_size = {
.height = 8,
};
+static const struct v4l2_fmtdesc mlx90640_format = {
+ .pixelformat = V4L2_PIX_FMT_Y16_BE,
+};
+
+static const struct v4l2_frmsize_discrete mlx90640_size = {
+ .width = 32,
+ .height = 26, /* 24 lines of pixel data + 2 lines of processing data */
+};
+
static const struct regmap_config amg88xx_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
.max_register = 0xff
};
+static const struct regmap_config mlx90640_regmap_config = {
+ .reg_bits = 16,
+ .val_bits = 16,
+};
+
struct video_i2c_chip {
/* video dimensions */
const struct v4l2_fmtdesc *format;
@@ -88,6 +104,7 @@ struct video_i2c_chip {
unsigned int bpp;
const struct regmap_config *regmap_config;
+ struct nvmem_config *nvmem_config;
/* setup function */
int (*setup)(struct video_i2c_data *data);
@@ -102,6 +119,22 @@ struct video_i2c_chip {
int (*hwmon_init)(struct video_i2c_data *data);
};
+static int mlx90640_nvram_read(void *priv, unsigned int offset, void *val,
+size_t bytes)
+{
+ struct video_i2c_data *data = priv;
+
+ return regmap_bulk_read(data->regmap, 0x2400 + offset, val, bytes);
+}
+
+static struct nvmem_config mlx90640_nvram_config = {
+ .name = "mlx90640_nvram",
+ .word_size = 2,
+ .stride = 1,
+ .size = 1664,
+ .reg_read = mlx90640_nvram_read,
+};
+
/* Power control register */
#define AMG88XX_REG_PCTL 0x00
#define AMG88XX_PCTL_NORMAL0x00
@@ -122,12 +155,23 @@ struct video_i2c_chip {
/* Temperature register */
#define AMG88XX_REG_T01L 0x80
+/* Control register */
+#define MLX90640_REG_CTL1 0x800d
+#define MLX90640_REG_CTL1_MASK 0x0380
+#define MLX90640_REG_CTL1_MASK_SHIFT 7
+
static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
{
return regmap_bulk_read(data->regmap, AMG88XX_REG_T01L, buf,
data->chip->buffer_size);
}
+static int mlx90640_xfer(struct video_i2c_data *data, char *buf)
+{
+ return regmap_bulk_read(data->regmap, 0x400, buf,
+ data->chip->buffer_size);
+}
+
static int amg88xx_setup(struct video_i2c_data *data)
{
unsigned int mask = AMG88XX_FPSC_1FPS;
@@ -141,6 +185,27 @@ static int amg88xx_setup(struct video_i2c_data *data)
return regmap_update_bits(data->regmap, AMG88XX_REG_FPSC, mask, val);
}
+static int mlx90640_setup(struct video_i2c_data *data)
+{
+ unsigned int n, idx;
+
+ for (n = 0; n < data->chip->num_frame_intervals - 1; n++) {
+ if (data->frame_interval.numerator
+ != data->chip->frame_intervals[n].numerator)
+ continue;
+
+ if (data->frame_interval.denominator
+ == data->chip->frame_intervals[n].denominator)
+ break;
+ }
+
+ idx = data->chip->num_frame_intervals - n - 1;
+
+ return regmap_update_bits(data->regmap, MLX90640_REG_CTL1,
+ MLX90640_REG_CTL1_MASK,
+ idx << MLX90640_REG_CTL1_MASK_SHIFT);
+}
+
static int amg88xx_set_power_on(struct video_i2c_data *data)
{
int ret;
@@ -274,13 +339,27 @@ static int amg88xx_hwmon_init(struct video_i2c_data *data)
#defineamg88xx_hwmon_init NULL
#endif
-#define AMG88XX0
+
cron job: media_tree daily build: OK
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Thu Nov 1 05:00:11 CET 2018 media-tree git hash:3b796aa60af087f5fec75aee9b17f2130f2b9adc media_build git hash: 0c8bb27f3aaa682b9548b656f77505c3d1f11e71 v4l-utils git hash: c36dbbdfa8b30b2badd4f893b59d0bd4f0bd12aa edid-decode git hash: 5eeb151a748788666534d6ea3da07f90400d24c2 gcc version:i686-linux-gcc (GCC) 8.2.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.18.0-2-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-arm-stm32: OK linux-git-arm64: OK linux-git-i686: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK Check COMPILE_TEST: OK linux-3.10.108-i686: OK linux-3.10.108-x86_64: OK linux-3.11.10-i686: OK linux-3.11.10-x86_64: OK linux-3.12.74-i686: OK linux-3.12.74-x86_64: OK linux-3.13.11-i686: OK linux-3.13.11-x86_64: OK linux-3.14.79-i686: OK linux-3.14.79-x86_64: OK linux-3.15.10-i686: OK linux-3.15.10-x86_64: OK linux-3.16.57-i686: OK linux-3.16.57-x86_64: OK linux-3.17.8-i686: OK linux-3.17.8-x86_64: OK linux-3.18.123-i686: OK linux-3.18.123-x86_64: OK linux-3.19.8-i686: OK linux-3.19.8-x86_64: OK linux-4.0.9-i686: OK linux-4.0.9-x86_64: OK linux-4.1.52-i686: OK linux-4.1.52-x86_64: OK linux-4.2.8-i686: OK linux-4.2.8-x86_64: OK linux-4.3.6-i686: OK linux-4.3.6-x86_64: OK linux-4.4.159-i686: OK linux-4.4.159-x86_64: OK linux-4.5.7-i686: OK linux-4.5.7-x86_64: OK linux-4.6.7-i686: OK linux-4.6.7-x86_64: OK linux-4.7.10-i686: OK linux-4.7.10-x86_64: OK linux-4.8.17-i686: OK linux-4.8.17-x86_64: OK linux-4.9.131-i686: OK linux-4.9.131-x86_64: OK linux-4.10.17-i686: OK linux-4.10.17-x86_64: OK linux-4.11.12-i686: OK linux-4.11.12-x86_64: OK linux-4.12.14-i686: OK linux-4.12.14-x86_64: OK linux-4.13.16-i686: OK linux-4.13.16-x86_64: OK linux-4.14.74-i686: OK linux-4.14.74-x86_64: OK linux-4.15.18-i686: OK linux-4.15.18-x86_64: OK linux-4.16.18-i686: OK linux-4.16.18-x86_64: OK linux-4.17.19-i686: OK linux-4.17.19-x86_64: OK linux-4.18.12-i686: OK linux-4.18.12-x86_64: OK linux-4.19-i686: OK linux-4.19-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Thursday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Thursday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
[PATCH] media: venus: dynamic handling of bitrate
Any request for a change in bitrate after both planes
are streamed on is handled by setting the target bitrate
property to hardware.
Signed-off-by: Malathi Gottam
---
drivers/media/platform/qcom/venus/venc_ctrls.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c
b/drivers/media/platform/qcom/venus/venc_ctrls.c
index 45910172..54f310c 100644
--- a/drivers/media/platform/qcom/venus/venc_ctrls.c
+++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
@@ -79,7 +79,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
{
struct venus_inst *inst = ctrl_to_inst(ctrl);
struct venc_controls *ctr = &inst->controls.enc;
+ struct hfi_bitrate brate;
u32 bframes;
+ u32 ptype;
int ret;
switch (ctrl->id) {
@@ -88,6 +90,15 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
break;
case V4L2_CID_MPEG_VIDEO_BITRATE:
ctr->bitrate = ctrl->val;
+ if (inst->streamon_out && inst->streamon_cap) {
+ ptype = HFI_PROPERTY_CONFIG_VENC_TARGET_BITRATE;
+ brate.bitrate = ctr->bitrate;
+ brate.layer_id = 0;
+
+ ret = hfi_session_set_property(inst, ptype, &brate);
+ if (ret)
+ return ret;
+ }
break;
case V4L2_CID_MPEG_VIDEO_BITRATE_PEAK:
ctr->bitrate_peak = ctrl->val;
--
1.9.1
Re: [PATCH] dvb: Allow MAC addresses to be mapped to stable device names with udev
On Tue, Oct 30, 2018 at 09:35:31PM -0300, Mauro Carvalho Chehab wrote:
> Em Tue, 30 Oct 2018 22:32:50 +
> Sean Young escreveu:
>
> Thanks for reviewing it!
>
> > On Tue, Oct 30, 2018 at 11:03:19AM -0300, Mauro Carvalho Chehab wrote:
> > > Em Mon, 24 Sep 2018 11:10:31 +0100
> > > David Howells escreveu:
> > >
> > > > Some devices, such as the DVBSky S952 and T982 cards, are dual port
> > > > cards
> > > > that provide two cx23885 devices on the same PCI device, which means the
> > > > attributes available for writing udev rules are exactly the same, apart
> > > > from the adapter number. Unfortunately, the adapter numbers are
> > > > dependent
> > > > on the order in which things are initialised, so this can change over
> > > > different releases of the kernel.
> > > >
> > > > Devices have a MAC address available, which is printed during boot:
> >
> > Not all dvb devices have a mac address.
>
> True. Usually, devices without eeprom don't have, specially the too old ones.
>
> On others, the MAC address only appear after the firmware is loaded.
>
> > > >
> > > > [ 10.951517] DVBSky T982 port 1 MAC address: 00:11:22:33:44:55
> > > > ...
> > > > [ 10.984875] DVBSky T982 port 2 MAC address: 00:11:22:33:44:56
> > > >
> > > > To make it possible to distinguish these in udev, provide sysfs
> > > > attributes
> > > > to make the MAC address, adapter number and type available. There are
> > > > other fields that could perhaps be exported also. In particular, it
> > > > would
> > > > be nice to provide the port number, but somehow that doesn't manage to
> > > > propagate through the labyrinthine initialisation process.
> > > >
> > > > The new sysfs attributes can be seen from userspace as:
> > > >
> > > > [root@deneb ~]# ls /sys/class/dvb/dvb0.frontend0/
> > > > dev device dvb_adapter dvb_mac dvb_type
> > > > power subsystem uevent
> > > > [root@deneb ~]# cat /sys/class/dvb/dvb0.frontend0/dvb_*
> > > > 0
> > > > 00:11:22:33:44:55
> > > > frontend
> > > >
> > > > They can be used in udev rules:
> > > >
> > > > SUBSYSTEM=="dvb", ATTRS{vendor}=="0x14f1",
> > > > ATTRS{device}=="0x8852", ATTRS{subsystem_device}=="0x0982",
> > > > ATTR{dvb_mac}=="00:11:22:33:44:55", PROGRAM="/bin/sh -c 'K=%k;
> > > > K=$${K#dvb}; printf dvb/adapter9820/%%s $${K#*.}'", SYMLINK+="%c"
> > > > SUBSYSTEM=="dvb", ATTRS{vendor}=="0x14f1",
> > > > ATTRS{device}=="0x8852", ATTRS{subsystem_device}=="0x0982",
> > > > ATTR{dvb_mac}=="00:11.22.33.44.56", PROGRAM="/bin/sh -c 'K=%k;
> > > > K=$${K#dvb}; printf dvb/adapter9821/%%s $${K#*.}'", SYMLINK+="%c"
> > > >
> > > > where the match is made with ATTR{dvb_mac} or similar. The rules above
> > > > make symlinks from /dev/dvb/adapter982/* to /dev/dvb/adapterXX/*.
> > > >
> > > > Note that binding the dvb-net device to a network interface and
> > > > changing it
> > > > there does not reflect back into the the dvb_adapter struct and doesn't
> > > > change the MAC address here. This means that a system with two
> > > > identical
> > > > cards in it may need to distinguish them by some other means than MAC
> > > > address.
> > > >
> > > > Signed-off-by: David Howells
> > >
> > > Looks OK to me.
> > >
> > > Michael/Sean/Brad,
> > >
> > > Any comments? If not, I'll probably submit it this week upstream.
> >
> > With this patch, with a usb Hauppauge Nova-T Stick I get:
>
> Weird. Normally, Hauppauge devices have MAC address, as they all have
> eeproms. On several models, the MAC is even printed at the label on
> its back.
>
> Perhaps the logic didn't wait for the firmware to load?
This is an ancient dib0700 device; the firmware did load but there is no
mac.
> > $ tail /sys/class/dvb/*/dvb_*
> > ==> /sys/class/dvb/dvb0.demux0/dvb_adapter <==
> > 0
> >
> > ==> /sys/class/dvb/dvb0.demux0/dvb_mac <==
> > 00:00:00:00:00:00
> >
> > ==> /sys/class/dvb/dvb0.demux0/dvb_type <==
> > demux
> >
> > ==> /sys/class/dvb/dvb0.dvr0/dvb_adapter <==
> > 0
> >
> > ==> /sys/class/dvb/dvb0.dvr0/dvb_mac <==
> > 00:00:00:00:00:00
> >
> > ==> /sys/class/dvb/dvb0.dvr0/dvb_type <==
> > dvr
> >
> > ==> /sys/class/dvb/dvb0.frontend0/dvb_adapter <==
> > 0
> >
> > ==> /sys/class/dvb/dvb0.frontend0/dvb_mac <==
> > 00:00:00:00:00:00
> >
> > ==> /sys/class/dvb/dvb0.frontend0/dvb_type <==
> > frontend
> >
> > ==> /sys/class/dvb/dvb0.net0/dvb_adapter <==
> > 0
> >
> > ==> /sys/class/dvb/dvb0.net0/dvb_mac <==
> > 00:00:00:00:00:00
> >
> > ==> /sys/class/dvb/dvb0.net0/dvb_type <==
> > net
> >
> >
> > This would mean a stable name is based on a mac of 0, and there are many
> > more devices don't have a mac so they would all match this stable name.
>
> It can only provide information when the device has it.
>
> > Devices without a mac address shouldn't have a mac_dvb sysfs attribute,
> > I think.
>
> Hmm... do you mean that, if the mac is reported a
Re: [PATCH] dvb: Allow MAC addresses to be mapped to stable device names with udev
Sean Young wrote:
> > > Devices have a MAC address available, which is printed during boot:
>
> Not all dvb devices have a mac address.
How do I tell? If it's all zeros it's not there?
> Devices without a mac address shouldn't have a mac_dvb sysfs attribute,
> I think.
I'm not sure that's possible within the core infrastructure. It's a class
attribute set when the class is created; I'm not sure it can be overridden on
a per-device basis.
Possibly the file could return "" or "none" in this case?
> The dvb type and dvb adapter no is already present in the device name,
> I'm not sure why this needs duplicating.
They can be used with ATTR{} in udev rules. I'm not clear that the name can.
> With this patch, with a usb Hauppauge Nova-T Stick I get:
> ...
> ==> /sys/class/dvb/dvb0.demux0/dvb_mac <==
> 00:00:00:00:00:00
I can't say why that happens. I don't have access to this hardware. Should
it have a MAC address there? Is the MAC address getting stored in
dvbdev->adapter->proposed_mac? Maybe it's not getting read - on the card I
use it's read by the cx23885 driver... I think... The nova-t-usb2.c file
doesn't mention proposed_mac.
David
Re: [PATCH] dvb: Allow MAC addresses to be mapped to stable device names with udev
On Wed, Oct 31, 2018 at 10:36:22AM +, David Howells wrote:
> Sean Young wrote:
>
> > > > Devices have a MAC address available, which is printed during boot:
> >
> > Not all dvb devices have a mac address.
>
> How do I tell? If it's all zeros it's not there?
The mac gets populated through read_mac_address member of
dvb_usb_device_properties. If that's not called (or does not succeed), then
there is no mac address. I think you can safely assume that if it's all 0's
then it was not read.
> > Devices without a mac address shouldn't have a mac_dvb sysfs attribute,
> > I think.
>
> I'm not sure that's possible within the core infrastructure. It's a class
> attribute set when the class is created; I'm not sure it can be overridden on
> a per-device basis.
>
> Possibly the file could return "" or "none" in this case?
That's very ugly. Have a look at, for example, rc-core wakeup filters:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/rc/rc-main.c#n1844
> > The dvb type and dvb adapter no is already present in the device name,
> > I'm not sure why this needs duplicating.
>
> They can be used with ATTR{} in udev rules. I'm not clear that the name can.
See my other email.
KERNEL=="dvb[0-9]+\.demux\.[0-9]+"
> > With this patch, with a usb Hauppauge Nova-T Stick I get:
> > ...
> > ==> /sys/class/dvb/dvb0.demux0/dvb_mac <==
> > 00:00:00:00:00:00
>
> I can't say why that happens. I don't have access to this hardware. Should
> it have a MAC address there? Is the MAC address getting stored in
> dvbdev->adapter->proposed_mac? Maybe it's not getting read - on the card I
> use it's read by the cx23885 driver... I think... The nova-t-usb2.c file
> doesn't mention proposed_mac.
This is a dib0700-type device (much older).
Sean
Re: [PATCH] dvb: Allow MAC addresses to be mapped to stable device names with udev
Sean Young wrote:
> With this patch, with a usb Hauppauge Nova-T Stick I get:
>
> $ tail /sys/class/dvb/*/dvb_*
> ...
> ==> /sys/class/dvb/dvb0.demux0/dvb_mac <==
> 00:00:00:00:00:00
I presume you're complaining, then, that the file exists in this instance
rather than it doesn't have the real MAC address in it?
> Having said that dvb_type does look a little nicer:
>
> ATTR{dvb_type}=="demux"
So I should keep that or drop that?
David
Re: [PATCH] dvb: Allow MAC addresses to be mapped to stable device names with udev
Sean Young wrote: > > How do I tell? If it's all zeros it's not there? > > The mac gets populated through read_mac_address member of > dvb_usb_device_properties. That doesn't seem to be true for all drivers. The cx23885 driver does things differently, for instance. David
Re: [PATCH] dvb: Allow MAC addresses to be mapped to stable device names with udev
Sean Young wrote: > > > Devices without a mac address shouldn't have a mac_dvb sysfs attribute, > > > I think. > > > > I'm not sure that's possible within the core infrastructure. It's a class > > attribute set when the class is created; I'm not sure it can be overridden > > on > > a per-device basis. > > > > Possibly the file could return "" or "none" in this case? > > That's very ugly. Have a look at, for example, rc-core wakeup filters: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/rc/rc-main.c#n1844 By analogy, then, I think the thing to do is to put something like struct rc_dev::sysfs_groups[] into struct dvb_device (or maybe struct dvb_adapter) and then the dvb_mac attribute in there during dvb_register_device() based on whether or not the MAC address is not all zeros at that point. David
Re: [PATCH] dvb: Allow MAC addresses to be mapped to stable device names with udev
David Howells wrote:
> > > > Devices without a mac address shouldn't have a mac_dvb sysfs attribute,
> > > > I think.
> > >
> > > I'm not sure that's possible within the core infrastructure. It's a
> > > class attribute set when the class is created; I'm not sure it can be
> > > overridden on a per-device basis.
> > >
> > > Possibly the file could return "" or "none" in this case?
> >
> > That's very ugly. Have a look at, for example, rc-core wakeup filters:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/rc/rc-main.c#n1844
>
> By analogy, then, I think the thing to do is to put something like struct
> rc_dev::sysfs_groups[] into struct dvb_device (or maybe struct dvb_adapter)
> and then the dvb_mac attribute in there during dvb_register_device() based on
> whether or not the MAC address is not all zeros at that point.
Hmmm... This is trickier than it seems. At the point the device struct is
registered, the MAC address hasn't yet been read:
[ 13.865905] cx23885: CORE cx23885[1]: subsystem: 4254:0952, board: DVBSky
S952 [card=50,autodetected]
[ 14.095559] cx25840 8-0044: cx23885 A/V decoder found @ 0x88 (cx23885[1])
[ 14.723127] cx25840 8-0044: loaded v4l-cx23885-avcore-01.fw firmware (16382
bytes)
[ 14.738377] cx23885: cx23885_dvb_register() allocating 1 frontend(s)
[ 14.738378] cx23885: cx23885[1]: cx23885 based dvb card
[ 14.742536] i2c i2c-7: Added multiplexed i2c bus 9
[ 15.096912] ts2020 9-0060: Montage Technology TS2022 successfully identified
[ 15.096933] dvbdev: DVB: registering new adapter (cx23885[1])
[ 15.096936] cx23885 :06:00.0: DVB: registering adapter 2 frontend 0
(Montage Technology M88DS3103)...
[ 15.124665] cx23885: DVBSky S952 port 1 MAC address: 00:17:42:54:09:52
[ 15.124666] cx23885: cx23885_dvb_register() allocating 1 frontend(s)
[ 15.124674] cx23885: cx23885[1]: cx23885 based dvb card
[ 15.128860] i2c i2c-6: Added multiplexed i2c bus 10
[ 15.228172] ts2020 10-0060: Montage Technology TS2022 successfully identified
[ 15.228188] dvbdev: DVB: registering new adapter (cx23885[1])
[ 15.228190] cx23885 :06:00.0: DVB: registering adapter 3 frontend 0
(Montage Technology M88DS3103)...
[ 15.255996] cx23885: DVBSky S952 port 2 MAC address: 00:17:42:54:09:53
[ 15.255999] cx23885: cx23885_dev_checkrevision() Hardware revision = 0xa5
[ 15.256004] cx23885: cx23885[1]/0: found at :06:00.0, rev: 4, irq: 19,
latency: 0, mmio: 0xf7a0
The device structs are registered at 15.096936 and 15.228190 and this is the
point by which I think I have to set the device::groups pointer.
However, the device isn't fully initialised at this point and the MAC address
hasn't yet been read - and so the attribute doesn't appear. proposed_mac is
set right after lines 15.124665 and 15.255996. Interestingly, a third of a
second elapses between the device registration and the MAC being printed for
each adapter.
Any suggestions?
David
---
diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index b7171bf094fb..edbfa5549994 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -450,6 +450,23 @@ static int dvb_register_media_device(struct dvb_device
*dvbdev,
return 0;
}
+static ssize_t dvb_mac_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct dvb_device *dvbdev = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%pM\n", dvbdev->adapter->proposed_mac);
+}
+static DEVICE_ATTR_RO(dvb_mac);
+
+static struct attribute *dvb_device_attrs[] = {
+ &dev_attr_dvb_mac.attr,
+ NULL
+};
+static const struct attribute_group dvb_device_attr_grp = {
+ .attrs = dvb_device_attrs,
+};
+
int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
const struct dvb_device *template, void *priv,
enum dvb_device_type type, int demux_sink_pads)
@@ -533,6 +550,14 @@ int dvb_register_device(struct dvb_adapter *adap, struct
dvb_device **pdvbdev,
mutex_unlock(&dvbdev_register_lock);
+ if (adap->proposed_mac[0] || adap->proposed_mac[1] ||
+ adap->proposed_mac[2] || adap->proposed_mac[3] ||
+ adap->proposed_mac[4] || adap->proposed_mac[5]) {
+ dvbdev->sysfs_groups[0] = &dvb_device_attr_grp;
+ dvbdev->sysfs_groups[1] = NULL;
+ adap->device->groups = dvbdev->sysfs_groups;
+ }
+
clsdev = device_create(dvb_class, adap->device,
MKDEV(DVB_MAJOR, minor),
dvbdev, "dvb%d.%s%d", adap->num, dnames[type],
id);
@@ -1010,6 +1035,31 @@ void dvb_module_release(struct i2c_client *client)
EXPORT_SYMBOL_GPL(dvb_module_release);
#endif
+static ssize_t dvb_adapter_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct dvb
Re: [PATCH] dvb: Allow MAC addresses to be mapped to stable device names with udev
Em Wed, 31 Oct 2018 15:51:40 +
David Howells escreveu:
> David Howells wrote:
>
> > > > > Devices without a mac address shouldn't have a mac_dvb sysfs
> > > > > attribute,
> > > > > I think.
> > > >
> > > > I'm not sure that's possible within the core infrastructure. It's a
> > > > class attribute set when the class is created; I'm not sure it can be
> > > > overridden on a per-device basis.
> > > >
> > > > Possibly the file could return "" or "none" in this case?
> > >
> > > That's very ugly. Have a look at, for example, rc-core wakeup filters:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/rc/rc-main.c#n1844
> > >
> >
> > By analogy, then, I think the thing to do is to put something like struct
> > rc_dev::sysfs_groups[] into struct dvb_device (or maybe struct dvb_adapter)
> > and then the dvb_mac attribute in there during dvb_register_device() based
> > on
> > whether or not the MAC address is not all zeros at that point.
>
> Hmmm... This is trickier than it seems. At the point the device struct is
> registered, the MAC address hasn't yet been read:
>
> [ 13.865905] cx23885: CORE cx23885[1]: subsystem: 4254:0952, board: DVBSky
> S952 [card=50,autodetected]
> [ 14.095559] cx25840 8-0044: cx23885 A/V decoder found @ 0x88 (cx23885[1])
> [ 14.723127] cx25840 8-0044: loaded v4l-cx23885-avcore-01.fw firmware
> (16382 bytes)
> [ 14.738377] cx23885: cx23885_dvb_register() allocating 1 frontend(s)
> [ 14.738378] cx23885: cx23885[1]: cx23885 based dvb card
> [ 14.742536] i2c i2c-7: Added multiplexed i2c bus 9
> [ 15.096912] ts2020 9-0060: Montage Technology TS2022 successfully
> identified
> [ 15.096933] dvbdev: DVB: registering new adapter (cx23885[1])
> [ 15.096936] cx23885 :06:00.0: DVB: registering adapter 2 frontend 0
> (Montage Technology M88DS3103)...
> [ 15.124665] cx23885: DVBSky S952 port 1 MAC address: 00:17:42:54:09:52
> [ 15.124666] cx23885: cx23885_dvb_register() allocating 1 frontend(s)
> [ 15.124674] cx23885: cx23885[1]: cx23885 based dvb card
> [ 15.128860] i2c i2c-6: Added multiplexed i2c bus 10
> [ 15.228172] ts2020 10-0060: Montage Technology TS2022 successfully
> identified
> [ 15.228188] dvbdev: DVB: registering new adapter (cx23885[1])
> [ 15.228190] cx23885 :06:00.0: DVB: registering adapter 3 frontend 0
> (Montage Technology M88DS3103)...
> [ 15.255996] cx23885: DVBSky S952 port 2 MAC address: 00:17:42:54:09:53
> [ 15.255999] cx23885: cx23885_dev_checkrevision() Hardware revision = 0xa5
> [ 15.256004] cx23885: cx23885[1]/0: found at :06:00.0, rev: 4, irq: 19,
> latency: 0, mmio: 0xf7a0
>
> The device structs are registered at 15.096936 and 15.228190 and this is the
> point by which I think I have to set the device::groups pointer.
>
> However, the device isn't fully initialised at this point and the MAC address
> hasn't yet been read - and so the attribute doesn't appear. proposed_mac is
> set right after lines 15.124665 and 15.255996. Interestingly, a third of a
> second elapses between the device registration and the MAC being printed for
> each adapter.
>
> Any suggestions?
Yeah, I was afraid about that. At V4L2 core, what we do is that we first
do everything, including firmware load, and only after having everything
setup, we register the char device.
I suspect it should be possible to do that too at dvb-usb and dvb-usb-v2,
but some work would be needed.
>
> David
> ---
> diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> index b7171bf094fb..edbfa5549994 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -450,6 +450,23 @@ static int dvb_register_media_device(struct dvb_device
> *dvbdev,
> return 0;
> }
>
> +static ssize_t dvb_mac_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct dvb_device *dvbdev = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%pM\n", dvbdev->adapter->proposed_mac);
> +}
> +static DEVICE_ATTR_RO(dvb_mac);
> +
> +static struct attribute *dvb_device_attrs[] = {
> + &dev_attr_dvb_mac.attr,
> + NULL
> +};
> +static const struct attribute_group dvb_device_attr_grp = {
> + .attrs = dvb_device_attrs,
> +};
> +
> int dvb_register_device(struct dvb_adapter *adap, struct dvb_device
> **pdvbdev,
> const struct dvb_device *template, void *priv,
> enum dvb_device_type type, int demux_sink_pads)
> @@ -533,6 +550,14 @@ int dvb_register_device(struct dvb_adapter *adap, struct
> dvb_device **pdvbdev,
>
> mutex_unlock(&dvbdev_register_lock);
>
> + if (adap->proposed_mac[0] || adap->proposed_mac[1] ||
> + adap->proposed_mac[2] || adap->proposed_mac[3] ||
> + adap->proposed_mac[4] || adap->proposed_mac[5]) {
> + dvbdev->sysfs_groups[0] = &dvb_device_attr_grp;
> + dvbdev->sysf
Re: [PATCH] dvb: Allow MAC addresses to be mapped to stable device names with udev
On Wed, Oct 31, 2018 at 03:51:40PM +, David Howells wrote:
> David Howells wrote:
>
> > > > > Devices without a mac address shouldn't have a mac_dvb sysfs
> > > > > attribute,
> > > > > I think.
> > > >
> > > > I'm not sure that's possible within the core infrastructure. It's a
> > > > class attribute set when the class is created; I'm not sure it can be
> > > > overridden on a per-device basis.
> > > >
> > > > Possibly the file could return "" or "none" in this case?
> > >
> > > That's very ugly. Have a look at, for example, rc-core wakeup filters:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/rc/rc-main.c#n1844
> >
> > By analogy, then, I think the thing to do is to put something like struct
> > rc_dev::sysfs_groups[] into struct dvb_device (or maybe struct dvb_adapter)
> > and then the dvb_mac attribute in there during dvb_register_device() based
> > on
> > whether or not the MAC address is not all zeros at that point.
>
> Hmmm... This is trickier than it seems. At the point the device struct is
> registered, the MAC address hasn't yet been read:
>
> [ 13.865905] cx23885: CORE cx23885[1]: subsystem: 4254:0952, board: DVBSky
> S952 [card=50,autodetected]
> [ 14.095559] cx25840 8-0044: cx23885 A/V decoder found @ 0x88 (cx23885[1])
> [ 14.723127] cx25840 8-0044: loaded v4l-cx23885-avcore-01.fw firmware
> (16382 bytes)
> [ 14.738377] cx23885: cx23885_dvb_register() allocating 1 frontend(s)
> [ 14.738378] cx23885: cx23885[1]: cx23885 based dvb card
> [ 14.742536] i2c i2c-7: Added multiplexed i2c bus 9
> [ 15.096912] ts2020 9-0060: Montage Technology TS2022 successfully
> identified
> [ 15.096933] dvbdev: DVB: registering new adapter (cx23885[1])
> [ 15.096936] cx23885 :06:00.0: DVB: registering adapter 2 frontend 0
> (Montage Technology M88DS3103)...
> [ 15.124665] cx23885: DVBSky S952 port 1 MAC address: 00:17:42:54:09:52
> [ 15.124666] cx23885: cx23885_dvb_register() allocating 1 frontend(s)
> [ 15.124674] cx23885: cx23885[1]: cx23885 based dvb card
> [ 15.128860] i2c i2c-6: Added multiplexed i2c bus 10
> [ 15.228172] ts2020 10-0060: Montage Technology TS2022 successfully
> identified
> [ 15.228188] dvbdev: DVB: registering new adapter (cx23885[1])
> [ 15.228190] cx23885 :06:00.0: DVB: registering adapter 3 frontend 0
> (Montage Technology M88DS3103)...
> [ 15.255996] cx23885: DVBSky S952 port 2 MAC address: 00:17:42:54:09:53
> [ 15.255999] cx23885: cx23885_dev_checkrevision() Hardware revision = 0xa5
> [ 15.256004] cx23885: cx23885[1]/0: found at :06:00.0, rev: 4, irq: 19,
> latency: 0, mmio: 0xf7a0
>
> The device structs are registered at 15.096936 and 15.228190 and this is the
> point by which I think I have to set the device::groups pointer.
>
> However, the device isn't fully initialised at this point and the MAC address
> hasn't yet been read - and so the attribute doesn't appear. proposed_mac is
> set right after lines 15.124665 and 15.255996. Interestingly, a third of a
> second elapses between the device registration and the MAC being printed for
> each adapter.
device_create() will register the device in sysfs and send uevent. So, your
original udev rule/code will not work, since it always would read
a mac address of 0, as proposed_mac is not populated when the device is
announced. That is, unless udev is scheduled after the mac is read.
I think the device_add/device_create() which triggers the uevent should be
delayed until everything is available.
Sean
>
> Any suggestions?
>
> David
> ---
> diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> index b7171bf094fb..edbfa5549994 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -450,6 +450,23 @@ static int dvb_register_media_device(struct dvb_device
> *dvbdev,
> return 0;
> }
>
> +static ssize_t dvb_mac_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct dvb_device *dvbdev = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%pM\n", dvbdev->adapter->proposed_mac);
> +}
> +static DEVICE_ATTR_RO(dvb_mac);
> +
> +static struct attribute *dvb_device_attrs[] = {
> + &dev_attr_dvb_mac.attr,
> + NULL
> +};
> +static const struct attribute_group dvb_device_attr_grp = {
> + .attrs = dvb_device_attrs,
> +};
> +
> int dvb_register_device(struct dvb_adapter *adap, struct dvb_device
> **pdvbdev,
> const struct dvb_device *template, void *priv,
> enum dvb_device_type type, int demux_sink_pads)
> @@ -533,6 +550,14 @@ int dvb_register_device(struct dvb_adapter *adap, struct
> dvb_device **pdvbdev,
>
> mutex_unlock(&dvbdev_register_lock);
>
> + if (adap->proposed_mac[0] || adap->proposed_mac[1] ||
> + adap->proposed_mac[2] || adap->proposed_mac[3] ||
> + adap->proposed_mac[4] || adap->proposed_
Re: [PATCH] dvb: Allow MAC addresses to be mapped to stable device names with udev
Em Wed, 31 Oct 2018 16:13:00 +
Sean Young escreveu:
> On Wed, Oct 31, 2018 at 03:51:40PM +, David Howells wrote:
> > David Howells wrote:
> >
> > > > > > Devices without a mac address shouldn't have a mac_dvb sysfs
> > > > > > attribute,
> > > > > > I think.
> > > > >
> > > > > I'm not sure that's possible within the core infrastructure. It's a
> > > > > class attribute set when the class is created; I'm not sure it can be
> > > > > overridden on a per-device basis.
> > > > >
> > > > > Possibly the file could return "" or "none" in this case?
> > > >
> > > > That's very ugly. Have a look at, for example, rc-core wakeup filters:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/rc/rc-main.c#n1844
> > > >
> > >
> > > By analogy, then, I think the thing to do is to put something like struct
> > > rc_dev::sysfs_groups[] into struct dvb_device (or maybe struct
> > > dvb_adapter)
> > > and then the dvb_mac attribute in there during dvb_register_device()
> > > based on
> > > whether or not the MAC address is not all zeros at that point.
> >
> > Hmmm... This is trickier than it seems. At the point the device struct is
> > registered, the MAC address hasn't yet been read:
> >
> > [ 13.865905] cx23885: CORE cx23885[1]: subsystem: 4254:0952, board:
> > DVBSky S952 [card=50,autodetected]
> > [ 14.095559] cx25840 8-0044: cx23885 A/V decoder found @ 0x88 (cx23885[1])
> > [ 14.723127] cx25840 8-0044: loaded v4l-cx23885-avcore-01.fw firmware
> > (16382 bytes)
> > [ 14.738377] cx23885: cx23885_dvb_register() allocating 1 frontend(s)
> > [ 14.738378] cx23885: cx23885[1]: cx23885 based dvb card
> > [ 14.742536] i2c i2c-7: Added multiplexed i2c bus 9
> > [ 15.096912] ts2020 9-0060: Montage Technology TS2022 successfully
> > identified
> > [ 15.096933] dvbdev: DVB: registering new adapter (cx23885[1])
> > [ 15.096936] cx23885 :06:00.0: DVB: registering adapter 2 frontend 0
> > (Montage Technology M88DS3103)...
> > [ 15.124665] cx23885: DVBSky S952 port 1 MAC address: 00:17:42:54:09:52
> > [ 15.124666] cx23885: cx23885_dvb_register() allocating 1 frontend(s)
> > [ 15.124674] cx23885: cx23885[1]: cx23885 based dvb card
> > [ 15.128860] i2c i2c-6: Added multiplexed i2c bus 10
> > [ 15.228172] ts2020 10-0060: Montage Technology TS2022 successfully
> > identified
> > [ 15.228188] dvbdev: DVB: registering new adapter (cx23885[1])
> > [ 15.228190] cx23885 :06:00.0: DVB: registering adapter 3 frontend 0
> > (Montage Technology M88DS3103)...
> > [ 15.255996] cx23885: DVBSky S952 port 2 MAC address: 00:17:42:54:09:53
> > [ 15.255999] cx23885: cx23885_dev_checkrevision() Hardware revision = 0xa5
> > [ 15.256004] cx23885: cx23885[1]/0: found at :06:00.0, rev: 4, irq:
> > 19, latency: 0, mmio: 0xf7a0
> >
> > The device structs are registered at 15.096936 and 15.228190 and this is the
> > point by which I think I have to set the device::groups pointer.
> >
> > However, the device isn't fully initialised at this point and the MAC
> > address
> > hasn't yet been read - and so the attribute doesn't appear. proposed_mac is
> > set right after lines 15.124665 and 15.255996. Interestingly, a third of a
> > second elapses between the device registration and the MAC being printed for
> > each adapter.
>
> device_create() will register the device in sysfs and send uevent. So, your
> original udev rule/code will not work, since it always would read
> a mac address of 0, as proposed_mac is not populated when the device is
> announced. That is, unless udev is scheduled after the mac is read.
>
> I think the device_add/device_create() which triggers the uevent should be
> delayed until everything is available.
Yes. For udev rules to work, the very last thing to do is to create
devices.
In practice, dvb-usb/dvb-usb-v2 should delay calling
dvb_register_device() until firmware is in warm state.
>
> Sean
>
> >
> > Any suggestions?
> >
> > David
> > ---
> > diff --git a/drivers/media/dvb-core/dvbdev.c
> > b/drivers/media/dvb-core/dvbdev.c
> > index b7171bf094fb..edbfa5549994 100644
> > --- a/drivers/media/dvb-core/dvbdev.c
> > +++ b/drivers/media/dvb-core/dvbdev.c
> > @@ -450,6 +450,23 @@ static int dvb_register_media_device(struct dvb_device
> > *dvbdev,
> > return 0;
> > }
> >
> > +static ssize_t dvb_mac_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct dvb_device *dvbdev = dev_get_drvdata(dev);
> > +
> > + return sprintf(buf, "%pM\n", dvbdev->adapter->proposed_mac);
> > +}
> > +static DEVICE_ATTR_RO(dvb_mac);
> > +
> > +static struct attribute *dvb_device_attrs[] = {
> > + &dev_attr_dvb_mac.attr,
> > + NULL
> > +};
> > +static const struct attribute_group dvb_device_attr_grp = {
> > + .attrs = dvb_device_attrs,
> > +};
> > +
> > int dvb_register_device(struct dvb_adapter *adap, struct dvb_device
> > **pdvbd
Re: [GIT PULL for v4.20-rc1] new experimental media request API
On Tue, Oct 30, 2018 at 6:53 AM Mauro Carvalho Chehab wrote: > > For a new media API: the request API Ugh. I don't know how much being in staging matters - if people start using it, they start using it. "Staging" does not mean "regressions don't matter". But pulled, Linus
Re: [GIT PULL for v4.20-rc1] new experimental media request API
Hi Linus, Em Wed, 31 Oct 2018 11:05:09 -0700 Linus Torvalds escreveu: > On Tue, Oct 30, 2018 at 6:53 AM Mauro Carvalho Chehab > wrote: > > > > For a new media API: the request API > > Ugh. I don't know how much being in staging matters - if people start > using it, they start using it. > > "Staging" does not mean "regressions don't matter". Yes, I know. This shouldn't affect normal cameras and generic V4L2 apps, as this is for very advanced use cases. So, we hope that people won't start using it for a while. The main interested party on this is Google CromeOS. We're working together with them in order to do upstream first. They are well aware that the API may change. So, I don't expect any complaints from their side if the API would require further changes. The point is that this API is complex and ensuring that it will work properly is not easy. We've been thinking about a solution for the Camera HAL 2 for a long time (I guess the first discussions were done back in 2008). The big problem is that V4L2 API was designed to work with a stream, while Google HAL API expects to have control for each individual frame. The Google API allows, for example that, inside a stream, the first frame would have a VGA resolution, the next one a 4K resolution (for example, when the user clicks on a camera button) and then returning back to VGA (it actually allows full control for every single frame). This is something that it is not possible to do with the "standard" V4L2 API without stopping and restarting a stream (with increases a lot the latency). Solving it is so complex that we decided to start with a completely new type of Linux media drivers (stateless decoders). In long term, the same API should be used by not only by decoders, but also for encoders and complex cameras (those with an image signal processor inside a SoC chipset). In order to be sure that it is possible to implement the way we did, We need to be able to add it to the Kernel somehow and to have enough drivers that will let us test all possible scenarios. That will allow to adapt a version of the camera HAL for testing and see if it behaves as expected. > But pulled, Thanks! Anyway, we'll try to rush the tests for this API in order to try sending any fixes that might be disruptive before the final release. Regards, Mauro
Re: [PATCH] dvb: Allow MAC addresses to be mapped to stable device names with udev
Sean Young wrote: > device_create() will register the device in sysfs and send uevent. So, your > original udev rule/code will not work, since it always would read > a mac address of 0, as proposed_mac is not populated when the device is > announced. That is, unless udev is scheduled after the mac is read. I guess that must be what is happening as it does seem to work for me. > I think the device_add/device_create() which triggers the uevent should be > delayed until everything is available. Is it possible to switch vb2_dvb_register_bus() and dvb_register_ci_mac() in dvb_register() in cx23885-dvb.c - or does that prevent the firmware from loading? And I'm guessing this change would have to be applied to all drivers? David
Re: [PATCH 2/7] dt-bindings: mfd: ds90ux9xx: add description of TI DS90Ux9xx I2C bridge
Hi Luca,
thank you for review.
On 10/30/2018 06:43 PM, Luca Ceresoli wrote:
> Hi Vladimir,
>
> On 08/10/18 23:12, Vladimir Zapolskiy wrote:
>> From: Vladimir Zapolskiy
>>
>> TI DS90Ux9xx de-/serializers are capable to route I2C messages to
>> I2C slave devices connected to a remote de-/serializer in a pair,
>> the change adds description of device tree bindings of the subcontroller
>> to configure and enable this functionality.
>>
>> Signed-off-by: Vladimir Zapolskiy
>> ---
>> .../bindings/mfd/ti,ds90ux9xx-i2c-bridge.txt | 61 +++
>> 1 file changed, 61 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/mfd/ti,ds90ux9xx-i2c-bridge.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/mfd/ti,ds90ux9xx-i2c-bridge.txt
>> b/Documentation/devicetree/bindings/mfd/ti,ds90ux9xx-i2c-bridge.txt
>> new file mode 100644
>> index ..4169e382073a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/ti,ds90ux9xx-i2c-bridge.txt
>> @@ -0,0 +1,61 @@
>> +TI DS90Ux9xx de-/serializer I2C bridge subcontroller
>> +
>> +Required properties:
>> +- compatible: Must contain a generic "ti,ds90ux9xx-i2c-bridge" value and
>> +may contain one more specific value from the list:
>> +"ti,ds90ux925-i2c-bridge",
>> +"ti,ds90ux926-i2c-bridge",
>> +"ti,ds90ux927-i2c-bridge",
>> +"ti,ds90ux928-i2c-bridge",
>> +"ti,ds90ux940-i2c-bridge".
>> +
>> +Required properties of a de-/serializer device connected to a local I2C bus:
>> +- ti,i2c-bridges: List of phandles to remote de-/serializer devices with
>> +two arguments: id of a local de-/serializer FPD link and an assigned
>> +I2C address of a remote de-/serializer to be accessed on a local
>> +I2C bus.
>> +
>> +Optional properties of a de-/serializer device connected to a local I2C bus:
>> +- ti,i2c-bridge-maps: List of 3-cell values:
>> +- the first argument is id of a local de-/serializer FPD link,
>> +- the second argument is an I2C address of a device connected to
>> + a remote de-/serializer IC,
>> +- the third argument is an I2C address of the remote I2C device
>> + for access on a local I2C bus.
>
> BTW I usually use names "remove slave" address and "alias" for bullets 2
> and 3. These are the names from the datasheets, and are clearer IMO.
>
Definitely you are correct, I find that verbose descriptions might be
more appropriate and self-explanatory for anyone, who is not closely familiar
with the IC series. I'll consider to add the names from the datasheets
as well.
> Now to the big stuff.
>
> I find a static map in the "local" chip DT node is a limit. You might
> have to support multiple models of remote device, where you'll know the
> model only when after it gets connected. Think Beaglebone capes, but
> over FPD-Link 3. This scenario opens several issues, but specifically
> for I2C address mapping I addressed it by adding in the "local" chip's
> DT node a pool of I2C aliases it can use. The DT author is responsible
> to pick addresses that are not used on the same I2C bus, which cannot be
> done at runtime reliably.
Here I see several important topics raised.
1) A static map in the "local" chip DT node is not a limit in sense that
it is optional, so it would be a working model just to omit the property,
however it may (or may not) require another handlers to bridge remote
I2C devices, for instance 'ti,i2c-bridge-pass-all' property, or new
UAPI.
2) About supporting multiple models of remote PCBs in the same dts file,
it might be an excessive complication to predict a proper description
of an unknown in advance complex device, so, a better solution should
be to apply DT overlays in runtime, but at any time the hardware
description and the mapping shall be precisely defined.
3) About a pool of vacant I2C addresses, I dislike the idea that there
will be no definite or constant I2C address in runtime for a particular
remote slave device. As I've mentioned above, it would be better to
utilize DT overlays to handle "multiple models of remote device"
dynamically in runtime, adding this feature could be done on top of
the shown code.
> Here's my current draft on a dual/quad port deserializer:
>
> &i2c0 {
> serializer@3d {
> reg = <0x3d>;
> ...
>
> /* Guaranteed not physically present on i2c0 */
> i2c-alias-pool = /bits/ 8 <0x20 0x21 0x22 0x23 0x24 0x25>;
>
> rxports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> rxport@0 {
> reg = <0>;
> remote-i2c-bus { /* The proxied I2C bus on rxport 0 */
> #address-cells = <1>;
> #size-cells = <0>;
>
> eeprom@51 {
> reg = <0x51>;
> compatible = "at,24c02";
> };
> };
>
> rxport@1 {
> reg = <1>
Re: [PATCH 3/7] dt-bindings: pinctrl: ds90ux9xx: add description of TI DS90Ux9xx pinmux
Hi Luca,
On 10/30/2018 06:44 PM, Luca Ceresoli wrote:
> Hi Vladimir,
>
> On 16/10/18 14:48, Laurent Pinchart wrote:
>> Hi Vladimir,
>>
>> On Saturday, 13 October 2018 16:47:48 EEST Vladimir Zapolskiy wrote:
>>> On 10/12/2018 03:01 PM, Laurent Pinchart wrote:
On Tuesday, 9 October 2018 00:12:01 EEST Vladimir Zapolskiy wrote:
> From: Vladimir Zapolskiy
>
> TI DS90Ux9xx de-/serializers have a capability to multiplex pin
> functions, in particular a pin may have selectable functions of GPIO,
> GPIO line transmitter, one of I2S lines, one of RGB24 video signal lines
> and so on.
>
> The change adds a description of DS90Ux9xx pin multiplexers and GPIO
> controllers.
>
> Signed-off-by: Vladimir Zapolskiy
> [...]
> +Available pins, groups and functions (reference to device datasheets):
> +
> +function: "gpio" ("gpio4" is on DS90Ux925 and DS90Ux926 only,
> + "gpio9" is on DS90Ux940 only)
> + - pins: "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6",
> + "gpio7", "gpio8", "gpio9"
> +
> +function: "gpio-remote"
> + - pins: "gpio0", "gpio1", "gpio2", "gpio3"
> +
> +function: "pass" (DS90Ux940 specific only)
> + - pins: "gpio0", "gpio3"
What do those functions mean ?
>>>
>>> "gpio" function should be already familiar to you.
>>
>> I assume this function is only available for the local device, not the
>> remote
>> one ?
>>
>>> "gpio-remote" function is the pin function for a GPIO line bridging.
>>>
>>> "pass" function sets a pin to a status pin function for detecting
>>> display timing issues, namely DE or Vsync length value mismatch.
>>
>> All this is not clear at all from the proposed DT bindings, it should be
>> properly documented.
>
> It's not clear to me as well. The "gpio-remote" can mean two different
> things (at least in the camera serdes TI chips):
>
> - a GPIO input on the the *local* chip, replicated as an output on the
>*remote* chip
> - a GPIO input on the the *remote* chip, replicated as an output on the
>*local* chip
>
> How to you differentiate them in DT?
>
"gpio-remote" function is directly translated into "GPIOx Remote Enable"
bit setting, the documentation says:
1) Deserializer IC pin configuration:
Enable GPIO control from remote Serializer. The GPIO pin will
be an output, and the value is received from the remote Serializer.
2) Serializer IC pin configuration:
Enable GPIO control from remote Deserializer. The GPIO pin will
be an output, and the value is received from the remote Deserializer.
So, it is always an output signal, the line signal is "bridged" (repeated)
as a corresponding line signal on a remote IC, note that there is no
difference between serializer and deserializer ICs.
> The "pass" function is also not clear. A comprehensive example would
> help a lot.
As this devicetree documentation says, the "pass" pin function is specific
for DS90Ux940 deserializer, I would suggest to check its datasheet for
getting a comprehensive answer, but I've already copy-pasted information
from the datasheet into my previous answer to Laurent.
The reason why the "pass" pin function is listed is quite simple, the
pin function interferes other pin functions, see DS90Ux940 GPIO0 and
GPIO3 controls, I hope I've managed to describe it properly by
DS90UX940_GPIO(0, ...) and DS90UX940_GPIO(3, ...) pin descriptions in
the pinctrl driver.
--
Best wishes,
Vladimir
Re: [GIT PULL for v4.20-rc1] new experimental media request API
On Wed, Oct 31, 2018 at 11:05 AM Linus Torvalds
wrote:
>
> But pulled,
I have no idea how I missed this during the actual test compile after
the pull (and yes, I'm sure I did one), but after doing a couple of
more pulls I finally did notice.
After the media pull I get this warning:
./usr/include/linux/v4l2-controls.h:1105: found __[us]{8,16,32,64}
type without #include
and sure enough, the recent changes to
include/uapi/linux/v4l2-controls.h
add those new structures use the "__uXY" types without including the
header to define them.
It's harmless in the short term and the kernel build itself obviously
doesn't care apart from the warning, but please fix it.
Linus
Re: [GIT PULL for v4.20-rc1] new experimental media request API
Em Wed, 31 Oct 2018 15:32:03 -0700
Linus Torvalds escreveu:
> On Wed, Oct 31, 2018 at 11:05 AM Linus Torvalds
> wrote:
> >
> > But pulled,
>
> I have no idea how I missed this during the actual test compile after
> the pull (and yes, I'm sure I did one), but after doing a couple of
> more pulls I finally did notice.
>
> After the media pull I get this warning:
>
> ./usr/include/linux/v4l2-controls.h:1105: found __[us]{8,16,32,64}
> type without #include
>
> and sure enough, the recent changes to
>
> include/uapi/linux/v4l2-controls.h
>
> add those new structures use the "__uXY" types without including the
> header to define them.
>
> It's harmless in the short term and the kernel build itself obviously
> doesn't care apart from the warning, but please fix it.
I also missed this one. Perhaps it depends on gcc version, or is it
a new warning after some changes? I remember there was some patchsets
floating around related to change some warnings.
Anyway, I'll send you a fix for it soon.
Thanks,
Mauro
