Previous versions of the line6 driver snooped MIDI traffic in order to
make device state accessible via sysfs attributes. This involved a lot
of logic in line6_pod_process_message() that has since been removed.
Drop unused conditionals in line6_pod_process_message() and reduce the
levels of indentation. Only two MIDI messages are still tracked: the
POD version message on startup and monitor level changes originating
from the device.
Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The CONFIG_LINE6_USB_DUMP_PCM config option prints a hexdump of PCM
audio data as URBs are sent and received. The usbmon feature should be
used instead of manually dumping PCM URBs. There are a few advantages
to using usbmon:
* Can be turned on/off at runtime
* Provides full USB-level traffic
* tcpdump and wireshark support for powerful analysis
* No driver-specific code is required
This is the last user of line6_write_hexdump() so we drop it too.
Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Enclose the macro into braces so that it can be closed by a semicolon
Signed-off-by: Sebastian Wankerl <sisewank@cip.cs.fau.de>
Signed-off-by: Sebastian Ehrenfels <qi50dube@cip.cs.fau.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The root of problem is carelessly zeroing pointer(in function __tty_buffer_flush()),
when another thread can use it. It can be cause of "NULL pointer dereference".
Main idea of the patch, this is never free last (struct tty_buffer) in the active buffer.
Only flush the data for ldisc(buf->head->read = buf->head->commit).
At that moment driver can collect(write) data in buffer without conflict.
It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc.
Also revert:
commit c56a00a165
tty: hold lock across tty buffer finding and buffer filling
In order to delete the unneeded locks any more.
Signed-off-by: Ilya Zykov <ilya@ilyx.ru>
CC: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The fields must be null-terminated, or next printk for %s, will cause issue.
Signed-off-by: Chen Gang <gang.chen@asianux.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Improve driver logging to be more helpful
Signed-off-by: Jon Mason <jon.mason@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
There is a race between disabling and enabling the tx queue, resulting
in tx timeouts. Since all the tx timeout does is re-enable the tx
queue, simple remove the start/stop of the queue and the tx timeout
routine.
Signed-off-by: Jon Mason <jon.mason@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
If ntb_netdev is unable to pass a new skb to the ntb transport for
future rx packets, it should free the newly alloc'ed skb in the error
case. Found by Kernel memory leak detector.
Signed-off-by: Jon Mason <jon.mason@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Remove init/exit from probe/remove routines to correct warnings of
"Section mismatch".
Signed-off-by: Jon Mason <jon.mason@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
If the NTB client driver enqueues the maximum number of rx buffers, it
will not be able to re-enqueue another in its callback handler due to a
lack of free entries. This can be avoided by adding the current entry
to the free queue prior to calling the client callback handler. With
this change, ntb_netdev will no longer encounter a rx error on its first
packet.
Signed-off-by: Jon Mason <jon.mason@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CPU reads across NTB are slow(er) and can hang the local system if an
ECC error is encountered on the remote. To work around the need for a
read, have the remote side write its current position in the rx buffer
to the local system's buffer and use that to see if there is room when
transmitting.
Signed-off-by: Jon Mason <jon.mason@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Correct gcc warning of using too much stack debugfs_read. This is done
by kmallocing the buffer instead of using the char array on stack.
Also, shrinking the buffer to something closer to what is currently
being used.
Signed-off-by: Jon Mason <jon.mason@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Declare ntb_bus_type static to remove it from name space, and remove
unused ntb_get_max_spads function. Found via `make namespacecheck`.
Signed-off-by: Jon Mason <jon.mason@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Use simple_open for debugfs instead of recreating it in the NTB driver.
Caught by coccicheck.
Signed-off-by: Jon Mason <jon.mason@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Move all cancel_delayed_work_sync to a work thread to prevent sleeping
in interrupt context (when the NTB link goes down). Caught via
'Sleep inside atomic section checking'
Signed-off-by: Jon Mason <jon.mason@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Since it is possible for the memory windows on the two NTB connected
systems to be different sizes, the divergent sizes must be accounted for
in the segmentation of the MW's on each side. Create separate size
variables and initialization as necessary.
Signed-off-by: Jon Mason <jon.mason@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
mmiowb is not sufficient to flush the data and is causing data
corruption. Change to wmb and the data corruption is no more.
Signed-off-by: Jon Mason <jon.mason@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Attempts to probe client ntb drivers without ntb hardware present will
result in null pointer dereference due to the lack of the ntb bus device
being registers. Check to see if this is the case, and fail all calls
by the clients registering their drivers.
Signed-off-by: Jon Mason <jon.mason@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Coverity complains about a use after free for 'res1' and 'res2' since
we use the value of the pointers in a 'dev_dbg()' after they have been
freed. That's not really a problem, but it still seems cleaner to
defer freeing until we are completely done with the pointers.
Signed-off-by: Jesper Juhl <jj@chaosbits.net>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
The of_find_matching_node_and_match() function incorrectly sets the matched
entry to 'matches' when the compatible value of a node matches one of the
possible values. This results in incorrectly selecting the the first entry in
the 'matches' list as the matched entry. Fix this by noting down the result of
the call to of_match_node() and setting that as the matched entry.
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
Signed-off-by: Rob Herring <rob.herring@calxeda.com>
commit 563d34d057 (tcp: dont drop MTU reduction indications)
added an error leading to incorrect accounting of
LINUX_MIB_LOCKDROPPEDICMPS
If socket is owned by the user, we want to increment
this SNMP counter, unless the message is a
(ICMP_DEST_UNREACH,ICMP_FRAG_NEEDED) one.
Reported-by: Maciej Żenczykowski <maze@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
Acked-by: Neal Cardwell <ncardwell@google.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Pull misc syscall fixes from Al Viro:
- compat syscall fixes (discussed back in December)
- a couple of "make life easier for sigaltstack stuff by reducing
inter-tree dependencies"
- fix up compiler/asmlinkage calling convention disagreement of
sys_clone()
- misc
* 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/signal:
sys_clone() needs asmlinkage_protect
make sure that /linuxrc has std{in,out,err}
x32: fix sigtimedwait
x32: fix waitid()
switch compat_sys_wait4() and compat_sys_waitid() to COMPAT_SYSCALL_DEFINE
switch compat_sys_sigaltstack() to COMPAT_SYSCALL_DEFINE
CONFIG_GENERIC_SIGALTSTACK build breakage with asm-generic/syscalls.h
Ensure that kernel_init_freeable() is not inlined into non __init code
The aim of this locking rework is that ioctls which a compositor should be
might call for every frame (set_cursor, page_flip, addfb, rmfb and
getfb/create_handle) should not be able to block on kms background
activities like output detection. And since each EDID read takes about
25ms (in the best case), that always means we'll drop at least one frame.
The solution is to add per-crtc locking for these ioctls, and restrict
background activities to only use the global lock. Change-the-world type
of events (modeset, dpms, ...) need to grab all locks.
Two tricky parts arose in the conversion:
- A lot of current code assumes that a kms fb object can't disappear while
holding the global lock, since the current code serializes fb
destruction with it. Hence proper lifetime management using the already
created refcounting for fbs need to be instantiated for all ioctls and
interfaces/users.
- The rmfb ioctl removes the to-be-deleted fb from all active users. But
unconditionally taking the global kms lock to do so introduces an
unacceptable potential stall point. And obviously changing the userspace
abi isn't on the table, either. Hence this conversion opportunistically
checks whether the rmfb ioctl holds the very last reference, which
guarantees that the fb isn't in active use on any crtc or plane (thanks
to the conversion to the new lifetime rules using proper refcounting).
Only if this is not the case will the code go through the slowpath and
grab all modeset locks. Sane compositors will never hit this path and so
avoid the stall, but userspace relying on these semantics will also not
break.
All these cases are exercised by the newly added subtests for the i-g-t
kms_flip, tested on a machine where a full detect cycle takes around 100
ms. It works, and no frames are dropped any more with these patches
applied. kms_flip also contains a special case to exercise the
above-describe rmfb slowpath.
* 'drm-kms-locking' of git://people.freedesktop.org/~danvet/drm-intel: (335 commits)
drm/fb_helper: check whether fbcon is bound
drm/doc: updates for new framebuffer lifetime rules
drm: don't hold crtc mutexes for connector ->detect callbacks
drm: only grab the crtc lock for pageflips
drm: optimize drm_framebuffer_remove
drm/vmwgfx: add proper framebuffer refcounting
drm/i915: dump refcount into framebuffer debugfs file
drm: refcounting for crtc framebuffers
drm: refcounting for sprite framebuffers
drm: fb refcounting for dirtyfb_ioctl
drm: don't take modeset locks in getfb ioctl
drm: push modeset_lock_all into ->fb_create driver callbacks
drm: nest modeset locks within fpriv->fbs_lock
drm: reference framebuffers which are on the idr
drm: revamp framebuffer cleanup interfaces
drm: create drm_framebuffer_lookup
drm: revamp locking around fb creation/destruction
drm: only take the crtc lock for ->cursor_move
drm: only take the crtc lock for ->cursor_set
drm: add per-crtc locks
...
We need to make sure that the fbcon is still bound when touching the
hw, since otherwise we might corrupt the modeset state of kms clients.
X mostly works around that with VT switching and setting the VT into
raw mode, which disables most fbcon events.
Raw kms test programs though don't do that dance, and in the future
we might want to aim to abolish CONFIG_VT anyway. So improve preventive
measures a bit. To do so, extract the existing logic for handling hotplug
events (which X can't block with the current set of tricks) and reuse
it for the fbdev blanking helper.
Long-term we really need to either scrap this all and only have a OOPS
console, or come up with a saner model for device ownership sharing
between fbdev/fbcon and kms userspace.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Now that framebuffer are reference-counted for all use-sites, update
the documentation accordingly to stress the new rules for
initialization and teardown.
Also add a short paragraph about the implications for drivers of the
new locking rules.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
The coup de grace of the entire journey. No more dropped frames every
10s on my testbox!
I've tried to audit all ->detect and ->get_modes callbacks, but things
became a bit fuzzy after trying to piece together the umpteenth
implemenation. Afaict most drivers just have bog-standard output
register frobbing with a notch of i2c edid reading, nothing which
could potentially race with the newly concurrent pageflip/set_cursor
code. The big exception is load-detection code which requires a
running pipe, but radeon/nouveau seem to to this without touching any
state which can be observed from page_flip (e.g. disabled crtcs
temporarily getting enabled and so a pageflip succeeding).
The only special case I could find is the i915 load detect code. That
uses the normal modeset interface to enable the load-detect crtc, and
so userspace could try to squeeze in a pageflip on the load-detect
pipe. So we need to grab the relevant crtc mutex in there, to avoid
the temporary crtc enabling to sneak out and be visible to userspace.
Note that the sysfs files already stopped grabbing the per-crtc locks,
since I didn't want to bother with doing a interruptible
modeset_lock_all. But since there's very little in-between breakage
(essentially just the ability for userspace to pageflip on load-detect
crtcs when it shouldn't on the i915 driver) I figured I don't need to
bother.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
The pagelip ioctl itself is rather simply, so the hard work for this
patch is auditing all the drivers:
- exynos: Pageflip is protect with dev->struct_mutex and ...
synchronous. But nothing fancy going on, besides a check whether the
crtc is enabled, which should probably be somewhere in the drm core
so that we have unified behaviour across all drivers.
- i915: hw-state is protected with dev->struct_mutex, the delayed
unpin work together with the other stuff the pageflip complete irq
handler needs is protected by the event_lock spinlock.
- nouveau: With the pin/unpin functions fixed, everything looks safe:
A bit of ttm wrestling and refcounting, and a few channel accesses.
The later are either already proteced sufficiently, or are now safe
with the channel locking introduced to make cursor updates safe.
- radeon: The irq_get/put functions look a bit race, since the
atomic_inc/dec isn't protect with locks. Otoh they're all per-crtc,
so we should be safe with per-crtc locking from the drm core. Then
there's tons of per-crtc register access, which could potentially go
through the indirect reg acces. But that's fixed to make cursor
updates concurrent. Bookeeping for the drm even is also protected
with the even_lock, which also protects against the pageflip irq
handler since radeon hw seems to have no way to queue these up
asynchronously. Otherwise just a bit of ttm-based buffer handling
and fencing, which is now safe with the previous patch to hold
bdev->fence_lock while grabbing the ttm fence.
- shmob: Only one crtc. That's an easy one ...
- vmwgfx: As usual a bit special with tons different things:
- Flippable check using is_implicit and num_implicit. Changes to
those seem to be nicely covered with the global modeset lock, so
we should be fine.
- Some dirty cliprect handling stuff, or at least that is my guess.
Looks like it's fine since either it's per-crtc, invariant or
(like the execbuf stuff launched) protected otherwise.
- Adding the actual flip to the fence_event list. On a quick look
this seems to have solid locking in place, too.
... but generally this is all way over my head.
- imx: Impressive display of races between the page_flip
implementation and the irq handler. Also, ipu_drm_set_base which
gets eventually called from the irq handler to update the display
base isn't really protected against concurrent set_config calls from
process context. In any case, going for per-crtc locking won't make
this worse, so nothing to do.
- omap: The new async callback code merged into 3.8 seems to have
solid locking in place, and there doesn't seem to be any shared
state at risk. Especially since the callbacks still use
modeset_lock_all and are so not converted.
v2: Update omapdrm analysis to 3.8 code per the discussion with Rob
Clark.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Now that all framebuffer usage is properly refcounted, we are no
longer required to hold the modeset locks while dropping the last
reference. Hence implemented a fastpath which avoids the potential
stalls associated with grabbing mode_config.lock for the case where
there's no other reference around.
Explain in a big comment why it is safe. Also update kerneldocs with
the new locking rules around drm_framebuffer_remove.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Afact vmwgfx already has all the right refcounting implemented on the
backing storage, and we only need to ensure that the drm fb doesn't
disappear untimely. So holding onto the fb reference from _lookup
until vmw_kms_present has completed should be enough.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
With the prep patch to encapsulate ->set_crtc calls, this is now
rather easy. Hooray for inconsistent semantics between ->set_crtc and
->page_flip, where the driver callback is supposed to update the fb
pointer, and ->update_plane, where the drm core does the same.
Also, since the drm core functions check crtc->fb before calling into
driver callbacks, we can't really reduce the critical sections
protected by the mode_config locks.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Now plane->fb holds a reference onto it's framebuffer. Nothing too
fancy going on here:
- Extract __drm_framebuffer_unreference to be called when we know
we're not dropping the last reference, e.g. useful in the fb cleanup
code.
- Reduce the locked sections in the set_plane ioctl to only protect
plane->fb/plane->crtc and the driver callback (i.e. hw state).
Everything either doesn't disappear (crtc, plane) or is refcounted
(fb), and all the data we check is invariant over the respective
object's lifetimes.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
We only need to ensure that the fb stays around for long enough. While
at it, only grab the modeset locks when we need them (since most
drivers don't implement the dirty callback, this should help jitter
and stalls when using the generic modeset driver).
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
We only need to push the fb unreference a bit down. While at it,
properly pass the return value from ->create_handle back to userspace.
Most drivers either return -ENODEV if they don't have a concept of
buffer objects (ast, cirrus, ...) or just install a handle for the
underlying gem object (which is ok since we hold a reference on that
through the framebuffer).
v2: Split out the ->create_handle rework in the individual drivers.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
And drop it where it's not needed. Most driver just lookup the gem
object, allocate an fb struct, fill in all the useful fields and then
register it with drm_framebuffer_init.
All of these operations are already separately locked, and since we
only put the fb into the fpriv->fbs list _after_ having called
->fb_create, we can't also race with rmfb. We can otoh race with other
ioctls that put the framebuffer to use, but all drivers have been
reorganized already to call drm_framebuffer_init last in the fb
creation sequence.
So essentially, we can completely remove any modeset locks from the
addfb ioctl paths. Yeah!
Also, reference-counting is solid - we get a reference from fb_create
which we transfer to the fpriv->fbs list. And after unlocking the
fpriv->fbs_lock we don't touch the framebuffer any longer. Furthermore
drm_framebuffer_init has added a 2nd reference for the idr lookup, and
any access through that table will do it's own refcounting.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Atm we still need to unconditionally take the modeset locks in the
rmfb paths. But eventually we only want to take them if there are
other users around as a slow-path. This way sane userspace avoids
blocking on edid reads and other stuff in rmfb if it ensures that the
fb isn't used anywhere by a crtc/plane.
We can do a quick check for such other users once framebuffers are
properly refcounting by locking at the refcount - if it's more than 1,
there are other users left. Again, rmfb racing against other ioctls
isn't a real problem, userspace is allowed to shoot its foot.
This patch just prepares this by moving the modeset locks to nest
within fpriv->fbs_lock. Now the distinction between the fbs_lock and
the device-global fb_lock is clear, since we need to hold the fbs_lock
outside of any modeset_locks in fb_release.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Since otherwise looking and reference-counting around
drm_framebuffer_lookup will be an unmanageable mess. With this change,
an object can either be found in the idr and will stay around once we
incremented the reference counter. Or it will be gone for good and
can't be looked up using its id any more.
Atomicity is guaranteed by the dev->mode_config.fb_lock. The
newly-introduce fpriv->fbs_lock looks a bit redundant, but the next
patch will shuffle the locking order between these two locks and all
the modeset locks taken in modeset_lock_all, so we'll need it.
Also, since userspace could do really funky stuff and race e.g. a
getresources with an rmfb, we need to make sure that the kernel
doesn't fall over trying to look-up an inexistent fb, or causing
confusion by having two fbs around with the same id. Simply reset the
framebuffer id to 0, which marks it as reaped. Any lookups of that id
will fail, so the object is really gone for good from userspace's pov.
Note that we still need to protect the "remove framebuffer from all
use-cases" and the final unreference with the modeset-lock, since most
framebuffer use-sites don't implement proper reference counting yet.
We can only lift this once _all_ users are converted.
With this change, two references are held on alife, but unused
framebuffers:
- The reference for the idr lookup, created in this patch.
- For user-created framebuffers the fpriv->fbs reference, for
driver-private fbs the driver is supposed to hold it's own last
reference.
Note that the dev->mode_config.fb_list itself does _not_ hold a
reference onto the framebuffers (this list is essentially only used
for debugfs files). Hence if there's anything left there when the
driver has cleaned up all it's modeset resources, this is a ref-leak.
WARN about it.
Now we only need to fix up all other places to properly reference
count framebuffers.
v2: Fix spelling fail in a comment spotted by Rob Clark.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
We have two classes of framebuffer
- Created by the driver (atm only for fbdev), and the driver holds
onto the last reference count until destruction.
- Created by userspace and associated with a given fd. These
framebuffers will be reaped when their assoiciated fb is closed.
Now these two cases are set up differently, the framebuffers are on
different lists and hence destruction needs to clean up different
things. Also, for userspace framebuffers we remove them from any
current usage, whereas for internal framebuffers it is assumed that
the driver has done this already.
Long story short, we need two different ways to cleanup such drivers.
Three functions are involved in total:
- drm_framebuffer_remove: Convenience function which removes the fb
from all active usage and then drops the passed-in reference.
- drm_framebuffer_unregister_private: Will remove driver-private
framebuffers from relevant lists and drop the corresponding
references. Should be called for driver-private framebuffers before
dropping the last reference (or like for a lot of the drivers where
the fbdev is embedded someplace else, before doing the cleanup
manually).
- drm_framebuffer_cleanup: Final cleanup for both classes of fbs,
should be called by the driver's ->destroy callback once the last
reference is gone.
This patch just rolls out the new interfaces and updates all drivers
(by adding calls to drm_framebuffer_unregister_private at all the
right places)- no functional changes yet. Follow-on patches will move
drm core code around and update the lifetime management for
framebuffers, so that we are no longer required to keep framebuffers
alive by locking mode_config.mutex.
I've also updated the kerneldoc already.
vmwgfx seems to again be a bit special, at least I haven't figured out
how the fbdev support in that driver works. It smells like it's
external though.
v2: The i915 driver creates another private framebuffer in the
load-detect code. Adjust its cleanup code, too.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
And replace all fb lookups with it. Also add a WARN to
drm_mode_object_find since that is now no longer the blessed interface
to look up an fb. And add kerneldoc to both functions.
This only updates all callsites, but immediately drops the acquired
refence again. Hence all callers still rely on the fact that a mode fb
can't disappear while they're holding the struct mutex. Subsequent
patches will instate proper use of refcounts, and then rework the rmfb
and unref code to no longer serialize fb destruction with the
mode_config lock. We don't want that since otherwise a compositor
might end up stalling for a few frames in rmfb.
v2: Don't use kref_get_unless_zero - Greg KH doesn't like that kind of
interface.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Well, at least step 1. The goal here is that framebuffer objects can
survive outside of the mode_config lock, with just a reference held
as protection. The first step to get there is to introduce a special
fb_lock which protects fb lookup, creation and destruction, to make
them appear atomic.
This new fb_lock can nest within the mode_config lock. But the idea is
(once the reference counting part is completed) that we only quickly
take that fb_lock to lookup a framebuffer and grab a reference,
without any other locks involved.
vmwgfx is the only driver which does framebuffer lookups itself, also
wrap those calls to drm_mode_object_find with the new lock.
Also protect the fb_list walking in i915 and omapdrm with the new lock.
As a slight complication there's also the list of user-created fbs
attached to the file private. The problem now is that at fclose() time
we need to walk that list, eventually do a modeset call to remove the
fb from active usage (and are required to be able to take the
mode_config lock), but in the end we need to grab the new fb_lock to
remove the fb from the list. The easiest solution is to add another
mutex to protect this per-file list.
Currently that new fbs_lock nests within the modeset locks and so
appears redudant. But later patches will switch around this sequence
so that taking the modeset locks in the fb destruction path is
optional in the fastpath. Ultimately the goal is that addfb and rmfb
do not require the mode_config lock, since otherwise they have the
potential to introduce stalls in the pageflip sequence of a compositor
(if the compositor e.g. switches to a fullscreen client or if it
enables a plane). But that requires a few more steps and hoops to jump
through.
Note that framebuffer creation/destruction is now double-protected -
once by the fb_lock and in parts by the idr_lock. The later would be
unnecessariy if framebuffers would have their own idr allocator. But
that's material for another patch (series).
v2: Properly initialize the fb->filp_head list in _init, otherwise the
newly added WARN to check whether the fb isn't on a fpriv list any
more will fail for driver-private objects.
v3: Fixup two error-case unlock bugs spotted by Richard Wilbur.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
->cursor_move uses mostly the same facilities in drivers as
->cursor_set, so pretty much nothing to fix up:
- ast/gma500/i915: They all use per-crtc registers to update the
cursor position. ast again touches the global cursor cache, but
that's ok since there's only one crtc.
- nouveau: nv50+ is again special, updates happen through the per-crtc
channel (without pushbufs), so it's not protected by the new evo
lock introduced earlier. But since this channel is per-crtc, we
should be fine anyway.
- radeon: A bit a mess: avivo asics need a workaround when both output
pipes are enabled, which means it'll access the crtc list. Just
reading that flag is ok though as long as radeon _always_ grabs all
locks when changing the crtc configuration. Which means with the
current scheme it cannot do an optimized modeset which only locks
the relevant crtcs. This can be fixed though by introducing a bit of
global state with separate locks and ensure in the modeset code that
the cursor will be updated appropriately when enabling the 2nd pipe
(on affected asics).
- vmwgfx: I still don't understand what it's doing exactly, so apply
the same trick for now.
v2: Fixup unlocking for the error cases, spotted by Richard Wilbur.
v3: Another error-case fixup.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
First convert ->cursor_set to only take the crtc lock, since that
seems to be the function with the least amount of state - the core
ioctl function doesn't check anything which can change at runtime, so
we don't have any object lifetime issues to contend.
The only thing which is important is that the driver's implementation
doesn't touch any state outside of that single crtc which is not yet
properly protected by other locking:
- ast: access the global ast->cache_kmap. Luckily we only have on crtc
on this driver, so this is fine. Add a comment.
- gma500: calls gma_power_begin|and and psb_gtt_pin|unpin, both which
have their own locking to protect their state. Everything else is
crtc-local.
- i915: touches a bit of global gem state, all protected by the One
Lock to Rule Them All (dev->struct_mutex).
- nouveau: Pre-nv50 is all nice, nv50+ uses the evo channels to queue
up all display changes. And some of these channels are device
global. But this is fine now since the previous patch introduced an
evo channel mutex.
- radeon: Uses some indirect register access for cursor updates, but
with the previous patches to protect these indirect 2-register
access patterns with a spinlock, this should be fine now, too.
- vmwgfx: I have no idea how that works - update_cursor_position
doesn't take any per-crtc argument and I haven't figured out any
other place where this could be set in some form of a side-channel.
But vmwgfx definitely has more than one crtc (or at least can
register more than one), so I have no idea how this is supposed to
not fail with the current code already. Hence take the easy way out
and simply acquire all locks (which requires dropping the crtc lock
the core acquired for us). That way it's not worse off for
consistency than the old code.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
*drumroll*
The basic idea is to protect per-crtc state which can change without
touching the output configuration with separate mutexes, i.e. all the
input side state to a crtc like framebuffers, cursor settings or plane
configuration. Holding such a crtc lock gives a read-lock on all the
other crtc state which can be changed by e.g. a modeset.
All non-crtc state is still protected by the mode_config mutex.
Callers that need to change modeset state of a crtc (e.g. dpms or
set_mode) need to grab both the mode_config lock and nested within any
crtc locks.
Note that since there can only ever be one holder of the mode_config
lock we can grab the subordinate crtc locks in any order (if we need
to grab more than one of them). Lockdep can handle such nesting with
the mutex_lock_nest_lock call correctly.
With this functions that only touch connectors/encoders but not crtcs
only need to take the mode_config lock. The biggest such case is the
output probing, which means that we can now pageflip and move cursors
while the output probe code is reading an edid.
Most cases neatly fall into the three buckets:
- Only touches connectors and similar output state and so only needs
the mode_config lock.
- Touches the global configuration and so needs all locks.
- Only touches the crtc input side and so only needs the crtc lock.
But a few cases that need special consideration:
- Load detection which requires a crtc. The mode_config lock already
prevents a modeset change, so we can use any unused crtc as we like
to do load detection. The only thing to consider is that such
temporary state changes don't leak out to userspace through ioctls
that only take the crtc look (like a pageflip). Hence the load
detect code needs to grab the crtc of any output pipes it touches
(but only if it touches state used by the pageflip or cursor
ioctls).
- Atomic pageflip when moving planes. The first case is sane hw, where
planes have a fixed association with crtcs - nothing needs to be
done there. More insane^Wflexible hw needs to have plane->crtc
mapping which is separately protect with a lock that nests within
the crtc lock. If the plane is unused we can just assign it to the
current crtc and continue. But if a plane is already in use by
another crtc we can't just reassign it.
Two solution present themselves: Either go back to a slow-path which
takes all modeset locks, potentially incure quite a hefty delay. Or
simply disallowing such changes in one atomic pageflip - in general
the vblanks of two crtcs are not synced, so there's no sane way to
atomically flip such plane changes accross more than one crtc. I'd
heavily favour the later approach, going as far as mandating it as
part of the ABI of such a new a nuclear pageflip.
And if we _really_ want such semantics, we can always get them by
introducing another pageflip mutex between the mode_config.mutex and
the individual crtc locks. Pageflips crossing more than one crtc
would then need to take that lock first, to lock out concurrent
multi-crtc pageflips.
- Optimized global modeset operations: We could just take the
mode_config lock and then lazily lock all crtc which are affected by
a modeset operation. This has the advantage that pageflip could
continue unhampered on unaffected crtc. But if e.g. global resources
like plls need to be reassigned and so affect unrelated crtcs we can
still do that - nested locking works in any order.
This patch just adds the locks and takes them in drm_modeset_lock_all,
no real locking changes yet.
v2: Need to initialize the new lock in crtc_init and lock it righ
away, for otherwise the modeset_unlock_all below will try to unlock a
not-locked mutex.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
I've left the locking in the debugfs code as-is, it's essentially just
used to keep the framebuffer object alive (which won't be necessary
any more later on). We don't need fb refcounting either, since the new
mode_config.fb_lock ensures that the framebuffers can't disappear
(once mode_config.mutex doesn't guarantee this any more later on in
the series).
The fbcon restore needs all modeset locks. The crtc callbacks seem to
only need the crtc locks, but I've quickly discussed things with Rob
Clark and he's fine with just using modeset_lock_all for those, too.
He'll look into converting things over later.
Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Ok, this one here is a bit more complicated, and I can't really claim
to fully understand the locking and lifetime rules of the vmwgfx
driver. So just convert ever mutex_lock call, including the
interruptible one. Since other places (e.g. in the execbuf ioctl) take
the mode_config.mutex without bothering with interruptible handling,
I've figured I should be able to get away with this in a few more
places ...
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
