Re: [RFC, v3, 4/4] media: platform: mtk-mdp3: Add Mediatek MDP3 driver

Re: [RFC, v3, 4/4] media: platform: mtk-mdp3: Add Mediatek MDP3 driver

[CK Hu]

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

Re: [RFC, v3, 4/4] media: platform: mtk-mdp3: Add Mediatek MDP3 driver

[Pi-Hsun Shih]

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