Re: [PATCH 5/6] drm/i915: Make updating pipe without modeset atomic.

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

Op 21-07-15 om 16:14 schreef Daniel Vetter:
> On Tue, Jul 21, 2015 at 01:29:01PM +0200, Maarten Lankhorst wrote:
>> Instead of doing a hack during primary plane commit the state
>> is updated during atomic evasion. It handles differences in
>> pipe size and the panel fitter.
>>
>> This is continuing on top of Daniel's work to make faster
>> modesets atomic, and not yet enabled by default.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c  |  2 +-
>>  drivers/gpu/drm/i915/intel_display.c | 93 ++++++++++++++++++++----------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  2 +
>>  3 files changed, 55 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 09a0ad611002..58d62fbe961b 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -98,8 +98,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>>  		return NULL;
>>  
>>  	__drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state->base);
>> -
>>  	crtc_state->base.crtc = crtc;
>> +	crtc_state->update_pipe = false;
>>  
>>  	return &crtc_state->base;
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 443328033981..480b2336c7ba 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -108,6 +108,9 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
>>  	struct intel_crtc_state *crtc_state);
>>  static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
>>  			   int num_connectors);
>> +static void skylake_pfit_enable(struct intel_crtc *crtc);
>> +static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force);
>> +static void ironlake_pfit_enable(struct intel_crtc *crtc);
>>  static void intel_modeset_setup_hw_state(struct drm_device *dev);
>>  
>>  typedef struct {
>> @@ -3268,14 +3271,17 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
>>  	return pending;
>>  }
>>  
>> -static void intel_update_pipe_size(struct intel_crtc *crtc)
>> +static void intel_update_pipe_config(struct intel_crtc *crtc,
>> +				     struct intel_crtc_state *old_crtc_state)
> upate_pipe_config sounds like it's just updating the hw state. Maybe call
> it intel_fastset_crtc or intel_fixup_crtc or just intel_update_crtc? The
> other functions are also called crtc_enable/disable, so going with crtc
> seems more consistent.
intel_crtc_update_pipe ?

>>  {
>>  	struct drm_device *dev = crtc->base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	const struct drm_display_mode *adjusted_mode;
>> +	struct intel_crtc_state *pipe_config =
>> +		to_intel_crtc_state(crtc->base.state);
>>  
>> -	if (!i915.fastboot)
>> -		return;
>> +	DRM_DEBUG_KMS("Updating pipe size %ix%i -> %ix%i\n",
>> +		      old_crtc_state->pipe_src_w, old_crtc_state->pipe_src_h,
>> +		      pipe_config->pipe_src_w, pipe_config->pipe_src_h);
>>  
>>  	if (HAS_DDI(dev))
>>  		intel_set_pipe_csc(&crtc->base);
>> @@ -3287,27 +3293,26 @@ static void intel_update_pipe_size(struct intel_crtc *crtc)
>>  	 * fastboot case, we'll flip, but if we don't update the pipesrc and
>>  	 * pfit state, we'll end up with a big fb scanned out into the wrong
>>  	 * sized surface.
>> -	 *
>> -	 * To fix this properly, we need to hoist the checks up into
>> -	 * compute_mode_changes (or above), check the actual pfit state and
>> -	 * whether the platform allows pfit disable with pipe active, and only
>> -	 * then update the pipesrc and pfit state, even on the flip path.
>>  	 */
>>  
>> -	adjusted_mode = &crtc->config->base.adjusted_mode;
>> -
>>  	I915_WRITE(PIPESRC(crtc->pipe),
>> -		   ((adjusted_mode->crtc_hdisplay - 1) << 16) |
>> -		   (adjusted_mode->crtc_vdisplay - 1));
>> -	if (!crtc->config->pch_pfit.enabled &&
>> -	    (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||
>> -	     intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
>> -		I915_WRITE(PF_CTL(crtc->pipe), 0);
>> -		I915_WRITE(PF_WIN_POS(crtc->pipe), 0);
>> -		I915_WRITE(PF_WIN_SZ(crtc->pipe), 0);
>> +		   ((pipe_config->pipe_src_w - 1) << 16) |
>> +		   (pipe_config->pipe_src_h - 1));
>> +
>> +	/* on skylake this is done by detaching scalers */
>> +	if (INTEL_INFO(dev)->gen == 9) {
>> +		skl_detach_scalers(crtc);
>> +
>> +		if (pipe_config->pch_pfit.enabled)
>> +			skylake_pfit_enable(crtc);
>> +	}
>> +	else if (INTEL_INFO(dev)->gen < 9 &&
>> +	         HAS_PCH_SPLIT(dev)) {
>> +		if (pipe_config->pch_pfit.enabled)
>> +			ironlake_pfit_enable(crtc);
>> +		else if (old_crtc_state->pch_pfit.enabled)
>> +			ironlake_pfit_disable(crtc, true);
> Iirc Jesse's experiments showed that you could only disable the pfit
> without a modeset, but not enable it without a modeset. Not sure how that
> works on skl, but that's what I remember for ealier platforms at least.
Is that really the case, or because of the power domain updates? Which, come to think of it, I don't
handle correctly in this patch. :/

intel_ddi_enable_transcoder_func seems to have a special case for haswell on eDP with panel fitter on/off.
So perhaps disallow off -> on with haswell.

Broadwell might not be able to change panel fitter from on to off, because of cdclk calculations:
broadwell_modeset_calc_cdclk -> ilk_max_pixel_rate -> ilk_pipe_pixel_rate

Those reasons might be contributing to failures.

> Also it should work the same for gmch platforms really.
Probably, but I wasn't able to test it and current code didn't seem to handle it.

> Also big thing still missing is the special testcase which does direct
> modesets trying to force fastset pfit changes on edp/dsi/lvds.
Indeed! What should I expect on failure, and how do I define success?

~Maarten

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