Re: [PATCH v5 00/39] i.MX Media Driver
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
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, &fmt) != 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
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
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 do
Re: [PATCH v5 00/39] i.MX Media Driver
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, &format, pad, which); > > if (ret != 0) > > return; > > > > - ret = v4l2_subdev_get_frame_interval(entity, &interval, pad); > > - if (ret != 0 && ret != -ENOTTY) > > - return; > > + err_fi = v4l2_subdev_get_frame_interval(entity, &interval, 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
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, &format, pad, which); > if (ret != 0) > return; > > - ret = v4l2_subdev_get_frame_interval(entity, &interval, pad); > - if (ret != 0 && ret != -ENOTTY) > - return; > + err_fi = v4l2_subdev_get_frame_interval(entity, &interval, 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
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.) T
Re: [PATCH v5 00/39] i.MX Media Driver
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, &format, pad, which); if (ret != 0) return; - ret = v4l2_subdev_get_frame_interval(entity, &interval, pad); - if (ret != 0 && ret != -ENOTTY) - return; + err_fi = v4l2_subdev_get_frame_interval(entity, &interval, 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
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
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
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
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
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
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: 4,
Re: [PATCH v5 00/39] i.MX Media Driver
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 paramete
Re: [PATCH v5 00/39] i.MX Media Driver
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
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 to
Re: [PATCH v5 00/39] i.MX Media Driver
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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(&gsc->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(&gsc->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
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, &image); - 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_
Re: [PATCH v5 00/39] i.MX Media Driver
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
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, &image); - 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-
Re: [PATCH v5 00/39] i.MX Media Driver
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
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, &image); - - 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, &image); + burst_size = (outfmt->width & 0xf) ? 8 : 16; /*
Re: [PATCH v5 00/39] i.MX Media Driver
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(&gsc->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(&gsc->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
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
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
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
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
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
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
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, &image); - - 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, &image); + 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
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
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
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
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
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
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
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
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 [] (imx_media_p
Re: [PATCH v5 00/39] i.MX Media Driver
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 r7:ede00
Re: [PATCH v5 00/39] i.MX Media Driver
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
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(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg); - return ret; - } - + if (ret) + v4l2_warn(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg); return 0; }
Re: [PATCH v5 00/39] i.MX Media Driver
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
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 for
[PATCH v5 00/39] i.MX Media Driver
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 for i.MX media driver [media] dt/bindings: Add bindings for OV5640 ARM: dts: imx6qdl: Add compatible, clocks, irqs to MIPI CSI-2 node ARM: dts: imx6qdl: add capture-subsystem device ARM: dts: imx6qdl-sabrelite: remove erratum ERR006687 workaround ARM: dts: imx6-sabrelite: add OV5642 and OV5640 camera sensors ARM: dts: imx6-sabresd: add OV5642 and OV5640 camera sensors ARM: dts: imx6-sabreauto: create i2cmux for i2c3 ARM: dts: imx6-sabreauto: add reset-gpios property for max7310_b ARM: dts: imx6-sabreauto: add pinctrl for gpt input capture ARM: dts: imx6-sabreauto: add the ADV7180 video decoder [media] v4l2: add a frame interval error event [media] v4l2: add a new-frame before end-of-frame event [media] v4l2-mc: add a function to i