On Sat, Jul 13, 2024 at 05:48:02PM +0200, Marek Vasut wrote:

There should be some clean up on this error path.  Ideally
imx_media_mem2mem_vdic_init() would have a matching uninit() function
we could call.

+		}
+	}
+
 	ret = imx_media_csc_scaler_device_register(imxmd->m2m_vdev);
+	if (ret)
+		goto unlock;
+
+	ret = imx_media_mem2mem_vdic_register(imxmd->m2m_vdic[0]);
+	if (ret)
+		goto unreg_csc;
+
+	/* MX6S/DL has one IPUv3, init second VDI only on MX6Q/QP */
+	if (imxmd->ipu[1]) {
+		ret = imx_media_mem2mem_vdic_register(imxmd->m2m_vdic[1]);
+		if (ret)
+			goto unreg_vdic;
+	}
+
+	mutex_unlock(&imxmd->mutex);
+	return ret;

return 0;

+
+unreg_vdic:
+	imx_media_mem2mem_vdic_unregister(imxmd->m2m_vdic[0]);
+	imxmd->m2m_vdic[0] = NULL;
+unreg_csc:
+	imx_media_csc_scaler_device_unregister(imxmd->m2m_vdev);
+	imxmd->m2m_vdev = NULL;
 unlock:
 	mutex_unlock(&imxmd->mutex);
 	return ret;

[ snip ]

+static void ipu_mem2mem_vdic_device_run(void *_ctx)
+{
+	struct ipu_mem2mem_vdic_ctx *ctx = _ctx;
+	struct ipu_mem2mem_vdic_priv *priv = ctx->priv;
+	struct vb2_v4l2_buffer *curr_buf, *dst_buf;
+	dma_addr_t prev_phys, curr_phys, out_phys;
+	struct v4l2_pix_format *infmt;
+	u32 phys_offset = 0;
+	unsigned long flags;
+	int ret;
+
+	infmt = ipu_mem2mem_vdic_get_format(priv, V4L2_BUF_TYPE_VIDEO_OUTPUT);
+	if (V4L2_FIELD_IS_SEQUENTIAL(infmt->field))
+		phys_offset = infmt->sizeimage / 2;
+	else if (V4L2_FIELD_IS_INTERLACED(infmt->field))
+		phys_offset = infmt->bytesperline;
+	else
+		dev_err(priv->dev, "Invalid field %d\n", infmt->field);
+
+	dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
+	out_phys = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
+
+	curr_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
+	if (!curr_buf) {
+		dev_err(priv->dev, "Not enough buffers %d\n", ret);

ret is uninitialized.  Just delete it.

+		return;
+	}
+
+	spin_lock_irqsave(&priv->irqlock, flags);
+
+	if (ctx->curr_buf) {
+		ctx->prev_buf = ctx->curr_buf;
+		ctx->curr_buf = curr_buf;
+	} else {
+		ctx->prev_buf = curr_buf;
+		ctx->curr_buf = curr_buf;
+		dev_warn(priv->dev, "Single-buffer mode, fix your userspace\n");
+	}
+
+	prev_phys = vb2_dma_contig_plane_dma_addr(&ctx->prev_buf->vb2_buf, 0);
+	curr_phys = vb2_dma_contig_plane_dma_addr(&ctx->curr_buf->vb2_buf, 0);
+
+	priv->curr_ctx = ctx;
+	spin_unlock_irqrestore(&priv->irqlock, flags);
+
+	ipu_cpmem_set_buffer(priv->vdi_out_ch,  0, out_phys);
+	ipu_cpmem_set_buffer(priv->vdi_in_ch_p, 0, prev_phys + phys_offset);
+	ipu_cpmem_set_buffer(priv->vdi_in_ch,   0, curr_phys);
+	ipu_cpmem_set_buffer(priv->vdi_in_ch_n, 0, curr_phys + phys_offset);
+
+	/* No double buffering, always pick buffer 0 */
+	ipu_idmac_select_buffer(priv->vdi_out_ch, 0);
+	ipu_idmac_select_buffer(priv->vdi_in_ch_p, 0);
+	ipu_idmac_select_buffer(priv->vdi_in_ch, 0);
+	ipu_idmac_select_buffer(priv->vdi_in_ch_n, 0);
+
+	/* Enable the channels */
+	ipu_idmac_enable_channel(priv->vdi_out_ch);
+	ipu_idmac_enable_channel(priv->vdi_in_ch_p);
+	ipu_idmac_enable_channel(priv->vdi_in_ch);
+	ipu_idmac_enable_channel(priv->vdi_in_ch_n);
+}

[ snip ]

+static int ipu_mem2mem_vdic_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
+					unsigned int *nplanes, unsigned int sizes[],
+					struct device *alloc_devs[])
+{
+	struct ipu_mem2mem_vdic_ctx *ctx = vb2_get_drv_priv(vq);
+	struct ipu_mem2mem_vdic_priv *priv = ctx->priv;
+	struct v4l2_pix_format *fmt = ipu_mem2mem_vdic_get_format(priv, vq->type);
+	unsigned int count = *nbuffers;
+
+	*nbuffers = count;

count and *nbuffers are the same already.  Delete this statement.

+
+	if (*nplanes)
+		return sizes[0] < fmt->sizeimage ? -EINVAL : 0;
+
+	*nplanes = 1;
+	sizes[0] = fmt->sizeimage;
+
+	dev_dbg(ctx->priv->dev, "get %d buffer(s) of size %d each.\n",

                                     ^^
%u for unsigned int.

+		count, fmt->sizeimage);
+
+	return 0;
+}

[ snip ]

+static int ipu_mem2mem_vdic_get_ipu_resources(struct ipu_mem2mem_vdic_priv *priv,
+					      struct video_device *vfd)
+{
+	char *nfbname, *eofname;
+	int ret;
+
+	nfbname = devm_kasprintf(priv->dev, GFP_KERNEL, "%s_nfb4eof:%u",
+				 vfd->name, priv->ipu_id);
+	if (!nfbname)
+		return -ENOMEM;
+
+	eofname = devm_kasprintf(priv->dev, GFP_KERNEL, "%s_eof:%u",
+				 vfd->name, priv->ipu_id);
+	if (!eofname)
+		goto err_eof;
+
+	priv->vdi = ipu_vdi_get(priv->ipu_dev);
+	if (IS_ERR(priv->vdi)) {
+		ret = PTR_ERR(priv->vdi);
+		goto err_vdi;
+	}
+
+	priv->ic = ipu_ic_get(priv->ipu_dev, IC_TASK_VIEWFINDER);
+	if (IS_ERR(priv->ic)) {
+		ret = PTR_ERR(priv->ic);
+		goto err_ic;
+	}
+
+	priv->vdi_in_ch_p = ipu_idmac_get(priv->ipu_dev,
+					  IPUV3_CHANNEL_MEM_VDI_PREV);
+	if (IS_ERR(priv->vdi_in_ch_p)) {
+		ret = PTR_ERR(priv->vdi_in_ch_p);
+		goto err_prev;
+	}
+
+	priv->vdi_in_ch = ipu_idmac_get(priv->ipu_dev,
+					IPUV3_CHANNEL_MEM_VDI_CUR);
+	if (IS_ERR(priv->vdi_in_ch)) {
+		ret = PTR_ERR(priv->vdi_in_ch);
+		goto err_curr;
+	}
+
+	priv->vdi_in_ch_n = ipu_idmac_get(priv->ipu_dev,
+					  IPUV3_CHANNEL_MEM_VDI_NEXT);
+	if (IS_ERR(priv->vdi_in_ch_n)) {
+		ret = PTR_ERR(priv->vdi_in_ch_n);
+		goto err_next;
+	}
+
+	priv->vdi_out_ch = ipu_idmac_get(priv->ipu_dev,
+					 IPUV3_CHANNEL_IC_PRP_VF_MEM);
+	if (IS_ERR(priv->vdi_out_ch)) {
+		ret = PTR_ERR(priv->vdi_out_ch);
+		goto err_out;
+	}
+
+	priv->nfb4eof_irq = ipu_idmac_channel_irq(priv->ipu_dev,
+						  priv->vdi_out_ch,
+						  IPU_IRQ_NFB4EOF);
+	ret = devm_request_irq(priv->dev, priv->nfb4eof_irq,
+			       ipu_mem2mem_vdic_nfb4eof_interrupt, 0,
+			       nfbname, priv);
+	if (ret)
+		goto err_irq_nfb4eof;
+
+	priv->eof_irq = ipu_idmac_channel_irq(priv->ipu_dev,
+					      priv->vdi_out_ch,
+					      IPU_IRQ_EOF);
+	ret = devm_request_irq(priv->dev, priv->eof_irq,
+			       ipu_mem2mem_vdic_eof_interrupt, 0,
+			       eofname, priv);
+	if (ret)
+		goto err_irq_eof;
+
+	/*
+	 * Enable PRG, without PRG clock enabled (CCGR6:prg_clk_enable[0]
+	 * and CCGR6:prg_clk_enable[1]), the VDI does not produce any
+	 * interrupts at all.
+	 */
+	if (ipu_prg_present(priv->ipu_dev))
+		ipu_prg_enable(priv->ipu_dev);
+
+	return 0;
+
+err_irq_eof:
+	devm_free_irq(priv->dev, priv->nfb4eof_irq, priv);

        ^^^^

+err_irq_nfb4eof:
+	ipu_idmac_put(priv->vdi_out_ch);
+err_out:
+	ipu_idmac_put(priv->vdi_in_ch_n);
+err_next:
+	ipu_idmac_put(priv->vdi_in_ch);
+err_curr:
+	ipu_idmac_put(priv->vdi_in_ch_p);
+err_prev:
+	ipu_ic_put(priv->ic);
+err_ic:
+	ipu_vdi_put(priv->vdi);
+err_vdi:
+	devm_kfree(priv->dev, eofname);

        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

+err_eof:
+	devm_kfree(priv->dev, nfbname);

        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Any time we call devm_kfree() it's a red flag.  Sometimes it makes sense
but I haven't looked at it closely enough to see how it makes sense
here.  Is it an ordering issue where we had to do devm_free_irq() and
then we just freed oefname and nfbname for consistency and because why
not?

+	return ret;
+}
+
+static void ipu_mem2mem_vdic_put_ipu_resources(struct ipu_mem2mem_vdic_priv *priv)
+{
+	devm_free_irq(priv->dev, priv->eof_irq, priv);
+	devm_free_irq(priv->dev, priv->nfb4eof_irq, priv);
+	ipu_idmac_put(priv->vdi_out_ch);
+	ipu_idmac_put(priv->vdi_in_ch_n);
+	ipu_idmac_put(priv->vdi_in_ch);
+	ipu_idmac_put(priv->vdi_in_ch_p);
+	ipu_ic_put(priv->ic);
+	ipu_vdi_put(priv->vdi);
+}
+
+int imx_media_mem2mem_vdic_register(struct imx_media_video_dev *vdev)
+{
+	struct ipu_mem2mem_vdic_priv *priv = to_mem2mem_priv(vdev);
+	struct video_device *vfd = vdev->vfd;
+	int ret;
+
+	vfd->v4l2_dev = &priv->md->v4l2_dev;
+
+	ret = ipu_mem2mem_vdic_get_ipu_resources(priv, vfd);
+	if (ret) {
+		v4l2_err(vfd->v4l2_dev, "Failed to get VDIC resources (%d)\n", ret);
+		return ret;
+	}
+
+	ret = video_register_device(vfd, VFL_TYPE_VIDEO, -1);
+	if (ret) {
+		v4l2_err(vfd->v4l2_dev, "Failed to register video device\n");

Probably should call ipu_mem2mem_vdic_put_ipu_resources() on this error
path?

+		return ret;
+	}
+
+	v4l2_info(vfd->v4l2_dev, "Registered %s as /dev/%s\n", vfd->name,
+		  video_device_node_name(vfd));
+
+	return 0;
+}
+
+void imx_media_mem2mem_vdic_unregister(struct imx_media_video_dev *vdev)
+{
+	struct ipu_mem2mem_vdic_priv *priv = to_mem2mem_priv(vdev);
+	struct video_device *vfd = priv->vdev.vfd;
+
+	video_unregister_device(vfd);
+
+	ipu_mem2mem_vdic_put_ipu_resources(priv);
+}
+
+struct imx_media_video_dev *
+imx_media_mem2mem_vdic_init(struct imx_media_dev *md, int ipu_id)
+{
+	struct ipu_mem2mem_vdic_priv *priv;
+	struct video_device *vfd;
+	int ret;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return ERR_PTR(-ENOMEM);
+
+	priv->md = md;
+	priv->ipu_id = ipu_id;
+	priv->ipu_dev = md->ipu[ipu_id];
+	priv->dev = md->md.dev;
+
+	mutex_init(&priv->mutex);
+
+	vfd = video_device_alloc();
+	if (!vfd) {
+		ret = -ENOMEM;
+		goto err_vfd;
+	}
+
+	*vfd = mem2mem_template;
+	vfd->lock = &priv->mutex;
+	priv->vdev.vfd = vfd;
+
+	INIT_LIST_HEAD(&priv->vdev.list);
+	spin_lock_init(&priv->irqlock);
+	atomic_set(&priv->stream_count, 0);
+
+	video_set_drvdata(vfd, priv);
+
+	priv->m2m_dev = v4l2_m2m_init(&m2m_ops);
+	if (IS_ERR(priv->m2m_dev)) {
+		ret = PTR_ERR(priv->m2m_dev);
+		v4l2_err(&md->v4l2_dev, "Failed to init mem2mem device: %d\n",
+			 ret);
+		goto err_m2m;
+	}
+
+	/* Reset formats */
+	priv->fmt[V4L2_M2M_SRC] = ipu_mem2mem_vdic_default;
+	priv->fmt[V4L2_M2M_SRC].pixelformat = V4L2_PIX_FMT_YUV420;
+	priv->fmt[V4L2_M2M_SRC].field = V4L2_FIELD_SEQ_TB;
+	priv->fmt[V4L2_M2M_SRC].bytesperline = DEFAULT_WIDTH;
+	priv->fmt[V4L2_M2M_SRC].sizeimage = DEFAULT_WIDTH * DEFAULT_HEIGHT * 3 / 2;
+
+	priv->fmt[V4L2_M2M_DST] = ipu_mem2mem_vdic_default;
+	priv->fmt[V4L2_M2M_DST].pixelformat = V4L2_PIX_FMT_RGB565;
+	priv->fmt[V4L2_M2M_DST].field = V4L2_FIELD_NONE;
+	priv->fmt[V4L2_M2M_DST].bytesperline = DEFAULT_WIDTH * 2;
+	priv->fmt[V4L2_M2M_DST].sizeimage = DEFAULT_WIDTH * DEFAULT_HEIGHT * 2;
+
+	return &priv->vdev;
+
+err_m2m:
+	video_set_drvdata(vfd, NULL);

video_device_release(vfd)

regards,
dan carpenter

+err_vfd:
+	kfree(priv);
+	return ERR_PTR(ret);
+}