Re: [PATCH v2 08/13] media: platform: mediatek: add isp_7x camsys unit

["CK Hu (胡俊光)" <ck.hu@mediatek.com>]

On Mon, 2025-07-07 at 09:31 +0800, shangyao lin wrote:
> From: "shangyao.lin" <shangyao.lin@mediatek.com>
> 
> Introduces the top media device driver for the MediaTek ISP7X CAMSYS. The driver maintains the camera system, including sub-device management, DMA operations, and integration with the V4L2 framework. It handles request stream data, buffer management, MediaTek-specific features, pipeline management, streaming control, and error handling mechanisms. Additionally, it aggregates sub-drivers for the camera interface, raw, and yuv pipelines.
> 
> ---

[snip]

> +struct mtk_cam_ctx *mtk_cam_start_ctx(struct mtk_cam_device *cam,
> +				      struct mtk_cam_video_device *node)
> +{
> +	struct mtk_cam_ctx *ctx = node->ctx;
> +	struct device *dev;
> +	struct v4l2_subdev **target_sd;
> +	int ret, i, is_first_ctx;
> +	struct media_entity *entity = &node->vdev.entity;
> +	struct media_graph graph;
> +
> +	dev_info(cam->dev, "%s:ctx(%d): triggered by %s\n",
> +		 __func__, ctx->stream_id, entity->name);
> +
> +	atomic_set(&ctx->enqueued_frame_seq_no, 0);
> +	ctx->composed_frame_seq_no = 0;
> +	ctx->dequeued_frame_seq_no = 0;
> +	atomic_set(&ctx->running_s_data_cnt, 0);
> +	init_completion(&ctx->session_complete);
> +
> +	is_first_ctx = !cam->composer_cnt;
> +	if (is_first_ctx) {
> +		spin_lock(&cam->dma_processing_lock);
> +		cam->dma_processing_count = 0;
> +		spin_unlock(&cam->dma_processing_lock);
> +		spin_lock(&cam->running_job_lock);
> +		cam->running_job_count = 0;
> +		spin_unlock(&cam->running_job_lock);
> +
> +		dev_info(cam->dev, "%s: power on camsys\n", __func__);
> +		ret = pm_runtime_resume_and_get(cam->dev);
> +		if (ret < 0) {
> +			dev_info(cam->dev, "%s: power on camsys failed\n",
> +				 __func__);
> +			return NULL;
> +		}
> +
> +		ret = isp_composer_init(cam);
> +		if (ret)
> +			goto fail_shutdown;
> +	}
> +	cam->composer_cnt++;
> +	if (is_yuv_node(node->desc.id))
> +		dev = cam->raw.yuvs[0];
> +	else
> +		dev = cam->raw.devs[0];
> +
> +	ret = mtk_cam_working_buf_pool_init(ctx, dev);
> +	if (ret) {
> +		dev_info(cam->dev, "failed to reserve DMA memory:%d\n", ret);
> +		goto fail_uninit_composer;
> +	}
> +
> +	kthread_init_worker(&ctx->sensor_worker);

You does not do anything to sensor_worker, so it's usedless.
Drop it.

> +	ctx->sensor_worker_task = kthread_run(kthread_worker_fn,
> +					      &ctx->sensor_worker,
> +					      "sensor_worker-%d",
> +					      ctx->stream_id);
> +	if (IS_ERR(ctx->sensor_worker_task)) {
> +		dev_info(cam->dev,
> +			 "%s:ctx(%d): could not create sensor_worker_task\n",
> +			 __func__, ctx->stream_id);
> +		goto fail_release_buffer_pool;
> +	}
> +
> +	sched_set_fifo(ctx->sensor_worker_task);
> +
> +	ctx->composer_wq = alloc_ordered_workqueue(dev_name(cam->dev),
> +						   WQ_HIGHPRI | WQ_FREEZABLE);
> +	if (!ctx->composer_wq) {
> +		dev_info(cam->dev, "failed to alloc composer workqueue\n");
> +		goto fail_uninit_sensor_worker_task;
> +	}
> +
> +	ctx->frame_done_wq = alloc_ordered_workqueue(dev_name(cam->dev),
> +						     WQ_HIGHPRI | WQ_FREEZABLE);
> +	if (!ctx->frame_done_wq) {
> +		dev_info(cam->dev, "failed to alloc frame_done workqueue\n");
> +		goto fail_uninit_composer_wq;
> +	}
> +
> +	ret = media_pipeline_start(&entity->pads[0], &ctx->pipeline);
> +	if (ret) {
> +		dev_warn(cam->dev,
> +			 "%s:pipe(%d):failed in media_pipeline_start:%d\n",
> +			 __func__, node->uid.pipe_id, ret);
> +		goto fail_uninit_frame_done_wq;
> +	}
> +
> +	/* traverse to update used subdevs & number of nodes */
> +	i = 0;
> +	ret = media_graph_walk_init(&graph, entity->graph_obj.mdev);
> +	if (ret)
> +		goto fail_stop_pipeline;
> +
> +	media_graph_walk_start(&graph, entity);
> +	while ((entity = media_graph_walk_next(&graph))) {
> +		dev_dbg(cam->dev, "linked entity %s\n", entity->name);
> +
> +		target_sd = NULL;
> +
> +		switch (entity->function) {
> +		case MEDIA_ENT_F_IO_V4L:
> +			ctx->enabled_node_cnt++;
> +			break;
> +		case MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER: /* pipeline */
> +			if (i >= MAX_PIPES_PER_STREAM)
> +				goto fail_stop_pipeline;
> +			target_sd = ctx->pipe_subdevs + i;
> +			i++;
> +			break;
> +		case MEDIA_ENT_F_VID_IF_BRIDGE: /* seninf */
> +			target_sd = &ctx->seninf;
> +			break;
> +		case MEDIA_ENT_F_CAM_SENSOR:
> +			target_sd = &ctx->sensor;

In this series, no entity's function is MEDIA_ENT_F_CAM_SENSOR,
so ctx->sensor is always NULL.
Drop it.

Regards,
CK

> +			break;
> +		default:
> +			break;
> +		}
> +
> +		if (!target_sd)
> +			continue;
> +
> +		if (*target_sd) {
> +			dev_info(cam->dev, "duplicated subdevs!!!\n");
> +			goto fail_traverse_subdev;
> +		}
> +
> +		if (is_media_entity_v4l2_subdev(entity))
> +			*target_sd = media_entity_to_v4l2_subdev(entity);
> +	}
> +	media_graph_walk_cleanup(&graph);
> +
> +	return ctx;
> +
> +fail_traverse_subdev:
> +	media_graph_walk_cleanup(&graph);
> +fail_stop_pipeline:
> +	media_pipeline_stop(&entity->pads[0]);
> +fail_uninit_frame_done_wq:
> +	destroy_workqueue(ctx->frame_done_wq);
> +fail_uninit_composer_wq:
> +	destroy_workqueue(ctx->composer_wq);
> +fail_uninit_sensor_worker_task:
> +	kthread_stop(ctx->sensor_worker_task);
> +	ctx->sensor_worker_task = NULL;
> +fail_release_buffer_pool:
> +	mtk_cam_working_buf_pool_release(ctx, dev);
> +fail_uninit_composer:
> +	isp_composer_uninit(cam);
> +	cam->composer_cnt--;
> +fail_shutdown:
> +	if (is_first_ctx)
> +		rproc_shutdown(cam->rproc_handle);
> +
> +	return NULL;
> +}
> +