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

[CK Hu <ck.hu@mediatek.com>]

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;
> +}