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

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

 



On Thu, Aug 27, 2015 at 04:06:46PM +0200, Maarten Lankhorst wrote:
> Op 27-08-15 om 15:58 schreef Ville Syrjälä:
> > On Thu, Aug 27, 2015 at 03:44:05PM +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@xxxxxxxxxxxxxxx>
> >> ---
> >>  drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
> >>  drivers/gpu/drm/i915/intel_display.c | 110 ++++++++++++++++++++++-------------
> >>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
> >>  3 files changed, 72 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> >> index 9336e8030980..8287b81287a0 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 7b5dfe2f7b2d..d3874a68cdb9 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 {
> >> @@ -3305,14 +3308,20 @@ 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)
> >>  {
> >>  	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_atomic_helper_update_legacy_modeset_state might not be called. */
> >> +	crtc->base.mode = crtc->base.state->mode;
> >> +
> >> +	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);
> >> @@ -3324,27 +3333,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);
> >>  	}
> >> -	crtc->config->pipe_src_w = adjusted_mode->crtc_hdisplay;
> >> -	crtc->config->pipe_src_h = adjusted_mode->crtc_vdisplay;
> >>  }
> >>  
> >>  static void intel_fdi_normal_train(struct drm_crtc *crtc)
> >> @@ -5003,7 +5011,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
> >>  	}
> >>  }
> >>  
> >> -static void ironlake_pfit_disable(struct intel_crtc *crtc)
> >> +static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force)
> >>  {
> >>  	struct drm_device *dev = crtc->base.dev;
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >> @@ -5011,7 +5019,7 @@ static void ironlake_pfit_disable(struct intel_crtc *crtc)
> >>  
> >>  	/* To avoid upsetting the power well on haswell only disable the pfit if
> >>  	 * it's in use. The hw state code will make sure we get this right. */
> >> -	if (crtc->config->pch_pfit.enabled) {
> >> +	if (force || crtc->config->pch_pfit.enabled) {
> >>  		I915_WRITE(PF_CTL(pipe), 0);
> >>  		I915_WRITE(PF_WIN_POS(pipe), 0);
> >>  		I915_WRITE(PF_WIN_SZ(pipe), 0);
> >> @@ -5038,7 +5046,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
> >>  
> >>  	intel_disable_pipe(intel_crtc);
> >>  
> >> -	ironlake_pfit_disable(intel_crtc);
> >> +	ironlake_pfit_disable(intel_crtc, false);
> >>  
> >>  	if (intel_crtc->config->has_pch_encoder)
> >>  		ironlake_fdi_disable(crtc);
> >> @@ -5101,7 +5109,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
> >>  	if (INTEL_INFO(dev)->gen == 9)
> >>  		skylake_scaler_disable(intel_crtc);
> >>  	else if (INTEL_INFO(dev)->gen < 9)
> >> -		ironlake_pfit_disable(intel_crtc);
> >> +		ironlake_pfit_disable(intel_crtc, false);
> >>  	else
> >>  		MISSING_CASE(INTEL_INFO(dev)->gen);
> >>  
> >> @@ -12246,7 +12254,6 @@ static bool intel_fuzzy_clock_check(int clock1, int clock2)
> >>  			    base.head) \
> >>  		if (mask & (1 <<(intel_crtc)->pipe))
> >>  
> >> -
> >>  static bool
> >>  intel_compare_m_n(unsigned int m, unsigned int n,
> >>  		  unsigned int m2, unsigned int n2,
> >> @@ -12467,8 +12474,16 @@ intel_pipe_config_compare(struct drm_device *dev,
> >>  				      DRM_MODE_FLAG_NVSYNC);
> >>  	}
> >>  
> >> -	PIPE_CONF_CHECK_I(pipe_src_w);
> >> -	PIPE_CONF_CHECK_I(pipe_src_h);
> >> +	if (!adjust) {
> >> +		PIPE_CONF_CHECK_I(pipe_src_w);
> >> +		PIPE_CONF_CHECK_I(pipe_src_h);
> >> +
> >> +		PIPE_CONF_CHECK_I(pch_pfit.enabled);
> >> +		if (current_config->pch_pfit.enabled) {
> >> +			PIPE_CONF_CHECK_I(pch_pfit.pos);
> >> +			PIPE_CONF_CHECK_I(pch_pfit.size);
> >> +		}
> >> +	}
> > What's this hack now? Aren't you computing the correct config or why do
> > you need to skip the state check?
> >
> This is not a hack but intentional. This function is called with adjust=true when comparing the state
> to see if a full blown modeset is needed or not. Because  intel_update_pipe_config can modify
> pch_pfit and pipe_src* there's no need to consider it when doing a modeset.
> 
> But you do need the true values, or intel_update_pipe_config will do the wrong thing.

Ah. I didn't realize we'd given it a dual purpose. Does make some sense
though to avoid having write similar tests twice.

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