We can reduce the critical section in vkms_vblank_simulate under
output->lock quite a lot:
- hrtimer_forward_now just needs to be ordered correctly wrt
drm_crtc_handle_vblank. We already access the hrtimer timestamp
without locks. While auditing that I noticed that we don't correctly
annotate the read there, so sprinkle a READ_ONCE to make sure the
compiler doesn't do anything foolish.
- drm_crtc_handle_vblank must stay under the lock to avoid races with
drm_crtc_arm_vblank_event.
- The access to vkms_ouptut->crc_state also must stay under the lock.
- next problem is making sure the output->state structure doesn't get
freed too early. First we rely on a given hrtimer being serialized:
If we call drm_crtc_handle_vblank, then we are guaranteed that the
previous call to vkms_vblank_simulate has completed. The other side
of the coin is that the atomic updates waits for the vblank to
happen before it releases the old state. Both taken together means
that by the time the atomic update releases the old state, the
hrtimer won't access it anymore (it might be accessing the new state
at the same time, but that's ok).
- state is invariant, except the few fields separate protected by
state->crc_lock. So no need to hold the lock for that.
- finally the queue_work. We need to make sure there's no races with
the flush_work, i.e. when we call flush_work we need to guarantee
that the hrtimer can't requeue the work again. This is guaranteed by
the same vblank/hrtimer ordering guarantees like the reasoning above
why state won't be freed too early: flush_work on the old state is
called after wait_for_flip_done in the atomic commit code.
Therefore we can also move everything after the output->crc_state out
of the critical section.
Motivated by suggestions from Rodrigo.
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190719152314.7706-3-daniel.vetter@ffwll.ch
Noticed while reviewing code. I'm not sure whether this might or might
not explain some of the missed vblank hilarity we've been seeing on
various drivers (but those got tracked down to driver issues, at least
mostly). I think those all go through the vblank completion event,
which has unconditional barriers - it always takes the spinlock.
Therefore no cc stable.
v2:
- Barrriers are hard, put them in in the right order (Chris).
- Improve the comments a bit.
v3:
Ville noticed that on 32bit we might be breaking up the load/stores,
now that the vblank counter has been switched over to be 64 bit. Fix
that up by switching to atomic64_t. This this happens so rarely in
practice I figured no need to cc: stable ...
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Keith Packard <keithp@keithp.com>
References: 570e86963a ("drm: Widen vblank count to 64-bits [v3]")
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190723131337.22031-1-daniel.vetter@ffwll.ch
Store the timestamp of the current vblank in the new field 'time' of the
vblank trace event. If the timestamp is calculated by a driver that
supports high-precision vblank timing, set the field 'high-prec' to
'true'.
User space can now access actual hardware vblank times via the tracing
infrastructure. Tracing applications (such as GPUVis, see [0] for
related discussion), can use the newly added information to conduct a
more accurate analysis of display timing.
v2 Fix author name (missing last name)
[0] https://github.com/mikesart/gpuvis/issues/30
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Heinrich Fink <heinrich.fink@daqri.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20190902142412.27846-2-heinrich.fink@daqri.com
Split virtqueue_kick() call into virtqueue_kick_prepare(), which
requires serialization, and virtqueue_notify(), which does not. Move
the virtqueue_notify() call out of the critical section protected by the
queue lock. This avoids triggering a vmexit while holding the lock and
thereby fixes a rather bad spinlock contention.
Suggested-by: Chia-I Wu <olvaffe@gmail.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Chia-I Wu <olvaffe@gmail.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20190813082509.29324-3-kraxel@redhat.com
This code will likely crash if we try to do a zero byte write. The code
looks like this:
/* strip trailing whitespace */
for (i = count - 1; i > 0; i--)
if (isspace(buf[i]))
...
We're writing zero bytes so count = 0. You would think that "count - 1"
would be negative one, but because "i" is unsigned it is a large
positive numer instead. The "i > 0" condition is true and the "buf[i]"
access will be out of bounds.
The fix is to make "i" signed and now everything works as expected. The
upper bound of "count" is capped in __kernel_write() at MAX_RW_COUNT so
we don't have to worry about it being higher than INT_MAX.
Fixes: 02dd95fe31 ("drm/tinydrm: Add MIPI DBI support")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
[noralf: Adjust title]
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Link: https://patchwork.freedesktop.org/patch/msgid/20190821072456.GJ26957@mwanda
Instead of requiring all drivers to set the dev and funcs fields of
drm_panel manually after calling drm_panel_init(), pass the data as
arguments to the function. This simplifies the panel drivers, and will
help future refactoring when adding new arguments to drm_panel_init().
The panel drivers have been updated with the following Coccinelle
semantic patch, with manual inspection to verify that no call to
drm_panel_init() with a single argument still exists.
@@
expression panel;
expression device;
identifier ops;
@@
drm_panel_init(&panel
+ , device, &ops
);
...
(
-panel.dev = device;
-panel.funcs = &ops;
|
-panel.funcs = &ops;
-panel.dev = device;
)
Suggested-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Link: https://patchwork.freedesktop.org/patch/msgid/20190823193245.23876-3-laurent.pinchart@ideasonboard.com
While newer kbase include only the numbers of errata, older kbase
releases included one-line descriptions for each errata, which is useful
for those working on the driver. Import these descriptions. Most are
from kbase verbatim; a few I edited for clarity.
v2: Wrote a description for the workaround of an issue whose cause is
still unknown (Stephen). Errata which pertain to newer models
unsupported by the mainline driver, for which Arm has not yet released
errata information, have been removed from the issue list as the kernel
need not concern itself with these.
v3: Readded errata not yet handled, adding descriptions based on the
workarounds in the latest kbase release.
Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Signed-off-by: Rob Herring <robh@kernel.org>
Link: https://patchwork.freedesktop.org/patch/msgid/20190823155149.7272-1-alyssa.rosenzweig@collabora.com
Lockdep reports a circular locking dependency with pages_lock taken in
the shrinker callback. The deadlock can't actually happen with current
users at least as a BO will never be purgeable when pages_lock is held.
To be safe, let's use mutex_trylock() instead and bail if a BO is locked
already.
WARNING: possible circular locking dependency detected
5.3.0-rc1+ #100 Tainted: G L
------------------------------------------------------
kswapd0/171 is trying to acquire lock:
000000009b9823fd (&shmem->pages_lock){+.+.}, at: drm_gem_shmem_purge+0x20/0x40
but task is already holding lock:
00000000f82369b6 (fs_reclaim){+.+.}, at: __fs_reclaim_acquire+0x0/0x40
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (fs_reclaim){+.+.}:
fs_reclaim_acquire.part.18+0x34/0x40
fs_reclaim_acquire+0x20/0x28
__kmalloc_node+0x6c/0x4c0
kvmalloc_node+0x38/0xa8
drm_gem_get_pages+0x80/0x1d0
drm_gem_shmem_get_pages+0x58/0xa0
drm_gem_shmem_get_pages_sgt+0x48/0xd0
panfrost_mmu_map+0x38/0xf8 [panfrost]
panfrost_gem_open+0xc0/0xe8 [panfrost]
drm_gem_handle_create_tail+0xe8/0x198
drm_gem_handle_create+0x3c/0x50
panfrost_gem_create_with_handle+0x70/0xa0 [panfrost]
panfrost_ioctl_create_bo+0x48/0x80 [panfrost]
drm_ioctl_kernel+0xb8/0x110
drm_ioctl+0x244/0x3f0
do_vfs_ioctl+0xbc/0x910
ksys_ioctl+0x78/0xa8
__arm64_sys_ioctl+0x1c/0x28
el0_svc_common.constprop.0+0x90/0x168
el0_svc_handler+0x28/0x78
el0_svc+0x8/0xc
-> #0 (&shmem->pages_lock){+.+.}:
__lock_acquire+0xa2c/0x1d70
lock_acquire+0xdc/0x228
__mutex_lock+0x8c/0x800
mutex_lock_nested+0x1c/0x28
drm_gem_shmem_purge+0x20/0x40
panfrost_gem_shrinker_scan+0xc0/0x180 [panfrost]
do_shrink_slab+0x208/0x500
shrink_slab+0x10c/0x2c0
shrink_node+0x28c/0x4d8
balance_pgdat+0x2c8/0x570
kswapd+0x22c/0x638
kthread+0x128/0x130
ret_from_fork+0x10/0x18
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(fs_reclaim);
lock(&shmem->pages_lock);
lock(fs_reclaim);
lock(&shmem->pages_lock);
*** DEADLOCK ***
3 locks held by kswapd0/171:
#0: 00000000f82369b6 (fs_reclaim){+.+.}, at: __fs_reclaim_acquire+0x0/0x40
#1: 00000000ceb37808 (shrinker_rwsem){++++}, at: shrink_slab+0xbc/0x2c0
#2: 00000000f31efa81 (&pfdev->shrinker_lock){+.+.}, at: panfrost_gem_shrinker_scan+0x34/0x180 [panfrost]
Fixes: 17acb9f35e ("drm/shmem: Add madvise state and purge helpers")
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Sean Paul <sean@poorly.run>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Rob Herring <robh@kernel.org>
Reviewed-by: Steven Price <steven.price@arm.com>
Acked-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190823021216.5862-6-robh@kernel.org
Since commit b0e999c955 ("fbdev: list all pci memory bars as
conflicting apertures") the parameter was used for some sanity checks
only, to make sure we detect any issues with the new approach to just
list all memory bars as apertures.
No issues turned up so far, so continue to cleanup: Drop the res_id
parameter, drop the sanity checks. Also downgrade the logging from
"info" level to "debug" level and update documentation.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: http://patchwork.freedesktop.org/patch/msgid/20190822090645.25410-2-kraxel@redhat.com
