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

["Vodapalli, Ravi Kumar" <ravi.kumar.vodapalli@intel.com>]

Hi,

Reviewed-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com>

On 2/21/2024 6:06 PM, Gustavo Sousa wrote:
> 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@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