Re: [PATCH 01/14] drm/i915: Track visible planes in a bitmask

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Dec 15, 2016 at 06:11:24PM +0100, Maarten Lankhorst wrote:
> Op 15-12-16 om 16:02 schreef Ville Syrjälä:
> > On Thu, Dec 15, 2016 at 03:56:03PM +0100, Maarten Lankhorst wrote:
> >> Op 12-12-16 om 21:35 schreef ville.syrjala@linux.intel.com:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> In a lot of place we wish to know which planes on the crtc are actually
> >>> visible, or how many of them there are. Let's start tracking that in a
> >>> bitmask in the crtc state.
> >>>
> >>> We already track enabled planes (ie. ones with an fb and crtc specified by
> >>> the user) but that's not quite the same thing as enabled planes may
> >>> still end up being invisible due to clipping and whatnot.
> >>>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_atomic_plane.c |  6 +++
> >>>  drivers/gpu/drm/i915/intel_display.c      | 79 +++++++++++++++++++++----------
> >>>  drivers/gpu/drm/i915/intel_drv.h          |  3 ++
> >>>  3 files changed, 64 insertions(+), 24 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>> index dbe9fb41ae53..7ec12edda4d4 100644
> >>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>> @@ -181,6 +181,12 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >>>  	if (ret)
> >>>  		return ret;
> >>>  
> >>> +	/* FIXME pre-g4x don't work like this */
> >>> +	if (intel_state->base.visible)
> >>> +		crtc_state->active_planes |= BIT(intel_plane->id);
> >>> +	else
> >>> +		crtc_state->active_planes &= ~BIT(intel_plane->id);
> >>> +
> >>>  	return intel_plane_atomic_calc_changes(&crtc_state->base, state);
> >>>  }
> >>>  
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>> index bc1af87789bc..3f027341b0f3 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -2741,6 +2741,29 @@ update_state_fb(struct drm_plane *plane)
> >>>  }
> >>>  
> >>>  static void
> >>> +intel_set_plane_visible(struct intel_crtc_state *crtc_state,
> >>> +			struct intel_plane_state *plane_state,
> >>> +			bool visible)
> >>> +{
> >>> +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> >>> +
> >>> +	plane_state->base.visible = visible;
> >>> +
> >>> +	/* FIXME pre-g4x don't work like this */
> >>> +	if (visible) {
> >>> +		crtc_state->base.plane_mask |= BIT(drm_plane_index(&plane->base));
> >>> +		crtc_state->active_planes |= BIT(plane->id);
> >>> +	} else {
> >>> +		crtc_state->base.plane_mask &= ~BIT(drm_plane_index(&plane->base));
> >>> +		crtc_state->active_planes &= ~BIT(plane->id);
> >>> +	}
> >>> +
> >>> +	DRM_DEBUG_KMS("%s active planes 0x%x\n",
> >>> +		      crtc_state->base.crtc->name,
> >>> +		      crtc_state->active_planes);
> >>> +}
> >>> +
> >>> +static void
> >>>  intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> >>>  			     struct intel_initial_plane_config *plane_config)
> >>>  {
> >>> @@ -2798,8 +2821,9 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> >>>  	 * simplest solution is to just disable the primary plane now and
> >>>  	 * pretend the BIOS never had it enabled.
> >>>  	 */
> >>> -	to_intel_plane_state(plane_state)->base.visible = false;
> >>> -	crtc_state->plane_mask &= ~(1 << drm_plane_index(primary));
> >>> +	intel_set_plane_visible(to_intel_crtc_state(crtc_state),
> >>> +				to_intel_plane_state(plane_state),
> >>> +				false);
> >>>  	intel_pre_disable_primary_noatomic(&intel_crtc->base);
> >>>  	intel_plane->disable_plane(primary, &intel_crtc->base);
> >>>  
> >>> @@ -2826,7 +2850,11 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> >>>  	drm_framebuffer_reference(fb);
> >>>  	primary->fb = primary->state->fb = fb;
> >>>  	primary->crtc = primary->state->crtc = &intel_crtc->base;
> >>> -	intel_crtc->base.state->plane_mask |= (1 << drm_plane_index(primary));
> >>> +
> >>> +	intel_set_plane_visible(to_intel_crtc_state(intel_crtc->base.state),
> >>> +				to_intel_plane_state(primary->state),
> >>> +				true);
> >>> +
> >>>  	atomic_or(to_intel_plane(primary)->frontbuffer_bit,
> >>>  		  &obj->frontbuffer_bits);
> >>>  }
> >>> @@ -12440,11 +12468,11 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> >>>  	struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state);
> >>>  	struct drm_crtc *crtc = crtc_state->crtc;
> >>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >>> -	struct drm_plane *plane = plane_state->plane;
> >>> +	struct intel_plane *plane = to_intel_plane(plane_state->plane);
> >>>  	struct drm_device *dev = crtc->dev;
> >>>  	struct drm_i915_private *dev_priv = to_i915(dev);
> >>>  	struct intel_plane_state *old_plane_state =
> >>> -		to_intel_plane_state(plane->state);
> >>> +		to_intel_plane_state(plane->base.state);
> >>>  	bool mode_changed = needs_modeset(crtc_state);
> >>>  	bool was_crtc_enabled = crtc->state->active;
> >>>  	bool is_crtc_enabled = crtc_state->active;
> >>> @@ -12452,7 +12480,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> >>>  	struct drm_framebuffer *fb = plane_state->fb;
> >>>  	int ret;
> >>>  
> >>> -	if (INTEL_GEN(dev_priv) >= 9 && plane->type != DRM_PLANE_TYPE_CURSOR) {
> >>> +	if (INTEL_GEN(dev_priv) >= 9 && plane->id != PLANE_CURSOR) {
> >>>  		ret = skl_update_scaler_plane(
> >>>  			to_intel_crtc_state(crtc_state),
> >>>  			to_intel_plane_state(plane_state));
> >>> @@ -12476,8 +12504,10 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> >>>  	 * per-plane wm computation to the .check_plane() hook, and
> >>>  	 * only combine the results from all planes in the current place?
> >>>  	 */
> >>> -	if (!is_crtc_enabled)
> >>> +	if (!is_crtc_enabled) {
> >>>  		to_intel_plane_state(plane_state)->base.visible = visible = false;
> >>> +		to_intel_crtc_state(crtc_state)->active_planes &= ~BIT(plane->id);
> >>> +	}
> >>>  
> >>>  	if (!was_visible && !visible)
> >>>  		return 0;
> >>> @@ -12489,13 +12519,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> >>>  	turn_on = visible && (!was_visible || mode_changed);
> >>>  
> >>>  	DRM_DEBUG_ATOMIC("[CRTC:%d:%s] has [PLANE:%d:%s] with fb %i\n",
> >>> -			 intel_crtc->base.base.id,
> >>> -			 intel_crtc->base.name,
> >>> -			 plane->base.id, plane->name,
> >>> +			 intel_crtc->base.base.id, intel_crtc->base.name,
> >>> +			 plane->base.base.id, plane->base.name,
> >>>  			 fb ? fb->base.id : -1);
> >>>  
> >>>  	DRM_DEBUG_ATOMIC("[PLANE:%d:%s] visible %i -> %i, off %i, on %i, ms %i\n",
> >>> -			 plane->base.id, plane->name,
> >>> +			 plane->base.base.id, plane->base.name,
> >>>  			 was_visible, visible,
> >>>  			 turn_off, turn_on, mode_changed);
> >>>  
> >>> @@ -12503,15 +12532,15 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> >>>  		pipe_config->update_wm_pre = true;
> >>>  
> >>>  		/* must disable cxsr around plane enable/disable */
> >>> -		if (plane->type != DRM_PLANE_TYPE_CURSOR)
> >>> +		if (plane->id != PLANE_CURSOR)
> >>>  			pipe_config->disable_cxsr = true;
> >>>  	} else if (turn_off) {
> >>>  		pipe_config->update_wm_post = true;
> >>>  
> >>>  		/* must disable cxsr around plane enable/disable */
> >>> -		if (plane->type != DRM_PLANE_TYPE_CURSOR)
> >>> +		if (plane->id != PLANE_CURSOR)
> >>>  			pipe_config->disable_cxsr = true;
> >>> -	} else if (intel_wm_need_update(plane, plane_state)) {
> >>> +	} else if (intel_wm_need_update(&plane->base, plane_state)) {
> >>>  		/* FIXME bollocks */
> >>>  		pipe_config->update_wm_pre = true;
> >>>  		pipe_config->update_wm_post = true;
> >>> @@ -12523,7 +12552,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> >>>  		to_intel_crtc_state(crtc_state)->wm.need_postvbl_update = true;
> >>>  
> >>>  	if (visible || was_visible)
> >>> -		pipe_config->fb_bits |= to_intel_plane(plane)->frontbuffer_bit;
> >>> +		pipe_config->fb_bits |= plane->frontbuffer_bit;
> >>>  
> >>>  	/*
> >>>  	 * WaCxSRDisabledForSpriteScaling:ivb
> >>> @@ -12531,7 +12560,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> >>>  	 * cstate->update_wm was already set above, so this flag will
> >>>  	 * take effect when we commit and program watermarks.
> >>>  	 */
> >>> -	if (plane->type == DRM_PLANE_TYPE_OVERLAY && IS_IVYBRIDGE(dev_priv) &&
> >>> +	if (plane->id == PLANE_SPRITE0 && IS_IVYBRIDGE(dev_priv) &&
> >>>  	    needs_scaling(to_intel_plane_state(plane_state)) &&
> >>>  	    !needs_scaling(old_plane_state))
> >>>  		pipe_config->disable_lp_wm = true;
> >>> @@ -16876,15 +16905,14 @@ static bool primary_get_hw_state(struct intel_plane *plane)
> >>>  /* FIXME read out full plane state for all planes */
> >>>  static void readout_plane_state(struct intel_crtc *crtc)
> >>>  {
> >>> -	struct drm_plane *primary = crtc->base.primary;
> >>> -	struct intel_plane_state *plane_state =
> >>> -		to_intel_plane_state(primary->state);
> >>> +	struct intel_plane *primary = to_intel_plane(crtc->base.primary);
> >>> +	bool visible;
> >>>  
> >>> -	plane_state->base.visible = crtc->active &&
> >>> -		primary_get_hw_state(to_intel_plane(primary));
> >>> +	visible = crtc->active && primary_get_hw_state(primary);
> >>>  
> >>> -	if (plane_state->base.visible)
> >>> -		crtc->base.state->plane_mask |= 1 << drm_plane_index(primary);
> >>> +	intel_set_plane_visible(to_intel_crtc_state(crtc->base.state),
> >>> +				to_intel_plane_state(primary->base.state),
> >>> +				visible);
> >>>  }
> >>>  
> >>>  static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >>> @@ -17171,7 +17199,10 @@ void intel_modeset_gem_init(struct drm_device *dev)
> >>>  			c->primary->fb = NULL;
> >>>  			c->primary->crtc = c->primary->state->crtc = NULL;
> >>>  			update_state_fb(c->primary);
> >>> -			c->state->plane_mask &= ~(1 << drm_plane_index(c->primary));
> >>> +
> >>> +			intel_set_plane_visible(to_intel_crtc_state(c->state),
> >>> +						to_intel_plane_state(c->primary->state),
> >>> +						false);
> >>>  		}
> >>>  	}
> >>>  }
> >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>> index 8f4ddca0f521..20ba8f48bc3b 100644
> >>> --- a/drivers/gpu/drm/i915/intel_drv.h
> >>> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >>> @@ -669,6 +669,9 @@ struct intel_crtc_state {
> >>>  
> >>>  	/* Gamma mode programmed on the pipe */
> >>>  	uint32_t gamma_mode;
> >>> +
> >>> +	/* bitmask of visible planes (enum plane_id) */
> >>> +	u8 active_planes;
> >> Can we change this to uint32_t 1<<drm_plane_index like the other masks?
> >>
> >> plane_mask, connector_mask and encoder_mask use 1 << index.
> >> Perhaps name it active_planes_mask too.
> > The way I use it wouldn't lend itself well to that. I don't have any
> > plane structs around in a bunch of the places where this gets used.
> >
> Do you use this outside of 13/14? If not please drop the patch from this series. :)

It's used in multiple places.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx