Hi, Tomasz:

On Thu, 2019-07-25 at 19:56 +0900, Tomasz Figa wrote:

Hi Jungo,

On Sun, Jul 21, 2019 at 11:18 AM Jungo Lin [off-list ref] wrote:
[snip]

quoted

quoted

quoted

+           wake_up_interruptible(&isp_ctx->composer_tx_thread.wq);
+           isp_ctx->composer_tx_thread.thread = NULL;
+   }
+
+   if (isp_ctx->composer_deinit_thread.thread) {
+           wake_up(&isp_ctx->composer_deinit_thread.wq);
+           isp_ctx->composer_deinit_thread.thread = NULL;
+   }
+   mutex_unlock(&isp_ctx->lock);
+
+   pm_runtime_put_sync(&p1_dev->pdev->dev);

No need to use the sync variant.

We don't get this point. If we will call pm_runtime_get_sync in
mtk_isp_hw_init function, will we need to call
pm_runtime_put_sync_autosuspend in mtk_isp_hw_release in next patch?
As we know, we should call runtime pm functions in pair.

My point is that pm_runtime_put_sync() is only needed if one wants the
runtime count to be decremented after the function returns. Normally
there is no need to do so and one would call pm_runtime_put(), or if
autosuspend is used, pm_runtime_put_autosuspend() (note there is no
"sync" in the name).

[snip]

Ok, got your point.
We will change to use pm_runtime_put_autosuspend() which has ASYNC flag.

quoted

quoted

+static void isp_composer_handler(void *data, unsigned int len, void *priv)

quoted

+{
+   struct mtk_isp_p1_ctx *isp_ctx = (struct mtk_isp_p1_ctx *)priv;
+   struct isp_p1_device *p1_dev = p1_ctx_to_dev(isp_ctx);
+   struct device *dev = &p1_dev->pdev->dev;
+   struct mtk_isp_scp_p1_cmd *ipi_msg;
+
+   ipi_msg = (struct mtk_isp_scp_p1_cmd *)data;

Should we check that len == sizeof(*ipi_msg)? (Or at least >=, if data could
contain some extra bytes at the end.)

The len parameter is the actual sending bytes from SCP to kernel.
In the runtime, it is only 6 bytes for isp_ack_info command
However, sizeof(*ipi_msg) is large due to struct mtk_isp_scp_p1_cmd is
union structure.

That said we still should check if len is enough to cover the data
we're accessing below.

Ok, we will add the len checking before accessing the data.

quoted

quoted

quoted

+
+   if (ipi_msg->cmd_id != ISP_CMD_ACK)
+           return;
+
+   if (ipi_msg->ack_info.cmd_id == ISP_CMD_FRAME_ACK) {
+           dev_dbg(dev, "ack frame_num:%d",
+                   ipi_msg->ack_info.frame_seq_no);
+           atomic_set(&isp_ctx->composed_frame_id,
+                      ipi_msg->ack_info.frame_seq_no);

I suppose we are expecting here that ipi_msg->ack_info.frame_seq_no would be
just isp_ctx->composed_frame_id + 1, right? If not, we probably dropped some
frames and we should handle that somehow.

No, we use isp_ctx->composed_frame_id to save which frame sequence
number are composed done in SCP. In new design, we will move this from
isp_ctx to p1_dev.

But we compose the frames in order, don't we? Wouldn't every composed
frame would be just previous frame ID + 1?

[snip]

Yes, we compose the frames in order.
At the same time, we already increased "frame ID + 1" in
mtk_isp_req_enqueue() for each new request before sending to SCP for
composing. After receiving the ACK from SCP, we think the frame ID is
composed done and save by isp_ctx->composed_frame_id(v3).

[RFC v3]
void mtk_isp_req_enqueue(struct device *dev, struct media_request *req)
{
	...
	frameparams.frame_seq_no = isp_ctx->frame_seq_no++;

[RFC v4]
void mtk_isp_req_enqueue(struct mtk_cam_dev *cam,
			 struct mtk_cam_dev_request *req)
{
	struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(cam->dev);

	/* Accumulated frame sequence number */
	req->frame_params.frame_seq_no = ++p1_dev->enqueue_frame_seq_no;

 

quoted

quoted

quoted

+void isp_composer_hw_init(struct device *dev)
+{
+   struct mtk_isp_scp_p1_cmd composer_tx_cmd;
+   struct isp_p1_device *p1_dev = get_p1_device(dev);
+   struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
+
+   memset(&composer_tx_cmd, 0, sizeof(composer_tx_cmd));
+   composer_tx_cmd.cmd_id = ISP_CMD_INIT;
+   composer_tx_cmd.frameparam.hw_module = isp_ctx->isp_hw_module;
+   composer_tx_cmd.frameparam.cq_addr.iova = isp_ctx->scp_mem_iova;
+   composer_tx_cmd.frameparam.cq_addr.scp_addr = isp_ctx->scp_mem_pa;

Should we also specify the size of the buffer? Otherwise we could end up
with some undetectable overruns.

The size of SCP composer's memory is fixed to 0x200000.
Is it necessary to specify the size of this buffer?

#define MTK_ISP_COMPOSER_MEM_SIZE 0x200000

ptr = dma_alloc_coherent(p1_dev->cam_dev.smem_dev,
                        MTK_ISP_COMPOSER_MEM_SIZE, &addr, GFP_KERNEL);

Okay, but please add a comment saying that this is an implicit
requirement of the firmware.

Best regards,
Tomasz

Ok, we will add comments.

Best regards,


Jungo





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel