Re: [PATCH v2 6/7] drm/i915: only timeout on fence waits for display reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling a sleeping function (dma_fence_wait_timeout) from an atomic context path (intel_atomic_commit_tail).
--

commit 70671f62d3480e316b53a7ec34d9d047347fa4af
Author: Jani Nikula <jani.nikula@intel.com>

drm/i915: only timeout on fence waits for display reset

This commit restricts the timeout on fence waits to older platforms where
a GPU reset clobbers the display, or when running forced display reset
tests. For other platforms, it switches the timeout to MAX_SCHEDULE_TIMEOUT.

> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 64fa582a64102..42222b930943d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c

[ ... ]

> @@ -7228,15 +7229,20 @@ static void intel_atomic_commit_fence_wait(struct intel_atomic_state *state)
>  	struct intel_display *display = to_intel_display(state);
>  	struct drm_plane *plane;
>  	struct drm_plane_state *new_plane_state;
> -	long ret;
> +	long timeout, ret;
>  	int i;
>  
> +	if (intel_display_reset_needed_after_gpu_reset(display) ||
> +	    intel_display_reset_test(display))
> +		timeout = i915_fence_timeout();
> +	else
> +		timeout = MAX_SCHEDULE_TIMEOUT;
> +
>  	for_each_new_plane_in_state(&state->base, plane, new_plane_state, i) {
>  		if (!new_plane_state->fence)
>  			continue;
>  
> -		ret = dma_fence_wait_timeout(new_plane_state->fence, false,
> -					     i915_fence_timeout());
> +		ret = dma_fence_wait_timeout(new_plane_state->fence, false, timeout);

[Severity: High]
This isn't a bug introduced by this patch, but does this result in calling a
sleeping function from an atomic context?

The DRM subsystem guidelines explicitly define intel_atomic_commit_tail() and
its variants as atomic context paths where sleeping is forbidden. This code
is reached during an atomic display commit via:

intel_atomic_commit_tail()
   intel_atomic_commit_fence_wait()
      dma_fence_wait_timeout() -> might_sleep()

While this patch only modifies the timeout value (which doesn't introduce the
bug), the underlying call to a sleeping function from a forbidden atomic
context remains. Can this cause kernel warnings or potential deadlocks during
userspace modeset or pageflip requests?

>  		if (!ret)
>  			ret = -ETIMEDOUT;
>  		if (ret < 0) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787833879.git.jani.nikula@intel.com?part=6