Op 08-08-16 om 17:34 schreef Daniel Vetter:
> On Mon, Aug 8, 2016 at 1:23 PM, Maarten Lankhorst
> <maarten.lankhorst@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>> Cc: Keith Packard <keithp@xxxxxxxxxx>
>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>> Cc: David Airlie <airlied@xxxxxxxx>
>> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
