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

[Matt Roper]

On Wed, Feb 28, 2024 at 12:05:48AM +0530, Vodapalli, Ravi Kumar wrote:
> Hi,
> 
> Reviewed-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli at intel.com>

Applied.  Thanks for the patch and review.


Matt

> 
> 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 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
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation