Re: [PATCH v2 6/6] drm/atomic: Make async plane update checks work as intended, v2.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 04.09.2017 13:48, Maarten Lankhorst wrote:
> By always keeping track of the last commit in plane_state, we know
> whether there is an active update on the plane or not. With that
> information we can reject the fast update, and force the slowpath
> to be used as was originally intended.
> 
> We cannot use plane_state->crtc->state here, because this only mentions
> the most recent commit for the crtc, but not the planes that were part
> of it. We specifically care about what the last commit involving this
> plane is, which can only be tracked with a pointer in the plane state.
> 
> Changes since v1:
> - Clean up the whole function here, instead of partially earlier.
> - Add mention in the commit message why we need commit in plane_state.
> - Swap plane->state in intel_legacy_cursor_update, instead of
>   reassigning all variables. With this commit We know that the cursor
>   is not part of any active commits so this hack can be removed.
> 
> Cc: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxx>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Reviewed-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxx>
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> #v1
> ---
>  drivers/gpu/drm/drm_atomic_helper.c  | 53 ++++++++++--------------------------
>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++-------
>  include/drm/drm_plane.h              |  5 ++--
>  3 files changed, 35 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c81d46927a74..a2cd432d8b2d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1388,35 +1388,31 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> -	struct drm_crtc_commit *commit;
> -	struct drm_plane *__plane, *plane = NULL;
> -	struct drm_plane_state *__plane_state, *plane_state = NULL;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *old_plane_state, *new_plane_state;
>  	const struct drm_plane_helper_funcs *funcs;
> -	int i, j, n_planes = 0;
> +	int i, n_planes = 0;
>  
>  	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  		if (drm_atomic_crtc_needs_modeset(crtc_state))
>  			return -EINVAL;
>  	}
>  
> -	for_each_new_plane_in_state(state, __plane, __plane_state, i) {
> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
>  		n_planes++;
> -		plane = __plane;
> -		plane_state = __plane_state;
> -	}
>  
>  	/* FIXME: we support only single plane updates for now */
> -	if (!plane || n_planes != 1)
> +	if (n_planes != 1)
>  		return -EINVAL;
>  
> -	if (!plane_state->crtc)
> +	if (!new_plane_state->crtc)
>  		return -EINVAL;
>  

Hello,

It looks like a NULL-checking of new_plane_state is missed here.

[   70.138947] [<c03dd670>] (drm_atomic_helper_async_check) from [<c03e0424>]
(drm_atomic_helper_check+0x4c/0x64)
[   70.138958] [<c03e0424>] (drm_atomic_helper_check) from [<c03fb6d4>]
(drm_atomic_check_only+0x2f4/0x56c)
[   70.138969] [<c03fb6d4>] (drm_atomic_check_only) from [<c03fb95c>]
(drm_atomic_commit+0x10/0x50)
[   70.138979] [<c03fb95c>] (drm_atomic_commit) from [<c03dde50>]
(drm_atomic_helper_update_plane+0xf0/0x100)
[   70.138991] [<c03dde50>] (drm_atomic_helper_update_plane) from [<c0403548>]
(__setplane_internal+0x178/0x214)
[   70.139003] [<c0403548>] (__setplane_internal) from [<c04036fc>]
(drm_mode_cursor_universal+0x118/0x1a8)
[   70.139014] [<c04036fc>] (drm_mode_cursor_universal) from [<c0403900>]
(drm_mode_cursor_common+0x174/0x1ec)
[   70.139026] [<c0403900>] (drm_mode_cursor_common) from [<c0403fa4>]
(drm_mode_cursor_ioctl+0x58/0x60)
[   70.139036] [<c0403fa4>] (drm_mode_cursor_ioctl) from [<c03eb0b4>]
(drm_ioctl+0x198/0x368)
[   70.139047] [<c03eb0b4>] (drm_ioctl) from [<c015fffc>] (do_vfs_ioctl+0x9c/0x8f0)
[   70.139058] [<c015fffc>] (do_vfs_ioctl) from [<c0160884>] (SyS_ioctl+0x34/0x5c)
[   70.139071] [<c0160884>] (SyS_ioctl) from [<c000f900>]
(ret_fast_syscall+0x0/0x48)

It's triggered by Tegra DRM driver on Xorg's startup. I also should notice that
currently Tegra DRM doesn't have a working HW cursor, I'm going to send out
Tegra cursor patches sometime soon.

This variant works for me:

	if (!new_plane_state || !new_plane_state->crtc)
		return -EINVAL;

>  	funcs = plane->helper_private;
>  	if (!funcs->atomic_async_update)
>  		return -EINVAL;
>  
> -	if (plane_state->fence)
> +	if (new_plane_state->fence)
>  		return -EINVAL;
>  
>  	/*
> @@ -1424,31 +1420,11 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>  	 * the plane.  This prevents our async update's changes from getting
>  	 * overridden by a previous synchronous update's state.
>  	 */
> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (plane->crtc != crtc)
> -			continue;
> -
> -		spin_lock(&crtc->commit_lock);
> -		commit = list_first_entry_or_null(&crtc->commit_list,
> -						  struct drm_crtc_commit,
> -						  commit_entry);
> -		if (!commit) {
> -			spin_unlock(&crtc->commit_lock);
> -			continue;
> -		}
> -		spin_unlock(&crtc->commit_lock);
> -
> -		if (!crtc->state->state)
> -			continue;
> +	if (old_plane_state->commit &&
> +	    !try_wait_for_completion(&old_plane_state->commit->hw_done))
> +		return -EBUSY;
>  
> -		for_each_plane_in_state(crtc->state->state, __plane,
> -					__plane_state, j) {
> -			if (__plane == plane)
> -				return -EINVAL;
> -		}
> -	}
> -
> -	return funcs->atomic_async_check(plane, plane_state);
> +	return funcs->atomic_async_check(plane, new_plane_state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_async_check);
>  
> @@ -1814,9 +1790,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  	}
>  
>  	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> -		/* commit tracked through new_crtc_state->commit, no need to do it explicitly */
> -		if (new_plane_state->crtc)
> -			continue;
> +		/*
> +		 * Unlike connectors, always track planes explicitly for
> +		 * async pageflip support.
> +		 */
>  
>  		/* Userspace is not allowed to get ahead of the previous
>  		 * commit with nonblocking ones. */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7abbc761a635..0add029d95f6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13064,6 +13064,14 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  		goto slow;
>  
>  	old_plane_state = plane->state;
> +	/*
> +	 * Don't do an async update if there is an outstanding commit modifying
> +	 * the plane.  This prevents our async update's changes from getting
> +	 * overridden by a previous synchronous update's state.
> +	 */
> +	if (old_plane_state->commit &&
> +	    !try_wait_for_completion(&old_plane_state->commit->hw_done))
> +		goto slow;
>  
>  	/*
>  	 * If any parameters change that may affect watermarks,
> @@ -13125,19 +13133,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  	}
>  
>  	old_fb = old_plane_state->fb;
> -	old_vma = to_intel_plane_state(old_plane_state)->vma;
>  
>  	i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
>  			  intel_plane->frontbuffer_bit);
>  
>  	/* Swap plane state */
> -	new_plane_state->fence = old_plane_state->fence;
> -	new_plane_state->commit = old_plane_state->commit;
> -	*to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
> -	new_plane_state->fence = NULL;
> -	new_plane_state->commit = NULL;
> -	new_plane_state->fb = old_fb;
> -	to_intel_plane_state(new_plane_state)->vma = NULL;
> +	plane->state = new_plane_state;
>  
>  	if (plane->state->visible) {
>  		trace_intel_update_plane(plane, to_intel_crtc(crtc));
> @@ -13149,13 +13150,19 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  		intel_plane->disable_plane(intel_plane, to_intel_crtc(crtc));
>  	}
>  
> -	if (old_vma)
> +	old_vma = to_intel_plane_state(old_plane_state)->vma;
> +	if (old_vma) {
> +		to_intel_plane_state(old_plane_state)->vma = NULL;
>  		intel_unpin_fb_vma(old_vma);
> +	}
>  
>  out_unlock:
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
>  out_free:
> -	intel_plane_destroy_state(plane, new_plane_state);
> +	if (ret)
> +		intel_plane_destroy_state(plane, new_plane_state);
> +	else
> +		intel_plane_destroy_state(plane, old_plane_state);
>  	return ret;
>  
>  slow:
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 7d96116fd4c4..feb9941d0cdb 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -124,9 +124,10 @@ struct drm_plane_state {
>  	bool visible;
>  
>  	/**
> -	 * @commit: Tracks the pending commit to prevent use-after-free conditions.
> +	 * @commit: Tracks the pending commit to prevent use-after-free conditions,
> +	 * and for async plane updates.
>  	 *
> -	 * Is only set when @crtc is NULL.
> +	 * May be NULL.
>  	 */
>  	struct drm_crtc_commit *commit;
>  
> 


-- 
Dmitry
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux