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

[Devarsh Thakkar]

Hi Krzysztof,

Thanks for the review.

On 27/07/23 17:43, Krzysztof Kozlowski wrote:
> 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.
> 

This are dynamic debug prints to debug/analyze v4l2 drivers, It's a general
mechanism used in v4l2 drivers for debugging as Nicholas mentioned.

>> +	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.
> 

Agreed, will change the name.

>> +	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.

Ok, will remove of_match_device altogether as not needed.

> 
>> +		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
> 

Ok, will check on this.

>> +	}
>> +
>> +	dev->m2m_dev = v4l2_m2m_init(&e5010_m2m_ops);
>> +	if (!dev->m2m_dev) {
> 
> This cannot happen. Read the function.

Agreed will use IS_ERR here.
> 
>> +		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?
> 

Will propagate the return value instead.

>> +		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.

Ok will try using devm_platform_ioremap_resource_byname().

> 
>> +	if (!dev->jasper_base) {
>> +		ret = -ENOMEM;
> 
> This shouldn't be ENOMEM
> 

Ok, will return the error.

>> +		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.
> 

Ok, will return the error.
>> +		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
> 

Ok, will use above helper.

>> +		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.

Agreed, will correct.

> 
>> +};
>> +
>> +/*
>> + * 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.

Agreed, will correct.

Best Regards
Devarsh
> 
>> +	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
>