Re: [PATCH v8 3/5] drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Reed-Solomon (RS) corrections are incorrectly restricted to 4-lane FRL configurations.

Pre-existing issues:
- [High] Missing lock release (deadlock) in scdc_status_show error path.
--

--- Patch [3]: [PATCH v8 3/5] drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields ---
commit a8f03be96ae72bbee1ab3aab07506aef81f2031e
Author: Nicolas Frattaroli <[email protected]>

    drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields
    
    HDMI 2.1 redefines previously reserved fields in SCDC for various new
    uses. No version check needs to be performed, as an HDMI 2.0 sink's
    reserved SCDC fields are well-defined to be 0, and any zero-ness of
    these fields for an HDMI 2.0 sink is not a surprise for SCDC parsers for
    HDMI 2.1.
    
    Implement reading and outputting these fields over debugfs.

> diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
> index 97256b272329a..73d46de33ab55 100644
> --- a/drivers/gpu/drm/display/drm_scdc_helper.c
> +++ b/drivers/gpu/drm/display/drm_scdc_helper.c
[ ... ]
> @@ -350,11 +417,26 @@ int drm_scdc_read_state(struct drm_connector *connector, struct drm_scdc_state *
>  
>  	state->scrambling_detected = buf[SCDC_SCRAMBLER_STATUS] & SCDC_SCRAMBLING_STATUS;
>  
> +	state->rate = FIELD_GET(SCDC_FRL_RATE, buf[SCDC_CONFIG_1]);
> +	num_lanes = drm_scdc_num_frl_lanes(state->rate);
> +	if (num_lanes < 0)
> +		return num_lanes;
> +	if (!num_lanes)
> +		num_lanes = 3;
> +
> +	state->ffe_levels = FIELD_GET(SCDC_FFE_LEVELS, buf[SCDC_CONFIG_1]);
> +
>  	drm_scdc_parse_status0_flags(buf[SCDC_STATUS_FLAGS_0], &state->stf);
> -	ret = drm_scdc_parse_error_counters(buf, state->error_count);
> +	drm_scdc_parse_status1_2_flags(buf[SCDC_STATUS_FLAGS_1],
> +				       buf[SCDC_STATUS_FLAGS_2], &state->stf);
> +	ret = drm_scdc_parse_error_counters(buf, state->error_count, num_lanes);
>  	if (ret)
>  		return ret;
>  
> +	if (num_lanes == 4 && (buf[SCDC_ERR_DET_RS_H] & SCDC_CHANNEL_VALID))
> +		state->rs_corrections = (buf[SCDC_ERR_DET_RS_H] & ~SCDC_CHANNEL_VALID) << 8 |
> +					buf[SCDC_ERR_DET_RS_L];

[Severity: Medium]
Since HDMI 2.1 Fixed Rate Link (FRL) uses Reed-Solomon Forward Error
Correction for both 3-lane and 4-lane configurations, is it intentional
to only read RS corrections when num_lanes == 4?

Valid RS correction data for 3-lane FRL links (e.g., 3 Gbps and 6 Gbps on
3 lanes) seems to be silently ignored here.

> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_scdc_read_state);
> @@ -416,6 +498,8 @@ static int scdc_status_show(struct seq_file *m, void *data)

[Severity: High]
This is a pre-existing issue, but could the early exit for a disconnected
connector create a deadlock in this function?

When a connector is disconnected, the code jumps to err_conn_put:

	ret = mutex_lock_interruptible(&connector->dev->mode_config.mutex);
	if (ret)
		goto err_conn_put;

	if (connector->status != connector_status_connected) {
		ret = -ENODEV;
		goto err_conn_put;
	}

But the err_conn_put label skips err_unlock:

err_unlock:
	mutex_unlock(&connector->dev->mode_config.mutex);
err_conn_put:
	drm_connector_put(connector);

Does this leave mode_config.mutex permanently locked?

>  
>  	scdc_print_flag(m, "Scrambling Enabled", st->scrambling_enabled);
>  	scdc_print_flag(m, "Scrambling Detected", st->scrambling_detected);
> +	scdc_print_str(m, "FRL Rate", drm_scdc_frl_rate_str(st->rate));
> +	scdc_print_dec(m, "FFE Levels", st->ffe_levels);
>  
>  	if (st->tmds_bclk_x40)
> @@ -426,10 +510,19 @@ static int scdc_status_show(struct seq_file *m, void *data)
>  	scdc_print_flag(m, "Channel 0 Locked", st->stf.ch0_locked);
>  	scdc_print_flag(m, "Channel 1 Locked", st->stf.ch1_locked);
>  	scdc_print_flag(m, "Channel 2 Locked", st->stf.ch2_locked);
> +	if (drm_scdc_num_frl_lanes(st->rate) == 4)
> +		scdc_print_flag(m, "Lane 3 Locked", st->stf.ln3_locked);
> +
> +	scdc_print_flag(m, "Sink Ready For Link Training", st->stf.flt_ready);
> +	scdc_print_flag(m, "Sink Failed To Decode DSC", st->stf.dsc_fail);
>  
>  	scdc_print_dec(m, "Channel 0 Errors", st->error_count[0]);
>  	scdc_print_dec(m, "Channel 1 Errors", st->error_count[1]);
>  	scdc_print_dec(m, "Channel 2 Errors", st->error_count[2]);
> +	if (drm_scdc_num_frl_lanes(st->rate) == 4) {
> +		scdc_print_dec(m, "Lane 3 Errors", st->error_count[3]);
> +		scdc_print_dec(m, "Reed-Solomon Corrections", st->rs_corrections);

[Severity: Medium]
Similarly here, shouldn't Reed-Solomon corrections also be printed for 3-lane
FRL configurations?

> +	}
>  
>  	kfree(st);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3