Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1493861 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver CK Hu <ck.hu@mediatek.com> - 2016-09-30 05:10 +0200
    Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver Horng-Shyang Liao <hs.liao@mediatek.com> - 2016-09-30 11:00 +0200
      Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver CK Hu <ck.hu@mediatek.com> - 2016-09-30 11:20 +0200
        Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver Horng-Shyang Liao <hs.liao@mediatek.com> - 2016-09-30 11:50 +0200
          Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver Horng-Shyang Liao <hs.liao@mediatek.com> - 2016-10-05 05:00 +0200
            Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver Jassi Brar <jaswinder.singh@linaro.org> - 2016-10-05 05:40 +0200
              Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver Horng-Shyang Liao <hs.liao@mediatek.com> - 2016-10-05 14:40 +0200
                Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver Jassi Brar <jaswinder.singh@linaro.org> - 2016-10-05 16:50 +0200
                  Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver Horng-Shyang Liao <hs.liao@mediatek.com> - 2016-10-06 15:10 +0200
                    Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver Jassi Brar <jaswinder.singh@linaro.org> - 2016-10-06 15:20 +0200
                      Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver Horng-Shyang Liao <hs.liao@mediatek.com> - 2016-10-11 04:50 +0200
                        Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver Jassi Brar <jaswinder.singh@linaro.org> - 2016-10-11 06:30 +0200

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

Hi, HS:

On Mon, 2016-09-05 at 09:44 +0800, HS Liao wrote:
> This patch is first version of Mediatek Command Queue(CMDQ) driver. The
> CMDQ is used to help write registers with critical time limitation,
> such as updating display configuration during the vblank. It controls
> Global Command Engine (GCE) hardware to achieve this requirement.
> Currently, CMDQ only supports display related hardwares, but we expect
> it can be extended to other hardwares for future requirements.
> 
> Signed-off-by: HS Liao <hs.liao@mediatek.com>
> Signed-off-by: CK Hu <ck.hu@mediatek.com>
> ---

[snip...]

> +
> +struct cmdq_task {
> +	struct cmdq		*cmdq;
> +	struct list_head	list_entry;
> +	void			*va_base;
> +	dma_addr_t		pa_base;
> +	size_t			cmd_buf_size; /* command occupied size */
> +	size_t			buf_size; /* real buffer size */
> +	bool			finalized;
> +	struct cmdq_thread	*thread;

I think thread info could be removed from cmdq_task. Only
cmdq_task_handle_error() and cmdq_task_insert_into_thread() use
task->thread and caller of both function has the thread info. So you
could just pass thread info into these two function and remove thread
info in cmdq_task.

> +	struct cmdq_task_cb	cb;

I think this callback function is equal to mailbox client tx_done
callback. It's better to use already-defined interface rather than
creating your own.

> +};
> +

[snip...]

> +
> +static int cmdq_suspend(struct device *dev)
> +{
> +	struct cmdq *cmdq = dev_get_drvdata(dev);
> +	struct cmdq_thread *thread;
> +	int i;
> +	bool task_running = false;
> +
> +	mutex_lock(&cmdq->task_mutex);
> +	cmdq->suspended = true;
> +	mutex_unlock(&cmdq->task_mutex);
> +
> +	for (i = 0; i < ARRAY_SIZE(cmdq->thread); i++) {
> +		thread = &cmdq->thread[i];
> +		if (!list_empty(&thread->task_busy_list)) {
> +			mod_timer(&thread->timeout, jiffies + 1);
> +			task_running = true;
> +		}
> +	}
> +
> +	if (task_running) {
> +		dev_warn(dev, "exist running task(s) in suspend\n");
> +		msleep(20);

Why sleep here? It looks like a recovery but could 20ms recovery
something? I think warning message is enough because you see the warning
message, and you fix the bug, so no need to recovery anything.

> +	}
> +
> +	clk_unprepare(cmdq->clock);
> +	return 0;
> +}
> +

Regards,
CK

[toc] | [next] | [standalone]

#1493965

Hi CK,

Please see my inline reply.

On Fri, 2016-09-30 at 11:06 +0800, CK Hu wrote:
> Hi, HS:
> 
> On Mon, 2016-09-05 at 09:44 +0800, HS Liao wrote:
> > This patch is first version of Mediatek Command Queue(CMDQ) driver. The
> > CMDQ is used to help write registers with critical time limitation,
> > such as updating display configuration during the vblank. It controls
> > Global Command Engine (GCE) hardware to achieve this requirement.
> > Currently, CMDQ only supports display related hardwares, but we expect
> > it can be extended to other hardwares for future requirements.
> > 
> > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > Signed-off-by: CK Hu <ck.hu@mediatek.com>
> > ---
> 
> [snip...]
> 
> > +
> > +struct cmdq_task {
> > +	struct cmdq		*cmdq;
> > +	struct list_head	list_entry;
> > +	void			*va_base;
> > +	dma_addr_t		pa_base;
> > +	size_t			cmd_buf_size; /* command occupied size */
> > +	size_t			buf_size; /* real buffer size */
> > +	bool			finalized;
> > +	struct cmdq_thread	*thread;
> 
> I think thread info could be removed from cmdq_task. Only
> cmdq_task_handle_error() and cmdq_task_insert_into_thread() use
> task->thread and caller of both function has the thread info. So you
> could just pass thread info into these two function and remove thread
> info in cmdq_task.

This modification will remove 1 pointer but add 2 pointers. Moreover,
more pointers will need to be delivered between functions for future
extension. IMHO, it would be better to keep thread pointer inside
cmdq_task.

> > +	struct cmdq_task_cb	cb;
> 
> I think this callback function is equal to mailbox client tx_done
> callback. It's better to use already-defined interface rather than
> creating your own.

This is because CMDQ driver allows different callback functions for
different tasks, but mailbox only allows one callback function per
channel. But, I think I can add a wrapper for tx_done to call CMDQ
callback functions. So, I will use tx_done in CMDQ v15.

> > +};
> > +
> 
> [snip...]
> 
> > +
> > +static int cmdq_suspend(struct device *dev)
> > +{
> > +	struct cmdq *cmdq = dev_get_drvdata(dev);
> > +	struct cmdq_thread *thread;
> > +	int i;
> > +	bool task_running = false;
> > +
> > +	mutex_lock(&cmdq->task_mutex);
> > +	cmdq->suspended = true;
> > +	mutex_unlock(&cmdq->task_mutex);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(cmdq->thread); i++) {
> > +		thread = &cmdq->thread[i];
> > +		if (!list_empty(&thread->task_busy_list)) {
> > +			mod_timer(&thread->timeout, jiffies + 1);
> > +			task_running = true;
> > +		}
> > +	}
> > +
> > +	if (task_running) {
> > +		dev_warn(dev, "exist running task(s) in suspend\n");
> > +		msleep(20);
> 
> Why sleep here? It looks like a recovery but could 20ms recovery
> something? I think warning message is enough because you see the warning
> message, and you fix the bug, so no need to recovery anything.

My purpose is context switch to finish timer's work.
I will replace it by schedule().

> > +	}
> > +
> > +	clk_unprepare(cmdq->clock);
> > +	return 0;
> > +}
> > +
> 
> Regards,
> CK

Thanks,
HS

[toc] | [prev] | [next] | [standalone]

#1493982

Hi, HS:

One comment inline

On Fri, 2016-09-30 at 16:56 +0800, Horng-Shyang Liao wrote:
> Hi CK,
> 
> Please see my inline reply.
> 
> On Fri, 2016-09-30 at 11:06 +0800, CK Hu wrote:
> > Hi, HS:
> > 
> > On Mon, 2016-09-05 at 09:44 +0800, HS Liao wrote:
> > > This patch is first version of Mediatek Command Queue(CMDQ) driver. The
> > > CMDQ is used to help write registers with critical time limitation,
> > > such as updating display configuration during the vblank. It controls
> > > Global Command Engine (GCE) hardware to achieve this requirement.
> > > Currently, CMDQ only supports display related hardwares, but we expect
> > > it can be extended to other hardwares for future requirements.
> > > 
> > > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > > Signed-off-by: CK Hu <ck.hu@mediatek.com>
> > > ---
> > 
> > [snip...]
> > 
> > > +
> > > +struct cmdq_task {
> > > +	struct cmdq		*cmdq;
> > > +	struct list_head	list_entry;
> > > +	void			*va_base;
> > > +	dma_addr_t		pa_base;
> > > +	size_t			cmd_buf_size; /* command occupied size */
> > > +	size_t			buf_size; /* real buffer size */
> > > +	bool			finalized;
> > > +	struct cmdq_thread	*thread;
> > 
> > I think thread info could be removed from cmdq_task. Only
> > cmdq_task_handle_error() and cmdq_task_insert_into_thread() use
> > task->thread and caller of both function has the thread info. So you
> > could just pass thread info into these two function and remove thread
> > info in cmdq_task.
> 
> This modification will remove 1 pointer but add 2 pointers. Moreover,
> more pointers will need to be delivered between functions for future
> extension. IMHO, it would be better to keep thread pointer inside
> cmdq_task.
> 
> > > +	struct cmdq_task_cb	cb;
> > 
> > I think this callback function is equal to mailbox client tx_done
> > callback. It's better to use already-defined interface rather than
> > creating your own.
> 
> This is because CMDQ driver allows different callback functions for
> different tasks, but mailbox only allows one callback function per
> channel. But, I think I can add a wrapper for tx_done to call CMDQ
> callback functions. So, I will use tx_done in CMDQ v15.

Up to now, one callback function for one channel is enough for DRM. So
'different callback function for different sent-message' looks like an
advanced function. Maybe you should not include it in first patch. 

Regards,
CK

> 
> > > +};
> > > +
> > 
> > [snip...]
> > 
> > > +
> > > +static int cmdq_suspend(struct device *dev)
> > > +{
> > > +	struct cmdq *cmdq = dev_get_drvdata(dev);
> > > +	struct cmdq_thread *thread;
> > > +	int i;
> > > +	bool task_running = false;
> > > +
> > > +	mutex_lock(&cmdq->task_mutex);
> > > +	cmdq->suspended = true;
> > > +	mutex_unlock(&cmdq->task_mutex);
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(cmdq->thread); i++) {
> > > +		thread = &cmdq->thread[i];
> > > +		if (!list_empty(&thread->task_busy_list)) {
> > > +			mod_timer(&thread->timeout, jiffies + 1);
> > > +			task_running = true;
> > > +		}
> > > +	}
> > > +
> > > +	if (task_running) {
> > > +		dev_warn(dev, "exist running task(s) in suspend\n");
> > > +		msleep(20);
> > 
> > Why sleep here? It looks like a recovery but could 20ms recovery
> > something? I think warning message is enough because you see the warning
> > message, and you fix the bug, so no need to recovery anything.
> 
> My purpose is context switch to finish timer's work.
> I will replace it by schedule().
> 
> > > +	}
> > > +
> > > +	clk_unprepare(cmdq->clock);
> > > +	return 0;
> > > +}
> > > +
> > 
> > Regards,
> > CK
> 
> Thanks,
> HS
> 
> 

[toc] | [prev] | [next] | [standalone]

#1493998

On Fri, 2016-09-30 at 17:11 +0800, CK Hu wrote:
> Hi, HS:
> 
> One comment inline
> 
> On Fri, 2016-09-30 at 16:56 +0800, Horng-Shyang Liao wrote:
> > Hi CK,
> > 
> > Please see my inline reply.
> > 
> > On Fri, 2016-09-30 at 11:06 +0800, CK Hu wrote:
> > > Hi, HS:
> > > 
> > > On Mon, 2016-09-05 at 09:44 +0800, HS Liao wrote:
> > > > This patch is first version of Mediatek Command Queue(CMDQ) driver. The
> > > > CMDQ is used to help write registers with critical time limitation,
> > > > such as updating display configuration during the vblank. It controls
> > > > Global Command Engine (GCE) hardware to achieve this requirement.
> > > > Currently, CMDQ only supports display related hardwares, but we expect
> > > > it can be extended to other hardwares for future requirements.
> > > > 
> > > > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > > > Signed-off-by: CK Hu <ck.hu@mediatek.com>
> > > > ---
> > > 
> > > [snip...]
> > > 
> > > > +
> > > > +struct cmdq_task {
> > > > +	struct cmdq		*cmdq;
> > > > +	struct list_head	list_entry;
> > > > +	void			*va_base;
> > > > +	dma_addr_t		pa_base;
> > > > +	size_t			cmd_buf_size; /* command occupied size */
> > > > +	size_t			buf_size; /* real buffer size */
> > > > +	bool			finalized;
> > > > +	struct cmdq_thread	*thread;
> > > 
> > > I think thread info could be removed from cmdq_task. Only
> > > cmdq_task_handle_error() and cmdq_task_insert_into_thread() use
> > > task->thread and caller of both function has the thread info. So you
> > > could just pass thread info into these two function and remove thread
> > > info in cmdq_task.
> > 
> > This modification will remove 1 pointer but add 2 pointers. Moreover,
> > more pointers will need to be delivered between functions for future
> > extension. IMHO, it would be better to keep thread pointer inside
> > cmdq_task.
> > 
> > > > +	struct cmdq_task_cb	cb;
> > > 
> > > I think this callback function is equal to mailbox client tx_done
> > > callback. It's better to use already-defined interface rather than
> > > creating your own.
> > 
> > This is because CMDQ driver allows different callback functions for
> > different tasks, but mailbox only allows one callback function per
> > channel. But, I think I can add a wrapper for tx_done to call CMDQ
> > callback functions. So, I will use tx_done in CMDQ v15.
> 
> Up to now, one callback function for one channel is enough for DRM. So
> 'different callback function for different sent-message' looks like an
> advanced function. Maybe you should not include it in first patch. 
> 
> Regards,
> CK

Hi CK,

OK. I will do it.

Thanks,
HS

[snip...]

[toc] | [prev] | [next] | [standalone]

#1495676

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:
> > Hi, HS:
> > 
> > One comment inline
> > 
> > On Fri, 2016-09-30 at 16:56 +0800, Horng-Shyang Liao wrote:
> > > Hi CK,
> > > 
> > > Please see my inline reply.
> > > 
> > > On Fri, 2016-09-30 at 11:06 +0800, CK Hu wrote:
> > > > Hi, HS:
> > > > 
> > > > On Mon, 2016-09-05 at 09:44 +0800, HS Liao wrote:
> > > > > This patch is first version of Mediatek Command Queue(CMDQ) driver. The
> > > > > CMDQ is used to help write registers with critical time limitation,
> > > > > such as updating display configuration during the vblank. It controls
> > > > > Global Command Engine (GCE) hardware to achieve this requirement.
> > > > > Currently, CMDQ only supports display related hardwares, but we expect
> > > > > it can be extended to other hardwares for future requirements.
> > > > > 
> > > > > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > > > > Signed-off-by: CK Hu <ck.hu@mediatek.com>
> > > > > ---
> > > > 
> > > > [snip...]
> > > > 
> > > > > +
> > > > > +struct cmdq_task {
> > > > > +	struct cmdq		*cmdq;
> > > > > +	struct list_head	list_entry;
> > > > > +	void			*va_base;
> > > > > +	dma_addr_t		pa_base;
> > > > > +	size_t			cmd_buf_size; /* command occupied size */
> > > > > +	size_t			buf_size; /* real buffer size */
> > > > > +	bool			finalized;
> > > > > +	struct cmdq_thread	*thread;
> > > > 
> > > > I think thread info could be removed from cmdq_task. Only
> > > > cmdq_task_handle_error() and cmdq_task_insert_into_thread() use
> > > > task->thread and caller of both function has the thread info. So you
> > > > could just pass thread info into these two function and remove thread
> > > > info in cmdq_task.
> > > 
> > > This modification will remove 1 pointer but add 2 pointers. Moreover,
> > > more pointers will need to be delivered between functions for future
> > > extension. IMHO, it would be better to keep thread pointer inside
> > > cmdq_task.
> > > 
> > > > > +	struct cmdq_task_cb	cb;
> > > > 
> > > > I think this callback function is equal to mailbox client tx_done
> > > > callback. It's better to use already-defined interface rather than
> > > > creating your own.
> > > 
> > > This is because CMDQ driver allows different callback functions for
> > > different tasks, but mailbox only allows one callback function per
> > > channel. But, I think I can add a wrapper for tx_done to call CMDQ
> > > callback functions. So, I will use tx_done in CMDQ v15.
> > 
> > Up to now, one callback function for one channel is enough for DRM. So
> > 'different callback function for different sent-message' looks like an
> > advanced function. Maybe you should not include it in first patch. 
> > 
> > Regards,
> > CK
> 
> Hi CK,
> 
> OK. I will do it.
> 
> Thanks,
> HS
> 
> [snip...]


Hi CK,

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.

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?

Thanks,
HS


Hi Jassi,

Do you have any suggestion about previous situation?

Thanks,
HS

[toc] | [prev] | [next] | [standalone]

#1495686

On 5 October 2016 at 08:24, Horng-Shyang Liao <hs.liao@mediatek.com> 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.

[toc] | [prev] | [next] | [standalone]

#1495850

On Wed, 2016-10-05 at 09:07 +0530, Jassi Brar wrote:
> On 5 October 2016 at 08:24, Horng-Shyang Liao <hs.liao@mediatek.com> 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.

CMDQ needs to call callback function to notice clients which tasks are
done. In my previous e-mail, I mentioned that rx_callback may be an
alternative solution. However, it seems to violate the design of
mailbox. Therefore, I think mailbox may not have a good solution for
CMDQ callback currently. IMHO, the better way is to use CMDQ self
callback for now.

Thanks,
HS

[toc] | [prev] | [next] | [standalone]

#1495903

On 5 October 2016 at 18:01, Horng-Shyang Liao <hs.liao@mediatek.com> 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@mediatek.com> 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.

[toc] | [prev] | [next] | [standalone]

#1496642

On Wed, 2016-10-05 at 20:13 +0530, Jassi Brar wrote:
> On 5 October 2016 at 18:01, Horng-Shyang Liao <hs.liao@mediatek.com> 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@mediatek.com> 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

[toc] | [prev] | [next] | [standalone]

#1496647

On 6 October 2016 at 18:31, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:

> 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.
>
mbox_client_txdone() is called by the client driver when only it knows
the messages has been transmitted (i.e your submitted tasks are done).
Obviously the client driver should do any callbacks to its users
upstream.

[toc] | [prev] | [next] | [standalone]

#1498585

On Thu, 2016-10-06 at 18:40 +0530, Jassi Brar wrote:
> On 6 October 2016 at 18:31, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> 
> > 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.
> >
> mbox_client_txdone() is called by the client driver when only it knows
> the messages has been transmitted (i.e your submitted tasks are done).
> Obviously the client driver should do any callbacks to its users
> upstream.

Hi Jassi,

In current CMDQ driver, mbox_client_txdone() is called to prevent the
blocking of chan->active_req. It is not the real point of CMDQ task
done, so the client driver cannot do any callbacks to its user upstream.

(1) If we don't use mbox_client_txdone(), could you tell us an
    alternative way to prevent the blocking of chan->active_req?
    And then we can use tx_done when CMDQ task is relly done.
(2) If we use mbox_client_txdone() to prevent the blocking of
    chan->active_req, could CMDQ driver just uses self-defined callback
    function to notice client driver CMDQ task done?

Thanks,
HS

[toc] | [prev] | [next] | [standalone]

#1498601

On 11 October 2016 at 08:10, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> On Thu, 2016-10-06 at 18:40 +0530, Jassi Brar wrote:
>> On 6 October 2016 at 18:31, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
>>
>> > 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.
>> >
>> mbox_client_txdone() is called by the client driver when only it knows
>> the messages has been transmitted (i.e your submitted tasks are done).
>> Obviously the client driver should do any callbacks to its users
>> upstream.
>
> Hi Jassi,
>
> In current CMDQ driver, mbox_client_txdone() is called to prevent the
> blocking of chan->active_req. It is not the real point of CMDQ task
> done, so the client driver cannot do any callbacks to its user upstream.
>
> (1) If we don't use mbox_client_txdone(), could you tell us an
>     alternative way to prevent the blocking of chan->active_req?
>     And then we can use tx_done when CMDQ task is relly done.
>
mbox_client_txdone() should be used only when the mailbox controller
driver can't figure when the TX is done. Client driver (by like some
reply packet) realises the TX is done (for the reply to have arrived).

 If your hardware does flag/irq when tx is done, you should prefer
that over mbox_client_txdone().


> (2) If we use mbox_client_txdone() to prevent the blocking of
>     chan->active_req, could CMDQ driver just uses self-defined callback
>     function to notice client driver CMDQ task done?
>
Anything above the mailbox api, is none of its business. Your platform
specific 'server' driver can implement its own callbacks to notify its
users.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web