Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver

2017-02-06 Thread Steve Longerbeam 


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

2017-02-02 Thread Russell King - ARM Linux 
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   = _fops,
> + .ioctl_ops  = _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(>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 = >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

2017-01-31 Thread Russell King - ARM Linux 
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

2017-01-31 Thread Steve Longerbeam 



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

2017-01-31 Thread Steve Longerbeam 



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

2017-01-31 Thread Ian Arkver 

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

2017-01-31 Thread Russell King - ARM Linux 
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

2017-01-31 Thread Ian Arkver 

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

2017-01-31 Thread Russell King - ARM Linux 
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

2017-01-31 Thread Steve Longerbeam 



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(, 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

2017-01-31 Thread Russell King - ARM Linux 
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(, 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

2017-01-23 Thread Steve Longerbeam 



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, >sd.entity);
+   if (IS_ERR(sensor)) {
+   v4l2_err(>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,
+>status);
+   v4l2_subdev_call(sensor->sd, video, querystd, >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

2017-01-20 Thread Hans Verkuil 
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(, 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

[PATCH v3 20/24] media: imx: Add Camera Interface subdev driver

2017-01-06 Thread Steve Longerbeam 
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(, 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 = >format_mbus[priv->output_pad];
+   return imx_media_mbus_fmt_to_pix_fmt(>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(>buffer_queue)) {
+