Re: [PATCH v4 20/36] media: imx: Add CSI subdev driver

2017-02-16 Thread Russell King - ARM Linux 
On Thu, Feb 16, 2017 at 10:44:16AM -0800, Steve Longerbeam wrote:
> On 02/16/2017 04:40 AM, Russell King - ARM Linux wrote:
> >[8.012191] imx_media_common: module is from the staging directory, the 
> >quality is unknown, you have been warned.
> >[8.018175] imx_media: module is from the staging directory, the quality 
> >is unknown, you have been warned.
> >[8.748345] imx-media: Registered subdev ipu1_csi0_mux
> >[8.753451] imx-media: Registered subdev ipu2_csi1_mux
> >[9.055196] imx219 0-0010: detected IMX219 sensor
> >[9.090733] imx6_mipi_csi2: module is from the staging directory, the 
> >quality is unknown, you have been warned.
> >[9.092247] imx-media: Registered subdev imx219 0-0010
> >[9.334338] imx-media: Registered subdev imx6-mipi-csi2
> >[9.372452] imx_media_capture: module is from the staging directory, the 
> >quality is unknown, you have been warned.
> >[9.378163] imx_media_capture: module is from the staging directory, the 
> >quality is unknown, you have been warned.
> >[9.390033] imx_media_csi: module is from the staging directory, the 
> >quality is unknown, you have been warned.
> >[9.394362] imx-media: Received unknown subdev ipu1_csi0
> 
> The root problem is here. I don't know why the CSI entities are not
> being recognized. Can you share the changes you made?

No, it's not the root problem that's causing the BUG/etc, but it is
_a_ problem.  Nevertheless, it's something I fixed - disconnecting
the of_node from the struct device needed one other change in the
imx-media code that was missing at this time.

However, that's no excuse what so ever for the BUG_ON() and lack of
error cleanup (causing use-after-free, which is just another way of
saying "data corruption waiting to happen") that I identified.

-- 
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.

Re: [PATCH v4 20/36] media: imx: Add CSI subdev driver

2017-02-16 Thread Steve Longerbeam 



On 02/16/2017 06:20 AM, Russell King - ARM Linux wrote:

On Thu, Feb 16, 2017 at 01:09:35PM +, Russell King - ARM Linux wrote:



More crap.

If the "complete" method fails (or, in fact, anything in
v4l2_async_test_notify() fails) then all hell breaks loose, because
of the total lack of clean up (and no, this isn't anything to do with
some stupid justification of those BUG_ON()s above.)

v4l2_async_notifier_register() gets called, it adds the notifier to
the global notifier list.  v4l2_async_test_notify() gets called.  It
returns an error, which is propagated out of
v4l2_async_notifier_register().

So the caller thinks that v4l2_async_notifier_register() failed, which
will cause imx_media_probe() to fail, causing imxmd->subdev_notifier
to be kfree()'d.  We now have a use-after free bug.

Second case.  v4l2_async_register_subdev().  Almost exactly the same,
except in this case adding sd->async_list to the notifier->done list
may have succeeded, and failure after that, again, results in an
in-use list_head being kfree()'d.


And here's a patch which, combined with the fixes for ipuv3, results in
everything appearing to work properly.  Feel free to tear out the bits
for your area and turn them into proper patches.

 drivers/gpu/ipu-v3/ipu-common.c   |  6 ---
 drivers/media/media-entity.c  |  7 +--
 drivers/media/v4l2-core/v4l2-async.c  | 71 +++
 drivers/staging/media/imx/imx-media-csi.c |  1 +
 drivers/staging/media/imx/imx-media-dev.c |  2 +-
 5 files changed, 59 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-common.c b/drivers/gpu/ipu-v3/ipu-common.c
index 97218af4fe75..8368e6f766ee 100644
--- a/drivers/gpu/ipu-v3/ipu-common.c
+++ b/drivers/gpu/ipu-v3/ipu-common.c
@@ -1238,12 +1238,6 @@ static int ipu_add_client_devices(struct ipu_soc *ipu, 
unsigned long ipu_base)
platform_device_put(pdev);
goto err_register;
}
-
-   /*
-* Set of_node only after calling platform_device_add. Otherwise
-* the platform:imx-ipuv3-crtc modalias won't be used.
-*/
-   pdev->dev.of_node = of_node;
}



Ah, never mind my question earlier, I see now why the CSI's were likely
not recognized, probably because of this. Anyway I agree with this 
change and I made the accompanying requisite change to imx-media-csi.c

and imx-media-dev.c below.

Steve






return 0;
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index f9f723f5e4f0..154593a168df 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -625,9 +625,10 @@ media_create_pad_link(struct media_entity *source, u16 
source_pad,
struct media_link *link;
struct media_link *backlink;

-   BUG_ON(source == NULL || sink == NULL);
-   BUG_ON(source_pad >= source->num_pads);
-   BUG_ON(sink_pad >= sink->num_pads);
+   if (WARN_ON(source == NULL || sink == NULL) ||
+   WARN_ON(source_pad >= source->num_pads) ||
+   WARN_ON(sink_pad >= sink->num_pads))
+   return -EINVAL;

link = media_add_link(>links);
if (link == NULL)
diff --git a/drivers/media/v4l2-core/v4l2-async.c 
b/drivers/media/v4l2-core/v4l2-async.c
index 5bada202b2d3..09934fb96a8d 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -94,7 +94,7 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct 
v4l2_async_notifier *
 }

 static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
- struct v4l2_subdev *sd,
+ struct list_head *new, struct v4l2_subdev *sd,
  struct v4l2_async_subdev *asd)
 {
int ret;
@@ -107,22 +107,36 @@ static int v4l2_async_test_notify(struct 
v4l2_async_notifier *notifier,
if (notifier->bound) {
ret = notifier->bound(notifier, sd, asd);
if (ret < 0)
-   return ret;
+   goto err_bind;
}
+
/* Move from the global subdevice list to notifier's done */
-   list_move(>async_list, >done);
+   list_move(>async_list, new);

ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
-   if (ret < 0) {
-   if (notifier->unbind)
-   notifier->unbind(notifier, sd, asd);
-   return ret;
-   }
+   if (ret < 0)
+   goto err_register;

-   if (list_empty(>waiting) && notifier->complete)
-   return notifier->complete(notifier);
+   if (list_empty(>waiting) && notifier->complete) {
+   ret = notifier->complete(notifier);
+   if (ret < 0)
+   goto err_complete;
+   }

return 0;
+
+err_complete:
+   v4l2_device_unregister_subdev(sd);
+err_register:
+

Re: [PATCH v4 20/36] media: imx: Add CSI subdev driver

2017-02-16 Thread Steve Longerbeam 



On 02/16/2017 04:40 AM, Russell King - ARM Linux wrote:

On Thu, Feb 16, 2017 at 11:52:06AM +, Russell King - ARM Linux wrote:

On Wed, Feb 15, 2017 at 06:19:22PM -0800, Steve Longerbeam wrote:

+static const struct platform_device_id imx_csi_ids[] = {
+   { .name = "imx-ipuv3-csi" },
+   { },
+};
+MODULE_DEVICE_TABLE(platform, imx_csi_ids);
+
+static struct platform_driver imx_csi_driver = {
+   .probe = imx_csi_probe,
+   .remove = imx_csi_remove,
+   .id_table = imx_csi_ids,
+   .driver = {
+   .name = "imx-ipuv3-csi",
+   },
+};
+module_platform_driver(imx_csi_driver);
+
+MODULE_DESCRIPTION("i.MX CSI subdev driver");
+MODULE_AUTHOR("Steve Longerbeam ");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:imx-ipuv3-csi");


Just a reminder that automatic module loading of this is completely
broken right now (not your problem) due to this stupid idea in the
IPUv3 code:

if (!ret)
ret = platform_device_add(pdev);
if (ret) {
platform_device_put(pdev);
goto err_register;
}

/*
 * Set of_node only after calling platform_device_add. Otherwise
 * the platform:imx-ipuv3-crtc modalias won't be used.
 */
pdev->dev.of_node = of_node;

setting pdev->dev.of_node changes the modalias exported to userspace,
so udev sees a DT based modalias, which causes it to totally miss any
driver using a non-DT based modalias.

The IPUv3 code needs fixing, not only for imx-media-csi, but also for
imx-ipuv3-crtc too, because that module will also suffer the same
issue.

The only solution is... don't fsck with dev->of_node assignment.  In
this case, it's probably much better to pass it in via platform data.
If you then absolutely must have dev->of_node, doing it in the driver
means that you avoid the modalias mess before the appropriate driver
is loaded.  However, that's still not a nice solution because the
modalias file still ends up randomly changing its contents.

As I say, not _your_ problem, but it's still a problem that needs
solving, and I don't want it forgotten about.


I've just hacked up a solution to this, and unfortunately it reveals a
problem with Steve's code.  Picking out the imx & media-related messages:

[8.012191] imx_media_common: module is from the staging directory, the 
quality is unknown, you have been warned.
[8.018175] imx_media: module is from the staging directory, the quality is 
unknown, you have been warned.
[8.748345] imx-media: Registered subdev ipu1_csi0_mux
[8.753451] imx-media: Registered subdev ipu2_csi1_mux
[9.055196] imx219 0-0010: detected IMX219 sensor
[9.090733] imx6_mipi_csi2: module is from the staging directory, the 
quality is unknown, you have been warned.
[9.092247] imx-media: Registered subdev imx219 0-0010
[9.334338] imx-media: Registered subdev imx6-mipi-csi2
[9.372452] imx_media_capture: module is from the staging directory, the 
quality is unknown, you have been warned.
[9.378163] imx_media_capture: module is from the staging directory, the 
quality is unknown, you have been warned.
[9.390033] imx_media_csi: module is from the staging directory, the quality 
is unknown, you have been warned.
[9.394362] imx-media: Received unknown subdev ipu1_csi0


The root problem is here. I don't know why the CSI entities are not
being recognized. Can you share the changes you made?

So imx_media_subdev_bound() returns error because it didn't recognize
the subdev that was bound.

And for some reason, even though some of the subdev bound ops return
error, v4l2-core still calls the async completion notifier
(imx_media_probe_complete()).

I'll add some checks to imx_media_probe_complete() to try and detect
when not all subdevs were bound correctly to get around this issue.
That should prevent the kernel BUG() below.

Steve



[9.394699] imx-ipuv3-csi: probe of imx-ipuv3-csi.0 failed with error -22
[9.394840] imx-media: Received unknown subdev ipu1_csi1
[9.394887] imx-ipuv3-csi: probe of imx-ipuv3-csi.1 failed with error -22
[9.394992] imx-media: Received unknown subdev ipu2_csi0
[9.395026] imx-ipuv3-csi: probe of imx-ipuv3-csi.4 failed with error -22
[9.395119] imx-media: Received unknown subdev ipu2_csi1
[9.395159] imx-ipuv3-csi: probe of imx-ipuv3-csi.5 failed with error -22
[9.411722] imx_media_vdic: module is from the staging directory, the 
quality is unknown, you have been warned.
[9.412820] imx-media: Registered subdev ipu1_vdic
[9.424687] imx-media: Registered subdev ipu2_vdic
[9.436074] imx_media_ic: module is from the staging directory, the quality 
is unknown, you have been warned.
[9.437455] imx-media: Registered subdev ipu1_ic_prp
[9.437788] imx_media_ic: module is from the staging directory, the quality 
is unknown,

Re: [PATCH v4 20/36] media: imx: Add CSI subdev driver

2017-02-16 Thread Russell King - ARM Linux 
On Thu, Feb 16, 2017 at 01:09:35PM +, Russell King - ARM Linux wrote:
> On Thu, Feb 16, 2017 at 12:40:27PM +, Russell King - ARM Linux wrote:
> > However, the following is primerily directed at Laurent as the one who
> > introduced the BUG_ON() in question...
> > 
> > NEVER EVER USE BUG_ON() IN A PATH THAT CAN RETURN AN ERROR.
> > 
> > It's possible to find Linus rants about this, eg,
> > https://www.spinics.net/lists/stable/msg146439.html
> > 
> >  I should have reacted to the damn added BUG_ON() lines. I suspect I
> >  will have to finally just remove the idiotic BUG_ON() concept once and
> >  for all, because there is NO F*CKING EXCUSE to knowingly kill the
> >  kernel.
> > 
> > Also: http://yarchive.net/comp/linux/BUG.html
> > 
> >  Rule of thumb: BUG() is only good for something that never happens and
> >  that we really have no other option for (ie state is so corrupt that
> >  continuing is deadly).
> > 
> > So, _unless_ people want to see BUG_ON() removed from the kernel, I
> > strongly suggest to _STOP_ using it as "we didn't like the function
> > arguments, let's use it as an assert() statement instead of returning
> > an error."
> > 
> > There's no excuse what so ever to be killing the machine in
> > media_create_pad_link().  If it doesn't like a NULL pointer, it's damn
> > well got an error path to report that fact.  Use that mechanism and
> > stop needlessly killing the kernel.
> > 
> > BUG_ON() IS NOT ASSERT().  DO NOT USE IT AS SUCH.
> > 
> > Linus is absolutely right about BUG_ON() - it hurts debuggability,
> > because now the only way to do further tests is to reboot the damned
> > machine after removing those fscking BUG_ON()s that should *never*
> > have been there in the first place.
> > 
> > As Linus went on to say:
> > 
> >  And dammit, if anybody else feels that they had done "debugging
> >  messages with BUG_ON()", I would suggest you
> > 
> >   (a) rethink your approach to programming
> > 
> >   (b) send me patches to remove the crap entirely, or make them real
> >  *DEBUGGING* messages, not "kill the whole machine" messages.
> > 
> >  I've ranted against people using BUG_ON() for debugging in the past.
> >  Why the f*ck does this still happen? And Andrew - please stop taking
> >  those kinds of patches! Lookie here:
> > 
> >  https://lwn.net/Articles/13183/
> > 
> >  so excuse me for being upset that people still do this shit almost 15
> >  years later.
> > 
> > So I suggest people heed that advice and start fixing these stupid
> > BUG_ON()s that they've created.
> 
> More crap.
> 
> If the "complete" method fails (or, in fact, anything in
> v4l2_async_test_notify() fails) then all hell breaks loose, because
> of the total lack of clean up (and no, this isn't anything to do with
> some stupid justification of those BUG_ON()s above.)
> 
> v4l2_async_notifier_register() gets called, it adds the notifier to
> the global notifier list.  v4l2_async_test_notify() gets called.  It
> returns an error, which is propagated out of
> v4l2_async_notifier_register().
> 
> So the caller thinks that v4l2_async_notifier_register() failed, which
> will cause imx_media_probe() to fail, causing imxmd->subdev_notifier
> to be kfree()'d.  We now have a use-after free bug.
> 
> Second case.  v4l2_async_register_subdev().  Almost exactly the same,
> except in this case adding sd->async_list to the notifier->done list
> may have succeeded, and failure after that, again, results in an
> in-use list_head being kfree()'d.

And here's a patch which, combined with the fixes for ipuv3, results in
everything appearing to work properly.  Feel free to tear out the bits
for your area and turn them into proper patches.

 drivers/gpu/ipu-v3/ipu-common.c   |  6 ---
 drivers/media/media-entity.c  |  7 +--
 drivers/media/v4l2-core/v4l2-async.c  | 71 +++
 drivers/staging/media/imx/imx-media-csi.c |  1 +
 drivers/staging/media/imx/imx-media-dev.c |  2 +-
 5 files changed, 59 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-common.c b/drivers/gpu/ipu-v3/ipu-common.c
index 97218af4fe75..8368e6f766ee 100644
--- a/drivers/gpu/ipu-v3/ipu-common.c
+++ b/drivers/gpu/ipu-v3/ipu-common.c
@@ -1238,12 +1238,6 @@ static int ipu_add_client_devices(struct ipu_soc *ipu, 
unsigned long ipu_base)
platform_device_put(pdev);
goto err_register;
}
-
-   /*
-* Set of_node only after calling platform_device_add. Otherwise
-* the platform:imx-ipuv3-crtc modalias won't be used.
-*/
-   pdev->dev.of_node = of_node;
}
 
return 0;
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index f9f723f5e4f0..154593a168df 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -625,9 +625,10 @@ media_create_pad_link(struct media_entity *source, u16 
source_pad,
struct

Re: [PATCH v4 20/36] media: imx: Add CSI subdev driver

2017-02-16 Thread Russell King - ARM Linux 
On Thu, Feb 16, 2017 at 12:40:27PM +, Russell King - ARM Linux wrote:
> However, the following is primerily directed at Laurent as the one who
> introduced the BUG_ON() in question...
> 
> NEVER EVER USE BUG_ON() IN A PATH THAT CAN RETURN AN ERROR.
> 
> It's possible to find Linus rants about this, eg,
> https://www.spinics.net/lists/stable/msg146439.html
> 
>  I should have reacted to the damn added BUG_ON() lines. I suspect I
>  will have to finally just remove the idiotic BUG_ON() concept once and
>  for all, because there is NO F*CKING EXCUSE to knowingly kill the
>  kernel.
> 
> Also: http://yarchive.net/comp/linux/BUG.html
> 
>  Rule of thumb: BUG() is only good for something that never happens and
>  that we really have no other option for (ie state is so corrupt that
>  continuing is deadly).
> 
> So, _unless_ people want to see BUG_ON() removed from the kernel, I
> strongly suggest to _STOP_ using it as "we didn't like the function
> arguments, let's use it as an assert() statement instead of returning
> an error."
> 
> There's no excuse what so ever to be killing the machine in
> media_create_pad_link().  If it doesn't like a NULL pointer, it's damn
> well got an error path to report that fact.  Use that mechanism and
> stop needlessly killing the kernel.
> 
> BUG_ON() IS NOT ASSERT().  DO NOT USE IT AS SUCH.
> 
> Linus is absolutely right about BUG_ON() - it hurts debuggability,
> because now the only way to do further tests is to reboot the damned
> machine after removing those fscking BUG_ON()s that should *never*
> have been there in the first place.
> 
> As Linus went on to say:
> 
>  And dammit, if anybody else feels that they had done "debugging
>  messages with BUG_ON()", I would suggest you
> 
>   (a) rethink your approach to programming
> 
>   (b) send me patches to remove the crap entirely, or make them real
>  *DEBUGGING* messages, not "kill the whole machine" messages.
> 
>  I've ranted against people using BUG_ON() for debugging in the past.
>  Why the f*ck does this still happen? And Andrew - please stop taking
>  those kinds of patches! Lookie here:
> 
>  https://lwn.net/Articles/13183/
> 
>  so excuse me for being upset that people still do this shit almost 15
>  years later.
> 
> So I suggest people heed that advice and start fixing these stupid
> BUG_ON()s that they've created.

More crap.

If the "complete" method fails (or, in fact, anything in
v4l2_async_test_notify() fails) then all hell breaks loose, because
of the total lack of clean up (and no, this isn't anything to do with
some stupid justification of those BUG_ON()s above.)

v4l2_async_notifier_register() gets called, it adds the notifier to
the global notifier list.  v4l2_async_test_notify() gets called.  It
returns an error, which is propagated out of
v4l2_async_notifier_register().

So the caller thinks that v4l2_async_notifier_register() failed, which
will cause imx_media_probe() to fail, causing imxmd->subdev_notifier
to be kfree()'d.  We now have a use-after free bug.

Second case.  v4l2_async_register_subdev().  Almost exactly the same,
except in this case adding sd->async_list to the notifier->done list
may have succeeded, and failure after that, again, results in an
in-use list_head being kfree()'d.

-- 
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.

Re: [PATCH v4 20/36] media: imx: Add CSI subdev driver

2017-02-16 Thread Russell King - ARM Linux 
On Thu, Feb 16, 2017 at 11:52:06AM +, Russell King - ARM Linux wrote:
> On Wed, Feb 15, 2017 at 06:19:22PM -0800, Steve Longerbeam wrote:
> > +static const struct platform_device_id imx_csi_ids[] = {
> > +   { .name = "imx-ipuv3-csi" },
> > +   { },
> > +};
> > +MODULE_DEVICE_TABLE(platform, imx_csi_ids);
> > +
> > +static struct platform_driver imx_csi_driver = {
> > +   .probe = imx_csi_probe,
> > +   .remove = imx_csi_remove,
> > +   .id_table = imx_csi_ids,
> > +   .driver = {
> > +   .name = "imx-ipuv3-csi",
> > +   },
> > +};
> > +module_platform_driver(imx_csi_driver);
> > +
> > +MODULE_DESCRIPTION("i.MX CSI subdev driver");
> > +MODULE_AUTHOR("Steve Longerbeam ");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:imx-ipuv3-csi");
> 
> Just a reminder that automatic module loading of this is completely
> broken right now (not your problem) due to this stupid idea in the
> IPUv3 code:
> 
>   if (!ret)
>   ret = platform_device_add(pdev);
>   if (ret) {
>   platform_device_put(pdev);
>   goto err_register;
>   }
> 
>   /*
>* Set of_node only after calling platform_device_add. Otherwise
>* the platform:imx-ipuv3-crtc modalias won't be used.
>*/
>   pdev->dev.of_node = of_node;
> 
> setting pdev->dev.of_node changes the modalias exported to userspace,
> so udev sees a DT based modalias, which causes it to totally miss any
> driver using a non-DT based modalias.
> 
> The IPUv3 code needs fixing, not only for imx-media-csi, but also for
> imx-ipuv3-crtc too, because that module will also suffer the same
> issue.
> 
> The only solution is... don't fsck with dev->of_node assignment.  In
> this case, it's probably much better to pass it in via platform data.
> If you then absolutely must have dev->of_node, doing it in the driver
> means that you avoid the modalias mess before the appropriate driver
> is loaded.  However, that's still not a nice solution because the
> modalias file still ends up randomly changing its contents.
> 
> As I say, not _your_ problem, but it's still a problem that needs
> solving, and I don't want it forgotten about.

I've just hacked up a solution to this, and unfortunately it reveals a
problem with Steve's code.  Picking out the imx & media-related messages:

[8.012191] imx_media_common: module is from the staging directory, the 
quality is unknown, you have been warned.
[8.018175] imx_media: module is from the staging directory, the quality is 
unknown, you have been warned.
[8.748345] imx-media: Registered subdev ipu1_csi0_mux
[8.753451] imx-media: Registered subdev ipu2_csi1_mux
[9.055196] imx219 0-0010: detected IMX219 sensor
[9.090733] imx6_mipi_csi2: module is from the staging directory, the 
quality is unknown, you have been warned.
[9.092247] imx-media: Registered subdev imx219 0-0010
[9.334338] imx-media: Registered subdev imx6-mipi-csi2
[9.372452] imx_media_capture: module is from the staging directory, the 
quality is unknown, you have been warned.
[9.378163] imx_media_capture: module is from the staging directory, the 
quality is unknown, you have been warned.
[9.390033] imx_media_csi: module is from the staging directory, the quality 
is unknown, you have been warned.
[9.394362] imx-media: Received unknown subdev ipu1_csi0
[9.394699] imx-ipuv3-csi: probe of imx-ipuv3-csi.0 failed with error -22
[9.394840] imx-media: Received unknown subdev ipu1_csi1
[9.394887] imx-ipuv3-csi: probe of imx-ipuv3-csi.1 failed with error -22
[9.394992] imx-media: Received unknown subdev ipu2_csi0
[9.395026] imx-ipuv3-csi: probe of imx-ipuv3-csi.4 failed with error -22
[9.395119] imx-media: Received unknown subdev ipu2_csi1
[9.395159] imx-ipuv3-csi: probe of imx-ipuv3-csi.5 failed with error -22
[9.411722] imx_media_vdic: module is from the staging directory, the 
quality is unknown, you have been warned.
[9.412820] imx-media: Registered subdev ipu1_vdic
[9.424687] imx-media: Registered subdev ipu2_vdic
[9.436074] imx_media_ic: module is from the staging directory, the quality 
is unknown, you have been warned.
[9.437455] imx-media: Registered subdev ipu1_ic_prp
[9.437788] imx_media_ic: module is from the staging directory, the quality 
is unknown, you have been warned.
[9.447542] imx-media: Registered subdev ipu1_ic_prpenc
[9.455225] ipu1_ic_prpenc: Registered ipu1_ic_prpenc capture as /dev/video3
[9.459203] imx-media: Registered subdev ipu1_ic_prpvf
[9.460484] imx_media_ic: module is from the staging directory, the quality 
is unknown, you have been warned.
[9.460726] ipu1_ic_prpvf: Registered ipu1_ic_prpvf capture as /dev/video4
[9.460983] imx-media: Registered subdev ipu2_ic_prp
[9.461161] imx-media: Registered subdev ipu2_ic_prpenc
[9.461737]

Re: [PATCH v4 20/36] media: imx: Add CSI subdev driver

2017-02-16 Thread Russell King - ARM Linux 
On Wed, Feb 15, 2017 at 06:19:22PM -0800, Steve Longerbeam wrote:
> +static const struct platform_device_id imx_csi_ids[] = {
> + { .name = "imx-ipuv3-csi" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(platform, imx_csi_ids);
> +
> +static struct platform_driver imx_csi_driver = {
> + .probe = imx_csi_probe,
> + .remove = imx_csi_remove,
> + .id_table = imx_csi_ids,
> + .driver = {
> + .name = "imx-ipuv3-csi",
> + },
> +};
> +module_platform_driver(imx_csi_driver);
> +
> +MODULE_DESCRIPTION("i.MX CSI subdev driver");
> +MODULE_AUTHOR("Steve Longerbeam ");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:imx-ipuv3-csi");

Just a reminder that automatic module loading of this is completely
broken right now (not your problem) due to this stupid idea in the
IPUv3 code:

if (!ret)
ret = platform_device_add(pdev);
if (ret) {
platform_device_put(pdev);
goto err_register;
}

/*
 * Set of_node only after calling platform_device_add. Otherwise
 * the platform:imx-ipuv3-crtc modalias won't be used.
 */
pdev->dev.of_node = of_node;

setting pdev->dev.of_node changes the modalias exported to userspace,
so udev sees a DT based modalias, which causes it to totally miss any
driver using a non-DT based modalias.

The IPUv3 code needs fixing, not only for imx-media-csi, but also for
imx-ipuv3-crtc too, because that module will also suffer the same
issue.

The only solution is... don't fsck with dev->of_node assignment.  In
this case, it's probably much better to pass it in via platform data.
If you then absolutely must have dev->of_node, doing it in the driver
means that you avoid the modalias mess before the appropriate driver
is loaded.  However, that's still not a nice solution because the
modalias file still ends up randomly changing its contents.

As I say, not _your_ problem, but it's still a problem that needs
solving, and I don't want it forgotten about.

-- 
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.

[PATCH v4 20/36] media: imx: Add CSI subdev driver

2017-02-15 Thread Steve Longerbeam 
This is a media entity subdevice for the i.MX Camera
Sensor Interface module.

Signed-off-by: Steve Longerbeam 
---
 drivers/staging/media/imx/Kconfig |   13 +
 drivers/staging/media/imx/Makefile|2 +
 drivers/staging/media/imx/imx-media-csi.c | 1220 +
 3 files changed, 1235 insertions(+)
 create mode 100644 drivers/staging/media/imx/imx-media-csi.c

diff --git a/drivers/staging/media/imx/Kconfig 
b/drivers/staging/media/imx/Kconfig
index 722ed55..e27ad6d 100644
--- a/drivers/staging/media/imx/Kconfig
+++ b/drivers/staging/media/imx/Kconfig
@@ -5,3 +5,16 @@ config VIDEO_IMX_MEDIA
  Say yes here to enable support for video4linux media controller
  driver for the i.MX5/6 SOC.
 
+if VIDEO_IMX_MEDIA
+menu "i.MX5/6 Media Sub devices"
+
+config VIDEO_IMX_CSI
+   tristate "i.MX5/6 Camera Sensor Interface driver"
+   depends on VIDEO_IMX_MEDIA && VIDEO_DEV && I2C
+   select VIDEOBUF2_DMA_CONTIG
+   default y
+   ---help---
+ A video4linux camera sensor interface driver for i.MX5/6.
+
+endmenu
+endif
diff --git a/drivers/staging/media/imx/Makefile 
b/drivers/staging/media/imx/Makefile
index 4606a3a..c054490 100644
--- a/drivers/staging/media/imx/Makefile
+++ b/drivers/staging/media/imx/Makefile
@@ -4,3 +4,5 @@ imx-media-common-objs := imx-media-utils.o imx-media-fim.o
 obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media.o
 obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-common.o
 obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-capture.o
+
+obj-$(CONFIG_VIDEO_IMX_CSI) += imx-media-csi.o
diff --git a/drivers/staging/media/imx/imx-media-csi.c 
b/drivers/staging/media/imx/imx-media-csi.c
new file mode 100644
index 000..0343fc3
--- /dev/null
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -0,0 +1,1220 @@
+/*
+ * V4L2 Capture CSI Subdev for Freescale i.MX5/6 SOC
+ *
+ * Copyright (c) 2014-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 "imx-media.h"
+
+/*
+ * Min/Max supported width and heights.
+ *
+ * We allow planar output, so we have to align width by 16 pixels
+ * to meet IDMAC alignment requirements.
+ *
+ * TODO: move this into pad format negotiation, if capture device
+ * has not requested planar formats, we should allow 8 pixel
+ * alignment.
+ */
+#define MIN_W   176
+#define MIN_H   144
+#define MAX_W  4096
+#define MAX_H  4096
+#define W_ALIGN4 /* multiple of 16 pixels */
+#define H_ALIGN1 /* multiple of 2 lines */
+#define S_ALIGN1 /* multiple of 2 */
+
+struct csi_priv {
+   struct device *dev;
+   struct ipu_soc *ipu;
+   struct imx_media_dev *md;
+   struct v4l2_subdev sd;
+   struct media_pad pad[CSI_NUM_PADS];
+   int active_output_pad;
+   int csi_id;
+   int smfc_id;
+
+   struct ipuv3_channel *idmac_ch;
+   struct ipu_smfc *smfc;
+   struct ipu_csi *csi;
+
+   struct v4l2_mbus_framefmt format_mbus[CSI_NUM_PADS];
+   const struct imx_media_pixfmt *cc[CSI_NUM_PADS];
+   struct v4l2_rect crop;
+
+   /* the video device at IDMAC output pad */
+   struct imx_media_video_dev *vdev;
+
+   /* active vb2 buffers to send to video dev sink */
+   struct imx_media_buffer *active_vb2_buf[2];
+   struct imx_media_dma_buf underrun_buf;
+
+   int ipu_buf_num;  /* ipu double buffer index: 0-1 */
+
+   /* the sink for the captured frames */
+   struct media_entity *sink;
+   enum ipu_csi_dest dest;
+   /* the source subdev */
+   struct v4l2_subdev *src_sd;
+
+   /* the mipi virtual channel number at link validate */
+   int vc_num;
+
+   /* the attached sensor at stream on */
+   struct imx_media_subdev *sensor;
+
+   spinlock_t irqlock; /* protect eof_irq handler */
+   struct timer_list eof_timeout_timer;
+   int eof_irq;
+   int nfb4eof_irq;
+
+   struct v4l2_ctrl_handler ctrl_hdlr;
+   struct imx_media_fim *fim;
+
+   bool power_on;  /* power is on */
+   bool stream_on; /* streaming is on */
+   bool last_eof;  /* waiting for last EOF at stream off */
+   struct completion last_eof_comp;
+};
+
+static inline struct csi_priv *sd_to_dev(struct v4l2_subdev *sdev)
+{
+   return container_of(sdev, struct csi_priv, sd);
+}
+
+static void csi_idmac_put_ipu_resources(struct csi_priv *priv)
+{
+   if (!IS_ERR_OR_NULL(priv->idmac_ch))
+   ipu_idmac_put(priv->idmac_ch);
+   priv->idmac_ch = NULL;
+
+   if (!IS_ERR_OR_NULL(priv->smfc))
+   ipu_smfc_put(priv->smfc);
+   priv->smfc = NULL;
+}
+
+static int