* Re: [RFC, v3, 4/4] media: platform: mtk-mdp3: Add Mediatek MDP3 driver
[not found] <20190911094013.5892-1-bibby.hsieh@mediatek.com>
@ 2019-09-23 1:58 ` CK Hu
2019-11-07 3:58 ` Pi-Hsun Shih
1 sibling, 0 replies; 2+ messages in thread
From: CK Hu @ 2019-09-23 1:58 UTC (permalink / raw)
To: Bibby Hsieh
Cc: laurent.pinchart+renesas, Rynn.Wu, Jerry-ch.Chen, jungo.lin,
hans.verkuil, Ping-Hsun Wu, frederic.chen, linux-media,
devicetree, daoyuan huang, holmes.chiou, sj.huang, yuzhao,
linux-mediatek, matthias.bgg, mchehab, linux-arm-kernel,
Sean.Cheng, srv_heupstream, tfiga, christie.yu, zwisler
Hi, Bibby:
On Wed, 2019-09-11 at 17:40 +0800, Bibby Hsieh wrote:
> From: daoyuan huang <daoyuan.huang@mediatek.com>
>
> 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
>
> Signed-off-by: Ping-Hsun Wu <ping-hsun.wu@mediatek.com>
> Signed-off-by: daoyuan huang <daoyuan.huang@mediatek.com>
> ---
[snip]
> +
> diff --git a/drivers/media/platform/mtk-mdp3/mtk-mdp3-cmdq.c b/drivers/media/platform/mtk-mdp3/mtk-mdp3-cmdq.c
> new file mode 100644
> index 000000000000..bd8f274fcaa9
> --- /dev/null
> +++ b/drivers/media/platform/mtk-mdp3/mtk-mdp3-cmdq.c
> @@ -0,0 +1,504 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Ping-Hsun Wu <ping-hsun.wu@mediatek.com>
> + */
> +
> +#include <linux/platform_device.h>
> +#include "mtk-mdp3-cmdq.h"
> +#include "mtk-mdp3-comp.h"
> +#include "mtk-mdp3-core.h"
> +#include "mtk-mdp3-m2m.h"
> +
> +#include "mdp-platform.h"
> +#include "mmsys_mutex.h"
> +
> +#define DISP_MUTEX_MDP_FIRST (5)
> +#define DISP_MUTEX_MDP_COUNT (5)
> +
> +#define MDP_PATH_MAX_COMPS IMG_MAX_COMPONENTS
> +
> +struct mdp_path {
> + struct mdp_dev *mdp_dev;
> + struct mdp_comp_ctx comps[MDP_PATH_MAX_COMPS];
> + u32 num_comps;
> + const struct img_config *config;
> + const struct img_ipi_frameparam *param;
> + const struct v4l2_rect *composes[IMG_MAX_HW_OUTPUTS];
> + struct v4l2_rect bounds[IMG_MAX_HW_OUTPUTS];
> +};
> +
> +#define has_op(ctx, op) \
> + (ctx->comp->ops && ctx->comp->ops->op)
> +#define call_op(ctx, op, ...) \
> + (has_op(ctx, op) ? ctx->comp->ops->op(ctx, ##__VA_ARGS__) : 0)
> +
> +struct mdp_path_subfrm {
> + s32 mutex_id;
> + u32 mutex_mod;
> + s32 sofs[MDP_PATH_MAX_COMPS];
> + u32 num_sofs;
> +};
> +
> +static bool is_output_disable(const struct img_compparam *param, u32 count)
> +{
> + return (count < param->num_subfrms) ?
> + (param->frame.output_disable ||
> + param->subfrms[count].tile_disable) :
> + true;
> +}
> +
> +static int mdp_path_subfrm_require(struct mdp_path_subfrm *subfrm,
> + const struct mdp_path *path,
> + struct mdp_cmd *cmd, u32 count)
> +{
> + const struct img_config *config = path->config;
> + const struct mdp_comp_ctx *ctx;
> + phys_addr_t mm_mutex = path->mdp_dev->mm_mutex.reg_base;
> + s32 mutex_id = -1;
> + u32 mutex_sof = 0;
> + int mdp_color = 0;
> + int index;
> + u8 subsys_id = path->mdp_dev->mm_mutex.subsys_id;
> +
> + /* Default value */
> + memset(subfrm, 0, sizeof(*subfrm));
> +
> + for (index = 0; index < config->num_components; index++) {
> + ctx = &path->comps[index];
> + if (is_output_disable(ctx->param, count))
> + continue;
> + switch (ctx->comp->id) {
> + /**********************************************
> + * Name MSB LSB
> + * DISP_MUTEX_MOD 23 0
> + *
> + * Specifies which modules are in this mutex.
> + * Every bit denotes a module. Bit definition:
> + * 2 mdp_rdma0
> + * 4 mdp_rsz0
> + * 5 mdp_rsz1
> + * 6 mdp_tdshp
> + * 7 mdp_wrot0
> + * 8 mdp_wdma
> + * 13 mdp_color
> + * 23 mdp_aal
> + * 24 mdp_ccorr
> + **********************************************/
> + case MDP_AAL0:
> + subfrm->mutex_mod |= 1 << 23;
> + break;
> + case MDP_CCORR0:
> + subfrm->mutex_mod |= 1 << 24;
> + break;
> + case MDP_COLOR0:
> + if (mdp_color)
> + subfrm->mutex_mod |= 1 << 13;
> + break;
> + case MDP_WDMA:
> + subfrm->mutex_mod |= 1 << 8;
> + subfrm->sofs[subfrm->num_sofs++] = MDP_WDMA;
> + break;
> + case MDP_WROT0:
> + subfrm->mutex_mod |= 1 << 7;
> + subfrm->sofs[subfrm->num_sofs++] = MDP_WROT0;
> + break;
> + case MDP_TDSHP0:
> + subfrm->mutex_mod |= 1 << 6;
> + subfrm->sofs[subfrm->num_sofs++] = MDP_TDSHP0;
> + break;
> + case MDP_SCL1:
> + subfrm->mutex_mod |= 1 << 5;
> + subfrm->sofs[subfrm->num_sofs++] = MDP_SCL1;
> + break;
> + case MDP_SCL0:
> + subfrm->mutex_mod |= 1 << 4;
> + subfrm->sofs[subfrm->num_sofs++] = MDP_SCL0;
> + break;
> + case MDP_RDMA0:
> + mutex_id = DISP_MUTEX_MDP_FIRST + 1;
> + subfrm->mutex_mod |= 1 << 2;
> + subfrm->sofs[subfrm->num_sofs++] = MDP_RDMA0;
> + break;
> + case MDP_IMGI:
> + mutex_id = DISP_MUTEX_MDP_FIRST;
> + break;
> + case MDP_WPEI:
> + mutex_id = DISP_MUTEX_MDP_FIRST + 3;
> + break;
> + case MDP_WPEI2:
> + mutex_id = DISP_MUTEX_MDP_FIRST + 4;
> + break;
> + default:
> + break;
> + }
> + }
> +
> + subfrm->mutex_id = mutex_id;
> + if (-1 == mutex_id) {
> + mdp_err("No mutex assigned");
> + return -EINVAL;
> + }
> +
> + if (subfrm->mutex_mod) {
> + /* Set mutex modules */
> + MM_REG_WRITE(cmd, subsys_id, mm_mutex, MM_MUTEX_MOD,
> + subfrm->mutex_mod, 0x07FFFFFF);
> + MM_REG_WRITE(cmd, subsys_id, mm_mutex, MM_MUTEX_SOF,
> + mutex_sof, 0x00000007);
> + }
In [1], mdp3 device has a pointer to mutex device, and you directly
write mutex register in mdp3 driver.
mdp_rdma0: mdp_rdma0@14001000 {
compatible = "mediatek,mt8183-mdp-rdma",
"mediatek,mt8183-mdp3";
mediatek,mm-mutex = <&mutex>;
};
mutex: mutex@14016000 {
compatible = "mediatek,mt8183-disp-mutex";
reg = <0 0x14016000 0 0x1000>;
};
But mutex already has its own driver in mediatek drm driver [2]. This is
not a good design. I would like all mutex device register is controlled
in single driver.
[1] https://patchwork.kernel.org/patch/11140747/
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/mediatek/mtk_drm_ddp.c?h=v5.3#n429
Regards,
CK
> + return 0;
> +}
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [RFC, v3, 4/4] media: platform: mtk-mdp3: Add Mediatek MDP3 driver
[not found] <20190911094013.5892-1-bibby.hsieh@mediatek.com>
2019-09-23 1:58 ` [RFC, v3, 4/4] media: platform: mtk-mdp3: Add Mediatek MDP3 driver CK Hu
@ 2019-11-07 3:58 ` Pi-Hsun Shih
1 sibling, 0 replies; 2+ messages in thread
From: Pi-Hsun Shih @ 2019-11-07 3:58 UTC (permalink / raw)
To: Bibby Hsieh, hans.verkuil, laurent.pinchart+renesas, tfiga,
matthias.bgg, mchehab
Cc: devicetree, Sean.Cheng, Rynn.Wu, srv_heupstream, Daoyuan.Huang,
holmes.chiou, Jerry-ch.Chen, jungo.lin, sj.huang, yuzhao,
linux-mediatek, Ping-Hsun Wu, zwisler, christie.yu, frederic.chen,
linux-arm-kernel, linux-media
Hi,
On 9/11/19 5:40 PM, Bibby Hsieh wrote:
> From: daoyuan huang <daoyuan.huang@mediatek.com>
>
> 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
>
> Signed-off-by: Ping-Hsun Wu <ping-hsun.wu@mediatek.com>
> Signed-off-by: daoyuan huang <daoyuan.huang@mediatek.com>
> ---
> ...
> diff --git a/drivers/media/platform/mtk-mdp3/mtk-mdp3-core.c b/drivers/media/platform/mtk-mdp3/mtk-mdp3-core.c
> new file mode 100644
> ... > +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(mdp);
> + if (ret) {
> + dev_err(dev, "Failed to initialize mdp components\n");
> + goto err_return;
> + }
> +
> + mdp->job_wq = alloc_workqueue(MDP_MODULE_NAME, WQ_FREEZABLE, 0);
> + if (!mdp->job_wq) {
> + dev_err(dev, "Unable to create job workqueue\n");
> + ret = -ENOMEM;
> + goto err_destroy_job_wq;
> + }
> +
> + mdp->clock_wq = alloc_workqueue(MDP_MODULE_NAME "-clock", WQ_FREEZABLE,
> + 0);
> + if (!mdp->clock_wq) {
> + dev_err(dev, "Unable to create clock workqueue\n");
> + ret = -ENOMEM;
> + goto err_destroy_clock_wq;
> + }
> +
> + mdp->vpu_dev = scp_get_pdev(pdev);
> +
> + ret = of_property_read_u32(pdev->dev.of_node, "mediatek,scp",
> + &rproc_phandle);
> + if (ret) {
> + dev_err(&pdev->dev, "Could not get scp device\n");
> + goto err_destroy_clock_wq;
> + }
> +
> + 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");
"ret" is not set in this error path, ret = -ENODEV?
> + goto err_destroy_clock_wq;
> + }
> +
> + mutex_init(&mdp->vpu_lock);
> + mutex_init(&mdp->m2m_lock);
> +
> + mdp->cmdq_clt = cmdq_mbox_create(dev, 0, 1200);
> + if (IS_ERR(mdp->cmdq_clt))
Same here, ret = PTR_ERR(mdp->cmdq_clt)?
> + goto err_destroy_clock_wq;
> +
> + init_waitqueue_head(&mdp->callback_wq);
> + ida_init(&mdp->mdp_ida);
> + platform_set_drvdata(pdev, mdp);
> +
> + vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
> + pm_runtime_enable(dev);
> +
> + ret = v4l2_device_register(dev, &mdp->v4l2_dev);
> + if (ret) {
> + dev_err(dev, "Failed to register v4l2 device\n");
> + ret = -EINVAL;
> + goto err_mbox_destroy;
> + }
> +
> + ret = mdp_m2m_device_register(mdp);
> + if (ret) {
> + v4l2_err(&mdp->v4l2_dev, "Failed to register m2m device\n");
> + goto err_unregister_device;
> + }
> +
> + dev_dbg(dev, "mdp-%d registered successfully\n", pdev->id);
> + return 0;
> +
> +err_unregister_device:
> + v4l2_device_unregister(&mdp->v4l2_dev);
> +err_mbox_destroy:
> + cmdq_mbox_destroy(mdp->cmdq_clt);
> +err_destroy_clock_wq:
> + destroy_workqueue(mdp->clock_wq);
> +err_destroy_job_wq:
> + destroy_workqueue(mdp->job_wq);
> +err_return:
> + dev_dbg(dev, "Errno %d\n", ret);
> + return ret;
> +}
> ...
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 2+ messages in thread