Skip to content
GitLab
Switch branch/tag
  1. 25 Sep, 2019 1 commit
    • Maarten Lankhorst's avatar
      drm/i915/dp: Fix dsc bpp calculations, v5. · ed06efb8
      Maarten Lankhorst authored 
      

There was a integer wraparound when mode_clock became too high,
and we didn't correct for the FEC overhead factor when dividing,
with the calculations breaking at HBR3.

As a result our calculated bpp was way too high, and the link width
limitation never came into effect.

Print out the resulting bpp calcululations as a sanity check, just
in case we ever have to debug it later on again.

We also used the wrong factor for FEC. While bspec mentions 2.4%,
all the calculations use 1/0.972261, and the same ratio should be
applied to data M/N as well, so use it there when FEC is enabled.

This fixes the FIFO underrun we are seeing with FEC enabled.

Changes since v2:
- Handle fec_enable in intel_link_compute_m_n, so only data M/N is adjusted. (Ville)
- Fix initial hardware readout for FEC. (Ville)
Changes since v3:
- Remove bogus fec_to_mode_clock. (Ville)
Changes since v4:
- Use the correct register for icl. (Ville)
- Split hw readout to a separate patch.
Signed-off-by: default avatarMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: d9218c8f ("drm/i915/dp: Add helpers for Compressed BPP and Slice Count for DSC")
Cc: <stable@vger.kernel.org> # v5.0+
Cc: Manasi Navare <manasi.d.navare@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190925082110.17439-1-maarten.lankhorst@linux.intel.com

Reviewed-by: default avatarVille Syrjälä <ville.syrjala@linux.intel.com>
      ed06efb8
  3. 05 Sep, 2019 2 commits
  5. 04 Sep, 2019 1 commit
  7. 02 Sep, 2019 1 commit
  9. 21 Aug, 2019 1 commit
    • Ville Syrjälä's avatar
      drm/i915: Do not create a new max_bpc prop for MST connectors · 1b9bd096
      Ville Syrjälä authored 
      We're not allowed to create new properties after device registration
so for MST connectors we need to either create the max_bpc property
earlier, or we reuse one we already have. Let's do the latter apporach
since the corresponding SST connector already has the prop and its
min/max are correct also for the MST connector.

The problem was highlighted by commit 4f5368b5 ("drm/kms:
Catch mode_object lifetime errors") which results in the following
spew:
[ 1330.878941] WARNING: CPU: 2 PID: 1554 at drivers/gpu/drm/drm_mode_object.c:45 __drm_mode_object_add+0xa0/0xb0 [drm]
...
[ 1330.879008] Call Trace:
[ 1330.879023]  drm_property_create+0xba/0x180 [drm]
[ 1330.879036]  drm_property_create_range+0x15/0x30 [drm]
[ 1330.879048]  drm_connector_attach_max_bpc_property+0x62/0x80 [drm]
[ 1330.879086]  intel_dp_add_mst_connector+0x11f/0x140 [i915]
[ 1330.879094]  drm_dp_add_port.isra.20+0x20b/0x440 [drm_kms_helper]
...

Cc: stable@vger.kernel.org
Cc: Lyude Paul <lyude@redhat.com>
Cc: sunpeng.li@amd.com
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Sean Paul <sean@poorly.run>
Fixes: 5ca0ef8a

 ("drm/i915: Add max_bpc property for DP MST")
Signed-off-by: default avatarVille Syrjälä <ville.syrjala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190820161657.9658-1-ville.syrjala@linux.intel.com

Reviewed-by: default avatarJosé Roberto de Souza <jose.souza@intel.com>
Reviewed-by: default avatarLyude Paul <lyude@redhat.com>
      1b9bd096
  11. 20 Aug, 2019 1 commit
  13. 16 Aug, 2019 1 commit
  15. 07 Aug, 2019 2 commits
  17. 17 Jun, 2019 1 commit
  19. 13 Jun, 2019 1 commit
  21. 03 May, 2019 1 commit
  23. 30 Apr, 2019 2 commits
  25. 11 Apr, 2019 1 commit
  27. 08 Apr, 2019 4 commits
  29. 27 Mar, 2019 4 commits
  31. 26 Mar, 2019 1 commit
  33. 05 Feb, 2019 1 commit
  35. 24 Jan, 2019 1 commit
  37. 17 Jan, 2019 1 commit
  39. 16 Jan, 2019 1 commit
    • Lyude Paul's avatar
      drm/i915: Pass down rc in intel_encoder->compute_config() · 204474a6
      Lyude Paul authored 
      

Something that I completely missed when implementing the new MST VCPI
atomic helpers is that with those helpers, there's technically a chance
of us having to grab additional modeset locks in ->compute_config() and
furthermore, that means we have the potential to hit a normal modeset
deadlock. However, because ->compute_config() only returns a bool this
means we can't return -EDEADLK when we need to drop locks and try again
which means we end up just failing the atomic check permanently. Whoops.

So, fix this by modifying ->compute_config() to pass down an actual
error code instead of a bool so that the atomic check can be restarted
on modeset deadlocks.

Thanks to Ville Syrjälä for pointing this out!

Changes since v1:
* Add some newlines
* Return only -EINVAL from hsw_crt_compute_config()
* Propogate return code from intel_dp_compute_dsc_params()
* Change all of the intel_dp_compute_link_config*() variants
* Don't miss if (hdmi_port_clock_valid()) branch in
  intel_hdmi_compute_config()

[Cherry-picked from drm-misc-next to drm-intel-next-queued to fix
 linux-next & drm-tip conflict, while waiting for proper propagation of
 the DP MST series that this commit fixes. In hindsight, a topic branch
 might have been a better approach for it.]
Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Fixes: eceae147 ("drm/dp_mst: Start tracking per-port VCPI allocations")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109320

Reviewed-by: default avatarVille Syrjälä <ville.syrjala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190115200800.3121-1-lyude@redhat.com
(cherry picked from commit 96550555

)
Signed-off-by: default avatarJani Nikula <jani.nikula@intel.com>
Acked-by: default avatarDaniel Vetter <daniel@ffwll.ch>
      204474a6
  41. 15 Jan, 2019 1 commit
    • Lyude Paul's avatar
      drm/i915: Pass down rc in intel_encoder->compute_config() · 96550555
      Lyude Paul authored 
      

Something that I completely missed when implementing the new MST VCPI
atomic helpers is that with those helpers, there's technically a chance
of us having to grab additional modeset locks in ->compute_config() and
furthermore, that means we have the potential to hit a normal modeset
deadlock. However, because ->compute_config() only returns a bool this
means we can't return -EDEADLK when we need to drop locks and try again
which means we end up just failing the atomic check permanently. Whoops.

So, fix this by modifying ->compute_config() to pass down an actual
error code instead of a bool so that the atomic check can be restarted
on modeset deadlocks.

Thanks to Ville Syrjälä for pointing this out!

Changes since v1:
* Add some newlines
* Return only -EINVAL from hsw_crt_compute_config()
* Propogate return code from intel_dp_compute_dsc_params()
* Change all of the intel_dp_compute_link_config*() variants
* Don't miss if (hdmi_port_clock_valid()) branch in
  intel_hdmi_compute_config()
Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Fixes: eceae147 ("drm/dp_mst: Start tracking per-port VCPI allocations")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109320

Reviewed-by: default avatarVille Syrjälä <ville.syrjala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190115200800.3121-1-lyude@redhat.com
      96550555
  43. 11 Jan, 2019 2 commits
    • Lyude Paul's avatar
      drm/dp_mst: Start tracking per-port VCPI allocations · eceae147
      Lyude Paul authored 
      

There has been a TODO waiting for quite a long time in
drm_dp_mst_topology.c:

	/* We cannot rely on port->vcpi.num_slots to update
	 * topology_state->avail_slots as the port may not exist if the parent
	 * branch device was unplugged. This should be fixed by tracking
	 * per-port slot allocation in drm_dp_mst_topology_state instead of
	 * depending on the caller to tell us how many slots to release.
	 */

That's not the only reason we should fix this: forcing the driver to
track the VCPI allocations throughout a state's atomic check is
error prone, because it means that extra care has to be taken with the
order that drm_dp_atomic_find_vcpi_slots() and
drm_dp_atomic_release_vcpi_slots() are called in in order to ensure
idempotency. Currently the only driver actually using these helpers,
i915, doesn't even do this correctly: multiple ->best_encoder() checks
with i915's current implementation would not be idempotent and would
over-allocate VCPI slots, something I learned trying to implement
fallback retraining in MST.

So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots()
and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for
each port. This allows us to ensure idempotency without having to rely
on the driver as much. Additionally: the driver doesn't need to do any
kind of VCPI slot tracking anymore if it doesn't need it for it's own
internal state.

Additionally; this adds a new drm_dp_mst_atomic_check() helper which
must be used by atomic drivers to perform validity checks for the new
VCPI allocations incurred by a state.

Also: update the documentation and make it more obvious that these
/must/ be called by /all/ atomic drivers supporting MST.

Changes since v9:
* Add some missing changes that were requested by danvet that I forgot
  about after I redid all of the kref stuff:
  * Remove unnecessary state changes in intel_dp_mst_atomic_check
  * Cleanup atomic check logic for VCPI allocations - all we need to check in
    compute_config is whether or not this state disables a CRTC, then free
    VCPI based off that

Changes since v8:
 * Fix compile errors, whoops!

Changes since v7:
 - Don't check for mixed stale/valid VCPI allocations, just rely on
 connector registration to stop such erroneous modesets

Changes since v6:
 - Keep a kref to all of the ports we have allocations on. This required
   a good bit of changing to when we call drm_dp_find_vcpi_slots(),
   mainly that we need to ensure that we only redo VCPI allocations on
   actual mode or CRTC changes, not crtc_state->active changes.
   Additionally, we no longer take the registration of the DRM connector
   for each port into account because so long as we have a kref to the
   port in the new or previous atomic state, the connector will stay
   registered.
 - Use the small changes to drm_dp_put_port() to add even more error
   checking to make misusage of the helpers more obvious. I added this
   after having to chase down various use-after-free conditions that
   started popping up from the new helpers so no one else has to
   troubleshoot that.
 - Move some accidental DRM_DEBUG_KMS() calls to DRM_DEBUG_ATOMIC()
 - Update documentation again, note that find/release() should both not be
   called on the same port in a single atomic check phase (but multiple
   calls to one or the other is OK)

Changes since v4:
 - Don't skip the atomic checks for VCPI allocations if no new VCPI
   allocations happen in a state. This makes the next change I'm about
   to list here a lot easier to implement.
 - Don't ignore VCPI allocations on destroyed ports, instead ensure that
   when ports are destroyed and still have VCPI allocations in the
   topology state, the only state changes allowed are releasing said
   ports' VCPI. This prevents a state with a mix of VCPI allocations
   from destroyed ports, and allocations from valid ports.

Changes since v3:
 - Don't release VCPI allocations in the topology state immediately in
   drm_dp_atomic_release_vcpi_slots(), instead mark them as 0 and skip
   over them in drm_dp_mst_duplicate_state(). This makes it so
   drm_dp_atomic_release_vcpi_slots() is still idempotent while also
   throwing warnings if the driver messes up it's book keeping and tries
   to release VCPI slots on a port that doesn't have any pre-existing
   VCPI allocation - danvet
 - Change mst_state/state in some debugging messages to "mst state"

Changes since v2:
 - Use kmemdup() for duplicating MST state - danvet
 - Move port validation out of duplicate state callback - danvet
 - Handle looping through MST topology states in
   drm_dp_mst_atomic_check() so the driver doesn't have to do it
 - Fix documentation in drm_dp_atomic_find_vcpi_slots()
 - Move the atomic check for each individual topology state into it's
   own function, reduces indenting
 - Don't consider "stale" MST ports when calculating the bandwidth
   requirements. This is needed because originally we relied on the
   state duplication functions to prune any stale ports from the new
   state, which would prevent us from incorrectly considering their
   bandwidth requirements alongside legitimate new payloads.
 - Add function references in drm_dp_atomic_release_vcpi_slots() - danvet
 - Annotate atomic VCPI and atomic check functions with __must_check
   - danvet

Changes since v1:
 - Don't use the now-removed ->atomic_check() for private objects hook,
   just give drivers a function to call themselves
Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
Reviewed-by: default avatarDaniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@redhat.com>
Cc: Jerry Zuo <Jerry.Zuo@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Juston Li <juston.li@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-19-lyude@redhat.com
      eceae147
    • Lyude Paul's avatar
      drm/i915: Keep malloc references to MST ports · 79a47cd3
      Lyude Paul authored 
      

So that the ports stay around until we've destroyed the connectors, in
order to ensure that we don't pass an invalid pointer to any MST helpers
once we introduce the new MST VCPI helpers.

Changes since v1:
* Move drm_dp_mst_get_port_malloc() to where we assign
  intel_connector->port - danvet
Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
Reviewed-by: default avatarDaniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@redhat.com>
Cc: Jerry Zuo <Jerry.Zuo@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Juston Li <juston.li@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-11-lyude@redhat.com
      79a47cd3
  45. 09 Jan, 2019 1 commit
  47. 10 Dec, 2018 1 commit
  49. 12 Nov, 2018 1 commit
  51. 09 Nov, 2018 2 commits
  53. 07 Nov, 2018 1 commit
  55. 19 Oct, 2018 1 commit
    • Lyude Paul's avatar
      drm/atomic_helper: Stop modesets on unregistered connectors harder · de9f8eea
      Lyude Paul authored 
      Unfortunately, it appears our fix in:
commit b5d29843 ("drm/atomic_helper: Allow DPMS On<->Off changes
for unregistered connectors")

Which attempted to work around the problems introduced by:
commit 4d802739 ("drm/atomic_helper: Disallow new modesets on
unregistered connectors")

Is still not the right solution, as modesets can still be triggered
outside of drm_atomic_set_crtc_for_connector().

So in order to fix this, while still being careful that we don't break
modesets that a driver may perform before being registered with
userspace, we replace connector->registered with a tristate member,
connector->registration_state. This allows us to keep track of whether
or not a connector is still initializing and hasn't been exposed to
userspace, is currently registered and exposed to userspace, or has been
legitimately removed from the system after having once been present.

Using this info, we can prevent userspace from performing new modesets
on unregistered connectors while still allowing the driver to perform
modesets on unregistered connectors before the driver has finished being
registered.

Changes since v1:
- Fix WARN_ON() in drm_connector_cleanup() that CI caught with this
  patchset in igt@drv_module_reload@basic-reload-inject and
  igt@drv_module_reload@basic-reload by checking if the connector is
  registered instead of unregistered, as calling drm_connector_cleanup()
  on a connector that hasn't been registered with userspace yet should
  stay valid.
- Remove unregistered_connector_check(), and just go back to what we
  were doing before in commit 4d802739 ("drm/atomic_helper: Disallow
  new modesets on unregistered connectors") except replacing
  READ_ONCE(connector->registered) with drm_connector_is_unregistered().
  This gets rid of the behavior of allowing DPMS On<->Off, but that should
  be fine as it's more consistent with the UAPI we had before - danvet
- s/drm_connector_unregistered/drm_connector_is_unregistered/ - danvet
- Update documentation, fix some typos.

Fixes: b5d29843

 ("drm/atomic_helper: Allow DPMS On<->Off changes for unregistered connectors")
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: stable@vger.kernel.org
Cc: David Airlie <airlied@linux.ie>
Signed-off-by: default avatarLyude Paul <lyude@redhat.com>
Reviewed-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20181016203946.9601-1-lyude@redhat.com
(cherry picked from commit 39b50c60)
Fixes: e9655095 ("drm/atomic_helper: Disallow new modesets on unregistered connectors")
Fixes: 34ca26a9

 ("drm/atomic_helper: Allow DPMS On<->Off changes for unregistered connectors")
Cc: stable@vger.kernel.org
Signed-off-by: default avatarJoonas Lahtinen <joonas.lahtinen@linux.intel.com>
      de9f8eea
  57. 18 Oct, 2018 1 commit