Re: [PATCH v9 09/13] [media] exynos5-fimc-is: Add the hardware pipeline control

[Hans Verkuil <hverkuil@xs4all.nl>]

On 09/27/2013 12:59 PM, Arun Kumar K wrote:
> This patch adds the crucial hardware pipeline control for the
> fimc-is driver. All the subdev nodes will call this pipeline
> interfaces to reach the hardware. Responsibilities of this module
> involves configuring and maintaining the hardware pipeline involving
> multiple sub-ips like ISP, DRC, Scalers, ODC, 3DNR, FD etc.
> 
> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
> Signed-off-by: Kilyeon Im <kilyeon.im@samsung.com>
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
>  .../media/platform/exynos5-is/fimc-is-pipeline.c   | 1708 ++++++++++++++++++++
>  .../media/platform/exynos5-is/fimc-is-pipeline.h   |  129 ++
>  2 files changed, 1837 insertions(+)
>  create mode 100644 drivers/media/platform/exynos5-is/fimc-is-pipeline.c
>  create mode 100644 drivers/media/platform/exynos5-is/fimc-is-pipeline.h
> 
> diff --git a/drivers/media/platform/exynos5-is/fimc-is-pipeline.c b/drivers/media/platform/exynos5-is/fimc-is-pipeline.c
> new file mode 100644
> index 0000000..a73d952
> --- /dev/null
> +++ b/drivers/media/platform/exynos5-is/fimc-is-pipeline.c

<snip>

> +int fimc_is_pipeline_open(struct fimc_is_pipeline *pipeline,
> +			struct fimc_is_sensor *sensor)
> +{
> +	struct fimc_is *is = pipeline->is;
> +	struct is_region *region;
> +	unsigned long index[2] = {0};
> +	int ret;
> +
> +	if (!sensor)
> +		return -EINVAL;
> +
> +	mutex_lock(&pipeline->pipe_lock);
> +
> +	if (test_bit(PIPELINE_OPEN, &pipeline->state)) {
> +		dev_err(pipeline->dev, "Pipeline already open\n");
> +		ret = -EINVAL;
> +		goto err_exit;
> +	}
> +
> +	pipeline->fcount = 0;
> +	pipeline->sensor = sensor;
> +
> +	if (is->num_pipelines == 0) {
> +		/* Init memory */
> +		ret = fimc_is_pipeline_initmem(pipeline);
> +		if (ret) {
> +			dev_err(pipeline->dev, "Pipeline memory init failed\n");
> +			goto err_exit;
> +		}
> +
> +		/* Load firmware */
> +		ret = fimc_is_pipeline_load_firmware(pipeline);
> +		if (ret) {
> +			dev_err(pipeline->dev, "Firmware load failed\n");
> +			goto err_fw;
> +		}
> +
> +		/* Power ON */
> +		ret = fimc_is_pipeline_power(pipeline, 1);
> +		if (ret) {
> +			dev_err(pipeline->dev, "A5 power on failed\n");
> +			goto err_fw;
> +		}
> +
> +		/* Wait for FW Init to complete */
> +		ret = fimc_is_itf_wait_init_state(&is->interface);
> +		if (ret) {
> +			dev_err(pipeline->dev, "FW init failed\n");
> +			goto err_fw;
> +		}
> +	}
> +
> +	/* Open Sensor */
> +	region = pipeline->is_region;
> +	ret = fimc_is_itf_open_sensor(&is->interface,
> +			pipeline->instance,
> +			sensor->drvdata->id,
> +			sensor->i2c_bus,
> +			pipeline->minfo->shared.paddr);
> +	if (ret) {
> +		dev_err(pipeline->dev, "Open sensor failed\n");
> +		goto err_exit;
> +	}
> +
> +	/* Load setfile */
> +	ret = fimc_is_pipeline_setfile(pipeline);
> +	if (ret)
> +		goto err_exit;
> +
> +	/* Stream off */
> +	ret = fimc_is_itf_stream_off(&is->interface, pipeline->instance);
> +	if (ret)
> +		goto err_exit;
> +
> +	/* Process off */
> +	ret = fimc_is_itf_process_off(&is->interface, pipeline->instance);
> +	if (ret)
> +		goto err_exit;
> +
> +	if (is->num_pipelines == 0) {
> +		/* Copy init params to FW region */
> +		memset(&region->parameter, 0x0, sizeof(struct is_param_region));
> +
> +		memcpy(&region->parameter.sensor, &init_sensor_param,
> +				sizeof(struct sensor_param));

How about:

		region->parameter.sensor = init_sensor_param;

Shorter and type-safe.

Ditto for the memcpy's below.

> +		memcpy(&region->parameter.isp, &init_isp_param,
> +				sizeof(struct isp_param));
> +		memcpy(&region->parameter.drc, &init_drc_param,
> +				sizeof(struct drc_param));
> +		memcpy(&region->parameter.scalerc, &init_scalerc_param,
> +				sizeof(struct scalerc_param));
> +		memcpy(&region->parameter.odc, &init_odc_param,
> +				sizeof(struct odc_param));
> +		memcpy(&region->parameter.dis, &init_dis_param,
> +				sizeof(struct dis_param));
> +		memcpy(&region->parameter.tdnr, &init_tdnr_param,
> +				sizeof(struct tdnr_param));
> +		memcpy(&region->parameter.scalerp, &init_scalerp_param,
> +				sizeof(struct scalerp_param));
> +		memcpy(&region->parameter.fd, &init_fd_param,
> +				sizeof(struct fd_param));
> +		wmb();
> +
> +		/* Set all init params to FW */
> +		index[0] = 0xffffffff;
> +		index[1] = 0xffffffff;
> +		ret = fimc_is_itf_set_param(&is->interface, pipeline->instance,
> +				index[0], index[1]);
> +		if (ret) {
> +			dev_err(pipeline->dev, "%s failed\n", __func__);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* Set state to OPEN */
> +	set_bit(PIPELINE_OPEN, &pipeline->state);
> +	is->num_pipelines++;
> +
> +	mutex_unlock(&pipeline->pipe_lock);
> +	return 0;
> +
> +err_fw:
> +	fimc_is_pipeline_freemem(pipeline);
> +err_exit:
> +	mutex_unlock(&pipeline->pipe_lock);
> +	return ret;
> +}

Regards,

	Hans