Op 08-08-16 om 17:34 schreef Daniel Vetter: > On Mon, Aug 8, 2016 at 1:23 PM, Maarten Lankhorst > <[email protected]> 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 <[email protected]> >> Cc: Keith Packard <[email protected]> >> Cc: Daniel Vetter <[email protected]> >> Cc: David Airlie <[email protected]> >> Cc: [email protected] >> Cc: [email protected] >> 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
