[Intel-gfx] [PATCH 1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Wed Oct 25 06:03:47 UTC 2017
Op 25-10-17 om 01:01 schreef Matt Roper:
> On Thu, Oct 19, 2017 at 05:13:40PM +0200, Maarten Lankhorst wrote:
>> The original intent was to preserve watermarks as much as possible
>> in intel_pipe_wm.raw_wm, and put the validated ones in intel_pipe_wm.wm.
>>
>> It seems this approach is insufficient and we don't always preserve
>> the raw watermarks, so just use the atomic iterator we're already using
>> to get a const pointer to all bound planes on the crtc.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102373
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Cc: stable at vger.kernel.org #v4.8+
> It's been a while since I looked at this code, so I'm not sure I'm
> following all the context/history here. Before this patch, we had
> calculated watermarks for all levels (even those above the maximum
> usable watermark level as determined by sprite usage) into raw_wm. But
> as far as I can tell, we never actually used those values for anything
> so that was no different than just throwing the values away?
No, the trick introduced there was that we preserved the raw watermarks for future
calculations to not include all planes in a commit. This is now solved by using the
read-only drm_atomic_crtc_state_for_each_plane_state iterator, to get all
const plane states without having to include them.
> Is my understanding correct that this is mostly a revert of 71f0a62614
> ("drm/i915: Only use sanitized values for ILK watermarks) except that it
> also modifies the level validation loop so that we're only calling
> ilk_compute_wm_level() on the levels up to usable_level whereas before
> that patch we were calling it on all levels, but then
> setting wm->enable = false for the unusable levels?
>
> By my understanding, this looks like a safe simplification of the
> current logic, but I don't see where the functional change is here.
> Was the Buzilla: reference supposed to be tied to patch #2 of this
> series or am I missing something important?
We now throw away all watermarks, and use drm_atomic_crtc_state_for_each_plane_state
to get the plane states. As a result we can throw away the raw watermarks, which have
proven to be insufficiently preserved. :)
>> ---
>> drivers/gpu/drm/i915/intel_drv.h | 1 -
>> drivers/gpu/drm/i915/intel_pm.c | 51 +++++++++++++++++-----------------------
>> 2 files changed, 21 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 47d022d48718..7bc60c848940 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -499,7 +499,6 @@ struct intel_crtc_scaler_state {
>>
>> struct intel_pipe_wm {
>> struct intel_wm_level wm[5];
>> - struct intel_wm_level raw_wm[5];
>> uint32_t linetime;
>> bool fbc_wm_enabled;
>> bool pipe_enabled;
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index c42a65a93b3a..ea70c720f492 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -2721,9 +2721,9 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
>> const struct intel_crtc *intel_crtc,
>> int level,
>> struct intel_crtc_state *cstate,
>> - struct intel_plane_state *pristate,
>> - struct intel_plane_state *sprstate,
>> - struct intel_plane_state *curstate,
>> + const struct intel_plane_state *pristate,
>> + const struct intel_plane_state *sprstate,
>> + const struct intel_plane_state *curstate,
>> struct intel_wm_level *result)
>> {
>> uint16_t pri_latency = dev_priv->wm.pri_latency[level];
>> @@ -3043,28 +3043,24 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>> struct intel_pipe_wm *pipe_wm;
>> struct drm_device *dev = state->dev;
>> const struct drm_i915_private *dev_priv = to_i915(dev);
>> - struct intel_plane *intel_plane;
>> - struct intel_plane_state *pristate = NULL;
>> - struct intel_plane_state *sprstate = NULL;
>> - struct intel_plane_state *curstate = NULL;
>> + struct drm_plane *plane;
>> + const struct drm_plane_state *plane_state;
>> + const struct intel_plane_state *pristate = NULL;
>> + const struct intel_plane_state *sprstate = NULL;
>> + const struct intel_plane_state *curstate = NULL;
>> int level, max_level = ilk_wm_max_level(dev_priv), usable_level;
>> struct ilk_wm_maximums max;
>>
>> pipe_wm = &cstate->wm.ilk.optimal;
>>
>> - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>> - struct intel_plane_state *ps;
>> -
>> - ps = intel_atomic_get_existing_plane_state(state,
>> - intel_plane);
>> - if (!ps)
>> - continue;
>> + drm_atomic_crtc_state_for_each_plane_state(plane, plane_state, &cstate->base) {
>> + const struct intel_plane_state *ps = to_intel_plane_state(plane_state);
>>
>> - if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
>> + if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>> pristate = ps;
>> - else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
>> + else if (plane->type == DRM_PLANE_TYPE_OVERLAY)
>> sprstate = ps;
>> - else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
>> + else if (plane->type == DRM_PLANE_TYPE_CURSOR)
>> curstate = ps;
>> }
>>
>> @@ -3086,11 +3082,9 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>> if (pipe_wm->sprites_scaled)
>> usable_level = 0;
>>
>> - ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
>> - pristate, sprstate, curstate, &pipe_wm->raw_wm[0]);
>> -
>> memset(&pipe_wm->wm, 0, sizeof(pipe_wm->wm));
>> - pipe_wm->wm[0] = pipe_wm->raw_wm[0];
>> + ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
>> + pristate, sprstate, curstate, &pipe_wm->wm[0]);
>>
>> if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>> pipe_wm->linetime = hsw_compute_linetime_wm(cstate);
>> @@ -3100,8 +3094,8 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>>
>> ilk_compute_wm_reg_maximums(dev_priv, 1, &max);
>>
>> - for (level = 1; level <= max_level; level++) {
>> - struct intel_wm_level *wm = &pipe_wm->raw_wm[level];
>> + for (level = 1; level <= usable_level; level++) {
>> + struct intel_wm_level *wm = &pipe_wm->wm[level];
>>
>> ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
>> pristate, sprstate, curstate, wm);
>> @@ -3111,13 +3105,10 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>> * register maximums since such watermarks are
>> * always invalid.
>> */
>> - if (level > usable_level)
>> - continue;
>> -
>> - if (ilk_validate_wm_level(level, &max, wm))
>> - pipe_wm->wm[level] = *wm;
>> - else
>> - usable_level = level;
>> + if (!ilk_validate_wm_level(level, &max, wm)) {
>> + memset(wm, 0, sizeof(*wm));
>> + break;
>> + }
>> }
>>
>> return 0;
>> --
>> 2.14.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list