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

[Maarten Lankhorst]

Op 08-08-16 om 17:34 schreef Daniel Vetter:
> On Mon, Aug 8, 2016 at 1:23 PM, Maarten Lankhorst
> <maarten.lankhorst at 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 at linux.intel.com>
>> Cc: Keith Packard <keithp at keithp.com>
>> Cc: Daniel Vetter <daniel.vetter at intel.com>
>> Cc: David Airlie <airlied at linux.ie>
>> Cc: intel-gfx at lists.freedesktop.org
>> Cc: dri-devel at 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