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 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.

> 
> 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.
I'll find out why it won't call cmdq_runtime_suspend() > 100ms.

Regards,
Jason-JH.Lin

> 
> -jassi
>