| Message ID | 20190117162008.25217-1-stanimir.varbanov@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Venus stateful Codec API | expand |
Hi Stanimir, On Fri, Jan 18, 2019 at 1:20 AM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Hello, > > This aims to make Venus decoder compliant with stateful Codec API [1]. > The patches 1-9 are preparation for the cherry on the cake patch 10 > which implements the decoder state machine similar to the one in the > stateful codec API documentation. Thanks *a lot* for this series! I am still stress-testing it against the Chromium decoder tests, but so far it has been rock-solid. I have a few inline comments on some patches ; I will also want to review the state machine more thoroughly after refreshing my mind on Tomasz doc, but this looks pretty promising already. Cheers, Alex. > > There few things which are still TODO: > - V4L2_DEC_CMD_START implementation as per decoder documentation. > - Dynamic resolution change V4L2_BUF_FLAG_LAST for the last buffer > before the resolution change. > > The patches are tested with chromium VDA unittests at [2]. > > Note that the patchset depends on Venus various fixes at [3]. > > Comments are welcome! > > regards, > Stan > > [1] https://patchwork.kernel.org/patch/10652199/ > [2] https://chromium.googlesource.com/chromium/src/+/lkgr/docs/media/gpu/vdatest_usage.md > [3] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1894510.html > > Stanimir Varbanov (10): > venus: hfi_cmds: add more not-implemented properties > venus: helpers: fix dynamic buffer mode for v4 > venus: helpers: export few helper functions > venus: hfi: add type argument to hfi flush function > venus: hfi: export few HFI functions > venus: hfi: return an error if session_init is already called > venus: helpers: add three more helper functions > venus: vdec_ctrls: get real minimum buffers for capture > venus: vdec: allow bigger sizeimage set by clients > venus: dec: make decoder compliant with stateful codec API > > drivers/media/platform/qcom/venus/core.h | 20 +- > drivers/media/platform/qcom/venus/helpers.c | 141 +++++- > drivers/media/platform/qcom/venus/helpers.h | 14 + > drivers/media/platform/qcom/venus/hfi.c | 11 +- > drivers/media/platform/qcom/venus/hfi.h | 2 +- > drivers/media/platform/qcom/venus/hfi_cmds.c | 2 + > drivers/media/platform/qcom/venus/vdec.c | 467 ++++++++++++++---- > .../media/platform/qcom/venus/vdec_ctrls.c | 7 +- > 8 files changed, 535 insertions(+), 129 deletions(-) > > -- > 2.17.1 >
Hi Alex, Thank you for review and valuable comments! On 1/24/19 10:43 AM, Alexandre Courbot wrote: > Hi Stanimir, > > On Fri, Jan 18, 2019 at 1:20 AM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> Hello, >> >> This aims to make Venus decoder compliant with stateful Codec API [1]. >> The patches 1-9 are preparation for the cherry on the cake patch 10 >> which implements the decoder state machine similar to the one in the >> stateful codec API documentation. > > Thanks *a lot* for this series! I am still stress-testing it against > the Chromium decoder tests, but so far it has been rock-solid. I have > a few inline comments on some patches ; I will also want to review the > state machine more thoroughly after refreshing my mind on Tomasz doc, > but this looks pretty promising already. I'm expecting problems with ResetAfterFirstConfigInfo. I don't know why but this test case is very dirty. I'd appreciate any help to decipher what is the sequence of v4l2 calls made by this unittest case.
On Thu, Jan 24, 2019 at 7:13 PM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Hi Alex, > > Thank you for review and valuable comments! > > On 1/24/19 10:43 AM, Alexandre Courbot wrote: > > Hi Stanimir, > > > > On Fri, Jan 18, 2019 at 1:20 AM Stanimir Varbanov > > <stanimir.varbanov@linaro.org> wrote: > >> > >> Hello, > >> > >> This aims to make Venus decoder compliant with stateful Codec API [1]. > >> The patches 1-9 are preparation for the cherry on the cake patch 10 > >> which implements the decoder state machine similar to the one in the > >> stateful codec API documentation. > > > > Thanks *a lot* for this series! I am still stress-testing it against > > the Chromium decoder tests, but so far it has been rock-solid. I have > > a few inline comments on some patches ; I will also want to review the > > state machine more thoroughly after refreshing my mind on Tomasz doc, > > but this looks pretty promising already. > > I'm expecting problems with ResetAfterFirstConfigInfo. I don't know why > but this test case is very dirty. I'd appreciate any help to decipher > what is the sequence of v4l2 calls made by this unittest case. I did not see any issue with ResetAfterFirstConfigInfo, however ResourceExhaustion seems to hang once in a while. But I could already see this behavior with the older patchset. In any case I plan to thoroughly review the state machine. I agree it is a bit complex to grasp.
Hi Alex, On 1/25/19 7:34 AM, Alexandre Courbot wrote: > On Thu, Jan 24, 2019 at 7:13 PM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> Hi Alex, >> >> Thank you for review and valuable comments! >> >> On 1/24/19 10:43 AM, Alexandre Courbot wrote: >>> Hi Stanimir, >>> >>> On Fri, Jan 18, 2019 at 1:20 AM Stanimir Varbanov >>> <stanimir.varbanov@linaro.org> wrote: >>>> >>>> Hello, >>>> >>>> This aims to make Venus decoder compliant with stateful Codec API [1]. >>>> The patches 1-9 are preparation for the cherry on the cake patch 10 >>>> which implements the decoder state machine similar to the one in the >>>> stateful codec API documentation. >>> >>> Thanks *a lot* for this series! I am still stress-testing it against >>> the Chromium decoder tests, but so far it has been rock-solid. I have >>> a few inline comments on some patches ; I will also want to review the >>> state machine more thoroughly after refreshing my mind on Tomasz doc, >>> but this looks pretty promising already. >> >> I'm expecting problems with ResetAfterFirstConfigInfo. I don't know why >> but this test case is very dirty. I'd appreciate any help to decipher >> what is the sequence of v4l2 calls made by this unittest case. > > I did not see any issue with ResetAfterFirstConfigInfo, however > ResourceExhaustion seems to hang once in a while. But I could already > see this behavior with the older patchset. Is it hangs badly? > > In any case I plan to thoroughly review the state machine. I agree it > is a bit complex to grasp. yes the state machine isn't simple and I blamed Tomasz many times for that :)
On Fri, Jan 25, 2019 at 7:35 PM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Hi Alex, > > On 1/25/19 7:34 AM, Alexandre Courbot wrote: > > On Thu, Jan 24, 2019 at 7:13 PM Stanimir Varbanov > > <stanimir.varbanov@linaro.org> wrote: > >> > >> Hi Alex, > >> > >> Thank you for review and valuable comments! > >> > >> On 1/24/19 10:43 AM, Alexandre Courbot wrote: > >>> Hi Stanimir, > >>> > >>> On Fri, Jan 18, 2019 at 1:20 AM Stanimir Varbanov > >>> <stanimir.varbanov@linaro.org> wrote: > >>>> > >>>> Hello, > >>>> > >>>> This aims to make Venus decoder compliant with stateful Codec API [1]. > >>>> The patches 1-9 are preparation for the cherry on the cake patch 10 > >>>> which implements the decoder state machine similar to the one in the > >>>> stateful codec API documentation. > >>> > >>> Thanks *a lot* for this series! I am still stress-testing it against > >>> the Chromium decoder tests, but so far it has been rock-solid. I have > >>> a few inline comments on some patches ; I will also want to review the > >>> state machine more thoroughly after refreshing my mind on Tomasz doc, > >>> but this looks pretty promising already. > >> > >> I'm expecting problems with ResetAfterFirstConfigInfo. I don't know why > >> but this test case is very dirty. I'd appreciate any help to decipher > >> what is the sequence of v4l2 calls made by this unittest case. > > > > I did not see any issue with ResetAfterFirstConfigInfo, however > > ResourceExhaustion seems to hang once in a while. But I could already > > see this behavior with the older patchset. > > Is it hangs badly? It just stops processing, no crash. I need to investigate more closely. > > > > In any case I plan to thoroughly review the state machine. I agree it > > is a bit complex to grasp. > > yes the state machine isn't simple and I blamed Tomasz many times for > that :) If I feel inspired I may try to extract the useful part of your patch into a framework that we can use everywhere. :) We don't want every driver to reimplement this complex state machine and introduce new bugs every time.
On Mon, Jan 28, 2019 at 1:16 PM Alexandre Courbot <acourbot@chromium.org> wrote: > > On Fri, Jan 25, 2019 at 7:35 PM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: > > > > Hi Alex, > > > > On 1/25/19 7:34 AM, Alexandre Courbot wrote: > > > On Thu, Jan 24, 2019 at 7:13 PM Stanimir Varbanov > > > <stanimir.varbanov@linaro.org> wrote: > > >> > > >> Hi Alex, > > >> > > >> Thank you for review and valuable comments! > > >> > > >> On 1/24/19 10:43 AM, Alexandre Courbot wrote: > > >>> Hi Stanimir, > > >>> > > >>> On Fri, Jan 18, 2019 at 1:20 AM Stanimir Varbanov > > >>> <stanimir.varbanov@linaro.org> wrote: > > >>>> > > >>>> Hello, > > >>>> > > >>>> This aims to make Venus decoder compliant with stateful Codec API [1]. > > >>>> The patches 1-9 are preparation for the cherry on the cake patch 10 > > >>>> which implements the decoder state machine similar to the one in the > > >>>> stateful codec API documentation. > > >>> > > >>> Thanks *a lot* for this series! I am still stress-testing it against > > >>> the Chromium decoder tests, but so far it has been rock-solid. I have > > >>> a few inline comments on some patches ; I will also want to review the > > >>> state machine more thoroughly after refreshing my mind on Tomasz doc, > > >>> but this looks pretty promising already. > > >> > > >> I'm expecting problems with ResetAfterFirstConfigInfo. I don't know why > > >> but this test case is very dirty. I'd appreciate any help to decipher > > >> what is the sequence of v4l2 calls made by this unittest case. > > > > > > I did not see any issue with ResetAfterFirstConfigInfo, however > > > ResourceExhaustion seems to hang once in a while. But I could already > > > see this behavior with the older patchset. > > > > Is it hangs badly? > > It just stops processing, no crash. I need to investigate more closely. > > > > > > > In any case I plan to thoroughly review the state machine. I agree it > > > is a bit complex to grasp. > > > > yes the state machine isn't simple and I blamed Tomasz many times for > > that :) Don't shoot the messenger! I'm just carrying what hardware and userspace made for us. ;) > > If I feel inspired I may try to extract the useful part of your patch > into a framework that we can use everywhere. :) We don't want every > driver to reimplement this complex state machine and introduce new > bugs every time. Yeah, we definitely need such a framework, especially given how the m2m framework is overused for handling codecs. Best regards, Tomasz