Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[Horng-Shyang Liao]

On Wed, 2016-10-05 at 20:13 +0530, Jassi Brar wrote:
> On 5 October 2016 at 18:01, Horng-Shyang Liao <hs.liao@xxxxxxxxxxxx> wrote:
> > On Wed, 2016-10-05 at 09:07 +0530, Jassi Brar wrote:
> >> On 5 October 2016 at 08:24, Horng-Shyang Liao <hs.liao@xxxxxxxxxxxx> wrote:
> >> > On Fri, 2016-09-30 at 17:47 +0800, Horng-Shyang Liao wrote:
> >> >> On Fri, 2016-09-30 at 17:11 +0800, CK Hu wrote:
> >>
> >> >
> >> > After I trace mailbox driver, I realize that CMDQ driver cannot use
> >> > tx_done.
> >> >
> >> > CMDQ clients will flush many tasks into CMDQ driver, and then CMDQ
> >> > driver will apply these tasks into GCE HW "immediately". These tasks,
> >> > which are queued in GCE HW, may not execute immediately since they
> >> > may need to wait event(s), e.g. vsync.
> >> >
> >> > However, in mailbox driver, mailbox uses a software buffer to queue
> >> > sent messages. It only sends next message until previous message is
> >> > done. This cannot fulfill CMDQ's requirement.
> >> >
> >> I understand
> >>  a) GCE HW can internally queue many tasks in some 'FIFO'
> >>  b) Execution of some task may have to wait until some external event
> >> occurs (like vsync)
> >>  c) GCE does not generate irq/flag for each task executed (?)
> >>
> >> If so, may be your tx_done should return 'true' so long as the GCE HW
> >> can accept tasks in its 'FIFO'. For mailbox api, any task that is
> >> queued on GCE, is assumed to be transmitted.
> >>
> >> > Quote some code from mailbox driver. Please notice "active_req" part.
> >> >
> >> > static void msg_submit(struct mbox_chan *chan)
> >> > {
> >> >         ...
> >> >         if (!chan->msg_count || chan->active_req)
> >> >                 goto exit;
> >> >         ...
> >> >         err = chan->mbox->ops->send_data(chan, data);
> >> >         if (!err) {
> >> >                 chan->active_req = data;
> >> >                 chan->msg_count--;
> >> >         }
> >> >         ...
> >> > }
> >> >
> >> > static void tx_tick(struct mbox_chan *chan, int r)
> >> > {
> >> >         ...
> >> >         spin_lock_irqsave(&chan->lock, flags);
> >> >         mssg = chan->active_req;
> >> >         chan->active_req = NULL;
> >> >         spin_unlock_irqrestore(&chan->lock, flags);
> >> >         ...
> >> > }
> >> >
> >> > Current workable CMDQ driver uses mbox_client_txdone() to prevent
> >> > this issue, and then uses self callback functions to handle done tasks.
> >> >
> >> > int cmdq_task_flush_async(struct cmdq_client *client, struct cmdq_task
> >> > *task, cmdq_async_flush_cb cb, void *data)
> >> > {
> >> >         ...
> >> >         mbox_send_message(client->chan, task);
> >> >         /* We can send next task immediately, so just call txdone. */
> >> >         mbox_client_txdone(client->chan, 0);
> >> >         ...
> >> > }
> >> >
> >> > Another solution is to use rx_callback; i.e. CMDQ mailbox controller
> >> > call mbox_chan_received_data() when CMDQ task is done. But, this may
> >> > violate the design of mailbox. What do you think?
> >> >
> >> If my point (c) above does not hold, maybe look at implementing
> >> tx_done() callback and submit next task from the callback of last
> >> done.
> >
> >
> > Hi Jassi,
> >
> > For point (c), GCE irq means 1~n tasks done or
> > 0~n tasks done + 1 task error.
> > In irq, we can know which tasks are done by register and GCE pc.
> >
> > As I mentioned before, we cannot submit next task after previous task
> > call tx_done. We need to submit multiple tasks to GCE HW immediately
> > and queue them in GCE HW.
> 
> > Let me explain this requirement by mouse
> > cursor example. User may move mouse quickly between two vsync, so DRM
> > may update display registers frequently. For CMDQ, that means many tasks
> > are flushed into CMDQ driver, and CMDQ driver needs to process all of
> > them in next vblank. Therefore, we cannot block any CMDQ task in SW
> > buffer.
> >
> We are interested only in the current position of cursor and not its
> trail. Also the current position should be updated at next vsync (and
> not the one after it).
> Going by this example, if the GCE HW can take in 'N' tasks at a time,
> then the N+1th submission should shift out (drop) the 1st task queued.
> So that at any time GCE HW has only the latest N tasks. Right?
> 
>  If yes, maybe you don't need to care about tx-done and simply keep
> shoving tasks as you generate them.
> 
>  If no, maybe your client driver need to emulate such a circular
> buffer where oldest task is overwritten by newest submission. And you
> submit the circular buffer (most relevant tasks) at one go to the GCE
> HW.


Hi Jassi,

CMDQ driver doesn't know the task type, so CMDQ cannot decide which
tasks can be dropped. So, I think the answer is "no".

Client driver is also hard to emulate a circular buffer where oldest
task is overwritten by newest submission. Let me illustrate with DRM
client (display driver). For DRM, the only way to know the latest
mouse cursor task before vsync is vsync interrupt. However, if we
keep latest mouse cursor task and start to send it to CMDQ after
vsync, the task may be executed outside the vblank and generate garbage
on screen in this case. (since we allow nested interrupt) We can wait
one more vsync to prevent garbage, but we will lose mouse cursor
performance. Therefore, our current solution is to flush all DRM tasks
into CMDQ, and queue all tasks in GCE HW. When GCE get vsync event, it
can finish all tasks within vblank to fulfill DRM's requirement.

Back to our original statement, we need to flush all tasks to queue
in GCE HW; i.e. we need to use mbox_client_txdone after
mbox_send_message, or send tx_done once mailbox controller receive
message (task). However, we still need a way to notice done tasks to
clients. Currently, we don't have a good way to call callback in mailbox
framework. Therefore, CMDQ driver has its owner callback functions.

Thanks,
HS