Op 18-02-16 om 21:51 schreef Zanoni, Paulo R:
> 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@xxxxxxxxxxxxxxx>
>> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
>> ---
>> 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.
Makes sense, will do so.
>>
>> /* 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.
>
Well caught, I think we need to calculate even invalid values, but set ->enable = false in that case.
That is the only way we can ensure that the wm levels are calculated correctly.
I've sent those as separate patches, so I get a full CI run from them.
[PATCH 1/2] drm/i915: Allow preservation of watermarks.
[PATCH 2/2] drm/i915: Only recalculate wm's for planes part of the state, v2.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
