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

[AngeloGioacchino Del Regno]

Il 17/05/24 11:49, Michael Walle ha scritto:
> Hi Angelo,
>
> On Thu May 16, 2024 at 10:11 AM CEST, AngeloGioacchino Del Regno wrote:
> > 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.
>
> paths might be optional, see comment in mtk_drm_kms_init(). But with
> this patch, you'll get an -EINVAL with a disabled path. See my
> proposals how to fix that below.

I might not be understanding the reason behind allowing that but, per my logic, if
a board does have a path, then it's written in devicetree and enabled - otherwise,
it should not be there at all, in principle.

Can you explain a bit more extensively the reason(s) why we need to account
for disabled paths?

> With these changes and the following two patches I was able to get
> DisplayPort working on vdosys1. vdosys0 wasn't used at all.
> https://lore.kernel.org/r/20240516145824.1669263-1-mwalle@xxxxxxxxxx/
> https://lore.kernel.org/r/20240517093024.1702750-1-mwalle@xxxxxxxxxx/
>
> I've already successfully tested a former version with DSI output on
> vdosys0.
>
> Thanks for working on this!

Thank you for helping with the testing! :-)

Cheers,
Angelo

> > +/**
> > + * 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) {
> > +			dev_err(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) {
> > +			if (!ovl_adaptor_comp_added)
> > +				ovl_adaptor_comp_added = true;
> > +			else
> > +				idx--;
> > +		}
> > +	} while (++idx < DDP_COMPONENT_DRM_ID_MAX);
>
> /* The device might not be disabled. In that case, don't check the last
>   * entry but just report the missing device. */
> 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;
> > +}
> > +
> > +static int mtk_drm_of_ddp_path_build(struct device *dev, struct device_node *node,
> > +				     struct mtk_mmsys_driver_data *data)
> > +{
> > +	struct device_node *ep_node;
> > +	struct of_endpoint of_ep;
> > +	bool output_present[MAX_CRTC] = { false };
> > +	int ret;
> > +
> > +	for_each_endpoint_of_node(node, ep_node) {
> > +		ret = of_graph_parse_endpoint(ep_node, &of_ep);
> > +		of_node_put(ep_node);
> > +		if (ret) {
> > +			dev_err_probe(dev, ret, "Cannot parse endpoint\n");
> > +			break;
> > +		}
> > +
> > +		if (of_ep.id >= MAX_CRTC) {
> > +			ret = dev_err_probe(dev, -EINVAL,
> > +					    "Invalid endpoint%u number\n", of_ep.port);
> > +			break;
> > +		}
> > +
> > +		output_present[of_ep.id] = true;
> > +	}
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (output_present[CRTC_MAIN]) {
> > +		ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_MAIN,
> > +						    &data->main_path, &data->main_len);
> > +		if (ret)
> if (ret && ret != -ENODEV)
>
> > +			return ret;
> > +	}
> > +
> > +	if (output_present[CRTC_EXT]) {
> > +		ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_EXT,
> > +						    &data->ext_path, &data->ext_len);
> > +		if (ret)
> likewise
>
> > +			return ret;
> > +	}
> > +
> > +	if (output_present[CRTC_THIRD]) {
> > +		ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_THIRD,
> > +						    &data->third_path, &data->third_len);
> > +		if (ret)
> likewise
>
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
>
> -michael