Hi Hans, Thanks for the review. I will answer, because some of your comments apply to my patch. Sorry for replying so late - I've been ill for the past few days. I also have a question about the control framework. > -----Original Message----- > From: Hans Verkuil [mailto:hverkuil@xxxxxxxxx] > Sent: 22 December 2010 14:01 > To: Jeongtae Park > Cc: linux-media@xxxxxxxxxxxxxxx; linux-samsung-soc@xxxxxxxxxxxxxxx; > k.debski@xxxxxxxxxxx; jaeryul.oh@xxxxxxxxxxx; kgene.kim@xxxxxxxxxxx; > ben-linux@xxxxxxxxx; jonghun.han@xxxxxxxxxxx > Subject: Re: [PATCH 7/9] media: MFC: Add MFC v5.1 V4L2 driver > > A quick review: > > On Wednesday, December 22, 2010 12:54:43 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> > > --- > > drivers/media/video/Kconfig | 8 + > > drivers/media/video/Makefile | 1 + > > drivers/media/video/s5p-mfc/Makefile | 3 + > > drivers/media/video/s5p-mfc/regs-mfc5.h | 356 +++ > > drivers/media/video/s5p-mfc/s5p_mfc.c | 3237 > ++++++++++++++++++++++++++ > > Over 3000 lines?! I strongly suggest splitting up this source, it's way > too > big. In my patch it was around 2000 lines. I understand that after adding encoding by Jeongtae Park this became even larger. > > drivers/media/video/s5p-mfc/s5p_mfc_common.h | 333 +++ > > drivers/media/video/s5p-mfc/s5p_mfc_ctrls.h | 622 +++++ > > drivers/media/video/s5p-mfc/s5p_mfc_debug.h | 55 + > > drivers/media/video/s5p-mfc/s5p_mfc_intr.c | 94 + > > drivers/media/video/s5p-mfc/s5p_mfc_intr.h | 26 + > > drivers/media/video/s5p-mfc/s5p_mfc_memory.h | 42 + > > drivers/media/video/s5p-mfc/s5p_mfc_opr.c | 1349 +++++++++++ > > drivers/media/video/s5p-mfc/s5p_mfc_opr.h | 147 ++ > > 13 files changed, 6273 insertions(+), 0 deletions(-) > > create mode 100644 drivers/media/video/s5p-mfc/Makefile > > create mode 100644 drivers/media/video/s5p-mfc/regs-mfc5.h > > create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc.c > > create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_common.h > > create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_ctrls.h > > create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_debug.h > > create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_intr.c > > create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_intr.h > > create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_memory.h > > create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_opr.c > > create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_opr.h > > > > <snip> > > > diff --git a/drivers/media/video/s5p-mfc/s5p_mfc.c > b/drivers/media/video/s5p-mfc/s5p_mfc.c > > new file mode 100644 > > index 0000000..2653864 > > --- /dev/null > > +++ b/drivers/media/video/s5p-mfc/s5p_mfc.c > > @@ -0,0 +1,3237 @@ > > <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; > > Create a temp variable pointing to &f->fmt.pix_mp and use that in this > and > the next function. It makes it easier to read. Good idea, I will do it that way. > > + 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; > > +} > > + <snip> > > + > > +/* Try format */ > > +static int vidioc_try_fmt(struct file *file, void *priv, struct > v4l2_format *f) > > +{ > > + struct s5p_mfc_fmt *fmt; > > + > > + mfc_debug("Type is %d\n", f->type); > > + if (f->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) { > > + mfc_err("Currently only decoding is supported.\n"); > > + return -EINVAL; > > + } > > + if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) { > > Invert this test and return 0, then the rest can be indented one level > to the > left, making it easier to read. Another good suggestion, thanks. > > + fmt = find_format(f, MFC_FMT_DEC); > > + if (!fmt) { > > + mfc_err("Unsupported format.\n"); > > + return -EINVAL; > > + } > > + if (f->fmt.pix_mp.plane_fmt[0].sizeimage == 0) { > > + mfc_err("Application is required to " > > + "specify input buffer size (via sizeimage)\n"); > > + return -EINVAL; > > + } > > + /* As this buffer will contain compressed data, the size is > set > > + * to the maximum size. > > + * Width and height are left intact as they may be relevant > for > > + * DivX 3.11 decoding. */ > > + f->fmt.pix_mp.plane_fmt[0].bytesperline = > > + f->fmt.pix_mp.plane_fmt[0].sizeimage; > > + } > > + return 0; > > +} > > + <snip> > > + > > +/* Set format */ > > +static int vidioc_s_fmt(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(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_OUTPUT_MPLANE) { > > Same trick can be done here and elsewhere as well. The basic idea is to > do the short tests first, then the main work can be done without it > being inside a huge 'if' statement. Again, good suggestion. > > + fmt = find_format(f, MFC_FMT_DEC); > > + if (!fmt || fmt->codec_mode == MFC_FORMATS_NO_CODEC) { > > + mfc_err("Unknown codec.\n"); > > + ret = -EINVAL; > > + goto out; > > + } > > + ctx->src_fmt = fmt; > > + ctx->codec_mode = fmt->codec_mode; > > + mfc_debug("The codec number is: %d\n", ctx->codec_mode); > > + ctx->pix_format = f->fmt.pix_mp.pixelformat; > > + if (f->fmt.pix_mp.pixelformat != V4L2_PIX_FMT_DIVX3) { > > + f->fmt.pix_mp.height = 1; > > + f->fmt.pix_mp.width = 1; > > + } else { > > + ctx->img_height = f->fmt.pix_mp.height; > > + ctx->img_width = f->fmt.pix_mp.width; > > + } > > + mfc_debug("s_fmt w/h: %dx%d, ctx: %dx%d\n", f- > >fmt.pix_mp.width, > > + f->fmt.pix_mp.height, ctx->img_width, ctx- > >img_height); > > + ctx->dec_src_buf_size = f- > >fmt.pix_mp.plane_fmt[0].sizeimage; > > + f->fmt.pix_mp.plane_fmt[0].bytesperline = 0; > > + ctx->state = MFCINST_INIT; > > + ctx->dst_bufs_cnt = 0; > > + ctx->src_bufs_cnt = 0; > > + ctx->capture_state = QUEUE_FREE; > > + ctx->output_state = QUEUE_FREE; > > + s5p_mfc_alloc_instance_buffer(ctx); > > + s5p_mfc_alloc_dec_temp_buffers(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); > > + s5p_mfc_release_dec_desc_buffer(ctx); > > + ret = -EIO; > > + goto out; > > + } > > + mfc_debug("Got instance number: %d\n", ctx->inst_no); > > + } > > + if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { > > + mfc_err("Currently only decoding is supported.\n"); > > + ret = -EINVAL; > > + } > > +out: > > + mfc_debug_leave(); > > + return ret; > > +} > > + <snip> > > +/* Query a ctrl */ > > +static int vidioc_queryctrl(struct file *file, void *priv, > > + struct v4l2_queryctrl *qc) > > Use the new control framework. Much simpler. Particularly if you have > controls > that have dependencies on one another. Here I do have a question. As this driver uses the "context" approach, where each instance of file handle can be setup differently. I have looked at the control framework. In MFC driver the control values are stored in s5p_mfc_ctx structure and are written to hardware only at certain steps - such as decoding initialization. I did not see a mechanism in control framework that would support this approach. The model use of the control framework is setting hardware registers in s_ctrl ops. This would not work with MFC, as it requires the registers to be written on specific moments. Using s_ctrl to change values in s5p_mfc_ctx and writing registers the way it is done now seems to me bad. This would introduce data redundancy - control value would be stored in two places - s5p_mfc_ctx and in the control framework. > > +{ > > + struct v4l2_queryctrl *c; > > + > > + c = get_ctrl(qc->id); > > + if (!c) > > + return -EINVAL; > > + *qc = *c; > > + return 0; > > +} > > <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); > > Huh? Why not embed the mutex in dev? Fixed. > > + 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); > > This should be the very last action. As soon as this device is created > apps > can start to use it, so all other initializations must be done before > you do > this. This code was based on the v4 version of my patch. In the v6 this has been already fixed. > > + 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; > > +} > > + > > +/* Remove the driver */ > > +static int s5p_mfc_remove(struct platform_device *pdev) > > +{ > > + dev_dbg(&pdev->dev, "%s++\n", __func__); > > + v4l2_info(&dev->v4l2_dev, "Removing %s\n", pdev->name); > > + del_timer_sync(&dev->watchdog_timer); > > + flush_workqueue(dev->watchdog_workqueue); > > + destroy_workqueue(dev->watchdog_workqueue); > > + video_unregister_device(dev->vfd_enc); > > + video_unregister_device(dev->vfd_dec); > > + v4l2_device_unregister(&dev->v4l2_dev); > > + vb2_cma_cleanup_multi(dev->alloc_ctx); > > + if (dev->mfc_mutex) { > > + mutex_destroy(dev->mfc_mutex); > > + kfree(dev->mfc_mutex); > > + } > > + mfc_debug("Will now deinit HW\n"); > > + s5p_mfc_deinit_hw(dev); > > + free_irq(dev->irq, dev); > > + iounmap(dev->base_virt_addr); > > + if (dev->mfc_mem != NULL) { > > + release_resource(dev->mfc_mem); > > + kfree(dev->mfc_mem); > > + dev->mfc_mem = NULL; > > + } > > + clk_put(dev->clock1); > > + clk_put(dev->clock2); > > + kfree(dev); > > + dev_dbg(&pdev->dev, "%s--\n", __func__); > > + return 0; > > +} > > + > > +static int s5p_mfc_suspend(struct device *dev) > > +{ > > + return 0; > > +} > > + > > +static int s5p_mfc_resume(struct device *dev) > > +{ > > + return 0; > > +} > > + > > +/* Power management */ > > +static const struct dev_pm_ops s5p_mfc_pm_ops = { > > + .suspend = s5p_mfc_suspend, > > + .resume = s5p_mfc_resume, > > +}; > > + > > +static struct platform_driver s5p_mfc_pdrv = { > > + .probe = s5p_mfc_probe, > > + .remove = __devexit_p(s5p_mfc_remove), > > + .driver = { > > + .name = S5P_MFC_NAME, > > + .owner = THIS_MODULE, > > + .pm = &s5p_mfc_pm_ops}, > > +}; > > + > > +static char banner[] __initdata = > > + "S5P MFC V4L2 Driver, (c) 2010 Samsung > Electronics\n"; > > + > > +static int __init s5p_mfc_init(void) > > +{ > > + pr_info("%s", banner); > > + if (platform_driver_register(&s5p_mfc_pdrv) != 0) { > > + pr_err("Platform device registration failed..\n"); > > + return -1; > > + } > > + return 0; > > +} > > + > > +static void __devexit s5p_mfc_exit(void) > > +{ > > + platform_driver_unregister(&s5p_mfc_pdrv); > > +} > > + > > +module_init(s5p_mfc_init); > > +module_exit(s5p_mfc_exit); > > + > > +MODULE_LICENSE("GPL"); > > +MODULE_AUTHOR("Kamil Debski <k.debski@xxxxxxxxxxx>"); > > +MODULE_AUTHOR("Jeongtae Park <jtp.park@xxxxxxxxxxx>"); > > <snip> -- Kamil Debski Linux Platform Group Samsung Poland R&D Center -- 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
