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

[Francisco Jerez]

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."

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

> The changelog should be more explicit on what you mean by optimal and
> why other avenues are not pursued.
>
>> +/**
>> + * emit_mocs_control_table() - emit the mocs control table
>> + * @req:	Request to set up the MOCS table for.
>> + * @table:	The values to program into the control regs.
>> + * @reg_base:	The base for the engine that needs to be programmed.
>> + *
>> + * This function simply emits a MI_LOAD_REGISTER_IMM command for the
>> + * given table starting at the given address.
>> + *
>> + * @return 0 on success, otherwise the error status.
>> + */
>> +static int emit_mocs_control_table(struct drm_i915_gem_request *req,
>> +				   const struct drm_i915_mocs_table *table,
>> +				   u32 reg_base)
>> +{
>> +	struct intel_ringbuffer *ringbuf = req->ringbuf;
>> +	unsigned int index;
>> +	int ret;
>> +
>> +	ret = intel_logical_ring_begin(req, 2 + 2 * GEN9_NUM_MOCS_ENTRIES);
>> +	if (ret) {
>> +		DRM_DEBUG("intel_logical_ring_begin failed %d\n", ret);
>> +		return ret;
>> +	}
>
> Much happier now. I don't have to jump back and forth to check you have
> reserved the correct amount of space.
>
> One last thing to do would be
>
> if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES))
> 	return -ENODEV;
>
> It's nice here as we have the two loops and this sanity check helps
> explain the relationship between those loops and the ring emission. But
> equally you may feel that doing that check in get_mocs_table() (or just
> after) is sufficient. It needs to be done somewhere though.
>
> (If you go with adding the sanity check here, it should also be done in 
> emit_mocs_l3cc_table.)

OK, I'll fix that and resend.

Thanks!

>
> Considering that's my only real critique, pretty good!
> -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/12df3e14/attachment.sig>