From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, Intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator
Date: Wed, 13 Jan 2016 14:35:06 +0000 [thread overview]
Message-ID: <5696609A.10902@linux.intel.com> (raw)
In-Reply-To: <20160112161928.GH19130@phenom.ffwll.local>
On 12/01/16 16:19, Daniel Vetter wrote:
> On Mon, Jan 11, 2016 at 09:51:38AM +0000, Tvrtko Ursulin wrote:
>>
>> On 11/01/16 08:43, Daniel Vetter wrote:
>>> On Fri, Jan 08, 2016 at 01:29:14PM +0000, Tvrtko Ursulin wrote:
>>>>
>>>> On 08/01/16 11:29, Tvrtko Ursulin wrote:
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>
>>>>> Purpose is to catch places which iterate the object VMA list
>>>>> without holding the big lock.
>>>>>
>>>>> Implemented by open coding list_for_each_entry to make the
>>>>> macro compatible with existing call sites.
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>> ---
>>>>> drivers/gpu/drm/i915/i915_debugfs.c | 8 ++++----
>>>>> drivers/gpu/drm/i915/i915_drv.h | 6 ++++++
>>>>> drivers/gpu/drm/i915/i915_gem.c | 24 ++++++++++++------------
>>>>> drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
>>>>> drivers/gpu/drm/i915/i915_gem_shrinker.c | 2 +-
>>>>> drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
>>>>> 6 files changed, 26 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> index 714a45cf8a51..d7c2a3201161 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> @@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)
>>>>> u64 size = 0;
>>>>> struct i915_vma *vma;
>>>>>
>>>>> - list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>>>> + i915_gem_obj_for_each_vma(vma, obj) {
>>>>> if (i915_is_ggtt(vma->vm) &&
>>>>> drm_mm_node_allocated(&vma->node))
>>>>> size += vma->node.size;
>>>>> @@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>>>>> obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
>>>>> if (obj->base.name)
>>>>> seq_printf(m, " (name: %d)", obj->base.name);
>>>>> - list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>>>> + i915_gem_obj_for_each_vma(vma, obj) {
>>>>> if (vma->pin_count > 0)
>>>>> pin_count++;
>>>>> }
>>>>> @@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>>>>> seq_printf(m, " (display)");
>>>>> if (obj->fence_reg != I915_FENCE_REG_NONE)
>>>>> seq_printf(m, " (fence: %d)", obj->fence_reg);
>>>>> - list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>>>> + i915_gem_obj_for_each_vma(vma, obj) {
>>>>> seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
>>>>> i915_is_ggtt(vma->vm) ? "g" : "pp",
>>>>> vma->node.start, vma->node.size);
>>>>> @@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data)
>>>>> stats->shared += obj->base.size;
>>>>>
>>>>> if (USES_FULL_PPGTT(obj->base.dev)) {
>>>>> - list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>>>> + i915_gem_obj_for_each_vma(vma, obj) {
>>>>> struct i915_hw_ppgtt *ppgtt;
>>>>>
>>>>> if (!drm_mm_node_allocated(&vma->node))
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>>> index b77a5d84eac2..0406a020dfcc 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>> @@ -2852,6 +2852,12 @@ struct drm_i915_gem_object *i915_gem_object_create_from_data(
>>>>> void i915_gem_free_object(struct drm_gem_object *obj);
>>>>> void i915_gem_vma_destroy(struct i915_vma *vma);
>>>>>
>>>>> +#define i915_gem_obj_for_each_vma(vma, obj) \
>>>>> + for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
>>>>> + vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
>>>>> + &vma->vma_link != (&(obj)->vma_list); \
>>>>> + vma = list_next_entry(vma, vma_link))
>>>>> +
>>>>
>>>>
>>>> Unfortunately error capture is not happy with this approach. Can't even see
>>>> that error capture attempts to grab the mutex anywhere.
>>>>
>>>> So what? Drop the idea or add a "doing error capture" flag somewhere?
>>>
>>> Fix the bugs. Not surprise at all that we've screwed this up all over the
>>> place ;-) Afaics modeset code isn't much better either ...
>>
>> Ok I'll drop this patch then since the series contains fixes to all but one
>> related issues. The remaining one is then:
>>
>> [ 17.370366] ------------[ cut here ]------------
>> [ 17.375633] WARNING: CPU: 0 PID: 1128 at drivers/gpu/drm/i915/i915_gem.c:5166 i915_gem_obj_ggtt_offset_view+0x10f/0x120 [i915]()
>> [ 17.388879] WARN_ON_ONCE(!mutex_is_locked(&(o)->base.dev->struct_mutex))
>> [ 17.396364] Modules linked in: hid_generic usbhid coretemp asix usbnet libphy mii i915 gpio_lynxpoint i2c_hid hid video i2c_algo_bit drm_kms_helper acpi_pad drm lpc_ich mfd_core nls_iso8859_1 e1000e ptp ahci libahci pps_core
>> [ 17.419484] CPU: 0 PID: 1128 Comm: Xorg Tainted: G U 4.4.0-rc8-160107+ #105
>> [ 17.428771] Hardware name: Intel Corporation Broadwell Client platform/WhiteTip Mountain 1, BIOS BDW-E1R1.86C.0080.R01.1406120446 06/12/2014
>> [ 17.443161] ffffffffa0227790 ffff8800a98439b8 ffffffff81280d82 ffff8800a9843a00
>> [ 17.451677] ffff8800a98439f0 ffffffff81049c8c ffff8801495d0000 ffff8800aa934900
>> [ 17.460166] ffff8801495d8668 ffffffffa0242520 ffff8800aacea000 ffff8800a9843a50
>> [ 17.468674] Call Trace:
>> [ 17.471470] [<ffffffff81280d82>] dump_stack+0x4b/0x79
>> [ 17.477355] [<ffffffff81049c8c>] warn_slowpath_common+0x7c/0xc0
>> [ 17.484255] [<ffffffff81049d17>] warn_slowpath_fmt+0x47/0x50
>> [ 17.490869] [<ffffffffa018f7ef>] i915_gem_obj_ggtt_offset_view+0x10f/0x120 [i915]
>> [ 17.499572] [<ffffffffa01a7290>] ? gen9_write8+0x2d0/0x2d0 [i915]
>> [ 17.506663] [<ffffffffa01bdbfe>] ironlake_update_primary_plane+0x1ee/0x3a0 [i915]
>> [ 17.515358] [<ffffffffa01ace1f>] intel_plane_atomic_update+0x5f/0x70 [i915]
>> [ 17.523391] [<ffffffffa00ef122>] drm_atomic_helper_commit_planes_on_crtc+0x142/0x230 [drm_kms_helper]
>> [ 17.534063] [<ffffffffa01c1744>] intel_atomic_commit+0x424/0x2270 [i915]
>> [ 17.541763] [<ffffffff814ee04a>] ? __ww_mutex_lock+0x4a/0x82
>> [ 17.548355] [<ffffffffa0097edb>] ? drm_atomic_check_only+0x12b/0x5e0 [drm]
>> [ 17.556352] [<ffffffffa0097a07>] ? drm_atomic_set_crtc_for_connector+0x77/0xf0 [drm]
>> [ 17.565346] [<ffffffffa00983c2>] drm_atomic_commit+0x32/0x50 [drm]
>> [ 17.572553] [<ffffffffa00ef782>] drm_atomic_helper_set_config+0x72/0xb0 [drm_kms_helper]
>> [ 17.581954] [<ffffffffa0088e8f>] drm_mode_set_config_internal+0x5f/0x100 [drm]
>> [ 17.590313] [<ffffffffa008cdc8>] drm_mode_setcrtc+0xd8/0x500 [drm]
>> [ 17.597491] [<ffffffffa007f5d8>] drm_ioctl+0x258/0x4f0 [drm]
>> [ 17.604065] [<ffffffff810c34ac>] ? unlock_page+0x4c/0x50
>> [ 17.610245] [<ffffffffa008ccf0>] ? drm_mode_setplane+0x1c0/0x1c0 [drm]
>> [ 17.617844] [<ffffffff8111597d>] do_vfs_ioctl+0x2cd/0x4a0
>> [ 17.624137] [<ffffffff81052572>] ? recalc_sigpending+0x12/0x40
>> [ 17.630943] [<ffffffff8111eb32>] ? __fget+0x72/0xb0
>> [ 17.636645] [<ffffffff81115b8c>] SyS_ioctl+0x3c/0x70
>> [ 17.642448] [<ffffffff81055301>] ? SyS_rt_sigprocmask+0x81/0xa0
>> [ 17.649334] [<ffffffff814f0017>] entry_SYSCALL_64_fastpath+0x12/0x6a
>> [ 17.656740] ---[ end trace 3f29190006c97826 ]---
>>
>> Would you like a BZ for this or maybe it is known already?
>
> Maarten is working on this, or at least we're tracking it already in
> VIZ-7000.
>
> Can you pls coordinate with Maarten to make sure the final "enable the
> warning" patch lands only once all the bits are in?
Oh I dropped that patch due the error capture problem - do you have a
suggestion how to handle that?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2016-01-13 14:35 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-08 11:29 [PATCH v2 00/13] Misc cleanups and locking fixes Tvrtko Ursulin
2016-01-08 11:29 ` [PATCH 01/13] drm/i915/bdw+: Replace list_del+list_add_tail with list_move_tail Tvrtko Ursulin
2016-01-11 8:22 ` Daniel Vetter
2016-01-08 11:29 ` [PATCH 02/13] drm/i915: Don't need a timer to wake us up Tvrtko Ursulin
2016-01-11 8:26 ` Daniel Vetter
2016-01-08 11:29 ` [PATCH 03/13] drm/i915: Avoid invariant conditionals in lrc interrupt handler Tvrtko Ursulin
2016-01-11 8:29 ` Daniel Vetter
2016-01-11 9:43 ` Tvrtko Ursulin
2016-01-08 11:29 ` [PATCH 04/13] drm/i915: Fail engine initialization if LRCA is incorrectly aligned Tvrtko Ursulin
2016-01-11 8:31 ` Daniel Vetter
2016-01-11 16:02 ` Dave Gordon
2016-01-11 23:31 ` Chris Wilson
2016-01-08 11:29 ` [PATCH 05/13] drm/i915: Cache LRCA in the context Tvrtko Ursulin
2016-01-11 8:38 ` Daniel Vetter
2016-01-11 19:41 ` Dave Gordon
2016-01-12 9:59 ` Tvrtko Ursulin
2016-01-08 11:29 ` [PATCH 06/13] drm/i915: Only grab timestamps when needed Tvrtko Ursulin
2016-01-11 8:42 ` Daniel Vetter
2016-01-11 9:45 ` Tvrtko Ursulin
2016-01-08 11:29 ` [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator Tvrtko Ursulin
2016-01-08 11:44 ` Chris Wilson
2016-01-11 8:48 ` Daniel Vetter
2016-01-08 13:29 ` Tvrtko Ursulin
2016-01-11 8:43 ` Daniel Vetter
2016-01-11 9:51 ` Tvrtko Ursulin
2016-01-12 16:19 ` Daniel Vetter
2016-01-12 16:43 ` Maarten Lankhorst
2016-01-13 14:35 ` Tvrtko Ursulin [this message]
2016-01-08 11:29 ` [PATCH 08/13] drm/i915: GEM operations need to be done under the big lock Tvrtko Ursulin
2016-01-08 11:40 ` Chris Wilson
2016-01-08 11:45 ` Chris Wilson
2016-01-08 11:47 ` Tvrtko Ursulin
2016-01-08 12:09 ` Chris Wilson
2016-01-08 12:40 ` Tvrtko Ursulin
2016-01-08 13:04 ` [PATCH v3 " Tvrtko Ursulin
2016-01-08 11:29 ` [PATCH 09/13] drm/i915: Remove two impossible asserts Tvrtko Ursulin
2016-01-11 8:49 ` Daniel Vetter
2016-01-08 11:29 ` [PATCH 10/13] drm/i915: Introduce dedicated safe object VMA iterator Tvrtko Ursulin
2016-01-08 11:29 ` [PATCH 11/13] drm/i915: Cache ringbuffer GTT address Tvrtko Ursulin
2016-01-08 11:37 ` Chris Wilson
2016-01-11 8:49 ` Daniel Vetter
2016-01-12 11:42 ` [PATCH v3 3/7] drm/i915: Cache ringbuffer GTT VMA Tvrtko Ursulin
2016-01-12 11:54 ` Chris Wilson
2016-01-11 16:16 ` [PATCH 11/13] drm/i915: Cache ringbuffer GTT address Dave Gordon
2016-01-11 17:44 ` Tvrtko Ursulin
2016-01-08 11:29 ` [PATCH 12/13] drm/i915: Add BKL asserts to get page helpers Tvrtko Ursulin
2016-01-08 11:37 ` Chris Wilson
2016-01-08 11:29 ` [PATCH 13/13] drm/i915: Cache LRC state page in the context Tvrtko Ursulin
2016-01-08 11:38 ` [PATCH v2 00/13] Misc cleanups and locking fixes Chris Wilson
2016-01-11 9:44 ` ✗ failure: Fi.CI.BAT Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5696609A.10902@linux.intel.com \
--to=tvrtko.ursulin@linux.intel.com \
--cc=Intel-gfx@lists.freedesktop.org \
--cc=daniel.vetter@ffwll.ch \
--cc=daniel@ffwll.ch \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).