Re: [Intel-gfx] [PATCH] drm/i915: Postpone plane readout until after encoder readout, v3.
Op 13-10-15 om 14:40 schreef Jani Nikula: > On Thu, 27 Aug 2015, Maarten Lankhorst > wrote: >> When reading out hw state for planes we disable inactive planes which in >> turn triggers an update of the watermarks. The update depends on the >> crtc_clock being set which is done when reading out encoders. Thus >> postpone the plane readout until after encoder readout. >> >> This prevents a warning in skl_compute_linetime_wm() where pixel_rate >> becomes 0 when crtc_clock is 0. >> >> Signed-off-by: Patrik Jakobsson >> Signed-off-by: Maarten Lankhorst >> References: https://bugs.freedesktop.org/show_bug.cgi?id=91428 > Maarten, I presume this patch is superseded by the commits referenced in > that bug. Is that correct? > Correct. :) ~Maarten ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Postpone plane readout until after encoder readout, v3.
On Thu, 27 Aug 2015, Maarten Lankhorst wrote: > When reading out hw state for planes we disable inactive planes which in > turn triggers an update of the watermarks. The update depends on the > crtc_clock being set which is done when reading out encoders. Thus > postpone the plane readout until after encoder readout. > > This prevents a warning in skl_compute_linetime_wm() where pixel_rate > becomes 0 when crtc_clock is 0. > > Signed-off-by: Patrik Jakobsson > Signed-off-by: Maarten Lankhorst > References: https://bugs.freedesktop.org/show_bug.cgi?id=91428 Maarten, I presume this patch is superseded by the commits referenced in that bug. Is that correct? BR, Jani. > --- > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index e3922d973db0..118b205e7bd5 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15033,30 +15033,6 @@ static bool primary_get_hw_state(struct intel_crtc > *crtc) > return !!(I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE); > } > > -static void readout_plane_state(struct intel_crtc *crtc, > - struct intel_crtc_state *crtc_state) > -{ > - struct intel_plane *p; > - struct intel_plane_state *plane_state; > - bool active = crtc_state->base.active; > - > - for_each_intel_plane(crtc->base.dev, p) { > - if (crtc->pipe != p->pipe) > - continue; > - > - plane_state = to_intel_plane_state(p->base.state); > - > - if (p->base.type == DRM_PLANE_TYPE_PRIMARY) > - plane_state->visible = primary_get_hw_state(crtc); > - else { > - if (active) > - p->disable_plane(&p->base, &crtc->base); > - > - plane_state->visible = false; > - } > - } > -} > - > static void intel_modeset_readout_hw_state(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -15076,35 +15052,8 @@ static void intel_modeset_readout_hw_state(struct > drm_device *dev) > > crtc->base.state->active = crtc->active; > crtc->base.enabled = crtc->active; > - > - memset(&crtc->base.mode, 0, sizeof(crtc->base.mode)); > - if (crtc->base.state->active) { > - intel_mode_from_pipe_config(&crtc->base.mode, > crtc->config); > - > intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config); > - WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, > &crtc->base.mode)); > - > - /* > - * The initial mode needs to be set in order to keep > - * the atomic core happy. It wants a valid mode if the > - * crtc's enabled, so we do the above call. > - * > - * At this point some state updated by the connectors > - * in their ->detect() callback has not run yet, so > - * no recalculation can be done yet. > - * > - * Even if we could do a recalculation and modeset > - * right now it would cause a double modeset if > - * fbdev or userspace chooses a different initial mode. > - * > - * If that happens, someone indicated they wanted a > - * mode change, which means it's safe to do a full > - * recalculation. > - */ > - crtc->base.state->mode.private_flags = > I915_MODE_FLAG_INHERITED; > - } > - > - crtc->base.hwmode = crtc->config->base.adjusted_mode; > - readout_plane_state(crtc, > to_intel_crtc_state(crtc->base.state)); > + to_intel_plane_state(crtc->base.primary->state)->visible = > + primary_get_hw_state(crtc); > > DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n", > crtc->base.base.id, > @@ -15186,6 +15135,33 @@ intel_modeset_setup_hw_state(struct drm_device *dev) > > for_each_pipe(dev_priv, pipe) { > crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > + crtc->base.hwmode = crtc->config->base.adjusted_mode; > + memset(&crtc->base.mode, 0, sizeof(crtc->base.mode)); > + if (crtc->base.state->active) { > + intel_mode_from_pipe_config(&crtc->base.mode, > crtc->config); > + > intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config); > + WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, > &crtc->base.mode)); > + > + /* > + * The initial mode needs to be set in order to keep > +
Re: [Intel-gfx] [PATCH] drm/i915: Postpone plane readout until after encoder readout, v3.
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 7272 -Summary- Platform Delta drm-intel-nightly Series Applied ILK -1 302/302 301/302 SNB 315/315 315/315 IVB 336/336 336/336 BYT -1 283/283 282/283 HSW 378/378 378/378 -Detailed- Platform Testdrm-intel-nightly Series Applied *ILK igt@kms_flip@wf_vblank-vs-modeset-interruptible PASS(1) DMESG_WARN(1) *BYT igt@gem_tiled_partial_pwrite_pread@reads PASS(1) FAIL(1) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Postpone plane readout until after encoder readout, v3.
On Thu, Aug 27, 2015 at 01:47:26PM +0200, Maarten Lankhorst wrote: > When reading out hw state for planes we disable inactive planes which in > turn triggers an update of the watermarks. The update depends on the > crtc_clock being set which is done when reading out encoders. Thus > postpone the plane readout until after encoder readout. > > This prevents a warning in skl_compute_linetime_wm() where pixel_rate > becomes 0 when crtc_clock is 0. > > Signed-off-by: Patrik Jakobsson > Signed-off-by: Maarten Lankhorst > References: https://bugs.freedesktop.org/show_bug.cgi?id=91428 > --- > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index e3922d973db0..118b205e7bd5 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15033,30 +15033,6 @@ static bool primary_get_hw_state(struct intel_crtc > *crtc) > return !!(I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE); > } > > -static void readout_plane_state(struct intel_crtc *crtc, > - struct intel_crtc_state *crtc_state) > -{ > - struct intel_plane *p; > - struct intel_plane_state *plane_state; > - bool active = crtc_state->base.active; > - > - for_each_intel_plane(crtc->base.dev, p) { > - if (crtc->pipe != p->pipe) > - continue; > - > - plane_state = to_intel_plane_state(p->base.state); > - > - if (p->base.type == DRM_PLANE_TYPE_PRIMARY) > - plane_state->visible = primary_get_hw_state(crtc); > - else { > - if (active) > - p->disable_plane(&p->base, &crtc->base); > - > - plane_state->visible = false; > - } > - } > -} > - > static void intel_modeset_readout_hw_state(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -15076,35 +15052,8 @@ static void intel_modeset_readout_hw_state(struct > drm_device *dev) > > crtc->base.state->active = crtc->active; > crtc->base.enabled = crtc->active; > - > - memset(&crtc->base.mode, 0, sizeof(crtc->base.mode)); > - if (crtc->base.state->active) { > - intel_mode_from_pipe_config(&crtc->base.mode, > crtc->config); > - > intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config); > - WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, > &crtc->base.mode)); > - > - /* > - * The initial mode needs to be set in order to keep > - * the atomic core happy. It wants a valid mode if the > - * crtc's enabled, so we do the above call. > - * > - * At this point some state updated by the connectors > - * in their ->detect() callback has not run yet, so > - * no recalculation can be done yet. > - * > - * Even if we could do a recalculation and modeset > - * right now it would cause a double modeset if > - * fbdev or userspace chooses a different initial mode. > - * > - * If that happens, someone indicated they wanted a > - * mode change, which means it's safe to do a full > - * recalculation. > - */ > - crtc->base.state->mode.private_flags = > I915_MODE_FLAG_INHERITED; > - } > - > - crtc->base.hwmode = crtc->config->base.adjusted_mode; > - readout_plane_state(crtc, > to_intel_crtc_state(crtc->base.state)); > + to_intel_plane_state(crtc->base.primary->state)->visible = > + primary_get_hw_state(crtc); The commit message is for a totally different patch it seems. Hmm. Looking at this, I should have just moved the entire mode setup block in my hwmode fix patch to the end of the function, and things would be fixed. > > DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n", > crtc->base.base.id, > @@ -15186,6 +15135,33 @@ intel_modeset_setup_hw_state(struct drm_device *dev) > > for_each_pipe(dev_priv, pipe) { > crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > + crtc->base.hwmode = crtc->config->base.adjusted_mode; > + memset(&crtc->base.mode, 0, sizeof(crtc->base.mode)); > + if (crtc->base.state->active) { > + intel_mode_from_pipe_config(&crtc->base.mode, > crtc->config); > + > intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config); > + WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, > &crtc->base.mode)); > +
[Intel-gfx] [PATCH] drm/i915: Postpone plane readout until after encoder readout, v3.
When reading out hw state for planes we disable inactive planes which in turn triggers an update of the watermarks. The update depends on the crtc_clock being set which is done when reading out encoders. Thus postpone the plane readout until after encoder readout. This prevents a warning in skl_compute_linetime_wm() where pixel_rate becomes 0 when crtc_clock is 0. Signed-off-by: Patrik Jakobsson Signed-off-by: Maarten Lankhorst References: https://bugs.freedesktop.org/show_bug.cgi?id=91428 --- diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e3922d973db0..118b205e7bd5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15033,30 +15033,6 @@ static bool primary_get_hw_state(struct intel_crtc *crtc) return !!(I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE); } -static void readout_plane_state(struct intel_crtc *crtc, - struct intel_crtc_state *crtc_state) -{ - struct intel_plane *p; - struct intel_plane_state *plane_state; - bool active = crtc_state->base.active; - - for_each_intel_plane(crtc->base.dev, p) { - if (crtc->pipe != p->pipe) - continue; - - plane_state = to_intel_plane_state(p->base.state); - - if (p->base.type == DRM_PLANE_TYPE_PRIMARY) - plane_state->visible = primary_get_hw_state(crtc); - else { - if (active) - p->disable_plane(&p->base, &crtc->base); - - plane_state->visible = false; - } - } -} - static void intel_modeset_readout_hw_state(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -15076,35 +15052,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active; - - memset(&crtc->base.mode, 0, sizeof(crtc->base.mode)); - if (crtc->base.state->active) { - intel_mode_from_pipe_config(&crtc->base.mode, crtc->config); - intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config); - WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode)); - - /* -* The initial mode needs to be set in order to keep -* the atomic core happy. It wants a valid mode if the -* crtc's enabled, so we do the above call. -* -* At this point some state updated by the connectors -* in their ->detect() callback has not run yet, so -* no recalculation can be done yet. -* -* Even if we could do a recalculation and modeset -* right now it would cause a double modeset if -* fbdev or userspace chooses a different initial mode. -* -* If that happens, someone indicated they wanted a -* mode change, which means it's safe to do a full -* recalculation. -*/ - crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED; - } - - crtc->base.hwmode = crtc->config->base.adjusted_mode; - readout_plane_state(crtc, to_intel_crtc_state(crtc->base.state)); + to_intel_plane_state(crtc->base.primary->state)->visible = + primary_get_hw_state(crtc); DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n", crtc->base.base.id, @@ -15186,6 +15135,33 @@ intel_modeset_setup_hw_state(struct drm_device *dev) for_each_pipe(dev_priv, pipe) { crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); + crtc->base.hwmode = crtc->config->base.adjusted_mode; + memset(&crtc->base.mode, 0, sizeof(crtc->base.mode)); + if (crtc->base.state->active) { + intel_mode_from_pipe_config(&crtc->base.mode, crtc->config); + intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config); + WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode)); + + /* +* The initial mode needs to be set in order to keep +* the atomic core happy. It wants a valid mode if the +* crtc's enabled, so we do the above call. +* +* At this point some state updated by the connectors +*
Re: [Intel-gfx] [PATCH] drm/i915: Postpone plane readout until after encoder readout
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 6911 -Summary- Platform Delta drm-intel-nightly Series Applied ILK -1 302/302 301/302 SNB 315/315 315/315 IVB 336/336 336/336 BYT -2 283/283 281/283 HSW 378/378 378/378 -Detailed- Platform Testdrm-intel-nightly Series Applied *ILK igt@kms_flip@flip-vs-dpms-interruptible PASS(1) DMESG_WARN(1) *BYT igt@gem_partial_pwrite_pread@reads-uncached PASS(1) FAIL(1) *BYT igt@gem_tiled_partial_pwrite_pread@reads PASS(1) FAIL(1) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Postpone plane readout until after encoder readout
On Mon, Aug 3, 2015 at 4:36 PM, Maarten Lankhorst wrote: > Hey, > > Op 31-07-15 om 15:04 schreef Patrik Jakobsson: >> When reading out hw state for planes we disable inactive planes which in >> turn triggers an update of the watermarks. The update depends on the >> crtc_clock being set which is done when reading out encoders. Thus >> postpone the plane readout until after encoder readout. >> >> This prevents a warning in skl_compute_linetime_wm() where pixel_rate >> becomes 0 when crtc_clock is 0. >> > Running this on skylake it looks like like it's a partial fix. > You would need to move hwmode like below too? > > Also: > > References: https://bugs.freedesktop.org/show_bug.cgi?id=91428 Ah yes, I'll send a v2. Thanks. > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index add7f807a82d..49907f4e939a 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14888,8 +14888,6 @@ static void intel_modeset_readout_hw_state(struct > drm_device *dev) > crtc->base.state->mode.private_flags = > I915_MODE_FLAG_INHERITED; > } > > - crtc->base.hwmode = crtc->config->base.adjusted_mode; > - > DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n", > crtc->base.base.id, > crtc->active ? "enabled" : "disabled"); > @@ -14971,6 +14969,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev) > > for_each_pipe(dev_priv, pipe) { > crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > + crtc->base.hwmode = crtc->config->base.adjusted_mode; > > for_each_intel_plane(crtc->base.dev, plane) { > if (crtc->pipe != plane->pipe) > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Postpone plane readout until after encoder readout
Hey, Op 31-07-15 om 15:04 schreef Patrik Jakobsson: > When reading out hw state for planes we disable inactive planes which in > turn triggers an update of the watermarks. The update depends on the > crtc_clock being set which is done when reading out encoders. Thus > postpone the plane readout until after encoder readout. > > This prevents a warning in skl_compute_linetime_wm() where pixel_rate > becomes 0 when crtc_clock is 0. > Running this on skylake it looks like like it's a partial fix. You would need to move hwmode like below too? Also: References: https://bugs.freedesktop.org/show_bug.cgi?id=91428 diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index add7f807a82d..49907f4e939a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14888,8 +14888,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED; } - crtc->base.hwmode = crtc->config->base.adjusted_mode; - DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n", crtc->base.base.id, crtc->active ? "enabled" : "disabled"); @@ -14971,6 +14969,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev) for_each_pipe(dev_priv, pipe) { crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); + crtc->base.hwmode = crtc->config->base.adjusted_mode; for_each_intel_plane(crtc->base.dev, plane) { if (crtc->pipe != plane->pipe) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Postpone plane readout until after encoder readout
Op 31-07-15 om 15:04 schreef Patrik Jakobsson: > When reading out hw state for planes we disable inactive planes which in > turn triggers an update of the watermarks. The update depends on the > crtc_clock being set which is done when reading out encoders. Thus > postpone the plane readout until after encoder readout. > > This prevents a warning in skl_compute_linetime_wm() where pixel_rate > becomes 0 when crtc_clock is 0. > The plane loop doesn't have to be nested. But that's just a minor nitpick, feel free to ignore.. Reviewed-By: Maarten Lankhorst ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Postpone plane readout until after encoder readout
When reading out hw state for planes we disable inactive planes which in turn triggers an update of the watermarks. The update depends on the crtc_clock being set which is done when reading out encoders. Thus postpone the plane readout until after encoder readout. This prevents a warning in skl_compute_linetime_wm() where pixel_rate becomes 0 when crtc_clock is 0. Signed-off-by: Patrik Jakobsson --- drivers/gpu/drm/i915/intel_display.c | 38 +--- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 13a6608..73b2d4a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15205,27 +15205,25 @@ static bool primary_get_hw_state(struct intel_crtc *crtc) return !!(I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE); } -static void readout_plane_state(struct intel_crtc *crtc, - struct intel_crtc_state *crtc_state) +static void intel_sanitize_plane(struct intel_plane *plane) { - struct intel_plane *p; struct intel_plane_state *plane_state; - bool active = crtc_state->base.active; + struct intel_crtc *crtc; - for_each_intel_plane(crtc->base.dev, p) { - if (crtc->pipe != p->pipe) - continue; + plane_state = to_intel_plane_state(plane->base.state); - plane_state = to_intel_plane_state(p->base.state); + if (!plane_state->base.crtc) + return; - if (p->base.type == DRM_PLANE_TYPE_PRIMARY) - plane_state->visible = primary_get_hw_state(crtc); - else { - if (active) - p->disable_plane(&p->base, &crtc->base); + crtc = to_intel_crtc(plane_state->base.crtc); - plane_state->visible = false; - } + if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) { + plane_state->visible = primary_get_hw_state(crtc); + } else { + if (crtc->base.state->active) + plane->disable_plane(&plane->base, &crtc->base); + + plane_state->visible = false; } } @@ -15276,7 +15274,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) } crtc->base.hwmode = crtc->config->base.adjusted_mode; - readout_plane_state(crtc, to_intel_crtc_state(crtc->base.state)); DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n", crtc->base.base.id, @@ -15349,6 +15346,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev) enum pipe pipe; struct intel_crtc *crtc; struct intel_encoder *encoder; + struct intel_plane *plane; int i; intel_modeset_readout_hw_state(dev); @@ -15360,6 +15358,14 @@ intel_modeset_setup_hw_state(struct drm_device *dev) for_each_pipe(dev_priv, pipe) { crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); + + for_each_intel_plane(crtc->base.dev, plane) { + if (crtc->pipe != plane->pipe) + continue; + + intel_sanitize_plane(plane); + } + intel_sanitize_crtc(crtc); intel_dump_pipe_config(crtc, crtc->config, "[setup_hw_state]"); -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx