> >
> > ...
> >
> > > +static int fops_vcodec_open(struct file *file)
> > > +{
> > > + struct video_device *vfd = video_devdata(file);
> > > + struct mtk_vcodec_dev *dev = video_drvdata(file);
> > > + struct mtk_vcodec_ctx *ctx = NULL;
> > > + int ret = 0;
> > > +
> > > + if (dev->instance_mask == ~0UL) {
> > > + /* ffz Undefined if no zero exists, err handling here */
> > > + mtk_v4l2_err("Too many open contexts");
> > > + ret = -EBUSY;
> > > + goto err_alloc;
> >
> > I'm not happy seeing this here. You should always be able to open the device.
> > I would expect to see a check like this in e.g. start_streaming, since that's
> > where you start to use the hardware for real, and checking if you have enough
> > resources there is perfectly fine.
> >
> > If this is an artificial constraint (i.e. not based on a real hardware limitation),
> > then it should perhaps just be dropped. Such constraints tend to be pointless.
> > If you want to encode 20 streams simultaneously, then why not? It will be very
> > slow, but that's not this driver's problem :-)
> >
> We use ffz to get instance index.
> This only make sure that instance id is correct since in ffz
> description,
> "Undefined if no zero exists, so code should check against ~0UL first."
> In this case, it may not be able to move to start_streaming.
> Any suggestion that how we do this?
The instance index is only used for printing the debug information.
No? If that's the case, you can remove instance index and print
mtk_vcodec_ctx address for debugging.
>
>
> > > + }
> > > +
> > > + mutex_lock(&dev->dev_mutex);
> > > +
> > > + ctx = devm_kzalloc(&dev->plat_dev->dev, sizeof(*ctx), GFP_KERNEL);
> >
> > Why is this a devm_ call? It is not managed by a device, so it seems to me that
> > a regular kzalloc is good enough here.
> >
> > > + if (!ctx) {
> > > + ret = -ENOMEM;
> > > + goto err_alloc;
> > > + }
> > > +
> > > + ctx->idx = ffz(dev->instance_mask);
> > > + v4l2_fh_init(&ctx->fh, video_devdata(file));
> > > + file->private_data = &ctx->fh;
> > > + v4l2_fh_add(&ctx->fh);
> > > + INIT_LIST_HEAD(&ctx->list);
> > > + ctx->dev = dev;
> > > + init_waitqueue_head(&ctx->queue);
> > > +
> > > + if (vfd == dev->vfd_enc) {
> > > + ctx->type = MTK_INST_ENCODER;
> > > + ret = mtk_vcodec_enc_ctrls_setup(ctx);
> > > + if (ret) {
> > > + mtk_v4l2_err("Failed to setup controls() (%d)",
> > > + ret);
> > > + goto err_ctrls_setup;
> > > + }
> > > + ctx->m2m_ctx = v4l2_m2m_ctx_init(dev->m2m_dev_enc, ctx,
> > > + &mtk_vcodec_enc_queue_init);
> > > + if (IS_ERR(ctx->m2m_ctx)) {
> > > + ret = PTR_ERR(ctx->m2m_ctx);
> > > + mtk_v4l2_err("Failed to v4l2_m2m_ctx_init() (%d)",
> > > + ret);
> > > + goto err_m2m_ctx_init;
> > > + }
> > > + mtk_vcodec_enc_set_default_params(ctx);
> > > + } else {
> > > + mtk_v4l2_err("Invalid vfd !");
> >
> > This shouldn't be possible at all. I would just drop the 'if (vfd == dev->vfd_enc)' check.
> Got it, will remove in next version.
>
> >
> > > + ret = -ENOENT;
> > > + goto err_m2m_ctx_init;
> > > + }
> > > +
> > > + if (v4l2_fh_is_singular(&ctx->fh)) {
> > > + ret = vpu_load_firmware(dev->vpu_plat_dev);
> > > + if (ret < 0) {
> > > + /*
> > > + * Return 0 if downloading firmware successfully,
> > > + * otherwise it is failed
> > > + */
> > > + mtk_v4l2_err("vpu_load_firmware failed!");
> > > + goto err_load_fw;
> > > + }
> >
> > The fw load seems to be a one-time thing, but here it is done every time
> > someone opens the device and nobody else had it open.
> >
> > If this is a one time thing, then using a bool 'loaded_fw' makes more sense.
> >
> More than one module use vpu firmware, encoder/decoder/mdp...etc.
> If this is first encode instance, we need to check and load vpu
> firmware.
> vpu_load_firmware will check and load firmware when necessary.
>
> best regards,
> Tiffany
> > > +
> > > + dev->enc_capability =
> > > + vpu_get_venc_hw_capa(dev->vpu_plat_dev);
> > > + mtk_v4l2_debug(0, "encoder capability %x", dev->enc_capability);
> > > + }
> > > +
> > > + mtk_v4l2_debug(2, "Create instance [%d]@%p m2m_ctx=%p ",
> > > + ctx->idx, ctx, ctx->m2m_ctx);
> > > + set_bit(ctx->idx, &dev->instance_mask);
> > > + dev->num_instances++;
> > > + list_add(&ctx->list, &dev->ctx_list);
> > > +
> > > + mutex_unlock(&dev->dev_mutex);
> > > + mtk_v4l2_debug(0, "%s encoder [%d]", dev_name(&dev->plat_dev->dev),
> > > + ctx->idx);
> > > + return ret;
> > > +
> > > + /* Deinit when failure occurred */
> > > +err_load_fw:
> > > + v4l2_m2m_ctx_release(ctx->m2m_ctx);
> > > +err_m2m_ctx_init:
> > > + v4l2_ctrl_handler_free(&ctx->ctrl_hdl);
> > > +err_ctrls_setup:
> > > + v4l2_fh_del(&ctx->fh);
> > > + v4l2_fh_exit(&ctx->fh);
> > > + devm_kfree(&dev->plat_dev->dev, ctx);
> > > +err_alloc:
> > > + mutex_unlock(&dev->dev_mutex);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int fops_vcodec_release(struct file *file)
> > > +{
> > > + struct mtk_vcodec_dev *dev = video_drvdata(file);
> > > + struct mtk_vcodec_ctx *ctx = fh_to_ctx(file->private_data);
> > > +
> > > + mtk_v4l2_debug(1, "[%d] encoder", ctx->idx);
> > > + mutex_lock(&dev->dev_mutex);
> > > +
> > > + mtk_vcodec_enc_release(ctx);
> > > + v4l2_fh_del(&ctx->fh);
> > > + v4l2_fh_exit(&ctx->fh);
> > > + v4l2_ctrl_handler_free(&ctx->ctrl_hdl);
> > > + v4l2_m2m_ctx_release(ctx->m2m_ctx);
> > > +
> > > + list_del_init(&ctx->list);
> > > + dev->num_instances--;
> > > + clear_bit(ctx->idx, &dev->instance_mask);
> > > + devm_kfree(&dev->plat_dev->dev, ctx);
> > > + mutex_unlock(&dev->dev_mutex);
> > > + return 0;
> > > +}
> > > +
> > > +static const struct v4l2_file_operations mtk_vcodec_fops = {
> > > + .owner = THIS_MODULE,
> > > + .open = fops_vcodec_open,
> > > + .release = fops_vcodec_release,
> > > + .poll = v4l2_m2m_fop_poll,
> > > + .unlocked_ioctl = video_ioctl2,
> > > + .mmap = v4l2_m2m_fop_mmap,
> > > +};
> > > +
> > > +static int mtk_vcodec_probe(struct platform_device *pdev)
> > > +{
> > > + struct mtk_vcodec_dev *dev;
> > > + struct video_device *vfd_enc;
> > > + struct resource *res;
> > > + int i, j, ret;
> > > + DEFINE_DMA_ATTRS(attrs);
> > > +
> > > + dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> > > + if (!dev)
> > > + return -ENOMEM;
> > > +
> > > + INIT_LIST_HEAD(&dev->ctx_list);
> > > + dev->plat_dev = pdev;
> > > +
> > > + dev->vpu_plat_dev = vpu_get_plat_device(dev->plat_dev);
> > > + if (dev->vpu_plat_dev == NULL) {
> > > + mtk_v4l2_err("[VPU] vpu device in not ready");
> > > + return -EPROBE_DEFER;
> > > + }
> > > +
> > > + vpu_wdt_reg_handler(dev->vpu_plat_dev, mtk_vcodec_enc_reset_handler,
> > > + dev, VPU_RST_ENC);
> > > +
> > > + ret = mtk_vcodec_init_enc_pm(dev);
> > > + if (ret < 0) {
> > > + dev_err(&pdev->dev, "Failed to get mt vcodec clock source!");
> > > + return ret;
> > > + }
> > > +
> > > + for (i = VENC_SYS, j = 0; i < NUM_MAX_VCODEC_REG_BASE; i++, j++) {
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, j);
> > > + if (res == NULL) {
> > > + dev_err(&pdev->dev, "get memory resource failed.");
> > > + ret = -ENXIO;
> > > + goto err_res;
> > > + }
> > > + dev->reg_base[i] = devm_ioremap_resource(&pdev->dev, res);
> > > + if (IS_ERR(dev->reg_base[i])) {
> > > + dev_err(&pdev->dev,
> > > + "devm_ioremap_resource %d failed.", i);
> > > + ret = PTR_ERR(dev->reg_base[i]);
> > > + goto err_res;
> > > + }
> > > + mtk_v4l2_debug(2, "reg[%d] base=0x%p", i, dev->reg_base[i]);
> > > + }
> > > +
> > > + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > > + if (res == NULL) {
> > > + dev_err(&pdev->dev, "failed to get irq resource");
> > > + ret = -ENOENT;
> > > + goto err_res;
> > > + }
> > > +
> > > + dev->enc_irq = platform_get_irq(pdev, 0);
> > > + ret = devm_request_irq(&pdev->dev, dev->enc_irq,
> > > + mtk_vcodec_enc_irq_handler,
> > > + 0, pdev->name, dev);
> > > + if (ret) {
> > > + dev_err(&pdev->dev, "Failed to install dev->enc_irq %d (%d)",
> > > + dev->enc_irq,
> > > + ret);
> > > + ret = -EINVAL;
> > > + goto err_res;
> > > + }
> > > +
> > > + dev->enc_lt_irq = platform_get_irq(pdev, 1);
> > > + ret = devm_request_irq(&pdev->dev,
> > > + dev->enc_lt_irq, mtk_vcodec_enc_lt_irq_handler,
> > > + 0, pdev->name, dev);
> > > + if (ret) {
> > > + dev_err(&pdev->dev,
> > > + "Failed to install dev->enc_lt_irq %d (%d)",
> > > + dev->enc_lt_irq, ret);
> > > + ret = -EINVAL;
> > > + goto err_res;
> > > + }
> > > +
> > > + disable_irq(dev->enc_irq);
> > > + disable_irq(dev->enc_lt_irq); /* VENC_LT */
> > > + mutex_init(&dev->enc_mutex);
> > > + mutex_init(&dev->dev_mutex);
> > > + spin_lock_init(&dev->irqlock);
> > > +
> > > + snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name), "%s",
> > > + "[MTK_V4L2_VENC]");
> > > +
> > > + ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
> > > + if (ret) {
> > > + mtk_v4l2_err("v4l2_device_register err=%d", ret);
> > > + goto err_res;
> > > + }
> > > +
> > > + init_waitqueue_head(&dev->queue);
> > > +
> > > + /* allocate video device for encoder and register it */
> > > + vfd_enc = video_device_alloc();
> > > + if (!vfd_enc) {
> > > + mtk_v4l2_err("Failed to allocate video device");
> > > + ret = -ENOMEM;
> > > + goto err_enc_alloc;
> > > + }
> > > + vfd_enc->fops = &mtk_vcodec_fops;
> > > + vfd_enc->ioctl_ops = &mtk_venc_ioctl_ops;
> > > + vfd_enc->release = video_device_release;
> > > + vfd_enc->lock = &dev->dev_mutex;
> > > + vfd_enc->v4l2_dev = &dev->v4l2_dev;
> > > + vfd_enc->vfl_dir = VFL_DIR_M2M;
> > > + vfd_enc->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE |
> > > + V4L2_CAP_STREAMING;
> > > +
> > > + snprintf(vfd_enc->name, sizeof(vfd_enc->name), "%s",
> > > + MTK_VCODEC_ENC_NAME);
> > > + video_set_drvdata(vfd_enc, dev);
> > > + dev->vfd_enc = vfd_enc;
> > > + platform_set_drvdata(pdev, dev);
> > > +
> > > + dev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
> > > + if (IS_ERR(dev->alloc_ctx)) {
> > > + dev->alloc_ctx = NULL;
> > > + mtk_v4l2_err("Failed to alloc vb2 dma context 0");
> > > + ret = PTR_ERR(dev->alloc_ctx);
> > > + goto err_vb2_ctx_init;
> > > + }
> > > +
> > > + dev->m2m_dev_enc = v4l2_m2m_init(&mtk_venc_m2m_ops);
> > > + if (IS_ERR(dev->m2m_dev_enc)) {
> > > + mtk_v4l2_err("Failed to init mem2mem enc device");
> > > + ret = PTR_ERR(dev->m2m_dev_enc);
> > > + goto err_enc_mem_init;
> > > + }
> > > +
> > > + dev->encode_workqueue =
> > > + alloc_ordered_workqueue(MTK_VCODEC_ENC_NAME,
> > > + WQ_MEM_RECLAIM |
> > > + WQ_FREEZABLE);
> > > + if (!dev->encode_workqueue) {
> > > + mtk_v4l2_err("Failed to create encode workqueue");
> > > + ret = -EINVAL;
> > > + goto err_event_workq;
> > > + }
> > > +
> > > + ret = video_register_device(vfd_enc, VFL_TYPE_GRABBER, 1);
> > > + if (ret) {
> > > + mtk_v4l2_err("Failed to register video device");
> > > + goto err_enc_reg;
> > > + }
> > > +
> > > + /* Avoid the iommu eat big hunks */
> > > + dma_set_attr(DMA_ATTR_ALLOC_SINGLE_PAGES, &attrs);
> > > +
> > > + mtk_v4l2_debug(0, "encoder registered as /dev/video%d",
> > > + vfd_enc->num);
> > > +
> > > + return 0;
> > > +
> > > +err_enc_reg:
> > > + destroy_workqueue(dev->encode_workqueue);
> > > +err_event_workq:
> > > + v4l2_m2m_release(dev->m2m_dev_enc);
> > > +err_enc_mem_init:
> > > + vb2_dma_contig_cleanup_ctx(dev->alloc_ctx);
> > > +err_vb2_ctx_init:
> > > + video_unregister_device(vfd_enc);
> > > +err_enc_alloc:
> > > + v4l2_device_unregister(&dev->v4l2_dev);
> > > +err_res:
> > > + mtk_vcodec_release_enc_pm(dev);
> > > + return ret;
> > > +}
> > > +
> > > +static const struct of_device_id mtk_vcodec_enc_match[] = {
> > > + {.compatible = "mediatek,mt8173-vcodec-enc",},
> > > + {},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, mtk_vcodec_enc_match);
> > > +
> > > +static int mtk_vcodec_enc_remove(struct platform_device *pdev)
> > > +{
> > > + struct mtk_vcodec_dev *dev = platform_get_drvdata(pdev);
> > > +
> > > + mtk_v4l2_debug_enter();
> > > + flush_workqueue(dev->encode_workqueue);
> > > + destroy_workqueue(dev->encode_workqueue);
> > > + if (dev->m2m_dev_enc)
> > > + v4l2_m2m_release(dev->m2m_dev_enc);
> > > + if (dev->alloc_ctx)
> > > + vb2_dma_contig_cleanup_ctx(dev->alloc_ctx);
> > > +
> > > + if (dev->vfd_enc)
> > > + video_unregister_device(dev->vfd_enc);
> > > +
> > > + v4l2_device_unregister(&dev->v4l2_dev);
> > > + mtk_vcodec_release_enc_pm(dev);
> > > + return 0;
> > > +}
> > > +
> > > +static struct platform_driver mtk_vcodec_enc_driver = {
> > > + .probe = mtk_vcodec_probe,
> > > + .remove = mtk_vcodec_enc_remove,
> > > + .driver = {
> > > + .name = MTK_VCODEC_ENC_NAME,
> > > + .owner = THIS_MODULE,
> > > + .of_match_table = mtk_vcodec_enc_match,
> > > + },
> > > +};
> > > +
> > > +module_platform_driver(mtk_vcodec_enc_driver);
> > > +
> > > +
> > > +MODULE_LICENSE("GPL v2");
> > > +MODULE_DESCRIPTION("Mediatek video codec V4L2 encoder driver");
> >
> > 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
--
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
