Re: [PATCH v10 3/3] drm/mediatek: Implement OF graphs support for display paths

[CK Hu (胡俊光)]

Hi, Angelo:

On Tue, 2024-09-10 at 10:51 +0000, AngeloGioacchino Del Regno wrote:
> It is impossible to add each and every possible DDP path combination
> for each and every possible combination of SoC and board: right now,
> this driver hardcodes configuration for 10 SoCs and this is going to
> grow larger and larger, and with new hacks like the introduction of
> mtk_drm_route which is anyway not enough for all final routes as the
> DSI cannot be connected to MERGE if it's not a dual-DSI, or enabling
> DSC preventively doesn't work if the display doesn't support it, or
> others.
> 
> Since practically all display IPs in MediaTek SoCs support being
> interconnected with different instances of other, or the same, IPs
> or with different IPs and in different combinations, the final DDP
> pipeline is effectively a board specific configuration.
> 
> Implement OF graphs support to the mediatek-drm drivers, allowing to
> stop hardcoding the paths, and preventing this driver to get a huge
> amount of arrays for each board and SoC combination, also paving the
> way to share the same mtk_mmsys_driver_data between multiple SoCs,
> making it more straightforward to add support for new chips.
> 
> Reviewed-by: Alexandre Mergnat <amergnat@xxxxxxxxxxxx>
> Tested-by: Alexandre Mergnat <amergnat@xxxxxxxxxxxx>
> Acked-by: Sui Jingfeng <sui.jingfeng@xxxxxxxxx>
> Tested-by: Michael Walle <mwalle@xxxxxxxxxx> # on kontron-sbc-i1200
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> ---

[snip]

> +
> +bool mtk_ovl_adaptor_is_comp_present(struct device_node *node)
> +{
> +	enum mtk_ovl_adaptor_comp_type type;
> +	int ret;
> +
> +	ret = ovl_adaptor_of_get_ddp_comp_type(node, &type);
> +	if (ret)
> +		return false;
> +
> +	if (type >= OVL_ADAPTOR_TYPE_NUM)
> +		return false;
> +
> +	/*
> +	 * ETHDR and Padding are used exclusively in OVL Adaptor: if this
> +	 * component is not one of those, it's likely not an OVL Adaptor path.
> +	 */

I don't know your logic here.
The OVL Adaptor pipeline is:

mdp_rdma -> padding ---+      +-------+
                     Merge -> |       |
mdp_rdma -> padding ---+      |       |
                              |       |
mdp_rdma -> padding ---+      |       |
                     Merge -> |       |
mdp_rdma -> padding ---+      |       |
                              | ETHDR |
mdp_rdma -> padding ---+      |       |
                     Merge -> |       |
mdp_rdma -> padding ---+      |       |
                              |       |
mdp_rdma -> padding ---+      |       |
                     Merge -> |       |
mdp_rdma -> padding ---+      +-------+

So mdp_rdma and merge is not OVL Adaptor?


> +	return type == OVL_ADAPTOR_TYPE_ETHDR || type == OVL_ADAPTOR_TYPE_PADDING;
> +}
> +
>  

[snip]

> +
> +/**
> + * mtk_drm_of_ddp_path_build_one - Build a Display HW Pipeline for a CRTC Path
> + * @dev:          The mediatek-drm device
> + * @cpath:        CRTC Path relative to a VDO or MMSYS
> + * @out_path:     Pointer to an array that will contain the new pipeline
> + * @out_path_len: Number of entries in the pipeline array
> + *
> + * MediaTek SoCs can use different DDP hardware pipelines (or paths) depending
> + * on the board-specific desired display configuration; this function walks
> + * through all of the output endpoints starting from a VDO or MMSYS hardware
> + * instance and builds the right pipeline as specified in device trees.
> + *
> + * Return:
> + * * %0       - Display HW Pipeline successfully built and validated
> + * * %-ENOENT - Display pipeline was not specified in device tree
> + * * %-EINVAL - Display pipeline built but validation failed
> + * * %-ENOMEM - Failure to allocate pipeline array to pass to the caller
> + */
> +static int mtk_drm_of_ddp_path_build_one(struct device *dev, enum mtk_crtc_path cpath,
> +					 const unsigned int **out_path,
> +					 unsigned int *out_path_len)
> +{
> +	struct device_node *next, *prev, *vdo = dev->parent->of_node;
> +	unsigned int temp_path[DDP_COMPONENT_DRM_ID_MAX] = { 0 };
> +	unsigned int *final_ddp_path;
> +	unsigned short int idx = 0;
> +	bool ovl_adaptor_comp_added = false;
> +	int ret;
> +
> +	/* Get the first entry for the temp_path array */
> +	ret = mtk_drm_of_get_ddp_ep_cid(vdo, 0, cpath, &next, &temp_path[idx]);
> +	if (ret) {
> +		if (next && temp_path[idx] == DDP_COMPONENT_DRM_OVL_ADAPTOR) {

mdp_rdma would not be DDP_COMPONENT_DRM_OVL_ADAPTOR.

> +			dev_dbg(dev, "Adding OVL Adaptor for %pOF\n", next);
> +			ovl_adaptor_comp_added = true;
> +		} else {
> +			if (next)
> +				dev_err(dev, "Invalid component %pOF\n", next);
> +			else
> +				dev_err(dev, "Cannot find first endpoint for path %d\n", cpath);
> +
> +			return ret;
> +		}
> +	}
> +	idx++;
> +
> +	/*
> +	 * Walk through port outputs until we reach the last valid mediatek-drm component.
> +	 * To be valid, this must end with an "invalid" component that is a display node.
> +	 */
> +	do {
> +		prev = next;
> +		ret = mtk_drm_of_get_ddp_ep_cid(next, 1, cpath, &next, &temp_path[idx]);
> +		of_node_put(prev);
> +		if (ret) {
> +			of_node_put(next);
> +			break;
> +		}
> +
> +		/*
> +		 * If this is an OVL adaptor exclusive component and one of those
> +		 * was already added, don't add another instance of the generic
> +		 * DDP_COMPONENT_OVL_ADAPTOR, as this is used only to decide whether
> +		 * to probe that component master driver of which only one instance
> +		 * is needed and possible.
> +		 */
> +		if (temp_path[idx] == DDP_COMPONENT_DRM_OVL_ADAPTOR) {

merge would not be DDP_COMPONENT_DRM_OVL_ADAPTOR.
Finally, the path would be:

mdp_rdma -> ovl adaptor (padding) -> merge -> (ethdr is skipped here) ...

Regards,
CK

> +			if (!ovl_adaptor_comp_added)
> +				ovl_adaptor_comp_added = true;
> +			else
> +				idx--;
> +		}
> +	} while (++idx < DDP_COMPONENT_DRM_ID_MAX);
> +
> +	/*
> +	 * The device component might not be enabled: in that case, don't
> +	 * check the last entry and just report that the device is missing.
> +	 */
> +	if (ret == -ENODEV)
> +		return ret;
> +
> +	/* If the last entry is not a final display output, the configuration is wrong */
> +	switch (temp_path[idx - 1]) {
> +	case DDP_COMPONENT_DP_INTF0:
> +	case DDP_COMPONENT_DP_INTF1:
> +	case DDP_COMPONENT_DPI0:
> +	case DDP_COMPONENT_DPI1:
> +	case DDP_COMPONENT_DSI0:
> +	case DDP_COMPONENT_DSI1:
> +	case DDP_COMPONENT_DSI2:
> +	case DDP_COMPONENT_DSI3:
> +		break;
> +	default:
> +		dev_err(dev, "Invalid display hw pipeline. Last component: %d (ret=%d)\n",
> +			temp_path[idx - 1], ret);
> +		return -EINVAL;
> +	}
> +
> +	final_ddp_path = devm_kmemdup(dev, temp_path, idx * sizeof(temp_path[0]), GFP_KERNEL);
> +	if (!final_ddp_path)
> +		return -ENOMEM;
> +
> +	dev_dbg(dev, "Display HW Pipeline built with %d components.\n", idx);
> +
> +	/* Pipeline built! */
> +	*out_path = final_ddp_path;
> +	*out_path_len = idx;
> +
> +	return 0;
> +}
> +