Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1600662 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event Nicolas Dufresne <nicolas@ndufresne.ca> - 2017-03-14 17:30 +0100
    Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-14 17:50 +0100
      Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event Sakari Ailus <sakari.ailus@iki.fi> - 2017-03-16 23:30 +0100
    Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-14 17:50 +0100
      Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-14 18:00 +0100

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

[Multipart message — attachments visible in raw view] — view raw

Le lundi 13 mars 2017 à 10:45 +0000, 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.
> 

[toc] | [next] | [standalone]

#1600680

On 03/14/2017 09:21 AM, Nicolas Dufresne wrote:
> Le lundi 13 mars 2017 à 10:45 +0000, 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 of progress away from this
>> which is all down to the flexibility afforded by _not_ killing the
>> capture process.

[toc] | [prev] | [next] | [standalone]

#1602884

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 +0000, 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.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1600692

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.

[toc] | [prev] | [next] | [standalone]

#1600704

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web