Bug: s5p-mfc should allow multiple call to REQBUFS before we start streaming

Bug: s5p-mfc should allow multiple call to REQBUFS before we start streaming

[Nicolas Dufresne]

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

[Kamil Debski]

Hi Nicolas,


> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
> Sent: Friday, August 29, 2014 3:47 PM
> 
> 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?

> 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>

How is this connected to the renegotiation scenario?
Are you sure you wanted to mention s5p_mfc_open_mfc_inst?
 
> cheers,
> Nicolas
> <http://lxr.free-electrons.com/ident?i=reqbufs_output>

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

Re: s5p-mfc should allow multiple call to REQBUFS before we start streaming

[Nicolas Dufresne]

Le 2014-09-01 05:43, Kamil Debski a écrit :
> Hi Nicolas,
>
>
>> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
>> Sent: Friday, August 29, 2014 3:47 PM
>>
>> 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.
>> 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>
> 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

[Kamil Debski]

Hi Hans, Nicolas,

Hans, would you mind commenting on this?

> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
> Sent: Monday, September 01, 2014 5:46 PM
> 
> 
> Le 2014-09-01 05:43, Kamil Debski a écrit :
> > Hi Nicolas,
> >
> >
> >> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
> >> Sent: Friday, August 29, 2014 3:47 PM
> >>
> >> 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.
> >> 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>
> > 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.

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.

> 
> cheers,
> Nicolas

[1] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-reqbufs.html

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

Re: s5p-mfc should allow multiple call to REQBUFS before we start streaming

[Hans Verkuil]

On 09/02/14 11:02, Kamil Debski wrote:
> Hi Hans, Nicolas,
> 
> Hans, would you mind commenting on this?
> 
>> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
>> Sent: Monday, September 01, 2014 5:46 PM
>>
>>
>> Le 2014-09-01 05:43, Kamil Debski a écrit :
>>> Hi Nicolas,
>>>
>>>
>>>> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
>>>> Sent: Friday, August 29, 2014 3:47 PM
>>>>
>>>> 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.
>>>> 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>
>>> 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.
> 
> 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

> 
> 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.
> 
>>
>> 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

[Kamil Debski]

Hi Hans,

> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
> Sent: Tuesday, September 02, 2014 11:29 AM
> 
> On 09/02/14 11:02, Kamil Debski wrote:
> > Hi Hans, Nicolas,
> >
> > Hans, would you mind commenting on this?
> >
> >> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
> >> Sent: Monday, September 01, 2014 5:46 PM
> >>
> >>
> >> Le 2014-09-01 05:43, Kamil Debski a écrit :
> >>> Hi Nicolas,
> >>>
> >>>
> >>>> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
> >>>> Sent: Friday, August 29, 2014 3:47 PM
> >>>>
> >>>> 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.
> >>>> 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>
> >>> 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.
> >
> > 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.

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,
-- 
Kamil Debski
Samsung R&D Institute Poland

> 
> Regards,
> 
> 	Hans
> 
> >
> > 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.

Re: s5p-mfc should allow multiple call to REQBUFS before we start streaming

[Nicolas Dufresne]

Le 2014-09-02 05:02, Kamil Debski a écrit
>> 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.
> 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?
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.
>
> 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.
>
> "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]
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