Op 21-09-18 om 01:18 schreef Matt Roper:
> On Thu, Sep 20, 2018 at 12:27:05PM +0200, Maarten Lankhorst wrote:
>> While we may not update new_crtc_state, we may clear active_planes
>> if the new cursor update state will disable the cursor, but we fail
>> after. If this is immediately followed by a modeset disable, we may
>> soon not disable the planes correctly when we start depending on
>> active_planes.
>>
>> Changes since v1:
>> - Clarify why we cannot swap crtc_state. (Matt)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++-------
>> 1 file changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 58c188482c78..078cdcca88e1 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13515,14 +13515,16 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>> struct drm_plane_state *old_plane_state, *new_plane_state;
>> struct intel_plane *intel_plane = to_intel_plane(plane);
>> struct drm_framebuffer *old_fb;
>> - struct drm_crtc_state *crtc_state = crtc->state;
>> + struct intel_crtc_state *crtc_state =
>> + to_intel_crtc_state(crtc->state);
>> + struct intel_crtc_state *new_crtc_state;
>>
>> /*
>> * When crtc is inactive or there is a modeset pending,
>> * wait for it to complete in the slowpath
>> */
>> - if (!crtc_state->active || needs_modeset(crtc_state) ||
>> - to_intel_crtc_state(crtc_state)->update_pipe)
>> + if (!crtc_state->base.active || needs_modeset(&crtc_state->base) ||
>> + crtc_state->update_pipe)
>> goto slow;
>>
>> old_plane_state = plane->state;
>> @@ -13552,6 +13554,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>> if (!new_plane_state)
>> return -ENOMEM;
>>
>> + new_crtc_state = to_intel_crtc_state(intel_crtc_duplicate_state(crtc));
>> + if (!new_crtc_state) {
>> + ret = -ENOMEM;
>> + goto out_free;
>> + }
>> +
>> drm_atomic_set_fb_for_plane(new_plane_state, fb);
>>
>> new_plane_state->src_x = src_x;
>> @@ -13563,9 +13571,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>> new_plane_state->crtc_w = crtc_w;
>> new_plane_state->crtc_h = crtc_h;
>>
>> - ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),
>> - to_intel_crtc_state(crtc->state), /* FIXME need a new crtc state? */
>> - to_intel_plane_state(plane->state),
>> + ret = intel_plane_atomic_check_with_state(crtc_state, new_crtc_state,
>> + to_intel_plane_state(old_plane_state),
>> to_intel_plane_state(new_plane_state));
>> if (ret)
>> goto out_free;
>> @@ -13587,10 +13594,21 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>> /* Swap plane state */
>> plane->state = new_plane_state;
>>
>> + /*
>> + * We cannot swap crtc_state as it may be in use by an atomic commit or
>> + * page flip that's running simultaneously. If we swap crtc_state and
>> + * destroy the old state, we will cause a use-after-free there.
> Just to confirm, the commit running simultaneously would have to have
> already dropped locks (specifically the crtc lock) and returned to
> userspace for us to have this problem, right? So it's either a
> non-blocking commit in the process of doing its programming via
> workqueue, or a blocking commit that's done everything except
> intel_atomic_cleanup_work?
>
> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
Anything before drm_atomic_helper_commit_hw_done().
Cleanup work is done after, and isn't allowed to look at new state any more. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
