Re: [PATCH v2 18/21] drm/i915: Make unpin async.

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

Op 24-05-16 om 16:00 schreef Daniel Vetter:
> On Tue, May 17, 2016 at 03:08:01PM +0200, Maarten Lankhorst wrote:
>> All of intel_post_plane_update is handled there now, so move it over.
>> This is run after the hw state checker because it can't handle checking
>> crtc's separately yet.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> First this patch is massive, and imo too big and should have been split
> up.
>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c  |  11 ++
>>  drivers/gpu/drm/i915/intel_display.c | 344 ++++++++++++++++++++++-------------
>>  drivers/gpu/drm/i915/intel_drv.h     |   5 +-
>>  3 files changed, 228 insertions(+), 132 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 50ff90aea721..b4927f6bbeac 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -311,6 +311,17 @@ intel_atomic_state_alloc(struct drm_device *dev)
>>  void intel_atomic_state_clear(struct drm_atomic_state *s)
>>  {
>>  	struct intel_atomic_state *state = to_intel_atomic_state(s);
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(state->work); i++) {
>> +		struct intel_flip_work *work = state->work[i];
>> +
>> +		if (work)
>> +			intel_free_flip_work(work);
>> +
>> +		state->work[i] = NULL;
>> +	}
>> +
>>  	drm_atomic_state_default_clear(&state->base);
>>  	state->dpll_set = state->modeset = false;
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 69abc808a2c4..16d8e299994d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4537,39 +4537,6 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
>>  	}
>>  }
>>  
>> -static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>> -{
>> -	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
>> -	struct drm_atomic_state *old_state = old_crtc_state->base.state;
>> -	struct intel_crtc_state *pipe_config =
>> -		to_intel_crtc_state(crtc->base.state);
>> -	struct drm_device *dev = crtc->base.dev;
>> -	struct drm_plane *primary = crtc->base.primary;
>> -	struct drm_plane_state *old_pri_state =
>> -		drm_atomic_get_existing_plane_state(old_state, primary);
>> -
>> -	intel_frontbuffer_flip(dev, pipe_config->fb_bits);
>> -
>> -	crtc->wm.cxsr_allowed = true;
>> -
>> -	if (pipe_config->update_wm_post && pipe_config->base.active)
>> -		intel_update_watermarks(&crtc->base);
>> -
>> -	if (old_pri_state) {
>> -		struct intel_plane_state *primary_state =
>> -			to_intel_plane_state(primary->state);
>> -		struct intel_plane_state *old_primary_state =
>> -			to_intel_plane_state(old_pri_state);
>> -
>> -		intel_fbc_post_update(crtc);
>> -
>> -		if (primary_state->visible &&
>> -		    (needs_modeset(&pipe_config->base) ||
>> -		     !old_primary_state->visible))
>> -			intel_post_enable_primary(&crtc->base);
>> -	}
>> -}
> The above seems to have moved/disappeared entirely, and is definitely not
> unpin related. Really, this must be split up. I guess another reason to
> revert this for now.
>
> Or at least the commit should have a more accurate summary. But since I
> can't even find the new callsite of these functions in this diff here
> something seems fishy, but I didn't look at the entire series.
>
> Anyway, with that rant out of the way see below for what I really wanted
> to comment on.
>
>
> [snip]
>
>> +static void intel_prepare_work(struct drm_crtc *crtc,
>> +			       struct intel_flip_work *work,
>> +			       struct drm_atomic_state *state,
>> +			       struct drm_crtc_state *old_crtc_state)
>>  {
>> -	unsigned last_vblank_count[I915_MAX_PIPES];
>> -	enum pipe pipe;
>> -	int ret;
>> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> +	struct drm_plane_state *old_plane_state;
>> +	struct drm_plane *plane;
>> +	int i, j = 0;
>>  
>> -	if (!crtc_mask)
>> -		return;
>> +	INIT_WORK(&work->unpin_work, intel_unpin_work_fn);
>> +	INIT_WORK(&work->mmio_work, intel_mmio_flip_work_func);
>> +	atomic_inc(&intel_crtc->unpin_work_count);
>>  
>> -	for_each_pipe(dev_priv, pipe) {
>> -		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
>> +	for_each_plane_in_state(state, plane, old_plane_state, i) {
>> +		struct intel_plane_state *old_state = to_intel_plane_state(old_plane_state);
>> +		struct intel_plane_state *new_state = to_intel_plane_state(plane->state);
>>  
>> -		if (!((1 << pipe) & crtc_mask))
>> +		if (old_state->base.crtc != crtc &&
>> +		    new_state->base.crtc != crtc)
>>  			continue;
>>  
>> -		ret = drm_crtc_vblank_get(crtc);
>> -		if (WARN_ON(ret != 0)) {
>> -			crtc_mask &= ~(1 << pipe);
>> -			continue;
>> +		if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
>> +			plane->fb = new_state->base.fb;
>> +			crtc->x = new_state->base.src_x >> 16;
>> +			crtc->y = new_state->base.src_y >> 16;
>>  		}
>>  
>> -		last_vblank_count[pipe] = drm_crtc_vblank_count(crtc);
>> +		old_state->wait_req = new_state->wait_req;
>> +		new_state->wait_req = NULL;
>> +
>> +		old_state->base.fence = new_state->base.fence;
>> +		new_state->base.fence = NULL;
>> +
>> +		/* remove plane state from the atomic state and move it to work */
>> +		old_plane_state->state = NULL;
>> +		state->planes[i] = NULL;
>> +		state->plane_states[i] = NULL;
>> +
>> +		work->old_plane_state[j] = old_state;
>> +		work->new_plane_state[j++] = new_state;
>>  	}
>>  
>> -	for_each_pipe(dev_priv, pipe) {
>> -		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
>> -		long lret;
>> +	old_crtc_state->state = NULL;
>> +	state->crtcs[drm_crtc_index(crtc)] = NULL;
>> +	state->crtc_states[drm_crtc_index(crtc)] = NULL;
>>  
>> -		if (!((1 << pipe) & crtc_mask))
>> -			continue;
>> +	work->old_crtc_state = to_intel_crtc_state(old_crtc_state);
>> +	work->new_crtc_state = to_intel_crtc_state(crtc->state);
>> +	work->num_planes = j;
>>  
>> -		lret = wait_event_timeout(dev->vblank[pipe].queue,
>> -				last_vblank_count[pipe] !=
>> -					drm_crtc_vblank_count(crtc),
>> -				msecs_to_jiffies(50));
>> +	work->event = crtc->state->event;
>> +	crtc->state->event = NULL;
>>  
>> -		WARN(!lret, "pipe %c vblank wait timed out\n", pipe_name(pipe));
>> +	if (needs_modeset(crtc->state) || work->new_crtc_state->update_pipe) {
>> +		struct drm_connector *conn;
>> +		struct drm_connector_state *old_conn_state;
>> +		int k = 0;
>>  
>> -		drm_crtc_vblank_put(crtc);
>> -	}
>> +		j = 0;
>> +
>> +		/*
>> +		 * intel_unpin_work_fn cannot depend on the connector list
>> +		 * because it may be freed from underneath it, so add
>> +		 * them all to the work struct while we're holding locks.
>> +		 */
> Thanks to connector refcounting no longer true, and as long as you hold
> onto drm_atomic_state the connectors will no longer disappear. Please
> revise/update.
This comment only became true thanks to connector refcounting. unpin_work
cannot take connection_mutex or mode_config->mutex because that would deadlock
if flushed from intel_atomic_commit with those locks held, but needs it for the
hw state verifier.

Before connector refcounting I could perform a flush in intel_dp_destroy_mst_connector
so even though unpin_work wouldn't have the locks, it would never see a modified list.

With connector refcounting I move the connector_states to the work, and free them there.
But it looks like the locking for unreferencing a connector seems to be messed up.

drm_atomic_state_free can be called without locking anything,
so drm_connector_free needs to handle its own locking when removing
the connector from the list. This affects all atomic drivers, and probably means
requiring a work item to free all connectors with the right locks..

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx