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

[Datczuk, Andrzej]

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); }
>> +
>> +


--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.