| Message ID | 20171117153756.33558-2-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 17, 2017 at 04:37:54PM +0100, Maarten Lankhorst wrote: > ips_enabled was used as a variable of whether IPS can be enabled or not, > but should be used to test whether IPS is actually enabled. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 75 ++++++++++++++++------------------- > drivers/gpu/drm/i915/intel_pipe_crc.c | 2 - > 2 files changed, 35 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 3b3dec1e6640..8283e80597bd 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state) > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = to_i915(dev); > > - if (!crtc->config->ips_enabled) > + if (!crtc_state->ips_enabled) > return; > > /* > @@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc, > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > int pipe = intel_crtc->pipe; > > - /* > - * FIXME IPS should be fine as long as one plane is > - * enabled, but in practice it seems to have problems > - * when going from primary only to sprite only and vice > - * versa. > - */ > - hsw_enable_ips(new_crtc_state); > - > /* > * Gen2 reports pipe underruns whenever all planes are disabled. > * So don't enable underrun reporting before at least some planes > @@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc, > intel_check_pch_fifo_underruns(dev_priv); > } > > -/* FIXME move all this to pre_plane_update() with proper state tracking */ > +/* FIXME get rid of this and use pre_plane_update */ > static void > -intel_pre_disable_primary(struct drm_crtc *crtc, > - const struct intel_crtc_state *old_crtc_state) > +intel_pre_disable_primary_noatomic(struct drm_crtc *crtc) > { > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = to_i915(dev); > @@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc, > > /* > * Gen2 reports pipe underruns whenever all planes are disabled. > - * So diasble underrun reporting before all the planes get disabled. > - * FIXME: Need to fix the logic to work when we turn off all planes > - * but leave the pipe running. > + * So disable underrun reporting before all the planes get disabled. > */ > if (IS_GEN2(dev_priv)) > intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false); > > - /* > - * FIXME IPS should be fine as long as one plane is > - * enabled, but in practice it seems to have problems > - * when going from primary only to sprite only and vice > - * versa. > - */ > - hsw_disable_ips(old_crtc_state); > -} > - > -/* FIXME get rid of this and use pre_plane_update */ > -static void > -intel_pre_disable_primary_noatomic(struct drm_crtc *crtc) > -{ > - struct drm_device *dev = crtc->dev; > - struct drm_i915_private *dev_priv = to_i915(dev); > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - int pipe = intel_crtc->pipe; > - > - intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state)); > + hsw_disable_ips(to_intel_crtc_state(crtc->state)); BTW thinking about the lack of readout on BDW. In case the BIOS ever enables IPS, I think we should sanitize ips_enabled to true on BDW. Or maybe just have the redaout code always set ips_enabled=true on BDW? That way the first modeset will try to turn it off regardless of whether it was enabled or not. > > /* > * Vblank time updates from the shadow to live plane control register > @@ -5058,6 +5029,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) > if (pipe_config->update_wm_post && pipe_config->base.active) > intel_update_watermarks(crtc); > > + if (!old_crtc_state->ips_enabled || > + needs_modeset(&old_crtc_state->base) || Shoudldn't that be needs_modeset(new_crtc_state)? > + pipe_config->update_pipe) Why do we have to check that? > + hsw_enable_ips(pipe_config); > + > if (old_pri_state) { > struct intel_plane_state *primary_state = > intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state), > @@ -5088,6 +5064,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, > struct intel_atomic_state *old_intel_state = > to_intel_atomic_state(old_state); > > + if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled) > + hsw_disable_ips(old_crtc_state); > + > if (old_pri_state) { > struct intel_plane_state *primary_state = > intel_atomic_get_new_plane_state(old_intel_state, > @@ -5096,10 +5075,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, > to_intel_plane_state(old_pri_state); > > intel_fbc_pre_update(crtc, pipe_config, primary_state); > - > - if (old_primary_state->base.visible && > + /* > + * Gen2 reports pipe underruns whenever all planes are disabled. > + * So disable underrun reporting before all the planes get disabled. > + */ > + if (IS_GEN2(dev_priv) && old_primary_state->base.visible && > (modeset || !primary_state->base.visible)) > - intel_pre_disable_primary(&crtc->base, old_crtc_state); > + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false); > } > > /* > @@ -6231,12 +6213,25 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc, > static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv, > struct intel_crtc_state *pipe_config) > { > + u8 visible_planes; > + > if (pipe_config->ips_force_disable) > return false; > > if (pipe_config->pipe_bpp > 24) > return false; > > + visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR); > + > + /* > + * FIXME IPS should be fine as long as one plane is > + * enabled, but in practice it seems to have problems > + * when going from primary only to sprite only and vice > + * versa. > + */ > + if (visible_planes != BIT(PLANE_PRIMARY)) > + return false; I think that should be '(active_planes & PRIMARY) == 0'. As in we don't care what the other planes are doing, IPS just tracks the primary state (for the time being). What's missing from this patch is changing the cdclk vs. ips workaround to not assume that cdclk can be increased to accomodate ips. > + > /* HSW can handle pixel rate up to cdclk? */ > if (IS_HASWELL(dev_priv)) > return true; > @@ -6378,9 +6373,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, > > intel_crtc_compute_pixel_rate(pipe_config); > > - if (HAS_IPS(dev_priv)) > - hsw_compute_ips_config(crtc, pipe_config); > - > if (pipe_config->has_pch_encoder) > return ironlake_fdi_compute_config(crtc, pipe_config); > > @@ -10488,6 +10480,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, > pipe_config); > } > > + if (HAS_IPS(dev_priv)) > + hsw_compute_ips_config(intel_crtc, pipe_config); > + > return ret; > } > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c > index 61641d479b93..1f5cd572a7ff 100644 > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c > @@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv, > * completely disable it. > */ > pipe_config->ips_force_disable = enable; > - if (pipe_config->ips_enabled == enable) > - pipe_config->base.connectors_changed = true; > } > > if (IS_HASWELL(dev_priv)) { > -- > 2.15.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 17-11-17 om 16:52 schreef Ville Syrjälä: > On Fri, Nov 17, 2017 at 04:37:54PM +0100, Maarten Lankhorst wrote: >> ips_enabled was used as a variable of whether IPS can be enabled or not, >> but should be used to test whether IPS is actually enabled. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 75 ++++++++++++++++------------------- >> drivers/gpu/drm/i915/intel_pipe_crc.c | 2 - >> 2 files changed, 35 insertions(+), 42 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 3b3dec1e6640..8283e80597bd 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state) >> struct drm_device *dev = crtc->base.dev; >> struct drm_i915_private *dev_priv = to_i915(dev); >> >> - if (!crtc->config->ips_enabled) >> + if (!crtc_state->ips_enabled) >> return; >> >> /* >> @@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc, >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> int pipe = intel_crtc->pipe; >> >> - /* >> - * FIXME IPS should be fine as long as one plane is >> - * enabled, but in practice it seems to have problems >> - * when going from primary only to sprite only and vice >> - * versa. >> - */ >> - hsw_enable_ips(new_crtc_state); >> - >> /* >> * Gen2 reports pipe underruns whenever all planes are disabled. >> * So don't enable underrun reporting before at least some planes >> @@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc, >> intel_check_pch_fifo_underruns(dev_priv); >> } >> >> -/* FIXME move all this to pre_plane_update() with proper state tracking */ >> +/* FIXME get rid of this and use pre_plane_update */ >> static void >> -intel_pre_disable_primary(struct drm_crtc *crtc, >> - const struct intel_crtc_state *old_crtc_state) >> +intel_pre_disable_primary_noatomic(struct drm_crtc *crtc) >> { >> struct drm_device *dev = crtc->dev; >> struct drm_i915_private *dev_priv = to_i915(dev); >> @@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc, >> >> /* >> * Gen2 reports pipe underruns whenever all planes are disabled. >> - * So diasble underrun reporting before all the planes get disabled. >> - * FIXME: Need to fix the logic to work when we turn off all planes >> - * but leave the pipe running. >> + * So disable underrun reporting before all the planes get disabled. >> */ >> if (IS_GEN2(dev_priv)) >> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false); >> >> - /* >> - * FIXME IPS should be fine as long as one plane is >> - * enabled, but in practice it seems to have problems >> - * when going from primary only to sprite only and vice >> - * versa. >> - */ >> - hsw_disable_ips(old_crtc_state); >> -} >> - >> -/* FIXME get rid of this and use pre_plane_update */ >> -static void >> -intel_pre_disable_primary_noatomic(struct drm_crtc *crtc) >> -{ >> - struct drm_device *dev = crtc->dev; >> - struct drm_i915_private *dev_priv = to_i915(dev); >> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> - int pipe = intel_crtc->pipe; >> - >> - intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state)); >> + hsw_disable_ips(to_intel_crtc_state(crtc->state)); > BTW thinking about the lack of readout on BDW. In case the BIOS ever > enables IPS, I think we should sanitize ips_enabled to true on BDW. > Or maybe just have the redaout code always set ips_enabled=true on BDW? > That way the first modeset will try to turn it off regardless of whether it > was enabled or not. Yes, good idea. :) >> >> /* >> * Vblank time updates from the shadow to live plane control register >> @@ -5058,6 +5029,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) >> if (pipe_config->update_wm_post && pipe_config->base.active) >> intel_update_watermarks(crtc); >> >> + if (!old_crtc_state->ips_enabled || >> + needs_modeset(&old_crtc_state->base) || > Shoudldn't that be needs_modeset(new_crtc_state)? Yes, oops.. >> + pipe_config->update_pipe) > Why do we have to check that? In case we take over the hw state from bios.. If we default ips_enabled to true on BDW and read out on HSW again, then this check will make IPS work, regardless of bios setting.. >> + hsw_enable_ips(pipe_config); >> + >> if (old_pri_state) { >> struct intel_plane_state *primary_state = >> intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state), >> @@ -5088,6 +5064,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, >> struct intel_atomic_state *old_intel_state = >> to_intel_atomic_state(old_state); >> >> + if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled) >> + hsw_disable_ips(old_crtc_state); >> + >> if (old_pri_state) { >> struct intel_plane_state *primary_state = >> intel_atomic_get_new_plane_state(old_intel_state, >> @@ -5096,10 +5075,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, >> to_intel_plane_state(old_pri_state); >> >> intel_fbc_pre_update(crtc, pipe_config, primary_state); >> - >> - if (old_primary_state->base.visible && >> + /* >> + * Gen2 reports pipe underruns whenever all planes are disabled. >> + * So disable underrun reporting before all the planes get disabled. >> + */ >> + if (IS_GEN2(dev_priv) && old_primary_state->base.visible && >> (modeset || !primary_state->base.visible)) >> - intel_pre_disable_primary(&crtc->base, old_crtc_state); >> + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false); >> } >> >> /* >> @@ -6231,12 +6213,25 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc, >> static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv, >> struct intel_crtc_state *pipe_config) >> { >> + u8 visible_planes; >> + >> if (pipe_config->ips_force_disable) >> return false; >> >> if (pipe_config->pipe_bpp > 24) >> return false; >> >> + visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR); >> + >> + /* >> + * FIXME IPS should be fine as long as one plane is >> + * enabled, but in practice it seems to have problems >> + * when going from primary only to sprite only and vice >> + * versa. >> + */ >> + if (visible_planes != BIT(PLANE_PRIMARY)) >> + return false; > I think that should be '(active_planes & PRIMARY) == 0'. As in we don't > care what the other planes are doing, IPS just tracks the primary state > (for the time being). Oops, faultily assumed from the comment that IPS only worked with the primary plane or sprite plane, not both. I'll correct it. > What's missing from this patch is changing the cdclk vs. ips workaround > to not assume that cdclk can be increased to accomodate ips. Hm true, I missed that.. Will send a new version in a bit. >> + >> /* HSW can handle pixel rate up to cdclk? */ >> if (IS_HASWELL(dev_priv)) >> return true; >> @@ -6378,9 +6373,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, >> >> intel_crtc_compute_pixel_rate(pipe_config); >> >> - if (HAS_IPS(dev_priv)) >> - hsw_compute_ips_config(crtc, pipe_config); >> - >> if (pipe_config->has_pch_encoder) >> return ironlake_fdi_compute_config(crtc, pipe_config); >> >> @@ -10488,6 +10480,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, >> pipe_config); >> } >> >> + if (HAS_IPS(dev_priv)) >> + hsw_compute_ips_config(intel_crtc, pipe_config); >> + >> return ret; >> } >> >> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c >> index 61641d479b93..1f5cd572a7ff 100644 >> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c >> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c >> @@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv, >> * completely disable it. >> */ >> pipe_config->ips_force_disable = enable; >> - if (pipe_config->ips_enabled == enable) >> - pipe_config->base.connectors_changed = true; >> } >> >> if (IS_HASWELL(dev_priv)) { >> -- >> 2.15.0 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Nov 20, 2017 at 11:48:22AM +0100, Maarten Lankhorst wrote: > Op 17-11-17 om 16:52 schreef Ville Syrjälä: > > On Fri, Nov 17, 2017 at 04:37:54PM +0100, Maarten Lankhorst wrote: > >> ips_enabled was used as a variable of whether IPS can be enabled or not, > >> but should be used to test whether IPS is actually enabled. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 75 ++++++++++++++++------------------- > >> drivers/gpu/drm/i915/intel_pipe_crc.c | 2 - > >> 2 files changed, 35 insertions(+), 42 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 3b3dec1e6640..8283e80597bd 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state) > >> struct drm_device *dev = crtc->base.dev; > >> struct drm_i915_private *dev_priv = to_i915(dev); > >> > >> - if (!crtc->config->ips_enabled) > >> + if (!crtc_state->ips_enabled) > >> return; > >> > >> /* > >> @@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc, > >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > >> int pipe = intel_crtc->pipe; > >> > >> - /* > >> - * FIXME IPS should be fine as long as one plane is > >> - * enabled, but in practice it seems to have problems > >> - * when going from primary only to sprite only and vice > >> - * versa. > >> - */ > >> - hsw_enable_ips(new_crtc_state); > >> - > >> /* > >> * Gen2 reports pipe underruns whenever all planes are disabled. > >> * So don't enable underrun reporting before at least some planes > >> @@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc, > >> intel_check_pch_fifo_underruns(dev_priv); > >> } > >> > >> -/* FIXME move all this to pre_plane_update() with proper state tracking */ > >> +/* FIXME get rid of this and use pre_plane_update */ > >> static void > >> -intel_pre_disable_primary(struct drm_crtc *crtc, > >> - const struct intel_crtc_state *old_crtc_state) > >> +intel_pre_disable_primary_noatomic(struct drm_crtc *crtc) > >> { > >> struct drm_device *dev = crtc->dev; > >> struct drm_i915_private *dev_priv = to_i915(dev); > >> @@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc, > >> > >> /* > >> * Gen2 reports pipe underruns whenever all planes are disabled. > >> - * So diasble underrun reporting before all the planes get disabled. > >> - * FIXME: Need to fix the logic to work when we turn off all planes > >> - * but leave the pipe running. > >> + * So disable underrun reporting before all the planes get disabled. > >> */ > >> if (IS_GEN2(dev_priv)) > >> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false); > >> > >> - /* > >> - * FIXME IPS should be fine as long as one plane is > >> - * enabled, but in practice it seems to have problems > >> - * when going from primary only to sprite only and vice > >> - * versa. > >> - */ > >> - hsw_disable_ips(old_crtc_state); > >> -} > >> - > >> -/* FIXME get rid of this and use pre_plane_update */ > >> -static void > >> -intel_pre_disable_primary_noatomic(struct drm_crtc *crtc) > >> -{ > >> - struct drm_device *dev = crtc->dev; > >> - struct drm_i915_private *dev_priv = to_i915(dev); > >> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > >> - int pipe = intel_crtc->pipe; > >> - > >> - intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state)); > >> + hsw_disable_ips(to_intel_crtc_state(crtc->state)); > > BTW thinking about the lack of readout on BDW. In case the BIOS ever > > enables IPS, I think we should sanitize ips_enabled to true on BDW. > > Or maybe just have the redaout code always set ips_enabled=true on BDW? > > That way the first modeset will try to turn it off regardless of whether it > > was enabled or not. > Yes, good idea. :) > >> > >> /* > >> * Vblank time updates from the shadow to live plane control register > >> @@ -5058,6 +5029,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) > >> if (pipe_config->update_wm_post && pipe_config->base.active) > >> intel_update_watermarks(crtc); > >> > >> + if (!old_crtc_state->ips_enabled || > >> + needs_modeset(&old_crtc_state->base) || > > Shoudldn't that be needs_modeset(new_crtc_state)? > Yes, oops.. > >> + pipe_config->update_pipe) > > Why do we have to check that? > In case we take over the hw state from bios.. > > If we default ips_enabled to true on BDW and read out on HSW again, then this > check will make IPS work, regardless of bios setting.. Why aren't we just checking something like if ((!old_state->ips_enabled || needs_modeset()) && new_state->ips_enabled) ? IPS state may have to change whenever plane visibility changes, so I don't see any reson to involve ->update_pipe in these decision. > > >> + hsw_enable_ips(pipe_config); > >> + > >> if (old_pri_state) { > >> struct intel_plane_state *primary_state = > >> intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state), > >> @@ -5088,6 +5064,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, > >> struct intel_atomic_state *old_intel_state = > >> to_intel_atomic_state(old_state); > >> > >> + if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled) > >> + hsw_disable_ips(old_crtc_state); > >> + > >> if (old_pri_state) { > >> struct intel_plane_state *primary_state = > >> intel_atomic_get_new_plane_state(old_intel_state, > >> @@ -5096,10 +5075,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, > >> to_intel_plane_state(old_pri_state); > >> > >> intel_fbc_pre_update(crtc, pipe_config, primary_state); > >> - > >> - if (old_primary_state->base.visible && > >> + /* > >> + * Gen2 reports pipe underruns whenever all planes are disabled. > >> + * So disable underrun reporting before all the planes get disabled. > >> + */ > >> + if (IS_GEN2(dev_priv) && old_primary_state->base.visible && > >> (modeset || !primary_state->base.visible)) > >> - intel_pre_disable_primary(&crtc->base, old_crtc_state); > >> + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false); > >> } > >> > >> /* > >> @@ -6231,12 +6213,25 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc, > >> static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv, > >> struct intel_crtc_state *pipe_config) > >> { > >> + u8 visible_planes; > >> + > >> if (pipe_config->ips_force_disable) > >> return false; > >> > >> if (pipe_config->pipe_bpp > 24) > >> return false; > >> > >> + visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR); > >> + > >> + /* > >> + * FIXME IPS should be fine as long as one plane is > >> + * enabled, but in practice it seems to have problems > >> + * when going from primary only to sprite only and vice > >> + * versa. > >> + */ > >> + if (visible_planes != BIT(PLANE_PRIMARY)) > >> + return false; > > I think that should be '(active_planes & PRIMARY) == 0'. As in we don't > > care what the other planes are doing, IPS just tracks the primary state > > (for the time being). > Oops, faultily assumed from the comment that IPS only worked with the primary plane > or sprite plane, not both. I'll correct it. > > What's missing from this patch is changing the cdclk vs. ips workaround > > to not assume that cdclk can be increased to accomodate ips. > Hm true, I missed that.. > > Will send a new version in a bit. > >> + > >> /* HSW can handle pixel rate up to cdclk? */ > >> if (IS_HASWELL(dev_priv)) > >> return true; > >> @@ -6378,9 +6373,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, > >> > >> intel_crtc_compute_pixel_rate(pipe_config); > >> > >> - if (HAS_IPS(dev_priv)) > >> - hsw_compute_ips_config(crtc, pipe_config); > >> - > >> if (pipe_config->has_pch_encoder) > >> return ironlake_fdi_compute_config(crtc, pipe_config); > >> > >> @@ -10488,6 +10480,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, > >> pipe_config); > >> } > >> > >> + if (HAS_IPS(dev_priv)) > >> + hsw_compute_ips_config(intel_crtc, pipe_config); > >> + > >> return ret; > >> } > >> > >> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c > >> index 61641d479b93..1f5cd572a7ff 100644 > >> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c > >> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c > >> @@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv, > >> * completely disable it. > >> */ > >> pipe_config->ips_force_disable = enable; > >> - if (pipe_config->ips_enabled == enable) > >> - pipe_config->base.connectors_changed = true; > >> } > >> > >> if (IS_HASWELL(dev_priv)) { > >> -- > >> 2.15.0 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
Op 20-11-17 om 11:58 schreef Ville Syrjälä: > On Mon, Nov 20, 2017 at 11:48:22AM +0100, Maarten Lankhorst wrote: >> Op 17-11-17 om 16:52 schreef Ville Syrjälä: >>> On Fri, Nov 17, 2017 at 04:37:54PM +0100, Maarten Lankhorst wrote: >>>> ips_enabled was used as a variable of whether IPS can be enabled or not, >>>> but should be used to test whether IPS is actually enabled. >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_display.c | 75 ++++++++++++++++------------------- >>>> drivers/gpu/drm/i915/intel_pipe_crc.c | 2 - >>>> 2 files changed, 35 insertions(+), 42 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>> index 3b3dec1e6640..8283e80597bd 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state) >>>> struct drm_device *dev = crtc->base.dev; >>>> struct drm_i915_private *dev_priv = to_i915(dev); >>>> >>>> - if (!crtc->config->ips_enabled) >>>> + if (!crtc_state->ips_enabled) >>>> return; >>>> >>>> /* >>>> @@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc, >>>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >>>> int pipe = intel_crtc->pipe; >>>> >>>> - /* >>>> - * FIXME IPS should be fine as long as one plane is >>>> - * enabled, but in practice it seems to have problems >>>> - * when going from primary only to sprite only and vice >>>> - * versa. >>>> - */ >>>> - hsw_enable_ips(new_crtc_state); >>>> - >>>> /* >>>> * Gen2 reports pipe underruns whenever all planes are disabled. >>>> * So don't enable underrun reporting before at least some planes >>>> @@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc, >>>> intel_check_pch_fifo_underruns(dev_priv); >>>> } >>>> >>>> -/* FIXME move all this to pre_plane_update() with proper state tracking */ >>>> +/* FIXME get rid of this and use pre_plane_update */ >>>> static void >>>> -intel_pre_disable_primary(struct drm_crtc *crtc, >>>> - const struct intel_crtc_state *old_crtc_state) >>>> +intel_pre_disable_primary_noatomic(struct drm_crtc *crtc) >>>> { >>>> struct drm_device *dev = crtc->dev; >>>> struct drm_i915_private *dev_priv = to_i915(dev); >>>> @@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc, >>>> >>>> /* >>>> * Gen2 reports pipe underruns whenever all planes are disabled. >>>> - * So diasble underrun reporting before all the planes get disabled. >>>> - * FIXME: Need to fix the logic to work when we turn off all planes >>>> - * but leave the pipe running. >>>> + * So disable underrun reporting before all the planes get disabled. >>>> */ >>>> if (IS_GEN2(dev_priv)) >>>> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false); >>>> >>>> - /* >>>> - * FIXME IPS should be fine as long as one plane is >>>> - * enabled, but in practice it seems to have problems >>>> - * when going from primary only to sprite only and vice >>>> - * versa. >>>> - */ >>>> - hsw_disable_ips(old_crtc_state); >>>> -} >>>> - >>>> -/* FIXME get rid of this and use pre_plane_update */ >>>> -static void >>>> -intel_pre_disable_primary_noatomic(struct drm_crtc *crtc) >>>> -{ >>>> - struct drm_device *dev = crtc->dev; >>>> - struct drm_i915_private *dev_priv = to_i915(dev); >>>> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >>>> - int pipe = intel_crtc->pipe; >>>> - >>>> - intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state)); >>>> + hsw_disable_ips(to_intel_crtc_state(crtc->state)); >>> BTW thinking about the lack of readout on BDW. In case the BIOS ever >>> enables IPS, I think we should sanitize ips_enabled to true on BDW. >>> Or maybe just have the redaout code always set ips_enabled=true on BDW? >>> That way the first modeset will try to turn it off regardless of whether it >>> was enabled or not. >> Yes, good idea. :) >>>> >>>> /* >>>> * Vblank time updates from the shadow to live plane control register >>>> @@ -5058,6 +5029,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) >>>> if (pipe_config->update_wm_post && pipe_config->base.active) >>>> intel_update_watermarks(crtc); >>>> >>>> + if (!old_crtc_state->ips_enabled || >>>> + needs_modeset(&old_crtc_state->base) || >>> Shoudldn't that be needs_modeset(new_crtc_state)? >> Yes, oops.. >>>> + pipe_config->update_pipe) >>> Why do we have to check that? >> In case we take over the hw state from bios.. >> >> If we default ips_enabled to true on BDW and read out on HSW again, then this >> check will make IPS work, regardless of bios setting.. > Why aren't we just checking something like > if ((!old_state->ips_enabled || needs_modeset()) && new_state->ips_enabled) ? > > IPS state may have to change whenever plane visibility changes, so I don't > see any reson to involve ->update_pipe in these decision. This is what we do, except the new_state->ips_enabled check is in hsw_enable_ips. We also allow update_pipe for broadwell, in which case we don't know the hw state. ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3b3dec1e6640..8283e80597bd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4870,7 +4870,7 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state) struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = to_i915(dev); - if (!crtc->config->ips_enabled) + if (!crtc_state->ips_enabled) return; /* @@ -4966,14 +4966,6 @@ intel_post_enable_primary(struct drm_crtc *crtc, struct intel_crtc *intel_crtc = to_intel_crtc(crtc); int pipe = intel_crtc->pipe; - /* - * FIXME IPS should be fine as long as one plane is - * enabled, but in practice it seems to have problems - * when going from primary only to sprite only and vice - * versa. - */ - hsw_enable_ips(new_crtc_state); - /* * Gen2 reports pipe underruns whenever all planes are disabled. * So don't enable underrun reporting before at least some planes @@ -4989,10 +4981,9 @@ intel_post_enable_primary(struct drm_crtc *crtc, intel_check_pch_fifo_underruns(dev_priv); } -/* FIXME move all this to pre_plane_update() with proper state tracking */ +/* FIXME get rid of this and use pre_plane_update */ static void -intel_pre_disable_primary(struct drm_crtc *crtc, - const struct intel_crtc_state *old_crtc_state) +intel_pre_disable_primary_noatomic(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = to_i915(dev); @@ -5001,32 +4992,12 @@ intel_pre_disable_primary(struct drm_crtc *crtc, /* * Gen2 reports pipe underruns whenever all planes are disabled. - * So diasble underrun reporting before all the planes get disabled. - * FIXME: Need to fix the logic to work when we turn off all planes - * but leave the pipe running. + * So disable underrun reporting before all the planes get disabled. */ if (IS_GEN2(dev_priv)) intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false); - /* - * FIXME IPS should be fine as long as one plane is - * enabled, but in practice it seems to have problems - * when going from primary only to sprite only and vice - * versa. - */ - hsw_disable_ips(old_crtc_state); -} - -/* FIXME get rid of this and use pre_plane_update */ -static void -intel_pre_disable_primary_noatomic(struct drm_crtc *crtc) -{ - struct drm_device *dev = crtc->dev; - struct drm_i915_private *dev_priv = to_i915(dev); - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - int pipe = intel_crtc->pipe; - - intel_pre_disable_primary(crtc, to_intel_crtc_state(crtc->state)); + hsw_disable_ips(to_intel_crtc_state(crtc->state)); /* * Vblank time updates from the shadow to live plane control register @@ -5058,6 +5029,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) if (pipe_config->update_wm_post && pipe_config->base.active) intel_update_watermarks(crtc); + if (!old_crtc_state->ips_enabled || + needs_modeset(&old_crtc_state->base) || + pipe_config->update_pipe) + hsw_enable_ips(pipe_config); + if (old_pri_state) { struct intel_plane_state *primary_state = intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state), @@ -5088,6 +5064,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, struct intel_atomic_state *old_intel_state = to_intel_atomic_state(old_state); + if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled) + hsw_disable_ips(old_crtc_state); + if (old_pri_state) { struct intel_plane_state *primary_state = intel_atomic_get_new_plane_state(old_intel_state, @@ -5096,10 +5075,13 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, to_intel_plane_state(old_pri_state); intel_fbc_pre_update(crtc, pipe_config, primary_state); - - if (old_primary_state->base.visible && + /* + * Gen2 reports pipe underruns whenever all planes are disabled. + * So disable underrun reporting before all the planes get disabled. + */ + if (IS_GEN2(dev_priv) && old_primary_state->base.visible && (modeset || !primary_state->base.visible)) - intel_pre_disable_primary(&crtc->base, old_crtc_state); + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false); } /* @@ -6231,12 +6213,25 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc, static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv, struct intel_crtc_state *pipe_config) { + u8 visible_planes; + if (pipe_config->ips_force_disable) return false; if (pipe_config->pipe_bpp > 24) return false; + visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR); + + /* + * FIXME IPS should be fine as long as one plane is + * enabled, but in practice it seems to have problems + * when going from primary only to sprite only and vice + * versa. + */ + if (visible_planes != BIT(PLANE_PRIMARY)) + return false; + /* HSW can handle pixel rate up to cdclk? */ if (IS_HASWELL(dev_priv)) return true; @@ -6378,9 +6373,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, intel_crtc_compute_pixel_rate(pipe_config); - if (HAS_IPS(dev_priv)) - hsw_compute_ips_config(crtc, pipe_config); - if (pipe_config->has_pch_encoder) return ironlake_fdi_compute_config(crtc, pipe_config); @@ -10488,6 +10480,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, pipe_config); } + if (HAS_IPS(dev_priv)) + hsw_compute_ips_config(intel_crtc, pipe_config); + return ret; } diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c index 61641d479b93..1f5cd572a7ff 100644 --- a/drivers/gpu/drm/i915/intel_pipe_crc.c +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c @@ -541,8 +541,6 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv, * completely disable it. */ pipe_config->ips_force_disable = enable; - if (pipe_config->ips_enabled == enable) - pipe_config->base.connectors_changed = true; } if (IS_HASWELL(dev_priv)) {
ips_enabled was used as a variable of whether IPS can be enabled or not, but should be used to test whether IPS is actually enabled. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 75 ++++++++++++++++------------------- drivers/gpu/drm/i915/intel_pipe_crc.c | 2 - 2 files changed, 35 insertions(+), 42 deletions(-)