Date: Mon, 17 Jun 2024 15:24:39 +0200
From: "Michael Walle" <michael@...le.cc>
To: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@...labora.com>,
 <chunkuang.hu@...nel.org>
Cc: <robh@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
 <conor+dt@...nel.org>, <p.zabel@...gutronix.de>, <airlied@...il.com>,
 <daniel@...ll.ch>, <maarten.lankhorst@...ux.intel.com>,
 <mripard@...nel.org>, <tzimmermann@...e.de>, <matthias.bgg@...il.com>,
 <shawn.sung@...iatek.com>, <yu-chang.lee@...iatek.com>,
 <ck.hu@...iatek.com>, <jitao.shi@...iatek.com>,
 <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <dri-devel@...ts.freedesktop.org>, <linux-mediatek@...ts.infradead.org>,
 <linux-arm-kernel@...ts.infradead.org>, <wenst@...omium.org>,
 <kernel@...labora.com>, <sui.jinfeng@...ux.dev>, "Alexandre Mergnat"
 <amergnat@...libre.com>
Subject: Re: [PATCH v7 3/3] drm/mediatek: Implement OF graphs support for
 display paths

Hi 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 component might not be disabled: in that case, don't

Sorry there was a typo in my proposal, This should either be
"not be enabled" or "be disabled".

> +	 * check the last entry and just report that the device is missing.
> +	 */
> +	if (ret == -ENODEV)
> +		return ret;
> +

..

> +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 };
> +	bool valid_output_found = false;
> +	int ret;
> +
> +	for_each_endpoint_of_node(node, ep_node) {
> +		ret = of_graph_parse_endpoint(ep_node, &of_ep);
> +		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) {
> +		of_node_put(ep_node);
> +		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 == 0)
> +			valid_output_found = true;
> +		else if (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 == 0)
> +			valid_output_found = true;
> +		else if (ret != -ENODEV)
> +			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 == 0)
> +			valid_output_found = true;
> +		else if (ret != -ENODEV)
> +			return ret;
> +	}
> +
> +	if (!valid_output_found)
> +		return -ENODEV;

This doesn't work. My proposal just ignored the ENODEV error. Now
you'll return ENODEV if there is no output for a given mmsys. In my
case, that is true for the first mmsys. Subsequent mmsys's doesn't
get probed in that case, it seems.

Anyway, you shouldn't return ENODEV here because disabled just
means not available, i.e. it should be treated the same as
"output_present[] == false".

-michael

Download attachment "signature.asc" of type "application/pgp-signature" (298 bytes)

Powered by blists - more mailing lists