Re: [PATCH v3 0/4] Performance improvement of decoder

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 12536 bytes --]

Hi Jackson,

Le jeudi 11 septembre 2025 à 02:04 +0000, jackson.lee a écrit :
> Hi Nicolas
> 
> > -----Original Message-----
> > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Sent: Wednesday, September 10, 2025 9:57 PM
> > To: jackson.lee <jackson.lee@chipsnmedia.com>; mchehab@kernel.org;
> > hverkuil-cisco@xs4all.nl; bob.beckett@collabora.com
> > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; lafley.kim
> > <lafley.kim@chipsnmedia.com>; b-brnich@ti.com; hverkuil@xs4all.nl; Nas
> > Chung <nas.chung@chipsnmedia.com>
> > Subject: Re: [PATCH v3 0/4] Performance improvement of decoder
> > 
> > Hi Jackson,
> > 
> > Le mercredi 10 septembre 2025 à 06:59 +0000, jackson.lee a écrit :
> > [...]
> > 
> > > I have reproduced the stall problem, I can see it with the latest
> > Gstreamer version.
> > > The root cause is we checked an incorrect return value while flushing,
> > so in spite of not finished flushing, the checking loop if the flushing
> > was finished was exited.
> > > When stop streaming was called and the instance queue count was 1,  the
> > checking function put infinite loop, so the stall problem happened.
> > > 
> > > The below patch should be needed.
> > > 
> > > diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> > > b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> > > index edbe69540ef1..2e0128cd0e4d 100644
> > > --- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> > > +++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> > > @@ -52,6 +52,7 @@ int wave5_vpu_init_with_bitcode(struct device *dev,
> > > u8 *bitcode, size_t size)
> > >  int wave5_vpu_flush_instance(struct vpu_instance *inst)
> > >  {
> > >         int ret = 0;
> > > +       int mutex_ret = 0;
> > >         int retry = 0;
> > > 
> > >         ret = mutex_lock_interruptible(&inst->dev->hw_lock);
> > > @@ -80,9 +81,9 @@ int wave5_vpu_flush_instance(struct vpu_instance
> > > *inst)
> > > 
> > >                         mutex_unlock(&inst->dev->hw_lock);
> > >                         wave5_vpu_dec_get_output_info(inst,
> > > &dec_info);
> > > -                       ret =
> > > mutex_lock_interruptible(&inst->dev->hw_lock);
> > > -                       if (ret)
> > > -                               return ret;
> > > +                       mutex_ret =
> > > +mutex_lock_interruptible(&inst->dev->hw_lock);
> > > +                       if (mutex_ret)
> > > +                               return mutex_ret;
> > >                         if (dec_info.index_frame_display > 0)
> > >                                 wave5_vpu_dec_set_disp_flag(inst,
> > > dec_info.index_frame_display);
> > >                 }
> > 
> > Good catch, unfortunately it does not completely fix the problem for me.
> > You can find a the end of this message the patch I actually tested. Note
> > I ,ove the mutex_ret in a close scope, and fixed other occurence of this
> > pattern, except one that I highlighted to you with a FIXME.
> > 
> > Some new information, I had this trace from GStreamer when the bug occured
> > on forward seeks (very rare):
> 
> " gst-play-1.0 --audiosink=fakesink --videosink=fakevideosink long-drc.h264"
> 
> When the above command is executed, to do forward seeks, I can use "shift key + '+'key " this key combination?

Note that I saw your next message stating you have managed to reproduce, this is
great.

The +/- changes the playback speed, if you did not enable instant rate changes,
this will effectively cause a seek, so its quite likely you can reproduce with
that way too.

What I was using is left/right arrows. I would just seek forward (right arrow)
close to the end, and then press and hold the left arrow till it reaches
position 0. Rince and repeat.

regards,
Nicolas

> 
> Thanks
> Jackson
> 
> > 
> > ** (gst-play-1.0:604): WARNING **: 00:03:59.965: v4l2h264dec0: Too old
> > frames, bug in decoder -- please file a bug
> > 
> > [root@jacinto nicolas]# echo w > /proc/sysrq-trigger [  335.116289] sysrq:
> > Show Blocked State
> > [  335.120054] task:typefind:sink   state:D stack:0     pid:607   tgid:604
> > ppid:543    task_flags:0x40044c flags:0x00000019
> > [  335.131147] Call trace:
> > [  335.133584]  __switch_to+0xf0/0x1c0 (T) [  335.137442]
> > __schedule+0x35c/0x9bc [  335.140935]  schedule+0x34/0x110 [  335.144162]
> > schedule_timeout+0x80/0x104 [  335.148081]
> > wait_for_completion_timeout+0x74/0x158
> > [  335.152955]  wave5_vpu_wait_interrupt+0x28/0x60 [wave5] [  335.158252]
> > wave5_vpu_dec_stop_streaming+0x68/0x28c [wave5] [  335.163915]
> > __vb2_queue_cancel+0x2c/0x2d4 [videobuf2_common] [  335.169668]
> > vb2_core_queue_release+0x20/0x74 [videobuf2_common] [  335.175678]
> > vb2_queue_release+0x10/0x1c [videobuf2_v4l2] [  335.181081]
> > v4l2_m2m_ctx_release+0x20/0x40 [v4l2_mem2mem] [  335.186567]
> > wave5_vpu_release_device+0x44/0x150 [wave5] [  335.191879]
> > wave5_vpu_dec_release+0x20/0x2c [wave5] [  335.196841]
> > v4l2_release+0xb4/0xf0 [videodev] [  335.201709]  __fput+0xd0/0x2e0
> > [  335.205090]  ____fput+0x14/0x20 [  335.208468]  task_work_run+0x64/0xd4
> > [  335.212164]  do_exit+0x240/0x8e0 [  335.215552]  do_group_exit+0x30/0xa4
> > [  335.219177]  get_signal+0x790/0x860 [  335.222676]  do_signal+0x94/0x394
> > [  335.225986]  do_notify_resume+0xd0/0x14c [  335.229910]
> > el0_svc+0xe4/0xe8 [  335.232967]  el0t_64_sync_handler+0xa0/0xe4
> > [  335.237154]  el0t_64_sync+0x198/0x19c
> > 
> > regards,
> > Nicolas
> > 
> > ---
> > 
> > diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> > b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> > index edbe69540ef1e..2faca2eee41fe 100644
> > --- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> > +++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> > @@ -77,12 +77,13 @@ int wave5_vpu_flush_instance(struct vpu_instance *inst)
> >                         return -ETIMEDOUT;
> >                 } else if (ret == -EBUSY) {
> >                         struct dec_output_info dec_info;
> > +                       int ret_mutex;
> > 
> >                         mutex_unlock(&inst->dev->hw_lock);
> >                         wave5_vpu_dec_get_output_info(inst, &dec_info);
> > -                       ret = mutex_lock_interruptible(&inst->dev->hw_lock);
> > -                       if (ret)
> > -                               return ret;
> > +                       ret_mutex = mutex_lock_interruptible(&inst->dev-
> > > hw_lock);
> > +                       if (ret_mutex)
> > +                               return ret_mutex;
> >                         if (dec_info.index_frame_display > 0)
> >                                 wave5_vpu_dec_set_disp_flag(inst,
> > dec_info.index_frame_display);
> >                 }
> > @@ -222,6 +223,8 @@ int wave5_vpu_dec_close(struct vpu_instance *inst, u32
> > *fail_res)
> >         }
> > 
> >         do {
> > +               int ret_mutex;
> > +
> >                 ret = wave5_vpu_dec_finish_seq(inst, fail_res);
> >                 if (ret < 0 && *fail_res != WAVE5_SYSERR_VPU_STILL_RUNNING) {
> >                         dev_warn(inst->dev->dev, "dec_finish_seq timed out\n");
> > @@ -243,10 +246,10 @@ int wave5_vpu_dec_close(struct vpu_instance *inst,
> > u32 *fail_res)
> > 
> >                 mutex_unlock(&vpu_dev->hw_lock);
> >                 wave5_vpu_dec_get_output_info(inst, &dec_info);
> > -               ret = mutex_lock_interruptible(&vpu_dev->hw_lock);
> > -               if (ret) {
> > +               ret_mutex = mutex_lock_interruptible(&vpu_dev->hw_lock);
> > +               if (ret_mutex) {
> >                         pm_runtime_put_sync(inst->dev->dev);
> > -                       return ret;
> > +                       return ret_mutex;
> >                 }
> >         } while (ret != 0);
> > 
> > @@ -482,6 +485,7 @@ dma_addr_t wave5_vpu_dec_get_rd_ptr(struct
> > vpu_instance *inst)
> > 
> >         ret = mutex_lock_interruptible(&inst->dev->hw_lock);
> >         if (ret)
> > +               // FIXME this return type is wrong
> >                 return ret;
> > 
> >         rd_ptr = wave5_dec_get_rd_ptr(inst);
> > nicolas@whitebuilder:~/Sources/TI/jacinto/linux$ git diff diff --git
> > a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> > b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> > index edbe69540ef1e..2faca2eee41fe 100644
> > --- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> > +++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> > @@ -77,12 +77,13 @@ int wave5_vpu_flush_instance(struct vpu_instance *inst)
> >                         return -ETIMEDOUT;
> >                 } else if (ret == -EBUSY) {
> >                         struct dec_output_info dec_info;
> > +                       int ret_mutex;
> > 
> >                         mutex_unlock(&inst->dev->hw_lock);
> >                         wave5_vpu_dec_get_output_info(inst, &dec_info);
> > -                       ret = mutex_lock_interruptible(&inst->dev->hw_lock);
> > -                       if (ret)
> > -                               return ret;
> > +                       ret_mutex = mutex_lock_interruptible(&inst->dev-
> > > hw_lock);
> > +                       if (ret_mutex)
> > +                               return ret_mutex;
> >                         if (dec_info.index_frame_display > 0)
> >                                 wave5_vpu_dec_set_disp_flag(inst,
> > dec_info.index_frame_display);
> >                 }
> > @@ -222,6 +223,8 @@ int wave5_vpu_dec_close(struct vpu_instance *inst, u32
> > *fail_res)
> >         }
> > 
> >         do {
> > +               int ret_mutex;
> > +
> >                 ret = wave5_vpu_dec_finish_seq(inst, fail_res);
> >                 if (ret < 0 && *fail_res != WAVE5_SYSERR_VPU_STILL_RUNNING) {
> >                         dev_warn(inst->dev->dev, "dec_finish_seq timed out\n");
> > @@ -243,10 +246,10 @@ int wave5_vpu_dec_close(struct vpu_instance *inst,
> > u32 *fail_res)
> > 
> >                 mutex_unlock(&vpu_dev->hw_lock);
> >                 wave5_vpu_dec_get_output_info(inst, &dec_info);
> > -               ret = mutex_lock_interruptible(&vpu_dev->hw_lock);
> > -               if (ret) {
> > +               ret_mutex = mutex_lock_interruptible(&vpu_dev->hw_lock);
> > +               if (ret_mutex) {
> >                         pm_runtime_put_sync(inst->dev->dev);
> > -                       return ret;
> > +                       return ret_mutex;
> >                 }
> >         } while (ret != 0);
> > 
> > @@ -482,6 +485,7 @@ dma_addr_t wave5_vpu_dec_get_rd_ptr(struct
> > vpu_instance *inst)
> > 
> >         ret = mutex_lock_interruptible(&inst->dev->hw_lock);
> >         if (ret)
> > +               // FIXME this return type is wrong
> >                 return ret;
> > 
> >         rd_ptr = wave5_dec_get_rd_ptr(inst);

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]