The stack_depot member was added without kernel-doc, leading to below
warning. Fix it.
./include/drm/drm_modeset_lock.h:74: warning: Function parameter or
member 'stack_depot' not described in 'drm_modeset_acquire_ctx'
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Fixes: cd06ab2fd4 ("drm/locking: add backtrace for locking contended locks without backoff")
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Tested-by: Stephen Rothwell <sfr@canb.auug.org.au>
Link: https://patchwork.freedesktop.org/patch/msgid/20220120094856.3004147-1-jani.nikula@intel.com
If drm_modeset_lock() returns -EDEADLK, the caller is supposed to drop
all currently held locks using drm_modeset_backoff(). Failing to do so
will result in warnings and backtraces on the paths trying to lock a
contended lock. Add support for optionally printing the backtrace on the
path that hit the deadlock and didn't gracefully handle the situation.
For example, the patch [1] inadvertently dropped the return value check
and error return on replacing calc_watermark_data() with
intel_compute_global_watermarks(). The backtraces on the subsequent
locking paths hitting WARN_ON(ctx->contended) were unhelpful, but adding
the backtrace to the deadlock path produced this helpful printout:
<7> [98.002465] drm_modeset_lock attempting to lock a contended lock without backoff:
drm_modeset_lock+0x107/0x130
drm_atomic_get_plane_state+0x76/0x150
skl_compute_wm+0x251d/0x2b20 [i915]
intel_atomic_check+0x1942/0x29e0 [i915]
drm_atomic_check_only+0x554/0x910
drm_atomic_nonblocking_commit+0xe/0x50
drm_mode_atomic_ioctl+0x8c2/0xab0
drm_ioctl_kernel+0xac/0x140
Add new CONFIG_DRM_DEBUG_MODESET_LOCK to enable modeset lock debugging
with stack depot and trace.
[1] https://lore.kernel.org/r/20210924114741.15940-4-jani.nikula@intel.com
v2:
- default y if DEBUG_WW_MUTEX_SLOWPATH (Daniel)
- depends on DEBUG_KERNEL
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Dave Airlie <airlied@gmail.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20211001091444.8177-1-jani.nikula@intel.com
This reverts commit 8d813d1a53.
This patchset breaks on intel platforms and was previously NACK'd by
Ville.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Fernando Ramos <greenfoo@u92.eu>
Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Link: https://patchwork.freedesktop.org/patch/msgid/20211002154542.15800-1-sean@poorly.run
This fell off in the conversion in
commit 9bcaa3fe58
Author: Michal Orzel <michalorzel.eng@gmail.com>
Date: Tue Apr 28 19:10:04 2020 +0200
drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
but it's caught by the drm_warn_on_modeset_not_all_locked() that the
legacy modeset code uses. Since this is the bkl and it's unclear
what's all protected, play it safe and grab it again for legacy
drivers.
Unfortunately this means we need to sprinkle a few more #includes
around.
Also we need to add the drm_device as a parameter to the _END macro.
Finally remove the mute_lock() from setcrtc, since that's now done by
the macro.
Cc: Alex Deucher <alexdeucher@gmail.com>
References: https://gitlab.freedesktop.org/drm/amd/-/issues/1224
Fixes: 9bcaa3fe58 ("drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers")
Cc: Michal Orzel <michalorzel.eng@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v5.8+
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20200814093842.3048472-1-daniel.vetter@ffwll.ch
This patch adds a couple of helpers to remove the boilerplate involved
in grabbing all of the modeset locks.
I've also converted the obvious cases in drm core to use the helpers.
The only remaining instance of drm_modeset_lock_all_ctx() is in
drm_framebuffer. It's complicated by the state clear that occurs on
deadlock. ATM, there's no way to inject code in the deadlock path with
the helpers, so it's unfit for conversion.
Changes in v2:
- Relocate ret argument to the end of the list (Daniel)
- Incorporate Daniel's doc suggestions (Daniel)
Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Link: https://patchwork.freedesktop.org/patch/msgid/20181129150423.239081-4-sean@poorly.run
When we want to make drm_atomic_commit interruptible, there are a lot of
places that call the lock function, which we don't have control over.
Rather than trying to convert every single one, it's easier to toggle
interruptible waiting per acquire_ctx. If drm_modeset_acquire_init is
called with DRM_MODESET_ACQUIRE_INTERRUPTIBLE, then we will perform
interruptible waits in drm_modeset_lock and drm_modeset_backoff.
Changes since v1:
- Fix locking example in drm_modeset_lock.c to be compatible
with interruptible waiting (xexaxo) and make it default.
Uninterruptible waiting shouldn't happen except in corner cases,
but the example will still apply if the flag is removed.
- Add drm_modeset_lock_single_interruptible() to documentation.
- Fix dead link to removed drm_modeset_lock_interruptible() in
drm_modeset_lock().
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> #v1
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20170912133749.6532-2-maarten.lankhorst@linux.intel.com
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
With all the callers of drm_modeset_lock_crtc gone, and all the places
it was formerly used properly wiring the acquire ctx through, we can
remove this.
The only hidden context magic we still have is now the global one.
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20170403083304.9083-4-daniel.vetter@ffwll.ch
The last user, the cursor ioctl, can just open-code this too. We
simply have to move the acquire ctx dance from the universal function
up into the top-level ioctl handler.
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20170403083304.9083-3-daniel.vetter@ffwll.ch
This is only for legacy paths that need to grab the crtc/plane lock
combo. If you want to lock a crtc, just use drm_modeset_lock().
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20170403083304.9083-2-daniel.vetter@ffwll.ch
I just learned that &struct_name.member_name works and looks pretty
even. It doesn't (yet) link to the member directly though, which would
be really good for big structures or vfunc tables (where the
per-member kerneldoc tends to be long).
Also some minor drive-by polish where it makes sense, I read a lot
of docs ...
v2: Review from Eric.
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20170125062657.19270-4-daniel.vetter@ffwll.ch
kbuild spotted this error, with drm/msm patches that add a new
modeset-lock in the driver and driver built as a module:
ERROR: "crtc_ww_class" [drivers/gpu/drm/msm/msm.ko] undefined!
Really the only reason for crtc_ww_class not being internal to
drm_modeset_lock.c is that drm_modeset_lock_init() was static-inline
(for no particularly good reason).
Fix that, and move crtc_ww_class into drm_modeset_lock.c.
Signed-off-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: http://patchwork.freedesktop.org/patch/msgid/1479163257-18703-1-git-send-email-robdclark@gmail.com
This function is like drm_modeset_lock_all(), but it takes the lock
acquisition context as a parameter rather than storing it in the DRM
device's mode_config structure.
Implement drm_modeset_{,un}lock_all() in terms of the new function for
better code reuse, and add a note to the kerneldoc that new code should
use the new functions.
v2: improve kerneldoc
v4: rename drm_modeset_lock_all_crtcs() to drm_modeset_lock_all_ctx()
and take mode_config's .connection_mutex instead of .mutex lock to
avoid lock inversion (Daniel Vetter), use drm_modeset_drop_locks()
which is now the equivalent of drm_modeset_unlock_all_ctx()
v5: do not take the dev->mode_config.connection_mutex in
drm_atomic_legacy_backoff() since drm_modeset_lock_all_ctx()
already keeps it, enhance kerneldoc for drm_modeset_lock_all_ctx()
(Daniel Vetter)
Signed-off-by: Thierry Reding <treding@nvidia.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1449075005-13937-1-git-send-email-thierry.reding@gmail.com
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
"/**" should be used for kernel-doc documentation only.
It causes a warning with the new "in struct body" format.
Signed-off-by: Danilo Cesar Lemes de Paula <danilo.cesar@collabora.co.uk>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Stephan Mueller <smueller@chronox.de>
Cc: Michal Marek <mmarek@suse.cz>
Cc: linux-kernel@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: intel-gfx <intel-gfx@lists.freedesktop.org>
Cc: dri-devel <dri-devel@lists.freedesktop.org>
Cc: Graham Whaley <graham.whaley@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
The last user is gone, no need for trylocking any more in this legacy
helper.
Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Turned out to be much simpler on top of my latest atomic stuff than
what I've feared. Some details:
- Drop the modeset_lock_all snakeoil in drm_plane_init. Same
justification as for the equivalent change in drm_crtc_init done in
commit d0fa1af40e
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date: Mon Sep 8 09:02:49 2014 +0200
drm: Drop modeset locking from crtc init function
Without these the drm_modeset_lock_init would fall over the exact
same way.
- Since the atomic core code wraps the locking switching it to
per-plane locks was a one-line change.
- For the legacy ioctls add a plane argument to the locking helper so
that we can grab the right plane lock (cursor or primary). Since the
universal cursor plane might not be there, or someone really crazy
might forgoe the primary plane even accept NULL.
- Add some locking WARN_ON to the atomic helpers for good paranoid
measure and to check that it all works out.
Tested on my exynos atomic hackfest with full lockdep checks and ww
backoff injection.
v2: I've forgotten about the load-detect code in i915.
v3: Thierry reported that in latest 3.18-rc vmwgfx doesn't compile any
more due to
commit 21e88620aa
Author: Rob Clark <robdclark@gmail.com>
Date: Thu Oct 30 13:39:04 2014 -0400
drm/vmwgfx: fix lock breakage
Rebased and fix this up.
Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Reviewed-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Dave Airlie <airlied@redhat.com>
I've forgotten to do this in:
commit cb597bb3a2
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date: Sun Jul 27 19:09:33 2014 +0200
drm: trylock modest locking for fbdev panics
Oops, fix this asap.
In my defense kerneldoc is really awful and there's no way it can pick
up structured comments per struct member. Which means we need both
since people won't scroll up even a few lines.
Reviewed-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Fix drm kernel-doc notation to squelch these warnings:
Warning(..//include/drm/drm_modeset_lock.h:41): cannot understand function prototype: 'struct drm_modeset_acquire_ctx '
Warning(..//include/drm/drm_modeset_lock.h:66): cannot understand function prototype: 'struct drm_modeset_lock '
Need to include the keyword 'struct' for structure descriptions.
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
In the fbdev code we want to do trylocks only to avoid deadlocks and
other ugly issues. Thus far we've only grabbed the overall modeset
lock, but that already failed to exclude a pile of potential
concurrent operations. With proper atomic support this will be worse.
So add a trylock mode to the modeset locking code which attempts all
locks only with trylocks, if possible. We need to track this in the
locking functions themselves and can't restrict this to drivers since
driver-private w/w mutexes must be treated the same way.
There's still the issue that other driver private locks aren't handled
here at all, but well can't have everything. With this we will at
least not regress, even once atomic allows lots of concurrent kms
activity.
Aside: We should move the acquire context to stack-based allocation in
the callers to get rid of that awful WARN_ON(kmalloc_failed) control
flow which just blows up when memory is short. But that's material for
separate patches.
v2:
- Fix logic inversion fumble in the fb helper.
- Add proper kerneldoc.
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
So drivers using the atomic interfaces expect that they can acquire
additional locks internal to the driver as-needed. Examples would be
locks to protect shared state like shared display PLLs.
Unfortunately the legacy ioctls assume that all locking is fully done
by the drm core. Now for those paths which grab all locks we already
have to keep around an acquire context in dev->mode_config. Helper
functions that implement legacy interfaces in terms of atomic support
can therefore grab this acquire contexts and reuse it.
The only interfaces left are the cursor and pageflip ioctls. So add
functions to grab the crtc lock these need using an acquire context
and preserve it for atomic drivers to reuse.
v2:
- Fixup comments&kerneldoc.
- Drop the WARNING from modeset_lock_all_crtcs since that can be used
in legacy paths with crtc locking.
v3: Fix a type on the kerneldoc Dave spotted.
Cc: Dave Airlie <airlied@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Somehow we've forgotten about this little bit of OCD.
Reviewed-by: Dave Airlie <airlied@redhat.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
For atomic, it will be quite necessary to not need to care so much
about locking order. And 'state' gives us a convenient place to stash a
ww_ctx for any sort of update that needs to grab multiple crtc locks.
Because we will want to eventually make locking even more fine grained
(giving locks to planes, connectors, etc), split out drm_modeset_lock
and drm_modeset_acquire_ctx to track acquired locks.
Atomic will use this to keep track of which locks have been acquired
in a transaction.
v1: original
v2: remove a few things not needed until atomic, for now
v3: update for v3 of connection_mutex patch..
v4: squash in docbook
v5: doc tweaks/fixes
Signed-off-by: Rob Clark <robdclark@gmail.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
Jani Nikula
Sean Paul
Fernando Ramos
Daniel Vetter
Ville Syrjälä
Matt Roper
Maarten Lankhorst
Daniel Vetter
Rob Clark
Thierry Reding
Danilo Cesar Lemes de Paula
Randy Dunlap