| Message ID | 1470655403-24036-3-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 8, 2016 at 1:23 PM, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> 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 <maarten.lankhorst@linux.intel.com> > Cc: Keith Packard <keithp@keithp.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: David Airlie <airlied@linux.ie> > Cc: intel-gfx@lists.freedesktop.org > Cc: dri-devel@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(&intel_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(&intel_state->wait_req, NULL); > i915_gem_request_assign(&old_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 <daniel@ffwll.ch> 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.
Op 08-08-16 om 17:34 schreef Daniel Vetter: > On Mon, Aug 8, 2016 at 1:23 PM, Maarten Lankhorst > <maarten.lankhorst@linux.intel.com> 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 <maarten.lankhorst@linux.intel.com> >> Cc: Keith Packard <keithp@keithp.com> >> Cc: Daniel Vetter <daniel.vetter@intel.com> >> Cc: David Airlie <airlied@linux.ie> >> Cc: intel-gfx@lists.freedesktop.org >> Cc: dri-devel@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
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(&intel_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(&intel_state->wait_req, NULL); i915_gem_request_assign(&old_intel_state->wait_req, NULL); }
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 <maarten.lankhorst@linux.intel.com> Cc: Keith Packard <keithp@keithp.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: David Airlie <airlied@linux.ie> Cc: intel-gfx@lists.freedesktop.org Cc: dri-devel@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(-)