[igt,v2] lib: Update intel_require_memory to handle +4GB cases
diff mbox

Message ID 1435153231-19606-1-git-send-email-michel.thierry@intel.com
State New
Headers show

Commit Message

Michel Thierry June 24, 2015, 1:40 p.m. UTC
Changed size from u32 to u64 to support +4GB.
48-bit PPGTT test cases may need extra memory available.

v2: Use thousands separator (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 lib/igt_aux.h  | 2 +-
 lib/intel_os.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Chris Wilson June 24, 2015, 3:38 p.m. UTC | #1
On Wed, Jun 24, 2015 at 02:40:31PM +0100, Michel Thierry wrote:
> Changed size from u32 to u64 to support +4GB.
> 48-bit PPGTT test cases may need extra memory available.
> 
> v2: Use thousands separator (Chris)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

One last thing, whilst you're in this function, mind moving the
igt_skip_on_simulation() to the start?

intel_require_memory()
{
  /* We use intel_require_memory() to detect tests that are designed to
   * with large working sets to stress boundaries such as aperture, and/or
   * memory exhaustion. Functional tests that also require large working
   * sets are split into two, a small subtest to verify the operation with the
   * absolute minimum working set, and the full subtest to verify the
   * interesting corner cases. The former test doesn't require the
   * memory check and so judicious use of intel_require_memory() helps
   * segregate such functional tests from the broader tests, useful for
   * slow verification systems such as the simulator.
   *
   * To recap, lay out behaviour tests like:
   *    igt_subtest("small") {
   *       run_test({.num_surfaces = 2 });
   *    }
   *    igt_subtest("full") {
   *      intel_require_memory(NUM_SURFACES, SURFACE_SIZE, CHECK_RAM);
   *      run_test({.num_surfaces = NUM_SURFACES});
   *    }
   * so that we have a simple check that is run anywhere and everywhere,
   * useful to prove the test itself works as expected, and the full
   * slow check that needs to be run on real hardware.
   */
-Chris
Daniel Vetter June 25, 2015, 7:46 a.m. UTC | #2
On Wed, Jun 24, 2015 at 04:38:55PM +0100, Chris Wilson wrote:
> On Wed, Jun 24, 2015 at 02:40:31PM +0100, Michel Thierry wrote:
> > Changed size from u32 to u64 to support +4GB.
> > 48-bit PPGTT test cases may need extra memory available.
> > 
> > v2: Use thousands separator (Chris)
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> One last thing, whilst you're in this function, mind moving the
> igt_skip_on_simulation() to the start?
> 
> intel_require_memory()
> {
>   /* We use intel_require_memory() to detect tests that are designed to
>    * with large working sets to stress boundaries such as aperture, and/or
>    * memory exhaustion. Functional tests that also require large working
>    * sets are split into two, a small subtest to verify the operation with the
>    * absolute minimum working set, and the full subtest to verify the
>    * interesting corner cases. The former test doesn't require the
>    * memory check and so judicious use of intel_require_memory() helps
>    * segregate such functional tests from the broader tests, useful for
>    * slow verification systems such as the simulator.
>    *
>    * To recap, lay out behaviour tests like:
>    *    igt_subtest("small") {
>    *       run_test({.num_surfaces = 2 });
>    *    }
>    *    igt_subtest("full") {
>    *      intel_require_memory(NUM_SURFACES, SURFACE_SIZE, CHECK_RAM);
>    *      run_test({.num_surfaces = NUM_SURFACES});
>    *    }
>    * so that we have a simple check that is run anywhere and everywhere,
>    * useful to prove the test itself works as expected, and the full
>    * slow check that needs to be run on real hardware.
>    */

This is an excellent usage hint, care to add it to the gtkdoc section? Not
sure whether the code quote needs any special treatment. But probably good
to insert a suitable # heading above it, so that this example usage
discussion stands out from the more general function description.

Thanks, Daniel

Patch
diff mbox

diff --git a/lib/igt_aux.h b/lib/igt_aux.h
index 9ea50de..139e5da 100644
--- a/lib/igt_aux.h
+++ b/lib/igt_aux.h
@@ -86,7 +86,7 @@  uint64_t intel_get_avail_ram_mb(void);
 uint64_t intel_get_total_ram_mb(void);
 uint64_t intel_get_total_swap_mb(void);
 
-void intel_require_memory(uint32_t count, uint32_t size, unsigned mode);
+void intel_require_memory(uint32_t count, uint64_t size, unsigned mode);
 #define CHECK_RAM 0x1
 #define CHECK_SWAP 0x2
 
diff --git a/lib/intel_os.c b/lib/intel_os.c
index f82847f..984bb6a 100644
--- a/lib/intel_os.c
+++ b/lib/intel_os.c
@@ -215,7 +215,7 @@  intel_get_total_swap_mb(void)
  * assumption that any test that needs to check for memory requirements is a
  * thrashing test unsuitable for slow simulated systems.
  */
-void intel_require_memory(uint32_t count, uint32_t size, unsigned mode)
+void intel_require_memory(uint32_t count, uint64_t size, unsigned mode)
 {
 /* rough estimate of how many bytes the kernel requires to track each object */
 #define KERNEL_BO_OVERHEAD 512
@@ -225,8 +225,8 @@  void intel_require_memory(uint32_t count, uint32_t size, unsigned mode)
 	required *= size + KERNEL_BO_OVERHEAD;
 	required = ALIGN(required, 4096);
 
-	igt_debug("Checking %u surfaces of size %u bytes (total %'llu) against %s%s\n",
-		  count, size, (long long)required,
+	igt_debug("Checking %u surfaces of size %'llu bytes (total %'llu) against %s%s\n",
+		  count, (long long)size, (long long)required,
 		  mode & (CHECK_RAM | CHECK_SWAP) ? "RAM" : "",
 		  mode & CHECK_SWAP ? " + swap": "");