• Lyude Paul's avatar
    drm/nouveau/kms/nv50-: Refactor and cleanup DP HPD handling · a0922278
    Lyude Paul authored
    First some backstory here: Currently, we keep track of whether or not
    we've enabled MST or not by trying to piggy-back off the MST helpers.
    This means that in order to check whether MST is enabled or not, we
    actually need to grab drm_dp_mst_topology_mgr.lock.
    
    Back when I originally wrote this, I did this piggy-backing with the
    intention that I'd eventually be teaching our MST helpers how to recover
    when an MST device has stopped responding, which in turn would require
    the MST helpers having a way of disabling MST independently of the
    driver. Note that this was before I reworked locking in the MST helpers,
    so at the time we were sticking random things under &mgr->lock - which
    grabbing this lock was meant to protect against.
    
    This never came to fruition because doing such a reset safely turned out
    to be a lot more painful and impossible then it sounds, and also just
    risks us working around issues with our MST handlers that should be
    properly fixed instead. Even if it did though, simply calling
    drm_dp_mst_topology_mgr_set_mst() from the MST helpers (with the
    exception of when we're tearing down our MST managers, that's always OK)
    wouldn't have been a bad idea, since drivers like nouveau and i915 need
    to do their own book keeping immediately after disabling MST.
    So-implementing that would likely require adding a hook for
    helper-triggered MST disables anyway.
    
    So, fast forward to now - we want to start adding support for all of the
    miscellaneous bits of the DP protocol (for both SST and MST) we're
    missing before moving on to supporting more complicated features like
    supporting different BPP values on MST, DSC, etc. Since many of these
    features only exist on SST and make use of DP HPD IRQs, we want to be
    able to atomically check whether we're servicing an MST IRQ or SST IRQ
    in nouveau_connector_hotplug(). Currently we literally don't do this at
    all, and just handle any kind of possible DP IRQ we could get including
    ESIs - even if MST isn't actually enabled.
    
    This would be very complicated and difficult to fix if we need to hold
    &mgr->lock while handling SST IRQs to ensure that the MST topology
    state doesn't change under us. What we really want here is to do our own
    tracking of whether MST is enabled or not, similar to drivers like i915,
    and define our own locking order to decomplicate things and avoid
    hitting locking issues in the future.
    
    So, let's do this by refactoring our MST probing/enabling code to use
    our own MST bookkeeping, along with adding a lock for protecting DP
    state that needs to be checked outside of our connector probing
    functions. While we're at it, we also remove a bunch of unneeded steps
    we perform when probing/enabling MST:
    
    * Enabling bits in MSTM_CTRL before calling drm_dp_mst_topology_mgr_set_mst().
      I don't think these ever actually did anything, since the nvif methods
      for enabling MST don't actually do anything DPCD related and merely
      indicate to nvkm that we've turned on MST.
    * Checking the MSTM_CTRL bit is intact when checking the state of an
      enabled MST topology in nv50_mstm_detect(). I just added this to be safe
      originally, but now that we try reading the DPCD when probing DP
      connectors it shouldn't be needed as that will abort our hotplug probing
      if the device was removed well before we start checking for MST..
    * All of the duplicate DPCD version checks.
    
    This leaves us with much nicer looking code, a much more sensible
    locking scheme, and an easy way of checking whether MST is enabled or
    not for handling DP HPD IRQs.
    
    v2:
    * Get rid of accidental newlines
    v4:
    * Fix uninitialized usage of mstm in nv50_mstm_detect() - thanks kernel
      bot!
    Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
    Reviewed-by: default avatarBen Skeggs <bskeggs@redhat.com>
    Link: https://patchwork.freedesktop.org/patch/msgid/20200826182456.322681-9-lyude@redhat.com
    a0922278
disp.c 8.65 KB