Re: [PATCH v5 00/39] i.MX Media Driver

[Nicolas Dufresne]

Le mardi 21 mars 2017 à 11:36 +, Russell King - ARM Linux a écrit :
>     warn: v4l2-test-formats.cpp(1187): S_PARM is
> supported for buftype 2, but not ENUM_FRAMEINTERVALS
>     warn: v4l2-test-formats.cpp(1194): S_PARM is
> supported but doesn't report V4L2_CAP_TIMEPERFRAME

For encoders, the framerate value is used as numerical value to
implement bitrate control. So in most cases any interval is accepted.
Though, it would be cleaner to just implement the enumeration. It's
quite simple when you support everything.

Nicolas

signature.asc
Description: This is a digitally signed message part

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Tue, Mar 21, 2017 at 11:59:02AM +0100, Hans Verkuil wrote:
> Ah, good to hear that -s works with MC. I was not sure about that.
> Thanks for the feedback!

Not soo good on iMX6:

$ v4l2-compliance -d /dev/video10 -s --expbuf-device=/dev/video0
...
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(420): G_INPUT not supported 
for a capture device
test VIDIOC_G/S/ENUMINPUT: FAIL
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 0 Audio Inputs: 0 Tuners: 0
...
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(782): subscribe event for control 
'User Controls' failed
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL
...
Streaming ioctls:
test read/write: OK (Not Supported)
fail: v4l2-test-buffers.cpp(297): g_field() == V4L2_FIELD_ANY
fail: v4l2-test-buffers.cpp(703): buf.check(q, last_seq)
fail: v4l2-test-buffers.cpp(973): captureBufs(node, q, m2m_q, 
frame_count, false)
test MMAP: FAIL
test USERPTR: OK (Not Supported)
fail: v4l2-test-buffers.cpp(1188): can_stream && ret != EINVAL
test DMABUF: FAIL

(/dev/video0 being CODA).  CODA itself seems to have failures:

Format ioctls:
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
warn: v4l2-test-formats.cpp(1187): S_PARM is supported for 
buftype 2, but not ENUM_FRAMEINTERVALS
warn: v4l2-test-formats.cpp(1194): S_PARM is supported but 
doesn't report V4L2_CAP_TIMEPERFRAME
test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
fail: v4l2-test-formats.cpp(774): fmt_out.g_colorspace() != col
...
Streaming ioctls:
test read/write: OK (Not Supported)
fail: v4l2-test-buffers.cpp(956): q.create_bufs(node, 1, ) 
!= EINVAL
test MMAP: FAIL
test USERPTR: OK (Not Supported)
test DMABUF: Cannot test, specify --expbuf-device

-- 
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 v5 00/39] i.MX Media Driver

[Hans Verkuil]

On 03/21/17 11:42, Niklas Söderlund wrote:
> On 2017-03-20 16:57:54 +0100, Hans Verkuil wrote:
>> On 03/20/2017 03:11 PM, Russell King - ARM Linux wrote:
>>> On Mon, Mar 20, 2017 at 02:57:03PM +0100, Hans Verkuil wrote:
 On 03/20/2017 02:29 PM, Russell King - ARM Linux wrote:
> It's what I have - remember, not everyone is happy to constantly replace
> their distro packages with random new stuff.

 This is a compliance test, which is continuously developed in tandem with
 new kernel versions. If you are working with an upstream kernel, then you
 should also use the corresponding v4l2-compliance test. What's the point
 of using an old one?

 I will not support driver developers that use an old version of the
 compliance test, that's a waste of my time.
>>>
>>> The reason that people may _not_ wish to constantly update v4l-utils
>>> is that it changes the libraries installed on their systems.
>>>
>>> So, the solution to that is not to complain about developers not using
>>> the latest version, but instead to de-couple it from the rest of the
>>> package, and provide it as a separate, stand-alone package that doesn't
>>> come with all the extra baggage.
>>>
>>> Now, there's two possible answers to that:
>>>
>>> 1. it depends on the libv4l2 version.  If that's so, then you are
>>>insisting that people constantly move to the latest libv4l2 because
>>>of API changes, and those API changes may upset applications they're
>>>using.  So this isn't really on.
>>>
>>> 2. it doesn't depend on libv4l2 version, in which case there's no reason
>>>for it to be packaged with v4l-utils.
>>
>> Run configure with --disable-v4l2-compliance-libv4l.
>>
>> This avoids linking with libv4l and allows you to build it stand-alone.
>>
>> Perhaps I should invert this option since in most cases you don't want to
>> run v4l2-compliance with libv4l (it's off by default unless you pass the
>> -w option to v4l2-compliance).
>>
>>>
>>> The reality is that v4l2-compliance links with libv4l2, so I'm not sure
>>> which it is.  What I am sure of is that I don't want to upgrade libv4l2
>>> on an ad-hoc basis, potentially causing issues with applications.
>>>
>> To test actual streaming you need to provide the -s option.
>>
>> Note: v4l2-compliance has been developed for 'regular' video devices,
>> not MC devices. It may or may not work with the -s option.
>
> Right, and it exists to verify that the establised v4l2 API is correctly
> implemented.  If the v4l2 API is being offered to user applications,
> then it must be conformant, otherwise it's not offering the v4l2 API.
> (That's very much a definition statement in itself.)
>
> So, are we really going to say MC devices do not offer the v4l2 API to
> userspace, but something that might work?  We've already seen today
> one user say that they're not going to use mainline because of the
> crud surrounding MC.
>

 Actually, my understanding was that he was stuck on the old kernel code.
>>>
>>> Err, sorry, I really don't follow.  Who is "he"?
>>
>> "one user say that they're not going to use mainline because of the
>> crud surrounding MC."
>>
>>>
>>> _I_ was the one who reported the EXPBUF problem.  Your comment makes no
>>> sense.
>>>
 In the case of v4l2-compliance, I never had the time to make it work with
 MC devices. Same for that matter of certain memory to memory devices.

 Just like MC devices these too behave differently. They are partially
 supported in v4l2-compliance, but not fully.
>>>
>>> It seems you saying that the API provided by /dev/video* for a MC device
>>> breaks the v4l2-compliance tests?
>>
>> There may be tests in the compliance suite that do not apply for MC devices
>> and for which I never check. The compliance suite was never written with MC
>> devices in mind, and it certainly hasn't been tested much with such devices.
>>
>> It's only very recent that I even got hardware that has MC support...
>>
>> From what I can tell from the feedback I got it seems to be OKish, but I
>> just can't guarantee that the compliance utility is correct for such devices.
>>
>> In particular I doubt the streaming tests (-s, -f, etc.) will work. The -s
>> test *might* work if the pipeline is configured correctly before running
>> v4l2-compliance. I can't imagine that the -f option would work at all since
>> I would expect pipeline validation errors.
> 
> I successfully use v4l2-compliance with the -s option to test the 
> Renesas R-Car Gen3 driver which uses MC, I first have to setup the 
> pipeline using media-ctl. I have had much use of the tool and it have 
> helped me catch many errors in the rcar-vin driver both on Gen2 (no MC 
> involved) and Gen3. And yes the -f option is only usable on Gen2 where 
> MC is not used.

Ah, good to hear that -s works with MC. I was not sure about that.
Thanks for the feedback!

Regards,

Hans

Re: [PATCH v5 00/39] i.MX Media Driver

[Niklas Söderlund]

On 2017-03-20 16:57:54 +0100, Hans Verkuil wrote:
> On 03/20/2017 03:11 PM, Russell King - ARM Linux wrote:
> > On Mon, Mar 20, 2017 at 02:57:03PM +0100, Hans Verkuil wrote:
> >> On 03/20/2017 02:29 PM, Russell King - ARM Linux wrote:
> >>> It's what I have - remember, not everyone is happy to constantly replace
> >>> their distro packages with random new stuff.
> >>
> >> This is a compliance test, which is continuously developed in tandem with
> >> new kernel versions. If you are working with an upstream kernel, then you
> >> should also use the corresponding v4l2-compliance test. What's the point
> >> of using an old one?
> >>
> >> I will not support driver developers that use an old version of the
> >> compliance test, that's a waste of my time.
> > 
> > The reason that people may _not_ wish to constantly update v4l-utils
> > is that it changes the libraries installed on their systems.
> > 
> > So, the solution to that is not to complain about developers not using
> > the latest version, but instead to de-couple it from the rest of the
> > package, and provide it as a separate, stand-alone package that doesn't
> > come with all the extra baggage.
> > 
> > Now, there's two possible answers to that:
> > 
> > 1. it depends on the libv4l2 version.  If that's so, then you are
> >insisting that people constantly move to the latest libv4l2 because
> >of API changes, and those API changes may upset applications they're
> >using.  So this isn't really on.
> > 
> > 2. it doesn't depend on libv4l2 version, in which case there's no reason
> >for it to be packaged with v4l-utils.
> 
> Run configure with --disable-v4l2-compliance-libv4l.
> 
> This avoids linking with libv4l and allows you to build it stand-alone.
> 
> Perhaps I should invert this option since in most cases you don't want to
> run v4l2-compliance with libv4l (it's off by default unless you pass the
> -w option to v4l2-compliance).
> 
> > 
> > The reality is that v4l2-compliance links with libv4l2, so I'm not sure
> > which it is.  What I am sure of is that I don't want to upgrade libv4l2
> > on an ad-hoc basis, potentially causing issues with applications.
> > 
>  To test actual streaming you need to provide the -s option.
> 
>  Note: v4l2-compliance has been developed for 'regular' video devices,
>  not MC devices. It may or may not work with the -s option.
> >>>
> >>> Right, and it exists to verify that the establised v4l2 API is correctly
> >>> implemented.  If the v4l2 API is being offered to user applications,
> >>> then it must be conformant, otherwise it's not offering the v4l2 API.
> >>> (That's very much a definition statement in itself.)
> >>>
> >>> So, are we really going to say MC devices do not offer the v4l2 API to
> >>> userspace, but something that might work?  We've already seen today
> >>> one user say that they're not going to use mainline because of the
> >>> crud surrounding MC.
> >>>
> >>
> >> Actually, my understanding was that he was stuck on the old kernel code.
> > 
> > Err, sorry, I really don't follow.  Who is "he"?
> 
> "one user say that they're not going to use mainline because of the
> crud surrounding MC."
> 
> > 
> > _I_ was the one who reported the EXPBUF problem.  Your comment makes no
> > sense.
> > 
> >> In the case of v4l2-compliance, I never had the time to make it work with
> >> MC devices. Same for that matter of certain memory to memory devices.
> >>
> >> Just like MC devices these too behave differently. They are partially
> >> supported in v4l2-compliance, but not fully.
> > 
> > It seems you saying that the API provided by /dev/video* for a MC device
> > breaks the v4l2-compliance tests?
> 
> There may be tests in the compliance suite that do not apply for MC devices
> and for which I never check. The compliance suite was never written with MC
> devices in mind, and it certainly hasn't been tested much with such devices.
> 
> It's only very recent that I even got hardware that has MC support...
> 
> From what I can tell from the feedback I got it seems to be OKish, but I
> just can't guarantee that the compliance utility is correct for such devices.
> 
> In particular I doubt the streaming tests (-s, -f, etc.) will work. The -s
> test *might* work if the pipeline is configured correctly before running
> v4l2-compliance. I can't imagine that the -f option would work at all since
> I would expect pipeline validation errors.

I successfully use v4l2-compliance with the -s option to test the 
Renesas R-Car Gen3 driver which uses MC, I first have to setup the 
pipeline using media-ctl. I have had much use of the tool and it have 
helped me catch many errors in the rcar-vin driver both on Gen2 (no MC 
involved) and Gen3. And yes the -f option is only usable on Gen2 where 
MC is not used.

For what it's worth, the first versions of the R-Car Gen3 patches did 
not use MC for anything else then setting up the pipeline, all format 
propagation and communication with subdevice where 

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 05:29:07PM +0100, Philipp Zabel wrote:
> According to the documentation [1], you are doing the right thing:
> 
> The struct v4l2_subdev_frame_interval pad references a non-existing
> pad, or the pad doesn’t support frame intervals.
> 
> But v4l2_subdev_call returns -ENOIOCTLCMD if the g_frame_interval op is
> not implemented at all, which is turned into -ENOTTY by video_usercopy.
> 
> [1] 
> https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-subdev-g-frame-interval.html#return-value

Thanks for confirming.

> > Maybe something like the following would be a better idea?
> > 
> >  utils/media-ctl/media-ctl.c | 10 +-
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c
> > index f61963a..a50a559 100644
> > --- a/utils/media-ctl/media-ctl.c
> > +++ b/utils/media-ctl/media-ctl.c
> > @@ -81,22 +81,22 @@ static void v4l2_subdev_print_format(struct 
> > media_entity *entity,
> > struct v4l2_mbus_framefmt format;
> > struct v4l2_fract interval = { 0, 0 };
> > struct v4l2_rect rect;
> > -   int ret;
> > +   int ret, err_fi;
> >  
> > ret = v4l2_subdev_get_format(entity, , pad, which);
> > if (ret != 0)
> > return;
> >  
> > -   ret = v4l2_subdev_get_frame_interval(entity, , pad);
> > -   if (ret != 0 && ret != -ENOTTY)
> > -   return;
> > +   err_fi = v4l2_subdev_get_frame_interval(entity, , pad);
> 
> Not supporting frame intervals doesn't warrant a visible error message,
> I think -EINVAL should also be ignored above, if the spec is to be
> believed.
> 
> >  
> > printf("\t\t[fmt:%s/%ux%u",
> >v4l2_subdev_pixelcode_to_string(format.code),
> >format.width, format.height);
> >  
> > -   if (interval.numerator || interval.denominator)
> > +   if (err_fi == 0 && (interval.numerator || interval.denominator))
> > printf("@%u/%u", interval.numerator, interval.denominator);
> > +   else if (err_fi != -ENOTTY)
> > +   printf("@", strerror(-err_fi));
> 
> Or here.

I don't mind which - I could change this to:

else if (err_fi != -ENOTTY && err_fi != -EINVAL)

Or an alternative would be to print an error (ignoring ENOTTY and EINVAL)
to stderr at the "v4l2_subdev_get_frame_interval" callsite and continue
on (ensuring that interval is zeroed).

-- 
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 v5 00/39] i.MX Media Driver

[Philipp Zabel]

On Mon, 2017-03-20 at 15:43 +, Russell King - ARM Linux wrote:
> On Mon, Mar 20, 2017 at 02:20:16PM +0100, Philipp Zabel wrote:
> > To set and read colorimetry information:
> > https://patchwork.linuxtv.org/patch/39350/
> 
> Thanks, I've applied all four of your patches, but there's a side effect
> from that.  Old media-ctl (modified by me):
> 
> - entity 53: imx219 0-0010 (2 pads, 2 links)
>  type V4L2 subdev subtype Unknown flags 0
>  device node name /dev/v4l-subdev9
> pad0: Source
> [fmt:SRGGB8/816x616 field:none
>  frame_interval:1/25]
> -> "imx6-mipi-csi2":0 [ENABLED]
> pad1: Sink
> [fmt:SRGGB10/3280x2464 field:none
>  crop.bounds:(0,0)/3280x2464
>  crop:(0,0)/3264x2464
>  compose.bounds:(0,0)/3264x2464
>  compose:(0,0)/816x616]
> <- "imx219 pixel 0-0010":0 [ENABLED,IMMUTABLE]
> 
> New media-ctl:
> 
> - entity 53: imx219 0-0010 (2 pads, 2 links)
>  type V4L2 subdev subtype Unknown flags 0
>  device node name /dev/v4l-subdev9
> pad0: Source
> [fmt:SRGGB8_1X8/816x616@1/25 field:none colorspace:srgb 
> xfer:srgb]
> -> "imx6-mipi-csi2":0 [ENABLED]
> pad1: Sink
> <- "imx219 pixel 0-0010":0 [ENABLED,IMMUTABLE]
> 
> It looks like we successfully retrieve the frame interval for pad 0
> and print it, but when we try to retrieve the frame interval for pad 1,
> we get EINVAL (because that's what I'm returning, but I'm wondering if
> that's the correct thing to do...) and that prevents _all_ format
> information being output.

According to the documentation [1], you are doing the right thing:

The struct v4l2_subdev_frame_interval pad references a non-existing
pad, or the pad doesn’t support frame intervals.

But v4l2_subdev_call returns -ENOIOCTLCMD if the g_frame_interval op is
not implemented at all, which is turned into -ENOTTY by video_usercopy.

[1] 
https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-subdev-g-frame-interval.html#return-value

> Maybe something like the following would be a better idea?
> 
>  utils/media-ctl/media-ctl.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c
> index f61963a..a50a559 100644
> --- a/utils/media-ctl/media-ctl.c
> +++ b/utils/media-ctl/media-ctl.c
> @@ -81,22 +81,22 @@ static void v4l2_subdev_print_format(struct media_entity 
> *entity,
>   struct v4l2_mbus_framefmt format;
>   struct v4l2_fract interval = { 0, 0 };
>   struct v4l2_rect rect;
> - int ret;
> + int ret, err_fi;
>  
>   ret = v4l2_subdev_get_format(entity, , pad, which);
>   if (ret != 0)
>   return;
>  
> - ret = v4l2_subdev_get_frame_interval(entity, , pad);
> - if (ret != 0 && ret != -ENOTTY)
> - return;
> + err_fi = v4l2_subdev_get_frame_interval(entity, , pad);

Not supporting frame intervals doesn't warrant a visible error message,
I think -EINVAL should also be ignored above, if the spec is to be
believed.

>  
>   printf("\t\t[fmt:%s/%ux%u",
>  v4l2_subdev_pixelcode_to_string(format.code),
>  format.width, format.height);
>  
> - if (interval.numerator || interval.denominator)
> + if (err_fi == 0 && (interval.numerator || interval.denominator))
>   printf("@%u/%u", interval.numerator, interval.denominator);
> + else if (err_fi != -ENOTTY)
> + printf("@", strerror(-err_fi));

Or here.

>  
>   if (format.field)
>   printf(" field:%s", v4l2_subdev_field_to_string(format.field));
> 
> 

regards
Philipp

Re: [PATCH v5 00/39] i.MX Media Driver

[Hans Verkuil]

On 03/20/2017 03:11 PM, Russell King - ARM Linux wrote:
> On Mon, Mar 20, 2017 at 02:57:03PM +0100, Hans Verkuil wrote:
>> On 03/20/2017 02:29 PM, Russell King - ARM Linux wrote:
>>> It's what I have - remember, not everyone is happy to constantly replace
>>> their distro packages with random new stuff.
>>
>> This is a compliance test, which is continuously developed in tandem with
>> new kernel versions. If you are working with an upstream kernel, then you
>> should also use the corresponding v4l2-compliance test. What's the point
>> of using an old one?
>>
>> I will not support driver developers that use an old version of the
>> compliance test, that's a waste of my time.
> 
> The reason that people may _not_ wish to constantly update v4l-utils
> is that it changes the libraries installed on their systems.
> 
> So, the solution to that is not to complain about developers not using
> the latest version, but instead to de-couple it from the rest of the
> package, and provide it as a separate, stand-alone package that doesn't
> come with all the extra baggage.
> 
> Now, there's two possible answers to that:
> 
> 1. it depends on the libv4l2 version.  If that's so, then you are
>insisting that people constantly move to the latest libv4l2 because
>of API changes, and those API changes may upset applications they're
>using.  So this isn't really on.
> 
> 2. it doesn't depend on libv4l2 version, in which case there's no reason
>for it to be packaged with v4l-utils.

Run configure with --disable-v4l2-compliance-libv4l.

This avoids linking with libv4l and allows you to build it stand-alone.

Perhaps I should invert this option since in most cases you don't want to
run v4l2-compliance with libv4l (it's off by default unless you pass the
-w option to v4l2-compliance).

> 
> The reality is that v4l2-compliance links with libv4l2, so I'm not sure
> which it is.  What I am sure of is that I don't want to upgrade libv4l2
> on an ad-hoc basis, potentially causing issues with applications.
> 
 To test actual streaming you need to provide the -s option.

 Note: v4l2-compliance has been developed for 'regular' video devices,
 not MC devices. It may or may not work with the -s option.
>>>
>>> Right, and it exists to verify that the establised v4l2 API is correctly
>>> implemented.  If the v4l2 API is being offered to user applications,
>>> then it must be conformant, otherwise it's not offering the v4l2 API.
>>> (That's very much a definition statement in itself.)
>>>
>>> So, are we really going to say MC devices do not offer the v4l2 API to
>>> userspace, but something that might work?  We've already seen today
>>> one user say that they're not going to use mainline because of the
>>> crud surrounding MC.
>>>
>>
>> Actually, my understanding was that he was stuck on the old kernel code.
> 
> Err, sorry, I really don't follow.  Who is "he"?

"one user say that they're not going to use mainline because of the
crud surrounding MC."

> 
> _I_ was the one who reported the EXPBUF problem.  Your comment makes no
> sense.
> 
>> In the case of v4l2-compliance, I never had the time to make it work with
>> MC devices. Same for that matter of certain memory to memory devices.
>>
>> Just like MC devices these too behave differently. They are partially
>> supported in v4l2-compliance, but not fully.
> 
> It seems you saying that the API provided by /dev/video* for a MC device
> breaks the v4l2-compliance tests?

There may be tests in the compliance suite that do not apply for MC devices
and for which I never check. The compliance suite was never written with MC
devices in mind, and it certainly hasn't been tested much with such devices.

It's only very recent that I even got hardware that has MC support...

>From what I can tell from the feedback I got it seems to be OKish, but I
just can't guarantee that the compliance utility is correct for such devices.

In particular I doubt the streaming tests (-s, -f, etc.) will work. The -s
test *might* work if the pipeline is configured correctly before running
v4l2-compliance. I can't imagine that the -f option would work at all since
I would expect pipeline validation errors.

I've been gently pushing Helen Koike to finish her virtual MC driver
(https://patchwork.kernel.org/patch/9312783/) since having a virtual driver
makes writing compliance tests much easier.

> _No one_ has mentioned using v4l2-compliance on the subdevs.
> 
>> Complaining about this really won't help. We know it's a problem and unless
>> someone (me perhaps?) manages to get paid to work on this it's unlikely to
>> change for now.
> 
> Like the above comment, your comment makes no sense.  I'm not complaining,
> I'm trying to find out the details.

Must be me then, it did feel like complaining...

> Yes, MC stuff sucks big time right now, the documentation is poor, there's
> a lack of understanding on all sides of the issues (which can be seen by
> the different opinions that people hold.)  

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 02:20:16PM +0100, Philipp Zabel wrote:
> To set and read colorimetry information:
> https://patchwork.linuxtv.org/patch/39350/

Thanks, I've applied all four of your patches, but there's a side effect
from that.  Old media-ctl (modified by me):

- entity 53: imx219 0-0010 (2 pads, 2 links)
 type V4L2 subdev subtype Unknown flags 0
 device node name /dev/v4l-subdev9
pad0: Source
[fmt:SRGGB8/816x616 field:none
 frame_interval:1/25]
-> "imx6-mipi-csi2":0 [ENABLED]
pad1: Sink
[fmt:SRGGB10/3280x2464 field:none
 crop.bounds:(0,0)/3280x2464
 crop:(0,0)/3264x2464
 compose.bounds:(0,0)/3264x2464
 compose:(0,0)/816x616]
<- "imx219 pixel 0-0010":0 [ENABLED,IMMUTABLE]

New media-ctl:

- entity 53: imx219 0-0010 (2 pads, 2 links)
 type V4L2 subdev subtype Unknown flags 0
 device node name /dev/v4l-subdev9
pad0: Source
[fmt:SRGGB8_1X8/816x616@1/25 field:none colorspace:srgb 
xfer:srgb]
-> "imx6-mipi-csi2":0 [ENABLED]
pad1: Sink
<- "imx219 pixel 0-0010":0 [ENABLED,IMMUTABLE]

It looks like we successfully retrieve the frame interval for pad 0
and print it, but when we try to retrieve the frame interval for pad 1,
we get EINVAL (because that's what I'm returning, but I'm wondering if
that's the correct thing to do...) and that prevents _all_ format
information being output.

Maybe something like the following would be a better idea?

 utils/media-ctl/media-ctl.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c
index f61963a..a50a559 100644
--- a/utils/media-ctl/media-ctl.c
+++ b/utils/media-ctl/media-ctl.c
@@ -81,22 +81,22 @@ static void v4l2_subdev_print_format(struct media_entity 
*entity,
struct v4l2_mbus_framefmt format;
struct v4l2_fract interval = { 0, 0 };
struct v4l2_rect rect;
-   int ret;
+   int ret, err_fi;
 
ret = v4l2_subdev_get_format(entity, , pad, which);
if (ret != 0)
return;
 
-   ret = v4l2_subdev_get_frame_interval(entity, , pad);
-   if (ret != 0 && ret != -ENOTTY)
-   return;
+   err_fi = v4l2_subdev_get_frame_interval(entity, , pad);
 
printf("\t\t[fmt:%s/%ux%u",
   v4l2_subdev_pixelcode_to_string(format.code),
   format.width, format.height);
 
-   if (interval.numerator || interval.denominator)
+   if (err_fi == 0 && (interval.numerator || interval.denominator))
printf("@%u/%u", interval.numerator, interval.denominator);
+   else if (err_fi != -ENOTTY)
+   printf("@", strerror(-err_fi));
 
if (format.field)
printf(" field:%s", v4l2_subdev_field_to_string(format.field));


-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 02:57:03PM +0100, Hans Verkuil wrote:
> On 03/20/2017 02:29 PM, Russell King - ARM Linux wrote:
> > It's what I have - remember, not everyone is happy to constantly replace
> > their distro packages with random new stuff.
> 
> This is a compliance test, which is continuously developed in tandem with
> new kernel versions. If you are working with an upstream kernel, then you
> should also use the corresponding v4l2-compliance test. What's the point
> of using an old one?
> 
> I will not support driver developers that use an old version of the
> compliance test, that's a waste of my time.

The reason that people may _not_ wish to constantly update v4l-utils
is that it changes the libraries installed on their systems.

So, the solution to that is not to complain about developers not using
the latest version, but instead to de-couple it from the rest of the
package, and provide it as a separate, stand-alone package that doesn't
come with all the extra baggage.

Now, there's two possible answers to that:

1. it depends on the libv4l2 version.  If that's so, then you are
   insisting that people constantly move to the latest libv4l2 because
   of API changes, and those API changes may upset applications they're
   using.  So this isn't really on.

2. it doesn't depend on libv4l2 version, in which case there's no reason
   for it to be packaged with v4l-utils.

The reality is that v4l2-compliance links with libv4l2, so I'm not sure
which it is.  What I am sure of is that I don't want to upgrade libv4l2
on an ad-hoc basis, potentially causing issues with applications.

> >> To test actual streaming you need to provide the -s option.
> >>
> >> Note: v4l2-compliance has been developed for 'regular' video devices,
> >> not MC devices. It may or may not work with the -s option.
> > 
> > Right, and it exists to verify that the establised v4l2 API is correctly
> > implemented.  If the v4l2 API is being offered to user applications,
> > then it must be conformant, otherwise it's not offering the v4l2 API.
> > (That's very much a definition statement in itself.)
> > 
> > So, are we really going to say MC devices do not offer the v4l2 API to
> > userspace, but something that might work?  We've already seen today
> > one user say that they're not going to use mainline because of the
> > crud surrounding MC.
> > 
> 
> Actually, my understanding was that he was stuck on the old kernel code.

Err, sorry, I really don't follow.  Who is "he"?

_I_ was the one who reported the EXPBUF problem.  Your comment makes no
sense.

> In the case of v4l2-compliance, I never had the time to make it work with
> MC devices. Same for that matter of certain memory to memory devices.
> 
> Just like MC devices these too behave differently. They are partially
> supported in v4l2-compliance, but not fully.

It seems you saying that the API provided by /dev/video* for a MC device
breaks the v4l2-compliance tests?

_No one_ has mentioned using v4l2-compliance on the subdevs.

> Complaining about this really won't help. We know it's a problem and unless
> someone (me perhaps?) manages to get paid to work on this it's unlikely to
> change for now.

Like the above comment, your comment makes no sense.  I'm not complaining,
I'm trying to find out the details.

Yes, MC stuff sucks big time right now, the documentation is poor, there's
a lack of understanding on all sides of the issues (which can be seen by
the different opinions that people hold.)  The only way to resolve these
differences is via discussion, and if you're going to start thinking that
everyone is complaining, then there's not going to be any forward progress.

-- 
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 v5 00/39] i.MX Media Driver

[Hans Verkuil]

On 03/20/2017 02:29 PM, Russell King - ARM Linux wrote:
> On Mon, Mar 20, 2017 at 02:01:58PM +0100, Hans Verkuil wrote:
>> On 03/19/2017 06:54 PM, Steve Longerbeam wrote:
>>>
>>>
>>> On 03/19/2017 03:38 AM, Russell King - ARM Linux wrote:
 What did you do with:

 ioctl(3, VIDIOC_REQBUFS, {count=0, type=0 /* V4L2_BUF_TYPE_??? */, 
 memory=0 /* V4L2_MEMORY_??? */}) = -1 EINVAL (Invalid argument)
  test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
 ioctl(3, VIDIOC_EXPBUF, 0xbef405bc) = -1 EINVAL (Invalid argument)
  fail: v4l2-test-buffers.cpp(571): q.has_expbuf(node)
>>
>> This is really a knock-on effect from an earlier issue where the compliance 
>> test
>> didn't detect support for MEMORY_MMAP.
> 
> So why does it succeed when I fix the compliance errors with VIDIOC_G_FMT?
> With that fixed, I now get:
> 
> Format ioctls:
> test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> test VIDIOC_G/S_PARM: OK
> test VIDIOC_G_FBUF: OK (Not Supported)
> test VIDIOC_G_FMT: OK
> test VIDIOC_TRY_FMT: OK
> test VIDIOC_S_FMT: OK
> test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> test Cropping: OK (Not Supported)
> test Composing: OK (Not Supported)
> test Scaling: OK (Not Supported)
> 
> Buffer ioctls:
> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> test VIDIOC_EXPBUF: OK
> 
> The reason is, if you look at the code, VIDIOC_G_FMT populates a list
> of possible buffer formats "node->valid_buftypes".  If the VIDIOC_G_FMT
> test fails, then node->valid_buftypes is zero.
> 
> This causes testReqBufs() to only check for the all-zeroed VIDIOC_REQBUFS
> and declare it conformant, without creating any buffers (it can't, it
> doesn't know which formats are supported.)
> 
> This causes node->valid_memorytype to be zero.

It should fail on this and return a more understandable error message.

> 
> We then go on to testExpBuf(), and valid_memorytype zero, claiming (falsely)
> that MMAP is not supported.  The reality is that it _is_ supported, but
> it's just the non-compliant VICIOC_G_FMT call (due to the colorspace
> issue) causes the sequence of tests to fail.

Yeah, you're not the first to complain about this. I plan on fixing this this
week.

> 
>> Always build from the master repo. 1.10 is pretty old.
> 
> It's what I have - remember, not everyone is happy to constantly replace
> their distro packages with random new stuff.

This is a compliance test, which is continuously developed in tandem with
new kernel versions. If you are working with an upstream kernel, then you
should also use the corresponding v4l2-compliance test. What's the point
of using an old one?

I will not support driver developers that use an old version of the
compliance test, that's a waste of my time.

> 
 In any case, it doesn't look like the buffer management is being
 tested at all by v4l2-compliance - we know that gstreamer works, so
 buffers _can_ be allocated, and I've also used dmabufs with gstreamer,
 so I also know that VIDIOC_EXPBUF works there.
>>
>> To test actual streaming you need to provide the -s option.
>>
>> Note: v4l2-compliance has been developed for 'regular' video devices,
>> not MC devices. It may or may not work with the -s option.
> 
> Right, and it exists to verify that the establised v4l2 API is correctly
> implemented.  If the v4l2 API is being offered to user applications,
> then it must be conformant, otherwise it's not offering the v4l2 API.
> (That's very much a definition statement in itself.)
> 
> So, are we really going to say MC devices do not offer the v4l2 API to
> userspace, but something that might work?  We've already seen today
> one user say that they're not going to use mainline because of the
> crud surrounding MC.
> 

Actually, my understanding was that he was stuck on the old kernel code.

In the case of v4l2-compliance, I never had the time to make it work with
MC devices. Same for that matter of certain memory to memory devices.

Just like MC devices these too behave differently. They are partially
supported in v4l2-compliance, but not fully.

Why? NO TIME.

Be glad there *is* a v4l2-compliance test at all! It's really, really useful
already, but it took *years* to develop, little bit by little bit. And yes,
I would really like to update it to fully support codecs and MC devices.
And with a bit of luck I hope to get permission from my boss to work on
this (among others) later in the year.

Complaining about this really won't help. We know it's a problem and unless
someone (me perhaps?) manages to get paid to work on this it's unlikely to
change for now.

Regards,

Hans

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Mon, Mar 20, 2017 at 02:01:58PM +0100, Hans Verkuil wrote:
> On 03/19/2017 06:54 PM, Steve Longerbeam wrote:
> > 
> > 
> > On 03/19/2017 03:38 AM, Russell King - ARM Linux wrote:
> >> What did you do with:
> >>
> >> ioctl(3, VIDIOC_REQBUFS, {count=0, type=0 /* V4L2_BUF_TYPE_??? */, 
> >> memory=0 /* V4L2_MEMORY_??? */}) = -1 EINVAL (Invalid argument)
> >>  test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> >> ioctl(3, VIDIOC_EXPBUF, 0xbef405bc) = -1 EINVAL (Invalid argument)
> >>  fail: v4l2-test-buffers.cpp(571): q.has_expbuf(node)
> 
> This is really a knock-on effect from an earlier issue where the compliance 
> test
> didn't detect support for MEMORY_MMAP.

So why does it succeed when I fix the compliance errors with VIDIOC_G_FMT?
With that fixed, I now get:

Format ioctls:
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
test Cropping: OK (Not Supported)
test Composing: OK (Not Supported)
test Scaling: OK (Not Supported)

Buffer ioctls:
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK

The reason is, if you look at the code, VIDIOC_G_FMT populates a list
of possible buffer formats "node->valid_buftypes".  If the VIDIOC_G_FMT
test fails, then node->valid_buftypes is zero.

This causes testReqBufs() to only check for the all-zeroed VIDIOC_REQBUFS
and declare it conformant, without creating any buffers (it can't, it
doesn't know which formats are supported.)

This causes node->valid_memorytype to be zero.

We then go on to testExpBuf(), and valid_memorytype zero, claiming (falsely)
that MMAP is not supported.  The reality is that it _is_ supported, but
it's just the non-compliant VICIOC_G_FMT call (due to the colorspace
issue) causes the sequence of tests to fail.

> Always build from the master repo. 1.10 is pretty old.

It's what I have - remember, not everyone is happy to constantly replace
their distro packages with random new stuff.

> >> In any case, it doesn't look like the buffer management is being
> >> tested at all by v4l2-compliance - we know that gstreamer works, so
> >> buffers _can_ be allocated, and I've also used dmabufs with gstreamer,
> >> so I also know that VIDIOC_EXPBUF works there.
> 
> To test actual streaming you need to provide the -s option.
> 
> Note: v4l2-compliance has been developed for 'regular' video devices,
> not MC devices. It may or may not work with the -s option.

Right, and it exists to verify that the establised v4l2 API is correctly
implemented.  If the v4l2 API is being offered to user applications,
then it must be conformant, otherwise it's not offering the v4l2 API.
(That's very much a definition statement in itself.)

So, are we really going to say MC devices do not offer the v4l2 API to
userspace, but something that might work?  We've already seen today
one user say that they're not going to use mainline because of the
crud surrounding MC.

-- 
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 v5 00/39] i.MX Media Driver

[Philipp Zabel]

On Sun, 2017-03-19 at 12:14 +, Russell King - ARM Linux wrote:
> On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> > On 03/18/2017 12:22 PM, Russell King - ARM Linux wrote:
> > >0:00:01.955927879 20954  0x15ffe90 INFOv4l2 
> > >gstv4l2object.c:3811:gst_v4l2_object_get_caps: probed caps: 
> > >video/x-bayer, format=(string)rggb, framerate=(fraction)3/1001, 
> > >width=(int)816, height=(int)616, pixel-aspect-ratio=(fraction)1/1; 
> > >video/x-raw, format=(string)I420, framerate=(fraction)3/1001, 
> > >width=(int)816, height=(int)616, interlace-mode=(string)progressive, 
> > >pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)YV12, 
> > >framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
> > >interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; 
> > >video/x-raw, format=(string)BGR, framerate=(fraction)3/1001, 
> > >width=(int)816, height=(int)616, interlace-mode=(string)progressive, 
> > >pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)RGB, 
> > >framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
> > >interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1
> > >
> > >despite the media pipeline actually being configured for 60fps.
> > >
> > >Forcing it by adjusting the pipeline only results in gstreamer
> > >failing, because it believes that v4l2 is unable to operate at
> > >60fps.
> > >
> > >Also note the complaints from v4l2src about the non-compliance...
> > 
> > Thanks, I've fixed most of v4l2-compliance issues, but this is not
> > done yet. Is that something you can help with?
> 
> I've looked at this, and IMHO it's yet another media control API mess.
> 
> - media-ctl itself allows setting the format on subdev pads via
>   struct v4l2_subdev_format.
> 
> - struct v4l2_subdev_format contains a struct v4l2_mbus_framefmt.
> 
> - struct v4l2_mbus_framefmt contains:
>   * @width:  frame width
>   * @height: frame height
>   * @code:   data format code (from enum v4l2_mbus_pixelcode)
>   * @field:  used interlacing type (from enum v4l2_field)
>   * @colorspace: colorspace of the data (from enum v4l2_colorspace)
>   * @ycbcr_enc:  YCbCr encoding of the data (from enum v4l2_ycbcr_encoding)
>   * @quantization: quantization of the data (from enum v4l2_quantization)
>   * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func)
> 
> - media-ctl sets width, height, code and field, but nothing else.
> 
> We're already agreed that the fields that media-ctl are part of the
> format negotiation between the ultimate source, flowing down to the
> capture device.  However, there's no support in media-ctl to deal
> with these other fields - so media-ctl in itself is only half-
> implemented.

To set and read colorimetry information:
https://patchwork.linuxtv.org/patch/39350/

regards
Philipp

Re: [PATCH v5 00/39] i.MX Media Driver

[Philipp Zabel]

On Sat, 2017-03-18 at 12:58 -0700, Steve Longerbeam wrote:
> 
> On 03/18/2017 12:22 PM, Russell King - ARM Linux wrote:
> > Hi Steve,
> >
> > I've just been trying to get gstreamer to capture and h264 encode
> > video from my camera at various frame rates, and what I've discovered
> > does not look good.
> >
> > 1) when setting frame rates, media-ctl _always_ calls
> > VIDIOC_SUBDEV_S_FRAME_INTERVAL with pad=0.

To allow setting pad > 0:
https://patchwork.linuxtv.org/patch/39348/

> > 2) media-ctl never retrieves the frame interval information, so there's
> > no way to read it back with standard tools, and no indication that
> > this is going on...
> 
> I think Philipp Zabel submitted a patch which addresses these
> in media-ctl. Check with him.

To read back and propagate the frame interval:
https://patchwork.linuxtv.org/patch/39349/
https://patchwork.linuxtv.org/patch/39351/

regards
Philipp

Re: [PATCH v5 00/39] i.MX Media Driver

[Hans Verkuil]

On 03/19/2017 06:54 PM, Steve Longerbeam wrote:
> 
> 
> On 03/19/2017 03:38 AM, Russell King - ARM Linux wrote:
>> On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
>>> Right, imx-media-capture.c (the "standard" v4l2 user interface module)
>>> is not implementing VIDIOC_ENUM_FRAMESIZES. It should, but it can only
>>> return the single frame size that the pipeline has configured (the mbus
>>> format of the attached source pad).
>> I now have a set of patches that enumerate the frame sizes and intervals
>> from the source pad of the first subdev (since you're setting the formats
>> etc there from the capture device, it seems sensible to return what it
>> can support.)  This means my patch set doesn't add to non-CSI subdevs.
>>
>>> Can you share your gstreamer pipeline? For now, until
>>> VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
>>> does not attempt to specify a frame rate. I use the attached
>>> script for testing, which works for me.
>> Note that I'm not specifying a frame rate on gstreamer - I'm setting
>> the pipeline up for 60fps, but gstreamer in its wisdom is unable to
>> enumerate the frame sizes, and therefore is unable to enumerate the
>> frame intervals (frame intervals depend on frame sizes), so it
>> falls back to the "tvnorms" which are basically 25/1 and 3/1001.
>>
>> It sees 60fps via G_PARM, and then decides to set 3/1001 via S_PARM.
>> So, we end up with most of the pipeline operating at 60fps, with CSI
>> doing frame skipping to reduce the frame rate to 30fps.
>>
>> gstreamer doesn't complain, doesn't issue any warnings, the only way
>> you can spot this is to enable debugging and look through the copious
>> debug log, or use -v and check the pad capabilities.
>>
>> Testing using gstreamer, and only using "does it produce video" is a
>> good simple test, but it's just that - it's a simple test.  It doesn't
>> tell you that what you're seeing is what you intended to see (such as
>> video at the frame rate you expected) without more work.
>>
>>> Thanks, I've fixed most of v4l2-compliance issues, but this is not
>>> done yet. Is that something you can help with?
>> What did you do with:
>>
>> ioctl(3, VIDIOC_REQBUFS, {count=0, type=0 /* V4L2_BUF_TYPE_??? */, memory=0 
>> /* V4L2_MEMORY_??? */}) = -1 EINVAL (Invalid argument)
>>  test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
>> ioctl(3, VIDIOC_EXPBUF, 0xbef405bc) = -1 EINVAL (Invalid argument)
>>  fail: v4l2-test-buffers.cpp(571): q.has_expbuf(node)

This is really a knock-on effect from an earlier issue where the compliance test
didn't detect support for MEMORY_MMAP.

>>  test VIDIOC_EXPBUF: FAIL
>>
>> To me, this looks like a bug in v4l2-compliance (I'm using 1.10.0).

Always build from the master repo. 1.10 is pretty old.

>> I'm not sure what buffer VIDIOC_EXPBUF is expected to export, since
>> afaics no buffers have been allocated, so of course it's going to fail.

It just tests if EXPBUF is supported.

I think I will modify v4l2-compliance to bail out if it doesn't find support
for MEMORY_MMAP. Even though in theory support for this is optional, in practice
all applications expect that it is supported. That should fix this
hard-to-understand error.

>> Either that, or the v4l2 core vb2 code is non-compliant with v4l2's
>> interface requirements.
>>
>> In any case, it doesn't look like the buffer management is being
>> tested at all by v4l2-compliance - we know that gstreamer works, so
>> buffers _can_ be allocated, and I've also used dmabufs with gstreamer,
>> so I also know that VIDIOC_EXPBUF works there.

To test actual streaming you need to provide the -s option.

Note: v4l2-compliance has been developed for 'regular' video devices,
not MC devices. It may or may not work with the -s option.

As I think I mentioned somewhere else, creating a compliance test for
MC devices would help enormously in verifying drivers. I'm not sure if
it is better to create a new test or integrate it in v4l2-compliance.

I'm leaning towards the latter since there is a lot of overlap.

>>
> 
> I wouldn't be surprised if you hit on a bug in v4l2-compliance. I 
> stopped with v4l2-compliance
> at a different test failure that also didn't make sense to me:
> 
> Streaming ioctls:
>  test read/write: OK (Not Supported)
>  Video Capture:
>  Buffer: 0 Sequence: 0 Field: Any Timestamp: 41.664259s
>  fail: 
> .../v4l-utils-1.6.2/utils/v4l2-compliance/v4l2-test-buffers.cpp(281): 
> !(g_flags() & (V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_ERROR))
>  fail: 
> .../v4l-utils-1.6.2/utils/v4l2-compliance/v4l2-test-buffers.cpp(610): 
> buf.check(q, last_seq)
>  fail: 
> .../v4l-utils-1.6.2/utils/v4l2-compliance/v4l2-test-buffers.cpp(883): 
> captureBufs(node, q, m2m_q, frame_count, false)
>  test MMAP: FAIL
>  test USERPTR: OK (Not Supported)
>  test DMABUF: Cannot test, specify --expbuf-device
> 
> Total: 42, Succeeded: 38, Failed: 

Re: [PATCH v5 00/39] i.MX Media Driver

[Hans Verkuil]

On 03/19/2017 01:14 PM, Russell King - ARM Linux wrote:
> On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
>> On 03/18/2017 12:22 PM, Russell King - ARM Linux wrote:
>>> 0:00:01.955927879 20954  0x15ffe90 INFOv4l2 
>>> gstv4l2object.c:3811:gst_v4l2_object_get_caps: probed caps: 
>>> video/x-bayer, format=(string)rggb, framerate=(fraction)3/1001, 
>>> width=(int)816, height=(int)616, pixel-aspect-ratio=(fraction)1/1; 
>>> video/x-raw, format=(string)I420, framerate=(fraction)3/1001, 
>>> width=(int)816, height=(int)616, interlace-mode=(string)progressive, 
>>> pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)YV12, 
>>> framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
>>> interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; 
>>> video/x-raw, format=(string)BGR, framerate=(fraction)3/1001, 
>>> width=(int)816, height=(int)616, interlace-mode=(string)progressive, 
>>> pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)RGB, 
>>> framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
>>> interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1
>>>
>>>despite the media pipeline actually being configured for 60fps.
>>>
>>>Forcing it by adjusting the pipeline only results in gstreamer
>>>failing, because it believes that v4l2 is unable to operate at
>>>60fps.
>>>
>>>Also note the complaints from v4l2src about the non-compliance...
>>
>> Thanks, I've fixed most of v4l2-compliance issues, but this is not
>> done yet. Is that something you can help with?
> 
> I've looked at this, and IMHO it's yet another media control API mess.
> 
> - media-ctl itself allows setting the format on subdev pads via
>   struct v4l2_subdev_format.
> 
> - struct v4l2_subdev_format contains a struct v4l2_mbus_framefmt.
> 
> - struct v4l2_mbus_framefmt contains:
>   * @width:  frame width
>   * @height: frame height
>   * @code:   data format code (from enum v4l2_mbus_pixelcode)
>   * @field:  used interlacing type (from enum v4l2_field)
>   * @colorspace: colorspace of the data (from enum v4l2_colorspace)
>   * @ycbcr_enc:  YCbCr encoding of the data (from enum v4l2_ycbcr_encoding)
>   * @quantization: quantization of the data (from enum v4l2_quantization)
>   * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func)
> 
> - media-ctl sets width, height, code and field, but nothing else.
> 
> We're already agreed that the fields that media-ctl are part of the
> format negotiation between the ultimate source, flowing down to the
> capture device.  However, there's no support in media-ctl to deal
> with these other fields - so media-ctl in itself is only half-
> implemented.

Correct. The colorspace et al fields are in practice unimportant for sensors.
For HDMI/DP they are very important, though.

It's the reason why nobody worked on adding support for this to media-ctl,
it's almost exclusively used with sensors. Not saying that it is right that
it hasn't been added to media-ctl, just that it never had a high enough prio.

Regards,

Hans

> 
> From what I can tell, _we_ are doing the right thing in imx-media-capture.
> 
> However, I think part of the problem is the set_fmt implementation.
> When a source pad is configured via set_fmt(), any fields that can
> not be altered (eg, because the subdev doesn't support colorspace
> conversion) need to be preserved from the subdev's sink pad.
> 
> Right now, CSI doesn't do that - it only looks at the width, height,
> code, and field.
> 
> I think we've got other bugs though that haven't been picked up by any
> review - csi_try_fmt() adjusts the format using the _current_
> configuration of the sink pad, even when using V4L2_SUBDEV_FORMAT_TRY.
> This seems wrong according to the docs: the purpose of the try
> mechanism is to be able to setup the _entire_ pipeline using the TRY
> mechanism to work out whether the configuration works, before then
> setting for real.  If we're validating the TRY formats against the
> live configuration, then we're not doing that.
> 
> There's calls for:
> 
> v4l2_subdev_get_try_format
> v4l2_subdev_get_try_crop
> v4l2_subdev_get_try_compose
> 
> to get the try configuration - we hardly make use of all of these.  I
> would suggest that we change the approach to implementing the various
> subdevs such that:
> 
> 1) like __csi_get_fmt(), we have accessors that gets a pointer to the
>correct state for the TRY/live settings.
> 
> 2) everywhere we're asked to get or set parameters that can be TRY/live,
>we use these accessors to retrieve a pointer to the correct state to
>not only read, but also modify.
> 
> 3) when we're evaluating parameters against another pad, we use these
>accessors to obtain the other pad's configuration, rather than poking
>about in the state saved in the subdev's priv-> (which is irrelevant
>for the TRY variant.)
> 
> 4) ensure that all 

Re: [PATCH v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/19/2017 05:14 AM, Russell King - ARM Linux wrote:

On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:

On 03/18/2017 12:22 PM, Russell King - ARM Linux wrote:

0:00:01.955927879 20954  0x15ffe90 INFOv4l2 
gstv4l2object.c:3811:gst_v4l2_object_get_caps: probed caps: 
video/x-bayer, format=(string)rggb, framerate=(fraction)3/1001, width=(int)816, 
height=(int)616, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)I420, 
framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, 
format=(string)YV12, framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, 
format=(string)BGR, framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, 
format=(string)RGB, framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1

despite the media pipeline actually being configured for 60fps.

Forcing it by adjusting the pipeline only results in gstreamer
failing, because it believes that v4l2 is unable to operate at
60fps.

Also note the complaints from v4l2src about the non-compliance...

Thanks, I've fixed most of v4l2-compliance issues, but this is not
done yet. Is that something you can help with?

I've looked at this, and IMHO it's yet another media control API mess.

- media-ctl itself allows setting the format on subdev pads via
   struct v4l2_subdev_format.

- struct v4l2_subdev_format contains a struct v4l2_mbus_framefmt.

- struct v4l2_mbus_framefmt contains:
   * @width:  frame width
   * @height: frame height
   * @code:   data format code (from enum v4l2_mbus_pixelcode)
   * @field:  used interlacing type (from enum v4l2_field)
   * @colorspace: colorspace of the data (from enum v4l2_colorspace)
   * @ycbcr_enc:  YCbCr encoding of the data (from enum v4l2_ycbcr_encoding)
   * @quantization: quantization of the data (from enum v4l2_quantization)
   * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func)

- media-ctl sets width, height, code and field, but nothing else.

We're already agreed that the fields that media-ctl are part of the
format negotiation between the ultimate source, flowing down to the
capture device.  However, there's no support in media-ctl to deal
with these other fields - so media-ctl in itself is only half-
implemented.

 From what I can tell, _we_ are doing the right thing in imx-media-capture.

However, I think part of the problem is the set_fmt implementation.
When a source pad is configured via set_fmt(), any fields that can
not be altered (eg, because the subdev doesn't support colorspace
conversion) need to be preserved from the subdev's sink pad.

Right now, CSI doesn't do that - it only looks at the width, height,
code, and field.


Correct, there is currently no propagation of the colorimetry
parameters (colorspace, ycbcr_enc, quantization, and xfer_func).
For the most part, those are just ignored ATM. Philipp Zabel did
do some work earlier to start propagating those, but that's still
TODO.



I think we've got other bugs though that haven't been picked up by any
review - csi_try_fmt() adjusts the format using the _current_
configuration of the sink pad, even when using V4L2_SUBDEV_FORMAT_TRY.
This seems wrong according to the docs: the purpose of the try
mechanism is to be able to setup the _entire_ pipeline using the TRY
mechanism to work out whether the configuration works, before then
setting for real.  If we're validating the TRY formats against the
live configuration, then we're not doing that.


I don't believe that is correct. csi_try_fmt() for the source pads calls
__csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which) to get
the sink format, and for the TRY trial-run from csi_set_fmt(),
sdformat->which will be set to TRY, so the returned sink format
is the TRY format.

But I haven't tested a complete pipeline configuration under the
TRY case, there still could be issues there. But I've checked the
CSI, VDIC, and PRPENCVF subdevs, and for set_fmt() trial-runs,
those should be working correctly using the TRY mechanism.



There's calls for:

v4l2_subdev_get_try_format
v4l2_subdev_get_try_crop
v4l2_subdev_get_try_compose

to get the try configuration - we hardly make use of all of these.


Not sure what you mean, the first two are currently
being used for TRY setup. And I don't think
v4l2_subdev_get_try_compose() is needed.


  I
would suggest that we change the approach to implementing the various
subdevs such that:

1) like __csi_get_fmt(), we have accessors that gets a pointer to the
correct state for the TRY/live settings.


I've verified that CSI, VDIC, and PRPENCVF subdevs do that.



2) everywhere we're asked 

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 19, 2017 at 11:37:15AM -0700, Steve Longerbeam wrote:
> On 03/19/2017 05:14 AM, Russell King - ARM Linux wrote:
> >Right now, CSI doesn't do that - it only looks at the width, height,
> >code, and field.
> 
> Correct, there is currently no propagation of the colorimetry
> parameters (colorspace, ycbcr_enc, quantization, and xfer_func).
> For the most part, those are just ignored ATM. Philipp Zabel did
> do some work earlier to start propagating those, but that's still
> TODO.


> 
> >
> >I think we've got other bugs though that haven't been picked up by any
> >review - csi_try_fmt() adjusts the format using the _current_
> >configuration of the sink pad, even when using V4L2_SUBDEV_FORMAT_TRY.
> >This seems wrong according to the docs: the purpose of the try
> >mechanism is to be able to setup the _entire_ pipeline using the TRY
> >mechanism to work out whether the configuration works, before then
> >setting for real.  If we're validating the TRY formats against the
> >live configuration, then we're not doing that.
> 
> I don't believe that is correct. csi_try_fmt() for the source pads calls
> __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which) to get
> the sink format, and for the TRY trial-run from csi_set_fmt(),
> sdformat->which will be set to TRY, so the returned sink format
> is the TRY format.

Look at csi_try_fmt() - it validates the source pad against
priv->crop, which is the actively live cropping rectangle, not the
one which has been configured for the TRY trial-run.

Also, as I mention elsewhere, I believe the way we're doing scaling
is completely wrong...

-- 
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 v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/19/2017 11:51 AM, Russell King - ARM Linux wrote:

On Sun, Mar 19, 2017 at 11:37:15AM -0700, Steve Longerbeam wrote:

On 03/19/2017 05:14 AM, Russell King - ARM Linux wrote:

Right now, CSI doesn't do that - it only looks at the width, height,
code, and field.

Correct, there is currently no propagation of the colorimetry
parameters (colorspace, ycbcr_enc, quantization, and xfer_func).
For the most part, those are just ignored ATM. Philipp Zabel did
do some work earlier to start propagating those, but that's still
TODO.



I think we've got other bugs though that haven't been picked up by any
review - csi_try_fmt() adjusts the format using the _current_
configuration of the sink pad, even when using V4L2_SUBDEV_FORMAT_TRY.
This seems wrong according to the docs: the purpose of the try
mechanism is to be able to setup the _entire_ pipeline using the TRY
mechanism to work out whether the configuration works, before then
setting for real.  If we're validating the TRY formats against the
live configuration, then we're not doing that.

I don't believe that is correct. csi_try_fmt() for the source pads calls
__csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which) to get
the sink format, and for the TRY trial-run from csi_set_fmt(),
sdformat->which will be set to TRY, so the returned sink format
is the TRY format.

Look at csi_try_fmt() - it validates the source pad against
priv->crop, which is the actively live cropping rectangle, not the
one which has been configured for the TRY trial-run.


Ah yes, crop, I missed that. Yes you are right, looks like we
need to add a __csi_get_crop().



Also, as I mention elsewhere, I believe the way we're doing scaling
is completely wrong...


You might be right there too. Initially, I had no support for the 
down-scaling
in the CSI. That was added later by Philipp, I will respond with more 
there...


Steve

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 19, 2017 at 10:54:22AM -0700, Steve Longerbeam wrote:
> 
> 
> On 03/19/2017 03:38 AM, Russell King - ARM Linux wrote:
> >On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> >>Right, imx-media-capture.c (the "standard" v4l2 user interface module)
> >>is not implementing VIDIOC_ENUM_FRAMESIZES. It should, but it can only
> >>return the single frame size that the pipeline has configured (the mbus
> >>format of the attached source pad).
> >I now have a set of patches that enumerate the frame sizes and intervals
> >from the source pad of the first subdev (since you're setting the formats
> >etc there from the capture device, it seems sensible to return what it
> >can support.)  This means my patch set doesn't add to non-CSI subdevs.
> >
> >>Can you share your gstreamer pipeline? For now, until
> >>VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
> >>does not attempt to specify a frame rate. I use the attached
> >>script for testing, which works for me.
> >Note that I'm not specifying a frame rate on gstreamer - I'm setting
> >the pipeline up for 60fps, but gstreamer in its wisdom is unable to
> >enumerate the frame sizes, and therefore is unable to enumerate the
> >frame intervals (frame intervals depend on frame sizes), so it
> >falls back to the "tvnorms" which are basically 25/1 and 3/1001.
> >
> >It sees 60fps via G_PARM, and then decides to set 3/1001 via S_PARM.
> >So, we end up with most of the pipeline operating at 60fps, with CSI
> >doing frame skipping to reduce the frame rate to 30fps.
> >
> >gstreamer doesn't complain, doesn't issue any warnings, the only way
> >you can spot this is to enable debugging and look through the copious
> >debug log, or use -v and check the pad capabilities.
> >
> >Testing using gstreamer, and only using "does it produce video" is a
> >good simple test, but it's just that - it's a simple test.  It doesn't
> >tell you that what you're seeing is what you intended to see (such as
> >video at the frame rate you expected) without more work.
> >
> >>Thanks, I've fixed most of v4l2-compliance issues, but this is not
> >>done yet. Is that something you can help with?
> >What did you do with:
> >
> >ioctl(3, VIDIOC_REQBUFS, {count=0, type=0 /* V4L2_BUF_TYPE_??? */, memory=0 
> >/* V4L2_MEMORY_??? */}) = -1 EINVAL (Invalid argument)
> > test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> >ioctl(3, VIDIOC_EXPBUF, 0xbef405bc) = -1 EINVAL (Invalid argument)
> > fail: v4l2-test-buffers.cpp(571): q.has_expbuf(node)
> > test VIDIOC_EXPBUF: FAIL
> >
> >To me, this looks like a bug in v4l2-compliance (I'm using 1.10.0).
> >I'm not sure what buffer VIDIOC_EXPBUF is expected to export, since
> >afaics no buffers have been allocated, so of course it's going to fail.
> >Either that, or the v4l2 core vb2 code is non-compliant with v4l2's
> >interface requirements.
> >
> >In any case, it doesn't look like the buffer management is being
> >tested at all by v4l2-compliance - we know that gstreamer works, so
> >buffers _can_ be allocated, and I've also used dmabufs with gstreamer,
> >so I also know that VIDIOC_EXPBUF works there.
> >
> 
> I wouldn't be surprised if you hit on a bug in v4l2-compliance. I stopped
> with v4l2-compliance
> at a different test failure that also didn't make sense to me:

It isn't - the problem is that the results are misleading.  The
VIDIOC_REQBUFS depends on the GET_FMT test succeeding, so it knows
which buffer formats are valid.

Since the GET_FMT test fails due to the colorspace issue, it decides
that it can't trust the format, so it ends up with no formats to test.
This causes the "VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF" test to pass,
but then it moves on to testing "VIDIOC_EXPBUF" with no available
buffers, which then fails.

Fixing GET_FMT (which I've done locally) to return proper colorspace
information results in GET_FMT passing, and also solves the EXPBUF
problem too.

-- 
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 v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/19/2017 03:38 AM, Russell King - ARM Linux wrote:

On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:

Right, imx-media-capture.c (the "standard" v4l2 user interface module)
is not implementing VIDIOC_ENUM_FRAMESIZES. It should, but it can only
return the single frame size that the pipeline has configured (the mbus
format of the attached source pad).

I now have a set of patches that enumerate the frame sizes and intervals
from the source pad of the first subdev (since you're setting the formats
etc there from the capture device, it seems sensible to return what it
can support.)  This means my patch set doesn't add to non-CSI subdevs.


Can you share your gstreamer pipeline? For now, until
VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
does not attempt to specify a frame rate. I use the attached
script for testing, which works for me.

Note that I'm not specifying a frame rate on gstreamer - I'm setting
the pipeline up for 60fps, but gstreamer in its wisdom is unable to
enumerate the frame sizes, and therefore is unable to enumerate the
frame intervals (frame intervals depend on frame sizes), so it
falls back to the "tvnorms" which are basically 25/1 and 3/1001.

It sees 60fps via G_PARM, and then decides to set 3/1001 via S_PARM.
So, we end up with most of the pipeline operating at 60fps, with CSI
doing frame skipping to reduce the frame rate to 30fps.

gstreamer doesn't complain, doesn't issue any warnings, the only way
you can spot this is to enable debugging and look through the copious
debug log, or use -v and check the pad capabilities.

Testing using gstreamer, and only using "does it produce video" is a
good simple test, but it's just that - it's a simple test.  It doesn't
tell you that what you're seeing is what you intended to see (such as
video at the frame rate you expected) without more work.


Thanks, I've fixed most of v4l2-compliance issues, but this is not
done yet. Is that something you can help with?

What did you do with:

ioctl(3, VIDIOC_REQBUFS, {count=0, type=0 /* V4L2_BUF_TYPE_??? */, memory=0 /* 
V4L2_MEMORY_??? */}) = -1 EINVAL (Invalid argument)
 test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
ioctl(3, VIDIOC_EXPBUF, 0xbef405bc) = -1 EINVAL (Invalid argument)
 fail: v4l2-test-buffers.cpp(571): q.has_expbuf(node)
 test VIDIOC_EXPBUF: FAIL

To me, this looks like a bug in v4l2-compliance (I'm using 1.10.0).
I'm not sure what buffer VIDIOC_EXPBUF is expected to export, since
afaics no buffers have been allocated, so of course it's going to fail.
Either that, or the v4l2 core vb2 code is non-compliant with v4l2's
interface requirements.

In any case, it doesn't look like the buffer management is being
tested at all by v4l2-compliance - we know that gstreamer works, so
buffers _can_ be allocated, and I've also used dmabufs with gstreamer,
so I also know that VIDIOC_EXPBUF works there.



I wouldn't be surprised if you hit on a bug in v4l2-compliance. I 
stopped with v4l2-compliance

at a different test failure that also didn't make sense to me:

Streaming ioctls:
test read/write: OK (Not Supported)
Video Capture:
Buffer: 0 Sequence: 0 Field: Any Timestamp: 41.664259s
fail: 
.../v4l-utils-1.6.2/utils/v4l2-compliance/v4l2-test-buffers.cpp(281): 
!(g_flags() & (V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_ERROR))
fail: 
.../v4l-utils-1.6.2/utils/v4l2-compliance/v4l2-test-buffers.cpp(610): 
buf.check(q, last_seq)
fail: 
.../v4l-utils-1.6.2/utils/v4l2-compliance/v4l2-test-buffers.cpp(883): 
captureBufs(node, q, m2m_q, frame_count, false)

test MMAP: FAIL
test USERPTR: OK (Not Supported)
test DMABUF: Cannot test, specify --expbuf-device

Total: 42, Succeeded: 38, Failed: 4, Warnings: 0


In this case the driver completed and returned only one buffer, and it set
VB2_BUF_STATE_DONE, so these test failures didn't make sense to me. I
was using version 1.6.2 at the time.

Steve

Re: [PATCH v5 00/39] i.MX Media Driver

[Nicolas Dufresne]

Le dimanche 19 mars 2017 à 00:54 +, Russell King - ARM Linux a
écrit :
> > 
> > In practice, I have the impression there is a fair reason why
> > framerate
> > enumeration isn't implemented (considering there is only 1 valid
> > rate).
> 
> That's actually completely incorrect.
> 
> With the capture device interfacing directly with CSI, it's possible
> _today_ to select:
> 
> * the CSI sink pad's resolution
> * the CSI sink pad's resolution with the width and/or height halved
> * the CSI sink pad's frame rate
> * the CSI sink pad's frame rate divided by the frame drop factor
> 
> To put it another way, these are possible:
> 
> # v4l2-ctl -d /dev/video10 --list-formats-ext
> ioctl: VIDIOC_ENUM_FMT
>     Index   : 0
>     Type    : Video Capture
>     Pixel Format: 'RGGB'
>     Name    : 8-bit Bayer RGRG/GBGB
>     Size: Discrete 816x616
>     Interval: Discrete 0.040s (25.000 fps)
>     Interval: Discrete 0.048s (20.833 fps)
>     Interval: Discrete 0.050s (20.000 fps)
>     Interval: Discrete 0.053s (18.750 fps)
>     Interval: Discrete 0.060s (16.667 fps)
>     Interval: Discrete 0.067s (15.000 fps)
>     Interval: Discrete 0.080s (12.500 fps)
>     Interval: Discrete 0.100s (10.000 fps)
>     Interval: Discrete 0.120s (8.333 fps)
>     Interval: Discrete 0.160s (6.250 fps)
>     Interval: Discrete 0.200s (5.000 fps)
>     Interval: Discrete 0.240s (4.167 fps)
>     Size: Discrete 408x616
> 
>     Size: Discrete 816x308
> 
>     Size: Discrete 408x308
> 
> 
> These don't become possible as a result of implementing the enums,
> they're all already requestable through /dev/video10.

Ok that wasn't clear. So basically video9 is a front-end to video10,
and it does not proxy the enumerations. I understand this is what you
are now fixing. And this has to be fixed, because I can image cases
where the front-end could support only a subset of the sub-dev. So
having userspace enumerate on another device (and having to find this
device by walking the tree) is unlikely to work in all scenarios.

regards,
Nicolas

p.s. This is why caps negotiation is annoyingly complex in GStreamer,
specially that there is no shortcut, you connect pads, and they figure-
out what format they will use between each other.

signature.asc
Description: This is a digitally signed message part

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 19, 2017 at 05:00:08PM +0200, Vladimir Zapolskiy wrote:
> On 03/19/2017 04:22 PM, Russell King - ARM Linux wrote:
> > On Sun, Mar 19, 2017 at 02:21:10PM +, Russell King - ARM Linux wrote:
> >> There's a good reason why I dumped a full debug log using GST_DEBUG=*:9,
> >> analysed it for the cause of the failure, and tried several different
> >> pipelines, including the standard bayer2rgb plugin.
> >>
> >> Please don't blame this on random stuff after analysis of the logs _and_
> >> reading the appropriate plugin code has shown where the problem is.  I
> >> know gstreamer can be very complex, but it's very possible to analyse
> >> the cause of problems and pin them down with detailed logs in conjunction
> >> with the source code.
> > 
> > Oh, and the proof of correct analysis is that fixing the kernel capture
> > driver to enumerate the frame sizes and intervals fixes the issue, even
> > with bayer2rgbneon being used.
> > 
> > Therefore, there is _no way_ what so ever that it could be caused by that
> > plugin.
> > 
> 
> Hey, no blaming of the unknown to me bayer2rgbneon element from my side,
> I've just asked an innocent question, thanks for reply. I failed to find
> the source code of the plugin, I was interested to compare its performance
> and features with mine in-house NEON powered RGGB/BGGR to RGB24 GStreamer
> conversion element, which is written years ago. My question was offtopic.

If you wanted to know where to get it from, you should've asked that.
You can find all the bits here:

https://git.phytec.de/

You need bayer2rgb-neon and gst-bayer2rgb-neon, and it requires some
fixes to the configure script and Makefiles get it to build if you
don't have gengenopt available.

-- 
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 v5 00/39] i.MX Media Driver

[Vladimir Zapolskiy]

On 03/19/2017 04:22 PM, Russell King - ARM Linux wrote:
> On Sun, Mar 19, 2017 at 02:21:10PM +, Russell King - ARM Linux wrote:
>> There's a good reason why I dumped a full debug log using GST_DEBUG=*:9,
>> analysed it for the cause of the failure, and tried several different
>> pipelines, including the standard bayer2rgb plugin.
>>
>> Please don't blame this on random stuff after analysis of the logs _and_
>> reading the appropriate plugin code has shown where the problem is.  I
>> know gstreamer can be very complex, but it's very possible to analyse
>> the cause of problems and pin them down with detailed logs in conjunction
>> with the source code.
> 
> Oh, and the proof of correct analysis is that fixing the kernel capture
> driver to enumerate the frame sizes and intervals fixes the issue, even
> with bayer2rgbneon being used.
> 
> Therefore, there is _no way_ what so ever that it could be caused by that
> plugin.
> 

Hey, no blaming of the unknown to me bayer2rgbneon element from my side,
I've just asked an innocent question, thanks for reply. I failed to find
the source code of the plugin, I was interested to compare its performance
and features with mine in-house NEON powered RGGB/BGGR to RGB24 GStreamer
conversion element, which is written years ago. My question was offtopic.

--
With best wishes,
Vladimir

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 19, 2017 at 10:33:25AM -0400, Nicolas Dufresne wrote:
> Le dimanche 19 mars 2017 à 00:54 +, Russell King - ARM Linux a
> écrit :
> > > 
> > > In practice, I have the impression there is a fair reason why
> > > framerate
> > > enumeration isn't implemented (considering there is only 1 valid
> > > rate).
> > 
> > That's actually completely incorrect.
> > 
> > With the capture device interfacing directly with CSI, it's possible
> > _today_ to select:
> > 
> > * the CSI sink pad's resolution
> > * the CSI sink pad's resolution with the width and/or height halved
> > * the CSI sink pad's frame rate
> > * the CSI sink pad's frame rate divided by the frame drop factor
> > 
> > To put it another way, these are possible:
> > 
> > # v4l2-ctl -d /dev/video10 --list-formats-ext
> > ioctl: VIDIOC_ENUM_FMT
> >     Index   : 0
> >     Type    : Video Capture
> >     Pixel Format: 'RGGB'
> >     Name    : 8-bit Bayer RGRG/GBGB
> >     Size: Discrete 816x616
> >     Interval: Discrete 0.040s (25.000 fps)
> >     Interval: Discrete 0.048s (20.833 fps)
> >     Interval: Discrete 0.050s (20.000 fps)
> >     Interval: Discrete 0.053s (18.750 fps)
> >     Interval: Discrete 0.060s (16.667 fps)
> >     Interval: Discrete 0.067s (15.000 fps)
> >     Interval: Discrete 0.080s (12.500 fps)
> >     Interval: Discrete 0.100s (10.000 fps)
> >     Interval: Discrete 0.120s (8.333 fps)
> >     Interval: Discrete 0.160s (6.250 fps)
> >     Interval: Discrete 0.200s (5.000 fps)
> >     Interval: Discrete 0.240s (4.167 fps)
> >     Size: Discrete 408x616
> > 
> >     Size: Discrete 816x308
> > 
> >     Size: Discrete 408x308
> > 
> > 
> > These don't become possible as a result of implementing the enums,
> > they're all already requestable through /dev/video10.
> 
> Ok that wasn't clear. So basically video9 is a front-end to video10,
> and it does not proxy the enumerations.

No.  We've sent .dot graphs which show the structure of the imx capture
driver.

What we have wrt video nodes is (eg):

sensor --->  csi2 > mux ---> csi +--> csi capture
subdev  subdevsubdevsubdev   |   /dev/video10
 |
 +-\
 |  \
 +--> vdic ---> ic_prpenc ---> ic_prpenc
 subdev subdev capture

... etc ... for full details, see the .dot diagrams that have been
sent (sorry I can't recall where they are in the threads.)

> I understand this is what you
> are now fixing. And this has to be fixed, because I can image cases
> where the front-end could support only a subset of the sub-dev. So
> having userspace enumerate on another device (and having to find this
> device by walking the tree) is unlikely to work in all scenarios.

The capture blocks (imx-media-capture) all talk to their immediate
upstream subdev and configure its source pad according to the formats,
frame size and frame interval requested by the capture application.
The subdev source pad decides whether the request is valid, and allows
it, modifies it or rejects it as appropriate.

Without working enumeration support, there's no way for an application
to find out what possible settings there are, and, as I've already
explained, the CSI subdev is capable itself of two things:

* Scaling down the image by a factor of two independently in the
  horizontal and vertical directions
* Deterministically dropping frames received from its upstream
  element, thereby reducing the frame rate.

> p.s. This is why caps negotiation is annoyingly complex in GStreamer,
> specially that there is no shortcut, you connect pads, and they figure-
> out what format they will use between each other.

Right, so when you specify video/x-raw,...,framerate=60/1 it introduces
a new element which has one source and sink pad, which only supports
the specification given.  If the neighbour's pad doesn't support it,
gstreamer fails because the caps negotiation fails.

So, if v4l2src believes (via the tvnorms, because it's lacking any
other information) that the capture device can only do 25fps and
30fps, then trying to set 60fps _even if S_PARM may accept it_ will
cause gstreamer to fail - because v4l2src can only advertise that
it supports a source of 25fps and 30fps.

-- 
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 v5 00/39] i.MX Media Driver

[Nicolas Dufresne]

Le dimanche 19 mars 2017 à 14:21 +, Russell King - ARM Linux a
écrit :
> > Can it be a point of failure?
> 
> There's a good reason why I dumped a full debug log using
> GST_DEBUG=*:9,
> analysed it for the cause of the failure, and tried several different
> pipelines, including the standard bayer2rgb plugin.
> 
> Please don't blame this on random stuff after analysis of the logs
> _and_
> reading the appropriate plugin code has shown where the problem is. 
> I
> know gstreamer can be very complex, but it's very possible to analyse
> the cause of problems and pin them down with detailed logs in
> conjunction
> with the source code.

I read your analyses with GStreamer, and it was all correct.

Nicolas

signature.asc
Description: This is a digitally signed message part

Re: [PATCH v5 00/39] i.MX Media Driver

[Nicolas Dufresne]

Le dimanche 19 mars 2017 à 09:55 +, Russell King - ARM Linux a
écrit :
> 2) would it also make sense to allow gstreamer's v4l2src to try
> setting
>    a these parameters, and only fail if it's unable to set it?  IOW,
> if
>    I use:
> 
> gst-launch-1.0 v4l2src device=/dev/video10 ! \
> video/x-bayer,format=RGGB,framerate=20/1 ! ...
> 
>    where G_PARM says its currently configured for 25fps, but a S_PARM
>    with 20fps would actually succeed.

In current design, v4l2src will "probe" all possible formats, cache
this, and use this information for negotiation. So after the caps has
been probed, there will be no TRY_FMT or anything like this happening
until it's too late. You have spotted a bug though, it should be
reading back the parm structure to validate (and probably produce a
not-negotiated error here).

Recently, specially for the IMX work done by Pengutronix, there was
contributions to enhance this probing to support probing capabilities
that are not enumerable (e.g. interlacing, colorimetry) using TRY_FMT.
There is no TRY_PARM in the API to implement similar fallback. Also,
those ended up creating a massive disaster for slow cameras. We now
have UVC cameras that takes 6s or more to start. I have no other choice
but to rewrite that now. We will negotiate the non-enumerable at the
last minute with TRY_FMT (when the subset is at it's smallest). This
will by accident add support for this camera interface, but that wasn't
the goal. It would still fail with application that enumerates the
possible resolutions and framerate and let you select them with a drop-
down (like cheese). In general, I can only conclude that making
everything that matter enumerable is the only working way to go for
generic userspace. 

Nicolas

signature.asc
Description: This is a digitally signed message part

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 19, 2017 at 02:21:10PM +, Russell King - ARM Linux wrote:
> There's a good reason why I dumped a full debug log using GST_DEBUG=*:9,
> analysed it for the cause of the failure, and tried several different
> pipelines, including the standard bayer2rgb plugin.
> 
> Please don't blame this on random stuff after analysis of the logs _and_
> reading the appropriate plugin code has shown where the problem is.  I
> know gstreamer can be very complex, but it's very possible to analyse
> the cause of problems and pin them down with detailed logs in conjunction
> with the source code.

Oh, and the proof of correct analysis is that fixing the kernel capture
driver to enumerate the frame sizes and intervals fixes the issue, even
with bayer2rgbneon being used.

Therefore, there is _no way_ what so ever that it could be caused by that
plugin.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 19, 2017 at 03:57:56PM +0200, Vladimir Zapolskiy wrote:
> Hi Russell,
> 
> On 03/18/2017 10:43 PM, Russell King - ARM Linux wrote:
> > On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> >> Can you share your gstreamer pipeline? For now, until
> >> VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
> >> does not attempt to specify a frame rate. I use the attached
> >> script for testing, which works for me.
> > 
> > It's nothing more than
> > 
> >   gst-launch-1.0 -v v4l2src !  ! xvimagesink
> > 
> > in my case, the conversions are bayer2rgbneon.  However, this only shows
> > you the frame rate negotiated on the pads (which is actually good enough
> > to show the issue.)
> 
> I'm sorry for potential offtopic, but is bayer2rgbneon element found in
> any officially supported by GStreamer plugin?

No it isn't.  Google is wonderful, please make use of planetary search
facilities.

> Can it be a point of failure?

There's a good reason why I dumped a full debug log using GST_DEBUG=*:9,
analysed it for the cause of the failure, and tried several different
pipelines, including the standard bayer2rgb plugin.

Please don't blame this on random stuff after analysis of the logs _and_
reading the appropriate plugin code has shown where the problem is.  I
know gstreamer can be very complex, but it's very possible to analyse
the cause of problems and pin them down with detailed logs in conjunction
with the source code.

-- 
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 v5 00/39] i.MX Media Driver

[Vladimir Zapolskiy]

Hi Russell,

On 03/18/2017 10:43 PM, Russell King - ARM Linux wrote:
> On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
>> Can you share your gstreamer pipeline? For now, until
>> VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
>> does not attempt to specify a frame rate. I use the attached
>> script for testing, which works for me.
> 
> It's nothing more than
> 
>   gst-launch-1.0 -v v4l2src !  ! xvimagesink
> 
> in my case, the conversions are bayer2rgbneon.  However, this only shows
> you the frame rate negotiated on the pads (which is actually good enough
> to show the issue.)

I'm sorry for potential offtopic, but is bayer2rgbneon element found in
any officially supported by GStreamer plugin? Can it be a point of
failure?

--
With best wishes,
Vladimir

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> On 03/18/2017 12:22 PM, Russell King - ARM Linux wrote:
> >0:00:01.955927879 20954  0x15ffe90 INFOv4l2 
> >gstv4l2object.c:3811:gst_v4l2_object_get_caps: probed caps: 
> >video/x-bayer, format=(string)rggb, framerate=(fraction)3/1001, 
> >width=(int)816, height=(int)616, pixel-aspect-ratio=(fraction)1/1; 
> >video/x-raw, format=(string)I420, framerate=(fraction)3/1001, 
> >width=(int)816, height=(int)616, interlace-mode=(string)progressive, 
> >pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)YV12, 
> >framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
> >interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; 
> >video/x-raw, format=(string)BGR, framerate=(fraction)3/1001, 
> >width=(int)816, height=(int)616, interlace-mode=(string)progressive, 
> >pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)RGB, 
> >framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
> >interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1
> >
> >despite the media pipeline actually being configured for 60fps.
> >
> >Forcing it by adjusting the pipeline only results in gstreamer
> >failing, because it believes that v4l2 is unable to operate at
> >60fps.
> >
> >Also note the complaints from v4l2src about the non-compliance...
> 
> Thanks, I've fixed most of v4l2-compliance issues, but this is not
> done yet. Is that something you can help with?

I've looked at this, and IMHO it's yet another media control API mess.

- media-ctl itself allows setting the format on subdev pads via
  struct v4l2_subdev_format.

- struct v4l2_subdev_format contains a struct v4l2_mbus_framefmt.

- struct v4l2_mbus_framefmt contains:
  * @width:  frame width
  * @height: frame height
  * @code:   data format code (from enum v4l2_mbus_pixelcode)
  * @field:  used interlacing type (from enum v4l2_field)
  * @colorspace: colorspace of the data (from enum v4l2_colorspace)
  * @ycbcr_enc:  YCbCr encoding of the data (from enum v4l2_ycbcr_encoding)
  * @quantization: quantization of the data (from enum v4l2_quantization)
  * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func)

- media-ctl sets width, height, code and field, but nothing else.

We're already agreed that the fields that media-ctl are part of the
format negotiation between the ultimate source, flowing down to the
capture device.  However, there's no support in media-ctl to deal
with these other fields - so media-ctl in itself is only half-
implemented.

>From what I can tell, _we_ are doing the right thing in imx-media-capture.

However, I think part of the problem is the set_fmt implementation.
When a source pad is configured via set_fmt(), any fields that can
not be altered (eg, because the subdev doesn't support colorspace
conversion) need to be preserved from the subdev's sink pad.

Right now, CSI doesn't do that - it only looks at the width, height,
code, and field.

I think we've got other bugs though that haven't been picked up by any
review - csi_try_fmt() adjusts the format using the _current_
configuration of the sink pad, even when using V4L2_SUBDEV_FORMAT_TRY.
This seems wrong according to the docs: the purpose of the try
mechanism is to be able to setup the _entire_ pipeline using the TRY
mechanism to work out whether the configuration works, before then
setting for real.  If we're validating the TRY formats against the
live configuration, then we're not doing that.

There's calls for:

v4l2_subdev_get_try_format
v4l2_subdev_get_try_crop
v4l2_subdev_get_try_compose

to get the try configuration - we hardly make use of all of these.  I
would suggest that we change the approach to implementing the various
subdevs such that:

1) like __csi_get_fmt(), we have accessors that gets a pointer to the
   correct state for the TRY/live settings.

2) everywhere we're asked to get or set parameters that can be TRY/live,
   we use these accessors to retrieve a pointer to the correct state to
   not only read, but also modify.

3) when we're evaluating parameters against another pad, we use these
   accessors to obtain the other pad's configuration, rather than poking
   about in the state saved in the subdev's priv-> (which is irrelevant
   for the TRY variant.)

4) ensure that all parameters which the subdev itself does not support
   modification of are correctly propagated from the sink pad to all
   source pads, and are unable to be modified via the source pad.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> Right, imx-media-capture.c (the "standard" v4l2 user interface module)
> is not implementing VIDIOC_ENUM_FRAMESIZES. It should, but it can only
> return the single frame size that the pipeline has configured (the mbus
> format of the attached source pad).

I now have a set of patches that enumerate the frame sizes and intervals
from the source pad of the first subdev (since you're setting the formats
etc there from the capture device, it seems sensible to return what it
can support.)  This means my patch set doesn't add to non-CSI subdevs.

> Can you share your gstreamer pipeline? For now, until
> VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
> does not attempt to specify a frame rate. I use the attached
> script for testing, which works for me.

Note that I'm not specifying a frame rate on gstreamer - I'm setting
the pipeline up for 60fps, but gstreamer in its wisdom is unable to
enumerate the frame sizes, and therefore is unable to enumerate the
frame intervals (frame intervals depend on frame sizes), so it
falls back to the "tvnorms" which are basically 25/1 and 3/1001.

It sees 60fps via G_PARM, and then decides to set 3/1001 via S_PARM.
So, we end up with most of the pipeline operating at 60fps, with CSI
doing frame skipping to reduce the frame rate to 30fps.

gstreamer doesn't complain, doesn't issue any warnings, the only way
you can spot this is to enable debugging and look through the copious
debug log, or use -v and check the pad capabilities.

Testing using gstreamer, and only using "does it produce video" is a
good simple test, but it's just that - it's a simple test.  It doesn't
tell you that what you're seeing is what you intended to see (such as
video at the frame rate you expected) without more work.

> Thanks, I've fixed most of v4l2-compliance issues, but this is not
> done yet. Is that something you can help with?

What did you do with:

ioctl(3, VIDIOC_REQBUFS, {count=0, type=0 /* V4L2_BUF_TYPE_??? */, memory=0 /* 
V4L2_MEMORY_??? */}) = -1 EINVAL (Invalid argument)
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
ioctl(3, VIDIOC_EXPBUF, 0xbef405bc) = -1 EINVAL (Invalid argument)
fail: v4l2-test-buffers.cpp(571): q.has_expbuf(node)
test VIDIOC_EXPBUF: FAIL

To me, this looks like a bug in v4l2-compliance (I'm using 1.10.0).
I'm not sure what buffer VIDIOC_EXPBUF is expected to export, since
afaics no buffers have been allocated, so of course it's going to fail.
Either that, or the v4l2 core vb2 code is non-compliant with v4l2's
interface requirements.

In any case, it doesn't look like the buffer management is being
tested at all by v4l2-compliance - we know that gstreamer works, so
buffers _can_ be allocated, and I've also used dmabufs with gstreamer,
so I also know that VIDIOC_EXPBUF works there.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sat, Mar 18, 2017 at 08:41:14PM -0400, Nicolas Dufresne wrote:
> Along with the norm fallback, GStreamer could could also consider the
> currently set framerate as returned by VIDIOC_G_PARM. At the same time,
> implementing that enumeration shall be straightforward, and will make a
> large amount of existing userspace work.

Since, according to v4l2-compliance, providing the enumeration ioctls
appears to be optional:

1) should v4l2-compliance be checking whether other frame sizes/frame
   intervals are possible, and failing if the enumeration ioctls are
   not supported?

2) would it also make sense to allow gstreamer's v4l2src to try setting
   a these parameters, and only fail if it's unable to set it?  IOW, if
   I use:

gst-launch-1.0 v4l2src device=/dev/video10 ! \
video/x-bayer,format=RGGB,framerate=20/1 ! ...

   where G_PARM says its currently configured for 25fps, but a S_PARM
   with 20fps would actually succeed.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sat, Mar 18, 2017 at 08:41:14PM -0400, Nicolas Dufresne wrote:
> Le samedi 18 mars 2017 à 20:43 +, Russell King - ARM Linux a
> écrit :
> > On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> > > Can you share your gstreamer pipeline? For now, until
> > > VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
> > > does not attempt to specify a frame rate. I use the attached
> > > script for testing, which works for me.
> > 
> > It's nothing more than
> > 
> >   gst-launch-1.0 -v v4l2src !  ! xvimagesink
> > 
> > in my case, the conversions are bayer2rgbneon.  However, this only
> > shows
> > you the frame rate negotiated on the pads (which is actually good
> > enough
> > to show the issue.)
> > 
> > How I stumbled across though this was when I was trying to encode:
> > 
> >  gst-launch-1.0 v4l2src device=/dev/video9 ! bayer2rgbneon ! \
> > videoconvert ! x264enc speed-preset=1 ! avimux ! \
> > filesink location=test.avi
> > 
> > I noticed that vlc would always say it was playing the resulting AVI
> > at 30fps.
> 
> In practice, I have the impression there is a fair reason why framerate
> enumeration isn't implemented (considering there is only 1 valid rate).

That's actually completely incorrect.

With the capture device interfacing directly with CSI, it's possible
_today_ to select:

* the CSI sink pad's resolution
* the CSI sink pad's resolution with the width and/or height halved
* the CSI sink pad's frame rate
* the CSI sink pad's frame rate divided by the frame drop factor

To put it another way, these are possible:

# v4l2-ctl -d /dev/video10 --list-formats-ext
ioctl: VIDIOC_ENUM_FMT
Index   : 0
Type: Video Capture
Pixel Format: 'RGGB'
Name: 8-bit Bayer RGRG/GBGB
Size: Discrete 816x616
Interval: Discrete 0.040s (25.000 fps)
Interval: Discrete 0.048s (20.833 fps)
Interval: Discrete 0.050s (20.000 fps)
Interval: Discrete 0.053s (18.750 fps)
Interval: Discrete 0.060s (16.667 fps)
Interval: Discrete 0.067s (15.000 fps)
Interval: Discrete 0.080s (12.500 fps)
Interval: Discrete 0.100s (10.000 fps)
Interval: Discrete 0.120s (8.333 fps)
Interval: Discrete 0.160s (6.250 fps)
Interval: Discrete 0.200s (5.000 fps)
Interval: Discrete 0.240s (4.167 fps)
Size: Discrete 408x616

Size: Discrete 816x308

Size: Discrete 408x308


These don't become possible as a result of implementing the enums,
they're all already requestable through /dev/video10.

-- 
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 v5 00/39] i.MX Media Driver

[Nicolas Dufresne]

Le samedi 18 mars 2017 à 20:43 +, Russell King - ARM Linux a
écrit :
> On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> > Can you share your gstreamer pipeline? For now, until
> > VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
> > does not attempt to specify a frame rate. I use the attached
> > script for testing, which works for me.
> 
> It's nothing more than
> 
>   gst-launch-1.0 -v v4l2src !  ! xvimagesink
> 
> in my case, the conversions are bayer2rgbneon.  However, this only
> shows
> you the frame rate negotiated on the pads (which is actually good
> enough
> to show the issue.)
> 
> How I stumbled across though this was when I was trying to encode:
> 
>  gst-launch-1.0 v4l2src device=/dev/video9 ! bayer2rgbneon ! \
> videoconvert ! x264enc speed-preset=1 ! avimux ! \
> filesink location=test.avi
> 
> I noticed that vlc would always say it was playing the resulting AVI
> at 30fps.

In practice, I have the impression there is a fair reason why framerate
enumeration isn't implemented (considering there is only 1 valid rate).
Along with the norm fallback, GStreamer could could also consider the
currently set framerate as returned by VIDIOC_G_PARM. At the same time,
implementing that enumeration shall be straightforward, and will make a
large amount of existing userspace work.

regards,
Nicolas

signature.asc
Description: This is a digitally signed message part

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> Can you share your gstreamer pipeline? For now, until
> VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
> does not attempt to specify a frame rate. I use the attached
> script for testing, which works for me.

It's nothing more than

  gst-launch-1.0 -v v4l2src !  ! xvimagesink

in my case, the conversions are bayer2rgbneon.  However, this only shows
you the frame rate negotiated on the pads (which is actually good enough
to show the issue.)

How I stumbled across though this was when I was trying to encode:

 gst-launch-1.0 v4l2src device=/dev/video9 ! bayer2rgbneon ! \
videoconvert ! x264enc speed-preset=1 ! avimux ! \
filesink location=test.avi

I noticed that vlc would always say it was playing the resulting AVI
at 30fps.

-- 
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 v5 00/39] i.MX Media Driver

[Steve Longerbeam]

Hi Russell,


On 03/14/2017 10:29 AM, Steve Longerbeam wrote:



On 03/12/2017 02:09 PM, Russell King - ARM Linux wrote:
On Sun, Mar 12, 2017 at 08:40:37PM +, Russell King - ARM Linux 
wrote:

On Sun, Mar 12, 2017 at 01:36:32PM -0700, Steve Longerbeam wrote:

But hold on, if my logic is correct, then why did the CSI power-off
get reached in your case, multiple times? Yes I think there is a bug,
link_notify() is not checking if the link has already been disabled.
I will fix this. But I'm surprised media core's link_notify handling
doesn't do this.

Well, I think there's something incredibly fishy going on here.  I
turned that dev_dbg() at the top of the function into a dev_info(),
and I get:

root@hbi2ex:~# dmesg |grep -A2 imx-ipuv3-csi
[   53.370949] imx-ipuv3-csi imx-ipuv3-csi.0: power OFF
[   53.371015] [ cut here ]
[   53.371075] WARNING: CPU: 0 PID: 1515 at 
drivers/staging/media/imx/imx-media-csi.c:806 csi_s_power+0xb8/0xd0 
[imx_media_csi]

--
[   53.372624] imx-ipuv3-csi imx-ipuv3-csi.0: power OFF
[   53.372637] [ cut here ]
[   53.372663] WARNING: CPU: 0 PID: 1515 at 
drivers/staging/media/imx/imx-media-csi.c:806 csi_s_power+0xb8/0xd0 
[imx_media_csi]


There isn't a power on event being generated before these two power
off events.  I don't see a power on event even when I attempt to
start streaming either (which fails due to the lack of bayer
support.)

Found it - my imx219 driver returns '1' from its s_power function when
powering up, which triggers a bug in your code - when 
imx_media_set_power()

fails to power up, you call imx_media_set_power() telling it to power
everything off - including devices that are already powered off.


Yep, there's a bug in the error cleanup in 
imx_media_pipeline_set_power().
On error, it needs to backout by calling s_power(off) as it is doing, 
but not

through the whole pipeline, but needs to stop at the subdev encountered
just before the subdev that failed. This was causing the s_power() 
imbalance.

I will fix.



Due to some fixes to ov5640 from version 4, v4l2_pipeline_pm APIs are 
working

now, so I've removed imx_media_pipeline_set_power() and switched to
v4l2_pipeline_pm_use() in capture device open/release and 
v4l2_pipeline_link_notify()

in imx_media_link_notify(), for the pipeline power management.

Steve

Re: [PATCH v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/18/2017 12:22 PM, Russell King - ARM Linux wrote:

Hi Steve,

I've just been trying to get gstreamer to capture and h264 encode
video from my camera at various frame rates, and what I've discovered
does not look good.

1) when setting frame rates, media-ctl _always_ calls
VIDIOC_SUBDEV_S_FRAME_INTERVAL with pad=0.

2) media-ctl never retrieves the frame interval information, so there's
no way to read it back with standard tools, and no indication that
this is going on...


I think Philipp Zabel submitted a patch which addresses these
in media-ctl. Check with him.



3) gstreamer v4l2src is getting upset, because it can't enumerate the
frame sizes (VIDIOC_ENUM_FRAMESIZES fails),


Right, imx-media-capture.c (the "standard" v4l2 user interface module)
is not implementing VIDIOC_ENUM_FRAMESIZES. It should, but it can only
return the single frame size that the pipeline has configured (the mbus
format of the attached source pad).


  which causes it to
fallback to using the "tvnorms" to decide about frame rates.  This
makes it impossible to use frame rates higher than 3/1001, and
causes the pipeline validation to fail.


In v5 I added validation of frame intervals between pads,
but due to negative feedback I've pulled that. So next
version will not attempt to validate frame intervals between
source->sink pads.

Can you share your gstreamer pipeline? For now, until
VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
does not attempt to specify a frame rate. I use the attached
script for testing, which works for me.





0:00:01.937465845 20954  0x15ffe90 DEBUG   v4l2 
gstv4l2object.c:2474:gst_v4l2_object_probe_caps_for_format: 
Enumerating frame sizes for RGGB
0:00:01.937588518 20954  0x15ffe90 DEBUG   v4l2 
gstv4l2object.c:2601:gst_v4l2_object_probe_caps_for_format: Failed to 
enumerate frame sizes for pixelformat RGGB (Inappropriate ioctl for device)
0:00:01.937879535 20954  0x15ffe90 LOG v4l2 
gstv4l2object.c:2708:gst_v4l2_object_get_nearest_size: getting 
nearest size to 1x1 with format RGGB
0:00:01.937990874 20954  0x15ffe90 LOG v4l2 
gstv4l2object.c:2724:gst_v4l2_object_get_nearest_size: got nearest 
size 816x616
0:00:01.938250889 20954  0x15ffe90 ERROR   v4l2 
gstv4l2object.c:1873:gst_v4l2_object_get_interlace_mode: Driver bug detected - 
check driver with v4l2-compliance from http://git.linuxtv.org/v4l-utils.git
0:00:01.938326893 20954  0x15ffe90 LOG v4l2 
gstv4l2object.c:2708:gst_v4l2_object_get_nearest_size: getting 
nearest size to 32768x32768 with format RGGB
0:00:01.938431566 20954  0x15ffe90 LOG v4l2 
gstv4l2object.c:2724:gst_v4l2_object_get_nearest_size: got nearest 
size 816x616
0:00:01.939776641 20954  0x15ffe90 ERROR   v4l2 
gstv4l2object.c:1873:gst_v4l2_object_get_interlace_mode: Driver bug detected - 
check driver with v4l2-compliance from http://git.linuxtv.org/v4l-utils.git
0:00:01.940110660 20954  0x15ffe90 DEBUG   v4l2 
gstv4l2object.c:1955:gst_v4l2_object_get_colorspace: Unknown enum 
v4l2_colorspace 0

This triggers the "/* Since we can't get framerate directly, try to
use the current norm */" code in v4l2object.c, which causes it to
select one of the 3/1001 norms:

0:00:01.955927879 20954  0x15ffe90 INFOv4l2 
gstv4l2object.c:3811:gst_v4l2_object_get_caps: probed caps: 
video/x-bayer, format=(string)rggb, framerate=(fraction)3/1001, width=(int)816, 
height=(int)616, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)I420, 
framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, 
format=(string)YV12, framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, 
format=(string)BGR, framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, 
format=(string)RGB, framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1

despite the media pipeline actually being configured for 60fps.

Forcing it by adjusting the pipeline only results in gstreamer
failing, because it believes that v4l2 is unable to operate at
60fps.

Also note the complaints from v4l2src about the non-compliance...


Thanks, I've fixed most of v4l2-compliance issues, but this is not
done yet. Is that something you can help with?

Steve



udpstream.sh
Description: application/shellscript

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

Hi Steve,

I've just been trying to get gstreamer to capture and h264 encode
video from my camera at various frame rates, and what I've discovered
does not look good.

1) when setting frame rates, media-ctl _always_ calls
   VIDIOC_SUBDEV_S_FRAME_INTERVAL with pad=0.

2) media-ctl never retrieves the frame interval information, so there's
   no way to read it back with standard tools, and no indication that
   this is going on...

3) gstreamer v4l2src is getting upset, because it can't enumerate the
   frame sizes (VIDIOC_ENUM_FRAMESIZES fails), which causes it to
   fallback to using the "tvnorms" to decide about frame rates.  This
   makes it impossible to use frame rates higher than 3/1001, and
   causes the pipeline validation to fail.

0:00:01.937465845 20954  0x15ffe90 DEBUG   v4l2 
gstv4l2object.c:2474:gst_v4l2_object_probe_caps_for_format: 
Enumerating frame sizes for RGGB
0:00:01.937588518 20954  0x15ffe90 DEBUG   v4l2 
gstv4l2object.c:2601:gst_v4l2_object_probe_caps_for_format: Failed to 
enumerate frame sizes for pixelformat RGGB (Inappropriate ioctl for device)
0:00:01.937879535 20954  0x15ffe90 LOG v4l2 
gstv4l2object.c:2708:gst_v4l2_object_get_nearest_size: getting 
nearest size to 1x1 with format RGGB
0:00:01.937990874 20954  0x15ffe90 LOG v4l2 
gstv4l2object.c:2724:gst_v4l2_object_get_nearest_size: got nearest 
size 816x616
0:00:01.938250889 20954  0x15ffe90 ERROR   v4l2 
gstv4l2object.c:1873:gst_v4l2_object_get_interlace_mode: Driver bug detected - 
check driver with v4l2-compliance from http://git.linuxtv.org/v4l-utils.git
0:00:01.938326893 20954  0x15ffe90 LOG v4l2 
gstv4l2object.c:2708:gst_v4l2_object_get_nearest_size: getting 
nearest size to 32768x32768 with format RGGB
0:00:01.938431566 20954  0x15ffe90 LOG v4l2 
gstv4l2object.c:2724:gst_v4l2_object_get_nearest_size: got nearest 
size 816x616
0:00:01.939776641 20954  0x15ffe90 ERROR   v4l2 
gstv4l2object.c:1873:gst_v4l2_object_get_interlace_mode: Driver bug detected - 
check driver with v4l2-compliance from http://git.linuxtv.org/v4l-utils.git
0:00:01.940110660 20954  0x15ffe90 DEBUG   v4l2 
gstv4l2object.c:1955:gst_v4l2_object_get_colorspace: Unknown enum 
v4l2_colorspace 0

   This triggers the "/* Since we can't get framerate directly, try to
   use the current norm */" code in v4l2object.c, which causes it to
   select one of the 3/1001 norms:

0:00:01.955927879 20954  0x15ffe90 INFOv4l2 
gstv4l2object.c:3811:gst_v4l2_object_get_caps: probed caps: 
video/x-bayer, format=(string)rggb, framerate=(fraction)3/1001, 
width=(int)816, height=(int)616, pixel-aspect-ratio=(fraction)1/1; video/x-raw, 
format=(string)I420, framerate=(fraction)3/1001, width=(int)816, 
height=(int)616, interlace-mode=(string)progressive, 
pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)YV12, 
framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; 
video/x-raw, format=(string)BGR, framerate=(fraction)3/1001, 
width=(int)816, height=(int)616, interlace-mode=(string)progressive, 
pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)RGB, 
framerate=(fraction)3/1001, width=(int)816, height=(int)616, 
interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1

   despite the media pipeline actually being configured for 60fps.

   Forcing it by adjusting the pipeline only results in gstreamer
   failing, because it believes that v4l2 is unable to operate at
   60fps.

   Also note the complaints from v4l2src about the non-compliance...

-- 
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 v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/12/2017 02:09 PM, Russell King - ARM Linux wrote:

On Sun, Mar 12, 2017 at 08:40:37PM +, Russell King - ARM Linux wrote:

On Sun, Mar 12, 2017 at 01:36:32PM -0700, Steve Longerbeam wrote:

But hold on, if my logic is correct, then why did the CSI power-off
get reached in your case, multiple times? Yes I think there is a bug,
link_notify() is not checking if the link has already been disabled.
I will fix this. But I'm surprised media core's link_notify handling
doesn't do this.

Well, I think there's something incredibly fishy going on here.  I
turned that dev_dbg() at the top of the function into a dev_info(),
and I get:

root@hbi2ex:~# dmesg |grep -A2 imx-ipuv3-csi
[   53.370949] imx-ipuv3-csi imx-ipuv3-csi.0: power OFF
[   53.371015] [ cut here ]
[   53.371075] WARNING: CPU: 0 PID: 1515 at 
drivers/staging/media/imx/imx-media-csi.c:806 csi_s_power+0xb8/0xd0 
[imx_media_csi]
--
[   53.372624] imx-ipuv3-csi imx-ipuv3-csi.0: power OFF
[   53.372637] [ cut here ]
[   53.372663] WARNING: CPU: 0 PID: 1515 at 
drivers/staging/media/imx/imx-media-csi.c:806 csi_s_power+0xb8/0xd0 
[imx_media_csi]

There isn't a power on event being generated before these two power
off events.  I don't see a power on event even when I attempt to
start streaming either (which fails due to the lack of bayer
support.)

Found it - my imx219 driver returns '1' from its s_power function when
powering up, which triggers a bug in your code - when imx_media_set_power()
fails to power up, you call imx_media_set_power() telling it to power
everything off - including devices that are already powered off.


Yep, there's a bug in the error cleanup in imx_media_pipeline_set_power().
On error, it needs to backout by calling s_power(off) as it is doing, 
but not

through the whole pipeline, but needs to stop at the subdev encountered
just before the subdev that failed. This was causing the s_power() 
imbalance.

I will fix.




This is really bad news - s_power() may be called via other paths,
such as when the subdev is opened.


I don't think that is a problem, as long as power_count is working
as it should, and the caller from the other paths has not created an
imbalance.

Steve

Re: [PATCH v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/12/2017 03:10 PM, Mauro Carvalho Chehab wrote:

Em Sun, 12 Mar 2017 21:13:24 +
Russell King - ARM Linux  escreveu:


On Sun, Mar 12, 2017 at 05:59:28PM -0300, Mauro Carvalho Chehab wrote:

Yet, udev/systemd has some rules that provide an unique name for V4L
devices at /lib/udev/rules.d/60-persistent-v4l.rules. Basically, it
runs a small application (v4l_id) with creates a persistent symling
using rules like this:

KERNEL=="video*", ENV{ID_SERIAL}=="?*", 
SYMLINK+="v4l/by-id/$env{ID_BUS}-$env{ID_SERIAL}-video-index$attr{index}"

Those names are stored at /dev/v4l/by-path.

This doesn't help:

$ ls -Al /dev/v4l/by-id/
total 0
lrwxrwxrwx 1 root root 13 Mar 12 19:54 
usb-Sonix_Technology_Co.__Ltd._USB_2.0_Camera-video-index0 -> ../../video10
$ ls -Al /dev/v4l/by-path/
total 0
lrwxrwxrwx 1 root root 12 Mar 12 19:54 platform-204.vpu-video-index0 -> 
../../video0
lrwxrwxrwx 1 root root 12 Mar 12 19:54 platform-204.vpu-video-index1 -> 
../../video1
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index0 
-> ../../video2
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index1 
-> ../../video3
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index2 
-> ../../video4
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index3 
-> ../../video5
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index4 
-> ../../video6
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index5 
-> ../../video7
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index6 
-> ../../video8
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index7 
-> ../../video9
lrwxrwxrwx 1 root root 13 Mar 12 19:54 platform-ci_hdrc.0-usb-0:1:1.0-video-index0 
-> ../../video10

The problem is the "platform-capture-subsystem-video-index" entries.
These themselves change order.  For instance, I now have:

- entity 72: ipu1_csi0 capture (1 pad, 1 link)
  type Node subtype V4L flags 0
  device node name /dev/video6

which means it's platform-capture-subsystem-video-index4.  Before, it
was platform-capture-subsystem-video-index2.

That's a driver problem. v4l_id gets information to build the persistent
name from the result of VIDIOC_QUERYCAP.

In the case of Exynos gsc driver, for example, the information is here:

static int gsc_m2m_querycap(struct file *file, void *fh,
   struct v4l2_capability *cap)
{
struct gsc_ctx *ctx = fh_to_ctx(fh);
struct gsc_dev *gsc = ctx->gsc_dev;

strlcpy(cap->driver, GSC_MODULE_NAME, sizeof(cap->driver));
strlcpy(cap->card, GSC_MODULE_NAME " gscaler", sizeof(cap->card));
snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
 dev_name(>pdev->dev));
cap->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_M2M_MPLANE |
V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_VIDEO_OUTPUT_MPLANE;

cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
return 0;
}

See that the bus_info there is filled with:

snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", 
dev_name(>pdev->dev));

 From the output you printed, it seems that the i.MX6 is just doing:
snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:");
for some devices.


imx6 is setting bus_info string on all capture devices as:

snprintf(cap->bus_info, sizeof(cap->bus_info),
 "platform:%s", dev_name(priv->dev));

dev_name(priv->dev) is the device name of the attached subdev.
So the bus_info string, at least for attached CSI subdevs, should
be "platform:imx-ipuv3-csi".

Maybe there is something else missing, I haven't had a chance to
look at this yet.

Steve




If you change the i.MX6 driver to do the same, you'll likely be able to
have unique names there too.

Regards,
Mauro

Re: [PATCH v5 00/39] i.MX Media Driver

[Steve Longerbeam]

er, I meant I will integrate this patch. And verify/fix
possible breakage for non-bayer passthrough.

Steve


On 03/13/2017 02:30 AM, Russell King - ARM Linux wrote:

On Mon, Mar 13, 2017 at 08:16:25AM +, Russell King - ARM Linux wrote:

On Sun, Mar 12, 2017 at 09:26:41PM -0700, Steve Longerbeam wrote:

On 03/12/2017 01:22 PM, Russell King - ARM Linux wrote:

What I had was this patch for your v3.  I never got to testing your
v4 because of the LP-11 problem.

In v5, you've changed to propagate the ipu_cpmem_set_image() error
code to avoid the resulting corruption, but that leaves the other bits
of this patch unaddressed, along my "media: imx: smfc: add support
for bayer formats" patch.

Your driver basically has no support for bayer formats.

You added the patches to this driver that adds the bayer support,
I don't think there is anything more required of the driver at this
point to support bayer, the remaining work needs to happen in the IPUv3
driver.

There is more work, because the way you've merged my changes to
imx_smfc_setup_channel() into csi_idmac_setup_channel() is wrong with
respect to the burst size.

You always set it to 8 or 16 depending on the width:

burst_size = (image.pix.width & 0xf) ? 8 : 16;

ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size);

and then you have my switch() statement which assigns burst_size.
My _tested_ code removed the above, added the switch, which had
a default case which reflected the above setting:

default:
burst_size = (outfmt->width & 0xf) ? 8 : 16;

and then went on to set the burst size _after_ the switch statement:

ipu_cpmem_set_burstsize(priv->smfc_ch, burst_size);

The effect is unchanged for non-bayer formats.  For bayer formats, the
burst size is determined by the bayer data size.

So, even if it's appropriate to fix ipu_cpmem_set_image(), fixing the
above is still required.

I'm not convinced that fixing ipu_cpmem_set_image() is even the best
solution - it's not as trivial as it looks on the surface:

 ipu_cpmem_set_resolution(ch, image->rect.width, image->rect.height);
 ipu_cpmem_set_stride(ch, pix->bytesperline);

this is fine, it doesn't depend on the format.  However, the next line:

 ipu_cpmem_set_fmt(ch, v4l2_pix_fmt_to_drm_fourcc(pix->pixelformat));

does - v4l2_pix_fmt_to_drm_fourcc() is a locally defined function (it
isn't v4l2 code) that converts a v4l2 pixel format to a DRM fourcc.
DRM knows nothing about bayer formats, there aren't fourcc codes in
DRM for it.  The result is that v4l2_pix_fmt_to_drm_fourcc() returns
-EINVAL cast to a u32, which gets passed unchecked into ipu_cpmem_set_fmt().

ipu_cpmem_set_fmt() won't recognise that, and also returns -EINVAL - and
it's a bug that this is not checked and propagated.  If it is checked and
propagated, then we need this to support bayer formats, and I don't see
DRM people wanting bayer format fourcc codes added without there being
a real DRM driver wanting to use them.

Then there's the business of calculating the top-left offset of the image,
which for bayer always needs to be an even number of pixels - as this
function takes the top-left offset, it ought to respect it, but if it
doesn't meet this criteria, what should it do?  csi_idmac_setup_channel()
always sets them to zero, but that's not really something that
ipu_cpmem_set_image() should assume.

For the time being, I've restored the functionality along the same lines
as I originally had.  This seems to get me working capture, but might
break non-bayer passthrough mode:

diff --git a/drivers/staging/media/imx/imx-media-csi.c 
b/drivers/staging/media/imx/imx-media-csi.c
index fc0036aa84d0..df336971a009 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -314,14 +314,6 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
image.phys0 = phys[0];
image.phys1 = phys[1];
  
-	ret = ipu_cpmem_set_image(priv->idmac_ch, );

-   if (ret)
-   return ret;
-
-   burst_size = (image.pix.width & 0xf) ? 8 : 16;
-
-   ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size);
-
/*
 * Check for conditions that require the IPU to handle the
 * data internally as generic data, aka passthrough mode:
@@ -346,15 +338,29 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
passthrough_bits = 16;
break;
default:
+   burst_size = (image.pix.width & 0xf) ? 8 : 16;
passthrough = (sensor_ep->bus_type != V4L2_MBUS_CSI2 &&
   sensor_ep->bus.parallel.bus_width >= 16);
passthrough_bits = 16;
break;
}
  
-	if (passthrough)

+   if (passthrough) {
+   ipu_cpmem_set_resolution(priv->idmac_ch, image.rect.width,
+image.rect.height);
+   ipu_cpmem_set_stride(priv->idmac_ch, 

Re: [PATCH v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/13/2017 01:16 AM, Russell King - ARM Linux wrote:

On Sun, Mar 12, 2017 at 09:26:41PM -0700, Steve Longerbeam wrote:

On 03/12/2017 01:22 PM, Russell King - ARM Linux wrote:

What I had was this patch for your v3.  I never got to testing your
v4 because of the LP-11 problem.

In v5, you've changed to propagate the ipu_cpmem_set_image() error
code to avoid the resulting corruption, but that leaves the other bits
of this patch unaddressed, along my "media: imx: smfc: add support
for bayer formats" patch.

Your driver basically has no support for bayer formats.

You added the patches to this driver that adds the bayer support,
I don't think there is anything more required of the driver at this
point to support bayer, the remaining work needs to happen in the IPUv3
driver.

There is more work, because the way you've merged my changes to
imx_smfc_setup_channel() into csi_idmac_setup_channel() is wrong with
respect to the burst size.

You always set it to 8 or 16 depending on the width:

burst_size = (image.pix.width & 0xf) ? 8 : 16;

ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size);

and then you have my switch() statement which assigns burst_size.
My _tested_ code removed the above, added the switch, which had
a default case which reflected the above setting:

default:
burst_size = (outfmt->width & 0xf) ? 8 : 16;

and then went on to set the burst size _after_ the switch statement:

ipu_cpmem_set_burstsize(priv->smfc_ch, burst_size);

The effect is unchanged for non-bayer formats.  For bayer formats, the
burst size is determined by the bayer data size.

So, even if it's appropriate to fix ipu_cpmem_set_image(), fixing the
above is still required.


Oops, sorry missed that. I'll fix.



I'm not convinced that fixing ipu_cpmem_set_image() is even the best
solution - it's not as trivial as it looks on the surface:

 ipu_cpmem_set_resolution(ch, image->rect.width, image->rect.height);
 ipu_cpmem_set_stride(ch, pix->bytesperline);

this is fine, it doesn't depend on the format.  However, the next line:

 ipu_cpmem_set_fmt(ch, v4l2_pix_fmt_to_drm_fourcc(pix->pixelformat));

does - v4l2_pix_fmt_to_drm_fourcc() is a locally defined function (it
isn't v4l2 code) that converts a v4l2 pixel format to a DRM fourcc.
DRM knows nothing about bayer formats, there aren't fourcc codes in
DRM for it.


right, yeah that's a problem.


   The result is that v4l2_pix_fmt_to_drm_fourcc() returns
-EINVAL cast to a u32, which gets passed unchecked into ipu_cpmem_set_fmt().


Ugh.



ipu_cpmem_set_fmt() won't recognise that, and also returns -EINVAL - and
it's a bug that this is not checked and propagated.  If it is checked and
propagated, then we need this to support bayer formats, and I don't see
DRM people wanting bayer format fourcc codes added without there being
a real DRM driver wanting to use them.


true.



Then there's the business of calculating the top-left offset of the image,
which for bayer always needs to be an even number of pixels - as this
function takes the top-left offset, it ought to respect it, but if it
doesn't meet this criteria, what should it do?  csi_idmac_setup_channel()
always sets them to zero, but that's not really something that
ipu_cpmem_set_image() should assume.


Well, I will integrate your patch above. Thanks for doing this
work for me.

We do need to address the issues you brought up in ipu_cpmem at
some point.

Steve

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Mon, Mar 13, 2017 at 08:16:25AM +, Russell King - ARM Linux wrote:
> On Sun, Mar 12, 2017 at 09:26:41PM -0700, Steve Longerbeam wrote:
> > On 03/12/2017 01:22 PM, Russell King - ARM Linux wrote:
> > >What I had was this patch for your v3.  I never got to testing your
> > >v4 because of the LP-11 problem.
> > >
> > >In v5, you've changed to propagate the ipu_cpmem_set_image() error
> > >code to avoid the resulting corruption, but that leaves the other bits
> > >of this patch unaddressed, along my "media: imx: smfc: add support
> > >for bayer formats" patch.
> > >
> > >Your driver basically has no support for bayer formats.
> > 
> > You added the patches to this driver that adds the bayer support,
> > I don't think there is anything more required of the driver at this
> > point to support bayer, the remaining work needs to happen in the IPUv3
> > driver.
> 
> There is more work, because the way you've merged my changes to
> imx_smfc_setup_channel() into csi_idmac_setup_channel() is wrong with
> respect to the burst size.
> 
> You always set it to 8 or 16 depending on the width:
> 
>   burst_size = (image.pix.width & 0xf) ? 8 : 16;
> 
>   ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size);
> 
> and then you have my switch() statement which assigns burst_size.
> My _tested_ code removed the above, added the switch, which had
> a default case which reflected the above setting:
> 
>   default:
>   burst_size = (outfmt->width & 0xf) ? 8 : 16;
> 
> and then went on to set the burst size _after_ the switch statement:
> 
>   ipu_cpmem_set_burstsize(priv->smfc_ch, burst_size);
> 
> The effect is unchanged for non-bayer formats.  For bayer formats, the
> burst size is determined by the bayer data size.
> 
> So, even if it's appropriate to fix ipu_cpmem_set_image(), fixing the
> above is still required.
> 
> I'm not convinced that fixing ipu_cpmem_set_image() is even the best
> solution - it's not as trivial as it looks on the surface:
> 
> ipu_cpmem_set_resolution(ch, image->rect.width, image->rect.height);
> ipu_cpmem_set_stride(ch, pix->bytesperline);
> 
> this is fine, it doesn't depend on the format.  However, the next line:
> 
> ipu_cpmem_set_fmt(ch, v4l2_pix_fmt_to_drm_fourcc(pix->pixelformat));
> 
> does - v4l2_pix_fmt_to_drm_fourcc() is a locally defined function (it
> isn't v4l2 code) that converts a v4l2 pixel format to a DRM fourcc.
> DRM knows nothing about bayer formats, there aren't fourcc codes in
> DRM for it.  The result is that v4l2_pix_fmt_to_drm_fourcc() returns
> -EINVAL cast to a u32, which gets passed unchecked into ipu_cpmem_set_fmt().
> 
> ipu_cpmem_set_fmt() won't recognise that, and also returns -EINVAL - and
> it's a bug that this is not checked and propagated.  If it is checked and
> propagated, then we need this to support bayer formats, and I don't see
> DRM people wanting bayer format fourcc codes added without there being
> a real DRM driver wanting to use them.
> 
> Then there's the business of calculating the top-left offset of the image,
> which for bayer always needs to be an even number of pixels - as this
> function takes the top-left offset, it ought to respect it, but if it
> doesn't meet this criteria, what should it do?  csi_idmac_setup_channel()
> always sets them to zero, but that's not really something that
> ipu_cpmem_set_image() should assume.

For the time being, I've restored the functionality along the same lines
as I originally had.  This seems to get me working capture, but might
break non-bayer passthrough mode:

diff --git a/drivers/staging/media/imx/imx-media-csi.c 
b/drivers/staging/media/imx/imx-media-csi.c
index fc0036aa84d0..df336971a009 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -314,14 +314,6 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
image.phys0 = phys[0];
image.phys1 = phys[1];
 
-   ret = ipu_cpmem_set_image(priv->idmac_ch, );
-   if (ret)
-   return ret;
-
-   burst_size = (image.pix.width & 0xf) ? 8 : 16;
-
-   ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size);
-
/*
 * Check for conditions that require the IPU to handle the
 * data internally as generic data, aka passthrough mode:
@@ -346,15 +338,29 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
passthrough_bits = 16;
break;
default:
+   burst_size = (image.pix.width & 0xf) ? 8 : 16;
passthrough = (sensor_ep->bus_type != V4L2_MBUS_CSI2 &&
   sensor_ep->bus.parallel.bus_width >= 16);
passthrough_bits = 16;
break;
}
 
-   if (passthrough)
+   if (passthrough) {
+   ipu_cpmem_set_resolution(priv->idmac_ch, image.rect.width,
+image.rect.height);
+   

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 12, 2017 at 09:26:41PM -0700, Steve Longerbeam wrote:
> On 03/12/2017 01:22 PM, Russell King - ARM Linux wrote:
> >What I had was this patch for your v3.  I never got to testing your
> >v4 because of the LP-11 problem.
> >
> >In v5, you've changed to propagate the ipu_cpmem_set_image() error
> >code to avoid the resulting corruption, but that leaves the other bits
> >of this patch unaddressed, along my "media: imx: smfc: add support
> >for bayer formats" patch.
> >
> >Your driver basically has no support for bayer formats.
> 
> You added the patches to this driver that adds the bayer support,
> I don't think there is anything more required of the driver at this
> point to support bayer, the remaining work needs to happen in the IPUv3
> driver.

There is more work, because the way you've merged my changes to
imx_smfc_setup_channel() into csi_idmac_setup_channel() is wrong with
respect to the burst size.

You always set it to 8 or 16 depending on the width:

burst_size = (image.pix.width & 0xf) ? 8 : 16;

ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size);

and then you have my switch() statement which assigns burst_size.
My _tested_ code removed the above, added the switch, which had
a default case which reflected the above setting:

default:
burst_size = (outfmt->width & 0xf) ? 8 : 16;

and then went on to set the burst size _after_ the switch statement:

ipu_cpmem_set_burstsize(priv->smfc_ch, burst_size);

The effect is unchanged for non-bayer formats.  For bayer formats, the
burst size is determined by the bayer data size.

So, even if it's appropriate to fix ipu_cpmem_set_image(), fixing the
above is still required.

I'm not convinced that fixing ipu_cpmem_set_image() is even the best
solution - it's not as trivial as it looks on the surface:

ipu_cpmem_set_resolution(ch, image->rect.width, image->rect.height);
ipu_cpmem_set_stride(ch, pix->bytesperline);

this is fine, it doesn't depend on the format.  However, the next line:

ipu_cpmem_set_fmt(ch, v4l2_pix_fmt_to_drm_fourcc(pix->pixelformat));

does - v4l2_pix_fmt_to_drm_fourcc() is a locally defined function (it
isn't v4l2 code) that converts a v4l2 pixel format to a DRM fourcc.
DRM knows nothing about bayer formats, there aren't fourcc codes in
DRM for it.  The result is that v4l2_pix_fmt_to_drm_fourcc() returns
-EINVAL cast to a u32, which gets passed unchecked into ipu_cpmem_set_fmt().

ipu_cpmem_set_fmt() won't recognise that, and also returns -EINVAL - and
it's a bug that this is not checked and propagated.  If it is checked and
propagated, then we need this to support bayer formats, and I don't see
DRM people wanting bayer format fourcc codes added without there being
a real DRM driver wanting to use them.

Then there's the business of calculating the top-left offset of the image,
which for bayer always needs to be an even number of pixels - as this
function takes the top-left offset, it ought to respect it, but if it
doesn't meet this criteria, what should it do?  csi_idmac_setup_channel()
always sets them to zero, but that's not really something that
ipu_cpmem_set_image() should assume.

-- 
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 v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/12/2017 01:22 PM, Russell King - ARM Linux wrote:

On Sun, Mar 12, 2017 at 01:05:06PM -0700, Steve Longerbeam wrote:



On 03/12/2017 12:57 PM, Russell King - ARM Linux wrote:

On Sat, Mar 11, 2017 at 04:30:53PM -0800, Steve Longerbeam wrote:

If it's too difficult to get the imx219 csi-2 transmitter into the
LP-11 state on power on, perhaps the csi-2 receiver can be a little
more lenient on the transmitter and make the LP-11 timeout a warning
instead of error-out.

Can you try the attached change on top of the version 5 patchset?

If that doesn't work then you're just going to have to fix the bug
in imx219.


That patch gets me past that hurdle, only to reveal that there's another
issue:


Yeah, ipu_cpmem_set_image() failed because it doesn't recognize the
bayer formats. Wait, didn't we fix this already? I've lost track.
Ah, right, we were going to move this support into the IPUv3 driver,
but in the meantime I think you had some patches to get around this.


What I had was this patch for your v3.  I never got to testing your
v4 because of the LP-11 problem.

In v5, you've changed to propagate the ipu_cpmem_set_image() error
code to avoid the resulting corruption, but that leaves the other bits
of this patch unaddressed, along my "media: imx: smfc: add support
for bayer formats" patch.

Your driver basically has no support for bayer formats.


You added the patches to this driver that adds the bayer support,
I don't think there is anything more required of the driver at this
point to support bayer, the remaining work needs to happen in the IPUv3
driver.

I'll see if I have time to write that patch to IPUv3, but it's simple,
in fact what you wrote below can be translate directly into
ipu_cpmem_set_image(). There's a few other places bayer needs to be
treated in IPUv3, but it should be obvious by grepping for the
reference to pixel formats.

Steve




diff --git a/drivers/staging/media/imx/imx-smfc.c 
b/drivers/staging/media/imx/imx-smfc.c
index 313732201a52..4351c0365cf4 100644
--- a/drivers/staging/media/imx/imx-smfc.c
+++ b/drivers/staging/media/imx/imx-smfc.c
@@ -234,11 +234,6 @@ static void imx_smfc_setup_channel(struct imx_smfc_priv 
*priv)
buf1 = imx_media_dma_buf_get_next_queued(priv->out_ring);
priv->next = buf1;

-   image.phys0 = buf0->phys;
-   image.phys1 = buf1->phys;
-   ipu_cpmem_set_image(priv->smfc_ch, );
-
-
switch (image.pix.pixelformat) {
case V4L2_PIX_FMT_SBGGR8:
case V4L2_PIX_FMT_SGBRG8:
@@ -247,6 +242,10 @@ static void imx_smfc_setup_channel(struct imx_smfc_priv 
*priv)
burst_size = 8;
passthrough = true;
passthrough_bits = 8;
+   ipu_cpmem_set_resolution(priv->smfc_ch, image.rect.width, 
image.rect.height);
+   ipu_cpmem_set_stride(priv->smfc_ch, image.pix.bytesperline);
+   ipu_cpmem_set_buffer(priv->smfc_ch, 0, buf0->phys);
+   ipu_cpmem_set_buffer(priv->smfc_ch, 1, buf1->phys);
break;

case V4L2_PIX_FMT_SBGGR16:
@@ -256,9 +255,17 @@ static void imx_smfc_setup_channel(struct imx_smfc_priv 
*priv)
burst_size = 4;
passthrough = true;
passthrough_bits = 16;
+   ipu_cpmem_set_resolution(priv->smfc_ch, image.rect.width, 
image.rect.height);
+   ipu_cpmem_set_stride(priv->smfc_ch, image.pix.bytesperline);
+   ipu_cpmem_set_buffer(priv->smfc_ch, 0, buf0->phys);
+   ipu_cpmem_set_buffer(priv->smfc_ch, 1, buf1->phys);
break;

default:
+   image.phys0 = buf0->phys;
+   image.phys1 = buf1->phys;
+   ipu_cpmem_set_image(priv->smfc_ch, );
+
burst_size = (outfmt->width & 0xf) ? 8 : 16;

/*

Re: [PATCH v5 00/39] i.MX Media Driver

[Mauro Carvalho Chehab]

Em Sun, 12 Mar 2017 21:13:24 +
Russell King - ARM Linux  escreveu:

> On Sun, Mar 12, 2017 at 05:59:28PM -0300, Mauro Carvalho Chehab wrote:
> > Yet, udev/systemd has some rules that provide an unique name for V4L
> > devices at /lib/udev/rules.d/60-persistent-v4l.rules. Basically, it
> > runs a small application (v4l_id) with creates a persistent symling
> > using rules like this:
> > 
> > KERNEL=="video*", ENV{ID_SERIAL}=="?*", 
> > SYMLINK+="v4l/by-id/$env{ID_BUS}-$env{ID_SERIAL}-video-index$attr{index}"
> > 
> > Those names are stored at /dev/v4l/by-path.  
> 
> This doesn't help:
> 
> $ ls -Al /dev/v4l/by-id/
> total 0
> lrwxrwxrwx 1 root root 13 Mar 12 19:54 
> usb-Sonix_Technology_Co.__Ltd._USB_2.0_Camera-video-index0 -> ../../video10
> $ ls -Al /dev/v4l/by-path/
> total 0
> lrwxrwxrwx 1 root root 12 Mar 12 19:54 platform-204.vpu-video-index0 -> 
> ../../video0
> lrwxrwxrwx 1 root root 12 Mar 12 19:54 platform-204.vpu-video-index1 -> 
> ../../video1
> lrwxrwxrwx 1 root root 12 Mar 12 20:53 
> platform-capture-subsystem-video-index0 -> ../../video2
> lrwxrwxrwx 1 root root 12 Mar 12 20:53 
> platform-capture-subsystem-video-index1 -> ../../video3
> lrwxrwxrwx 1 root root 12 Mar 12 20:53 
> platform-capture-subsystem-video-index2 -> ../../video4
> lrwxrwxrwx 1 root root 12 Mar 12 20:53 
> platform-capture-subsystem-video-index3 -> ../../video5
> lrwxrwxrwx 1 root root 12 Mar 12 20:53 
> platform-capture-subsystem-video-index4 -> ../../video6
> lrwxrwxrwx 1 root root 12 Mar 12 20:53 
> platform-capture-subsystem-video-index5 -> ../../video7
> lrwxrwxrwx 1 root root 12 Mar 12 20:53 
> platform-capture-subsystem-video-index6 -> ../../video8
> lrwxrwxrwx 1 root root 12 Mar 12 20:53 
> platform-capture-subsystem-video-index7 -> ../../video9
> lrwxrwxrwx 1 root root 13 Mar 12 19:54 
> platform-ci_hdrc.0-usb-0:1:1.0-video-index0 -> ../../video10
> 
> The problem is the "platform-capture-subsystem-video-index" entries.
> These themselves change order.  For instance, I now have:
> 
> - entity 72: ipu1_csi0 capture (1 pad, 1 link)
>  type Node subtype V4L flags 0
>  device node name /dev/video6
> 
> which means it's platform-capture-subsystem-video-index4.  Before, it
> was platform-capture-subsystem-video-index2.

That's a driver problem. v4l_id gets information to build the persistent
name from the result of VIDIOC_QUERYCAP.

In the case of Exynos gsc driver, for example, the information is here:

static int gsc_m2m_querycap(struct file *file, void *fh,
   struct v4l2_capability *cap)
{
struct gsc_ctx *ctx = fh_to_ctx(fh);
struct gsc_dev *gsc = ctx->gsc_dev;

strlcpy(cap->driver, GSC_MODULE_NAME, sizeof(cap->driver));
strlcpy(cap->card, GSC_MODULE_NAME " gscaler", sizeof(cap->card));
snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
 dev_name(>pdev->dev));
cap->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_M2M_MPLANE |
V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_VIDEO_OUTPUT_MPLANE;

cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
return 0;
}

See that the bus_info there is filled with:

snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", 
dev_name(>pdev->dev));

>From the output you printed, it seems that the i.MX6 is just doing:
snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:");
for some devices.

If you change the i.MX6 driver to do the same, you'll likely be able to
have unique names there too.

Regards,
Mauro

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 12, 2017 at 05:59:28PM -0300, Mauro Carvalho Chehab wrote:
> Yet, udev/systemd has some rules that provide an unique name for V4L
> devices at /lib/udev/rules.d/60-persistent-v4l.rules. Basically, it
> runs a small application (v4l_id) with creates a persistent symling
> using rules like this:
> 
>   KERNEL=="video*", ENV{ID_SERIAL}=="?*", 
> SYMLINK+="v4l/by-id/$env{ID_BUS}-$env{ID_SERIAL}-video-index$attr{index}"
> 
> Those names are stored at /dev/v4l/by-path.

This doesn't help:

$ ls -Al /dev/v4l/by-id/
total 0
lrwxrwxrwx 1 root root 13 Mar 12 19:54 
usb-Sonix_Technology_Co.__Ltd._USB_2.0_Camera-video-index0 -> ../../video10
$ ls -Al /dev/v4l/by-path/
total 0
lrwxrwxrwx 1 root root 12 Mar 12 19:54 platform-204.vpu-video-index0 -> 
../../video0
lrwxrwxrwx 1 root root 12 Mar 12 19:54 platform-204.vpu-video-index1 -> 
../../video1
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index0 
-> ../../video2
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index1 
-> ../../video3
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index2 
-> ../../video4
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index3 
-> ../../video5
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index4 
-> ../../video6
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index5 
-> ../../video7
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index6 
-> ../../video8
lrwxrwxrwx 1 root root 12 Mar 12 20:53 platform-capture-subsystem-video-index7 
-> ../../video9
lrwxrwxrwx 1 root root 13 Mar 12 19:54 
platform-ci_hdrc.0-usb-0:1:1.0-video-index0 -> ../../video10

The problem is the "platform-capture-subsystem-video-index" entries.
These themselves change order.  For instance, I now have:

- entity 72: ipu1_csi0 capture (1 pad, 1 link)
 type Node subtype V4L flags 0
 device node name /dev/video6

which means it's platform-capture-subsystem-video-index4.  Before, it
was platform-capture-subsystem-video-index2.

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 12, 2017 at 08:40:37PM +, Russell King - ARM Linux wrote:
> On Sun, Mar 12, 2017 at 01:36:32PM -0700, Steve Longerbeam wrote:
> > But hold on, if my logic is correct, then why did the CSI power-off
> > get reached in your case, multiple times? Yes I think there is a bug,
> > link_notify() is not checking if the link has already been disabled.
> > I will fix this. But I'm surprised media core's link_notify handling
> > doesn't do this.
> 
> Well, I think there's something incredibly fishy going on here.  I
> turned that dev_dbg() at the top of the function into a dev_info(),
> and I get:
> 
> root@hbi2ex:~# dmesg |grep -A2 imx-ipuv3-csi
> [   53.370949] imx-ipuv3-csi imx-ipuv3-csi.0: power OFF
> [   53.371015] [ cut here ]
> [   53.371075] WARNING: CPU: 0 PID: 1515 at 
> drivers/staging/media/imx/imx-media-csi.c:806 csi_s_power+0xb8/0xd0 
> [imx_media_csi]
> --
> [   53.372624] imx-ipuv3-csi imx-ipuv3-csi.0: power OFF
> [   53.372637] [ cut here ]
> [   53.372663] WARNING: CPU: 0 PID: 1515 at 
> drivers/staging/media/imx/imx-media-csi.c:806 csi_s_power+0xb8/0xd0 
> [imx_media_csi]
> 
> There isn't a power on event being generated before these two power
> off events.  I don't see a power on event even when I attempt to
> start streaming either (which fails due to the lack of bayer
> support.)

Found it - my imx219 driver returns '1' from its s_power function when
powering up, which triggers a bug in your code - when imx_media_set_power()
fails to power up, you call imx_media_set_power() telling it to power
everything off - including devices that are already powered off.

This is really bad news - s_power() may be called via other paths,
such as when the subdev is opened.

-- 
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 v5 00/39] i.MX Media Driver

[Mauro Carvalho Chehab]

Em Sun, 12 Mar 2017 19:47:00 +
Russell King - ARM Linux  escreveu:

> Another issue.
> 
> The "reboot and the /dev/video* devices come up in a completely
> different order" problem seems to exist with this version.
> 
> The dot graph I supplied previously had "ipu1_csi0 capture" on
> /dev/video4.  I've just rebooted, and now I find it's on
> /dev/video2 instead.
> 
> Here's the extract from the .dot file of the old listing:
> 
> n0018 [label="ipu1_ic_prpenc capture\n/dev/video0", shape=box, 
> style=filled, fillcolor=yellow]
> n0021 [label="ipu1_ic_prpvf capture\n/dev/video1", shape=box, 
> style=filled, fillcolor=yellow]
> n002e [label="ipu2_ic_prpenc capture\n/dev/video2", shape=box, 
> style=filled, fillcolor=yellow]
> n0037 [label="ipu2_ic_prpvf capture\n/dev/video3", shape=box, 
> style=filled, fillcolor=yellow]
> n0048 [label="ipu1_csi0 capture\n/dev/video4", shape=box, 
> style=filled, fillcolor=yellow]
> n0052 [label="ipu1_csi1 capture\n/dev/video5", shape=box, 
> style=filled, fillcolor=yellow]
> n0062 [label="ipu2_csi0 capture\n/dev/video6", shape=box, 
> style=filled, fillcolor=yellow]
> n006c [label="ipu2_csi1 capture\n/dev/video7", shape=box, 
> style=filled, fillcolor=yellow]
> 
> and here's the same after reboot:
> 
> n0014 [label="ipu1_csi0 capture\n/dev/video2", shape=box, 
> style=filled, fillcolor=yellow]
> n001e [label="ipu1_csi1 capture\n/dev/video3", shape=box, 
> style=filled, fillcolor=yellow]
> n0028 [label="ipu2_csi0 capture\n/dev/video4", shape=box, 
> style=filled, fillcolor=yellow]
> n0035 [label="ipu1_ic_prpenc capture\n/dev/video5", shape=box, 
> style=filled, fillcolor=yellow]
> n003e [label="ipu1_ic_prpvf capture\n/dev/video6", shape=box, 
> style=filled, fillcolor=yellow]
> n004c [label="ipu2_csi1 capture\n/dev/video7", shape=box, 
> style=filled, fillcolor=yellow]
> n0059 [label="ipu2_ic_prpenc capture\n/dev/video8", shape=box, 
> style=filled, fillcolor=yellow]
> n0062 [label="ipu2_ic_prpvf capture\n/dev/video9", shape=box, 
> style=filled, fillcolor=yellow]
> 
> (/dev/video0 and /dev/video1 are taken up by CODA, since I updated the
> names of the firmware files, and now CODA initialises... seems the
> back-compat filenames don't work, but that's not a problem with imx6
> capture.)
> 

Didn't have time yet to read/comment the other e-mails in this thread.

Yet, as this is a simple issue, let me answer it first.

With regards to /dev/video?, the device number depends on the probing 
order, with can be random on SoC drivers, due to the way OF works. 

Yet, udev/systemd has some rules that provide an unique name for V4L
devices at /lib/udev/rules.d/60-persistent-v4l.rules. Basically, it
runs a small application (v4l_id) with creates a persistent symling
using rules like this:

KERNEL=="video*", ENV{ID_SERIAL}=="?*", 
SYMLINK+="v4l/by-id/$env{ID_BUS}-$env{ID_SERIAL}-video-index$attr{index}"

Those names are stored at /dev/v4l/by-path.

For example, on Exynos, we have:

$ ls -lctra /dev/v4l/by-path/
total 0
lrwxrwxrwx 1 root root  12 Mar 11 07:19 
platform-13e1.video-scaler-video-index0 -> ../../video7
lrwxrwxrwx 1 root root  12 Mar 11 07:19 
platform-13e0.video-scaler-video-index0 -> ../../video6
lrwxrwxrwx 1 root root  12 Mar 11 07:19 platform-11f6.jpeg-video-index0 -> 
../../video4
lrwxrwxrwx 1 root root  12 Mar 11 07:19 platform-11f5.jpeg-video-index1 -> 
../../video3
lrwxrwxrwx 1 root root  12 Mar 11 07:19 platform-11f5.jpeg-video-index0 -> 
../../video2
drwxr-xr-x 3 root root  60 Mar 11 07:19 ..
lrwxrwxrwx 1 root root  12 Mar 11 07:19 platform-11f6.jpeg-video-index1 -> 
../../video5
lrwxrwxrwx 1 root root  12 Mar 11 07:19 platform-1100.codec-video-index1 -> 
../../video1
lrwxrwxrwx 1 root root  12 Mar 11 07:19 platform-1100.codec-video-index0 -> 
../../video0

No matter what driver gets probed first, the above names should not
change.

So, if you want to write a script, the best is to use the /dev/v4l/by-path.

Unfortunately, gstreamer has some issues with that, as some of their plugins
don't seem to allow passing the name of the devnode, but just the number of
/dev/video?.

So, you need some script to convert from /dev/v4l/by-path/foo to
/dev/video?.

What I'm using on Exynos scripts is this logic:

NEEDED1=platform-13e0.video-scaler-video-index0
DEV1=$(ls -l /dev/v4l/by-path/$NEEDED1|perl -ne ' print $1 if 
(m,/video(\d+),)')

Then, if I need to talk with this mem2mem driver using the v4l2video
convert plugin, I can launch gst with something like:

gst-launch-1.0 videotestsrc ! v4l2video${DEV1}convert ! fakesink

Thanks,
Mauro


Thanks,
Mauro

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 12, 2017 at 01:36:32PM -0700, Steve Longerbeam wrote:
> But hold on, if my logic is correct, then why did the CSI power-off
> get reached in your case, multiple times? Yes I think there is a bug,
> link_notify() is not checking if the link has already been disabled.
> I will fix this. But I'm surprised media core's link_notify handling
> doesn't do this.

Well, I think there's something incredibly fishy going on here.  I
turned that dev_dbg() at the top of the function into a dev_info(),
and I get:

root@hbi2ex:~# dmesg |grep -A2 imx-ipuv3-csi
[   53.370949] imx-ipuv3-csi imx-ipuv3-csi.0: power OFF
[   53.371015] [ cut here ]
[   53.371075] WARNING: CPU: 0 PID: 1515 at 
drivers/staging/media/imx/imx-media-csi.c:806 csi_s_power+0xb8/0xd0 
[imx_media_csi]
--
[   53.372624] imx-ipuv3-csi imx-ipuv3-csi.0: power OFF
[   53.372637] [ cut here ]
[   53.372663] WARNING: CPU: 0 PID: 1515 at 
drivers/staging/media/imx/imx-media-csi.c:806 csi_s_power+0xb8/0xd0 
[imx_media_csi]

There isn't a power on event being generated before these two power
off events.  I don't see a power on event even when I attempt to
start streaming either (which fails due to the lack of bayer
support.)

-- 
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 v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/12/2017 01:36 PM, Steve Longerbeam wrote:



On 03/12/2017 01:16 PM, Steve Longerbeam wrote:



On 03/12/2017 12:44 PM, Steve Longerbeam wrote:



On 03/12/2017 12:29 PM, Russell King - ARM Linux wrote:

On Sun, Mar 12, 2017 at 12:21:45PM -0700, Steve Longerbeam wrote:

There's actually nothing preventing userland from disabling a link
multiple times, and imx_media_link_notify() complies, and so
csi_s_power(OFF) gets called multiple times, and so that WARN_ON()
in there is silly, I borrowed this from other MC driver examples,
but it makes no sense to me, I'll remove it and prevent the power
count from going negative.


Hmm.  So what happens if one of the CSI's links is enabled, and we
disable a different link from the CSI several times?  Doesn't that
mean the power count will go to zero despite there being an enabled
link?


Yes, the CSI will be powered off even if it still has an enabled link.
But one of its other links has been disabled, meaning the pipeline as
a whole is disabled. So I think it makes sense to power down the CSI,
the pipeline isn't usable at that point.

And remember that the CSI does not allow both output pads to be enabled
at the same time. If that were so then indeed there would be a problem,
because it would mean there is another active pipeline that requires the
CSI being powered on, but that's not the case.

I think this is consistent with the other entities as well, but I will
double check.



At first I thought this could be a problem for one entity, the csi-2
receiver.

It can enable all four of its output pads at once (if the input stream
contains all 4 virtual channels, the csi-2 receiver must support
demuxing all of them onto all 4 of its output pads).

But after more review, this should not be an issue. If a csi-2 sink
(a CSI or a CSI mux) link is disabled, the csi-2 receiver is no longer
reachable from that sink, so attempts to disable the csi-2 via that
path again is not possible. The other potential problem is disabling
from the csi-2's own sink pad, but in that case the csi-2 no longer
has a source, so again it makes sense to power off the csi-2 even
if it has enabled output pads.



But hold on, if my logic is correct, then why did the CSI power-off
get reached in your case, multiple times? Yes I think there is a bug,
link_notify() is not checking if the link has already been disabled.
I will fix this. But I'm surprised media core's link_notify handling
doesn't do this.


but it does:

int __media_entity_setup_link(struct media_link *link, u32 flags)
{
...
if (link->flags == flags)
return 0;
...
}

What the heck. Anyway, I'll track this down.

Steve



Steve

Re: [PATCH v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/12/2017 01:16 PM, Steve Longerbeam wrote:



On 03/12/2017 12:44 PM, Steve Longerbeam wrote:



On 03/12/2017 12:29 PM, Russell King - ARM Linux wrote:

On Sun, Mar 12, 2017 at 12:21:45PM -0700, Steve Longerbeam wrote:

There's actually nothing preventing userland from disabling a link
multiple times, and imx_media_link_notify() complies, and so
csi_s_power(OFF) gets called multiple times, and so that WARN_ON()
in there is silly, I borrowed this from other MC driver examples,
but it makes no sense to me, I'll remove it and prevent the power
count from going negative.


Hmm.  So what happens if one of the CSI's links is enabled, and we
disable a different link from the CSI several times?  Doesn't that
mean the power count will go to zero despite there being an enabled
link?


Yes, the CSI will be powered off even if it still has an enabled link.
But one of its other links has been disabled, meaning the pipeline as
a whole is disabled. So I think it makes sense to power down the CSI,
the pipeline isn't usable at that point.

And remember that the CSI does not allow both output pads to be enabled
at the same time. If that were so then indeed there would be a problem,
because it would mean there is another active pipeline that requires the
CSI being powered on, but that's not the case.

I think this is consistent with the other entities as well, but I will
double check.



At first I thought this could be a problem for one entity, the csi-2
receiver.

It can enable all four of its output pads at once (if the input stream
contains all 4 virtual channels, the csi-2 receiver must support
demuxing all of them onto all 4 of its output pads).

But after more review, this should not be an issue. If a csi-2 sink
(a CSI or a CSI mux) link is disabled, the csi-2 receiver is no longer
reachable from that sink, so attempts to disable the csi-2 via that
path again is not possible. The other potential problem is disabling
from the csi-2's own sink pad, but in that case the csi-2 no longer
has a source, so again it makes sense to power off the csi-2 even
if it has enabled output pads.



But hold on, if my logic is correct, then why did the CSI power-off
get reached in your case, multiple times? Yes I think there is a bug,
link_notify() is not checking if the link has already been disabled.
I will fix this. But I'm surprised media core's link_notify handling
doesn't do this.

Steve

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 12, 2017 at 01:05:06PM -0700, Steve Longerbeam wrote:
> 
> 
> On 03/12/2017 12:57 PM, Russell King - ARM Linux wrote:
> >On Sat, Mar 11, 2017 at 04:30:53PM -0800, Steve Longerbeam wrote:
> >>If it's too difficult to get the imx219 csi-2 transmitter into the
> >>LP-11 state on power on, perhaps the csi-2 receiver can be a little
> >>more lenient on the transmitter and make the LP-11 timeout a warning
> >>instead of error-out.
> >>
> >>Can you try the attached change on top of the version 5 patchset?
> >>
> >>If that doesn't work then you're just going to have to fix the bug
> >>in imx219.
> >
> >That patch gets me past that hurdle, only to reveal that there's another
> >issue:
> 
> Yeah, ipu_cpmem_set_image() failed because it doesn't recognize the
> bayer formats. Wait, didn't we fix this already? I've lost track.
> Ah, right, we were going to move this support into the IPUv3 driver,
> but in the meantime I think you had some patches to get around this.

What I had was this patch for your v3.  I never got to testing your
v4 because of the LP-11 problem.

In v5, you've changed to propagate the ipu_cpmem_set_image() error
code to avoid the resulting corruption, but that leaves the other bits
of this patch unaddressed, along my "media: imx: smfc: add support
for bayer formats" patch.

Your driver basically has no support for bayer formats.

diff --git a/drivers/staging/media/imx/imx-smfc.c 
b/drivers/staging/media/imx/imx-smfc.c
index 313732201a52..4351c0365cf4 100644
--- a/drivers/staging/media/imx/imx-smfc.c
+++ b/drivers/staging/media/imx/imx-smfc.c
@@ -234,11 +234,6 @@ static void imx_smfc_setup_channel(struct imx_smfc_priv 
*priv)
buf1 = imx_media_dma_buf_get_next_queued(priv->out_ring);
priv->next = buf1;
 
-   image.phys0 = buf0->phys;
-   image.phys1 = buf1->phys;
-   ipu_cpmem_set_image(priv->smfc_ch, );
-
-
switch (image.pix.pixelformat) {
case V4L2_PIX_FMT_SBGGR8:
case V4L2_PIX_FMT_SGBRG8:
@@ -247,6 +242,10 @@ static void imx_smfc_setup_channel(struct imx_smfc_priv 
*priv)
burst_size = 8;
passthrough = true;
passthrough_bits = 8;
+   ipu_cpmem_set_resolution(priv->smfc_ch, image.rect.width, 
image.rect.height);
+   ipu_cpmem_set_stride(priv->smfc_ch, image.pix.bytesperline);
+   ipu_cpmem_set_buffer(priv->smfc_ch, 0, buf0->phys);
+   ipu_cpmem_set_buffer(priv->smfc_ch, 1, buf1->phys);
break;
 
case V4L2_PIX_FMT_SBGGR16:
@@ -256,9 +255,17 @@ static void imx_smfc_setup_channel(struct imx_smfc_priv 
*priv)
burst_size = 4;
passthrough = true;
passthrough_bits = 16;
+   ipu_cpmem_set_resolution(priv->smfc_ch, image.rect.width, 
image.rect.height);
+   ipu_cpmem_set_stride(priv->smfc_ch, image.pix.bytesperline);
+   ipu_cpmem_set_buffer(priv->smfc_ch, 0, buf0->phys);
+   ipu_cpmem_set_buffer(priv->smfc_ch, 1, buf1->phys);
break;
 
default:
+   image.phys0 = buf0->phys;
+   image.phys1 = buf1->phys;
+   ipu_cpmem_set_image(priv->smfc_ch, );
+
burst_size = (outfmt->width & 0xf) ? 8 : 16;
 
/*

-- 
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 v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/12/2017 12:44 PM, Steve Longerbeam wrote:



On 03/12/2017 12:29 PM, Russell King - ARM Linux wrote:

On Sun, Mar 12, 2017 at 12:21:45PM -0700, Steve Longerbeam wrote:

There's actually nothing preventing userland from disabling a link
multiple times, and imx_media_link_notify() complies, and so
csi_s_power(OFF) gets called multiple times, and so that WARN_ON()
in there is silly, I borrowed this from other MC driver examples,
but it makes no sense to me, I'll remove it and prevent the power
count from going negative.


Hmm.  So what happens if one of the CSI's links is enabled, and we
disable a different link from the CSI several times?  Doesn't that
mean the power count will go to zero despite there being an enabled
link?


Yes, the CSI will be powered off even if it still has an enabled link.
But one of its other links has been disabled, meaning the pipeline as
a whole is disabled. So I think it makes sense to power down the CSI,
the pipeline isn't usable at that point.

And remember that the CSI does not allow both output pads to be enabled
at the same time. If that were so then indeed there would be a problem,
because it would mean there is another active pipeline that requires the
CSI being powered on, but that's not the case.

I think this is consistent with the other entities as well, but I will
double check.



At first I thought this could be a problem for one entity, the csi-2
receiver.

It can enable all four of its output pads at once (if the input stream
contains all 4 virtual channels, the csi-2 receiver must support
demuxing all of them onto all 4 of its output pads).

But after more review, this should not be an issue. If a csi-2 sink
(a CSI or a CSI mux) link is disabled, the csi-2 receiver is no longer
reachable from that sink, so attempts to disable the csi-2 via that
path again is not possible. The other potential problem is disabling
from the csi-2's own sink pad, but in that case the csi-2 no longer
has a source, so again it makes sense to power off the csi-2 even
if it has enabled output pads.


Steve

Re: [PATCH v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/12/2017 12:57 PM, Russell King - ARM Linux wrote:

On Sat, Mar 11, 2017 at 04:30:53PM -0800, Steve Longerbeam wrote:

If it's too difficult to get the imx219 csi-2 transmitter into the
LP-11 state on power on, perhaps the csi-2 receiver can be a little
more lenient on the transmitter and make the LP-11 timeout a warning
instead of error-out.

Can you try the attached change on top of the version 5 patchset?

If that doesn't work then you're just going to have to fix the bug
in imx219.


That patch gets me past that hurdle, only to reveal that there's another
issue:


Yeah, ipu_cpmem_set_image() failed because it doesn't recognize the
bayer formats. Wait, didn't we fix this already? I've lost track.
Ah, right, we were going to move this support into the IPUv3 driver,
but in the meantime I think you had some patches to get around this.

Steve




imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
imx219 0-0010: VT: pixclk 13920Hz line 80742Hz frame 30.0Hz
imx219 0-0010: VT: line period 12385ns
imx219 0-0010: OP: pixclk 3850Hz, 2 lanes, 308Mbps peak each
imx219 0-0010: OP: 3288 bits/line/lane act=10675ns lp/idle=1710ns
ipu1_csi0: csi_idmac_setup failed: -22
ipu1_csi0: pipeline start failed with -22
[ cut here ]
WARNING: CPU: 0 PID: 1860 at 
/home/rmk/git/linux-rmk/drivers/media/v4l2-core/videobuf2-core.c:1340 
vb2_start_streaming+0x124/0x1b4 [videobuf2_core]

Re: [PATCH v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/12/2017 12:47 PM, Russell King - ARM Linux wrote:

Another issue.

The "reboot and the /dev/video* devices come up in a completely
different order" problem seems to exist with this version.

The dot graph I supplied previously had "ipu1_csi0 capture" on
/dev/video4.  I've just rebooted, and now I find it's on
/dev/video2 instead.


Yes, that's still an issue I haven't had the chance to get to
yet.

It could be as simple as passing a fixed device node # to
video_register_device(), but something tells me it won't be
that easy. But I'll get to this in next version.

Steve



Here's the extract from the .dot file of the old listing:

n0018 [label="ipu1_ic_prpenc capture\n/dev/video0", shape=box, 
style=filled, fillcolor=yellow]
n0021 [label="ipu1_ic_prpvf capture\n/dev/video1", shape=box, 
style=filled, fillcolor=yellow]
n002e [label="ipu2_ic_prpenc capture\n/dev/video2", shape=box, 
style=filled, fillcolor=yellow]
n0037 [label="ipu2_ic_prpvf capture\n/dev/video3", shape=box, 
style=filled, fillcolor=yellow]
n0048 [label="ipu1_csi0 capture\n/dev/video4", shape=box, 
style=filled, fillcolor=yellow]
n0052 [label="ipu1_csi1 capture\n/dev/video5", shape=box, 
style=filled, fillcolor=yellow]
n0062 [label="ipu2_csi0 capture\n/dev/video6", shape=box, 
style=filled, fillcolor=yellow]
n006c [label="ipu2_csi1 capture\n/dev/video7", shape=box, 
style=filled, fillcolor=yellow]

and here's the same after reboot:

n0014 [label="ipu1_csi0 capture\n/dev/video2", shape=box, 
style=filled, fillcolor=yellow]
n001e [label="ipu1_csi1 capture\n/dev/video3", shape=box, 
style=filled, fillcolor=yellow]
n0028 [label="ipu2_csi0 capture\n/dev/video4", shape=box, 
style=filled, fillcolor=yellow]
n0035 [label="ipu1_ic_prpenc capture\n/dev/video5", shape=box, 
style=filled, fillcolor=yellow]
n003e [label="ipu1_ic_prpvf capture\n/dev/video6", shape=box, 
style=filled, fillcolor=yellow]
n004c [label="ipu2_csi1 capture\n/dev/video7", shape=box, 
style=filled, fillcolor=yellow]
n0059 [label="ipu2_ic_prpenc capture\n/dev/video8", shape=box, 
style=filled, fillcolor=yellow]
n0062 [label="ipu2_ic_prpvf capture\n/dev/video9", shape=box, 
style=filled, fillcolor=yellow]

(/dev/video0 and /dev/video1 are taken up by CODA, since I updated the
names of the firmware files, and now CODA initialises... seems the
back-compat filenames don't work, but that's not a problem with imx6
capture.)

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sat, Mar 11, 2017 at 04:30:53PM -0800, Steve Longerbeam wrote:
> If it's too difficult to get the imx219 csi-2 transmitter into the
> LP-11 state on power on, perhaps the csi-2 receiver can be a little
> more lenient on the transmitter and make the LP-11 timeout a warning
> instead of error-out.
> 
> Can you try the attached change on top of the version 5 patchset?
> 
> If that doesn't work then you're just going to have to fix the bug
> in imx219.

That patch gets me past that hurdle, only to reveal that there's another
issue:

imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
imx219 0-0010: VT: pixclk 13920Hz line 80742Hz frame 30.0Hz
imx219 0-0010: VT: line period 12385ns
imx219 0-0010: OP: pixclk 3850Hz, 2 lanes, 308Mbps peak each
imx219 0-0010: OP: 3288 bits/line/lane act=10675ns lp/idle=1710ns
ipu1_csi0: csi_idmac_setup failed: -22
ipu1_csi0: pipeline start failed with -22
[ cut here ]
WARNING: CPU: 0 PID: 1860 at 
/home/rmk/git/linux-rmk/drivers/media/v4l2-core/videobuf2-core.c:1340 
vb2_start_streaming+0x124/0x1b4 [videobuf2_core]

-- 
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 v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

Another issue.

The "reboot and the /dev/video* devices come up in a completely
different order" problem seems to exist with this version.

The dot graph I supplied previously had "ipu1_csi0 capture" on
/dev/video4.  I've just rebooted, and now I find it's on
/dev/video2 instead.

Here's the extract from the .dot file of the old listing:

n0018 [label="ipu1_ic_prpenc capture\n/dev/video0", shape=box, 
style=filled, fillcolor=yellow]
n0021 [label="ipu1_ic_prpvf capture\n/dev/video1", shape=box, 
style=filled, fillcolor=yellow]
n002e [label="ipu2_ic_prpenc capture\n/dev/video2", shape=box, 
style=filled, fillcolor=yellow]
n0037 [label="ipu2_ic_prpvf capture\n/dev/video3", shape=box, 
style=filled, fillcolor=yellow]
n0048 [label="ipu1_csi0 capture\n/dev/video4", shape=box, 
style=filled, fillcolor=yellow]
n0052 [label="ipu1_csi1 capture\n/dev/video5", shape=box, 
style=filled, fillcolor=yellow]
n0062 [label="ipu2_csi0 capture\n/dev/video6", shape=box, 
style=filled, fillcolor=yellow]
n006c [label="ipu2_csi1 capture\n/dev/video7", shape=box, 
style=filled, fillcolor=yellow]

and here's the same after reboot:

n0014 [label="ipu1_csi0 capture\n/dev/video2", shape=box, 
style=filled, fillcolor=yellow]
n001e [label="ipu1_csi1 capture\n/dev/video3", shape=box, 
style=filled, fillcolor=yellow]
n0028 [label="ipu2_csi0 capture\n/dev/video4", shape=box, 
style=filled, fillcolor=yellow]
n0035 [label="ipu1_ic_prpenc capture\n/dev/video5", shape=box, 
style=filled, fillcolor=yellow]
n003e [label="ipu1_ic_prpvf capture\n/dev/video6", shape=box, 
style=filled, fillcolor=yellow]
n004c [label="ipu2_csi1 capture\n/dev/video7", shape=box, 
style=filled, fillcolor=yellow]
n0059 [label="ipu2_ic_prpenc capture\n/dev/video8", shape=box, 
style=filled, fillcolor=yellow]
n0062 [label="ipu2_ic_prpvf capture\n/dev/video9", shape=box, 
style=filled, fillcolor=yellow]

(/dev/video0 and /dev/video1 are taken up by CODA, since I updated the
names of the firmware files, and now CODA initialises... seems the
back-compat filenames don't work, but that's not a problem with imx6
capture.)

-- 
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 v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/12/2017 12:29 PM, Russell King - ARM Linux wrote:

On Sun, Mar 12, 2017 at 12:21:45PM -0700, Steve Longerbeam wrote:

There's actually nothing preventing userland from disabling a link
multiple times, and imx_media_link_notify() complies, and so
csi_s_power(OFF) gets called multiple times, and so that WARN_ON()
in there is silly, I borrowed this from other MC driver examples,
but it makes no sense to me, I'll remove it and prevent the power
count from going negative.


Hmm.  So what happens if one of the CSI's links is enabled, and we
disable a different link from the CSI several times?  Doesn't that
mean the power count will go to zero despite there being an enabled
link?


Yes, the CSI will be powered off even if it still has an enabled link.
But one of its other links has been disabled, meaning the pipeline as
a whole is disabled. So I think it makes sense to power down the CSI,
the pipeline isn't usable at that point.

And remember that the CSI does not allow both output pads to be enabled
at the same time. If that were so then indeed there would be a problem,
because it would mean there is another active pipeline that requires the
CSI being powered on, but that's not the case.

I think this is consistent with the other entities as well, but I will
double check.

Steve

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Sun, Mar 12, 2017 at 12:21:45PM -0700, Steve Longerbeam wrote:
> There's actually nothing preventing userland from disabling a link
> multiple times, and imx_media_link_notify() complies, and so
> csi_s_power(OFF) gets called multiple times, and so that WARN_ON()
> in there is silly, I borrowed this from other MC driver examples,
> but it makes no sense to me, I'll remove it and prevent the power
> count from going negative.

Hmm.  So what happens if one of the CSI's links is enabled, and we
disable a different link from the CSI several times?  Doesn't that
mean the power count will go to zero despite there being an enabled
link?

-- 
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 v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/12/2017 10:51 AM, Russell King - ARM Linux wrote:

I've just looked at my test system's dmesg, and spotted this in the log.
It's been a while since these popped out of the kernel, so I don't know
what caused them (other than the obvious, a media-ctl command.)

My script which sets this up only enables links, and then configures the
formats etc, and doesn't disable them, so I don't see why the power
count should be going negative.


There's actually nothing preventing userland from disabling a link
multiple times, and imx_media_link_notify() complies, and so
csi_s_power(OFF) gets called multiple times, and so that WARN_ON()
in there is silly, I borrowed this from other MC driver examples,
but it makes no sense to me, I'll remove it and prevent the power
count from going negative.

Steve





[ cut here ]
WARNING: CPU: 1 PID: 1889 at drivers/staging/media/imx/imx-media-csi.c:806 
csi_s_power+0x9c/0xa8 [imx_media_csi]
Modules linked in: caam_jr uvcvideo snd_soc_imx_sgtl5000 snd_soc_fsl_asoc_card 
snd_soc_imx_spdif imx_media_csi(C) imx6_mipi_csi2(C) snd_soc_imx_audmux 
snd_soc_sgtl5000 imx219 imx_media_ic(C) imx_media_capture(C) imx_media_vdic(C) 
caam video_multiplexer imx_sdma coda v4l2_mem2mem videobuf2_v4l2 imx2_wdt 
imx_vdoa videobuf2_dma_contig videobuf2_core videobuf2_vmalloc videobuf2_memops 
snd_soc_fsl_ssi
imx_thermal snd_soc_fsl_spdif imx_pcm_dma imx_media(C) imx_media_common(C) nfsd
rc_pinnacle_pctv_hd dw_hdmi_ahb_audio dw_hdmi_cec etnaviv
CPU: 1 PID: 1889 Comm: media-ctl Tainted: G C  4.11.0-rc1+ #2125
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
Backtrace:
[] (dump_backtrace) from [] (show_stack+0x18/0x1c)
 r6:600e0013 r5: r4: r3:
[] (show_stack) from [] (dump_stack+0xa4/0xdc)
[] (dump_stack) from [] (__warn+0xdc/0x108)
 r6:bf124014 r5: r4: r3:c09ea4a8
[] (__warn) from [] (warn_slowpath_null+0x28/0x30)
 r10:ede00010 r8:ede4a348 r7:d039501c r6:d0395140 r5: r4:d0395010
[] (warn_slowpath_null) from [] (csi_s_power+0x9c/0xa8 
[imx_media_csi])
[] (csi_s_power [imx_media_csi]) from [] 
(imx_media_set_power+0x3c/0x108 [imx_media_common])
 r7:d039501c r6: r5: r4:000c
[] (imx_media_set_power [imx_media_common]) from [] 
(imx_media_pipeline_set_power+0x38/0x40 [imx_media_common])
 r10:0001 r9:0001 r8:ede4a348 r7:ede00010 r6:ede4a348 r5:d039501c
 r4:0001
[] (imx_media_pipeline_set_power [imx_media_common]) from 
[] (imx_media_link_notify+0xf0/0x144 [imx_media])
 r7:ede00010 r6:ed59f900 r5: r4:d039501c
[] (imx_media_link_notify [imx_media]) from [] 
(__media_entity_setup_link+0x110/0x1d8)
 r10:c0347c03 r9:d7eb3dc8 r8:befe92b0 r7:ede00010 r6: r5:0001
 r4:ed59f900 r3:bf052058
[] (__media_entity_setup_link) from [] 
(media_device_setup_link+0x84/0x90)
 r7:ede00010 r6:ede00010 r5:ef3fd810 r4:d7eb3dc8
[] (media_device_setup_link) from [] 
(media_device_ioctl+0xa4/0x148)
 r6: r5:d7eb3dc8 r4:c077b014 r3:c04f9b2c
[] (media_device_ioctl) from [] (media_ioctl+0x38/0x4c)
 r10:ed5eca68 r9:d7eb2000 r8:befe92b0 r7:0003 r6:0003 r5:e82ca280
 r4:c0190304
[] (media_ioctl) from [] (do_vfs_ioctl+0x98/0x9a0)
[] (do_vfs_ioctl) from [] (SyS_ioctl+0x3c/0x60)
 r10: r9:d7eb2000 r8:befe92b0 r7:0003 r6:c0347c03 r5:e82ca280
 r4:e82ca280
[] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x1c)
 r8:c000ff04 r7:0036 r6:000261d0 r5:0001 r4:009162e8 r3:0001
---[ end trace 4fdd40e5adfc4485 ]---
[ cut here ]
WARNING: CPU: 1 PID: 1889 at drivers/staging/media/imx/imx-media-csi.c:806 
csi_s_power+0x9c/0xa8 [imx_media_csi]
Modules linked in: caam_jr uvcvideo snd_soc_imx_sgtl5000 snd_soc_fsl_asoc_card 
snd_soc_imx_spdif imx_media_csi(C) imx6_mipi_csi2(C) snd_soc_imx_audmux 
snd_soc_sgtl5000 imx219 imx_media_ic(C) imx_media_capture(C) imx_media_vdic(C) 
caam video_multiplexer imx_sdma coda v4l2_mem2mem videobuf2_v4l2 imx2_wdt 
imx_vdoa videobuf2_dma_contig videobuf2_core videobuf2_vmalloc videobuf2_memops 
snd_soc_fsl_ssi
imx_thermal snd_soc_fsl_spdif imx_pcm_dma imx_media(C) imx_media_common(C) nfsd
rc_pinnacle_pctv_hd dw_hdmi_ahb_audio dw_hdmi_cec etnaviv
CPU: 1 PID: 1889 Comm: media-ctl Tainted: GWC  4.11.0-rc1+ #2125
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
Backtrace:
[] (dump_backtrace) from [] (show_stack+0x18/0x1c)
 r6:600e0013 r5: r4: r3:
[] (show_stack) from [] (dump_stack+0xa4/0xdc)
[] (dump_stack) from [] (__warn+0xdc/0x108)
 r6:bf124014 r5: r4: r3:c09ea4a8
[] (__warn) from [] (warn_slowpath_null+0x28/0x30)
 r10:ede00010 r8:ede4a348 r7:ee000800 r6:d0395140 r5: r4:d0395010
[] (warn_slowpath_null) from [] (csi_s_power+0x9c/0xa8 
[imx_media_csi])
[] (csi_s_power [imx_media_csi]) from [] 
(imx_media_set_power+0x3c/0x108 [imx_media_common])
 r7:ee000800 r6: r5: r4:000c
[] (imx_media_set_power [imx_media_common]) from [] 

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

I've just looked at my test system's dmesg, and spotted this in the log.
It's been a while since these popped out of the kernel, so I don't know
what caused them (other than the obvious, a media-ctl command.)

My script which sets this up only enables links, and then configures the
formats etc, and doesn't disable them, so I don't see why the power
count should be going negative.

[ cut here ]
WARNING: CPU: 1 PID: 1889 at drivers/staging/media/imx/imx-media-csi.c:806 
csi_s_power+0x9c/0xa8 [imx_media_csi]
Modules linked in: caam_jr uvcvideo snd_soc_imx_sgtl5000 snd_soc_fsl_asoc_card 
snd_soc_imx_spdif imx_media_csi(C) imx6_mipi_csi2(C) snd_soc_imx_audmux 
snd_soc_sgtl5000 imx219 imx_media_ic(C) imx_media_capture(C) imx_media_vdic(C) 
caam video_multiplexer imx_sdma coda v4l2_mem2mem videobuf2_v4l2 imx2_wdt 
imx_vdoa videobuf2_dma_contig videobuf2_core videobuf2_vmalloc videobuf2_memops 
snd_soc_fsl_ssi
imx_thermal snd_soc_fsl_spdif imx_pcm_dma imx_media(C) imx_media_common(C) nfsd
rc_pinnacle_pctv_hd dw_hdmi_ahb_audio dw_hdmi_cec etnaviv
CPU: 1 PID: 1889 Comm: media-ctl Tainted: G C  4.11.0-rc1+ #2125
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
Backtrace:
[] (dump_backtrace) from [] (show_stack+0x18/0x1c)
 r6:600e0013 r5: r4: r3:
[] (show_stack) from [] (dump_stack+0xa4/0xdc)
[] (dump_stack) from [] (__warn+0xdc/0x108)
 r6:bf124014 r5: r4: r3:c09ea4a8
[] (__warn) from [] (warn_slowpath_null+0x28/0x30)
 r10:ede00010 r8:ede4a348 r7:d039501c r6:d0395140 r5: r4:d0395010
[] (warn_slowpath_null) from [] (csi_s_power+0x9c/0xa8 
[imx_media_csi])
[] (csi_s_power [imx_media_csi]) from [] 
(imx_media_set_power+0x3c/0x108 [imx_media_common])
 r7:d039501c r6: r5: r4:000c
[] (imx_media_set_power [imx_media_common]) from [] 
(imx_media_pipeline_set_power+0x38/0x40 [imx_media_common])
 r10:0001 r9:0001 r8:ede4a348 r7:ede00010 r6:ede4a348 r5:d039501c
 r4:0001
[] (imx_media_pipeline_set_power [imx_media_common]) from 
[] (imx_media_link_notify+0xf0/0x144 [imx_media])
 r7:ede00010 r6:ed59f900 r5: r4:d039501c
[] (imx_media_link_notify [imx_media]) from [] 
(__media_entity_setup_link+0x110/0x1d8)
 r10:c0347c03 r9:d7eb3dc8 r8:befe92b0 r7:ede00010 r6: r5:0001
 r4:ed59f900 r3:bf052058
[] (__media_entity_setup_link) from [] 
(media_device_setup_link+0x84/0x90)
 r7:ede00010 r6:ede00010 r5:ef3fd810 r4:d7eb3dc8
[] (media_device_setup_link) from [] 
(media_device_ioctl+0xa4/0x148)
 r6: r5:d7eb3dc8 r4:c077b014 r3:c04f9b2c
[] (media_device_ioctl) from [] (media_ioctl+0x38/0x4c)
 r10:ed5eca68 r9:d7eb2000 r8:befe92b0 r7:0003 r6:0003 r5:e82ca280
 r4:c0190304
[] (media_ioctl) from [] (do_vfs_ioctl+0x98/0x9a0)
[] (do_vfs_ioctl) from [] (SyS_ioctl+0x3c/0x60)
 r10: r9:d7eb2000 r8:befe92b0 r7:0003 r6:c0347c03 r5:e82ca280
 r4:e82ca280
[] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x1c)
 r8:c000ff04 r7:0036 r6:000261d0 r5:0001 r4:009162e8 r3:0001
---[ end trace 4fdd40e5adfc4485 ]---
[ cut here ]
WARNING: CPU: 1 PID: 1889 at drivers/staging/media/imx/imx-media-csi.c:806 
csi_s_power+0x9c/0xa8 [imx_media_csi]
Modules linked in: caam_jr uvcvideo snd_soc_imx_sgtl5000 snd_soc_fsl_asoc_card 
snd_soc_imx_spdif imx_media_csi(C) imx6_mipi_csi2(C) snd_soc_imx_audmux 
snd_soc_sgtl5000 imx219 imx_media_ic(C) imx_media_capture(C) imx_media_vdic(C) 
caam video_multiplexer imx_sdma coda v4l2_mem2mem videobuf2_v4l2 imx2_wdt 
imx_vdoa videobuf2_dma_contig videobuf2_core videobuf2_vmalloc videobuf2_memops 
snd_soc_fsl_ssi
imx_thermal snd_soc_fsl_spdif imx_pcm_dma imx_media(C) imx_media_common(C) nfsd
rc_pinnacle_pctv_hd dw_hdmi_ahb_audio dw_hdmi_cec etnaviv
CPU: 1 PID: 1889 Comm: media-ctl Tainted: GWC  4.11.0-rc1+ #2125
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
Backtrace:
[] (dump_backtrace) from [] (show_stack+0x18/0x1c)
 r6:600e0013 r5: r4: r3:
[] (show_stack) from [] (dump_stack+0xa4/0xdc)
[] (dump_stack) from [] (__warn+0xdc/0x108)
 r6:bf124014 r5: r4: r3:c09ea4a8
[] (__warn) from [] (warn_slowpath_null+0x28/0x30)
 r10:ede00010 r8:ede4a348 r7:ee000800 r6:d0395140 r5: r4:d0395010
[] (warn_slowpath_null) from [] (csi_s_power+0x9c/0xa8 
[imx_media_csi])
[] (csi_s_power [imx_media_csi]) from [] 
(imx_media_set_power+0x3c/0x108 [imx_media_common])
 r7:ee000800 r6: r5: r4:000c
[] (imx_media_set_power [imx_media_common]) from [] 
(imx_media_pipeline_set_power+0x38/0x40 [imx_media_common])
 r10:0001 r9:0001 r8:ede4a348 r7:ede00010 r6:ede4a348 r5:ee000800
 r4:0001
[] (imx_media_pipeline_set_power [imx_media_common]) from 
[] (imx_media_link_notify+0xf0/0x144 [imx_media])
 r7:ede00010 r6:d0320480 r5:ee000800 r4:ee000800
[] (imx_media_link_notify [imx_media]) from [] 
(__media_entity_setup_link+0x110/0x1d8)
 r10:c0347c03 r9:d7eb3dc8 r8:befe92b0 

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

On Fri, Mar 10, 2017 at 03:20:34PM -0800, Steve Longerbeam wrote:
> 
> 
> On 03/10/2017 12:13 PM, Russell King - ARM Linux wrote:
> >Version 5 gives me no v4l2 controls exposed through the video device
> >interface.
> >
> >Just like with version 4, version 5 is completely useless with IMX219:
> >
> >imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
> >ipu1_csi0: pipeline start failed with -110
> >imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
> >ipu1_csi0: pipeline start failed with -110
> >imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
> >ipu1_csi0: pipeline start failed with -110
> >
> >So, like v4, I can't do any further testing.
> >
> 
> Is the imx219 placing the csi-2 bus in LP-11 state on exit
> from s_power(ON)?
> 
> I realize that probably means bringing the chip up to a
> completely operational state and then setting it to stream
> OFF in the s_power() op.
> 
> The same had to be done for the OV5640.

What do you suggest - setting it to the highest CSI2 bus speed that
it supports?  That's likely to be over the maximum data rate specified
for iMX6Q if it's wired up using four lanes.


Also, as I've already said, I think that powering on the sensor just
because it's got an enabled media-controller link is a silly idea.

Right now, the only way of using the imx6 capture stuff is to manually
configure it with media-ctl, which means that happens either at boot
due to a custom boot script, or when you first use it (by manually
running a script.)

This results in the sensor staying powered from that point onwards,
wasting power unnecessarily.

-- 
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 v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/10/2017 12:13 PM, Russell King - ARM Linux wrote:

Version 5 gives me no v4l2 controls exposed through the video device
interface.

Just like with version 4, version 5 is completely useless with IMX219:

imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
ipu1_csi0: pipeline start failed with -110
imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
ipu1_csi0: pipeline start failed with -110
imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
ipu1_csi0: pipeline start failed with -110



If it's too difficult to get the imx219 csi-2 transmitter into the
LP-11 state on power on, perhaps the csi-2 receiver can be a little
more lenient on the transmitter and make the LP-11 timeout a warning
instead of error-out.

Can you try the attached change on top of the version 5 patchset?

If that doesn't work then you're just going to have to fix the bug
in imx219.

Steve
diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
index d8f931e..720bf4d 100644
--- a/drivers/staging/media/imx/imx6-mipi-csi2.c
+++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
@@ -224,11 +224,8 @@ static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
 
 	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
  (reg & mask) == mask, 0, 50);
-	if (ret) {
-		v4l2_err(>sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
-		return ret;
-	}
-
+	if (ret)
+		v4l2_warn(>sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
 	return 0;
 }
 

Re: [PATCH v5 00/39] i.MX Media Driver

[Steve Longerbeam]

On 03/10/2017 12:13 PM, Russell King - ARM Linux wrote:

Version 5 gives me no v4l2 controls exposed through the video device
interface.

Just like with version 4, version 5 is completely useless with IMX219:

imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
ipu1_csi0: pipeline start failed with -110
imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
ipu1_csi0: pipeline start failed with -110
imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
ipu1_csi0: pipeline start failed with -110

So, like v4, I can't do any further testing.



Is the imx219 placing the csi-2 bus in LP-11 state on exit
from s_power(ON)?

I realize that probably means bringing the chip up to a
completely operational state and then setting it to stream
OFF in the s_power() op.

The same had to be done for the OV5640.

Steve

Re: [PATCH v5 00/39] i.MX Media Driver

[Russell King - ARM Linux]

Version 5 gives me no v4l2 controls exposed through the video device
interface.

Just like with version 4, version 5 is completely useless with IMX219:

imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
ipu1_csi0: pipeline start failed with -110
imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
ipu1_csi0: pipeline start failed with -110
imx6-mipi-csi2: LP-11 timeout, phy_state = 0x0200
ipu1_csi0: pipeline start failed with -110

So, like v4, I can't do any further testing.

On Thu, Mar 09, 2017 at 08:52:40PM -0800, Steve Longerbeam wrote:
> In version 5:
> 
> - ov5640: renamed "pwdn-gpios" to "powerdown-gpios"
> 
> - ov5640: add mutex lock around the subdev op entry points.
> 
> - ov5640: don't attempt to program the new mode in ov5640_set_fmt().
>   Instead set a new flag, pending_mode_change, and program the new
>   mode at s_stream() if flag is set.
> 
> - ov5640: implement [gs]_frame_interval. As part of that, create
>   ov5640_try_frame_interval(), which is used by both [gs]_frame_interval
>   and [gs]_parm.
> 
> - ov5640: don't attempt to set controls in ov5640_s_ctrl(), or at
>   mode change, do it instead after first power-up.
> 
> - video-multiplexer: include link_validate in media_entity_operations.
> 
> - video-multiplexer: enforce that output pad frame interval must match
>   input pad frame interval in vidsw_s_frame_interval().
> 
> - video-multiplexer: initialize frame interval to a default 30 fps.
> 
> - mipi csi-2: renamed "cfg" clock name property to "ref". This is the
>   27 MHz mipi csi-2 PLL reference clock.
> 
> - mipi csi-2: create a hsfreq_map[] table based on
>   https://community.nxp.com/docs/DOC-94312. Use it to select
>   a hsfreqrange_sel value when programming the D-PHY, based on
>   a max Mbps per lane. This is computed from the source subdev
>   via V4L2_CID_LINK_FREQ control, and if the subdev doesn't implement
>   that control, use a default hard-coded max Mbps per lane.
> 
> - added required ports property description to imx-media binding doc.
> 
> - removed event V4L2_EVENT_FRAME_TIMEOUT. On a frame timeout, which
>   is always unrecoverable, call vb2_queue_error() instead.
> 
> - export the remaining custom events to V4L2_EVENT_FRAME_INTERVAL_ERROR
>   and V4L2_EVENT_NEW_FRAME_BEFORE_EOF.
> 
> - vdic: use V4L2_CID_DEINTERLACING_MODE for motion compensation control
>   instead of a custom control.
> 
> - add v4l2_subdev_link_validate_frame_interval(). Call this in the
>   link_validate imx-media subdev callbacks and video-multiplexer.
> 
> - fix subdev event registration: implementation of subscribe_event()
>   and unsubscribe_event() subdev ops were missing.
> 
> - all calls from the pipeline to the sensor subdev have been removed.
>   Only the CSI subdev still refers to a sensor, and only to retrieve
>   its media bus config, which is necessary to setup the CSI interface.
> 
> - add mutex locks around the imx-media subdev op entry points.
> 
> - completed the propagation of all pad format parameters from sink
>   pads to source pads within every imx-media subdev.
> 
> - implement [gs]_frame_interval in all the imx-media subdevs.
> 
> - imx-ic-prpencvf: there isn't necessarily a CSI subdev in the pipeline
>   in the future, so make sure this is optional when calling the CSI's
>   FIM.
> 
> - the source pads that attach to capture device nodes now require the
>   IPU internal pixel codes. The capture device translates these to
>   v4l2 fourcc memory formats.
> 
> - fix control inheritance to the capture device. When the pipeline
>   was modified, the inherited controls were not being refreshed.
>   v4l2_pipeline_inherit_controls() is now called only in imx-media
>   link_notify() callback when a pipelink link is disabled or modified.
>   imx_media_find_pipeline_video_device() is created to locate the
>   capture device in the pipeline.
> 
> - fix a possible race when propagating formats to the capture device.
>   The subdevs and capture device use different mutex locks when setting
>   formats. imx_media_capture_device_set_format() is created which acquires
>   the capture device mutex when updating the capture device format.
> 
> - verify all subdevs were bound in the async completion callback.
>  
> 
> Philipp Zabel (7):
>   [media] dt-bindings: Add bindings for video-multiplexer device
>   ARM: dts: imx6qdl: Add mipi_ipu1/2 multiplexers, mipi_csi, and their
> connections
>   add mux and video interface bridge entity functions
>   platform: add video-multiplexer subdevice driver
>   media: imx: csi: fix crop rectangle changes in set_fmt
>   media: imx: csi: add frame skipping support
>   media: imx: csi: fix crop rectangle reset in sink set_fmt
> 
> Russell King (4):
>   media: imx: add support for bayer formats
>   media: imx: csi: add support for bayer formats
>   media: imx: mipi-csi2: enable setting and getting of frame rates
>   media: imx: csi/fim: add support for frame intervals
> 
> Steve Longerbeam (28):
>   [media] dt-bindings: Add bindings