Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On 02/02/2017 02:35 PM, Russell King - ARM Linux wrote: However, "*vfd" contains a struct device, and you _correctly_ set the release function for "*vfd" to video_device_release via camif_videodev. However, if you try to rmmod imx-media, then you end up with a kernel warning that you're freeing memory containing a held lock, and later chaos ensues because kmalloc has been corrupted. The root cause of this is embedding the device structure within the video_device into the driver's private data. *Any* structure what so ever that contains a kref is reference counted, and that includes struct device, and therefore also includes struct video_device. What that means is that its lifetime is _not_ under _your_ control, and you may not free it except through its release function (which is video_device_release().) However, that also tries to kfree (with an offset of 4) your private data, which results in the warning and the corrupted kmalloc free lists. The solution is simple, make "vfd" a pointer in your private data structure and kmalloc() it separately, letting video_device_release() kfree() that data when it needs to. Thanks Russell for tracking this down. I remember doing this when I was reviewing the code for opportunities to "optimize" :-/, and carelessly caused this bug by not reviewing how video_device is freed. Fixed. Steve -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On Fri, Jan 06, 2017 at 06:11:38PM -0800, Steve Longerbeam wrote: > +struct camif_priv { > + struct device *dev; > + struct video_devicevfd; You can't do this. > +static struct video_device camif_videodev = { > + .fops = &camif_fops, > + .ioctl_ops = &camif_ioctl_ops, > + .minor = -1, > + .release= video_device_release, > + .vfl_dir= VFL_DIR_RX, > + .tvnorms= V4L2_STD_NTSC | V4L2_STD_PAL | V4L2_STD_SECAM, > +}; > +static int camif_probe(struct platform_device *pdev) > +{ > + struct imx_media_internal_sd_platformdata *pdata; > + struct camif_priv *priv; > + struct video_device *vfd; > + int ret; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; You kmalloc this structure, so this structure has the lifetime of the driver being bound to the platform device. > + vfd = &priv->vfd; > + *vfd = camif_videodev; However, "*vfd" contains a struct device, and you _correctly_ set the release function for "*vfd" to video_device_release via camif_videodev. However, if you try to rmmod imx-media, then you end up with a kernel warning that you're freeing memory containing a held lock, and later chaos ensues because kmalloc has been corrupted. The root cause of this is embedding the device structure within the video_device into the driver's private data. *Any* structure what so ever that contains a kref is reference counted, and that includes struct device, and therefore also includes struct video_device. What that means is that its lifetime is _not_ under _your_ control, and you may not free it except through its release function (which is video_device_release().) However, that also tries to kfree (with an offset of 4) your private data, which results in the warning and the corrupted kmalloc free lists. The solution is simple, make "vfd" a pointer in your private data structure and kmalloc() it separately, letting video_device_release() kfree() that data when it needs to. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On Tue, Jan 31, 2017 at 02:36:53PM -0800, Steve Longerbeam wrote: > On 01/31/2017 02:04 PM, Russell King - ARM Linux wrote: > >I don't want master though, I want v4.10-rc1, and if I ask for that > >it tells me it knows nothing about v4.10-rc1, despite the fact that's > >a tag in the mainline kernel repository which was merged into the > >linux-media tree that this tree is based upon. > > Hi Russell, yes g...@github.com:slongerbeam/mediatree.git is a fork > of the linux-media tree, and the imx-media-staging-md-wip branch > is up-to-date with master, currently at 4.10-rc1. "up to date" is different from "contains other stuff other than is in 4.10-rc1". What I see in your tree is that your code is based off a merge commit between something called "patchwork" (which I assume is a branch in the media tree containing stuff commited from patch work) and v4.10-rc1. Now, you don't get a commit when merging unless there's changes that aren't in the commit you're merging - if "patchwork" was up to date with v4.10-rc1, then git would have done a "fast forward" to v4.10-rc1. Therefore, while it may be "up to date" with v4.10-rc1 in so far that it's had v4.10-rc1 merged into it, that's not what I've been saying. There are other changes below that merge commit which aren't in v4.10-rc1. It's those other changes that I'm talking about, and it's those other changes I do not want without knowing what they are. It may be that those other changes have since been merged into v4.10-rc6 - but github's web interface can't show me that. In fact, github's web interface is pretty damned useless as far as this stuff goes. So, what I'll get if I clone or pull your imx-media-staging-md-wip branch is, yes, a copy of all your changes, but _also_ all the changes that are in the media tree that _aren't_ in mainline at the point that v4.10-rc1 was merged. > You don't need to use the web interface, just git clone the repo. You're assuming I want to work off the top of your commits. I don't. I've got other dependencies. Then there's yet another problem - lets say that I get a copy of your patches that haven't been on the mailing list, and I then want to make a comment about it. I can't reply to a patch that hasn't been on the mailing list. So, the long established mechanism by which the Linux community does patch review breaks down. So no, sorry, I'm not fetching your tree, and I will persist with your v3 patch set for the time being. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On 01/31/2017 03:30 PM, Russell King - ARM Linux wrote: On Tue, Jan 31, 2017 at 02:36:53PM -0800, Steve Longerbeam wrote: On 01/31/2017 02:04 PM, Russell King - ARM Linux wrote: I don't want master though, I want v4.10-rc1, and if I ask for that it tells me it knows nothing about v4.10-rc1, despite the fact that's a tag in the mainline kernel repository which was merged into the linux-media tree that this tree is based upon. Hi Russell, yes g...@github.com:slongerbeam/mediatree.git is a fork of the linux-media tree, and the imx-media-staging-md-wip branch is up-to-date with master, currently at 4.10-rc1. "up to date" is different from "contains other stuff other than is in 4.10-rc1". What I see in your tree is that your code is based off a merge commit between something called "patchwork" (which I assume is a branch in the media tree containing stuff commited from patch work) and v4.10-rc1. Now, you don't get a commit when merging unless there's changes that aren't in the commit you're merging - if "patchwork" was up to date with v4.10-rc1, then git would have done a "fast forward" to v4.10-rc1. Therefore, while it may be "up to date" with v4.10-rc1 in so far that it's had v4.10-rc1 merged into it, that's not what I've been saying. There are other changes below that merge commit which aren't in v4.10-rc1. It's those other changes that I'm talking about, and it's those other changes I do not want without knowing what they are. It may be that those other changes have since been merged into v4.10-rc6 - but github's web interface can't show me that. In fact, github's web interface is pretty damned useless as far as this stuff goes. So, what I'll get if I clone or pull your imx-media-staging-md-wip branch is, yes, a copy of all your changes, but _also_ all the changes that are in the media tree that _aren't_ in mainline at the point that v4.10-rc1 was merged. You don't need to use the web interface, just git clone the repo. You're assuming I want to work off the top of your commits. I don't. I've got other dependencies. Well, I was suggesting cloning it just to have a look at the new code, but I understand you don't want to attempt to bring in your SMIA/bayer format changes and run from this branch, due to the other changes in the mediatree. I suppose I should post the next version then. Trouble is, I see issues in the current driver that prevents working with your SMIA pipeline. But I guess that will have to be worked out in another version. Steve Then there's yet another problem - lets say that I get a copy of your patches that haven't been on the mailing list, and I then want to make a comment about it. I can't reply to a patch that hasn't been on the mailing list. So, the long established mechanism by which the Linux community does patch review breaks down. So no, sorry, I'm not fetching your tree, and I will persist with your v3 patch set for the time being. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On 01/31/2017 02:04 PM, Russell King - ARM Linux wrote: On Tue, Jan 31, 2017 at 09:55:29PM +, Ian Arkver wrote: On 31/01/17 20:33, Russell King - ARM Linux wrote: On Tue, Jan 31, 2017 at 10:21:26AM -0800, Steve Longerbeam wrote: On 01/31/2017 05:42 AM, Russell King - ARM Linux wrote: On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote: Should be set to something like 'platform:imx-media-camif'. v4l2-compliance should complain about this. ... and more. Right, in version 3 that you are working with, no v4l2-compliance fixes were in yet. A lot of the compliance errors are fixed, please look in latest branch imx-media-staging-md-wip at g...@github.com:slongerbeam/mediatree.git. Sorry, I'm not prepared to pull random trees from github as there's no easy way to see what's in the branch. I've always disliked github because its web interface makes it soo difficult to navigate around git trees hosted there. You can see a commit, you can see a diff of the commit. You can get a list of branches. But there seems to be no way to get a list of commits similar to "git log" or even a one-line summary of each commit on a branch. If there is, it's completely non-obvious (which I think is much of the problem with github, it's web interface is horrendous.) Or you can clone/pull the tree without knowing what you're fetching (eg, what the tree is based upon.) Or you can waste time clicking repeatedly on the "parent" commit link on each patch working your way back through the history... Well, it looks like it's bsaed on 4.10-rc1 with who-knows-what work >from the linux-media tree (I didn't try and go back any further.) As I don't want to take a whole pile of other changes into my tree, I'm certainly not going to pull from your github tree. Sorry. https://github.com/slongerbeam/mediatree/compare/master...imx-media-staging-md-wip It's under the "Compare" button from the main view. It would be nice though if the first commit's parent was some clearly tagged start point. I don't want master though, I want v4.10-rc1, and if I ask for that it tells me it knows nothing about v4.10-rc1, despite the fact that's a tag in the mainline kernel repository which was merged into the linux-media tree that this tree is based upon. Hi Russell, yes g...@github.com:slongerbeam/mediatree.git is a fork of the linux-media tree, and the imx-media-staging-md-wip branch is up-to-date with master, currently at 4.10-rc1. You don't need to use the web interface, just git clone the repo. Steve -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On 31/01/17 22:04, Russell King - ARM Linux wrote: On Tue, Jan 31, 2017 at 09:55:29PM +, Ian Arkver wrote: On 31/01/17 20:33, Russell King - ARM Linux wrote: On Tue, Jan 31, 2017 at 10:21:26AM -0800, Steve Longerbeam wrote: On 01/31/2017 05:42 AM, Russell King - ARM Linux wrote: On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote: Should be set to something like 'platform:imx-media-camif'. v4l2-compliance should complain about this. ... and more. Right, in version 3 that you are working with, no v4l2-compliance fixes were in yet. A lot of the compliance errors are fixed, please look in latest branch imx-media-staging-md-wip at g...@github.com:slongerbeam/mediatree.git. Sorry, I'm not prepared to pull random trees from github as there's no easy way to see what's in the branch. I've always disliked github because its web interface makes it soo difficult to navigate around git trees hosted there. You can see a commit, you can see a diff of the commit. You can get a list of branches. But there seems to be no way to get a list of commits similar to "git log" or even a one-line summary of each commit on a branch. If there is, it's completely non-obvious (which I think is much of the problem with github, it's web interface is horrendous.) Or you can clone/pull the tree without knowing what you're fetching (eg, what the tree is based upon.) Or you can waste time clicking repeatedly on the "parent" commit link on each patch working your way back through the history... Well, it looks like it's bsaed on 4.10-rc1 with who-knows-what work >from the linux-media tree (I didn't try and go back any further.) As I don't want to take a whole pile of other changes into my tree, I'm certainly not going to pull from your github tree. Sorry. https://github.com/slongerbeam/mediatree/compare/master...imx-media-staging-md-wip It's under the "Compare" button from the main view. It would be nice though if the first commit's parent was some clearly tagged start point. I don't want master though, I want v4.10-rc1, and if I ask for that it tells me it knows nothing about v4.10-rc1, despite the fact that's a tag in the mainline kernel repository which was merged into the linux-media tree that this tree is based upon. Yeah, that's what I meant about the first parent's commit not being a clearly tagged branch point. At least you get the series on one page. Maybe it's time for a rebase or a v4 series Steve? Personally, I use a bare repo with multiple remotes and fetch branches from various trees. Then gitk --all --since(etc) is pretty good at giving the overview picture. You don't need to pull the commits over into any of your working branches if you don't want to. Regards, Ian -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On Tue, Jan 31, 2017 at 09:55:29PM +, Ian Arkver wrote: > On 31/01/17 20:33, Russell King - ARM Linux wrote: > >On Tue, Jan 31, 2017 at 10:21:26AM -0800, Steve Longerbeam wrote: > >>On 01/31/2017 05:42 AM, Russell King - ARM Linux wrote: > >>>On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote: > Should be set to something like 'platform:imx-media-camif'. > v4l2-compliance > should complain about this. > >>>... and more. > >> > >>Right, in version 3 that you are working with, no v4l2-compliance fixes were > >>in yet. A lot of the compliance errors are fixed, please look in latest > >>branch > >>imx-media-staging-md-wip at g...@github.com:slongerbeam/mediatree.git. > > > >Sorry, I'm not prepared to pull random trees from github as there's > >no easy way to see what's in the branch. > > > >I've always disliked github because its web interface makes it soo > >difficult to navigate around git trees hosted there. You can see > >a commit, you can see a diff of the commit. You can get a list of > >branches. But there seems to be no way to get a list of commits > >similar to "git log" or even a one-line summary of each commit on > >a branch. If there is, it's completely non-obvious (which I think is > >much of the problem with github, it's web interface is horrendous.) > > > >Or you can clone/pull the tree without knowing what you're fetching > >(eg, what the tree is based upon.) > > > >Or you can waste time clicking repeatedly on the "parent" commit link > >on each patch working your way back through the history... > > > >Well, it looks like it's bsaed on 4.10-rc1 with who-knows-what work > >from the linux-media tree (I didn't try and go back any further.) > >As I don't want to take a whole pile of other changes into my tree, > >I'm certainly not going to pull from your github tree. Sorry. > > > > https://github.com/slongerbeam/mediatree/compare/master...imx-media-staging-md-wip > > It's under the "Compare" button from the main view. It would be nice though > if the first commit's parent was some clearly tagged start point. I don't want master though, I want v4.10-rc1, and if I ask for that it tells me it knows nothing about v4.10-rc1, despite the fact that's a tag in the mainline kernel repository which was merged into the linux-media tree that this tree is based upon. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On 31/01/17 20:33, Russell King - ARM Linux wrote: On Tue, Jan 31, 2017 at 10:21:26AM -0800, Steve Longerbeam wrote: On 01/31/2017 05:42 AM, Russell King - ARM Linux wrote: On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote: Should be set to something like 'platform:imx-media-camif'. v4l2-compliance should complain about this. ... and more. Right, in version 3 that you are working with, no v4l2-compliance fixes were in yet. A lot of the compliance errors are fixed, please look in latest branch imx-media-staging-md-wip at g...@github.com:slongerbeam/mediatree.git. Sorry, I'm not prepared to pull random trees from github as there's no easy way to see what's in the branch. I've always disliked github because its web interface makes it soo difficult to navigate around git trees hosted there. You can see a commit, you can see a diff of the commit. You can get a list of branches. But there seems to be no way to get a list of commits similar to "git log" or even a one-line summary of each commit on a branch. If there is, it's completely non-obvious (which I think is much of the problem with github, it's web interface is horrendous.) Or you can clone/pull the tree without knowing what you're fetching (eg, what the tree is based upon.) Or you can waste time clicking repeatedly on the "parent" commit link on each patch working your way back through the history... Well, it looks like it's bsaed on 4.10-rc1 with who-knows-what work from the linux-media tree (I didn't try and go back any further.) As I don't want to take a whole pile of other changes into my tree, I'm certainly not going to pull from your github tree. Sorry. https://github.com/slongerbeam/mediatree/compare/master...imx-media-staging-md-wip It's under the "Compare" button from the main view. It would be nice though if the first commit's parent was some clearly tagged start point. Regards, Ian -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On Tue, Jan 31, 2017 at 10:21:26AM -0800, Steve Longerbeam wrote: > On 01/31/2017 05:42 AM, Russell King - ARM Linux wrote: > >On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote: > >>Should be set to something like 'platform:imx-media-camif'. v4l2-compliance > >>should complain about this. > >... and more. > > Right, in version 3 that you are working with, no v4l2-compliance fixes were > in yet. A lot of the compliance errors are fixed, please look in latest > branch > imx-media-staging-md-wip at g...@github.com:slongerbeam/mediatree.git. Sorry, I'm not prepared to pull random trees from github as there's no easy way to see what's in the branch. I've always disliked github because its web interface makes it soo difficult to navigate around git trees hosted there. You can see a commit, you can see a diff of the commit. You can get a list of branches. But there seems to be no way to get a list of commits similar to "git log" or even a one-line summary of each commit on a branch. If there is, it's completely non-obvious (which I think is much of the problem with github, it's web interface is horrendous.) Or you can clone/pull the tree without knowing what you're fetching (eg, what the tree is based upon.) Or you can waste time clicking repeatedly on the "parent" commit link on each patch working your way back through the history... Well, it looks like it's bsaed on 4.10-rc1 with who-knows-what work from the linux-media tree (I didn't try and go back any further.) As I don't want to take a whole pile of other changes into my tree, I'm certainly not going to pull from your github tree. Sorry. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On 01/31/2017 05:42 AM, Russell King - ARM Linux wrote: On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote: Should be set to something like 'platform:imx-media-camif'. v4l2-compliance should complain about this. ... and more. Right, in version 3 that you are working with, no v4l2-compliance fixes were in yet. A lot of the compliance errors are fixed, please look in latest branch imx-media-staging-md-wip at g...@github.com:slongerbeam/mediatree.git. Total: 39, Succeeded: 33, Failed: 6, Warnings: 0 Not all of these may be a result of Steve's code - this is running against my gradually modified version to support bayer formats (which seems to be the cause of the v4l2-test-formats.cpp failure... for some reason the driver isn't enumerating all the formats.) And that reason is the way that the formats are enumerated: static int camif_enum_fmt_vid_cap(struct file *file, void *fh, struct v4l2_fmtdesc *f) { const struct imx_media_pixfmt *cc; u32 code; int ret; ret = imx_media_enum_format(&code, f->index, true, true); if (ret) return ret; cc = imx_media_find_format(0, code, true, true); if (!cc) return -EINVAL; When imx_media_enum_format() hits this entry in the table: }, { .fourcc = V4L2_PIX_FMT_BGR24, .cs = IPUV3_COLORSPACE_RGB, .bpp= 24, }, { becaues there's no .codes defined: int imx_media_enum_format(u32 *code, u32 index, bool allow_rgb, bool allow_planar) { ... *code = fmt->codes[0]; return 0; } So, we end up calling imx_media_find_format(0, 0, true, true), which fails, returning NULL. That causes camif_enum_fmt_vid_cap() to return -EINVAL. So everything past this entry is unable to be enumerated. I think this is a really round-about way of enumerating the pixel formats when there are soo many entries in the table which have no media bus code - there's absolutely no way that any of those entries can ever be enumerated in this fashion, so they might as well not be in the table... That's my present solution to this problem, to #if 0 out all the entries without any .codes field. I think the real answer is that this needs a _separate_ function to enumerate the pixel formats for camif_enum_fmt_vid_cap(). However, there may be other issues lurking that I've not yet found (still trying to get this code to work...) I believe this has been fixed in imx-media-staging-md-wip as well, see imx-media-capture.c:capture_enum_fmt_vid_cap() Camif subdev is gone, replaced with a set of exported functions that allow attaching a capture device (and v4l2 interface) to a calling subdev's output pad. See imx-media-capture.c. The subdev's capture device interface is the only subdev that can request a planar format from imx_media_enum_format(). All the others now (the non-device node pads), request only RGB or packed YUV, or the IPU internal formats for IPU internal connections, and these are the first entries in the table. The planar formats all are at the end, which can only be enumerated by the capture device interfaces. Steve -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote: > Should be set to something like 'platform:imx-media-camif'. v4l2-compliance > should complain about this. ... and more. Driver Info: Driver name : imx-media-camif Card type : imx-media-camif Bus info : Driver version: 4.10.0 Capabilities : 0x8421 Video Capture Streaming Extended Pix Format Device Capabilities Device Caps : 0x0421 Video Capture Streaming Extended Pix Format Compliance test for device /dev/video3 (not using libv4l2): Required ioctls: fail: v4l2-compliance.cpp(244): string empty fail: v4l2-compliance.cpp(297): check_ustring(vcap.bus_info, sizeof(vcap.bus_info)) test VIDIOC_QUERYCAP: FAIL Allow for multiple opens: test second video open: OK fail: v4l2-compliance.cpp(244): string empty fail: v4l2-compliance.cpp(297): check_ustring(vcap.bus_info, sizeof(vcap.bus_info)) test VIDIOC_QUERYCAP: FAIL test VIDIOC_G/S_PRIORITY: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) fail: v4l2-test-input-output.cpp(382): std == 0 fail: v4l2-test-input-output.cpp(437): invalid attributes for input 0 test VIDIOC_G/S/ENUMINPUT: FAIL test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 0 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK fail: v4l2-test-controls.cpp(779): subscribe event for control 'Camera Controls' failed test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 13 Private Controls: 0 Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) fail: v4l2-test-formats.cpp(414): unknown pixelformat 42474752 for buftype 1 test VIDIOC_G_FMT: FAIL test VIDIOC_TRY_FMT: OK (Not Supported) test VIDIOC_S_FMT: OK (Not Supported) test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK fail: v4l2-test-buffers.cpp(500): q.has_expbuf(node) test VIDIOC_EXPBUF: FAIL Total: 39, Succeeded: 33, Failed: 6, Warnings: 0 Not all of these may be a result of Steve's code - this is running against my gradually modified version to support bayer formats (which seems to be the cause of the v4l2-test-formats.cpp failure... for some reason the driver isn't enumerating all the formats.) And that reason is the way that the formats are enumerated: static int camif_enum_fmt_vid_cap(struct file *file, void *fh, struct v4l2_fmtdesc *f) { const struct imx_media_pixfmt *cc; u32 code; int ret; ret = imx_media_enum_format(&code, f->index, true, true); if (ret) return ret; cc = imx_media_find_format(0, code, true, true); if (!cc) return -EINVAL; When imx_media_enum_format() hits this entry in the table: }, { .fourcc = V4L2_PIX_FMT_BGR24, .cs = IPUV3_COLORSPACE_RGB, .bpp= 24, }, { becaues there's no .codes defined: int imx_media_enum_format(u32 *code, u32 index, bool allow_rgb, bool allow_planar)
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On 01/20/2017 06:38 AM, Hans Verkuil wrote: On 01/07/2017 03:11 AM, Steve Longerbeam wrote: +static int vidioc_querycap(struct file *file, void *fh, + struct v4l2_capability *cap) +{ + strncpy(cap->driver, "imx-media-camif", sizeof(cap->driver) - 1); + strncpy(cap->card, "imx-media-camif", sizeof(cap->card) - 1); + cap->bus_info[0] = 0; Should be set to something like 'platform:imx-media-camif'. v4l2-compliance should complain about this. Right, I've fixed this already as part of v4l2-compliance testing. + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; + cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS; Set device_caps in struct video_device, then drop these two lines since the core will set these up based on the device_caps field in struct video_device. done. + +static int camif_enum_input(struct file *file, void *fh, + struct v4l2_input *input) +{ + struct camif_priv *priv = video_drvdata(file); + struct imx_media_subdev *sensor; + int index = input->index; + + sensor = imx_media_find_sensor(priv->md, &priv->sd.entity); + if (IS_ERR(sensor)) { + v4l2_err(&priv->sd, "no sensor attached\n"); + return PTR_ERR(sensor); + } + + if (index >= sensor->input.num) + return -EINVAL; + + input->type = V4L2_INPUT_TYPE_CAMERA; + strncpy(input->name, sensor->input.name[index], sizeof(input->name)); + + if (index == priv->current_input) { + v4l2_subdev_call(sensor->sd, video, g_input_status, +&input->status); + v4l2_subdev_call(sensor->sd, video, querystd, &input->std); Wrong op, use g_tvnorms instead. done. Steve -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On 01/07/2017 03:11 AM, Steve Longerbeam wrote: > This is the camera interface driver that provides the v4l2 > user interface. Frames can be received from various sources: > > - directly from SMFC for capturing unconverted images directly from > camera sensors. > > - from the IC pre-process encode task. > > - from the IC pre-process viewfinder task. > > - from the IC post-process task. > > Signed-off-by: Steve Longerbeam > --- > drivers/staging/media/imx/Makefile|2 +- > drivers/staging/media/imx/imx-camif.c | 1000 > + > 2 files changed, 1001 insertions(+), 1 deletion(-) > create mode 100644 drivers/staging/media/imx/imx-camif.c > > diff --git a/drivers/staging/media/imx/Makefile > b/drivers/staging/media/imx/Makefile > index d2a962c..fe9e992 100644 > --- a/drivers/staging/media/imx/Makefile > +++ b/drivers/staging/media/imx/Makefile > @@ -8,4 +8,4 @@ obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-ic.o > > obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-csi.o > obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-smfc.o > - > +obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-camif.o > diff --git a/drivers/staging/media/imx/imx-camif.c > b/drivers/staging/media/imx/imx-camif.c > new file mode 100644 > index 000..404f724 > --- /dev/null > +++ b/drivers/staging/media/imx/imx-camif.c > @@ -0,0 +1,1000 @@ > +/* > + * Video Camera Capture Subdev for Freescale i.MX5/6 SOC > + * > + * Copyright (c) 2012-2016 Mentor Graphics Inc. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include "imx-media.h" > + > +#define CAMIF_NUM_PADS 2 > + > +struct camif_priv { > + struct device *dev; > + struct video_devicevfd; > + struct media_pipeline mp; > + struct imx_media_dev *md; > + struct v4l2_subdev sd; > + struct media_pad pad[CAMIF_NUM_PADS]; > + struct media_pad vd_pad; > + int id; > + int input_pad; > + int output_pad; > + > + struct v4l2_mbus_framefmt format_mbus[CAMIF_NUM_PADS]; > + const struct imx_media_pixfmt *cc[CAMIF_NUM_PADS]; > + > + /* dma buffer ring */ > + struct imx_media_dma_buf_ring *in_ring; > + struct v4l2_subdev *src_sd; > + > + struct mutex mutex; /* capture device mutex */ > + spinlock_t q_lock; /* protect ready_q */ > + > + /* buffer queue used in videobuf2 */ > + struct vb2_queue buffer_queue; > + > + /* streaming buffer queue */ > + struct list_head ready_q; > + > + /* controls inherited from subdevs */ > + struct v4l2_ctrl_handler ctrl_hdlr; > + > + /* misc status */ > + intcurrent_input; /* the current input */ > + v4l2_std_idcurrent_std; /* current standard */ > + bool stop; /* streaming is stopping */ > +}; > + > +/* In bytes, per queue */ > +#define VID_MEM_LIMITSZ_64M > + > +static struct vb2_ops camif_qops; > + > +/* > + * Video ioctls follow > + */ > + > +static int vidioc_querycap(struct file *file, void *fh, > +struct v4l2_capability *cap) > +{ > + strncpy(cap->driver, "imx-media-camif", sizeof(cap->driver) - 1); > + strncpy(cap->card, "imx-media-camif", sizeof(cap->card) - 1); > + cap->bus_info[0] = 0; Should be set to something like 'platform:imx-media-camif'. v4l2-compliance should complain about this. > + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; > + cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS; Set device_caps in struct video_device, then drop these two lines since the core will set these up based on the device_caps field in struct video_device. > + > + return 0; > +} > + > +static int camif_enum_fmt_vid_cap(struct file *file, void *fh, > + struct v4l2_fmtdesc *f) > +{ > + const struct imx_media_pixfmt *cc; > + u32 code; > + int ret; > + > + ret = imx_media_enum_format(&code, f->index, true, true); > + if (ret) > + return ret; > + cc = imx_media_find_format(0, code, true, true); > + if (!cc) > + return -EINVAL; > + > + f->pixelformat = cc->fourcc; > + > + return 0; > +} > + > +static int camif_g_fmt_vid_cap(struct file *file, void *fh, > +struct v4l2_format *f) > +{ > + struct camif_priv *priv = video_drvdata(file); > + struct v4l2_mbus_framefmt *outfmt; > + > + /* user format is the same as the format from output pad */ > +
[PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
This is the camera interface driver that provides the v4l2 user interface. Frames can be received from various sources: - directly from SMFC for capturing unconverted images directly from camera sensors. - from the IC pre-process encode task. - from the IC pre-process viewfinder task. - from the IC post-process task. Signed-off-by: Steve Longerbeam --- drivers/staging/media/imx/Makefile|2 +- drivers/staging/media/imx/imx-camif.c | 1000 + 2 files changed, 1001 insertions(+), 1 deletion(-) create mode 100644 drivers/staging/media/imx/imx-camif.c diff --git a/drivers/staging/media/imx/Makefile b/drivers/staging/media/imx/Makefile index d2a962c..fe9e992 100644 --- a/drivers/staging/media/imx/Makefile +++ b/drivers/staging/media/imx/Makefile @@ -8,4 +8,4 @@ obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-ic.o obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-csi.o obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-smfc.o - +obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-camif.o diff --git a/drivers/staging/media/imx/imx-camif.c b/drivers/staging/media/imx/imx-camif.c new file mode 100644 index 000..404f724 --- /dev/null +++ b/drivers/staging/media/imx/imx-camif.c @@ -0,0 +1,1000 @@ +/* + * Video Camera Capture Subdev for Freescale i.MX5/6 SOC + * + * Copyright (c) 2012-2016 Mentor Graphics Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "imx-media.h" + +#define CAMIF_NUM_PADS 2 + +struct camif_priv { + struct device *dev; + struct video_devicevfd; + struct media_pipeline mp; + struct imx_media_dev *md; + struct v4l2_subdev sd; + struct media_pad pad[CAMIF_NUM_PADS]; + struct media_pad vd_pad; + int id; + int input_pad; + int output_pad; + + struct v4l2_mbus_framefmt format_mbus[CAMIF_NUM_PADS]; + const struct imx_media_pixfmt *cc[CAMIF_NUM_PADS]; + + /* dma buffer ring */ + struct imx_media_dma_buf_ring *in_ring; + struct v4l2_subdev *src_sd; + + struct mutex mutex; /* capture device mutex */ + spinlock_t q_lock; /* protect ready_q */ + + /* buffer queue used in videobuf2 */ + struct vb2_queue buffer_queue; + + /* streaming buffer queue */ + struct list_head ready_q; + + /* controls inherited from subdevs */ + struct v4l2_ctrl_handler ctrl_hdlr; + + /* misc status */ + intcurrent_input; /* the current input */ + v4l2_std_idcurrent_std; /* current standard */ + bool stop; /* streaming is stopping */ +}; + +/* In bytes, per queue */ +#define VID_MEM_LIMIT SZ_64M + +static struct vb2_ops camif_qops; + +/* + * Video ioctls follow + */ + +static int vidioc_querycap(struct file *file, void *fh, + struct v4l2_capability *cap) +{ + strncpy(cap->driver, "imx-media-camif", sizeof(cap->driver) - 1); + strncpy(cap->card, "imx-media-camif", sizeof(cap->card) - 1); + cap->bus_info[0] = 0; + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; + cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS; + + return 0; +} + +static int camif_enum_fmt_vid_cap(struct file *file, void *fh, + struct v4l2_fmtdesc *f) +{ + const struct imx_media_pixfmt *cc; + u32 code; + int ret; + + ret = imx_media_enum_format(&code, f->index, true, true); + if (ret) + return ret; + cc = imx_media_find_format(0, code, true, true); + if (!cc) + return -EINVAL; + + f->pixelformat = cc->fourcc; + + return 0; +} + +static int camif_g_fmt_vid_cap(struct file *file, void *fh, + struct v4l2_format *f) +{ + struct camif_priv *priv = video_drvdata(file); + struct v4l2_mbus_framefmt *outfmt; + + /* user format is the same as the format from output pad */ + outfmt = &priv->format_mbus[priv->output_pad]; + return imx_media_mbus_fmt_to_pix_fmt(&f->fmt.pix, outfmt); +} + +static int camif_try_fmt_vid_cap(struct file *file, void *fh, +struct v4l2_format *f) +{ + return camif_g_fmt_vid_cap(file, fh, f); +} + +static int camif_s_fmt_vid_cap(struct file *file, void *fh, + struct v4l2_format *f) +{ + struct camif_priv *priv = video_drvdata(file); + + if (vb2_is_busy(&priv->buffer_queue)) { +