Re: [PATCH v2 2/2] media: imagination: Add E5010 JPEG Encoder driver

[Krzysztof Kozlowski]

On 27/07/2023 13:25, Devarsh Thakkar wrote:
> This adds support for stateful V4L2 M2M based driver
> for Imagination E5010 JPEG Encoder [1] which supports baseline
> encoding with two different quantization tables and compression
> ratio as demanded.
> 
> Support for both contigous and non-contigous YUV420 and YUV422
> semiplanar formats is added along with alignment restrictions
> as required by the hardware.
> 
> System and runtime PM hooks are added in the driver along with v4l2
> crop and selection API support.
> 
> Minimum resolution supported is 64x64 and
> Maximum resolution supported is 8192x8192.
> 


...

> +
> +static int e5010_release(struct file *file)
> +{
> +	struct e5010_dev *dev = video_drvdata(file);
> +	struct e5010_context *ctx = file->private_data;
> +
> +	dprintk(dev, 1, "Releasing instance: 0x%p, m2m_ctx: 0x%p\n", ctx, ctx->fh.m2m_ctx);

Why do you print pointers? Looks like code is buggy and you still keep
debugging it.

> +	mutex_lock(&dev->mutex);
> +	v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> +	v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
> +	v4l2_fh_del(&ctx->fh);
> +	v4l2_fh_exit(&ctx->fh);
> +	kfree(ctx);
> +	mutex_unlock(&dev->mutex);
> +
> +	return 0;
> +}
> +

...

> +static int e5010_init_device(struct e5010_dev *dev)
> +{
> +	int ret = 0;
> +
> +	/*TODO: Set MMU in bypass mode until support for the same is added in driver*/
> +	e5010_hw_bypass_mmu(dev->mmu_base, 1);
> +
> +	if (e5010_hw_enable_auto_clock_gating(dev->jasper_base, 1))
> +		dev_warn(dev->dev, "Failed to enable auto clock gating\n");
> +
> +	if (e5010_hw_enable_manual_clock_gating(dev->jasper_base, 0))
> +		dev_warn(dev->dev, "Failed to disable manual clock gating\n");
> +
> +	if (e5010_hw_enable_crc_check(dev->jasper_base, 0))
> +		dev_warn(dev->dev, "Failed to disable CRC check\n");
> +
> +	if (e5010_hw_enable_output_address_error_irq(dev->jasper_base, 1))
> +		dev_err(dev->dev, "Failed to enable Output Address Error interrupts\n");
> +
> +	ret = e5010_hw_set_input_source_to_memory(dev->jasper_base, 1);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to set input source to memory\n");
> +		goto fail;

retturn ret;

> +	}
> +
> +	ret = e5010_hw_enable_picture_done_irq(dev->jasper_base, 1);
> +	if (ret)
> +		dev_err(dev->dev, "Failed to enable Picture Done interrupts\n");
> +fail:
> +	return ret;
> +}
> +
> +static int e5010_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *of_dev_id;
> +	struct e5010_dev *dev;

dev is struct device, so call it differently.

> +	struct resource *res;
> +	int irq, ret = 0;
> +
> +	of_dev_id = of_match_device(e5010_of_match, &pdev->dev);
> +	if (!of_dev_id) {
> +		dev_err(&pdev->dev, "%s: Unable to match device\n", __func__);

I don't think this can happen.

> +		return -ENODEV;
> +	}
> +
> +	ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> +	if (ret) {
> +		dev_err(&pdev->dev, "32-bit consistent DMA enable failed\n");
> +		return ret;
> +	}
> +
> +	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, dev);
> +
> +	dev->dev = &pdev->dev;
> +
> +	mutex_init(&dev->mutex);
> +	spin_lock_init(&dev->hw_lock);
> +
> +	dev->vdev = &e5010_videodev;
> +	dev->vdev->v4l2_dev = &dev->v4l2_dev;
> +	dev->vdev->lock = &dev->mutex;
> +	dev->vdev->queue = NULL;
> +	dev->vdev->prio = NULL;
> +	dev->vdev->dev_parent = NULL;
> +	dev->vdev->minor = -1;
> +
> +	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to register v4l2 device\n");
> +		return ret;

return dev_err_probe

> +	}
> +
> +	dev->m2m_dev = v4l2_m2m_init(&e5010_m2m_ops);
> +	if (!dev->m2m_dev) {

This cannot happen. Read the function.

> +		dev_err(dev->dev, "Failed to initialize m2m device\n");
> +		ret = -ENOMEM;
> +		goto fail_after_v4l2_register;
> +	}
> +
> +	video_set_drvdata(dev->vdev, dev);
> +
> +	ret = video_register_device(dev->vdev, VFL_TYPE_VIDEO, 0);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to register video device\n");
> +		ret = -ENOMEM;

Why?

> +		goto fail_after_v4l2_register;
> +	}
> +
> +	dev_info(dev->dev, "Device registered as /dev/video%d\n",
> +		 dev->vdev->num);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regjasper");
> +	if (!res) {
> +		dev_err(dev->dev, "Missing 'regjasper' resources area\n");
> +		ret = -ENOMEM;
> +		goto fail_after_video_register_device;
> +	}
> +	dev->jasper_base = devm_ioremap_resource(&pdev->dev, res);

Use helper function to combine two calls into one.

> +	if (!dev->jasper_base) {
> +		ret = -ENOMEM;

This shouldn't be ENOMEM

> +		goto fail_after_video_register_device;
> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regmmu");
> +	if (!res) {
> +		dev_err(dev->dev, "Missing 'regmmu' resources area\n");
> +		ret = -ENOMEM;
> +		goto fail_after_video_register_device;
> +	}
> +	dev->mmu_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (!dev->mmu_base) {
> +		ret = -ENOMEM;

The same.

> +		goto fail_after_video_register_device;
> +	}
> +
> +	dev->last_context_run = NULL;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	ret = devm_request_irq(dev->dev, irq, e5010_irq, 0,
> +			       E5010_MODULE_NAME, dev);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to register IRQ %d\n", irq);
> +		goto fail_after_video_register_device;
> +	}
> +
> +	dev->clk = devm_clk_get(&pdev->dev, "core_clk");
> +	if (IS_ERR(dev->clk)) {
> +		dev_err(dev->dev, "failed to get clock\n");
> +		ret = PTR_ERR(dev->clk);

ret = dev_err_probe

> +		goto fail_after_video_register_device;
> +	}
> +
> +	pm_runtime_enable(dev->dev);
> +
> +	return 0;
> +
> +fail_after_video_register_device:
> +	v4l2_m2m_release(dev->m2m_dev);
> +fail_after_v4l2_register:
> +	v4l2_device_unregister(&dev->v4l2_dev);
> +	return ret;
> +}
> +
> +static int e5010_remove(struct platform_device *pdev)
> +{
> +	struct e5010_dev *dev = platform_get_drvdata(pdev);
> +
> +	pm_runtime_disable(dev->dev);
> +	video_unregister_device(dev->vdev);
> +	v4l2_m2m_release(dev->m2m_dev);
> +	v4l2_device_unregister(&dev->v4l2_dev);
> +
> +	return 0;
> +}

...

> +#define MAX_PLANES			2
> +#define HEADER_SIZE			0x025E
> +#define MIN_DIMENSION			64
> +#define MAX_DIMENSION			8192
> +#define DEFAULT_WIDTH			640
> +#define DEFAULT_HEIGHT			480
> +#define E5010_MODULE_NAME		"e5010"
> +
> +/* JPEG marker definitions */
> +#define START_OF_IMAGE			0xFFD8
> +#define SOF_BASELINE_DCT		0xFFC0
> +#define END_OF_IMAGE			0xFFD9
> +#define START_OF_SCAN			0xFFDA
> +
> +/* Definitions for the huffman table specification in the Marker segment */
> +#define DHT_MARKER			0xFFC4
> +#define LH_DC				0x001F
> +#define LH_AC				0x00B5
> +
> +/* Definitions for the quantization table specification in the Marker segment */
> +#define DQT_MARKER			0xFFDB
> +#define ACMAX				0x03FF
> +#define DCMAX				0x07FF
> +
> +/* Length and precision of the quantization table parameters */
> +#define LQPQ				0x00430
> +#define QMAX				255
> +
> +/* Misc JPEG header definitions */
> +#define UC_NUM_COMP			3
> +#define PRECISION			8
> +#define HORZ_SAMPLING_FACTOR		(2 << 4)
> +#define VERT_SAMPLING_FACTOR_422	1
> +#define VERT_SAMPLING_FACTOR_420	2
> +#define COMPONENTS_IN_SCAN		3
> +#define PELS_IN_BLOCK			64
> +
> +/* Used for Qp table generation */
> +#define LUMINOSITY			10
> +#define CONTRAST			1
> +#define INCREASE			2
> +#define QP_TABLE_SIZE			(8 * 8)
> +#define QP_TABLE_FIELD_OFFSET		0x04
> +
> +/*
> + * vb2 queue structure
> + * contains queue data information
> + *
> + * @fmt: format info
> + * @width: frame width
> + * @height: frame height
> + * @bytesperline: bytes per line in memory
> + * @size_image: image size in memory
> + */
> +struct e5010_q_data {
> +	struct e5010_fmt *fmt;
> +	u32 width;
> +	u32 height;
> +	u32 width_adjusted;
> +	u32 height_adjusted;
> +	u32 sizeimage[MAX_PLANES];
> +	u32 bytesperline[MAX_PLANES];
> +	bool format_set;
> +	bool streaming;
> +	u32			sequence;
> +	struct v4l2_rect	crop;

Unexpected indentation.

> +};
> +
> +/*
> + * Driver device structure
> + * Holds all memory handles and global parameters
> + * Shared by all instances
> + */
> +struct e5010_dev {
> +	struct device *dev;
> +	struct v4l2_device v4l2_dev;
> +	struct v4l2_m2m_dev *m2m_dev;
> +	struct video_device *vdev;
> +	void __iomem *jasper_base;
> +	void __iomem *mmu_base;
> +	struct clk   *clk;

Please keep style consistent.

> +	struct e5010_context *last_context_run;
> +	/* Protect access to device data */
> +	struct mutex mutex;
> +	/* Protect access to hardware*/
> +	spinlock_t hw_lock;
> +};
> +


Best regards,
Krzysztof