[Intel-gfx] [PATCH 3/3] intel: Make driver aware of MOCS table version
Ben Widawsky
ben at bwidawsk.net
Fri Jul 21 04:24:15 UTC 2017
On 17-07-07 09:28:08, Jason Ekstrand wrote:
>On Thu, Jul 6, 2017 at 4:27 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
>
>> We don't yet have optimal MOCS settings, but we have enough to know how
>> to at least determine when we might have non-optimal settings within our
>> driver.
>>
>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>> ---
>> src/intel/vulkan/anv_device.c | 12 ++++++++++++
>> src/intel/vulkan/anv_private.h | 2 ++
>> src/mesa/drivers/dri/i915/intel_context.c | 7 ++++++-
>> src/mesa/drivers/dri/i965/intel_screen.c | 14 ++++++++++++++
>> src/mesa/drivers/dri/i965/intel_screen.h | 2 ++
>> 5 files changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
>> index 3dc55dbb8d..8e180dbf18 100644
>> --- a/src/intel/vulkan/anv_device.c
>> +++ b/src/intel/vulkan/anv_device.c
>> @@ -368,6 +368,18 @@ anv_physical_device_init(struct anv_physical_device
>> *device,
>> device->info.max_cs_threads = max_cs_threads;
>> }
>>
>> + if (device->info.gen >= 9) {
>> + device->mocs_version = anv_gem_get_param(fd,
>> +
>> I915_PARAM_MOCS_TABLE_VERSION);
>> + switch (device->mocs_version) {
>> + default:
>> + anv_perf_warn("Kernel exposes newer MOCS table\n");
>>
>
>A perf_warn here seems reasonable though it makes more sense to me to make
>it
>
>if (device->mocs_version > ANV_MAX_KNOWN_MOCS_VERSION)
> anv_perf_warn("...");
>
>
One thing to keep in mind: the max MOCS version can vary by platform (hopefully
it doesn't).
>> + case 1:
>> + case 0:
>> + device->mocs_version = MOCS_TABLE_VERSION;
>>
>
>Why are we stomping device->mocs_version to MOCS_TABLE_VERSION? Are you
>just trying to avoid the version 0? If so, why not just have
>
>/* If the MOCS_TABLE_VERSION query fails, assume version 1 */
>if (device->mocs_version == 0)
> device->mocs_version = 1;
>
I think the switch looks better, especially as the versions increase.
>I don't think we want to have it dependent on a #define in an external
>header file. What if someone updates it for i965 and doesn't update anv or
>vice-versa?
>
>
Yeah, I am removing that external define as mentioned in the other thread. I
think it was a bad idea that I jammed in at the last minute.
>> + }
>> + }
>> +
>> brw_process_intel_debug_variable();
>>
>> device->compiler = brw_compiler_create(NULL, &device->info);
>> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
>> private.h
>> index 573778dad5..b8241a9b22 100644
>> --- a/src/intel/vulkan/anv_private.h
>> +++ b/src/intel/vulkan/anv_private.h
>> @@ -684,6 +684,8 @@ struct anv_physical_device {
>> uint32_t eu_total;
>> uint32_t subslice_total;
>>
>> + uint8_t mocs_version;
>> +
>> struct {
>> uint32_t type_count;
>> struct anv_memory_type
>> types[VK_MAX_MEMORY_TYPES];
>> diff --git a/src/mesa/drivers/dri/i915/intel_context.c
>> b/src/mesa/drivers/dri/i915/intel_context.c
>> index e0766a0e3f..9169ea650e 100644
>> --- a/src/mesa/drivers/dri/i915/intel_context.c
>> +++ b/src/mesa/drivers/dri/i915/intel_context.c
>> @@ -521,8 +521,13 @@ intelInitContext(struct intel_context *intel,
>> INTEL_DEBUG = parse_debug_string(getenv("INTEL_DEBUG"),
>> debug_control);
>> if (INTEL_DEBUG & DEBUG_BUFMGR)
>> dri_bufmgr_set_debug(intel->bufmgr, true);
>> - if (INTEL_DEBUG & DEBUG_PERF)
>> + if (INTEL_DEBUG & DEBUG_PERF) {
>> intel->perf_debug = true;
>> + if (screen->mocs_version > MOCS_TABLE_VERSION) {
>> + fprintf(stderr, "Kernel exposes newer MOCS table\n");
>> + screen->mocs_version = MOCS_TABLE_VERSION;
>> + }
>> + }
>>
>> if (INTEL_DEBUG & DEBUG_AUB)
>> drm_intel_bufmgr_gem_set_aub_dump(intel->bufmgr, true);
>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
>> b/src/mesa/drivers/dri/i965/intel_screen.c
>> index c75f2125d4..c53f133d49 100644
>> --- a/src/mesa/drivers/dri/i965/intel_screen.c
>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>> @@ -2301,6 +2301,20 @@ __DRIconfig **intelInitScreen2(__DRIscreen
>> *dri_screen)
>> (ret != -1 || errno != EINVAL);
>> }
>>
>> + if (devinfo->gen >= 9) {
>> + screen->mocs_version = intel_get_integer(screen,
>> +
>> I915_PARAM_MOCS_TABLE_VERSION);
>> + switch (screen->mocs_version) {
>> + case 1:
>> + case 0:
>> + screen->mocs_version = MOCS_TABLE_VERSION;
>>
>
>Same comments apply here.
>
>
>> + break;
>> + default:
>> + /* We want to perf debug, but we can't yet */
>> + break;
>> + }
>> + }
>> +
>> dri_screen->extensions = !screen->has_context_reset_notification
>> ? screenExtensions : intelRobustScreenExtensions;
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.h
>> b/src/mesa/drivers/dri/i965/intel_screen.h
>> index f78b3e8f74..eb801f8155 100644
>> --- a/src/mesa/drivers/dri/i965/intel_screen.h
>> +++ b/src/mesa/drivers/dri/i965/intel_screen.h
>> @@ -112,6 +112,8 @@ struct intel_screen
>> bool mesa_format_supports_texture[MESA_FORMAT_COUNT];
>> bool mesa_format_supports_render[MESA_FORMAT_COUNT];
>> enum isl_format mesa_to_isl_render_format[MESA_FORMAT_COUNT];
>> +
>> + unsigned mocs_version;
>> };
>>
>> extern void intelDestroyContext(__DRIcontext * driContextPriv);
If you feel strongly about moving to if/else, I can do it. Let me know.
More information about the Intel-gfx
mailing list