hi Tomasz:
Thanks for your review comments, the corresponding modification
& explanation is under preparation, will update soon.
Thanks.
BR
Daoyuan
On Tue, 2019-06-04 at 20:20 +0900, Tomasz Figa wrote:
> Hi Daoyuan,
>
> On Thu, May 16, 2019 at 11:23:32AM +0800, Daoyuan Huang wrote:
> > From: daoyuan huang <daoyuan.huang@xxxxxxxxxxxx>
> >
> > This patch adds driver for Media Data Path 3 (MDP3).
> > Each modules' related operation control is sited in mtk-mdp3-comp.c
> > Each modules' register table is defined in file with "mdp_reg_"
> > and "mmsys_" prefix
> > GCE related API, operation control sited in mtk-mdp3-cmdq.c
> > V4L2 m2m device functions are implemented in mtk-mdp3-m2m.c
> > Probe, power, suspend/resume, system level functions are defined in
> > mtk-mdp3-core.c
>
> Thanks for the patch. Some initial comments inline.
>
> [snip]
> > +void mdp_comp_clock_on(struct device *dev, struct mdp_comp *comp)
> > +{
> > + int i, err;
> > +
> > + if (comp->larb_dev) {
> > + err = pm_runtime_get_sync(comp->larb_dev);
> > + if (err < 0)
> > + dev_err(dev,
> > + "Failed to get larb, err %d. type:%d id:%d\n",
> > + err, comp->type, comp->id);
>
> There is no need to make this conditional. Actually, we always need to grab
> a runtime PM enable count, otherwise the power domain would power off (if
> this SoC has gate'able power domains).
>
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(comp->clks); i++) {
> > + if (IS_ERR(comp->clks[i]))
> > + break;
> > + err = clk_prepare_enable(comp->clks[i]);
> > + if (err)
> > + dev_err(dev,
> > + "Failed to enable clock %d, err %d. type:%d id:%d\n",
> > + i, err, comp->type, comp->id);
> > + }
> > +}
> > +
> > +void mdp_comp_clock_off(struct device *dev, struct mdp_comp *comp)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(comp->clks); i++) {
> > + if (IS_ERR(comp->clks[i]))
> > + break;
> > + clk_disable_unprepare(comp->clks[i]);
> > + }
> > +
> > + if (comp->larb_dev)
> > + pm_runtime_put_sync(comp->larb_dev);
>
> Ditto. Also, it doesn't make sense to use the _sync variant here, we don't
> care if it powers off before the function returns or a bit after.
>
> [snip]
>
> > +static int mdp_comp_init(struct device *dev, struct mdp_dev *mdp,
> > + struct device_node *node, struct mdp_comp *comp,
> > + enum mdp_comp_id id)
> > +{
> > + struct device_node *larb_node;
> > + struct platform_device *larb_pdev;
> > + int i;
> > +
> > + if (id < 0 || id >= MDP_MAX_COMP_COUNT) {
> > + dev_err(dev, "Invalid component id %d\n", id);
> > + return -EINVAL;
> > + }
> > +
> > + __mdp_comp_init(mdp, node, comp);
> > + comp->type = mdp_comp_matches[id].type;
> > + comp->id = id;
> > + comp->alias_id = mdp_comp_matches[id].alias_id;
> > + comp->ops = mdp_comp_ops[comp->type];
> > +
> > + for (i = 0; i < ARRAY_SIZE(comp->clks); i++) {
> > + comp->clks[i] = of_clk_get(node, i);
> > + if (IS_ERR(comp->clks[i]))
> > + break;
> > + }
> > +
> > + mdp_get_subsys_id(dev, node, comp);
> > +
> > + /* Only DMA capable components need the LARB property */
> > + comp->larb_dev = NULL;
> > + if (comp->type != MDP_COMP_TYPE_RDMA &&
> > + comp->type != MDP_COMP_TYPE_WROT &&
> > + comp->type != MDP_COMP_TYPE_WDMA)
> > + return 0;
> > +
> > + larb_node = of_parse_phandle(node, "mediatek,larb", 0);
> > + if (!larb_node) {
> > + dev_err(dev, "Missing mediatek,larb phandle in %pOF node\n",
> > + node);
> > + return -EINVAL;
> > + }
> > +
> > + larb_pdev = of_find_device_by_node(larb_node);
> > + if (!larb_pdev) {
> > + dev_warn(dev, "Waiting for larb device %pOF\n", larb_node);
> > + of_node_put(larb_node);
> > + return -EPROBE_DEFER;
> > + }
> > + of_node_put(larb_node);
> > +
> > + comp->larb_dev = &larb_pdev->dev;
>
> Why do we need this larb_dev? I believe we already made the handling
> transparent by using device links, so as long as the driver handles runtime
> PM properly, it should automatically handle larbs.
>
> > +
> > + return 0;
> > +}
> > +
> > +static void mdp_comp_deinit(struct device *dev, struct mdp_comp *comp)
> > +{
> > + iounmap(comp->regs);
> > + /* of_node_put(comp->dev_node); */
> > +}
> > +
> > +void mdp_component_deinit(struct device *dev, struct mdp_dev *mdp)
> > +{
> > + int i;
> > +
> > + mdp_comp_deinit(dev, &mdp->mmsys);
> > + mdp_comp_deinit(dev, &mdp->mm_mutex);
> > + for (i = 0; i < ARRAY_SIZE(mdp->comp); i++) {
> > + if (mdp->comp[i]) {
> > + mdp_comp_deinit(dev, mdp->comp[i]);
> > + kfree(mdp->comp[i]);
> > + }
> > + }
> > +}
> > +
> > +int mdp_component_init(struct device *dev, struct mdp_dev *mdp)
> > +{
> > + struct device_node *node, *parent;
> > + int i, ret;
> > +
> > + for (i = 0; i < ARRAY_SIZE(gce_event_names); i++) {
> > + s32 event_id;
> > +
> > + event_id = cmdq_dev_get_event(dev, gce_event_names[i]);
> > + mdp->event[i] = (event_id < 0) ? -i : event_id;
> > + dev_info(dev, "Get event %s id:%d\n",
> > + gce_event_names[i], mdp->event[i]);
> > + }
> > +
> > + ret = mdp_mm_init(dev, mdp, &mdp->mmsys, "mediatek,mmsys");
> > + if (ret)
> > + goto err_init_mm;
> > +
> > + ret = mdp_mm_init(dev, mdp, &mdp->mm_mutex, "mediatek,mm-mutex");
> > + if (ret)
> > + goto err_init_mm;
> > +
> > + parent = dev->of_node->parent;
> > + /* Iterate over sibling MDP function blocks */
> > + for_each_child_of_node(parent, node) {
> > + const struct of_device_id *of_id;
> > + enum mdp_comp_type type;
> > + int id;
> > + struct mdp_comp *comp;
> > +
> > + of_id = of_match_node(mdp_comp_dt_ids, node);
> > + if (!of_id)
> > + continue;
> > +
> > + if (!of_device_is_available(node)) {
> > + dev_err(dev, "Skipping disabled component %pOF\n",
> > + node);
>
> The function doesn't fail, so this doesn't look like error. Perhaps
> dev_info()?
>
> > + continue;
> > + }
> > +
> > + type = (enum mdp_comp_type)of_id->data;
> > + id = mdp_comp_get_id(dev, node, type);
> > + if (id < 0) {
> > + dev_warn(dev, "Skipping unknown component %pOF\n",
> > + node);
> > + continue;
> > + }
> > +
> > + comp = devm_kzalloc(dev, sizeof(*comp), GFP_KERNEL);
> > + if (!comp) {
> > + ret = -ENOMEM;
> > + goto err_init_comps;
> > + }
> > + mdp->comp[id] = comp;
> > +
> > + ret = mdp_comp_init(dev, mdp, node, comp, id);
> > + if (ret)
> > + goto err_init_comps;
> > +
> > + dev_info(dev, "%s type:%d alias:%d id:%d base:%#x regs:%p\n",
> > + of_id->compatible, type, comp->alias_id, id,
> > + (u32)comp->reg_base, comp->regs);
> > + }
> > + return 0;
> > +
> > +err_init_comps:
> > + mdp_component_deinit(dev, mdp);
> > +err_init_mm:
> > + return ret;
> > +}
> [snip]
> > +static int mdp_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct mdp_dev *mdp;
> > + phandle rproc_phandle;
> > + int ret;
> > +
> > + mdp = devm_kzalloc(dev, sizeof(*mdp), GFP_KERNEL);
> > + if (!mdp)
> > + return -ENOMEM;
> > +
> > + mdp->pdev = pdev;
> > + ret = mdp_component_init(dev, mdp);
> > + if (ret) {
> > + dev_err(dev, "Failed to initialize mdp components\n");
> > + goto err_init_comp;
>
> There should be no cleanup needed here.
>
> Please always name the labels after the first cleanup step that is needed
> after the jump. That helps avoiding such mistakes.
>
> > + }
> > +
> > + mdp->job_wq = create_singlethread_workqueue(MDP_MODULE_NAME);
>
> We should make it freezable.
>
> > + if (!mdp->job_wq) {
> > + dev_err(dev, "Unable to create job workqueue\n");
> > + ret = -ENOMEM;
> > + goto err_create_job_wq;
> > + }
> > +
> > + mdp->vpu_dev = scp_get_pdev(pdev);
> > +
> > + if (of_property_read_u32(pdev->dev.of_node, "mediatek,scp",
> > + &rproc_phandle))
> > + dev_err(&pdev->dev, "Could not get scp device\n");
>
> This should fail the probe.
>
> > + else
> > + dev_info(&pdev->dev, "Find mediatek,scp phandle:%llx\n",
> > + (unsigned long long)rproc_phandle);
>
> No need for this positive log.
>
> > +
> > + mdp->rproc_handle = rproc_get_by_phandle(rproc_phandle);
> > +
> > + dev_info(&pdev->dev, "MDP rproc_handle: %llx",
> > + (unsigned long long)mdp->rproc_handle);
> > +
> > + if (!mdp->rproc_handle)
> > + dev_err(&pdev->dev, "Could not get MDP's rproc_handle\n");
>
> This should fail the probe too.
>
> > +
> > + /* vpu_wdt_reg_handler(mdp->vpu_dev, mdp_reset_handler, mdp,
> > + * VPU_RST_MDP);
> > + */
>
> Please remove unnecessary code.
>
> > + mutex_init(&mdp->vpu_lock);
> > + mdp->vpu_count = 0;
> > + mdp->id_count = 0;
>
> No need to explicitly initialize to 0, because we just allocated a zeroed
> struct earlier in this function.
>
> > +
> > + mdp->cmdq_clt = cmdq_mbox_create(dev, 0, 1200);
> > + if (IS_ERR(mdp->cmdq_clt))
> > + goto err_mbox_create;
> > +
> > + ret = v4l2_device_register(dev, &mdp->v4l2_dev);
> > + if (ret) {
> > + dev_err(dev, "Failed to register v4l2 device\n");
> > + ret = -EINVAL;
> > + goto err_v4l2_register;
> > + }
> > +
> > + ret = mdp_m2m_device_register(mdp);
> > + if (ret) {
> > + v4l2_err(&mdp->v4l2_dev, "Failed to register m2m device\n");
> > + goto err_m2m_register;
> > + }
> > + mutex_init(&mdp->m2m_lock);
> > +
> > + platform_set_drvdata(pdev, mdp);
> > +
> > + vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
> > + pm_runtime_enable(dev);
>
> This is racy, because mdp_m2m_device_register() already exposed the video
> node to the userspace, before we ended the initialization. Please reorder the
> initialization so that the device node is registered at the end, when the
> driver is fully ready to accept userspace requests.
>
> > + dev_dbg(dev, "mdp-%d registered successfully\n", pdev->id);
> > + return 0;
> > +
> > +err_m2m_register:
> > + v4l2_device_unregister(&mdp->v4l2_dev);
> > +err_v4l2_register:
>
> Shouldn't we destroy the cmdq_mbox?
>
> > +err_mbox_create:
> > + destroy_workqueue(mdp->job_wq);
> > +err_create_job_wq:
> > +err_init_comp:
> > + kfree(mdp);
>
> This was allocated using devm_kzalloc(), so no need to free it explicitly.
>
> > +
> > + dev_dbg(dev, "Errno %d\n", ret);
> > + return ret;
> > +}
> > +
> > +static int mdp_remove(struct platform_device *pdev)
> > +{
> > + struct mdp_dev *mdp = platform_get_drvdata(pdev);
> > +
> > + pm_runtime_disable(&pdev->dev);
> > + vb2_dma_contig_clear_max_seg_size(&pdev->dev);
> > + mdp_m2m_device_unregister(mdp);
>
> Same as in probe, removing must start with unregistering the userspace
> interfaces.
>
> > + v4l2_device_unregister(&mdp->v4l2_dev);
> > +
> > + flush_workqueue(mdp->job_wq);
> > + destroy_workqueue(mdp->job_wq);
>
> destroy_workqueue() includes a flush (or drain specifically, which is
> stricter than flush).
>
> > +
> > + mdp_component_deinit(&pdev->dev, mdp);
> > + kfree(mdp);
>
> Allocated by devm_kzalloc(), so no need to free here.
>
> > +
> > + dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused mdp_pm_suspend(struct device *dev)
> > +{
> > + // TODO: mdp clock off
>
> Is MDP inside a power domain that is controlled by genpd? If yes, runtime
> suspend would be cover for the power domain sequencing latency, which would
> cause the clock to be running unnecessarily for that time. To avoid that,
> one would control the clocks separately, outside of runtime PM callbacks.
>
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused mdp_pm_resume(struct device *dev)
> > +{
> > + // TODO: mdp clock on
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused mdp_suspend(struct device *dev)
> > +{
> > + if (pm_runtime_suspended(dev))
> > + return 0;
> > +
>
> We need to ensure here that the hardware is not doing any processing
> anymore.
>
> Hint: Looks like mdp_m2m_worker() is synchronous, so we could make the
> workqueue freezable and the PM core would wait for the current pending work
> to complete and then freeze the workqueue.
>
> > + return mdp_pm_suspend(dev);
> > +}
> > +
> > +static int __maybe_unused mdp_resume(struct device *dev)
> > +{
> > + if (pm_runtime_suspended(dev))
> > + return 0;
> > +
> > + return mdp_pm_resume(dev);
>
> We need to resume any processing that was queued to the driver before the
> system suspended. Should be possible to handle by switching the workqueue to
> freezable too.
>
> > +}
> > +
> > +static const struct dev_pm_ops mdp_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(mdp_suspend, mdp_resume)
> > + SET_RUNTIME_PM_OPS(mdp_pm_suspend, mdp_pm_resume, NULL)
> > +};
> > +
> > +static struct platform_driver mdp_driver = {
> > + .probe = mdp_probe,
> > + .remove = mdp_remove,
> > + .driver = {
> > + .name = MDP_MODULE_NAME,
> > + .pm = &mdp_pm_ops,
> > + .of_match_table = mdp_of_ids,
>
> Please use the of_match_ptr() wrapper.
>
> [snip]
> > +static int mdp_m2m_ctrls_create(struct mdp_m2m_ctx *ctx)
> > +{
> > + v4l2_ctrl_handler_init(&ctx->ctrl_handler, MDP_MAX_CTRLS);
> > + ctx->ctrls.hflip = v4l2_ctrl_new_std(&ctx->ctrl_handler,
> > + &mdp_m2m_ctrl_ops, V4L2_CID_HFLIP,
> > + 0, 1, 1, 0);
> > + ctx->ctrls.vflip = v4l2_ctrl_new_std(&ctx->ctrl_handler,
> > + &mdp_m2m_ctrl_ops, V4L2_CID_VFLIP,
> > + 0, 1, 1, 0);
> > + ctx->ctrls.rotate = v4l2_ctrl_new_std(&ctx->ctrl_handler,
> > + &mdp_m2m_ctrl_ops,
> > + V4L2_CID_ROTATE, 0, 270, 90, 0);
> > +
> > + if (ctx->ctrl_handler.error) {
> > + int err = ctx->ctrl_handler.error;
> > +
> > + v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> > + dev_err(&ctx->mdp_dev->pdev->dev,
> > + "Failed to create control handler\n");
>
> Perhaps "Failed to register controls"?
>
> > + return err;
> > + }
> > + return 0;
> > +}
> > +
> > +static int mdp_m2m_open(struct file *file)
> > +{
> > + struct video_device *vdev = video_devdata(file);
> > + struct mdp_dev *mdp = video_get_drvdata(vdev);
> > + struct mdp_m2m_ctx *ctx;
> > + int ret;
> > +
> > + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> > + if (!ctx)
> > + return -ENOMEM;
> > +
> > + if (mutex_lock_interruptible(&mdp->m2m_lock)) {
> > + ret = -ERESTARTSYS;
> > + goto err_lock;
> > + }
> > +
> > + ctx->id = mdp->id_count++;
>
> Hmm, wouldn't this leave holes in the id space after we close? Should we use
> something like ida?
> See: https://www.kernel.org/doc/htmldocs/kernel-api/idr.html
>
> > + ctx->mdp_dev = mdp;
> > +
> > + v4l2_fh_init(&ctx->fh, vdev);
> > + file->private_data = &ctx->fh;
> > + ret = mdp_m2m_ctrls_create(ctx);
> > + if (ret)
> > + goto err_ctrls_create;
> > +
> > + /* Use separate control handler per file handle */
> > + ctx->fh.ctrl_handler = &ctx->ctrl_handler;
> > + v4l2_fh_add(&ctx->fh);
> > +
> > + ctx->m2m_ctx = v4l2_m2m_ctx_init(mdp->m2m_dev, ctx, mdp_m2m_queue_init);
> > + if (IS_ERR(ctx->m2m_ctx)) {
> > + dev_err(&mdp->pdev->dev, "Failed to initialize m2m context\n");
> > + ret = PTR_ERR(ctx->m2m_ctx);
> > + goto err_m2m_ctx;
> > + }
> > + ctx->fh.m2m_ctx = ctx->m2m_ctx;
> > +
> > + INIT_WORK(&ctx->work, mdp_m2m_worker);
> > + ctx->frame_count = 0;
>
> No need to explicitly initialize fields to 0, because we just allocated the
> struct using kzalloc().
>
> > +
> > + ctx->curr_param = mdp_frameparam_init();
> > + if (IS_ERR(ctx->curr_param)) {
> > + dev_err(&mdp->pdev->dev,
> > + "Failed to initialize mdp parameter\n");
> > + ret = PTR_ERR(ctx->curr_param);
> > + goto err_param_init;
> > + }
> > + ctx->curr_param->type = MDP_STREAM_TYPE_BITBLT;
>
> Why not initialize this in mdp_frameparam_init()? We only seem to be using
> this value in this driver.
>
> > +
> > + INIT_LIST_HEAD(&ctx->param_list);
> > + list_add_tail(&ctx->curr_param->list, &ctx->param_list);
>
> Why do we need this list? We only seem to have ctx->curr_param in this
> driver.
>
> > +
> > + ret = mdp_vpu_get_locked(mdp);
> > + if (ret < 0)
> > + goto err_load_vpu;
>
> This shouldn't happen in open(), but rather the latest possible point in
> time. If one needs to keep the VPU running for the time of streaming, then
> it should be start_streaming. If one can safely turn the VPU off if there is
> no frame queued for long time, it should be just in m2m job_run.
>
> Generally the userspace should be able to
> just open an m2m device for querying it, without any side effects like
> actually powering on the hardware or grabbing a hardware instance (which
> could block some other processes, trying to grab one too).
>
> > +
> > + mutex_unlock(&mdp->m2m_lock);
> > +
> > + mdp_dbg(0, "%s [%d]", dev_name(&mdp->pdev->dev), ctx->id);
> > +
> > + return 0;
> > +
> > +err_load_vpu:
> > + mdp_frameparam_release(ctx->curr_param);
> > +err_param_init:
> > + v4l2_m2m_ctx_release(ctx->m2m_ctx);
> > +err_m2m_ctx:
> > + v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> > + v4l2_fh_del(&ctx->fh);
> > +err_ctrls_create:
> > + v4l2_fh_exit(&ctx->fh);
> > + mutex_unlock(&mdp->m2m_lock);
> > +err_lock:
>
> Incorrect naming of all the error labels here.
>
> > + kfree(ctx);
> > +
> > + return ret;
> > +}
> [snip]
> > +enum mdp_ycbcr_profile mdp_map_ycbcr_prof_mplane(struct v4l2_format *f,
> > + u32 mdp_color)
> > +{
> > + struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > +
> > + if (MDP_COLOR_IS_RGB(mdp_color))
> > + return MDP_YCBCR_PROFILE_FULL_BT601;
> > +
> > + switch (pix_mp->colorspace) {
> > + case V4L2_COLORSPACE_JPEG:
> > + return MDP_YCBCR_PROFILE_JPEG;
> > + case V4L2_COLORSPACE_REC709:
> > + case V4L2_COLORSPACE_DCI_P3:
> > + if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> > + return MDP_YCBCR_PROFILE_FULL_BT709;
> > + return MDP_YCBCR_PROFILE_BT709;
> > + case V4L2_COLORSPACE_BT2020:
> > + if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> > + return MDP_YCBCR_PROFILE_FULL_BT2020;
> > + return MDP_YCBCR_PROFILE_BT2020;
> > + }
> > + /* V4L2_COLORSPACE_SRGB or else */
> > + if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> > + return MDP_YCBCR_PROFILE_FULL_BT601;
> > + return MDP_YCBCR_PROFILE_BT601;
>
> Putting this under the default clause of the switch statement would be
> cleaner and the comment wouldn't be needed.
>
> [snip]
> > +struct mdp_frameparam *mdp_frameparam_init(void)
> > +{
> > + struct mdp_frameparam *param;
> > + struct mdp_frame *frame;
> > +
> > + param = kzalloc(sizeof(*param), GFP_KERNEL);
> > + if (!param)
> > + return ERR_PTR(-ENOMEM);
>
> We could just embed mdp_frameparam into the mdp_m2m_ctx struct and then
> wouldn't need any dynamic allocation here anymore. And as a side effect, the
> function could be just made void, because it couldn't fail.
>
> > +
> > + INIT_LIST_HEAD(¶m->list);
> > + mutex_init(¶m->lock);
> > + param->state = 0;
> > + param->limit = &mdp_def_limit;
> > + param->type = MDP_STREAM_TYPE_UNKNOWN;
>
> We always seem to use MDP_STREAM_TYPE_BITBLT in this driver.
>
> > + param->frame_no = 0;
>
> No need for explicit initialization to 0.
>
> Best regards,
> Tomasz
>
