1. 21 1月, 2013 13 次提交
    • D
      drm: revamp framebuffer cleanup interfaces · 36206361
      Daniel Vetter 提交于
      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: NRob Clark <rob@ti.com>
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      36206361
    • D
      drm: create drm_framebuffer_lookup · 786b99ed
      Daniel Vetter 提交于
      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: NRob Clark <rob@ti.com>
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      786b99ed
    • D
      drm: revamp locking around fb creation/destruction · 4b096ac1
      Daniel Vetter 提交于
      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: NRob Clark <rob@ti.com>
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      4b096ac1
    • D
      drm: only take the crtc lock for ->cursor_move · dac35663
      Daniel Vetter 提交于
      ->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: NRob Clark <rob@ti.com>
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      dac35663
    • D
      drm: only take the crtc lock for ->cursor_set · bfb89928
      Daniel Vetter 提交于
      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: NRob Clark <rob@ti.com>
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      bfb89928
    • D
      drm: add per-crtc locks · 29494c17
      Daniel Vetter 提交于
      *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: NRob Clark <rob@ti.com>
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      29494c17
    • D
      omapdrm: use modeset_lock_all · d5d2636e
      Daniel Vetter 提交于
      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: NRob Clark <rob@ti.com>
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      d5d2636e
    • D
      drm/vmwgfx: use drm_modeset_lock_all · bbe4b99f
      Daniel Vetter 提交于
      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: NDaniel Vetter <daniel.vetter@ffwll.ch>
      bbe4b99f
    • D
      drm/shmobile: use drm_modeset_lock_all · b13f5980
      Daniel Vetter 提交于
      Only a resume method to account for.
      
      v2: Fixup compilation.
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      b13f5980
    • D
      drm/ast: use drm_modeset_lock_all · 795f1426
      Daniel Vetter 提交于
      Just a call to drm_helper_resume_force_mode, obviously wants full
      locking for that.
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      795f1426
    • D
      drm/gma500: use drm_modeset_lock_all · 0a819515
      Daniel Vetter 提交于
      Only two places:
      - suspend/resume
      - Some really strange mode validation tool with too much funny-lucking
        hand-rolled conversion code.
      - The recently-added lastclose fbdev restore code.
      
      Better safe than sorry, so convert both places to keep the locking
      semantics as much as possible.
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      0a819515
    • D
      drm/i915: use drm_modeset_lock_all · a0e99e68
      Daniel Vetter 提交于
      Two exceptions:
      - debugfs files only read information which is not related to crtc, so
        can stay on the modeset_config lock.
      - Same holds for the edp vdd work in intel_dp.c. Add a corresponding
        WARN_ON and a comment next to the intel_dp struct fields for
        documentation.
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      a0e99e68
    • D
      drm: add drm_modeset_lock|unlock_all · 84849903
      Daniel Vetter 提交于
      This is the first step towards introducing the new modeset locking
      scheme. The plan is to put helper functions into place at all the
      right places step-by-step, so that the final patch to switch on the
      new locking scheme doesn't need to touch every single driver.
      
      This helper here will serve as the shotgun solutions for all places
      where a more fine-grained locking isn't (yet) implemented.
      
      v2: Fixup kerneldoc for unlock_all.
      Reviewed-by: NRob Clark <rob@ti.com>
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      84849903
  2. 20 1月, 2013 9 次提交
    • D
      drm: encapsulate crtc->set_config calls · 2d13b679
      Daniel Vetter 提交于
      With refcounting we need to adjust framebuffer refcounts at each
      callsite - much easier to do if they all call the same little helper
      function.
      Reviewed-by: NRob Clark <rob@ti.com>
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      2d13b679
    • D
      drm/<drivers>: Unified handling of unimplemented fb->create_handle · af26ef3b
      Daniel Vetter 提交于
      Some drivers don't have real ->create_handle callbacks.
      
      - cirrus/ast/mga200: Returns either 0 or -EINVAL.
      
      - udl: Didn't even bother with a callback, leading to a nice
        userspace-triggerable OOPS.
      
      - vmwgfx: This driver bothered with an implementation to return 0 as
        the handle (which is the canonical no-obj gem handle).
      
      All have in common that ->create_handle doesn't really make too much
      sense for them - that ioctl is used only for seamless fb takeover in
      the radeon/nouveau/i915 ddx drivers. So allow drivers to not implement
      this and return a consistent -ENODEV.
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      af26ef3b
    • D
      drm/nouveau: try to protect nbo->pin_refcount · 0ae6d7bc
      Daniel Vetter 提交于
      ... by moving the bo_pin/bo_unpin manipulation of the pin_refcount
      under the protection of the ttm reservation lock. pin/unpin seems
      to get called from all over the place, so atm this is completely racy.
      
      After this patch there are only a few places in cleanup functions
      left which access ->pin_refcount without locking. But I'm hoping that
      those are safe and some other code invariant guarantees that this
      won't blow up.
      
      In any case, I only need to fix up pin/unpin to make ->pageflip work
      safely, so let's keep it at that.
      
      Add a comment to the header to explain the new locking rule.
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      0ae6d7bc
    • D
      drm/nouveau: protect evo_wait/evo_kick sections with a channel mutex · 59ad1465
      Daniel Vetter 提交于
      With per-crtc locks modeset operations can run in parallel, and the
      cursor code uses the device-global evo master channel for hw frobbing.
      But the pageflip code can also sync with the master under some
      circumstances. Hence just wrap things up in a mutex to ensure that
      pushbuf access doesn't intermingle.
      
      The approach here is a bit overkill since the per-crtc channels used
      to schedule the pageflips could probably be used without this pushbuf
      locking, but I'm not familiar enough with the nouveau codebase to be
      sure of that.
      
      v2: Add missing mutex_init to avoid angering lockdep.
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      59ad1465
    • D
      drm/gma500: move fbcon restore to lastclose · 7147573a
      Daniel Vetter 提交于
      Doing this within the fb->destroy callback leads to a locking
      nightmare. And all other drm drivers that restore the fbcon do
      it in lastclose, too.
      
      With this adjustments all fb->destroy callbacks optionally drop
      references to any gem objects used as backing storage, call
      drm_framebuffer_cleanup and then kfree the struct. Which nicely
      simplifies the locking for framebuffer unreferencing and freeing,
      since this doesn't require that we hold the mode_config lock. A
      slight exception is the vmwgfx surface backed framebuffer, it also
      calls drm_master_put and removes the object from a device-private
      framebuffer list. Both seem to have solid locking in place already.
      
      Conclusion is that now it is no longer required to hold the
      mode_config lock while freeing a framebuffer.
      
      v2: Drop the corresponding mutex_lock WARN check from
      drm_framebuffer_unreference.
      
      v3: Use just the mode_config lock not modeset_lock_all, due to patch
      reordering.
      Acked-by: NAlan Cox <alan@lxorguk.ukuu.org.uk>
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      7147573a
    • D
      drm/vmwgfx: reorder framebuffer init sequence · 80f0b5af
      Daniel Vetter 提交于
      vmwgfx has an oddity, when failing to reference the surface it'll
      return 0, since that's what the successfull drm_framebuffer_init will
      leave behind in ret. Fix this up by returning -EINVAL.
      
      Split out from all the other driver updates due to the above tiny
      semantic change. Shouldn't matter though since the reference grabbing
      seemingly can't fail.
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      80f0b5af
    • D
      drm/<drivers>: reorder framebuffer init sequence · c7d73f6a
      Daniel Vetter 提交于
      With more fine-grained locking we can no longer rely on the big
      mode_config lock to prevent concurrent access to mode resources
      like framebuffers. Instead a framebuffer becomes accessible to
      other threads as soon as it is added to the relevant lookup
      structures. Hence it needs to be fully set up by the time drivers
      call drm_framebuffer_init.
      
      This patch here is the drivers part of that reorg. Nothing really fancy
      going on safe for three special cases.
      
      - exynos needs to be careful to properly unref all handles.
      - nouveau gets a resource leak fixed for free: one of the error
        cases didn't cleanup the framebuffer, which is now moot since
        the framebuffer is only registered once it is fully set up.
      - vmwgfx requires a slight reordering of operations, I'm hoping I didn't
        break anything (but it's refcount management only, so should be safe).
      
      v2: Split out exynos, since it's a bit more hairy than expected.
      
      v3: Drop bogus cirrus hunk noticed by Richard Wilbur.
      
      v4: Split out vmwgfx since there's a small change in return values.
      
      Reviewed-by: Rob Clark <rob@ti.com> (core + omapdrm)
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      c7d73f6a
    • D
      drm/doc: integrate drm_crtc.c kerneldoc · 065a50ed
      Daniel Vetter 提交于
      And do a quick pass to adjust them to the last few (years?) of changes
      ...
      
      This time actually compile-tested ;-)
      Reviewed-by: NRob Clark <rob@ti.com>
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      065a50ed
    • D
      drm: review locking rules in drm_crtc.c · 8faf6b18
      Daniel Vetter 提交于
      - config_cleanup was confused: It claimed that callers need to hold
        the modeset lock, but the connector|encoder_cleanup helpers grabbed
        that themselves (note that crtc_cleanup did _not_ grab the modeset
        lock). Which resulted in all drivers _not_ hodling the lock. Since
        this is for single-threaded cleanup code, drop the requirement from
        docs and also drop the lock_grabbing from all _cleanup functions.
      
      - Kill the LOCKING section in the doctype, since clearly we're not
        good enough to keep them up-to-date. And misleading locking
        documentation is worse than useless (see e.g. the comment in the
        vmgfx driver about the cleanup mess). And since for most functions
        the very first line either grabs the lock or has a WARN_ON(!locked)
        the documentation doesn't really add anything.
      
      - Instead put in some effort into explaining the only two special
        cases a bit better: config_init and config_cleanup are both called
        from single-threaded setup/teardown code, so don't do any locking.
        It's the driver's job though to enforce this.
      
      - Where lacking, add a WARN_ON(!is_locked). Not many places though,
        since locking around fbdev setup/teardown is through-roughly screwed
        up, and so will break almost every single WARN annotation I've tried
        to add.
      
      - Add a drm_modeset_is_locked helper - the Grate Modset Locking Rework
        will use the compiler to assist in the big reorg by renaming the
        mode lock, so start encapsulating things. Unfortunately this ended
        up in the "wrong" header file since it needs the definition of
        struct drm_device.
      
      v2: Drop most WARNS again - we hit them all over the place, mostly in
      the setup and teardown sequences. And trying to fix it up leads to
      nice deadlocks, since the locking in the setup code is really
      inconsistent.
      Reviewed-by: NRob Clark <rob@ti.com>
      Signed-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
      8faf6b18
  3. 18 1月, 2013 2 次提交
  4. 17 1月, 2013 16 次提交