[Intel-gfx] [RFC] drm/i915: Allow the UMD to configure their own power clock state

[Oscar Mateo]

On 05/10/2017 02:09 PM, Michał Winiarski wrote:
> On Wed, May 10, 2017 at 04:47:50PM +0300, Mika Kuoppala wrote:
>> Oscar Mateo <oscar.mateo at intel.com> writes:
>>
>>> This allows userspace to shutdown slices at will for performance/power reasons
>>> (because it doesn't have a use for more slices).
>>>
>>> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin at intel.com>
>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_engine_cs.c | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
>>> index 402769d..17ff88d 100644
>>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>>> @@ -628,6 +628,7 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs *engine,
>>>   static int gen8_init_workarounds(struct intel_engine_cs *engine)
>>>   {
>>>   	struct drm_i915_private *dev_priv = engine->i915;
>>> +	int ret;
>>>   
>>>   	WA_SET_BIT_MASKED(INSTPM, INSTPM_FORCE_ORDERING);
>>>   
>>> @@ -673,6 +674,11 @@ static int gen8_init_workarounds(struct intel_engine_cs *engine)
>>>   			    GEN6_WIZ_HASHING_MASK,
>>>   			    GEN6_WIZ_HASHING_16x4);
>>>   
>>> +	/* Allow the UMD to configure their own power clock state */
>>> +	ret = wa_ring_whitelist_reg(engine, GEN8_R_PWR_CLK_STATE);
>>> +	if (ret)
>>> +		return ret;
>>> +
>> How will the different userspace clients coordinate with the slice
>> state? I guess in another words, is this part of the context?
>>
>> -Mika

As Michal said, this is context-saved/restored, so no need to coordinate 
between clients.

> Spec says (see IHD-OS-SKL-Vol2c-05.16 page 614):
> "This register must be initialized correctly when the context is submitted for
> the first time. This register is context save/restored as part of Exec-List
> context image in both Exec-List and Ring-Buffer mode of scheduling."
>
> So we're good, right?

Ditto :)

>
> Except there's also:
> "This register must not be programmed using MI_LOAD_REGISTER_IMM command in ring
> buffer or in batch buffer, however programming "NON - SLM Indication" field
> through MI_LOAD_REGISTER_IMM is an exception defined below. If a need arises to
> change the render configuration for a context being executed in HW, Scheduler
> must preempt the context and update the desired render configuration in the
> logical render context image in memory and resubmit the context"
>
> So... are we sure that we're fine with giving userspace control over this
> register? If so - it would still be a good idea to mention that the info in the
> spec is bogus (commit message or just a comment).

This approach has been double-checked with HW and they believe there 
should be no problem, as long as the update is done as:

- PIPECONTROL - Stalling flish, flush all caches (color, depth, DC$)
- LOAD_REGISTER_IMMEDIATE - R_PWR_CLK_STATE
- Reprogram complete state

I'm trying to get them to change the BSpec accordingly.

>>>   	return 0;
>>>   }
>>>   
>>> @@ -841,6 +847,11 @@ static int gen9_init_workarounds(struct intel_engine_cs *engine)
>>>   	if (ret)
>>>   		return ret;
>>>   
>>> +	/* Allow the UMD to configure their own power clock state */
>>> +	ret = wa_ring_whitelist_reg(engine, GEN8_R_PWR_CLK_STATE);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>   	return 0;
>>>   }
>>>   
>>> -- 
>>> 1.9.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>