Hi Krzysztof,

Thanks for the review.

On 27/07/23 17:43, Krzysztof Kozlowski wrote:

On 27/07/2023 13:25, Devarsh Thakkar wrote:

quoted

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.

...

quoted

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

quoted

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

...

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

This shouldn't be ENOMEM

Ok, will return the error.

quoted

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

quoted

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

quoted

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

...

quoted

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

quoted

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

quoted

+	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