Re: [PATCH v3 8/9] mailbox: mediatek: Add CMDQ secure mailbox driver

[CK Hu (胡俊光)]

Hi, Jason:

On Fri, 2023-12-22 at 12:52 +0800, Jason-JH.Lin wrote:
> To support secure video path feature, GCE have to read/write
> registgers
> in the secure world. GCE will enable the secure access permission to
> the
> HW who wants to access the secure content buffer.
> 
> Add CMDQ secure mailbox driver to make CMDQ client user is able to
> sending their HW settings to the secure world. So that GCE can
> execute
> all instructions to configure HW in the secure world.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@xxxxxxxxxxxx>
> ---

[snip]

> +
> +static int cmdq_sec_session_send(struct cmdq_sec_context *context,
> +				 struct cmdq_sec_task *sec_task, const
> u32 iwc_cmd,
> +				 const u32 thrd_idx, struct cmdq_sec
> *cmdq)
> +{
> +	int err = 0;
> +	u64 cost;
> +	struct iwc_cmdq_message_t *iwc_msg = NULL;
> +
> +	iwc_msg = (struct iwc_cmdq_message_t *)context->iwc_msg;
> +
> +	memset(iwc_msg, 0, sizeof(*iwc_msg));
> +	iwc_msg->cmd = iwc_cmd;
> +	iwc_msg->cmdq_id = cmdq->pdata->hwid;
> +	iwc_msg->command.thread = thrd_idx;
> +
> +	switch (iwc_cmd) {
> +	case CMD_CMDQ_IWC_SUBMIT_TASK:
> +		err = cmdq_sec_fill_iwc_msg(context, sec_task,
> thrd_idx);
> +		if (err)
> +			return err;
> +		break;
> +	case CMD_CMDQ_IWC_CANCEL_TASK:
> +		iwc_msg->cancel_task.wait_cookie = sec_task-
> >wait_cookie;
> +		iwc_msg->cancel_task.thread = thrd_idx;
> +		break;
> +	case CMD_CMDQ_IWC_PATH_RES_ALLOCATE:
> +		if (!cmdq->shared_mem || !cmdq->shared_mem->va) {
> +			dev_err(cmdq->mbox.dev, "%s %d: shared_mem is
> NULL", __func__, __LINE__);
> +			return -EFAULT;
> +		}
> +		iwc_msg->path_resource.size = cmdq->shared_mem->size;
> +		iwc_msg->path_resource.share_memoy_pa = cmdq-
> >shared_mem->pa;
> +		iwc_msg->path_resource.use_normal_irq = 1;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	cmdq->sec_invoke = sched_clock();
> +	dev_dbg(cmdq->mbox.dev, "%s execute cmdq:%p sec_task:%p
> command:%u thread:%u cookie:%d",
> +		__func__, cmdq, sec_task, iwc_cmd, thrd_idx,
> +		sec_task ? sec_task->wait_cookie : -1);
> +
> +	/* send message */
> +	err = cmdq_sec_execute_session(&context->tee_ctx, iwc_cmd,
> CMDQ_TIMEOUT_DEFAULT);
> +
> +	cmdq->sec_done = sched_clock();
> +	cost = div_u64(cmdq->sec_done - cmdq->sec_invoke, 1000000);
> +	if (cost >= CMDQ_TIMEOUT_DEFAULT)

Maybe for some client driver, 1 ms is too long, and for some client
driver 1 second is not long. So I think the timeout detection should be
done by client driver. And the execution time depend on the command
buffer generated by client driver, so only client driver has the
ability to debug the command buffer it generated. So it's not necessary
to detect timeout in cmdq driver.

Regards,
CK

> +		dev_err(cmdq->mbox.dev, "%s execute timeout cmdq:%p
> sec_task:%p cost:%lluus",
> +			__func__, cmdq, sec_task, cost);
> +	else
> +		dev_dbg(cmdq->mbox.dev, "%s execute done cmdq:%p
> sec_task:%p cost:%lluus",
> +			__func__, cmdq, sec_task, cost);
> +
> +	if (err)
> +		return err;
> +
> +	context->state = IWC_SES_ON_TRANSACTED;
> +	return 0;
> +}
> +