Re: [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection"

[Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>]

Hi

On 4/21/22 09:13, Paul Kocialkowski wrote:
> Hi,
>
> On Wed 20 Apr 22, 16:19, Bjorn Andersson wrote:
>> On Wed 20 Apr 16:12 PDT 2022, Bjorn Andersson wrote:
>>
>> Sorry, I missed Jagan and Linus, author and reviewer of the reverted
>> patch 2, among the recipients.
> I'd be curious to have Jagan's feedback on why the change was needed in the
> first place and whether an accepted dt binding relies on it.
>
> We might be able to just keep the whole thing reverted (forever).


This patch was merged within the last mainline kernel and was breaking our STM32MP15 platforms.

Switching back to drm-based kernel (drm-next-fixes branch) made me realize this patch was faulty.
I'm glad it was reverted.


Thanks,

Raphaël

>
> Paul
>
>> Regards,
>> Bjorn
>>
>>> Commit '80253168dbfd ("drm: of: Lookup if child node has panel or
>>> bridge")' introduced the ability to describe a panel under a display
>>> controller without having to use a graph to connect the controller to
>>> its single child panel (or bridge).
>>>
>>> The implementation of this would find the first non-graph node and
>>> attempt to acquire the related panel or bridge. This prevents cases
>>> where any other child node, such as a aux bus for a DisplayPort
>>> controller, or an opp-table to find the referenced panel.
>>>
>>> Commit '67bae5f28c89 ("drm: of: Properly try all possible cases for
>>> bridge/panel detection")' attempted to solve this problem by not
>>> bypassing the graph reference lookup before attempting to find the panel
>>> or bridge.
>>>
>>> While this does solve the case where a proper graph reference is
>>> present, it does not allow the caller to distinguish between a
>>> yet-to-be-probed panel or bridge and the absence of a reference to a
>>> panel.
>>>
>>> One such case is a DisplayPort controller that on some boards have an
>>> explicitly described reference to a panel, but on others have a
>>> discoverable DisplayPort display attached (which doesn't need to be
>>> expressed in DeviceTree).
>>>
>>> This reverts commit '67bae5f28c89 ("drm: of: Properly try all possible
>>> cases for bridge/panel detection")', as a step towards reverting commit
>>> '80253168dbfd ("drm: of: Lookup if child node has panel or bridge")'.
>>>
>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>> ---
>>>  drivers/gpu/drm/drm_of.c | 99 ++++++++++++++++++++--------------------
>>>  1 file changed, 49 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
>>> index f4df344509a8..026e4e29a0f3 100644
>>> --- a/drivers/gpu/drm/drm_of.c
>>> +++ b/drivers/gpu/drm/drm_of.c
>>> @@ -214,29 +214,6 @@ int drm_of_encoder_active_endpoint(struct device_node *node,
>>>  }
>>>  EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
>>>  
>>> -static int find_panel_or_bridge(struct device_node *node,
>>> -				struct drm_panel **panel,
>>> -				struct drm_bridge **bridge)
>>> -{
>>> -	if (panel) {
>>> -		*panel = of_drm_find_panel(node);
>>> -		if (!IS_ERR(*panel))
>>> -			return 0;
>>> -
>>> -		/* Clear the panel pointer in case of error. */
>>> -		*panel = NULL;
>>> -	}
>>> -
>>> -	/* No panel found yet, check for a bridge next. */
>>> -	if (bridge) {
>>> -		*bridge = of_drm_find_bridge(node);
>>> -		if (*bridge)
>>> -			return 0;
>>> -	}
>>> -
>>> -	return -EPROBE_DEFER;
>>> -}
>>> -
>>>  /**
>>>   * drm_of_find_panel_or_bridge - return connected panel or bridge device
>>>   * @np: device tree node containing encoder output ports
>>> @@ -259,44 +236,66 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>>>  				struct drm_panel **panel,
>>>  				struct drm_bridge **bridge)
>>>  {
>>> -	struct device_node *node;
>>> -	int ret;
>>> +	int ret = -EPROBE_DEFER;
>>> +	struct device_node *remote;
>>>  
>>>  	if (!panel && !bridge)
>>>  		return -EINVAL;
>>> -
>>>  	if (panel)
>>>  		*panel = NULL;
>>> -	if (bridge)
>>> -		*bridge = NULL;
>>> -
>>> -	/* Check for a graph on the device node first. */
>>> -	if (of_graph_is_present(np)) {
>>> -		node = of_graph_get_remote_node(np, port, endpoint);
>>> -		if (node) {
>>> -			ret = find_panel_or_bridge(node, panel, bridge);
>>> -			of_node_put(node);
>>> -
>>> -			if (!ret)
>>> -				return 0;
>>> -		}
>>> -	}
>>>  
>>> -	/* Otherwise check for any child node other than port/ports. */
>>> -	for_each_available_child_of_node(np, node) {
>>> -		if (of_node_name_eq(node, "port") ||
>>> -		    of_node_name_eq(node, "ports"))
>>> +	/**
>>> +	 * Devices can also be child nodes when we also control that device
>>> +	 * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
>>> +	 *
>>> +	 * Lookup for a child node of the given parent that isn't either port
>>> +	 * or ports.
>>> +	 */
>>> +	for_each_available_child_of_node(np, remote) {
>>> +		if (of_node_name_eq(remote, "port") ||
>>> +		    of_node_name_eq(remote, "ports"))
>>>  			continue;
>>>  
>>> -		ret = find_panel_or_bridge(node, panel, bridge);
>>> -		of_node_put(node);
>>> +		goto of_find_panel_or_bridge;
>>> +	}
>>> +
>>> +	/*
>>> +	 * of_graph_get_remote_node() produces a noisy error message if port
>>> +	 * node isn't found and the absence of the port is a legit case here,
>>> +	 * so at first we silently check whether graph presents in the
>>> +	 * device-tree node.
>>> +	 */
>>> +	if (!of_graph_is_present(np))
>>> +		return -ENODEV;
>>> +
>>> +	remote = of_graph_get_remote_node(np, port, endpoint);
>>> +
>>> +of_find_panel_or_bridge:
>>> +	if (!remote)
>>> +		return -ENODEV;
>>> +
>>> +	if (panel) {
>>> +		*panel = of_drm_find_panel(remote);
>>> +		if (!IS_ERR(*panel))
>>> +			ret = 0;
>>> +		else
>>> +			*panel = NULL;
>>> +	}
>>> +
>>> +	/* No panel found yet, check for a bridge next. */
>>> +	if (bridge) {
>>> +		if (ret) {
>>> +			*bridge = of_drm_find_bridge(remote);
>>> +			if (*bridge)
>>> +				ret = 0;
>>> +		} else {
>>> +			*bridge = NULL;
>>> +		}
>>>  
>>> -		/* Stop at the first found occurrence. */
>>> -		if (!ret)
>>> -			return 0;
>>>  	}
>>>  
>>> -	return -EPROBE_DEFER;
>>> +	of_node_put(remote);
>>> +	return ret;
>>>  }
>>>  EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
>>>  
>>> -- 
>>> 2.35.1
>>>