Re: [PATCH i-g-t 02/14] lib/igt_kms: add helper to get current link rate/ lane count for connector

[Imre Deak <imre.deak@intel.com>]

On Mon, Aug 26, 2024 at 01:36:00AM +0530, Kunal Joshi wrote:
> i915_dp_force_(lane_count/link_rate) debugfs files expoose
> current link rate/lane count for connector.add helper to get
> current link rate/ lane count for connector
> 
> Signed-off-by: Kunal Joshi <kunal1.joshi@intel.com>
> ---
>  lib/igt_kms.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_kms.h |  2 ++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index dd530dbab..4ef7f4d7f 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -6705,3 +6705,61 @@ int get_num_scalers(igt_display_t *display, enum pipe pipe)
>  
>  	return num_scalers;
>  }
> +
> +/**
> + * igt_get_dp_link_rate_set_for_output:
> + * @drm_fd: A drm file descriptor
> + * @output: Target output
> + *
> + * Returns: Link rate set for the output.
> + */
> +enum dp_link_rate igt_get_dp_link_rate_set_for_output(int drm_fd, igt_output_t *output)
> +{
> +	char buf[512];
> +	int dir, res;
> +	int link_rate;
> +
> +	igt_require_f(output->name, "Invalid output");

It's strange to check this in these library functions. I'd think the
caller should make sure already that it's a valid output, how can it
become invalid in the test this patchset adds?

> +	dir = igt_debugfs_connector_dir(drm_fd, output->name, O_RDONLY);
> +	igt_assert_f(dir >= 0, "Failed to open debugfs dir for connector %s\n",
> +				 igt_output_name(output));
> +	res = igt_debugfs_simple_read(dir, "i915_dp_force_link_rate", buf, sizeof(buf));
> +	close(dir);
> +	igt_require(res > 0);

res == 0 is also valid.

> +
> +	if (strstr(buf, "[auto]"))
> +		igt_assert(sscanf(strstr(buf, "*") - 6, "%d", &link_rate) == 1);
> +	else
> +		igt_assert(sscanf(strstr(buf, "[") + 1, "%d", &link_rate) == 1);

I suppose this should return the rate used for an active output. It
should be the one with the '*' marker, regardless if the rate is getting
selected automatically or forced. In case it's forced the selected rate
could be different than the target with the '[]' markers (if the sink
doesn't support the target rate). So I think this should return the one
marked with '*' or an error if such is not found (when the output is
disabled).

> +
> +	return link_rate;
> +}
> +
> +/**
> + * igt_get_dp_lane_count_set_for_output:
> + * @drm_fd: A drm file descriptor
> + * @output: Target output
> + *
> + * Returns: Lane count set for the output.
> + */
> +enum dp_lane_count igt_get_dp_lane_count_set_for_output(int drm_fd, igt_output_t *output)
> +{
> +	char buf[512];
> +	int dir, res;
> +	int lane_count;
> +
> +	igt_require_f(output->name, "Invalid output");
> +	dir = igt_debugfs_connector_dir(drm_fd, output->name, O_RDONLY);
> +	igt_assert_f(dir >= 0, "Failed to open debugfs dir for connector %s\n",
> +				 igt_output_name(output));
> +	res = igt_debugfs_simple_read(dir, "i915_dp_force_lane_count", buf, sizeof(buf));
> +	close(dir);
> +	igt_require(res > 0);
> +
> +	if (strstr(buf, "[auto]"))
> +		igt_assert(sscanf(strstr(buf, "*") - 2, "%d", &lane_count) == 1);
> +	else
> +		igt_assert(sscanf(strstr(buf, "[") + 1, "%d", &lane_count) == 1);

The same as above apply in this function too.

> +
> +	return lane_count;
> +}
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 999921f9f..9d42a2b35 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -1247,5 +1247,7 @@ bool intel_pipe_output_combo_valid(igt_display_t *display);
>  bool igt_check_output_is_dp_mst(igt_output_t *output);
>  int igt_get_dp_mst_connector_id(igt_output_t *output);
>  int get_num_scalers(igt_display_t *display, enum pipe pipe);
> +enum dp_link_rate igt_get_dp_link_rate_set_for_output(int drm_fd, igt_output_t *output);
> +enum dp_lane_count igt_get_dp_lane_count_set_for_output(int drm_fd, igt_output_t *output);
>  
>  #endif /* __IGT_KMS_H__ */
> -- 
> 2.34.1
>