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

[Chris Wilson]

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.

> >> +/**
> >> + * 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