Hi Philipp, Hans,
2015-07-24 17:12 GMT+02:00 Hans Verkuil <hverkuil@xxxxxxxxx>:
> Hi Philipp,
>
> A quick review of this driver:
>
> On 07/16/2015 06:24 PM, Philipp Zabel wrote:
>> From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
>>
>> This patch adds support for hardware accelerated scaling and color
>> space conversion between memory buffers using the IPUv3 IC.
>> Since the maximum output size of the IC unit is 1024x1024 pixels, multiple
>> IC tasks with overlapping tiles are used internally to scale and convert
>> larger frames.
>>
>> The IC operates with a burst size of at least 8 pixels. Depending on the
>> frame width and scaling factor, up to 7 junk pixels may be written after
>> the end of the frame. The sizeimage is increased accordingly.
>>
>> Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
>> Signed-off-by: Michael Olbrich <m.olbrich@xxxxxxxxxxxxxx>
>> Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
>> ---
>> Changes since v2:
>> - Disabled USERPTR memory, hardware doesn't support it.
>> - Embedded struct video_device in ipu_scale_dev.
>> - Set icc pointer to NULL on error.
>> - Fixed module description.
>> ---
>> drivers/media/platform/imx/Kconfig | 9 +
>> drivers/media/platform/imx/Makefile | 1 +
>> drivers/media/platform/imx/imx-ipu-scaler.c | 859 ++++++++++++++++++++++++++++
>> drivers/media/platform/imx/imx-ipu.c | 2 +-
>> drivers/media/platform/imx/imx-ipu.h | 1 +
>> 5 files changed, 871 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/media/platform/imx/imx-ipu-scaler.c
>>
>> diff --git a/drivers/media/platform/imx/Kconfig b/drivers/media/platform/imx/Kconfig
>> index a90c973..4694367 100644
>> --- a/drivers/media/platform/imx/Kconfig
>> +++ b/drivers/media/platform/imx/Kconfig
>> @@ -1,2 +1,11 @@
>> config VIDEO_IMX_IPU_COMMON
>> tristate
>> +
>> +config VIDEO_IMX_IPU_SCALER
>> + tristate "i.MX5/6 IPUv3 based image scaler driver"
>> + depends on VIDEO_DEV && IMX_IPUV3_CORE
>> + select VIDEOBUF2_DMA_CONTIG
>> + select VIDEO_IMX_IPU_COMMON
>> + select V4L2_MEM2MEM_DEV
>> + ---help---
>> + This is a v4l2 scaler video driver for the IPUv3 on i.MX5/6.
>> diff --git a/drivers/media/platform/imx/Makefile b/drivers/media/platform/imx/Makefile
>> index 5de119c..f20aa0b 100644
>> --- a/drivers/media/platform/imx/Makefile
>> +++ b/drivers/media/platform/imx/Makefile
>> @@ -1 +1,2 @@
>> obj-$(CONFIG_VIDEO_IMX_IPU_COMMON) += imx-ipu.o
>> +obj-$(CONFIG_VIDEO_IMX_IPU_SCALER) += imx-ipu-scaler.o
>> diff --git a/drivers/media/platform/imx/imx-ipu-scaler.c b/drivers/media/platform/imx/imx-ipu-scaler.c
>> new file mode 100644
>> index 0000000..6c6d0aa
>> --- /dev/null
>> +++ b/drivers/media/platform/imx/imx-ipu-scaler.c
>> @@ -0,0 +1,860 @@
>> +/*
>> + * i.MX IPUv3 scaler driver
>> + *
>> + * Copyright (C) 2011 Sascha Hauer, Pengutronix
>> + *
>> + * based on the mem2mem test 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.
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +#include <linux/module.h>
>> +#include <linux/delay.h>
>> +#include <linux/fs.h>
>> +#include <linux/version.h>
>> +#include <linux/sched.h>
>> +#include <linux/slab.h>
>> +#include <video/imx-ipu-v3.h>
>> +
>> +#include <linux/platform_device.h>
>> +#include <media/v4l2-mem2mem.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-ioctl.h>
>> +#include <media/videobuf2-dma-contig.h>
>> +
>> +#include "imx-ipu.h"
>> +
>> +#define MIN_W 32
>> +#define MIN_H 32
>> +#define MAX_W 4096
>> +#define MAX_H 4096
>> +#define DIM_ALIGN_MASK 0x08 /* 8-alignment for dimensions */
>> +
>> +/* Flags that indicate a format can be used for capture/output */
>> +#define MEM2MEM_CAPTURE (1 << 0)
>> +#define MEM2MEM_OUTPUT (1 << 1)
>> +
>> +#define MEM2MEM_NAME "imx-ipuv3-scale"
>> +
>> +/* Per queue */
>> +#define MEM2MEM_DEF_NUM_BUFS VIDEO_MAX_FRAME
>> +/* In bytes, per queue */
>> +#define MEM2MEM_VID_MEM_LIMIT (64 * 1024 * 1024)
>> +
>> +#define fh_to_ctx(__fh) container_of(__fh, struct ipu_scale_ctx, fh)
>> +
>> +enum {
>> + V4L2_M2M_SRC = 0,
>> + V4L2_M2M_DST = 1,
>> +};
>> +
>> +struct ipu_scale_dev {
>> + struct v4l2_device v4l2_dev;
>> + struct video_device vfd;
>> + struct device *dev;
>> + struct ipu_soc *ipu;
>> +
>> + atomic_t num_inst;
>> + spinlock_t irqlock;
>> +
>> + struct v4l2_m2m_dev *m2m_dev;
>> + struct mutex dev_mutex;
>> +};
>> +
>> +/* Per-queue, driver-specific private data */
>> +struct ipu_scale_q_data {
>> + struct v4l2_pix_format cur_fmt;
>> + struct v4l2_rect rect;
>> +};
>> +
>> +struct ipu_scale_ctx {
>> + struct ipu_scale_dev *ipu_scaler;
>> +
>> + struct v4l2_fh fh;
>> + struct vb2_alloc_ctx *alloc_ctx;
>> + struct ipu_scale_q_data q_data[2];
>> + struct work_struct work;
>> + struct completion completion;
>> + struct work_struct skip_run;
>> + int error;
>> + int aborting;
>> + enum ipu_image_scale_ctrl ctrl;
>> + struct image_convert_ctx *icc;
>> + int num_tiles;
>> +};
>> +
>> +static struct ipu_scale_q_data *get_q_data(struct ipu_scale_ctx *ctx,
>> + enum v4l2_buf_type type)
>> +{
>> + switch (type) {
>> + case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>> + return &ctx->q_data[V4L2_M2M_SRC];
>> + case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>> + return &ctx->q_data[V4L2_M2M_DST];
>> + default:
>> + BUG();
>> + }
>> + return NULL;
>> +}
>> +
>> +/*
>> + * mem2mem callbacks
>> + */
>> +
>> +/**
>> + * job_ready() - check whether an instance is ready to be scheduled to run
>> + */
>> +static int job_ready(void *priv)
>> +{
>> + struct ipu_scale_ctx *ctx = priv;
>> +
>> + if (ctx->aborting)
>> + return 0;
>> +
>> + if (v4l2_m2m_num_src_bufs_ready(ctx->fh.m2m_ctx) < 1
>> + || v4l2_m2m_num_dst_bufs_ready(ctx->fh.m2m_ctx) < 1) {
>> + dev_dbg(ctx->ipu_scaler->dev, "Not enough buffers available\n");
>> + return 0;
>> + }
>> +
>> + return 1;
>> +}
>> +
>> +static void job_abort(void *priv)
>> +{
>> + struct ipu_scale_ctx *ctx = priv;
>> +
>> + ctx->aborting = 1;
>> +}
>> +
>> +static void ipu_complete(void *priv, int err)
>> +{
>> + struct ipu_scale_dev *ipu_scaler = priv;
>> + struct ipu_scale_ctx *curr_ctx;
>> +
>> + curr_ctx = v4l2_m2m_get_curr_priv(ipu_scaler->m2m_dev);
>> +
>> + if (NULL == curr_ctx) {
>> + dev_dbg(ipu_scaler->dev,
>> + "Instance released before the end of transaction\n");
>> + return;
>> + }
>> +
>> + curr_ctx->error = err;
>> + complete(&curr_ctx->completion);
>> +}
>> +
>> +static void device_run(void *priv)
>> +{
>> + struct ipu_scale_ctx *ctx = priv;
>> +
>> + schedule_work(&ctx->work);
>> +}
>> +
>> +static void ipu_scaler_work(struct work_struct *work)
>> +{
>> + struct ipu_scale_ctx *ctx = container_of(work, struct ipu_scale_ctx,
>> + work);
>> + struct ipu_scale_dev *ipu_scaler = ctx->ipu_scaler;
>> + struct vb2_buffer *src_buf, *dst_buf;
>> + struct ipu_scale_q_data *q_data;
>> + struct v4l2_pix_format *pix;
>> + struct ipu_image in, out;
>> + int err = -ETIMEDOUT;
>> + unsigned long flags;
>> +
>> + /*
>> + * If streamoff dequeued all buffers before we could get the lock,
>> + * just bail out immediately.
>> + */
>> + if (!v4l2_m2m_num_src_bufs_ready(ctx->fh.m2m_ctx) ||
>> + !v4l2_m2m_num_dst_bufs_ready(ctx->fh.m2m_ctx)) {
>> + WARN_ON(1);
>> + schedule_work(&ctx->skip_run);
>> + return;
>> + }
>> +
>> + src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
>> + dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
>> +
>> + in.phys0 = vb2_dma_contig_plane_dma_addr(src_buf, 0);
>> + out.phys0 = vb2_dma_contig_plane_dma_addr(dst_buf, 0);
>> +
>> + if (!ctx->num_tiles) {
>> + q_data = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
>> + pix = &q_data->cur_fmt;
>> +
>> + in.pix.width = pix->width;
>> + in.pix.height = pix->height;
>> + in.pix.bytesperline = pix->bytesperline;
>> + in.pix.pixelformat = pix->pixelformat;
>> + in.rect = q_data->rect;
>> +
>> + q_data = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>> + pix = &q_data->cur_fmt;
>> +
>> + out.pix.width = pix->width;
>> + out.pix.height = pix->height;
>> + out.pix.bytesperline = pix->bytesperline;
>> + out.pix.pixelformat = pix->pixelformat;
>> + out.rect = q_data->rect;
>> +
>> + kfree(ctx->icc);
>> + ctx->icc = ipu_image_convert_prepare(ipu_scaler->ipu, &in,
>> + &out, ctx->ctrl,
>> + &ctx->num_tiles);
>> + if (IS_ERR(ctx->icc)) {
>> + ctx->icc = NULL;
>> + schedule_work(&ctx->skip_run);
>> + return;
>> + }
>> + }
>> +
>> + ipu_image_convert_run(ipu_scaler->ipu, &in, &out, ctx->icc,
>> + ctx->num_tiles, ipu_complete, ipu_scaler, false);
>> +
>> + if (!wait_for_completion_timeout(&ctx->completion,
>> + msecs_to_jiffies(300))) {
>> + dev_err(ipu_scaler->dev,
>> + "Timeout waiting for scaling result\n");
>> + err = -ETIMEDOUT;
>> + } else {
>> + err = ctx->error;
>> + }
>> +
>> + src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
>> + dst_buf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
>> +
>> + dst_buf->v4l2_buf.timestamp = src_buf->v4l2_buf.timestamp;
>> + dst_buf->v4l2_buf.timecode = src_buf->v4l2_buf.timecode;
>> +
>> + spin_lock_irqsave(&ipu_scaler->irqlock, flags);
>> + v4l2_m2m_buf_done(src_buf, err ? VB2_BUF_STATE_ERROR :
>> + VB2_BUF_STATE_DONE);
>> + v4l2_m2m_buf_done(dst_buf, err ? VB2_BUF_STATE_ERROR :
>> + VB2_BUF_STATE_DONE);
>> + spin_unlock_irqrestore(&ipu_scaler->irqlock, flags);
>> +
>> + v4l2_m2m_job_finish(ipu_scaler->m2m_dev, ctx->fh.m2m_ctx);
>> +}
>> +
>> +/*
>> + * video ioctls
>> + */
>> +static int vidioc_querycap(struct file *file, void *priv,
>> + struct v4l2_capability *cap)
>> +{
>> + strncpy(cap->driver, MEM2MEM_NAME, sizeof(cap->driver) - 1);
>> + strncpy(cap->card, MEM2MEM_NAME, sizeof(cap->card) - 1);
>> + strncpy(cap->bus_info, "platform:" MEM2MEM_NAME,
>> + sizeof(cap->bus_info) - 1);
>> + /*
>> + * This is only a mem-to-mem video device. The capture and output
>> + * device capability flags are left for backward compatibility and
>> + * are scheduled for removal.
>> + */
>> + cap->device_caps = V4L2_CAP_VIDEO_M2M | V4L2_CAP_STREAMING;
>> + cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
>> +
>> + return 0;
>> +}
>> +
>> +static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv,
>> + struct v4l2_fmtdesc *f)
>> +{
>> + return ipu_enum_fmt(file, priv, f);
>> +}
>> +
>> +static int vidioc_enum_fmt_vid_out(struct file *file, void *priv,
>> + struct v4l2_fmtdesc *f)
>> +{
>> + return ipu_enum_fmt(file, priv, f);
>> +}
>> +
>> +static int vidioc_g_fmt(struct ipu_scale_ctx *ctx, struct v4l2_format *f)
>> +{
>> + struct vb2_queue *vq;
>> + struct ipu_scale_q_data *q_data;
>> + struct v4l2_pix_format *pix;
>> +
>> + vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
>> + if (!vq)
>> + return -EINVAL;
>> +
>> + q_data = get_q_data(ctx, f->type);
>> + pix = &q_data->cur_fmt;
>> +
>> + return ipu_g_fmt(f, pix);
>> +}
>> +
>> +static int vidioc_g_fmt_vid_out(struct file *file, void *priv,
>> + struct v4l2_format *f)
>> +{
>> + return vidioc_g_fmt(priv, f);
>> +}
>> +
>> +static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
>> + struct v4l2_format *f)
>> +{
>> + return vidioc_g_fmt(priv, f);
>> +}
>
> Why these wrappers? A single vidioc_g_fmt should suffice.
>
>> +
>> +static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
>> + struct v4l2_format *f)
>> +{
>> + int ret;
>> +
>> + ret = ipu_try_fmt(file, priv, f);
>> +
>> + /*
>> + * Leave enough output space for worst-case overhead caused by 8 pixel
>> + * burst size: 7 RGBA pixels.
>> + */
>> + f->fmt.pix.sizeimage += 7 * 4;
>> +
>> + return ret;
>> +}
>> +
>> +static int vidioc_try_fmt_vid_out(struct file *file, void *priv,
>> + struct v4l2_format *f)
>> +{
>> + f->fmt.pix.width &= ~0x7;
>> + return ipu_try_fmt(file, priv, f);
>> +}
>> +
>> +static int vidioc_s_fmt(struct file *file, void *priv,
>> + struct v4l2_format *f)
>> +{
>> + struct ipu_scale_q_data *q_data;
>> + struct vb2_queue *vq;
>> + struct ipu_scale_ctx *ctx = fh_to_ctx(priv);
>> + int ret;
>> +
>> + vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
>> + if (!vq)
>> + return -EINVAL;
>> +
>> + q_data = get_q_data(ctx, f->type);
>> + if (!q_data)
>> + return -EINVAL;
>> +
>> + if (vb2_is_busy(vq)) {
>> + v4l2_err(&ctx->ipu_scaler->v4l2_dev, "%s queue busy\n",
>> + __func__);
>> + return -EBUSY;
>> + }
>> +
>> + ret = ipu_s_fmt(file, priv, f, &q_data->cur_fmt);
>> + if (ret < 0)
>> + return ret;
>> +
>> + /*
>> + * Leave enough output space for worst-case overhead caused by 8 pixel
>> + * burst size: 7 RGBA pixels.
>> + */
>> + q_data->cur_fmt.sizeimage = f->fmt.pix.sizeimage;
>> + if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> + f->fmt.pix.sizeimage += 7 * 4;
>
> This should also be done in try_fmt! Otherwise what try_fmt and s_fmt return as the
> sizeimage would be inconsistent. And I think g_fmt needs the same adjustment if I
> understand the code correctly.
>
>> +
>> + /* Reset cropping/composing rectangle */
>> + q_data->rect.left = 0;
>> + q_data->rect.top = 0;
>> + q_data->rect.width = q_data->cur_fmt.width;
>> + q_data->rect.height = q_data->cur_fmt.height;
>> +
>> + /* reset scaling ctrl */
>> + ctx->ctrl = IPU_IMAGE_SCALE_PIXELPERFECT;
>> +
>> + return 0;
>> +}
>> +
>> +static int vidioc_g_selection(struct file *file, void *priv,
>> + struct v4l2_selection *s)
>> +{
>> + struct ipu_scale_ctx *ctx = fh_to_ctx(priv);
>> + struct ipu_scale_q_data *q_data;
>> +
>> + switch (s->target) {
>> + case V4L2_SEL_TGT_CROP:
>> + if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
>> + return -EINVAL;
>> + q_data = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
>> + s->r.left = 0;
>> + s->r.top = 0;
>> + s->r.width = q_data->cur_fmt.width;
>> + s->r.height = q_data->cur_fmt.height;
>
> Shouldn't this be s->r = q_data->rect?
>
>> + break;
>> + case V4L2_SEL_TGT_CROP_DEFAULT:
>> + case V4L2_SEL_TGT_CROP_BOUNDS:
>> + if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
>> + return -EINVAL;
>> + q_data = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
>> + s->r.left = 0;
>> + s->r.top = 0;
>> + s->r.width = q_data->cur_fmt.width;
>> + s->r.height = q_data->cur_fmt.height;
>> + break;
>> + case V4L2_SEL_TGT_COMPOSE:
>> + if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> + return -EINVAL;
>> + q_data = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>> + s->r = q_data->rect;
>> + break;
>> + case V4L2_SEL_TGT_COMPOSE_DEFAULT:
>> + case V4L2_SEL_TGT_COMPOSE_BOUNDS:
>> + case V4L2_SEL_TGT_COMPOSE_PADDED:
>> + if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> + return -EINVAL;
>> + q_data = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>> + s->r.left = 0;
>> + s->r.top = 0;
>> + s->r.width = q_data->cur_fmt.width;
>> + s->r.height = q_data->cur_fmt.height;
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int vidioc_s_selection(struct file *file, void *priv,
>> + struct v4l2_selection *s)
>> +{
>> + struct ipu_scale_ctx *ctx = fh_to_ctx(priv);
>> + struct ipu_scale_q_data *q_data;
>> +
>> + switch (s->target) {
>> + case V4L2_SEL_TGT_CROP:
>> + if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
>> + return -EINVAL;
>> + break;
>> + case V4L2_SEL_TGT_COMPOSE:
>> + if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> + return -EINVAL;
>> + ctx->ctrl = IPU_IMAGE_SCALE_ROUND_DOWN;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + q_data = get_q_data(ctx, s->type);
>> +
>> + /* The input's frame width to the IC must be a multiple of 8 pixels
>> + * When performing resizing the frame width must be multiple of burst
>> + * size - 8 or 16 pixels as defined by CB#_BURST_16 parameter.
>> + */
>> + if (s->flags & V4L2_SEL_FLAG_GE)
>> + s->r.width = round_up(s->r.width, 8);
>> + if (s->flags & V4L2_SEL_FLAG_LE)
>> + s->r.width = round_down(s->r.width, 8);
>> + s->r.width = clamp_t(unsigned int, s->r.width, 8,
>> + round_down(q_data->cur_fmt.width, 8));
>> + s->r.height = clamp_t(unsigned int, s->r.height, 1,
>> + q_data->cur_fmt.height);
>> + s->r.left = clamp_t(unsigned int, s->r.left, 0,
>> + q_data->cur_fmt.width - s->r.width);
>> + s->r.top = clamp_t(unsigned int, s->r.top, 0,
>> + q_data->cur_fmt.height - s->r.height);
>> +
>> + /* V4L2_SEL_FLAG_KEEP_CONFIG is only valid for subdevices */
>> + q_data->rect = s->r;
>> + ctx->num_tiles = 0;
>> +
>> + return 0;
>> +}
>> +
>> +static int vidioc_enum_framesizes(struct file *file, void *fh,
>> + struct v4l2_frmsizeenum *fsize)
>> +{
>> + return ipu_enum_framesizes(file, fh, fsize);
>> +}
>> +
>> +static const struct v4l2_ioctl_ops ipu_scale_ioctl_ops = {
>> + .vidioc_querycap = vidioc_querycap,
>> +
>> + .vidioc_enum_fmt_vid_cap = vidioc_enum_fmt_vid_cap,
>> + .vidioc_g_fmt_vid_cap = vidioc_g_fmt_vid_cap,
>> + .vidioc_try_fmt_vid_cap = vidioc_try_fmt_vid_cap,
>> + .vidioc_s_fmt_vid_cap = vidioc_s_fmt,
>> +
>> + .vidioc_enum_fmt_vid_out = vidioc_enum_fmt_vid_out,
>> + .vidioc_g_fmt_vid_out = vidioc_g_fmt_vid_out,
>> + .vidioc_try_fmt_vid_out = vidioc_try_fmt_vid_out,
>> + .vidioc_s_fmt_vid_out = vidioc_s_fmt,
>> +
>> + .vidioc_g_selection = vidioc_g_selection,
>> + .vidioc_s_selection = vidioc_s_selection,
>> +
>> + .vidioc_reqbufs = v4l2_m2m_ioctl_reqbufs,
>> + .vidioc_querybuf = v4l2_m2m_ioctl_querybuf,
>> +
>> + .vidioc_qbuf = v4l2_m2m_ioctl_qbuf,
>> + .vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
>> + .vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf,
>> + .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
>> +
>> + .vidioc_streamon = v4l2_m2m_ioctl_streamon,
>> + .vidioc_streamoff = v4l2_m2m_ioctl_streamoff,
>> +
>> + .vidioc_enum_framesizes = vidioc_enum_framesizes,
>> +};
>> +
>> +static void ipu_scale_skip_run(struct work_struct *work)
>> +{
>> + struct ipu_scale_ctx *ctx = container_of(work, struct ipu_scale_ctx,
>> + skip_run);
>> +
>> + v4l2_m2m_job_finish(ctx->ipu_scaler->m2m_dev, ctx->fh.m2m_ctx);
>> +}
>> +
>> +
>> +/*
>> + * Queue operations
>> + */
>> +
>> +static int ipu_scale_queue_setup(struct vb2_queue *vq,
>> + const struct v4l2_format *fmt,
>> + unsigned int *nbuffers,
>> + unsigned int *nplanes, unsigned int sizes[],
>> + void *alloc_ctxs[])
>> +{
>> + struct ipu_scale_ctx *ctx = vb2_get_drv_priv(vq);
>> + struct ipu_scale_q_data *q_data;
>> + unsigned int size, count = *nbuffers;
>> + struct v4l2_pix_format *pix;
>> +
>> + q_data = get_q_data(ctx, vq->type);
>> + pix = &q_data->cur_fmt;
>> +
>> + size = pix->sizeimage;
>> + /*
>> + * Leave enough output space for worst-case overhead caused by 8 pixel
>> + * burst size: 7 RGBA pixels.
>> + */
>> + if (vq->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> + size += 7 * 4;
>
> pix->sizeimage should have this adjustment already.
>
>> +
>> + while (size * count > MEM2MEM_VID_MEM_LIMIT)
>> + (count)--;
>
> Why would you want this check?
>
>> +
>> + *nplanes = 1;
>> + *nbuffers = count;
>> + sizes[0] = size;
>> +
>> + ctx->alloc_ctx = vb2_dma_contig_init_ctx(ctx->ipu_scaler->dev);
>
> This is wrong: do this in the probe() function.
>
> queue_setup can be called multiple times if VIDIOC_CREATE_BUFS is used, and then
> you would allocate this multiple times.
>
>> + if (IS_ERR(ctx->alloc_ctx))
>> + return PTR_ERR(ctx->alloc_ctx);
>> +
>> + alloc_ctxs[0] = ctx->alloc_ctx;
>> +
>> + dev_dbg(ctx->ipu_scaler->dev, "get %d buffer(s) of size %d each.\n",
>> + count, size);
>> +
>> + return 0;
>> +}
>> +
>> +static int ipu_scale_buf_prepare(struct vb2_buffer *vb)
>> +{
>> + struct ipu_scale_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
>> + struct ipu_scale_q_data *q_data;
>> + struct v4l2_pix_format *pix;
>> + unsigned int plane_size;
>> +
>> + dev_dbg(ctx->ipu_scaler->dev, "type: %d\n", vb->vb2_queue->type);
>> +
>> + q_data = get_q_data(ctx, vb->vb2_queue->type);
>> + pix = &q_data->cur_fmt;
>> + plane_size = pix->sizeimage;
>> +
>> + /*
>> + * Leave enough output space for worst-case overhead caused by 8 pixel
>> + * burst size: 7 RGBA pixels.
>> + */
>> + if (vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> + plane_size += 7 * 4;
>
> As before: should be part of pix->sizeimage already.
>
>> + if (vb2_plane_size(vb, 0) < plane_size) {
>> + dev_dbg(ctx->ipu_scaler->dev,
>> + "%s data will not fit into plane (%lu < %lu)\n",
>> + __func__, vb2_plane_size(vb, 0),
>> + (long)plane_size);
>> + return -EINVAL;
>> + }
>> +
>> + vb2_set_plane_payload(vb, 0, pix->sizeimage);
>> +
>> + return 0;
>> +}
>> +
>> +static void ipu_scale_buf_queue(struct vb2_buffer *vb)
>> +{
>> + struct ipu_scale_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
>> +
>> + v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vb);
>> +}
>> +
>> +static void ipu_scale_stop_streaming(struct vb2_queue *q)
>> +{
>> + struct ipu_scale_ctx *ctx = vb2_get_drv_priv(q);
>> + struct vb2_buffer *buf;
>> +
>> + if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
>> + while ((buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx)))
>> + v4l2_m2m_buf_done(buf, VB2_BUF_STATE_ERROR);
>> + } else {
>> + while ((buf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx)))
>> + v4l2_m2m_buf_done(buf, VB2_BUF_STATE_ERROR);
>> + }
>> +}
>> +
>> +static struct vb2_ops ipu_scale_qops = {
>> + .queue_setup = ipu_scale_queue_setup,
>> + .buf_prepare = ipu_scale_buf_prepare,
>> + .buf_queue = ipu_scale_buf_queue,
>> + .wait_prepare = vb2_ops_wait_prepare,
>> + .wait_finish = vb2_ops_wait_finish,
>> + .stop_streaming = ipu_scale_stop_streaming,
>> +};
>> +
>> +static int queue_init(void *priv, struct vb2_queue *src_vq,
>> + struct vb2_queue *dst_vq)
>> +{
>> + struct ipu_scale_ctx *ctx = priv;
>> + int ret;
>> +
>> + memset(src_vq, 0, sizeof(*src_vq));
>> + src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
>> + src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
>> + src_vq->drv_priv = ctx;
>> + src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
>> + src_vq->ops = &ipu_scale_qops;
>> + src_vq->mem_ops = &vb2_dma_contig_memops;
>> + src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
>> + src_vq->lock = &ctx->ipu_scaler->dev_mutex;
>> +
>> + 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;
>> + dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
>> + dst_vq->drv_priv = ctx;
>> + dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
>> + dst_vq->ops = &ipu_scale_qops;
>> + dst_vq->mem_ops = &vb2_dma_contig_memops;
>> + dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
>> + dst_vq->lock = &ctx->ipu_scaler->dev_mutex;
>> +
>> + return vb2_queue_init(dst_vq);
>> +}
>> +
>> +/*
>> + * File operations
>> + */
>> +static int ipu_scale_open(struct file *file)
>> +{
>> + struct ipu_scale_dev *ipu_scaler = video_drvdata(file);
>> + struct ipu_scale_ctx *ctx = NULL;
>> + const int width = 720;
>> + const int height = 576;
>> + int i;
>> +
>> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>> + if (!ctx)
>> + return -ENOMEM;
>> +
>> + INIT_WORK(&ctx->skip_run, ipu_scale_skip_run);
>> + INIT_WORK(&ctx->work, ipu_scaler_work);
>> + init_completion(&ctx->completion);
>> + v4l2_fh_init(&ctx->fh, video_devdata(file));
>> + file->private_data = &ctx->fh;
>> + v4l2_fh_add(&ctx->fh);
>> + ctx->ipu_scaler = ipu_scaler;
>> +
>> + ctx->fh.m2m_ctx = v4l2_m2m_ctx_init(ipu_scaler->m2m_dev, ctx,
>> + &queue_init);
>> + if (IS_ERR(ctx->fh.m2m_ctx)) {
>> + int ret = PTR_ERR(ctx->fh.m2m_ctx);
>> +
>> + kfree(ctx);
>> + return ret;
>> + }
>> +
>> + for (i = 0; i < 2; i++) {
>> + ctx->q_data[i].cur_fmt.width = width;
>> + ctx->q_data[i].cur_fmt.height = height;
>> + ctx->q_data[i].cur_fmt.bytesperline = width;
>> + ctx->q_data[i].cur_fmt.pixelformat = V4L2_PIX_FMT_YUV420;
>> + ctx->q_data[i].cur_fmt.sizeimage = width * height * 3 / 2;
>
> The adjustment of 7 * 4 should be added here as well.
>
>> + ctx->q_data[i].cur_fmt.colorspace = V4L2_COLORSPACE_REC709;
>> + ctx->q_data[i].rect.left = 0;
>> + ctx->q_data[i].rect.top = 0;
>> + ctx->q_data[i].rect.width = width;
>> + ctx->q_data[i].rect.height = height;
>> + }
>> +
>> + ctx->ctrl = IPU_IMAGE_SCALE_PIXELPERFECT;
>> + ctx->num_tiles = 0;
>> +
>> + atomic_inc(&ipu_scaler->num_inst);
>> +
>> + dev_dbg(ipu_scaler->dev, "Created instance %p, m2m_ctx: %p\n",
>> + ctx, ctx->fh.m2m_ctx);
>> +
>> + return 0;
>> +}
>> +
>> +static int ipu_scale_release(struct file *file)
>> +{
>> + struct ipu_scale_dev *ipu_scaler = video_drvdata(file);
>> + struct ipu_scale_ctx *ctx = fh_to_ctx(file->private_data);
>> +
>> + dev_dbg(ipu_scaler->dev, "Releasing instance %p\n", ctx);
>> +
>> + v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
>> + v4l2_fh_del(&ctx->fh);
>> + v4l2_fh_exit(&ctx->fh);
>> + kfree(ctx->icc);
>> + kfree(ctx);
>> +
>> + atomic_dec(&ipu_scaler->num_inst);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct v4l2_file_operations ipu_scale_fops = {
>> + .owner = THIS_MODULE,
>> + .open = ipu_scale_open,
>> + .release = ipu_scale_release,
>> + .poll = v4l2_m2m_fop_poll,
>> + .unlocked_ioctl = video_ioctl2,
>> + .mmap = v4l2_m2m_fop_mmap,
>> +};
>> +
>> +static struct video_device ipu_scale_videodev = {
>> + .name = MEM2MEM_NAME,
>> + .fops = &ipu_scale_fops,
>> + .ioctl_ops = &ipu_scale_ioctl_ops,
>> + .minor = -1,
>> + .release = video_device_release_empty,
>> + .vfl_dir = VFL_DIR_M2M,
>> +};
>> +
>> +static struct v4l2_m2m_ops m2m_ops = {
>> + .device_run = device_run,
>> + .job_ready = job_ready,
>> + .job_abort = job_abort,
>> +};
>> +
>> +static u64 vout_dmamask = ~(u32)0;
>> +
>> +static int ipu_scale_probe(struct platform_device *pdev)
>> +{
>> + struct ipu_scale_dev *ipu_scaler;
>> + struct video_device *vfd;
>> + struct ipu_soc *ipu = dev_get_drvdata(pdev->dev.parent);
>> + int ret;
>> +
>> + pdev->dev.dma_mask = &vout_dmamask;
>> + pdev->dev.coherent_dma_mask = 0xffffffff;
>> +
>> + ipu_scaler = devm_kzalloc(&pdev->dev, sizeof(*ipu_scaler), GFP_KERNEL);
>> + if (!ipu_scaler)
>> + return -ENOMEM;
>> +
>> + ipu_scaler->ipu = ipu;
>> + ipu_scaler->dev = &pdev->dev;
>> +
>> + spin_lock_init(&ipu_scaler->irqlock);
>> + mutex_init(&ipu_scaler->dev_mutex);
>> +
>> + ret = v4l2_device_register(&pdev->dev, &ipu_scaler->v4l2_dev);
>> + if (ret)
>> + return ret;
>> +
>> + atomic_set(&ipu_scaler->num_inst, 0);
>> +
>> + ipu_scaler->vfd = ipu_scale_videodev;
>> + vfd = &ipu_scaler->vfd;
>> + vfd->lock = &ipu_scaler->dev_mutex;
>> + vfd->v4l2_dev = &ipu_scaler->v4l2_dev;
>> +
>> + ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
>> + if (ret) {
>> + dev_err(ipu_scaler->dev, "Failed to register video device\n");
>> + goto unreg_dev;
>> + }
>
> This should be at the end, just before the 'return 0'. As soon as this device
> is created can it be used, so everything should be initialized and configured
> before you create it.
>
>> +
>> + video_set_drvdata(vfd, ipu_scaler);
>> + snprintf(vfd->name, sizeof(vfd->name), "%s", ipu_scale_videodev.name);
>> + dev_dbg(ipu_scaler->dev, "Device registered as /dev/video%d\n",
>> + vfd->num);
>> +
>> + platform_set_drvdata(pdev, ipu_scaler);
>> +
>> + ipu_scaler->m2m_dev = v4l2_m2m_init(&m2m_ops);
>> + if (IS_ERR(ipu_scaler->m2m_dev)) {
>> + dev_err(ipu_scaler->dev, "Failed to init mem2mem device\n");
>> + ret = PTR_ERR(ipu_scaler->m2m_dev);
>> + goto err_m2m;
>> + }
>> +
>> + return 0;
>> +
>> + v4l2_m2m_release(ipu_scaler->m2m_dev);
>> +err_m2m:
>> + video_unregister_device(&ipu_scaler->vfd);
>> +unreg_dev:
>> + v4l2_device_unregister(&ipu_scaler->v4l2_dev);
>> +
>> + return ret;
>> +}
>> +
>> +static int ipu_scale_remove(struct platform_device *pdev)
>> +{
>> + struct ipu_scale_dev *ipu_scaler =
>> + (struct ipu_scale_dev *)platform_get_drvdata(pdev);
>> +
>> + v4l2_m2m_release(ipu_scaler->m2m_dev);
>> + video_unregister_device(&ipu_scaler->vfd);
>> + v4l2_device_unregister(&ipu_scaler->v4l2_dev);
>> + kfree(ipu_scaler);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct platform_device_id ipu_scale_id[] = {
>> + { "imx-ipuv3-scaler" },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(platform, ipu_scale_id);
>> +
>> +static struct platform_driver ipu_scale_pdrv = {
>> + .probe = ipu_scale_probe,
>> + .remove = ipu_scale_remove,
>> + .driver = {
>> + .name = "imx-ipuv3-scaler",
>> + },
>> +};
>> +
>> +static void __exit ipu_scale_exit(void)
>> +{
>> + platform_driver_unregister(&ipu_scale_pdrv);
>> +}
>> +
>> +static int __init ipu_scale_init(void)
>> +{
>> + return platform_driver_register(&ipu_scale_pdrv);
>> +}
>> +
>> +module_init(ipu_scale_init);
>> +module_exit(ipu_scale_exit);
>> +
>> +MODULE_DESCRIPTION("i.MX IPUv3 mem2mem scaler/CSC");
>> +MODULE_AUTHOR("Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/media/platform/imx/imx-ipu.c b/drivers/media/platform/imx/imx-ipu.c
>> index c1b8637..402cc8b 100644
>> --- a/drivers/media/platform/imx/imx-ipu.c
>> +++ b/drivers/media/platform/imx/imx-ipu.c
>> @@ -102,7 +102,7 @@ struct ipu_fmt *ipu_find_fmt_rgb(unsigned int pixelformat)
>> }
>> EXPORT_SYMBOL_GPL(ipu_find_fmt_rgb);
>>
>> -static struct ipu_fmt *ipu_find_fmt(unsigned long pixelformat)
>> +struct ipu_fmt *ipu_find_fmt(unsigned long pixelformat)
>> {
>> struct ipu_fmt *fmt;
>>
>> diff --git a/drivers/media/platform/imx/imx-ipu.h b/drivers/media/platform/imx/imx-ipu.h
>> index 51c0982..288ee79 100644
>> --- a/drivers/media/platform/imx/imx-ipu.h
>> +++ b/drivers/media/platform/imx/imx-ipu.h
>> @@ -16,6 +16,7 @@ int ipu_enum_fmt_yuv(struct file *file, void *fh,
>> struct v4l2_fmtdesc *f);
>> struct ipu_fmt *ipu_find_fmt_rgb(unsigned int pixelformat);
>> struct ipu_fmt *ipu_find_fmt_yuv(unsigned int pixelformat);
>> +struct ipu_fmt *ipu_find_fmt(unsigned long pixelformat);
>> int ipu_try_fmt(struct file *file, void *fh,
>> struct v4l2_format *f);
>> int ipu_try_fmt_rgb(struct file *file, void *fh,
>>
>
> Regards,
>
> Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
What is the status of this driver ?
I can test it here, Philipp, are you planning to take Hans remarks
into account in one of your trees ?
Thanks,
JM
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
