Re: [igt-dev] [PATCH i-g-t 2/3] lib/rendercopy: Implement support for 8/16 bpp

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 16, 2018 at 04:41:48PM +0100, Maarten Lankhorst wrote:
> To handle drawing 16 bpp formats correctly with odd x/w, we need to
> use the correct bpp to rendercopy. Now that everything sets bpp in
> igt_buf, fix the rendercopy support to use it and set the correct
> format.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  lib/rendercopy_gen4.c | 15 +++++++++------
>  lib/rendercopy_gen6.c | 16 ++++++++++------
>  lib/rendercopy_gen7.c | 16 ++++++++++------
>  lib/rendercopy_gen8.c | 18 +++++++++---------
>  lib/rendercopy_gen9.c | 15 +++++++++------
>  lib/rendercopy_i830.c | 20 ++++++++++++++++++--
>  lib/rendercopy_i915.c | 23 +++++++++++++++++++----
>  7 files changed, 84 insertions(+), 39 deletions(-)
> 
> diff --git a/lib/rendercopy_gen4.c b/lib/rendercopy_gen4.c
> index 0416b8d6d506..f8816edc5455 100644
> --- a/lib/rendercopy_gen4.c
> +++ b/lib/rendercopy_gen4.c
> @@ -136,7 +136,7 @@ gen4_render_flush(struct intel_batchbuffer *batch,
>  static uint32_t
>  gen4_bind_buf(struct intel_batchbuffer *batch,
>  	      const struct igt_buf *buf,
> -	      uint32_t format, int is_dst)
> +	      int is_dst)
>  {
>  	struct gen4_surface_state *ss;
>  	uint32_t write_domain, read_domain;
> @@ -152,7 +152,12 @@ gen4_bind_buf(struct intel_batchbuffer *batch,
>  	ss = intel_batchbuffer_subdata_alloc(batch, sizeof(*ss), 32);
>  
>  	ss->ss0.surface_type = SURFACE_2D;
> -	ss->ss0.surface_format = format;
> +	switch (buf->bpp) {
> +		case 8: ss->ss0.surface_format = SURFACEFORMAT_R8_UNORM; break;
> +		case 16: ss->ss0.surface_format = SURFACEFORMAT_R8G8_UNORM; break;
> +		case 32: ss->ss0.surface_format = SURFACEFORMAT_B8G8R8A8_UNORM; break;
> +		default: igt_assert(0);

Hmm. Maybe we actually want to specify a format rather than bpp? That
way we could convert between different sized pixel formats, and 
interpolation would also be possible so we could even scale the image.
But as not all formats could be supported so I guess a 1:1 copy is
a pretty good starting point at least. It's what we do with 32bpp
currently anyway.

Looks like all of these formats are supported by the sampler and
render target on all gens. So that makes this nice and simple.

> +	}
>  
>  	ss->ss0.data_return_format = SURFACERETURNFORMAT_FLOAT32;
>  	ss->ss0.color_blend = 1;
> @@ -182,10 +187,8 @@ gen4_bind_surfaces(struct intel_batchbuffer *batch,
>  
>  	binding_table = intel_batchbuffer_subdata_alloc(batch, 32, 32);
>  
> -	binding_table[0] =
> -		gen4_bind_buf(batch, dst, SURFACEFORMAT_B8G8R8A8_UNORM, 1);
> -	binding_table[1] =
> -		gen4_bind_buf(batch, src, SURFACEFORMAT_B8G8R8A8_UNORM, 0);
> +	binding_table[0] = gen4_bind_buf(batch, dst, 1);
> +	binding_table[1] = gen4_bind_buf(batch, src, 0);
>  
>  	return intel_batchbuffer_subdata_offset(batch, binding_table);
>  }
> diff --git a/lib/rendercopy_gen6.c b/lib/rendercopy_gen6.c
> index 87916927c7d7..297042868d1c 100644
> --- a/lib/rendercopy_gen6.c
> +++ b/lib/rendercopy_gen6.c
> @@ -73,7 +73,7 @@ gen6_render_flush(struct intel_batchbuffer *batch,
>  
>  static uint32_t
>  gen6_bind_buf(struct intel_batchbuffer *batch, const struct igt_buf *buf,
> -	      uint32_t format, int is_dst)
> +	      int is_dst)
>  {
>  	struct gen6_surface_state *ss;
>  	uint32_t write_domain, read_domain;
> @@ -88,7 +88,13 @@ gen6_bind_buf(struct intel_batchbuffer *batch, const struct igt_buf *buf,
>  
>  	ss = intel_batchbuffer_subdata_alloc(batch, sizeof(*ss), 32);
>  	ss->ss0.surface_type = SURFACE_2D;
> -	ss->ss0.surface_format = format;
> +
> +	switch (buf->bpp) {
> +		case 8: ss->ss0.surface_format = SURFACEFORMAT_R8_UNORM; break;
> +		case 16: ss->ss0.surface_format = SURFACEFORMAT_R8G8_UNORM; break;
> +		case 32: ss->ss0.surface_format = SURFACEFORMAT_B8G8R8A8_UNORM; break;
> +		default: igt_assert(0);
> +	}
>  
>  	ss->ss0.data_return_format = SURFACERETURNFORMAT_FLOAT32;
>  	ss->ss0.color_blend = 1;
> @@ -118,10 +124,8 @@ gen6_bind_surfaces(struct intel_batchbuffer *batch,
>  
>  	binding_table = intel_batchbuffer_subdata_alloc(batch, 32, 32);
>  
> -	binding_table[0] =
> -		gen6_bind_buf(batch, dst, SURFACEFORMAT_B8G8R8A8_UNORM, 1);
> -	binding_table[1] =
> -		gen6_bind_buf(batch, src, SURFACEFORMAT_B8G8R8A8_UNORM, 0);
> +	binding_table[0] = gen6_bind_buf(batch, dst, 1);
> +	binding_table[1] = gen6_bind_buf(batch, src, 0);
>  
>  	return intel_batchbuffer_subdata_offset(batch, binding_table);
>  }
> diff --git a/lib/rendercopy_gen7.c b/lib/rendercopy_gen7.c
> index 9ad619d8620f..799998588b59 100644
> --- a/lib/rendercopy_gen7.c
> +++ b/lib/rendercopy_gen7.c
> @@ -59,13 +59,19 @@ gen7_tiling_bits(uint32_t tiling)
>  static uint32_t
>  gen7_bind_buf(struct intel_batchbuffer *batch,
>  	      const struct igt_buf *buf,
> -	      uint32_t format,
>  	      int is_dst)
>  {
> -	uint32_t *ss;
> +	uint32_t format, *ss;
>  	uint32_t write_domain, read_domain;
>  	int ret;
>  
> +	switch (buf->bpp) {
> +		case 8: format = SURFACEFORMAT_R8_UNORM; break;
> +		case 16: format = SURFACEFORMAT_R8G8_UNORM; break;
> +		case 32: format = SURFACEFORMAT_B8G8R8A8_UNORM; break;
> +		default: igt_assert(0);
> +	}
> +
>  	if (is_dst) {
>  		write_domain = read_domain = I915_GEM_DOMAIN_RENDER;
>  	} else {
> @@ -186,10 +192,8 @@ gen7_bind_surfaces(struct intel_batchbuffer *batch,
>  
>  	binding_table = intel_batchbuffer_subdata_alloc(batch, 8, 32);
>  
> -	binding_table[0] =
> -		gen7_bind_buf(batch, dst, SURFACEFORMAT_B8G8R8A8_UNORM, 1);
> -	binding_table[1] =
> -		gen7_bind_buf(batch, src, SURFACEFORMAT_B8G8R8A8_UNORM, 0);
> +	binding_table[0] = gen7_bind_buf(batch, dst, 1);
> +	binding_table[1] = gen7_bind_buf(batch, src, 0);
>  
>  	return intel_batchbuffer_subdata_offset(batch, binding_table);
>  }
> diff --git a/lib/rendercopy_gen8.c b/lib/rendercopy_gen8.c
> index beb7a9b34987..8ec811633f09 100644
> --- a/lib/rendercopy_gen8.c
> +++ b/lib/rendercopy_gen8.c
> @@ -144,8 +144,7 @@ gen6_render_flush(struct intel_batchbuffer *batch,
>  static uint32_t
>  gen8_bind_buf(struct intel_batchbuffer *batch,
>  	      struct annotations_context *aub,
> -	      const struct igt_buf *buf,
> -	      uint32_t format, int is_dst)
> +	      const struct igt_buf *buf, int is_dst)
>  {
>  	struct gen8_surface_state *ss;
>  	uint32_t write_domain, read_domain, offset;
> @@ -163,7 +162,12 @@ gen8_bind_buf(struct intel_batchbuffer *batch,
>  	annotation_add_state(aub, AUB_TRACE_SURFACE_STATE, offset, sizeof(*ss));
>  
>  	ss->ss0.surface_type = SURFACE_2D;
> -	ss->ss0.surface_format = format;
> +	switch (buf->bpp) {
> +		case 8: ss->ss0.surface_format = SURFACEFORMAT_R8_UNORM; break;
> +		case 16: ss->ss0.surface_format = SURFACEFORMAT_R8G8_UNORM; break;
> +		case 32: ss->ss0.surface_format = SURFACEFORMAT_B8G8R8A8_UNORM; break;
> +		default: igt_assert(0);
> +	}
>  	ss->ss0.render_cache_read_write = 1;
>  	ss->ss0.vertical_alignment = 1; /* align 4 */
>  	ss->ss0.horizontal_alignment = 1; /* align 4 */
> @@ -205,12 +209,8 @@ gen8_bind_surfaces(struct intel_batchbuffer *batch,
>  	offset = intel_batchbuffer_subdata_offset(batch, binding_table);
>  	annotation_add_state(aub, AUB_TRACE_BINDING_TABLE, offset, 8);
>  
> -	binding_table[0] =
> -		gen8_bind_buf(batch, aub,
> -			      dst, SURFACEFORMAT_B8G8R8A8_UNORM, 1);
> -	binding_table[1] =
> -		gen8_bind_buf(batch, aub,
> -			      src, SURFACEFORMAT_B8G8R8A8_UNORM, 0);
> +	binding_table[0] = gen8_bind_buf(batch, aub, dst, 1);
> +	binding_table[1] = gen8_bind_buf(batch, aub, src, 0);
>  
>  	return offset;
>  }
> diff --git a/lib/rendercopy_gen9.c b/lib/rendercopy_gen9.c
> index adbd8124e082..6d719ce3ccc6 100644
> --- a/lib/rendercopy_gen9.c
> +++ b/lib/rendercopy_gen9.c
> @@ -175,7 +175,7 @@ gen6_render_flush(struct intel_batchbuffer *batch,
>  /* Mostly copy+paste from gen6, except height, width, pitch moved */
>  static uint32_t
>  gen8_bind_buf(struct intel_batchbuffer *batch, const struct igt_buf *buf,
> -	      uint32_t format, int is_dst) {
> +	      int is_dst) {
>  	struct gen8_surface_state *ss;
>  	uint32_t write_domain, read_domain, offset;
>  	int ret;
> @@ -193,7 +193,12 @@ gen8_bind_buf(struct intel_batchbuffer *batch, const struct igt_buf *buf,
>  			     offset, sizeof(*ss));
>  
>  	ss->ss0.surface_type = SURFACE_2D;
> -	ss->ss0.surface_format = format;
> +	switch (buf->bpp) {
> +		case 8: ss->ss0.surface_format = SURFACEFORMAT_R8_UNORM; break;
> +		case 16: ss->ss0.surface_format = SURFACEFORMAT_R8G8_UNORM; break;
> +		case 32: ss->ss0.surface_format = SURFACEFORMAT_B8G8R8A8_UNORM; break;
> +		default: igt_assert(0);
> +	}
>  	ss->ss0.render_cache_read_write = 1;
>  	ss->ss0.vertical_alignment = 1; /* align 4 */
>  	ss->ss0.horizontal_alignment = 1; /* align 4 */
> @@ -249,10 +254,8 @@ gen8_bind_surfaces(struct intel_batchbuffer *batch,
>  	annotation_add_state(&aub_annotations, AUB_TRACE_BINDING_TABLE,
>  			     offset, 8);
>  
> -	binding_table[0] =
> -		gen8_bind_buf(batch, dst, SURFACEFORMAT_B8G8R8A8_UNORM, 1);
> -	binding_table[1] =
> -		gen8_bind_buf(batch, src, SURFACEFORMAT_B8G8R8A8_UNORM, 0);
> +	binding_table[0] = gen8_bind_buf(batch, dst, 1);
> +	binding_table[1] = gen8_bind_buf(batch, src, 0);
>  
>  	return offset;
>  }
> diff --git a/lib/rendercopy_i830.c b/lib/rendercopy_i830.c
> index 2b07ad5d750a..a027da4da89d 100644
> --- a/lib/rendercopy_i830.c
> +++ b/lib/rendercopy_i830.c
> @@ -136,6 +136,14 @@ static void gen2_emit_target(struct intel_batchbuffer *batch,
>  			     const struct igt_buf *dst)
>  {
>  	uint32_t tiling;
> +	uint32_t format;
> +
> +	switch (dst->bpp) {
> +		case 8: format = COLR_BUF_8BIT; break;

This one writes the green channel...

> +		case 16: format = COLR_BUF_RGB565; break;
> +		case 32: format = COLR_BUF_ARGB8888; break;
> +		default: igt_assert(0);
> +	}
>  
>  	tiling = 0;
>  	if (dst->tiling != I915_TILING_NONE)
> @@ -148,7 +156,7 @@ static void gen2_emit_target(struct intel_batchbuffer *batch,
>  	OUT_RELOC(dst->bo, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
>  
>  	OUT_BATCH(_3DSTATE_DST_BUF_VARS_CMD);
> -	OUT_BATCH(COLR_BUF_ARGB8888 |
> +	OUT_BATCH(format |
>  		  DSTORG_HORT_BIAS(0x8) |
>  		  DSTORG_VERT_BIAS(0x8));
>  
> @@ -165,6 +173,14 @@ static void gen2_emit_texture(struct intel_batchbuffer *batch,
>  			      int unit)
>  {
>  	uint32_t tiling;
> +	uint32_t format;
> +
> +	switch (src->bpp) {
> +		case 8: format = MAPSURF_8BIT | MT_8BIT_L8; break;

.. and L8 replicates the same 8 bit value to RGB, so
looks like this should work just fine. Same for gen3.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		case 16: format = MAPSURF_16BIT | MT_16BIT_RGB565; break;
> +		case 32: format = MAPSURF_32BIT | MT_32BIT_ARGB8888; break;
> +		default: igt_assert(0);
> +	}
>  
>  	tiling = 0;
>  	if (src->tiling != I915_TILING_NONE)
> @@ -176,7 +192,7 @@ static void gen2_emit_texture(struct intel_batchbuffer *batch,
>  	OUT_RELOC(src->bo, I915_GEM_DOMAIN_SAMPLER, 0, 0);
>  	OUT_BATCH((igt_buf_height(src) - 1) << TM0S1_HEIGHT_SHIFT |
>  		  (igt_buf_width(src) - 1) << TM0S1_WIDTH_SHIFT |
> -		  MAPSURF_32BIT | MT_32BIT_ARGB8888 | tiling);
> +		  format | tiling);
>  	OUT_BATCH((src->stride / 4 - 1) << TM0S2_PITCH_SHIFT | TM0S2_MAP_2D);
>  	OUT_BATCH(FILTER_NEAREST << TM0S3_MAG_FILTER_SHIFT |
>  		  FILTER_NEAREST << TM0S3_MIN_FILTER_SHIFT |
> diff --git a/lib/rendercopy_i915.c b/lib/rendercopy_i915.c
> index f68e7c1f2806..9cd12a72991c 100644
> --- a/lib/rendercopy_i915.c
> +++ b/lib/rendercopy_i915.c
> @@ -84,17 +84,23 @@ void gen3_render_copyfunc(struct intel_batchbuffer *batch,
>  	/* samler state */
>  	{
>  #define TEX_COUNT 1
> -		uint32_t tiling_bits = 0;
> +		uint32_t format_bits, tiling_bits = 0;
>  		if (src->tiling != I915_TILING_NONE)
>  			tiling_bits = MS3_TILED_SURFACE;
>  		if (src->tiling == I915_TILING_Y)
>  			tiling_bits |= MS3_TILE_WALK;
>  
> +		switch (src->bpp) {
> +			case 8: format_bits = MAPSURF_8BIT | MT_8BIT_L8; break;
> +			case 16: format_bits = MAPSURF_16BIT | MT_16BIT_RGB565; break;
> +			case 32: format_bits = MAPSURF_32BIT | MT_32BIT_ARGB8888; break;
> +			default: igt_assert(0);
> +		}
> +
>  		OUT_BATCH(_3DSTATE_MAP_STATE | (3 * TEX_COUNT));
>  		OUT_BATCH((1 << TEX_COUNT) - 1);
>  		OUT_RELOC(src->bo, I915_GEM_DOMAIN_SAMPLER, 0, 0);
> -		OUT_BATCH(MAPSURF_32BIT | MT_32BIT_ARGB8888 |
> -			  tiling_bits |
> +		OUT_BATCH(format_bits | tiling_bits |
>  			  (igt_buf_height(src) - 1) << MS3_HEIGHT_SHIFT |
>  			  (igt_buf_width(src) - 1) << MS3_WIDTH_SHIFT);
>  		OUT_BATCH((src->stride/4-1) << MS4_PITCH_SHIFT);
> @@ -113,6 +119,15 @@ void gen3_render_copyfunc(struct intel_batchbuffer *batch,
>  	/* render target state */
>  	{
>  		uint32_t tiling_bits = 0;
> +		uint32_t format_bits;
> +
> +		switch (dst->bpp) {
> +			case 8: format_bits = COLR_BUF_8BIT; break;
> +			case 16: format_bits = COLR_BUF_RGB565; break;
> +			case 32: format_bits = COLR_BUF_ARGB8888; break;
> +			default: igt_assert(0);
> +		}
> +
>  		if (dst->tiling != I915_TILING_NONE)
>  			tiling_bits = BUF_3D_TILED_SURFACE;
>  		if (dst->tiling == I915_TILING_Y)
> @@ -124,7 +139,7 @@ void gen3_render_copyfunc(struct intel_batchbuffer *batch,
>  		OUT_RELOC(dst->bo, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
>  
>  		OUT_BATCH(_3DSTATE_DST_BUF_VARS_CMD);
> -		OUT_BATCH(COLR_BUF_ARGB8888 |
> +		OUT_BATCH(format_bits |
>  			  DSTORG_HORT_BIAS(0x8) |
>  			  DSTORG_VERT_BIAS(0x8));
>  
> -- 
> 2.19.1
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev