Re: [Intel-gfx] [PATCH 2/2] drm/i915: Drop reference to current state wait req as soon as it goes unused

2016-08-09 Thread Maarten Lankhorst 
Op 08-08-16 om 17:34 schreef Daniel Vetter:
> On Mon, Aug 8, 2016 at 1:23 PM, Maarten Lankhorst
>  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, wait_req is not freed until
>> the next commit, which is no longer used after waiting.
>>
>> Free it as soon as possible, so we don't hold on to it indefinitely.
>>
>> Signed-off-by: Maarten Lankhorst 
>> Cc: Keith Packard 
>> Cc: Daniel Vetter 
>> Cc: David Airlie 
>> Cc: intel-gfx@lists.freedesktop.org
>> Cc: dri-de...@lists.freedesktop.org
>> Fixes: 849782575325 ("drm/i915: cleanup_plane_fb: also drop reference to 
>> current state wait_req")
> We still need to clean up the reference in case of failure, which
> means latest in intel_plane_destroy_state(). Also hanging onto a
> request isn't that evil really, why can't we just only clean up in the
> destroy function?
Hm true, we're still guaranteed to call cleanup_plane_fb in case of failure 
though, else the WARN in unref would trigger.

I think it's harmless to hang onto the request, worst case we keep an extra 
MAX_PLANES * MAX_CRTCS * sizeof(i915_gem_request) allocated,
which is slightly more than 4k memory. (4 * 3 * 352)

If we unref the request in the destructor, it's also one step closer to 
constifying plane_state.

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

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Drop reference to current state wait req as soon as it goes unused

2016-08-08 Thread Keith Packard 
Daniel Vetter  writes:

> We still need to clean up the reference in case of failure, which
> means latest in intel_plane_destroy_state(). Also hanging onto a
> request isn't that evil really, why can't we just only clean up in the
> destroy function?

Sticking a cleanup there as well is good insurance, but it seems like a
reasonable policy to drop references when values go 'out of scope' as
they do in cleanup_plane_fb. But, you're right, we only *need* to drop
the reference in the destroy function, it's just that the state hangs
around until the next mode setting operation, which is likely to be days
away.

-- 
-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 2/2] drm/i915: Drop reference to current state wait req as soon as it goes unused

2016-08-08 Thread Daniel Vetter 
On Mon, Aug 8, 2016 at 1:23 PM, Maarten Lankhorst
 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, wait_req is not freed until
> the next commit, which is no longer used after waiting.
>
> Free it as soon as possible, so we don't hold on to it indefinitely.
>
> Signed-off-by: Maarten Lankhorst 
> Cc: Keith Packard 
> Cc: Daniel Vetter 
> Cc: David Airlie 
> Cc: intel-gfx@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org
> Fixes: 849782575325 ("drm/i915: cleanup_plane_fb: also drop reference to 
> current state wait_req")

We still need to clean up the reference in case of failure, which
means latest in intel_plane_destroy_state(). Also hanging onto a
request isn't that evil really, why can't we just only clean up in the
destroy function?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index f56707289615..e72ad97998a4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13754,6 +13754,8 @@ static void intel_atomic_commit_tail(struct 
> drm_atomic_state *state)
> /* EIO should be eaten, and we can't get interrupted in the
>  * worker, and blocking commits have waited already. */
> WARN_ON(ret);
> +
> +   i915_gem_request_assign(_plane_state->wait_req, NULL);
> }
>
> if (!state->legacy_cursor_update)
> @@ -14190,7 +14192,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>const struct drm_plane_state *old_state)
>  {
> struct drm_device *dev = plane->dev;
> -   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 intel_plane_state *old_intel_state =
> to_intel_plane_state(old_state);
> @@ -14199,7 +14200,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
> !INTEL_INFO(dev)->cursor_needs_physical))
> intel_unpin_fb_obj(old_state->fb, old_state->rotation);
>
> -   i915_gem_request_assign(_state->wait_req, NULL);
> i915_gem_request_assign(_intel_state->wait_req, NULL);
>  }
>
> --
> 2.7.4
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/2] drm/i915: Drop reference to current state wait req as soon as it goes unused

2016-08-08 Thread Maarten Lankhorst 
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, wait_req is not freed until
the next commit, which is no longer used after waiting.

Free it as soon as possible, so we don't hold on to it indefinitely.

Signed-off-by: Maarten Lankhorst 
Cc: Keith Packard 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: intel-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Fixes: 849782575325 ("drm/i915: cleanup_plane_fb: also drop reference to 
current state wait_req")
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index f56707289615..e72ad97998a4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13754,6 +13754,8 @@ static void intel_atomic_commit_tail(struct 
drm_atomic_state *state)
/* EIO should be eaten, and we can't get interrupted in the
 * worker, and blocking commits have waited already. */
WARN_ON(ret);
+
+   i915_gem_request_assign(_plane_state->wait_req, NULL);
}
 
if (!state->legacy_cursor_update)
@@ -14190,7 +14192,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
   const struct drm_plane_state *old_state)
 {
struct drm_device *dev = plane->dev;
-   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 intel_plane_state *old_intel_state =
to_intel_plane_state(old_state);
@@ -14199,7 +14200,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
!INTEL_INFO(dev)->cursor_needs_physical))
intel_unpin_fb_obj(old_state->fb, old_state->rotation);
 
-   i915_gem_request_assign(_state->wait_req, NULL);
i915_gem_request_assign(_intel_state->wait_req, NULL);
 }
 
-- 
2.7.4

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