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

[Keith Packard]

Daniel Vetter  writes:

> 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.

Thanks. It's required for the DRM patch I posted that makes moving the
cursor not block on rendering. I'm hoping Dave will get that merged for
4.8; it's really annoying to have the server moving the cursor in a
separate thread and then have the motion blocked in the kernel for a few
seconds worth of rendering.

-- 
-keith


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [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 
> cc: David Airlie 
> cc: intel-gfx@lists.freedesktop.org
> cc: dri-de...@lists.freedesktop.org
> Signed-off-by: Keith Packard 

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(_state->wait_req, NULL);
>   i915_gem_request_assign(_intel_state->wait_req, NULL);
>  }
>  
> -- 
> 2.8.1
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: cleanup_plane_fb: also drop reference to current state wait_req

[Keith Packard]

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 
cc: David Airlie 
cc: intel-gfx@lists.freedesktop.org
cc: dri-de...@lists.freedesktop.org
Signed-off-by: Keith Packard 
---
 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(_state->wait_req, NULL);
i915_gem_request_assign(_intel_state->wait_req, NULL);
 }
 
-- 
2.8.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx