[Intel-gfx] [PATCH v7 3/3] drm/i915: Implement I915_PERF_ADD/REMOVE_CONFIG interface

Wed Jul 26 15:24:25 UTC 2017

On 26/07/17 12:28, Szulc, Sebastian wrote:
> 0x2b04 - what is the setting for this register in the kernel? It is a debug register and we would like to control it too

We've disabled the clock ratio changes in the kernel because on gen9 
they produce a lot of reports unhelpful reports :

https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/i915_perf.c#n1786

>
> <0x91B8,0x91C8> - these are not simple counters only - a couple of bits are used to choose the right event to measure, so we need read/write access

Sure, I will add them in the next series. Thanks!

>
> Regards,
> S.
>
>
> -----Original Message-----
> From: Datczuk, Andrzej
> Sent: Wednesday, July 26, 2017 9:50 AM
> To: Landwerlin, Lionel G <lionel.g.landwerlin at intel.com>; intel-gfx at lists.freedesktop.org; Rudniewski, Marek <Marek.Rudniewski at intel.com>; Szulc, Sebastian <Sebastian.Szulc at intel.com>
> Cc: Auld, Matthew <matthew.auld at intel.com>; chris at chris-wilson.co.uk
> Subject: RE: [PATCH v7 3/3] drm/i915: Implement I915_PERF_ADD/REMOVE_CONFIG interface
>
> Adding Marek and Sebastian for Perf config register whitelist expertise :) Please comment the previous Lionel's message (Tuesday, July 25, 2017 6:04 PM).
>
> As for 0x2b00, it isn't included, note the different bracket (exclusive range).
>
> Regards,
> Andrzej
>
> -----Original Message-----
> From: Landwerlin, Lionel G
> Sent: Tuesday, July 25, 2017 6:04 PM
> To: Datczuk, Andrzej <andrzej.datczuk at intel.com>; intel-gfx at lists.freedesktop.org
> Cc: Auld, Matthew <matthew.auld at intel.com>; chris at chris-wilson.co.uk
> Subject: Re: [PATCH v7 3/3] drm/i915: Implement I915_PERF_ADD/REMOVE_CONFIG interface
>
> On 25/07/17 12:30, Datczuk, Andrzej wrote:
>> I think you looked only at the changes prepared previously by Matthew and just ported by me. I made a change on top of it in a third patch to alight the whitelist with MDAPI needs.
>> The change you're looking for is "Subject: [PATCH 3/3] drm/i915: extended oa reg addresses whitelist". To make it easier below is the list:
> Thanks for the list!
>
>> Perf modified 							Perf modified Name
>> <0x2710,2b00), 0x2b04 						b_counter
> 0x2b00 & 0x2b04 are managed by the kernel, I don't think we should let userspace touch those at the same time.
> This could only lead to painful debug when a bug appears.
>
>> 0xE458, 0xE45C, 0xE558, 0xE55C, 0xE658, 0xE65C, 0xE758 	flex
>    We should be good on the flex registers.
>
>> <0x9800,0x9ec0> 						mux
> Looking at the internal code base, I can't see anything using those registers.
> We have the kernel program GDT_CHICKEN_BITS (0x9840) to enable NOA.
> If that's all we're doing from userspace, we can probably let the kernel do it.
>
>> <0x25100,0x2ff90> 						mux
> Those should be in (HSW specific).
>
>> <0xD04,0xD2C>, 0xE180 					mux
> 0xe180 is named HALF_SLICE_CHICKEN2 and touched by the kernel.
> Would it be possible to provide some explanation as to why it's modified?
> I'm concerned we're going to break something if we touch it from userspace, potentially dropping what the kernel put in.
>
>> <0x182300,0x1823A4> 						mux
> Should be in (CHV specific).
>
>> 0x20CC 								mux
> Can you expand why we need this?
> I don't see any internal configs using it.
>
>> <0x91B8,0x91C8> 						mux
> I guess we can whitelist those.
> Again, I can't see anyone actually writing them, just reading.
>
>> Regards,
>> Andrzej
>>
>> -----Original Message-----
>> From: Landwerlin, Lionel G
>> Sent: Tuesday, July 25, 2017 12:52 PM
>> To: Datczuk, Andrzej <andrzej.datczuk at intel.com>;
>> intel-gfx at lists.freedesktop.org
>> Cc: Auld, Matthew <matthew.auld at intel.com>; chris at chris-wilson.co.uk
>> Subject: Re: [PATCH v7 3/3] drm/i915: Implement
>> I915_PERF_ADD/REMOVE_CONFIG interface
>>
>> Hi Andrzej,
>>
>> Thanks for the feedback. Can you tell me if that following changes are correct?
>>
>> Cheers,
>>
>> -
>> Lionel
>>
>> On 25/07/17 10:06, Datczuk, Andrzej wrote:
>>> Hi Lionel,
>>>
>>> What about the corrected whitelist I sent you before? Without allowing those registers the patch for MDAPI is basically useless.
>>> FYI The whitelist from the patches I sent you contained merged ranges for gen7-9 platforms.
>>>
>>> Regards,
>>> Andrzej
>>>
>>>     
>>> +static bool gen8_is_valid_flex_addr(struct drm_i915_private
>>> +*dev_priv,
>>> +u32 addr) {
>>> +	static const i915_reg_t flex_eu_regs[] = {
>>> +		EU_PERF_CNTL0,
>>> +		EU_PERF_CNTL1,
>>> +		EU_PERF_CNTL2,
>>> +		EU_PERF_CNTL3,
>>> +		EU_PERF_CNTL4,
>>> +		EU_PERF_CNTL5,
>>> +		EU_PERF_CNTL6,
>>> +	};
>>> +	int i;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(flex_eu_regs); i++) {
>>> +		if (flex_eu_regs[i].reg == addr)
>>> +			return true;
>>> +	}
>>> +	return false;
>>> +}
>>> +
>>> +static bool gen7_is_valid_b_counter_addr(struct drm_i915_private
>>> +*dev_priv, u32 addr) {
>>> +	return (addr >= 0x2380 && addr <= 0x27ac); }
>> Here I dropped 0x2b20 because it's an interruption register. It probably should be left to its default value and only managed by the kernel.
>>
>> Though I made a mistake as this should be addr >= 0x2374 && addr <= 0x27ac.
>>
>>> +
>>> +static bool gen7_is_valid_mux_addr(struct drm_i915_private
>>> +*dev_priv,
>>> +u32 addr) {
>>> +	return addr == NOA_WRITE.reg ||
>>> +		(addr >= 0xd0c && addr <= 0xd3c) ||
>> Arg, missing 0xd24/0xd28 here...
>>
>>> +		(addr >= 0x25100 && addr <= 0x2FB9C); }
>>> +
>>> +static bool hsw_is_valid_mux_addr(struct drm_i915_private *dev_priv,
>>> +u32 addr) {
>>> +	return (addr >= 0x25100 && addr <= 0x2FF90) ||
>>> +		gen7_is_valid_mux_addr(dev_priv, addr); }
>>> +
>>> +static bool chv_is_valid_mux_addr(struct drm_i915_private *dev_priv,
>>> +u32 addr) {
>>> +	return (addr >= 0x182300 && addr <= 0x1823A4) ||
>>> +		gen7_is_valid_mux_addr(dev_priv, addr); }
>>> +
>>> +
>