Re: [igt-dev] [i-g-t 5/5] tests/intel/kms_dirtyfb: Restructure dynamic subtests

["Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>]

Hi Jouni,

On Wed-15-11-2023 03:49 pm, Hogander, Jouni wrote:
> On Wed, 2023-11-15 at 12:33 +0530, Bhanuprakash Modem wrote:
>> Instead of having all features under single subtest, create
>> a subtest for each feature.
>>
>> Before:
>> igt@kms_dirtyfb@dirtyfb-ioctl@psr-eDP-1
>> igt@kms_dirtyfb@dirtyfb-ioctl@fbc-eDP-1
>> igt@kms_dirtyfb@dirtyfb-ioctl@drrs-eDP-1
>>
>> After:
>> igt@kms_dirtyfb@psr-dirtyfb-ioctl@pipe-A-eDP-1
>> igt@kms_dirtyfb@fbc-dirtyfb-ioctl@pipe-A-eDP-1
>> igt@kms_dirtyfb@drrs-dirtyfb-ioctl@pipe-A-eDP-1
> 
> Obviously you have some specific reason for this change:
> 
> Reviewed-by: Jouni Högander <jouni.hogander@intel.com>
> 
> Just for curiosity, what is the problem with the original approach?

There is no problem with previous approach, but this patch helps in 
below areas.

1. Better reporting: As features are at dynamic subtest level, single 
dynamic subtest failure treats entire subtest as fail. With this change, 
we can get the feature level pass/fail rate.
2. Execution time: Earlier time taken to complete one subtest is (F*C*T) 
secs, Now, it'll reduce to (C*T) secs which will help to prevent CI 
timeouts.

Where F is number of features, C is number of connectors, and T is time 
to complete one dynamic subtest.

- Bhanu

> 
> BR,
> 
> Jouni Högander
>>
>> Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> Cc: Jouni Högander <jouni.hogander@intel.com>
>> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
>> ---
>>   tests/intel/kms_dirtyfb.c | 67 +++++++++++++++++++++++++++----------
>> --
>>   1 file changed, 46 insertions(+), 21 deletions(-)
>>
>> diff --git a/tests/intel/kms_dirtyfb.c b/tests/intel/kms_dirtyfb.c
>> index a0f61b7a3..5ab2dc59f 100644
>> --- a/tests/intel/kms_dirtyfb.c
>> +++ b/tests/intel/kms_dirtyfb.c
>> @@ -20,15 +20,26 @@ IGT_TEST_DESCRIPTION("Test the DIRTYFB ioctl is
>> working properly with "
>>    * TEST: kms dirtyfb
>>    * Category: Display
>>    * Description: Test DIRTYFB ioctl functionality.
>> + * Driver requirement: i915, xe
>> + * Functionality: dirtyfb
>> + * Mega feature: General Display Features
>> + * Test category: functionality test
>>    *
>> - * SUBTEST: dirtyfb-ioctl
>> + * SUBTEST: default-dirtyfb-ioctl
>>    * Description: Test DIRTYFB ioctl is working properly using GPU
>>    *              frontbuffer rendering with features like FBC, PSR
>>    *              and DRRS.
>> - * Driver requirement: i915, xe
>> - * Functionality: dirtyfb, fbc, psr, drrs
>> - * Mega feature: General Display Features
>> - * Test category: functionality test
>> + *
>> + * SUBTEST: %s-dirtyfb-ioctl
>> + * Description: Test DIRTYFB ioctl is working properly using GPU
>> + *              frontbuffer rendering with %arg[1] feature.
>> + * Functionality: dirtyfb, %arg[1]
>> + *
>> + * arg[1]:
>> + *
>> + * @drrs:    drrs
>> + * @fbc:     fbc
>> + * @psr:     psr1
>>    */
>>   
>>   #ifndef PAGE_ALIGN
>> @@ -160,10 +171,6 @@ static void prepare(data_t *data)
>>   {
>>          igt_plane_t *primary;
>>   
>> -       igt_skip_on(!check_support(data));
>> -
>> -       igt_display_reset(&data->display);
>> -
>>          data->mode = igt_output_get_mode(data->output);
>>   
>>          igt_output_set_pipe(data->output, data->pipe);
>> @@ -297,19 +304,37 @@ igt_main
>>                  igt_display_reset(&data.display);
>>          }
>>   
>> -       igt_describe("Test dirtyFB ioctl");
>> -       igt_subtest_with_dynamic("dirtyfb-ioctl") {
>> -               data.pipe = PIPE_A;
>> -               for_each_valid_output_on_pipe(&data.display,
>> data.pipe,
>> -                                             data.output) {
>> -                       for (data.feature = FEATURE_DEFAULT;
>> data.feature > 0;
>> -                            data.feature = data.feature >> 1) {
>> -                               igt_dynamic_f("%s-%s",
>> feature_str(data.feature),
>> -
>> igt_output_name(data.output)) {
>> -                                       prepare(&data);
>> -                                       run_test(&data);
>> -                                       cleanup(&data);
>> +       for (data.feature = FEATURE_DEFAULT; data.feature > 0;
>> +            data.feature = data.feature >> 1) {
>> +               igt_describe_f("Test dirtyFB ioctl with %s",
>> feature_str(data.feature));
>> +               igt_subtest_with_dynamic_f("%s-dirtyfb-ioctl",
>> feature_str(data.feature)) {
>> +                       for_each_pipe(&data.display, data.pipe) {
>> +                               int valid_tests = 0;
>> +
>> +                               for_each_valid_output_on_pipe(&data.d
>> isplay,
>> +
>> data.pipe,
>> +
>> data.output) {
>> +                                       if (!check_support(&data))
>> +                                               continue;
>> +
>> +                                       igt_display_reset(&data.displ
>> ay);
>> +                                       igt_output_set_pipe(data.outp
>> ut, data.pipe);
>> +                                       if
>> (!intel_pipe_output_combo_valid(&data.display))
>> +                                               continue;
>> +
>> +                                       valid_tests++;
>> +                                       igt_dynamic_f("%s-%s",
>> +
>> kmstest_pipe_name(data.pipe),
>> +
>> igt_output_name(data.output)) {
>> +                                               prepare(&data);
>> +                                               run_test(&data);
>> +                                               cleanup(&data);
>> +                                       }
>>                                  }
>> +
>> +                               /* One pipe is enough. */
>> +                               if (valid_tests)
>> +                                       break;
>>                          }
>>                  }
>>          }
>