Hi Nicolas and Devarsh
> -----Original Message-----
> From: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
> Sent: Friday, June 21, 2024 12:24 AM
> To: Devarsh Thakkar <devarsht@xxxxxx>; jackson.lee
> <jackson.lee@xxxxxxxxxxxxxxx>; mchehab@xxxxxxxxxx;
> sebastian.fricke@xxxxxxxxxxxxx
> Cc: linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> hverkuil@xxxxxxxxx; Nas Chung <nas.chung@xxxxxxxxxxxxxxx>; lafley.kim
> <lafley.kim@xxxxxxxxxxxxxxx>; b-brnich@xxxxxx
> Subject: Re: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support runtime
> suspend/resume
>
> Le jeudi 20 juin 2024 à 20:22 +0530, Devarsh Thakkar a écrit :
> > Hi Jackson, Nicolas,
> >
> > On 20/06/24 19:33, Nicolas Dufresne wrote:
> > > Hi Jackson, Devarsh,
> > >
> > > Le mercredi 19 juin 2024 à 23:56 +0000, jackson.lee a écrit :
> > > > Hi Devarsh
> > > >
> > > > If there is no feeding bitstreams during encoding and decoding frames,
> then driver's status is switched to suspended automatically by autosuspend.
> > > > And if we don’t use autosuspend, it is very difficult for us to catch
> if there is feeding or not while working a pipeline.
> > > > So it is very efficient for managing power status.
> > > >
> > > > If the delay is very great value, we can adjust it.
> > >
> > > One way to resolve this, would be if someone share measurement of
> > > the suspend / resume cycle duration. With firmware (third party OS)
> > > like this, the cost and duration is few order of magnitude higher
> > > then with more basic ASIC like Hantro and other single function HW.
> > >
> > > Yet, 5s might be to much (but clearly safe), but getting two low may
> > > means that we suspect "between two frames", and if that happens, we
> > > may endup with various range of side effect, like reduce throughput
> > > due to suspend collisions, or even worse power footprint. Some lab
> > > testing to adjust the value will be needed, we have very little of that
> happening at the moment as I understood.
> > >
> >
> > Okay I see the intention here is that if there is a process holding
> > the vpu device handle and the input feed is stalled for some seconds
> > due to network delay or CPU throughput then after a specified timeout
> > say 5 seconds we want to suspend even if the process is still active
> > and holding the vpu device handle ? I agree then if we want to support
> > this feature a safer/slightly larger value is required to avoid
> > frequent suspend/resume due to network jitter or any other bottleneck and
> maybe 5s is a good value to start with.
> >
> > But if last instance is closed/stops streaming and there is no process
> > holding the device handle anymore then I think we should suspend
> > immediately without any delay.
>
> Our emails crossed each other, but see my explanation about gapless playback
> transiton, were userspace may destroy and create a new video session. I
> believe 5s is way too long to be honest.
>
I investigated why it takes 5 sec to go to suspend even if the last instance is closed
The reason is if autosuspend is used, timeout should happen to go to suspend even though the power.usage count is 0 after an instance is closed.
So I made the below modification, when the last instance is closed, autosuspend turns off, and when the first instance is opened, the autosuspend turns on, again.
When I tested with the change, it works well.
Can you review the below code?
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
index 1aa5b6788266..87932d1550ce 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -1714,6 +1714,8 @@ static int wave5_vpu_open_dec(struct file *filp)
struct vpu_device *dev = video_drvdata(filp);
struct vpu_instance *inst = NULL;
struct v4l2_m2m_ctx *m2m_ctx;
+ int inst_count = 0;
+ struct vpu_instance *inst_elm;
int ret = 0;
inst = kzalloc(sizeof(*inst), GFP_KERNEL);
@@ -1799,6 +1801,12 @@ static int wave5_vpu_open_dec(struct file *filp)
hrtimer_start(&dev->hrtimer, ns_to_ktime(dev->vpu_poll_interval * NSEC_PER_MSEC),
HRTIMER_MODE_REL_PINNED);
+ list_for_each_entry(inst_elm, &dev->instances, list)
+ inst_count++;
+
+ if (!inst_count)
+ pm_runtime_use_autosuspend(inst->dev->dev);
+
list_add_tail(&inst->list, &dev->instances);
mutex_unlock(&dev->dev_lock);
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
index b0911fef232f..05b83445c650 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
@@ -197,6 +197,8 @@ int wave5_vpu_dec_close(struct vpu_instance *inst, u32 *fail_res)
int retry = 0;
struct vpu_device *vpu_dev = inst->dev;
int i;
+ int inst_count = 0;
+ struct vpu_instance *inst_elm;
*fail_res = 0;
if (!inst->codec_info)
@@ -239,8 +241,14 @@ int wave5_vpu_dec_close(struct vpu_instance *inst, u32 *fail_res)
wave5_vdi_free_dma_memory(vpu_dev, &p_dec_info->vb_task);
unlock_and_return:
- mutex_unlock(&vpu_dev->hw_lock);
+ list_for_each_entry(inst_elm, &vpu_dev->instances, list)
+ inst_count++;
+
+ if (inst_count == 1)
+ pm_runtime_dont_use_autosuspend(inst->dev->dev);
+
pm_runtime_put_sync(inst->dev->dev);
+ mutex_unlock(&vpu_dev->hw_lock);
return ret;
}
> Nicolas
>
> >
> > Regards
> > Devarsh
