On 26/07/17 13:40, Rudniewski, Marek wrote:
Hi Lionel,

You wrote:
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.

Some parts of NOA tree requires additional muxing; HALF_SLICE_CHICKEN2 contains 
bits used to select groups of NOA signals.
The write access to it may be limited to few bits if possible: 0x000e000e

Thanks, I guess we'll have to add a special case for this one.
We implement a workaround in the kernel WaDisableSTUnitPowerOptimization, it looks like it's only touching other bits.




You wrote:
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.

CNL uses another register to enable (depending on revision); BDW (depending on 
revision) may require some sequence of writes to this register.
I suggest to leave this register as a part of configuration.

We do this around the mux registers writes :

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

Assuming we only care about producing steppings, would it be okay?




You wrote:
0x20CC  
Can you expand why we need this?
I don't see any internal configs using it

0x20CC (Gen9) may be used (together with flex counter programming) to get 
per-subslice flex counter scores.
It's not a part of any official config, but was requested to be added.


Regards,
Marek


-----Original Message-----
From: Szulc, Sebastian
Sent: Wednesday, July 26, 2017 1:29 PM
To: Datczuk, Andrzej <andrzej.datc...@intel.com>; Landwerlin, Lionel G 
<lionel.g.landwer...@intel.com>; intel-gfx@lists.freedesktop.org; Rudniewski, Marek 
<marek.rudniew...@intel.com>
Cc: Auld, Matthew <matthew.a...@intel.com>; ch...@chris-wilson.co.uk
Subject: RE: [PATCH v7 3/3] drm/i915: Implement I915_PERF_ADD/REMOVE_CONFIG 
interface

0x2b04 - what is the setting for this register in the kernel? It is a debug 
register and we would like to control it too

<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

Regards,
S.


-----Original Message-----
From: Datczuk, Andrzej
Sent: Wednesday, July 26, 2017 9:50 AM
To: Landwerlin, Lionel G <lionel.g.landwer...@intel.com>; intel-gfx@lists.freedesktop.org; 
Rudniewski, Marek <marek.rudniew...@intel.com>; Szulc, Sebastian 
<sebastian.sz...@intel.com>
Cc: Auld, Matthew <matthew.a...@intel.com>; ch...@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.datc...@intel.com>; 
intel-gfx@lists.freedesktop.org
Cc: Auld, Matthew <matthew.a...@intel.com>; ch...@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.datc...@intel.com>;
intel-gfx@lists.freedesktop.org
Cc: Auld, Matthew <matthew.a...@intel.com>; ch...@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-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to