[PATCH 1/2] drm/i915/perf: Initialise dynamic sysfs group before creation

[PATCH 1/2] drm/i915/perf: Initialise dynamic sysfs group before creation

[Chris Wilson]

Another case where we need to call sysfs_attr_init() to setup the
internal lockdep class prior to use:

[    9.325229] BUG: key ffff880168bc7bb0 not in .data!
[    9.325240] DEBUG_LOCKS_WARN_ON(1)
[    9.325250] ------------[ cut here ]------------
[    9.325280] WARNING: CPU: 1 PID: 275 at kernel/locking/lockdep.c:3156 lockdep_init_map+0x1b2/0x1c0
[    9.325301] Modules linked in: intel_powerclamp(+) coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel i915(+) snd_hda_intel snd_hda_codec snd_hwdep r8169 mii snd_hda_core snd_pcm prime_numbers i2c_hid pinctrl_geminilake pinctrl_intel
[    9.325375] CPU: 1 PID: 275 Comm: modprobe Not tainted 4.13.0-rc4-CI-Trybot_1040+ #1
[    9.325395] Hardware name: Intel Corp. Geminilake/GLK RVP2 LP4SD (07), BIOS GELKRVPA.X64.0045.B51.1704281422 04/28/2017
[    9.325422] task: ffff8801721a4ec0 task.stack: ffffc900001dc000
[    9.325440] RIP: 0010:lockdep_init_map+0x1b2/0x1c0
[    9.325456] RSP: 0018:ffffc900001dfa10 EFLAGS: 00010282
[    9.325473] RAX: 0000000000000016 RBX: ffff880168d54b80 RCX: 0000000000000000
[    9.325488] RDX: 0000000080000001 RSI: 0000000000000001 RDI: ffffffff810f0800
[    9.325505] RBP: ffffc900001dfa30 R08: 0000000000000001 R09: 0000000000000000
[    9.325521] R10: 0000000000000000 R11: 0000000000000000 R12: ffff880168bc7bb0
[    9.325537] R13: 0000000000000000 R14: ffff880168bc7b98 R15: ffffffff81a263a0
[    9.325554] FS:  00007fb60c3fd700(0000) GS:ffff88017fc80000(0000) knlGS:0000000000000000
[    9.325574] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    9.325588] CR2: 0000006582777d80 CR3: 000000016d818000 CR4: 00000000003406e0
[    9.325604] Call Trace:
[    9.325618]  __kernfs_create_file+0x76/0xe0
[    9.325632]  sysfs_add_file_mode_ns+0x8a/0x1a0
[    9.325646]  internal_create_group+0xea/0x2c0
[    9.325660]  sysfs_create_group+0x13/0x20
[    9.325737]  i915_perf_register+0xde/0x220 [i915]
[    9.325800]  i915_driver_load+0xa77/0x16c0 [i915]
[    9.325863]  i915_pci_probe+0x37/0x90 [i915]
[    9.325880]  pci_device_probe+0xa8/0x130
[    9.325894]  driver_probe_device+0x29c/0x450
[    9.325908]  __driver_attach+0xe3/0xf0
[    9.325922]  ? driver_probe_device+0x450/0x450
[    9.325935]  bus_for_each_dev+0x62/0xa0
[    9.325948]  driver_attach+0x1e/0x20
[    9.325960]  bus_add_driver+0x173/0x270
[    9.325974]  driver_register+0x60/0xe0
[    9.325986]  __pci_register_driver+0x60/0x70
[    9.326044]  i915_init+0x6f/0x78 [i915]
[    9.326066]  ? 0xffffffffa024e000
[    9.326079]  do_one_initcall+0x43/0x170
[    9.326094]  ? rcu_read_lock_sched_held+0x7a/0x90
[    9.326109]  ? kmem_cache_alloc_trace+0x261/0x2d0
[    9.326124]  do_init_module+0x5f/0x206
[    9.326137]  load_module+0x2561/0x2da0
[    9.326150]  ? show_coresize+0x30/0x30
[    9.326165]  ? kernel_read_file+0x105/0x190
[    9.326180]  SyS_finit_module+0xc1/0x100
[    9.326192]  ? SyS_finit_module+0xc1/0x100
[    9.326210]  entry_SYSCALL_64_fastpath+0x1c/0xb1
[    9.326223] RIP: 0033:0x7fb60bf359f9
[    9.326234] RSP: 002b:00007fff92b47c48 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[    9.326255] RAX: ffffffffffffffda RBX: ffffffff814898a3 RCX: 00007fb60bf359f9
[    9.326271] RDX: 0000000000000000 RSI: 00000028a9ceef8b RDI: 0000000000000000
[    9.326287] RBP: ffffc900001dff88 R08: 0000000000000000 R09: 0000000000000000
[    9.326303] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000040000
[    9.326319] R13: 00000028aaef2a70 R14: 0000000000000000 R15: 00000028aaeee5d0
[    9.326339]  ? __this_cpu_preempt_check+0x13/0x20
[    9.326353] Code: f1 39 00 85 c0 0f 84 38 ff ff ff 83 3d 9f 44 ce 01 00 0f 85 2b ff ff ff 48 c7 c6 b2 a2 c7 81 48 c7 c7 53 40 c5 81 e8 3f 82 01 00 <0f> ff e9 11 ff ff ff 0f 1f 80 00 00 00 00 55 31 c9 31 d2 31 f6

Fixes: 701f8231a2fe ("drm/i915/perf: prune OA configs")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_perf.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index e3e2663117e9..1be355d14e8a 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2908,8 +2908,7 @@ void i915_perf_register(struct drm_i915_private *dev_priv)
 	if (!dev_priv->perf.metrics_kobj)
 		goto exit;
 
-	memset(&dev_priv->perf.oa.test_config, 0,
-	       sizeof(dev_priv->perf.oa.test_config));
+	sysfs_attr_init(&dev_priv->perf.oa.test_config.sysfs_metric_id.attr);
 
 	if (IS_HASWELL(dev_priv)) {
 		i915_perf_load_test_config_hsw(dev_priv);
-- 
2.13.3

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

[PATCH 2/2] drm/i915/perf: Drop lockdep assert for i915_oa_init_reg_state()

[Chris Wilson]

This is called from execlist context init which we need to be unlocked.
Commit f89823c21224 ("drm/i915/perf: Implement
I915_PERF_ADD/REMOVE_CONFIG interface") added a lockdep assert to this
path for unclear reasons, remove it again!

Fixes: 701f8231a2fe ("drm/i915/perf: prune OA configs")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_perf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 1be355d14e8a..3bdf53faae24 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2173,8 +2173,6 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
 	struct drm_i915_private *dev_priv = engine->i915;
 	struct i915_perf_stream *stream = dev_priv->perf.oa.exclusive_stream;
 
-	lockdep_assert_held(&dev_priv->drm.struct_mutex);
-
 	if (engine->id != RCS)
 		return;
 
-- 
2.13.3

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

Re: [PATCH 2/2] drm/i915/perf: Drop lockdep assert for i915_oa_init_reg_state()

[Lionel Landwerlin]

On 09/08/17 16:38, Chris Wilson wrote:
> This is called from execlist context init which we need to be unlocked.
> Commit f89823c21224 ("drm/i915/perf: Implement
> I915_PERF_ADD/REMOVE_CONFIG interface") added a lockdep assert to this
> path for unclear reasons, remove it again!
>
> Fixes: 701f8231a2fe ("drm/i915/perf: prune OA configs")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_perf.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 1be355d14e8a..3bdf53faae24 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -2173,8 +2173,6 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
>   	struct drm_i915_private *dev_priv = engine->i915;
>   	struct i915_perf_stream *stream = dev_priv->perf.oa.exclusive_stream;

I was trying to avoid adding a new lock for exclusive_stream.
If we can't rely on dev_priv->drm.struct_mutex to update 
exclusive_stream, I believe we need to add a new lock.
Or maybe some other mechanism?

>   
> -	lockdep_assert_held(&dev_priv->drm.struct_mutex);
> -
>   	if (engine->id != RCS)
>   		return;
>   


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

Re: [PATCH 2/2] drm/i915/perf: Drop lockdep assert for i915_oa_init_reg_state()

[Chris Wilson]

Quoting Lionel Landwerlin (2017-08-09 23:47:20)
> On 09/08/17 16:38, Chris Wilson wrote:
> > This is called from execlist context init which we need to be unlocked.
> > Commit f89823c21224 ("drm/i915/perf: Implement
> > I915_PERF_ADD/REMOVE_CONFIG interface") added a lockdep assert to this
> > path for unclear reasons, remove it again!
> >
> > Fixes: 701f8231a2fe ("drm/i915/perf: prune OA configs")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > Cc: Matthew Auld <matthew.auld@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_perf.c | 2 --
> >   1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index 1be355d14e8a..3bdf53faae24 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -2173,8 +2173,6 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
> >       struct drm_i915_private *dev_priv = engine->i915;
> >       struct i915_perf_stream *stream = dev_priv->perf.oa.exclusive_stream;
> 
> I was trying to avoid adding a new lock for exclusive_stream.
> If we can't rely on dev_priv->drm.struct_mutex to update 
> exclusive_stream, I believe we need to add a new lock.
> Or maybe some other mechanism?

Doesn't changing the exclusive_stream require serialization against the
requests? Therefore we would be safe here for reset as the existence of
the incomplete request that we are altering is the barrier.

Wait... You don't have that barrier anymore? So you never change the
context image for the exclusive stream on setting and force the reload
of the image? I expected to see something like
gen8_configure_all_contexts(), but only needing to change the old/new
exclusive_streams.

At any rate, just wrapping exclusive_stream = bah inside struct_mutex is
not the barrier you intended, or at least not the barrier I think you need
wrt to request construction and concurrent execution thereof.

Longer term, I don't plan for the context allocation/initialisation to
be under the struct_mutex, but we can still expect it to be part of the
request construction, so would still be serialized by a future semaphore
between oa and execbuf.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/perf: Drop lockdep assert for i915_oa_init_reg_state()

[Lionel Landwerlin]

On 10/08/17 00:13, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2017-08-09 23:47:20)
>> On 09/08/17 16:38, Chris Wilson wrote:
>>> This is called from execlist context init which we need to be unlocked.
>>> Commit f89823c21224 ("drm/i915/perf: Implement
>>> I915_PERF_ADD/REMOVE_CONFIG interface") added a lockdep assert to this
>>> path for unclear reasons, remove it again!
>>>
>>> Fixes: 701f8231a2fe ("drm/i915/perf: prune OA configs")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_perf.c | 2 --
>>>    1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>>> index 1be355d14e8a..3bdf53faae24 100644
>>> --- a/drivers/gpu/drm/i915/i915_perf.c
>>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>>> @@ -2173,8 +2173,6 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
>>>        struct drm_i915_private *dev_priv = engine->i915;
>>>        struct i915_perf_stream *stream = dev_priv->perf.oa.exclusive_stream;
>> I was trying to avoid adding a new lock for exclusive_stream.
>> If we can't rely on dev_priv->drm.struct_mutex to update
>> exclusive_stream, I believe we need to add a new lock.
>> Or maybe some other mechanism?
> Doesn't changing the exclusive_stream require serialization against the
> requests? Therefore we would be safe here for reset as the existence of
> the incomplete request that we are altering is the barrier.

We serialize the requests to make sure that our modifications to the 
context image have been applied.
So that when we return the perf stream fd, all work that was submitted 
before the context image was modified has retired.

When new contexts are created, there is a need to set the OA parameters 
in their image. But that has to read from the currently opened stream.
So there must be some kind of synchronization there.

I can make the access to exclusive_stream go through an srcu. Does that 
sound like the right way to do it?

>
> Wait... You don't have that barrier anymore? So you never change the
> context image for the exclusive stream on setting and force the reload
> of the image? I expected to see something like
> gen8_configure_all_contexts(), but only needing to change the old/new
> exclusive_streams.
>
> At any rate, just wrapping exclusive_stream = bah inside struct_mutex is
> not the barrier you intended, or at least not the barrier I think you need
> wrt to request construction and concurrent execution thereof.
>
> Longer term, I don't plan for the context allocation/initialisation to
> be under the struct_mutex, but we can still expect it to be part of the
> request construction, so would still be serialized by a future semaphore
> between oa and execbuf.
> -Chris
>

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

Re: [PATCH 2/2] drm/i915/perf: Drop lockdep assert for i915_oa_init_reg_state()

[Lionel Landwerlin]

On 10/08/17 00:13, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2017-08-09 23:47:20)
>> On 09/08/17 16:38, Chris Wilson wrote:
>>> This is called from execlist context init which we need to be unlocked.
>>> Commit f89823c21224 ("drm/i915/perf: Implement
>>> I915_PERF_ADD/REMOVE_CONFIG interface") added a lockdep assert to this
>>> path for unclear reasons, remove it again!
>>>
>>> Fixes: 701f8231a2fe ("drm/i915/perf: prune OA configs")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_perf.c | 2 --
>>>    1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>>> index 1be355d14e8a..3bdf53faae24 100644
>>> --- a/drivers/gpu/drm/i915/i915_perf.c
>>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>>> @@ -2173,8 +2173,6 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
>>>        struct drm_i915_private *dev_priv = engine->i915;
>>>        struct i915_perf_stream *stream = dev_priv->perf.oa.exclusive_stream;
>> I was trying to avoid adding a new lock for exclusive_stream.
>> If we can't rely on dev_priv->drm.struct_mutex to update
>> exclusive_stream, I believe we need to add a new lock.
>> Or maybe some other mechanism?
> Doesn't changing the exclusive_stream require serialization against the
> requests? Therefore we would be safe here for reset as the existence of
> the incomplete request that we are altering is the barrier.
>
> Wait... You don't have that barrier anymore? So you never change the
> context image for the exclusive stream on setting and force the reload
> of the image? I expected to see something like
> gen8_configure_all_contexts(), but only needing to change the old/new
> exclusive_streams.

We serialize the requests to make sure that our modifications to the 
context image have been applied.
So that when we return the perf stream fd, all work that was submitted 
before the context image was modified has retired.


When new contexts are created, there is a need to set the OA parameters 
in their images. But that has to read configuration from the currently 
opened stream.
So there must be some kind of synchronization there.

I can make the access to exclusive_stream go through a srcu. Do that 
sound right?


>
> At any rate, just wrapping exclusive_stream = bah inside struct_mutex is
> not the barrier you intended, or at least not the barrier I think you need
> wrt to request construction and concurrent execution thereof.
>
> Longer term, I don't plan for the context allocation/initialisation to
> be under the struct_mutex, but we can still expect it to be part of the
> request construction, so would still be serialized by a future semaphore
> between oa and execbuf.
> -Chris
>

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

Re: [PATCH 2/2] drm/i915/perf: Drop lockdep assert for i915_oa_init_reg_state()

[Chris Wilson]

Quoting Lionel Landwerlin (2017-08-10 08:23:20)
> On 10/08/17 00:13, Chris Wilson wrote:
> > Quoting Lionel Landwerlin (2017-08-09 23:47:20)
> >> On 09/08/17 16:38, Chris Wilson wrote:
> >>> This is called from execlist context init which we need to be unlocked.
> >>> Commit f89823c21224 ("drm/i915/perf: Implement
> >>> I915_PERF_ADD/REMOVE_CONFIG interface") added a lockdep assert to this
> >>> path for unclear reasons, remove it again!
> >>>
> >>> Fixes: 701f8231a2fe ("drm/i915/perf: prune OA configs")
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> >>> Cc: Matthew Auld <matthew.auld@intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_perf.c | 2 --
> >>>    1 file changed, 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> >>> index 1be355d14e8a..3bdf53faae24 100644
> >>> --- a/drivers/gpu/drm/i915/i915_perf.c
> >>> +++ b/drivers/gpu/drm/i915/i915_perf.c
> >>> @@ -2173,8 +2173,6 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
> >>>        struct drm_i915_private *dev_priv = engine->i915;
> >>>        struct i915_perf_stream *stream = dev_priv->perf.oa.exclusive_stream;
> >> I was trying to avoid adding a new lock for exclusive_stream.
> >> If we can't rely on dev_priv->drm.struct_mutex to update
> >> exclusive_stream, I believe we need to add a new lock.
> >> Or maybe some other mechanism?
> > Doesn't changing the exclusive_stream require serialization against the
> > requests? Therefore we would be safe here for reset as the existence of
> > the incomplete request that we are altering is the barrier.
> 
> We serialize the requests to make sure that our modifications to the 
> context image have been applied.
> So that when we return the perf stream fd, all work that was submitted 
> before the context image was modified has retired.
> 
> When new contexts are created, there is a need to set the OA parameters 
> in their image. But that has to read from the currently opened stream.
> So there must be some kind of synchronization there.
> 
> I can make the access to exclusive_stream go through an srcu. Does that 
> sound like the right way to do it?

To put it simply any sleeping lock within the reset path is a nuisance.
We may very well want to perform the reset from interrupt/softirq
context (for handling watchdogs).

All you need to do is to stage the change, and only apply it once you
have serialized the request stream. That's your barrier.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx