Bug: s5p-mfc should allow multiple call to REQBUFS before we start streaming
Hi Kamil,
after a discussion on IRC, we concluded that s5p-mfc have this bug that disallow multiple reqbufs calls before streaming. This has the impact that it forces to call REQBUFS(0) before setting the new number of buffers during re-negotiation, and is against the spec too.
As an example, in reqbufs_output() REQBUFS is only allowed in QUEUE_FREE state, and setting buffers exits this state. We think that the call to <http://lxr.free-electrons.com/ident?i=reqbufs_output>s5p_mfc_open_mfc_inst() should be post-poned until STREAMON is called. <http://lxr.free-electrons.com/ident?i=reqbufs_output>
cheers,
Nicolas
<http://lxr.free-electrons.com/ident?i=reqbufs_output>
RE: s5p-mfc should allow multiple call to REQBUFS before we start streaming
Hi Nicolas,
Quoted 9 lines
From: Nicolas Dufresne [mailto:[email protected]]
Sent: Friday, August 29, 2014 3:47 PMHi Kamil,
after a discussion on IRC, we concluded that s5p-mfc have this bug that disallow multiple reqbufs calls before streaming. This has the impact that it forces to call REQBUFS(0) before setting the new number of buffers during re-negotiation, and is against the spec too.
I was out of office last week. Could you shed more light on this subject? Do you have the irc log?
Quoted 7 lines
As an example, in
reqbufs_output()REQBUFS is only allowed inQUEUE_FREEstate, and setting buffers exits this state. We think that the call to
<http://lxr.free-
electrons.com/ident?i=reqbufs_output>s5p_mfc_open_mfc_inst() should be post-poned until STREAMON is called.
<http://lxr.free-electrons.com/ident?i=reqbufs_output>
How is this connected to the renegotiation scenario? Are you sure you wanted to mention s5p_mfc_open_mfc_inst?
Quoted 3 lines
cheers,
Nicolas
<http://lxr.free-electrons.com/ident?i=reqbufs_output>
Best wishes,
Signature · 2 lines
Kamil Debski Samsung R&D Institute Poland
Re: s5p-mfc should allow multiple call to REQBUFS before we start streaming
Quoted 6 lines · Kamil Debski
Hi Nicolas,
Quoted 3 lines
From: Nicolas Dufresne [mailto:[email protected]]
Sent: Friday, August 29, 2014 3:47 PM
Quoted 8 lines
Quoted 6 lines
Hi Kamil,
after a discussion on IRC, we concluded that s5p-mfc have this bug that disallow multiple reqbufs calls before streaming. This has the impact that it forces to call REQBUFS(0) before setting the new number of buffers during re-negotiation, and is against the spec too.
I was out of office last week. Could you shed more light on this subject? Do you have the irc log?
Sorry I didn't record this one, but feel free to ping Hans V.
Quoted 9 lines
Quoted 7 lines
As an example, in
reqbufs_output()REQBUFS is only allowed inQUEUE_FREEstate, and setting buffers exits this state. We think that the call to
<http://lxr.free-
electrons.com/ident?i=reqbufs_output>s5p_mfc_open_mfc_inst() should be post-poned until STREAMON is called.
<http://lxr.free-electrons.com/ident?i=reqbufs_output>How is this connected to the renegotiation scenario? Are you sure you wanted to mention
s5p_mfc_open_mfc_inst?
This limitation in MFC causes an important conflict between old videobuf and new videobuf2 drivers. Old videobuf driver (before Hans G. proposed patch) refuse REQBUFS(0). As MFC code has this bug that refuse REBBUFS(N) if buffers are already allocated, it makes all this completely incompatible. This open_mfc call seems to be the only reason REQBUFS() cannot be called multiple time, though I must say you are better placed then me to figure this out.
cheers,
Nicolas
RE: s5p-mfc should allow multiple call to REQBUFS before we start streaming
Hi Hans, Nicolas,
Hans, would you mind commenting on this?
Quoted 32 lines
From: Nicolas Dufresne [mailto:[email protected]]
Sent: Monday, September 01, 2014 5:46 PMQuoted 6 lines · Kamil Debski
Hi Nicolas,
Quoted 3 lines
From: Nicolas Dufresne [mailto:[email protected]]
Sent: Friday, August 29, 2014 3:47 PMQuoted 7 lines
Quoted 6 lines
Hi Kamil,
after a discussion on IRC, we concluded that s5p-mfc have this bug that disallow multiple reqbufs calls before streaming. This has the impact that it forces to call REQBUFS(0) before setting the new number of buffers during re-negotiation, and is against the spec too.
I was out of office last week. Could you shed more light on this
subject?
Quoted 1 line
Do you have the irc log?
Sorry I didn't record this one, but feel free to ping Hans V.
Quoted 2 lines
Quoted 2 lines
As an example, in
reqbufs_output()REQBUFS is only allowed inQUEUE_FREEstate, and setting buffers exits this state. We thinkthat
Quoted 7 lines
Quoted 5 lines
the call to
<http://lxr.free-
electrons.com/ident?i=reqbufs_output>s5p_mfc_open_mfc_inst() should be post-poned until STREAMON is called.
<http://lxr.free-electrons.com/ident?i=reqbufs_output>How is this connected to the renegotiation scenario? Are you sure you wanted to mention
s5p_mfc_open_mfc_inst?
Quoted 8 lines
This limitation in MFC causes an important conflict between old videobuf and new videobuf2 drivers. Old videobuf driver (before Hans G. proposed
patch) refuse REQBUFS(0). As MFC code has this bug that refuse REBBUFS(N) if buffers are already allocated, it makes all this completely incompatible. Thisopen_mfccall seems to be the only reasonREQBUFS()cannot be called multiple time, though I must say you are better placed then me to figure this out.
After MFCs decoding is initialized it has a fixed number of buffers. Changing
this can be done when the stream changes its properties and resolution change is initiated. Even then all buffers have to be unmapped/reallocated/mapped.
There is a single hardware call that is a part of hardware initialization that sets the buffers' addresses.
Changing the number of buffers during decoding without explicit reason to do so (resolution change/stream properties change) would be problematic. For hardware it is very close to reinitiating decoding - it needs to be done on an I-frame, the header needs to be present. Otherwise some buffers would be lost and corruption would be introduced (lack of reference frames).
I think you mentioned negotiating the number of buffers? Did you use the V4L2_CID_MIN_BUFFERS_FOR_CAPTURE control?
I understand that before calling REQBUFS(N) with the new N value you properly
unmap the buffers just like the documentation is telling?
"Applications can call VIDIOC_REQBUFS again to change the number of buffers, however this cannot succeed when any buffers are still mapped. A count value of zero frees all buffers, after aborting or finishing any DMA in progress, an implicit VIDIOC_STREAMOFF." [1]
Could you tell me what is the scenario where you want to use the REQBUFS(N) to change number of buffers? Maybe I could understand better the problem.
Quoted 3 lines
cheers,
Nicolas
[1] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-reqbufs.html
Best wishes,
Signature · 2 lines
Kamil Debski Samsung R&D Institute Poland
Re: s5p-mfc should allow multiple call to REQBUFS before we start streaming
Quoted 45 lines · Kamil Debski
Hi Hans, Nicolas,
Hans, would you mind commenting on this?
Quoted 32 lines
From: Nicolas Dufresne [mailto:[email protected]]
Sent: Monday, September 01, 2014 5:46 PMQuoted 6 lines · Kamil Debski
Hi Nicolas,
Quoted 3 lines
From: Nicolas Dufresne [mailto:[email protected]]
Sent: Friday, August 29, 2014 3:47 PMQuoted 7 lines
Quoted 6 lines
Hi Kamil,
after a discussion on IRC, we concluded that s5p-mfc have this bug that disallow multiple reqbufs calls before streaming. This has the impact that it forces to call REQBUFS(0) before setting the new number of buffers during re-negotiation, and is against the spec too.
I was out of office last week. Could you shed more light on this
subject?
Quoted 1 line
Do you have the irc log?
Sorry I didn't record this one, but feel free to ping Hans V.
Quoted 2 lines
Quoted 2 lines
As an example, in
reqbufs_output()REQBUFS is only allowed inQUEUE_FREEstate, and setting buffers exits this state. We thinkthat
Quoted 7 lines
Quoted 5 lines
the call to
<http://lxr.free-
electrons.com/ident?i=reqbufs_output>s5p_mfc_open_mfc_inst() should be post-poned until STREAMON is called.
<http://lxr.free-electrons.com/ident?i=reqbufs_output>How is this connected to the renegotiation scenario? Are you sure you wanted to mention
s5p_mfc_open_mfc_inst?Quoted 8 lines
This limitation in MFC causes an important conflict between old videobuf and new videobuf2 drivers. Old videobuf driver (before Hans G. proposed
patch) refuse REQBUFS(0). As MFC code has this bug that refuse REBBUFS(N) if buffers are already allocated, it makes all this completely incompatible. Thisopen_mfccall seems to be the only reasonREQBUFS()cannot be called multiple time, though I must say you are better placed then me to figure this out.
Quoted 8 lines
After MFCs decoding is initialized it has a fixed number of buffers. Changing
this can be done when the stream changes its properties and resolution change is initiated. Even then all buffers have to be unmapped/reallocated/mapped.There is a single hardware call that is a part of hardware initialization that sets the buffers' addresses.
But is reqbufs the right place to initial MFC decoding? Wouldn't start_streaming be a much more logical place for this? Until start_streaming is called apps are free to request more buffers (with CREATE_BUFS), or request a new set of buffers (REQBUFS(N)). Only once start_streaming is called does everything have to be locked down and changes are no longer allowed until after stop_streaming() (or as long as buffers are still mapped).
I see no reason why REQBUFS should be doing anything other than requesting buffers.
Regards,
Hans
Quoted 30 lines
Changing the number of buffers during decoding without explicit reason to do so (resolution change/stream properties change) would be problematic. For hardware it is very close to reinitiating decoding - it needs to be done on an I-frame, the header needs to be present. Otherwise some buffers would be lost and corruption would be introduced (lack of reference frames).
I think you mentioned negotiating the number of buffers? Did you use the
V4L2_CID_MIN_BUFFERS_FOR_CAPTUREcontrol?I understand that before calling REQBUFS(N) with the new N value you properly
unmap the buffers just like the documentation is telling?"Applications can call
VIDIOC_REQBUFSagain to change the number of buffers, however this cannot succeed when any buffers are still mapped. A count value of zero frees all buffers, after aborting or finishing any DMA in progress, an implicitVIDIOC_STREAMOFF." [1]Could you tell me what is the scenario where you want to use the REQBUFS(N) to change number of buffers? Maybe I could understand better the problem.
Quoted 3 lines
cheers,
Nicolas[1] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-reqbufs.html
Best wishes,
RE: s5p-mfc should allow multiple call to REQBUFS before we start streaming
Hi Hans,
Quoted 71 lines
From: Hans Verkuil [mailto:[email protected]]
Sent: Tuesday, September 02, 2014 11:29 AMQuoted 19 lines · Kamil Debski
Hi Hans, Nicolas,
Hans, would you mind commenting on this?
Quoted 15 lines
From: Nicolas Dufresne [mailto:[email protected]]
Sent: Monday, September 01, 2014 5:46 PMQuoted 10 lines · Kamil Debski
Hi Nicolas,
Quoted 7 lines
From: Nicolas Dufresne [mailto:[email protected]]
Sent: Friday, August 29, 2014 3:47 PMHi Kamil,
after a discussion on IRC, we concluded that s5p-mfc have this bug that disallow multiple reqbufs calls before streaming. This has
the
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
impact that it forces to call REQBUFS(0) before setting the new number of buffers during re-negotiation, and is against the spec
too.
Quoted 13 lines
Quoted 13 lines
Quoted 1 line
I was out of office last week. Could you shed more light on this
subject?
Quoted 1 line
Do you have the irc log?
Sorry I didn't record this one, but feel free to ping Hans V.
Quoted 2 lines
Quoted 2 lines
As an example, in
reqbufs_output()REQBUFS is only allowed inQUEUE_FREEstate, and setting buffers exits this state. We thinkthat
Quoted 5 lines
Quoted 5 lines
the call to
<http://lxr.free-
electrons.com/ident?i=reqbufs_output>s5p_mfc_open_mfc_inst() should be post-poned until STREAMON is called.
<http://lxr.free-electrons.com/ident?i=reqbufs_output>Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
How is this connected to the renegotiation scenario? Are you sure you wanted to mention
s5p_mfc_open_mfc_inst?This limitation in MFC causes an important conflict between old videobuf and new videobuf2 drivers. Old videobuf driver (before Hans
G.
Quoted 11 lines
Quoted 7 lines
proposed
patch) refuse REQBUFS(0). As MFC code has this bug that refuse REBBUFS(N) if buffers are already allocated, it makes all this completely incompatible. Thisopen_mfccall seems to be the only reasonREQBUFS()cannot be called multiple time, though I must say you are better placed then me to figure this out.After MFCs decoding is initialized it has a fixed number of buffers. Changing
this can be done when the stream changes its properties andresolution
Quoted 5 lines
change is initiated. Even then all buffers have to be unmapped/reallocated/mapped.
There is a single hardware call that is a part of hardware initialization that sets the buffers' addresses.
But is reqbufs the right place to initial MFC decoding? Wouldn't
start_streamingbe a much more logical place for this? Untilstart_streamingis called apps are free to request more buffers (withCREATE_BUFS), or request a new set of buffers (REQBUFS(N)). Only oncestart_streamingis called does everything have to be locked down and changes are no longer allowed until afterstop_streaming()(or as long as buffers are still mapped).
Quoted 2 lines
I see no reason why REQBUFS should be doing anything other than requesting buffers.
Hans, initializing MFC decoding is quite complicated. Saying that it is initialized in reqbufs is not true.
Initialization starts with opening an instance. Then OUTPUT queue is set up and stream on. Then MFC can parse the header, know how big the CAPTURE buffers are and how many of them are necessary. Then CAPTURE queue can be configured - reqbufs, query_bufs, mapping...
If the problem is that you cannot use REQBUFS(N) to change the number of buffers then I think it can be changed with some amount of work.
MFC does not support CREATE_BUFS call as this would be problematic. A new buffers cannot be added without invalidating current buffers, which will break continuity of the stream and most likely cause corruption in subsequent frames. That is how the hardware works - buffers cannot be added freely. Before starting decoding CREATE_BUFS could be possibly used, but in case of resolution change I think it would cause a problem.
Best wishes,
Signature · 2 lines
Kamil Debski Samsung R&D Institute Poland
Quoted 28 lines
Regards,
Hans
Quoted 2 lines
Changing the number of buffers during decoding without explicit
reason
Quoted 4 lines
to do so (resolution change/stream properties change) would be problematic. For hardware it is very close to reinitiating decoding - it needs to be done on an I-frame, the header needs to be present. Otherwise some buffers would be lost and corruption would be
introduced (lack of reference frames).
Quoted 7 lines
I think you mentioned negotiating the number of buffers? Did you use the
V4L2_CID_MIN_BUFFERS_FOR_CAPTUREcontrol?I understand that before calling REQBUFS(N) with the new N value you properly unmap the buffers just like the documentation is telling?
Quoted 7 lines
"Applications can call
VIDIOC_REQBUFSagain to change the number of buffers, however this cannot succeed when any buffers are still mapped. A count value of zero frees all buffers, after aborting or finishing any DMA in progress, an implicitVIDIOC_STREAMOFF." [1]Could you tell me what is the scenario where you want to use the REQBUFS(N) to change number of buffers? Maybe I could understand
better the problem.
Re: s5p-mfc should allow multiple call to REQBUFS before we start streaming
In reply to Kamil DebskiLe 2014-09-02 05:02, Kamil Debski a écrit
Quoted 17 lines
Quoted 8 lines
This limitation in MFC causes an important conflict between old videobuf and new videobuf2 drivers. Old videobuf driver (before Hans G. proposed
patch) refuse REQBUFS(0). As MFC code has this bug that refuse REBBUFS(N) if buffers are already allocated, it makes all this completely incompatible. Thisopen_mfccall seems to be the only reasonREQBUFS()cannot be called multiple time, though I must say you are better placed then me to figure this out.After MFCs decoding is initialized it has a fixed number of buffers. Changing
this can be done when the stream changes its properties and resolution change is initiated. Even then all buffers have to be unmapped/reallocated/mapped.There is a single hardware call that is a part of hardware initialization that sets the buffers' addresses.
Quoted 8 lines
Changing the number of buffers during decoding without explicit reason to do so (resolution change/stream properties change) would be problematic. For hardware it is very close to reinitiating decoding - it needs to be done on an I-frame, the header needs to be present. Otherwise some buffers would be lost and corruption would be introduced (lack of reference frames).
I think you mentioned negotiating the number of buffers? Did you use the
V4L2_CID_MIN_BUFFERS_FOR_CAPTUREcontrol?
That is true, though the issue isn't there. Let's say you haven't started decoding yet. You do REQBUFS(2). because the hardware need that. Then before you start anything else, the topology of your display path is change, and the application need an extra 2 buffers to work properly. With all other drivers, all we'd have to do is REQBUFS(5), with MFC we would need to do REQBUFS(0) and then REQBUFS(5). This is a bug, there is no reason to force this extra step.
Quoted 4 lines
I understand that before calling REQBUFS(N) with the new N value you properly
unmap the buffers just like the documentation is telling?
As describe in the scenario, nothing has been mapped yet.
Quoted 5 lines
"Applications can call
VIDIOC_REQBUFSagain to change the number of buffers, however this cannot succeed when any buffers are still mapped. A count value of zero frees all buffers, after aborting or finishing any DMA in progress, an implicitVIDIOC_STREAMOFF." [1]
As you can see, the spec says it can be changed, having this extra step to change it does not seems compliant to me, at least it's not consistent.
cheers,
Nicolas