Re: [PATCH v6 5/5] drm/bridge: analogix_dp: Add support for optional data-lanes mapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `data-lanes` array is interpreted backwards when configuring the `ANALOGIX_DP_LANE_MAP` register, applying the inverse of the intended hardware lane mapping.
- [High] The external PHY's analog configuration (voltage and pre-emphasis) ignores the logical-to-physical lane mapping.
- [Medium] Mismatched endpoint specifiers are used when counting data lanes versus reading the `data-lanes` property, which could lead to parsing the wrong endpoint.
--

--- Patch [5]: [PATCH v6 5/5] drm/bridge: analogix_dp: Add support for optional data-lanes mapping ---
commit 99a86aa8f074546e71df9c8b7f51f7d473ef6dac
Author: Damon Ding <damon.ding@rock-chips.com>

drm/bridge: analogix_dp: Add support for optional data-lanes mapping
    
Parse the optional data-lanes device tree property to support custom physical
lane mapping configuration. If no valid configuration is found, fall back to
the default lane map automatically.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index b69743837092b..5c5cb6148cf6c 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
[ ... ]
> @@ -1234,6 +1234,59 @@ static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
>  static int analogix_dp_dt_parse_lanes_map(struct analogix_dp_device *dp)
>  {
>  	struct video_info *video_info = &dp->video_info;
>  	struct device_node *endpoint;
>  	u32 tmp[LANE_COUNT4];
>  	u32 map[LANE_COUNT4] = {0, 1, 2, 3};
>  	bool used[LANE_COUNT4] = {false};
>  	int num_lanes;
>  	int ret, i;
>  
>  	memcpy(video_info->lane_map, map, sizeof(map));
>  
>  	num_lanes = drm_of_get_data_lanes_count_ep(dp->dev->of_node, 1, 0, 1,
>  						   video_info->max_lane_count);
>  	if (num_lanes < 0)
>  		return -EINVAL;
>  
>  	endpoint = of_graph_get_endpoint_by_regs(dp->dev->of_node, 1, -1);

[Severity: Medium]
Will this return the correct endpoint if there are multiple endpoints on
port 1?

When counting the data lanes, drm_of_get_data_lanes_count_ep() is called with
reg = 0. However, of_graph_get_endpoint_by_regs() uses reg = -1 (wildcard),
which returns the first available endpoint.

If the device tree has multiple endpoints, could this result in counting lanes
on one endpoint but reading the data-lanes property from a different one?

>  	if (!endpoint)
>  		return -EINVAL;
>  
>  	ret = of_property_read_u32_array(endpoint, "data-lanes", tmp, num_lanes);
>  	of_node_put(endpoint);
>  	if (ret)
>  		return -EINVAL;
>  
>  	for (i = 0; i < num_lanes; i++) {
>  		if (tmp[i] >= LANE_COUNT4) {
>  			dev_dbg(dp->dev, "data-lanes[%d] = %u is out of range\n", i, tmp[i]);
>  			return -EINVAL;
>  		}
>  
>  		if (used[tmp[i]]) {
>  			dev_dbg(dp->dev, "data-lanes[%d] = %u is duplicate\n", i, tmp[i]);
>  			return -EINVAL;
>  		}
>  
>  		used[tmp[i]] = true;
>  		map[i] = tmp[i];

[Severity: High]
Does analogix_dp_set_lane_link_training() need to be updated to use this lane
map?

This parsing logic creates a logical-to-physical mapping. However, looking at
analogix_dp_set_lane_link_training(), it appears to still use the logical lane
index to populate the physical PHY configuration structure:

	for (lane = 0; lane < lane_count; lane++) {
		...
		phy_cfg.dp.voltage[lane] = vs;
		phy_cfg.dp.pre[lane] = pe;
	...

Will this cause the external PHY to apply the analog settings to the wrong
physical lanes when an asymmetric mapping is used?

>  	}
>  
>  	for (i = 0; i < LANE_COUNT4 && num_lanes < LANE_COUNT4; i++) {
>  		if (!used[i])
>  			map[num_lanes++] = i;
>  	}
[ ... ]
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index 38fd8d5014d29..45c7652645a13 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
[ ... ]
> @@ -48,16 +48,15 @@ void analogix_dp_stop_video(struct analogix_dp_device *dp)
>  void analogix_dp_lane_mapping(struct analogix_dp_device *dp)
>  {
> +	u32 *lane_map = dp->video_info.lane_map;
>  	u32 reg;
>  
> -	if (enable)
> -		reg = LANE3_MAP_LOGIC_LANE_0 | LANE2_MAP_LOGIC_LANE_1 |
> -		      LANE1_MAP_LOGIC_LANE_2 | LANE0_MAP_LOGIC_LANE_3;
> -	else
> -		reg = LANE3_MAP_LOGIC_LANE_3 | LANE2_MAP_LOGIC_LANE_2 |
> -		      LANE1_MAP_LOGIC_LANE_1 | LANE0_MAP_LOGIC_LANE_0;
> +	reg = lane_map[0] << LANE0_MAP_SHIFT;
> +	reg |= lane_map[1] << LANE1_MAP_SHIFT;
> +	reg |= lane_map[2] << LANE2_MAP_SHIFT;
> +	reg |= lane_map[3] << LANE3_MAP_SHIFT;

[Severity: High]
Does this logic configure the lane mapping backwards?

The lane map parsed from the device tree establishes lane_map[logical] =
physical. However, this register assignment shifts the physical lane index
(lane_map[i]) by the shift for the physical lane's register field
(LANEi_MAP_SHIFT).

Because the hardware register expects the logical lane number to be written
to the physical lane's bitfield, won't this write the physical lane number
into the physical lane's field instead? This effectively applies the inverse
permutation and could break output for any asymmetric lane mappings.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707040613.1992841-1-damon.ding@rock-chips.com?part=5