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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx>

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.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux