David Weinehall <david.weinehall@xxxxxxxxxxxxxxx> writes:
> On Thu, May 04, 2017 at 10:51:29AM +0100, Chris Wilson wrote:
>> A good default for garbage entries from the user is to follow the
>> default setting of the object (i.e. the PTE). Currently they use the
>> uncached entry, and now the only way to accidentally hit uncached
>> performance is via explicit use of the uncached MOCS or setting the
>> object to uncached. Note that these entries are currently undefined in
>> the ABI and we reserve the right to change them. We originally chose
>> uncached to eliminate any problem with reducing the caching level in
>> future, but the object is a much better definition of the minimum
>> caching level.
>>
NAK. The reason for the default being UC is that it's the only setting
that guarantees full forwards compatibility with any other entry that
might be added in the future. If you default to PTE on (e)LLC and WB on
L3, userspace will no longer be able to use any newly introduced entry
with stricter coherency guarantees than that (e.g. any L3-uncached
entry) in a backwards-compatible way. Attempting to do so may break
memory coherency assumptions of the application and lead to misrendering
when run on older kernel versions (which to my judgment is a scarier
failure mode than reduced performance).
My other concern is that this change may make inadvertent use of
undefined MOCS entries extremely difficult to detect in some cases -- UC
gives userspace a pretty obvious (if functionally harmless) indicative
that it's got its caching settings wrong, and is a strong motivation for
userspace developers to contribute MOCS table changes to the kernel
instead of blindly making assumptions about them (e.g. that they match
the Android kernel as media-sdk was probably doing). With this change
checked in, the performance drawback from using media-sdk on an upstream
kernel may have been subtle enough that David would never have bothered
to look into the issue. People may have started shipping copies of
media-sdk making bogus MOCS table assumptions (with potential
correctness implications), at which point you would have to deal with
userspace regressions anytime the MOCS table is extended in the future.
>> Fixes: 3bbaba0ceaa2 ("drm/i915: Added Programming of the MOCS")
>> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>> Cc: David Weinehall <david.weinehall@xxxxxxxxxxxxxxx>
>> Cc: Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx
>
> LGTM, and passes our nightly msdk test case.
>
> Tested-by: David Weinehall <david.weinehall@xxxxxxxxxxxxxxx>
> Reviewed-by: David Weinehall <david.weinehall@xxxxxxxxxxxxxxx>
>
>> ---
>> drivers/gpu/drm/i915/intel_mocs.c | 39 +++++++++++++++------------------------
>> 1 file changed, 15 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
>> index 92e461c68385..e7a7781ca457 100644
>> --- a/drivers/gpu/drm/i915/intel_mocs.c
>> +++ b/drivers/gpu/drm/i915/intel_mocs.c
>> @@ -85,10 +85,7 @@ struct drm_i915_mocs_table {
>> *
>> * Entries not part of the following tables are undefined as far as
>> * userspace is concerned and shouldn't be relied upon. For the time
>> - * being they will be implicitly initialized to the strictest caching
>> - * configuration (uncached) to guarantee forwards compatibility with
>> - * userspace programs written against more recent kernels providing
>> - * additional MOCS entries.
>> + * being they will be implicitly initialized to follow the PTE.
>> *
>> * NOTE: These tables MUST start with being uncached and the length
>> * MUST be less than 63 as the last two registers are reserved
>> @@ -249,16 +246,13 @@ int intel_mocs_init_engine(struct intel_engine_cs *engine)
>> table.table[index].control_value);
>>
>> /*
>> - * Ok, now set the unused entries to uncached. These entries
>> + * Ok, now set the unused entries to follow the PTE. These entries
>> * are officially undefined and no contract for the contents
>> * and settings is given for these entries.
>> - *
>> - * Entry 0 in the table is uncached - so we are just writing
>> - * that value to all the used entries.
>> */
>> for (; index < GEN9_NUM_MOCS_ENTRIES; index++)
>> I915_WRITE(mocs_register(engine->id, index),
>> - table.table[0].control_value);
>> + table.table[I915_MOCS_PTE].control_value);
>>
>> return 0;
>> }
>> @@ -295,16 +289,13 @@ static int emit_mocs_control_table(struct drm_i915_gem_request *req,
>> }
>>
>> /*
>> - * Ok, now set the unused entries to uncached. These entries
>> + * Ok, now set the unused entries to follow the PTE. These entries
>> * are officially undefined and no contract for the contents
>> * and settings is given for these entries.
>> - *
>> - * Entry 0 in the table is uncached - so we are just writing
>> - * that value to all the used entries.
>> */
>> for (; index < GEN9_NUM_MOCS_ENTRIES; index++) {
>> *cs++ = i915_mmio_reg_offset(mocs_register(engine, index));
>> - *cs++ = table->table[0].control_value;
>> + *cs++ = table->table[I915_MOCS_PTE].control_value;
>> }
>>
>> *cs++ = MI_NOOP;
>> @@ -355,18 +346,17 @@ static int emit_mocs_l3cc_table(struct drm_i915_gem_request *req,
>> if (table->size & 0x01) {
>> /* Odd table size - 1 left over */
>> *cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
>> - *cs++ = l3cc_combine(table, 2 * i, 0);
>> + *cs++ = l3cc_combine(table, 2 * i, I915_MOCS_PTE);
>> i++;
>> }
>>
>> /*
>> - * Now set the rest of the table to uncached - use entry 0 as
>> - * this will be uncached. Leave the last pair uninitialised as
>> - * they are reserved by the hardware.
>> + * Now set the rest of the table to follow the PTE.
>> + * Leave the last pair as they are reserved by the hardware.
>> */
>> for (; i < GEN9_NUM_MOCS_ENTRIES / 2; i++) {
>> *cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
>> - *cs++ = l3cc_combine(table, 0, 0);
>> + *cs++ = l3cc_combine(table, I915_MOCS_PTE, I915_MOCS_PTE);
>> }
>>
>> *cs++ = MI_NOOP;
>> @@ -402,17 +392,18 @@ void intel_mocs_init_l3cc_table(struct drm_i915_private *dev_priv)
>>
>> /* Odd table size - 1 left over */
>> if (table.size & 0x01) {
>> - I915_WRITE(GEN9_LNCFCMOCS(i), l3cc_combine(&table, 2*i, 0));
>> + I915_WRITE(GEN9_LNCFCMOCS(i),
>> + l3cc_combine(&table, 2*i, I915_MOCS_PTE));
>> i++;
>> }
>>
>> /*
>> - * Now set the rest of the table to uncached - use entry 0 as
>> - * this will be uncached. Leave the last pair as initialised as
>> - * they are reserved by the hardware.
>> + * Now set the rest of the table to follow the PTE.
>> + * Leave the last pair as they are reserved by the hardware.
>> */
>> for (; i < (GEN9_NUM_MOCS_ENTRIES / 2); i++)
>> - I915_WRITE(GEN9_LNCFCMOCS(i), l3cc_combine(&table, 0, 0));
>> + I915_WRITE(GEN9_LNCFCMOCS(i),
>> + l3cc_combine(&table, I915_MOCS_PTE, I915_MOCS_PTE));
>> }
>>
>> /**
>> --
>> 2.11.0
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Attachment:
signature.asc
Description: PGP signature