[Intel-gfx] [PATCH i-g-t] lib: Use unsigned gen for forward compatible tests

[Intel-gfx] [PATCH i-g-t] lib: Use unsigned gen for forward compatible tests

[Chris Wilson]

Unknown, so future, gen are marked as -1 which we want to treat as -1u
so that always pass >= gen checks.

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2298
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
---
 lib/intel_batchbuffer.c | 10 +++++-----
 lib/intel_batchbuffer.h | 10 ++++++----
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
index 99c2e1489..375f11962 100644
--- a/lib/intel_batchbuffer.c
+++ b/lib/intel_batchbuffer.c
@@ -412,7 +412,7 @@ intel_blt_copy(struct intel_batchbuffer *batch,
 	       drm_intel_bo *dst_bo, int dst_x1, int dst_y1, int dst_pitch,
 	       int width, int height, int bpp)
 {
-	const int gen = batch->gen;
+	const unsigned int gen = batch->gen;
 	uint32_t src_tiling, dst_tiling, swizzle;
 	uint32_t cmd_bits = 0;
 	uint32_t br13_bits;
@@ -551,7 +551,7 @@ unsigned igt_buf_height(const struct igt_buf *buf)
  * Returns:
  * The width of the ccs buffer data.
  */
-unsigned int igt_buf_intel_ccs_width(int gen, const struct igt_buf *buf)
+unsigned int igt_buf_intel_ccs_width(unsigned int gen, const struct igt_buf *buf)
 {
 	/*
 	 * GEN12+: The CCS unit size is 64 bytes mapping 4 main surface
@@ -574,7 +574,7 @@ unsigned int igt_buf_intel_ccs_width(int gen, const struct igt_buf *buf)
  * Returns:
  * The height of the ccs buffer data.
  */
-unsigned int igt_buf_intel_ccs_height(int gen, const struct igt_buf *buf)
+unsigned int igt_buf_intel_ccs_height(unsigned int gen, const struct igt_buf *buf)
 {
 	/*
 	 * GEN12+: The CCS unit size is 64 bytes mapping 4 main surface
@@ -701,7 +701,7 @@ fill_object(struct drm_i915_gem_exec_object2 *obj, uint32_t gem_handle,
 
 static void exec_blit(int fd,
 		      struct drm_i915_gem_exec_object2 *objs, uint32_t count,
-		      int gen)
+		      unsigned int gen)
 {
 	struct drm_i915_gem_execbuffer2 exec = {
 		.buffers_ptr = to_user_pointer(objs),
@@ -2024,7 +2024,7 @@ void intel_bb_emit_blt_copy(struct intel_bb *ibb,
 			    int dst_x1, int dst_y1, int dst_pitch,
 			    int width, int height, int bpp)
 {
-	const int gen = ibb->gen;
+	const unsigned int gen = ibb->gen;
 	uint32_t cmd_bits = 0;
 	uint32_t br13_bits;
 	uint32_t mask;
diff --git a/lib/intel_batchbuffer.h b/lib/intel_batchbuffer.h
index a69fffd8d..cfe743edc 100644
--- a/lib/intel_batchbuffer.h
+++ b/lib/intel_batchbuffer.h
@@ -15,7 +15,7 @@
 struct intel_batchbuffer {
 	drm_intel_bufmgr *bufmgr;
 	uint32_t devid;
-	int gen;
+	unsigned int gen;
 
 	drm_intel_context *ctx;
 	drm_intel_bo *bo;
@@ -263,8 +263,10 @@ static inline bool igt_buf_compressed(const struct igt_buf *buf)
 
 unsigned igt_buf_width(const struct igt_buf *buf);
 unsigned igt_buf_height(const struct igt_buf *buf);
-unsigned int igt_buf_intel_ccs_width(int gen, const struct igt_buf *buf);
-unsigned int igt_buf_intel_ccs_height(int gen, const struct igt_buf *buf);
+unsigned int igt_buf_intel_ccs_width(unsigned int gen,
+				     const struct igt_buf *buf);
+unsigned int igt_buf_intel_ccs_height(unsigned int gen,
+				      const struct igt_buf *buf);
 
 void igt_blitter_src_copy(int fd,
 			  /* src */
@@ -431,7 +433,7 @@ igt_media_spinfunc_t igt_get_media_spinfunc(int devid);
  */
 struct intel_bb {
 	int i915;
-	int gen;
+	unsigned int gen;
 	bool debug;
 	bool enforce_relocs;
 	uint32_t devid;
-- 
2.28.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t] lib: Use unsigned gen for forward compatible tests

[Zbigniew Kempczyński]

On Thu, Aug 06, 2020 at 03:45:29PM +0100, Chris Wilson wrote:
> Unknown, so future, gen are marked as -1 which we want to treat as -1u
> so that always pass >= gen checks.

Do we really want to enable the tests when platform is not fully
enabled in IGT?

--
Zbigniew

> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2298
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> ---
>  lib/intel_batchbuffer.c | 10 +++++-----
>  lib/intel_batchbuffer.h | 10 ++++++----
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
> index 99c2e1489..375f11962 100644
> --- a/lib/intel_batchbuffer.c
> +++ b/lib/intel_batchbuffer.c
> @@ -412,7 +412,7 @@ intel_blt_copy(struct intel_batchbuffer *batch,
>  	       drm_intel_bo *dst_bo, int dst_x1, int dst_y1, int dst_pitch,
>  	       int width, int height, int bpp)
>  {
> -	const int gen = batch->gen;
> +	const unsigned int gen = batch->gen;
>  	uint32_t src_tiling, dst_tiling, swizzle;
>  	uint32_t cmd_bits = 0;
>  	uint32_t br13_bits;
> @@ -551,7 +551,7 @@ unsigned igt_buf_height(const struct igt_buf *buf)
>   * Returns:
>   * The width of the ccs buffer data.
>   */
> -unsigned int igt_buf_intel_ccs_width(int gen, const struct igt_buf *buf)
> +unsigned int igt_buf_intel_ccs_width(unsigned int gen, const struct igt_buf *buf)
>  {
>  	/*
>  	 * GEN12+: The CCS unit size is 64 bytes mapping 4 main surface
> @@ -574,7 +574,7 @@ unsigned int igt_buf_intel_ccs_width(int gen, const struct igt_buf *buf)
>   * Returns:
>   * The height of the ccs buffer data.
>   */
> -unsigned int igt_buf_intel_ccs_height(int gen, const struct igt_buf *buf)
> +unsigned int igt_buf_intel_ccs_height(unsigned int gen, const struct igt_buf *buf)
>  {
>  	/*
>  	 * GEN12+: The CCS unit size is 64 bytes mapping 4 main surface
> @@ -701,7 +701,7 @@ fill_object(struct drm_i915_gem_exec_object2 *obj, uint32_t gem_handle,
>  
>  static void exec_blit(int fd,
>  		      struct drm_i915_gem_exec_object2 *objs, uint32_t count,
> -		      int gen)
> +		      unsigned int gen)
>  {
>  	struct drm_i915_gem_execbuffer2 exec = {
>  		.buffers_ptr = to_user_pointer(objs),
> @@ -2024,7 +2024,7 @@ void intel_bb_emit_blt_copy(struct intel_bb *ibb,
>  			    int dst_x1, int dst_y1, int dst_pitch,
>  			    int width, int height, int bpp)
>  {
> -	const int gen = ibb->gen;
> +	const unsigned int gen = ibb->gen;
>  	uint32_t cmd_bits = 0;
>  	uint32_t br13_bits;
>  	uint32_t mask;
> diff --git a/lib/intel_batchbuffer.h b/lib/intel_batchbuffer.h
> index a69fffd8d..cfe743edc 100644
> --- a/lib/intel_batchbuffer.h
> +++ b/lib/intel_batchbuffer.h
> @@ -15,7 +15,7 @@
>  struct intel_batchbuffer {
>  	drm_intel_bufmgr *bufmgr;
>  	uint32_t devid;
> -	int gen;
> +	unsigned int gen;
>  
>  	drm_intel_context *ctx;
>  	drm_intel_bo *bo;
> @@ -263,8 +263,10 @@ static inline bool igt_buf_compressed(const struct igt_buf *buf)
>  
>  unsigned igt_buf_width(const struct igt_buf *buf);
>  unsigned igt_buf_height(const struct igt_buf *buf);
> -unsigned int igt_buf_intel_ccs_width(int gen, const struct igt_buf *buf);
> -unsigned int igt_buf_intel_ccs_height(int gen, const struct igt_buf *buf);
> +unsigned int igt_buf_intel_ccs_width(unsigned int gen,
> +				     const struct igt_buf *buf);
> +unsigned int igt_buf_intel_ccs_height(unsigned int gen,
> +				      const struct igt_buf *buf);
>  
>  void igt_blitter_src_copy(int fd,
>  			  /* src */
> @@ -431,7 +433,7 @@ igt_media_spinfunc_t igt_get_media_spinfunc(int devid);
>   */
>  struct intel_bb {
>  	int i915;
> -	int gen;
> +	unsigned int gen;
>  	bool debug;
>  	bool enforce_relocs;
>  	uint32_t devid;
> -- 
> 2.28.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] lib: Use unsigned gen for forward compatible tests

[Petri Latvala]

On Mon, Aug 10, 2020 at 10:09:46AM +0200, Zbigniew Kempczyński wrote:
> On Thu, Aug 06, 2020 at 03:45:29PM +0100, Chris Wilson wrote:
> > Unknown, so future, gen are marked as -1 which we want to treat as -1u
> > so that always pass >= gen checks.
> 
> Do we really want to enable the tests when platform is not fully
> enabled in IGT?

What does "fully enabled" mean?

If the test is checking for just "gen > x", the test should work
already. If the test is also checking for "gen < y" then we get a
spurious failure, but either way CI is going to tell you that
something is not passing. Without this it will be a skip, along with
skipping in the case that should just work already without actual test
changes.


-- 
Petri Latvala
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] lib: Use unsigned gen for forward compatible tests

[Chris Wilson]

Quoting Petri Latvala (2020-08-10 09:22:42)
> On Mon, Aug 10, 2020 at 10:09:46AM +0200, Zbigniew Kempczyński wrote:
> > On Thu, Aug 06, 2020 at 03:45:29PM +0100, Chris Wilson wrote:
> > > Unknown, so future, gen are marked as -1 which we want to treat as -1u
> > > so that always pass >= gen checks.
> > 
> > Do we really want to enable the tests when platform is not fully
> > enabled in IGT?
> 
> What does "fully enabled" mean?
> 
> If the test is checking for just "gen > x", the test should work
> already. If the test is also checking for "gen < y" then we get a
> spurious failure, but either way CI is going to tell you that
> something is not passing. Without this it will be a skip, along with
> skipping in the case that should just work already without actual test
> changes.

And for a very large fraction of tests, the behaviour of next gen
follows current gen. Those that do not are very interesting, and will be
a much smaller number than the volume of skips we have to process.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] lib: Use unsigned gen for forward compatible tests

[Zbigniew Kempczyński]

On Mon, Aug 10, 2020 at 09:56:34AM +0100, Chris Wilson wrote:
> Quoting Petri Latvala (2020-08-10 09:22:42)
> > On Mon, Aug 10, 2020 at 10:09:46AM +0200, Zbigniew Kempczyński wrote:
> > > On Thu, Aug 06, 2020 at 03:45:29PM +0100, Chris Wilson wrote:
> > > > Unknown, so future, gen are marked as -1 which we want to treat as -1u
> > > > so that always pass >= gen checks.
> > > 
> > > Do we really want to enable the tests when platform is not fully
> > > enabled in IGT?
> > 
> > What does "fully enabled" mean?
> > 
> > If the test is checking for just "gen > x", the test should work
> > already. If the test is also checking for "gen < y" then we get a
> > spurious failure, but either way CI is going to tell you that
> > something is not passing. Without this it will be a skip, along with
> > skipping in the case that should just work already without actual test
> > changes.
> 
> And for a very large fraction of tests, the behaviour of next gen
> follows current gen. Those that do not are very interesting, and will be
> a much smaller number than the volume of skips we have to process.
> -Chris

So patch only defers in time adding explicit platform definition.
For each tests which requires exact gen test will fail anyway, but
other will likely work. If they won't we'll know it from CI.

I see no big risk but let Petri decide, from my side:

Acked-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx