oneapi-src / level-zero-spec

MIT License
18 stars 27 forks source link

Corrections needed for zes_mem_timestamp_bits_exp_t #241

Closed AshwinKumarKulkarni closed 11 months ago

AshwinKumarKulkarni commented 1 year ago
  1. There is no stype field in this struct to identify the extension structure type
  2. zes_mem_timestamp_bits_exp_t should be named as zes_mem_bandwidth_counter_bits_exp_t as it is for getting valid bandwidth counter bits
  3. pnext could be added here to extend further
aravindksg commented 1 year ago

@AshwinKumarKulkarni we did add stype for zes_mem_timestamp_bits_exp_t in #231 and it's also now in the loader: ZES_STRUCTURE_TYPE_MEM_TIMESTAMP_BITS_EXP = 0x00020002, ///< ::zes_mem_timestamp_bits_exp_t

we don't need stype and pNext in zes_mem_timestamp_bits_exp_t itself. mainly desc and properties structs are derived from the base structs which add pNext and stype but yes I agree there is some inconsistency in Sysman structs. In any case, for this particular struct, we had added it earlier and we only missed stype so we can't change the struct itself and we fixed stype enum in the PR. We should now be able to detect this extension struct being passed via pNext of zes_Mem_properties_t

AshwinKumarKulkarni commented 1 year ago

As discussed with @aravindksg , we will need stype and pNext in zes_mem_timestamp_bits_exp_t. I will create PR with proposed change and we can discuss further.

joshuaranjan commented 1 year ago

Since the existing structure is experimental we could remove / modify the same structure.

aravindksg commented 1 year ago

Since the existing structure is experimental we could remove / modify the same structure.

interesting, yeah if that's doable then it'd be simpler. @jandres742 @wdamon-intel thoughts? In our Extensions page we do specify that:

Experimental extensions are not guaranteed to be forward- or backward-compatible between versions.

jandres742 commented 1 year ago

@aravindksg : correct. We can change them. Only consideration to have is to change the version of the extension.

AshwinKumarKulkarni commented 1 year ago

Created a PR for this issue https://github.com/oneapi-src/level-zero-spec/pull/244 A new extension struct is created since it involved a name change also. Please suggest if modifying existing struct is better.