Re: [PATCH v2 18/27] drm/i915: Handle disabling planes better.

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

Op 11-06-15 om 03:37 schreef Matt Roper:
> On Thu, Jun 04, 2015 at 02:47:48PM +0200, Maarten Lankhorst wrote:
>> Read out the initial state, and add a quirk to force disable all plane
>> that were initially active. Use this to disable planes during modeset
>> too.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c  |   7 ++
>>  drivers/gpu/drm/i915/intel_display.c | 139 +++++++++++++++++++++++++----------
>>  drivers/gpu/drm/i915/intel_drv.h     |   7 ++
>>  3 files changed, 114 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 8447a1fef332..5627df2807b0 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -96,6 +96,13 @@ int intel_atomic_check(struct drm_device *dev,
>>  		return -EINVAL;
>>  	}
>>  
>> +	if (crtc_state &&
>> +	    crtc_state->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE) {
>> +		ret = drm_atomic_add_affected_planes(state, &nuclear_crtc->base);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>  	ret = drm_atomic_helper_check_planes(dev, state);
>>  	if (ret)
>>  		return ret;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index b72724121f57..3b5d23692935 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4830,11 +4830,20 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
>>  	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
>>  }
>>  
>> +static void intel_disable_planes_on_crtc(struct drm_crtc *c)
>> +{
>> +	struct intel_crtc *crtc = to_intel_crtc(c);
>> +	struct drm_plane *plane;
>> +
>> +	drm_for_each_plane_mask(plane, crtc->base.dev,
>> +				crtc->atomic.force_disabled_planes)
>> +		to_intel_plane(plane)->disable_plane(plane, c);
>> +}
>> +
>>  static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -	struct intel_plane *intel_plane;
>>  	int pipe = intel_crtc->pipe;
>>  
>>  	intel_crtc_wait_for_pending_flips(crtc);
>> @@ -4842,14 +4851,7 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>>  	intel_pre_disable_primary(crtc);
>>  
>>  	intel_crtc_dpms_overlay_disable(intel_crtc);
>> -	for_each_intel_plane(dev, intel_plane) {
>> -		if (intel_plane->pipe == pipe) {
>> -			struct drm_crtc *from = intel_plane->base.crtc;
>> -
>> -			intel_plane->disable_plane(&intel_plane->base,
>> -						   from ?: crtc);
>> -		}
>> -	}
>> +	intel_disable_planes_on_crtc(crtc);
>>  
>>  	/*
>>  	 * FIXME: Once we grow proper nuclear flip support out of this we need
>> @@ -11554,6 +11556,21 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>>  	if (!intel_crtc->active || mode_changed)
>>  		return 0;
>>  
>> +	if (to_intel_crtc_state(crtc_state)->quirks &
>> +	    PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
>> +		if (!plane_state->crtc) {
>> +			intel_crtc->atomic.force_disabled_planes |=
>> +				1 << drm_plane_index(plane);
>> +			crtc_state->plane_mask &=
>> +				~(1 << drm_plane_index(plane));
>> +		}
>> +
>> +		/* Initial state for sprite planes is unknown,
>> +		 * no need to update sprite watermarks */
>> +		if (plane->type == DRM_PLANE_TYPE_OVERLAY)
>> +			mode_changed = true;
>> +	}
>> +
>>  	was_visible = old_plane_state->visible;
>>  	visible = to_intel_plane_state(plane_state)->visible;
>>  
>> @@ -11633,7 +11650,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>>  			intel_crtc->atomic.fb_bits |=
>>  			    INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
>>  
>> -		if (turn_off && is_crtc_enabled) {
>> +		if (turn_off && !mode_changed) {
>>  			intel_crtc->atomic.wait_vblank = true;
>>  			intel_crtc->atomic.update_sprite_watermarks |=
>>  				1 << i;
>> @@ -11714,6 +11731,11 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>>  		"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
>>  		idx, crtc->state->active, intel_crtc->active);
>>  
>> +	/* plane mask is fixed up after all initial planes are calculated */
>> +	pipe_config->quirks &= ~PIPE_CONFIG_QUIRK_INITIAL_PLANES;
>> +	if (mode_changed)
>> +		intel_crtc->atomic.force_disabled_planes = crtc->state->plane_mask;
>> +
>>  	if (mode_changed && crtc_state->enable &&
>>  	    dev_priv->display.crtc_compute_clock &&
>>  	    !WARN_ON(pipe_config->shared_dpll != DPLL_ID_PRIVATE)) {
>> @@ -12883,6 +12905,13 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
>>  		return ret;
>>  
>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +		if (to_intel_crtc_state(crtc_state)->quirks &
>> +		    PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
>> +			ret = drm_atomic_add_affected_planes(state, crtc);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +
>>  		if (!needs_modeset(crtc_state))
>>  			continue;
>>  
>> @@ -13529,13 +13558,17 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>>  	intel_runtime_pm_get(dev_priv);
>>  
>>  	/* Perform vblank evasion around commit operation */
>> -	if (crtc->state->active && !needs_modeset(crtc->state))
>> +	if (crtc->state->active && !needs_modeset(crtc->state)) {
>>  		intel_crtc->atomic.evade =
>>  			intel_pipe_update_start(intel_crtc,
>>  						&intel_crtc->atomic.start_vbl_count);
>>  
>> +		if (intel_crtc->atomic.force_disabled_planes)
>> +			intel_disable_planes_on_crtc(crtc);
>> +	}
>> +
>>  	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
>> -		skl_detach_scalers(intel_crtc);
>> +                skl_detach_scalers(intel_crtc);
> Unintentional change from tabs to spaces on this line?
Oops.

>>  }
>>  
>>  static void intel_finish_crtc_commit(struct drm_crtc *crtc)
>> @@ -15040,14 +15073,63 @@ void i915_redisable_vga(struct drm_device *dev)
>>  	i915_redisable_vga_power_on(dev);
>>  }
>>  
>> -static bool primary_get_hw_state(struct intel_crtc *crtc)
>> +static bool intel_read_hw_plane_state(struct intel_crtc *crtc,
>> +				      struct intel_plane *intel_plane)
>>  {
>> -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> +	struct drm_device *dev = crtc->base.dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>  
>> -	if (!crtc->base.enabled)
>> -		return false;
>> +	switch (intel_plane->base.type) {
>> +	case DRM_PLANE_TYPE_PRIMARY:
>> +		return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
>> +
>> +	case DRM_PLANE_TYPE_CURSOR:
>> +		if (IS_845G(dev) || IS_I865G(dev))
>> +			return I915_READ(_CURACNTR) & CURSOR_ENABLE;
>> +		else
>> +			return I915_READ(CURCNTR(crtc->plane)) & CURSOR_MODE;
>> +
> If we're not trying to seamlessly inherit the cursor image, I'm not sure
> I understand what the benefit of figuring out whether the cursor is
> actually on or not is.  Just assuming true, as we do for sprites, would
> be enough to make the cursor always turn off, right?
Yeah, when reworking this patch to apply after modeset revert I fixed that, but kept cursor readout since I wrote it anyway.

Also got me rid of tracking force disabled planes. :-)
>> +	default:
>> +		return true;
>> +	}
>> +}
>> +
>> +static int readout_plane_state(struct drm_atomic_state *state,
>> +			       struct intel_crtc *crtc,
>> +			       struct intel_crtc_state *crtc_state)
>> +{
>> +	struct intel_plane *p;
>> +	struct drm_plane_state *drm_plane_state;
>> +	bool active = crtc_state->base.active;
>> +
>> +	if (active) {
>> +		crtc_state->quirks |= PIPE_CONFIG_QUIRK_INITIAL_PLANES;
>>  
>> -	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
>> +		/* apply to previous sw state too */
>> +		to_intel_crtc_state(crtc->base.state)->quirks |=
>> +			PIPE_CONFIG_QUIRK_INITIAL_PLANES;
>> +	}
>> +
>> +	for_each_intel_plane(state->dev, p) {
>> +		if (crtc->plane != p->plane)
>> +			continue;
> Was this supposed to be comparing ->pipe rather than ->plane?  For
> primary and cursor planes I believe this works out okay since ->plane
> and ->pipe are basically the same (except for pre-gen4 FBC swapping),
> but for sprite planes, the ->plane pointer indicates whether it's the
> first, second, third, etc. sprite of that specific CRTC.
>
> As written, I think you'll only be operating on the first sprite plane
> of CRTC 0, the second sprite plane of CRTC 1, etc.
Ah thanks, that would explain it. :)

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