Re: [PATCH i-g-t 3/6] tests/intel/kms_dp_linktrain_fallback: refactor dp-fallback subtest

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 2/11/2025 12:26 AM, Kunal Joshi wrote:
> Extract some of the logic into below function for better reuse.
>    - force_failure_and_wait()
>    - wait_for_hotplug_and_check_bad()
>    - fix_link_status_and_recommit()
>
> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Signed-off-by: Kunal Joshi <kunal1.joshi@intel.com>
> ---
>   tests/intel/kms_dp_linktrain_fallback.c | 160 +++++++++++++++++++-----
>   1 file changed, 130 insertions(+), 30 deletions(-)
>
> diff --git a/tests/intel/kms_dp_linktrain_fallback.c b/tests/intel/kms_dp_linktrain_fallback.c
> index 95aa7eee4..284e6b9aa 100644
> --- a/tests/intel/kms_dp_linktrain_fallback.c
> +++ b/tests/intel/kms_dp_linktrain_fallback.c
> @@ -238,16 +238,124 @@ static int check_condition_with_timeout(int drm_fd, igt_output_t *output,
>   	}
>   }
>   
> +/*
> + * Force a link training failure followed by link retrain, then
> + * block until the driver has no further pending retrain/failure.
> + * Returns false if we time out waiting.
> + */
> +static bool force_failure_and_wait(data_t *data,
> +                                   igt_output_t *output,
> +                                   int failure_type,
> +                                   int retrain_count,
> +                                   double interval,
> +                                   double timeout)
> +{
> +        igt_force_lt_failure(data->drm_fd, output, failure_type);
> +        igt_force_link_retrain(data->drm_fd, output, retrain_count);
> +
> +        /* Wait until there's no pending retrain */
> +        if (check_condition_with_timeout(data->drm_fd, output,
> +                                         igt_get_dp_pending_retrain,
> +                                         interval, timeout)) {
> +                igt_info("Timed out waiting for pending retrain.\n");

Generally debug msgs do not have full stop/period, so should remove this 
from here and in other places below.


> +                return false;
> +        }
> +
> +        /* Wait until there's no pending LT failures */
> +        if (check_condition_with_timeout(data->drm_fd, output,
> +                                         igt_get_dp_pending_lt_failures,
> +                                         interval, timeout)) {
> +                igt_info("Timed out waiting for pending LT failures.\n");
> +                return false;
> +        }
> +
> +        return true;
> +}
> +
> +/*
> + * Waits for a hotplug event, then checks that the link-status is BAD.
> + * Returns false if the link-status isn't BAD or no hotplug arrives in time.
> + */
> +static bool wait_for_hotplug_and_check_bad(int drm_fd,
> +                                           data_t *data,
> +                                           igt_output_t *output,
> +                                           struct udev_monitor *mon,
> +                                           double hotplug_timeout)
> +{
> +        uint32_t link_status_prop_id;
> +        uint64_t link_status_value;
> +        drmModePropertyPtr link_status_prop;
> +
> +        if (!igt_hotplug_detected(mon, hotplug_timeout)) {
> +                igt_info("No hotplug event within %.2f seconds.\n", hotplug_timeout);
> +                return false;
> +        }
> +
> +        kmstest_get_property(drm_fd,
> +                             output->config.connector->connector_id,
> +                             DRM_MODE_OBJECT_CONNECTOR,
> +                             "link-status",
> +                             &link_status_prop_id, &link_status_value,
> +                             &link_status_prop);
> +
> +        if (link_status_value != DRM_MODE_LINK_STATUS_BAD) {
> +                igt_info("Expected link-status=BAD but got %" PRIu64 "\n",
> +                         link_status_value);
> +                return false;
> +        }
> +
> +        return true;
> +}
> +
> +/*
> + * Sets link status=GOOD for the specified outputs, then calls
> + * validate_modeset_for_outputs() to re-commit. Returns false
> + * if the re-commit fails.
> + */
> +static bool fix_link_status_and_recommit(data_t *data,
> +                                         igt_output_t *outputs[],
> +                                         int *output_count,
> +                                         drmModeModeInfo * modes[],
> +                                         struct igt_fb fbs[],
> +                                         struct igt_plane *primarys[])

This should be changed to `primaries` or `primary_planes`, perhaps in a 
separate patch.


> +{
> +        int i;
> +        igt_output_t *out;
> +
> +        /* Set link-status=GOOD on each tested output */
> +        for_each_connected_output(&data->display, out) {
> +                for (i = 0; i < *output_count; i++) {
> +                        if (out->id == outputs[i]->id) {
> +                                igt_output_set_prop_value(
> +                                        out, IGT_CONNECTOR_LINK_STATUS,
> +                                        DRM_MODE_LINK_STATUS_GOOD);
> +                        }
> +                }
> +        }
> +
> +        if (!validate_modeset_for_outputs(data, outputs, output_count,
> +                                          modes, fbs, primarys)) {
> +                igt_info("Modeset validation failed after forcing link-status=GOOD.\n");
> +                return false;
> +        }
> +
> +        if (igt_display_try_commit_atomic(&data->display,
> +                                          DRM_MODE_ATOMIC_ALLOW_MODESET,
> +                                          NULL) != 0) {
> +                igt_info("Commit failed after restoring link-status=GOOD.\n");
> +                return false;
> +        }
> +
> +        return true;
> +}
> +
>   static void test_fallback(data_t *data, bool is_mst)
>   {
>   	int output_count, retries;
>   	int max_link_rate, curr_link_rate, prev_link_rate;
>   	int max_lane_count, curr_lane_count, prev_lane_count;
>   	igt_output_t *outputs[IGT_MAX_PIPES];
> -	uint32_t link_status_prop_id;
> -	uint64_t link_status_value;
> -	drmModeModeInfo *modes[IGT_MAX_PIPES];
> -	drmModePropertyPtr link_status_prop;
> +	drmModeModeInfo * modes[IGT_MAX_PIPES];
>   	struct igt_fb fbs[IGT_MAX_PIPES];
>   	struct igt_plane *primarys[IGT_MAX_PIPES];
>   	struct udev_monitor *mon;
> @@ -274,19 +382,16 @@ static void test_fallback(data_t *data, bool is_mst)
>   			 prev_link_rate,
>   			 prev_lane_count);
>   		mon = igt_watch_uevents();
> -		igt_force_lt_failure(data->drm_fd, data->output,
> -				     LT_FAILURE_REDUCED_CAPS);
> -		igt_force_link_retrain(data->drm_fd, data->output,
> -				       RETRAIN_COUNT);
> -
> -		igt_assert_eq(check_condition_with_timeout(data->drm_fd,
> -							   data->output,
> -							   igt_get_dp_pending_retrain,
> -							   1.0, 20.0), 0);
> -		igt_assert_eq(check_condition_with_timeout(data->drm_fd,
> -							   data->output,
> -							   igt_get_dp_pending_lt_failures,
> -							   1.0, 20.0), 0);
> +
> +		/*
> +		 * Force link failure and retrain also wait for
> +		 * force_lf_failure/retrain status to be cleared
> +		 */

These comments can be removed, as we already have comments just above 
the function definition.


> +		igt_assert_f(force_failure_and_wait(data, data->output,
> +						    LT_FAILURE_REDUCED_CAPS,
> +						    RETRAIN_COUNT,
> +						    1.0, 20.0),
> +						    "Link training failure steps timed out\n");
>   
>   		if (igt_get_dp_link_retrain_disabled(data->drm_fd,
>   						     data->output)) {
> @@ -294,26 +399,21 @@ static void test_fallback(data_t *data, bool is_mst)
>   			return;
>   		}
>   
> -		igt_assert_f(igt_hotplug_detected(mon, 20),
> -			     "Didn't get hotplug for force link training failure\n");
> +		/* Wait for hotplug and check link status is BAD */

Same as above, this comment is not required.

With above things addressed, this is:

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>


> +		igt_assert_f(wait_for_hotplug_and_check_bad(data->drm_fd,
> +							    data,
> +							    data->output,
> +							    mon,
> +							    20.0), "Didn't get hotplug or link status != BAD\n");
>   
> -		kmstest_get_property(data->drm_fd,
> -				data->output->config.connector->connector_id,
> -				DRM_MODE_OBJECT_CONNECTOR, "link-status",
> -				&link_status_prop_id, &link_status_value,
> -				&link_status_prop);
> -		igt_assert_eq(link_status_value, DRM_MODE_LINK_STATUS_BAD);
>   		igt_flush_uevents(mon);
>   		set_connector_link_status_good(data, outputs, &output_count);
> -		igt_assert_f(validate_modeset_for_outputs(data,
> +		igt_assert_f(fix_link_status_and_recommit(data,
>   							  outputs,
>   							  &output_count,
>   							  modes,
>   							  fbs,
> -							  primarys),
> -			     "modeset failed\n");
> -		igt_display_commit2(&data->display, COMMIT_ATOMIC);
> -
> +							  primarys), "modeset failed\n");
>   		igt_assert_eq(data->output->values[IGT_CONNECTOR_LINK_STATUS], DRM_MODE_LINK_STATUS_GOOD);
>   		curr_link_rate = igt_get_current_link_rate(data->drm_fd, data->output);
>   		curr_lane_count = igt_get_current_lane_count(data->drm_fd, data->output);