| Message ID | 20171117153756.33558-3-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 17, 2017 at 04:37:55PM +0100, Maarten Lankhorst wrote: > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 8283e80597bd..38a1cdb3dbb2 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) > intel_fbc_post_update(crtc); > > if (primary_state->base.visible && > - (needs_modeset(&pipe_config->base) || > + (pipe_config->disable_cxsr || > !old_primary_state->base.visible)) > intel_post_enable_primary(&crtc->base, pipe_config); > } > @@ -5064,7 +5064,7 @@ 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) > + if (pipe_config->disable_cxsr || !pipe_config->ips_enabled) What does IPS have to do with cxsr? > hsw_disable_ips(old_crtc_state); > > if (old_pri_state) { > @@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv, > 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. > + * IPS should be fine as long as one plane is enabled, but > + * temporarily disable it when when going from primary only > + * to sprite only and vice versa. > */ > - if (visible_planes != BIT(PLANE_PRIMARY)) > + if (hweight32(visible_planes) != 1) > return false; That should just be if (active_planes == 0) return false; assuming we have no problems with the toggling between primary only and sprite only. I can't recall how the cursor affecrs IPS. But I think IPS should work as long as any plane (including the cursor) is enabled. > > /* HSW can handle pixel rate up to cdclk? */ > -- > 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:55 schreef Ville Syrjälä: > On Fri, Nov 17, 2017 at 04:37:55PM +0100, Maarten Lankhorst wrote: >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 13 ++++++------- >> 1 file changed, 6 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 8283e80597bd..38a1cdb3dbb2 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) >> intel_fbc_post_update(crtc); >> >> if (primary_state->base.visible && >> - (needs_modeset(&pipe_config->base) || >> + (pipe_config->disable_cxsr || >> !old_primary_state->base.visible)) >> intel_post_enable_primary(&crtc->base, pipe_config); >> } >> @@ -5064,7 +5064,7 @@ 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) >> + if (pipe_config->disable_cxsr || !pipe_config->ips_enabled) > What does IPS have to do with cxsr? Nothing, laziness. disable cxsr is set when planes get disabled. >> hsw_disable_ips(old_crtc_state); >> >> if (old_pri_state) { >> @@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv, >> 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. >> + * IPS should be fine as long as one plane is enabled, but >> + * temporarily disable it when when going from primary only >> + * to sprite only and vice versa. >> */ >> - if (visible_planes != BIT(PLANE_PRIMARY)) >> + if (hweight32(visible_planes) != 1) >> return false; > That should just be > if (active_planes == 0) > return false; > > assuming we have no problems with the toggling between > primary only and sprite only. > > I can't recall how the cursor affecrs IPS. But I think IPS should > work as long as any plane (including the cursor) is enabled. But cursor visibility can change without the full atomic commit, so it's too unreliable to take into account with the calculations. What happens when it doesn't work well? Would it be caught by any tests? ~Maarten
On Mon, Nov 20, 2017 at 12:12:40PM +0100, Maarten Lankhorst wrote: > Op 17-11-17 om 16:55 schreef Ville Syrjälä: > > On Fri, Nov 17, 2017 at 04:37:55PM +0100, Maarten Lankhorst wrote: > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 13 ++++++------- > >> 1 file changed, 6 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 8283e80597bd..38a1cdb3dbb2 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) > >> intel_fbc_post_update(crtc); > >> > >> if (primary_state->base.visible && > >> - (needs_modeset(&pipe_config->base) || > >> + (pipe_config->disable_cxsr || > >> !old_primary_state->base.visible)) > >> intel_post_enable_primary(&crtc->base, pipe_config); > >> } > >> @@ -5064,7 +5064,7 @@ 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) > >> + if (pipe_config->disable_cxsr || !pipe_config->ips_enabled) > > What does IPS have to do with cxsr? > Nothing, laziness. disable cxsr is set when planes get disabled. > >> hsw_disable_ips(old_crtc_state); > >> > >> if (old_pri_state) { > >> @@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv, > >> 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. > >> + * IPS should be fine as long as one plane is enabled, but > >> + * temporarily disable it when when going from primary only > >> + * to sprite only and vice versa. > >> */ > >> - if (visible_planes != BIT(PLANE_PRIMARY)) > >> + if (hweight32(visible_planes) != 1) > >> return false; > > That should just be > > if (active_planes == 0) > > return false; > > > > assuming we have no problems with the toggling between > > primary only and sprite only. > > > > I can't recall how the cursor affecrs IPS. But I think IPS should > > work as long as any plane (including the cursor) is enabled. > But cursor visibility can change without the full atomic commit, so it's too unreliable to take into > account with the calculations. Hmm. I guess we can ignore the cursor then. It's generally pretty small, so probably no huge benefit from using IPS with cursor only anyway. > > What happens when it doesn't work well? Would it be caught by any tests? I think it should be caught by CRCs, if we have any that capture CRCs across plane enable/disable. Except, there is a problem on ilk-ivb at least, maybe it went all the way to bdw. IIRC when I last tried a test like that the CRC for a black primary plane didn't match the CRC for no planes. And I think I tried to eliminate gamma/csc as the source of the discrepancy without success. Also IIRC there was always a single frame with an even stranger CRC just after turning off the last plane.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 8283e80597bd..38a1cdb3dbb2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) intel_fbc_post_update(crtc); if (primary_state->base.visible && - (needs_modeset(&pipe_config->base) || + (pipe_config->disable_cxsr || !old_primary_state->base.visible)) intel_post_enable_primary(&crtc->base, pipe_config); } @@ -5064,7 +5064,7 @@ 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) + if (pipe_config->disable_cxsr || !pipe_config->ips_enabled) hsw_disable_ips(old_crtc_state); if (old_pri_state) { @@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv, 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. + * IPS should be fine as long as one plane is enabled, but + * temporarily disable it when when going from primary only + * to sprite only and vice versa. */ - if (visible_planes != BIT(PLANE_PRIMARY)) + if (hweight32(visible_planes) != 1) return false; /* HSW can handle pixel rate up to cdclk? */
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)