| Message ID | 1452515269-6221-4-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 11, 2016 at 01:27:43PM +0100, Maarten Lankhorst wrote: > This is a revert of commit 066cf55b9ce3 "drm/i915: Fix IPS related flicker". > intel_pre_disable_primary already handles this, and now everything > goes through the atomic path there's no need to try to disable ips twice. If it's a revert why wasn't it done with 'git revert'? Also I'm not sure it isn't a step backwards. Based on the spec we should be able to keep IPS enabled as long as one plane (possibly referring to either primary or sprite) is enabled on the pipe. So in theory we should keep the IPS handling out of the primary plane code, and instead we should compute the IPS state based on the set of active planes on the pipe. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> > --- > drivers/gpu/drm/i915/intel_display.c | 16 +--------------- > drivers/gpu/drm/i915/intel_drv.h | 1 - > 2 files changed, 1 insertion(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index eac73f005a70..62044ad5c6ec 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4823,9 +4823,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc) > if (atomic->disable_fbc) > intel_fbc_deactivate(crtc); > > - if (crtc->atomic.disable_ips) > - hsw_disable_ips(crtc); > - > if (atomic->pre_disable_primary) > intel_pre_disable_primary(&crtc->base); > > @@ -11907,19 +11904,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > intel_crtc->atomic.pre_disable_primary = turn_off; > intel_crtc->atomic.post_enable_primary = turn_on; > > - if (turn_off) { > - /* > - * FIXME: Actually if we will still have any other > - * plane enabled on the pipe we could let IPS enabled > - * still, but for now lets consider that when we make > - * primary invisible by setting DSPCNTR to 0 on > - * update_primary_plane function IPS needs to be > - * disable. > - */ > - intel_crtc->atomic.disable_ips = true; > - > + if (turn_off) > intel_crtc->atomic.disable_fbc = true; > - } > > /* > * FBC does not work on some platforms for rotated > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 8940aa4cf50c..39adf7cd0b36 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -566,7 +566,6 @@ struct intel_mmio_flip { > struct intel_crtc_atomic_commit { > /* Sleepable operations to perform before commit */ > bool disable_fbc; > - bool disable_ips; > bool pre_disable_primary; > > /* Sleepable operations to perform after commit */ > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 13-01-16 om 14:02 schreef Ville Syrjälä: > On Mon, Jan 11, 2016 at 01:27:43PM +0100, Maarten Lankhorst wrote: >> This is a revert of commit 066cf55b9ce3 "drm/i915: Fix IPS related flicker". >> intel_pre_disable_primary already handles this, and now everything >> goes through the atomic path there's no need to try to disable ips twice. > If it's a revert why wasn't it done with 'git revert'? A straight revert would fail to apply. > Also I'm not sure it isn't a step backwards. Based on the spec we should > be able to keep IPS enabled as long as one plane (possibly referring to > either primary or sprite) is enabled on the pipe. So in theory we should > keep the IPS handling out of the primary plane code, and instead we > should compute the IPS state based on the set of active planes on the > pipe. You're probably right. But pre_disable_primary has the following warning: * 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. Hence I'm not doing anything to support that. Even if I did it should be a separate commit.
Hi, On 18 January 2016 at 12:10, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > Op 13-01-16 om 14:02 schreef Ville Syrjälä: >> Also I'm not sure it isn't a step backwards. Based on the spec we should >> be able to keep IPS enabled as long as one plane (possibly referring to >> either primary or sprite) is enabled on the pipe. So in theory we should >> keep the IPS handling out of the primary plane code, and instead we >> should compute the IPS state based on the set of active planes on the >> pipe. > You're probably right. But pre_disable_primary has the following warning: > * 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. > > Hence I'm not doing anything to support that. Even if I did it should be a separate commit. Right. I don't think it's worth trying to bandaid the current IPS setup to deal with that, but instead - as per discussion on the last round of patches - moving from 'if we're doing thing X then bash Y into registers' to calculating desired targets based on the state given. Cheers, Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index eac73f005a70..62044ad5c6ec 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4823,9 +4823,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc) if (atomic->disable_fbc) intel_fbc_deactivate(crtc); - if (crtc->atomic.disable_ips) - hsw_disable_ips(crtc); - if (atomic->pre_disable_primary) intel_pre_disable_primary(&crtc->base); @@ -11907,19 +11904,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, intel_crtc->atomic.pre_disable_primary = turn_off; intel_crtc->atomic.post_enable_primary = turn_on; - if (turn_off) { - /* - * FIXME: Actually if we will still have any other - * plane enabled on the pipe we could let IPS enabled - * still, but for now lets consider that when we make - * primary invisible by setting DSPCNTR to 0 on - * update_primary_plane function IPS needs to be - * disable. - */ - intel_crtc->atomic.disable_ips = true; - + if (turn_off) intel_crtc->atomic.disable_fbc = true; - } /* * FBC does not work on some platforms for rotated diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 8940aa4cf50c..39adf7cd0b36 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -566,7 +566,6 @@ struct intel_mmio_flip { struct intel_crtc_atomic_commit { /* Sleepable operations to perform before commit */ bool disable_fbc; - bool disable_ips; bool pre_disable_primary; /* Sleepable operations to perform after commit */