[Intel-gfx] [PATCHv7] drm/i915: Added Programming of the MOCS

[Francisco Jerez]

Chris Wilson <chris at chris-wilson.co.uk> writes:

> On Wed, Jul 08, 2015 at 03:50:06PM +0300, Francisco Jerez wrote:
>> Chris Wilson <chris at chris-wilson.co.uk> writes:
>> 
>> > On Tue, Jul 07, 2015 at 10:13:01PM +0300, Francisco Jerez wrote:
>> >> From: Peter Antoine <peter.antoine at intel.com>
>> >> 
>> >> This change adds the programming of the MOCS registers to the gen 9+
>> >> platforms. This change set programs the MOCS register values to a set
>> >> of values that are defined to be optimal.
>> >
>> > If they were optimal why weren't they defaults? I'm not feeling very
>> > satisfied by this explanation.
>> >
>> Hah, yeah, they are definitely not optimal, it's just that I didn't feel
>> fully confident modifying the original message above Peter's Signed-off
>> line.  How about we replace that sentence with: "The set of MOCS
>> configuration entries introduced by this patch is intended to be minimal
>> but sufficient to cover the needs of current userspace, it's expected to
>> be extended in the future based on the userspace demand for additional
>> caching configurations."
>
> That's much clearer. However, one now wonders how it will change in the
> future and whether we have sufficient future proofing. Do you mean that
> there is a safe subset of the current tables with higher entries
> reserved for future use (and perhaps a future param to tell userspace
> when they can use them), or perhaps in the future we will allow
> userspace to replace select MOCS tables.
>
> Bikeshedding:
>
> The set of MOCS configuration entries introduced by this patch is intended
> to be minimal but sufficient to cover the needs of current userspace - i.e.
> a good set of defaults. It is expected to be extended in the future to
> provide further default values or to allow userspace to redefine its private
> MOCS tables based on its demand for additional caching configurations.
> In this setup, userspace should only utilize the first N entries, higher
> entries are reserved for future use.
>
Sounds good to me, I'll use your text and also add a comment on top of
the tables saying that currently undefined entries are implicitly
initialized to uncached for forwards compatibility, so that applications
written against newer kernel versions with extended MOCS tables
(i.e. providing more relaxed caching semantics for some of the undefined
entries) will still work when run on older kernels, if not blazingly
fast.

>> >> +/**
>> >> + * get_mocs_settings
>> >> + *
>> >> + * This function will return the values of the MOCS table that needs to
>> >> + * be programmed for the platform. It will return the values that need
>> >> + * to be programmed and if they need to be programmed.
>> >> + *
>> >> + * If the return values is false then the registers do not need programming.
>> >> + */
>> >> +static bool get_mocs_settings(struct drm_device *dev,
>> >> +			      struct drm_i915_mocs_table *table)
>> >> +{
>> >> +	bool result = false;
>> >
>> > This looks easy enough to extend to get_mocs_settings(ctx, &table) and
>> > use CONTEXT_SET_PARAM to load a user defined mocs table. These defaults
>> > have the smell of policy in the era where even CPU PAT are becoming user
>> > defined (with per-process definitions). Is it worth shifting these to
>> > userspace? Having sane/safe defaults is good definitely.
>> >
>> 
>> Yeah, I also have the feeling that we may want to switch to some dynamic
>> set-up scheme in the future.  With only three of the available 62
>> entries in use right now it's likely to take a *long* time until we run
>> out of entries from the global tables though, when (if) that happens we
>> can always switch to dynamic set-up via some new IOCTL without
>> disrupting older userspace relying on the fixed MOCS defaults.
>> 
>> Implementing dynamic set-up is less straightforward than it may seem
>> though, because only the MOCS tables for some of the engines are context
>> saved and restored automatically by the hardware, so the settings
>> specified by one context would partially leak into other applications,
>> unless we context-switch the remaining engines manually from the kernel.
>
> Sounds like we need a set of common unalterable mocs tables for the
> shared configs, and limit userspace to doing interesting things on
> select engines. :(
> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
-------------- 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/intel-gfx/attachments/20150708/77e2a59e/attachment.sig>