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

Hi Jassi,

On Mon, 2024-06-17 at 13:18 -0500, Jassi Brar wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Thu, Jun 13, 2024 at 11:01 PM Jason-JH.Lin <
> jason-jh.lin@mediatek.com> wrote:
> >
> > When we run kernel with lockdebug option, we will get the BUG
> below:
> >   BUG: sleeping function called from invalid context at
> drivers/base/power/runtime.c:1164
> >   in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 3616,
> name: kworker/u17:3
> >     preempt_count: 1, expected: 0
> >     RCU nest depth: 0, expected: 0
> >     INFO: lockdep is turned off.
> >     irq event stamp: 0
> >     CPU: 1 PID: 3616 Comm: kworker/u17:3 Not tainted 6.1.87-
> lockdep-14133-g26e933aca785 #1
> >     Hardware name: Google Ciri sku0/unprovisioned board (DT)
> >     Workqueue: imgsys_runner imgsys_runner_func
> >     Call trace:
> >      dump_backtrace+0x100/0x120
> >      show_stack+0x20/0x2c
> >      dump_stack_lvl+0x84/0xb4
> >      dump_stack+0x18/0x48
> >      __might_resched+0x354/0x4c0
> >      __might_sleep+0x98/0xe4
> >      __pm_runtime_resume+0x70/0x124
> >      cmdq_mbox_send_data+0xe4/0xb1c
> >      msg_submit+0x194/0x2dc
> >      mbox_send_message+0x190/0x330
> >      imgsys_cmdq_sendtask+0x1618/0x2224
> >      imgsys_runner_func+0xac/0x11c
> >      process_one_work+0x638/0xf84
> >      worker_thread+0x808/0xcd0
> >      kthread+0x24c/0x324
> >      ret_from_fork+0x10/0x20
> >
> > We found that there is a spin_lock_irqsave protection in
> msg_submit()
> > of mailbox.c and it is in the atomic context.
> > So when cmdq driver calls pm_runtime_get_sync() in
> cmdq_mbox_send_data(),
> > it will get this BUG report.
> >
> > To avoid using sleep in atomic context, move pm_runtime_get_sync to
> > mbox_chan_ops->power_get and also move pm_runtime_put_autosuspend
> to
> > mbox_chan_ops->power_put.
> >
> > Fixes: 8afe816b0c99 ("mailbox: mtk-cmdq-mailbox: Implement Runtime
> PM with autosuspend")
> 
> Can you please share the pattern of mailbox transfers on your
> platform?

I'm not familiar with imgsys driver, so I provide the mbox transfer
pattern of DRM driver in this scenario:

1. mbox_request_channel() in mtk_crtc.c: mtk_crtc_create()
    mtk_crtc->cmdq_client.client.dev = mtk_crtc->mmsys_dev;
    mtk_crtc->cmdq_client.client.tx_block = false;
    mtk_crtc->cmdq_client.client.knows_txdone = true;
    mtk_crtc->cmdq_client.client.rx_callback = ddp_cmdq_cb;
    mtk_crtc->cmdq_client.chan = mbox_request_channel(&client, i);

2. mbox_send_message() in mtk_crtc.c: mtk_crtc_update_config()
    mbox_flush(cmdq_client.chan, 2000);
    // prepare cmd to a buffer in cmdq_handle structure
    mbox_send_message(cmdq_client.chan, cmdq_handle);
    mbox_client_txdone(cmdq_client.chan, 0);

3. mbox_chan_received_data() in mtk-cmdq-mailbox.c: cmdq_task_done()
   // When CMDQ driver get an GCE IRQ from hardware means all cmd in
cmd buffer are executed

> As in, how often and long are the "channel idle" periods? And when
> active, how many transfers/sec ?
Is there any debug logs in mailbox can measure this?

This mailbox channel is use to configure display hardware in every
VSYNC, so the channel idle periods may be less than 16.66ms.

It should call rx_callback() before VACTIVE, but sometimes it will be
dropped by mbox_flush() if the new message is coming.

> I see every TX is acked by one RX packet. How long is the typical gap
> between a TX and its ack?

Typical gap between a TX and its ack is less than 16.66ms.


I'll address to imgsys part after confirming with imgsys owner.

Regards,
Jason-JH Lin
> 
> Thanks