| Message ID | 1437478142-416-6-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > { > 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. Also it should work the same for gmch platforms really. Also big thing still missing is the special testcase which does direct modesets trying to force fastset pfit changes on edp/dsi/lvds. -Daniel > } > - 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) > @@ -4956,7 +4961,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; > @@ -4964,7 +4969,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); > @@ -4991,7 +4996,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); > @@ -5054,7 +5059,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); > > @@ -5208,7 +5213,8 @@ static void modeset_update_crtc_power_domains(struct drm_atomic_state *state) > int i; > > for_each_crtc_in_state(state, crtc, crtc_state, i) { > - if (needs_modeset(crtc->state)) > + if (needs_modeset(crtc->state) || > + to_intel_crtc_state(crtc->state)->update_pipe) > put_domains[to_intel_crtc(crtc)->pipe] = > modeset_get_crtc_power_domains(crtc); > } > @@ -12180,7 +12186,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, > @@ -12400,8 +12405,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); > + } > + } > > PIPE_CONF_CHECK_I(gmch_pfit.control); > /* pfit ratios are autocomputed by the hw on gen4+ */ > @@ -12409,12 +12422,6 @@ intel_pipe_config_compare(struct drm_device *dev, > PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios); > PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits); > > - 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); > - } > - > PIPE_CONF_CHECK_I(scaler_state.scaler_id); > > /* BDW+ don't expose a synchronous way to read the state */ > @@ -12877,7 +12884,6 @@ static int intel_modeset_all_pipes(struct drm_atomic_state *state) > return ret; > } > > - > static int intel_modeset_checks(struct drm_atomic_state *state) > { > struct drm_device *dev = state->dev; > @@ -12968,6 +12974,7 @@ static int intel_atomic_check(struct drm_device *dev, > to_intel_crtc_state(crtc->state), > pipe_config, true)) { > crtc_state->mode_changed = false; > + to_intel_crtc_state(crtc_state)->update_pipe = true; > } > > if (needs_modeset(crtc_state)) { > @@ -13426,10 +13433,6 @@ intel_commit_primary_plane(struct drm_plane *plane, > if (!crtc->state->active) > return; > > - if (state->visible) > - /* FIXME: kill this fastboot hack */ > - intel_update_pipe_size(intel_crtc); > - > dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y); > } > > @@ -13448,6 +13451,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, > { > struct drm_device *dev = crtc->dev; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_crtc_state *old_intel_state = > + to_intel_crtc_state(old_crtc_state); > + bool modeset = needs_modeset(crtc->state); > > if (intel_crtc->atomic.update_wm_pre) > intel_update_watermarks(crtc); > @@ -13456,7 +13462,12 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, > if (crtc->state->active) > intel_pipe_update_start(intel_crtc, &intel_crtc->start_vbl_count); > > - if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9) > + if (modeset) > + return; > + > + if (to_intel_crtc_state(crtc->state)->update_pipe) > + intel_update_pipe_config(intel_crtc, old_intel_state); > + else if (INTEL_INFO(dev)->gen >= 9) > skl_detach_scalers(intel_crtc); > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index cbe20250c901..1af5717e2e8a 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -333,6 +333,8 @@ struct intel_crtc_state { > #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS (1<<0) /* unreliable sync mode.flags */ > unsigned long quirks; > > + bool update_pipe; > + > /* Pipe source size (ie. panel fitter input size) > * All planes will be positioned inside this space, > * and get clipped at the edges. */ > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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
On Tue, Jul 21, 2015 at 04:32:26PM +0200, Maarten Lankhorst wrote: > 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? One part is just exercising the code, i.e. do various modesets from different modes to other modes to cover all the cases: - off->on - on->off - on->on but chaning upscaling. That would be really good already. Bonus points if you bother with an atomic version which checks that it's indeed done in one vblank (using timestamps and clearing the ALLOW_MODESET flag). But imo that's overkill. Most of the correctness prove should be possible by catching pipe underruns when we screw it up somehow. -Daniel
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) { 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); } - 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) @@ -4956,7 +4961,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; @@ -4964,7 +4969,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); @@ -4991,7 +4996,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); @@ -5054,7 +5059,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); @@ -5208,7 +5213,8 @@ static void modeset_update_crtc_power_domains(struct drm_atomic_state *state) int i; for_each_crtc_in_state(state, crtc, crtc_state, i) { - if (needs_modeset(crtc->state)) + if (needs_modeset(crtc->state) || + to_intel_crtc_state(crtc->state)->update_pipe) put_domains[to_intel_crtc(crtc)->pipe] = modeset_get_crtc_power_domains(crtc); } @@ -12180,7 +12186,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, @@ -12400,8 +12405,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); + } + } PIPE_CONF_CHECK_I(gmch_pfit.control); /* pfit ratios are autocomputed by the hw on gen4+ */ @@ -12409,12 +12422,6 @@ intel_pipe_config_compare(struct drm_device *dev, PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios); PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits); - 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); - } - PIPE_CONF_CHECK_I(scaler_state.scaler_id); /* BDW+ don't expose a synchronous way to read the state */ @@ -12877,7 +12884,6 @@ static int intel_modeset_all_pipes(struct drm_atomic_state *state) return ret; } - static int intel_modeset_checks(struct drm_atomic_state *state) { struct drm_device *dev = state->dev; @@ -12968,6 +12974,7 @@ static int intel_atomic_check(struct drm_device *dev, to_intel_crtc_state(crtc->state), pipe_config, true)) { crtc_state->mode_changed = false; + to_intel_crtc_state(crtc_state)->update_pipe = true; } if (needs_modeset(crtc_state)) { @@ -13426,10 +13433,6 @@ intel_commit_primary_plane(struct drm_plane *plane, if (!crtc->state->active) return; - if (state->visible) - /* FIXME: kill this fastboot hack */ - intel_update_pipe_size(intel_crtc); - dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y); } @@ -13448,6 +13451,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, { struct drm_device *dev = crtc->dev; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct intel_crtc_state *old_intel_state = + to_intel_crtc_state(old_crtc_state); + bool modeset = needs_modeset(crtc->state); if (intel_crtc->atomic.update_wm_pre) intel_update_watermarks(crtc); @@ -13456,7 +13462,12 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, if (crtc->state->active) intel_pipe_update_start(intel_crtc, &intel_crtc->start_vbl_count); - if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9) + if (modeset) + return; + + if (to_intel_crtc_state(crtc->state)->update_pipe) + intel_update_pipe_config(intel_crtc, old_intel_state); + else if (INTEL_INFO(dev)->gen >= 9) skl_detach_scalers(intel_crtc); } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index cbe20250c901..1af5717e2e8a 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -333,6 +333,8 @@ struct intel_crtc_state { #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS (1<<0) /* unreliable sync mode.flags */ unsigned long quirks; + bool update_pipe; + /* Pipe source size (ie. panel fitter input size) * All planes will be positioned inside this space, * and get clipped at the edges. */
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(-)