[igt-dev] [Intel-xe] [PATCH i-g-t 07/14] lib/intel_mocs: Stop encoding mocs

Lucas De Marchi lucas.demarchi at intel.com
Wed Sep 27 22:01:37 UTC 2023


On Tue, Sep 26, 2023 at 03:41:59PM -0700, Matt Roper wrote:
>On Fri, Sep 15, 2023 at 06:23:39AM -0700, Lucas De Marchi wrote:
>> From gen9 until xe2, the various gpu commands use a mocs object that is
>> defined as
>>
>> 	[0]   pad
>
>Might be more accurate to say "pxp" here since that's how it's actually
>being used in the code.

ok

...

>> @@ -276,9 +276,9 @@ gen11_fill_surface_state(struct intel_bb *ibb,
>>  	ss->ss0.horizontal_alignment = horizontal_alignment; /* align 4 */
>>
>>  	if (mocs == INTEL_BUF_MOCS_UC)
>> -		ss->ss1.memory_object_control = intel_get_uc_mocs(ibb->fd);
>> +		ss->ss1.mocs_index = intel_get_uc_mocs_index(ibb->fd);
>>  	else if (mocs == INTEL_BUF_MOCS_WB)
>> -		ss->ss1.memory_object_control = intel_get_wb_mocs(ibb->fd);
>> +		ss->ss1.mocs_index = intel_get_wb_mocs_index(ibb->fd);
>>
>>  	if (buf->tiling == I915_TILING_X)
>>  		ss->ss0.tiled_mode = 2;
>> @@ -295,7 +295,7 @@ gen11_fill_surface_state(struct intel_bb *ibb,
>>  	ss->ss9.base_addr_hi = address >> 32;
>>
>>  	if (is_dst) {
>> -		ss->ss1.memory_object_control = 2;
>> +		ss->ss1.mocs_index = 1;
>
>This is an accurate conversion of the existing code, but it doesn't seem
>like there should be a magic number here.  Looks like this originates
>from
>
>        commit c4b2b2f07fe8231afb8514fab19218dec95d562d
>        Author:     Tony Ye <tony.ye at intel.com>
>        AuthorDate: Tue Nov 13 14:36:28 2018 +0000
>        Commit:     Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>        CommitDate: Tue Feb 5 12:23:09 2019 +0000
>
>            tests/gem_media_vme: Simple test to exercise the VME block
>
>with no real explanation.
>
>Since this is a gen11 function, I guess we could probably switch this
>to I915_MOCS_PTE to try to match the presumed intent here.

ok

>> @@ -2763,9 +2763,10 @@ static struct blt_copy_object *blt_fb_init(const struct igt_fb *fb,
>>
>>  	blt_tile = fb_tile_to_blt_tile(fb->modifier);
>>  	stride = blt_tile == T_LINEAR ? fb->strides[plane] : fb->strides[plane] / 4;
>> +	mocs = intel_get_uc_mocs_index(fb->fd) << XY_FAST_COLOR_BLT_MOCS_INDEX_SHIFT;
>>
>>  	blt_set_object(blt, handle, fb->size, memregion,
>> -		       intel_get_uc_mocs(fb->fd),
>> +		       mocs,
>
>Doesn't this function expect just the (non-shifted) index?  If so, we
>shouldn't shift by XY_FAST_COLOR_BLT_MOCS_INDEX_SHIFT above.

It seems it doesn´t expect the non-shifted, but it should and I missed
that in this patch.

>
>>  		       blt_tile,
>>  		       is_ccs_modifier(fb->modifier) ? COMPRESSION_ENABLED : COMPRESSION_DISABLED,
>>  		       is_gen12_mc_ccs_modifier(fb->modifier) ? COMPRESSION_TYPE_MEDIA : COMPRESSION_TYPE_3D);
>> diff --git a/lib/intel_blt.c b/lib/intel_blt.c
>> index 429511920..9f8e3dea3 100644
>> --- a/lib/intel_blt.c
>> +++ b/lib/intel_blt.c
>> @@ -58,7 +58,8 @@ struct gen12_block_copy_data {
>>  	struct {
>>  		uint32_t dst_pitch:			BITRANGE(0, 17);
>>  		uint32_t dst_aux_mode:			BITRANGE(18, 20);
>> -		uint32_t dst_mocs:			BITRANGE(21, 27);
>> +		uint32_t pad:				BITRANGE(21, 21);
>> +		uint32_t dst_mocs:			BITRANGE(22, 27);
>
>Should we also rename all these dst_mocs and src_mocs with "_index" too
>as you've done elsewhere?  Or will that cause too much churn?

ok


>
>>  		uint32_t dst_ctrl_surface_type:		BITRANGE(28, 28);
>>  		uint32_t dst_compression:		BITRANGE(29, 29);
>>  		uint32_t dst_tiling:			BITRANGE(30, 31);
>> @@ -98,7 +99,8 @@ struct gen12_block_copy_data {
>>  	struct {
>>  		uint32_t src_pitch:			BITRANGE(0, 17);
>>  		uint32_t src_aux_mode:			BITRANGE(18, 20);
>> -		uint32_t src_mocs:			BITRANGE(21, 27);
>> +		uint32_t pad:				BITRANGE(21, 21);
>> +		uint32_t src_mocs:			BITRANGE(22, 27);
>>  		uint32_t src_ctrl_surface_type:		BITRANGE(28, 28);
>>  		uint32_t src_compression:		BITRANGE(29, 29);
>>  		uint32_t src_tiling:			BITRANGE(30, 31);
>> @@ -628,7 +630,7 @@ static void dump_bb_cmd(struct gen12_block_copy_data *data)
>>  		 cmd[0],
>>  		 data->dw00.client, data->dw00.opcode, data->dw00.color_depth,
>>  		 data->dw00.special_mode, data->dw00.length);
>> -	igt_info(" dw01: [%08x] dst <pitch: %d, aux: %d, mocs: %d, compr: %d, "
>> +	igt_info(" dw01: [%08x] dst <pitch: %d, aux: %d, mocs_idx: %d, compr: %d, "
>>  		 "tiling: %d, ctrl surf type: %d>\n",
>>  		 cmd[1], data->dw01.dst_pitch, data->dw01.dst_aux_mode,
>>  		 data->dw01.dst_mocs, data->dw01.dst_compression,
>> @@ -646,7 +648,7 @@ static void dump_bb_cmd(struct gen12_block_copy_data *data)
>>  		 data->dw06.dst_target_memory);
>>  	igt_info(" dw07: [%08x] src geom <x1: %d, y1: %d>\n",
>>  		 cmd[7], data->dw07.src_x1, data->dw07.src_y1);
>> -	igt_info(" dw08: [%08x] src <pitch: %d, aux: %d, mocs: %d, compr: %d, "
>> +	igt_info(" dw08: [%08x] src <pitch: %d, aux: %d, mocs_idx: %d, compr: %d, "
>>  		 "tiling: %d, ctrl surf type: %d>\n",
>>  		 cmd[8], data->dw08.src_pitch, data->dw08.src_aux_mode,
>>  		 data->dw08.src_mocs, data->dw08.src_compression,
>> @@ -914,7 +916,8 @@ struct gen12_ctrl_surf_copy_data {
>>
>>  	struct {
>>  		uint32_t src_address_hi:		BITRANGE(0, 24);
>> -		uint32_t src_mocs:			BITRANGE(25, 31);
>> +		uint32_t pad:				BITRANGE(25, 25);
>> +		uint32_t src_mocs:			BITRANGE(26, 31);
>>  	} dw02;
>>
>>  	struct {
>> @@ -923,7 +926,8 @@ struct gen12_ctrl_surf_copy_data {
>>
>>  	struct {
>>  		uint32_t dst_address_hi:		BITRANGE(0, 24);
>> -		uint32_t dst_mocs:			BITRANGE(25, 31);
>> +		uint32_t pad:				BITRANGE(25, 25);
>> +		uint32_t dst_mocs:			BITRANGE(26, 31);
>>  	} dw04;
>>  };
>>
>> @@ -940,11 +944,11 @@ static void dump_bb_surf_ctrl_cmd(const struct gen12_ctrl_surf_copy_data *data)
>>  		 data->dw00.size_of_ctrl_copy, data->dw00.length);
>>  	igt_info(" dw01: [%08x] src offset lo (0x%x)\n",
>>  		 cmd[1], data->dw01.src_address_lo);
>> -	igt_info(" dw02: [%08x] src offset hi (0x%x), src mocs: %u\n",
>> +	igt_info(" dw02: [%08x] src offset hi (0x%x), src mocs idx: %u\n",
>>  		 cmd[2], data->dw02.src_address_hi, data->dw02.src_mocs);
>>  	igt_info(" dw03: [%08x] dst offset lo (0x%x)\n",
>>  		 cmd[3], data->dw03.dst_address_lo);
>> -	igt_info(" dw04: [%08x] dst offset hi (0x%x), src mocs: %u\n",
>> +	igt_info(" dw04: [%08x] dst offset hi (0x%x), src mocs idx: %u\n",
>>  		 cmd[4], data->dw04.dst_address_hi, data->dw04.dst_mocs);
>>  }
>>
>> @@ -1557,7 +1561,7 @@ void blt_surface_fill_rect(int fd, const struct blt_copy_object *obj,
>>  void blt_surface_info(const char *info, const struct blt_copy_object *obj)
>>  {
>>  	igt_info("[%s]\n", info);
>> -	igt_info("surface <handle: %u, size: %llx, region: %x, mocs: %x>\n",
>> +	igt_info("surface <handle: %u, size: %llx, region: %x, mocs_idx: %x>\n",
>>  		 obj->handle, (long long) obj->size, obj->region, obj->mocs);
>>  	igt_info("        <tiling: %s, compression: %u, compression type: %d>\n",
>>  		 blt_tiling_name(obj->tiling), obj->compression, obj->compression_type);
>> diff --git a/lib/intel_mocs.c b/lib/intel_mocs.c
>> index cf3b8e4fe..6a4ec25e4 100644
>> --- a/lib/intel_mocs.c
>> +++ b/lib/intel_mocs.c
>> @@ -51,22 +51,20 @@ static void get_mocs_index(int fd, struct drm_intel_mocs_index *mocs)
>>  	}
>>  }
>>
>> -/* BitField [6:1] represents index to MOCS Tables
>> - * BitField [0] represents Encryption/Decryption
>> - */
>> -
>> -uint8_t intel_get_wb_mocs(int fd)
>> +uint8_t intel_get_wb_mocs_index(int fd)
>>  {
>>  	struct drm_intel_mocs_index mocs;
>>
>>  	get_mocs_index(fd, &mocs);
>> -	return mocs.wb_index << 1;
>> +
>> +	return mocs.wb_index;
>>  }
>>
>> -uint8_t intel_get_uc_mocs(int fd)
>> +uint8_t intel_get_uc_mocs_index(int fd)
>>  {
>>  	struct drm_intel_mocs_index mocs;
>>
>>  	get_mocs_index(fd, &mocs);
>> -	return mocs.uc_index << 1;
>> +
>> +	return mocs.uc_index;
>>  }
>> diff --git a/lib/intel_mocs.h b/lib/intel_mocs.h
>> index 255eac0ff..278f143ec 100644
>> --- a/lib/intel_mocs.h
>> +++ b/lib/intel_mocs.h
>> @@ -6,7 +6,7 @@
>>  #ifndef _INTEL_MOCS_H
>>  #define _INTEL_MOCS_H
>>
>> -uint8_t intel_get_wb_mocs(int fd);
>> -uint8_t intel_get_uc_mocs(int fd);
>> +uint8_t intel_get_wb_mocs_index(int fd);
>> +uint8_t intel_get_uc_mocs_index(int fd);
>>
>>  #endif /* _INTEL_MOCS_H */
>> diff --git a/lib/intel_reg.h b/lib/intel_reg.h
>> index 3bf3676dc..f97154d2b 100644
>> --- a/lib/intel_reg.h
>> +++ b/lib/intel_reg.h
>> @@ -2562,7 +2562,8 @@ SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>>  #define XY_MONO_SRC_BLT_WRITE_ALPHA	(1<<21)
>>  #define XY_MONO_SRC_BLT_WRITE_RGB	(1<<20)
>>
>> -#define XY_FAST_COLOR_BLT		((0x2<<29)|(0x44<<22)|0xe)
>> +#define XY_FAST_COLOR_BLT				((0x2<<29)|(0x44<<22)|0xe)
>> +#define   XY_FAST_COLOR_BLT_MOCS_INDEX_SHIFT		22
>
>Not a problem with your patch, but I'm surprised to see GPU instructions
>defined in intel_reg.h...   Seems like the code has been organized that
>way since 2009 and nobody has ever reorganized it.  :-(

it's worse

$ git grep "define XY_FAST_COLOR_BLT"
include/intel_gpu_commands.h:#define XY_FAST_COLOR_BLT_CMD              (2 << 29 | 0x44 << 22)
lib/intel_reg.h:#define XY_FAST_COLOR_BLT                               ((0x2<<29)|(0x44<<22)|0xe)

but the _CMD variant is not really being used or updated. Someone will
have to cleanup this mess, but it's beyond what I'm trying with this
series. One mess at a time :)

Judging by commit e5fe17165cbf ("intel_gpu_commands: Use kernel gpu
command definitions") I believe there's an ongoing conversion.

>
>>
>>  #define XY_FAST_COPY_BLT				((2<<29)|(0x42<<22)|0x8)
>>  /* dword 0 */
>> diff --git a/lib/rendercopy_gen9.c b/lib/rendercopy_gen9.c
>> index b8959d073..6c410e8a8 100644
>> --- a/lib/rendercopy_gen9.c
>> +++ b/lib/rendercopy_gen9.c
>> @@ -138,7 +138,7 @@ static const uint32_t gen12p71_render_copy[][4] = {
>>
>>  /* Mostly copy+paste from gen6, except height, width, pitch moved */
>>  static uint32_t
>> -gen8_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst,
>> +gen9_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst,
>>  	      bool fast_clear) {
>>  	struct gen9_surface_state *ss;
>>  	uint32_t write_domain, read_domain;
>> @@ -173,11 +173,11 @@ gen8_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst,
>>  		/*
>>  		 * mocs table version 1 index 3 groub wb use l3
>>  		 */
>> -		ss->ss1.memory_object_control = 3 << 1;
>> +		ss->ss1.mocs_index= 3;
>
>Minor spacing glitch.

ok

>
>>  		ss->ss5.mip_tail_start_lod = 0;
>>  	} else {
>>  		ss->ss0.render_cache_read_write = 1;
>> -		ss->ss1.memory_object_control = intel_get_uc_mocs(i915);
>> +		ss->ss1.mocs_index = intel_get_uc_mocs_index(i915);
>>  		ss->ss5.mip_tail_start_lod = 1; /* needed with trmode */
>>  	}
>>
>> @@ -187,7 +187,7 @@ gen8_bind_buf(struct intel_bb *ibb, const struct intel_buf *buf, int is_dst,
>>  		ss->ss0.tiled_mode = 3;
>>
>>  	if (intel_buf_pxp(buf))
>> -		ss->ss1.memory_object_control |= 1;
>> +		ss->ss1.pxp = 1;
>>
>>  	if (buf->tiling == I915_TILING_Yf)
>>  		ss->ss5.trmode = 1;
>> @@ -282,10 +282,10 @@ gen8_bind_surfaces(struct intel_bb *ibb,
>>  	binding_table = intel_bb_ptr_align(ibb, 32);
>>  	binding_table_offset = intel_bb_ptr_add_return_prev_offset(ibb, 32);
>>
>> -	binding_table[0] = gen8_bind_buf(ibb, dst, 1, fast_clear);
>> +	binding_table[0] = gen9_bind_buf(ibb, dst, 1, fast_clear);
>>
>>  	if (src != NULL)
>> -		binding_table[1] = gen8_bind_buf(ibb, src, 0, false);
>> +		binding_table[1] = gen9_bind_buf(ibb, src, 0, false);
>>
>>  	return binding_table_offset;
>>  }
>> diff --git a/lib/xehp_media.h b/lib/xehp_media.h
>> index 65a72d950..087eb2ec0 100644
>> --- a/lib/xehp_media.h
>> +++ b/lib/xehp_media.h
>> @@ -109,7 +109,8 @@ struct xehp_surface_state {
>>  		uint32_t double_fetch_disable: BITRANGE(17, 17);
>>  		uint32_t corner_texel_mode: BITRANGE(18, 18);
>>  		uint32_t base_mip_level: BITRANGE(19, 23);
>> -		uint32_t memory_object_control: BITRANGE(24, 30);
>> +		uint32_t pad1: BITRANGE(24, 24);
>
>Should we name this 'pxp' to match the name used in gen9_surface_state?

ok

thanks

Lucas De Marchi



More information about the igt-dev mailing list