My review also imply Kamil's original patch.
Jeongtae Park wrote:
> Multi Format Codec v5.1 is a module available on S5PC110 and S5PC210
> Samsung SoCs. Hardware is capable of handling a range of video codecs
> and this driver provides V4L2 interface for video decoding & encoding.
>
> Reviewed-by: Peter Oh <jaeryul.oh@xxxxxxxxxxx>
> Signed-off-by: Jeongtae Park <jtp.park@xxxxxxxxxxx>
> ---
<snip>
> +
> +/* Display status */
> +#define S5P_FIMV_DEC_STATUS_DECODING_ONLY 0
> +#define S5P_FIMV_DEC_STATUS_DECODING_DISPLAY 1
> +#define S5P_FIMV_DEC_STATUS_DISPLAY_ONLY 2
> +#define S5P_FIMV_DEC_STATUS_DECODING_EMPTY 3
> +#define S5P_FIMV_DEC_STATUS_DECODING_STATUS_MASK 7
> +#define S5P_FIMV_DEC_STATUS_PROGRESSIVE (0<<3)
> +#define S5P_FIMV_DEC_STATUS_INTERLACE (1<<3)
> +#define S5P_FIMV_DEC_STATUS_INTERLACE_MASK (1<<3)
> +#define S5P_FIMV_DEC_STATUS_CRC_NUMBER_TWO (0<<4)
> +#define S5P_FIMV_DEC_STATUS_CRC_NUMBER_FOUR (1<<4)
> +#define S5P_FIMV_DEC_STATUS_CRC_NUMBER_MASK (1<<4)
> +#define S5P_FIMV_DEC_STATUS_CRC_GENERATED (1<<5)
> +#define S5P_FIMV_DEC_STATUS_CRC_NOT_GENERATED (0<<5)
> +#define S5P_FIMV_DEC_STATUS_CRC_MASK (1<<5)
Use like (0 << 3), (1 << 3) ...
<snip>
> +/* Enumerate format */
> +static int vidioc_enum_fmt(struct v4l2_fmtdesc *f, bool mplane, bool out,
> + enum s5p_mfc_node_type
> node)
> +{
> + struct s5p_mfc_fmt *fmt;
> + int i, j = 0;
> +
> + if (node == MFCNODE_INVALID)
> + return 0;
> +
> + for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> + if (mplane && formats[i].num_planes == 1)
> + continue;
> + else if (!mplane && formats[i].num_planes > 1)
> + continue;
> + if (node == MFCNODE_DECODER) {
> + if (out && formats[i].type != MFC_FMT_DEC)
> + continue;
> + else if (!out && formats[i].type != MFC_FMT_RAW)
> + continue;
> + } else if (node == MFCNODE_ENCODER) {
> + if (out && formats[i].type != MFC_FMT_RAW)
> + continue;
> + else if (!out && formats[i].type != MFC_FMT_ENC)
> + continue;
> + }
> +
> + if (j == f->index) {
> + fmt = &formats[i];
> + strlcpy(f->description, fmt->name,
> + sizeof(f->description));
> + f->pixelformat = fmt->fourcc;
> +
> + return 0;
> + }
> +
> + ++j;
> + }
> +
> + return -EINVAL;
> +}
> +
At a glance, no needed to use j variable.
if (i == f->index) instead of if (j == f->index)
<snip>
> +/* Get format */
> +static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format
*f)
> +{
> + struct s5p_mfc_ctx *ctx = priv;
> +
> + mfc_debug_enter();
> + mfc_debug("f->type = %d ctx->state = %d\n", f->type, ctx->state);
> + if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
> + ctx->state == MFCINST_GOT_INST) {
> + /* If the MFC is parsing the header,
> + * so wait until it is finished */
> + s5p_mfc_clean_ctx_int_flags(ctx);
> + s5p_mfc_wait_for_done_ctx(ctx,
> S5P_FIMV_R2H_CMD_SEQ_DONE_RET,
> + 1);
> + }
> + if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
> + ctx->state >= MFCINST_HEAD_PARSED &&
> + ctx->state <= MFCINST_ABORT) {
> + /* This is run on CAPTURE (deocde output) */
> + /* Width and height are set to the dimensions
> + of the movie, the buffer is bigger and
> + further processing stages should crop to this
> + rectangle. */
> + f->fmt.pix_mp.width = ctx->buf_width;
> + f->fmt.pix_mp.height = ctx->buf_height;
> + f->fmt.pix_mp.field = V4L2_FIELD_NONE;
> + f->fmt.pix_mp.num_planes = 2;
> + /* Set pixelformat to the format in which MFC
> + outputs the decoded frame */
> + f->fmt.pix_mp.pixelformat = V4L2_PIX_FMT_NV12MT;
> + f->fmt.pix_mp.plane_fmt[0].bytesperline = ctx->buf_width;
> + f->fmt.pix_mp.plane_fmt[0].sizeimage = ctx->luma_size;
> + f->fmt.pix_mp.plane_fmt[1].bytesperline = ctx->buf_width;
> + f->fmt.pix_mp.plane_fmt[1].sizeimage = ctx->chroma_size;
> + } else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> + /* This is run on OUTPUT
> + The buffer contains compressed image
> + so width and height have no meaning */
> + f->fmt.pix_mp.width = 1;
> + f->fmt.pix_mp.height = 1;
> + f->fmt.pix_mp.field = V4L2_FIELD_NONE;
> + f->fmt.pix_mp.plane_fmt[0].bytesperline = ctx-
> >dec_src_buf_size;
> + f->fmt.pix_mp.plane_fmt[0].sizeimage =
ctx->dec_src_buf_size;
> + f->fmt.pix_mp.pixelformat = ctx->src_fmt->fourcc;
> + f->fmt.pix_mp.num_planes = ctx->src_fmt->num_planes;
> + } else {
> + mfc_err("Format could not be read\n");
> + mfc_debug("%s-- with error\n", __func__);
> + return -EINVAL;
> + }
> + mfc_debug_leave();
> + return 0;
> +}
> +
How about using size 0 instead of 1 for no meaning value?
In my opinion f->fmt.pix_mp.plane_fmt[1].bytesperline should be
(ctx->buf_width >> 1), isn't it ?
<snip>
> +static int vidioc_try_fmt_enc(struct file *file, void *priv, struct
v4l2_format *f)
> +{
> + struct s5p_mfc_fmt *fmt;
> +
> + if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> + fmt = find_format(f, MFC_FMT_ENC);
> + if (!fmt) {
> + mfc_err("failed to try output format\n");
> + return -EINVAL;
> + }
> +
> + if (f->fmt.pix_mp.plane_fmt[0].sizeimage == 0) {
> + mfc_err("must be set encoding output size\n");
> + return -EINVAL;
> + }
> +
> + f->fmt.pix_mp.plane_fmt[0].bytesperline =
> + f->fmt.pix_mp.plane_fmt[0].sizeimage;
> + } else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> + fmt = find_format(f, MFC_FMT_RAW);
> + if (!fmt) {
> + mfc_err("failed to try output format\n");
> + return -EINVAL;
> + }
> +
> + if (fmt->num_planes != f->fmt.pix_mp.num_planes) {
> + mfc_err("failed to try output format\n");
> + return -EINVAL;
> + }
> + } else {
> + mfc_err("invalid buf type\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
How about check the valid range ?
This driver already has pix_limit variable in variant structure.
<snip>
> +static int vidioc_s_fmt_enc(struct file *file, void *priv, struct
v4l2_format *f)
> +{
> + struct s5p_mfc_ctx *ctx = priv;
> + unsigned long flags;
> + int ret = 0;
> + struct s5p_mfc_fmt *fmt;
> +
> + mfc_debug_enter();
> +
> + ret = vidioc_try_fmt_enc(file, priv, f);
> + if (ret)
> + return ret;
> +
> + if (ctx->vq_src.streaming || ctx->vq_dst.streaming) {
> + v4l2_err(&dev->v4l2_dev, "%s queue busy\n", __func__);
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> + fmt = find_format(f, MFC_FMT_ENC);
> + if (!fmt) {
> + mfc_err("failed to set capture format\n");
> + return -EINVAL;
> + }
> + ctx->state = MFCINST_INIT;
> +
> + ctx->dst_fmt = fmt;
> + ctx->codec_mode = ctx->dst_fmt->codec_mode;
> + mfc_debug("codec number: %d\n", ctx->dst_fmt->codec_mode);
> +
> + ctx->enc_dst_buf_size =
f->fmt.pix_mp.plane_fmt[0].sizeimage;
> + f->fmt.pix_mp.plane_fmt[0].bytesperline = 0;
> +
> + ctx->dst_bufs_cnt = 0;
> + ctx->capture_state = QUEUE_FREE;
> +
> + s5p_mfc_alloc_instance_buffer(ctx);
> +
> + spin_lock_irqsave(&dev->condlock, flags);
> + set_bit(ctx->num, &dev->ctx_work_bits);
> + spin_unlock_irqrestore(&dev->condlock, flags);
> +
> + s5p_mfc_clean_ctx_int_flags(ctx);
> + s5p_mfc_try_run();
> + if (s5p_mfc_wait_for_done_ctx(ctx, \
> + S5P_FIMV_R2H_CMD_OPEN_INSTANCE_RET,
> 1)) {
> + /* Error or timeout */
> + mfc_err("Error getting instance from hardware.\n");
> + s5p_mfc_release_instance_buffer(ctx);
> + ret = -EIO;
> + goto out;
> + }
> + mfc_debug("Got instance number: %d\n", ctx->inst_no);
> + } else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> + fmt = find_format(f, MFC_FMT_RAW);
> + if (!fmt) {
> + mfc_err("failed to set output format\n");
> + return -EINVAL;
> + }
> +
> + if (fmt->num_planes != f->fmt.pix_mp.num_planes) {
> + mfc_err("failed to set output format\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ctx->src_fmt = fmt;
> + ctx->img_width = f->fmt.pix_mp.width;
> + ctx->img_height = f->fmt.pix_mp.height;
> +
> + mfc_debug("codec number: %d\n", ctx->src_fmt->codec_mode);
> + mfc_debug("fmt - w: %d, h: %d, ctx - w: %d, h: %d\n",
> + f->fmt.pix_mp.width, f->fmt.pix_mp.height,
> + ctx->img_width, ctx->img_height);
> +
> + ctx->buf_width = f->fmt.pix_mp.plane_fmt[0].bytesperline;
> + ctx->luma_size = f->fmt.pix_mp.plane_fmt[0].sizeimage;
> + ctx->buf_width = f->fmt.pix_mp.plane_fmt[1].bytesperline;
> + ctx->chroma_size = f->fmt.pix_mp.plane_fmt[1].sizeimage;
> +
> + ctx->src_bufs_cnt = 0;
> + ctx->output_state = QUEUE_FREE;
> + } else {
> + mfc_err("invalid buf type\n");
> + return -EINVAL;
> + }
> +out:
> + mfc_debug_leave();
> + return ret;
> +}
> +
ctx->buf_width is overwritten by " f->fmt.pix_mp.plane_fmt[1].bytesperline".
<snip>
> +static int vidioc_reqbufs_enc(struct file *file, void *priv,
> + struct v4l2_requestbuffers
*reqbufs)
> +{
> + struct s5p_mfc_ctx *ctx = priv;
> + int ret = 0;
> +
> + mfc_debug_enter();
> +
> + mfc_debug("type: %d\n", reqbufs->memory);
> +
> + /* if memory is not mmp or userptr return error */
> + if ((reqbufs->memory != V4L2_MEMORY_MMAP) &&
> + (reqbufs->memory != V4L2_MEMORY_USERPTR))
> + return -EINVAL;
> +
> + if (reqbufs->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> + if (ctx->capture_state != QUEUE_FREE) {
> + mfc_err("invalid capture state: %d\n", ctx-
> >capture_state);
> + return -EINVAL;
> + }
> +
> + ret = vb2_reqbufs(&ctx->vq_dst, reqbufs);
> + if (ret != 0) {
> + mfc_err("error in vb2_reqbufs() for E(D)\n");
> + return ret;
> + }
> + ctx->capture_state = QUEUE_BUFS_REQUESTED;
> +
> + ret = s5p_mfc_alloc_codec_buffers(ctx);
> + if (ret) {
> + mfc_err("Failed to allocate encoding buffers.\n");
> + reqbufs->count = 0;
> + ret = vb2_reqbufs(&ctx->vq_dst, reqbufs);
> + return -ENOMEM;
> + }
> + } else if (reqbufs->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> {
> + if (ctx->output_state != QUEUE_FREE) {
> + mfc_err("invalid output state: %d\n",
ctx->output_state);
> + return -EINVAL;
> + }
> +
> + ret = vb2_reqbufs(&ctx->vq_src, reqbufs);
> + if (ret != 0) {
> + mfc_err("error in vb2_reqbufs() for E(S)\n");
> + return ret;
> + }
> + ctx->output_state = QUEUE_BUFS_REQUESTED;
> + } else {
> + mfc_err("invalid buf type\n");
> + return -EINVAL;
> + }
> +
> + mfc_debug("--\n");
> +
> + return ret;
> +}
> +
if (ctx->capture_state != QUEUE_FREE), conditional statement looks strange.
If REQBUFS(0) directly follows REQBUFS(n), state is QUEUE_BUFS_REQUESTED.
So in that case REQBUFS(0) will be failed.
<snip>
> +static int set_ctrl_val_enc(struct s5p_mfc_ctx *ctx, struct v4l2_control
*ctrl)
> +{
> + struct s5p_mfc_enc_params *p = &ctx->enc_params;
> + int ret = 0;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_CODEC_MFC51_ENC_GOP_SIZE:
> + p->gop_size = ctrl->value;
> + break;
> + case V4L2_CID_CODEC_MFC51_ENC_MULTI_SLICE_MODE:
> + p->slice_mode = ctrl->value;
> + break;
> + case V4L2_CID_CODEC_MFC51_ENC_MULTI_SLICE_MB:
> + p->slice_mb = ctrl->value;
> + break;
> + case V4L2_CID_CODEC_MFC51_ENC_MULTI_SLICE_BIT:
> + p->slice_bit = ctrl->value;
> + break;
> + case V4L2_CID_CODEC_MFC51_ENC_INTRA_REFRESH_MB:
> + p->intra_refresh_mb = ctrl->value;
> + break;
> + case V4L2_CID_CODEC_MFC51_ENC_PAD_CTRL_ENABLE:
> + p->pad = ctrl->value;
> + break;
<snip>
> + case V4L2_CID_CODEC_MFC51_ENC_H264_I_PERIOD:
> + p->codec.h264.open_gop_size = ctrl->value;
> + break;
> + default:
> + v4l2_err(&dev->v4l2_dev, "Invalid control\n");
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
> +
How about use MACRO like cmd_input_size in
./drivers/media/video/v4l2-ioctl.c.
<snip>
> +static int vidioc_g_ext_ctrls(struct file *file, void *priv,
> + struct v4l2_ext_controls *f)
> +{
> + struct s5p_mfc_ctx *ctx = priv;
> + struct v4l2_ext_control *ext_ctrl;
> + struct v4l2_control ctrl;
> + int i;
> + int ret = 0;
> +
> + if (s5p_mfc_get_node_type(file) != MFCNODE_ENCODER)
> + return -EINVAL;
> +
> + if (f->ctrl_class != V4L2_CTRL_CLASS_CODEC)
> + return -EINVAL;
> +
> + for (i = 0; i < f->count; i++) {
> + ext_ctrl = (f->controls + i);
> +
> + ctrl.id = ext_ctrl->id;
> +
> + ret = get_ctrl_val_enc(ctx, &ctrl);
> + if (ret == 0) {
> + ext_ctrl->value = ctrl.value;
> + } else {
> + f->error_idx = i;
> + break;
> + }
> +
> + mfc_debug("[%d] id: 0x%08x, value: %d", i, ext_ctrl->id,
> + ext_ctrl->value);
> + }
> +
> + return ret;
> +}
> +
How about use vidioc_g_ext_ctrls_enc instead of vidioc_g_ext_ctrls ?
Because the function is only for encoder.
vidioc_s_ext_ctrls also.
<snip>
> +/* Get cropping information */
> +static int vidioc_g_crop(struct file *file, void *priv,
> + struct v4l2_crop *cr)
> +{
> + struct s5p_mfc_ctx *ctx = priv;
> + u32 left, right, top, bottom;
> +
> + mfc_debug_enter();
> + if (ctx->state != MFCINST_HEAD_PARSED &&
> + ctx->state != MFCINST_RUNNING && ctx->state != MFCINST_FINISHING
> + && ctx->state != MFCINST_FINISHED)
> {
> + mfc_debug("%s-- with error\n", __func__);
> + return -EINVAL;
> + }
Keep indent "}"
<snip>
> +static int s5p_mfc_enc_queue_setup(struct vb2_queue *vq,
> + unsigned int *buf_count, unsigned int *plane_count,
> + unsigned long psize[], void *allocators[])
> +{
> + struct s5p_mfc_ctx *ctx = vq->drv_priv;
> + int i;
> +
> + mfc_debug_enter();
> +
> + if (ctx->state != MFCINST_GOT_INST) {
> + mfc_err("inavlid state: %d\n", ctx->state);
> + return -EINVAL;
> + }
> +
> + if (vq->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> + if (ctx->dst_fmt)
> + *plane_count = ctx->dst_fmt->num_planes;
> + else
> + *plane_count = MFC_ENC_CAP_PLANE_COUNT;
> +
> + if (*buf_count < 1)
> + *buf_count = 1;
> + if (*buf_count > MFC_MAX_BUFFERS)
> + *buf_count = MFC_MAX_BUFFERS;
> +
> + psize[0] = ctx->enc_dst_buf_size;
> + allocators[0] = ctx->dev-
> >alloc_ctx[MFC_CMA_BANK1_ALLOC_CTX];
> + } else if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> + if (ctx->src_fmt)
> + *plane_count = ctx->src_fmt->num_planes;
> + else
> + *plane_count = MFC_ENC_OUT_PLANE_COUNT;
> +
> + if (*buf_count < 1)
> + *buf_count = 1;
> + if (*buf_count > MFC_MAX_BUFFERS)
> + *buf_count = MFC_MAX_BUFFERS;
> +
> + psize[0] = ctx->luma_size;
> + psize[1] = ctx->chroma_size;
> + allocators[0] = ctx->dev-
> >alloc_ctx[MFC_CMA_BANK2_ALLOC_CTX];
> + allocators[1] = ctx->dev-
> >alloc_ctx[MFC_CMA_BANK2_ALLOC_CTX];
> +
> + } else {
> + mfc_err("inavlid queue type: %d\n", vq->type);
> + return -EINVAL;
> + }
> +
> + mfc_debug("buf_count: %d, plane_count: %d\n", *buf_count,
> *plane_count);
> + for (i = 0; i < *plane_count; i++)
> + mfc_debug("plane[%d] size=%lu\n", i, psize[i]);
> +
> + mfc_debug_leave();
> +
> + return 0;
> +}
> +
Below two lines are dead line.
else
*plane_count = MFC_ENC_OUT_PLANE_COUNT;
Because ctx->src_fmt is set in s5p_mfc_open.
<snip>
> +/* Let the streaming begin. */
> +static int s5p_mfc_start_streaming(struct vb2_queue *q)
> +{
> + struct s5p_mfc_ctx *ctx = q->drv_priv;
> +
> + unsigned long flags;
> + /* If context is ready then schedule it to run */
> + if (s5p_mfc_ctx_ready(ctx)) {
> + spin_lock_irqsave(&dev->condlock, flags);
> + set_bit(ctx->num, &dev->ctx_work_bits);
> + spin_unlock_irqrestore(&dev->condlock, flags);
> + }
> +
> + s5p_mfc_try_run();
> + return 0;
> +}
> +
In my opinion s5p_mfc_start_streaming is useless.
Because in the following sequence s5p_mfc_try_run will be called in
s5p_mfc_enc_buf_queue without s5p_mfc_start_streaming.
VIDIOC_STREAMON -> vidioc_streamon -> vb2_streamon -> __enqueue_in_driver ->
buf_queue -> s5p_mfc_enc_buf_queue
<snip>
> +/* Open an MFC node */
> +static int s5p_mfc_open(struct file *file)
> +{
> + struct s5p_mfc_ctx *ctx = NULL;
> + struct vb2_queue *q;
> + unsigned long flags;
> + int ret = 0;
> +
> + mfc_debug_enter();
> + dev->num_inst++; /* It is guarded by mfc_mutex in vfd */
> + /* Allocate memory for context */
> + ctx = kzalloc(sizeof *ctx, GFP_KERNEL);
> + if (!ctx) {
> + mfc_err("Not enough memory.\n");
> + ret = -ENOMEM;
> + goto out_open;
> + }
> + file->private_data = ctx;
> + ctx->dev = dev;
> + INIT_LIST_HEAD(&ctx->src_queue);
> + INIT_LIST_HEAD(&ctx->dst_queue);
> + ctx->src_queue_cnt = 0;
> + ctx->dst_queue_cnt = 0;
> + /* Get context number */
> + ctx->num = 0;
> + while (dev->ctx[ctx->num]) {
> + ctx->num++;
> + if (ctx->num >= MFC_NUM_CONTEXTS) {
> + mfc_err("Too many open contexts.\n");
> + ret = -EAGAIN;
> + goto out_open;
> + }
> + }
> + /* Mark context as idle */
> + spin_lock_irqsave(&dev->condlock, flags);
> + clear_bit(ctx->num, &dev->ctx_work_bits);
> + spin_unlock_irqrestore(&dev->condlock, flags);
> + dev->ctx[ctx->num] = ctx;
> + if (s5p_mfc_get_node_type(file) == MFCNODE_DECODER) {
> + ctx->type = MFCINST_DECODER;
> + ctx->c_ops = &decoder_codec_ops;
> + /* Default format */
> + ctx->src_fmt = &formats[DEC_DEF_SRC_FMT];
> + ctx->dst_fmt = &formats[DEC_DEF_DST_FMT];
> + } else if (s5p_mfc_get_node_type(file) == MFCNODE_ENCODER) {
> + ctx->type = MFCINST_ENCODER;
> + ctx->c_ops = &encoder_codec_ops;
> + /* Default format */
> + ctx->src_fmt = &formats[ENC_DEF_SRC_FMT];
> + ctx->dst_fmt = &formats[ENC_DEF_DST_FMT];
> + } else {
> + ret = -ENOENT;
> + goto out_open;
> + }
> + ctx->inst_no = -1;
> + /* Load firmware if this is the first instance */
> + if (dev->num_inst == 1) {
> + dev->watchdog_timer.expires = jiffies +
> +
> msecs_to_jiffies(MFC_WATCHDOG_INTERVAL);
> + add_timer(&dev->watchdog_timer);
> +
> + /* Load the FW */
> + ret = s5p_mfc_alloc_firmware(dev);
> + if (ret != 0)
> + goto out_open_2a;
> + ret = s5p_mfc_load_firmware(dev);
> + if (ret != 0)
> + goto out_open_2;
> + mfc_debug("Enabling clocks.\n");
> + clk_enable(dev->clock1);
> + clk_enable(dev->clock2);
> + /* Init the FW */
> + ret = s5p_mfc_init_hw(dev);
> + if (ret != 0)
> + goto out_open_3;
> + }
> +
> + /* Init videobuf2 queue for CAPTURE */
> + q = &ctx->vq_dst;
> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> + q->drv_priv = ctx;
> + if (s5p_mfc_get_node_type(file) == MFCNODE_DECODER) {
> + q->io_modes = VB2_MMAP;
> + q->ops = &s5p_mfc_qops;
> + } else {
> + q->io_modes = VB2_MMAP | VB2_USERPTR;
> + q->ops = &s5p_mfc_enc_qops;
> + }
> + q->mem_ops = &vb2_cma_memops;
> + ret = vb2_queue_init(q);
> + if (ret) {
> + mfc_err("Failed to initialize videobuf2 queue(capture)\n");
> + goto out_open_3;
> + }
> +
> + /* Init videobuf2 queue for OUTPUT */
> + q = &ctx->vq_src;
> + q->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> + q->io_modes = VB2_MMAP;
> + q->drv_priv = ctx;
> + if (s5p_mfc_get_node_type(file) == MFCNODE_DECODER) {
> + q->io_modes = VB2_MMAP;
> + q->ops = &s5p_mfc_qops;
> + } else {
> + q->io_modes = VB2_MMAP | VB2_USERPTR;
> + q->ops = &s5p_mfc_enc_qops;
> + }
> + q->mem_ops = &vb2_cma_memops;
> + ret = vb2_queue_init(q);
> + if (ret) {
> + mfc_err("Failed to initialize videobuf2 queue(output)\n");
> + goto out_open_3;
> + }
> + init_waitqueue_head(&ctx->queue);
> + mfc_debug("%s-- (via irq_cleanup_hw)\n", __func__);
> + return ret;
> + /* Deinit when failure occured */
> +out_open_3:
> + if (dev->num_inst == 1) {
> + clk_disable(dev->clock1);
> + clk_disable(dev->clock2);
> + s5p_mfc_release_firmware(dev);
> + }
> +out_open_2:
> + s5p_mfc_release_firmware(dev);
> +out_open_2a:
> + dev->ctx[ctx->num] = 0;
> + kfree(ctx);
> + del_timer_sync(&dev->watchdog_timer);
> +out_open:
> + dev->num_inst--;
> + mfc_debug_leave();
> + return ret;
> +}
You had better use atomic operation for increment and decrement instead
dev->num_inst++.
In my opinion -EBUSY is good for return value instead of -EAGAIN if ctx->num
is exceed MFC_NUM_CONTEXTS.
In my opinion, " /* Load firmware if this is the first instance */" is not
suitable comment.
Because clk_enable is included in the conditional statements.
How about exchange init videobuf2 sequence OUTPUT, CAPTURE ?
Because OUTPUT is the source of device and CAPTURE is the destination of
device.
<snip>
> +/* MFC probe function */
> +static int s5p_mfc_probe(struct platform_device *pdev)
> +{
> + struct video_device *vfd;
> + struct resource *res;
> + int ret = -ENOENT;
> + size_t size;
> +
> + pr_debug("%s++\n", __func__);
> + dev = kzalloc(sizeof *dev, GFP_KERNEL);
> + if (!dev) {
> + dev_err(&pdev->dev, "Not enough memory for MFC device.\n");
> + return -ENOMEM;
> + }
> +
> + spin_lock_init(&dev->irqlock);
> + spin_lock_init(&dev->condlock);
> + dev_dbg(&pdev->dev, "Initialised spin lock\n");
> + dev->plat_dev = pdev;
> + if (!dev->plat_dev) {
> + dev_err(&pdev->dev, "No platform data specified\n");
> + ret = -ENODEV;
> + goto free_dev;
> + }
> + dev_dbg(&pdev->dev, "Getting clocks\n");
> + dev->clock1 = clk_get(&pdev->dev, "sclk_mfc");
> + dev->clock2 = clk_get(&pdev->dev, "mfc");
> + if (IS_ERR(dev->clock1) || IS_ERR(dev->clock2)) {
> + dev_err(&pdev->dev, "failed to get mfc clock source\n");
> + goto free_clk;
> + }
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL) {
> + dev_err(&pdev->dev, "failed to get memory region
resource.\n");
> + ret = -ENOENT;
> + goto probe_out1;
> + }
> + size = (res->end - res->start) + 1;
> + dev->mfc_mem = request_mem_region(res->start, size, pdev->name);
> + if (dev->mfc_mem == NULL) {
> + dev_err(&pdev->dev, "failed to get memory region.\n");
> + ret = -ENOENT;
> + goto probe_out2;
> + }
> + dev->base_virt_addr = ioremap(dev->mfc_mem->start,
> + dev->mfc_mem->end - dev->mfc_mem->start + 1);
> + if (dev->base_virt_addr == NULL) {
> + dev_err(&pdev->dev, "failed to ioremap address region.\n");
> + ret = -ENOENT;
> + goto probe_out3;
> + }
> + dev->regs_base = dev->base_virt_addr;
> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (res == NULL) {
> + dev_err(&pdev->dev, "failed to get irq resource.\n");
> + ret = -ENOENT;
> + goto probe_out4;
> + }
> + dev->irq = res->start;
> + ret = request_irq(dev->irq, s5p_mfc_irq, IRQF_DISABLED, pdev->name,
> +
> dev);
> + if (ret != 0) {
> + dev_err(&pdev->dev, "Failed to install irq (%d)\n", ret);
> + goto probe_out5;
> + }
> + dev->mfc_mutex = kmalloc(sizeof(struct mutex), GFP_KERNEL);
> + if (dev->mfc_mutex == NULL) {
> + dev_err(&pdev->dev, "Memory allocation failed\n");
> + ret = -ENOMEM;
> + goto probe_out6;
> + }
> + mutex_init(dev->mfc_mutex);
> + ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
> + if (ret)
> + goto probe_out7;
> + init_waitqueue_head(&dev->queue);
> +
> + /* decoder */
> + vfd = video_device_alloc();
> + if (!vfd) {
> + v4l2_err(&dev->v4l2_dev, "Failed to allocate video
device\n");
> + ret = -ENOMEM;
> + goto unreg_dev;
> + }
> + *vfd = s5p_mfc_videodev;
> + vfd->lock = dev->mfc_mutex;
> + vfd->v4l2_dev = &dev->v4l2_dev;
> + snprintf(vfd->name, sizeof(vfd->name), "%s", s5p_mfc_videodev.name);
> +
> + ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
> + if (ret) {
> + v4l2_err(&dev->v4l2_dev, "Failed to register video
device\n");
> + video_device_release(vfd);
> + goto rel_vdev_dec;
> + }
> + v4l2_info(&dev->v4l2_dev,
> + "MFC decoder device registered as /dev/video%d\n",
> + vfd->num);
> +
> + dev->vfd_dec = vfd;
> +
> + /* encoder */
> + vfd = video_device_alloc();
> + if (!vfd) {
> + v4l2_err(&dev->v4l2_dev,
> + "Failed to allocate video device\n");
> + ret = -ENOMEM;
> + goto unreg_vdev_dec;
> + }
> + *vfd = s5p_mfc_enc_videodev;
> + vfd->lock = dev->mfc_mutex;
> + vfd->v4l2_dev = &dev->v4l2_dev;
> + snprintf(vfd->name, sizeof(vfd->name), "%s",
> s5p_mfc_enc_videodev.name);
> +
> + ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
> + if (ret) {
> + v4l2_err(&dev->v4l2_dev, "Failed to register video
device\n");
> + video_device_release(vfd);
> + goto rel_vdev_enc;
> + }
> + v4l2_info(&dev->v4l2_dev,
> + "MFC encoder device registered as /dev/video%d\n",
> + vfd->num);
> +
> + dev->vfd_enc = vfd;
> +
> + video_set_drvdata(vfd, dev);
> +
> + platform_set_drvdata(pdev, dev);
> + dev->hw_lock = 0;
> + dev->watchdog_workqueue = create_singlethread_workqueue("s5p-mfc");
> + INIT_WORK(&dev->watchdog_work, s5p_mfc_watchdog_worker);
> + atomic_set(&dev->watchdog_cnt, 0);
> + init_timer(&dev->watchdog_timer);
> + dev->watchdog_timer.data = 0;
> + dev->watchdog_timer.function = s5p_mfc_watchdog;
> +
> + dev->alloc_ctx = vb2_cma_init_multi(&pdev->dev,
> MFC_CMA_ALLOC_CTX_NUM,
> + s5p_mem_types,
> s5p_mem_alignments);
> + if (IS_ERR(dev->alloc_ctx)) {
> + mfc_err("Couldn't prepare allocator ctx.\n");
> + ret = PTR_ERR(dev->alloc_ctx);
> + goto alloc_ctx_fail;
> + }
> +
> + pr_debug("%s--\n", __func__);
> + return 0;
> +
> +/* Deinit MFC if probe had failed */
> +alloc_ctx_fail:
> + video_unregister_device(dev->vfd_enc);
> +rel_vdev_enc:
> + video_device_release(dev->vfd_enc);
> +unreg_vdev_dec:
> + video_unregister_device(dev->vfd_dec);
> +rel_vdev_dec:
> + video_device_release(dev->vfd_dec);
> +unreg_dev:
> + v4l2_device_unregister(&dev->v4l2_dev);
> +probe_out7:
> + if (dev->mfc_mutex) {
> + mutex_destroy(dev->mfc_mutex);
> + kfree(dev->mfc_mutex);
> + }
> +probe_out6:
> + free_irq(dev->irq, dev);
> +probe_out5:
> +probe_out4:
> + iounmap(dev->base_virt_addr);
> + dev->base_virt_addr = NULL;
> +probe_out3:
> + release_resource(dev->mfc_mem);
> + kfree(dev->mfc_mem);
> +probe_out2:
> +probe_out1:
> + clk_put(dev->clock1);
> + clk_put(dev->clock2);
> +free_clk:
> +
> +free_dev:
> + kfree(dev);
> + pr_debug("%s-- with error\n", __func__);
> + return ret;
> +}
> +
What's the diff btw dev->regs_base and dev->base_virt_addr ?
In your implementation I cannot find video_get_drvdata.
Is video_set_drvdata needed ?
<snip>
> +#define MFC_NUM_CONTEXTS 4
How about use MFC_NUM_INSTANT instead MFC_NUM_CONTEXTS ?
Because too many terms can make confusion.
<snip>
> +/**
> + * struct s5p_mfc_buf - MFC buffer
> + *
> + */
> +struct s5p_mfc_buf {
> + struct list_head list;
> + struct vb2_buffer *b;
> + union {
> + struct {
> + size_t luma;
> + size_t chroma;
> + } raw;
> + size_t stream;
> + } paddr;
> +};
How about use cookie instead paddr ?
<snip>
> +/**
> + * struct s5p_mfc_ctx - This struct contains the instance context
> + */
> +struct s5p_mfc_ctx {
> + struct s5p_mfc_dev *dev;
> + int num;
> +
> + int int_cond;
> + int int_type;
> + unsigned int int_err;
> + wait_queue_head_t queue;
> +
> + struct s5p_mfc_fmt *src_fmt;
> + struct s5p_mfc_fmt *dst_fmt;
> +
> + struct vb2_queue vq_src;
> + struct vb2_queue vq_dst;
> +
> + struct list_head src_queue;
> + struct list_head dst_queue;
> +
> + unsigned int src_queue_cnt;
> + unsigned int dst_queue_cnt;
> +
> + enum s5p_mfc_inst_type type;
> + enum s5p_mfc_inst_state state;
> + int inst_no;
> +
> + /* Decoder parameters */
> + int img_width;
> + int img_height;
> + int buf_width;
> + int buf_height;
> + int dpb_count;
> + int total_dpb_count;
> +
> + int luma_size;
> + int chroma_size;
> + int mv_size;
> +
> + unsigned long consumed_stream;
> + int slice_interface;
> +
> + /* Buffers */
> + void *port_a_buf;
> + size_t port_a_phys;
> + size_t port_a_size;
> +
> + void *port_b_buf;
> + size_t port_b_phys;
> + size_t port_b_size;
> +
> +
> + enum s5p_mfc_queue_state capture_state;
> + enum s5p_mfc_queue_state output_state;
> +
> + struct s5p_mfc_buf src_bufs[MFC_MAX_BUFFERS];
> + int src_bufs_cnt;
> + struct s5p_mfc_buf dst_bufs[MFC_MAX_BUFFERS];
> + int dst_bufs_cnt;
> +
> + unsigned int sequence;
> + unsigned long dec_dst_flag;
> + size_t dec_src_buf_size;
> +
> + /* Control values */
> + int codec_mode;
> + __u32 pix_format;
> + int loop_filter_mpeg4;
> + int display_delay;
> +
> + /* Buffers */
> + void *instance_buf;
> + size_t instance_phys;
> + size_t instance_size;
> +
> + void *desc_buf;
> + size_t desc_phys;
> +
> + void *shared_buf;
> + size_t shared_phys;
> + void *shared_virt;
> +
> + /* Encoder parameters */
> + struct s5p_mfc_enc_params enc_params;
> +
> + size_t enc_dst_buf_size;
> +
> + int frame_count;
> + enum v4l2_codec_mfc5x_enc_frame_type frame_type;
> + enum v4l2_codec_mfc5x_enc_force_frame_type force_frame_type;
> +
> + struct s5p_mfc_codec_ops *c_ops;
> +};
> +
Data structure is too big. How about split it like as below ?
struct s5p_mfc_ctx {
struct s5p_mfc_dec *dec;
struct s5p_mfc_dec *enc;
....
};
Or like this:
struct s5p_mfc_ctx {
union {
struct s5p_mfc_dec *dec;
struct s5p_mfc_dec *enc;
};
....
};
What's the difference btw num and inst_no ?
It looks very similar.
And how about define common data structure for source, destination about
size, format ... like mfc_frame ?
It can make simplify the structure.
<snip>
> +static struct v4l2_queryctrl s5p_mfc_ctrls[] = {
> +/* For decoding */
> + {
> + .id = V4L2_CID_CODEC_DISPLAY_DELAY,
> + .type = V4L2_CTRL_TYPE_INTEGER,
> + .name = "",
> + .minimum = 0,
> + .maximum = 16383,
> + .step = 1,
> + .default_value = 0,
> + },
> + {
> + .id = V4L2_CID_CODEC_LOOP_FILTER_MPEG4_ENABLE,
> + .type = V4L2_CTRL_TYPE_BOOLEAN,
> + .name = "Mpeg4 Loop Filter Enable",
> + .minimum = 0,
> + .maximum = 1,
> + .step = 1,
> + .default_value = 0,
> + },
> + {
> + .id = V4L2_CID_CODEC_SLICE_INTERFACE,
> + .type = V4L2_CTRL_TYPE_BOOLEAN,
> + .name = "Slice Interface Enable",
> + .minimum = 0,
> + .maximum = 1,
> + .step = 1,
> + .default_value = 0,
> + },
> +/* For encoding */
> + {
> + .id = V4L2_CID_CODEC_MFC51_ENC_GOP_SIZE,
> + .type = V4L2_CTRL_TYPE_INTEGER,
> + .name = "The period of intra frame",
> + .minimum = 0,
> + .maximum = (1 << 16) - 1,
> + .step = 1,
> + .default_value = 0,
> + },
You had better use control-framework.
<snip>
> +/* Allocate firmware */
> +int s5p_mfc_alloc_firmware(struct s5p_mfc_dev *dev)
> +{
> + int err;
> + struct cma_info mem_info_f, mem_info_a, mem_info_b;
How about exchange above two lines ?
<snip>
> --
> 1.6.2.5
>
> --
> 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
--
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
