Op 05-10-16 om 22:33 schreef Paulo Zanoni:
> Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
>> Having skl_wm_level contain all of the watermarks for each plane is
>> annoying since it prevents us from having any sort of object to
>> represent a single watermark level, something we take advantage of in
>> the next commit to cut down on all of the copy paste code in here.
> I'd like to start my review pointing that I really like this patch. I
> agree the current form is annoying.
>
> See below for some details.
>
>
>> Signed-off-by: Lyude <cp...@redhat.com>
>> Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
>> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
>> Cc: Matt Roper <matthew.d.ro...@intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 6 +-
>> drivers/gpu/drm/i915/intel_drv.h | 6 +-
>> drivers/gpu/drm/i915/intel_pm.c | 208 +++++++++++++++++----------
>> ------------
>> 3 files changed, 100 insertions(+), 120 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index d26e5999..0f97d43 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1648,9 +1648,9 @@ struct skl_wm_values {
>> };
>>
>> struct skl_wm_level {
>> - bool plane_en[I915_MAX_PLANES];
>> - uint16_t plane_res_b[I915_MAX_PLANES];
>> - uint8_t plane_res_l[I915_MAX_PLANES];
>> + bool plane_en;
>> + uint16_t plane_res_b;
>> + uint8_t plane_res_l;
>> };
>>
>> /*
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 35ba282..d684f4f 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -468,9 +468,13 @@ struct intel_pipe_wm {
>> bool sprites_scaled;
>> };
>>
>> -struct skl_pipe_wm {
>> +struct skl_plane_wm {
>> struct skl_wm_level wm[8];
>> struct skl_wm_level trans_wm;
>> +};
>> +
>> +struct skl_pipe_wm {
>> + struct skl_plane_wm planes[I915_MAX_PLANES];
>> uint32_t linetime;
>> };
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index af96888..250f12d 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3668,67 +3668,52 @@ static int
>> skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>> struct skl_ddb_allocation *ddb,
>> struct intel_crtc_state *cstate,
>> + struct intel_plane *intel_plane,
>> int level,
>> struct skl_wm_level *result)
>> {
>> struct drm_atomic_state *state = cstate->base.state;
>> struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
>>> base.crtc);
>> - struct drm_plane *plane;
>> - struct intel_plane *intel_plane;
>> - struct intel_plane_state *intel_pstate;
>> + struct drm_plane *plane = &intel_plane->base;
>> + struct intel_plane_state *intel_pstate = NULL;
>> uint16_t ddb_blocks;
>> enum pipe pipe = intel_crtc->pipe;
>> int ret;
>> + int i = skl_wm_plane_id(intel_plane);
>> +
>> + if (state)
>> + intel_pstate =
>> + intel_atomic_get_existing_plane_state(state,
>> + intel_
>> plane);
>>
>> /*
>> - * We'll only calculate watermarks for planes that are
>> actually
>> - * enabled, so make sure all other planes are set as
>> disabled.
>> + * Note: If we start supporting multiple pending atomic
>> commits against
>> + * the same planes/CRTC's in the future, plane->state will
>> no longer be
>> + * the correct pre-state to use for the calculations here
>> and we'll
>> + * need to change where we get the 'unchanged' plane data
>> from.
>> + *
>> + * For now this is fine because we only allow one queued
>> commit against
>> + * a CRTC. Even if the plane isn't modified by this
>> transaction and we
>> + * don't have a plane lock, we still have the CRTC's lock,
>> so we know
>> + * that no other transactions are racing with us to update
>> it.
>> */
>> - memset(result, 0, sizeof(*result));
>> -
>> - for_each_intel_plane_mask(&dev_priv->drm,
>> - intel_plane,
>> - cstate->base.plane_mask) {
>> - int i = skl_wm_plane_id(intel_plane);
>> -
>> - plane = &intel_plane->base;
>> - intel_pstate = NULL;
>> - if (state)
>> - intel_pstate =
>> - intel_atomic_get_existing_plane_stat
>> e(state,
>> -
>> intel_plane);
>> + if (!intel_pstate)
>> + intel_pstate = to_intel_plane_state(plane->state);
>>
>> - /*
>> - * Note: If we start supporting multiple pending
>> atomic commits
>> - * against the same planes/CRTC's in the future,
>> plane->state
>> - * will no longer be the correct pre-state to use
>> for the
>> - * calculations here and we'll need to change where
>> we get the
>> - * 'unchanged' plane data from.
>> - *
>> - * For now this is fine because we only allow one
>> queued commit
>> - * against a CRTC. Even if the plane isn't modified
>> by this
>> - * transaction and we don't have a plane lock, we
>> still have
>> - * the CRTC's lock, so we know that no other
>> transactions are
>> - * racing with us to update it.
>> - */
>> - if (!intel_pstate)
>> - intel_pstate = to_intel_plane_state(plane-
>>> state);
>> + WARN_ON(!intel_pstate->base.fb);
>>
>> - WARN_ON(!intel_pstate->base.fb);
>> + ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
>>
>> - ddb_blocks = skl_ddb_entry_size(&ddb-
>>> plane[pipe][i]);
>> -
>> - ret = skl_compute_plane_wm(dev_priv,
>> - cstate,
>> - intel_pstate,
>> - ddb_blocks,
>> - level,
>> - &result->plane_res_b[i],
>> - &result->plane_res_l[i],
>> - &result->plane_en[i]);
>> - if (ret)
>> - return ret;
>> - }
>> + ret = skl_compute_plane_wm(dev_priv,
>> + cstate,
>> + intel_pstate,
>> + ddb_blocks,
>> + level,
>> + &result->plane_res_b,
>> + &result->plane_res_l,
>> + &result->plane_en);
>> + if (ret)
>> + return ret;
>>
>> return 0;
>> }
>> @@ -3749,19 +3734,11 @@ skl_compute_linetime_wm(struct
>> intel_crtc_state *cstate)
>> static void skl_compute_transition_wm(struct intel_crtc_state
>> *cstate,
>> struct skl_wm_level *trans_wm
>> /* out */)
>> {
>> - struct drm_crtc *crtc = cstate->base.crtc;
>> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> - struct intel_plane *intel_plane;
>> -
>> if (!cstate->base.active)
>> return;
>>
>> /* Until we know more, just disable transition WMs */
>> - for_each_intel_plane_on_crtc(crtc->dev, intel_crtc,
>> intel_plane) {
>> - int i = skl_wm_plane_id(intel_plane);
>> -
>> - trans_wm->plane_en[i] = false;
>> - }
>> + trans_wm->plane_en = false;
>> }
>>
>> static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
>> @@ -3770,19 +3747,33 @@ static int skl_build_pipe_wm(struct
>> intel_crtc_state *cstate,
>> {
>> struct drm_device *dev = cstate->base.crtc->dev;
>> const struct drm_i915_private *dev_priv = to_i915(dev);
>> + struct intel_plane *intel_plane;
>> + struct skl_plane_wm *wm;
>> int level, max_level = ilk_wm_max_level(dev);
>> int ret;
>>
>> - for (level = 0; level <= max_level; level++) {
>> - ret = skl_compute_wm_level(dev_priv, ddb, cstate,
>> - level, &pipe_wm-
>>> wm[level]);
>> - if (ret)
>> - return ret;
>> + /*
>> + * We'll only calculate watermarks for planes that are
>> actually
>> + * enabled, so make sure all other planes are set as
>> disabled.
>> + */
>> + memset(pipe_wm->planes, 0, sizeof(pipe_wm->planes));
>> +
>> + for_each_intel_plane_mask(&dev_priv->drm,
>> + intel_plane,
>> + cstate->base.plane_mask) {
>> + wm = &pipe_wm->planes[skl_wm_plane_id(intel_plane)];
>> +
>> + for (level = 0; level <= max_level; level++) {
>> + ret = skl_compute_wm_level(dev_priv, ddb,
>> cstate,
>> + intel_plane,
>> level,
>> + &wm->wm[level]);
>> + if (ret)
>> + return ret;
>> + }
>> + skl_compute_transition_wm(cstate, &wm->trans_wm);
>> }
>> pipe_wm->linetime = skl_compute_linetime_wm(cstate);
>>
>> - skl_compute_transition_wm(cstate, &pipe_wm->trans_wm);
>> -
>> return 0;
>> }
>>
>> @@ -3792,50 +3783,56 @@ static void skl_compute_wm_results(struct
>> drm_device *dev,
>> struct intel_crtc *intel_crtc)
>> {
>> int level, max_level = ilk_wm_max_level(dev);
>> + struct skl_plane_wm *plane_wm;
>> enum pipe pipe = intel_crtc->pipe;
>> uint32_t temp;
>> int i;
>>
>> - for (level = 0; level <= max_level; level++) {
>> - for (i = 0; i < intel_num_planes(intel_crtc); i++) {
>> + for (i = 0; i < intel_num_planes(intel_crtc); i++) {
>> + plane_wm = &p_wm->planes[i];
>> +
>> + for (level = 0; level <= max_level; level++) {
>> temp = 0;
>>
>> - temp |= p_wm->wm[level].plane_res_l[i] <<
>> + temp |= plane_wm->wm[level].plane_res_l <<
>> PLANE_WM_LINES_SHIFT;
>> - temp |= p_wm->wm[level].plane_res_b[i];
>> - if (p_wm->wm[level].plane_en[i])
>> + temp |= plane_wm->wm[level].plane_res_b;
>> + if (plane_wm->wm[level].plane_en)
>> temp |= PLANE_WM_EN;
>>
>> r->plane[pipe][i][level] = temp;
>> }
>>
>> - temp = 0;
>> -
>> - temp |= p_wm->wm[level].plane_res_l[PLANE_CURSOR] <<
>> PLANE_WM_LINES_SHIFT;
>> - temp |= p_wm->wm[level].plane_res_b[PLANE_CURSOR];
>> + }
>>
>> - if (p_wm->wm[level].plane_en[PLANE_CURSOR])
>> + for (level = 0; level <= max_level; level++) {
>> + plane_wm = &p_wm->planes[PLANE_CURSOR];
>> + temp = 0;
>> + temp |= plane_wm->wm[level].plane_res_l <<
>> PLANE_WM_LINES_SHIFT;
>> + temp |= plane_wm->wm[level].plane_res_b;
>> + if (plane_wm->wm[level].plane_en)
>> temp |= PLANE_WM_EN;
>>
>> r->plane[pipe][PLANE_CURSOR][level] = temp;
>> -
>> }
>>
>> /* transition WMs */
>> for (i = 0; i < intel_num_planes(intel_crtc); i++) {
>> + plane_wm = &p_wm->planes[i];
>> temp = 0;
>> - temp |= p_wm->trans_wm.plane_res_l[i] <<
>> PLANE_WM_LINES_SHIFT;
>> - temp |= p_wm->trans_wm.plane_res_b[i];
>> - if (p_wm->trans_wm.plane_en[i])
>> + temp |= plane_wm->trans_wm.plane_res_l <<
>> PLANE_WM_LINES_SHIFT;
>> + temp |= plane_wm->trans_wm.plane_res_b;
>> + if (plane_wm->trans_wm.plane_en)
>> temp |= PLANE_WM_EN;
>>
>> r->plane_trans[pipe][i] = temp;
>> }
>>
>> + plane_wm = &p_wm->planes[PLANE_CURSOR];
>> temp = 0;
>> - temp |= p_wm->trans_wm.plane_res_l[PLANE_CURSOR] <<
>> PLANE_WM_LINES_SHIFT;
>> - temp |= p_wm->trans_wm.plane_res_b[PLANE_CURSOR];
>> - if (p_wm->trans_wm.plane_en[PLANE_CURSOR])
>> + temp |= plane_wm->trans_wm.plane_res_l <<
>> PLANE_WM_LINES_SHIFT;
>> + temp |= plane_wm->trans_wm.plane_res_b;
>> + if (plane_wm->trans_wm.plane_en)
>> temp |= PLANE_WM_EN;
>>
>> r->plane_trans[pipe][PLANE_CURSOR] = temp;
>> @@ -4262,44 +4259,24 @@ static void ilk_optimize_watermarks(struct
>> intel_crtc_state *cstate)
>> static void skl_pipe_wm_active_state(uint32_t val,
>> struct skl_pipe_wm *active,
>> bool is_transwm,
>> - bool is_cursor,
>> int i,
>> int level)
>> {
>> + struct skl_plane_wm *plane_wm = &active->planes[i];
>> bool is_enabled = (val & PLANE_WM_EN) != 0;
>>
>> if (!is_transwm) {
>> - if (!is_cursor) {
>> - active->wm[level].plane_en[i] = is_enabled;
>> - active->wm[level].plane_res_b[i] =
>> - val & PLANE_WM_BLOCKS_MASK;
>> - active->wm[level].plane_res_l[i] =
>> - (val >>
>> PLANE_WM_LINES_SHIFT) &
>> - PLANE_WM_LINES_MASK;
>> - } else {
>> - active->wm[level].plane_en[PLANE_CURSOR] =
>> is_enabled;
>> - active->wm[level].plane_res_b[PLANE_CURSOR]
>> =
>> - val & PLANE_WM_BLOCKS_MASK;
>> - active->wm[level].plane_res_l[PLANE_CURSOR]
>> =
>> - (val >>
>> PLANE_WM_LINES_SHIFT) &
>> - PLANE_WM_LINES_MASK;
>> - }
>> + plane_wm->wm[level].plane_en = is_enabled;
>> + plane_wm->wm[level].plane_res_b = val &
>> PLANE_WM_BLOCKS_MASK;
>> + plane_wm->wm[level].plane_res_l =
>> + (val >> PLANE_WM_LINES_SHIFT) &
>> + PLANE_WM_LINES_MASK;
> Nitpick: you can join the two lines above and still stay under 80
> columns.
>
>
>> } else {
>> - if (!is_cursor) {
>> - active->trans_wm.plane_en[i] = is_enabled;
>> - active->trans_wm.plane_res_b[i] =
>> - val & PLANE_WM_BLOCKS_MASK;
>> - active->trans_wm.plane_res_l[i] =
>> - (val >>
>> PLANE_WM_LINES_SHIFT) &
>> - PLANE_WM_LINES_MASK;
>> - } else {
>> - active->trans_wm.plane_en[PLANE_CURSOR] =
>> is_enabled;
>> - active->trans_wm.plane_res_b[PLANE_CURSOR] =
>> - val & PLANE_WM_BLOCKS_MASK;
>> - active->trans_wm.plane_res_l[PLANE_CURSOR] =
>> - (val >>
>> PLANE_WM_LINES_SHIFT) &
>> - PLANE_WM_LINES_MASK;
>> - }
>> + plane_wm->trans_wm.plane_en = is_enabled;
>> + plane_wm->trans_wm.plane_res_b = val &
>> PLANE_WM_BLOCKS_MASK;
>> + plane_wm->trans_wm.plane_res_l =
>> + (val >> PLANE_WM_LINES_SHIFT) &
>> + PLANE_WM_LINES_MASK;
> Same here.
>
>
>> }
>> }
>>
>> @@ -4338,20 +4315,19 @@ static void skl_pipe_wm_get_hw_state(struct
>> drm_crtc *crtc)
>> for (level = 0; level <= max_level; level++) {
>> for (i = 0; i < intel_num_planes(intel_crtc); i++) {
>> temp = hw->plane[pipe][i][level];
>> - skl_pipe_wm_active_state(temp, active,
>> false,
>> - false, i, level);
>> + skl_pipe_wm_active_state(temp, active,
>> false, i, level);
>> }
>> temp = hw->plane[pipe][PLANE_CURSOR][level];
>> - skl_pipe_wm_active_state(temp, active, false, true,
>> i, level);
>> + skl_pipe_wm_active_state(temp, active, false, i,
>> level);
> While this is not wrong today, history shows that the number of planes
> increases over time, so we may at some point in the future add PLANE_D
> and more, so the code will become wrong. Just pass PLANE_CURSOR instead
> of "i" here and below. Also, this simplification could have been a
> separate patch.
Agreed, but I want to note that PLANE_CURSOR is always supposed to be the last
member.
Unless you have sprite planes covering the cursor, which doesn't ever happen.
~Maarten
