Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Sakari Ailus]

Hi Steve,

On Tue, Mar 14, 2017 at 09:43:09AM -0700, Steve Longerbeam wrote:
> 
> 
> On 03/14/2017 09:21 AM, Nicolas Dufresne wrote:
> >Le lundi 13 mars 2017 à 10:45 +, Russell King - ARM Linux a écrit :
> >>On Mon, Mar 13, 2017 at 11:02:34AM +0100, Hans Verkuil wrote:
> >>>On 03/11/2017 07:14 PM, Steve Longerbeam wrote:
> The event must be user visible, otherwise the user has no indication
> the error, and can't correct it by stream restart.
> >>>In that case the driver can detect this and call vb2_queue_error. It's
> >>>what it is there for.
> >>>
> >>>The event doesn't help you since only this driver has this issue. So nobody
> >>>will watch this event, unless it is sw specifically written for this SoC.
> >>>
> >>>Much better to call vb2_queue_error to signal a fatal error (which this
> >>>apparently is) since there are more drivers that do this, and vivid 
> >>>supports
> >>>triggering this condition as well.
> >>So today, I can fiddle around with the IMX219 registers to help gain
> >>an understanding of how this sensor works.  Several of the registers
> >>(such as the PLL setup [*]) require me to disable streaming on the
> >>sensor while changing them.
> >>
> >>This is something I've done many times while testing various ideas,
> >>and is my primary way of figuring out and testing such things.
> >>
> >>Whenever I resume streaming (provided I've let the sensor stop
> >>streaming at a frame boundary) it resumes as if nothing happened.  If I
> >>stop the sensor mid-frame, then I get the rolling issue that Steve
> >>reports, but once the top of the frame becomes aligned with the top of
> >>the capture, everything then becomes stable again as if nothing happened.
> >>
> >>The side effect of what you're proposing is that when I disable streaming
> >>at the sensor by poking at its registers, rather than the capture just
> >>stopping, an error is going to be delivered to gstreamer, and gstreamer
> >>is going to exit, taking the entire capture process down.
> >Indeed, there is no recovery attempt in GStreamer code, and it's hard
> >for an higher level programs to handle this. Nothing prevents from
> >adding something of course, but the errors are really un-specific, so
> >it would be something pretty blind. For what it has been tested, this
> >case was never met, usually the error is triggered by a USB camera
> >being un-plugged, a driver failure or even a firmware crash. Most of
> >the time, this is not recoverable.
> >
> >My main concern here based on what I'm reading, is that this driver is
> >not even able to notice immediately that a produced frame was corrupted
> >(because it's out of sync). From usability perspective, this is really
> >bad.
> 
> First, this is an isolated problem, specific to bt.656 and it only
> occurs when disrupting the analog video source signal in some
> way (by unplugging the RCA cable from the ADV718x connector
> for example).
> 
> Second, there is no DMA status support in i.MX6 to catch these
> shifted bt.656 codes, and the ADV718x does not provide any
> status indicators of this event either. So monitoring frame intervals
> is the only solution available, until FSL/NXP issues a new silicon rev.
> 
> 
> >  Can't the driver derive a clock from some irq and calculate for
> >each frame if the timing was correct ?
> 
> That's what is being done, essentially.
> 
> >  And if not mark the buffer with
> >V4L2_BUF_FLAG_ERROR ?
> 
> I prefer to keep the private event, V4L2_BUF_FLAG_ERROR is too
> unspecific.

Is the reason you prefer an event that you have multiple drivers involved,
or that the error flag is, well, only telling there was an error with a
particular frame?

Returning -EIO (by calling vb2_queue_error()) would be a better choice as it
is documented behaviour.

-- 
Regard,s

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Pavel Machek]

On Mon 2017-03-13 10:45:38, Russell King - ARM Linux wrote:
> On Mon, Mar 13, 2017 at 11:02:34AM +0100, Hans Verkuil wrote:
> > On 03/11/2017 07:14 PM, Steve Longerbeam wrote:
> > > The event must be user visible, otherwise the user has no indication
> > > the error, and can't correct it by stream restart.
> > 
> > In that case the driver can detect this and call vb2_queue_error. It's
> > what it is there for.
> > 
> > The event doesn't help you since only this driver has this issue. So nobody
> > will watch this event, unless it is sw specifically written for this SoC.
> > 
> > Much better to call vb2_queue_error to signal a fatal error (which this
> > apparently is) since there are more drivers that do this, and vivid supports
> > triggering this condition as well.
> 
> So today, I can fiddle around with the IMX219 registers to help gain
> an understanding of how this sensor works.  Several of the registers
> (such as the PLL setup [*]) require me to disable streaming on the
> sensor while changing them.
> 
> This is something I've done many times while testing various ideas,
> and is my primary way of figuring out and testing such things.
> 
> Whenever I resume streaming (provided I've let the sensor stop
> streaming at a frame boundary) it resumes as if nothing happened.  If I
> stop the sensor mid-frame, then I get the rolling issue that Steve
> reports, but once the top of the frame becomes aligned with the top of
> the capture, everything then becomes stable again as if nothing happened.
> 
> The side effect of what you're proposing is that when I disable streaming
> at the sensor by poking at its registers, rather than the capture just
> stopping, an error is going to be delivered to gstreamer, and gstreamer
> is going to exit, taking the entire capture process down.
> 
> This severely restricts the ability to be able to develop and test
> sensor drivers.

Well, but kernel should do what is best for production, not what is
best for driver debugging.

And yes, I guess you can have #ifdef or module parameter or something
switching for behaviour you prefer when you are debugging. But for
production, vb2_queue_error() seems to be the right solution.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Steve Longerbeam]

On 03/14/2017 09:47 AM, Russell King - ARM Linux wrote:

On Tue, Mar 14, 2017 at 12:21:31PM -0400, Nicolas Dufresne wrote:

My main concern here based on what I'm reading, is that this driver is
not even able to notice immediately that a produced frame was corrupted
(because it's out of sync). From usability perspective, this is really
bad. Can't the driver derive a clock from some irq and calculate for
each frame if the timing was correct ? And if not mark the buffer with
V4L2_BUF_FLAG_ERROR ?

One of the issues of measuring timing with IRQs is the fact that the
IRQ subsystem only allows one IRQ to run at a time.  If an IRQ takes
a relatively long time to process, then it throws the timing of other
IRQs out.

If you're going to decide that a buffer should be marked in error on
the basis of an interrupt arriving late, this can trigger spuriously.

It wasn't that long ago that USB HID was regularly eating something
like 20ms of interrupt time... that's been solved, but that doesn't
mean all cases are solved - there are still interrupt handlers in the
kernel that are on the order of milliseconds to complete.

Given the quality I observe of some USB serial devices (eg, running at
115200 baud, but feeling like they deliver characters to userspace at
9600 baud) I wouldn't be surprised if some USB serial drivers eat a lot
of IRQ time... and if so, all it'll take is to plug such a device in
to disrupt capture.

That sounds way too fragile to me.


exactly, hence the imx6 timer input capture support.

Steve

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Russell King - ARM Linux]

On Tue, Mar 14, 2017 at 12:21:31PM -0400, Nicolas Dufresne wrote:
> My main concern here based on what I'm reading, is that this driver is
> not even able to notice immediately that a produced frame was corrupted
> (because it's out of sync). From usability perspective, this is really
> bad. Can't the driver derive a clock from some irq and calculate for
> each frame if the timing was correct ? And if not mark the buffer with
> V4L2_BUF_FLAG_ERROR ?

One of the issues of measuring timing with IRQs is the fact that the
IRQ subsystem only allows one IRQ to run at a time.  If an IRQ takes
a relatively long time to process, then it throws the timing of other
IRQs out.

If you're going to decide that a buffer should be marked in error on
the basis of an interrupt arriving late, this can trigger spuriously.

It wasn't that long ago that USB HID was regularly eating something
like 20ms of interrupt time... that's been solved, but that doesn't
mean all cases are solved - there are still interrupt handlers in the
kernel that are on the order of milliseconds to complete.

Given the quality I observe of some USB serial devices (eg, running at
115200 baud, but feeling like they deliver characters to userspace at
9600 baud) I wouldn't be surprised if some USB serial drivers eat a lot
of IRQ time... and if so, all it'll take is to plug such a device in
to disrupt capture.

That sounds way too fragile to me.

-- 
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 15/39] [media] v4l2: add a frame interval error event

[Steve Longerbeam]

On 03/14/2017 09:21 AM, Nicolas Dufresne wrote:

Le lundi 13 mars 2017 à 10:45 +, Russell King - ARM Linux a écrit :

On Mon, Mar 13, 2017 at 11:02:34AM +0100, Hans Verkuil wrote:

On 03/11/2017 07:14 PM, Steve Longerbeam wrote:

The event must be user visible, otherwise the user has no indication
the error, and can't correct it by stream restart.

In that case the driver can detect this and call vb2_queue_error. It's
what it is there for.

The event doesn't help you since only this driver has this issue. So nobody
will watch this event, unless it is sw specifically written for this SoC.

Much better to call vb2_queue_error to signal a fatal error (which this
apparently is) since there are more drivers that do this, and vivid supports
triggering this condition as well.

So today, I can fiddle around with the IMX219 registers to help gain
an understanding of how this sensor works.  Several of the registers
(such as the PLL setup [*]) require me to disable streaming on the
sensor while changing them.

This is something I've done many times while testing various ideas,
and is my primary way of figuring out and testing such things.

Whenever I resume streaming (provided I've let the sensor stop
streaming at a frame boundary) it resumes as if nothing happened.  If I
stop the sensor mid-frame, then I get the rolling issue that Steve
reports, but once the top of the frame becomes aligned with the top of
the capture, everything then becomes stable again as if nothing happened.

The side effect of what you're proposing is that when I disable streaming
at the sensor by poking at its registers, rather than the capture just
stopping, an error is going to be delivered to gstreamer, and gstreamer
is going to exit, taking the entire capture process down.

Indeed, there is no recovery attempt in GStreamer code, and it's hard
for an higher level programs to handle this. Nothing prevents from
adding something of course, but the errors are really un-specific, so
it would be something pretty blind. For what it has been tested, this
case was never met, usually the error is triggered by a USB camera
being un-plugged, a driver failure or even a firmware crash. Most of
the time, this is not recoverable.

My main concern here based on what I'm reading, is that this driver is
not even able to notice immediately that a produced frame was corrupted
(because it's out of sync). From usability perspective, this is really
bad.


First, this is an isolated problem, specific to bt.656 and it only
occurs when disrupting the analog video source signal in some
way (by unplugging the RCA cable from the ADV718x connector
for example).

Second, there is no DMA status support in i.MX6 to catch these
shifted bt.656 codes, and the ADV718x does not provide any
status indicators of this event either. So monitoring frame intervals
is the only solution available, until FSL/NXP issues a new silicon rev.



  Can't the driver derive a clock from some irq and calculate for
each frame if the timing was correct ?


That's what is being done, essentially.


  And if not mark the buffer with
V4L2_BUF_FLAG_ERROR ?


I prefer to keep the private event, V4L2_BUF_FLAG_ERROR is too
unspecific.

Steve





This severely restricts the ability to be able to develop and test
sensor drivers.

So, I strongly disagree with you.

Loss of capture frames is not necessarily a fatal error - as I have been
saying repeatedly.  In Steve's case, there's some unknown interaction
between the source and iMX6 hardware that is causing the instability,
but that is simply not true of other sources, and I oppose any idea that
we should cripple the iMX6 side of the capture based upon just one
hardware combination where this is a problem.

Indeed, it happens all the time with slow USB port and UVC devices.
Though, the driver is well aware, and mark the buffers with
V4L2_BUF_FLAG_ERROR.


Steve suggested that the problem could be in the iMX6 CSI block - and I
note comparing Steve's code with the code in FSL's repository that there
are some changes that are missing in Steve's code to do with the CCIR656
sync code setup, particularly for >8 bit.  The progressive CCIR656 8-bit
setup looks pretty similar though - but I think what needs to be asked
is whether the same problem is visible using the FSL/NXP vendor kernel.


* - the PLL setup is something that requires research at the moment.
Sony's official position (even to their customers) is that they do not
supply the necessary information, instead they expect customers to tell
them the capture settings they want, and Sony will throw the values into
a spreadsheet, and they'll supply the register settings back to the
customer.  Hence, the only way to proceed with a generic driver for
this sensor is to experiment, and experimenting requires the ability to
pause the stream at the sensor while making changes.  Take this away,
and we're stuck with the tables-of-register-settings-for-set-of-fixed-
capture-settings approach.  I've made a lot 

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Nicolas Dufresne]

Le lundi 13 mars 2017 à 10:45 +, Russell King - ARM Linux a écrit :
> On Mon, Mar 13, 2017 at 11:02:34AM +0100, Hans Verkuil wrote:
> > On 03/11/2017 07:14 PM, Steve Longerbeam wrote:
> > > The event must be user visible, otherwise the user has no indication
> > > the error, and can't correct it by stream restart.
> > 
> > In that case the driver can detect this and call vb2_queue_error. It's
> > what it is there for.
> > 
> > The event doesn't help you since only this driver has this issue. So nobody
> > will watch this event, unless it is sw specifically written for this SoC.
> > 
> > Much better to call vb2_queue_error to signal a fatal error (which this
> > apparently is) since there are more drivers that do this, and vivid supports
> > triggering this condition as well.
> 
> So today, I can fiddle around with the IMX219 registers to help gain
> an understanding of how this sensor works.  Several of the registers
> (such as the PLL setup [*]) require me to disable streaming on the
> sensor while changing them.
> 
> This is something I've done many times while testing various ideas,
> and is my primary way of figuring out and testing such things.
> 
> Whenever I resume streaming (provided I've let the sensor stop
> streaming at a frame boundary) it resumes as if nothing happened.  If I
> stop the sensor mid-frame, then I get the rolling issue that Steve
> reports, but once the top of the frame becomes aligned with the top of
> the capture, everything then becomes stable again as if nothing happened.
> 
> The side effect of what you're proposing is that when I disable streaming
> at the sensor by poking at its registers, rather than the capture just
> stopping, an error is going to be delivered to gstreamer, and gstreamer
> is going to exit, taking the entire capture process down.

Indeed, there is no recovery attempt in GStreamer code, and it's hard
for an higher level programs to handle this. Nothing prevents from
adding something of course, but the errors are really un-specific, so
it would be something pretty blind. For what it has been tested, this
case was never met, usually the error is triggered by a USB camera
being un-plugged, a driver failure or even a firmware crash. Most of
the time, this is not recoverable.

My main concern here based on what I'm reading, is that this driver is
not even able to notice immediately that a produced frame was corrupted
(because it's out of sync). From usability perspective, this is really
bad. Can't the driver derive a clock from some irq and calculate for
each frame if the timing was correct ? And if not mark the buffer with
V4L2_BUF_FLAG_ERROR ?

> 
> This severely restricts the ability to be able to develop and test
> sensor drivers.
> 
> So, I strongly disagree with you.
> 
> Loss of capture frames is not necessarily a fatal error - as I have been
> saying repeatedly.  In Steve's case, there's some unknown interaction
> between the source and iMX6 hardware that is causing the instability,
> but that is simply not true of other sources, and I oppose any idea that
> we should cripple the iMX6 side of the capture based upon just one
> hardware combination where this is a problem.

Indeed, it happens all the time with slow USB port and UVC devices.
Though, the driver is well aware, and mark the buffers with
V4L2_BUF_FLAG_ERROR.

> 
> Steve suggested that the problem could be in the iMX6 CSI block - and I
> note comparing Steve's code with the code in FSL's repository that there
> are some changes that are missing in Steve's code to do with the CCIR656
> sync code setup, particularly for >8 bit.  The progressive CCIR656 8-bit
> setup looks pretty similar though - but I think what needs to be asked
> is whether the same problem is visible using the FSL/NXP vendor kernel.
> 
> 
> * - the PLL setup is something that requires research at the moment.
> Sony's official position (even to their customers) is that they do not
> supply the necessary information, instead they expect customers to tell
> them the capture settings they want, and Sony will throw the values into
> a spreadsheet, and they'll supply the register settings back to the
> customer.  Hence, the only way to proceed with a generic driver for
> this sensor is to experiment, and experimenting requires the ability to
> pause the stream at the sensor while making changes.  Take this away,
> and we're stuck with the tables-of-register-settings-for-set-of-fixed-
> capture-settings approach.  I've made a lot of progress away from this
> which is all down to the flexibility afforded by _not_ killing the
> capture process.
> 

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

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Steve Longerbeam]

On 03/13/2017 10:10 AM, Hans Verkuil wrote:

On 03/13/2017 06:06 PM, Steve Longerbeam wrote:



On 03/13/2017 03:53 AM, Hans Verkuil wrote:

On 03/13/2017 11:45 AM, Russell King - ARM Linux wrote:

On Mon, Mar 13, 2017 at 11:02:34AM +0100, Hans Verkuil wrote:

On 03/11/2017 07:14 PM, Steve Longerbeam wrote:

The event must be user visible, otherwise the user has no indication
the error, and can't correct it by stream restart.


In that case the driver can detect this and call vb2_queue_error. It's
what it is there for.

The event doesn't help you since only this driver has this issue. So nobody
will watch this event, unless it is sw specifically written for this SoC.

Much better to call vb2_queue_error to signal a fatal error (which this
apparently is) since there are more drivers that do this, and vivid supports
triggering this condition as well.


So today, I can fiddle around with the IMX219 registers to help gain
an understanding of how this sensor works.  Several of the registers
(such as the PLL setup [*]) require me to disable streaming on the
sensor while changing them.

This is something I've done many times while testing various ideas,
and is my primary way of figuring out and testing such things.

Whenever I resume streaming (provided I've let the sensor stop
streaming at a frame boundary) it resumes as if nothing happened.  If I
stop the sensor mid-frame, then I get the rolling issue that Steve
reports, but once the top of the frame becomes aligned with the top of
the capture, everything then becomes stable again as if nothing happened.

The side effect of what you're proposing is that when I disable streaming
at the sensor by poking at its registers, rather than the capture just
stopping, an error is going to be delivered to gstreamer, and gstreamer
is going to exit, taking the entire capture process down.

This severely restricts the ability to be able to develop and test
sensor drivers.

So, I strongly disagree with you.

Loss of capture frames is not necessarily a fatal error - as I have been
saying repeatedly.  In Steve's case, there's some unknown interaction
between the source and iMX6 hardware that is causing the instability,
but that is simply not true of other sources, and I oppose any idea that
we should cripple the iMX6 side of the capture based upon just one
hardware combination where this is a problem.

Steve suggested that the problem could be in the iMX6 CSI block - and I
note comparing Steve's code with the code in FSL's repository that there
are some changes that are missing in Steve's code to do with the CCIR656
sync code setup, particularly for >8 bit.  The progressive CCIR656 8-bit
setup looks pretty similar though - but I think what needs to be asked
is whether the same problem is visible using the FSL/NXP vendor kernel.


* - the PLL setup is something that requires research at the moment.
Sony's official position (even to their customers) is that they do not
supply the necessary information, instead they expect customers to tell
them the capture settings they want, and Sony will throw the values into
a spreadsheet, and they'll supply the register settings back to the
customer.  Hence, the only way to proceed with a generic driver for
this sensor is to experiment, and experimenting requires the ability to
pause the stream at the sensor while making changes.  Take this away,
and we're stuck with the tables-of-register-settings-for-set-of-fixed-
capture-settings approach.  I've made a lot of progress away from this
which is all down to the flexibility afforded by _not_ killing the
capture process.



In other words: Steve should either find a proper fix for this, or only
call vb2_queue_error in this specific case. Sending an event that nobody
will know how to handle or what to do with is pretty pointless IMHO.

Let's just give him time to try and figure out the real issue here.



This is a long-standing issue, I've traveled to Hildesheim working with
our customer to try and get to the bottom of it. I can go into a lot of
details from those trips, we probed the bt.656 bus with a logic analyzer
and I can share those results with anyone who asks. But the results of
those investigations indicate the CSI is not handling the SAV/EAV sync
codes correctly - if there is a shift in the line position at which
those codes occur, the CSI/IPU does not abort the frame capture DMA
and start from the new sync code position, it just continues to capture
lines until the programmed number of lines are transferred, hence you
get these split images. Freescale also informed us of a mechanism in the
IPU that will add lines if it detects these short frames, until the
programmed number of lines are reached. Apparently that is what creates
the rolling effect, but this rolling can last for up to a full minute,
which is completely unacceptable, it must be corrected as soon as
possible.

So the only thing we could come up with was to monitor frame intervals,
this is purely empirical, but we 

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Hans Verkuil]

On 03/13/2017 06:06 PM, Steve Longerbeam wrote:
> 
> 
> On 03/13/2017 03:53 AM, Hans Verkuil wrote:
>> On 03/13/2017 11:45 AM, Russell King - ARM Linux wrote:
>>> On Mon, Mar 13, 2017 at 11:02:34AM +0100, Hans Verkuil wrote:
 On 03/11/2017 07:14 PM, Steve Longerbeam wrote:
> The event must be user visible, otherwise the user has no indication
> the error, and can't correct it by stream restart.

 In that case the driver can detect this and call vb2_queue_error. It's
 what it is there for.

 The event doesn't help you since only this driver has this issue. So nobody
 will watch this event, unless it is sw specifically written for this SoC.

 Much better to call vb2_queue_error to signal a fatal error (which this
 apparently is) since there are more drivers that do this, and vivid 
 supports
 triggering this condition as well.
>>>
>>> So today, I can fiddle around with the IMX219 registers to help gain
>>> an understanding of how this sensor works.  Several of the registers
>>> (such as the PLL setup [*]) require me to disable streaming on the
>>> sensor while changing them.
>>>
>>> This is something I've done many times while testing various ideas,
>>> and is my primary way of figuring out and testing such things.
>>>
>>> Whenever I resume streaming (provided I've let the sensor stop
>>> streaming at a frame boundary) it resumes as if nothing happened.  If I
>>> stop the sensor mid-frame, then I get the rolling issue that Steve
>>> reports, but once the top of the frame becomes aligned with the top of
>>> the capture, everything then becomes stable again as if nothing happened.
>>>
>>> The side effect of what you're proposing is that when I disable streaming
>>> at the sensor by poking at its registers, rather than the capture just
>>> stopping, an error is going to be delivered to gstreamer, and gstreamer
>>> is going to exit, taking the entire capture process down.
>>>
>>> This severely restricts the ability to be able to develop and test
>>> sensor drivers.
>>>
>>> So, I strongly disagree with you.
>>>
>>> Loss of capture frames is not necessarily a fatal error - as I have been
>>> saying repeatedly.  In Steve's case, there's some unknown interaction
>>> between the source and iMX6 hardware that is causing the instability,
>>> but that is simply not true of other sources, and I oppose any idea that
>>> we should cripple the iMX6 side of the capture based upon just one
>>> hardware combination where this is a problem.
>>>
>>> Steve suggested that the problem could be in the iMX6 CSI block - and I
>>> note comparing Steve's code with the code in FSL's repository that there
>>> are some changes that are missing in Steve's code to do with the CCIR656
>>> sync code setup, particularly for >8 bit.  The progressive CCIR656 8-bit
>>> setup looks pretty similar though - but I think what needs to be asked
>>> is whether the same problem is visible using the FSL/NXP vendor kernel.
>>>
>>>
>>> * - the PLL setup is something that requires research at the moment.
>>> Sony's official position (even to their customers) is that they do not
>>> supply the necessary information, instead they expect customers to tell
>>> them the capture settings they want, and Sony will throw the values into
>>> a spreadsheet, and they'll supply the register settings back to the
>>> customer.  Hence, the only way to proceed with a generic driver for
>>> this sensor is to experiment, and experimenting requires the ability to
>>> pause the stream at the sensor while making changes.  Take this away,
>>> and we're stuck with the tables-of-register-settings-for-set-of-fixed-
>>> capture-settings approach.  I've made a lot of progress away from this
>>> which is all down to the flexibility afforded by _not_ killing the
>>> capture process.
>>>
>>
>> In other words: Steve should either find a proper fix for this, or only
>> call vb2_queue_error in this specific case. Sending an event that nobody
>> will know how to handle or what to do with is pretty pointless IMHO.
>>
>> Let's just give him time to try and figure out the real issue here.
> 
> 
> This is a long-standing issue, I've traveled to Hildesheim working with
> our customer to try and get to the bottom of it. I can go into a lot of
> details from those trips, we probed the bt.656 bus with a logic analyzer
> and I can share those results with anyone who asks. But the results of
> those investigations indicate the CSI is not handling the SAV/EAV sync
> codes correctly - if there is a shift in the line position at which
> those codes occur, the CSI/IPU does not abort the frame capture DMA
> and start from the new sync code position, it just continues to capture
> lines until the programmed number of lines are transferred, hence you
> get these split images. Freescale also informed us of a mechanism in the
> IPU that will add lines if it detects these short frames, until the
> programmed number of lines are reached. Apparently that is 

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Steve Longerbeam]

On 03/13/2017 03:53 AM, Hans Verkuil wrote:

On 03/13/2017 11:45 AM, Russell King - ARM Linux wrote:

On Mon, Mar 13, 2017 at 11:02:34AM +0100, Hans Verkuil wrote:

On 03/11/2017 07:14 PM, Steve Longerbeam wrote:

The event must be user visible, otherwise the user has no indication
the error, and can't correct it by stream restart.


In that case the driver can detect this and call vb2_queue_error. It's
what it is there for.

The event doesn't help you since only this driver has this issue. So nobody
will watch this event, unless it is sw specifically written for this SoC.

Much better to call vb2_queue_error to signal a fatal error (which this
apparently is) since there are more drivers that do this, and vivid supports
triggering this condition as well.


So today, I can fiddle around with the IMX219 registers to help gain
an understanding of how this sensor works.  Several of the registers
(such as the PLL setup [*]) require me to disable streaming on the
sensor while changing them.

This is something I've done many times while testing various ideas,
and is my primary way of figuring out and testing such things.

Whenever I resume streaming (provided I've let the sensor stop
streaming at a frame boundary) it resumes as if nothing happened.  If I
stop the sensor mid-frame, then I get the rolling issue that Steve
reports, but once the top of the frame becomes aligned with the top of
the capture, everything then becomes stable again as if nothing happened.

The side effect of what you're proposing is that when I disable streaming
at the sensor by poking at its registers, rather than the capture just
stopping, an error is going to be delivered to gstreamer, and gstreamer
is going to exit, taking the entire capture process down.

This severely restricts the ability to be able to develop and test
sensor drivers.

So, I strongly disagree with you.

Loss of capture frames is not necessarily a fatal error - as I have been
saying repeatedly.  In Steve's case, there's some unknown interaction
between the source and iMX6 hardware that is causing the instability,
but that is simply not true of other sources, and I oppose any idea that
we should cripple the iMX6 side of the capture based upon just one
hardware combination where this is a problem.

Steve suggested that the problem could be in the iMX6 CSI block - and I
note comparing Steve's code with the code in FSL's repository that there
are some changes that are missing in Steve's code to do with the CCIR656
sync code setup, particularly for >8 bit.  The progressive CCIR656 8-bit
setup looks pretty similar though - but I think what needs to be asked
is whether the same problem is visible using the FSL/NXP vendor kernel.


* - the PLL setup is something that requires research at the moment.
Sony's official position (even to their customers) is that they do not
supply the necessary information, instead they expect customers to tell
them the capture settings they want, and Sony will throw the values into
a spreadsheet, and they'll supply the register settings back to the
customer.  Hence, the only way to proceed with a generic driver for
this sensor is to experiment, and experimenting requires the ability to
pause the stream at the sensor while making changes.  Take this away,
and we're stuck with the tables-of-register-settings-for-set-of-fixed-
capture-settings approach.  I've made a lot of progress away from this
which is all down to the flexibility afforded by _not_ killing the
capture process.



In other words: Steve should either find a proper fix for this, or only
call vb2_queue_error in this specific case. Sending an event that nobody
will know how to handle or what to do with is pretty pointless IMHO.

Let's just give him time to try and figure out the real issue here.



This is a long-standing issue, I've traveled to Hildesheim working with
our customer to try and get to the bottom of it. I can go into a lot of
details from those trips, we probed the bt.656 bus with a logic analyzer
and I can share those results with anyone who asks. But the results of
those investigations indicate the CSI is not handling the SAV/EAV sync
codes correctly - if there is a shift in the line position at which
those codes occur, the CSI/IPU does not abort the frame capture DMA
and start from the new sync code position, it just continues to capture
lines until the programmed number of lines are transferred, hence you
get these split images. Freescale also informed us of a mechanism in the
IPU that will add lines if it detects these short frames, until the
programmed number of lines are reached. Apparently that is what creates 
the rolling effect, but this rolling can last for up to a full minute,

which is completely unacceptable, it must be corrected as soon as
possible.

So the only thing we could come up with was to monitor frame intervals,
this is purely empirical, but we observed that frame intervals drop
by approx. one line time (~60 usec) when these short frames are

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Hans Verkuil]

On 03/13/2017 11:45 AM, Russell King - ARM Linux wrote:
> On Mon, Mar 13, 2017 at 11:02:34AM +0100, Hans Verkuil wrote:
>> On 03/11/2017 07:14 PM, Steve Longerbeam wrote:
>>> The event must be user visible, otherwise the user has no indication
>>> the error, and can't correct it by stream restart.
>>
>> In that case the driver can detect this and call vb2_queue_error. It's
>> what it is there for.
>>
>> The event doesn't help you since only this driver has this issue. So nobody
>> will watch this event, unless it is sw specifically written for this SoC.
>>
>> Much better to call vb2_queue_error to signal a fatal error (which this
>> apparently is) since there are more drivers that do this, and vivid supports
>> triggering this condition as well.
> 
> So today, I can fiddle around with the IMX219 registers to help gain
> an understanding of how this sensor works.  Several of the registers
> (such as the PLL setup [*]) require me to disable streaming on the
> sensor while changing them.
> 
> This is something I've done many times while testing various ideas,
> and is my primary way of figuring out and testing such things.
> 
> Whenever I resume streaming (provided I've let the sensor stop
> streaming at a frame boundary) it resumes as if nothing happened.  If I
> stop the sensor mid-frame, then I get the rolling issue that Steve
> reports, but once the top of the frame becomes aligned with the top of
> the capture, everything then becomes stable again as if nothing happened.
> 
> The side effect of what you're proposing is that when I disable streaming
> at the sensor by poking at its registers, rather than the capture just
> stopping, an error is going to be delivered to gstreamer, and gstreamer
> is going to exit, taking the entire capture process down.
> 
> This severely restricts the ability to be able to develop and test
> sensor drivers.
> 
> So, I strongly disagree with you.
> 
> Loss of capture frames is not necessarily a fatal error - as I have been
> saying repeatedly.  In Steve's case, there's some unknown interaction
> between the source and iMX6 hardware that is causing the instability,
> but that is simply not true of other sources, and I oppose any idea that
> we should cripple the iMX6 side of the capture based upon just one
> hardware combination where this is a problem.
> 
> Steve suggested that the problem could be in the iMX6 CSI block - and I
> note comparing Steve's code with the code in FSL's repository that there
> are some changes that are missing in Steve's code to do with the CCIR656
> sync code setup, particularly for >8 bit.  The progressive CCIR656 8-bit
> setup looks pretty similar though - but I think what needs to be asked
> is whether the same problem is visible using the FSL/NXP vendor kernel.
> 
> 
> * - the PLL setup is something that requires research at the moment.
> Sony's official position (even to their customers) is that they do not
> supply the necessary information, instead they expect customers to tell
> them the capture settings they want, and Sony will throw the values into
> a spreadsheet, and they'll supply the register settings back to the
> customer.  Hence, the only way to proceed with a generic driver for
> this sensor is to experiment, and experimenting requires the ability to
> pause the stream at the sensor while making changes.  Take this away,
> and we're stuck with the tables-of-register-settings-for-set-of-fixed-
> capture-settings approach.  I've made a lot of progress away from this
> which is all down to the flexibility afforded by _not_ killing the
> capture process.
> 

In other words: Steve should either find a proper fix for this, or only
call vb2_queue_error in this specific case. Sending an event that nobody
will know how to handle or what to do with is pretty pointless IMHO.

Let's just give him time to try and figure out the real issue here.

Regards,

Hans

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Russell King - ARM Linux]

On Mon, Mar 13, 2017 at 11:02:34AM +0100, Hans Verkuil wrote:
> On 03/11/2017 07:14 PM, Steve Longerbeam wrote:
> > The event must be user visible, otherwise the user has no indication
> > the error, and can't correct it by stream restart.
> 
> In that case the driver can detect this and call vb2_queue_error. It's
> what it is there for.
> 
> The event doesn't help you since only this driver has this issue. So nobody
> will watch this event, unless it is sw specifically written for this SoC.
> 
> Much better to call vb2_queue_error to signal a fatal error (which this
> apparently is) since there are more drivers that do this, and vivid supports
> triggering this condition as well.

So today, I can fiddle around with the IMX219 registers to help gain
an understanding of how this sensor works.  Several of the registers
(such as the PLL setup [*]) require me to disable streaming on the
sensor while changing them.

This is something I've done many times while testing various ideas,
and is my primary way of figuring out and testing such things.

Whenever I resume streaming (provided I've let the sensor stop
streaming at a frame boundary) it resumes as if nothing happened.  If I
stop the sensor mid-frame, then I get the rolling issue that Steve
reports, but once the top of the frame becomes aligned with the top of
the capture, everything then becomes stable again as if nothing happened.

The side effect of what you're proposing is that when I disable streaming
at the sensor by poking at its registers, rather than the capture just
stopping, an error is going to be delivered to gstreamer, and gstreamer
is going to exit, taking the entire capture process down.

This severely restricts the ability to be able to develop and test
sensor drivers.

So, I strongly disagree with you.

Loss of capture frames is not necessarily a fatal error - as I have been
saying repeatedly.  In Steve's case, there's some unknown interaction
between the source and iMX6 hardware that is causing the instability,
but that is simply not true of other sources, and I oppose any idea that
we should cripple the iMX6 side of the capture based upon just one
hardware combination where this is a problem.

Steve suggested that the problem could be in the iMX6 CSI block - and I
note comparing Steve's code with the code in FSL's repository that there
are some changes that are missing in Steve's code to do with the CCIR656
sync code setup, particularly for >8 bit.  The progressive CCIR656 8-bit
setup looks pretty similar though - but I think what needs to be asked
is whether the same problem is visible using the FSL/NXP vendor kernel.


* - the PLL setup is something that requires research at the moment.
Sony's official position (even to their customers) is that they do not
supply the necessary information, instead they expect customers to tell
them the capture settings they want, and Sony will throw the values into
a spreadsheet, and they'll supply the register settings back to the
customer.  Hence, the only way to proceed with a generic driver for
this sensor is to experiment, and experimenting requires the ability to
pause the stream at the sensor while making changes.  Take this away,
and we're stuck with the tables-of-register-settings-for-set-of-fixed-
capture-settings approach.  I've made a lot of progress away from this
which is all down to the flexibility afforded by _not_ killing the
capture process.

-- 
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 15/39] [media] v4l2: add a frame interval error event

[Hans Verkuil]

On 03/11/2017 07:14 PM, Steve Longerbeam wrote:
> 
> 
> On 03/11/2017 03:39 AM, Hans Verkuil wrote:
>> On 10/03/17 19:37, Steve Longerbeam wrote:
>>> Hi Hans,
>>>
>>> On 03/10/2017 04:03 AM, Hans Verkuil wrote:
 On 10/03/17 05:52, Steve Longerbeam wrote:
> Add a new FRAME_INTERVAL_ERROR event to signal that a video capture or
> output device has measured an interval between the reception or transmit
> completion of two consecutive frames of video that is outside the nominal
> frame interval by some tolerance value.

 Reading back what was said on this I agree with Sakari that this doesn't
 belong here.

 Userspace can detect this just as easily (if not easier) with a timeout.

>>>
>>>
>>> Unfortunately measuring frame intervals from userland is not accurate
>>> enough for i.MX6.
>>>
>>> The issue here is that the IPUv3, specifically the CSI unit, can
>>> permanently lose vertical sync if there are truncated frames sent
>>> on the bt.656 bus. We have seen a single missing line of video cause
>>> loss of vertical sync. The only way to correct this is to shutdown
>>> the IPU capture hardware and restart, which can be accomplished
>>> simply by restarting streaming from userland.
>>>
>>> There are no other indicators from the sensor about these short
>>> frame events (believe me, we've exhausted all avenues with the ADV718x).
>>> And the IPUv3 DMA engine has no status indicators for short frames
>>> either. So the only way to detect them is by measuring frame intervals.
>>>
>>> The intervals have to be able to resolve a single line of missing video.
>>> With a PAL video source that requires better than 58 usec accuracy.
>>>
>>> There is too much uncertainty to resolve this at user level. The
>>> driver is able to resolve this by measuring intervals between hardware
>>> interrupts as long as interrupt latency is reasonably low, and we
>>> have another method using the i.MX6 hardware input capture support
>>> that can measure these intervals very accurately with no errors
>>> introduced by interrupt latency.
>>>
>>> I made this event a private event to imx-media driver in a previous
>>> iteration, so I can return it to a private event, but this can't be
>>> done at user level.
>>
>> It's fine to use an internal event as long as the end-user doesn't
>> see it. But if you lose vsyncs, then you never capture another frame,
>> right?
> 
> No, that's not correct. By loss of vertical sync, I mean the IPU
> captures portions of two different frames, resulting in a permanent
> "split image", with one frame containing portions of two consecutive
> images. Or, the video rolls continuously, if you remember the old CRT
> television sets of yore, it's the same rolling effect.
> 
> 
>> So userspace can detect that (i.e. no new frames arrive) and
>> it can timeout on that. Or you detect it in the driver and restart there,
>> or call vb2_queue_error().
>>
> 
> There is no timeout, the frames keep coming, but they are split images
> or rolling.

Ah, OK. That wasn't clear to me from the description.

> 
>> Anything really as long as this event isn't user-visible :-)
> 
> The event must be user visible, otherwise the user has no indication
> the error, and can't correct it by stream restart.

In that case the driver can detect this and call vb2_queue_error. It's
what it is there for.

The event doesn't help you since only this driver has this issue. So nobody
will watch this event, unless it is sw specifically written for this SoC.

Much better to call vb2_queue_error to signal a fatal error (which this
apparently is) since there are more drivers that do this, and vivid supports
triggering this condition as well.

Regards,

Hans

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Steve Longerbeam]

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

On Sat, Mar 11, 2017 at 10:14:49AM -0800, Steve Longerbeam wrote:

On 03/11/2017 03:39 AM, Hans Verkuil wrote:

It's fine to use an internal event as long as the end-user doesn't
see it. But if you lose vsyncs, then you never capture another frame,
right?


No, that's not correct. By loss of vertical sync, I mean the IPU
captures portions of two different frames, resulting in a permanent
"split image", with one frame containing portions of two consecutive
images. Or, the video rolls continuously, if you remember the old CRT
television sets of yore, it's the same rolling effect.


I have seen that rolling effect, but the iMX6 regains correct sync
within one complete "roll" just fine here with IMX219.  However, it
has always recovered.

So, I don't think there's a problem with the iMX6 part of the
processing, and so I don't think we should cripple the iMX6 capture
drivers for this problem.


And there is no crippling going on. Measuring the frame intervals
adds very little overhead.

Steve

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Steve Longerbeam]

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

On Sat, Mar 11, 2017 at 10:14:49AM -0800, Steve Longerbeam wrote:

On 03/11/2017 03:39 AM, Hans Verkuil wrote:

It's fine to use an internal event as long as the end-user doesn't
see it. But if you lose vsyncs, then you never capture another frame,
right?


No, that's not correct. By loss of vertical sync, I mean the IPU
captures portions of two different frames, resulting in a permanent
"split image", with one frame containing portions of two consecutive
images. Or, the video rolls continuously, if you remember the old CRT
television sets of yore, it's the same rolling effect.


I have seen that rolling effect, but the iMX6 regains correct sync
within one complete "roll" just fine here with IMX219.  However, it
has always recovered.


I've seen permanent split images, and rolling that continues for
minutes.




So, I don't think there's a problem with the iMX6 part of the
processing, and so I don't think we should cripple the iMX6 capture
drivers for this problem.

It seems to me that the problem is with the source.


The the problem is almost surely in the CSI. It's handling of the
bt.656 sync codes is broken in some way.

Steve

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Russell King - ARM Linux]

On Sat, Mar 11, 2017 at 10:14:49AM -0800, Steve Longerbeam wrote:
> On 03/11/2017 03:39 AM, Hans Verkuil wrote:
> >It's fine to use an internal event as long as the end-user doesn't
> >see it. But if you lose vsyncs, then you never capture another frame,
> >right?
> 
> No, that's not correct. By loss of vertical sync, I mean the IPU
> captures portions of two different frames, resulting in a permanent
> "split image", with one frame containing portions of two consecutive
> images. Or, the video rolls continuously, if you remember the old CRT
> television sets of yore, it's the same rolling effect.

I have seen that rolling effect, but the iMX6 regains correct sync
within one complete "roll" just fine here with IMX219.  However, it
has always recovered.

So, I don't think there's a problem with the iMX6 part of the
processing, and so I don't think we should cripple the iMX6 capture
drivers for this problem.

It seems to me that the problem is with the source.

-- 
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 15/39] [media] v4l2: add a frame interval error event

[Steve Longerbeam]

On 03/11/2017 03:39 AM, Hans Verkuil wrote:

On 10/03/17 19:37, Steve Longerbeam wrote:

Hi Hans,

On 03/10/2017 04:03 AM, Hans Verkuil wrote:

On 10/03/17 05:52, Steve Longerbeam wrote:

Add a new FRAME_INTERVAL_ERROR event to signal that a video capture or
output device has measured an interval between the reception or transmit
completion of two consecutive frames of video that is outside the nominal
frame interval by some tolerance value.


Reading back what was said on this I agree with Sakari that this doesn't
belong here.

Userspace can detect this just as easily (if not easier) with a timeout.




Unfortunately measuring frame intervals from userland is not accurate
enough for i.MX6.

The issue here is that the IPUv3, specifically the CSI unit, can
permanently lose vertical sync if there are truncated frames sent
on the bt.656 bus. We have seen a single missing line of video cause
loss of vertical sync. The only way to correct this is to shutdown
the IPU capture hardware and restart, which can be accomplished
simply by restarting streaming from userland.

There are no other indicators from the sensor about these short
frame events (believe me, we've exhausted all avenues with the ADV718x).
And the IPUv3 DMA engine has no status indicators for short frames
either. So the only way to detect them is by measuring frame intervals.

The intervals have to be able to resolve a single line of missing video.
With a PAL video source that requires better than 58 usec accuracy.

There is too much uncertainty to resolve this at user level. The
driver is able to resolve this by measuring intervals between hardware
interrupts as long as interrupt latency is reasonably low, and we
have another method using the i.MX6 hardware input capture support
that can measure these intervals very accurately with no errors
introduced by interrupt latency.

I made this event a private event to imx-media driver in a previous
iteration, so I can return it to a private event, but this can't be
done at user level.


It's fine to use an internal event as long as the end-user doesn't
see it. But if you lose vsyncs, then you never capture another frame,
right?


No, that's not correct. By loss of vertical sync, I mean the IPU
captures portions of two different frames, resulting in a permanent
"split image", with one frame containing portions of two consecutive
images. Or, the video rolls continuously, if you remember the old CRT
television sets of yore, it's the same rolling effect.



So userspace can detect that (i.e. no new frames arrive) and
it can timeout on that. Or you detect it in the driver and restart there,
or call vb2_queue_error().



There is no timeout, the frames keep coming, but they are split images
or rolling.


Anything really as long as this event isn't user-visible :-)


The event must be user visible, otherwise the user has no indication
the error, and can't correct it by stream restart.

Steve

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Hans Verkuil]

On 10/03/17 19:37, Steve Longerbeam wrote:
> Hi Hans,
> 
> On 03/10/2017 04:03 AM, Hans Verkuil wrote:
>> On 10/03/17 05:52, Steve Longerbeam wrote:
>>> Add a new FRAME_INTERVAL_ERROR event to signal that a video capture or
>>> output device has measured an interval between the reception or transmit
>>> completion of two consecutive frames of video that is outside the nominal
>>> frame interval by some tolerance value.
>>
>> Reading back what was said on this I agree with Sakari that this doesn't
>> belong here.
>>
>> Userspace can detect this just as easily (if not easier) with a timeout.
>>
> 
> 
> Unfortunately measuring frame intervals from userland is not accurate
> enough for i.MX6.
> 
> The issue here is that the IPUv3, specifically the CSI unit, can
> permanently lose vertical sync if there are truncated frames sent
> on the bt.656 bus. We have seen a single missing line of video cause
> loss of vertical sync. The only way to correct this is to shutdown
> the IPU capture hardware and restart, which can be accomplished
> simply by restarting streaming from userland.
> 
> There are no other indicators from the sensor about these short
> frame events (believe me, we've exhausted all avenues with the ADV718x).
> And the IPUv3 DMA engine has no status indicators for short frames
> either. So the only way to detect them is by measuring frame intervals.
> 
> The intervals have to be able to resolve a single line of missing video.
> With a PAL video source that requires better than 58 usec accuracy.
> 
> There is too much uncertainty to resolve this at user level. The
> driver is able to resolve this by measuring intervals between hardware
> interrupts as long as interrupt latency is reasonably low, and we
> have another method using the i.MX6 hardware input capture support
> that can measure these intervals very accurately with no errors
> introduced by interrupt latency.
> 
> I made this event a private event to imx-media driver in a previous
> iteration, so I can return it to a private event, but this can't be
> done at user level.

It's fine to use an internal event as long as the end-user doesn't
see it. But if you lose vsyncs, then you never capture another frame,
right? So userspace can detect that (i.e. no new frames arrive) and
it can timeout on that. Or you detect it in the driver and restart there,
or call vb2_queue_error().

Anything really as long as this event isn't user-visible :-)

Regards,

Hans

> 
> Steve
> 
> 
>>
>>>
>>> Signed-off-by: Steve Longerbeam 
>>> ---
>>>  Documentation/media/uapi/v4l/vidioc-dqevent.rst | 6 ++
>>>  Documentation/media/videodev2.h.rst.exceptions  | 1 +
>>>  include/uapi/linux/videodev2.h  | 1 +
>>>  3 files changed, 8 insertions(+)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst 
>>> b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
>>> index 8d663a7..dc77363 100644
>>> --- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst
>>> +++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
>>> @@ -197,6 +197,12 @@ call.
>>>  the regions changes. This event has a struct
>>>  :c:type:`v4l2_event_motion_det`
>>>  associated with it.
>>> +* - ``V4L2_EVENT_FRAME_INTERVAL_ERROR``
>>> +  - 7
>>> +  - This event is triggered when the video capture or output device
>>> +has measured an interval between the reception or transmit
>>> +completion of two consecutive frames of video that is outside
>>> +the nominal frame interval by some tolerance value.
>>>  * - ``V4L2_EVENT_PRIVATE_START``
>>>- 0x0800
>>>- Base event number for driver-private events.
>>> diff --git a/Documentation/media/videodev2.h.rst.exceptions 
>>> b/Documentation/media/videodev2.h.rst.exceptions
>>> index e11a0d0..c7d8fad 100644
>>> --- a/Documentation/media/videodev2.h.rst.exceptions
>>> +++ b/Documentation/media/videodev2.h.rst.exceptions
>>> @@ -459,6 +459,7 @@ replace define V4L2_EVENT_CTRL event-type
>>>  replace define V4L2_EVENT_FRAME_SYNC event-type
>>>  replace define V4L2_EVENT_SOURCE_CHANGE event-type
>>>  replace define V4L2_EVENT_MOTION_DET event-type
>>> +replace define V4L2_EVENT_FRAME_INTERVAL_ERROR event-type
>>>  replace define V4L2_EVENT_PRIVATE_START event-type
>>>
>>>  replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 45184a2..cf5a0d0 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -2131,6 +2131,7 @@ struct v4l2_streamparm {
>>>  #define V4L2_EVENT_FRAME_SYNC4
>>>  #define V4L2_EVENT_SOURCE_CHANGE5
>>>  #define V4L2_EVENT_MOTION_DET6
>>> +#define V4L2_EVENT_FRAME_INTERVAL_ERROR7
>>>  #define V4L2_EVENT_PRIVATE_START0x0800
>>>
>>>  /* Payload for V4L2_EVENT_VSYNC */
>>>
>>
> 

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Steve Longerbeam]

On 03/10/2017 03:30 PM, Pavel Machek wrote:

On Fri 2017-03-10 10:37:21, Steve Longerbeam wrote:

Hi Hans,

On 03/10/2017 04:03 AM, Hans Verkuil wrote:

On 10/03/17 05:52, Steve Longerbeam wrote:

Add a new FRAME_INTERVAL_ERROR event to signal that a video capture or
output device has measured an interval between the reception or transmit
completion of two consecutive frames of video that is outside the nominal
frame interval by some tolerance value.


Reading back what was said on this I agree with Sakari that this doesn't
belong here.

Userspace can detect this just as easily (if not easier) with a timeout.




Unfortunately measuring frame intervals from userland is not accurate
enough for i.MX6.

The issue here is that the IPUv3, specifically the CSI unit, can
permanently lose vertical sync if there are truncated frames sent
on the bt.656 bus. We have seen a single missing line of video cause
loss of vertical sync. The only way to correct this is to shutdown
the IPU capture hardware and restart, which can be accomplished
simply by restarting streaming from userland.

There are no other indicators from the sensor about these short
frame events (believe me, we've exhausted all avenues with the ADV718x).
And the IPUv3 DMA engine has no status indicators for short frames
either. So the only way to detect them is by measuring frame intervals.

The intervals have to be able to resolve a single line of missing video.
With a PAL video source that requires better than 58 usec accuracy.

There is too much uncertainty to resolve this at user level. The
driver is able to resolve this by measuring intervals between hardware
interrupts as long as interrupt latency is reasonably low, and we
have another method using the i.MX6 hardware input capture support
that can measure these intervals very accurately with no errors
introduced by interrupt latency.


Requiring < 58 usec interrupt latency for correct operation is a
little too optimistic, no?



No it's not too optimistic, from experience the imx6 kernel has irq
latency less than 10 usec under normal system load. False events can be
generated if the latency gets bad, it's true, and that's why there is
the imx6 timer input capture approach.

Steve

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Pavel Machek]

On Fri 2017-03-10 10:37:21, Steve Longerbeam wrote:
> Hi Hans,
> 
> On 03/10/2017 04:03 AM, Hans Verkuil wrote:
> >On 10/03/17 05:52, Steve Longerbeam wrote:
> >>Add a new FRAME_INTERVAL_ERROR event to signal that a video capture or
> >>output device has measured an interval between the reception or transmit
> >>completion of two consecutive frames of video that is outside the nominal
> >>frame interval by some tolerance value.
> >
> >Reading back what was said on this I agree with Sakari that this doesn't
> >belong here.
> >
> >Userspace can detect this just as easily (if not easier) with a timeout.
> >
> 
> 
> Unfortunately measuring frame intervals from userland is not accurate
> enough for i.MX6.
> 
> The issue here is that the IPUv3, specifically the CSI unit, can
> permanently lose vertical sync if there are truncated frames sent
> on the bt.656 bus. We have seen a single missing line of video cause
> loss of vertical sync. The only way to correct this is to shutdown
> the IPU capture hardware and restart, which can be accomplished
> simply by restarting streaming from userland.
> 
> There are no other indicators from the sensor about these short
> frame events (believe me, we've exhausted all avenues with the ADV718x).
> And the IPUv3 DMA engine has no status indicators for short frames
> either. So the only way to detect them is by measuring frame intervals.
> 
> The intervals have to be able to resolve a single line of missing video.
> With a PAL video source that requires better than 58 usec accuracy.
> 
> There is too much uncertainty to resolve this at user level. The
> driver is able to resolve this by measuring intervals between hardware
> interrupts as long as interrupt latency is reasonably low, and we
> have another method using the i.MX6 hardware input capture support
> that can measure these intervals very accurately with no errors
> introduced by interrupt latency.

Requiring < 58 usec interrupt latency for correct operation is a
little too optimistic, no?
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Steve Longerbeam]

Hi Hans,

On 03/10/2017 04:03 AM, Hans Verkuil wrote:

On 10/03/17 05:52, Steve Longerbeam wrote:

Add a new FRAME_INTERVAL_ERROR event to signal that a video capture or
output device has measured an interval between the reception or transmit
completion of two consecutive frames of video that is outside the nominal
frame interval by some tolerance value.


Reading back what was said on this I agree with Sakari that this doesn't
belong here.

Userspace can detect this just as easily (if not easier) with a timeout.




Unfortunately measuring frame intervals from userland is not accurate
enough for i.MX6.

The issue here is that the IPUv3, specifically the CSI unit, can
permanently lose vertical sync if there are truncated frames sent
on the bt.656 bus. We have seen a single missing line of video cause
loss of vertical sync. The only way to correct this is to shutdown
the IPU capture hardware and restart, which can be accomplished
simply by restarting streaming from userland.

There are no other indicators from the sensor about these short
frame events (believe me, we've exhausted all avenues with the ADV718x).
And the IPUv3 DMA engine has no status indicators for short frames
either. So the only way to detect them is by measuring frame intervals.

The intervals have to be able to resolve a single line of missing video.
With a PAL video source that requires better than 58 usec accuracy.

There is too much uncertainty to resolve this at user level. The
driver is able to resolve this by measuring intervals between hardware
interrupts as long as interrupt latency is reasonably low, and we
have another method using the i.MX6 hardware input capture support
that can measure these intervals very accurately with no errors
introduced by interrupt latency.

I made this event a private event to imx-media driver in a previous
iteration, so I can return it to a private event, but this can't be
done at user level.

Steve






Signed-off-by: Steve Longerbeam 
---
 Documentation/media/uapi/v4l/vidioc-dqevent.rst | 6 ++
 Documentation/media/videodev2.h.rst.exceptions  | 1 +
 include/uapi/linux/videodev2.h  | 1 +
 3 files changed, 8 insertions(+)

diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst 
b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
index 8d663a7..dc77363 100644
--- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst
+++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
@@ -197,6 +197,12 @@ call.
the regions changes. This event has a struct
:c:type:`v4l2_event_motion_det`
associated with it.
+* - ``V4L2_EVENT_FRAME_INTERVAL_ERROR``
+  - 7
+  - This event is triggered when the video capture or output device
+   has measured an interval between the reception or transmit
+   completion of two consecutive frames of video that is outside
+   the nominal frame interval by some tolerance value.
 * - ``V4L2_EVENT_PRIVATE_START``
   - 0x0800
   - Base event number for driver-private events.
diff --git a/Documentation/media/videodev2.h.rst.exceptions 
b/Documentation/media/videodev2.h.rst.exceptions
index e11a0d0..c7d8fad 100644
--- a/Documentation/media/videodev2.h.rst.exceptions
+++ b/Documentation/media/videodev2.h.rst.exceptions
@@ -459,6 +459,7 @@ replace define V4L2_EVENT_CTRL event-type
 replace define V4L2_EVENT_FRAME_SYNC event-type
 replace define V4L2_EVENT_SOURCE_CHANGE event-type
 replace define V4L2_EVENT_MOTION_DET event-type
+replace define V4L2_EVENT_FRAME_INTERVAL_ERROR event-type
 replace define V4L2_EVENT_PRIVATE_START event-type

 replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 45184a2..cf5a0d0 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2131,6 +2131,7 @@ struct v4l2_streamparm {
 #define V4L2_EVENT_FRAME_SYNC  4
 #define V4L2_EVENT_SOURCE_CHANGE   5
 #define V4L2_EVENT_MOTION_DET  6
+#define V4L2_EVENT_FRAME_INTERVAL_ERROR7
 #define V4L2_EVENT_PRIVATE_START   0x0800

 /* Payload for V4L2_EVENT_VSYNC */

Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event

[Hans Verkuil]

On 10/03/17 05:52, Steve Longerbeam wrote:
> Add a new FRAME_INTERVAL_ERROR event to signal that a video capture or
> output device has measured an interval between the reception or transmit
> completion of two consecutive frames of video that is outside the nominal
> frame interval by some tolerance value.

Reading back what was said on this I agree with Sakari that this doesn't
belong here.

Userspace can detect this just as easily (if not easier) with a timeout.

Regards,

Hans

> 
> Signed-off-by: Steve Longerbeam 
> ---
>  Documentation/media/uapi/v4l/vidioc-dqevent.rst | 6 ++
>  Documentation/media/videodev2.h.rst.exceptions  | 1 +
>  include/uapi/linux/videodev2.h  | 1 +
>  3 files changed, 8 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst 
> b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
> index 8d663a7..dc77363 100644
> --- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
> @@ -197,6 +197,12 @@ call.
>   the regions changes. This event has a struct
>   :c:type:`v4l2_event_motion_det`
>   associated with it.
> +* - ``V4L2_EVENT_FRAME_INTERVAL_ERROR``
> +  - 7
> +  - This event is triggered when the video capture or output device
> + has measured an interval between the reception or transmit
> + completion of two consecutive frames of video that is outside
> + the nominal frame interval by some tolerance value.
>  * - ``V4L2_EVENT_PRIVATE_START``
>- 0x0800
>- Base event number for driver-private events.
> diff --git a/Documentation/media/videodev2.h.rst.exceptions 
> b/Documentation/media/videodev2.h.rst.exceptions
> index e11a0d0..c7d8fad 100644
> --- a/Documentation/media/videodev2.h.rst.exceptions
> +++ b/Documentation/media/videodev2.h.rst.exceptions
> @@ -459,6 +459,7 @@ replace define V4L2_EVENT_CTRL event-type
>  replace define V4L2_EVENT_FRAME_SYNC event-type
>  replace define V4L2_EVENT_SOURCE_CHANGE event-type
>  replace define V4L2_EVENT_MOTION_DET event-type
> +replace define V4L2_EVENT_FRAME_INTERVAL_ERROR event-type
>  replace define V4L2_EVENT_PRIVATE_START event-type
>  
>  replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 45184a2..cf5a0d0 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2131,6 +2131,7 @@ struct v4l2_streamparm {
>  #define V4L2_EVENT_FRAME_SYNC4
>  #define V4L2_EVENT_SOURCE_CHANGE 5
>  #define V4L2_EVENT_MOTION_DET6
> +#define V4L2_EVENT_FRAME_INTERVAL_ERROR  7
>  #define V4L2_EVENT_PRIVATE_START 0x0800
>  
>  /* Payload for V4L2_EVENT_VSYNC */
>