Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On Mon, Jun 3, 2019 at 4:26 PM Hans Verkuil wrote: > > On 5/31/19 10:01 AM, Stanimir Varbanov wrote: > > Hi, > > > > On 5/27/19 11:18 AM, Tomasz Figa wrote: > >> On Mon, May 27, 2019 at 4:39 PM Hans Verkuil wrote: > >>> > >>> On 5/27/19 5:51 AM, Tomasz Figa wrote: > On Tue, May 21, 2019 at 9:27 PM Hans Verkuil wrote: > > > > On 5/21/19 11:09 AM, Tomasz Figa wrote: > >> Hi Stan, > >> > >> On Mon, May 20, 2019 at 11:47 PM Stanimir Varbanov > >> wrote: > >>> > >>> Hi Tomasz, > >>> > >>> On 4/24/19 3:39 PM, Tomasz Figa wrote: > On Wed, Apr 24, 2019 at 9:15 PM Stanimir Varbanov > wrote: > > > > Hi Hans, > > > > On 2/15/19 3:44 PM, Hans Verkuil wrote: > >> Hi Stanimir, > >> > >> I never paid much attention to this patch series since others were > >> busy > >> discussing it and I had a lot of other things on my plate, but > >> then I heard > >> that this patch made G_FMT blocking. > > > > OK, another option could be to block REQBUF(CAPTURE) until event > > from hw > > is received that the stream is parsed and the resolution is > > correctly > > set by application. Just to note that I'd think to this like a > > temporal > > solution until gstreamer implements v4l events. > > > > Is that looks good to you? > > Hmm, I thought we concluded that gstreamer sets the width and height > in OUTPUT queue before querying the CAPTURE queue and so making the > driver calculate the CAPTURE format based on what's set on OUTPUT > would work fine. Did I miss something? > >>> > >>> Nobody is miss something. > >>> > >>> First some background about how Venus implements stateful codec API. > >>> > >>> The Venus firmware can generate two events "sufficient" and > >>> "insufficient" buffer requirements (this includes decoder output > >>> buffer > >>> size and internal/scratch buffer sizes). Presently I always set > >>> minimum > >>> possible decoder resolution no matter what the user said, and by that > >>> way I'm sure that "insufficient" event will always be triggered by the > >>> firmware (the other reason to take this path is because this is the > >>> least-common-divider for all supported Venus hw/fw versions thus > >>> common > >>> code in the driver). The reconfiguration (during codec Initialization > >>> sequence) is made from STREAMON(CAPTURE) context. Now, to make that > >>> re-configuration happen I need to wait for "insufficient" event from > >>> firmware in order to know the real coded resolution. > >>> > >>> In the case of gstreamer where v4l2_events support is missing I have > >>> to > >>> block (wait for firmware event) REQBUF(CAPTURE) (vb2::queue_setup) or > >>> STREAMON(CAPTURE) (vb2::start_streaming). > >>> > >>> I tried to set the coded resolution to the firmware as-is it set by > >>> gstreamer but then I cannot receive the "sufficient" event for VP8 and > >>> VP9 codecs. So I return back to the solution with minimum resolution > >>> above. > >>> > >>> I'm open for suggestions. > >> > >> I think you could still keep setting the minimum size and wait for the > >> "insufficient" event. At the same time, you could speculatively > >> advertise the expected "sufficient" size on the CAPTURE queue before > >> the hardware signals those. Even if you mispredict them, you'll get > >> the event, update the CAPTURE resolution and send the source change > >> event to the application, which would then give you the correct > >> buffers. Would that work for you? > > > > As I understand it this still would require event support, which > > gstreamer > > doesn't have. > > I don't think it matches what I remember from the earlier discussion. > As long as Gstreamer sets the visible resolution (from the container > AFAIR) on OUTPUT, the driver would adjust it to something that is > expected to be the right framebuffer resolution and so Gstreamer would > be able to continue. Of course if the expected value doesn't match, it > wouldn't work, but it's the same as currently for Coda AFAICT. > > > > > I think it is OK to have REQBUFS sleep in this case. However, I would > > only > > Why REQBUFS? While that could possibly allow us to allocate the right > buffers, Gstreamer wouldn't be able to know the right format, because > it would query it before REQBUFS, wouldn't it? > >>> > >>> Oops, you are right. It's got to be in G_FMT(CAPTURE), but *only* if > >>> nobody subscribed to the SOURCE_CHANGE event. > >>> > > For this reason, s5p-mfc makes G_FMT(CAPTURE) blocking and if we > decide to forcefully keep
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On 5/31/19 10:01 AM, Stanimir Varbanov wrote: > Hi, > > On 5/27/19 11:18 AM, Tomasz Figa wrote: >> On Mon, May 27, 2019 at 4:39 PM Hans Verkuil wrote: >>> >>> On 5/27/19 5:51 AM, Tomasz Figa wrote: On Tue, May 21, 2019 at 9:27 PM Hans Verkuil wrote: > > On 5/21/19 11:09 AM, Tomasz Figa wrote: >> Hi Stan, >> >> On Mon, May 20, 2019 at 11:47 PM Stanimir Varbanov >> wrote: >>> >>> Hi Tomasz, >>> >>> On 4/24/19 3:39 PM, Tomasz Figa wrote: On Wed, Apr 24, 2019 at 9:15 PM Stanimir Varbanov wrote: > > Hi Hans, > > On 2/15/19 3:44 PM, Hans Verkuil wrote: >> Hi Stanimir, >> >> I never paid much attention to this patch series since others were >> busy >> discussing it and I had a lot of other things on my plate, but then >> I heard >> that this patch made G_FMT blocking. > > OK, another option could be to block REQBUF(CAPTURE) until event from > hw > is received that the stream is parsed and the resolution is correctly > set by application. Just to note that I'd think to this like a > temporal > solution until gstreamer implements v4l events. > > Is that looks good to you? Hmm, I thought we concluded that gstreamer sets the width and height in OUTPUT queue before querying the CAPTURE queue and so making the driver calculate the CAPTURE format based on what's set on OUTPUT would work fine. Did I miss something? >>> >>> Nobody is miss something. >>> >>> First some background about how Venus implements stateful codec API. >>> >>> The Venus firmware can generate two events "sufficient" and >>> "insufficient" buffer requirements (this includes decoder output buffer >>> size and internal/scratch buffer sizes). Presently I always set minimum >>> possible decoder resolution no matter what the user said, and by that >>> way I'm sure that "insufficient" event will always be triggered by the >>> firmware (the other reason to take this path is because this is the >>> least-common-divider for all supported Venus hw/fw versions thus common >>> code in the driver). The reconfiguration (during codec Initialization >>> sequence) is made from STREAMON(CAPTURE) context. Now, to make that >>> re-configuration happen I need to wait for "insufficient" event from >>> firmware in order to know the real coded resolution. >>> >>> In the case of gstreamer where v4l2_events support is missing I have to >>> block (wait for firmware event) REQBUF(CAPTURE) (vb2::queue_setup) or >>> STREAMON(CAPTURE) (vb2::start_streaming). >>> >>> I tried to set the coded resolution to the firmware as-is it set by >>> gstreamer but then I cannot receive the "sufficient" event for VP8 and >>> VP9 codecs. So I return back to the solution with minimum resolution >>> above. >>> >>> I'm open for suggestions. >> >> I think you could still keep setting the minimum size and wait for the >> "insufficient" event. At the same time, you could speculatively >> advertise the expected "sufficient" size on the CAPTURE queue before >> the hardware signals those. Even if you mispredict them, you'll get >> the event, update the CAPTURE resolution and send the source change >> event to the application, which would then give you the correct >> buffers. Would that work for you? > > As I understand it this still would require event support, which gstreamer > doesn't have. I don't think it matches what I remember from the earlier discussion. As long as Gstreamer sets the visible resolution (from the container AFAIR) on OUTPUT, the driver would adjust it to something that is expected to be the right framebuffer resolution and so Gstreamer would be able to continue. Of course if the expected value doesn't match, it wouldn't work, but it's the same as currently for Coda AFAICT. > > I think it is OK to have REQBUFS sleep in this case. However, I would only Why REQBUFS? While that could possibly allow us to allocate the right buffers, Gstreamer wouldn't be able to know the right format, because it would query it before REQBUFS, wouldn't it? >>> >>> Oops, you are right. It's got to be in G_FMT(CAPTURE), but *only* if >>> nobody subscribed to the SOURCE_CHANGE event. >>> For this reason, s5p-mfc makes G_FMT(CAPTURE) blocking and if we decide to forcefully keep the compatibility, even with in drivers, we should probably do the same here. > enable this behavior if the application didn't subscribe to the > SOURCE_CHANGE > event. That's easy enough to check in the driver. And that means that if > the > application is well written, then the driver will behave in
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
Le vendredi 31 mai 2019 à 11:01 +0300, Stanimir Varbanov a écrit : > Hi, > > On 5/27/19 11:18 AM, Tomasz Figa wrote: > > On Mon, May 27, 2019 at 4:39 PM Hans Verkuil wrote: > > > On 5/27/19 5:51 AM, Tomasz Figa wrote: > > > > On Tue, May 21, 2019 at 9:27 PM Hans Verkuil wrote: > > > > > On 5/21/19 11:09 AM, Tomasz Figa wrote: > > > > > > Hi Stan, > > > > > > > > > > > > On Mon, May 20, 2019 at 11:47 PM Stanimir Varbanov > > > > > > wrote: > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > On 4/24/19 3:39 PM, Tomasz Figa wrote: > > > > > > > > On Wed, Apr 24, 2019 at 9:15 PM Stanimir Varbanov > > > > > > > > wrote: > > > > > > > > > Hi Hans, > > > > > > > > > > > > > > > > > > On 2/15/19 3:44 PM, Hans Verkuil wrote: > > > > > > > > > > Hi Stanimir, > > > > > > > > > > > > > > > > > > > > I never paid much attention to this patch series since > > > > > > > > > > others were busy > > > > > > > > > > discussing it and I had a lot of other things on my plate, > > > > > > > > > > but then I heard > > > > > > > > > > that this patch made G_FMT blocking. > > > > > > > > > > > > > > > > > > OK, another option could be to block REQBUF(CAPTURE) until > > > > > > > > > event from hw > > > > > > > > > is received that the stream is parsed and the resolution is > > > > > > > > > correctly > > > > > > > > > set by application. Just to note that I'd think to this like > > > > > > > > > a temporal > > > > > > > > > solution until gstreamer implements v4l events. > > > > > > > > > > > > > > > > > > Is that looks good to you? > > > > > > > > > > > > > > > > Hmm, I thought we concluded that gstreamer sets the width and > > > > > > > > height > > > > > > > > in OUTPUT queue before querying the CAPTURE queue and so making > > > > > > > > the > > > > > > > > driver calculate the CAPTURE format based on what's set on > > > > > > > > OUTPUT > > > > > > > > would work fine. Did I miss something? > > > > > > > > > > > > > > Nobody is miss something. > > > > > > > > > > > > > > First some background about how Venus implements stateful codec > > > > > > > API. > > > > > > > > > > > > > > The Venus firmware can generate two events "sufficient" and > > > > > > > "insufficient" buffer requirements (this includes decoder output > > > > > > > buffer > > > > > > > size and internal/scratch buffer sizes). Presently I always set > > > > > > > minimum > > > > > > > possible decoder resolution no matter what the user said, and by > > > > > > > that > > > > > > > way I'm sure that "insufficient" event will always be triggered > > > > > > > by the > > > > > > > firmware (the other reason to take this path is because this is > > > > > > > the > > > > > > > least-common-divider for all supported Venus hw/fw versions thus > > > > > > > common > > > > > > > code in the driver). The reconfiguration (during codec > > > > > > > Initialization > > > > > > > sequence) is made from STREAMON(CAPTURE) context. Now, to make > > > > > > > that > > > > > > > re-configuration happen I need to wait for "insufficient" event > > > > > > > from > > > > > > > firmware in order to know the real coded resolution. > > > > > > > > > > > > > > In the case of gstreamer where v4l2_events support is missing I > > > > > > > have to > > > > > > > block (wait for firmware event) REQBUF(CAPTURE) > > > > > > > (vb2::queue_setup) or > > > > > > > STREAMON(CAPTURE) (vb2::start_streaming). > > > > > > > > > > > > > > I tried to set the coded resolution to the firmware as-is it set > > > > > > > by > > > > > > > gstreamer but then I cannot receive the "sufficient" event for > > > > > > > VP8 and > > > > > > > VP9 codecs. So I return back to the solution with minimum > > > > > > > resolution above. > > > > > > > > > > > > > > I'm open for suggestions. > > > > > > > > > > > > I think you could still keep setting the minimum size and wait for > > > > > > the > > > > > > "insufficient" event. At the same time, you could speculatively > > > > > > advertise the expected "sufficient" size on the CAPTURE queue before > > > > > > the hardware signals those. Even if you mispredict them, you'll get > > > > > > the event, update the CAPTURE resolution and send the source change > > > > > > event to the application, which would then give you the correct > > > > > > buffers. Would that work for you? > > > > > > > > > > As I understand it this still would require event support, which > > > > > gstreamer > > > > > doesn't have. > > > > > > > > I don't think it matches what I remember from the earlier discussion. > > > > As long as Gstreamer sets the visible resolution (from the container > > > > AFAIR) on OUTPUT, the driver would adjust it to something that is > > > > expected to be the right framebuffer resolution and so Gstreamer would > > > > be able to continue. Of course if the expected value doesn't match, it > > > > wouldn't work, but it's the same as currently for Coda AFAICT. > > > > > > > > > I think it is OK to have REQBUFS sleep in this
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
Hi, On 5/27/19 11:18 AM, Tomasz Figa wrote: > On Mon, May 27, 2019 at 4:39 PM Hans Verkuil wrote: >> >> On 5/27/19 5:51 AM, Tomasz Figa wrote: >>> On Tue, May 21, 2019 at 9:27 PM Hans Verkuil wrote: On 5/21/19 11:09 AM, Tomasz Figa wrote: > Hi Stan, > > On Mon, May 20, 2019 at 11:47 PM Stanimir Varbanov > wrote: >> >> Hi Tomasz, >> >> On 4/24/19 3:39 PM, Tomasz Figa wrote: >>> On Wed, Apr 24, 2019 at 9:15 PM Stanimir Varbanov >>> wrote: Hi Hans, On 2/15/19 3:44 PM, Hans Verkuil wrote: > Hi Stanimir, > > I never paid much attention to this patch series since others were > busy > discussing it and I had a lot of other things on my plate, but then I > heard > that this patch made G_FMT blocking. OK, another option could be to block REQBUF(CAPTURE) until event from hw is received that the stream is parsed and the resolution is correctly set by application. Just to note that I'd think to this like a temporal solution until gstreamer implements v4l events. Is that looks good to you? >>> >>> Hmm, I thought we concluded that gstreamer sets the width and height >>> in OUTPUT queue before querying the CAPTURE queue and so making the >>> driver calculate the CAPTURE format based on what's set on OUTPUT >>> would work fine. Did I miss something? >> >> Nobody is miss something. >> >> First some background about how Venus implements stateful codec API. >> >> The Venus firmware can generate two events "sufficient" and >> "insufficient" buffer requirements (this includes decoder output buffer >> size and internal/scratch buffer sizes). Presently I always set minimum >> possible decoder resolution no matter what the user said, and by that >> way I'm sure that "insufficient" event will always be triggered by the >> firmware (the other reason to take this path is because this is the >> least-common-divider for all supported Venus hw/fw versions thus common >> code in the driver). The reconfiguration (during codec Initialization >> sequence) is made from STREAMON(CAPTURE) context. Now, to make that >> re-configuration happen I need to wait for "insufficient" event from >> firmware in order to know the real coded resolution. >> >> In the case of gstreamer where v4l2_events support is missing I have to >> block (wait for firmware event) REQBUF(CAPTURE) (vb2::queue_setup) or >> STREAMON(CAPTURE) (vb2::start_streaming). >> >> I tried to set the coded resolution to the firmware as-is it set by >> gstreamer but then I cannot receive the "sufficient" event for VP8 and >> VP9 codecs. So I return back to the solution with minimum resolution >> above. >> >> I'm open for suggestions. > > I think you could still keep setting the minimum size and wait for the > "insufficient" event. At the same time, you could speculatively > advertise the expected "sufficient" size on the CAPTURE queue before > the hardware signals those. Even if you mispredict them, you'll get > the event, update the CAPTURE resolution and send the source change > event to the application, which would then give you the correct > buffers. Would that work for you? As I understand it this still would require event support, which gstreamer doesn't have. >>> >>> I don't think it matches what I remember from the earlier discussion. >>> As long as Gstreamer sets the visible resolution (from the container >>> AFAIR) on OUTPUT, the driver would adjust it to something that is >>> expected to be the right framebuffer resolution and so Gstreamer would >>> be able to continue. Of course if the expected value doesn't match, it >>> wouldn't work, but it's the same as currently for Coda AFAICT. >>> I think it is OK to have REQBUFS sleep in this case. However, I would only >>> >>> Why REQBUFS? While that could possibly allow us to allocate the right >>> buffers, Gstreamer wouldn't be able to know the right format, because >>> it would query it before REQBUFS, wouldn't it? >> >> Oops, you are right. It's got to be in G_FMT(CAPTURE), but *only* if >> nobody subscribed to the SOURCE_CHANGE event. >> >>> >>> For this reason, s5p-mfc makes G_FMT(CAPTURE) blocking and if we >>> decide to forcefully keep the compatibility, even with in drivers, we >>> should probably do the same here. >>> enable this behavior if the application didn't subscribe to the SOURCE_CHANGE event. That's easy enough to check in the driver. And that means that if the application is well written, then the driver will behave in a completely standard way that the compliance test can check. >>> >>> I guess one could have some helpers for this. They would listen to the >>> source change
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On Mon, May 27, 2019 at 4:39 PM Hans Verkuil wrote: > > On 5/27/19 5:51 AM, Tomasz Figa wrote: > > On Tue, May 21, 2019 at 9:27 PM Hans Verkuil wrote: > >> > >> On 5/21/19 11:09 AM, Tomasz Figa wrote: > >>> Hi Stan, > >>> > >>> On Mon, May 20, 2019 at 11:47 PM Stanimir Varbanov > >>> wrote: > > Hi Tomasz, > > On 4/24/19 3:39 PM, Tomasz Figa wrote: > > On Wed, Apr 24, 2019 at 9:15 PM Stanimir Varbanov > > wrote: > >> > >> Hi Hans, > >> > >> On 2/15/19 3:44 PM, Hans Verkuil wrote: > >>> Hi Stanimir, > >>> > >>> I never paid much attention to this patch series since others were > >>> busy > >>> discussing it and I had a lot of other things on my plate, but then I > >>> heard > >>> that this patch made G_FMT blocking. > >> > >> OK, another option could be to block REQBUF(CAPTURE) until event from > >> hw > >> is received that the stream is parsed and the resolution is correctly > >> set by application. Just to note that I'd think to this like a temporal > >> solution until gstreamer implements v4l events. > >> > >> Is that looks good to you? > > > > Hmm, I thought we concluded that gstreamer sets the width and height > > in OUTPUT queue before querying the CAPTURE queue and so making the > > driver calculate the CAPTURE format based on what's set on OUTPUT > > would work fine. Did I miss something? > > Nobody is miss something. > > First some background about how Venus implements stateful codec API. > > The Venus firmware can generate two events "sufficient" and > "insufficient" buffer requirements (this includes decoder output buffer > size and internal/scratch buffer sizes). Presently I always set minimum > possible decoder resolution no matter what the user said, and by that > way I'm sure that "insufficient" event will always be triggered by the > firmware (the other reason to take this path is because this is the > least-common-divider for all supported Venus hw/fw versions thus common > code in the driver). The reconfiguration (during codec Initialization > sequence) is made from STREAMON(CAPTURE) context. Now, to make that > re-configuration happen I need to wait for "insufficient" event from > firmware in order to know the real coded resolution. > > In the case of gstreamer where v4l2_events support is missing I have to > block (wait for firmware event) REQBUF(CAPTURE) (vb2::queue_setup) or > STREAMON(CAPTURE) (vb2::start_streaming). > > I tried to set the coded resolution to the firmware as-is it set by > gstreamer but then I cannot receive the "sufficient" event for VP8 and > VP9 codecs. So I return back to the solution with minimum resolution > above. > > I'm open for suggestions. > >>> > >>> I think you could still keep setting the minimum size and wait for the > >>> "insufficient" event. At the same time, you could speculatively > >>> advertise the expected "sufficient" size on the CAPTURE queue before > >>> the hardware signals those. Even if you mispredict them, you'll get > >>> the event, update the CAPTURE resolution and send the source change > >>> event to the application, which would then give you the correct > >>> buffers. Would that work for you? > >> > >> As I understand it this still would require event support, which gstreamer > >> doesn't have. > > > > I don't think it matches what I remember from the earlier discussion. > > As long as Gstreamer sets the visible resolution (from the container > > AFAIR) on OUTPUT, the driver would adjust it to something that is > > expected to be the right framebuffer resolution and so Gstreamer would > > be able to continue. Of course if the expected value doesn't match, it > > wouldn't work, but it's the same as currently for Coda AFAICT. > > > >> > >> I think it is OK to have REQBUFS sleep in this case. However, I would only > > > > Why REQBUFS? While that could possibly allow us to allocate the right > > buffers, Gstreamer wouldn't be able to know the right format, because > > it would query it before REQBUFS, wouldn't it? > > Oops, you are right. It's got to be in G_FMT(CAPTURE), but *only* if > nobody subscribed to the SOURCE_CHANGE event. > > > > > For this reason, s5p-mfc makes G_FMT(CAPTURE) blocking and if we > > decide to forcefully keep the compatibility, even with in drivers, we > > should probably do the same here. > > > >> enable this behavior if the application didn't subscribe to the > >> SOURCE_CHANGE > >> event. That's easy enough to check in the driver. And that means that if > >> the > >> application is well written, then the driver will behave in a completely > >> standard way that the compliance test can check. > > > > I guess one could have some helpers for this. They would listen to the > > source change events internally and block / wake-up appropriate ioctls >
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On 5/27/19 5:51 AM, Tomasz Figa wrote: > On Tue, May 21, 2019 at 9:27 PM Hans Verkuil wrote: >> >> On 5/21/19 11:09 AM, Tomasz Figa wrote: >>> Hi Stan, >>> >>> On Mon, May 20, 2019 at 11:47 PM Stanimir Varbanov >>> wrote: Hi Tomasz, On 4/24/19 3:39 PM, Tomasz Figa wrote: > On Wed, Apr 24, 2019 at 9:15 PM Stanimir Varbanov > wrote: >> >> Hi Hans, >> >> On 2/15/19 3:44 PM, Hans Verkuil wrote: >>> Hi Stanimir, >>> >>> I never paid much attention to this patch series since others were busy >>> discussing it and I had a lot of other things on my plate, but then I >>> heard >>> that this patch made G_FMT blocking. >> >> OK, another option could be to block REQBUF(CAPTURE) until event from hw >> is received that the stream is parsed and the resolution is correctly >> set by application. Just to note that I'd think to this like a temporal >> solution until gstreamer implements v4l events. >> >> Is that looks good to you? > > Hmm, I thought we concluded that gstreamer sets the width and height > in OUTPUT queue before querying the CAPTURE queue and so making the > driver calculate the CAPTURE format based on what's set on OUTPUT > would work fine. Did I miss something? Nobody is miss something. First some background about how Venus implements stateful codec API. The Venus firmware can generate two events "sufficient" and "insufficient" buffer requirements (this includes decoder output buffer size and internal/scratch buffer sizes). Presently I always set minimum possible decoder resolution no matter what the user said, and by that way I'm sure that "insufficient" event will always be triggered by the firmware (the other reason to take this path is because this is the least-common-divider for all supported Venus hw/fw versions thus common code in the driver). The reconfiguration (during codec Initialization sequence) is made from STREAMON(CAPTURE) context. Now, to make that re-configuration happen I need to wait for "insufficient" event from firmware in order to know the real coded resolution. In the case of gstreamer where v4l2_events support is missing I have to block (wait for firmware event) REQBUF(CAPTURE) (vb2::queue_setup) or STREAMON(CAPTURE) (vb2::start_streaming). I tried to set the coded resolution to the firmware as-is it set by gstreamer but then I cannot receive the "sufficient" event for VP8 and VP9 codecs. So I return back to the solution with minimum resolution above. I'm open for suggestions. >>> >>> I think you could still keep setting the minimum size and wait for the >>> "insufficient" event. At the same time, you could speculatively >>> advertise the expected "sufficient" size on the CAPTURE queue before >>> the hardware signals those. Even if you mispredict them, you'll get >>> the event, update the CAPTURE resolution and send the source change >>> event to the application, which would then give you the correct >>> buffers. Would that work for you? >> >> As I understand it this still would require event support, which gstreamer >> doesn't have. > > I don't think it matches what I remember from the earlier discussion. > As long as Gstreamer sets the visible resolution (from the container > AFAIR) on OUTPUT, the driver would adjust it to something that is > expected to be the right framebuffer resolution and so Gstreamer would > be able to continue. Of course if the expected value doesn't match, it > wouldn't work, but it's the same as currently for Coda AFAICT. > >> >> I think it is OK to have REQBUFS sleep in this case. However, I would only > > Why REQBUFS? While that could possibly allow us to allocate the right > buffers, Gstreamer wouldn't be able to know the right format, because > it would query it before REQBUFS, wouldn't it? Oops, you are right. It's got to be in G_FMT(CAPTURE), but *only* if nobody subscribed to the SOURCE_CHANGE event. > > For this reason, s5p-mfc makes G_FMT(CAPTURE) blocking and if we > decide to forcefully keep the compatibility, even with in drivers, we > should probably do the same here. > >> enable this behavior if the application didn't subscribe to the SOURCE_CHANGE >> event. That's easy enough to check in the driver. And that means that if the >> application is well written, then the driver will behave in a completely >> standard way that the compliance test can check. > > I guess one could have some helpers for this. They would listen to the > source change events internally and block / wake-up appropriate ioctls > whenever necessary. I really do not want this for new drivers. gstreamer should be fixed. A blocking G_FMT is just plain bad. Only those drivers that do this, can still block if nobody subscribed to EVENT_SOURCE_CHANGE. > Another question: If we intend this to be implemented in new drivers
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On Tue, May 21, 2019 at 9:27 PM Hans Verkuil wrote: > > On 5/21/19 11:09 AM, Tomasz Figa wrote: > > Hi Stan, > > > > On Mon, May 20, 2019 at 11:47 PM Stanimir Varbanov > > wrote: > >> > >> Hi Tomasz, > >> > >> On 4/24/19 3:39 PM, Tomasz Figa wrote: > >>> On Wed, Apr 24, 2019 at 9:15 PM Stanimir Varbanov > >>> wrote: > > Hi Hans, > > On 2/15/19 3:44 PM, Hans Verkuil wrote: > > Hi Stanimir, > > > > I never paid much attention to this patch series since others were busy > > discussing it and I had a lot of other things on my plate, but then I > > heard > > that this patch made G_FMT blocking. > > OK, another option could be to block REQBUF(CAPTURE) until event from hw > is received that the stream is parsed and the resolution is correctly > set by application. Just to note that I'd think to this like a temporal > solution until gstreamer implements v4l events. > > Is that looks good to you? > >>> > >>> Hmm, I thought we concluded that gstreamer sets the width and height > >>> in OUTPUT queue before querying the CAPTURE queue and so making the > >>> driver calculate the CAPTURE format based on what's set on OUTPUT > >>> would work fine. Did I miss something? > >> > >> Nobody is miss something. > >> > >> First some background about how Venus implements stateful codec API. > >> > >> The Venus firmware can generate two events "sufficient" and > >> "insufficient" buffer requirements (this includes decoder output buffer > >> size and internal/scratch buffer sizes). Presently I always set minimum > >> possible decoder resolution no matter what the user said, and by that > >> way I'm sure that "insufficient" event will always be triggered by the > >> firmware (the other reason to take this path is because this is the > >> least-common-divider for all supported Venus hw/fw versions thus common > >> code in the driver). The reconfiguration (during codec Initialization > >> sequence) is made from STREAMON(CAPTURE) context. Now, to make that > >> re-configuration happen I need to wait for "insufficient" event from > >> firmware in order to know the real coded resolution. > >> > >> In the case of gstreamer where v4l2_events support is missing I have to > >> block (wait for firmware event) REQBUF(CAPTURE) (vb2::queue_setup) or > >> STREAMON(CAPTURE) (vb2::start_streaming). > >> > >> I tried to set the coded resolution to the firmware as-is it set by > >> gstreamer but then I cannot receive the "sufficient" event for VP8 and > >> VP9 codecs. So I return back to the solution with minimum resolution above. > >> > >> I'm open for suggestions. > > > > I think you could still keep setting the minimum size and wait for the > > "insufficient" event. At the same time, you could speculatively > > advertise the expected "sufficient" size on the CAPTURE queue before > > the hardware signals those. Even if you mispredict them, you'll get > > the event, update the CAPTURE resolution and send the source change > > event to the application, which would then give you the correct > > buffers. Would that work for you? > > As I understand it this still would require event support, which gstreamer > doesn't have. I don't think it matches what I remember from the earlier discussion. As long as Gstreamer sets the visible resolution (from the container AFAIR) on OUTPUT, the driver would adjust it to something that is expected to be the right framebuffer resolution and so Gstreamer would be able to continue. Of course if the expected value doesn't match, it wouldn't work, but it's the same as currently for Coda AFAICT. > > I think it is OK to have REQBUFS sleep in this case. However, I would only Why REQBUFS? While that could possibly allow us to allocate the right buffers, Gstreamer wouldn't be able to know the right format, because it would query it before REQBUFS, wouldn't it? For this reason, s5p-mfc makes G_FMT(CAPTURE) blocking and if we decide to forcefully keep the compatibility, even with in drivers, we should probably do the same here. > enable this behavior if the application didn't subscribe to the SOURCE_CHANGE > event. That's easy enough to check in the driver. And that means that if the > application is well written, then the driver will behave in a completely > standard way that the compliance test can check. I guess one could have some helpers for this. They would listen to the source change events internally and block / wake-up appropriate ioctls whenever necessary. Another question: If we intend this to be implemented in new drivers too, should it be documented in the spec? Best regards, Tomasz
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On 5/21/19 11:09 AM, Tomasz Figa wrote: > Hi Stan, > > On Mon, May 20, 2019 at 11:47 PM Stanimir Varbanov > wrote: >> >> Hi Tomasz, >> >> On 4/24/19 3:39 PM, Tomasz Figa wrote: >>> On Wed, Apr 24, 2019 at 9:15 PM Stanimir Varbanov >>> wrote: Hi Hans, On 2/15/19 3:44 PM, Hans Verkuil wrote: > Hi Stanimir, > > I never paid much attention to this patch series since others were busy > discussing it and I had a lot of other things on my plate, but then I > heard > that this patch made G_FMT blocking. OK, another option could be to block REQBUF(CAPTURE) until event from hw is received that the stream is parsed and the resolution is correctly set by application. Just to note that I'd think to this like a temporal solution until gstreamer implements v4l events. Is that looks good to you? >>> >>> Hmm, I thought we concluded that gstreamer sets the width and height >>> in OUTPUT queue before querying the CAPTURE queue and so making the >>> driver calculate the CAPTURE format based on what's set on OUTPUT >>> would work fine. Did I miss something? >> >> Nobody is miss something. >> >> First some background about how Venus implements stateful codec API. >> >> The Venus firmware can generate two events "sufficient" and >> "insufficient" buffer requirements (this includes decoder output buffer >> size and internal/scratch buffer sizes). Presently I always set minimum >> possible decoder resolution no matter what the user said, and by that >> way I'm sure that "insufficient" event will always be triggered by the >> firmware (the other reason to take this path is because this is the >> least-common-divider for all supported Venus hw/fw versions thus common >> code in the driver). The reconfiguration (during codec Initialization >> sequence) is made from STREAMON(CAPTURE) context. Now, to make that >> re-configuration happen I need to wait for "insufficient" event from >> firmware in order to know the real coded resolution. >> >> In the case of gstreamer where v4l2_events support is missing I have to >> block (wait for firmware event) REQBUF(CAPTURE) (vb2::queue_setup) or >> STREAMON(CAPTURE) (vb2::start_streaming). >> >> I tried to set the coded resolution to the firmware as-is it set by >> gstreamer but then I cannot receive the "sufficient" event for VP8 and >> VP9 codecs. So I return back to the solution with minimum resolution above. >> >> I'm open for suggestions. > > I think you could still keep setting the minimum size and wait for the > "insufficient" event. At the same time, you could speculatively > advertise the expected "sufficient" size on the CAPTURE queue before > the hardware signals those. Even if you mispredict them, you'll get > the event, update the CAPTURE resolution and send the source change > event to the application, which would then give you the correct > buffers. Would that work for you? As I understand it this still would require event support, which gstreamer doesn't have. I think it is OK to have REQBUFS sleep in this case. However, I would only enable this behavior if the application didn't subscribe to the SOURCE_CHANGE event. That's easy enough to check in the driver. And that means that if the application is well written, then the driver will behave in a completely standard way that the compliance test can check. Regards, Hans
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
Hi Stan, On Mon, May 20, 2019 at 11:47 PM Stanimir Varbanov wrote: > > Hi Tomasz, > > On 4/24/19 3:39 PM, Tomasz Figa wrote: > > On Wed, Apr 24, 2019 at 9:15 PM Stanimir Varbanov > > wrote: > >> > >> Hi Hans, > >> > >> On 2/15/19 3:44 PM, Hans Verkuil wrote: > >>> Hi Stanimir, > >>> > >>> I never paid much attention to this patch series since others were busy > >>> discussing it and I had a lot of other things on my plate, but then I > >>> heard > >>> that this patch made G_FMT blocking. > >> > >> OK, another option could be to block REQBUF(CAPTURE) until event from hw > >> is received that the stream is parsed and the resolution is correctly > >> set by application. Just to note that I'd think to this like a temporal > >> solution until gstreamer implements v4l events. > >> > >> Is that looks good to you? > > > > Hmm, I thought we concluded that gstreamer sets the width and height > > in OUTPUT queue before querying the CAPTURE queue and so making the > > driver calculate the CAPTURE format based on what's set on OUTPUT > > would work fine. Did I miss something? > > Nobody is miss something. > > First some background about how Venus implements stateful codec API. > > The Venus firmware can generate two events "sufficient" and > "insufficient" buffer requirements (this includes decoder output buffer > size and internal/scratch buffer sizes). Presently I always set minimum > possible decoder resolution no matter what the user said, and by that > way I'm sure that "insufficient" event will always be triggered by the > firmware (the other reason to take this path is because this is the > least-common-divider for all supported Venus hw/fw versions thus common > code in the driver). The reconfiguration (during codec Initialization > sequence) is made from STREAMON(CAPTURE) context. Now, to make that > re-configuration happen I need to wait for "insufficient" event from > firmware in order to know the real coded resolution. > > In the case of gstreamer where v4l2_events support is missing I have to > block (wait for firmware event) REQBUF(CAPTURE) (vb2::queue_setup) or > STREAMON(CAPTURE) (vb2::start_streaming). > > I tried to set the coded resolution to the firmware as-is it set by > gstreamer but then I cannot receive the "sufficient" event for VP8 and > VP9 codecs. So I return back to the solution with minimum resolution above. > > I'm open for suggestions. I think you could still keep setting the minimum size and wait for the "insufficient" event. At the same time, you could speculatively advertise the expected "sufficient" size on the CAPTURE queue before the hardware signals those. Even if you mispredict them, you'll get the event, update the CAPTURE resolution and send the source change event to the application, which would then give you the correct buffers. Would that work for you? Best regards, Tomasz
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
Hi Tomasz, On 4/24/19 3:39 PM, Tomasz Figa wrote: > On Wed, Apr 24, 2019 at 9:15 PM Stanimir Varbanov > wrote: >> >> Hi Hans, >> >> On 2/15/19 3:44 PM, Hans Verkuil wrote: >>> Hi Stanimir, >>> >>> I never paid much attention to this patch series since others were busy >>> discussing it and I had a lot of other things on my plate, but then I heard >>> that this patch made G_FMT blocking. >> >> OK, another option could be to block REQBUF(CAPTURE) until event from hw >> is received that the stream is parsed and the resolution is correctly >> set by application. Just to note that I'd think to this like a temporal >> solution until gstreamer implements v4l events. >> >> Is that looks good to you? > > Hmm, I thought we concluded that gstreamer sets the width and height > in OUTPUT queue before querying the CAPTURE queue and so making the > driver calculate the CAPTURE format based on what's set on OUTPUT > would work fine. Did I miss something? Nobody is miss something. First some background about how Venus implements stateful codec API. The Venus firmware can generate two events "sufficient" and "insufficient" buffer requirements (this includes decoder output buffer size and internal/scratch buffer sizes). Presently I always set minimum possible decoder resolution no matter what the user said, and by that way I'm sure that "insufficient" event will always be triggered by the firmware (the other reason to take this path is because this is the least-common-divider for all supported Venus hw/fw versions thus common code in the driver). The reconfiguration (during codec Initialization sequence) is made from STREAMON(CAPTURE) context. Now, to make that re-configuration happen I need to wait for "insufficient" event from firmware in order to know the real coded resolution. In the case of gstreamer where v4l2_events support is missing I have to block (wait for firmware event) REQBUF(CAPTURE) (vb2::queue_setup) or STREAMON(CAPTURE) (vb2::start_streaming). I tried to set the coded resolution to the firmware as-is it set by gstreamer but then I cannot receive the "sufficient" event for VP8 and VP9 codecs. So I return back to the solution with minimum resolution above. I'm open for suggestions. -- regards, Stan
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On Wed, Apr 10, 2019 at 1:31 AM Nicolas Dufresne wrote: > > Le mardi 09 avril 2019 à 18:59 +0900, Tomasz Figa a écrit : > > On Thu, Feb 7, 2019 at 4:33 PM Tomasz Figa wrote: > > > On Tue, Feb 5, 2019 at 7:35 PM Hans Verkuil wrote: > > > > On 2/5/19 10:31 AM, Tomasz Figa wrote: > > > > > On Tue, Feb 5, 2019 at 6:00 PM Hans Verkuil > > > > > wrote: > > > > > > On 2/5/19 7:26 AM, Tomasz Figa wrote: > > > > > > > On Fri, Feb 1, 2019 at 12:18 AM Nicolas Dufresne > > > > > > > wrote: > > > > > > > > Le jeudi 31 janvier 2019 à 22:34 +0900, Tomasz Figa a écrit : > > > > > > > > > On Thu, Jan 31, 2019 at 9:42 PM Philipp Zabel > > > > > > > > > wrote: > > > > > > > > > > Hi Nicolas, > > > > > > > > > > > > > > > > > > > > On Wed, 2019-01-30 at 10:32 -0500, Nicolas Dufresne wrote: > > > > > > > > > > > Le mercredi 30 janvier 2019 à 15:17 +0900, Tomasz Figa a > > > > > > > > > > > écrit : > > > > > > > > > > > > > I don't remember saying that, maybe I meant to say > > > > > > > > > > > > > there might be a > > > > > > > > > > > > > workaround ? > > > > > > > > > > > > > > > > > > > > > > > > > > For the fact, here we queue the headers (or first > > > > > > > > > > > > > frame): > > > > > > > > > > > > > > > > > > > > > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L624 > > > > > > > > > > > > > > > > > > > > > > > > > > Then few line below this helper does G_FMT internally: > > > > > > > > > > > > > > > > > > > > > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L634 > > > > > > > > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L3907 > > > > > > > > > > > > > > > > > > > > > > > > > > And just plainly fails if G_FMT returns an error of > > > > > > > > > > > > > any type. This was > > > > > > > > > > > > > how Kamil designed it initially for MFC driver. There > > > > > > > > > > > > > was no other > > > > > > > > > > > > > alternative back then (no EAGAIN yet either). > > > > > > > > > > > > > > > > > > > > > > > > Hmm, was that ffmpeg then? > > > > > > > > > > > > > > > > > > > > > > > > So would it just set the OUTPUT width and height to 0? > > > > > > > > > > > > Does it mean > > > > > > > > > > > > that gstreamer doesn't work with coda and mtk-vcodec, > > > > > > > > > > > > which don't have > > > > > > > > > > > > such wait in their g_fmt implementations? > > > > > > > > > > > > > > > > > > > > > > I don't know for MTK, I don't have the hardware and > > > > > > > > > > > didn't integrate > > > > > > > > > > > their vendor pixel format. For the CODA, I know it works, > > > > > > > > > > > if there is > > > > > > > > > > > no wait in the G_FMT, then I suppose we are being really > > > > > > > > > > > lucky with the > > > > > > > > > > > timing (it would be that the drivers process the SPS/PPS > > > > > > > > > > > synchronously, > > > > > > > > > > > and a simple lock in the G_FMT call is enough to wait). > > > > > > > > > > > Adding Philipp > > > > > > > > > > > in CC, he could explain how this works, I know they use > > > > > > > > > > > GStreamer in > > > > > > > > > > > production, and he would have fixed GStreamer already if > > > > > > > > > > > that was > > > > > > > > > > > causing important issue. > > > > > > > > > > > > > > > > > > > > CODA predates the width/height=0 rule on the coded/OUTPUT > > > > > > > > > > queue. > > > > > > > > > > It currently behaves more like a traditional mem2mem device. > > > > > > > > > > > > > > > > > > The rule in the latest spec is that if width/height is 0 then > > > > > > > > > CAPTURE > > > > > > > > > format is determined only after the stream is parsed. > > > > > > > > > Otherwise it's > > > > > > > > > instantly deduced from the OUTPUT resolution. > > > > > > > > > > > > > > > > > > > When width/height is set via S_FMT(OUT) or output crop > > > > > > > > > > selection, the > > > > > > > > > > driver will believe it and set the same (rounded up to > > > > > > > > > > macroblock > > > > > > > > > > alignment) on the capture queue without ever having seen > > > > > > > > > > the SPS. > > > > > > > > > > > > > > > > > > That's why I asked whether gstreamer sets width and height of > > > > > > > > > OUTPUT > > > > > > > > > to non-zero values. If so, there is no regression, as the > > > > > > > > > specs mimic > > > > > > > > > the coda behavior. > > > > > > > > > > > > > > > > I see, with Philipp's answer it explains why it works. Note that > > > > > > > > GStreamer sets the display size on the OUTPUT format (in fact > > > > > > > > we pass > > > > > > > > as much information as we have, because a) it's generic code > > > > > > > > and b) it > > > > > > > > will be needed someday when we enable pre-allocation (REQBUFS > > > > > > > > before > > > > > > > > SPS/PPS is passed, to avoid the setup delay introduce by > > > > > > > > allocation, > > > > > > > >
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On 2/15/19 5:27 PM, Nicolas Dufresne wrote:
> Le vendredi 15 février 2019 à 14:44 +0100, Hans Verkuil a écrit :
>> Hi Stanimir,
>>
>> I never paid much attention to this patch series since others were busy
>> discussing it and I had a lot of other things on my plate, but then I heard
>> that this patch made G_FMT blocking.
>>
>> That's a no-go. Apparently s5p-mfc does that as well, and that's against
>> the V4L2 spec as well (clearly I never realized that at the time).
>>
>> So, just to make it unambiguous:
>>
>> Nacked-by: Hans Verkuil
>
> Careful if you pull out this code from S5P MFC though, since you'll
> break userspace and we don't like when kernel do that. As we discussed,
> Philipp in CODA came up with a clever workaround, so if one have the
> idea of touching MFC, please make sure to do so correctly.
I'm not pulling any code out of S5P MFC, I just want to prevent propagating
a bad idea into other drivers.
Regards,
Hans
>
>>
>> Now, I plan to work on adding stateful codec checks to v4l2-compliance.
>> I had hoped to do some of that already, but didn't manage to find time
>> this week. I hope to have something decent in 2-3 weeks from now.
>>
>> So I would wait with making this driver compliant until I have written
>> the tests.
>>
>> Initially I'll test this with vicodec, but the next phase would be to
>> make it work with e.g. a venus codec. I might contact you for help
>> when I start work on that.
>>
>> Regards,
>>
>> Hans
>>
>> On 1/17/19 5:20 PM, Stanimir Varbanov wrote:
>>> This refactored code for start/stop streaming vb2 operations and
>>> adds a state machine handling similar to the one in stateful codec
>>> API documentation. One major change is that now the HFI session is
>>> started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0),
>>> during that time streamoff(cap,out) just flush buffers but doesn't
>>> stop the session. The other major change is that now the capture
>>> and output queues are completely separated.
>>>
>>> Signed-off-by: Stanimir Varbanov
>>> ---
>>> drivers/media/platform/qcom/venus/core.h| 20 +-
>>> drivers/media/platform/qcom/venus/helpers.c | 23 +-
>>> drivers/media/platform/qcom/venus/helpers.h | 5 +
>>> drivers/media/platform/qcom/venus/vdec.c| 449
>>> 4 files changed, 389 insertions(+), 108 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/core.h
>>> b/drivers/media/platform/qcom/venus/core.h
>>> index 79c7e816c706..5a133c203455 100644
>>> --- a/drivers/media/platform/qcom/venus/core.h
>>> +++ b/drivers/media/platform/qcom/venus/core.h
>>> @@ -218,6 +218,15 @@ struct venus_buffer {
>>>
>>> #define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer,
>>> vb)
>>>
>>> +#define DEC_STATE_UNINIT 0
>>> +#define DEC_STATE_INIT 1
>>> +#define DEC_STATE_CAPTURE_SETUP2
>>> +#define DEC_STATE_STOPPED 3
>>> +#define DEC_STATE_SEEK 4
>>> +#define DEC_STATE_DRAIN5
>>> +#define DEC_STATE_DECODING 6
>>> +#define DEC_STATE_DRC 7
>>> +
>>> /**
>>> * struct venus_inst - holds per instance paramerters
>>> *
>>> @@ -241,6 +250,10 @@ struct venus_buffer {
>>> * @colorspace:current color space
>>> * @quantization: current quantization
>>> * @xfer_func: current xfer function
>>> + * @codec_state: current codec API state (see DEC/ENC_STATE_)
>>> + * @reconf_wait: wait queue for resolution change event
>>> + * @ten_bits: does new stream is 10bits depth
>>> + * @buf_count: used to count number number of buffers
>>> (reqbuf(0))
>>> * @fps: holds current FPS
>>> * @timeperframe: holds current time per frame structure
>>> * @fmt_out: a reference to output format structure
>>> @@ -255,8 +268,6 @@ struct venus_buffer {
>>> * @opb_buftype: output picture buffer type
>>> * @opb_fmt: output picture buffer raw format
>>> * @reconfig: a flag raised by decoder when the stream resolution
>>> changed
>>> - * @reconfig_width:holds the new width
>>> - * @reconfig_height: holds the new height
>>> * @hfi_codec: current codec for this instance in HFI space
>>> * @sequence_cap: a sequence counter for capture queue
>>> * @sequence_out: a sequence counter for output queue
>>> @@ -296,6 +307,9 @@ struct venus_inst {
>>> u8 ycbcr_enc;
>>> u8 quantization;
>>> u8 xfer_func;
>>> + unsigned int codec_state;
>>> + wait_queue_head_t reconf_wait;
>>> + int buf_count;
>>> u64 fps;
>>> struct v4l2_fract timeperframe;
>>> const struct venus_format *fmt_out;
>>> @@ -310,8 +324,6 @@ struct venus_inst {
>>> u32 opb_buftype;
>>> u32 opb_fmt;
>>> bool reconfig;
>>> - u32 reconfig_width;
>>> - u32 reconfig_height;
>>> u32 hfi_codec;
>>> u32 sequence_cap;
>>> u32 sequence_out;
>>> diff --git
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
Le vendredi 15 février 2019 à 14:44 +0100, Hans Verkuil a écrit :
> Hi Stanimir,
>
> I never paid much attention to this patch series since others were busy
> discussing it and I had a lot of other things on my plate, but then I heard
> that this patch made G_FMT blocking.
>
> That's a no-go. Apparently s5p-mfc does that as well, and that's against
> the V4L2 spec as well (clearly I never realized that at the time).
>
> So, just to make it unambiguous:
>
> Nacked-by: Hans Verkuil
Careful if you pull out this code from S5P MFC though, since you'll
break userspace and we don't like when kernel do that. As we discussed,
Philipp in CODA came up with a clever workaround, so if one have the
idea of touching MFC, please make sure to do so correctly.
>
> Now, I plan to work on adding stateful codec checks to v4l2-compliance.
> I had hoped to do some of that already, but didn't manage to find time
> this week. I hope to have something decent in 2-3 weeks from now.
>
> So I would wait with making this driver compliant until I have written
> the tests.
>
> Initially I'll test this with vicodec, but the next phase would be to
> make it work with e.g. a venus codec. I might contact you for help
> when I start work on that.
>
> Regards,
>
> Hans
>
> On 1/17/19 5:20 PM, Stanimir Varbanov wrote:
> > This refactored code for start/stop streaming vb2 operations and
> > adds a state machine handling similar to the one in stateful codec
> > API documentation. One major change is that now the HFI session is
> > started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0),
> > during that time streamoff(cap,out) just flush buffers but doesn't
> > stop the session. The other major change is that now the capture
> > and output queues are completely separated.
> >
> > Signed-off-by: Stanimir Varbanov
> > ---
> > drivers/media/platform/qcom/venus/core.h| 20 +-
> > drivers/media/platform/qcom/venus/helpers.c | 23 +-
> > drivers/media/platform/qcom/venus/helpers.h | 5 +
> > drivers/media/platform/qcom/venus/vdec.c| 449
> > 4 files changed, 389 insertions(+), 108 deletions(-)
> >
> > diff --git a/drivers/media/platform/qcom/venus/core.h
> > b/drivers/media/platform/qcom/venus/core.h
> > index 79c7e816c706..5a133c203455 100644
> > --- a/drivers/media/platform/qcom/venus/core.h
> > +++ b/drivers/media/platform/qcom/venus/core.h
> > @@ -218,6 +218,15 @@ struct venus_buffer {
> >
> > #define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer,
> > vb)
> >
> > +#define DEC_STATE_UNINIT 0
> > +#define DEC_STATE_INIT 1
> > +#define DEC_STATE_CAPTURE_SETUP2
> > +#define DEC_STATE_STOPPED 3
> > +#define DEC_STATE_SEEK 4
> > +#define DEC_STATE_DRAIN5
> > +#define DEC_STATE_DECODING 6
> > +#define DEC_STATE_DRC 7
> > +
> > /**
> > * struct venus_inst - holds per instance paramerters
> > *
> > @@ -241,6 +250,10 @@ struct venus_buffer {
> > * @colorspace:current color space
> > * @quantization: current quantization
> > * @xfer_func: current xfer function
> > + * @codec_state: current codec API state (see DEC/ENC_STATE_)
> > + * @reconf_wait: wait queue for resolution change event
> > + * @ten_bits: does new stream is 10bits depth
> > + * @buf_count: used to count number number of buffers
> > (reqbuf(0))
> > * @fps: holds current FPS
> > * @timeperframe: holds current time per frame structure
> > * @fmt_out: a reference to output format structure
> > @@ -255,8 +268,6 @@ struct venus_buffer {
> > * @opb_buftype: output picture buffer type
> > * @opb_fmt: output picture buffer raw format
> > * @reconfig: a flag raised by decoder when the stream resolution
> > changed
> > - * @reconfig_width:holds the new width
> > - * @reconfig_height: holds the new height
> > * @hfi_codec: current codec for this instance in HFI space
> > * @sequence_cap: a sequence counter for capture queue
> > * @sequence_out: a sequence counter for output queue
> > @@ -296,6 +307,9 @@ struct venus_inst {
> > u8 ycbcr_enc;
> > u8 quantization;
> > u8 xfer_func;
> > + unsigned int codec_state;
> > + wait_queue_head_t reconf_wait;
> > + int buf_count;
> > u64 fps;
> > struct v4l2_fract timeperframe;
> > const struct venus_format *fmt_out;
> > @@ -310,8 +324,6 @@ struct venus_inst {
> > u32 opb_buftype;
> > u32 opb_fmt;
> > bool reconfig;
> > - u32 reconfig_width;
> > - u32 reconfig_height;
> > u32 hfi_codec;
> > u32 sequence_cap;
> > u32 sequence_out;
> > diff --git a/drivers/media/platform/qcom/venus/helpers.c
> > b/drivers/media/platform/qcom/venus/helpers.c
> > index 637ce7b82d94..25d8cceccae4 100644
> > --- a/drivers/media/platform/qcom/venus/helpers.c
> > +++
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
Hi Stanimir,
I never paid much attention to this patch series since others were busy
discussing it and I had a lot of other things on my plate, but then I heard
that this patch made G_FMT blocking.
That's a no-go. Apparently s5p-mfc does that as well, and that's against
the V4L2 spec as well (clearly I never realized that at the time).
So, just to make it unambiguous:
Nacked-by: Hans Verkuil
Now, I plan to work on adding stateful codec checks to v4l2-compliance.
I had hoped to do some of that already, but didn't manage to find time
this week. I hope to have something decent in 2-3 weeks from now.
So I would wait with making this driver compliant until I have written
the tests.
Initially I'll test this with vicodec, but the next phase would be to
make it work with e.g. a venus codec. I might contact you for help
when I start work on that.
Regards,
Hans
On 1/17/19 5:20 PM, Stanimir Varbanov wrote:
> This refactored code for start/stop streaming vb2 operations and
> adds a state machine handling similar to the one in stateful codec
> API documentation. One major change is that now the HFI session is
> started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0),
> during that time streamoff(cap,out) just flush buffers but doesn't
> stop the session. The other major change is that now the capture
> and output queues are completely separated.
>
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/core.h| 20 +-
> drivers/media/platform/qcom/venus/helpers.c | 23 +-
> drivers/media/platform/qcom/venus/helpers.h | 5 +
> drivers/media/platform/qcom/venus/vdec.c| 449
> 4 files changed, 389 insertions(+), 108 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h
> b/drivers/media/platform/qcom/venus/core.h
> index 79c7e816c706..5a133c203455 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -218,6 +218,15 @@ struct venus_buffer {
>
> #define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer, vb)
>
> +#define DEC_STATE_UNINIT 0
> +#define DEC_STATE_INIT 1
> +#define DEC_STATE_CAPTURE_SETUP 2
> +#define DEC_STATE_STOPPED3
> +#define DEC_STATE_SEEK 4
> +#define DEC_STATE_DRAIN 5
> +#define DEC_STATE_DECODING 6
> +#define DEC_STATE_DRC7
> +
> /**
> * struct venus_inst - holds per instance paramerters
> *
> @@ -241,6 +250,10 @@ struct venus_buffer {
> * @colorspace: current color space
> * @quantization:current quantization
> * @xfer_func: current xfer function
> + * @codec_state: current codec API state (see DEC/ENC_STATE_)
> + * @reconf_wait: wait queue for resolution change event
> + * @ten_bits:does new stream is 10bits depth
> + * @buf_count: used to count number number of buffers
> (reqbuf(0))
> * @fps: holds current FPS
> * @timeperframe:holds current time per frame structure
> * @fmt_out: a reference to output format structure
> @@ -255,8 +268,6 @@ struct venus_buffer {
> * @opb_buftype: output picture buffer type
> * @opb_fmt: output picture buffer raw format
> * @reconfig:a flag raised by decoder when the stream resolution
> changed
> - * @reconfig_width: holds the new width
> - * @reconfig_height: holds the new height
> * @hfi_codec: current codec for this instance in HFI space
> * @sequence_cap:a sequence counter for capture queue
> * @sequence_out:a sequence counter for output queue
> @@ -296,6 +307,9 @@ struct venus_inst {
> u8 ycbcr_enc;
> u8 quantization;
> u8 xfer_func;
> + unsigned int codec_state;
> + wait_queue_head_t reconf_wait;
> + int buf_count;
> u64 fps;
> struct v4l2_fract timeperframe;
> const struct venus_format *fmt_out;
> @@ -310,8 +324,6 @@ struct venus_inst {
> u32 opb_buftype;
> u32 opb_fmt;
> bool reconfig;
> - u32 reconfig_width;
> - u32 reconfig_height;
> u32 hfi_codec;
> u32 sequence_cap;
> u32 sequence_out;
> diff --git a/drivers/media/platform/qcom/venus/helpers.c
> b/drivers/media/platform/qcom/venus/helpers.c
> index 637ce7b82d94..25d8cceccae4 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb)
>
> v4l2_m2m_buf_queue(m2m_ctx, vbuf);
>
> - if (!(inst->streamon_out & inst->streamon_cap))
> - goto unlock;
> -
> - ret = is_buf_refed(inst, vbuf);
> - if (ret)
> - goto unlock;
> + if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) {
> + ret = is_buf_refed(inst, vbuf);
> + if (ret)
> + goto unlock;
>
>
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
Le jeudi 14 février 2019 à 11:43 +0900, Tomasz Figa a écrit : > > > > No, I exactly meant the OUTPUT queue. The behavior of s5p-mfc in case > > > > of the format not being detected yet is to waits for any pending > > > > bitstream buffers to be processed by the decoder before returning an > > > > error. > > > > > > > > See > > > > https://elixir.bootlin.com/linux/v5.0-rc5/source/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c#L329 > > > > > > It blocks?! That shouldn't happen. Totally against the spec. > > > > > > > Yeah and that's what this patch tries to implement in venus as well > > and is seemingly required for compatibility with gstreamer... > > > > Hans, Nicolas, any thoughts? Thinking about this, if CODA managed to make it work without this, and without the source change event, we should probably study more this code and propose to do this instead of blocking (which is the ugly but easy solution). I'm sure it was a bit of juggle to pass the information correctly from input to output, but that would bring "compatibility" with un-ported userspace. If we had a codec specific framework (making a wish here), we could handle that for the codec, a bit like the code that emulates CROP on top of G/S_SELECTION. Meanwhile, I'm trying to get some time allocated to implement the event in GStreamer, as this would be sufficient argument to say that newly introduce drivers don't need to care, you just need newer userspace. For Venus it's a bit difficult, since they have customers using GStreamer already, and it's quite common to run newer kernel with much older userspace (and expect the userspace to keep working). Nicolas
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On Thu, Feb 7, 2019 at 4:33 PM Tomasz Figa wrote: > > On Tue, Feb 5, 2019 at 7:35 PM Hans Verkuil wrote: > > > > On 2/5/19 10:31 AM, Tomasz Figa wrote: > > > On Tue, Feb 5, 2019 at 6:00 PM Hans Verkuil wrote: > > >> > > >> On 2/5/19 7:26 AM, Tomasz Figa wrote: > > >>> On Fri, Feb 1, 2019 at 12:18 AM Nicolas Dufresne > > >>> wrote: > > > > Le jeudi 31 janvier 2019 à 22:34 +0900, Tomasz Figa a écrit : > > > On Thu, Jan 31, 2019 at 9:42 PM Philipp Zabel > > > wrote: > > >> Hi Nicolas, > > >> > > >> On Wed, 2019-01-30 at 10:32 -0500, Nicolas Dufresne wrote: > > >>> Le mercredi 30 janvier 2019 à 15:17 +0900, Tomasz Figa a écrit : > > > I don't remember saying that, maybe I meant to say there might be > > > a > > > workaround ? > > > > > > For the fact, here we queue the headers (or first frame): > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L624 > > > > > > Then few line below this helper does G_FMT internally: > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L634 > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L3907 > > > > > > And just plainly fails if G_FMT returns an error of any type. > > > This was > > > how Kamil designed it initially for MFC driver. There was no other > > > alternative back then (no EAGAIN yet either). > > > > Hmm, was that ffmpeg then? > > > > So would it just set the OUTPUT width and height to 0? Does it mean > > that gstreamer doesn't work with coda and mtk-vcodec, which don't > > have > > such wait in their g_fmt implementations? > > >>> > > >>> I don't know for MTK, I don't have the hardware and didn't integrate > > >>> their vendor pixel format. For the CODA, I know it works, if there > > >>> is > > >>> no wait in the G_FMT, then I suppose we are being really lucky with > > >>> the > > >>> timing (it would be that the drivers process the SPS/PPS > > >>> synchronously, > > >>> and a simple lock in the G_FMT call is enough to wait). Adding > > >>> Philipp > > >>> in CC, he could explain how this works, I know they use GStreamer in > > >>> production, and he would have fixed GStreamer already if that was > > >>> causing important issue. > > >> > > >> CODA predates the width/height=0 rule on the coded/OUTPUT queue. > > >> It currently behaves more like a traditional mem2mem device. > > > > > > The rule in the latest spec is that if width/height is 0 then CAPTURE > > > format is determined only after the stream is parsed. Otherwise it's > > > instantly deduced from the OUTPUT resolution. > > > > > >> When width/height is set via S_FMT(OUT) or output crop selection, the > > >> driver will believe it and set the same (rounded up to macroblock > > >> alignment) on the capture queue without ever having seen the SPS. > > > > > > That's why I asked whether gstreamer sets width and height of OUTPUT > > > to non-zero values. If so, there is no regression, as the specs mimic > > > the coda behavior. > > > > I see, with Philipp's answer it explains why it works. Note that > > GStreamer sets the display size on the OUTPUT format (in fact we pass > > as much information as we have, because a) it's generic code and b) it > > will be needed someday when we enable pre-allocation (REQBUFS before > > SPS/PPS is passed, to avoid the setup delay introduce by allocation, > > mostly seen with CMA base decoder). In any case, the driver reported > > display size should always be ignored in GStreamer, the only > > information we look at is the G_SELECTION for the case the x/y or the > > cropping rectangle is non-zero. > > > > Note this can only work if the capture queue is not affected by the > > coded size, or if the round-up made by the driver is bigger or equal to > > that coded size. I believe CODA falls into the first category, since > > the decoding happens in a separate set of buffers and are then de-tiled > > into the capture buffers (if understood correctly). > > >>> > > >>> Sounds like it would work only if coded size is equal to the visible > > >>> size (that GStreamer sets) rounded up to full macroblocks. Non-zero x > > >>> or y in the crop could be problematic too. > > >>> > > >>> Hans, what's your view on this? Should we require G_FMT(CAPTURE) to > > >>> wait until a format becomes available or the OUTPUT queue runs out of > > >> > > >> You mean CAPTURE queue? If not, then I don't understand that part. > > > > > > No, I exactly meant the OUTPUT queue. The behavior of s5p-mfc in case > > > of
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On Tue, Feb 5, 2019 at 7:35 PM Hans Verkuil wrote: > > On 2/5/19 10:31 AM, Tomasz Figa wrote: > > On Tue, Feb 5, 2019 at 6:00 PM Hans Verkuil wrote: > >> > >> On 2/5/19 7:26 AM, Tomasz Figa wrote: > >>> On Fri, Feb 1, 2019 at 12:18 AM Nicolas Dufresne > >>> wrote: > > Le jeudi 31 janvier 2019 à 22:34 +0900, Tomasz Figa a écrit : > > On Thu, Jan 31, 2019 at 9:42 PM Philipp Zabel > > wrote: > >> Hi Nicolas, > >> > >> On Wed, 2019-01-30 at 10:32 -0500, Nicolas Dufresne wrote: > >>> Le mercredi 30 janvier 2019 à 15:17 +0900, Tomasz Figa a écrit : > > I don't remember saying that, maybe I meant to say there might be a > > workaround ? > > > > For the fact, here we queue the headers (or first frame): > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L624 > > > > Then few line below this helper does G_FMT internally: > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L634 > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L3907 > > > > And just plainly fails if G_FMT returns an error of any type. This > > was > > how Kamil designed it initially for MFC driver. There was no other > > alternative back then (no EAGAIN yet either). > > Hmm, was that ffmpeg then? > > So would it just set the OUTPUT width and height to 0? Does it mean > that gstreamer doesn't work with coda and mtk-vcodec, which don't > have > such wait in their g_fmt implementations? > >>> > >>> I don't know for MTK, I don't have the hardware and didn't integrate > >>> their vendor pixel format. For the CODA, I know it works, if there is > >>> no wait in the G_FMT, then I suppose we are being really lucky with > >>> the > >>> timing (it would be that the drivers process the SPS/PPS > >>> synchronously, > >>> and a simple lock in the G_FMT call is enough to wait). Adding Philipp > >>> in CC, he could explain how this works, I know they use GStreamer in > >>> production, and he would have fixed GStreamer already if that was > >>> causing important issue. > >> > >> CODA predates the width/height=0 rule on the coded/OUTPUT queue. > >> It currently behaves more like a traditional mem2mem device. > > > > The rule in the latest spec is that if width/height is 0 then CAPTURE > > format is determined only after the stream is parsed. Otherwise it's > > instantly deduced from the OUTPUT resolution. > > > >> When width/height is set via S_FMT(OUT) or output crop selection, the > >> driver will believe it and set the same (rounded up to macroblock > >> alignment) on the capture queue without ever having seen the SPS. > > > > That's why I asked whether gstreamer sets width and height of OUTPUT > > to non-zero values. If so, there is no regression, as the specs mimic > > the coda behavior. > > I see, with Philipp's answer it explains why it works. Note that > GStreamer sets the display size on the OUTPUT format (in fact we pass > as much information as we have, because a) it's generic code and b) it > will be needed someday when we enable pre-allocation (REQBUFS before > SPS/PPS is passed, to avoid the setup delay introduce by allocation, > mostly seen with CMA base decoder). In any case, the driver reported > display size should always be ignored in GStreamer, the only > information we look at is the G_SELECTION for the case the x/y or the > cropping rectangle is non-zero. > > Note this can only work if the capture queue is not affected by the > coded size, or if the round-up made by the driver is bigger or equal to > that coded size. I believe CODA falls into the first category, since > the decoding happens in a separate set of buffers and are then de-tiled > into the capture buffers (if understood correctly). > >>> > >>> Sounds like it would work only if coded size is equal to the visible > >>> size (that GStreamer sets) rounded up to full macroblocks. Non-zero x > >>> or y in the crop could be problematic too. > >>> > >>> Hans, what's your view on this? Should we require G_FMT(CAPTURE) to > >>> wait until a format becomes available or the OUTPUT queue runs out of > >> > >> You mean CAPTURE queue? If not, then I don't understand that part. > > > > No, I exactly meant the OUTPUT queue. The behavior of s5p-mfc in case > > of the format not being detected yet is to waits for any pending > > bitstream buffers to be processed by the decoder before returning an > > error. > > > > See > > https://elixir.bootlin.com/linux/v5.0-rc5/source/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c#L329 > > It blocks?!
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On 2/5/19 10:31 AM, Tomasz Figa wrote: > On Tue, Feb 5, 2019 at 6:00 PM Hans Verkuil wrote: >> >> On 2/5/19 7:26 AM, Tomasz Figa wrote: >>> On Fri, Feb 1, 2019 at 12:18 AM Nicolas Dufresne >>> wrote: Le jeudi 31 janvier 2019 à 22:34 +0900, Tomasz Figa a écrit : > On Thu, Jan 31, 2019 at 9:42 PM Philipp Zabel > wrote: >> Hi Nicolas, >> >> On Wed, 2019-01-30 at 10:32 -0500, Nicolas Dufresne wrote: >>> Le mercredi 30 janvier 2019 à 15:17 +0900, Tomasz Figa a écrit : > I don't remember saying that, maybe I meant to say there might be a > workaround ? > > For the fact, here we queue the headers (or first frame): > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L624 > > Then few line below this helper does G_FMT internally: > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L634 > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L3907 > > And just plainly fails if G_FMT returns an error of any type. This was > how Kamil designed it initially for MFC driver. There was no other > alternative back then (no EAGAIN yet either). Hmm, was that ffmpeg then? So would it just set the OUTPUT width and height to 0? Does it mean that gstreamer doesn't work with coda and mtk-vcodec, which don't have such wait in their g_fmt implementations? >>> >>> I don't know for MTK, I don't have the hardware and didn't integrate >>> their vendor pixel format. For the CODA, I know it works, if there is >>> no wait in the G_FMT, then I suppose we are being really lucky with the >>> timing (it would be that the drivers process the SPS/PPS synchronously, >>> and a simple lock in the G_FMT call is enough to wait). Adding Philipp >>> in CC, he could explain how this works, I know they use GStreamer in >>> production, and he would have fixed GStreamer already if that was >>> causing important issue. >> >> CODA predates the width/height=0 rule on the coded/OUTPUT queue. >> It currently behaves more like a traditional mem2mem device. > > The rule in the latest spec is that if width/height is 0 then CAPTURE > format is determined only after the stream is parsed. Otherwise it's > instantly deduced from the OUTPUT resolution. > >> When width/height is set via S_FMT(OUT) or output crop selection, the >> driver will believe it and set the same (rounded up to macroblock >> alignment) on the capture queue without ever having seen the SPS. > > That's why I asked whether gstreamer sets width and height of OUTPUT > to non-zero values. If so, there is no regression, as the specs mimic > the coda behavior. I see, with Philipp's answer it explains why it works. Note that GStreamer sets the display size on the OUTPUT format (in fact we pass as much information as we have, because a) it's generic code and b) it will be needed someday when we enable pre-allocation (REQBUFS before SPS/PPS is passed, to avoid the setup delay introduce by allocation, mostly seen with CMA base decoder). In any case, the driver reported display size should always be ignored in GStreamer, the only information we look at is the G_SELECTION for the case the x/y or the cropping rectangle is non-zero. Note this can only work if the capture queue is not affected by the coded size, or if the round-up made by the driver is bigger or equal to that coded size. I believe CODA falls into the first category, since the decoding happens in a separate set of buffers and are then de-tiled into the capture buffers (if understood correctly). >>> >>> Sounds like it would work only if coded size is equal to the visible >>> size (that GStreamer sets) rounded up to full macroblocks. Non-zero x >>> or y in the crop could be problematic too. >>> >>> Hans, what's your view on this? Should we require G_FMT(CAPTURE) to >>> wait until a format becomes available or the OUTPUT queue runs out of >> >> You mean CAPTURE queue? If not, then I don't understand that part. > > No, I exactly meant the OUTPUT queue. The behavior of s5p-mfc in case > of the format not being detected yet is to waits for any pending > bitstream buffers to be processed by the decoder before returning an > error. > > See > https://elixir.bootlin.com/linux/v5.0-rc5/source/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c#L329 It blocks?! That shouldn't happen. Totally against the spec. > . > >> >>> buffers? >> >> First see my comment here regarding G_FMT returning an error: >> >> https://www.spinics.net/lists/linux-media/msg146505.html >> >> In my view that is a bad idea. > > I don't like it either, but it seemed to
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On Tue, Feb 5, 2019 at 6:00 PM Hans Verkuil wrote: > > On 2/5/19 7:26 AM, Tomasz Figa wrote: > > On Fri, Feb 1, 2019 at 12:18 AM Nicolas Dufresne > > wrote: > >> > >> Le jeudi 31 janvier 2019 à 22:34 +0900, Tomasz Figa a écrit : > >>> On Thu, Jan 31, 2019 at 9:42 PM Philipp Zabel > >>> wrote: > Hi Nicolas, > > On Wed, 2019-01-30 at 10:32 -0500, Nicolas Dufresne wrote: > > Le mercredi 30 janvier 2019 à 15:17 +0900, Tomasz Figa a écrit : > >>> I don't remember saying that, maybe I meant to say there might be a > >>> workaround ? > >>> > >>> For the fact, here we queue the headers (or first frame): > >>> > >>> https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L624 > >>> > >>> Then few line below this helper does G_FMT internally: > >>> > >>> https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L634 > >>> https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L3907 > >>> > >>> And just plainly fails if G_FMT returns an error of any type. This was > >>> how Kamil designed it initially for MFC driver. There was no other > >>> alternative back then (no EAGAIN yet either). > >> > >> Hmm, was that ffmpeg then? > >> > >> So would it just set the OUTPUT width and height to 0? Does it mean > >> that gstreamer doesn't work with coda and mtk-vcodec, which don't have > >> such wait in their g_fmt implementations? > > > > I don't know for MTK, I don't have the hardware and didn't integrate > > their vendor pixel format. For the CODA, I know it works, if there is > > no wait in the G_FMT, then I suppose we are being really lucky with the > > timing (it would be that the drivers process the SPS/PPS synchronously, > > and a simple lock in the G_FMT call is enough to wait). Adding Philipp > > in CC, he could explain how this works, I know they use GStreamer in > > production, and he would have fixed GStreamer already if that was > > causing important issue. > > CODA predates the width/height=0 rule on the coded/OUTPUT queue. > It currently behaves more like a traditional mem2mem device. > >>> > >>> The rule in the latest spec is that if width/height is 0 then CAPTURE > >>> format is determined only after the stream is parsed. Otherwise it's > >>> instantly deduced from the OUTPUT resolution. > >>> > When width/height is set via S_FMT(OUT) or output crop selection, the > driver will believe it and set the same (rounded up to macroblock > alignment) on the capture queue without ever having seen the SPS. > >>> > >>> That's why I asked whether gstreamer sets width and height of OUTPUT > >>> to non-zero values. If so, there is no regression, as the specs mimic > >>> the coda behavior. > >> > >> I see, with Philipp's answer it explains why it works. Note that > >> GStreamer sets the display size on the OUTPUT format (in fact we pass > >> as much information as we have, because a) it's generic code and b) it > >> will be needed someday when we enable pre-allocation (REQBUFS before > >> SPS/PPS is passed, to avoid the setup delay introduce by allocation, > >> mostly seen with CMA base decoder). In any case, the driver reported > >> display size should always be ignored in GStreamer, the only > >> information we look at is the G_SELECTION for the case the x/y or the > >> cropping rectangle is non-zero. > >> > >> Note this can only work if the capture queue is not affected by the > >> coded size, or if the round-up made by the driver is bigger or equal to > >> that coded size. I believe CODA falls into the first category, since > >> the decoding happens in a separate set of buffers and are then de-tiled > >> into the capture buffers (if understood correctly). > > > > Sounds like it would work only if coded size is equal to the visible > > size (that GStreamer sets) rounded up to full macroblocks. Non-zero x > > or y in the crop could be problematic too. > > > > Hans, what's your view on this? Should we require G_FMT(CAPTURE) to > > wait until a format becomes available or the OUTPUT queue runs out of > > You mean CAPTURE queue? If not, then I don't understand that part. No, I exactly meant the OUTPUT queue. The behavior of s5p-mfc in case of the format not being detected yet is to waits for any pending bitstream buffers to be processed by the decoder before returning an error. See https://elixir.bootlin.com/linux/v5.0-rc5/source/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c#L329 . > > > buffers? > > First see my comment here regarding G_FMT returning an error: > > https://www.spinics.net/lists/linux-media/msg146505.html > > In my view that is a bad idea. I don't like it either, but it seemed to be the most consistent and compatible behavior, but I'm not sure anymore. > > What G_FMT should return between the time a resolution
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On 2/5/19 7:26 AM, Tomasz Figa wrote: > On Fri, Feb 1, 2019 at 12:18 AM Nicolas Dufresne wrote: >> >> Le jeudi 31 janvier 2019 à 22:34 +0900, Tomasz Figa a écrit : >>> On Thu, Jan 31, 2019 at 9:42 PM Philipp Zabel >>> wrote: Hi Nicolas, On Wed, 2019-01-30 at 10:32 -0500, Nicolas Dufresne wrote: > Le mercredi 30 janvier 2019 à 15:17 +0900, Tomasz Figa a écrit : >>> I don't remember saying that, maybe I meant to say there might be a >>> workaround ? >>> >>> For the fact, here we queue the headers (or first frame): >>> >>> https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L624 >>> >>> Then few line below this helper does G_FMT internally: >>> >>> https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L634 >>> https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L3907 >>> >>> And just plainly fails if G_FMT returns an error of any type. This was >>> how Kamil designed it initially for MFC driver. There was no other >>> alternative back then (no EAGAIN yet either). >> >> Hmm, was that ffmpeg then? >> >> So would it just set the OUTPUT width and height to 0? Does it mean >> that gstreamer doesn't work with coda and mtk-vcodec, which don't have >> such wait in their g_fmt implementations? > > I don't know for MTK, I don't have the hardware and didn't integrate > their vendor pixel format. For the CODA, I know it works, if there is > no wait in the G_FMT, then I suppose we are being really lucky with the > timing (it would be that the drivers process the SPS/PPS synchronously, > and a simple lock in the G_FMT call is enough to wait). Adding Philipp > in CC, he could explain how this works, I know they use GStreamer in > production, and he would have fixed GStreamer already if that was > causing important issue. CODA predates the width/height=0 rule on the coded/OUTPUT queue. It currently behaves more like a traditional mem2mem device. >>> >>> The rule in the latest spec is that if width/height is 0 then CAPTURE >>> format is determined only after the stream is parsed. Otherwise it's >>> instantly deduced from the OUTPUT resolution. >>> When width/height is set via S_FMT(OUT) or output crop selection, the driver will believe it and set the same (rounded up to macroblock alignment) on the capture queue without ever having seen the SPS. >>> >>> That's why I asked whether gstreamer sets width and height of OUTPUT >>> to non-zero values. If so, there is no regression, as the specs mimic >>> the coda behavior. >> >> I see, with Philipp's answer it explains why it works. Note that >> GStreamer sets the display size on the OUTPUT format (in fact we pass >> as much information as we have, because a) it's generic code and b) it >> will be needed someday when we enable pre-allocation (REQBUFS before >> SPS/PPS is passed, to avoid the setup delay introduce by allocation, >> mostly seen with CMA base decoder). In any case, the driver reported >> display size should always be ignored in GStreamer, the only >> information we look at is the G_SELECTION for the case the x/y or the >> cropping rectangle is non-zero. >> >> Note this can only work if the capture queue is not affected by the >> coded size, or if the round-up made by the driver is bigger or equal to >> that coded size. I believe CODA falls into the first category, since >> the decoding happens in a separate set of buffers and are then de-tiled >> into the capture buffers (if understood correctly). > > Sounds like it would work only if coded size is equal to the visible > size (that GStreamer sets) rounded up to full macroblocks. Non-zero x > or y in the crop could be problematic too. > > Hans, what's your view on this? Should we require G_FMT(CAPTURE) to > wait until a format becomes available or the OUTPUT queue runs out of You mean CAPTURE queue? If not, then I don't understand that part. > buffers? First see my comment here regarding G_FMT returning an error: https://www.spinics.net/lists/linux-media/msg146505.html In my view that is a bad idea. What G_FMT should return between the time a resolution change was detected and the CAPTURE queue being drained (i.e. the old or the new resolution?) is something I am not sure about. On the one hand it is desirable to have the new resolution asap, on the other hand, returning the new resolution would mean that the returned format is inconsistent with the capture buffer sizes. I'm leaning towards either returning the new resolution. Regards, Hans > >> >> I would say, best is just to test the updated Venus driver, which is in >> my queue. > > The updated Venus driver doesn't implement the behavior I referred to, > but rather the legacy wait in G_FMT(CAPTURE) as in s5p-mfc. >
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On Fri, Feb 1, 2019 at 12:18 AM Nicolas Dufresne wrote: > > Le jeudi 31 janvier 2019 à 22:34 +0900, Tomasz Figa a écrit : > > On Thu, Jan 31, 2019 at 9:42 PM Philipp Zabel > > wrote: > > > Hi Nicolas, > > > > > > On Wed, 2019-01-30 at 10:32 -0500, Nicolas Dufresne wrote: > > > > Le mercredi 30 janvier 2019 à 15:17 +0900, Tomasz Figa a écrit : > > > > > > I don't remember saying that, maybe I meant to say there might be a > > > > > > workaround ? > > > > > > > > > > > > For the fact, here we queue the headers (or first frame): > > > > > > > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L624 > > > > > > > > > > > > Then few line below this helper does G_FMT internally: > > > > > > > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L634 > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L3907 > > > > > > > > > > > > And just plainly fails if G_FMT returns an error of any type. This > > > > > > was > > > > > > how Kamil designed it initially for MFC driver. There was no other > > > > > > alternative back then (no EAGAIN yet either). > > > > > > > > > > Hmm, was that ffmpeg then? > > > > > > > > > > So would it just set the OUTPUT width and height to 0? Does it mean > > > > > that gstreamer doesn't work with coda and mtk-vcodec, which don't have > > > > > such wait in their g_fmt implementations? > > > > > > > > I don't know for MTK, I don't have the hardware and didn't integrate > > > > their vendor pixel format. For the CODA, I know it works, if there is > > > > no wait in the G_FMT, then I suppose we are being really lucky with the > > > > timing (it would be that the drivers process the SPS/PPS synchronously, > > > > and a simple lock in the G_FMT call is enough to wait). Adding Philipp > > > > in CC, he could explain how this works, I know they use GStreamer in > > > > production, and he would have fixed GStreamer already if that was > > > > causing important issue. > > > > > > CODA predates the width/height=0 rule on the coded/OUTPUT queue. > > > It currently behaves more like a traditional mem2mem device. > > > > The rule in the latest spec is that if width/height is 0 then CAPTURE > > format is determined only after the stream is parsed. Otherwise it's > > instantly deduced from the OUTPUT resolution. > > > > > When width/height is set via S_FMT(OUT) or output crop selection, the > > > driver will believe it and set the same (rounded up to macroblock > > > alignment) on the capture queue without ever having seen the SPS. > > > > That's why I asked whether gstreamer sets width and height of OUTPUT > > to non-zero values. If so, there is no regression, as the specs mimic > > the coda behavior. > > I see, with Philipp's answer it explains why it works. Note that > GStreamer sets the display size on the OUTPUT format (in fact we pass > as much information as we have, because a) it's generic code and b) it > will be needed someday when we enable pre-allocation (REQBUFS before > SPS/PPS is passed, to avoid the setup delay introduce by allocation, > mostly seen with CMA base decoder). In any case, the driver reported > display size should always be ignored in GStreamer, the only > information we look at is the G_SELECTION for the case the x/y or the > cropping rectangle is non-zero. > > Note this can only work if the capture queue is not affected by the > coded size, or if the round-up made by the driver is bigger or equal to > that coded size. I believe CODA falls into the first category, since > the decoding happens in a separate set of buffers and are then de-tiled > into the capture buffers (if understood correctly). Sounds like it would work only if coded size is equal to the visible size (that GStreamer sets) rounded up to full macroblocks. Non-zero x or y in the crop could be problematic too. Hans, what's your view on this? Should we require G_FMT(CAPTURE) to wait until a format becomes available or the OUTPUT queue runs out of buffers? > > I would say, best is just to test the updated Venus driver, which is in > my queue. The updated Venus driver doesn't implement the behavior I referred to, but rather the legacy wait in G_FMT(CAPTURE) as in s5p-mfc.
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
Le jeudi 31 janvier 2019 à 22:34 +0900, Tomasz Figa a écrit : > On Thu, Jan 31, 2019 at 9:42 PM Philipp Zabel wrote: > > Hi Nicolas, > > > > On Wed, 2019-01-30 at 10:32 -0500, Nicolas Dufresne wrote: > > > Le mercredi 30 janvier 2019 à 15:17 +0900, Tomasz Figa a écrit : > > > > > I don't remember saying that, maybe I meant to say there might be a > > > > > workaround ? > > > > > > > > > > For the fact, here we queue the headers (or first frame): > > > > > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L624 > > > > > > > > > > Then few line below this helper does G_FMT internally: > > > > > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L634 > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L3907 > > > > > > > > > > And just plainly fails if G_FMT returns an error of any type. This was > > > > > how Kamil designed it initially for MFC driver. There was no other > > > > > alternative back then (no EAGAIN yet either). > > > > > > > > Hmm, was that ffmpeg then? > > > > > > > > So would it just set the OUTPUT width and height to 0? Does it mean > > > > that gstreamer doesn't work with coda and mtk-vcodec, which don't have > > > > such wait in their g_fmt implementations? > > > > > > I don't know for MTK, I don't have the hardware and didn't integrate > > > their vendor pixel format. For the CODA, I know it works, if there is > > > no wait in the G_FMT, then I suppose we are being really lucky with the > > > timing (it would be that the drivers process the SPS/PPS synchronously, > > > and a simple lock in the G_FMT call is enough to wait). Adding Philipp > > > in CC, he could explain how this works, I know they use GStreamer in > > > production, and he would have fixed GStreamer already if that was > > > causing important issue. > > > > CODA predates the width/height=0 rule on the coded/OUTPUT queue. > > It currently behaves more like a traditional mem2mem device. > > The rule in the latest spec is that if width/height is 0 then CAPTURE > format is determined only after the stream is parsed. Otherwise it's > instantly deduced from the OUTPUT resolution. > > > When width/height is set via S_FMT(OUT) or output crop selection, the > > driver will believe it and set the same (rounded up to macroblock > > alignment) on the capture queue without ever having seen the SPS. > > That's why I asked whether gstreamer sets width and height of OUTPUT > to non-zero values. If so, there is no regression, as the specs mimic > the coda behavior. I see, with Philipp's answer it explains why it works. Note that GStreamer sets the display size on the OUTPUT format (in fact we pass as much information as we have, because a) it's generic code and b) it will be needed someday when we enable pre-allocation (REQBUFS before SPS/PPS is passed, to avoid the setup delay introduce by allocation, mostly seen with CMA base decoder). In any case, the driver reported display size should always be ignored in GStreamer, the only information we look at is the G_SELECTION for the case the x/y or the cropping rectangle is non-zero. Note this can only work if the capture queue is not affected by the coded size, or if the round-up made by the driver is bigger or equal to that coded size. I believe CODA falls into the first category, since the decoding happens in a separate set of buffers and are then de-tiled into the capture buffers (if understood correctly). I would say, best is just to test the updated Venus driver, which is in my queue. > > > The source change event after SPS parsing that the spec requires isn't > > even implemented yet. Just to make sure, if I try and register that event on CODA with the current driver, it will simply fail immediately right ? I don't need any other magic to detect that this isn't supported ? > > > > regards > > Philipp signature.asc Description: This is a digitally signed message part
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On Thu, Jan 31, 2019 at 9:42 PM Philipp Zabel wrote: > > Hi Nicolas, > > On Wed, 2019-01-30 at 10:32 -0500, Nicolas Dufresne wrote: > > Le mercredi 30 janvier 2019 à 15:17 +0900, Tomasz Figa a écrit : > > > > I don't remember saying that, maybe I meant to say there might be a > > > > workaround ? > > > > > > > > For the fact, here we queue the headers (or first frame): > > > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L624 > > > > > > > > Then few line below this helper does G_FMT internally: > > > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L634 > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L3907 > > > > > > > > And just plainly fails if G_FMT returns an error of any type. This was > > > > how Kamil designed it initially for MFC driver. There was no other > > > > alternative back then (no EAGAIN yet either). > > > > > > Hmm, was that ffmpeg then? > > > > > > So would it just set the OUTPUT width and height to 0? Does it mean > > > that gstreamer doesn't work with coda and mtk-vcodec, which don't have > > > such wait in their g_fmt implementations? > > > > I don't know for MTK, I don't have the hardware and didn't integrate > > their vendor pixel format. For the CODA, I know it works, if there is > > no wait in the G_FMT, then I suppose we are being really lucky with the > > timing (it would be that the drivers process the SPS/PPS synchronously, > > and a simple lock in the G_FMT call is enough to wait). Adding Philipp > > in CC, he could explain how this works, I know they use GStreamer in > > production, and he would have fixed GStreamer already if that was > > causing important issue. > > CODA predates the width/height=0 rule on the coded/OUTPUT queue. > It currently behaves more like a traditional mem2mem device. The rule in the latest spec is that if width/height is 0 then CAPTURE format is determined only after the stream is parsed. Otherwise it's instantly deduced from the OUTPUT resolution. > > When width/height is set via S_FMT(OUT) or output crop selection, the > driver will believe it and set the same (rounded up to macroblock > alignment) on the capture queue without ever having seen the SPS. That's why I asked whether gstreamer sets width and height of OUTPUT to non-zero values. If so, there is no regression, as the specs mimic the coda behavior. > > The source change event after SPS parsing that the spec requires isn't > even implemented yet. > > regards > Philipp
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
Hi Nicolas, On Wed, 2019-01-30 at 10:32 -0500, Nicolas Dufresne wrote: > Le mercredi 30 janvier 2019 à 15:17 +0900, Tomasz Figa a écrit : > > > I don't remember saying that, maybe I meant to say there might be a > > > workaround ? > > > > > > For the fact, here we queue the headers (or first frame): > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L624 > > > > > > Then few line below this helper does G_FMT internally: > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L634 > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L3907 > > > > > > And just plainly fails if G_FMT returns an error of any type. This was > > > how Kamil designed it initially for MFC driver. There was no other > > > alternative back then (no EAGAIN yet either). > > > > Hmm, was that ffmpeg then? > > > > So would it just set the OUTPUT width and height to 0? Does it mean > > that gstreamer doesn't work with coda and mtk-vcodec, which don't have > > such wait in their g_fmt implementations? > > I don't know for MTK, I don't have the hardware and didn't integrate > their vendor pixel format. For the CODA, I know it works, if there is > no wait in the G_FMT, then I suppose we are being really lucky with the > timing (it would be that the drivers process the SPS/PPS synchronously, > and a simple lock in the G_FMT call is enough to wait). Adding Philipp > in CC, he could explain how this works, I know they use GStreamer in > production, and he would have fixed GStreamer already if that was > causing important issue. CODA predates the width/height=0 rule on the coded/OUTPUT queue. It currently behaves more like a traditional mem2mem device. When width/height is set via S_FMT(OUT) or output crop selection, the driver will believe it and set the same (rounded up to macroblock alignment) on the capture queue without ever having seen the SPS. The source change event after SPS parsing that the spec requires isn't even implemented yet. regards Philipp
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
Le mercredi 30 janvier 2019 à 15:17 +0900, Tomasz Figa a écrit : > > I don't remember saying that, maybe I meant to say there might be a > > workaround ? > > > > For the fact, here we queue the headers (or first frame): > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L624 > > > > Then few line below this helper does G_FMT internally: > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L634 > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L3907 > > > > And just plainly fails if G_FMT returns an error of any type. This was > > how Kamil designed it initially for MFC driver. There was no other > > alternative back then (no EAGAIN yet either). > > Hmm, was that ffmpeg then? > > So would it just set the OUTPUT width and height to 0? Does it mean > that gstreamer doesn't work with coda and mtk-vcodec, which don't have > such wait in their g_fmt implementations? I don't know for MTK, I don't have the hardware and didn't integrate their vendor pixel format. For the CODA, I know it works, if there is no wait in the G_FMT, then I suppose we are being really lucky with the timing (it would be that the drivers process the SPS/PPS synchronously, and a simple lock in the G_FMT call is enough to wait). Adding Philipp in CC, he could explain how this works, I know they use GStreamer in production, and he would have fixed GStreamer already if that was causing important issue. Nicolas
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On Wed, Jan 30, 2019 at 1:21 PM Nicolas Dufresne wrote: > > Le mercredi 30 janvier 2019 à 12:38 +0900, Tomasz Figa a écrit : > > > Yes, unfortunately, GStreamer still rely on G_FMT waiting a minimal > > > amount of time of the headers to be processed. This was how things was > > > created back in 2011, I could not program GStreamer for the future. If > > > we stop doing this, we do break GStreamer as a valid userspace > > > application. > > > > Does it? Didn't you say earlier that you end up setting the OUTPUT > > format with the stream resolution as parsed on your own? If so, that > > would actually expose a matching framebuffer format on the CAPTURE > > queue, so there is no need to wait for the real parsing to happen. > > I don't remember saying that, maybe I meant to say there might be a > workaround ? > > For the fact, here we queue the headers (or first frame): > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L624 > > Then few line below this helper does G_FMT internally: > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L634 > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L3907 > > And just plainly fails if G_FMT returns an error of any type. This was > how Kamil designed it initially for MFC driver. There was no other > alternative back then (no EAGAIN yet either). Hmm, was that ffmpeg then? So would it just set the OUTPUT width and height to 0? Does it mean that gstreamer doesn't work with coda and mtk-vcodec, which don't have such wait in their g_fmt implementations? > > Nicolas > > p.s. it's still in my todo's to implement source change event as I > believe it is a better mechanism (specially if you header happened to > be corrupted, then the driver can consume the stream until it finds a > sync). So these sleep or normally wait exist all over to support this > legacy thing. It is unfortunate, the question is do you want to break > userspace now ? Without having first placed a patch that would maybe > warn or something for a while ? > I don't want and my understanding was that we could workaround it by the propagation of format from OUTPUT to CAPTURE. Also see above. Best regards, Tomasz
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
Le mercredi 30 janvier 2019 à 12:38 +0900, Tomasz Figa a écrit : > > Yes, unfortunately, GStreamer still rely on G_FMT waiting a minimal > > amount of time of the headers to be processed. This was how things was > > created back in 2011, I could not program GStreamer for the future. If > > we stop doing this, we do break GStreamer as a valid userspace > > application. > > Does it? Didn't you say earlier that you end up setting the OUTPUT > format with the stream resolution as parsed on your own? If so, that > would actually expose a matching framebuffer format on the CAPTURE > queue, so there is no need to wait for the real parsing to happen. I don't remember saying that, maybe I meant to say there might be a workaround ? For the fact, here we queue the headers (or first frame): https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L624 Then few line below this helper does G_FMT internally: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2videodec.c#L634 https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L3907 And just plainly fails if G_FMT returns an error of any type. This was how Kamil designed it initially for MFC driver. There was no other alternative back then (no EAGAIN yet either). Nicolas p.s. it's still in my todo's to implement source change event as I believe it is a better mechanism (specially if you header happened to be corrupted, then the driver can consume the stream until it finds a sync). So these sleep or normally wait exist all over to support this legacy thing. It is unfortunate, the question is do you want to break userspace now ? Without having first placed a patch that would maybe warn or something for a while ?
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On Wed, Jan 30, 2019 at 12:18 PM Nicolas Dufresne wrote: > > Le lundi 28 janvier 2019 à 16:38 +0900, Tomasz Figa a écrit : > > > > Nope, that's not what is expected to happen here. Especially since > > > > you're potentially in non-blocking IO mode. Regardless of that, the > > > > > > OK, how to handle that when userspace (for example gstreamer) hasn't > > > support for v4l2 events? The s5p-mfc decoder is doing the same sleep in > > > g_fmt. > > > > I don't think that sleep in s5p-mfc was needed for gstreamer and > > AFAICT other drivers don't have it. Doesn't gstreamer just set the > > coded format on OUTPUT queue on its own? That should propagate the > > format to the CAPTURE queue, without the need to parse the stream. > > Yes, unfortunately, GStreamer still rely on G_FMT waiting a minimal > amount of time of the headers to be processed. This was how things was > created back in 2011, I could not program GStreamer for the future. If > we stop doing this, we do break GStreamer as a valid userspace > application. Does it? Didn't you say earlier that you end up setting the OUTPUT format with the stream resolution as parsed on your own? If so, that would actually expose a matching framebuffer format on the CAPTURE queue, so there is no need to wait for the real parsing to happen. > > This is not what I want long term, but I haven't got time to add event > support, and there is a certain amount of time (years) when this is > implemented before all the old code goes away. > > Nicolas >
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
Le lundi 28 janvier 2019 à 16:38 +0900, Tomasz Figa a écrit : > > > Nope, that's not what is expected to happen here. Especially since > > > you're potentially in non-blocking IO mode. Regardless of that, the > > > > OK, how to handle that when userspace (for example gstreamer) hasn't > > support for v4l2 events? The s5p-mfc decoder is doing the same sleep in > > g_fmt. > > I don't think that sleep in s5p-mfc was needed for gstreamer and > AFAICT other drivers don't have it. Doesn't gstreamer just set the > coded format on OUTPUT queue on its own? That should propagate the > format to the CAPTURE queue, without the need to parse the stream. Yes, unfortunately, GStreamer still rely on G_FMT waiting a minimal amount of time of the headers to be processed. This was how things was created back in 2011, I could not program GStreamer for the future. If we stop doing this, we do break GStreamer as a valid userspace application. This is not what I want long term, but I haven't got time to add event support, and there is a certain amount of time (years) when this is implemented before all the old code goes away. Nicolas
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On Tue, Jan 29, 2019 at 1:28 AM Stanimir Varbanov
wrote:
>
> Hi Tomasz,
>
> On 1/28/19 9:38 AM, Tomasz Figa wrote:
> > On Fri, Jan 25, 2019 at 7:25 PM Stanimir Varbanov
> > wrote:
> >>
> >> Hi Tomasz,
> >>
> >> Thanks for the comments!
> >>
> >> On 1/25/19 9:59 AM, Tomasz Figa wrote:
> >>> .Hi Stan,
> >>>
> >>> On Fri, Jan 18, 2019 at 1:21 AM Stanimir Varbanov
> >>> wrote:
>
> This refactored code for start/stop streaming vb2 operations and
> adds a state machine handling similar to the one in stateful codec
> API documentation. One major change is that now the HFI session is
> started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0),
> during that time streamoff(cap,out) just flush buffers but doesn't
> stop the session. The other major change is that now the capture
> and output queues are completely separated.
>
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/core.h| 20 +-
> drivers/media/platform/qcom/venus/helpers.c | 23 +-
> drivers/media/platform/qcom/venus/helpers.h | 5 +
> drivers/media/platform/qcom/venus/vdec.c| 449
> 4 files changed, 389 insertions(+), 108 deletions(-)
>
> >>>
> >>> Thanks for the patch! Please see some comments inline.
> >>>
> diff --git a/drivers/media/platform/qcom/venus/core.h
> b/drivers/media/platform/qcom/venus/core.h
> index 79c7e816c706..5a133c203455 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -218,6 +218,15 @@ struct venus_buffer {
>
> #define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer,
> vb)
>
> +#define DEC_STATE_UNINIT 0
> +#define DEC_STATE_INIT 1
> +#define DEC_STATE_CAPTURE_SETUP2
> +#define DEC_STATE_STOPPED 3
> +#define DEC_STATE_SEEK 4
> +#define DEC_STATE_DRAIN5
> +#define DEC_STATE_DECODING 6
> +#define DEC_STATE_DRC 7
> +
> /**
> * struct venus_inst - holds per instance paramerters
> *
> @@ -241,6 +250,10 @@ struct venus_buffer {
> * @colorspace:current color space
> * @quantization: current quantization
> * @xfer_func: current xfer function
> + * @codec_state: current codec API state (see DEC/ENC_STATE_)
> + * @reconf_wait: wait queue for resolution change event
> + * @ten_bits: does new stream is 10bits depth
> + * @buf_count: used to count number number of buffers
> (reqbuf(0))
> * @fps: holds current FPS
> * @timeperframe: holds current time per frame structure
> * @fmt_out: a reference to output format structure
> @@ -255,8 +268,6 @@ struct venus_buffer {
> * @opb_buftype: output picture buffer type
> * @opb_fmt: output picture buffer raw format
> * @reconfig: a flag raised by decoder when the stream resolution
> changed
> - * @reconfig_width:holds the new width
> - * @reconfig_height: holds the new height
> * @hfi_codec: current codec for this instance in HFI space
> * @sequence_cap: a sequence counter for capture queue
> * @sequence_out: a sequence counter for output queue
> @@ -296,6 +307,9 @@ struct venus_inst {
> u8 ycbcr_enc;
> u8 quantization;
> u8 xfer_func;
> + unsigned int codec_state;
> + wait_queue_head_t reconf_wait;
> + int buf_count;
> u64 fps;
> struct v4l2_fract timeperframe;
> const struct venus_format *fmt_out;
> @@ -310,8 +324,6 @@ struct venus_inst {
> u32 opb_buftype;
> u32 opb_fmt;
> bool reconfig;
> - u32 reconfig_width;
> - u32 reconfig_height;
> u32 hfi_codec;
> u32 sequence_cap;
> u32 sequence_out;
> diff --git a/drivers/media/platform/qcom/venus/helpers.c
> b/drivers/media/platform/qcom/venus/helpers.c
> index 637ce7b82d94..25d8cceccae4 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct
> vb2_buffer *vb)
>
> v4l2_m2m_buf_queue(m2m_ctx, vbuf);
>
> - if (!(inst->streamon_out & inst->streamon_cap))
> - goto unlock;
> -
> - ret = is_buf_refed(inst, vbuf);
> - if (ret)
> - goto unlock;
> + if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) {
> >>>
> >>> Wouldn't a simple vb2_is_streaming() work here?
> >>
> >>
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
Hi Tomasz,
On 1/28/19 9:38 AM, Tomasz Figa wrote:
> On Fri, Jan 25, 2019 at 7:25 PM Stanimir Varbanov
> wrote:
>>
>> Hi Tomasz,
>>
>> Thanks for the comments!
>>
>> On 1/25/19 9:59 AM, Tomasz Figa wrote:
>>> .Hi Stan,
>>>
>>> On Fri, Jan 18, 2019 at 1:21 AM Stanimir Varbanov
>>> wrote:
This refactored code for start/stop streaming vb2 operations and
adds a state machine handling similar to the one in stateful codec
API documentation. One major change is that now the HFI session is
started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0),
during that time streamoff(cap,out) just flush buffers but doesn't
stop the session. The other major change is that now the capture
and output queues are completely separated.
Signed-off-by: Stanimir Varbanov
---
drivers/media/platform/qcom/venus/core.h| 20 +-
drivers/media/platform/qcom/venus/helpers.c | 23 +-
drivers/media/platform/qcom/venus/helpers.h | 5 +
drivers/media/platform/qcom/venus/vdec.c| 449
4 files changed, 389 insertions(+), 108 deletions(-)
>>>
>>> Thanks for the patch! Please see some comments inline.
>>>
diff --git a/drivers/media/platform/qcom/venus/core.h
b/drivers/media/platform/qcom/venus/core.h
index 79c7e816c706..5a133c203455 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -218,6 +218,15 @@ struct venus_buffer {
#define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer, vb)
+#define DEC_STATE_UNINIT 0
+#define DEC_STATE_INIT 1
+#define DEC_STATE_CAPTURE_SETUP2
+#define DEC_STATE_STOPPED 3
+#define DEC_STATE_SEEK 4
+#define DEC_STATE_DRAIN5
+#define DEC_STATE_DECODING 6
+#define DEC_STATE_DRC 7
+
/**
* struct venus_inst - holds per instance paramerters
*
@@ -241,6 +250,10 @@ struct venus_buffer {
* @colorspace:current color space
* @quantization: current quantization
* @xfer_func: current xfer function
+ * @codec_state: current codec API state (see DEC/ENC_STATE_)
+ * @reconf_wait: wait queue for resolution change event
+ * @ten_bits: does new stream is 10bits depth
+ * @buf_count: used to count number number of buffers (reqbuf(0))
* @fps: holds current FPS
* @timeperframe: holds current time per frame structure
* @fmt_out: a reference to output format structure
@@ -255,8 +268,6 @@ struct venus_buffer {
* @opb_buftype: output picture buffer type
* @opb_fmt: output picture buffer raw format
* @reconfig: a flag raised by decoder when the stream resolution changed
- * @reconfig_width:holds the new width
- * @reconfig_height: holds the new height
* @hfi_codec: current codec for this instance in HFI space
* @sequence_cap: a sequence counter for capture queue
* @sequence_out: a sequence counter for output queue
@@ -296,6 +307,9 @@ struct venus_inst {
u8 ycbcr_enc;
u8 quantization;
u8 xfer_func;
+ unsigned int codec_state;
+ wait_queue_head_t reconf_wait;
+ int buf_count;
u64 fps;
struct v4l2_fract timeperframe;
const struct venus_format *fmt_out;
@@ -310,8 +324,6 @@ struct venus_inst {
u32 opb_buftype;
u32 opb_fmt;
bool reconfig;
- u32 reconfig_width;
- u32 reconfig_height;
u32 hfi_codec;
u32 sequence_cap;
u32 sequence_out;
diff --git a/drivers/media/platform/qcom/venus/helpers.c
b/drivers/media/platform/qcom/venus/helpers.c
index 637ce7b82d94..25d8cceccae4 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer
*vb)
v4l2_m2m_buf_queue(m2m_ctx, vbuf);
- if (!(inst->streamon_out & inst->streamon_cap))
- goto unlock;
-
- ret = is_buf_refed(inst, vbuf);
- if (ret)
- goto unlock;
+ if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) {
>>>
>>> Wouldn't a simple vb2_is_streaming() work here?
>>
>> I'd say no, because the buffer can be queued but the streaming on that
>> queue isn't started yet. The idea is to send buffers to firmware only
>> when the streaming is on on that queue,
>
> Isn't it exactly what vb2_is_streaming(vb->vb2_queue) would check?
Not exactly, q->streaming is set when user call
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On Fri, Jan 25, 2019 at 7:25 PM Stanimir Varbanov
wrote:
>
> Hi Tomasz,
>
> Thanks for the comments!
>
> On 1/25/19 9:59 AM, Tomasz Figa wrote:
> > .Hi Stan,
> >
> > On Fri, Jan 18, 2019 at 1:21 AM Stanimir Varbanov
> > wrote:
> >>
> >> This refactored code for start/stop streaming vb2 operations and
> >> adds a state machine handling similar to the one in stateful codec
> >> API documentation. One major change is that now the HFI session is
> >> started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0),
> >> during that time streamoff(cap,out) just flush buffers but doesn't
> >> stop the session. The other major change is that now the capture
> >> and output queues are completely separated.
> >>
> >> Signed-off-by: Stanimir Varbanov
> >> ---
> >> drivers/media/platform/qcom/venus/core.h| 20 +-
> >> drivers/media/platform/qcom/venus/helpers.c | 23 +-
> >> drivers/media/platform/qcom/venus/helpers.h | 5 +
> >> drivers/media/platform/qcom/venus/vdec.c| 449
> >> 4 files changed, 389 insertions(+), 108 deletions(-)
> >>
> >
> > Thanks for the patch! Please see some comments inline.
> >
> >> diff --git a/drivers/media/platform/qcom/venus/core.h
> >> b/drivers/media/platform/qcom/venus/core.h
> >> index 79c7e816c706..5a133c203455 100644
> >> --- a/drivers/media/platform/qcom/venus/core.h
> >> +++ b/drivers/media/platform/qcom/venus/core.h
> >> @@ -218,6 +218,15 @@ struct venus_buffer {
> >>
> >> #define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer, vb)
> >>
> >> +#define DEC_STATE_UNINIT 0
> >> +#define DEC_STATE_INIT 1
> >> +#define DEC_STATE_CAPTURE_SETUP2
> >> +#define DEC_STATE_STOPPED 3
> >> +#define DEC_STATE_SEEK 4
> >> +#define DEC_STATE_DRAIN5
> >> +#define DEC_STATE_DECODING 6
> >> +#define DEC_STATE_DRC 7
> >> +
> >> /**
> >> * struct venus_inst - holds per instance paramerters
> >> *
> >> @@ -241,6 +250,10 @@ struct venus_buffer {
> >> * @colorspace:current color space
> >> * @quantization: current quantization
> >> * @xfer_func: current xfer function
> >> + * @codec_state: current codec API state (see DEC/ENC_STATE_)
> >> + * @reconf_wait: wait queue for resolution change event
> >> + * @ten_bits: does new stream is 10bits depth
> >> + * @buf_count: used to count number number of buffers (reqbuf(0))
> >> * @fps: holds current FPS
> >> * @timeperframe: holds current time per frame structure
> >> * @fmt_out: a reference to output format structure
> >> @@ -255,8 +268,6 @@ struct venus_buffer {
> >> * @opb_buftype: output picture buffer type
> >> * @opb_fmt: output picture buffer raw format
> >> * @reconfig: a flag raised by decoder when the stream resolution changed
> >> - * @reconfig_width:holds the new width
> >> - * @reconfig_height: holds the new height
> >> * @hfi_codec: current codec for this instance in HFI space
> >> * @sequence_cap: a sequence counter for capture queue
> >> * @sequence_out: a sequence counter for output queue
> >> @@ -296,6 +307,9 @@ struct venus_inst {
> >> u8 ycbcr_enc;
> >> u8 quantization;
> >> u8 xfer_func;
> >> + unsigned int codec_state;
> >> + wait_queue_head_t reconf_wait;
> >> + int buf_count;
> >> u64 fps;
> >> struct v4l2_fract timeperframe;
> >> const struct venus_format *fmt_out;
> >> @@ -310,8 +324,6 @@ struct venus_inst {
> >> u32 opb_buftype;
> >> u32 opb_fmt;
> >> bool reconfig;
> >> - u32 reconfig_width;
> >> - u32 reconfig_height;
> >> u32 hfi_codec;
> >> u32 sequence_cap;
> >> u32 sequence_out;
> >> diff --git a/drivers/media/platform/qcom/venus/helpers.c
> >> b/drivers/media/platform/qcom/venus/helpers.c
> >> index 637ce7b82d94..25d8cceccae4 100644
> >> --- a/drivers/media/platform/qcom/venus/helpers.c
> >> +++ b/drivers/media/platform/qcom/venus/helpers.c
> >> @@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer
> >> *vb)
> >>
> >> v4l2_m2m_buf_queue(m2m_ctx, vbuf);
> >>
> >> - if (!(inst->streamon_out & inst->streamon_cap))
> >> - goto unlock;
> >> -
> >> - ret = is_buf_refed(inst, vbuf);
> >> - if (ret)
> >> - goto unlock;
> >> + if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) {
> >
> > Wouldn't a simple vb2_is_streaming() work here?
>
> I'd say no, because the buffer can be queued but the streaming on that
> queue isn't started yet. The idea is to send buffers to firmware only
> when the streaming is on on that queue,
Isn't it exactly what vb2_is_streaming(vb->vb2_queue) would check?
> otherwise we only queue the
> buffer in m2m_buf_queue (and send for processing once the streaming on
> that particular
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
Hi Tomasz,
Thanks for the comments!
On 1/25/19 9:59 AM, Tomasz Figa wrote:
> .Hi Stan,
>
> On Fri, Jan 18, 2019 at 1:21 AM Stanimir Varbanov
> wrote:
>>
>> This refactored code for start/stop streaming vb2 operations and
>> adds a state machine handling similar to the one in stateful codec
>> API documentation. One major change is that now the HFI session is
>> started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0),
>> during that time streamoff(cap,out) just flush buffers but doesn't
>> stop the session. The other major change is that now the capture
>> and output queues are completely separated.
>>
>> Signed-off-by: Stanimir Varbanov
>> ---
>> drivers/media/platform/qcom/venus/core.h| 20 +-
>> drivers/media/platform/qcom/venus/helpers.c | 23 +-
>> drivers/media/platform/qcom/venus/helpers.h | 5 +
>> drivers/media/platform/qcom/venus/vdec.c| 449
>> 4 files changed, 389 insertions(+), 108 deletions(-)
>>
>
> Thanks for the patch! Please see some comments inline.
>
>> diff --git a/drivers/media/platform/qcom/venus/core.h
>> b/drivers/media/platform/qcom/venus/core.h
>> index 79c7e816c706..5a133c203455 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -218,6 +218,15 @@ struct venus_buffer {
>>
>> #define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer, vb)
>>
>> +#define DEC_STATE_UNINIT 0
>> +#define DEC_STATE_INIT 1
>> +#define DEC_STATE_CAPTURE_SETUP2
>> +#define DEC_STATE_STOPPED 3
>> +#define DEC_STATE_SEEK 4
>> +#define DEC_STATE_DRAIN5
>> +#define DEC_STATE_DECODING 6
>> +#define DEC_STATE_DRC 7
>> +
>> /**
>> * struct venus_inst - holds per instance paramerters
>> *
>> @@ -241,6 +250,10 @@ struct venus_buffer {
>> * @colorspace:current color space
>> * @quantization: current quantization
>> * @xfer_func: current xfer function
>> + * @codec_state: current codec API state (see DEC/ENC_STATE_)
>> + * @reconf_wait: wait queue for resolution change event
>> + * @ten_bits: does new stream is 10bits depth
>> + * @buf_count: used to count number number of buffers (reqbuf(0))
>> * @fps: holds current FPS
>> * @timeperframe: holds current time per frame structure
>> * @fmt_out: a reference to output format structure
>> @@ -255,8 +268,6 @@ struct venus_buffer {
>> * @opb_buftype: output picture buffer type
>> * @opb_fmt: output picture buffer raw format
>> * @reconfig: a flag raised by decoder when the stream resolution changed
>> - * @reconfig_width:holds the new width
>> - * @reconfig_height: holds the new height
>> * @hfi_codec: current codec for this instance in HFI space
>> * @sequence_cap: a sequence counter for capture queue
>> * @sequence_out: a sequence counter for output queue
>> @@ -296,6 +307,9 @@ struct venus_inst {
>> u8 ycbcr_enc;
>> u8 quantization;
>> u8 xfer_func;
>> + unsigned int codec_state;
>> + wait_queue_head_t reconf_wait;
>> + int buf_count;
>> u64 fps;
>> struct v4l2_fract timeperframe;
>> const struct venus_format *fmt_out;
>> @@ -310,8 +324,6 @@ struct venus_inst {
>> u32 opb_buftype;
>> u32 opb_fmt;
>> bool reconfig;
>> - u32 reconfig_width;
>> - u32 reconfig_height;
>> u32 hfi_codec;
>> u32 sequence_cap;
>> u32 sequence_out;
>> diff --git a/drivers/media/platform/qcom/venus/helpers.c
>> b/drivers/media/platform/qcom/venus/helpers.c
>> index 637ce7b82d94..25d8cceccae4 100644
>> --- a/drivers/media/platform/qcom/venus/helpers.c
>> +++ b/drivers/media/platform/qcom/venus/helpers.c
>> @@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer
>> *vb)
>>
>> v4l2_m2m_buf_queue(m2m_ctx, vbuf);
>>
>> - if (!(inst->streamon_out & inst->streamon_cap))
>> - goto unlock;
>> -
>> - ret = is_buf_refed(inst, vbuf);
>> - if (ret)
>> - goto unlock;
>> + if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) {
>
> Wouldn't a simple vb2_is_streaming() work here?
I'd say no, because the buffer can be queued but the streaming on that
queue isn't started yet. The idea is to send buffers to firmware only
when the streaming is on on that queue, otherwise we only queue the
buffer in m2m_buf_queue (and send for processing once the streaming on
that particular queue is started).
>
>> + ret = is_buf_refed(inst, vbuf);
>> + if (ret)
>> + goto unlock;
>>
>> - ret = session_process_buf(inst, vbuf);
>> - if (ret)
>> - return_buf_error(inst, vbuf);
>> + ret = session_process_buf(inst, vbuf);
>> + if (ret)
>> +
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
.Hi Stan,
On Fri, Jan 18, 2019 at 1:21 AM Stanimir Varbanov
wrote:
>
> This refactored code for start/stop streaming vb2 operations and
> adds a state machine handling similar to the one in stateful codec
> API documentation. One major change is that now the HFI session is
> started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0),
> during that time streamoff(cap,out) just flush buffers but doesn't
> stop the session. The other major change is that now the capture
> and output queues are completely separated.
>
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/core.h| 20 +-
> drivers/media/platform/qcom/venus/helpers.c | 23 +-
> drivers/media/platform/qcom/venus/helpers.h | 5 +
> drivers/media/platform/qcom/venus/vdec.c| 449
> 4 files changed, 389 insertions(+), 108 deletions(-)
>
Thanks for the patch! Please see some comments inline.
> diff --git a/drivers/media/platform/qcom/venus/core.h
> b/drivers/media/platform/qcom/venus/core.h
> index 79c7e816c706..5a133c203455 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -218,6 +218,15 @@ struct venus_buffer {
>
> #define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer, vb)
>
> +#define DEC_STATE_UNINIT 0
> +#define DEC_STATE_INIT 1
> +#define DEC_STATE_CAPTURE_SETUP2
> +#define DEC_STATE_STOPPED 3
> +#define DEC_STATE_SEEK 4
> +#define DEC_STATE_DRAIN5
> +#define DEC_STATE_DECODING 6
> +#define DEC_STATE_DRC 7
> +
> /**
> * struct venus_inst - holds per instance paramerters
> *
> @@ -241,6 +250,10 @@ struct venus_buffer {
> * @colorspace:current color space
> * @quantization: current quantization
> * @xfer_func: current xfer function
> + * @codec_state: current codec API state (see DEC/ENC_STATE_)
> + * @reconf_wait: wait queue for resolution change event
> + * @ten_bits: does new stream is 10bits depth
> + * @buf_count: used to count number number of buffers (reqbuf(0))
> * @fps: holds current FPS
> * @timeperframe: holds current time per frame structure
> * @fmt_out: a reference to output format structure
> @@ -255,8 +268,6 @@ struct venus_buffer {
> * @opb_buftype: output picture buffer type
> * @opb_fmt: output picture buffer raw format
> * @reconfig: a flag raised by decoder when the stream resolution changed
> - * @reconfig_width:holds the new width
> - * @reconfig_height: holds the new height
> * @hfi_codec: current codec for this instance in HFI space
> * @sequence_cap: a sequence counter for capture queue
> * @sequence_out: a sequence counter for output queue
> @@ -296,6 +307,9 @@ struct venus_inst {
> u8 ycbcr_enc;
> u8 quantization;
> u8 xfer_func;
> + unsigned int codec_state;
> + wait_queue_head_t reconf_wait;
> + int buf_count;
> u64 fps;
> struct v4l2_fract timeperframe;
> const struct venus_format *fmt_out;
> @@ -310,8 +324,6 @@ struct venus_inst {
> u32 opb_buftype;
> u32 opb_fmt;
> bool reconfig;
> - u32 reconfig_width;
> - u32 reconfig_height;
> u32 hfi_codec;
> u32 sequence_cap;
> u32 sequence_out;
> diff --git a/drivers/media/platform/qcom/venus/helpers.c
> b/drivers/media/platform/qcom/venus/helpers.c
> index 637ce7b82d94..25d8cceccae4 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb)
>
> v4l2_m2m_buf_queue(m2m_ctx, vbuf);
>
> - if (!(inst->streamon_out & inst->streamon_cap))
> - goto unlock;
> -
> - ret = is_buf_refed(inst, vbuf);
> - if (ret)
> - goto unlock;
> + if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) {
Wouldn't a simple vb2_is_streaming() work here?
> + ret = is_buf_refed(inst, vbuf);
> + if (ret)
> + goto unlock;
>
> - ret = session_process_buf(inst, vbuf);
> - if (ret)
> - return_buf_error(inst, vbuf);
> + ret = session_process_buf(inst, vbuf);
> + if (ret)
> + return_buf_error(inst, vbuf);
> + }
The driver is handing the buffer to the firmware directly, bypassing
the m2m helpers. What's the purpose for using those helpers then? (Or
the reason not to do this instead in the device_run m2m callback?)
>
> unlock:
> mutex_unlock(>lock);
> @@ -1155,14 +1154,8 @@ int venus_helper_vb2_start_streaming(struct venus_inst
> *inst)
> if (ret)
> goto err_unload_res;
>
> - ret = venus_helper_queue_dpb_bufs(inst);
> -
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On Thu, Jan 24, 2019 at 9:34 PM Stanimir Varbanov
wrote:
>
> Hi Alex,
>
> Thanks for the comments!
>
> On 1/24/19 10:44 AM, Alexandre Courbot wrote:
> > On Fri, Jan 18, 2019 at 1:21 AM Stanimir Varbanov
> > wrote:
> >>
> >> This refactored code for start/stop streaming vb2 operations and
> >
> > s/refactored/refactors?
>
> Ack.
>
> >
> >> adds a state machine handling similar to the one in stateful codec
> >> API documentation. One major change is that now the HFI session is
> >> started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0),
> >> during that time streamoff(cap,out) just flush buffers but doesn't
> >
> > streamoff(cap,out) should probably be in capitals for consistency.
>
> OK.
>
> >
> >> stop the session. The other major change is that now the capture
> >> and output queues are completely separated.
> >>
> >> Signed-off-by: Stanimir Varbanov
> >> ---
> >> drivers/media/platform/qcom/venus/core.h| 20 +-
> >> drivers/media/platform/qcom/venus/helpers.c | 23 +-
> >> drivers/media/platform/qcom/venus/helpers.h | 5 +
> >> drivers/media/platform/qcom/venus/vdec.c| 449
> >> 4 files changed, 389 insertions(+), 108 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/core.h
> >> b/drivers/media/platform/qcom/venus/core.h
> >> index 79c7e816c706..5a133c203455 100644
> >> --- a/drivers/media/platform/qcom/venus/core.h
> >> +++ b/drivers/media/platform/qcom/venus/core.h
> >> @@ -218,6 +218,15 @@ struct venus_buffer {
> >>
> >> #define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer, vb)
> >>
> >> +#define DEC_STATE_UNINIT 0
> >
> > Not sure about "uninit", DEC_STATE_DEINIT may be more explicit here?
>
> I don't have strong opinion on that, so I will change it.
>
> >
> >> +#define DEC_STATE_INIT 1
> >> +#define DEC_STATE_CAPTURE_SETUP2
> >> +#define DEC_STATE_STOPPED 3
> >> +#define DEC_STATE_SEEK 4
> >> +#define DEC_STATE_DRAIN5
> >> +#define DEC_STATE_DECODING 6
> >> +#define DEC_STATE_DRC 7
> >
> > How about defining these as an enum, for better type safety? I'd also
> > prefix with VENUS_ to avoid possible (if unlikely) name collisions.
>
> OK.
>
> >
> >> +
> >> /**
> >> * struct venus_inst - holds per instance paramerters
> >> *
> >> @@ -241,6 +250,10 @@ struct venus_buffer {
> >> * @colorspace:current color space
> >> * @quantization: current quantization
> >> * @xfer_func: current xfer function
> >> + * @codec_state: current codec API state (see DEC/ENC_STATE_)
> >> + * @reconf_wait: wait queue for resolution change event
> >> + * @ten_bits: does new stream is 10bits depth
> >
> > "is new stream 10 bits deep" maybe?
>
> that is better description, but it should be in this patch (I have made
> 10bits support but didn't included in that initial stateful codec patch).
>
> >
> >> + * @buf_count: used to count number number of buffers (reqbuf(0))
> >
> > "number" written twice here.
>
> OK.
>
> >
> >> * @fps: holds current FPS
> >> * @timeperframe: holds current time per frame structure
> >> * @fmt_out: a reference to output format structure
> >> @@ -255,8 +268,6 @@ struct venus_buffer {
> >> * @opb_buftype: output picture buffer type
> >> * @opb_fmt: output picture buffer raw format
> >> * @reconfig: a flag raised by decoder when the stream resolution changed
> >> - * @reconfig_width:holds the new width
> >> - * @reconfig_height: holds the new height
> >> * @hfi_codec: current codec for this instance in HFI space
> >> * @sequence_cap: a sequence counter for capture queue
> >> * @sequence_out: a sequence counter for output queue
> >> @@ -296,6 +307,9 @@ struct venus_inst {
> >> u8 ycbcr_enc;
> >> u8 quantization;
> >> u8 xfer_func;
> >> + unsigned int codec_state;
> >
> > As mentioned above, with an enum the type of this member would make it
> > obvious which values it can accept.
> >
> >> + wait_queue_head_t reconf_wait;
> >> + int buf_count;
> >> u64 fps;
> >> struct v4l2_fract timeperframe;
> >> const struct venus_format *fmt_out;
> >> @@ -310,8 +324,6 @@ struct venus_inst {
> >> u32 opb_buftype;
> >> u32 opb_fmt;
> >> bool reconfig;
> >> - u32 reconfig_width;
> >> - u32 reconfig_height;
> >> u32 hfi_codec;
> >> u32 sequence_cap;
> >> u32 sequence_out;
> >> diff --git a/drivers/media/platform/qcom/venus/helpers.c
> >> b/drivers/media/platform/qcom/venus/helpers.c
> >> index 637ce7b82d94..25d8cceccae4 100644
> >> --- a/drivers/media/platform/qcom/venus/helpers.c
> >> +++ b/drivers/media/platform/qcom/venus/helpers.c
> >> @@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer
> >> *vb)
> >>
> >>
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
Hi Alex,
Thanks for the comments!
On 1/24/19 10:44 AM, Alexandre Courbot wrote:
> On Fri, Jan 18, 2019 at 1:21 AM Stanimir Varbanov
> wrote:
>>
>> This refactored code for start/stop streaming vb2 operations and
>
> s/refactored/refactors?
Ack.
>
>> adds a state machine handling similar to the one in stateful codec
>> API documentation. One major change is that now the HFI session is
>> started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0),
>> during that time streamoff(cap,out) just flush buffers but doesn't
>
> streamoff(cap,out) should probably be in capitals for consistency.
OK.
>
>> stop the session. The other major change is that now the capture
>> and output queues are completely separated.
>>
>> Signed-off-by: Stanimir Varbanov
>> ---
>> drivers/media/platform/qcom/venus/core.h| 20 +-
>> drivers/media/platform/qcom/venus/helpers.c | 23 +-
>> drivers/media/platform/qcom/venus/helpers.h | 5 +
>> drivers/media/platform/qcom/venus/vdec.c| 449
>> 4 files changed, 389 insertions(+), 108 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.h
>> b/drivers/media/platform/qcom/venus/core.h
>> index 79c7e816c706..5a133c203455 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -218,6 +218,15 @@ struct venus_buffer {
>>
>> #define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer, vb)
>>
>> +#define DEC_STATE_UNINIT 0
>
> Not sure about "uninit", DEC_STATE_DEINIT may be more explicit here?
I don't have strong opinion on that, so I will change it.
>
>> +#define DEC_STATE_INIT 1
>> +#define DEC_STATE_CAPTURE_SETUP2
>> +#define DEC_STATE_STOPPED 3
>> +#define DEC_STATE_SEEK 4
>> +#define DEC_STATE_DRAIN5
>> +#define DEC_STATE_DECODING 6
>> +#define DEC_STATE_DRC 7
>
> How about defining these as an enum, for better type safety? I'd also
> prefix with VENUS_ to avoid possible (if unlikely) name collisions.
OK.
>
>> +
>> /**
>> * struct venus_inst - holds per instance paramerters
>> *
>> @@ -241,6 +250,10 @@ struct venus_buffer {
>> * @colorspace:current color space
>> * @quantization: current quantization
>> * @xfer_func: current xfer function
>> + * @codec_state: current codec API state (see DEC/ENC_STATE_)
>> + * @reconf_wait: wait queue for resolution change event
>> + * @ten_bits: does new stream is 10bits depth
>
> "is new stream 10 bits deep" maybe?
that is better description, but it should be in this patch (I have made
10bits support but didn't included in that initial stateful codec patch).
>
>> + * @buf_count: used to count number number of buffers (reqbuf(0))
>
> "number" written twice here.
OK.
>
>> * @fps: holds current FPS
>> * @timeperframe: holds current time per frame structure
>> * @fmt_out: a reference to output format structure
>> @@ -255,8 +268,6 @@ struct venus_buffer {
>> * @opb_buftype: output picture buffer type
>> * @opb_fmt: output picture buffer raw format
>> * @reconfig: a flag raised by decoder when the stream resolution changed
>> - * @reconfig_width:holds the new width
>> - * @reconfig_height: holds the new height
>> * @hfi_codec: current codec for this instance in HFI space
>> * @sequence_cap: a sequence counter for capture queue
>> * @sequence_out: a sequence counter for output queue
>> @@ -296,6 +307,9 @@ struct venus_inst {
>> u8 ycbcr_enc;
>> u8 quantization;
>> u8 xfer_func;
>> + unsigned int codec_state;
>
> As mentioned above, with an enum the type of this member would make it
> obvious which values it can accept.
>
>> + wait_queue_head_t reconf_wait;
>> + int buf_count;
>> u64 fps;
>> struct v4l2_fract timeperframe;
>> const struct venus_format *fmt_out;
>> @@ -310,8 +324,6 @@ struct venus_inst {
>> u32 opb_buftype;
>> u32 opb_fmt;
>> bool reconfig;
>> - u32 reconfig_width;
>> - u32 reconfig_height;
>> u32 hfi_codec;
>> u32 sequence_cap;
>> u32 sequence_out;
>> diff --git a/drivers/media/platform/qcom/venus/helpers.c
>> b/drivers/media/platform/qcom/venus/helpers.c
>> index 637ce7b82d94..25d8cceccae4 100644
>> --- a/drivers/media/platform/qcom/venus/helpers.c
>> +++ b/drivers/media/platform/qcom/venus/helpers.c
>> @@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer
>> *vb)
>>
>> v4l2_m2m_buf_queue(m2m_ctx, vbuf);
>>
>> - if (!(inst->streamon_out & inst->streamon_cap))
>> - goto unlock;
>> -
>> - ret = is_buf_refed(inst, vbuf);
>> - if (ret)
>> - goto unlock;
>> + if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) {
>> +
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
Hi Malathi,
On 1/21/19 1:20 PM, mgot...@codeaurora.org wrote:
> On 2019-01-17 21:50, Stanimir Varbanov wrote:
>> This refactored code for start/stop streaming vb2 operations and
>> adds a state machine handling similar to the one in stateful codec
>> API documentation. One major change is that now the HFI session is
>> started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0),
>> during that time streamoff(cap,out) just flush buffers but doesn't
>> stop the session. The other major change is that now the capture
>> and output queues are completely separated.
>>
>> Signed-off-by: Stanimir Varbanov
>> ---
>> drivers/media/platform/qcom/venus/core.h | 20 +-
>> drivers/media/platform/qcom/venus/helpers.c | 23 +-
>> drivers/media/platform/qcom/venus/helpers.h | 5 +
>> drivers/media/platform/qcom/venus/vdec.c | 449
>> 4 files changed, 389 insertions(+), 108 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.h
>> b/drivers/media/platform/qcom/venus/core.h
>> index 79c7e816c706..5a133c203455 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -218,6 +218,15 @@ struct venus_buffer {
>>
>> #define to_venus_buffer(ptr) container_of(ptr, struct
>> venus_buffer, vb)
>>
>> +#define DEC_STATE_UNINIT 0
>> +#define DEC_STATE_INIT 1
>> +#define DEC_STATE_CAPTURE_SETUP 2
>> +#define DEC_STATE_STOPPED 3
>> +#define DEC_STATE_SEEK 4
>> +#define DEC_STATE_DRAIN 5
>> +#define DEC_STATE_DECODING 6
>> +#define DEC_STATE_DRC 7
>> +
>> /**
>> * struct venus_inst - holds per instance paramerters
>> *
>> @@ -241,6 +250,10 @@ struct venus_buffer {
>> * @colorspace: current color space
>> * @quantization: current quantization
>> * @xfer_func: current xfer function
>> + * @codec_state: current codec API state (see DEC/ENC_STATE_)
>> + * @reconf_wait: wait queue for resolution change event
>> + * @ten_bits: does new stream is 10bits depth
>> + * @buf_count: used to count number number of buffers (reqbuf(0))
>> * @fps: holds current FPS
>> * @timeperframe: holds current time per frame structure
>> * @fmt_out: a reference to output format structure
>> @@ -255,8 +268,6 @@ struct venus_buffer {
>> * @opb_buftype: output picture buffer type
>> * @opb_fmt: output picture buffer raw format
>> * @reconfig: a flag raised by decoder when the stream resolution
>> changed
>> - * @reconfig_width: holds the new width
>> - * @reconfig_height: holds the new height
>> * @hfi_codec: current codec for this instance in HFI space
>> * @sequence_cap: a sequence counter for capture queue
>> * @sequence_out: a sequence counter for output queue
>> @@ -296,6 +307,9 @@ struct venus_inst {
>> u8 ycbcr_enc;
>> u8 quantization;
>> u8 xfer_func;
>> + unsigned int codec_state;
>> + wait_queue_head_t reconf_wait;
>> + int buf_count;
>> u64 fps;
>> struct v4l2_fract timeperframe;
>> const struct venus_format *fmt_out;
>> @@ -310,8 +324,6 @@ struct venus_inst {
>> u32 opb_buftype;
>> u32 opb_fmt;
>> bool reconfig;
>> - u32 reconfig_width;
>> - u32 reconfig_height;
>> u32 hfi_codec;
>> u32 sequence_cap;
>> u32 sequence_out;
>> diff --git a/drivers/media/platform/qcom/venus/helpers.c
>> b/drivers/media/platform/qcom/venus/helpers.c
>> index 637ce7b82d94..25d8cceccae4 100644
>> --- a/drivers/media/platform/qcom/venus/helpers.c
>> +++ b/drivers/media/platform/qcom/venus/helpers.c
>> @@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct
>> vb2_buffer *vb)
>>
>> v4l2_m2m_buf_queue(m2m_ctx, vbuf);
>>
>> - if (!(inst->streamon_out & inst->streamon_cap))
>> - goto unlock;
>> -
>> - ret = is_buf_refed(inst, vbuf);
>> - if (ret)
>> - goto unlock;
>> + if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) {
>> + ret = is_buf_refed(inst, vbuf);
>> + if (ret)
>> + goto unlock;
>>
>> - ret = session_process_buf(inst, vbuf);
>> - if (ret)
>> - return_buf_error(inst, vbuf);
>> + ret = session_process_buf(inst, vbuf);
>> + if (ret)
>> + return_buf_error(inst, vbuf);
>> + }
>>
>> unlock:
>> mutex_unlock(>lock);
>
> Hi Stan,
>
> In case of encoder, we are queuing buffers only after both planes are
> streamed on.
> As we don’t have any reconfig event in case of encoder,
> it’s better if we stick to the earlier implementation of queuing buffers.
>
> So I would recommend to add a check for the same in the below way :
>
> diff --git a/drivers/media/platform/qcom/venus/helpers.c
> b/drivers/media/platform/qcom/venus/helpers.c
> index 25d8cce..cc490fe2 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -1029,6 +1029,8 @@ void
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On Fri, Jan 18, 2019 at 1:21 AM Stanimir Varbanov
wrote:
>
> This refactored code for start/stop streaming vb2 operations and
s/refactored/refactors?
> adds a state machine handling similar to the one in stateful codec
> API documentation. One major change is that now the HFI session is
> started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0),
> during that time streamoff(cap,out) just flush buffers but doesn't
streamoff(cap,out) should probably be in capitals for consistency.
> stop the session. The other major change is that now the capture
> and output queues are completely separated.
>
> Signed-off-by: Stanimir Varbanov
> ---
> drivers/media/platform/qcom/venus/core.h| 20 +-
> drivers/media/platform/qcom/venus/helpers.c | 23 +-
> drivers/media/platform/qcom/venus/helpers.h | 5 +
> drivers/media/platform/qcom/venus/vdec.c| 449
> 4 files changed, 389 insertions(+), 108 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h
> b/drivers/media/platform/qcom/venus/core.h
> index 79c7e816c706..5a133c203455 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -218,6 +218,15 @@ struct venus_buffer {
>
> #define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer, vb)
>
> +#define DEC_STATE_UNINIT 0
Not sure about "uninit", DEC_STATE_DEINIT may be more explicit here?
> +#define DEC_STATE_INIT 1
> +#define DEC_STATE_CAPTURE_SETUP2
> +#define DEC_STATE_STOPPED 3
> +#define DEC_STATE_SEEK 4
> +#define DEC_STATE_DRAIN5
> +#define DEC_STATE_DECODING 6
> +#define DEC_STATE_DRC 7
How about defining these as an enum, for better type safety? I'd also
prefix with VENUS_ to avoid possible (if unlikely) name collisions.
> +
> /**
> * struct venus_inst - holds per instance paramerters
> *
> @@ -241,6 +250,10 @@ struct venus_buffer {
> * @colorspace:current color space
> * @quantization: current quantization
> * @xfer_func: current xfer function
> + * @codec_state: current codec API state (see DEC/ENC_STATE_)
> + * @reconf_wait: wait queue for resolution change event
> + * @ten_bits: does new stream is 10bits depth
"is new stream 10 bits deep" maybe?
> + * @buf_count: used to count number number of buffers (reqbuf(0))
"number" written twice here.
> * @fps: holds current FPS
> * @timeperframe: holds current time per frame structure
> * @fmt_out: a reference to output format structure
> @@ -255,8 +268,6 @@ struct venus_buffer {
> * @opb_buftype: output picture buffer type
> * @opb_fmt: output picture buffer raw format
> * @reconfig: a flag raised by decoder when the stream resolution changed
> - * @reconfig_width:holds the new width
> - * @reconfig_height: holds the new height
> * @hfi_codec: current codec for this instance in HFI space
> * @sequence_cap: a sequence counter for capture queue
> * @sequence_out: a sequence counter for output queue
> @@ -296,6 +307,9 @@ struct venus_inst {
> u8 ycbcr_enc;
> u8 quantization;
> u8 xfer_func;
> + unsigned int codec_state;
As mentioned above, with an enum the type of this member would make it
obvious which values it can accept.
> + wait_queue_head_t reconf_wait;
> + int buf_count;
> u64 fps;
> struct v4l2_fract timeperframe;
> const struct venus_format *fmt_out;
> @@ -310,8 +324,6 @@ struct venus_inst {
> u32 opb_buftype;
> u32 opb_fmt;
> bool reconfig;
> - u32 reconfig_width;
> - u32 reconfig_height;
> u32 hfi_codec;
> u32 sequence_cap;
> u32 sequence_out;
> diff --git a/drivers/media/platform/qcom/venus/helpers.c
> b/drivers/media/platform/qcom/venus/helpers.c
> index 637ce7b82d94..25d8cceccae4 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb)
>
> v4l2_m2m_buf_queue(m2m_ctx, vbuf);
>
> - if (!(inst->streamon_out & inst->streamon_cap))
> - goto unlock;
> -
> - ret = is_buf_refed(inst, vbuf);
> - if (ret)
> - goto unlock;
> + if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) {
> + ret = is_buf_refed(inst, vbuf);
> + if (ret)
> + goto unlock;
>
> - ret = session_process_buf(inst, vbuf);
> - if (ret)
> - return_buf_error(inst, vbuf);
> + ret = session_process_buf(inst, vbuf);
> + if (ret)
> + return_buf_error(inst, vbuf);
> + }
>
> unlock:
> mutex_unlock(>lock);
> @@ -1155,14 +1154,8 @@ int
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On 2019-01-21 16:50, mgot...@codeaurora.org wrote:
On 2019-01-17 21:50, Stanimir Varbanov wrote:
This refactored code for start/stop streaming vb2 operations and
adds a state machine handling similar to the one in stateful codec
API documentation. One major change is that now the HFI session is
started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0),
during that time streamoff(cap,out) just flush buffers but doesn't
stop the session. The other major change is that now the capture
and output queues are completely separated.
Signed-off-by: Stanimir Varbanov
---
drivers/media/platform/qcom/venus/core.h| 20 +-
drivers/media/platform/qcom/venus/helpers.c | 23 +-
drivers/media/platform/qcom/venus/helpers.h | 5 +
drivers/media/platform/qcom/venus/vdec.c| 449
4 files changed, 389 insertions(+), 108 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.h
b/drivers/media/platform/qcom/venus/core.h
index 79c7e816c706..5a133c203455 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -218,6 +218,15 @@ struct venus_buffer {
#define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer,
vb)
+#define DEC_STATE_UNINIT 0
+#define DEC_STATE_INIT 1
+#define DEC_STATE_CAPTURE_SETUP2
+#define DEC_STATE_STOPPED 3
+#define DEC_STATE_SEEK 4
+#define DEC_STATE_DRAIN5
+#define DEC_STATE_DECODING 6
+#define DEC_STATE_DRC 7
+
/**
* struct venus_inst - holds per instance paramerters
*
@@ -241,6 +250,10 @@ struct venus_buffer {
* @colorspace:current color space
* @quantization: current quantization
* @xfer_func: current xfer function
+ * @codec_state: current codec API state (see DEC/ENC_STATE_)
+ * @reconf_wait: wait queue for resolution change event
+ * @ten_bits: does new stream is 10bits depth
+ * @buf_count: used to count number number of buffers (reqbuf(0))
* @fps: holds current FPS
* @timeperframe: holds current time per frame structure
* @fmt_out: a reference to output format structure
@@ -255,8 +268,6 @@ struct venus_buffer {
* @opb_buftype: output picture buffer type
* @opb_fmt: output picture buffer raw format
* @reconfig: a flag raised by decoder when the stream resolution
changed
- * @reconfig_width:holds the new width
- * @reconfig_height: holds the new height
* @hfi_codec: current codec for this instance in HFI space
* @sequence_cap: a sequence counter for capture queue
* @sequence_out: a sequence counter for output queue
@@ -296,6 +307,9 @@ struct venus_inst {
u8 ycbcr_enc;
u8 quantization;
u8 xfer_func;
+ unsigned int codec_state;
+ wait_queue_head_t reconf_wait;
+ int buf_count;
u64 fps;
struct v4l2_fract timeperframe;
const struct venus_format *fmt_out;
@@ -310,8 +324,6 @@ struct venus_inst {
u32 opb_buftype;
u32 opb_fmt;
bool reconfig;
- u32 reconfig_width;
- u32 reconfig_height;
u32 hfi_codec;
u32 sequence_cap;
u32 sequence_out;
diff --git a/drivers/media/platform/qcom/venus/helpers.c
b/drivers/media/platform/qcom/venus/helpers.c
index 637ce7b82d94..25d8cceccae4 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct
vb2_buffer *vb)
v4l2_m2m_buf_queue(m2m_ctx, vbuf);
- if (!(inst->streamon_out & inst->streamon_cap))
- goto unlock;
-
- ret = is_buf_refed(inst, vbuf);
- if (ret)
- goto unlock;
+ if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) {
+ ret = is_buf_refed(inst, vbuf);
+ if (ret)
+ goto unlock;
- ret = session_process_buf(inst, vbuf);
- if (ret)
- return_buf_error(inst, vbuf);
+ ret = session_process_buf(inst, vbuf);
+ if (ret)
+ return_buf_error(inst, vbuf);
+ }
unlock:
mutex_unlock(>lock);
Hi Stan,
In case of encoder, we are queuing buffers only after both planes are
streamed on.
As we don’t have any reconfig event in case of encoder,
it’s better if we stick to the earlier implementation of queuing
buffers.
So I would recommend to add a check for the same in the below way :
diff --git a/drivers/media/platform/qcom/venus/helpers.c
b/drivers/media/platform/qcom/venus/helpers.c
index 25d8cce..cc490fe2 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -1029,6 +1029,8 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer
*vb)
mutex_lock(>lock);
v4l2_m2m_buf_queue(m2m_ctx, vbuf);
+ if
Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
On 2019-01-17 21:50, Stanimir Varbanov wrote:
This refactored code for start/stop streaming vb2 operations and
adds a state machine handling similar to the one in stateful codec
API documentation. One major change is that now the HFI session is
started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0),
during that time streamoff(cap,out) just flush buffers but doesn't
stop the session. The other major change is that now the capture
and output queues are completely separated.
Signed-off-by: Stanimir Varbanov
---
drivers/media/platform/qcom/venus/core.h| 20 +-
drivers/media/platform/qcom/venus/helpers.c | 23 +-
drivers/media/platform/qcom/venus/helpers.h | 5 +
drivers/media/platform/qcom/venus/vdec.c| 449
4 files changed, 389 insertions(+), 108 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.h
b/drivers/media/platform/qcom/venus/core.h
index 79c7e816c706..5a133c203455 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -218,6 +218,15 @@ struct venus_buffer {
#define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer,
vb)
+#define DEC_STATE_UNINIT 0
+#define DEC_STATE_INIT 1
+#define DEC_STATE_CAPTURE_SETUP2
+#define DEC_STATE_STOPPED 3
+#define DEC_STATE_SEEK 4
+#define DEC_STATE_DRAIN5
+#define DEC_STATE_DECODING 6
+#define DEC_STATE_DRC 7
+
/**
* struct venus_inst - holds per instance paramerters
*
@@ -241,6 +250,10 @@ struct venus_buffer {
* @colorspace:current color space
* @quantization: current quantization
* @xfer_func: current xfer function
+ * @codec_state: current codec API state (see DEC/ENC_STATE_)
+ * @reconf_wait: wait queue for resolution change event
+ * @ten_bits: does new stream is 10bits depth
+ * @buf_count: used to count number number of buffers (reqbuf(0))
* @fps: holds current FPS
* @timeperframe: holds current time per frame structure
* @fmt_out: a reference to output format structure
@@ -255,8 +268,6 @@ struct venus_buffer {
* @opb_buftype: output picture buffer type
* @opb_fmt: output picture buffer raw format
* @reconfig: a flag raised by decoder when the stream resolution
changed
- * @reconfig_width:holds the new width
- * @reconfig_height: holds the new height
* @hfi_codec: current codec for this instance in HFI space
* @sequence_cap: a sequence counter for capture queue
* @sequence_out: a sequence counter for output queue
@@ -296,6 +307,9 @@ struct venus_inst {
u8 ycbcr_enc;
u8 quantization;
u8 xfer_func;
+ unsigned int codec_state;
+ wait_queue_head_t reconf_wait;
+ int buf_count;
u64 fps;
struct v4l2_fract timeperframe;
const struct venus_format *fmt_out;
@@ -310,8 +324,6 @@ struct venus_inst {
u32 opb_buftype;
u32 opb_fmt;
bool reconfig;
- u32 reconfig_width;
- u32 reconfig_height;
u32 hfi_codec;
u32 sequence_cap;
u32 sequence_out;
diff --git a/drivers/media/platform/qcom/venus/helpers.c
b/drivers/media/platform/qcom/venus/helpers.c
index 637ce7b82d94..25d8cceccae4 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct
vb2_buffer *vb)
v4l2_m2m_buf_queue(m2m_ctx, vbuf);
- if (!(inst->streamon_out & inst->streamon_cap))
- goto unlock;
-
- ret = is_buf_refed(inst, vbuf);
- if (ret)
- goto unlock;
+ if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) {
+ ret = is_buf_refed(inst, vbuf);
+ if (ret)
+ goto unlock;
- ret = session_process_buf(inst, vbuf);
- if (ret)
- return_buf_error(inst, vbuf);
+ ret = session_process_buf(inst, vbuf);
+ if (ret)
+ return_buf_error(inst, vbuf);
+ }
unlock:
mutex_unlock(>lock);
Hi Stan,
In case of encoder, we are queuing buffers only after both planes are
streamed on.
As we don’t have any reconfig event in case of encoder,
it’s better if we stick to the earlier implementation of queuing
buffers.
So I would recommend to add a check for the same in the below way :
diff --git a/drivers/media/platform/qcom/venus/helpers.c
b/drivers/media/platform/qcom/venus/helpers.c
index 25d8cce..cc490fe2 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -1029,6 +1029,8 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer
*vb)
mutex_lock(>lock);
v4l2_m2m_buf_queue(m2m_ctx, vbuf);
+ if (inst->session_type == VIDC_SESSION_TYPE_ENC &&
[PATCH 10/10] venus: dec: make decoder compliant with stateful codec API
This refactored code for start/stop streaming vb2 operations and
adds a state machine handling similar to the one in stateful codec
API documentation. One major change is that now the HFI session is
started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0),
during that time streamoff(cap,out) just flush buffers but doesn't
stop the session. The other major change is that now the capture
and output queues are completely separated.
Signed-off-by: Stanimir Varbanov
---
drivers/media/platform/qcom/venus/core.h| 20 +-
drivers/media/platform/qcom/venus/helpers.c | 23 +-
drivers/media/platform/qcom/venus/helpers.h | 5 +
drivers/media/platform/qcom/venus/vdec.c| 449
4 files changed, 389 insertions(+), 108 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.h
b/drivers/media/platform/qcom/venus/core.h
index 79c7e816c706..5a133c203455 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -218,6 +218,15 @@ struct venus_buffer {
#define to_venus_buffer(ptr) container_of(ptr, struct venus_buffer, vb)
+#define DEC_STATE_UNINIT 0
+#define DEC_STATE_INIT 1
+#define DEC_STATE_CAPTURE_SETUP2
+#define DEC_STATE_STOPPED 3
+#define DEC_STATE_SEEK 4
+#define DEC_STATE_DRAIN5
+#define DEC_STATE_DECODING 6
+#define DEC_STATE_DRC 7
+
/**
* struct venus_inst - holds per instance paramerters
*
@@ -241,6 +250,10 @@ struct venus_buffer {
* @colorspace:current color space
* @quantization: current quantization
* @xfer_func: current xfer function
+ * @codec_state: current codec API state (see DEC/ENC_STATE_)
+ * @reconf_wait: wait queue for resolution change event
+ * @ten_bits: does new stream is 10bits depth
+ * @buf_count: used to count number number of buffers (reqbuf(0))
* @fps: holds current FPS
* @timeperframe: holds current time per frame structure
* @fmt_out: a reference to output format structure
@@ -255,8 +268,6 @@ struct venus_buffer {
* @opb_buftype: output picture buffer type
* @opb_fmt: output picture buffer raw format
* @reconfig: a flag raised by decoder when the stream resolution changed
- * @reconfig_width:holds the new width
- * @reconfig_height: holds the new height
* @hfi_codec: current codec for this instance in HFI space
* @sequence_cap: a sequence counter for capture queue
* @sequence_out: a sequence counter for output queue
@@ -296,6 +307,9 @@ struct venus_inst {
u8 ycbcr_enc;
u8 quantization;
u8 xfer_func;
+ unsigned int codec_state;
+ wait_queue_head_t reconf_wait;
+ int buf_count;
u64 fps;
struct v4l2_fract timeperframe;
const struct venus_format *fmt_out;
@@ -310,8 +324,6 @@ struct venus_inst {
u32 opb_buftype;
u32 opb_fmt;
bool reconfig;
- u32 reconfig_width;
- u32 reconfig_height;
u32 hfi_codec;
u32 sequence_cap;
u32 sequence_out;
diff --git a/drivers/media/platform/qcom/venus/helpers.c
b/drivers/media/platform/qcom/venus/helpers.c
index 637ce7b82d94..25d8cceccae4 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb)
v4l2_m2m_buf_queue(m2m_ctx, vbuf);
- if (!(inst->streamon_out & inst->streamon_cap))
- goto unlock;
-
- ret = is_buf_refed(inst, vbuf);
- if (ret)
- goto unlock;
+ if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) {
+ ret = is_buf_refed(inst, vbuf);
+ if (ret)
+ goto unlock;
- ret = session_process_buf(inst, vbuf);
- if (ret)
- return_buf_error(inst, vbuf);
+ ret = session_process_buf(inst, vbuf);
+ if (ret)
+ return_buf_error(inst, vbuf);
+ }
unlock:
mutex_unlock(>lock);
@@ -1155,14 +1154,8 @@ int venus_helper_vb2_start_streaming(struct venus_inst
*inst)
if (ret)
goto err_unload_res;
- ret = venus_helper_queue_dpb_bufs(inst);
- if (ret)
- goto err_session_stop;
-
return 0;
-err_session_stop:
- hfi_session_stop(inst);
err_unload_res:
hfi_session_unload_res(inst);
err_unreg_bufs:
diff --git a/drivers/media/platform/qcom/venus/helpers.h
b/drivers/media/platform/qcom/venus/helpers.h
index 2ec1c1a8b416..3b46139b5ee1 100644
--- a/drivers/media/platform/qcom/venus/helpers.h
+++ b/drivers/media/platform/qcom/venus/helpers.h
@@ -17,6 +17,11 @@
#include
+#define IS_OUT(q, inst) (inst->streamon_out && \
+ q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
