[PATCH i-g-t] tests/intel/kms_frontbuffer_tracking: Invalidate cached stuff

[Gustavo Sousa]

Quoting Vodapalli, Ravi Kumar (2024-02-20 15:10:22-03:00)
>Hi Gustavo,

Hi!

>
>Below my inline comments
>
>On 2/20/2024 11:31 PM, Gustavo Sousa wrote:
>> +cc Ravi.
>>
>> Quoting Gustavo Sousa (2024-02-09 10:55:27-03:00)
>>> The test pipe-fbc-rte updates the primary mode parameters for each valid
>>> dynamic test case. Because of that, we might endup with invalid cached
>>> data due to differences between the initial state of prim_mode_params
>>> defined from the beginning of the test program and the possibly changed
>>> state after pipe-fbc-rte.
>>>
>>> As an example, in a specific environment, the command
>>>
>>>   ./build/tests/kms_frontbuffer_tracking \
>>>       --run-subtest pipe-fbc-rte,fbc-1p-primscrn-pri-indfb-draw-mmap-wc
>>>
>>> would result in fbc-1p-primscrn-pri-indfb-draw-mmap-wc failing because
>>> it would try to read CRC from pipe B while the test was being actually
>>> done in pipe A.
>>>
>>> Another potential issue worth noting is that even pipe-fbc-rte could
>>> similarly fail if the set of dynamic subtests spanned across multiple
>>> pipes.
>>>
>>> Let's fix that by making sure that cached stuff that would depend on the
>>> primary mode parameters gets properly invalidated when prim_mode_params
>>> is the target of init_mode_params(). This should fix the issues
>>> mentioned above and also future-proof the code for any future test that
>>> would need to modify the prim_mode_params.
>>>
>>> Signed-off-by: Gustavo Sousa <gustavo.sousa at intel.com>
>>> ---
>>> tests/intel/kms_frontbuffer_tracking.c | 13 +++++++++++++
>>> 1 file changed, 13 insertions(+)
>>>
>>> diff --git a/tests/intel/kms_frontbuffer_tracking.c b/tests/intel/kms_frontbuffer_tracking.c
>>> index 912cca3f8d45..17f74990979e 100644
>>> --- a/tests/intel/kms_frontbuffer_tracking.c
>>> +++ b/tests/intel/kms_frontbuffer_tracking.c
>>> @@ -1484,6 +1484,7 @@ static drmModeModeInfo *connector_get_mode(igt_output_t *output)
>>> static void init_mode_params(struct modeset_params *params,
>>>                               igt_output_t *output, enum pipe pipe)
>>> {
>>> +        int i;
>can you rename variable i to cnt or any meaning full name to that the 
>variable is used

IMO, cnt isn't very meaningful to what the variable is actually doing.
The variable i is simply being used to iterate over an array. As such, I
think the current variable name is suitable and conforms to a pattern
that is already being widely used on the code base:

    $ git grep '\<for\s*(\s*i\s*=\s*[^;]\+;\s*i\s*<\s*[^;]\+;\s*i++)' | wc -l
    1535

--
Gustavo Sousa

>>>          drmModeModeInfo *mode;
>>>
>>>          igt_output_override_mode(output, NULL);
>>> @@ -1515,6 +1516,18 @@ static void init_mode_params(struct modeset_params *params,
>>>          params->sprite.w = 64;
>>>          params->sprite.h = 64;
>>>
>>> +        /* If we endup changing the primary mode parameters, we need to
>>> +         * invalidate any existing cached stuff from a previous configuration. */
>>> +        if (params == &prim_mode_params) {
>>> +                if (pipe_crc) {
>>> +                        igt_pipe_crc_free(pipe_crc);
>>> +                        pipe_crc = NULL;
>>> +                }
>>> +
>>> +                for (i = 0; i < FORMAT_COUNT; i++)
>>> +                        blue_crcs[i].initialized = false;
>>> +        }
>>> +
>>>          free(mode);
>>> }
>>> -- 
>>> 2.43.0
>>>
>Thanks,
>Ravi kumar V