add new PRIME type for more memory information #504
Conversation
the additional memory information include memory location and some addtional information Signed-off-by: Carl Zhang <[email protected]>
|
The intent of this is to connect to the memory_region stuff in the i915 kernel DRM driver, right? Given that it won't apply to anything other than i915, maybe that should be in the name rather than calling it Relatedly, what lies on the other side of this API? Is there a matching OpenCL/Vulkan extension or KMS feature which accepts/provides this information? |
| union{ | ||
| struct{ | ||
| /* whether the memory is allocated on local memory */ | ||
| uint32_t local_memory :1; |
There was a problem hiding this comment.
What does "local memory" mean? Is it memory local to the CPU running the user code, memory local to the device being used, something else?
There was a problem hiding this comment.
memory local to the device being used. it is on chip/device memory.
| /* whether the memory is allocated on local memory */ | ||
| uint32_t local_memory :1; | ||
| /* whether the memory region is enabled */ | ||
| uint32_t memory_region_enable: 1; |
There was a problem hiding this comment.
What does it mean for a memory region to be enabled? I can't find any concept like this in the i915 driver.
There was a problem hiding this comment.
if there are multiple memory chip, and has different control. it should be true.
There was a problem hiding this comment.
I am not sure that this flag is actually needed. If there is memory_region_index specified, does not that mean that "memory region is enabled?"
| /* whether the memory region is enabled */ | ||
| uint32_t memory_region_enable: 1; | ||
| /* memory region index */ | ||
| uint32_t memory_region_index: 8; |
There was a problem hiding this comment.
Index of what? Is this actually the ID of the memory region, as in enum intel_region_id?
There was a problem hiding this comment.
the ID of the memory region, sorry , seems it is confusing, because there are already intel_region_id defined. so. maybe I need use other name to distinguish with enum intel_region_id
There was a problem hiding this comment.
If there is an index, then there should be a range for this index which does not seem present in this PR. This is confusing and needs a correction: please, specify how to get a range?
yes, one of the reason is memory_region, but the main purpose is that OCL want us provide the memory location, want to know the surface is allocated on system memory or local memory( on chip/device memory). new memory type always indicate new structure, right? even we use VA_SURFACE_ATTRIB_MEM_TYPE_DRM_PRIME_I915_MEMORY_REGION, we still need a structure, but the structure also need to contain the information already in VADRMPRIMESurfaceDescriptor. actually, I suppose we forgot to reserve some bytes in VADRMPRIMESurfaceDescriptor.
Kernel user api does not provide a interface to query such information though a handle of buffer object. it's why I want to extend VADRMPRIMESurfaceDescriptor. about "Is there a matching OpenCL/Vulkan extension or KMS feature which accepts/provides this information?" I am not sure, need to check. |
| /* whether the memory region is enabled */ | ||
| uint32_t memory_region_enable: 1; | ||
| /* memory region index */ | ||
| uint32_t memory_region_index: 8; |
There was a problem hiding this comment.
If there is an index, then there should be a range for this index which does not seem present in this PR. This is confusing and needs a correction: please, specify how to get a range?
| /* whether the memory is allocated on local memory */ | ||
| uint32_t local_memory :1; | ||
| /* whether the memory region is enabled */ | ||
| uint32_t memory_region_enable: 1; |
There was a problem hiding this comment.
I am not sure that this flag is actually needed. If there is memory_region_index specified, does not that mean that "memory region is enabled?"
the additional memory information include memory location and some addtional information
Signed-off-by: Carl Zhang [email protected]