If the output on a DP-alt link with its sink disconnected is kept
enabled for too long (about 20 sec), then some IOM/TCSS firmware timeout
will cause havoc on the PCI bus, at least for other GFX devices on it
which will stop powering up. Since user space is not guaranteed to do a
disabling modeset in time, switch such disconnected but active links to
TBT mode - which is without such shortcomings - with a 2 second delay.
If the above condition is detected already during the driver load/system
resume sanitization step disable the output instead, as at that point no
user space or kernel client depends on a consistent output state yet and
because subsequent atomic modeset on such connectors - without the
actual sink capabilities available - can fail.
An active/disconnected port as above will also block the HPD status of
other active/disconnected ports to get updated (stuck in the connected
state), until the former port is disabled, its PHY is disconnected and
a ~10 ms delay has elapsed. This means the link state for all TypeC
ports/CRTCs must be rechecked after a CRTC is disabled due to the above
reason. For this disconnect the PHY synchronously after the CRTC/port is
disabled and recheck all CRTCs for the above condition whenever such a
port is disabled.
To account for a race condition during driver loading where the sink is
disconnected after the above sanitization step and before the HPD
interrupts get enabled, do an explicit check/link reset if needed from
the encoder's late_register hook, which is called after the HPD
interrupts are enabled already.
v2:
- Handle an active/disconnected port blocking the HPD state update of
another active/disconnected port.
- Cancel the delayed work resetting the link also from the encoder
enable/suspend/shutdown hooks.
- Rebase on the earlier intel_modeset_lock_ctx_retry() addition,
fixing here the missed atomic state reset in case of a retry.
- Fix handling of an error return from intel_atomic_get_crtc_state().
- Recheck if the port needs to be reset after all the atomic state
is locked and async commits are waited on.
v3:
- Add intel_crtc_needs_link_reset(), instead of open-coding it,
keep intel_crtc_has_encoders(). (Ville)
- Fix state dumping and use a bitmask to track disabled CRTCs in
intel_sanitize_all_crtcs(). (Ville)
- Set internal in intel_atomic_state right after allocating it.
(Ville)
- Recheck all CRTCs (not yet force-disabled) after a CRTC is
force-disabled for any reason (not only due to a link state)
in intel_sanitize_all_crtcs().
- Reduce delay after CRTC disabling to 20ms, and use the simpler
msleep().
- Clarify code comment about HPD behaviour in
intel_sanitize_all_crtcs().
- Move all the TC link reset logic to intel_tc.c .
- Cancel the link reset work synchronously during system suspend,
driver unload and shutdown.
v4:
- Rebased on previous patch, which allows calling the TC port
suspend/cleanup handlers without modeset locks held; remove the
display driver suspended assert from the link reset work
accordingly.
v5: (Ville)
- Remove reset work canceling from intel_ddi_pre_pll_enable().
- Track a crtc vs. pipe mask in intel_sanitize_all_crtcs().
- Add reset_link_commit() to clarify the
intel_modeset_lock_ctx_retry loop.
Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5860
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230512195513.2699-2-imre.deak@intel.com
Call the TypeC port flush_work and cleanup handlers without the modeset
locks held. These don't require the locks, as the work takes - as it
should be able to at any point in time - any locks it needs and by the
time cleanup is called and after cleanup returns the encoder is not in
use.
This is required by the next patch canceling a TypeC port work
synchronously during encoder suspend and shutdown, where the work can
take modeset locks as well, hence the canceling must be done without
holding the locks.
I also considered moving the modeset locking down to each encoder
suspend()/shutdown() hook instead, however locking the full modeset
state for each encoder separately would be odd, and the bigger change -
affecting all encoders - is beyond the scope of this patchset.
v2:
- Add a TODO: comment to remove modeset locks if no encoder depends
on this. (Ville)
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230512195513.2699-1-imre.deak@intel.com
This patch simplifying the handling of modeset locks and atomic state
for an atomic commit is based on
https://lore.kernel.org/all/20210715184954.7794-2-ville.syrjala@linux.intel.com/
adding the helper to i915. I find this approach preferrable than
open-coding the corresponding steps (fixed for me an atomic
state reset during a DEADLK retry, which I missed in the open-coded
version) and also better than the existing
DRM_MODESET_LOCK_ALL_BEGIN/END macros for the reasons described in the
above original patchset.
This change takes the helper into use only for atomic commits during DDI
hotplug handling, as a preparation for a follow-up patch adding a
similar commit started from the same spot. Other places doing a
driver-internal atomic commit is to be converted by a follow-up
patchset.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230510103131.1618266-13-imre.deak@intel.com
During HW readout/sanitization CRTCs can be disabled only if they don't
have an attached encoder (and so the encoder disable hooks don't need to
be called). An upcoming patch will need to disable CRTCs also with an
attached encoder, so add support for this.
For bigjoiner configs the encoder disabling hooks require the slave CRTC
states, so add these too to the atomic state. Since the connector atomic
state is already up-to-date when the CRTC is disabled the connector
state needs to be updated (reset) after the CRTC is disabled, make this
so. Follow the proper order of disabling first all bigjoiner slaves,
then any port synced CRTC slaves followed by the CRTC originally
requested to be disabled.
v2:
- Fix calculating the bigjoiner_masters mask in a port sync config,
(Ville)
- Keep _noatomic suffix in intel_crtc_disable_noatomic(). (Ville)
- Rebase on full CRTC state reset in this patchset, not requiring
resetting the bigjoiner state separately and (instead) resetting
the full atomic CRTC and related global state after all linked
pipes got disabled.
- Disable portsync slaves before a portsync master.
- Disable a portsync master if a linked portsync slave is disabled.
v3: (Ville)
- Use s/u32/u8 for transcoder and pipe masks.
- Use is_power_of_2() instead of hweight()==1.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230510103131.1618266-8-imre.deak@intel.com
During HW state readout/sanitization an up-to-date connector atomic
state will be required by a follow-up patch, which can disable CRTCs
with an encoder (and calling the correct encoder hooks happens via the
connector atomic state encoder pointer). So update the connector state
already before the CRTC sanitize/disable step. For now this doesn't make
a difference, since intel_modeset_update_connector_atomic_state() will
update/enable the atomic state only for connectors that have an enabled
encoder/CRTC. Such CRTCs/encoders will not be affected by
intel_sanitize_crtc().
v2: Add comment about why the connector state needs to be up-to-date.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230510103131.1618266-5-imre.deak@intel.com
Make sure that the CRTC state is reset correctly, as expected after
disabling the CRTC.
In particular this change will:
- Zero all the CSC blob pointers after intel_crtc_free_hw_state()
has freed them.
- Zero the shared DPLL and port PLL pointers and clear the
corresponding CRTC reference flag in the PLL state.
- Reset all the transcoder and pipe fields.
v2:
- Reset fully the CRTC state. (Ville)
- Clear pipe active flags in the DPLL state.
v3:
- Clear only the CRTC reference flag and add a helper for this.
(Ville)
v4:
- Rebased on previous patch, adding
intel_unreference_shared_dpll_crtc() separately. (Ville)
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230510103131.1618266-4-imre.deak@intel.com
For a bigjoiner configuration display->crtc_disable() will be called
first for the slave CRTCs and then for the master CRTC. However slave
CRTCs will be actually disabled only after the master CRTC is disabled
(from the encoder disable hooks called with the master CRTC state).
Hence the slave PIPEDMCs can be disabled only after the master CRTC is
disabled, make this so.
intel_encoders_post_pll_disable() must be called only for the master
CRTC, as for the other two encoder disable hooks. While at it fix this
up as well. This didn't cause a problem, since
intel_encoders_post_pll_disable() will call the corresponding hook only
for an encoder/connector connected to the given CRTC, however slave
CRTCs will have no associated encoder/connector.
Fixes: 3af2ff0840 ("drm/i915: Enable a PIPEDMC whenever its corresponding pipe is enabled")
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230510103131.1618266-2-imre.deak@intel.com
Since topology state is being added to drm_atomic_state now all
drm_modeset_lock required are being taken from core. This raises
an issue when we try to loop over connector and assign vcpi id to
our streams as we did not have atomic state to derive acquire_ctx
from. We fill in stream info if dpmst encoder is found before
enabling hdcp. intel_hdcp_required_stream will be broken which
will only set the content type.
--v2
-move prepare streams to beginning of intel_hdcp_enable to avoid
checking of mst encoder twice [Ankit]
--v3
-break intel_required_content_stream to two part and set the stream_id
at the beginning [Ankit]
--v4
-change return types for intel_hdcp_prepare_stream and
intel_hdcp_required content_stream [Ankit]
-rename intel_hdcp_set_content_stream to
intel_hdcp_set_stream [Ankit]
-place intel_hdcp_set_streams above caller [Ankit]
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230515103225.688830-4-suraj.kandpal@intel.com
The big switch+if statement mess in map_aux_ch() is
illegible. Split up into cleaner per-platform arrays
like we already have for the gmbus pins.
We use enum aux_ch as the index and the VBT thing as
the value. Slightly non-intuitive perhaps but if we
did it the other way around we'd have problems with
AUX_CH_A being zero, and thus any non-populated
element would look like AUX_CH_A.
v2: flip the index vs. value around
TODO: Didn't bother with the platform variants beyond the
ones that really need remapping, which means if the
VBT is bogus we end up with a nonexistent aux ch.
Might be nice to check this a bit better.
Yet another bitmask in device info?
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230509160206.25971-1-ville.syrjala@linux.intel.com
Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
