Re: [PATCH 2/8] drm/i915: Handle cursor updating active_planes correctly, v2.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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@linux.intel.com>
>> ---
>>  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@intel.com>
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx