[Intel-gfx] [PATCH v4 7/8] drm/i915: Do not compute watermarks on a noop.
Zanoni, Paulo R
paulo.r.zanoni at intel.com
Thu Feb 18 20:51:20 UTC 2016
Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> No atomic state should be included after all validation when nothing
> has
> changed. During modeset all active planes will be added to the state,
> while disabled planes won't change their state.
As someone who is also not super familiar with the new watermarks code,
I really really wish I had a more detailed commit message to allow me
to understand your train of thought. I'll ask some questions below to
validate my understanding.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Matt Roper <matthew.d.roper at intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 3 +-
> drivers/gpu/drm/i915/intel_drv.h | 13 ++++++++
> drivers/gpu/drm/i915/intel_pm.c | 61 +++++++++++++++++++++-----
> ----------
> 3 files changed, 51 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 00cb261c6787..6bb1f5dbc7a0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11910,7 +11910,8 @@ static int intel_crtc_atomic_check(struct
> drm_crtc *crtc,
> }
>
> ret = 0;
> - if (dev_priv->display.compute_pipe_wm) {
> + if (dev_priv->display.compute_pipe_wm &&
> + (mode_changed || pipe_config->update_pipe || crtc_state-
> >planes_changed)) {
> ret = dev_priv->display.compute_pipe_wm(intel_crtc,
> state);
> if (ret)
> return ret;
Can't this chunk be on its own separate commit? I'm not sure why the
rest of the commit is related to this change.
It seems the rest of the commit is aimed reducing the number of planes
we have to lock, not about not computing WMs if nothing in the pipe has
changed.
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 8effb9ece21e..144597ac74e3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1583,6 +1583,19 @@ intel_atomic_get_crtc_state(struct
> drm_atomic_state *state,
>
> return to_intel_crtc_state(crtc_state);
> }
> +
> +static inline struct intel_plane_state *
> +intel_atomic_get_existing_plane_state(struct drm_atomic_state
> *state,
> + struct intel_plane *plane)
> +{
> + struct drm_plane_state *plane_state;
> +
> + plane_state = drm_atomic_get_existing_plane_state(state,
> &plane->base);
> +
> + return to_intel_plane_state(plane_state);
> +}
> +
> +
Two newlines above.
It seems you'll be able to simplify a lot of stuff with this new
function. I'm looking forward to review that patch :)
> int intel_atomic_setup_scalers(struct drm_device *dev,
> struct intel_crtc *intel_crtc,
> struct intel_crtc_state *crtc_state);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 379eabe093cb..8fb8c6891ae6 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2010,11 +2010,18 @@ static void ilk_compute_wm_level(const struct
> drm_i915_private *dev_priv,
> cur_latency *= 5;
> }
>
> - result->pri_val = ilk_compute_pri_wm(cstate, pristate,
> - pri_latency, level);
> - result->spr_val = ilk_compute_spr_wm(cstate, sprstate,
> spr_latency);
> - result->cur_val = ilk_compute_cur_wm(cstate, curstate,
> cur_latency);
> - result->fbc_val = ilk_compute_fbc_wm(cstate, pristate,
> result->pri_val);
> + if (pristate) {
> + result->pri_val = ilk_compute_pri_wm(cstate,
> pristate,
> + pri_latency,
> level);
> + result->fbc_val = ilk_compute_fbc_wm(cstate,
> pristate, result->pri_val);
> + }
> +
> + if (sprstate)
> + result->spr_val = ilk_compute_spr_wm(cstate,
> sprstate, spr_latency);
> +
> + if (curstate)
> + result->cur_val = ilk_compute_cur_wm(cstate,
> curstate, cur_latency);
> +
> result->enable = true;
> }
>
> @@ -2287,7 +2294,6 @@ static int ilk_compute_pipe_wm(struct
> intel_crtc *intel_crtc,
> const struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc_state *cstate = NULL;
> struct intel_plane *intel_plane;
> - struct drm_plane_state *ps;
> struct intel_plane_state *pristate = NULL;
> struct intel_plane_state *sprstate = NULL;
> struct intel_plane_state *curstate = NULL;
> @@ -2306,30 +2312,37 @@ static int ilk_compute_pipe_wm(struct
> intel_crtc *intel_crtc,
> memset(pipe_wm, 0, sizeof(*pipe_wm));
>
> for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> - ps = drm_atomic_get_plane_state(state,
> - &intel_plane->base);
> - if (IS_ERR(ps))
> - return PTR_ERR(ps);
> + struct intel_plane_state *ps;
> +
> + ps = intel_atomic_get_existing_plane_state(state,
> + intel_pla
> ne);
> + if (!ps)
> + continue;
Ok, let me see if I understood: if some of these planes is not part of
the atomic commit, you're not going to include it in the calculations
since its value is not going to change. This would allow us lock less
planes, which is the main advantage. Is this correct?
If yes, how much do we really gain by saving some plane locking?
So by not assigning values to result->x_val at ilk_compute_wm_level()
when NULL is passed you're going to preserve whatever correct values
were already set earlier, so you don't need to recompute them. Is this
correct?
If the answer to the above is "yes", then don't we need to remove the
memset(pipe_wm, 0) at the beginning of ilk_compute_wm_level? Otherwise,
nothing will be preserved.
There's also the zero-initialization of "config" to consider.
Or maybe instead of all of the above, we're working under the
assumption that if some of the planes is not part of the atomic commit,
then all its relevant WM values will be zero?
>
> if (intel_plane->base.type ==
> DRM_PLANE_TYPE_PRIMARY)
> - pristate = to_intel_plane_state(ps);
> - else if (intel_plane->base.type ==
> DRM_PLANE_TYPE_OVERLAY)
> - sprstate = to_intel_plane_state(ps);
> - else if (intel_plane->base.type ==
> DRM_PLANE_TYPE_CURSOR)
> - curstate = to_intel_plane_state(ps);
> + pristate = ps;
> + else if (intel_plane->base.type ==
> DRM_PLANE_TYPE_OVERLAY) {
> + sprstate = ps;
> +
> + if (ps) {
> + pipe_wm->sprites_enabled = sprstate-
> >visible;
> + pipe_wm->sprites_scaled = sprstate-
> >visible &&
You're setting these values here...
> + (drm_rect_width(&sprstate-
> >dst) != drm_rect_width(&sprstate->src) >> 16 ||
> + drm_rect_height(&sprstate-
> >dst) != drm_rect_height(&sprstate->src) >> 16);
> + }
> + } else if (intel_plane->base.type ==
> DRM_PLANE_TYPE_CURSOR)
(missing brackets here, but if you follow my suggestion below you won't
need them)
> + curstate = ps;
> }
>
> - config.sprites_enabled = sprstate->visible;
> - config.sprites_scaled = sprstate->visible &&
> - (drm_rect_width(&sprstate->dst) !=
> drm_rect_width(&sprstate->src) >> 16 ||
> - drm_rect_height(&sprstate->dst) !=
> drm_rect_height(&sprstate->src) >> 16);
> + config.sprites_enabled = pipe_wm->sprites_enabled;
> + config.sprites_scaled = pipe_wm->sprites_scaled;
>
> pipe_wm->pipe_enabled = cstate->base.active;
> pipe_wm->sprites_enabled = config.sprites_enabled;
> pipe_wm->sprites_scaled = config.sprites_scaled;
...but then we re-set them here.
Either you could remove these assignments here, or you could move the
"if (ps)" from the loop to outside it, becoming "if (sprstate)", making
the post-patch code similar to the pre-patch code. Since both pipe_wm
and config are zero-initialized you don't even need to think about the
!sprstate case.
>
> /* ILK/SNB: LP2+ watermarks only w/o sprites */
> - if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
> + if (INTEL_INFO(dev)->gen <= 6 && pipe_wm->sprites_enabled)
> max_level = 1;
>
> /* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
> @@ -2352,20 +2365,18 @@ static int ilk_compute_pipe_wm(struct
> intel_crtc *intel_crtc,
> ilk_compute_wm_reg_maximums(dev, 1, &max);
>
> for (level = 1; level <= max_level; level++) {
> - struct intel_wm_level wm = {};
> + struct intel_wm_level *wm = &pipe_wm->wm[level];
>
> ilk_compute_wm_level(dev_priv, intel_crtc, level,
> cstate,
> - pristate, sprstate, curstate,
> &wm);
> + pristate, sprstate, curstate,
> wm);
>
> /*
> * Disable any watermark level that exceeds the
> * register maximums since such watermarks are
> * always invalid.
> */
> - if (!ilk_validate_wm_level(level, &max, &wm))
> + if (!ilk_validate_wm_level(level, &max, wm))
> break;
> -
> - pipe_wm->wm[level] = wm;
The change in the chunk above is that we're now leaving with non-zero
wm->{pri,spr,cur}_val if some level is invalid. Given the current
complexity of the code, it's not trivial verify whether nothing later
is assuming that invalid levels are all-zero, but it looks like we're
safe. Anyway, could you please move this to a separate patch that comes
before the other changes? It seems this change would be safe alone, and
we're seeing problems with WMs these days, so having nice bisectability
is a huge plus.
Thanks,
Paulo
> }
>
> return 0;
More information about the Intel-gfx
mailing list