Re: [PATCH 1/4] drm/i915: Make atomic use in-flight state for CRTC active value

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

 



On Wed, Apr 08, 2015 at 06:56:51PM -0700, Matt Roper wrote:
> Our atomic plane code currently uses intel_crtc->active to determine
> how/when to update some derived state values.  This works fine for pure
> plane updates at the moment since the CRTC state itself isn't changed as
> part of the operation.  However as we convert more of our driver
> internals over to atomic modesetting, we need to look at whether the
> CRTC will be active at the *end* of the atomic transaction (which may
> not match the currently committed state).
> 
> The in-flight value we want to use is generally in a crtc_state object
> associated with our top-level atomic transaction.  However there are a
> few cases where this isn't the case:
> 
>  * While using transitional atomic helpers (as we are at the moment),
>    SetPlane() calls will operate on orphaned plane states that aren't
>    part of a top-level atomic transaction.  In this case, we're not
>    touching the CRTC state, so it's fine to use the already-committed
>    value from crtc->state.
> 
>  * While updating properties of a disabled plane, we'll have a top-level
>    atomic state, but it may not contain the CRTC state we're looking
>    for.  Once again, this means we're not actually touching any CRTC
>    state so it's safe to use the value from crtc->state directly.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 11 +++--
>  drivers/gpu/drm/i915/intel_display.c      | 73 +++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h          |  3 ++
>  drivers/gpu/drm/i915/intel_sprite.c       | 16 +++++--
>  4 files changed, 93 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 976b891..90c4a82 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -111,12 +111,17 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  {
>  	struct drm_crtc *crtc = state->crtc;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> +	bool active;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(intel_state);
> +	active = intel_crtc_state->base.enable;
> +
>  	/*
>  	 * Both crtc and plane->crtc could be NULL if we're updating a
>  	 * property while the plane is disabled.  We don't actually have
> @@ -143,10 +148,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
>  	intel_state->clip.x1 = 0;
>  	intel_state->clip.y1 = 0;
> -	intel_state->clip.x2 =
> -		intel_crtc->active ? intel_crtc->config->pipe_src_w : 0;
> -	intel_state->clip.y2 =
> -		intel_crtc->active ? intel_crtc->config->pipe_src_h : 0;
> +	intel_state->clip.x2 = active ? intel_crtc_state->pipe_src_w : 0;
> +	intel_state->clip.y2 = active ? intel_crtc_state->pipe_src_h : 0;

We depend on the clipping to keep planes from getting enabled on a
disabled pipe. So I think this is going to blow up.

>  
>  	/*
>  	 * Disabling a plane is always okay; we just need to update
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7bfe2af..88b0f69 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12562,6 +12562,53 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  	}
>  }
>  
> +/**
> + * intel_crtc_state_for_plane - Obtain CRTC state for a plane
> + * @pstate: plane state to lookup corresponding crtc state for
> + *
> + * When working with a top-level atomic transaction (drm_atomic_state),
> + * a CRTC state should be present that corresponds to the provided
> + * plane state; this function provides a quick way to fetch that
> + * CRTC state.  In cases where we have a plane state unassociated with any
> + * top-level atomic transaction (e.g., while using the transitional atomic
> + * helpers), the current CRTC state from crtc->state will be returned
> + * instead.
> + */
> +struct intel_crtc_state *
> +intel_crtc_state_for_plane(struct intel_plane_state *pstate)
> +{
> +	struct drm_atomic_state *state = pstate->base.state;
> +	struct intel_plane *plane = to_intel_plane(pstate->base.plane);
> +	struct drm_crtc *crtc = intel_get_crtc_for_pipe(pstate->base.plane->dev,
> +							plane->pipe);
> +	int i;
> +
> +	/*
> +	 * While using transitional plane helpers, we may not have a top-level
> +	 * atomic transaction.  Of course that also means that we're not
> +	 * actually touching CRTC state, so just return the currently
> +	 * committed state.
> +	 */
> +	if (!state)
> +		return to_intel_crtc_state(crtc->state);
> +
> +	for (i = 0; i < state->dev->mode_config.num_crtc; i++) {
> +		if (!state->crtcs[i])
> +			continue;
> +
> +		if (to_intel_crtc(state->crtcs[i])->pipe == plane->pipe)
> +			return to_intel_crtc_state(state->crtc_states[i]);
> +	}
> +
> +	/*
> +	 * We may have a plane state without a corresponding CRTC state if
> +	 * we're updating a property of a disabled plane.  Again, just using
> +	 * the already-committed state for this plane's CRTC should be fine
> +	 * since we're not actually touching the CRTC here.
> +	 */
> +	return to_intel_crtc_state(crtc->state);
> +}
> +
>  static int
>  intel_check_primary_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
> @@ -12570,15 +12617,20 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc = state->base.crtc;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct drm_framebuffer *fb = state->base.fb;
>  	struct drm_rect *dest = &state->dst;
>  	struct drm_rect *src = &state->src;
>  	const struct drm_rect *clip = &state->clip;
> +	bool active;
>  	int ret;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	ret = drm_plane_helper_check_update(plane, crtc, fb,
>  					    src, dest, clip,
>  					    DRM_PLANE_HELPER_NO_SCALING,
> @@ -12587,7 +12639,7 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	if (ret)
>  		return ret;
>  
> -	if (intel_crtc->active) {
> +	if (active) {
>  		intel_crtc->atomic.wait_for_flips = true;
>  
>  		/*
> @@ -12638,11 +12690,16 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct drm_rect *src = &state->src;
> +	bool active;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	plane->fb = fb;
>  	crtc->x = src->x1 >> 16;
>  	crtc->y = src->y1 >> 16;
> @@ -12854,12 +12911,17 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	const struct drm_rect *clip = &state->clip;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	unsigned stride;
> +	bool active;
>  	int ret;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	ret = drm_plane_helper_check_update(plane, crtc, fb,
>  					    src, dest, clip,
>  					    DRM_PLANE_HELPER_NO_SCALING,
> @@ -12892,7 +12954,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	}
>  
>  finish:
> -	if (intel_crtc->active) {
> +	if (active) {
>  		if (plane->state->crtc_w != state->base.crtc_w)
>  			intel_crtc->atomic.update_wm = true;
>  
> @@ -12910,12 +12972,17 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	struct drm_crtc *crtc = state->base.crtc;
>  	struct drm_device *dev = plane->dev;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
>  	uint32_t addr;
> +	bool active;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	plane->fb = state->base.fb;
>  	crtc->cursor_x = state->base.crtc_x;
>  	crtc->cursor_y = state->base.crtc_y;
> @@ -12934,7 +13001,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	intel_crtc->cursor_bo = obj;
>  update:
>  
> -	if (intel_crtc->active)
> +	if (active)
>  		intel_crtc_update_cursor(crtc, state->visible);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index efa53d5..71e0152 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -998,6 +998,9 @@ intel_rotation_90_or_270(unsigned int rotation)
>  
>  bool intel_wm_need_update(struct drm_plane *plane,
>  			  struct drm_plane_state *state);
> +struct intel_crtc_state *
> +intel_crtc_state_for_plane(struct intel_plane_state *pstate);
> +
>  
>  /* shared dpll functions */
>  struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index a4c0a04..2016e78 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -864,6 +864,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  			 struct intel_plane_state *state)
>  {
>  	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_framebuffer *fb = state->base.fb;
>  	int crtc_x, crtc_y;
> @@ -875,9 +876,13 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	int hscale, vscale;
>  	int max_scale, min_scale;
>  	int pixel_size;
> +	bool active;
>  
>  	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	if (!fb) {
>  		state->visible = false;
>  		goto finish;
> @@ -1024,9 +1029,9 @@ finish:
>  	 */
>  	state->hides_primary = fb != NULL && drm_rect_equals(dst, clip) &&
>  		!colorkey_enabled(intel_plane);
> -	WARN_ON(state->hides_primary && !state->visible && intel_crtc->active);
> +	WARN_ON(state->hides_primary && !state->visible && active);
>  
> -	if (intel_crtc->active) {
> +	if (active) {
>  		if (intel_crtc->primary_enabled == state->hides_primary)
>  			intel_crtc->atomic.wait_for_flips = true;
>  
> @@ -1062,18 +1067,23 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  {
>  	struct drm_crtc *crtc = state->base.crtc;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *intel_crtc_state;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_framebuffer *fb = state->base.fb;
>  	int crtc_x, crtc_y;
>  	unsigned int crtc_w, crtc_h;
>  	uint32_t src_x, src_y, src_w, src_h;
> +	bool active;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_crtc_state = intel_crtc_state_for_plane(state);
> +	active = intel_crtc_state->base.enable;
> +
>  	plane->fb = fb;
>  
> -	if (intel_crtc->active) {
> +	if (active) {
>  		intel_crtc->primary_enabled = !state->hides_primary;
>  
>  		if (state->visible) {
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://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