[igt-dev] [PATCH 1/2] tests/i915/gem_render_tiled_blits : Added subtests description

[Kumar, Janga Rahul]

> -----Original Message-----
> From: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> Sent: 31 May 2022 00:09
> To: igt-dev at lists.freedesktop.org
> Cc: Gandi, Ramadevi <ramadevi.gandi at intel.com>; Ch, Sai Gowtham
> <sai.gowtham.ch at intel.com>; Dandamudi, Priyanka
> <priyanka.dandamudi at intel.com>; Kumar, Janga Rahul
> <janga.rahul.kumar at intel.com>
> Subject: Re: [PATCH 1/2] tests/i915/gem_render_tiled_blits : Added subtests
> description
> 
> Hi Janga,
> 
> On 2022-05-30 at 12:10:11 +0530, janga.rahul.kumar at intel.com wrote:
> > From: "Kumar, Janga Rahul" <janga.rahul.kumar at intel.com>
> >
> > Added test description to all the available subtests and Updated file
> > name in the file description comments.
> - ^
> s/Updated/fix/
> 
> Put description here what did you changed from previous version, see other
> patch series with v2 (or larger version).
> 
> >
> > Signed-off-by: Kumar, Janga Rahul <janga.rahul.kumar at intel.com>
> ---------------- ^
> 
> Please re-arrange you name in s-o-b to:
> 
> Janga Rahul Kumar <janga.rahul.kumar at intel.com>
> 
> Please add all addresses used for cc or to here (except this mailinglist), for
> example see patch 1/2 in this series:
> 
> HAX add description to gem_exec_basic
> https://patchwork.freedesktop.org/series/103196/
> 
> > ---
> >  tests/i915/gem_render_tiled_blits.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/i915/gem_render_tiled_blits.c
> > b/tests/i915/gem_render_tiled_blits.c
> > index 187714d6..61ae9a80 100644
> > --- a/tests/i915/gem_render_tiled_blits.c
> > +++ b/tests/i915/gem_render_tiled_blits.c
> > @@ -25,7 +25,7 @@
> >   *
> >   */
> >
> > -/** @file gem_linear_render_blits.c
> > +/** @file gem_render_tiled_blits.c
> 
> Nice catch.
> 
> >   *
> >   * This is a test of doing many blits, with a working set
> >   * larger than the aperture size.
> > @@ -49,6 +49,10 @@
> >  #include "i915/gem.h"
> >  #include "igt.h"
> >
> > +IGT_TEST_DESCRIPTION("Tests performs Cyclic forward, backward and
> random blits on tiled buffer "
> 
> s/Cyclic/cyclic/
> 
> > +		      "objects using render engine with various working set sizes
> and compares "
> > +		      "the blits outputs with expected outputs. ");
> -------------------------------------------------------------- ^
> 
> Remove spaces at strings end.
> 
> You can also shorten this a little, like:
> s/the blits outputs with expected outputs. /outputs with expected ones./
> 
> or even
> s/compares the blits outputs with expected outputs. /verify outputs./
> 
> Your choice (you can also rewrite it to some other form).
> 
> > +
> >  #define WIDTH 512
> >  #define STRIDE (WIDTH*4)
> >  #define HEIGHT 512
> > @@ -205,16 +209,20 @@ igt_main
> >  		igt_require(gem_available_fences(fd) > 0);
> >  	}
> >
> > +	igt_describe("Check with working set size 2.");
> ------------------------------------------------- ^ This number here do not describe
> anything, maybe just write "Run basic check." or "Verify basic functionality." or
> something like that.
> 
> >  	igt_subtest("basic") {
> >  		run_test(fd, 2);
> >  	}
> >
> > +	igt_describe("Check with working set size larger than aperture
> > +size.");
> >  	igt_subtest("aperture-thrash") {
> >  		count = 3 * gem_aperture_size(fd) / SIZE / 2;
> >  		intel_require_memory(count, SIZE, CHECK_RAM);
> >  		run_test(fd, count);
> >  	}
> >
> > +	igt_describe("Check with working set size larger than aperture size and "
> > +		     "helper process to shrink the aperture.");
> 
> I am not sure that this shrinks aperture, what I see there is drop caches. For
> more info see commit e8eb9afd4c5 with git log -p e8eb9afd4c5
[Janga Rahul Kumar] yes, it doesn’t shrink the aperture. It shrinks the buffer object caches and checks whether the required amount of memory can be met after dropping caches.
 
> 
> >  	igt_subtest("aperture-shrink") {
> >  		igt_fork_shrink_helper(fd);
> >
> > @@ -225,6 +233,7 @@ igt_main
> >  		igt_stop_shrink_helper();
> >  	}
> >
> > +	igt_describe("Check with working set size larger than swap memory
> > +size.");
> 
> Do we want cause oom here ? imho test wants to use up all memory and so
> system will try to use swap. This is in following line:
[Janga Rahul Kumar] Agree
> 
> 		count = ((intel_get_avail_ram_mb() + (swap_mb / 2)) *
> 1024*1024) / SIZE;
> 
> --
> Kamil
> 
> >  	igt_subtest("swap-thrash") {
> >  		uint64_t swap_mb = intel_get_total_swap_mb();
> >  		igt_require(swap_mb > 0);
> > --
> > 2.25.1
> >