Re: [PATCH 2/2] mailbox: mtk-cmdq: Move pm_runimte_get and put to mbox_chan_ops API

["Jason-JH Lin (林睿祥)" <Jason-JH.Lin@mediatek.com>]

On Wed, 2024-07-10 at 22:47 -0500, Jassi Brar wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Wed, Jul 10, 2024 at 9:00 PM Jason-JH Lin (林睿祥)
> <Jason-JH.Lin@mediatek.com> wrote:
> >
> > On Fri, 2024-07-05 at 11:43 -0500, Jassi Brar wrote:
> > >
> > > External email : Please do not click links or open attachments
> until
> > > you have verified the sender or the content.
> > >  On Fri, Jul 5, 2024 at 1:11 AM Jason-JH Lin (林睿祥)
> > > <Jason-JH.Lin@mediatek.com> wrote:
> > > >
> > > > [snip]
> > > >
> > > > > You may disable one to make it easy to capture.
> > > > > Make sure your kernel prints have timestamps.
> > > > > Over your patchset, apply the following diff and execute your
> > > > > usecase.
> > > > > Then share
> > > > >  $ dmesg | grep Jason
> > > > >
> > > > > diff --git a/drivers/mailbox/mailbox.c
> > > b/drivers/mailbox/mailbox.c
> > > > > index bafcc7b0c0b8d..90c0620c0ae63 100644
> > > > > --- a/drivers/mailbox/mailbox.c
> > > > > +++ b/drivers/mailbox/mailbox.c
> > > > > @@ -284,6 +284,7 @@ int mbox_send_message(struct mbox_chan
> *chan,
> > > > > void *mssg)
> > > > >  {
> > > > >   int t;
> > > > >
> > > > > + printk("Jason %s: %p\n", __func__, (void *)chan);
> > > > >   if (!chan || !chan->cl)
> > > > >   return -EINVAL;
> > > > >
> > > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> > > > > b/drivers/mailbox/mtk-cmdq-mailbox.c
> > > > > index 02cef3eee35a5..6436e1b22f353 100644
> > > > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > > > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > > > > @@ -317,6 +317,7 @@ static int cmdq_runtime_resume(struct
> device
> > > > > *dev)
> > > > >  {
> > > > >   struct cmdq *cmdq = dev_get_drvdata(dev);
> > > > >
> > > > > + printk("Jason %s: %p\n", __func__, (void *)dev);
> > > > >   return clk_bulk_enable(cmdq->pdata->gce_num, cmdq->clocks);
> > > > >  }
> > > > >
> > > > > @@ -324,6 +325,7 @@ static int cmdq_runtime_suspend(struct
> device
> > > > > *dev)
> > > > >  {
> > > > >   struct cmdq *cmdq = dev_get_drvdata(dev);
> > > > >
> > > > > + printk("Jason %s: %p\n", __func__, (void *)dev);
> > > > >   clk_bulk_disable(cmdq->pdata->gce_num, cmdq->clocks);
> > > > >   return 0;
> > > > >  }
> > > > >
> > > > > Thanks
> > > >
> > > > It seems CMDQ driver is not suspend over 100ms.
> > > >
> > > here are the time deltas in milli-seconds
> > > 10 6005 52 85 304 59 27 203 29 24 25 38 33 57 34 20 41 28 36 48
> 71 35
> > > 21 45 51 22 30 46 46 23 16 16 16 16 16 17 17 31 16 19 31 16 16 17
> 16
> > > 16 16 32 17 32 17 30 21 16 32 18 30 28 25 29 24 31 27 16 17 19 34
> 30
> > > 15 33 16 35 35 29 18 31 16 16 16 16 17 33 35 33 39 20 21 14 34 49
> 22
> > > 42 32 13 21 24 9 130 23 32 35 31 32 16 18 31 36 37 26 14 68 76
> 111
> > > 515
> > > 452 64 484 487 497 501 507 161 336 516 486 495 504 495 504 501
> 500
> > > 495
> > > 507 495 499 505 496 502 501 502 500 495 501 507 499 499 496 506
> 501
> > > 498 507 490 504 496 501 506 413 28 207 31731 34550 3784 30 23
> > >
> > > For detlas <100 we don't expect suspend because your set
> > > autosuspend_delay to 100ms.
> > > For deltas >100 you should have seen suspend. Apparently the
> changes
> > > you want in the api still don't help you.
> >
> > I think "deltas > 100 ms not autosuspend" and "calling
> > pm_runtime_get_sync() in atomic context" are 2 different problems.
> >
> Fixing the first may avoid the need of the second on your platform.
> 
> > >
> > > Also I see messages are being submitted on 4 channels, but only
> for
> > > the  '000000000a4d37b5' channel cmdq_mbox_send_data is ever
> called.
> > > Unless you selectively printk only for that channel, your driver
> has
> > > some even fundamental problems.  Maybe trace without your patch
> and
> > > with pm_runtime_xxx commented out (to avoid stack dump).
> >
> > I've confirmed that autosuspend didn't work without these API
> patches.
> >
> You mean _with_ the api patches. right? Because I had suggested "Over
> your patchset, apply the following diff ..."

No matter __with__ or __without__ the api patches, autosuspend didn't
work...

We have to find out the reason __without__ the api patches, then see if
we still need the api patches to fix "calling sleep in atomic context"
problem.

Regards,
Jason-JH.Lin

> 
> cheers.