After warning that the connector list is not empty on device
unregistration (i.e. module unload) also print out which connectors are
still hanging around to aide finding the leak.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: http://patchwork.freedesktop.org/patch/msgid/20170119090513.4154-1-chris@chris-wilson.co.uk
Only static connectors should be left at this point, and we should be
able to clean them out by simply dropping that last reference still
around from drm_connector_init.
If that leaves anything behind then we have a driver bug.
Doing the final cleanup this way also allows us to use
drm_connector_iter, removing the very last place where we walk
connector_list explicitly in drm core&helpers.
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20161213230814.19598-8-daniel.vetter@ffwll.ch
The requirements for connector_list locking are a bit tricky:
- We need to be able to jump over zombie conectors (i.e. with refcount
== 0, but not yet removed from the list). If instead we require that
there's no zombies on the list then the final kref_put must happen
under the list protection lock, which means that locking context
leaks all over the place. Not pretty - better to deal with zombies
and wrap the locking just around the list_del in the destructor.
- When we walk the list we must _not_ hold the connector list lock. We
walk the connector list at an absolutely massive amounts of places,
if all those places can't ever call drm_connector_unreference the
code would get unecessarily complicated.
- connector_list needs it own lock, again too many places that walk it
that we could reuse e.g. mode_config.mutex without resulting in
inversions.
- Lots of code uses these loops to look-up a connector, i.e. they want
to be able to call drm_connector_reference. But on the other hand we
want connectors to stay on that list until they're dead (i.e.
connector_list can't hold a full reference), which means despite the
"can't hold lock for the loop body" rule we need to make sure a
connector doesn't suddenly become a zombie.
At first Dave&I discussed various horror-show approaches using srcu,
but turns out it's fairly easy:
- For the loop body we always hold an additional reference to the
current connector. That means it can't zombify, and it also means
it'll stay on the list, which means we can use it as our iterator to
find the next connector.
- When we try to find the next connector we only have to jump over
zombies. To make sure we don't chase bad pointers that entire loop
is protected with the new connect_list_lock spinlock. And because we
know that we're starting out with a non-zombie (need to drop our
reference for the old connector only after we have our new one),
we're guranteed to still be on the connector_list and either find
the next non-zombie or complete the iteration.
- Only downside is that we need to make sure that the temporary
reference for the loop body doesn't leak. iter_get/put() functions +
lockdep make sure that's the case.
- To avoid a flag day the new iterator macro has an _iter postfix. We
can rename it back once all the users of the unsafe version are gone
(there's about 100 list walkers for the connector_list).
For now this patch only converts all the list walking in the core,
leaving helpers and drivers for later patches. The nice thing is that
we can now finally remove 2 FIXME comments from the
register/unregister functions.
v2:
- use irqsafe spinlocks, so that we can use this in drm_state_dump
too.
- nuke drm_modeset_lock_all from drm_connector_init, now entirely
cargo-culted nonsense.
v3:
- do {} while (!kref_get_unless_zero), makes for a tidier loop (Dave).
- pretty kerneldoc
- add EXPORT_SYMBOL, helpers&drivers are supposed to use this.
v4: Change lockdep annotations to only check whether we release the
iter fake lock again (i.e. make sure that iter_put is called), but
not check any locking dependecies itself. That seams to require a
recursive read lock in trylock mode.
Cc: Dave Airlie <airlied@gmail.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20161213230814.19598-6-daniel.vetter@ffwll.ch
This is single-threaded setup code, no need for locks. And anyway,
all properties need to be set up before the driver is registered
anyway, they can't be hot-added.
Reviewed-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20161213230814.19598-5-daniel.vetter@ffwll.ch
<drm/drm_crtc.h> used to define most of the in-kernel KMS API. It has
now been split into separate files for each object type, but still
includes most other KMS headers to avoid breaking driver compilation.
As a step towards fixing that problem, remove the inclusion of
<drm/drm_encoder.h> from <drm/drm_crtc.h> and include it instead where
appropriate. Also remove the forward declarations of the drm_encoder and
drm_encoder_helper_funcs structures from <drm/drm_crtc.h> as they're not
needed in the header.
<drm/drm_encoder.h> now has to include <drm/drm_mode.h> and contain a
forward declaration of struct drm_encoder in order to allow including it
as the first header in a compilation unit.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Sinclair Yeh <syeh@vmware.com> # For vmwgfx
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Archit Taneja <architt@codeaurora.org>
Link: http://patchwork.freedesktop.org/patch/msgid/1481709550-29226-2-git-send-email-laurent.pinchart+renesas@ideasonboard.com
Looping twice when we can do it once is silly. Also use a consistent
style. Note that there's a good race with the connector list walking,
since that is no longer protected by mode_config.mutex. But that's for
a later patch to fix.
v2: Actually try to not blow up, somehow I lost the hunk that checks
we don't copy too much. Noticed by Chris.
v3:
- squash all drm_mode_getresources cleanups into one
- use consistent style for walking objects (Chris)
v4:
- Use u64_to_user_ptr (Chris)
- Don't forget to copy the last connector (Chris)
v5: Chris was right ...
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20161211192019.29603-1-daniel.vetter@ffwll.ch
Support DRM out-fences by creating a sync_file with a fence for each CRTC
that sets the OUT_FENCE_PTR property.
We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
the sync_file fd back to userspace.
The sync_file and fd are allocated/created before commit, but the
fd_install operation only happens after we know that commit succeed.
v2: Comment by Rob Clark:
- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
Comment by Daniel Vetter:
- Add clean up code for out_fences
v3: Comments by Daniel Vetter:
- create DRM_MODE_ATOMIC_EVENT_MASK
- userspace should fill out_fences_ptr with the crtc_ids for which
it wants fences back.
v4: Create OUT_FENCE_PTR properties and remove old approach.
v5: Comments by Brian Starkey:
- Remove extra fence_get() in atomic_ioctl()
- Check ret before iterating on the crtc_state
- check ret before fd_install
- set fence_state to NULL at the beginning
- check fence_state->out_fence_ptr before put_user()
- change order of fput() and put_unused_fd() on failure
- Add access_ok() check to the out_fence_ptr received
- Rebase after fence -> dma_fence rename
- Store out_fence_ptr in the drm_atomic_state
- Split crtc_setup_out_fence()
- return -1 as out_fence with TEST_ONLY flag
v6: Comments by Daniel Vetter
- Add prepare/unprepare_crtc_signaling()
- move struct drm_out_fence_state to drm_atomic.c
- mark get_crtc_fence() as static
Comments by Brian Starkey
- proper set fence_ptr fence_state array
- isolate fence_idx increment
- improve error handling
v7: Comments by Daniel Vetter
- remove prefix from internal functions
- make out_fence_ptr an s64 pointer
- degrade DRM_INFO to DRM_DEBUG_ATOMIC when put_user fail
- fix doc issues
- filter out OUT_FENCE_PTR == NULL and do not fail in this case
- add complete_crtc_signalling()
- krealloc fence_state on demand
Comment by Brian Starkey
- remove unused crtc_state arg from get_out_fence()
v8: Comment by Brian Starkey
- cancel events before check for !fence_state
- convert a few lefovers u64 types for out_fence_ptr
- fix memleak by assign fence_state earlier after realloc
- proper accout num_fences in case of error
v9: Comment by Brian Starkey
- memset last position of fence_state after krealloc
Comments by Sean Paul
- pass install_fds in complete_crtc_signaling() instead of ret
- put_user(-1, fence_ptr) when decoding props
v10: Comment by Brian Starkey
- remove unneeded num_fences increment on error path
- kfree fence_state after installing fences fd
v11: rebase against latest drm-misc
v12: rebase again against latest drm-misc
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Brian Starkey <brian.starkey@arm.com> (v10)
Reviewed-by: Sean Paul <seanpaul@chromium.org>
Tested-by: Robert Foss <robert.foss@collabora.com> (v10)
[danvet: Appease checkpatch.]
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: http://patchwork.freedesktop.org/patch/msgid/1479301221-13056-1-git-send-email-gustavo@padovan.org
There is now a new property called IN_FENCE_FD attached to every plane
state that receives sync_file fds from userspace via the atomic commit
IOCTL.
The fd is then translated to a fence (that may be a fence_array
subclass or just a normal fence) and then used by DRM to fence_wait() for
all fences in the sync_file to signal. So it only commits when all
framebuffers are ready to scanout.
v2: Comments by Daniel Vetter:
- remove set state->fence = NULL in destroy phase
- accept fence -1 as valid and just return 0
- do not call fence_get() - sync_file_fences_get() already calls it
- fence_put() if state->fence is already set, in case userspace
set the property more than once.
v3: WARN_ON if fence is set but state has no FB
v4: Comment from Maarten Lankhorst
- allow set fence with no related fb
v5: rename FENCE_FD to IN_FENCE_FD
v6: Comments by Daniel Vetter:
- rename plane_state->in_fence back to "fence"
- re-introduce WARN_ON if fence set but no fb
- rebase after fence -> dma_fence rename
v7: Comments by Brian Starkey
- set state->fence to NULL when duplicating the state
- fail if IN_FENCE_FD was already set
v8: rebase against latest drm-misc
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Sean Paul <seanpaul@chromium.org>
Tested-by: Robert Foss <robert.foss@collabora.com>
[danvet: Rebase onto extracted drm_mode_config.[hc].]
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
And shuffle the kernel-doc structure a bit since drm_crtc.[hc] now
only contains CRTC-related functions and structures.
v2:
- rebase onto drm-misc
- don't forget to move drm_mode_config_cleanup.
- move 2 internal decls under the right heading (Chris)
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
