Re: [PATCH] tracing/treewide: Remove second parameter of __assign_str()

[Guenter Roeck]

On 5/17/24 11:00, Guenter Roeck wrote:

On 5/17/24 10:48, Steven Rostedt wrote:

On Fri, 17 May 2024 10:36:37 -0700
Guenter Roeck  wrote:


Building csky:allmodconfig (and others) ... failed
--
Error log:
In file included from include/trace/trace_events.h:419,
  from include/trace/define_trace.h:102,
  from drivers/cxl/core/trace.h:737,
  from drivers/cxl/core/trace.c:8:
drivers/cxl/core/./trace.h:383:1: error: macro "__assign_str" passed 2 
arguments, but takes just 1

This is with the patch applied on top of v6.9-8410-gff2632d7d08e.
So far that seems to be the only build failure.
Introduced with commit 6aec00139d3a8 ("cxl/core: Add region info to
cxl_general_media and cxl_dram events"). Guess we'll see more of those
towards the end of the commit window.


Looks like I made this patch just before this commit was pulled into
Linus's tree.

Which is why I'll apply and rerun the above again probably on Tuesday of
next week against Linus's latest.

This patch made it through both an allyesconfig and an allmodconfig, but on
the commit I had applied it to, which was:

   1b294a1f3561 ("Merge tag 'net-next-6.10' of 
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next")

I'll be compiling those two builds after I update it then.



I am currently repeating my test builds with the above errors fixed.
That should take a couple of hours. I'll let you know how it goes.



There are no more build failures caused by this patch after fixing the above
errors.

Tested-by: Guenter Roeck 

Guenter

Re: [RESEND v3 1/2] drm: enable (most) W=1 warnings by default across the subsystem

[Guenter Roeck]

Hi,

On Tue, Mar 05, 2024 at 11:07:35AM +0200, Jani Nikula wrote:
> At least the i915 and amd drivers enable a bunch more compiler warnings
> than the kernel defaults.
> 
> Extend most of the W=1 warnings to the entire drm subsystem by
> default. Use the copy-pasted warnings from scripts/Makefile.extrawarn
> with s/KBUILD_CFLAGS/subdir-ccflags-y/ to make it easier to compare and
> keep up with them in the future.
> 
> This is similar to the approach currently used in i915.
> 
> Some of the -Wextra warnings do need to be disabled, just like in
> Makefile.extrawarn, but take care to not disable them for W=2 or W=3
> builds, depending on the warning.
> 
> There are too many -Wformat-truncation warnings to cleanly fix up front;
> leave that warning disabled for now.
> 

With this patch in the mainline kernel, I get the following build error
when trying to build parisc:allmodconfig.

Error log:
drivers/gpu/drm/nouveau/nvif/object.c: In function 'nvif_object_mthd':
drivers/gpu/drm/nouveau/nvif/object.c:161:9: error: 'memcpy' accessing 
4294967264 or more bytes at offsets 0 and 32 overlaps 6442450881 bytes at 
offset -2147483617 [-Werror=restrict]
  161 | memcpy(data, args->mthd.data, size);
  | ^~~
drivers/gpu/drm/nouveau/nvif/object.c: In function 'nvif_object_ctor':
drivers/gpu/drm/nouveau/nvif/object.c:298:17: error: 'memcpy' accessing 
4294967240 or more bytes at offsets 0 and 56 overlaps 6442450833 bytes at 
offset -2147483593 [-Werror=restrict]
  298 | memcpy(data, args->new.data, size);

The problem is also seen with v6.9 when trying to build an image
with W=1, so it is not triggered by a code change. I don't know
if other architectures are affected. The problem is not seen with
gcc 11.4, but it is seen with gcc 12.3 and 13.2. I did not try
with older versions of gcc.

Bisect log is attached for reference.

The odd error makes me wonder if I should revert to testing with gcc 11.4
and no longer bother with later versions of gcc, at least for any affected
architectures. Any recommendations ?

Thanks,
Guenter

---
# bad: [7ee332c9f12bc5b380e36919cd7d056592a7073f] Merge tag 'parisc-for-6.10-1' 
of git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux
# good: [a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6] Linux 6.9
git bisect start 'HEAD' 'v6.9'
# good: [1b294a1f35616977caddaddf3e9d28e576a1adbc] Merge tag 'net-next-6.10' of 
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next
git bisect good 1b294a1f35616977caddaddf3e9d28e576a1adbc
# bad: [d34672777da3ea919e8adb0670ab91ddadf7dea0] Merge tag 
'fbdev-for-6.10-rc1' of 
git://git.kernel.org/pub/scm/linux/kernel/git/deller/linux-fbdev
git bisect bad d34672777da3ea919e8adb0670ab91ddadf7dea0
# bad: [2871ec40994912ce4f2e2d5072a428eb84c77d3c] Merge tag 
'drm-misc-next-2024-04-19' of https://gitlab.freedesktop.org/drm/misc/kernel 
into drm-next
git bisect bad 2871ec40994912ce4f2e2d5072a428eb84c77d3c
# bad: [34633158b8eb8fca145c9a73f8fe4f98c7275b06] Merge tag 
'amd-drm-next-6.10-2024-04-13' of https://gitlab.freedesktop.org/agd5f/linux 
into drm-next
git bisect bad 34633158b8eb8fca145c9a73f8fe4f98c7275b06
# good: [4b0cb230bdb71c23981acfa5e7b367c7dde02a41] drm/amdgpu: retire UMC v12 
mca_addr_to_pa
git bisect good 4b0cb230bdb71c23981acfa5e7b367c7dde02a41
# bad: [6376eb8b911534735fec104c1a0d780e4cf3116a] drm/dp: Clarify that 
wait_hpd_asserted() is not optional for panels
git bisect bad 6376eb8b911534735fec104c1a0d780e4cf3116a
# bad: [9c86b03863844ce69f99aa66404c79492ec9e208] drm/panthor: Fix 
panthor_devfreq kerneldoc
git bisect bad 9c86b03863844ce69f99aa66404c79492ec9e208
# bad: [b5d7cb76f2674c9d01b611141702723a95d12553] drm: add missing header 
guards to drm_internal.h
git bisect bad b5d7cb76f2674c9d01b611141702723a95d12553
# good: [4bdca11507928a4c9174e9b7240e9d058c12a71d] drm/panthor: Add the driver 
frontend block
git bisect good 4bdca11507928a4c9174e9b7240e9d058c12a71d
# good: [b2ec429b69280001d85029dc50b5427af41eb641] drm/tidss: Use 
dev_err_probe() over dev_dbg() when failing to probe the port
git bisect good b2ec429b69280001d85029dc50b5427af41eb641
# bad: [a61ddb4393ad1be61d2ffd92576d42707b05be17] drm: enable (most) W=1 
warnings by default across the subsystem
git bisect bad a61ddb4393ad1be61d2ffd92576d42707b05be17
# good: [113cc3ad8566e06d6c8ef4fc0075a938dedefab5] drm/bridge: Document bridge 
init order with pre_enable_prev_first
git bisect good 113cc3ad8566e06d6c8ef4fc0075a938dedefab5
# good: [460be1d527a8e296d85301e8b14923299508d4fc] drm/nouveau: move more 
missing UAPI bits
git bisect good 460be1d527a8e296d85301e8b14923299508d4fc
# first bad commit: [a61ddb4393ad1be61d2ffd92576d42707b05be17] drm: enable 
(most) W=1 warnings by default across the subsystem

Re: [PATCH] tracing/treewide: Remove second parameter of __assign_str()

[Guenter Roeck]

On 5/17/24 10:48, Steven Rostedt wrote:

On Fri, 17 May 2024 10:36:37 -0700
Guenter Roeck  wrote:


Building csky:allmodconfig (and others) ... failed
--
Error log:
In file included from include/trace/trace_events.h:419,
  from include/trace/define_trace.h:102,
  from drivers/cxl/core/trace.h:737,
  from drivers/cxl/core/trace.c:8:
drivers/cxl/core/./trace.h:383:1: error: macro "__assign_str" passed 2 
arguments, but takes just 1

This is with the patch applied on top of v6.9-8410-gff2632d7d08e.
So far that seems to be the only build failure.
Introduced with commit 6aec00139d3a8 ("cxl/core: Add region info to
cxl_general_media and cxl_dram events"). Guess we'll see more of those
towards the end of the commit window.


Looks like I made this patch just before this commit was pulled into
Linus's tree.

Which is why I'll apply and rerun the above again probably on Tuesday of
next week against Linus's latest.

This patch made it through both an allyesconfig and an allmodconfig, but on
the commit I had applied it to, which was:

   1b294a1f3561 ("Merge tag 'net-next-6.10' of 
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next")

I'll be compiling those two builds after I update it then.



I am currently repeating my test builds with the above errors fixed.
That should take a couple of hours. I'll let you know how it goes.

Guenter

Re: [PATCH] tracing/treewide: Remove second parameter of __assign_str()

[Guenter Roeck]

On Thu, May 16, 2024 at 01:34:54PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" 
> 
> [
>This is a treewide change. I will likely re-create this patch again in
>the second week of the merge window of v6.10 and submit it then. Hoping
>to keep the conflicts that it will cause to a minimum.
> ]
> 
> With the rework of how the __string() handles dynamic strings where it
> saves off the source string in field in the helper structure[1], the
> assignment of that value to the trace event field is stored in the helper
> value and does not need to be passed in again.
> 
> This means that with:
> 
>   __string(field, mystring)
> 
> Which use to be assigned with __assign_str(field, mystring), no longer
> needs the second parameter and it is unused. With this, __assign_str()
> will now only get a single parameter.
> 
> There's over 700 users of __assign_str() and because coccinelle does not
> handle the TRACE_EVENT() macro I ended up using the following sed script:
> 
>   git grep -l __assign_str | while read a ; do
>   sed -e 's/\(__assign_str([^,]*[^ ,]\) *,[^;]*/\1)/' $a > /tmp/test-file;
>   mv /tmp/test-file $a;
>   done
> 
> I then searched for __assign_str() that did not end with ';' as those
> were multi line assignments that the sed script above would fail to catch.
> 

Building csky:allmodconfig (and others) ... failed
--
Error log:
In file included from include/trace/trace_events.h:419,
 from include/trace/define_trace.h:102,
 from drivers/cxl/core/trace.h:737,
 from drivers/cxl/core/trace.c:8:
drivers/cxl/core/./trace.h:383:1: error: macro "__assign_str" passed 2 
arguments, but takes just 1

This is with the patch applied on top of v6.9-8410-gff2632d7d08e.
So far that seems to be the only build failure.
Introduced with commit 6aec00139d3a8 ("cxl/core: Add region info to
cxl_general_media and cxl_dram events"). Guess we'll see more of those
towards the end of the commit window.

Guenter

Re: [PATCH] drm/i915/hwmon: Fix locking inversion in sysfs getter

[Guenter Roeck]

On 3/11/24 09:58, Rodrigo Vivi wrote:

On Mon, Mar 11, 2024 at 09:06:46AM +0100, Janusz Krzysztofik wrote:

In i915 hwmon sysfs getter path we now take a hwmon_lock, then acquire an
rpm wakeref.  That results in lock inversion:

<4> [197.079335] ==
<4> [197.085473] WARNING: possible circular locking dependency detected
<4> [197.091611] 6.8.0-rc7-Patchwork_129026v7-gc4dc92fb1152+ #1 Not tainted
<4> [197.098096] --
<4> [197.104231] prometheus-node/839 is trying to acquire lock:
<4> [197.109680] 82764d80 (fs_reclaim){+.+.}-{0:0}, at: 
__kmalloc+0x9a/0x350
<4> [197.116939]
but task is already holding lock:
<4> [197.122730] 88811b772a40 (>hwmon_lock){+.+.}-{3:3}, at: 
hwm_energy+0x4b/0x100 [i915]
<4> [197.131543]
which lock already depends on the new lock.
...
<4> [197.507922] Chain exists of:
   fs_reclaim --> >reset.mutex --> >hwmon_lock
<4> [197.518528]  Possible unsafe locking scenario:
<4> [197.524411]CPU0CPU1
<4> [197.528916]
<4> [197.533418]   lock(>hwmon_lock);
<4> [197.537237]lock(>reset.mutex);
<4> [197.543376]lock(>hwmon_lock);
<4> [197.549682]   lock(fs_reclaim);
...
<4> [197.632548] Call Trace:
<4> [197.634990]  
<4> [197.637088]  dump_stack_lvl+0x64/0xb0
<4> [197.640738]  check_noncircular+0x15e/0x180
<4> [197.652968]  check_prev_add+0xe9/0xce0
<4> [197.656705]  __lock_acquire+0x179f/0x2300
<4> [197.660694]  lock_acquire+0xd8/0x2d0
<4> [197.673009]  fs_reclaim_acquire+0xa1/0xd0
<4> [197.680478]  __kmalloc+0x9a/0x350
<4> [197.689063]  acpi_ns_internalize_name.part.0+0x4a/0xb0
<4> [197.694170]  acpi_ns_get_node_unlocked+0x60/0xf0
<4> [197.720608]  acpi_ns_get_node+0x3b/0x60
<4> [197.724428]  acpi_get_handle+0x57/0xb0
<4> [197.728164]  acpi_has_method+0x20/0x50
<4> [197.731896]  acpi_pci_set_power_state+0x43/0x120
<4> [197.736485]  pci_power_up+0x24/0x1c0
<4> [197.740047]  pci_pm_default_resume_early+0x9/0x30
<4> [197.744725]  pci_pm_runtime_resume+0x2d/0x90
<4> [197.753911]  __rpm_callback+0x3c/0x110
<4> [197.762586]  rpm_callback+0x58/0x70
<4> [197.766064]  rpm_resume+0x51e/0x730
<4> [197.769542]  rpm_resume+0x267/0x730
<4> [197.773020]  rpm_resume+0x267/0x730
<4> [197.776498]  rpm_resume+0x267/0x730
<4> [197.779974]  __pm_runtime_resume+0x49/0x90
<4> [197.784055]  __intel_runtime_pm_get+0x19/0xa0 [i915]
<4> [197.789070]  hwm_energy+0x55/0x100 [i915]
<4> [197.793183]  hwm_read+0x9a/0x310 [i915]
<4> [197.797124]  hwmon_attr_show+0x36/0x120
<4> [197.800946]  dev_attr_show+0x15/0x60
<4> [197.804509]  sysfs_kf_seq_show+0xb5/0x100

However, the lock is only intended to protect either a hwmon overflow
counter or rmw hardware operations.  There is no need to hold the lock,
only the wakeref, while reading from hardware.

Acquire the lock after hardware read under rpm wakeref.

Fixes: c41b8bdcc297 ("drm/i915/hwmon: Show device level energy usage")
Signed-off-by: Janusz Krzysztofik 
Cc:  # v6.2+
---
  drivers/gpu/drm/i915/i915_hwmon.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_hwmon.c 
b/drivers/gpu/drm/i915/i915_hwmon.c
index 8c3f443c8347e..faf7670de6e06 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -136,11 +136,11 @@ hwm_energy(struct hwm_drvdata *ddat, long *energy)
else
rgaddr = hwmon->rg.energy_status_all;
  
-	mutex_lock(>hwmon_lock);

-
with_intel_runtime_pm(uncore->rpm, wakeref)
reg_val = intel_uncore_read(uncore, rgaddr);
  
+	mutex_lock(>hwmon_lock);

+


This is not enough.
check hwm_locked_with_pm_intel_uncore_rmw()

It looks like we need to rethink this lock entirely here.



I would have assumed that the lock was supposed to ensure that
reading the register value and the subsequent update of accum_energy
and reg_val_prev was synchronized. This is no longer the case
after this patch has been applied. Given that, I would agree that
it would make sense to define what the lock is supposed to protect
before changing its scope.

Guenter

Re: [PATCH v2 2/2] drm/tests/drm_buddy: add alloc_contiguous test

[Guenter Roeck]

On Wed, Feb 14, 2024 at 06:48:53PM +0530, Arunpravin Paneer Selvam wrote:
> From: Matthew Auld 
> 
> Sanity check DRM_BUDDY_CONTIGUOUS_ALLOCATION.
> 
> v2: Fix checkpatch warnings.
> 
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3097
> Signed-off-by: Matthew Auld 
> Cc: Arunpravin Paneer Selvam 
> Cc: Limonciello 
> Cc: Christian König 
> Reviewed-by: Arunpravin Paneer Selvam 
> Signed-off-by: Arunpravin Paneer Selvam 

Building csky:allmodconfig ... failed
Building openrisc:allmodconfig ... failed
Building parisc:allmodconfig ... failed
Building xtensa:allmodconfig ... failed

[ and presumably all other 32-bit systems which enable this test ]

--
Error log:
ERROR: modpost: "__umoddi3" [drivers/gpu/drm/tests/drm_buddy_test.ko] undefined!
ERROR: modpost: "__moddi3" [drivers/gpu/drm/tests/drm_buddy_test.ko] undefined!

Guenter

Re: [PATCH v8 1/6] pwm: Rename pwm_apply_state() to pwm_apply_might_sleep()

[Guenter Roeck]

On 12/12/23 03:41, Uwe Kleine-König wrote:

On Tue, Dec 12, 2023 at 08:34:00AM +, Sean Young wrote:

In order to introduce a pwm api which can be used from atomic context,
we will need two functions for applying pwm changes:

int pwm_apply_might_sleep(struct pwm *, struct pwm_state *);
int pwm_apply_atomic(struct pwm *, struct pwm_state *);

This commit just deals with renaming pwm_apply_state(), a following
commit will introduce the pwm_apply_atomic() function.

Acked-by: Dmitry Torokhov  # for input
Acked-by: Hans de Goede 
Acked-by: Jani Nikula 
Acked-by: Lee Jones 
Signed-off-by: Sean Young 


Acked-by: Uwe Kleine-König 

Several affected maintainers already acked, so I guess it's fine to take
this via the pwm tree. An Ack from the remaining maintainers would be
very welcome, an alternative would be to split the patch.

Missing Acks so far:

  - Jean Delvare / Guenter Roeck for drivers/hwmon/pwm-fan.c
  - Javier Martinez Canillas for drivers/gpu/drm/solomon/ssd130x.c
  - Liam Girdwood / Mark Brown for drivers/regulator/pwm-regulator.c
  - Helge Deller for drivers/video/fbdev/ssd1307fb.c

Best regards
Uwe




Personally I find the change unnecessary and pointless, which is why I
didn't ack it. Even if function names were deemed important enough, keeping
pwm_apply_state() for the time being and just adding pwm_apply_might_sleep()
as duplicate would have done it, all the changes could have gone in long
ago, and per-subsystem cleanup could have been orthogonal.

I refrained from commenting because it might be considered bike shedding,
but I don't want to ack something I deem unnecessary and pointless without
comment. But then don't want to keep arguing either, so

Acked-by: Guenter Roeck 

Guenter

Re: [Intel-gfx] [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting

[Guenter Roeck]

On Tue, Feb 28, 2023 at 01:18:55PM -0800, Dixit, Ashutosh wrote:
> On Fri, 12 Aug 2022 11:06:58 -0700, Guenter Roeck wrote:
> >
> 
> Hi Guenter/linux-hwmon,
> 
> 
> > On 8/12/22 10:37, Badal Nilawar wrote:
> > > From: Dale B Stimson 
> > >
> > > Use i915 HWMON to display/modify dGfx power PL1 limit and TDP setting.
> > >
> 
> /snip/
> 
> >
> > Acked-by: Guenter Roeck 
> >
> > > ---
> > >   .../ABI/testing/sysfs-driver-intel-i915-hwmon |  20 ++
> > >   drivers/gpu/drm/i915/i915_hwmon.c | 176 +-
> > >   drivers/gpu/drm/i915/i915_reg.h   |  16 ++
> > >   drivers/gpu/drm/i915/intel_mchbar_regs.h  |   7 +
> > >   4 files changed, 217 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon 
> > > b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > index 24c4b7477d51..9a2d10edfce8 100644
> > > --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > @@ -5,3 +5,23 @@ Contact: dri-de...@lists.freedesktop.org
> > >   Description:RO. Current Voltage in millivolt.
> > >   Only supported for particular Intel i915 graphics
> > > platforms.
> > > +
> > > +What:/sys/devices/.../hwmon/hwmon/power1_max
> > > +Date:June 2022
> > > +KernelVersion:   5.19
> > > +Contact: dri-de...@lists.freedesktop.org
> > > +Description: RW. Card reactive sustained  (PL1/Tau) power limit in 
> > > microwatts.
> > > +
> > > + The power controller will throttle the operating frequency
> > > + if the power averaged over a window (typically seconds)
> > > + exceeds this limit.
> 
> We exposed this as 'power1_max' previously. However this is a "power
> limit".
> 
> https://github.com/torvalds/linux/blob/master/Documentation/hwmon/sysfs-interface.rst
> 
> says power1_max is "Maximum power". On the other hand, power1_cap is "If
> power use rises above this limit, the system should take action to reduce
> power use". So it would seem we should have chosen power1_cap for this
> power limit instead of power1_max? So do you think we should change this to
> power1_cap instead? Though even power1_max has an associated alarm so it
> also seems to be a sort of limit.
> 
> Is there any guidance as to how these different power limits should be
> used? Generally speaking is: power1_max <= power1_cap <= power1_crit, or is
> it arbitrary or something else?
> 

Nothing should ever be "arbitrary" but have some reason. Arbitrary is
if you glue all the possible attributes onto a wall and then select the
ones to use by throwing darts at it.

powerX_min, powerX_max and powerX_crit are typically hard limits which
can not actively be influenced without drastic measures such as turning
off some hardware. powerX_cap is supposed to be more flexible; the
assumption is that the hardware or firmware has some means to control power
such that it does not exceed powerX_cap (while maintaining operational
status), for example by modifying operational frequencies.

Nowadays everything may be a bit more flexible; for example, one could
imagine that a modern system could (via software) reduce the operational
frequency of the system if power consumption exceeds powerX_max or
powerX_crit. The distinction would be that with powerX_cap, the hardware
or firmware would in general be in control, while with powerX_max
and powerX_crit the host software would be in control.

> Also, only power1_cap seems to have power1_cap_min and power1_cap_max (in
> case we wanted to use min/max values for the limits), not the others.

powerX_min is supported by the infrastructure. It not being documented
is an oversight.

Guenter

> 
> Separately, we have already used up power1_crit (which is the other limit
> in official hwmon power limits) so we can't use that.
> 
> Thanks.
> --
> Ashutosh

Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable

[Guenter Roeck]

On 2/16/23 10:57, Rodrigo Vivi wrote:

On Tue, Feb 14, 2023 at 07:11:16PM -0800, Dixit, Ashutosh wrote:

On Mon, 13 Feb 2023 22:16:44 -0800, Guenter Roeck wrote:




Hi Guenter,


On 2/13/23 21:33, Ashutosh Dixit wrote:

On ATSM the PL1 power limit is disabled at power up. The previous uapi
assumed that the PL1 limit is always enabled and therefore did not have a
notion of a disabled PL1 limit. This results in erroneous PL1 limit values
when PL1 limit is disabled. For example at power up, the disabled ATSM PL1
limit is shown as 0 which means a low PL1 limit whereas the limit being
disabled actually implies a high effective PL1 limit value.

To get round this problem, expose power1_max_enable as a custom hwmon
attribute. power1_max_enable can be used in conjunction with power1_max to
interpret power1_max (PL1 limit) values correctly. It can also be used to
enable/disable the PL1 power limit.

Signed-off-by: Ashutosh Dixit 
---
   .../ABI/testing/sysfs-driver-intel-i915-hwmon |  7 +++
   drivers/gpu/drm/i915/i915_hwmon.c | 48 +--
   2 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon 
b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
index 2d6a472eef885..edd94a44b4570 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
@@ -18,6 +18,13 @@ Description: RW. Card reactive sustained  (PL1/Tau) power 
limit in microwatts.
Only supported for particular Intel i915 graphics
platforms.
   +What:   /sys/devices/.../hwmon/hwmon/power1_max_enable


This is not a standard hwmon attribute. The standard attribute would be
power1_enable.

So from hwmon perspective this is a NACK.


Thanks for the feedback. I did consider power1_enable but decided to go
with the power1_max_enable custom attribute. Documentation for
power1_enable says it is to "Enable or disable the sensors" but in our case
we are not enabling/disabling sensors (which we don't have any ability to,
neither do we expose any power measurements, only energy from which power
can be derived) but enabling/disabling a "power limit" (a limit beyond
which HW takes steps to limit power).


Hi Guenter,

are you okay with this explanation to release the previous 'nack'?



Not really. My suggested solution would have been to use a value of '0'
to indicate 'disabled' and document it accordingly.


For me it looks like this case really doesn't fit in the standard ones.

But also this made me wonder what are the rules for non-standard cases?

I couldn't find any clear guidelines in here:
https://docs.kernel.org/hwmon/hwmon-kernel-api.html#driver-provided-sysfs-attributes

We are seeing drivers around to freely use non-standard hwmon.


Yes, sure, freely. You conveniently ignore

Do not create non-standard attributes unless really needed. If you have to use
non-standard attributes, or you believe you do, discuss it on the mailing list
first. Either case, provide a detailed explanation why you need the non-standard
attribute(s). Standard attributes are specified in Naming and data format
standards for sysfs files.

from Documentation/hwmon/submitting-patches.rst.


Are we free to add non standard ones as long if doesn't fit in the defined
standards, or should we really limit the use and do our own thing on our own?




I mean, for the new Xe driver I was considering to standardize everything
related to freq and power on top of the hwmon instead of separated sysfs
files. But this would mean a lot of non-standard stuff on top of a few
standard hwmon stuff. But I will hold this plan if you tell me that we
should avoid and limit the non-standard cases.



Oh, I really don't want to keep arguing, especially after your "freely"
above. Do whatever you want, just keep it out of drivers/hwmon.

Guenter

Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable

[Guenter Roeck]

On 2/13/23 21:33, Ashutosh Dixit wrote:

On ATSM the PL1 power limit is disabled at power up. The previous uapi
assumed that the PL1 limit is always enabled and therefore did not have a
notion of a disabled PL1 limit. This results in erroneous PL1 limit values
when PL1 limit is disabled. For example at power up, the disabled ATSM PL1
limit is shown as 0 which means a low PL1 limit whereas the limit being
disabled actually implies a high effective PL1 limit value.

To get round this problem, expose power1_max_enable as a custom hwmon
attribute. power1_max_enable can be used in conjunction with power1_max to
interpret power1_max (PL1 limit) values correctly. It can also be used to
enable/disable the PL1 power limit.

Signed-off-by: Ashutosh Dixit 
---
  .../ABI/testing/sysfs-driver-intel-i915-hwmon |  7 +++
  drivers/gpu/drm/i915/i915_hwmon.c | 48 +--
  2 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon 
b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
index 2d6a472eef885..edd94a44b4570 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
@@ -18,6 +18,13 @@ Description: RW. Card reactive sustained  (PL1/Tau) power 
limit in microwatts.
  
  		Only supported for particular Intel i915 graphics platforms.
  
+What:		/sys/devices/.../hwmon/hwmon/power1_max_enable


This is not a standard hwmon attribute. The standard attribute would be 
power1_enable.

So from hwmon perspective this is a NACK.

Guenter


+Date:  May 2023
+KernelVersion: 6.3
+Contact:   intel-gfx@lists.freedesktop.org
+Description:   RW. Enable/disable the PL1 power limit (power1_max).
+
+   Only supported for particular Intel i915 graphics platforms.
  What: /sys/devices/.../hwmon/hwmon/power1_rated_max
  Date: February 2023
  KernelVersion:6.2
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c 
b/drivers/gpu/drm/i915/i915_hwmon.c
index 7c20a6f47b92e..5665869d8602b 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -230,13 +230,52 @@ hwm_power1_max_interval_store(struct device *dev,
PKG_PWR_LIM_1_TIME, rxy);
return count;
  }
+static SENSOR_DEVICE_ATTR_RW(power1_max_interval, hwm_power1_max_interval, 0);
  
-static SENSOR_DEVICE_ATTR(power1_max_interval, 0664,

- hwm_power1_max_interval_show,
- hwm_power1_max_interval_store, 0);
+static ssize_t
+hwm_power1_max_enable_show(struct device *dev, struct device_attribute *attr, 
char *buf)
+{
+   struct hwm_drvdata *ddat = dev_get_drvdata(dev);
+   intel_wakeref_t wakeref;
+   u32 r;
+
+   with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
+   r = intel_uncore_read(ddat->uncore, 
ddat->hwmon->rg.pkg_rapl_limit);
+
+   return sysfs_emit(buf, "%u\n", !!(r & PKG_PWR_LIM_1_EN));
+}
+
+static ssize_t
+hwm_power1_max_enable_store(struct device *dev, struct device_attribute *attr,
+   const char *buf, size_t count)
+{
+   struct hwm_drvdata *ddat = dev_get_drvdata(dev);
+   intel_wakeref_t wakeref;
+   u32 en, r;
+   bool _en;
+   int ret;
+
+   ret = kstrtobool(buf, &_en);
+   if (ret)
+   return ret;
+
+   en = REG_FIELD_PREP(PKG_PWR_LIM_1_EN, _en);
+   hwm_locked_with_pm_intel_uncore_rmw(ddat, 
ddat->hwmon->rg.pkg_rapl_limit,
+   PKG_PWR_LIM_1_EN, en);
+
+   /* Verify, because PL1 limit cannot be disabled on all platforms */
+   with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
+   r = intel_uncore_read(ddat->uncore, 
ddat->hwmon->rg.pkg_rapl_limit);
+   if ((r & PKG_PWR_LIM_1_EN) != en)
+   return -EPERM;
+
+   return count;
+}
+static SENSOR_DEVICE_ATTR_RW(power1_max_enable, hwm_power1_max_enable, 0);
  
  static struct attribute *hwm_attributes[] = {

_dev_attr_power1_max_interval.dev_attr.attr,
+   _dev_attr_power1_max_enable.dev_attr.attr,
NULL
  };
  
@@ -247,7 +286,8 @@ static umode_t hwm_attributes_visible(struct kobject *kobj,

struct hwm_drvdata *ddat = dev_get_drvdata(dev);
struct i915_hwmon *hwmon = ddat->hwmon;
  
-	if (attr == _dev_attr_power1_max_interval.dev_attr.attr)

+   if (attr == _dev_attr_power1_max_interval.dev_attr.attr ||
+   attr == _dev_attr_power1_max_enable.dev_attr.attr)
return i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit) ? 
attr->mode : 0;
  
  	return 0;

Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers

[Guenter Roeck]

On Sat, Nov 05, 2022 at 02:00:24AM -0400, Steven Rostedt wrote:
> 
> Back in April, I posted an RFC patch set to help mitigate a common issue
> where a timer gets armed just before it is freed, and when the timer
> goes off, it crashes in the timer code without any evidence of who the
> culprit was. I got side tracked and never finished up on that patch set.
> Since this type of crash is still our #1 crash we are seeing in the field,
> it has become a priority again to finish it.
> 
> The last version of that patch set is here:
> 
>   https://lore.kernel.org/all/20221104054053.431922...@goodmis.org/
> 
> I'm calling this version 4a as it only has obvious changes were the timer that
> is being shutdown is in the same function where it will be freed or released,
> as this series should be "safe" for adding. I'll be calling the other patches
> 4b for the next merge window.
> 

For the series, as far as my testbed goes:

Build results:
total: 152 pass: 152 fail: 0
Qemu test results:
total: 500 pass: 500 fail: 0

No runtime crashes or warnings observed.

Tested-by: Guenter Roeck 

Guenter

Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers

[Guenter Roeck]

On Sat, Nov 05, 2022 at 02:00:24AM -0400, Steven Rostedt wrote:
> 
> Back in April, I posted an RFC patch set to help mitigate a common issue
> where a timer gets armed just before it is freed, and when the timer
> goes off, it crashes in the timer code without any evidence of who the
> culprit was. I got side tracked and never finished up on that patch set.
> Since this type of crash is still our #1 crash we are seeing in the field,
> it has become a priority again to finish it.
> 
> The last version of that patch set is here:
> 
>   https://lore.kernel.org/all/20221104054053.431922...@goodmis.org/
> 
> I'm calling this version 4a as it only has obvious changes were the timer that
> is being shutdown is in the same function where it will be freed or released,
> as this series should be "safe" for adding. I'll be calling the other patches
> 4b for the next merge window.
> 

Just in case you didn't notice:

Looking through the resulting code, I think some of the remaining
calls to del_singleshot_timer_sync() can be converted as well.

The calls in drivers/staging/wlan-ng/prism2usb.c:prism2sta_disconnect_usb()
are obvious (the containing data structure is freed in the same function).
For drivers/char/tpm/tpm-dev-common.c:tpm_common_release(), the containing
data structure is freed in the calling code.

Thanks,
Guenter

Re: [Intel-gfx] [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers

[Guenter Roeck]

On Fri, Nov 04, 2022 at 01:40:53AM -0400, Steven Rostedt wrote:
> 
> Back in April, I posted an RFC patch set to help mitigate a common issue
> where a timer gets armed just before it is freed, and when the timer
> goes off, it crashes in the timer code without any evidence of who the
> culprit was. I got side tracked and never finished up on that patch set.
> Since this type of crash is still our #1 crash we are seeing in the field,
> it has become a priority again to finish it.
> 

After applying the patches attached below, everything compiles for me,
and there are no crashes. There are still various warnings, most in
networking. I know I need to apply some patch(es) to fix the networking
warnings, but I didn't entirely understand what exactly to apply, so
I didn't try.

Complete logs are at https://kerneltests.org/builders, on the bottom half
of the page (qemu tests, in the 'testing' column).

Guenter

---
Warnings:

ODEBUG: free active (active state 0) object type: timer_list hint: 
tcp_write_timer+0x0/0x1d0
from tcp_close -> __sk_destruct -> tcp_write_timer

ODEBUG: free active (active state 0) object type: timer_list hint: 
tcp_keepalive_timer+0x0/0x4c0
from tcp_close -> __sk_destruct -> tcp_keepalive_timer -> 
__del_timer_sync

ODEBUG: free active (active state 0) object type: timer_list hint: 
blk_rq_timed_out_timer+0x0/0x40
blk_free_queue_rcu -> blk_free_queue_rcu -> blk_rq_timed_out_timer

---
Changes applied on top of patch set to fix build errors:

diff --git a/arch/arm/mach-spear/time.c b/arch/arm/mach-spear/time.c
index e979e2197f8e..5371c824786d 100644
--- a/arch/arm/mach-spear/time.c
+++ b/arch/arm/mach-spear/time.c
@@ -90,7 +90,7 @@ static void __init spear_clocksource_init(void)
200, 16, clocksource_mmio_readw_up);
 }
 
-static inline void timer_shutdown(struct clock_event_device *evt)
+static inline void spear_timer_shutdown(struct clock_event_device *evt)
 {
u16 val = readw(gpt_base + CR(CLKEVT));
 
@@ -101,7 +101,7 @@ static inline void timer_shutdown(struct clock_event_device 
*evt)
 
 static int spear_shutdown(struct clock_event_device *evt)
 {
-   timer_shutdown(evt);
+   spear_timer_shutdown(evt);
 
return 0;
 }
@@ -111,7 +111,7 @@ static int spear_set_oneshot(struct clock_event_device *evt)
u16 val;
 
/* stop the timer */
-   timer_shutdown(evt);
+   spear_timer_shutdown(evt);
 
val = readw(gpt_base + CR(CLKEVT));
val |= CTRL_ONE_SHOT;
@@ -126,7 +126,7 @@ static int spear_set_periodic(struct clock_event_device 
*evt)
u16 val;
 
/* stop the timer */
-   timer_shutdown(evt);
+   spear_timer_shutdown(evt);
 
period = clk_get_rate(gpt_clk) / HZ;
period >>= CTRL_PRESCALER16;
diff --git a/drivers/clocksource/arm_arch_timer.c 
b/drivers/clocksource/arm_arch_timer.c
index a7ff77550e17..9c3420a0d19d 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -687,8 +687,8 @@ static irqreturn_t arch_timer_handler_virt_mem(int irq, 
void *dev_id)
return timer_handler(ARCH_TIMER_MEM_VIRT_ACCESS, evt);
 }
 
-static __always_inline int timer_shutdown(const int access,
- struct clock_event_device *clk)
+static __always_inline int arch_timer_shutdown(const int access,
+  struct clock_event_device *clk)
 {
unsigned long ctrl;
 
@@ -701,22 +701,22 @@ static __always_inline int timer_shutdown(const int 
access,
 
 static int arch_timer_shutdown_virt(struct clock_event_device *clk)
 {
-   return timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk);
+   return arch_timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk);
 }
 
 static int arch_timer_shutdown_phys(struct clock_event_device *clk)
 {
-   return timer_shutdown(ARCH_TIMER_PHYS_ACCESS, clk);
+   return arch_timer_shutdown(ARCH_TIMER_PHYS_ACCESS, clk);
 }
 
 static int arch_timer_shutdown_virt_mem(struct clock_event_device *clk)
 {
-   return timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS, clk);
+   return arch_timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS, clk);
 }
 
 static int arch_timer_shutdown_phys_mem(struct clock_event_device *clk)
 {
-   return timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS, clk);
+   return arch_timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS, clk);
 }
 
 static __always_inline void set_next_event(const int access, unsigned long evt,
diff --git a/drivers/clocksource/timer-sp804.c 
b/drivers/clocksource/timer-sp804.c
index e6a87f4af2b5..a3c38e1343f0 100644
--- a/drivers/clocksource/timer-sp804.c
+++ b/drivers/clocksource/timer-sp804.c
@@ -155,14 +155,14 @@ static irqreturn_t sp804_timer_interrupt(int irq, void 
*dev_id)
return IRQ_HANDLED;
 }
 
-static inline void timer_shutdown(struct clock_event_device *evt)
+static inline void sp804_timer_shutdown(struct clock_event_device *evt)
 {
writel(0, common_clkevt->ctrl);
 }
 
 static int 

Re: [Intel-gfx] [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers

[Guenter Roeck]

On Fri, Nov 04, 2022 at 04:38:34PM -0400, Steven Rostedt wrote:
> On Fri, 4 Nov 2022 15:42:09 -0400
> Steven Rostedt  wrote:
> 
[ ... ]
> 
> > drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
> > drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
> > drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
> > drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown = 
> > ast2600_timer_shutdown;
> > drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown = 
> > fttmr010_timer_shutdown;
> > drivers/clocksource/timer-fttmr010.c:   fttmr010->clkevt.set_state_shutdown 
> > = fttmr010->timer_shutdown;
> > drivers/clocksource/timer-fttmr010.c:   fttmr010->clkevt.tick_resume = 
> > fttmr010->timer_shutdown;
> 
> I won't touch structure fields though.
> 

Agreed, same here.

Guenter

Re: [Intel-gfx] [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers

[Guenter Roeck]

On Fri, Nov 04, 2022 at 03:42:09PM -0400, Steven Rostedt wrote:
> On Fri, 4 Nov 2022 12:22:32 -0700
> Guenter Roeck  wrote:
> 
> > Unfortunately the renaming caused some symbol conflicts.
> > 
> > Global definition: timer_shutdown
> > 
> >   File Line
> > 0 time.c93 static inline void timer_shutdown(struct 
> > clock_event_device *evt)
> > 1 arm_arch_timer.c 690 static __always_inline int timer_shutdown(const int 
> > access,
> > 2 timer-fttmr010.c 105 int (*timer_shutdown)(struct clock_event_device 
> > *evt);
> > 3 timer-sp804.c158 static inline void timer_shutdown(struct 
> > clock_event_device *evt)
> > 4 timer.h  239 static inline int timer_shutdown(struct timer_list 
> > *timer)
> 
> $ git grep '\btimer_shutdown'
> arch/arm/mach-spear/time.c:static inline void timer_shutdown(struct 
> clock_event_device *evt)
> arch/arm/mach-spear/time.c: timer_shutdown(evt);
> arch/arm/mach-spear/time.c: timer_shutdown(evt);
> arch/arm/mach-spear/time.c: timer_shutdown(evt);
> drivers/clocksource/arm_arch_timer.c:static __always_inline int 
> timer_shutdown(const int access,
> drivers/clocksource/arm_arch_timer.c:   return 
> timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk);
> drivers/clocksource/arm_arch_timer.c:   return 
> timer_shutdown(ARCH_TIMER_PHYS_ACCESS, clk);
> drivers/clocksource/arm_arch_timer.c:   return 
> timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS, clk);
> drivers/clocksource/arm_arch_timer.c:   return 
> timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS, clk);
> drivers/clocksource/timer-fttmr010.c:   int (*timer_shutdown)(struct 
> clock_event_device *evt);
> drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
> drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
> drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown(evt);
> drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown = 
> ast2600_timer_shutdown;
> drivers/clocksource/timer-fttmr010.c:   fttmr010->timer_shutdown = 
> fttmr010_timer_shutdown;
> drivers/clocksource/timer-fttmr010.c:   fttmr010->clkevt.set_state_shutdown = 
> fttmr010->timer_shutdown;
> drivers/clocksource/timer-fttmr010.c:   fttmr010->clkevt.tick_resume = 
> fttmr010->timer_shutdown;
> drivers/clocksource/timer-sp804.c:static inline void timer_shutdown(struct 
> clock_event_device *evt)
> drivers/clocksource/timer-sp804.c:  timer_shutdown(evt);
> drivers/clocksource/timer-sp804.c:  timer_shutdown(evt);
> 
> Honestly, I think these need to be renamed, as "timer_shutdown()"
> should be specific to the timer code, and not individual timers.

Yes, that is what I did locally. I am repeating my test now with that
change made.

Guenter

Re: [Intel-gfx] [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers

[Guenter Roeck]

On Fri, Nov 04, 2022 at 01:40:53AM -0400, Steven Rostedt wrote:
> 
> Back in April, I posted an RFC patch set to help mitigate a common issue
> where a timer gets armed just before it is freed, and when the timer
> goes off, it crashes in the timer code without any evidence of who the
> culprit was. I got side tracked and never finished up on that patch set.
> Since this type of crash is still our #1 crash we are seeing in the field,
> it has become a priority again to finish it.
> 
> This is v3 of that patch set. Thomas Gleixner posted an untested version
> that makes timer->function NULL as the flag that it is shutdown. I took that
> code, tested it (fixed it up), added more comments, and changed the
> name to timer_shutdown_sync(). I also converted it to use WARN_ON_ONCE()
> instead of just WARN_ON() as Linus asked for.
> 

Unfortunately the renaming caused some symbol conflicts.

Global definition: timer_shutdown

  File Line
0 time.c93 static inline void timer_shutdown(struct 
clock_event_device *evt)
1 arm_arch_timer.c 690 static __always_inline int timer_shutdown(const int 
access,
2 timer-fttmr010.c 105 int (*timer_shutdown)(struct clock_event_device *evt);
3 timer-sp804.c158 static inline void timer_shutdown(struct 
clock_event_device *evt)
4 timer.h  239 static inline int timer_shutdown(struct timer_list 
*timer)

Guenter

Re: [Intel-gfx] [core-for-CI][PATCH] iommu: Remove iova cpu hotplugging flushing

[Guenter Roeck]

On Wed, Oct 05, 2022 at 05:15:49PM +0100, Robin Murphy wrote:
> On 2022-10-05 16:25, Guenter Roeck wrote:
> > On Wed, Oct 05, 2022 at 04:26:28PM +0200, Thorsten Leemhuis wrote:
> > > [adding the coretemp maintainer (Fenghua Yu) and the appropriate mailing
> > > list to the list of recipients, as there apparently is a coretemp bug
> > > that results in a iommu change causing a regression]
> > > 
> > > On 30.09.22 18:57, Janusz Krzysztofik wrote:
> > > > I think this issue can hit any user with a platform that loads iommu and
> > > > coretemp drivers.  Adding regressi...@lists.linux.dev to the loop.
> > > 
> > > f598a497bc7d was merged for 5.13-rc1, which is quite a while ago, so at
> > > least a quick revert is out of question as it might do more harm than
> > > good. The authors of the commit are kinda responsible for fixing
> > > situations like this; but well, did anybody ask the developers of the
> > > coretemp driver kindly if they are aware of the problem and maybe even
> > > willing to fix it? Doesn't look like it from here from search lore (hope
> > > I didn't miss anything), so let's give it a try.
> > > 
> > > Ciao, Thorsten
> > > 
> > > > On Thursday, 22 September 2022 14:09:35 CEST Robin Murphy wrote:
> > > > > On 22/09/2022 11:10 am, Janusz Krzysztofik wrote:
> > > > > > From: Chris Wilson 
> > > > > > 
> > > > > > Manual revert of commit f598a497bc7d ("iova: Add CPU hotplug 
> > > > > > handler to
> > > > > > flush rcaches").  It is trying to instantiate a cpuhp notifier from 
> > > > > > inside
> > > > > > a cpuhp callback.  That code replaced intel_iommu implementation of
> > > > > > flushing per-IOVA domain CPU rcaches which used a single instance 
> > > > > > of cpuhp
> > > > > > held for the module lifetime.
> > > > > 
> > > > > OK, *now* I see what's going on. It doesn't seem unreasonable to me 
> > > > > for
> > > > > bus notifiers to touch CPU hotplug - what seems more unexpected is the
> > > > > coretemp driver creating and adding a platform device from inside a
> > > > > hotplug callback.
> > 
> > It is only unexpected if it is documented that creating a platform driver
> > from a hotplug callback is off limits.
> > 
> > > > > 
> > > > > Once we start trying to revert multiple unrelated bits of important
> > > > > functionality from other subsystems because one driver is doing a 
> > > > > weird
> > > > > thing, maybe it's time to instead question whether that driver should 
> > > > > be
> > > > > doing a weird thing?
> > 
> > That isn't the point. This _used_ to work, after all. Maybe the 
> > functionality
> > introduced with f598a497bc7d is important, but there is still a regression
> > introduced by f598a497bc7d. Sure, maybe the coretemp driver is doing
> > "a weird thing", but if some generic code is changed causing something to 
> > fail
> > that previously worked, it is still a regression and the reponsibility of 
> > the
> > person or team making the generic code change to fix the problems caused by
> > that change.
> 
> Note that AFAICS I don't think anything's actually broken, and this is
> merely a lockdep false-positive. The coretemp device itself will not be
> associated with the IOMMU, so the IOMMU notifier will never get as far as
> taking any further locks in that particular instance.
> 
> Of course I *can* try writing the patch to fix things properly if I have to,
> but fair warning; I'm not familiar with this driver or the relevant hardware
> or the subsystem, and from a brief look it will involve some significant
> redesign that I have every chance of getting wrong. Plus I'm not sure I can
> test the hotplug stuff at all since the x86 box I have to hand only seems to
> have a single coretemp device.
> 
> The fact is, the wacky thing it's doing with platform_device_add() doesn't
> actually work *all* that well anyway:
> 

Hah, yes, that is obviously a bug. Unfortunately I don't have any systems
with Intel CPU left, so I can not test myself. FWIW, on v5.18.x (which is
what Google laptops use for whatever reason), I don't see the crash, but
"modprobe -r coretemp" followed by "modprobe coretemp" doesn't work -
the driver loads, but does not register with the hwmon subsystem.
There has been no relevant change to the driver si

Re: [Intel-gfx] [core-for-CI][PATCH] iommu: Remove iova cpu hotplugging flushing

[Guenter Roeck]

On Wed, Oct 05, 2022 at 04:26:28PM +0200, Thorsten Leemhuis wrote:
> [adding the coretemp maintainer (Fenghua Yu) and the appropriate mailing
> list to the list of recipients, as there apparently is a coretemp bug
> that results in a iommu change causing a regression]
> 
> On 30.09.22 18:57, Janusz Krzysztofik wrote:
> > I think this issue can hit any user with a platform that loads iommu and 
> > coretemp drivers.  Adding regressi...@lists.linux.dev to the loop.
> 
> f598a497bc7d was merged for 5.13-rc1, which is quite a while ago, so at
> least a quick revert is out of question as it might do more harm than
> good. The authors of the commit are kinda responsible for fixing
> situations like this; but well, did anybody ask the developers of the
> coretemp driver kindly if they are aware of the problem and maybe even
> willing to fix it? Doesn't look like it from here from search lore (hope
> I didn't miss anything), so let's give it a try.
> 
> Ciao, Thorsten
> 
> > On Thursday, 22 September 2022 14:09:35 CEST Robin Murphy wrote:
> >> On 22/09/2022 11:10 am, Janusz Krzysztofik wrote:
> >>> From: Chris Wilson 
> >>>
> >>> Manual revert of commit f598a497bc7d ("iova: Add CPU hotplug handler to
> >>> flush rcaches").  It is trying to instantiate a cpuhp notifier from inside
> >>> a cpuhp callback.  That code replaced intel_iommu implementation of
> >>> flushing per-IOVA domain CPU rcaches which used a single instance of cpuhp
> >>> held for the module lifetime.
> >>
> >> OK, *now* I see what's going on. It doesn't seem unreasonable to me for 
> >> bus notifiers to touch CPU hotplug - what seems more unexpected is the 
> >> coretemp driver creating and adding a platform device from inside a 
> >> hotplug callback.

It is only unexpected if it is documented that creating a platform driver
from a hotplug callback is off limits.

> >>
> >> Once we start trying to revert multiple unrelated bits of important 
> >> functionality from other subsystems because one driver is doing a weird 
> >> thing, maybe it's time to instead question whether that driver should be 
> >> doing a weird thing?

That isn't the point. This _used_ to work, after all. Maybe the functionality
introduced with f598a497bc7d is important, but there is still a regression
introduced by f598a497bc7d. Sure, maybe the coretemp driver is doing
"a weird thing", but if some generic code is changed causing something to fail
that previously worked, it is still a regression and the reponsibility of the
person or team making the generic code change to fix the problems caused by
that change.

Guenter

> >>
> >> Thanks,
> >> Robin.
> >>
> >>> <4>[6.928112] ==
> >>> <4>[6.928621] WARNING: possible circular locking dependency detected
> >>> <4>[6.929225] 6.0.0-rc6-CI_DRM_12164-ga1f63e144e54+ #1 Not tainted
> >>> <4>[6.929818] --
> >>> <4>[6.930415] cpuhp/0/15 is trying to acquire lock:
> >>> <4>[6.931011] 888100e02a78 (&(>bus_notifier)->rwsem){}-
> > {3:3}, at: blocking_notifier_call_chain+0x20/0x50
> >>> <4>[6.931533]
> >>>but task is already holding lock:
> >>> <4>[6.931534] 826490c0 (cpuhp_state-up){+.+.}-{0:0}, at: 
> > cpuhp_thread_fun+0x48/0x1f0
> >>> <4>[6.933069]
> >>>which lock already depends on the new lock.
> >>>
> >>> <4>[6.933070]
> >>>the existing dependency chain (in reverse order) is:
> >>> <4>[6.933071]
> >>>-> #2 (cpuhp_state-up){+.+.}-{0:0}:
> >>> <4>[6.933076]lock_acquire+0xd3/0x310
> >>> <4>[6.933079]cpuhp_thread_fun+0xa6/0x1f0
> >>> <4>[6.933082]smpboot_thread_fn+0x1b5/0x260
> >>> <4>[6.933084]kthread+0xed/0x120
> >>> <4>[6.933086]ret_from_fork+0x1f/0x30
> >>> <4>[6.933089]
> >>>-> #1 (cpu_hotplug_lock){}-{0:0}:
> >>> <4>[6.933092]lock_acquire+0xd3/0x310
> >>> <4>[6.933094]__cpuhp_state_add_instance+0x43/0x1c0
> >>> <4>[6.933096]iova_domain_init_rcaches+0x199/0x1c0
> >>> <4>[6.933099]iommu_setup_dma_ops+0x104/0x3d0
> >>> <4>[6.933101]iommu_probe_device+0xa4/0x180
> >>> <4>[6.933103]iommu_bus_notifier+0x2d/0x40
> >>> <4>[6.933105]notifier_call_chain+0x31/0x90
> >>> <4>[6.933108]blocking_notifier_call_chain+0x3a/0x50
> >>> <4>[6.933110]device_add+0x3c1/0x900
> >>> <4>[6.933112]pci_device_add+0x255/0x580
> >>> <4>[6.933115]pci_scan_single_device+0xa6/0xd0
> >>> <4>[6.933117]p2sb_bar+0x7f/0x220
> >>> <4>[6.933120]i801_add_tco_spt.isra.18+0x2b/0xca [i2c_i801]
> >>> <4>[6.933124]i801_add_tco+0xb1/0xfe [i2c_i801]
> >>> <4>[6.933126]i801_probe.cold.25+0xa9/0x3a7 [i2c_i801]
> >>> <4>[6.933129]pci_device_probe+0x95/0x110
> >>> <4>[  

Re: [Intel-gfx] [PATCH 0/7] Add HWMON support

[Guenter Roeck]

On 9/26/22 10:52, Badal Nilawar wrote:

This series adds the HWMON support for DGFX

Test-with: 20220919144408.251981-1-riana.ta...@intel.com

v2:
   - Reorganized series. Created first patch as infrastructure patch
 followed by feature patches. (Ashutosh)
   - Fixed review comments (Jani)
   - Fixed review comments (Ashutosh)

v3:
   - Fixed review comments from Guenter
   - Exposed energy inferface as standard hwmon interface (Ashutosh)
   - For power interface added entries for critical power and maintained
 standard interface for all the entries except
 power1_max_interval
   - Extended support for XEHPSDV (Ashutosh)

v4:
   - Fixed review comment from Guenter
   - Cleaned up unused code

v5:
   - Fixed review comments (Jani)

v6:
   - Fixed review comments (Ashutosh)
   - Updated date and kernel version in documentation

v7:
   - Fixed review comments (Anshuman)
   - KernelVersion: 6.2, Date: February 2023 in doc (Tvrtko)

v8: s/hwmon_device_register_with_info/
   devm_hwmon_device_register_with_info/ (Ashutosh)



Is there some reason for not actually versioning this patch series ?
Just wondering.

Thanks,
Guenter


Ashutosh Dixit (2):
   drm/i915/hwmon: Expose card reactive critical power
   drm/i915/hwmon: Expose power1_max_interval

Dale B Stimson (4):
   drm/i915/hwmon: Add HWMON infrastructure
   drm/i915/hwmon: Power PL1 limit and TDP setting
   drm/i915/hwmon: Show device level energy usage
   drm/i915/hwmon: Extend power/energy for XEHPSDV

Riana Tauro (1):
   drm/i915/hwmon: Add HWMON current voltage support

  .../ABI/testing/sysfs-driver-intel-i915-hwmon |  75 ++
  drivers/gpu/drm/i915/Makefile |   3 +
  drivers/gpu/drm/i915/gt/intel_gt_regs.h   |   8 +
  drivers/gpu/drm/i915/i915_driver.c|   5 +
  drivers/gpu/drm/i915/i915_drv.h   |   2 +
  drivers/gpu/drm/i915/i915_hwmon.c | 736 ++
  drivers/gpu/drm/i915/i915_hwmon.h |  20 +
  drivers/gpu/drm/i915/i915_reg.h   |   6 +
  drivers/gpu/drm/i915/intel_mchbar_regs.h  |  21 +
  9 files changed, 876 insertions(+)
  create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
  create mode 100644 drivers/gpu/drm/i915/i915_hwmon.c
  create mode 100644 drivers/gpu/drm/i915/i915_hwmon.h

Re: [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure

[Guenter Roeck]

On Thu, Aug 25, 2022 at 06:51:12PM +0530, Badal Nilawar wrote:
> From: Dale B Stimson 
> 
> The i915 HWMON module will be used to expose voltage, power and energy
> values for dGfx. Here we set up i915 hwmon infrastructure including i915
> hwmon registration, basic data structures and functions.
> 
> v2:
>   - Create HWMON infra patch (Ashutosh)
>   - Fixed review comments (Jani)
>   - Remove "select HWMON" from i915/Kconfig (Jani)
> v3: Use hwm_ prefix for static functions (Ashutosh)
> v4: s/#ifdef CONFIG_HWMON/#if IS_REACHABLE(CONFIG_HWMON)/ since the former
> doesn't work if hwmon is compiled as a module (Guenter)
> v5: Fixed review comments (Jani)
> 
> Cc: Guenter Roeck 
> Signed-off-by: Dale B Stimson 
> Signed-off-by: Ashutosh Dixit 
> Signed-off-by: Riana Tauro 
> Signed-off-by: Badal Nilawar 

Acked-by: Guenter Roeck 

> ---
>  drivers/gpu/drm/i915/Makefile  |   3 +
>  drivers/gpu/drm/i915/i915_driver.c |   5 ++
>  drivers/gpu/drm/i915/i915_drv.h|   2 +
>  drivers/gpu/drm/i915/i915_hwmon.c  | 136 +
>  drivers/gpu/drm/i915/i915_hwmon.h  |  20 +
>  5 files changed, 166 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/i915_hwmon.c
>  create mode 100644 drivers/gpu/drm/i915/i915_hwmon.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 522ef9b4aff3..2b235f747490 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -208,6 +208,9 @@ i915-y += gt/uc/intel_uc.o \
>  # graphics system controller (GSC) support
>  i915-y += gt/intel_gsc.o
>  
> +# graphics hardware monitoring (HWMON) support
> +i915-$(CONFIG_HWMON) += i915_hwmon.o
> +
>  # modesetting core code
>  i915-y += \
>   display/hsw_ips.o \
> diff --git a/drivers/gpu/drm/i915/i915_driver.c 
> b/drivers/gpu/drm/i915/i915_driver.c
> index 1332c70370a6..248deecd26a5 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -80,6 +80,7 @@
>  #include "i915_drm_client.h"
>  #include "i915_drv.h"
>  #include "i915_getparam.h"
> +#include "i915_hwmon.h"
>  #include "i915_ioc32.h"
>  #include "i915_ioctl.h"
>  #include "i915_irq.h"
> @@ -736,6 +737,8 @@ static void i915_driver_register(struct drm_i915_private 
> *dev_priv)
>  
>   intel_gt_driver_register(to_gt(dev_priv));
>  
> + i915_hwmon_register(dev_priv);
> +
>   intel_display_driver_register(dev_priv);
>  
>   intel_power_domains_enable(dev_priv);
> @@ -762,6 +765,8 @@ static void i915_driver_unregister(struct 
> drm_i915_private *dev_priv)
>  
>   intel_display_driver_unregister(dev_priv);
>  
> + i915_hwmon_unregister(dev_priv);
> +
>   intel_gt_driver_unregister(to_gt(dev_priv));
>  
>   i915_perf_unregister(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 69ce6db6a7c1..7b5b10df3404 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -705,6 +705,8 @@ struct drm_i915_private {
>  
>   struct i915_perf perf;
>  
> + struct i915_hwmon *hwmon;
> +
>   /* Abstract the submission mechanism (legacy ringbuffer or execlists) 
> away */
>   struct intel_gt gt0;
>  
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c 
> b/drivers/gpu/drm/i915/i915_hwmon.c
> new file mode 100644
> index ..103dd543a214
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -0,0 +1,136 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2022 Intel Corporation
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#include "i915_drv.h"
> +#include "i915_hwmon.h"
> +#include "i915_reg.h"
> +#include "intel_mchbar_regs.h"
> +
> +struct hwm_reg {
> +};
> +
> +struct hwm_drvdata {
> + struct i915_hwmon *hwmon;
> + struct intel_uncore *uncore;
> + struct device *hwmon_dev;
> + char name[12];
> +};
> +
> +struct i915_hwmon {
> + struct hwm_drvdata ddat;
> + struct mutex hwmon_lock;/* counter overflow logic and 
> rmw */
> + struct hwm_reg rg;
> +};
> +
> +static const struct hwmon_channel_info *hwm_info[] = {
> + NULL
> +};
> +
> +static umode_t
> +hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> +u32 attr, int channel)
> +{
> + switch (type) {
> + default:
> + return 0;
> + }
> +}
> +
> +static int
> +hwm_read(struct device *dev, enum hwmon_sensor_types t

Re: [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure

[Guenter Roeck]

On Tue, Aug 23, 2022 at 12:46:14PM +0300, Jani Nikula wrote:
[ ... ]
> >> 
> >> So why not do this in i915 Kconfig:
> >> 
> >> config DRM_I915
> >>...
> >>depends on HWMON || HWMON=n
> > With this change I am getting recursive dependancy error when I run make 
> > oldconfig
> >
> > badal@bnilawar-desk1:~/workspace/wp3/drm-tip$ make oldconfig
> >HOSTCC  scripts/basic/fixdep
> >HOSTCC  scripts/kconfig/conf.o
> >HOSTCC  scripts/kconfig/confdata.o
> >HOSTCC  scripts/kconfig/expr.o
> >LEX scripts/kconfig/lexer.lex.c
> >YACCscripts/kconfig/parser.tab.[ch]
> >HOSTCC  scripts/kconfig/lexer.lex.o
> >HOSTCC  scripts/kconfig/menu.o
> >HOSTCC  scripts/kconfig/parser.tab.o
> >HOSTCC  scripts/kconfig/preprocess.o
> >HOSTCC  scripts/kconfig/symbol.o
> >HOSTCC  scripts/kconfig/util.o
> >HOSTLD  scripts/kconfig/conf
> > drivers/gpu/drm/i915/Kconfig:2:error: recursive dependency detected!
> > drivers/gpu/drm/i915/Kconfig:2: symbol DRM_I915 depends on HWMON
> > drivers/hwmon/Kconfig:6:symbol HWMON is selected by EEEPC_LAPTOP
> > drivers/platform/x86/Kconfig:332:   symbol EEEPC_LAPTOP depends on INPUT
> > drivers/input/Kconfig:8:symbol INPUT is selected by DRM_I915
> > For a resolution refer to Documentation/kbuild/kconfig-language.rst
> > subsection "Kconfig recursive dependency limitations"
> 
> *sigh*
> 
>   Note:
>   select should be used with care. select will force
>   a symbol to a value without visiting the dependencies.
>   By abusing select you are able to select a symbol FOO even
>   if FOO depends on BAR that is not set.
>   In general use select only for non-visible symbols
>   (no prompts anywhere) and for symbols with no dependencies.
>   That will limit the usefulness but on the other hand avoid
>   the illegal configurations all over.
> 
Agreed. HWMON should not be selected anywhere. Unfortunately it is, and
drm is no exception. It is selected by DRM_RADEON and DRM_AMDGPU.
Maybe just select it in DRM_I915 as well after all; in practice it won't
make a difference.

Guenter

Re: [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure

[Guenter Roeck]

On Fri, Aug 19, 2022 at 01:35:52PM +0300, Jani Nikula wrote:
> On Fri, 19 Aug 2022, Badal Nilawar  wrote:
> > From: Dale B Stimson 
> >
> > The i915 HWMON module will be used to expose voltage, power and energy
> > values for dGfx. Here we set up i915 hwmon infrastructure including i915
> > hwmon registration, basic data structures and functions.
> >
> > v2:
> >   - Create HWMON infra patch (Ashutosh)
> >   - Fixed review comments (Jani)
> >   - Remove "select HWMON" from i915/Kconfig (Jani)
> > v3: Use hwm_ prefix for static functions (Ashutosh)
> > v4: s/#ifdef CONFIG_HWMON/#if IS_REACHABLE(CONFIG_HWMON)/ since the former
> > doesn't work if hwmon is compiled as a module (Guenter)
> 
> Is this really what we want to do?
> 
> In my books, it's a misconfiguration to have CONFIG_HWMON=m with
> CONFIG_DRM_I915=y. That's really the problematic combo, not just
> CONFIG_HWMON=m, right? Why do we allow it at the kconfig level, and then
> have ugly hacks around it at the code level? Especially as
> CONFIG_DRM_I915=y should really be thought of as a corner case.
> 
> So why not do this in i915 Kconfig:
> 
> config DRM_I915
>   ...
>   depends on HWMON || HWMON=n
> 

Ok with me, but not my call to make. The ifdef should then use
IS_ENABLED(), though.

Guenter

> Which rejects the CONFIG_HWMON=m && CONFIG_DRM_I915=y combo.
> 
> >
> > Cc: Guenter Roeck 
> > Signed-off-by: Dale B Stimson 
> > Signed-off-by: Ashutosh Dixit 
> > Signed-off-by: Riana Tauro 
> > Signed-off-by: Badal Nilawar 
> > ---
> >  drivers/gpu/drm/i915/Makefile  |   3 +
> >  drivers/gpu/drm/i915/i915_driver.c |   7 ++
> >  drivers/gpu/drm/i915/i915_drv.h|   2 +
> >  drivers/gpu/drm/i915/i915_hwmon.c  | 135 +
> >  drivers/gpu/drm/i915/i915_hwmon.h  |  20 +
> >  5 files changed, 167 insertions(+)
> >  create mode 100644 drivers/gpu/drm/i915/i915_hwmon.c
> >  create mode 100644 drivers/gpu/drm/i915/i915_hwmon.h
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index 522ef9b4aff3..2b235f747490 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -208,6 +208,9 @@ i915-y += gt/uc/intel_uc.o \
> >  # graphics system controller (GSC) support
> >  i915-y += gt/intel_gsc.o
> >  
> > +# graphics hardware monitoring (HWMON) support
> > +i915-$(CONFIG_HWMON) += i915_hwmon.o
> 
> Moreover, this builds i915_hwmon.o as part of i915.ko (or kernel as it's
> builtin) even if we can't use it!
> 
> 
> BR,
> Jani.
> 
> 
> > +
> >  # modesetting core code
> >  i915-y += \
> > display/hsw_ips.o \
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c 
> > b/drivers/gpu/drm/i915/i915_driver.c
> > index deb8a8b76965..62340cd01dde 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -80,6 +80,7 @@
> >  #include "i915_drm_client.h"
> >  #include "i915_drv.h"
> >  #include "i915_getparam.h"
> > +#include "i915_hwmon.h"
> >  #include "i915_ioc32.h"
> >  #include "i915_ioctl.h"
> >  #include "i915_irq.h"
> > @@ -736,6 +737,9 @@ static void i915_driver_register(struct 
> > drm_i915_private *dev_priv)
> >  
> > intel_gt_driver_register(to_gt(dev_priv));
> >  
> > +#if IS_REACHABLE(CONFIG_HWMON)
> > +   i915_hwmon_register(dev_priv);
> > +#endif
> > intel_display_driver_register(dev_priv);
> >  
> > intel_power_domains_enable(dev_priv);
> > @@ -762,6 +766,9 @@ static void i915_driver_unregister(struct 
> > drm_i915_private *dev_priv)
> >  
> > intel_display_driver_unregister(dev_priv);
> >  
> > +#if IS_REACHABLE(CONFIG_HWMON)
> > +   i915_hwmon_unregister(dev_priv);
> > +#endif
> > intel_gt_driver_unregister(to_gt(dev_priv));
> >  
> > i915_perf_unregister(dev_priv);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 086bbe8945d6..d437d588dec9 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -705,6 +705,8 @@ struct drm_i915_private {
> >  
> > struct i915_perf perf;
> >  
> > +   struct i915_hwmon *hwmon;
> > +
> > /* Abstract the submission mechanism (legacy ringbuffer or execlists) 
> > away */
> > struct intel_gt gt0;
> >  
> > diff --git a/

Re: [Intel-gfx] [PATCH 7/7] drm/i915/hwmon: Extend power/energy for XEHPSDV

[Guenter Roeck]

On 8/12/22 10:37, Badal Nilawar wrote:

From: Dale B Stimson 

Extend hwmon power/energy for XEHPSDV especially per gt level energy
usage.

v2: Update to latest HWMON spec (Ashutosh)

Signed-off-by: Ashutosh Dixit 
Signed-off-by: Dale B Stimson 
Signed-off-by: Badal Nilawar 


Acked-by: Guenter Roeck 


---
  .../ABI/testing/sysfs-driver-intel-i915-hwmon |   7 +-
  drivers/gpu/drm/i915/gt/intel_gt_regs.h   |   5 +
  drivers/gpu/drm/i915/i915_hwmon.c | 120 +-
  3 files changed, 128 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon 
b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
index 34668f6c2dc4..e69bc43d4c9e 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
@@ -65,6 +65,11 @@ What:
/sys/devices/.../hwmon/hwmon/energy1_input
  Date: June 2022
  KernelVersion:5.19
  Contact:  dri-de...@lists.freedesktop.org
-Description:   RO. Energy input of device in microjoules.
+Description:   RO. Energy input of device or gt in microjoules.
+
+   For i915 device level hwmon devices (name "i915") this
+   reflects energy input for the entire device. For gt level
+   hwmon devices (name "i915_gtN") this reflects energy input
+   for the gt.
  
  		Only supported for particular Intel i915 graphics platforms.

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h 
b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
index 4604f6dbf8b6..dc3bc07cdd24 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
@@ -1569,4 +1569,9 @@
  
  #define GEN12_SFC_DONE(n)			_MMIO(0x1cc000 + (n) * 0x1000)
  
+#define GT0_PACKAGE_ENERGY_STATUS		_MMIO(0x250004)

+#define GT0_PACKAGE_RAPL_LIMIT _MMIO(0x250008)
+#define GT0_PACKAGE_POWER_SKU_UNIT _MMIO(0x250068)
+#define GT0_PLATFORM_ENERGY_STATUS _MMIO(0x25006c)
+
  #endif /* __INTEL_GT_REGS__ */
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c 
b/drivers/gpu/drm/i915/i915_hwmon.c
index 6760133c7905..4c2e8d3cfe52 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -11,6 +11,7 @@
  #include "i915_hwmon.h"
  #include "intel_mchbar_regs.h"
  #include "intel_pcode.h"
+#include "gt/intel_gt.h"
  #include "gt/intel_gt_regs.h"
  
  /*

@@ -20,7 +21,7 @@
   * - curr   - milliamperes
   * - energy - microjoules
   */
-#define SF_TIME1000
+#define SF_TIME1000
  #define SF_POWER  100
  #define SF_CURR   1000
  #define SF_ENERGY 100
@@ -36,6 +37,7 @@ struct hwm_reg {
i915_reg_t pkg_power_sku;
i915_reg_t pkg_rapl_limit;
i915_reg_t energy_status_all;
+   i915_reg_t energy_status_tile;
  };
  
  struct hwm_energy_info {

@@ -49,10 +51,12 @@ struct hwm_drvdata {
struct device *hwmon_dev;
struct hwm_energy_info ei;  /*  Energy info for 
energy1_input */
char name[12];
+   int gt_n;
  };
  
  struct i915_hwmon {

struct hwm_drvdata ddat;
+   struct hwm_drvdata ddat_gt[I915_MAX_GT];
struct mutex hwmon_lock;/* counter overflow logic and 
rmw */
struct hwm_reg rg;
u32 power_max_initial_value;
@@ -148,7 +152,10 @@ hwm_energy(struct hwm_drvdata *ddat, long *energy)
i915_reg_t rgaddr;
u32 reg_val;
  
-	rgaddr = hwmon->rg.energy_status_all;

+   if (ddat->gt_n >= 0)
+   rgaddr = hwmon->rg.energy_status_tile;
+   else
+   rgaddr = hwmon->rg.energy_status_all;
  
  	if (!i915_mmio_reg_valid(rgaddr))

return -EOPNOTSUPP;
@@ -310,6 +317,11 @@ static const struct hwmon_channel_info *hwm_info[] = {
NULL
  };
  
+static const struct hwmon_channel_info *hwm_gt_info[] = {

+   HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
+   NULL
+};
+
  /* I1 is exposed as power_crit or as curr_crit depending on bit 31 */
  static int hwm_pcode_read_i1(struct drm_i915_private *i915, u32 *uval)
  {
@@ -442,7 +454,10 @@ hwm_energy_is_visible(const struct hwm_drvdata *ddat, u32 
attr)
  
  	switch (attr) {

case hwmon_energy_input:
-   rgaddr = hwmon->rg.energy_status_all;
+   if (ddat->gt_n >= 0)
+   rgaddr = hwmon->rg.energy_status_tile;
+   else
+   rgaddr = hwmon->rg.energy_status_all;
return i915_mmio_reg_valid(rgaddr) ? 0444 : 0;
default:
return 0;
@@ -577,6 +592,44 @@ static const struct hwmon_chip_info hwm_chip_info = {
.info = hwm_info,
  };
  
+static umode_t

+hwm_gt_is_visible(const void *drvdata, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+   struct hw

Re: [Intel-gfx] [PATCH 6/7] drm/i915/hwmon: Expose power1_max_interval

[Guenter Roeck]

On 8/12/22 10:37, Badal Nilawar wrote:

From: Ashutosh Dixit 

Expose power1_max_interval, that is the tau corresponding to PL1. Some bit
manipulation is needed because of the format of PKG_PWR_LIM_1_TIME in
GT0_PACKAGE_RAPL_LIMIT register (1.x * power(2,y)).

v2: Update date and kernel version in Documentation (Badal)

Signed-off-by: Ashutosh Dixit 
Signed-off-by: Badal Nilawar 


Acked-by: Guenter Roeck 


---
  .../ABI/testing/sysfs-driver-intel-i915-hwmon |   9 ++
  drivers/gpu/drm/i915/i915_hwmon.c | 128 +-
  drivers/gpu/drm/i915/i915_reg.h   |   4 +-
  drivers/gpu/drm/i915/intel_mchbar_regs.h  |   4 +
  4 files changed, 142 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon 
b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
index bb1101757154..34668f6c2dc4 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
@@ -26,6 +26,15 @@ Description: RO. Card default power limit (default TDP 
setting).
  
  		Only supported for particular Intel i915 graphics platforms.
  
+What:		/sys/devices/.../hwmon/hwmon/power1_max_interval

+Date:  June 2022
+KernelVersion: 5.19
+Contact:   dri-de...@lists.freedesktop.org
+Description:   RW. Sustained power limit interval (Tau in PL1/Tau) in
+   milliseconds over which sustained power is averaged.
+
+   Only supported for particular Intel i915 graphics platforms.
+
  What: /sys/devices/.../hwmon/hwmon/power1_crit
  Date: June 2022
  KernelVersion:5.19
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c 
b/drivers/gpu/drm/i915/i915_hwmon.c
index b1a89a6aa220..6760133c7905 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -15,10 +15,12 @@
  
  /*

   * SF_* - scale factors for particular quantities according to hwmon spec.
+ * - time   - milliseconds
   * - power  - microwatts
   * - curr   - milliamperes
   * - energy - microjoules
   */
+#define SF_TIME1000
  #define SF_POWER  100
  #define SF_CURR   1000
  #define SF_ENERGY 100
@@ -56,6 +58,7 @@ struct i915_hwmon {
u32 power_max_initial_value;
int scl_shift_power;
int scl_shift_energy;
+   int scl_shift_time;
  };
  
  static void

@@ -177,6 +180,128 @@ i915_hwmon_energy_status_get(struct drm_i915_private 
*i915, long *energy)
return hwm_energy(ddat, energy);
  }
  
+static ssize_t

+hwm_power1_max_interval_show(struct device *dev, struct device_attribute *attr,
+char *buf)
+{
+   struct hwm_drvdata *ddat = dev_get_drvdata(dev);
+   struct i915_hwmon *hwmon = ddat->hwmon;
+   intel_wakeref_t wakeref;
+   u32 r, x, y, x_w = 2; /* 2 bits */
+   u64 tau4, out;
+
+   with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
+   r = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_rapl_limit);
+
+   x = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_X, r);
+   y = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_Y, r);
+   /*
+* tau = 1.x * power(2,y), x = bits(23:22), y = bits(21:17)
+* = (4 | x) << (y - 2)
+* where (y - 2) ensures a 1.x fixed point representation of 1.x
+* However because y can be < 2, we compute
+* tau4 = (4 | x) << y
+* but add 2 when doing the final right shift to account for units
+*/
+   tau4 = ((1 << x_w) | x) << y;
+   /* val in hwmon interface units (millisec) */
+   out = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time + x_w);
+
+   return sysfs_emit(buf, "%llu\n", out);
+}
+
+static ssize_t
+hwm_power1_max_interval_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+   struct hwm_drvdata *ddat = dev_get_drvdata(dev);
+   struct i915_hwmon *hwmon = ddat->hwmon;
+   long val, max_win, ret;
+   u32 x, y, rxy, x_w = 2; /* 2 bits */
+   intel_wakeref_t wakeref;
+   u64 tau4, r;
+
+#define PKG_MAX_WIN_DEFAULT 0x12ull
+
+   ret = kstrtoul(buf, 0, );
+   if (ret)
+   return ret;
+
+   /* val must be < max in hwmon interface units */
+   if (i915_mmio_reg_valid(hwmon->rg.pkg_power_sku)) {
+   with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
+   r = intel_uncore_read64(ddat->uncore, 
hwmon->rg.pkg_power_sku);
+   /*
+* FIXME
+* Wa_22015381490:pvc rg.pkg_power_sku value is incorrect on PVC
+* at least. The following seems to work:
+*  r <<= 8;
+* However for now to be safe just use the default value
+* below. Once issue is resolved remove the one line below.
+

Re: [Intel-gfx] [PATCH 5/7] drm/i915/hwmon: Expose card reactive critical power

[Guenter Roeck]

On 8/12/22 10:37, Badal Nilawar wrote:

From: Ashutosh Dixit 

Expose the card reactive critical (I1) power. I1 is exposed as
power1_crit in microwatts (typically for client products) or as
curr1_crit in milliamperes (typically for server).

v2: Add curr1_crit functionality (Ashutosh)
v3:
   - Use HWMON_CHANNEL_INFO to define power1_crit, curr1_crit (Badal)
   - Update date and kernel version in Documentation.
v4: Use hwm_ prefix for static functions (Ashutosh)

Acked-by: Sujaritha Sundaresan 
Signed-off-by: Ashutosh Dixit 
Signed-off-by: Badal Nilawar 


Acked-by: Guenter Roeck 


---
  .../ABI/testing/sysfs-driver-intel-i915-hwmon | 26 +
  drivers/gpu/drm/i915/i915_hwmon.c | 95 ++-
  drivers/gpu/drm/i915/i915_reg.h   |  6 ++
  3 files changed, 126 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon 
b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
index 03d71c6869d3..bb1101757154 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
@@ -26,6 +26,32 @@ Description: RO. Card default power limit (default TDP 
setting).
  
  		Only supported for particular Intel i915 graphics platforms.
  
+What:		/sys/devices/.../hwmon/hwmon/power1_crit

+Date:  June 2022
+KernelVersion: 5.19
+Contact:   dri-de...@lists.freedesktop.org
+Description:   RW. Card reactive critical (I1) power limit in microwatts.
+
+   Card reactive critical (I1) power limit in microwatts is exposed
+   for client products. The power controller will throttle the
+   operating frequency if the power averaged over a window exceeds
+   this limit.
+
+   Only supported for particular Intel i915 graphics platforms.
+
+What:  /sys/devices/.../hwmon/hwmon/curr1_crit
+Date:  June 2022
+KernelVersion: 5.19
+Contact:   dri-de...@lists.freedesktop.org
+Description:   RW. Card reactive critical (I1) power limit in milliamperes.
+
+   Card reactive critical (I1) power limit in milliamperes is
+   exposed for server products. The power controller will throttle
+   the operating frequency if the power averaged over a window
+   exceeds this limit.
+
+   Only supported for particular Intel i915 graphics platforms.
+
  What: /sys/devices/.../hwmon/hwmon/energy1_input
  Date: June 2022
  KernelVersion:5.19
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c 
b/drivers/gpu/drm/i915/i915_hwmon.c
index 416c77c89609..b1a89a6aa220 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -10,14 +10,17 @@
  #include "i915_drv.h"
  #include "i915_hwmon.h"
  #include "intel_mchbar_regs.h"
+#include "intel_pcode.h"
  #include "gt/intel_gt_regs.h"
  
  /*

   * SF_* - scale factors for particular quantities according to hwmon spec.
   * - power  - microwatts
+ * - curr   - milliamperes
   * - energy - microjoules
   */
  #define SF_POWER  100
+#define SF_CURR1000
  #define SF_ENERGY 100
  
  #define FIELD_SHIFT(__mask)\

@@ -176,11 +179,25 @@ i915_hwmon_energy_status_get(struct drm_i915_private 
*i915, long *energy)
  
  static const struct hwmon_channel_info *hwm_info[] = {

HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
-   HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX),
+   HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | 
HWMON_P_CRIT),
HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
+   HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
NULL
  };
  
+/* I1 is exposed as power_crit or as curr_crit depending on bit 31 */

+static int hwm_pcode_read_i1(struct drm_i915_private *i915, u32 *uval)
+{
+   return snb_pcode_read_p(>uncore, PCODE_POWER_SETUP,
+   POWER_SETUP_SUBCOMMAND_READ_I1, 0, uval);
+}
+
+static int hwm_pcode_write_i1(struct drm_i915_private *i915, u32 uval)
+{
+   return  snb_pcode_write_p(>uncore, PCODE_POWER_SETUP,
+ POWER_SETUP_SUBCOMMAND_WRITE_I1, 0, uval);
+}
+
  static umode_t
  hwm_in_is_visible(const struct hwm_drvdata *ddat, u32 attr)
  {
@@ -214,13 +231,18 @@ hwm_in_read(struct hwm_drvdata *ddat, u32 attr, long *val)
  static umode_t
  hwm_power_is_visible(const struct hwm_drvdata *ddat, u32 attr, int chan)
  {
+   struct drm_i915_private *i915 = ddat->uncore->i915;
struct i915_hwmon *hwmon = ddat->hwmon;
+   u32 uval;
  
  	switch (attr) {

case hwmon_power_max:
return i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit) ? 0664 : 0;
case hwmon_power_rated_max:
return i915_mmio_reg_valid(hwmon->rg.pkg_power_sku) ? 0444 : 0;
+   case hwmon_power_crit:
+   return (hwm_pcode_read_i1(i915, 

Re: [Intel-gfx] [PATCH 4/7] drm/i915/hwmon: Show device level energy usage

[Guenter Roeck]

On 8/12/22 10:37, Badal Nilawar wrote:

From: Dale B Stimson 

Use i915 HWMON to display device level energy input.

v2:
   - Updated the date and kernel version in feature description

Signed-off-by: Dale B Stimson 
Signed-off-by: Ashutosh Dixit 
Signed-off-by: Riana Tauro 
Signed-off-by: Badal Nilawar 


Acked-by: Guenter Roeck 


---
  .../ABI/testing/sysfs-driver-intel-i915-hwmon |   8 ++
  drivers/gpu/drm/i915/i915_hwmon.c | 119 +-
  drivers/gpu/drm/i915/i915_hwmon.h |   1 +
  drivers/gpu/drm/i915/intel_mchbar_regs.h  |   2 +
  4 files changed, 128 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon 
b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
index 9a2d10edfce8..03d71c6869d3 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
@@ -25,3 +25,11 @@ Contact: dri-de...@lists.freedesktop.org
  Description:  RO. Card default power limit (default TDP setting).
  
  		Only supported for particular Intel i915 graphics platforms.

+
+What:  /sys/devices/.../hwmon/hwmon/energy1_input
+Date:  June 2022
+KernelVersion: 5.19
+Contact:   dri-de...@lists.freedesktop.org
+Description:   RO. Energy input of device in microjoules.
+
+   Only supported for particular Intel i915 graphics platforms.
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c 
b/drivers/gpu/drm/i915/i915_hwmon.c
index 2ce5bf94b220..416c77c89609 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -15,8 +15,10 @@
  /*
   * SF_* - scale factors for particular quantities according to hwmon spec.
   * - power  - microwatts
+ * - energy - microjoules
   */
  #define SF_POWER  100
+#define SF_ENERGY  100
  
  #define FIELD_SHIFT(__mask)\

(BUILD_BUG_ON_ZERO(!__builtin_constant_p(__mask)) + \
@@ -28,12 +30,19 @@ struct hwm_reg {
i915_reg_t pkg_power_sku_unit;
i915_reg_t pkg_power_sku;
i915_reg_t pkg_rapl_limit;
+   i915_reg_t energy_status_all;
+};
+
+struct hwm_energy_info {
+   u32 reg_val_prev;
+   long accum_energy;  /* Accumulated energy for 
energy1_input */
  };
  
  struct hwm_drvdata {

struct i915_hwmon *hwmon;
struct intel_uncore *uncore;
struct device *hwmon_dev;
+   struct hwm_energy_info ei;  /*  Energy info for 
energy1_input */
char name[12];
  };
  
@@ -43,6 +52,7 @@ struct i915_hwmon {

struct hwm_reg rg;
u32 power_max_initial_value;
int scl_shift_power;
+   int scl_shift_energy;
  };
  
  static void

@@ -102,9 +112,72 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat, 
i915_reg_t rgadr,
bits_to_clear, bits_to_set);
  }
  
+/*

+ * hwm_energy - Obtain energy value
+ *
+ * The underlying energy hardware register is 32-bits and is subject to
+ * overflow. How long before overflow? For example, with an example
+ * scaling bit shift of 14 bits (see register *PACKAGE_POWER_SKU_UNIT) and
+ * a power draw of 1000 watts, the 32-bit counter will overflow in
+ * approximately 4.36 minutes.
+ *
+ * Examples:
+ *1 watt:  (2^32 >> 14) /1 W / (60 * 60 * 24) secs/day -> 3 days
+ * 1000 watts: (2^32 >> 14) / 1000 W / 60 secs/min -> 4.36 minutes
+ *
+ * The function significantly increases overflow duration (from 4.36
+ * minutes) by accumulating the energy register into a 'long' as allowed by
+ * the hwmon API. Using x86_64 128 bit arithmetic (see mul_u64_u32_shr()),
+ * a 'long' of 63 bits, SF_ENERGY of 1e6 (~20 bits) and
+ * hwmon->scl_shift_energy of 14 bits we have 57 (63 - 20 + 14) bits before
+ * energy1_input overflows. This at 1000 W is an overflow duration of 278 
years.
+ */
+static int
+hwm_energy(struct hwm_drvdata *ddat, long *energy)
+{
+   struct intel_uncore *uncore = ddat->uncore;
+   struct i915_hwmon *hwmon = ddat->hwmon;
+   struct hwm_energy_info *ei = >ei;
+   intel_wakeref_t wakeref;
+   i915_reg_t rgaddr;
+   u32 reg_val;
+
+   rgaddr = hwmon->rg.energy_status_all;
+
+   if (!i915_mmio_reg_valid(rgaddr))
+   return -EOPNOTSUPP;
+
+   mutex_lock(>hwmon_lock);
+
+   with_intel_runtime_pm(uncore->rpm, wakeref)
+   reg_val = intel_uncore_read(uncore, rgaddr);
+
+   if (reg_val >= ei->reg_val_prev)
+   ei->accum_energy += reg_val - ei->reg_val_prev;
+   else
+   ei->accum_energy += UINT_MAX - ei->reg_val_prev + reg_val;
+   ei->reg_val_prev = reg_val;
+
+   *energy = mul_u64_u32_shr(ei->accum_energy, SF_ENERGY,
+ hwmon->scl_shift_energy);
+   mutex_unlock(>hwmon_lock);
+
+   return 0;
+}
+
+int
+i915_hwmon_energy_status_get(struct drm_i915_pr

Re: [Intel-gfx] [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting

[Guenter Roeck]

On 8/12/22 10:37, Badal Nilawar wrote:

From: Dale B Stimson 

Use i915 HWMON to display/modify dGfx power PL1 limit and TDP setting.

v2:
   - Fix review comments (Ashutosh)
   - Do not restore power1_max upon module unload/load sequence
 because on production systems modules are always loaded
 and not unloaded/reloaded (Ashutosh)
   - Fix review comments (Jani)
   - Remove endianness conversion (Ashutosh)
v3: Add power1_rated_max (Ashutosh)
v4:
   - Use macro HWMON_CHANNEL_INFO to define power channel (Guenter)
   - Update the date and kernel version in Documentation (Badal)
v5: Use hwm_ prefix for static functions (Ashutosh)

Cc: Guenter Roeck 
Signed-off-by: Dale B Stimson 
Signed-off-by: Ashutosh Dixit 
Signed-off-by: Riana Tauro 
Signed-off-by: Badal Nilawar 


Acked-by: Guenter Roeck 


---
  .../ABI/testing/sysfs-driver-intel-i915-hwmon |  20 ++
  drivers/gpu/drm/i915/i915_hwmon.c | 176 +-
  drivers/gpu/drm/i915/i915_reg.h   |  16 ++
  drivers/gpu/drm/i915/intel_mchbar_regs.h  |   7 +
  4 files changed, 217 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon 
b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
index 24c4b7477d51..9a2d10edfce8 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
@@ -5,3 +5,23 @@ Contact:   dri-de...@lists.freedesktop.org
  Description:  RO. Current Voltage in millivolt.
  
  		Only supported for particular Intel i915 graphics platforms.

+
+What:  /sys/devices/.../hwmon/hwmon/power1_max
+Date:  June 2022
+KernelVersion: 5.19
+Contact:   dri-de...@lists.freedesktop.org
+Description:   RW. Card reactive sustained  (PL1/Tau) power limit in 
microwatts.
+
+   The power controller will throttle the operating frequency
+   if the power averaged over a window (typically seconds)
+   exceeds this limit.
+
+   Only supported for particular Intel i915 graphics platforms.
+
+What:  /sys/devices/.../hwmon/hwmon/power1_rated_max
+Date:  June 2022
+KernelVersion: 5.19
+Contact:   dri-de...@lists.freedesktop.org
+Description:   RO. Card default power limit (default TDP setting).
+
+   Only supported for particular Intel i915 graphics platforms.
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c 
b/drivers/gpu/drm/i915/i915_hwmon.c
index 1893efe796a4..2ce5bf94b220 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -12,8 +12,22 @@
  #include "intel_mchbar_regs.h"
  #include "gt/intel_gt_regs.h"
  
+/*

+ * SF_* - scale factors for particular quantities according to hwmon spec.
+ * - power  - microwatts
+ */
+#define SF_POWER   100
+
+#define FIELD_SHIFT(__mask)\
+   (BUILD_BUG_ON_ZERO(!__builtin_constant_p(__mask)) + \
+   BUILD_BUG_ON_ZERO((__mask) == 0) +  \
+   __bf_shf(__mask))
+
  struct hwm_reg {
i915_reg_t gt_perf_status;
+   i915_reg_t pkg_power_sku_unit;
+   i915_reg_t pkg_power_sku;
+   i915_reg_t pkg_rapl_limit;
  };
  
  struct hwm_drvdata {

@@ -27,10 +41,70 @@ struct i915_hwmon {
struct hwm_drvdata ddat;
struct mutex hwmon_lock;/* counter overflow logic and 
rmw */
struct hwm_reg rg;
+   u32 power_max_initial_value;
+   int scl_shift_power;
  };
  
+static void

+hwm_locked_with_pm_intel_uncore_rmw(struct hwm_drvdata *ddat,
+   i915_reg_t reg, u32 clear, u32 set)
+{
+   struct i915_hwmon *hwmon = ddat->hwmon;
+   struct intel_uncore *uncore = ddat->uncore;
+   intel_wakeref_t wakeref;
+
+   mutex_lock(>hwmon_lock);
+
+   with_intel_runtime_pm(uncore->rpm, wakeref)
+   intel_uncore_rmw(uncore, reg, clear, set);
+
+   mutex_unlock(>hwmon_lock);
+}
+
+/*
+ * This function's return type of u64 allows for the case where the scaling
+ * of the field taken from the 32-bit register value might cause a result to
+ * exceed 32 bits.
+ */
+static u64
+hwm_field_read_and_scale(struct hwm_drvdata *ddat, i915_reg_t rgadr,
+u32 field_msk, int field_shift,
+int nshift, u32 scale_factor)
+{
+   struct intel_uncore *uncore = ddat->uncore;
+   intel_wakeref_t wakeref;
+   u32 reg_value;
+
+   with_intel_runtime_pm(uncore->rpm, wakeref)
+   reg_value = intel_uncore_read(uncore, rgadr);
+
+   reg_value = (reg_value & field_msk) >> field_shift;
+
+   return mul_u64_u32_shr(reg_value, scale_factor, nshift);
+}
+
+static void
+hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
+ u32 field_msk, int field_shift,
+ int nshift, unsigned int scale_factor, long l

Re: [Intel-gfx] [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support

[Guenter Roeck]

On 8/12/22 10:37, Badal Nilawar wrote:

From: Riana Tauro 

Use i915 HWMON subsystem to display current input voltage.

v2:
   - Updated date and kernel version in feature description
   - Fixed review comments (Ashutosh)
v3: Use macro HWMON_CHANNEL_INFO to define hwmon channel (Guenter)
v4:
   - Fixed review comments (Ashutosh)
   - Use hwm_ prefix for static functions (Ashutosh)

Cc: Guenter Roeck 
Cc: Anshuman Gupta 
Signed-off-by: Riana Tauro 
Signed-off-by: Badal Nilawar 


Acked-by: Guenter Roeck 


---
  .../ABI/testing/sysfs-driver-intel-i915-hwmon |  7 +++
  drivers/gpu/drm/i915/gt/intel_gt_regs.h   |  3 ++
  drivers/gpu/drm/i915/i915_hwmon.c | 47 +++
  3 files changed, 57 insertions(+)
  create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon 
b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
new file mode 100644
index ..24c4b7477d51
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
@@ -0,0 +1,7 @@
+What:  /sys/devices/.../hwmon/hwmon/in0_input
+Date:  June 2022
+KernelVersion: 5.19
+Contact:   dri-de...@lists.freedesktop.org
+Description:   RO. Current Voltage in millivolt.
+
+   Only supported for particular Intel i915 graphics platforms.
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h 
b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
index b3b49f6d6d1c..4604f6dbf8b6 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
@@ -1498,6 +1498,9 @@
  #define VLV_RENDER_C0_COUNT   _MMIO(0x138118)
  #define VLV_MEDIA_C0_COUNT_MMIO(0x13811c)
  
+#define GEN12_RPSTAT1_MMIO(0x1381b4)

+#define   GEN12_VOLTAGE_MASK   REG_GENMASK(10, 0)
+
  #define GEN11_GT_INTR_DW(x)   _MMIO(0x190018 + ((x) * 4))
  #define   GEN11_CSME  (31)
  #define   GEN11_GUNIT (28)
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c 
b/drivers/gpu/drm/i915/i915_hwmon.c
index 5b80a0f024f0..1893efe796a4 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -10,8 +10,10 @@
  #include "i915_drv.h"
  #include "i915_hwmon.h"
  #include "intel_mchbar_regs.h"
+#include "gt/intel_gt_regs.h"
  
  struct hwm_reg {

+   i915_reg_t gt_perf_status;
  };
  
  struct hwm_drvdata {

@@ -28,14 +30,49 @@ struct i915_hwmon {
  };
  
  static const struct hwmon_channel_info *hwm_info[] = {

+   HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
NULL
  };
  
+static umode_t

+hwm_in_is_visible(const struct hwm_drvdata *ddat, u32 attr)
+{
+   switch (attr) {
+   case hwmon_in_input:
+   return i915_mmio_reg_valid(ddat->hwmon->rg.gt_perf_status) ? 
0444 : 0;
+   default:
+   return 0;
+   }
+}
+
+static int
+hwm_in_read(struct hwm_drvdata *ddat, u32 attr, long *val)
+{
+   struct i915_hwmon *hwmon = ddat->hwmon;
+   intel_wakeref_t wakeref;
+   u32 reg_value;
+
+   switch (attr) {
+   case hwmon_in_input:
+   with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
+   reg_value = intel_uncore_read(ddat->uncore, 
hwmon->rg.gt_perf_status);
+   /* In units of 2.5 millivolt */
+   *val = DIV_ROUND_CLOSEST(REG_FIELD_GET(GEN12_VOLTAGE_MASK, 
reg_value) * 25, 10);
+   return 0;
+   default:
+   return -EOPNOTSUPP;
+   }
+}
+
  static umode_t
  hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
   u32 attr, int channel)
  {
+   struct hwm_drvdata *ddat = (struct hwm_drvdata *)drvdata;
+
switch (type) {
+   case hwmon_in:
+   return hwm_in_is_visible(ddat, attr);
default:
return 0;
}
@@ -45,7 +82,11 @@ static int
  hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 int channel, long *val)
  {
+   struct hwm_drvdata *ddat = dev_get_drvdata(dev);
+
switch (type) {
+   case hwmon_in:
+   return hwm_in_read(ddat, attr, val);
default:
return -EOPNOTSUPP;
}
@@ -75,6 +116,12 @@ static const struct hwmon_chip_info hwm_chip_info = {
  static void
  hwm_get_preregistration_info(struct drm_i915_private *i915)
  {
+   struct i915_hwmon *hwmon = i915->hwmon;
+
+   if (IS_DG1(i915) || IS_DG2(i915))
+   hwmon->rg.gt_perf_status = GEN12_RPSTAT1;
+   else
+   hwmon->rg.gt_perf_status = INVALID_MMIO_REG;
  }
  
  void i915_hwmon_register(struct drm_i915_private *i915)

Re: [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure

[Guenter Roeck]

On 8/12/22 10:37, Badal Nilawar wrote:

From: Dale B Stimson 

The i915 HWMON module will be used to expose voltage, power and energy
values for dGfx. Here we set up i915 hwmon infrastructure including i915
hwmon registration, basic data structures and functions.

v2:
   - Create HWMON infra patch (Ashutosh)
   - Fixed review comments (Jani)
   - Remove "select HWMON" from i915/Kconfig (Jani)
v3: Use hwm_ prefix for static functions (Ashutosh)

Signed-off-by: Dale B Stimson 
Signed-off-by: Ashutosh Dixit 
Signed-off-by: Riana Tauro 
Signed-off-by: Badal Nilawar 
---
  drivers/gpu/drm/i915/Makefile  |   3 +
  drivers/gpu/drm/i915/i915_driver.c |   7 ++
  drivers/gpu/drm/i915/i915_drv.h|   2 +
  drivers/gpu/drm/i915/i915_hwmon.c  | 135 +
  drivers/gpu/drm/i915/i915_hwmon.h  |  20 +
  5 files changed, 167 insertions(+)
  create mode 100644 drivers/gpu/drm/i915/i915_hwmon.c
  create mode 100644 drivers/gpu/drm/i915/i915_hwmon.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 522ef9b4aff3..2b235f747490 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -208,6 +208,9 @@ i915-y += gt/uc/intel_uc.o \
  # graphics system controller (GSC) support
  i915-y += gt/intel_gsc.o
  
+# graphics hardware monitoring (HWMON) support

+i915-$(CONFIG_HWMON) += i915_hwmon.o
+
  # modesetting core code
  i915-y += \
display/hsw_ips.o \
diff --git a/drivers/gpu/drm/i915/i915_driver.c 
b/drivers/gpu/drm/i915/i915_driver.c
index deb8a8b76965..949908dd7496 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -80,6 +80,7 @@
  #include "i915_drm_client.h"
  #include "i915_drv.h"
  #include "i915_getparam.h"
+#include "i915_hwmon.h"
  #include "i915_ioc32.h"
  #include "i915_ioctl.h"
  #include "i915_irq.h"
@@ -736,6 +737,9 @@ static void i915_driver_register(struct drm_i915_private 
*dev_priv)
  
  	intel_gt_driver_register(to_gt(dev_priv));
  
+#ifdef CONFIG_HWMON

+   i915_hwmon_register(dev_priv);
+#endif
intel_display_driver_register(dev_priv);
  
  	intel_power_domains_enable(dev_priv);

@@ -762,6 +766,9 @@ static void i915_driver_unregister(struct drm_i915_private 
*dev_priv)
  
  	intel_display_driver_unregister(dev_priv);
  
+#ifdef CONFIG_HWMON


IS_REACHABLE() might be more appropriate. Otherwise this won't be included
if HWMON=m. An alternate approach might be to have dummy functions if the
hwmon code isn't rechable to avoid conditional code, but that is really
personal preference.


+   i915_hwmon_unregister(dev_priv);
+#endif
intel_gt_driver_unregister(to_gt(dev_priv));
  
  	i915_perf_unregister(dev_priv);

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 086bbe8945d6..d437d588dec9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -705,6 +705,8 @@ struct drm_i915_private {
  
  	struct i915_perf perf;
  
+	struct i915_hwmon *hwmon;

+
/* Abstract the submission mechanism (legacy ringbuffer or execlists) 
away */
struct intel_gt gt0;
  
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c

new file mode 100644
index ..5b80a0f024f0
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include 
+#include 
+#include 
+
+#include "i915_drv.h"
+#include "i915_hwmon.h"
+#include "intel_mchbar_regs.h"
+
+struct hwm_reg {
+};
+
+struct hwm_drvdata {
+   struct i915_hwmon *hwmon;
+   struct intel_uncore *uncore;
+   struct device *hwmon_dev;
+   char name[12];
+};
+
+struct i915_hwmon {
+   struct hwm_drvdata ddat;
+   struct mutex hwmon_lock;/* counter overflow logic and 
rmw */
+   struct hwm_reg rg;
+};
+
+static const struct hwmon_channel_info *hwm_info[] = {
+   NULL
+};
+
+static umode_t
+hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
+  u32 attr, int channel)
+{
+   switch (type) {
+   default:
+   return 0;
+   }
+}
+
+static int
+hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+int channel, long *val)
+{
+   switch (type) {
+   default:
+   return -EOPNOTSUPP;
+   }
+}
+
+static int
+hwm_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+ int channel, long val)
+{
+   switch (type) {
+   default:
+   return -EOPNOTSUPP;
+   }
+}
+
+static const struct hwmon_ops hwm_ops = {
+   .is_visible = hwm_is_visible,
+   .read = hwm_read,
+   .write = hwm_write,
+};
+
+static const struct hwmon_chip_info hwm_chip_info = {
+   .ops = _ops,
+   .info = hwm_info,
+};
+
+static void
+hwm_get_preregistration_info(struct drm_i915_private *i915)
+{
+}
+
+void i915_hwmon_register(struct drm_i915_private *i915)
+{
+ 

Re: [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915: Fix 32-bit build

[Guenter Roeck]

On 7/17/22 09:35, Patchwork wrote:

== Series Details ==

Series: drm/i915: Fix 32-bit build
URL   : https://patchwork.freedesktop.org/series/106421/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2




Failure is unrelated to this patch. No problem seen with ToT sparse.

Guenter

[Intel-gfx] [PATCH] drm/i915: Fix 32-bit build

[Guenter Roeck]

Commit aff1e0b09b54 ("drm/i915/ttm: fix sg_table construction") introduces
an additional parameter to i915_rsgt_from_mm_node(). The parameter is used
to calculate segment_pages. This in turn is used in DIV_ROUND_UP() as
divisor. So far segment_pages was a constant and handled without divide
operation. Since it is no longer constant, a divide operation is now
necessary. This results in build errors on 32-bit builds.

x86_64-linux-ld: drivers/gpu/drm/i915/i915_scatterlist.o:
in function `i915_rsgt_from_mm_node':
i915_scatterlist.c:(.text+0x196): undefined reference to `__udivdi3'

Fix the problem by using DIV_ROUND_UP_ULL() instead of DIV_ROUND_UP().

Fixes: aff1e0b09b54 ("drm/i915/ttm: fix sg_table construction")
Cc: Matthew Auld 
Cc: Nirmoy Das 
Cc: Rodrigo Vivi 
Signed-off-by: Guenter Roeck 
---
I took a stab at the problem. Please ignore the noise if it has already
been fixed with a different patch.

 drivers/gpu/drm/i915/i915_scatterlist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_scatterlist.c 
b/drivers/gpu/drm/i915/i915_scatterlist.c
index f63b50b71e10..b81d5658c222 100644
--- a/drivers/gpu/drm/i915/i915_scatterlist.c
+++ b/drivers/gpu/drm/i915/i915_scatterlist.c
@@ -96,7 +96,7 @@ struct i915_refct_sgt *i915_rsgt_from_mm_node(const struct 
drm_mm_node *node,
 
i915_refct_sgt_init(rsgt, node->size << PAGE_SHIFT);
st = >table;
-   if (sg_alloc_table(st, DIV_ROUND_UP(node->size, segment_pages),
+   if (sg_alloc_table(st, DIV_ROUND_UP_ULL(node->size, segment_pages),
   GFP_KERNEL)) {
i915_refct_sgt_put(rsgt);
return ERR_PTR(-ENOMEM);
-- 
2.36.2

Re: [Intel-gfx] [v3] drm/i915/ttm: fix sg_table construction

[Guenter Roeck]

On Mon, Jul 11, 2022 at 09:58:59AM +0100, Matthew Auld wrote:
> If we encounter some monster sized local-memory page that exceeds the
> maximum sg length (UINT32_MAX), ensure that don't end up with some
> misaligned address in the entry that follows, leading to fireworks
> later. Also ensure we have some coverage of this in the selftests.
> 
> v2(Chris):
>   - Use round_down consistently to avoid udiv errors
> v3(Nirmoy):
>   - Also update the max_segment in the selftest
> 
> Fixes: f701b16d4cc5 ("drm/i915/ttm: add i915_sg_from_buddy_resource")
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6379
> Signed-off-by: Matthew Auld 
> Cc: Thomas Hellström 
> Cc: Nirmoy Das 
> Reviewed-by: Nirmoy Das 

Building i386:defconfig ... failed
--
Error log:
x86_64-linux-ld: drivers/gpu/drm/i915/i915_scatterlist.o: in function 
`i915_rsgt_from_mm_node':
i915_scatterlist.c:(.text+0x196): undefined reference to `__udivdi3'

Bisect log attached.

Guenter

---
# bad: [9b59ec8d50a1f28747ceff9a4f39af5deba9540e] Merge tag 
'riscv-for-linus-5.19-rc7' of 
git://git.kernel.org/pub/scm/linux/kernel/git/riscv/linux
# good: [e5d523f1ae8f2cef01f8e071aeee432654166708] ubsan: disable 
UBSAN_DIV_ZERO for clang
git bisect start 'HEAD' 'e5d523f1ae8f'
# bad: [2a347a06ebb1b186a5cb919c9f5ab6e040554be7] Merge tag 
'platform-drivers-x86-v5.19-4' of 
git://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86
git bisect bad 2a347a06ebb1b186a5cb919c9f5ab6e040554be7
# bad: [093f8d8f10aa22935bc8bf7100700f714ebaba9c] Merge tag 
'amd-drm-fixes-5.19-2022-07-13' of https://gitlab.freedesktop.org/agd5f/linux 
into drm-fixes
git bisect bad 093f8d8f10aa22935bc8bf7100700f714ebaba9c
# bad: [ad765fae792e16ce3c1d0b69ce939e3f7dba40ab] drm/i915/gem: Look for 
waitboosting across the whole object prior to individual waits
git bisect bad ad765fae792e16ce3c1d0b69ce939e3f7dba40ab
# good: [f99546298a4537965b75d518c210742f641be389] Merge tag 
'gvt-fixes-2022-07-11' of https://github.com/intel/gvt-linux into 
drm-intel-fixes
git bisect good f99546298a4537965b75d518c210742f641be389
# bad: [aff1e0b09b54b64944b7fe32997229552737b9e9] drm/i915/ttm: fix sg_table 
construction
git bisect bad aff1e0b09b54b64944b7fe32997229552737b9e9
# good: [896dcabd1f8f613c533d948df17408c41f8929f5] drm/i915/selftests: fix a 
couple IS_ERR() vs NULL tests
git bisect good 896dcabd1f8f613c533d948df17408c41f8929f5
# first bad commit: [aff1e0b09b54b64944b7fe32997229552737b9e9] drm/i915/ttm: 
fix sg_table construction

Re: [Intel-gfx] [PATCH 3/4] drm/i915/hwmon: Add HWMON power sensor support

[Guenter Roeck]

On Tue, Jun 21, 2022 at 12:29:21PM -0700, Dixit, Ashutosh wrote:
> On Tue, 21 Jun 2022 10:44:21 -0700, Guenter Roeck wrote:
> >
> > On Mon, Jun 20, 2022 at 11:41:41PM -0700, Dixit, Ashutosh wrote:
> > > On Mon, 20 Jun 2022 13:58:49 -0700, Guenter Roeck wrote:
> > > Hi Guenter, Thanks for taking a look.
> > >
> > > > > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon 
> > > > > b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > > > index 24c4b7477d51..945f472dd4a2 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > > > +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > > > @@ -5,3 +5,23 @@ Contact: dri-de...@lists.freedesktop.org
> > > > >   Description:RO. Current Voltage in millivolt.
> > > > >   Only supported for particular Intel i915 
> > > > > graphics
> > > > > platforms.
> > > > > +
> > > > > +What:/sys/devices/.../hwmon/hwmon/power1_max
> > > > > +Date:June 2022
> > > > > +KernelVersion:   5.19
> > > > > +Contact: dri-de...@lists.freedesktop.org
> > > > > +Description: RW. Card reactive sustained  (PL1/Tau) power limit in 
> > > > > microwatts.
> > > > > +
> > > > > + The power controller will throttle the operating 
> > > > > frequency
> > > > > + if the power averaged over a window (typically seconds)
> > > > > + exceeds this limit.
> > > > > +
> > > > > + Only supported for particular Intel i915 graphics 
> > > > > platforms.
> > > > > +
> > > > > +What:
> > > > > /sys/devices/.../hwmon/hwmon/power1_max_default
> > > >
> > > > I don't immediately see the reason for not using the standard power1_cap
> > > > attribute, which is described as
> > > >
> > > > If power use rises above this limit, the
> > > > system should take action to reduce power use.
> > > >
> > > > and pretty much matches the description above.
> > >
> > > Sorry I believe you are referring to the description above which is for 
> > > the
> > > standard power1_max attribute (as we have used it). The non-standard
> > > attribute is power1_max_default the description for which is below ("Card
> > > default power limit (default TDP setting)").
> > >
> >
> > If you use power1_max to limit power consumption if exceeded, power1_cap
> > might have been more appropriate.
> 
> Looks like in this case the file name is ok but the problem is with the
> description (which does not correspond to what PL1 is). Will fix.
> 
> > > > > +Date:June 2022
> > > > > +KernelVersion:   5.19
> > > > > +Contact: dri-de...@lists.freedesktop.org
> > > > > +Description: RO. Card default power limit (default TDP setting).
> > >
> > > Actually we do not want to use custom hwmon attributes as far as
> > > possible and are looking for some guidance on which standard attributes
> > > which we should use instead.
> > >
> > You could possibly have used power1_rated_max instead of power1_max_default.
> 
> Yes looks like this might work for TDP. We will consider this.
> 
> > > These are the power attributes we are interested in: the two above and
> > > another one which will come in a future patch:
> > >
> > > 1. PL1 (RW)
> > >
> > >
> > > https://www.hardwaretimes.com/intel-10th-gen-cpu-power-consumption-explained-pl1-pl2-and-tau/
> > >
> > > 2. TDP (RO)
> > >
> > >https://en.wikipedia.org/wiki/Thermal_design_power
> > >
> > Not sure I understand the difference between 'default TDP' (RO),
> > 'TDP' (RO), and PL1.
> 
> 'default TDP' (RO) and 'TDP' (RO) are the same but PL1 is somewhat
> different from TDP (see the first link above) and also I believe chip
> makers specify PL1 and TDP separately (as in this case).
> 
> > > 3. Tau (RW)
> > >
> > >
> > > https://www.hardwaretimes.com/intel-10th-gen-cpu-power-consumption-explained-pl1-pl2-and-tau/
> > >
> > > Would you be able to suggest if there are standard hwmon attributes which
> > > we would be able to use for these three? We also want to use the 
> > > read/write
> > > permissions as shown above.
> > >
> >
> > There are a number of standard power attributes available to set or report
> > limits (cap, cap_max, cap_min, max, crit, rated_min, rated_max). I would
> > suggest to pick from that list whatever you think fits best. I don't have
> > a recommendation for Tau.
> 
> OK, in that case would a custom setting for Tau also be ok?
> 
Yes.

> > Either case, when reporting power, make sure you don't hit any of the
> > security issues which caused power reporting to be deleted for other CPUs.
> > Restricting read access to hwmon attributes for non-privileged users is not
> > acceptable.
> 
> OK, I believe you are referring to 9049572fb145. Will keep this in mind too.
> 

Correct. Something similar is in the works for another architecture,
for the same reason. Also see 'Attribute access' in
Documentation/hwmon/sysfs-interface.rst.

Thanks,
Guenter

Re: [Intel-gfx] [PATCH 3/4] drm/i915/hwmon: Add HWMON power sensor support

[Guenter Roeck]

On Mon, Jun 20, 2022 at 11:41:41PM -0700, Dixit, Ashutosh wrote:
> On Mon, 20 Jun 2022 13:58:49 -0700, Guenter Roeck wrote:
> >
> 
> Hi Guenter, Thanks for taking a look.
> 
> > > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon 
> > > b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > index 24c4b7477d51..945f472dd4a2 100644
> > > --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > @@ -5,3 +5,23 @@ Contact: dri-de...@lists.freedesktop.org
> > >   Description:RO. Current Voltage in millivolt.
> > >   Only supported for particular Intel i915 graphics
> > > platforms.
> > > +
> > > +What:/sys/devices/.../hwmon/hwmon/power1_max
> > > +Date:June 2022
> > > +KernelVersion:   5.19
> > > +Contact: dri-de...@lists.freedesktop.org
> > > +Description: RW. Card reactive sustained  (PL1/Tau) power limit in 
> > > microwatts.
> > > +
> > > + The power controller will throttle the operating frequency
> > > + if the power averaged over a window (typically seconds)
> > > + exceeds this limit.
> > > +
> > > + Only supported for particular Intel i915 graphics platforms.
> > > +
> > > +What:/sys/devices/.../hwmon/hwmon/power1_max_default
> >
> > I don't immediately see the reason for not using the standard power1_cap
> > attribute, which is described as
> >
> > If power use rises above this limit, the
> > system should take action to reduce power use.
> >
> > and pretty much matches the description above.
> 
> Sorry I believe you are referring to the description above which is for the
> standard power1_max attribute (as we have used it). The non-standard
> attribute is power1_max_default the description for which is below ("Card
> default power limit (default TDP setting)").
> 

If you use power1_max to limit power consumption if exceeded, power1_cap
might have been more appropriate.

> > > +Date:June 2022
> > > +KernelVersion:   5.19
> > > +Contact: dri-de...@lists.freedesktop.org
> > > +Description: RO. Card default power limit (default TDP setting).
> 
> Actually we do not want to use custom hwmon attributes as far as
> possible and are looking for some guidance on which standard attributes
> which we should use instead.
> 
You could possibly have used power1_rated_max instead of power1_max_default.

> These are the power attributes we are interested in: the two above and
> another one which will come in a future patch:
> 
> 1. PL1 (RW)
> 
>
> https://www.hardwaretimes.com/intel-10th-gen-cpu-power-consumption-explained-pl1-pl2-and-tau/
> 
> 2. TDP (RO)
> 
>https://en.wikipedia.org/wiki/Thermal_design_power
> 
Not sure I understand the difference between 'default TDP' (RO),
'TDP' (RO), and PL1.

> 3. Tau (RW)
> 
>
> https://www.hardwaretimes.com/intel-10th-gen-cpu-power-consumption-explained-pl1-pl2-and-tau/
> 
> Would you be able to suggest if there are standard hwmon attributes which
> we would be able to use for these three? We also want to use the read/write
> permissions as shown above.
> 

There are a number of standard power attributes available to set or report
limits (cap, cap_max, cap_min, max, crit, rated_min, rated_max). I would
suggest to pick from that list whatever you think fits best. I don't have
a recommendation for Tau.

Either case, when reporting power, make sure you don't hit any of the
security issues which caused power reporting to be deleted for other CPUs.
Restricting read access to hwmon attributes for non-privileged users is not
acceptable.

Thanks,
Guenter

Re: [Intel-gfx] [PATCH 2/4] drm/i915/hwmon: Add HWMON current voltage support

[Guenter Roeck]

On 6/20/22 13:46, Badal Nilawar wrote:

From: Riana Tauro 

As part of the System Managemenent Interface (SMI), use the HWMON
subsystem to display current voltage

v2:
   - Updated date and kernel version in feature description
   - Fixed review comments (Ashutosh)

Cc: Anshuman Gupta 
Signed-off-by: Riana Tauro 
Signed-off-by: Badal Nilawar 
---
  .../ABI/testing/sysfs-driver-intel-i915-hwmon |  7 +++
  drivers/gpu/drm/i915/gt/intel_gt_regs.h   |  3 +
  drivers/gpu/drm/i915/i915_hwmon.c | 63 +++
  3 files changed, 73 insertions(+)
  create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon 
b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
new file mode 100644
index ..24c4b7477d51
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
@@ -0,0 +1,7 @@
+What:  /sys/devices/.../hwmon/hwmon/in0_input
+Date:  June 2022
+KernelVersion: 5.19
+Contact:   dri-de...@lists.freedesktop.org
+Description:   RO. Current Voltage in millivolt.
+
+   Only supported for particular Intel i915 graphics platforms.
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h 
b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
index 07ef111947b8..63a39e1e00e2 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
@@ -1487,6 +1487,9 @@
  #define VLV_RENDER_C0_COUNT   _MMIO(0x138118)
  #define VLV_MEDIA_C0_COUNT_MMIO(0x13811c)
  
+#define GEN12_RPSTAT1_MMIO(0x1381b4)

+#define   GEN12_VOLTAGE_MASK   REG_GENMASK(10, 0)
+
  #define GEN11_GT_INTR_DW(x)   _MMIO(0x190018 + ((x) * 4))
  #define   GEN11_CSME  (31)
  #define   GEN11_GUNIT (28)
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c 
b/drivers/gpu/drm/i915/i915_hwmon.c
index 2ef40b0c1e70..fc06db790243 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -14,9 +14,11 @@
  #include "i915_hwmon.h"
  #include "i915_drv.h"
  #include "intel_mchbar_regs.h"
+#include "gt/intel_gt_regs.h"
  
  
  struct i915_hwmon_reg {

+   i915_reg_t gt_perf_status;
  };
  
  struct i915_hwmon_drvdata {

@@ -53,15 +55,65 @@ static const struct attribute_group *hwmon_groups[] = {
  };
  
  
+/*

+ * HWMON SENSOR TYPE = hwmon_in
+ *  - Voltage Input value (in0_input)
+ */
+static const u32 i915_config_in[] = {
+   HWMON_I_INPUT,
+   0
+};
+
+static const struct hwmon_channel_info i915_in = {
+   .type = hwmon_in,
+   .config = i915_config_in,
+};
+
  static const struct hwmon_channel_info *i915_info[] = {
+   _in,
NULL
  };


Please use the HWMON_CHANNEL_INFO macro.

Thanks,
Guenter

  
+static umode_t

+i915_in_is_visible(const struct i915_hwmon_drvdata *ddat, u32 attr)
+{
+   struct drm_i915_private *i915 = ddat->uncore->i915;
+
+   switch (attr) {
+   case hwmon_in_input:
+   return (IS_DG1(i915) || IS_DG2(i915)) ? 0444 : 0;
+   default:
+   return 0;
+   }
+}
+
+static int
+i915_in_read(struct i915_hwmon_drvdata *ddat, u32 attr, long *val)
+{
+   struct i915_hwmon *hwmon = ddat->hwmon;
+   intel_wakeref_t wakeref;
+   u32 reg_value;
+
+   switch (attr) {
+   case hwmon_in_input:
+   with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
+   reg_value = intel_uncore_read(ddat->uncore, 
hwmon->rg.gt_perf_status);
+   *val = DIV_ROUND_CLOSEST(REG_FIELD_GET(GEN12_VOLTAGE_MASK, 
reg_value) * 25, 10);
+   return 0;
+   default:
+   return -EOPNOTSUPP;
+   }
+}
+
  static umode_t
  i915_is_visible(const void *drvdata, enum hwmon_sensor_types type,
u32 attr, int channel)
  {
+   struct i915_hwmon_drvdata *ddat = (struct i915_hwmon_drvdata *)drvdata;
+
switch (type) {
+   case hwmon_in:
+   return i915_in_is_visible(ddat, attr);
default:
return 0;
}
@@ -71,7 +123,11 @@ static int
  i915_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
  int channel, long *val)
  {
+   struct i915_hwmon_drvdata *ddat = dev_get_drvdata(dev);
+
switch (type) {
+   case hwmon_in:
+   return i915_in_read(ddat, attr, val);
default:
return -EOPNOTSUPP;
}
@@ -101,6 +157,13 @@ static const struct hwmon_chip_info i915_chip_info = {
  static void
  i915_hwmon_get_preregistration_info(struct drm_i915_private *i915)
  {
+   struct i915_hwmon *hwmon = i915->hwmon;
+
+   if (IS_DG1(i915) || IS_DG2(i915)) {
+   hwmon->rg.gt_perf_status = GEN12_RPSTAT1;
+   } else {
+   hwmon->rg.gt_perf_status = INVALID_MMIO_REG;
+   }
  
  }
  

Re: [Intel-gfx] [PATCH 4/4] drm/i915/hwmon: Add HWMON energy support

[Guenter Roeck]

On 6/20/22 13:46, Badal Nilawar wrote:

From: Dale B Stimson 

As part of the System Managemenent Interface (SMI), use the HWMON
subsystem to display energy utilization

v2:
   - Updated the date and kernel version in feature description

Signed-off-by: Dale B Stimson 
Signed-off-by: Ashutosh Dixit 
Signed-off-by: Riana Tauro 
Signed-off-by: Badal Nilawar 
---
  .../ABI/testing/sysfs-driver-intel-i915-hwmon |  16 ++
  drivers/gpu/drm/i915/i915_hwmon.c | 174 +-
  drivers/gpu/drm/i915/i915_hwmon.h |   1 +
  drivers/gpu/drm/i915/intel_mchbar_regs.h  |   2 +
  4 files changed, 185 insertions(+), 8 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon 
b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
index 945f472dd4a2..2e87d7422b73 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
@@ -25,3 +25,19 @@ Contact: dri-de...@lists.freedesktop.org
  Description:  RO. Card default power limit (default TDP setting).
  
  		Only supported for particular Intel i915 graphics platforms.

+
+What:  /sys/devices/.../hwmon/hwmon/energy1_input
+Date:  June 2022
+KernelVersion: 5.19
+Contact:   dri-de...@lists.freedesktop.org
+Description:   RO. Energy input of device in microjoules.
+
+   The returned textual representation is an unsigned integer
+   number that can be stored in 64-bits.  Warning: The hardware
+   register is 32-bits wide and can overflow by wrapping around.
+   A single wrap-around between calls to read this value can
+   be detected and will be accounted for in the returned value.
+   At a power consumption of 1 watt, the 32-bit hardware register
+   would wrap-around approximately every 3 days.
+
+   Only supported for particular Intel i915 graphics platforms.
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c 
b/drivers/gpu/drm/i915/i915_hwmon.c
index 75935a55f573..77d68f17316a 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -19,8 +19,10 @@
  /*
   * SF_* - scale factors for particular quantities according to hwmon spec.
   * - power  - microwatts
+ * - energy - microjoules
   */
  #define SF_POWER  100
+#define SF_ENERGY  100
  
  #define FIELD_SHIFT(__mask)\

(BUILD_BUG_ON_ZERO(!__builtin_constant_p(__mask)) + \
@@ -32,12 +34,20 @@ struct i915_hwmon_reg {
i915_reg_t pkg_power_sku_unit;
i915_reg_t pkg_power_sku;
i915_reg_t pkg_rapl_limit;
+   i915_reg_t energy_status_all;
+   i915_reg_t energy_status_tile;
+};
+
+struct i915_energy_info {
+   u32 energy_counter_overflow;
+   u32 energy_counter_prev;
  };
  
  struct i915_hwmon_drvdata {

struct i915_hwmon *hwmon;
struct intel_uncore *uncore;
struct device *hwmon_dev;
+   struct i915_energy_info ei; /*  Energy info for energy1_input */
char name[12];
  };
  
@@ -51,6 +61,7 @@ struct i915_hwmon {

u32 power_max_initial_value;
  
  	int scl_shift_power;

+   int scl_shift_energy;
  };
  
  static void

@@ -121,6 +132,136 @@ _field_scale_and_write(struct i915_hwmon_drvdata *ddat, 
i915_reg_t rgadr,
 bits_to_clear, bits_to_set);
  }
  
+/*

+ * _i915_energy1_input_sub - A custom function to obtain energy1_input.
+ * Use a custom function instead of the usual hwmon helpers in order to
+ * guarantee 64-bits of result to user-space.
+ * Units are microjoules.
+ *
+ * The underlying hardware register is 32-bits and is subject to overflow.
+ * This function compensates for overflow of the 32-bit register by detecting
+ * wrap-around and incrementing an overflow counter.
+ * This only works if the register is sampled often enough to avoid
+ * missing an instance of overflow - achieved either by repeated
+ * queries through the API, or via a possible timer (future - TBD) that
+ * ensures values are read often enough to catch all overflows.
+ *
+ * How long before overflow?  For example, with an example scaling bit
+ * shift of 14 bits (see register *PACKAGE_POWER_SKU_UNIT) and a power draw of
+ * 1000 watts, the 32-bit counter will overflow in approximately 4.36 minutes.
+ *
+ * Examples:
+ *1 watt:  (2^32 >> 14) /1 W / (60 * 60 * 24) secs/day -> 3 days
+ * 1000 watts: (2^32 >> 14) / 1000 W / 60 secs/min -> 4.36 minutes
+ */
+static int
+_i915_energy1_input_sub(struct i915_hwmon_drvdata *ddat, u64 *energy)
+{
+   struct intel_uncore *uncore = ddat->uncore;
+   struct i915_hwmon *hwmon = ddat->hwmon;
+   struct i915_energy_info *pei = >ei;
+   int nshift = hwmon->scl_shift_energy;
+   intel_wakeref_t wakeref;
+   u32 reg_value;
+   u64 vlo;
+   u64 vhi;
+   i915_reg_t rgaddr;
+
+   rgaddr = hwmon->rg.energy_status_all;
+
+   if 

Re: [Intel-gfx] [PATCH 3/4] drm/i915/hwmon: Add HWMON power sensor support

[Guenter Roeck]

On 6/20/22 13:46, Badal Nilawar wrote:

From: Dale B Stimson 

As part of the System Managemenent Interface (SMI), use the HWMON
subsystem to display power utilization.

v2:
   - Fix review comments (Ashutosh)
   - Do not restore power1_max upon module unload/load sequence
 because on production systems modules are always loaded
 and not unloaded/reloaded (Ashutosh)
   - Fix review comments (Jani)
   - Remove endianness conversion (Ashutosh)

Signed-off-by: Dale B Stimson 
Signed-off-by: Ashutosh Dixit 
Signed-off-by: Riana Tauro 
Signed-off-by: Badal Nilawar 
---
  .../ABI/testing/sysfs-driver-intel-i915-hwmon |  20 ++
  drivers/gpu/drm/i915/i915_hwmon.c | 226 +-
  drivers/gpu/drm/i915/i915_reg.h   |  15 ++
  drivers/gpu/drm/i915/intel_mchbar_regs.h  |   7 +
  4 files changed, 267 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon 
b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
index 24c4b7477d51..945f472dd4a2 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
@@ -5,3 +5,23 @@ Contact:   dri-de...@lists.freedesktop.org
  Description:  RO. Current Voltage in millivolt.
  
  		Only supported for particular Intel i915 graphics platforms.

+
+What:  /sys/devices/.../hwmon/hwmon/power1_max
+Date:  June 2022
+KernelVersion: 5.19
+Contact:   dri-de...@lists.freedesktop.org
+Description:   RW. Card reactive sustained  (PL1/Tau) power limit in 
microwatts.
+
+   The power controller will throttle the operating frequency
+   if the power averaged over a window (typically seconds)
+   exceeds this limit.
+
+   Only supported for particular Intel i915 graphics platforms.
+
+What:  /sys/devices/.../hwmon/hwmon/power1_max_default


I don't immediately see the reason for not using the standard power1_cap
attribute, which is described as

If power use rises above this limit, the
system should take action to reduce power use.

and pretty much matches the description above.


+Date:  June 2022
+KernelVersion: 5.19
+Contact:   dri-de...@lists.freedesktop.org
+Description:   RO. Card default power limit (default TDP setting).
+
+   Only supported for particular Intel i915 graphics platforms.
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c 
b/drivers/gpu/drm/i915/i915_hwmon.c
index fc06db790243..75935a55f573 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -16,9 +16,22 @@
  #include "intel_mchbar_regs.h"
  #include "gt/intel_gt_regs.h"
  
+/*

+ * SF_* - scale factors for particular quantities according to hwmon spec.
+ * - power  - microwatts
+ */
+#define SF_POWER   100
+
+#define FIELD_SHIFT(__mask)\
+   (BUILD_BUG_ON_ZERO(!__builtin_constant_p(__mask)) + \
+   BUILD_BUG_ON_ZERO((__mask) == 0) +  \
+   __bf_shf(__mask))
  
  struct i915_hwmon_reg {

i915_reg_t gt_perf_status;
+   i915_reg_t pkg_power_sku_unit;
+   i915_reg_t pkg_power_sku;
+   i915_reg_t pkg_rapl_limit;
  };
  
  struct i915_hwmon_drvdata {

@@ -30,18 +43,127 @@ struct i915_hwmon_drvdata {
  
  struct i915_hwmon {

struct i915_hwmon_drvdata ddat;
+
struct mutex hwmon_lock;/* counter overflow logic and rmw */
+
struct i915_hwmon_reg rg;
+
+   u32 power_max_initial_value;
+
+   int scl_shift_power;
  };
  
+static void

+_locked_with_pm_intel_uncore_rmw(struct i915_hwmon_drvdata *ddat,
+i915_reg_t reg, u32 clear, u32 set)
+{
+   struct i915_hwmon *hwmon = ddat->hwmon;
+   struct intel_uncore *uncore = ddat->uncore;
+   intel_wakeref_t wakeref;
+
+   mutex_lock(>hwmon_lock);
+
+   with_intel_runtime_pm(uncore->rpm, wakeref)
+   intel_uncore_rmw(uncore, reg, clear, set);
+
+   mutex_unlock(>hwmon_lock);
+}
+
+static u64
+_scale_and_shift(u32 in, u32 scale_factor, int nshift)
+{
+   u64 out = mul_u32_u32(scale_factor, in);
+
+   /* Shift, rounding to nearest */
+   if (nshift > 0)
+   out = (out + (1 << (nshift - 1))) >> nshift;
+   return out;
+}
+
+/*
+ * This function's return type of u64 allows for the case where the scaling
+ * of the field taken from the 32-bit register value might cause a result to
+ * exceed 32 bits.
+ */
+static u64
+_field_read_and_scale(struct i915_hwmon_drvdata *ddat, i915_reg_t rgadr,
+ u32 field_msk, int field_shift,
+ int nshift, u32 scale_factor)
+{
+   struct intel_uncore *uncore = ddat->uncore;
+   intel_wakeref_t wakeref;
+   u32 reg_value;
+
+   with_intel_runtime_pm(uncore->rpm, wakeref)
+   reg_value = intel_uncore_read(uncore, rgadr);
+
+   reg_value = 

Re: [Intel-gfx] [greybus-dev] Re: [PATCH] [v2] Kbuild: move to -std=gnu11

[Guenter Roeck]

On 5/18/22 00:46, Arnd Bergmann wrote:

On Mon, May 16, 2022 at 3:19 PM Guenter Roeck  wrote:

On 5/16/22 06:31, Greg KH wrote:

On Mon, May 16, 2022 at 06:10:23AM -0700, Guenter Roeck wrote:

On Mon, Feb 28, 2022 at 11:27:43AM +0100, Arnd Bergmann wrote:

From: Arnd Bergmann 

During a patch discussion, Linus brought up the option of changing
the C standard version from gnu89 to gnu99, which allows using variable
declaration inside of a for() loop. While the C99, C11 and later standards
introduce many other features, most of these are already available in
gnu89 as GNU extensions as well.


The downside is that backporting affected patches to older kernel branches
now fails with error messages such as

mm/kfence/core.c: In function ‘kfence_init_pool’:
mm/kfence/core.c:595:2: error: ‘for’ loop initial declarations are only allowed 
in C99 or C11 mode

Just something to keep in mind when writing patches.


I just ran across this very issue on this commit.  It's an easy fixup
for 5.17.y to make this work, so I did that in my tree.  If this gets to
be too much, we might need to reconsider adding c11 to older stable
kernels.



I think I'll do just that for ChromeOS; I don't want to have to deal
with the backports, and we are using recent compilers anyway.


I think it would be better not to have the --std=gnu11 change in the older
stable kernels by default, as this has introduced build warnings and other
smaller issues, as well as raising the minimum compiler version.

The users that are stuck on older kernels for some reason tend to
overlap with those on older compilers. One example here is Android,
which used to ship with a gcc-4.9 build as the only non-clang toolchain,
and was using this for building their kernels. If someone wants to
pull in stable updates into an older Android, this would fail with
-std=gnu11. Others may be in the same situation.

Changing some of the 5.x stable branches to -std=gnu11 is probably
less of a problem, but I would not know where to draw the line exactly.
Maybe check with the Android team to see what the newest kernel is
that they expect to be built with the old gcc-4.9.



I don't think they still build anything with gcc. We (ChromeOS) only
need it for test builds of chromeos-4.4 (sigh), and that will hopefully
be gone in a couple of months.

We already enabled -std=gnu11 in chromeos-5.10 and chromeos-5.15.
We'll see if that is possible with chromeos-5.4 as well.
We won't bother with older kernel branches, but those should not
get many patches from upstream outside stable release merges,
so it is less of a problem.

Guenter

Re: [Intel-gfx] [greybus-dev] Re: [PATCH] [v2] Kbuild: move to -std=gnu11

[Guenter Roeck]

On 5/16/22 06:31, Greg KH wrote:

On Mon, May 16, 2022 at 06:10:23AM -0700, Guenter Roeck wrote:

On Mon, Feb 28, 2022 at 11:27:43AM +0100, Arnd Bergmann wrote:

From: Arnd Bergmann 

During a patch discussion, Linus brought up the option of changing
the C standard version from gnu89 to gnu99, which allows using variable
declaration inside of a for() loop. While the C99, C11 and later standards
introduce many other features, most of these are already available in
gnu89 as GNU extensions as well.


The downside is that backporting affected patches to older kernel branches
now fails with error messages such as

mm/kfence/core.c: In function ‘kfence_init_pool’:
mm/kfence/core.c:595:2: error: ‘for’ loop initial declarations are only allowed 
in C99 or C11 mode

Just something to keep in mind when writing patches.


I just ran across this very issue on this commit.  It's an easy fixup
for 5.17.y to make this work, so I did that in my tree.  If this gets to
be too much, we might need to reconsider adding c11 to older stable
kernels.



I think I'll do just that for ChromeOS; I don't want to have to deal
with the backports, and we are using recent compilers anyway.

Guenter

Re: [Intel-gfx] [PATCH] [v2] Kbuild: move to -std=gnu11

[Guenter Roeck]

On Mon, Feb 28, 2022 at 11:27:43AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann 
> 
> During a patch discussion, Linus brought up the option of changing
> the C standard version from gnu89 to gnu99, which allows using variable
> declaration inside of a for() loop. While the C99, C11 and later standards
> introduce many other features, most of these are already available in
> gnu89 as GNU extensions as well.

The downside is that backporting affected patches to older kernel branches
now fails with error messages such as

mm/kfence/core.c: In function ‘kfence_init_pool’:
mm/kfence/core.c:595:2: error: ‘for’ loop initial declarations are only allowed 
in C99 or C11 mode

Just something to keep in mind when writing patches.

Guenter

Re: [Intel-gfx] [PATCH 4/9] drm/i915/dmabuf: add paranoid flush-on-acquire

[Guenter Roeck]

On Mon, Oct 18, 2021 at 06:45:03PM +0100, Matthew Auld wrote:
> As pointed out by Thomas, we likely need to flush the pages here if the
> GPU can read the page contents directly from main memory. Underneath we
> don't know what the sg_table is pointing to, so just add a
> wbinvd_on_all_cpus() here, for now.
> 
> Reported-by: Thomas Hellström 
> Signed-off-by: Matthew Auld 
> Cc: Thomas Hellström 

With nosmp builds:

Error log:
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c: In function 
'i915_gem_object_get_pages_dmabuf':
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c:248:17: error: implicit declaration 
of function 'wbinvd_on_all_cpus' [-Werror=implicit-function-declaration]
  248 | wbinvd_on_all_cpus();
  | ^~

Guenter

> ---
>  drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c 
> b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> index 5be505ebbb7b..1adcd8e02d29 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> @@ -232,6 +232,7 @@ struct dma_buf *i915_gem_prime_export(struct 
> drm_gem_object *gem_obj, int flags)
>  
>  static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
>  {
> + struct drm_i915_private *i915 = to_i915(obj->base.dev);
>   struct sg_table *pages;
>   unsigned int sg_page_sizes;
>  
> @@ -242,8 +243,11 @@ static int i915_gem_object_get_pages_dmabuf(struct 
> drm_i915_gem_object *obj)
>   if (IS_ERR(pages))
>   return PTR_ERR(pages);
>  
> - sg_page_sizes = i915_sg_dma_sizes(pages->sgl);
> + /* XXX: consider doing a vmap flush or something */
> + if (!HAS_LLC(i915) || i915_gem_object_can_bypass_llc(obj))
> + wbinvd_on_all_cpus();
>  
> + sg_page_sizes = i915_sg_dma_sizes(pages->sgl);
>   __i915_gem_object_set_pages(obj, pages, sg_page_sizes);
>  
>   return 0;
> -- 
> 2.26.3
> 

Re: [Intel-gfx] [PATCH v15 10/12] swiotlb: Add restricted DMA pool initialization

[Guenter Roeck]

Hi Claire,

On Thu, Jun 24, 2021 at 11:55:24PM +0800, Claire Chang wrote:
> Add the initialization function to create restricted DMA pools from
> matching reserved-memory nodes.
> 
> Regardless of swiotlb setting, the restricted DMA pool is preferred if
> available.
> 
> The restricted DMA pools provide a basic level of protection against the
> DMA overwriting buffer contents at unexpected times. However, to protect
> against general data leakage and system memory corruption, the system
> needs to provide a way to lock down the memory access, e.g., MPU.
> 
> Signed-off-by: Claire Chang 
> Reviewed-by: Christoph Hellwig 
> Tested-by: Stefano Stabellini 
> Tested-by: Will Deacon 
> ---
>  include/linux/swiotlb.h |  3 +-
>  kernel/dma/Kconfig  | 14 
>  kernel/dma/swiotlb.c| 76 +
>  3 files changed, 92 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index 3b9454d1e498..39284ff2a6cd 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -73,7 +73,8 @@ extern enum swiotlb_force swiotlb_force;
>   *   range check to see if the memory was in fact allocated by this
>   *   API.
>   * @nslabs:  The number of IO TLB blocks (in groups of 64) between @start and
> - *   @end. This is command line adjustable via setup_io_tlb_npages.
> + *   @end. For default swiotlb, this is command line adjustable via
> + *   setup_io_tlb_npages.
>   * @used:The number of used IO TLB block.
>   * @list:The free list describing the number of free entries available
>   *   from each index.
> diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig
> index 77b405508743..3e961dc39634 100644
> --- a/kernel/dma/Kconfig
> +++ b/kernel/dma/Kconfig
> @@ -80,6 +80,20 @@ config SWIOTLB
>   bool
>   select NEED_DMA_MAP_STATE
>  
> +config DMA_RESTRICTED_POOL
> + bool "DMA Restricted Pool"
> + depends on OF && OF_RESERVED_MEM
> + select SWIOTLB

This makes SWIOTLB user configurable, which in turn results in

mips64-linux-ld: arch/mips/kernel/setup.o: in function `arch_mem_init':
setup.c:(.init.text+0x19c8): undefined reference to `plat_swiotlb_setup'
make[1]: *** [Makefile:1280: vmlinux] Error 1

when building mips:allmodconfig.

Should this possibly be "depends on SWIOTLB" ?

Thanks,
Guenter

Re: [Intel-gfx] [PATCH v2 11/11] drm/i915: Extract i915_module.c

[Guenter Roeck]

On Tue, Jul 27, 2021 at 02:10:37PM +0200, Daniel Vetter wrote:
> The module init code is somewhat misplaced in i915_pci.c, since it
> needs to pull in init/exit functions from every part of the driver and
> pollutes the include list a lot.
> 
> Extract an i915_module.c file which pulls all the bits together, and
> allows us to massively trim the include list of i915_pci.c.
> 
> The downside is that have to drop the error path check Jason added to
> catch when we set up the pci driver too early. I think that risk is
> acceptable for this pretty nice include.
> 
> Cc: Jason Ekstrand 
> Cc: Tvrtko Ursulin 
> Signed-off-by: Daniel Vetter 
> Reviewed-by: Jason Ekstrand 

With gcc and CONFIG_GCC_PLUGIN_RANDSTRUCT=y, this patch results
in:

drivers/gpu/drm/i915/i915_module.c:50:11: error:
positional initialization of field in 'struct' declared with 
'designated_init' attribute

This is seen for each of the initializers.

Guenter

> ---
>  drivers/gpu/drm/i915/Makefile  |   1 +
>  drivers/gpu/drm/i915/i915_module.c | 113 
>  drivers/gpu/drm/i915/i915_pci.c| 117 +
>  drivers/gpu/drm/i915/i915_pci.h|   8 ++
>  4 files changed, 125 insertions(+), 114 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_module.c
>  create mode 100644 drivers/gpu/drm/i915/i915_pci.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 9022dc638ed6..4ebd9f417ddb 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -38,6 +38,7 @@ i915-y += i915_drv.o \
> i915_irq.o \
> i915_getparam.o \
> i915_mitigations.o \
> +   i915_module.o \
> i915_params.o \
> i915_pci.o \
> i915_scatterlist.o \
> diff --git a/drivers/gpu/drm/i915/i915_module.c 
> b/drivers/gpu/drm/i915/i915_module.c
> new file mode 100644
> index ..c578ea8f56a0
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_module.c
> @@ -0,0 +1,113 @@
> +/*
> + * SPDX-License-Identifier: MIT
> + *
> + * Copyright © 2021 Intel Corporation
> + */
> +
> +#include 
> +
> +#include "gem/i915_gem_context.h"
> +#include "gem/i915_gem_object.h"
> +#include "i915_active.h"
> +#include "i915_buddy.h"
> +#include "i915_params.h"
> +#include "i915_pci.h"
> +#include "i915_perf.h"
> +#include "i915_request.h"
> +#include "i915_scheduler.h"
> +#include "i915_selftest.h"
> +#include "i915_vma.h"
> +
> +static int i915_check_nomodeset(void)
> +{
> + bool use_kms = true;
> +
> + /*
> +  * Enable KMS by default, unless explicitly overriden by
> +  * either the i915.modeset prarameter or by the
> +  * vga_text_mode_force boot option.
> +  */
> +
> + if (i915_modparams.modeset == 0)
> + use_kms = false;
> +
> + if (vgacon_text_force() && i915_modparams.modeset == -1)
> + use_kms = false;
> +
> + if (!use_kms) {
> + /* Silently fail loading to not upset userspace. */
> + DRM_DEBUG_DRIVER("KMS disabled.\n");
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static const struct {
> +   int (*init)(void);
> +   void (*exit)(void);
> +} init_funcs[] = {
> + { i915_check_nomodeset, NULL },
> + { i915_active_module_init, i915_active_module_exit },
> + { i915_buddy_module_init, i915_buddy_module_exit },
> + { i915_context_module_init, i915_context_module_exit },
> + { i915_gem_context_module_init, i915_gem_context_module_exit },
> + { i915_objects_module_init, i915_objects_module_exit },
> + { i915_request_module_init, i915_request_module_exit },
> + { i915_scheduler_module_init, i915_scheduler_module_exit },
> + { i915_vma_module_init, i915_vma_module_exit },
> + { i915_mock_selftests, NULL },
> + { i915_pmu_init, i915_pmu_exit },
> + { i915_register_pci_driver, i915_unregister_pci_driver },
> + { i915_perf_sysctl_register, i915_perf_sysctl_unregister },
> +};
> +static int init_progress;
> +
> +static int __init i915_init(void)
> +{
> + int err, i;
> +
> + for (i = 0; i < ARRAY_SIZE(init_funcs); i++) {
> + err = init_funcs[i].init();
> + if (err < 0) {
> + while (i--) {
> + if (init_funcs[i].exit)
> + init_funcs[i].exit();
> + }
> + return err;
> + } else if (err > 0) {
> + /*
> +  * Early-exit success is reserved for things which
> +  * don't have an exit() function because we have no
> +  * idea how far they got or how to partially tear
> +  * them down.
> +  */
> + WARN_ON(init_funcs[i].exit);
> + break;
> + }
> + }
> +
> + init_progress = i;
> +
> + return 0;
> +}
> +
> +static void __exit 

Re: [Intel-gfx] [PATCH v15 12/12] of: Add plumbing for restricted DMA pool

[Guenter Roeck]

On 7/2/21 6:18 AM, Will Deacon wrote:

On Fri, Jul 02, 2021 at 12:39:41PM +0100, Robin Murphy wrote:

On 2021-07-02 04:08, Guenter Roeck wrote:

On Thu, Jun 24, 2021 at 11:55:26PM +0800, Claire Chang wrote:

If a device is not behind an IOMMU, we look up the device node and set
up the restricted DMA when the restricted-dma-pool is presented.

Signed-off-by: Claire Chang 
Tested-by: Stefano Stabellini 
Tested-by: Will Deacon 


With this patch in place, all sparc and sparc64 qemu emulations
fail to boot. Symptom is that the root file system is not found.
Reverting this patch fixes the problem. Bisect log is attached.


Ah, OF_ADDRESS depends on !SPARC, so of_dma_configure_id() is presumably
returning an unexpected -ENODEV from the of_dma_set_restricted_buffer()
stub. That should probably be returning 0 instead, since either way it's not
an error condition for it to simply do nothing.


Something like below?



Yes, that does the trick.


Will

--->8


From 4d9dcb9210c1f37435b6088284e04b6b36ee8c4d Mon Sep 17 00:00:00 2001

From: Will Deacon 
Date: Fri, 2 Jul 2021 14:13:28 +0100
Subject: [PATCH] of: Return success from of_dma_set_restricted_buffer() when
  !OF_ADDRESS

When CONFIG_OF_ADDRESS=n, of_dma_set_restricted_buffer() returns -ENODEV
and breaks the boot for sparc[64] machines. Return 0 instead, since the
function is essentially a glorified NOP in this configuration.

Cc: Claire Chang 
Cc: Konrad Rzeszutek Wilk 
Reported-by: Guenter Roeck 
Suggested-by: Robin Murphy 
Link: https://lore.kernel.org/r/20210702030807.ga2685...@roeck-us.net
Signed-off-by: Will Deacon 


Tested-by: Guenter Roeck 


---
  drivers/of/of_private.h | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 8fde97565d11..34dd548c5eac 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -173,7 +173,8 @@ static inline int of_dma_get_range(struct device_node *np,
  static inline int of_dma_set_restricted_buffer(struct device *dev,
   struct device_node *np)
  {
-   return -ENODEV;
+   /* Do nothing, successfully. */
+   return 0;
  }
  #endif
  



___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v15 12/12] of: Add plumbing for restricted DMA pool

[Guenter Roeck]

Hi,

On Thu, Jun 24, 2021 at 11:55:26PM +0800, Claire Chang wrote:
> If a device is not behind an IOMMU, we look up the device node and set
> up the restricted DMA when the restricted-dma-pool is presented.
> 
> Signed-off-by: Claire Chang 
> Tested-by: Stefano Stabellini 
> Tested-by: Will Deacon 

With this patch in place, all sparc and sparc64 qemu emulations
fail to boot. Symptom is that the root file system is not found.
Reverting this patch fixes the problem. Bisect log is attached.

Guenter

---
# bad: [fb0ca446157a86b75502c1636b0d81e642fe6bf1] Add linux-next specific files 
for 20210701
# good: [62fb9874f5da54fdb243003b386128037319b219] Linux 5.13
git bisect start 'HEAD' 'v5.13'
# bad: [f63c4fda987a19b1194cc45cb72fd5bf968d9d90] Merge remote-tracking branch 
'rdma/for-next'
git bisect bad f63c4fda987a19b1194cc45cb72fd5bf968d9d90
# good: [46bb5dd1d2a63e906e374e97dfd4a5e33934b1c4] Merge remote-tracking branch 
'ipsec/master'
git bisect good 46bb5dd1d2a63e906e374e97dfd4a5e33934b1c4
# good: [43ba6969cfb8185353a7a6fc79070f13b9e3d6d3] Merge remote-tracking branch 
'clk/clk-next'
git bisect good 43ba6969cfb8185353a7a6fc79070f13b9e3d6d3
# good: [1ca5eddcf8dca1d6345471c6404e7364af0d7019] Merge remote-tracking branch 
'fuse/for-next'
git bisect good 1ca5eddcf8dca1d6345471c6404e7364af0d7019
# good: [8f6d7b3248705920187263a4e7147b0752ec7dcf] Merge remote-tracking branch 
'pci/next'
git bisect good 8f6d7b3248705920187263a4e7147b0752ec7dcf
# good: [df1885a755784da3ef285f36d9230c1d090ef186] RDMA/rtrs_clt: Alloc less 
memory with write path fast memory registration
git bisect good df1885a755784da3ef285f36d9230c1d090ef186
# good: [93d31efb58c8ad4a66bbedbc2d082df458c04e45] Merge remote-tracking branch 
'cpufreq-arm/cpufreq/arm/linux-next'
git bisect good 93d31efb58c8ad4a66bbedbc2d082df458c04e45
# good: [46308965ae6fdc7c25deb2e8c048510ae51bbe66] RDMA/irdma: Check contents 
of user-space irdma_mem_reg_req object
git bisect good 46308965ae6fdc7c25deb2e8c048510ae51bbe66
# good: [6de7a1d006ea9db235492b288312838d6878385f] 
thermal/drivers/int340x/processor_thermal: Split enumeration and processing part
git bisect good 6de7a1d006ea9db235492b288312838d6878385f
# good: [081bec2577cda3d04f6559c60b6f4e2242853520] dt-bindings: of: Add 
restricted DMA pool
git bisect good 081bec2577cda3d04f6559c60b6f4e2242853520
# good: [bf95ac0bcd69979af146852f6a617a60285ebbc1] Merge remote-tracking branch 
'thermal/thermal/linux-next'
git bisect good bf95ac0bcd69979af146852f6a617a60285ebbc1
# good: [3d8287544223a3d2f37981c1f9ffd94d0b5e9ffc] RDMA/core: Always release 
restrack object
git bisect good 3d8287544223a3d2f37981c1f9ffd94d0b5e9ffc
# bad: [cff1f23fad6e0bd7d671acce0d15285c709f259c] Merge remote-tracking branch 
'swiotlb/linux-next'
git bisect bad cff1f23fad6e0bd7d671acce0d15285c709f259c
# bad: [b655006619b7bccd0dc1e055bd72de5d613e7b5c] of: Add plumbing for 
restricted DMA pool
git bisect bad b655006619b7bccd0dc1e055bd72de5d613e7b5c
# first bad commit: [b655006619b7bccd0dc1e055bd72de5d613e7b5c] of: Add plumbing 
for restricted DMA pool
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] drm/i915/gem: Use list_entry to access list members

[Guenter Roeck]

Use list_entry() instead of container_of() to access list members.
Also drop unnecessary and misleading NULL checks on the result of
list_entry().

Signed-off-by: Guenter Roeck 
---
v2: Checkpatch fixes:
- Fix alignment
- Replace comparison against NULL with !

 drivers/gpu/drm/i915/gvt/dmabuf.c | 18 +-
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c 
b/drivers/gpu/drm/i915/gvt/dmabuf.c
index d4f883f35b95..e3f488681484 100644
--- a/drivers/gpu/drm/i915/gvt/dmabuf.c
+++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
@@ -148,8 +148,7 @@ static void dmabuf_gem_object_free(struct kref *kref)
 
if (vgpu && vgpu->active && !list_empty(>dmabuf_obj_list_head)) {
list_for_each(pos, >dmabuf_obj_list_head) {
-   dmabuf_obj = container_of(pos,
-   struct intel_vgpu_dmabuf_obj, list);
+   dmabuf_obj = list_entry(pos, struct 
intel_vgpu_dmabuf_obj, list);
if (dmabuf_obj == obj) {
list_del(pos);
intel_gvt_hypervisor_put_vfio_device(vgpu);
@@ -357,10 +356,8 @@ pick_dmabuf_by_info(struct intel_vgpu *vgpu,
struct intel_vgpu_dmabuf_obj *ret = NULL;
 
list_for_each(pos, >dmabuf_obj_list_head) {
-   dmabuf_obj = container_of(pos, struct intel_vgpu_dmabuf_obj,
-   list);
-   if ((dmabuf_obj == NULL) ||
-   (dmabuf_obj->info == NULL))
+   dmabuf_obj = list_entry(pos, struct intel_vgpu_dmabuf_obj, 
list);
+   if (!dmabuf_obj->info)
continue;
 
fb_info = (struct intel_vgpu_fb_info *)dmabuf_obj->info;
@@ -387,11 +384,7 @@ pick_dmabuf_by_num(struct intel_vgpu *vgpu, u32 id)
struct intel_vgpu_dmabuf_obj *ret = NULL;
 
list_for_each(pos, >dmabuf_obj_list_head) {
-   dmabuf_obj = container_of(pos, struct intel_vgpu_dmabuf_obj,
-   list);
-   if (!dmabuf_obj)
-   continue;
-
+   dmabuf_obj = list_entry(pos, struct intel_vgpu_dmabuf_obj, 
list);
if (dmabuf_obj->dmabuf_id == id) {
ret = dmabuf_obj;
break;
@@ -600,8 +593,7 @@ void intel_vgpu_dmabuf_cleanup(struct intel_vgpu *vgpu)
 
mutex_lock(>dmabuf_lock);
list_for_each_safe(pos, n, >dmabuf_obj_list_head) {
-   dmabuf_obj = container_of(pos, struct intel_vgpu_dmabuf_obj,
-   list);
+   dmabuf_obj = list_entry(pos, struct intel_vgpu_dmabuf_obj, 
list);
dmabuf_obj->vgpu = NULL;
 
idr_remove(>object_idr, dmabuf_obj->dmabuf_id);
-- 
2.25.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915/gem: Use list_entry to access list members

[Guenter Roeck]

Use list_entry() instead of container_of() to access list members.
Also drop unnecessary and misleading NULL checks on the result of
list_entry().

Signed-off-by: Guenter Roeck 
---
 drivers/gpu/drm/i915/gvt/dmabuf.c | 17 +
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c 
b/drivers/gpu/drm/i915/gvt/dmabuf.c
index d4f883f35b95..4241af5074a9 100644
--- a/drivers/gpu/drm/i915/gvt/dmabuf.c
+++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
@@ -148,7 +148,7 @@ static void dmabuf_gem_object_free(struct kref *kref)
 
if (vgpu && vgpu->active && !list_empty(>dmabuf_obj_list_head)) {
list_for_each(pos, >dmabuf_obj_list_head) {
-   dmabuf_obj = container_of(pos,
+   dmabuf_obj = list_entry(pos,
struct intel_vgpu_dmabuf_obj, list);
if (dmabuf_obj == obj) {
list_del(pos);
@@ -357,10 +357,8 @@ pick_dmabuf_by_info(struct intel_vgpu *vgpu,
struct intel_vgpu_dmabuf_obj *ret = NULL;
 
list_for_each(pos, >dmabuf_obj_list_head) {
-   dmabuf_obj = container_of(pos, struct intel_vgpu_dmabuf_obj,
-   list);
-   if ((dmabuf_obj == NULL) ||
-   (dmabuf_obj->info == NULL))
+   dmabuf_obj = list_entry(pos, struct intel_vgpu_dmabuf_obj, 
list);
+   if (dmabuf_obj->info == NULL)
continue;
 
fb_info = (struct intel_vgpu_fb_info *)dmabuf_obj->info;
@@ -387,11 +385,7 @@ pick_dmabuf_by_num(struct intel_vgpu *vgpu, u32 id)
struct intel_vgpu_dmabuf_obj *ret = NULL;
 
list_for_each(pos, >dmabuf_obj_list_head) {
-   dmabuf_obj = container_of(pos, struct intel_vgpu_dmabuf_obj,
-   list);
-   if (!dmabuf_obj)
-   continue;
-
+   dmabuf_obj = list_entry(pos, struct intel_vgpu_dmabuf_obj, 
list);
if (dmabuf_obj->dmabuf_id == id) {
ret = dmabuf_obj;
break;
@@ -600,8 +594,7 @@ void intel_vgpu_dmabuf_cleanup(struct intel_vgpu *vgpu)
 
mutex_lock(>dmabuf_lock);
list_for_each_safe(pos, n, >dmabuf_obj_list_head) {
-   dmabuf_obj = container_of(pos, struct intel_vgpu_dmabuf_obj,
-   list);
+   dmabuf_obj = list_entry(pos, struct intel_vgpu_dmabuf_obj, 
list);
dmabuf_obj->vgpu = NULL;
 
idr_remove(>object_idr, dmabuf_obj->dmabuf_id);
-- 
2.25.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915/gem: Almagamate clflushes on freeze

[Guenter Roeck]

On Tue, Jan 19, 2021 at 02:49:08PM +, Chris Wilson wrote:
> When flushing objects larger than the CPU cache it is preferrable to use
> a single wbinvd() rather than overlapping clflush(). At runtime, we
> avoid wbinvd() due to its system-wide latencies, but during
> singlethreaded suspend, no one will observe the imposed latency and we
> can opt for the faster wbinvd to clear all objects in a single hit.
> 
> Signed-off-by: Chris Wilson 
> Reviewed-by: Matthew Auld 
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 16 +---
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c013148835e6..d3a287bf56c5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1175,19 +1175,13 @@ int i915_gem_freeze_late(struct drm_i915_private 
> *i915)
>* the objects as well, see i915_gem_freeze()
>*/
>  
> - wakeref = intel_runtime_pm_get(>runtime_pm);
> -
> - i915_gem_shrink(i915, -1UL, NULL, ~0);
> + with_intel_runtime_pm(>runtime_pm, wakeref)
> + i915_gem_shrink(i915, -1UL, NULL, ~0);
>   i915_gem_drain_freed_objects(i915);
>  
> - list_for_each_entry(obj, >mm.shrink_list, mm.link) {
> - i915_gem_object_lock(obj, NULL);
> - drm_WARN_ON(>drm,
> - i915_gem_object_set_to_cpu_domain(obj, true));
> - i915_gem_object_unlock(obj);
> - }
> -
> - intel_runtime_pm_put(>runtime_pm, wakeref);
> + wbinvd_on_all_cpus();

with CONFIG_SMP=n, this results in:

drivers/gpu/drm/i915/i915_gem.c: In function 'i915_gem_freeze_late':
drivers/gpu/drm/i915/i915_gem.c:1182:2: error: implicit declaration of function 
'wbinvd_on_all_cpus'

Other drivers calling this function include .

Guenter

> + list_for_each_entry(obj, >mm.shrink_list, mm.link)
> + __start_cpu_write(obj);
>  
>   return 0;
>  }
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [patch 02/30] genirq: Move status flag checks to core

[Guenter Roeck]

On Thu, Dec 10, 2020 at 08:25:38PM +0100, Thomas Gleixner wrote:
> These checks are used by modules and prevent the removal of the export of
> irq_to_desc(). Move the accessor into the core.
> 
> Signed-off-by: Thomas Gleixner 

Yes, but that means that irq_check_status_bit() may be called from modules,
but it is not exported, resulting in build errors such as the following.

arm64:allmodconfig:

ERROR: modpost: "irq_check_status_bit" [drivers/perf/arm_spe_pmu.ko] undefined!

Guenter

> ---
>  include/linux/irqdesc.h |   17 +
>  kernel/irq/manage.c |   17 +
>  2 files changed, 22 insertions(+), 12 deletions(-)
> 
> --- a/include/linux/irqdesc.h
> +++ b/include/linux/irqdesc.h
> @@ -223,28 +223,21 @@ irq_set_chip_handler_name_locked(struct
>   data->chip = chip;
>  }
>  
> +bool irq_check_status_bit(unsigned int irq, unsigned int bitmask);
> +
>  static inline bool irq_balancing_disabled(unsigned int irq)
>  {
> - struct irq_desc *desc;
> -
> - desc = irq_to_desc(irq);
> - return desc->status_use_accessors & IRQ_NO_BALANCING_MASK;
> + return irq_check_status_bit(irq, IRQ_NO_BALANCING_MASK);
>  }
>  
>  static inline bool irq_is_percpu(unsigned int irq)
>  {
> - struct irq_desc *desc;
> -
> - desc = irq_to_desc(irq);
> - return desc->status_use_accessors & IRQ_PER_CPU;
> + return irq_check_status_bit(irq, IRQ_PER_CPU);
>  }
>  
>  static inline bool irq_is_percpu_devid(unsigned int irq)
>  {
> - struct irq_desc *desc;
> -
> - desc = irq_to_desc(irq);
> - return desc->status_use_accessors & IRQ_PER_CPU_DEVID;
> + return irq_check_status_bit(irq, IRQ_PER_CPU_DEVID);
>  }
>  
>  static inline void
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -2769,3 +2769,23 @@ bool irq_has_action(unsigned int irq)
>   return res;
>  }
>  EXPORT_SYMBOL_GPL(irq_has_action);
> +
> +/**
> + * irq_check_status_bit - Check whether bits in the irq descriptor status 
> are set
> + * @irq: The linux irq number
> + * @bitmask: The bitmask to evaluate
> + *
> + * Returns: True if one of the bits in @bitmask is set
> + */
> +bool irq_check_status_bit(unsigned int irq, unsigned int bitmask)
> +{
> + struct irq_desc *desc;
> + bool res = false;
> +
> + rcu_read_lock();
> + desc = irq_to_desc(irq);
> + if (desc)
> + res = !!(desc->status_use_accessors & bitmask);
> + rcu_read_unlock();
> + return res;
> +}
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Protect debugfs per_file_stats with RCU lock

[Guenter Roeck]

On Tue, Jun 30, 2020 at 04:08:00PM +0100, Chris Wilson wrote:
> Quoting Guenter Roeck (2020-06-30 16:01:05)
> > On Tue, Jun 30, 2020 at 10:14:25AM +0100, Chris Wilson wrote:
> > [ ... ]
> > > > > @@ -328,9 +334,9 @@ static void print_context_stats(struct seq_file 
> > > > > *m,
> > > > >   struct task_struct *task;
> > > > >   char name[80];
> > > > >  
> > > > > - spin_lock(>table_lock);
> > > > > + rcu_read_lock();
> > > > >   idr_for_each(>object_idr, per_file_stats, 
> > > > > );
> > > > > - spin_unlock(>table_lock);
> > > > > + rcu_read_unlock();
> > > > >  
> > > > For my education - is it indeed possible and valid to replace 
> > > > spin_lock()
> > > > with rcu_read_lock() to prevent list manipulation for a list used by
> > > > idr_for_each(), even if that list is otherwise manipulated under the
> > > > spinlock ?
> > > 
> > > It's a pure read of a radixtree here, and is supposed to be RCU safe:
> > > 
> > >  * idr_for_each() can be called concurrently with idr_alloc() and
> > >  * idr_remove() if protected by RCU.  Newly added entries may not be
> > >  * seen and deleted entries may be seen, but adding and removing entries
> > >  * will not cause other entries to be skipped, nor spurious ones to be 
> > > seen.
> > > 
> > > That is the tree structure is stable.
> > > 
> > Ah, that makes sense. Thanks for the clarification.
> > 
> > > > Background: we are seeing a crash with the following call trace.
> > > > 
> > > > [ 1016.651593] BUG: kernel NULL pointer dereference, address: 
> > > > 
> > > > ...
> > > > [ 1016.651693] Call Trace:
> > > > [ 1016.651703]  idr_for_each+0x8a/0xe8
> > > > [ 1016.651711]  i915_gem_object_info+0x2a3/0x3eb
> > > > [ 1016.651720]  seq_read+0x162/0x3ca
> > > > [ 1016.651727]  full_proxy_read+0x5b/0x8d
> > > > [ 1016.651733]  __vfs_read+0x45/0x1bb
> > > > [ 1016.651741]  vfs_read+0xc9/0x15e
> > > > [ 1016.651746]  ksys_read+0x7e/0xde
> > > > [ 1016.651752]  do_syscall_64+0x54/0x68
> > > > [ 1016.651758]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > 
> > Actually, the crash is not in idr_for_each, but in per_file_stats:
> 
> Ok, let's assume that the object is being closed as we read the idr. The
> idr will temporarily hold an error pointer for the handle to indicate the
> in-progress closure, so something like:
> 
> @@ -230,7 +230,7 @@ static int per_file_stats(int id, void *ptr, void *data)
> struct file_stats *stats = data;
> struct i915_vma *vma;
> 
> -   if (!kref_get_unless_zero(>base.refcount))
> +   if (IS_ERR_OR_NULL(obj) || !kref_get_unless_zero(>base.refcount))
> return 0;
> 
Makes sense. Thanks a lot for the patch!

Guenter
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Protect debugfs per_file_stats with RCU lock

[Guenter Roeck]

On Tue, Jun 30, 2020 at 10:14:25AM +0100, Chris Wilson wrote:
[ ... ]
> > > @@ -328,9 +334,9 @@ static void print_context_stats(struct seq_file *m,
> > >   struct task_struct *task;
> > >   char name[80];
> > >  
> > > - spin_lock(>table_lock);
> > > + rcu_read_lock();
> > >   idr_for_each(>object_idr, per_file_stats, 
> > > );
> > > - spin_unlock(>table_lock);
> > > + rcu_read_unlock();
> > >  
> > For my education - is it indeed possible and valid to replace spin_lock()
> > with rcu_read_lock() to prevent list manipulation for a list used by
> > idr_for_each(), even if that list is otherwise manipulated under the
> > spinlock ?
> 
> It's a pure read of a radixtree here, and is supposed to be RCU safe:
> 
>  * idr_for_each() can be called concurrently with idr_alloc() and
>  * idr_remove() if protected by RCU.  Newly added entries may not be
>  * seen and deleted entries may be seen, but adding and removing entries
>  * will not cause other entries to be skipped, nor spurious ones to be seen.
> 
> That is the tree structure is stable.
> 
Ah, that makes sense. Thanks for the clarification.

> > Background: we are seeing a crash with the following call trace.
> > 
> > [ 1016.651593] BUG: kernel NULL pointer dereference, address: 
> > 
> > ...
> > [ 1016.651693] Call Trace:
> > [ 1016.651703]  idr_for_each+0x8a/0xe8
> > [ 1016.651711]  i915_gem_object_info+0x2a3/0x3eb
> > [ 1016.651720]  seq_read+0x162/0x3ca
> > [ 1016.651727]  full_proxy_read+0x5b/0x8d
> > [ 1016.651733]  __vfs_read+0x45/0x1bb
> > [ 1016.651741]  vfs_read+0xc9/0x15e
> > [ 1016.651746]  ksys_read+0x7e/0xde
> > [ 1016.651752]  do_syscall_64+0x54/0x68
> > [ 1016.651758]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
Actually, the crash is not in idr_for_each, but in per_file_stats:

[ 1016.651637] RIP: 0010:per_file_stats+0xe/0x16e
[ 1016.651646] Code: d2 41 0f b6 8e 69 8c 00 00 48 89 df 48 c7 c6 7b 74 8c be 
31 c0 e8 0c 89 cf ff eb d2 0f 1f 44 00 00 55 48 89 e5 41 57 41 56 53 <8b> 06 85 
c0 0f 84 4d 01 00 00 49 89 d6 48 89 f3 3d ff ff ff 7f 73
[ 1016.651651] RSP: 0018:ad3a01337ba0 EFLAGS: 00010293
[ 1016.651656] RAX: 0018 RBX: 96fe040d65e0 RCX: 0002
[ 1016.651660] RDX: ad3a01337c50 RSI:  RDI: 01e8
[ 1016.651663] RBP: ad3a01337bb8 R08:  R09: 01c0
[ 1016.651667] R10:  R11: bdbe5fce R12: 
[ 1016.651671] R13: bdbe5fce R14: ad3a01337c50 R15: 0001
[ 1016.651676] FS:  7a597e2d7480() GS:96ff3bb0() 
knlGS:
[ 1016.651680] CS:  0010 DS:  ES:  CR0: 80050033
[ 1016.651683] CR2:  CR3: 000171fc2001 CR4: 003606e0
[ 1016.651687] DR0:  DR1:  DR2: 
[ 1016.651690] DR3:  DR6: fffe0ff0 DR7: 0400
[ 1016.651693] Call Trace:

Sorry for the confusion. From the context it appears that the second parameter
of per_file_stats() may be NULL, though I am not entirely sure if I got that
correctly.

> Is there a reason you are using this slow debugfs in the first place?

AFAICS ChromeOS is using the information to calculate graphics memory use.

Guenter
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Protect debugfs per_file_stats with RCU lock

[Guenter Roeck]

On Tue, Sep 03, 2019 at 07:21:33AM +0100, Chris Wilson wrote:
> If we make sure we grab a strong reference to each object as we dump it,
> we can reduce the locks outside of our iterators to an rcu_read_lock.
> 
> This should prevent errors like:
> [ 2138.371911] BUG: KASAN: use-after-free in per_file_stats+0x43/0x380 [i915]
> [ 2138.371924] Read of size 8 at addr 888223651000 by task cat/8293
> 
> [ 2138.371947] CPU: 0 PID: 8293 Comm: cat Not tainted 
> 5.3.0-rc6-CI-Custom_4352+ #1
> [ 2138.371953] Hardware name: To Be Filled By O.E.M. To Be Filled By 
> O.E.M./J4205-ITX, BIOS P1.40 07/14/2017
> [ 2138.371959] Call Trace:
> [ 2138.371974]  dump_stack+0x7c/0xbb
> [ 2138.372099]  ? per_file_stats+0x43/0x380 [i915]
> [ 2138.372108]  print_address_description+0x73/0x3a0
> [ 2138.372231]  ? per_file_stats+0x43/0x380 [i915]
> [ 2138.372352]  ? per_file_stats+0x43/0x380 [i915]
> [ 2138.372362]  __kasan_report+0x14e/0x192
> [ 2138.372489]  ? per_file_stats+0x43/0x380 [i915]
> [ 2138.372502]  kasan_report+0xe/0x20
> [ 2138.372625]  per_file_stats+0x43/0x380 [i915]
> [ 2138.372751]  ? i915_panel_show+0x110/0x110 [i915]
> [ 2138.372761]  idr_for_each+0xa7/0x160
> [ 2138.372773]  ? idr_get_next_ul+0x110/0x110
> [ 2138.372782]  ? do_raw_spin_lock+0x10a/0x1d0
> [ 2138.372923]  print_context_stats+0x264/0x510 [i915]
> 
> Signed-off-by: Chris Wilson 
> Tested-by: David Weinehall 
> Reviewed-by: Mika Kuoppala 
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9798f27a697a..708855e051b5 100644
[ ... ]
>   }
> @@ -328,9 +334,9 @@ static void print_context_stats(struct seq_file *m,
>   struct task_struct *task;
>   char name[80];
>  
> - spin_lock(>table_lock);
> + rcu_read_lock();
>   idr_for_each(>object_idr, per_file_stats, );
> - spin_unlock(>table_lock);
> + rcu_read_unlock();
>  
For my education - is it indeed possible and valid to replace spin_lock()
with rcu_read_lock() to prevent list manipulation for a list used by
idr_for_each(), even if that list is otherwise manipulated under the
spinlock ?

Background: we are seeing a crash with the following call trace.

[ 1016.651593] BUG: kernel NULL pointer dereference, address: 
...
[ 1016.651693] Call Trace:
[ 1016.651703]  idr_for_each+0x8a/0xe8
[ 1016.651711]  i915_gem_object_info+0x2a3/0x3eb
[ 1016.651720]  seq_read+0x162/0x3ca
[ 1016.651727]  full_proxy_read+0x5b/0x8d
[ 1016.651733]  __vfs_read+0x45/0x1bb
[ 1016.651741]  vfs_read+0xc9/0x15e
[ 1016.651746]  ksys_read+0x7e/0xde
[ 1016.651752]  do_syscall_64+0x54/0x68
[ 1016.651758]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

I have not tried to track down what exactly is NULL in this case.
Before spending more time on it, I'd like to understand the above
change a little better.

Thanks,
Guenter
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [v5, 3/3] drm/i915: Add support for integrated privacy screens

[Guenter Roeck]

On Fri, Dec 20, 2019 at 12:03:53PM -0800, Rajat Jain wrote:
> Certain laptops now come with panels that have integrated privacy
> screens on them. This patch adds support for such panels by adding
> a privacy-screen property to the intel_connector for the panel, that
> the userspace can then use to control and check the status.
> 
> Identifying the presence of privacy screen, and controlling it, is done
> via ACPI _DSM methods.
> 
> Currently, this is done only for the Intel display ports. But in future,
> this can be done for any other ports if the hardware becomes available
> (e.g. external monitors supporting integrated privacy screens?).
> 
> Signed-off-by: Rajat Jain 
> ---
> v5: fix a cosmetic checkpatch warning
> v4: Fix a typo in intel_privacy_screen.h
> v3: * Change license to GPL-2.0 OR MIT
> * Move privacy screen enum from UAPI to intel_display_types.h
> * Rename parameter name and some other minor changes.
> v2: Formed by splitting the original patch into multiple patches.
> - All code has been moved into i915 now.
> - Privacy screen is a i915 property
> - Have a local state variable to store the prvacy screen. Don't read
>   it from hardware.
> 
>  drivers/gpu/drm/i915/Makefile |  3 +-
>  drivers/gpu/drm/i915/display/intel_atomic.c   | 13 +++-
>  .../gpu/drm/i915/display/intel_connector.c| 35 +
>  .../gpu/drm/i915/display/intel_connector.h|  1 +
>  .../drm/i915/display/intel_display_types.h| 18 +
>  drivers/gpu/drm/i915/display/intel_dp.c   |  6 ++
>  .../drm/i915/display/intel_privacy_screen.c   | 72 +++
>  .../drm/i915/display/intel_privacy_screen.h   | 27 +++
>  8 files changed, 171 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_privacy_screen.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_privacy_screen.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 90dcf09f52cc..f7067c8f0407 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -197,7 +197,8 @@ i915-y += \
>   display/intel_vga.o
>  i915-$(CONFIG_ACPI) += \
>   display/intel_acpi.o \
> - display/intel_opregion.o
> + display/intel_opregion.o \
> + display/intel_privacy_screen.o
>  i915-$(CONFIG_DRM_FBDEV_EMULATION) += \
>   display/intel_fbdev.o
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c 
> b/drivers/gpu/drm/i915/display/intel_atomic.c
> index c2875b10adf9..c73b81c4c3f6 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -37,6 +37,7 @@
>  #include "intel_atomic.h"
>  #include "intel_display_types.h"
>  #include "intel_hdcp.h"
> +#include "intel_privacy_screen.h"
>  #include "intel_sprite.h"
>  
>  /**
> @@ -57,11 +58,14 @@ int intel_digital_connector_atomic_get_property(struct 
> drm_connector *connector,
>   struct drm_i915_private *dev_priv = to_i915(dev);
>   struct intel_digital_connector_state *intel_conn_state =
>   to_intel_digital_connector_state(state);
> + struct intel_connector *intel_connector = to_intel_connector(connector);
>  
>   if (property == dev_priv->force_audio_property)
>   *val = intel_conn_state->force_audio;
>   else if (property == dev_priv->broadcast_rgb_property)
>   *val = intel_conn_state->broadcast_rgb;
> + else if (property == intel_connector->privacy_screen_property)
> + *val = intel_conn_state->privacy_screen_status;
>   else {
>   DRM_DEBUG_ATOMIC("Unknown property [PROP:%d:%s]\n",
>property->base.id, property->name);
> @@ -89,15 +93,18 @@ int intel_digital_connector_atomic_set_property(struct 
> drm_connector *connector,
>   struct drm_i915_private *dev_priv = to_i915(dev);
>   struct intel_digital_connector_state *intel_conn_state =
>   to_intel_digital_connector_state(state);
> + struct intel_connector *intel_connector = to_intel_connector(connector);
>  
>   if (property == dev_priv->force_audio_property) {
>   intel_conn_state->force_audio = val;
>   return 0;
> - }
> -
> - if (property == dev_priv->broadcast_rgb_property) {
> + } else if (property == dev_priv->broadcast_rgb_property) {
>   intel_conn_state->broadcast_rgb = val;
>   return 0;
> + } else if (property == intel_connector->privacy_screen_property) {
> + intel_privacy_screen_set_val(intel_connector, val);
> + intel_conn_state->privacy_screen_status = val;
> + return 0;
>   }
>  
>   DRM_DEBUG_ATOMIC("Unknown property [PROP:%d:%s]\n",
> diff --git a/drivers/gpu/drm/i915/display/intel_connector.c 
> b/drivers/gpu/drm/i915/display/intel_connector.c
> index 1133c4e97bb4..f3e041c737de 100644
> --- a/drivers/gpu/drm/i915/display/intel_connector.c
> +++ 

Re: [Intel-gfx] [PATCH v6 19/24] drm/bridge: dumb-vga-dac: Provide ddc symlink in connector sysfs directory

[Guenter Roeck]

On Fri, Jul 26, 2019 at 07:23:13PM +0200, Andrzej Pietrasiewicz wrote:
> Use the ddc pointer provided by the generic connector.
> 
> Signed-off-by: Andrzej Pietrasiewicz 
> Reviewed-by: Neil Armstrong 

This patch results in a crash when running qemu:versatilepb.

Unable to handle kernel NULL pointer dereference at virtual address 00c5
pgd = (ptrval)
[00c5] *pgd=
Internal error: Oops: 5 [#1] ARM
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 5.3.0-rc1+ #1
Hardware name: ARM-Versatile (Device Tree Support)
PC is at sysfs_do_create_link_sd+0x38/0xd8
LR is at sysfs_do_create_link_sd+0x38/0xd8
pc : []lr : []psr: a153
sp : c783bd18  ip :   fp : c783bde8
r10: c7ef5ea8  r9 : 0001  r8 : c0955dc0
r7 : c73cb5b0  r6 : c73cd800  r5 : 00ad  r4 : 
r3 : c7838ae0  r2 :   r1 : 0008  r0 : c0aa2898
Flags: NzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment none
Control: 00093177  Table: 4000  DAC: 0053
Process swapper (pid: 1, stack limit = 0x(ptrval))
Stack: (0xc783bd18 to 0xc783c000)
bd00:   c73ccc48 c73ccc74
bd20: c73cd800 c0ac7c88  c729cc80 c7ef5ea8 c04c7fc0 c73ccc48 c0a73068
bd40: c73cd800 c0ac7c88  c04c87e0 0001  c04cefcc c04dc3f8
bd60: c73a9030 c73cd800 c73ccc48 7fc2ce37  c73cd800  c04cefcc
bd80: c73cd800   c04b4ebc c0a73068 c7ef5ea8 c783bde8 c049ffcc
bda0: c73a9020 c73cd800 c78e6000 c73a9020  c73a9020 c0a73068 c04df2f8
bdc0: c783bde8 c095a76c c73a9020 c0065744 c73ccc20 c73a9020  0001
bde0: c7838ae0  c73ccc20 7fc2ce37  c78e6000  c0ac7c34
be00: c07dc1f8   c0a6b384 c0a59858 c045e8d8 c78e6000 c1173a78
be20:  c0ac7c34  c04e77c4 c78e6000 c0ac7c34 c0ac7c34 c0a73068
be40:  e000 c0a6b384 c04e7a34 c0ac7c34 c0ac7c34 c0a73068 c78e6000
be60:  c0ac7c34 c0a73068  e000 c0a6b384 c0a59858 c04e7cf0
be80:  c0ac7c34 c78e6000 c04e7d7c  c0ac7c34 c04e7cf8 c04e5928
bea0: c73b2800 c78d88a0 c78dd110 7fc2ce37 e000 c0ac7c34 c73b2800 c0ac16e0
bec0:  c04e6b28 c095a73c c0af0a60 c0a73068 c0ac7c34 c0af0a60 c0a73068
bee0: c0a401c4 c04e8968 e000 c0af0a60 c0a73068 c000b3bc 0115 
bf00: c7ffce6c c7ffce00 c09e15b0 0115 0115 c0048844 c09e000c c097cfd4
bf20:  0006 0006   c7ffce6c e000 c006954c
bf40: e000 7fc2ce37 c0afb000 c0af0a60 0115 c0afb000 0007 c0a59850
bf60: e000 c0a111e8 0006 0006  c0a10678  7fc2ce37
bf80:   c07824cc     
bfa0:  c07824d4  c00090b0    
bfc0:        
bfe0:     0013   
[] (sysfs_do_create_link_sd) from [] 
(drm_connector_register.part.1+0x40/0xa0)
[] (drm_connector_register.part.1) from [] 
(drm_connector_register_all+0x90/0xb8)
[] (drm_connector_register_all) from [] 
(drm_modeset_register_all+0x44/0x6c)
[] (drm_modeset_register_all) from [] 
(drm_dev_register+0x15c/0x1c0)
[] (drm_dev_register) from [] (pl111_amba_probe+0x2e0/0x4ac)
[] (pl111_amba_probe) from [] (amba_probe+0x9c/0x118)
[] (amba_probe) from [] (really_probe+0x1c0/0x2bc)
[] (really_probe) from [] (driver_probe_device+0x5c/0x170)
[] (driver_probe_device) from [] 
(device_driver_attach+0x58/0x60)
[] (device_driver_attach) from [] 
(__driver_attach+0x84/0xc0)
[] (__driver_attach) from [] (bus_for_each_dev+0x70/0xb4)
[] (bus_for_each_dev) from [] (bus_add_driver+0x154/0x1e0)
[] (bus_add_driver) from [] (driver_register+0x74/0x108)
[] (driver_register) from [] (do_one_initcall+0x84/0x2e4)
[] (do_one_initcall) from [] 
(kernel_init_freeable+0x2bc/0x394)
[] (kernel_init_freeable) from [] (kernel_init+0x8/0xf0)
[] (kernel_init) from [] (ret_from_fork+0x14/0x24)
Exception stack(0xc783bfb0 to 0xc783bff8)
bfa0:    
bfc0:        
bfe0:     0013 
Code: e59f00a0 e1a09003 e1a08002 eb176e54 (e5955018) 
---[ end trace f503b374936886c5 ]---

Bisect log attached.

Guenter

---
# bad: [3880be629e26f6c407593602398c6651860d5fae] Add linux-next specific files 
for 20190807
# good: [e21a712a9685488f5ce80495b37b9fdbe96c230d] Linux 5.3-rc3
git bisect start 'HEAD' 'v5.3-rc3'
# good: [83d74da9e6d2ca78b32e9e794c6bcbd433d5efaa] Merge remote-tracking branch 
'crypto/master'
git bisect good 83d74da9e6d2ca78b32e9e794c6bcbd433d5efaa
# bad: [3add021bff629f1792a5e4268afe13b3047b5523] Merge remote-tracking branch 
'sound/for-next'
git bisect bad 3add021bff629f1792a5e4268afe13b3047b5523
# good: [4ef58ee18a654b1992d00281501d6eff051a0c5e] Merge remote-tracking branch 
'amdgpu/drm-next'

Re: [Intel-gfx] [CI, v3, 1/2] drm/i915: Prevent a race during I915_GEM_MMAP ioctl with WC set

[Guenter Roeck]

On Thu, Feb 28, 2019 at 10:34:20PM +, Chris Wilson wrote:
> Quoting Guenter Roeck (2019-02-28 22:27:35)
> > On Thu, Feb 28, 2019 at 10:18:32PM +, Chris Wilson wrote:
> > > 
> > > If you have userspace that is broken, we need to fix it. We can get away
> > > with quietly changing ABI only so long as nobody notices. It sounds like
> > > you have some userspace that will break if you updated the kernel; ergo
> > > we have a problem.
> > > 
> > > We just need that as a clear statement so that we have the user impact
> > > recorded.
> > 
> > Yes, it does break our userspace code. I fixed the problem in our kernels
> > with the following patch:
> > 
> And what userspace is that? Details for the log, please.

Internal agreement is that we'll fix our userspace code.

Guenter
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [CI, v3, 1/2] drm/i915: Prevent a race during I915_GEM_MMAP ioctl with WC set

[Guenter Roeck]

On Thu, Feb 28, 2019 at 10:18:32PM +, Chris Wilson wrote:
> 
> If you have userspace that is broken, we need to fix it. We can get away
> with quietly changing ABI only so long as nobody notices. It sounds like
> you have some userspace that will break if you updated the kernel; ergo
> we have a problem.
> 
> We just need that as a clear statement so that we have the user impact
> recorded.

Yes, it does break our userspace code. I fixed the problem in our kernels
with the following patch:

-   return vma->vm_start == addr && (vma->vm_end - vma->vm_start) == size;
+   return vma->vm_start == addr && (vma->vm_end - vma->vm_start) == 
roundup(size, PAGE_SIZE);

Guenter
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [CI, v3, 1/2] drm/i915: Prevent a race during I915_GEM_MMAP ioctl with WC set

[Guenter Roeck]

On Thu, Feb 28, 2019 at 10:01:45PM +, Chris Wilson wrote:
> Quoting Guenter Roeck (2019-02-28 21:57:03)
> > On Thu, Feb 28, 2019 at 01:32:41PM -0800, Guenter Roeck wrote:
> > > On Thu, Feb 28, 2019 at 11:12:49AM -0800, Guenter Roeck wrote:
> > > > Hi,
> > > > 
> > > > On Thu, Feb 07, 2019 at 10:54:53AM +0200, Joonas Lahtinen wrote:
> > > > > Make sure the underlying VMA in the process address space is the
> > > > > same as it was during vm_mmap to avoid applying WC to wrong VMA.
> > > > > 
> > > > > A more long-term solution would be to have vm_mmap_locked variant
> > > > > in linux/mmap.h for when caller wants to hold mmap_sem for an
> > > > > extended duration.
> > > > > 
> > > > 
> > > > It seems like we may have a regression due to this patch. I am still
> > > > debugging, but I have a question; please see below.
> > > > 
> > > > Thanks,
> > > > Guenter
> > > > 
> > > > > v2:
> > > > > - Refactor the compare function
> > > > > 
> > > > > Fixes: 1816f9236303 ("drm/i915: Support creation of unbound wc user 
> > > > > mappings for objects")
> > > > > Reported-by: Adam Zabrocki 
> > > > > Suggested-by: Linus Torvalds 
> > > > > Signed-off-by: Joonas Lahtinen 
> > > > > Cc:  # v4.0+
> > > > > Cc: Akash Goel 
> > > > > Cc: Chris Wilson 
> > > > > Cc: Tvrtko Ursulin 
> > > > > Cc: Adam Zabrocki 
> > > > > Reviewed-by: Chris Wilson 
> > > > > Reviewed-by: Tvrtko Ursulin  #v1
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_gem.c | 12 +++-
> > > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > > > > b/drivers/gpu/drm/i915/i915_gem.c
> > > > > index 05ce9176ac4e..52639f749908 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > > @@ -1681,6 +1681,16 @@ i915_gem_sw_finish_ioctl(struct drm_device 
> > > > > *dev, void *data,
> > > > >   return 0;
> > > > >  }
> > > > >  
> > > > > +static inline bool
> > > > > +__vma_matches(struct vm_area_struct *vma, struct file *filp,
> > > > > +   unsigned long addr, unsigned long size)
> > > > > +{
> > > > > + if (vma->vm_file != filp)
> > > > > + return false;
> > > > > +
> > > > > + return vma->vm_start == addr && (vma->vm_end - vma->vm_start) == 
> > > > > size;
> > > > 
> > > > Shouldn't this be:
> > > > return vma->vm_start == addr && (vma->vm_end - vma->vm_start + 1) 
> > > > == size;
> > > > instead ?
> > > > 
> > > 
> > > Answer is no .. because vm_end points to the first byte after the
> > > end address.
> > > 
> > > The actual values are:
> > > 
> > > start=7d288f7f9000 end=7d288f84d000 end-start=54000 size=53400
> > > 
> > > meaning the size field passed in the ioctl is smaller than the total 
> > > length
> > > of the area.
> > > 
> > > Question is now: Is the request/ioctl indeed invalid, ie does the 
> > > requested
> > > size have to match the vma size ? This used to work until this patch was
> > > applied, and the change causes our test code to fail (and possibly 
> > > minigbm,
> > > which is used by the test code). That doesn't mean that our code is 
> > > correct
> > > (I see some related local changes in our version of minigbm), but it is
> > > annoying, and I am being asked to revert this patch as regression
> > > from our kernel releases.
> > > 
> > 
> > In i915_gem_create():
> > 
> > size = roundup(size, PAGE_SIZE);
> > if (size == 0)
> > return -EINVAL;
> > 
> > This suggests to me that the requested size can be smaller than the
> 
> Not really, the ABI has never handled less than page-sized requests.
> It's a mistake from the very beginning that it was not rejected as being
> the invalid size it was.
> 
> > allocated size, which in turn suggests that the check
> > (vma->vm_end - vma->vm_start) == size;
> > is wrong. Either it should be
> > (vma->vm_end - vma->vm_start) >= size;
> > or possibly
> > (vma->vm_end - vma->vm_start) == roundup(size, PAGE_SIZE);
> > 
> > Any comments/feedback/thoughts ?
> 
> It's a violation of mmap(2).
> 
> Is probably what we will have to do if you ring the regression bell loud
> enough, and do not see the folly of your ways. :-p

I won't ring any bells; I don't play such games. I'll make a local change
in our kernel to fix the problem, quoting your statement that less than
page-sized requests were never supposed to be supported, and add a note
that we'll have to handle this with a local patch going forward.

Guenter
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [CI, v3, 1/2] drm/i915: Prevent a race during I915_GEM_MMAP ioctl with WC set

[Guenter Roeck]

On Thu, Feb 28, 2019 at 01:32:41PM -0800, Guenter Roeck wrote:
> On Thu, Feb 28, 2019 at 11:12:49AM -0800, Guenter Roeck wrote:
> > Hi,
> > 
> > On Thu, Feb 07, 2019 at 10:54:53AM +0200, Joonas Lahtinen wrote:
> > > Make sure the underlying VMA in the process address space is the
> > > same as it was during vm_mmap to avoid applying WC to wrong VMA.
> > > 
> > > A more long-term solution would be to have vm_mmap_locked variant
> > > in linux/mmap.h for when caller wants to hold mmap_sem for an
> > > extended duration.
> > > 
> > 
> > It seems like we may have a regression due to this patch. I am still
> > debugging, but I have a question; please see below.
> > 
> > Thanks,
> > Guenter
> > 
> > > v2:
> > > - Refactor the compare function
> > > 
> > > Fixes: 1816f9236303 ("drm/i915: Support creation of unbound wc user 
> > > mappings for objects")
> > > Reported-by: Adam Zabrocki 
> > > Suggested-by: Linus Torvalds 
> > > Signed-off-by: Joonas Lahtinen 
> > > Cc:  # v4.0+
> > > Cc: Akash Goel 
> > > Cc: Chris Wilson 
> > > Cc: Tvrtko Ursulin 
> > > Cc: Adam Zabrocki 
> > > Reviewed-by: Chris Wilson 
> > > Reviewed-by: Tvrtko Ursulin  #v1
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c | 12 +++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > > b/drivers/gpu/drm/i915/i915_gem.c
> > > index 05ce9176ac4e..52639f749908 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -1681,6 +1681,16 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, 
> > > void *data,
> > >   return 0;
> > >  }
> > >  
> > > +static inline bool
> > > +__vma_matches(struct vm_area_struct *vma, struct file *filp,
> > > +   unsigned long addr, unsigned long size)
> > > +{
> > > + if (vma->vm_file != filp)
> > > + return false;
> > > +
> > > + return vma->vm_start == addr && (vma->vm_end - vma->vm_start) == size;
> > 
> > Shouldn't this be:
> > return vma->vm_start == addr && (vma->vm_end - vma->vm_start + 1) == 
> > size;
> > instead ?
> > 
> 
> Answer is no .. because vm_end points to the first byte after the
> end address.
> 
> The actual values are:
> 
> start=7d288f7f9000 end=7d288f84d000 end-start=54000 size=53400
> 
> meaning the size field passed in the ioctl is smaller than the total length
> of the area.
> 
> Question is now: Is the request/ioctl indeed invalid, ie does the requested
> size have to match the vma size ? This used to work until this patch was
> applied, and the change causes our test code to fail (and possibly minigbm,
> which is used by the test code). That doesn't mean that our code is correct
> (I see some related local changes in our version of minigbm), but it is
> annoying, and I am being asked to revert this patch as regression
> from our kernel releases.
> 

In i915_gem_create():

size = roundup(size, PAGE_SIZE);
if (size == 0)
return -EINVAL;

This suggests to me that the requested size can be smaller than the
allocated size, which in turn suggests that the check
(vma->vm_end - vma->vm_start) == size;
is wrong. Either it should be
(vma->vm_end - vma->vm_start) >= size;
or possibly
(vma->vm_end - vma->vm_start) == roundup(size, PAGE_SIZE);

Any comments/feedback/thoughts ?

Thanks,
Guenter

> Thanks,
> Guenter
> 
> > > +}
> > > +
> > >  /**
> > >   * i915_gem_mmap_ioctl - Maps the contents of an object, returning the 
> > > address
> > >   *it is mapped to.
> > > @@ -1739,7 +1749,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void 
> > > *data,
> > >   return -EINTR;
> > >   }
> > >   vma = find_vma(mm, addr);
> > > - if (vma)
> > > + if (vma && __vma_matches(vma, obj->base.filp, addr, args->size))
> > >   vma->vm_page_prot =
> > >   
> > > pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> > >   else
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [CI, v3, 1/2] drm/i915: Prevent a race during I915_GEM_MMAP ioctl with WC set

[Guenter Roeck]

On Thu, Feb 28, 2019 at 11:12:49AM -0800, Guenter Roeck wrote:
> Hi,
> 
> On Thu, Feb 07, 2019 at 10:54:53AM +0200, Joonas Lahtinen wrote:
> > Make sure the underlying VMA in the process address space is the
> > same as it was during vm_mmap to avoid applying WC to wrong VMA.
> > 
> > A more long-term solution would be to have vm_mmap_locked variant
> > in linux/mmap.h for when caller wants to hold mmap_sem for an
> > extended duration.
> > 
> 
> It seems like we may have a regression due to this patch. I am still
> debugging, but I have a question; please see below.
> 
> Thanks,
> Guenter
> 
> > v2:
> > - Refactor the compare function
> > 
> > Fixes: 1816f9236303 ("drm/i915: Support creation of unbound wc user 
> > mappings for objects")
> > Reported-by: Adam Zabrocki 
> > Suggested-by: Linus Torvalds 
> > Signed-off-by: Joonas Lahtinen 
> > Cc:  # v4.0+
> > Cc: Akash Goel 
> > Cc: Chris Wilson 
> > Cc: Tvrtko Ursulin 
> > Cc: Adam Zabrocki 
> > Reviewed-by: Chris Wilson 
> > Reviewed-by: Tvrtko Ursulin  #v1
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 12 +++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index 05ce9176ac4e..52639f749908 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1681,6 +1681,16 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, 
> > void *data,
> > return 0;
> >  }
> >  
> > +static inline bool
> > +__vma_matches(struct vm_area_struct *vma, struct file *filp,
> > + unsigned long addr, unsigned long size)
> > +{
> > +   if (vma->vm_file != filp)
> > +   return false;
> > +
> > +   return vma->vm_start == addr && (vma->vm_end - vma->vm_start) == size;
> 
> Shouldn't this be:
>   return vma->vm_start == addr && (vma->vm_end - vma->vm_start + 1) == 
> size;
> instead ?
> 

Answer is no .. because vm_end points to the first byte after the
end address.

The actual values are:

start=7d288f7f9000 end=7d288f84d000 end-start=54000 size=53400

meaning the size field passed in the ioctl is smaller than the total length
of the area.

Question is now: Is the request/ioctl indeed invalid, ie does the requested
size have to match the vma size ? This used to work until this patch was
applied, and the change causes our test code to fail (and possibly minigbm,
which is used by the test code). That doesn't mean that our code is correct
(I see some related local changes in our version of minigbm), but it is
annoying, and I am being asked to revert this patch as regression
from our kernel releases.

Thanks,
Guenter

> > +}
> > +
> >  /**
> >   * i915_gem_mmap_ioctl - Maps the contents of an object, returning the 
> > address
> >   *  it is mapped to.
> > @@ -1739,7 +1749,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void 
> > *data,
> > return -EINTR;
> > }
> > vma = find_vma(mm, addr);
> > -   if (vma)
> > +   if (vma && __vma_matches(vma, obj->base.filp, addr, args->size))
> > vma->vm_page_prot =
> > 
> > pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> > else
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [CI, v3, 1/2] drm/i915: Prevent a race during I915_GEM_MMAP ioctl with WC set

[Guenter Roeck]

Hi,

On Thu, Feb 07, 2019 at 10:54:53AM +0200, Joonas Lahtinen wrote:
> Make sure the underlying VMA in the process address space is the
> same as it was during vm_mmap to avoid applying WC to wrong VMA.
> 
> A more long-term solution would be to have vm_mmap_locked variant
> in linux/mmap.h for when caller wants to hold mmap_sem for an
> extended duration.
> 

It seems like we may have a regression due to this patch. I am still
debugging, but I have a question; please see below.

Thanks,
Guenter

> v2:
> - Refactor the compare function
> 
> Fixes: 1816f9236303 ("drm/i915: Support creation of unbound wc user mappings 
> for objects")
> Reported-by: Adam Zabrocki 
> Suggested-by: Linus Torvalds 
> Signed-off-by: Joonas Lahtinen 
> Cc:  # v4.0+
> Cc: Akash Goel 
> Cc: Chris Wilson 
> Cc: Tvrtko Ursulin 
> Cc: Adam Zabrocki 
> Reviewed-by: Chris Wilson 
> Reviewed-by: Tvrtko Ursulin  #v1
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 05ce9176ac4e..52639f749908 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1681,6 +1681,16 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void 
> *data,
>   return 0;
>  }
>  
> +static inline bool
> +__vma_matches(struct vm_area_struct *vma, struct file *filp,
> +   unsigned long addr, unsigned long size)
> +{
> + if (vma->vm_file != filp)
> + return false;
> +
> + return vma->vm_start == addr && (vma->vm_end - vma->vm_start) == size;

Shouldn't this be:
return vma->vm_start == addr && (vma->vm_end - vma->vm_start + 1) == 
size;
instead ?

> +}
> +
>  /**
>   * i915_gem_mmap_ioctl - Maps the contents of an object, returning the 
> address
>   *it is mapped to.
> @@ -1739,7 +1749,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
>   return -EINTR;
>   }
>   vma = find_vma(mm, addr);
> - if (vma)
> + if (vma && __vma_matches(vma, obj->base.filp, addr, args->size))
>   vma->vm_page_prot =
>   
> pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>   else
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Guenter Roeck]

On 1/21/19 7:17 AM, Vincent Guittot wrote:

On Fri, 18 Jan 2019 at 13:08, Guenter Roeck  wrote:


On 1/18/19 3:05 AM, Rafael J. Wysocki wrote:

On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot
 wrote:


On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
 wrote:


Hi Guenter,

Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :

On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:

From: Thara Gopinath 

This patch replaces jiffies based accounting for runtime_active_time
and runtime_suspended_time with ktime base accounting. This makes the
runtime debug counters inline with genpd and other pm subsytems which
uses ktime based accounting.

timekeeping is initialized before pm_runtime_init() so ktime_get() will
be ready before first call. In fact, timekeeping_init() is called early
in start_kernel() which is way before driver_init() (and that's when
devices can start to be initialized) called from rest_init() via
kernel_init_freeable() and do_basic_setup().


This is not (always) correct. My qemu "collie" boot test fails with this
patch applied. Reverting the patch fixes the problem. Bisect log attached.



Can you try the patch below ?
ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
it can be used at early_init.


Another possibility would be delay the init of the gpiochip


Well, right.

Initializing devices before timekeeping doesn't feel particularly
robust from the design perspective.

How exactly does that happen?



With an added 'initialized' flag and backtrace into the timekeeping code,
with the change suggested earlier applied:

[ cut here ]
WARNING: CPU: 0 PID: 0 at kernel/time/timekeeping.c:453 
ktime_get_mono_fast_ns+0x114/0x12c
Timekeeping not initialized
CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc2-next-20190117-dirty #2
Hardware name: Sharp-Collie
Backtrace:
[] (dump_backtrace) from [] (show_stack+0x18/0x1c)
   r7:0009 r6: r5:c065ba90 r4:c06d3e54
[] (show_stack) from [] (dump_stack+0x20/0x28)
[] (dump_stack) from [] (__warn+0xcc/0xf4)
[] (__warn) from [] (warn_slowpath_fmt+0x4c/0x6c)
   r8:df407b08 r7: r6:c0c01550 r5:c065bad8 r4:c06dd028
[] (warn_slowpath_fmt) from [] 
(ktime_get_mono_fast_ns+0x114/0x12c)
   r3: r2:c065bad8
   r5: r4:df407b08
[] (ktime_get_mono_fast_ns) from [] 
(pm_runtime_init+0x38/0xb8)
   r9:c06c9a5c r8:df407b08 r7: r6:c0c01550 r5: r4:df407b08
[] (pm_runtime_init) from [] (device_initialize+0xb0/0xec)
   r7: r6:c0c01550 r5: r4:df407b08
[] (device_initialize) from [] 
(gpiochip_add_data_with_key+0x9c/0x884)
   r7: r6:c06fca34 r5: r4:
[] (gpiochip_add_data_with_key) from [] 
(sa1100_init_gpio+0x40/0x98)
   r10:dfffcd60 r9:c06c9a5c r8:c06dd020 r7:c06dd028 r6: r5:
   r4:c06fca34
[] (sa1100_init_gpio) from [] (sa1100_init_irq+0x2c/0x3c)
   r7:c06dd028 r6: r5:c0713300 r4:c06e1070
[] (sa1100_init_irq) from [] (init_IRQ+0x20/0x28)
   r5:c0713300 r4:
[] (init_IRQ) from [] (start_kernel+0x254/0x4cc)
[] (start_kernel) from [<>] (  (null))
   r10:717f r9:6901b119 r8:c100 r7:0092 r6:313d r5:0053
   r4:c06a7330
---[ end trace 91e1bd00dd7cce32 ]---


Does it means that only the pm_runtime_init is done before
timekeeping_init() but no update_pm_runtime_accounting() ?
In this case, we can keep using ktimeçget in
update_pm_runtime_accounting() and find a solution to deal with
early_call of pm_runtime_init()



For this platform that is correct. I can't answer for the generic case.

Guenter

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Guenter Roeck]

On 1/18/19 3:05 AM, Rafael J. Wysocki wrote:

On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot
 wrote:


On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
 wrote:


Hi Guenter,

Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :

On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:

From: Thara Gopinath 

This patch replaces jiffies based accounting for runtime_active_time
and runtime_suspended_time with ktime base accounting. This makes the
runtime debug counters inline with genpd and other pm subsytems which
uses ktime based accounting.

timekeeping is initialized before pm_runtime_init() so ktime_get() will
be ready before first call. In fact, timekeeping_init() is called early
in start_kernel() which is way before driver_init() (and that's when
devices can start to be initialized) called from rest_init() via
kernel_init_freeable() and do_basic_setup().


This is not (always) correct. My qemu "collie" boot test fails with this
patch applied. Reverting the patch fixes the problem. Bisect log attached.



Can you try the patch below ?
ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
it can be used at early_init.


Another possibility would be delay the init of the gpiochip


Well, right.

Initializing devices before timekeeping doesn't feel particularly
robust from the design perspective.

How exactly does that happen?



With an added 'initialized' flag and backtrace into the timekeeping code,
with the change suggested earlier applied:

[ cut here ]
WARNING: CPU: 0 PID: 0 at kernel/time/timekeeping.c:453 
ktime_get_mono_fast_ns+0x114/0x12c
Timekeeping not initialized
CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc2-next-20190117-dirty #2
Hardware name: Sharp-Collie
Backtrace:
[] (dump_backtrace) from [] (show_stack+0x18/0x1c)
 r7:0009 r6: r5:c065ba90 r4:c06d3e54
[] (show_stack) from [] (dump_stack+0x20/0x28)
[] (dump_stack) from [] (__warn+0xcc/0xf4)
[] (__warn) from [] (warn_slowpath_fmt+0x4c/0x6c)
 r8:df407b08 r7: r6:c0c01550 r5:c065bad8 r4:c06dd028
[] (warn_slowpath_fmt) from [] 
(ktime_get_mono_fast_ns+0x114/0x12c)
 r3: r2:c065bad8
 r5: r4:df407b08
[] (ktime_get_mono_fast_ns) from [] 
(pm_runtime_init+0x38/0xb8)
 r9:c06c9a5c r8:df407b08 r7: r6:c0c01550 r5: r4:df407b08
[] (pm_runtime_init) from [] (device_initialize+0xb0/0xec)
 r7: r6:c0c01550 r5: r4:df407b08
[] (device_initialize) from [] 
(gpiochip_add_data_with_key+0x9c/0x884)
 r7: r6:c06fca34 r5: r4:
[] (gpiochip_add_data_with_key) from [] 
(sa1100_init_gpio+0x40/0x98)
 r10:dfffcd60 r9:c06c9a5c r8:c06dd020 r7:c06dd028 r6: r5:
 r4:c06fca34
[] (sa1100_init_gpio) from [] (sa1100_init_irq+0x2c/0x3c)
 r7:c06dd028 r6: r5:c0713300 r4:c06e1070
[] (sa1100_init_irq) from [] (init_IRQ+0x20/0x28)
 r5:c0713300 r4:
[] (init_IRQ) from [] (start_kernel+0x254/0x4cc)
[] (start_kernel) from [<>] (  (null))
 r10:717f r9:6901b119 r8:c100 r7:0092 r6:313d r5:0053
 r4:c06a7330
---[ end trace 91e1bd00dd7cce32 ]---

Guenter
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Guenter Roeck]

On 1/18/19 2:42 AM, Vincent Guittot wrote:

Hi Guenter,

Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :

On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:

From: Thara Gopinath 

This patch replaces jiffies based accounting for runtime_active_time
and runtime_suspended_time with ktime base accounting. This makes the
runtime debug counters inline with genpd and other pm subsytems which
uses ktime based accounting.

timekeeping is initialized before pm_runtime_init() so ktime_get() will
be ready before first call. In fact, timekeeping_init() is called early
in start_kernel() which is way before driver_init() (and that's when
devices can start to be initialized) called from rest_init() via
kernel_init_freeable() and do_basic_setup().


This is not (always) correct. My qemu "collie" boot test fails with this
patch applied. Reverting the patch fixes the problem. Bisect log attached.



Can you try the patch below ?
ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
it can be used at early_init.


Yes, that works.

Guenter


---
  drivers/base/power/runtime.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index ae1c728..118c7f6 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -66,7 +66,7 @@ static int rpm_suspend(struct device *dev, int rpmflags);
   */
  void update_pm_runtime_accounting(struct device *dev)
  {
-   u64 now = ktime_to_ns(ktime_get());
+   u64 now = ktime_get_mono_fast_ns();
u64 delta;
  
  	delta = now - dev->power.accounting_timestamp;

@@ -1507,7 +1507,7 @@ void pm_runtime_init(struct device *dev)
dev->power.request_pending = false;
dev->power.request = RPM_REQ_NONE;
dev->power.deferred_resume = false;
-   dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
+   dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
INIT_WORK(>power.work, pm_runtime_work);
  
  	dev->power.timer_expires = 0;




___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Guenter Roeck]

On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> From: Thara Gopinath 
> 
> This patch replaces jiffies based accounting for runtime_active_time
> and runtime_suspended_time with ktime base accounting. This makes the
> runtime debug counters inline with genpd and other pm subsytems which
> uses ktime based accounting.
> 
> timekeeping is initialized before pm_runtime_init() so ktime_get() will
> be ready before first call. In fact, timekeeping_init() is called early
> in start_kernel() which is way before driver_init() (and that's when
> devices can start to be initialized) called from rest_init() via
> kernel_init_freeable() and do_basic_setup().
> 
This is not (always) correct. My qemu "collie" boot test fails with this
patch applied. Reverting the patch fixes the problem. Bisect log attached.

With some added debugging:

...
IRQS: 16, nr_irqs: 65, preallocated irqs: 65
irq: Cannot allocate irq_descs @ IRQ1, assuming pre-allocated
gpio gpiochip0: ### pm_runtime_init() 
irq: Cannot allocate irq_descs @ IRQ33, assuming pre-allocated
## timekeeping_init() 
sched_clock: 32 bits at 3686kHz, resolution 271ns, wraps every 58254200ns^M
...

This is with:

void pm_runtime_init(struct device *dev)
{
+   dev_info(dev, "### pm_runtime_init() \n");
+
...
@@ -1526,6 +1526,8 @@ void __init timekeeping_init(void)
struct clocksource *clock;
unsigned long flags;
 
+   pr_info("## timekeeping_init() \n");
+

Guenter

---
# bad: [a37d50ca3b837c19a297f349365d11a20c1087d0] Add linux-next specific files 
for 20190117
# good: [1c7fc5cbc33980acd13d668f1c8f0313d6ae9fd8] Linux 5.0-rc2
git bisect start 'HEAD' 'v5.0-rc2'
# bad: [4edb817d29fdf19fb5d52784bb3c29c40e00f586] Merge remote-tracking branch 
'pm/linux-next'
git bisect bad 4edb817d29fdf19fb5d52784bb3c29c40e00f586
# good: [6d95886720d306a1914a7c9a8aeb0bcbc7aef018] Merge remote-tracking branch 
'omap/for-next'
git bisect good 6d95886720d306a1914a7c9a8aeb0bcbc7aef018
# good: [975b5cdd74430bc8a04f832d65a47cdb95b73fec] Merge remote-tracking branch 
'fuse/for-next'
git bisect good 975b5cdd74430bc8a04f832d65a47cdb95b73fec
# good: [112386d2189fc54b979c3a8bf64b2908df91e123] Merge remote-tracking branch 
'i2c/i2c/for-next'
git bisect good 112386d2189fc54b979c3a8bf64b2908df91e123
# good: [3943f059823b6e15884387f31618b84826e924b3] media: coda: Add control for 
h.264 chroma qp index offset
git bisect good 3943f059823b6e15884387f31618b84826e924b3
# good: [44970b5079ee100875b06e45db5d6e91a16e] Merge remote-tracking branch 
'v4l-dvb/master'
git bisect good 44970b5079ee100875b06e45db5d6e91a16e
# bad: [599170c2b860aca3364574f833bb9cc801c9668d] Merge branch 'pm-core' into 
linux-next
git bisect bad 599170c2b860aca3364574f833bb9cc801c9668d
# good: [347d570919ca9a3a3653ce9cbb7399c7c0ba8248] Merge branch 'acpi-pci' into 
linux-next
git bisect good 347d570919ca9a3a3653ce9cbb7399c7c0ba8248
# good: [e0a9fde86ba1bc26dd754c733b32e70cd8f1c187] Merge branches 'acpi-tables' 
and 'acpi-apei' into linux-next
git bisect good e0a9fde86ba1bc26dd754c733b32e70cd8f1c187
# good: [3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567] drm/i915: Move on the new pm 
runtime interface
git bisect good 3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567
# bad: [c75c303933a68c547f3352d1d708843f9449d3f4] PM: clock_ops: fix missing 
clk_prepare() return value check
git bisect bad c75c303933a68c547f3352d1d708843f9449d3f4
# bad: [3982ab9ce433efc72ca31eb9ddc85d9355966444] PM-runtime: Replace jiffies 
based accounting with ktime-based accounting
git bisect bad 3982ab9ce433efc72ca31eb9ddc85d9355966444
# first bad commit: [3982ab9ce433efc72ca31eb9ddc85d9355966444] PM-runtime: 
Replace jiffies based accounting with ktime-based accounting
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate

[Guenter Roeck]

On Wed, Mar 30, 2016 at 10:03:31PM +0200, Boris Brezillon wrote:
> The PWM framework has clarified the concept of reference PWM config
> (the platform dependent config retrieved from the DT or the PWM
> lookup table) and real PWM state.
> 
> Use pwm_get_args() when the PWM user wants to retrieve this reference
> config and not the current state.
> 
> This is part of the rework allowing the PWM framework to support
> hardware readout and expose real PWM state even when the PWM has
> just been requested (before the user calls pwm_config/enable/disable()).
> 
> Signed-off-by: Boris Brezillon 
> ---
>  drivers/hwmon/pwm-fan.c | 19 +--
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 3e23003..82c5656 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -40,15 +40,18 @@ struct pwm_fan_ctx {
>  
>  static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
>  {
> + struct pwm_args pargs = { };

Hi Boris,

I guess I am missing some context; sorry for that. Unfortunately,
I did not easily find an explanation, so please bear with me.

Two questions: Why do we need a local copy of struct pwm_args instead
of a pointer to it ? If it can change while being used, isn't it
inconsistent anyway ?

Also, assuming the local copy is necessary, why initialize pargs ? 
After all, pwm_get_args() just overwrites it.

Thanks,
Guenter

>   unsigned long duty;
>   int ret = 0;
>  
> + pwm_get_args(ctx->pwm, );
> +
>   mutex_lock(>lock);
>   if (ctx->pwm_value == pwm)
>   goto exit_set_pwm_err;
>  
> - duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
> - ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
> + duty = DIV_ROUND_UP(pwm * (pargs.period - 1), MAX_PWM);
> + ret = pwm_config(ctx->pwm, duty, pargs.period);
>   if (ret)
>   goto exit_set_pwm_err;
>  
> @@ -214,6 +217,7 @@ static int pwm_fan_of_get_cooling_data(struct device *dev,
>  static int pwm_fan_probe(struct platform_device *pdev)
>  {
>   struct thermal_cooling_device *cdev;
> + struct pwm_args pargs = { };
>   struct pwm_fan_ctx *ctx;
>   struct device *hwmon;
>   int duty_cycle;
> @@ -234,10 +238,11 @@ static int pwm_fan_probe(struct platform_device *pdev)
>   platform_set_drvdata(pdev, ctx);
>  
>   /* Set duty cycle to maximum allowed */
> - duty_cycle = ctx->pwm->period - 1;
> + pwm_get_args(ctx->pwm, );
> + duty_cycle = pargs.period - 1;
>   ctx->pwm_value = MAX_PWM;
>  
> - ret = pwm_config(ctx->pwm, duty_cycle, ctx->pwm->period);
> + ret = pwm_config(ctx->pwm, duty_cycle, pargs.period);
>   if (ret) {
>   dev_err(>dev, "Failed to configure PWM\n");
>   return ret;
> @@ -303,14 +308,16 @@ static int pwm_fan_suspend(struct device *dev)
>  static int pwm_fan_resume(struct device *dev)
>  {
>   struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> + struct pwm_args pargs = { };
>   unsigned long duty;
>   int ret;
>  
>   if (ctx->pwm_value == 0)
>   return 0;
>  
> - duty = DIV_ROUND_UP(ctx->pwm_value * (ctx->pwm->period - 1), MAX_PWM);
> - ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
> + pwm_get_args(ctx->pwm, );
> + duty = DIV_ROUND_UP(ctx->pwm_value * (pargs.period - 1), MAX_PWM);
> + ret = pwm_config(ctx->pwm, duty, pargs.period);
>   if (ret)
>   return ret;
>   return pwm_enable(ctx->pwm);
> -- 
> 2.5.0
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] INTEL DRM DRIVERS : No LVDS hardware on Intel D410PT and D425KT

[Guenter Roeck]

On 10/27/2013 10:33 AM, Greg KH wrote:

On Sun, Oct 27, 2013 at 04:13:42PM +, Rob Pearce wrote:

From: Rob Pearce r...@flitspace.org.uk

The Intel D410PT(LW) and D425KT Mini-ITX desktop boards both show up as
having LVDS but the hardware is not populated. This patch adds them to
the list of such systems. Patch is against 3.11.4

Signed-off-by: Rob Pearce r...@flitspace.org.uk
---
Patch revised to match the D425KT exactly as the D425KTW does have LVDS.
According to Intel's documentation, the D410PTL and D410PLTW don't.


Any reason you don't want this in the stable tree as well?



Hi Greg,

pardon my ignorance, but I thought this was supposed to be the maintainer's 
call to make ?
Did I get this wrong ?

Thanks,
Guenter




___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] Revert drm/i915: unconditionally use mt forcewake on hsw/ivb

[Guenter Roeck]

This reverts commit 36ec8f877481449bdfa072e6adf2060869e2b970.

The commit results in repeated 'Timed out waiting for forcewake old ack
to clear' messages on a Supermicro C7H61 board (BIOS version 2.00 and 2.00b)
with i7-3770K CPU. It ultimately results in a hangup if the system is highly
loaded. Reverting the commit fixes the issue.

While I understand that this should be fixed in the BIOS, a more recent BIOS
is not available fron the vendor. If/since no better solution is not available,
reverting the patch seems to be the way to go (otherwise me and others with
the same problem won't be able to run the upstream kernel on affected boards).

Cc: Jesse Barnes jbar...@virtuousgeek.org
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: Mika Kuoppala mika.kuopp...@intel.com
Signed-off-by: Guenter Roeck li...@roeck-us.net
---
 drivers/gpu/drm/i915/intel_pm.c |   31 +++
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index aa01128..5bae9ba 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4510,12 +4510,35 @@ void intel_gt_init(struct drm_device *dev)
if (IS_VALLEYVIEW(dev)) {
dev_priv-gt.force_wake_get = vlv_force_wake_get;
dev_priv-gt.force_wake_put = vlv_force_wake_put;
-   } else if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
-   dev_priv-gt.force_wake_get = __gen6_gt_force_wake_mt_get;
-   dev_priv-gt.force_wake_put = __gen6_gt_force_wake_mt_put;
-   } else if (IS_GEN6(dev)) {
+   } else if (INTEL_INFO(dev)-gen = 6) {
dev_priv-gt.force_wake_get = __gen6_gt_force_wake_get;
dev_priv-gt.force_wake_put = __gen6_gt_force_wake_put;
+
+   /* IVB configs may use multi-threaded forcewake */
+   if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
+   u32 ecobus;
+
+   /* A small trick here - if the bios hasn't configured
+* MT forcewake, and if the device is in RC6, then
+* force_wake_mt_get will not wake the device and the
+* ECOBUS read will return zero. Which will be
+* (correctly) interpreted by the test below as MT
+* forcewake being disabled.
+*/
+   mutex_lock(dev-struct_mutex);
+   __gen6_gt_force_wake_mt_get(dev_priv);
+   ecobus = I915_READ_NOTRACE(ECOBUS);
+   __gen6_gt_force_wake_mt_put(dev_priv);
+   mutex_unlock(dev-struct_mutex);
+
+   if (ecobus  FORCEWAKE_MT_ENABLE) {
+   DRM_DEBUG_KMS(Using MT version of 
forcewake\n);
+   dev_priv-gt.force_wake_get =
+   __gen6_gt_force_wake_mt_get;
+   dev_priv-gt.force_wake_put =
+   __gen6_gt_force_wake_mt_put;
+   }
+   }
}
INIT_DELAYED_WORK(dev_priv-rps.delayed_resume_work,
  intel_gen6_powersave_work);
-- 
1.7.9.7

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] Revert drm/i915: unconditionally use mt forcewake on hsw/ivb

[Guenter Roeck]

On Tue, Jul 09, 2013 at 10:33:22PM +0200, Daniel Vetter wrote:
 On Tue, Jul 09, 2013 at 01:14:09PM -0700, Guenter Roeck wrote:
  This reverts commit 36ec8f877481449bdfa072e6adf2060869e2b970.
  
  The commit results in repeated 'Timed out waiting for forcewake old ack
  to clear' messages on a Supermicro C7H61 board (BIOS version 2.00 and 2.00b)
  with i7-3770K CPU. It ultimately results in a hangup if the system is highly
  loaded. Reverting the commit fixes the issue.
  
  While I understand that this should be fixed in the BIOS, a more recent BIOS
  is not available fron the vendor. If/since no better solution is not 
  available,
  reverting the patch seems to be the way to go (otherwise me and others with
  the same problem won't be able to run the upstream kernel on affected 
  boards).
  
  Cc: Jesse Barnes jbar...@virtuousgeek.org
  Cc: Daniel Vetter daniel.vet...@ffwll.ch
  Cc: Mika Kuoppala mika.kuopp...@intel.com
  Signed-off-by: Guenter Roeck li...@roeck-us.net
 
 Can you please add the bugzilla link here?
 
I'll create one and add a reference.

Thanks,
Guenter
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] Partially revert drm/i915: unconditionally use mt forcewake on hsw/ivb

[Guenter Roeck]

This patch partially reverts commit 36ec8f877481449bdfa072e6adf2060869e2b970 for
IvyBridge CPUs.

The original commit results in repeated 'Timed out waiting for forcewake old
ack to clear' messages on a Supermicro C7H61 board (BIOS version 2.00 and 2.00b)
with i7-3770K CPU. It ultimately results in a hangup if the system is highly
loaded. Reverting the commit for IvyBridge CPUs fixes the issue.

Issue a warning if the CPU is IvyBridge and mt forcewake is disabled, since
this condition can result in secondary issues.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=60541
Cc: Jesse Barnes jbar...@virtuousgeek.org
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: Mika Kuoppala mika.kuopp...@intel.com
Signed-off-by: Guenter Roeck li...@roeck-us.net
---
v2: Only revert patch for Ivybridge CPUs
Issue info message if mt forcewake is disabled on Ivybridge

 drivers/gpu/drm/i915/intel_pm.c |   31 ++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index aa01128..0392c28 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4510,9 +4510,38 @@ void intel_gt_init(struct drm_device *dev)
if (IS_VALLEYVIEW(dev)) {
dev_priv-gt.force_wake_get = vlv_force_wake_get;
dev_priv-gt.force_wake_put = vlv_force_wake_put;
-   } else if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
+   } else if (IS_HASWELL(dev)) {
dev_priv-gt.force_wake_get = __gen6_gt_force_wake_mt_get;
dev_priv-gt.force_wake_put = __gen6_gt_force_wake_mt_put;
+   } else if (IS_IVYBRIDGE(dev)) {
+   u32 ecobus;
+
+   /* IVB configs may use multi-threaded forcewake */
+
+   /* A small trick here - if the bios hasn't configured
+* MT forcewake, and if the device is in RC6, then
+* force_wake_mt_get will not wake the device and the
+* ECOBUS read will return zero. Which will be
+* (correctly) interpreted by the test below as MT
+* forcewake being disabled.
+*/
+   mutex_lock(dev-struct_mutex);
+   __gen6_gt_force_wake_mt_get(dev_priv);
+   ecobus = I915_READ_NOTRACE(ECOBUS);
+   __gen6_gt_force_wake_mt_put(dev_priv);
+   mutex_unlock(dev-struct_mutex);
+
+   if (ecobus  FORCEWAKE_MT_ENABLE) {
+   dev_priv-gt.force_wake_get =
+   __gen6_gt_force_wake_mt_get;
+   dev_priv-gt.force_wake_put =
+   __gen6_gt_force_wake_mt_put;
+   } else {
+   DRM_INFO(No MT forcewake available on Ivybridge, this 
can result in issues\n);
+   DRM_INFO(when using vblank-synced partial screen 
updates.\n);
+   dev_priv-gt.force_wake_get = __gen6_gt_force_wake_get;
+   dev_priv-gt.force_wake_put = __gen6_gt_force_wake_put;
+   }
} else if (IS_GEN6(dev)) {
dev_priv-gt.force_wake_get = __gen6_gt_force_wake_get;
dev_priv-gt.force_wake_put = __gen6_gt_force_wake_put;
-- 
1.7.9.7

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] 'Timed out waiting for forcewake old ack to clear' and hangup on IvyBridge system

[Guenter Roeck]

On Wed, Jun 26, 2013 at 09:24:07AM -0700, Jesse Barnes wrote:
 On Sat, 22 Jun 2013 13:04:09 -0700
 Guenter Roeck li...@roeck-us.net wrote:
 
  On Sat, Jun 22, 2013 at 12:16:46PM -0700, Jesse Barnes wrote:
   On Fri, 21 Jun 2013 23:58:08 -0700
   Guenter Roeck li...@roeck-us.net wrote:
   
Hi all,

after upgrading one of my servers to 3.8, then 3.9.7 and 3.10-rc6, I 
started to
see lots of Timed out waiting for forcewake old ack to clear error 
messages,
including hang-ups especially if the system was highly loaded. With 
3.5.24
everything was fine.

After backing out commit 36ec8f877 (drm/i915: unconditionally use mt 
forcewake
on hsw/ivb), everything is back to normal. The log message is still 
there, but
only once during boot, and the system runs stable.

CPU is Intel(R) Core(TM) i7-3770K CPU @ 3.50GHz, mainboard is 
Supermicro
C7H61, BIOS version 2.00 dated 11/02/2012. Configuration file is 
whatever
comes with Ubuntu; I'll be happy to provide a copy if anyone thinks it 
might
help.

Any idea what else I can do besides using a special kernel with the 
backed out
commit ? Is it possible that others have the same problem ?
   
   Ouch, so a BIOS that uses the other forcewake mechanism seems to have
   escaped.  Is there a newer one available for your system?  I'm hoping
   it'll fix the issue, otherwise we may have to introduce both methods
   for IVB again...
   
  I installed the latest BIOS version (2.00b), but it did not fix the problem.
  
  Is there some info (such as an Intel document describing what needs to be 
  done)
  which I could pass on to Supermicro ?
  
  I think it would be helpful if the condition was detected and reported, if 
  that
  is possible. I spent two days so far tracking this down. It would be nice
  if others would not have to go through the same experience.
 
 I don't think there's anything public to share, but it's not a big deal
 to simply revert the patch in question.  That seems like the right
 thing to do anyway since we'd like stuff to work out of the box as
 much as possible.
 
Agreed. Kind of unlikely that I can get Supermicro to listen to me anyway :(.

Who can initiate the revert ?

Thanks,
Guenter
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] 'Timed out waiting for forcewake old ack to clear' and hangup on IvyBridge system

[Guenter Roeck]

Hi all,

after upgrading one of my servers to 3.8, then 3.9.7 and 3.10-rc6, I started to
see lots of Timed out waiting for forcewake old ack to clear error messages,
including hang-ups especially if the system was highly loaded. With 3.5.24
everything was fine.

After backing out commit 36ec8f877 (drm/i915: unconditionally use mt forcewake
on hsw/ivb), everything is back to normal. The log message is still there, but
only once during boot, and the system runs stable.

CPU is Intel(R) Core(TM) i7-3770K CPU @ 3.50GHz, mainboard is Supermicro
C7H61, BIOS version 2.00 dated 11/02/2012. Configuration file is whatever
comes with Ubuntu; I'll be happy to provide a copy if anyone thinks it might
help.

Any idea what else I can do besides using a special kernel with the backed out
commit ? Is it possible that others have the same problem ?

Thanks,
Guenter
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] 'Timed out waiting for forcewake old ack to clear' and hangup on IvyBridge system

[Guenter Roeck]

On Sat, Jun 22, 2013 at 12:16:46PM -0700, Jesse Barnes wrote:
 On Fri, 21 Jun 2013 23:58:08 -0700
 Guenter Roeck li...@roeck-us.net wrote:
 
  Hi all,
  
  after upgrading one of my servers to 3.8, then 3.9.7 and 3.10-rc6, I 
  started to
  see lots of Timed out waiting for forcewake old ack to clear error 
  messages,
  including hang-ups especially if the system was highly loaded. With 3.5.24
  everything was fine.
  
  After backing out commit 36ec8f877 (drm/i915: unconditionally use mt 
  forcewake
  on hsw/ivb), everything is back to normal. The log message is still there, 
  but
  only once during boot, and the system runs stable.
  
  CPU is Intel(R) Core(TM) i7-3770K CPU @ 3.50GHz, mainboard is Supermicro
  C7H61, BIOS version 2.00 dated 11/02/2012. Configuration file is whatever
  comes with Ubuntu; I'll be happy to provide a copy if anyone thinks it might
  help.
  
  Any idea what else I can do besides using a special kernel with the backed 
  out
  commit ? Is it possible that others have the same problem ?
 
 Ouch, so a BIOS that uses the other forcewake mechanism seems to have
 escaped.  Is there a newer one available for your system?  I'm hoping
 it'll fix the issue, otherwise we may have to introduce both methods
 for IVB again...
 
I installed the latest BIOS version (2.00b), but it did not fix the problem.

Is there some info (such as an Intel document describing what needs to be done)
which I could pass on to Supermicro ?

I think it would be helpful if the condition was detected and reported, if that
is possible. I spent two days so far tracking this down. It would be nice
if others would not have to go through the same experience.

Thanks,
Guenter
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx