Re: [PATCH v2 3/5] [media] exynos-mscl: Add m2m functionality for the M-Scaler driver

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

On 08/19/2013 12:58 PM, Shaik Ameer Basha wrote:
> This patch adds the memory to memory (m2m) interface functionality
> for the M-Scaler driver.
> 
> Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
> ---
>  drivers/media/platform/exynos-mscl/mscl-m2m.c |  763 +++++++++++++++++++++++++
>  1 file changed, 763 insertions(+)
>  create mode 100644 drivers/media/platform/exynos-mscl/mscl-m2m.c
> 
> diff --git a/drivers/media/platform/exynos-mscl/mscl-m2m.c b/drivers/media/platform/exynos-mscl/mscl-m2m.c
> new file mode 100644
> index 0000000..fecbb57
> --- /dev/null
> +++ b/drivers/media/platform/exynos-mscl/mscl-m2m.c
> @@ -0,0 +1,763 @@
> +/*
> + * Copyright (c) 2013 - 2014 Samsung Electronics Co., Ltd.

2013 - 2014 ??

> + *		http://www.samsung.com
> + *
> + * Samsung EXYNOS5 SoC series M-Scaler driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published
> + * by the Free Software Foundation, either version 2 of the License,
> + * or (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +
> +#include <media/v4l2-ioctl.h>
> +
> +#include "mscl-core.h"
> +
> +static int mscl_m2m_ctx_stop_req(struct mscl_ctx *ctx)
> +{
> +	struct mscl_ctx *curr_ctx;
> +	struct mscl_dev *mscl = ctx->mscl_dev;
> +	int ret;
> +
> +	curr_ctx = v4l2_m2m_get_curr_priv(mscl->m2m.m2m_dev);
> +	if (!mscl_m2m_pending(mscl) || (curr_ctx != ctx))
> +		return 0;
> +
> +	mscl_ctx_state_lock_set(MSCL_CTX_STOP_REQ, ctx);
> +	ret = wait_event_timeout(mscl->irq_queue,
> +			!mscl_ctx_state_is_set(MSCL_CTX_STOP_REQ, ctx),
> +			MSCL_SHUTDOWN_TIMEOUT);
> +
> +	return ret == 0 ? -ETIMEDOUT : ret;
> +}
> +
> +static int mscl_m2m_start_streaming(struct vb2_queue *q, unsigned int count)
> +{
> +	struct mscl_ctx *ctx = q->drv_priv;
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(&ctx->mscl_dev->pdev->dev);
> +
> +	return ret > 0 ? 0 : ret;
> +}
> +
> +static int mscl_m2m_stop_streaming(struct vb2_queue *q)
> +{
> +	struct mscl_ctx *ctx = q->drv_priv;
> +	int ret;
> +
> +	ret = mscl_m2m_ctx_stop_req(ctx);
> +	if (ret == -ETIMEDOUT)
> +		mscl_m2m_job_finish(ctx, VB2_BUF_STATE_ERROR);
> +
> +	pm_runtime_put(&ctx->mscl_dev->pdev->dev);
> +
> +	return 0;
> +}
> +
> +void mscl_m2m_job_finish(struct mscl_ctx *ctx, int vb_state)
> +{
> +	struct vb2_buffer *src_vb, *dst_vb;
> +
> +	if (!ctx || !ctx->m2m_ctx)
> +		return;
> +
> +	src_vb = v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
> +	dst_vb = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
> +
> +	if (src_vb && dst_vb) {
> +		v4l2_m2m_buf_done(src_vb, vb_state);
> +		v4l2_m2m_buf_done(dst_vb, vb_state);
> +
> +		v4l2_m2m_job_finish(ctx->mscl_dev->m2m.m2m_dev,
> +							ctx->m2m_ctx);
> +	}
> +}
> +
> +

Stray empty line.

> +static void mscl_m2m_job_abort(void *priv)
> +{
> +	struct mscl_ctx *ctx = priv;
> +	int ret;
> +
> +	ret = mscl_m2m_ctx_stop_req(ctx);
> +	if (ret == -ETIMEDOUT)
> +		mscl_m2m_job_finish(ctx, VB2_BUF_STATE_ERROR);
> +}
> +
> +static int mscl_get_bufs(struct mscl_ctx *ctx)
> +{
> +	struct mscl_frame *s_frame, *d_frame;
> +	struct vb2_buffer *src_vb, *dst_vb;
> +	int ret;
> +
> +	s_frame = &ctx->s_frame;
> +	d_frame = &ctx->d_frame;
> +
> +	src_vb = v4l2_m2m_next_src_buf(ctx->m2m_ctx);
> +	ret = mscl_prepare_addr(ctx, src_vb, s_frame, &s_frame->addr);
> +	if (ret)

How about using "if (ret < 0)" pattern consistently ?

> +		return ret;
> +
> +	dst_vb = v4l2_m2m_next_dst_buf(ctx->m2m_ctx);
> +	ret = mscl_prepare_addr(ctx, dst_vb, d_frame, &d_frame->addr);
> +	if (ret)
> +		return ret;
> +
> +	dst_vb->v4l2_buf.timestamp = src_vb->v4l2_buf.timestamp;
> +
> +	return 0;
> +}
> +
> +static void mscl_m2m_device_run(void *priv)
> +{
> +	struct mscl_ctx *ctx = priv;
> +	struct mscl_dev *mscl;
> +	unsigned long flags;
> +	int ret;
> +	bool is_set = false;

Unneeded initialization. And I can see a room for improvement WRT
the variable's name.

> +
> +	if (WARN(!ctx, "null hardware context\n"))
> +		return;
> +
> +	mscl = ctx->mscl_dev;
> +	spin_lock_irqsave(&mscl->slock, flags);
> +
> +	set_bit(ST_M2M_PEND, &mscl->state);
> +
> +	/* Reconfigure hardware if the context has changed. */
> +	if (mscl->m2m.ctx != ctx) {
> +		dev_dbg(&mscl->pdev->dev,
> +			"mscl->m2m.ctx = 0x%p, current_ctx = 0x%p",
> +			mscl->m2m.ctx, ctx);
> +		ctx->state |= MSCL_PARAMS;
> +		mscl->m2m.ctx = ctx;
> +	}
> +
> +	is_set = (ctx->state & MSCL_CTX_STOP_REQ) ? 1 : 0;

Just make it

	is_set = ctx->state & MSCL_CTX_STOP_REQ;

> +	ctx->state &= ~MSCL_CTX_STOP_REQ;
> +	if (is_set) {
> +		wake_up(&mscl->irq_queue);
> +		goto put_device;
> +	}
> +
> +	ret = mscl_get_bufs(ctx);
> +	if (ret) {
> +		dev_dbg(&mscl->pdev->dev, "Wrong address");

Missing '\n'.

> +		goto put_device;
> +	}
> +
> +	mscl_hw_address_queue_reset(ctx);
> +	mscl_set_prefbuf(mscl, &ctx->s_frame);
> +	mscl_hw_set_input_addr(mscl, &ctx->s_frame.addr);
> +	mscl_hw_set_output_addr(mscl, &ctx->d_frame.addr);
> +	mscl_hw_set_csc_coeff(ctx);
> +
> +	if (ctx->state & MSCL_PARAMS) {
> +		mscl_hw_set_irq_mask(mscl, MSCL_INT_FRAME_END, false);
> +		if (mscl_set_scaler_info(ctx)) {
> +			dev_dbg(&mscl->pdev->dev, "Scaler setup error");
> +			goto put_device;
> +		}
> +
> +		mscl_hw_set_in_size(ctx);
> +		mscl_hw_set_in_image_format(ctx);
> +
> +		mscl_hw_set_out_size(ctx);
> +		mscl_hw_set_out_image_format(ctx);
> +
> +		mscl_hw_set_scaler_ratio(ctx);
> +		mscl_hw_set_rotation(ctx);
> +	}
> +
> +	ctx->state &= ~MSCL_PARAMS;
> +	mscl_hw_enable_control(mscl, true);
> +
> +	spin_unlock_irqrestore(&mscl->slock, flags);
> +	return;
> +
> +put_device:

nit: Perhaps just "unlock". There is nothing like "put device" here.

> +	ctx->state &= ~MSCL_PARAMS;
> +	spin_unlock_irqrestore(&mscl->slock, flags);
> +}
> +
> +static int mscl_m2m_queue_setup(struct vb2_queue *vq,
> +			const struct v4l2_format *fmt,
> +			unsigned int *num_buffers, unsigned int *num_planes,
> +			unsigned int sizes[], void *allocators[])
> +{
> +	struct mscl_ctx *ctx = vb2_get_drv_priv(vq);
> +	struct mscl_frame *frame;
> +	int i;
> +
> +	frame = ctx_get_frame(ctx, vq->type);
> +	if (IS_ERR(frame))
> +		return PTR_ERR(frame);
> +
> +	if (!frame->fmt)
> +		return -EINVAL;
> +
> +	*num_planes = frame->fmt->num_planes;
> +	for (i = 0; i < frame->fmt->num_planes; i++) {
> +		sizes[i] = frame->payload[i];
> +		allocators[i] = ctx->mscl_dev->alloc_ctx;
> +	}
> +	return 0;
> +}
> +
> +static int mscl_m2m_buf_prepare(struct vb2_buffer *vb)
> +{
> +	struct mscl_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> +	struct mscl_frame *frame;
> +	int i;
> +
> +	frame = ctx_get_frame(ctx, vb->vb2_queue->type);
> +	if (IS_ERR(frame))
> +		return PTR_ERR(frame);

You should check if the buffer passed is suitable for currently set
image format. If not then error should be returned here.

Then you could remove all that MSCL_SRC_FMT/MSCL_DST_FMT logic.

> +	if (!V4L2_TYPE_IS_OUTPUT(vb->vb2_queue->type)) {
> +		for (i = 0; i < frame->fmt->num_planes; i++)
> +			vb2_set_plane_payload(vb, i, frame->payload[i]);
> +	}
> +
> +	return 0;
> +}
> +
> +static void mscl_m2m_buf_queue(struct vb2_buffer *vb)
> +{
> +	struct mscl_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> +
> +	dev_dbg(&ctx->mscl_dev->pdev->dev,
> +		"ctx: %p, ctx->state: 0x%x", ctx, ctx->state);
> +
> +	if (ctx->m2m_ctx)
> +		v4l2_m2m_buf_queue(ctx->m2m_ctx, vb);
> +}
> +
> +static struct vb2_ops mscl_m2m_qops = {
> +	.queue_setup	 = mscl_m2m_queue_setup,
> +	.buf_prepare	 = mscl_m2m_buf_prepare,
> +	.buf_queue	 = mscl_m2m_buf_queue,
> +	.wait_prepare	 = mscl_unlock,
> +	.wait_finish	 = mscl_lock,
> +	.stop_streaming	 = mscl_m2m_stop_streaming,
> +	.start_streaming = mscl_m2m_start_streaming,
> +};
> +
> +static int mscl_m2m_querycap(struct file *file, void *fh,
> +			   struct v4l2_capability *cap)
> +{
> +	struct mscl_ctx *ctx = fh_to_ctx(fh);
> +	struct mscl_dev *mscl = ctx->mscl_dev;
> +
> +	strlcpy(cap->driver, mscl->pdev->name, sizeof(cap->driver));
> +	strlcpy(cap->card, mscl->pdev->name, sizeof(cap->card));
> +	strlcpy(cap->bus_info, "platform", sizeof(cap->bus_info));
> +	cap->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_M2M_MPLANE |

> +		V4L2_CAP_VIDEO_CAPTURE_MPLANE |	V4L2_CAP_VIDEO_OUTPUT_MPLANE;

As Hans pointed out, this line need to be removed.

> +	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> +
> +	return 0;
> +}
> +
> +static int mscl_m2m_enum_fmt_mplane(struct file *file, void *priv,
> +				struct v4l2_fmtdesc *f)
> +{
> +	return mscl_enum_fmt_mplane(f);
> +}
> +
> +static int mscl_m2m_g_fmt_mplane(struct file *file, void *fh,
> +			     struct v4l2_format *f)
> +{
> +	struct mscl_ctx *ctx = fh_to_ctx(fh);
> +
> +	return mscl_g_fmt_mplane(ctx, f);
> +}
> +
> +static int mscl_m2m_try_fmt_mplane(struct file *file, void *fh,
> +				  struct v4l2_format *f)
> +{
> +	struct mscl_ctx *ctx = fh_to_ctx(fh);
> +
> +	return mscl_try_fmt_mplane(ctx, f);
> +}
> +
> +static int mscl_m2m_s_fmt_mplane(struct file *file, void *fh,
> +				 struct v4l2_format *f)
> +{
> +	struct mscl_ctx *ctx = fh_to_ctx(fh);
> +	struct vb2_queue *vq;
> +	struct mscl_frame *frame;
> +	struct v4l2_pix_format_mplane *pix;
> +	int i, ret = 0;
> +
> +	ret = mscl_m2m_try_fmt_mplane(file, fh, f);
> +	if (ret)
> +		return ret;
> +
> +	vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type);
> +
> +	if (vb2_is_streaming(vq)) {
> +		dev_dbg(&ctx->mscl_dev->pdev->dev, "queue (%d) busy", f->type);
> +		return -EBUSY;
> +	}
> +
> +	if (V4L2_TYPE_IS_OUTPUT(f->type))
> +		frame = &ctx->s_frame;
> +	else
> +		frame = &ctx->d_frame;
> +
> +	pix = &f->fmt.pix_mp;
> +	frame->fmt = mscl_find_fmt(&pix->pixelformat, NULL, 0);
> +	frame->colorspace = pix->colorspace;
> +	if (!frame->fmt)
> +		return -EINVAL;
> +
> +	for (i = 0; i < frame->fmt->num_planes; i++)
> +		frame->payload[i] = pix->plane_fmt[i].sizeimage;
> +
> +	mscl_set_frame_size(frame, pix->width, pix->height);
> +
> +	if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> +		mscl_ctx_state_lock_set(MSCL_PARAMS | MSCL_DST_FMT, ctx);
> +	else
> +		mscl_ctx_state_lock_set(MSCL_PARAMS | MSCL_SRC_FMT, ctx);
> +
> +	dev_dbg(&ctx->mscl_dev->pdev->dev, "f_w: %d, f_h: %d",
> +					   frame->f_width, frame->f_height);
> +
> +	return 0;
> +}
> +
> +static int mscl_m2m_reqbufs(struct file *file, void *fh,
> +			  struct v4l2_requestbuffers *reqbufs)
> +{
> +	struct mscl_ctx *ctx = fh_to_ctx(fh);
> +	struct mscl_dev *mscl = ctx->mscl_dev;
> +	struct mscl_frame *frame;
> +	u32 max_cnt;

> +	max_cnt = (reqbufs->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) ?
> +		mscl->variant->in_buf_cnt : mscl->variant->out_buf_cnt;

That looks pretty ugly, how about:

	if (reqbufs->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
		max_cnt = mscl->variant->in_buf_cnt;
	else
		max_cnt = mscl->variant->out_buf_cnt;
?
> +	if (reqbufs->count > max_cnt) {
> +		return -EINVAL;

Hell, no. Just adjust it to a supported value. Applications need to check
reqbufs->count upon VIDIOC_REQBUFS return.

> +	} else if (reqbufs->count == 0) {
> +		if (reqbufs->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> +			mscl_ctx_state_lock_clear(MSCL_SRC_FMT, ctx);
> +		else
> +			mscl_ctx_state_lock_clear(MSCL_DST_FMT, ctx);

Such logic shouldn't be here.

> +	}
> +
> +	frame = ctx_get_frame(ctx, reqbufs->type);
> +
> +	return v4l2_m2m_reqbufs(file, ctx->m2m_ctx, reqbufs);
> +}
> +
> +static int mscl_m2m_expbuf(struct file *file, void *fh,
> +				struct v4l2_exportbuffer *eb)
> +{
> +	struct mscl_ctx *ctx = fh_to_ctx(fh);
> +	return v4l2_m2m_expbuf(file, ctx->m2m_ctx, eb);
> +}
> +
> +static int mscl_m2m_querybuf(struct file *file, void *fh,
> +					struct v4l2_buffer *buf)
> +{
> +	struct mscl_ctx *ctx = fh_to_ctx(fh);
> +	return v4l2_m2m_querybuf(file, ctx->m2m_ctx, buf);
> +}
> +
> +static int mscl_m2m_qbuf(struct file *file, void *fh,
> +			  struct v4l2_buffer *buf)
> +{
> +	struct mscl_ctx *ctx = fh_to_ctx(fh);
> +	return v4l2_m2m_qbuf(file, ctx->m2m_ctx, buf);
> +}
> +
> +static int mscl_m2m_dqbuf(struct file *file, void *fh,
> +			   struct v4l2_buffer *buf)
> +{
> +	struct mscl_ctx *ctx = fh_to_ctx(fh);
> +	return v4l2_m2m_dqbuf(file, ctx->m2m_ctx, buf);
> +}
> +
> +static int mscl_m2m_streamon(struct file *file, void *fh,
> +			   enum v4l2_buf_type type)
> +{
> +	struct mscl_ctx *ctx = fh_to_ctx(fh);
> +
> +	/* The source and target color format need to be set */
> +	if (V4L2_TYPE_IS_OUTPUT(type)) {
> +		if (!mscl_ctx_state_is_set(MSCL_SRC_FMT, ctx))
> +			return -EINVAL;
> +	} else if (!mscl_ctx_state_is_set(MSCL_DST_FMT, ctx)) {
> +		return -EINVAL;
> +	}

That's not correct. There should always be some valid format set.
Buffer validation accroding to configred format should be done
in buffer_queue handler.

> +	return v4l2_m2m_streamon(file, ctx->m2m_ctx, type);
> +}
> +
> +static int mscl_m2m_streamoff(struct file *file, void *fh,
> +			    enum v4l2_buf_type type)
> +{
> +	struct mscl_ctx *ctx = fh_to_ctx(fh);
> +	return v4l2_m2m_streamoff(file, ctx->m2m_ctx, type);
> +}

I have prepared some helper functions, so such boiler plate can be avoided.
But this could be handled later. See
http://git.linuxtv.org/snawrocki/samsung.git/shortlog/refs/heads/m2m-helpers-v2

> +/* Return 1 if rectangle a is enclosed in rectangle b, or 0 otherwise. */
> +static int is_rectangle_enclosed(struct v4l2_rect *a, struct v4l2_rect *b)
> +{
> +	if (a->left < b->left || a->top < b->top)
> +		return 0;
> +
> +	if (a->left + a->width > b->left + b->width)
> +		return 0;
> +
> +	if (a->top + a->height > b->top + b->height)
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static int mscl_m2m_g_selection(struct file *file, void *fh,
> +			struct v4l2_selection *s)
> +{
> +	struct mscl_frame *frame;
> +	struct mscl_ctx *ctx = fh_to_ctx(fh);
> +
> +	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) &&
> +	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE))
> +		return -EINVAL;
> +
> +	frame = ctx_get_frame(ctx, s->type);
> +	if (IS_ERR(frame))
> +		return PTR_ERR(frame);
> +
> +	switch (s->target) {
> +	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +		s->r.left = 0;
> +		s->r.top = 0;
> +		s->r.width = frame->f_width;
> +		s->r.height = frame->f_height;
> +		return 0;
> +
> +	case V4L2_SEL_TGT_COMPOSE:
> +	case V4L2_SEL_TGT_CROP:
> +		s->r.left = frame->crop.left;
> +		s->r.top = frame->crop.top;
> +		s->r.width = frame->crop.width;
> +		s->r.height = frame->crop.height;
> +		return 0;
> +	}

Hmm, we still have the selection API not clearly defined for the
mem-to-mem devices...

> +	return -EINVAL;
> +}
> +
> +static int mscl_m2m_s_selection(struct file *file, void *fh,
> +				struct v4l2_selection *s)
> +{
> +	struct mscl_frame *frame;
> +	struct mscl_ctx *ctx = fh_to_ctx(fh);
> +	struct v4l2_crop cr;
> +	struct mscl_variant *variant = ctx->mscl_dev->variant;
> +	int ret;
> +
> +	cr.type = s->type;
> +	cr.c = s->r;
> +
> +	if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) &&
> +	    (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE))
> +		return -EINVAL;
> +
> +	ret = mscl_try_crop(ctx, &cr);
> +	if (ret)
> +		return ret;
> +
> +	if (s->flags & V4L2_SEL_FLAG_LE &&
> +	    !is_rectangle_enclosed(&cr.c, &s->r))
> +		return -ERANGE;
> +
> +	if (s->flags & V4L2_SEL_FLAG_GE &&
> +	    !is_rectangle_enclosed(&s->r, &cr.c))
> +		return -ERANGE;
> +
> +	s->r = cr.c;
> +
> +	switch (s->target) {
> +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> +	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> +	case V4L2_SEL_TGT_COMPOSE:
> +		frame = &ctx->s_frame;
> +		break;
> +
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +	case V4L2_SEL_TGT_CROP:
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +		frame = &ctx->d_frame;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* Check to see if scaling ratio is within supported range */
> +	if (mscl_ctx_state_is_set(MSCL_DST_FMT | MSCL_SRC_FMT, ctx)) {
> +		if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> +			ret = mscl_check_scaler_ratio(variant, cr.c.width,
> +				cr.c.height, ctx->d_frame.crop.width,
> +				ctx->d_frame.crop.height,
> +				ctx->ctrls_mscl.rotate->val);
> +		} else {
> +			ret = mscl_check_scaler_ratio(variant,
> +				ctx->s_frame.crop.width,
> +				ctx->s_frame.crop.height, cr.c.width,
> +				cr.c.height, ctx->ctrls_mscl.rotate->val);
> +		}
> +
> +		if (ret) {
> +			dev_dbg(&ctx->mscl_dev->pdev->dev,
> +				"Out of scaler range");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	frame->crop = cr.c;
> +
> +	mscl_ctx_state_lock_set(MSCL_PARAMS, ctx);
> +	return 0;
> +}
> +
> +static const struct v4l2_ioctl_ops mscl_m2m_ioctl_ops = {
> +	.vidioc_querycap		= mscl_m2m_querycap,
> +	.vidioc_enum_fmt_vid_cap_mplane	= mscl_m2m_enum_fmt_mplane,
> +	.vidioc_enum_fmt_vid_out_mplane	= mscl_m2m_enum_fmt_mplane,
> +	.vidioc_g_fmt_vid_cap_mplane	= mscl_m2m_g_fmt_mplane,
> +	.vidioc_g_fmt_vid_out_mplane	= mscl_m2m_g_fmt_mplane,
> +	.vidioc_try_fmt_vid_cap_mplane	= mscl_m2m_try_fmt_mplane,
> +	.vidioc_try_fmt_vid_out_mplane	= mscl_m2m_try_fmt_mplane,
> +	.vidioc_s_fmt_vid_cap_mplane	= mscl_m2m_s_fmt_mplane,
> +	.vidioc_s_fmt_vid_out_mplane	= mscl_m2m_s_fmt_mplane,
> +	.vidioc_reqbufs			= mscl_m2m_reqbufs,
> +	.vidioc_expbuf                  = mscl_m2m_expbuf,
> +	.vidioc_querybuf		= mscl_m2m_querybuf,
> +	.vidioc_qbuf			= mscl_m2m_qbuf,
> +	.vidioc_dqbuf			= mscl_m2m_dqbuf,
> +	.vidioc_streamon		= mscl_m2m_streamon,
> +	.vidioc_streamoff		= mscl_m2m_streamoff,
> +	.vidioc_g_selection		= mscl_m2m_g_selection,
> +	.vidioc_s_selection		= mscl_m2m_s_selection
> +};
> +
> +static int queue_init(void *priv, struct vb2_queue *src_vq,
> +			struct vb2_queue *dst_vq)
> +{
> +	struct mscl_ctx *ctx = priv;
> +	int ret;
> +
> +	memset(src_vq, 0, sizeof(*src_vq));
> +	src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> +	src_vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
> +	src_vq->drv_priv = ctx;
> +	src_vq->ops = &mscl_m2m_qops;
> +	src_vq->mem_ops = &vb2_dma_contig_memops;
> +	src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> +
> +	ret = vb2_queue_init(src_vq);
> +	if (ret)
> +		return ret;
> +
> +	memset(dst_vq, 0, sizeof(*dst_vq));
> +	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> +	dst_vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
> +	dst_vq->drv_priv = ctx;
> +	dst_vq->ops = &mscl_m2m_qops;
> +	dst_vq->mem_ops = &vb2_dma_contig_memops;
> +	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> +
> +	return vb2_queue_init(dst_vq);
> +}
> +
> +static int mscl_m2m_open(struct file *file)
> +{
> +	struct mscl_dev *mscl = video_drvdata(file);

Don't use video_drvdata(), this is racy! Instead use video_devdata() and
container_of() to get to struct mscl_dev.

> +	struct mscl_ctx *ctx = NULL;
> +	int ret;
> +
> +	dev_dbg(&mscl->pdev->dev,
> +		"pid: %d, state: 0x%lx", task_pid_nr(current), mscl->state);
> +
> +	if (mutex_lock_interruptible(&mscl->lock))
> +		return -ERESTARTSYS;
> +
> +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx) {
> +		ret = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	v4l2_fh_init(&ctx->fh, mscl->m2m.vfd);
> +	ret = mscl_ctrls_create(ctx);
> +	if (ret)
> +		goto error_fh;
> +
> +	/* Use separate control handler per file handle */
> +	ctx->fh.ctrl_handler = &ctx->ctrl_handler;
> +	file->private_data = &ctx->fh;
> +	v4l2_fh_add(&ctx->fh);
> +
> +	ctx->mscl_dev = mscl;
> +	/* Default color format */
> +	ctx->s_frame.fmt = mscl_get_format(0);
> +	ctx->d_frame.fmt = mscl_get_format(0);
> +	/* Setup the device context for mem2mem mode. */
> +	ctx->state = MSCL_CTX_M2M;
> +	ctx->flags = 0;
> +
> +	ctx->m2m_ctx = v4l2_m2m_ctx_init(mscl->m2m.m2m_dev, ctx, queue_init);
> +	if (IS_ERR(ctx->m2m_ctx)) {
> +		dev_dbg(&mscl->pdev->dev, "Failed to initialize m2m context");
> +		ret = PTR_ERR(ctx->m2m_ctx);
> +		goto error_ctrls;
> +	}
> +
> +	if (mscl->m2m.refcnt++ == 0)
> +		set_bit(ST_M2M_OPEN, &mscl->state);
> +
> +	dev_dbg(&mscl->pdev->dev, "mscl m2m driver is opened, ctx(0x%p)", ctx);
> +
> +	mutex_unlock(&mscl->lock);
> +	return 0;
> +
> +error_ctrls:
> +	mscl_ctrls_delete(ctx);
> +error_fh:
> +	v4l2_fh_del(&ctx->fh);
> +	v4l2_fh_exit(&ctx->fh);
> +	kfree(ctx);
> +unlock:
> +	mutex_unlock(&mscl->lock);
> +	return ret;
> +}
> +
> +static int mscl_m2m_release(struct file *file)
> +{
> +	struct mscl_ctx *ctx = fh_to_ctx(file->private_data);
> +	struct mscl_dev *mscl = ctx->mscl_dev;
> +
> +	dev_dbg(&mscl->pdev->dev, "pid: %d, state: 0x%lx, refcnt= %d",
> +		task_pid_nr(current), mscl->state, mscl->m2m.refcnt);
> +
> +	mutex_lock(&mscl->lock);
> +
> +	v4l2_m2m_ctx_release(ctx->m2m_ctx);
> +	mscl_ctrls_delete(ctx);
> +	v4l2_fh_del(&ctx->fh);
> +	v4l2_fh_exit(&ctx->fh);
> +
> +	if (--mscl->m2m.refcnt <= 0)
> +		clear_bit(ST_M2M_OPEN, &mscl->state);
> +	kfree(ctx);
> +
> +	mutex_unlock(&mscl->lock);
> +	return 0;
> +}
> +
> +static unsigned int mscl_m2m_poll(struct file *file,
> +					struct poll_table_struct *wait)
> +{
> +	struct mscl_ctx *ctx = fh_to_ctx(file->private_data);
> +	struct mscl_dev *mscl = ctx->mscl_dev;
> +	int ret;
> +
> +	if (mutex_lock_interruptible(&mscl->lock))
> +		return -ERESTARTSYS;
> +
> +	ret = v4l2_m2m_poll(file, ctx->m2m_ctx, wait);
> +	mutex_unlock(&mscl->lock);
> +
> +	return ret;
> +}
> +
> +static int mscl_m2m_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +	struct mscl_ctx *ctx = fh_to_ctx(file->private_data);
> +	struct mscl_dev *mscl = ctx->mscl_dev;
> +	int ret;
> +
> +	if (mutex_lock_interruptible(&mscl->lock))
> +		return -ERESTARTSYS;
> +
> +	ret = v4l2_m2m_mmap(file, ctx->m2m_ctx, vma);
> +	mutex_unlock(&mscl->lock);
> +
> +	return ret;
> +}
> +
> +static const struct v4l2_file_operations mscl_m2m_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= mscl_m2m_open,
> +	.release	= mscl_m2m_release,
> +	.poll		= mscl_m2m_poll,
> +	.unlocked_ioctl	= video_ioctl2,
> +	.mmap		= mscl_m2m_mmap,
> +};
> +
> +static struct v4l2_m2m_ops mscl_m2m_ops = {
> +	.device_run	= mscl_m2m_device_run,
> +	.job_abort	= mscl_m2m_job_abort,
> +};
> +
> +int mscl_register_m2m_device(struct mscl_dev *mscl)
> +{
> +	struct platform_device *pdev;
> +	int ret;
> +
> +	if (!mscl)
> +		return -ENODEV;
> +
> +	pdev = mscl->pdev;
> +
> +	mscl->vdev.fops		= &mscl_m2m_fops;
> +	mscl->vdev.ioctl_ops	= &mscl_m2m_ioctl_ops;
> +	mscl->vdev.release	= video_device_release_empty;

You need to provide this callback, and it would kfree struct mscl_dev.

> +	mscl->vdev.lock		= &mscl->lock;
> +	mscl->vdev.vfl_dir	= VFL_DIR_M2M;
> +	snprintf(mscl->vdev.name, sizeof(mscl->vdev.name), "%s.%d:m2m",
> +					MSCL_MODULE_NAME, mscl->id);

So video device names also use the id which comes from DT alias node.
Is it really needed to be able to identify to which IP instance
a /dev/video? netry corresponds ? I guess it is meaningles, given that
those IPs are identical ?

> +	video_set_drvdata(&mscl->vdev, mscl);
> +
> +	mscl->m2m.vfd = &mscl->vdev;
> +	mscl->m2m.m2m_dev = v4l2_m2m_init(&mscl_m2m_ops);
> +	if (IS_ERR(mscl->m2m.m2m_dev)) {
> +		dev_err(&pdev->dev, "failed to initialize v4l2-m2m device\n");
> +		return PTR_ERR(mscl->m2m.m2m_dev);
> +	}
> +
> +	ret = video_register_device(&mscl->vdev, VFL_TYPE_GRABBER, -1);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			 "%s(): failed to register video device\n", __func__);
> +		v4l2_m2m_release(mscl->m2m.m2m_dev);
> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev,
> +		 "mscl m2m driver registered as /dev/video%d", mscl->vdev.num);

s/mscl m2m/SCALER ?

> +	return 0;
> +}
> +
> +void mscl_unregister_m2m_device(struct mscl_dev *mscl)
> +{
> +	if (mscl)
> +		v4l2_m2m_release(mscl->m2m.m2m_dev);

Are you calling video_unregister_device() somewhere ?

> +}

--
Regards,
Sylwester