Invoke drm_plane_helper_funcs.end_fb_access before
drm_atomic_helper_commit_hw_done(). The latter function hands over
ownership of the plane state to the following commit, which might
free it. Releasing resources in end_fb_access then operates on undefined
state. This bug has been observed with non-blocking commits when they
are being queued up quickly.
Here is an example stack trace from the bug report. The plane state has
been free'd already, so the pages for drm_gem_fb_vunmap() are gone.
Unable to handle kernel paging request at virtual address 0000000100000049
[...]
drm_gem_fb_vunmap+0x18/0x74
drm_gem_end_shadow_fb_access+0x1c/0x2c
drm_atomic_helper_cleanup_planes+0x58/0xd8
drm_atomic_helper_commit_tail+0x90/0xa0
commit_tail+0x15c/0x188
commit_work+0x14/0x20
Fix this by running end_fb_access immediately after updating all planes
in drm_atomic_helper_commit_planes(). The existing clean-up helper
drm_atomic_helper_cleanup_planes() now only handles cleanup_fb.
For aborted commits, roll back from drm_atomic_helper_prepare_planes()
in the new helper drm_atomic_helper_unprepare_planes(). This case is
different from regular cleanup, as we have to release the new state;
regular cleanup releases the old state. The new helper also invokes
cleanup_fb for all planes.
The changes mostly involve DRM's atomic helpers. Only two drivers, i915
and nouveau, implement their own commit function. Update them to invoke
drm_atomic_helper_unprepare_planes(). Drivers with custom commit_tail
function do not require changes.
v4:
* fix documentation (kernel test robot)
v3:
* add drm_atomic_helper_unprepare_planes() for rolling back
* use correct state for end_fb_access
v2:
* fix test in drm_atomic_helper_cleanup_planes()
Reported-by: Alyssa Ross <hi@alyssa.is>
Closes: https://lore.kernel.org/dri-devel/87leazm0ya.fsf@alyssa.is/
Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Fixes: 94d879eaf7 ("drm/atomic-helper: Add {begin,end}_fb_access to plane helpers")
Tested-by: Alyssa Ross <hi@alyssa.is>
Reviewed-by: Alyssa Ross <hi@alyssa.is>
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: <stable@vger.kernel.org> # v6.2+
Link: https://patchwork.freedesktop.org/patch/msgid/20231204083247.22006-1-tzimmermann@suse.de
{planes,vrr}_{enabling,disabling}() are supposed to indicate
whether the specific hardware feature is supposed to be enabling
or disabling. That can only makes sense if the pipe is active
overall. So check for that before we go poking at the hardware.
I think we're semi-safe currently on due to:
- intel_pre_plane_update() doesn't get called when the pipe
was not-active prior to the commit, but this is actually a bug.
This saves vrr_disabling(), and vrr_enabling() is called from
deeper down where we have already checked hw.active.
- active_planes mirrors the crtc's hw.active
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20231121054324.9988-2-ville.syrjala@linux.intel.com
(cherry picked from commit bc53c4d56e)
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Use the >= and < operators for the DISPLAY_VER checks everywhere.
This is what most of the code does, but especially recently random
pieces of code have started doing this differently for no good reason.
Conversion done with the following cocci:
@find@
expression i915;
constant ver;
@@
(
DISPLAY_VER(i915) <= ver
|
DISPLAY_VER(i915) > ver
)
@script:python inc@
old_ver << find.ver;
new_ver;
@@
coccinelle.new_ver = str(int(old_ver) + 1)
@@
expression find.i915;
constant find.ver;
identifier inc.new_ver;
@@
(
- DISPLAY_VER(i915) <= ver
+ DISPLAY_VER(i915) < new_ver
|
- DISPLAY_VER(i915) > ver
+ DISPLAY_VER(i915) >= new_ver
)
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20231127145028.4899-4-ville.syrjala@linux.intel.com
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
{planes,vrr}_{enabling,disabling}() are supposed to indicate
whether the specific hardware feature is supposed to be enabling
or disabling. That can only makes sense if the pipe is active
overall. So check for that before we go poking at the hardware.
I think we're semi-safe currently on due to:
- intel_pre_plane_update() doesn't get called when the pipe
was not-active prior to the commit, but this is actually a bug.
This saves vrr_disabling(), and vrr_enabling() is called from
deeper down where we have already checked hw.active.
- active_planes mirrors the crtc's hw.active
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20231121054324.9988-2-ville.syrjala@linux.intel.com
The link M/N ratio is the data rate / link symbol clock rate, fix things
up accordingly. On DP 1.4 this ratio was correct as the link symbol clock
rate in that case matched the link data rate (in bytes/sec units, the
symbol size being 8 bits), however it wasn't correct for UHBR rates
where the symbol size is 32 bits.
Kudos to Arun noticing in Bspec the incorrect use of link data rate in
the ratio's N value.
Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20231116131841.1588781-7-imre.deak@intel.com
DSC parameter bits_per_pixel is stored in U6.4 format.
The 4 bits represent the fractional part of the bpp.
Currently we use compressed_bpp member of dsc structure to store
only the integral part of the bits_per_pixel.
To store the full bits_per_pixel along with the fractional part,
compressed_bpp is changed to store bpp in U6.4 formats. Intergral
part is retrieved by simply right shifting the member compressed_bpp by 4.
v2:
-Use to_bpp_int, to_bpp_frac_dec, to_bpp_x16 helpers while dealing
with compressed bpp. (Suraj)
-Fix comment styling. (Suraj)
v3:
-Add separate file for 6.4 fixed point helper(Jani, Nikula)
-Add comment for magic values(Suraj)
v4:
-Fix checkpatch warnings caused by renaming(Suraj)
v5:
-Rebase.
-Use existing helpers for conversion of bpp_int to bpp_x16
and vice versa.
Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
Link: https://patchwork.freedesktop.org/patch/msgid/20231110101020.4067342-3-ankit.k.nautiyal@intel.com
At the moment modesetting a stream CRTC will fail if the stream's BW
along with the current BW of all the other streams on the same MST link
is above the total BW of the MST link. Make the BW sharing more dynamic
by trying to reduce the link bpp of one or more streams on the MST link
in this case.
When selecting a stream to reduce the BW for, take into account which
link segment in the MST topology ran out of BW and which streams go
through this link segment. For instance with A,B,C streams in the same
MST topology A and B may share the BW of a link segment downstream of a
branch device, stream C not downstream of the branch device, hence not
affecting this BW. If this link segment's BW runs out one or both of
stream A/B's BW will be reduced until their total BW is within limits.
While reducing the link bpp for a given stream DSC may need to be
enabled for it, which requires FEC on the whole MST link. Check for this
condition and recompute the state for all streams taking the FEC
overhead into account (on 8b/10b links).
v2:
- Rebase on s/min_bpp_pipes/min_bpp_reached_pipes/ change.
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20231030155843.2251023-29-imre.deak@intel.com
Enabling / disabling DSC decompression in the branch device downstream
of the source may reset the whole branch device. To avoid this while the
streams are still active, force a modeset on all CRTC/ports connected to
this branch device.
v2:
- Check the CRTC state for each connector in the topology, instead of
the CRTC being checked for a modeset requirement. (Ville)
- Add DocBook for the new function.
v3:
- Rebased on a change not to use
intel_modeset_pipes_in_mask_early().
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> (v1)
Signed-off-by: Imre Deak <imre.deak@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20231107001505.3370108-8-imre.deak@intel.com
Factor out a helper to clear the pipe update flags, used by a follow-up
patch to modeset an MST topology.
v2:
- Move the intel_crtc_needs_modeset() check to the callers. (Ville)
v3 (Ville):
- Rename clear_pipe_update_flags_on_modeset_crtc() to
intel_crtc_flag_modeset().
- Also set crtc_state->uapi.mode_changed in the function.
- Leave out the unrelated change to use
intel_modeset_pipes_in_mask_early().
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> (v1)
Signed-off-by: Imre Deak <imre.deak@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20231107001505.3370108-7-imre.deak@intel.com
We are preparing for Xe driver. Xe driver doesn't have i915_sw_fence
implementation. Lets drop i915_sw_fence usage from display code and
use dma_fence interfaces directly.
For this purpose stack dma fences from related objects into new plane
state. Drm_gem_plane_helper_prepare_fb can be used for fences in new
fb. Separate local implementation is used for Stacking fences from old fb
into new plane state. Then wait for these stacked fences during atomic
commit. There is no be need for separate GPU reset handling in
intel_atomic_commit_fence_wait as the fences are signaled when GPU hang is
detected and GPU is being reset.
v4:
- Drop to_new_plane_state suffix from add_dma_resv_fences
- Use dma_resv_usage_rw(false) (DMA_RESV_USAGE_WRITE)
v3:
- Rename add_fences and it's parameters
- Remove signaled check
- Remove waiting old_plane_state fences
v2:
- Add fences from old fb into new_plane_state->uapi.fence rather than
into old_plane_state->uapi.fence
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20231031084557.1181630-1-jouni.hogander@intel.com
With MTL adding PICA between the port and the real phy, the path
add for DG2 stopped being followed and newer platforms are simply using
the older path for TC phys. LNL is no different than MTL in this aspect,
so just add it to the mess. In future the phy and port designation and
deciding if it's TC should better be cleaned up.
To make it just a bit better, also change intel_phy_is_snps() to show
this is DG2-only.
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20231026184045.1015655-3-lucas.demarchi@intel.com
Recompute the state of all CRTCs on an FDI link during a modeset that
may be affected by the modeset of other CRTCs on the same link. This
ensures that each CRTC on the link maximizes its BW use (after another
CRTC is disabled).
In practice this means recomputing pipe B's config on IVB if pipe C gets
disabled.
v2:
- Add the change recomputing affected CRTC states in a separate patch.
(Ville)
v3: (Ville)
- Constify old and new crtc states.
- Check for fused off pipe C.
- Fix new vs. old crtc state mixup.
- Drop check for pipe C's enabled state.
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/20230921195159.2646027-12-imre.deak@intel.com
At the moment modesetting pipe C on IVB will fail if pipe B uses 4 FDI
lanes. Make the BW sharing more dynamic by trying to reduce pipe B's
link bpp in this case, until pipe B uses only up to 2 FDI lanes.
For this instead of the encoder compute config retry loop - which
reduced link bpp only for the encoder's pipe - reduce the maximum link
bpp for pipe B/C as required after all CRTC states are computed and
recompute the CRTC states with the new bpp limit.
Atm, all FDI encoder's compute config function returns an error if a BW
constrain prevents increasing the pipe bpp value. The corresponding
crtc_state->bw_constrained check can be replaced with checking
crtc_state->max_link_bpp_x16, add TODO comments for this. SDVO is an
exception where this case is only handled in the outer config retry
loop, failing the modeset with a WARN, add a FIXME comment to handle
this in the encoder code similarly to other encoders.
v2:
- Don't assume that a CRTC is already in the atomic state, while
reducing its link bpp.
- Add DocBook description to intel_fdi_atomic_check_link().
v3:
- Enable BW management for FDI links in a separate patch. (Ville)
v4: (Ville)
- Fail the SDVO encoder config computation if it doesn't support the
link bpp limit.
- Add TODO: comments about checking link_bpp_x16 instead of
bw_constrained.
v5:
- Replace link bpp limit check with a FIXME: comment in
intel_sdvo_compute_config(). (Ville)
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
[Amended commit message wrt. changes in v5]
Signed-off-by: Imre Deak <imre.deak@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230921195159.2646027-11-imre.deak@intel.com
At the moment a modeset fails if the config computation of a pipe can't
fit its required BW to the available link BW even though the limitation
may be resolved by reducing the BW requirement of other pipes.
To improve the above this patch adds helper functions checking the
overall BW limits after all CRTC states have been computed. If the check
fails the maximum link bpp for a selected pipe will be reduced and all
the CRTC states will be recomputed until either the overall BW limit
check passes, or further bpp reduction is not possible (because all
pipes/encoders sharing the link BW reached their minimum link bpp).
Atm, the MST encoder allocates twice the required BW for YUV420 format
streams. A follow-up patchset will fix that, add a code comment about
this.
This change prepares for upcoming patches enabling the above BW
management on FDI and MST links.
v2:
- Rename intel_crtc_state::max_link_bpp to max_link_bpp_x16 and
intel_link_bw_limits::max_bpp to max_bpp_x16. (Jani)
v3:
- Add the helper functions in a separate patch. (Ville)
- Add the functions to intel_link_bw.c instead of intel_atomic.c (Ville)
- Return -ENOSPC instead of -EINVAL to userspace in case of a link BW
limit failure.
v4:
- Make intel_atomic_check_config() static.
v5: (Ville)
- Rename intel_link_bw_limits::min_bpp_pipes to min_bpp_reached_pipes
and intel_link_bw_reset_pipe_limit_to_min() to
intel_link_bw_set_min_bpp_for_pipe().
- Rename pipe_bpp to link_bpp in intel_link_bw_reduce_bpp().
- Add FIXME: comment about MST encoder's YUV420 BW allocation and
tracking the link bpp limit accordingly.
v6:
- Move intel_link_bw_compute_pipe_bpp() to intel_fdi.c (Ville)
- WARN_ON(BIT(pipe) & min_bpp_reached_pipes) in
intel_link_bw_set_bpp_limit_for_pipe(). (Ville)
- Rename intel_link_bw_set_min_bpp_for_pipe() to
intel_link_bw_set_bpp_limit_for_pipe() and
intel_link_bw_limits::min_bpp_reached_pipes to
bpp_limit_reached_pipes. (Ville)
- Remove unused header includes.
Cc: Jani Nikula <jani.nikula@linux.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/20230921195159.2646027-10-imre.deak@intel.com
Add intel_modeset_pipes_in_mask_early() to modeset a provided set of
pipes, used in a follow-up patch.
As opposed to intel_modeset_all_pipes() which modesets only the active
pipes - others don't requiring programming the HW - modeset all enabled
pipes in intel_modeset_pipes_in_mask_early() which may need to recompute
their state even if they are not active (that is in the DPMS off state).
While at it add DocBook descriptions for the two exported functions.
v2:
- Add a flag controlling if active planes are force updated as well.
- Add DockBook descriptions.
v3:
- For clarity use _early/_late suffixes for the exported functions
instead of the update_active_planes parameter. (Ville)
v4:
- In intel_modeset_pipes_in_mask_early() update only the crtc
flags relevant to the early phase. (Ville)
- Rename intel_modeset_all_pipes() in a separate patch.
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/20230921195159.2646027-7-imre.deak@intel.com
