[Mesa-dev] [Mesa-stable] [PATCHv2] i965/gen9: Use custom MOCS entries set up by the kernel.

[Francisco Jerez]

Ben Widawsky <ben at bwidawsk.net> writes:

> On Tue, Jul 07, 2015 at 10:21:28PM +0300, Francisco Jerez wrote:
>> Instead of relying on hardware defaults the i915 kernel driver is
>> going program custom MOCS tables system-wide on Gen9 hardware.  The
>> "WT" entry previously used for renderbuffers had a number of problems:
>> It disabled caching on eLLC, it used a reserved L3 cacheability
>> setting, and it used to override the PTE controls making renderbuffers
>> always WT on LLC regardless of the kernel's setting.  Instead use an
>> entry from the new MOCS tables with parameters: TC=LLC/eLLC, LeCC=PTE,
>> L3CC=WB.
>> 
>> The "WB" entry previously used for anything other than renderbuffers
>> has moved to a different index in the new MOCS tables but it should
>> have the same caching semantics as the old entry.
>> 
>> Even though the corresponding kernel change ("drm/i915: Added
>> Programming of the MOCS") is in a way an ABI break it doesn't seem
>> necessary to check that the kernel is recent enough because the change
>> should only affect Gen9 which is still unreleased hardware.
>> 
>> v2: Update MOCS values for the new Android-incompatible tables
>>     introduced in v7 of the kernel patch.
>> 
>> Cc: 10.6 <mesa-stable at lists.freedesktop.org>
>
> It'd be cool to get perf data, but certainly not a requirement here since the
> requirement to change is pretty obvious, IMO (mostly, I'm just curious). I do
> like having the References: in the commit for the kernel patch, but that's just
> me, and I can live with whatever.
>
I ran SynMark on SKL with this patch applied last Monday and didn't spot
any significant differences.  Some of the benchmarks seemed to give
quite erratic results regardless.  Meh...

>> ---
>>  src/mesa/drivers/dri/i965/brw_defines.h        | 11 ++++++-----
>>  src/mesa/drivers/dri/i965/gen8_surface_state.c |  3 +--
>>  2 files changed, 7 insertions(+), 7 deletions(-)
>> 
>> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
>> index 66b9abc..8ab8d62 100644
>> --- a/src/mesa/drivers/dri/i965/brw_defines.h
>> +++ b/src/mesa/drivers/dri/i965/brw_defines.h
>> @@ -2491,12 +2491,13 @@ enum brw_wm_barycentric_interp_mode {
>>  #define BDW_MOCS_WT  0x58
>>  #define BDW_MOCS_PTE 0x18
>>  
>> -/* Skylake: MOCS is now an index into an array of 64 different configurable
>> - * cache settings.  We still use only either write-back or write-through; and
>> - * rely on the documented default values.
>> +/* Skylake: MOCS is now an index into an array of 62 different caching
>> + * configurations programmed by the kernel.
>
> I'd keep the '64' instead of '62' the latter is a software construct, but
> whatever you like.

It's an actual hardware limitation, the last two entries are reserved by
the hardware and are neither configurable (as the previous comment said)
nor can be programmed by the kernel (as my comment would imply had I
left the 64).

>
>>   */
>> -#define SKL_MOCS_WB (0b001001 << 1)
>> -#define SKL_MOCS_WT (0b000101 << 1)
>> +/* TC=LLC/eLLC, LeCC=WB, LRUM=3, L3CC=WB */
>> +#define SKL_MOCS_WB  (2 << 1)
>> +/* TC=LLC/eLLC, LeCC=PTE, LRUM=3, L3CC=WB */
>> +#define SKL_MOCS_PTE (1 << 1)
>>  
>>  #define MEDIA_VFE_STATE                         0x7000
>>  /* GEN7 DW2, GEN8+ DW3 */
>> diff --git a/src/mesa/drivers/dri/i965/gen8_surface_state.c b/src/mesa/drivers/dri/i965/gen8_surface_state.c
>> index bd3eb00..dfaf762 100644
>> --- a/src/mesa/drivers/dri/i965/gen8_surface_state.c
>> +++ b/src/mesa/drivers/dri/i965/gen8_surface_state.c
>> @@ -401,8 +401,7 @@ gen8_update_renderbuffer_surface(struct brw_context *brw,
>>        irb->mt_layer : (irb->mt_layer / MAX2(mt->num_samples, 1));
>>     GLenum gl_target =
>>        rb->TexImage ? rb->TexImage->TexObject->Target : GL_TEXTURE_2D;
>> -   /* FINISHME: Use PTE MOCS on Skylake. */
>> -   uint32_t mocs = brw->gen >= 9 ? SKL_MOCS_WT : BDW_MOCS_PTE;
>> +   uint32_t mocs = brw->gen >= 9 ? SKL_MOCS_PTE : BDW_MOCS_PTE;
>
> I don't know the policy on const really, but this is a good opportunity to
> const.

Sure, why not, const is always good.

>>  
>>     intel_miptree_used_for_rendering(mt);
>>  
>
> Reviewed-by: Ben Widawsky <ben at bwidawsk.net>

Thanks.

>
> _______________________________________________
> mesa-stable mailing list
> mesa-stable at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-stable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150709/a4f49eeb/attachment.sig>