On Sun, Mar 08, 2015 at 02:00:44PM -0700, Matt Roper wrote:
> Existing watermark code calls intel_crtc_active() to determine whether a CRTC
> is active for the purpose of watermark calculations (and bails out early if it
> determines the CRTC is not active). However intel_crtc_active() only returns
> true if crtc->primary->fb is non-NULL, which isn't appropriate in the modern
> age of universal planes and atomic modeset since userspace can now disable the
> primary plane, but leave the CRTC (and other planes) running.
>
> Note that commit
>
> commit 0fda65680e92545caea5be7805a7f0a617fb6c20
> Author: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Date: Fri Feb 27 15:12:35 2015 +0000
>
> drm/i915/skl: Update watermarks for Y tiling
>
> adds a test for primary plane enable/disable to trigger a watermark update
> (previously we ignored updates to primary planes, which wasn't really correct,
> but we got lucky since we always pretended the primary plane was on). Tvrtko's
> patch tries to update watermarks when we re-enable the primary plane, but that
> watermark computation gets aborted early because intel_crtc_active() returns
> false due to the disabled primary plane.
>
> Switch the ILK and SKL watermark code over to use crtc->state->active rather
> than calling intel_crtc_active() so that we'll properly compute watermarks when
> re-enabling the primary plane.
>
> Note that this commit doesn't touch callsites in the watermark code for
> older platforms since there were concerns that doing so would lead to
> other types of breakage.
>
> Also note that all of the watermark calculation at the moment takes place after
> new crtc/plane states are swapped into the DRM objects. This will change in
> the future, so we'll be working with in-flight state objects, but for the time
> being, crtc->state is what we want to operate on.
>
> v2: Don't drop primary->fb check from intel_crtc_active(), but rather replace
> ILK/SKL callsites with direct tests of crtc->state->active. There is
> concern that messing with intel_crtc_active() will lead to other breakage for
> old hardware platforms. (Ville)
>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
If you make this use intel_crtc->active you can slap on my
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
The intel_crtc->active to crtc->state->active change is more
controversial so I think we need to look at the code in more detail
before we go make such a big change.
> ---
> drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d9b115e..dafd7de 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -822,7 +822,7 @@ static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
> * FIXME the plane might have an fb
> * but be invisible (eg. due to clipping)
> */
> - if (!intel_crtc->base.state->active || !plane->state->fb)
> + if (!crtc->state->active || !plane->state->fb)
> return 0;
>
> if (WARN(clock == 0, "Pixel clock is zero!\n"))
> @@ -1701,7 +1701,7 @@ hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc)
> struct drm_display_mode *mode = &intel_crtc->config->base.adjusted_mode;
> u32 linetime, ips_linetime;
>
> - if (!intel_crtc_active(crtc))
> + if (!crtc->state->active)
> return 0;
>
> /* The WM are computed with base on how long it takes to fill a single
> @@ -1956,7 +1956,7 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
> enum pipe pipe = intel_crtc->pipe;
> struct drm_plane *plane;
>
> - if (!intel_crtc_active(crtc))
> + if (!crtc->state->active)
> return;
>
> p->active = true;
> @@ -2468,7 +2468,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>
> nth_active_pipe = 0;
> for_each_crtc(dev, crtc) {
> - if (!intel_crtc_active(crtc))
> + if (!crtc->state->active)
> continue;
>
> if (crtc == for_crtc)
> @@ -2708,7 +2708,7 @@ static void skl_compute_wm_global_parameters(struct drm_device *dev,
> struct drm_plane *plane;
>
> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> - config->num_pipes_active += intel_crtc_active(crtc);
> + config->num_pipes_active += crtc->state->active;
>
> /* FIXME: I don't think we need those two global parameters on SKL */
> list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> @@ -2729,7 +2729,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
> struct drm_framebuffer *fb;
> int i = 1; /* Index for sprite planes start */
>
> - p->active = intel_crtc_active(crtc);
> + p->active = crtc->state->active;
> if (p->active) {
> p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
> p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
> @@ -2860,7 +2860,7 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
> static uint32_t
> skl_compute_linetime_wm(struct drm_crtc *crtc, struct skl_pipe_wm_parameters *p)
> {
> - if (!intel_crtc_active(crtc))
> + if (!crtc->state->active)
> return 0;
>
> return DIV_ROUND_UP(8 * p->pipe_htotal * 1000, p->pixel_rate);
> @@ -3191,7 +3191,7 @@ static void skl_update_other_pipe_wm(struct drm_device *dev,
> if (this_crtc->pipe == intel_crtc->pipe)
> continue;
>
> - if (!intel_crtc->base.state->active)
> + if (!crtc->state->active)
> continue;
>
> wm_changed = skl_update_pipe_wm(&intel_crtc->base,
> @@ -3407,7 +3407,7 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> hw->plane_trans[pipe][i] = I915_READ(PLANE_WM_TRANS(pipe, i));
> hw->cursor_trans[pipe] = I915_READ(CUR_WM_TRANS(pipe));
>
> - if (!intel_crtc_active(crtc))
> + if (!crtc->state->active)
> return;
>
> hw->dirty[pipe] = true;
> @@ -3462,7 +3462,7 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
>
> - active->pipe_enabled = intel_crtc_active(crtc);
> + active->pipe_enabled = crtc->state->active;
>
> if (active->pipe_enabled) {
> u32 tmp = hw->wm_pipe[pipe];
> --
> 1.8.5.1
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
