Re: [PATCH] drm/i915: cleanup_plane_fb: also drop reference to current state wait_req

[Daniel Vetter]

On Sun, Jul 31, 2016 at 12:54:51AM -0700, Keith Packard wrote:
> There are two paths into intel_cleanup_plane_fb, the normal completion
> path and the failure path.
> 
> In the failure case, intel_cleanup_plane_fb is called before
> drm_atomic_helper_swap_state, so any wait_req reference made in
> intel_prepare_plane_fb will be in old_intel_state->wait_req.
> 
> In the normal completion path, drm_atomic_helper_swap_state has
> already been called, so the plane state holding the just-used wait_req
> will not be in old_intel_state->wait_req, rather it will be in the
> state associated with the plane itself.
> 
> Clearing this reference ensures that the wait_req will be freed as
> soon as it the related mode setting operation is complete, rather than
> waiting for some future mode setting operation to eventually
> dereference it.
> 
> The existing dereference of old_intel_state->wait_req is still
> required as that will hold the wait_req when the mode setting
> operation fails.
> 
> cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> cc: David Airlie <airlied@xxxxxxxx>
> cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Keith Packard <keithp@xxxxxxxxxx>

Hm, I think we should just clean up wiat_req in ->atomic_destroy_state
instead of littering cleanup code all over. But this gets the job done, so
applied.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3074c56..dbabaf3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13924,6 +13924,7 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  	struct drm_device *dev = plane->dev;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct intel_plane_state *old_intel_state;
> +	struct intel_plane_state *intel_state = to_intel_plane_state(plane->state);
>  	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
>  	struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb);
>  
> @@ -13941,6 +13942,7 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  	    (obj && !(obj->frontbuffer_bits & intel_plane->frontbuffer_bit)))
>  		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
>  
> +	i915_gem_request_assign(&intel_state->wait_req, NULL);
>  	i915_gem_request_assign(&old_intel_state->wait_req, NULL);
>  }
>  
> -- 
> 2.8.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch