[PATCH RFC] media: staging/imx: fix complete handler

[PATCH RFC] media: staging/imx: fix complete handler

[Russell King]

The complete handler walks all entities, expecting to find an imx
subdevice for each and every entity.

However, camera drivers such as smiapp can themselves contain multiple
entities, for which there will not be an imx subdevice.  This causes
imx_media_find_subdev_by_sd() to fail, making the imx capture system
unusable with such cameras.

Work around this by killing the error entirely, thereby allowing
the imx capture to be used with such cameras.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
Not the best solution, but the only one I can think of to fix the
regression that happened sometime between a previous revision of
Steve's patch set and the version that got merged.

 drivers/staging/media/imx/imx-media-dev.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
index d96f4512224f..6ba59939dd7a 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -345,8 +345,11 @@ static int imx_media_add_vdev_to_pad(struct imx_media_dev *imxmd,
 
 	sd = media_entity_to_v4l2_subdev(entity);
 	imxsd = imx_media_find_subdev_by_sd(imxmd, sd);
-	if (IS_ERR(imxsd))
-		return PTR_ERR(imxsd);
+	if (IS_ERR(imxsd)) {
+		v4l2_err(&imxmd->v4l2_dev, "failed to find subdev for entity %s, sd %p err %ld\n",
+			 entity->name, sd, PTR_ERR(imxsd));
+		return 0;
+	}
 
 	imxpad = &imxsd->pad[srcpad->index];
 	vdev_idx = imxpad->num_vdevs;
-- 
2.7.4

Re: [PATCH RFC] media: staging/imx: fix complete handler

[Mauro Carvalho Chehab]

Em Fri, 29 Sep 2017 22:38:39 +0100
Russell King <rmk+kernel@armlinux.org.uk> escreveu:

> The complete handler walks all entities, expecting to find an imx
> subdevice for each and every entity.
> 
> However, camera drivers such as smiapp can themselves contain multiple
> entities, for which there will not be an imx subdevice.  This causes
> imx_media_find_subdev_by_sd() to fail, making the imx capture system
> unusable with such cameras.
> 
> Work around this by killing the error entirely, thereby allowing
> the imx capture to be used with such cameras.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> Not the best solution, but the only one I can think of to fix the
> regression that happened sometime between a previous revision of
> Steve's patch set and the version that got merged.

Yeah, ideally, the complete handling should somehow be aware
about smiapp entities and do the right thing, instead of
just ignoring the errors.

Sakari,

What do you think?

Regards,
Mauro


> 
>  drivers/staging/media/imx/imx-media-dev.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
> index d96f4512224f..6ba59939dd7a 100644
> --- a/drivers/staging/media/imx/imx-media-dev.c
> +++ b/drivers/staging/media/imx/imx-media-dev.c
> @@ -345,8 +345,11 @@ static int imx_media_add_vdev_to_pad(struct imx_media_dev *imxmd,
>  
>  	sd = media_entity_to_v4l2_subdev(entity);
>  	imxsd = imx_media_find_subdev_by_sd(imxmd, sd);
> -	if (IS_ERR(imxsd))
> -		return PTR_ERR(imxsd);
> +	if (IS_ERR(imxsd)) {
> +		v4l2_err(&imxmd->v4l2_dev, "failed to find subdev for entity %s, sd %p err %ld\n",
> +			 entity->name, sd, PTR_ERR(imxsd));
> +		return 0;
> +	}
>  
>  	imxpad = &imxsd->pad[srcpad->index];
>  	vdev_idx = imxpad->num_vdevs;



Thanks,
Mauro

Re: [PATCH RFC] media: staging/imx: fix complete handler

[Steve Longerbeam]

Hi Russell,


On 09/29/2017 02:38 PM, Russell King wrote:
> The complete handler walks all entities, expecting to find an imx
> subdevice for each and every entity.
>
> However, camera drivers such as smiapp can themselves contain multiple
> entities, for which there will not be an imx subdevice.  This causes
> imx_media_find_subdev_by_sd() to fail, making the imx capture system
> unusable with such cameras.
>
> Work around this by killing the error entirely, thereby allowing
> the imx capture to be used with such cameras.

Right, imx_media_add_vdev_to_pa() has followed a link to an
entity that imx is not aware of.

The only effect of this patch (besides allowing the driver to load
with smiapp cameras), is that no controls from the unknown entity
will be inherited to the capture device nodes. That's not a big deal
since the controls presumably can still be accessed from the subdev
node.

However, I still have some concerns about supporting smiapp cameras
in imx-media driver, and that is regarding pad indexes. The smiapp device
that exposes a source pad to the "outside world", which is either the binner
or the scaler entity, has a pad index of 1. But unless the device tree 
port for
the smiapp device is given a reg value of 1 for that port, imx-media
will assume it is pad 0, not 1.

I suppose 'reg = <1>;' for the smiapp source port is a workaround solution,
but I think more needs to be done to recognize smiapp in the imx-media
driver.

Can you please send output of 'media-ctrl --print-dot', I'd like to know 
what
your media graph looks like with smiapp.

Steve

>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> Not the best solution, but the only one I can think of to fix the
> regression that happened sometime between a previous revision of
> Steve's patch set and the version that got merged.
>
>   drivers/staging/media/imx/imx-media-dev.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
> index d96f4512224f..6ba59939dd7a 100644
> --- a/drivers/staging/media/imx/imx-media-dev.c
> +++ b/drivers/staging/media/imx/imx-media-dev.c
> @@ -345,8 +345,11 @@ static int imx_media_add_vdev_to_pad(struct imx_media_dev *imxmd,
>   
>   	sd = media_entity_to_v4l2_subdev(entity);
>   	imxsd = imx_media_find_subdev_by_sd(imxmd, sd);
> -	if (IS_ERR(imxsd))
> -		return PTR_ERR(imxsd);
> +	if (IS_ERR(imxsd)) {
> +		v4l2_err(&imxmd->v4l2_dev, "failed to find subdev for entity %s, sd %p err %ld\n",
> +			 entity->name, sd, PTR_ERR(imxsd));
> +		return 0;
> +	}
>   
>   	imxpad = &imxsd->pad[srcpad->index];
>   	vdev_idx = imxpad->num_vdevs;

Re: [PATCH RFC] media: staging/imx: fix complete handler

[Russell King - ARM Linux]

On Sun, Oct 01, 2017 at 01:16:53PM -0700, Steve Longerbeam wrote:
> Right, imx_media_add_vdev_to_pa() has followed a link to an
> entity that imx is not aware of.
> 
> The only effect of this patch (besides allowing the driver to load
> with smiapp cameras), is that no controls from the unknown entity
> will be inherited to the capture device nodes. That's not a big deal
> since the controls presumably can still be accessed from the subdev
> node.

smiapp is just one example I used to illustrate the problem.  The imx
media implementation must not claim subdevs exclusively for itself if
it's going to be useful, it has to cater for subdevs existing for
other hardware attached to it.

As you know, the camera that interests me is my IMX219 camera, and it's
regressed with your driver because of your insistence that you have sole
ownership over subdevs in the imx media stack - I'm having to carry more
and more hacks to workaround things that end up broken.  The imx-media
stack needs to start playing better with the needs of others, which it
can only do by allowing subdevs to be used by others.  One way to
achieve that change that results in something that works is the patch
that I've posted - and tested.

The reason that the IMX219 driver users subdevs is because it's capable
of image cropping and binning on the camera module - which helps greatly
if you want to achieve higher FPS for high speed capture [*].

It also results in all controls (which are spread over the IMX219's two
subdevs) to be visible via the v4l2 video interface - I have all the
controls attached to the pixel array subdev as well as the controls
attached to the scaling subdev.

> However, I still have some concerns about supporting smiapp cameras
> in imx-media driver, and that is regarding pad indexes. The smiapp device
> that exposes a source pad to the "outside world", which is either the binner
> or the scaler entity, has a pad index of 1. But unless the device tree port
> for the smiapp device is given a reg value of 1 for that port, imx-media
> will assume it is pad 0, not 1.

For IMX219, the source pad on the scaler (which is connected to the CSI
input pad) is pad 0 - always has been.  So I guess this problem is hidden
because of that choice.  Maybe that's a problem for someone who has a
SMIAPP camera to address.

Right now, my patch stack to get the imx219 on v4.14-rc1 working is:

media: staging/imx: fix complete handler
[media] v4l: async: don't bomb out on ->complete failure
media: imx-csi: fix burst size
media: imx: debug power on
ARM: dts: imx6qdl-hummingboard: add IMX219 camera
media: i2c: imx219 camera driver
media: imx: add frame intervals back in
fix lp-11 timeout

The frame interval patch is there because I just don't agree with the
position of the v4l2 folk, and keeping it means I don't have to screw
up my camera configuration scripts with special handling.  The
"fix lp-11 timeout" changes the LP-11 timeout to be a warning rather
than a failure - and contary to what FSL/NXP say, it works every time
on the iMX6 devices without needing to go through their workaround.


* - This is the whole reason I bought the IMX219, and have written the
IMX219 driver.  I want to use it for high speed capture of an arrow
leaving a recurve bow.  Why?  Everyone archer shoots subtly differently,
and I want to see what's happening to the arrows that are leaving my
bow.  However, for that to be achievable, I (a) need a working capture
implementation for imx6, and (b) I need to be able to quickly convert
bayer to an image, and (c) I need to either encode it on the fly, or
write the raw images to SSD.

(a) is thwarted by the breakage I keep stumbling over with the capture
code.

(b) I have using the GC320 GPU and a gstreamer plugin, trivially
converting the bayer data to grayscale.

(c) I've yet to achieve - encoding may be supported by the CODA v4l
driver, but nothing in userspace appears to support it, there's no
gstreamer v4l plugin for encoding, only one for decoding.  I also
suspect at either the 16G I have free on the SSD will get eaten up
rapidly without encoding, or the SSD may not keep up with the data
rate.

Right now, all my testing is around displaying on X:

DISPLAY=:0 gst-launch-1.0 -v v4l2src device=/dev/video9 io-mode=4 ! bayer2rgbgc ! clockoverlay halignment=2 valignment=1 ! xvimagesink

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH RFC] media: staging/imx: fix complete handler

[Steve Longerbeam]

On 10/01/2017 04:36 PM, Russell King - ARM Linux wrote:
> On Sun, Oct 01, 2017 at 01:16:53PM -0700, Steve Longerbeam wrote:
>> Right, imx_media_add_vdev_to_pa() has followed a link to an
>> entity that imx is not aware of.
>>
>> The only effect of this patch (besides allowing the driver to load
>> with smiapp cameras), is that no controls from the unknown entity
>> will be inherited to the capture device nodes. That's not a big deal
>> since the controls presumably can still be accessed from the subdev
>> node.
> smiapp is just one example I used to illustrate the problem.  The imx
> media implementation must not claim subdevs exclusively for itself if
> it's going to be useful, it has to cater for subdevs existing for
> other hardware attached to it.
>
> As you know, the camera that interests me is my IMX219 camera, and it's
> regressed with your driver because of your insistence that you have sole
> ownership over subdevs in the imx media stack

If by "sole ownership", you mean expecting async registration of subdevs
and setting up the media graph between them, imx-media will only do that
if those devices and the connections between them are described in the
device tree. If they are not, i.e. the subdevs and media pads and links are
created internally by the driver, then imx-media doesn't interfere with 
that.

But I agree your patch is a fix for a regression. It was created during some
final tweaks to the control inheritance code.


>   - I'm having to carry more
> and more hacks to workaround things that end up broken.  The imx-media
> stack needs to start playing better with the needs of others, which it
> can only do by allowing subdevs to be used by others.

Well, for example imx-media will chain s_stream until reaches your
IMX219 driver. It's then up to your driver to pass s_stream to the
subdevs that it owns.

>    One way to
> achieve that change that results in something that works is the patch
> that I've posted - and tested.

  Can you change the error message to be more descriptive, something
like "any controls for unknown subdev %s will not be inherited\n" and maybe
convert to a warn. After that I will ack it.

> The reason that the IMX219 driver users subdevs is because it's capable
> of image cropping and binning on the camera module - which helps greatly
> if you want to achieve higher FPS for high speed capture [*].
>
> It also results in all controls (which are spread over the IMX219's two
> subdevs) to be visible via the v4l2 video interface - I have all the
> controls attached to the pixel array subdev as well as the controls
> attached to the scaling subdev.
>
>> However, I still have some concerns about supporting smiapp cameras
>> in imx-media driver, and that is regarding pad indexes. The smiapp device
>> that exposes a source pad to the "outside world", which is either the binner
>> or the scaler entity, has a pad index of 1. But unless the device tree port
>> for the smiapp device is given a reg value of 1 for that port, imx-media
>> will assume it is pad 0, not 1.
> For IMX219, the source pad on the scaler (which is connected to the CSI
> input pad) is pad 0 - always has been.  So I guess this problem is hidden
> because of that choice.  Maybe that's a problem for someone who has a
> SMIAPP camera to address.

I'm working on a patch for smiapp.

Steve

>
> Right now, my patch stack to get the imx219 on v4.14-rc1 working is:
>
> media: staging/imx: fix complete handler
> [media] v4l: async: don't bomb out on ->complete failure
> media: imx-csi: fix burst size
> media: imx: debug power on
> ARM: dts: imx6qdl-hummingboard: add IMX219 camera
> media: i2c: imx219 camera driver
> media: imx: add frame intervals back in
> fix lp-11 timeout
>
> The frame interval patch is there because I just don't agree with the
> position of the v4l2 folk, and keeping it means I don't have to screw
> up my camera configuration scripts with special handling.  The
> "fix lp-11 timeout" changes the LP-11 timeout to be a warning rather
> than a failure - and contary to what FSL/NXP say, it works every time
> on the iMX6 devices without needing to go through their workaround.
>
>
> * - This is the whole reason I bought the IMX219, and have written the
> IMX219 driver.  I want to use it for high speed capture of an arrow
> leaving a recurve bow.  Why?  Everyone archer shoots subtly differently,
> and I want to see what's happening to the arrows that are leaving my
> bow.  However, for that to be achievable, I (a) need a working capture
> implementation for imx6, and (b) I need to be able to quickly convert
> bayer to an image, and (c) I need to either encode it on the fly, or
> write the raw images to SSD.
>
> (a) is thwarted by the breakage I keep stumbling over with the capture
> code.
>
> (b) I have using the GC320 GPU and a gstreamer plugin, trivially
> converting the bayer data to grayscale.
>
> (c) I've yet to achieve - encoding may be supported by the CODA v4l
> driver, but nothing in userspace appears to support it, there's no
> gstreamer v4l plugin for encoding, only one for decoding.  I also
> suspect at either the 16G I have free on the SSD will get eaten up
> rapidly without encoding, or the SSD may not keep up with the data
> rate.
>
> Right now, all my testing is around displaying on X:
>
> DISPLAY=:0 gst-launch-1.0 -v v4l2src device=/dev/video9 io-mode=4 ! bayer2rgbgc ! clockoverlay halignment=2 valignment=1 ! xvimagesink
>

Re: [PATCH RFC] media: staging/imx: fix complete handler

[Russell King - ARM Linux]

On Mon, Oct 02, 2017 at 05:59:30PM -0700, Steve Longerbeam wrote:
> 
> 
> On 10/01/2017 04:36 PM, Russell King - ARM Linux wrote:
> >On Sun, Oct 01, 2017 at 01:16:53PM -0700, Steve Longerbeam wrote:
> >>Right, imx_media_add_vdev_to_pa() has followed a link to an
> >>entity that imx is not aware of.
> >>
> >>The only effect of this patch (besides allowing the driver to load
> >>with smiapp cameras), is that no controls from the unknown entity
> >>will be inherited to the capture device nodes. That's not a big deal
> >>since the controls presumably can still be accessed from the subdev
> >>node.
> >smiapp is just one example I used to illustrate the problem.  The imx
> >media implementation must not claim subdevs exclusively for itself if
> >it's going to be useful, it has to cater for subdevs existing for
> >other hardware attached to it.
> >
> >As you know, the camera that interests me is my IMX219 camera, and it's
> >regressed with your driver because of your insistence that you have sole
> >ownership over subdevs in the imx media stack
> 
> If by "sole ownership", you mean expecting async registration of subdevs
> and setting up the media graph between them, imx-media will only do that
> if those devices and the connections between them are described in the
> device tree. If they are not, i.e. the subdevs and media pads and links are
> created internally by the driver, then imx-media doesn't interfere with
> that.

By "sole ownership" I mean that _at the moment_ imx-media believes
that it has sole right to make use of all subdevs with the exception
of one external subdev, and expects every subdev to have an imx media
subdev structure associated with it.

That's clearly true, because as soon as a multi-subdev device is
attempted to be connected to imx-media, imx-media falls apart because
it's unable to find its private imx media subdev structure for the
additional subdevs.

> >  - I'm having to carry more
> >and more hacks to workaround things that end up broken.  The imx-media
> >stack needs to start playing better with the needs of others, which it
> >can only do by allowing subdevs to be used by others.
> 
> Well, for example imx-media will chain s_stream until reaches your
> IMX219 driver. It's then up to your driver to pass s_stream to the
> subdevs that it owns.

Of course it is.  It's your responsibility to pass appropriate stuff
down the chain as far as you know how to, which is basically up to
the first external subdev facing imx-media.  What happens beyond there
is up to the external drivers.

> >   One way to
> >achieve that change that results in something that works is the patch
> >that I've posted - and tested.
> 
>  Can you change the error message to be more descriptive, something
> like "any controls for unknown subdev %s will not be inherited\n" and maybe
> convert to a warn. After that I will ack it.

No, that's plainly untrue as I said below:

> >It also results in all controls (which are spread over the IMX219's two
> >subdevs) to be visible via the v4l2 video interface - I have all the
> >controls attached to the pixel array subdev as well as the controls
> >attached to the scaling subdev.

Given that I said this, and I can prove that it does happen, I've no
idea why your reply seemed to totally ignore this paragraph.

So I refuse to add a warning message that is incorrect.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH RFC] media: staging/imx: fix complete handler

[Steve Longerbeam]

On 10/03/2017 02:06 AM, Russell King - ARM Linux wrote:
> On Mon, Oct 02, 2017 at 05:59:30PM -0700, Steve Longerbeam wrote:
>> On 10/01/2017 04:36 PM, Russell King - ARM Linux wrote:
>>> On Sun, Oct 01, 2017 at 01:16:53PM -0700, Steve Longerbeam wrote:
>>>> Right, imx_media_add_vdev_to_pa() has followed a link to an
>>>> entity that imx is not aware of.
>>>>
>>>> The only effect of this patch (besides allowing the driver to load
>>>> with smiapp cameras), is that no controls from the unknown entity
>>>> will be inherited to the capture device nodes. That's not a big deal
>>>> since the controls presumably can still be accessed from the subdev
>>>> node.
>>> smiapp is just one example I used to illustrate the problem.  The imx
>>> media implementation must not claim subdevs exclusively for itself if
>>> it's going to be useful, it has to cater for subdevs existing for
>>> other hardware attached to it.
>>>
>>> As you know, the camera that interests me is my IMX219 camera, and it's
>>> regressed with your driver because of your insistence that you have sole
>>> ownership over subdevs in the imx media stack
>> If by "sole ownership", you mean expecting async registration of subdevs
>> and setting up the media graph between them, imx-media will only do that
>> if those devices and the connections between them are described in the
>> device tree. If they are not, i.e. the subdevs and media pads and links are
>> created internally by the driver, then imx-media doesn't interfere with
>> that.
> By "sole ownership" I mean that _at the moment_ imx-media believes
> that it has sole right to make use of all subdevs with the exception
> of one external subdev, and expects every subdev to have an imx media
> subdev structure associated with it.
> That's clearly true, because as soon as a multi-subdev device is
> attempted to be connected to imx-media, imx-media falls apart because
> it's unable to find its private imx media subdev structure for the
> additional subdevs.

If imx-media finds a subdev in the device tree that is ultimately linked
to an IPU CSI port, then it needs to maintain information about that
subdev so that imx-media can field an async registration from it and setup
media links to/from it. That info is contained in struct imx_media_subdev.

Besides async registration and media link setup, I've done an audit on
the other ways struct imx_media_subdev is used.

One other usage is to locate a CSI entity in PRP entity, but CSI 
entities _must_
be known to imx-media, so no problem there.

Another usage is to locate the originating source entity (the sensor) in 
CSI entity,
to retrieve media bus config. But there will be an originating device in 
the OF
graph, and imx-media necessarily needs to know about that one.

Finally, besides the regression in imx_media_add_vdev_to_pad(), there is one
other problem usage of imx_media_find_subdev_by_sd() which I will post a 
patch
for. See below [1].


>>>   - I'm having to carry more
>>> and more hacks to workaround things that end up broken.  The imx-media
>>> stack needs to start playing better with the needs of others, which it
>>> can only do by allowing subdevs to be used by others.
>> Well, for example imx-media will chain s_stream until reaches your
>> IMX219 driver. It's then up to your driver to pass s_stream to the
>> subdevs that it owns.
> Of course it is.  It's your responsibility to pass appropriate stuff
> down the chain as far as you know how to, which is basically up to
> the first external subdev facing imx-media.  What happens beyond there
> is up to the external drivers.
>
>>>    One way to
>>> achieve that change that results in something that works is the patch
>>> that I've posted - and tested.
>>   Can you change the error message to be more descriptive, something
>> like "any controls for unknown subdev %s will not be inherited\n" and maybe
>> convert to a warn. After that I will ack it.
> No, that's plainly untrue as I said below:
>
>>> It also results in all controls (which are spread over the IMX219's two
>>> subdevs) to be visible via the v4l2 video interface - I have all the
>>> controls attached to the pixel array subdev as well as the controls
>>> attached to the scaling subdev.
> Given that I said this, and I can prove that it does happen, I've no
> idea why your reply seemed to totally ignore this paragraph.
> So I refuse to add a warning message that is incorrect.

Oops, sorry you are right. I reviewed the control inheritance code
and I've forgotten some details.

imx_media_inherit_controls() will start adding control handlers starting
from the source entity from a link_notify. As long as that source entity
is known to imx-media, imx_media_inherit_controls() moves upstream
without the need for a struct imx_media_subdev. If the source entity is
_not_ known, link_notify will fail before attempting to inherit controls 
[1].

So never mind my erroneously suggested error message, in fact I wouldn't
mind if you removed the error message altogether, or convert to debug.

Acked-by: Steve Longerbeam <steve_longerbeam@mentor.com>


[1] This is a bug, if imx_media_link_notify() gets a source subdev not know
to imx-media, it should return 0, not an error code. The link_notify op 
is only
used to inherit controls, and the controls for the unknown subdev will get
inherited later anyway, starting from a subdev that is known to imx-media.
I will post a patch to fix.


Steve